-
-
Notifications
You must be signed in to change notification settings - Fork 5.8k
Handle invalid issues #18111
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Handle invalid issues #18111
Changes from all commits
202c3aa
3b87a0d
cc7658c
01f2dfc
b177198
d95ccf1
663b97b
b14f115
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -30,6 +30,11 @@ type APIContext struct { | |
Org *APIOrganization | ||
} | ||
|
||
// Currently, we have the following common fields in error response: | ||
// * message: the message for end users (it shouldn't be used for error type detection) | ||
// if we need to indicate some errors, we should introduce some new fields like ErrorCode or ErrorType | ||
// * url: the swagger document URL | ||
|
||
// APIError is error format response | ||
// swagger:response error | ||
type APIError struct { | ||
|
@@ -47,8 +52,8 @@ type APIValidationError struct { | |
// APIInvalidTopicsError is error format response to invalid topics | ||
// swagger:response invalidTopicsError | ||
type APIInvalidTopicsError struct { | ||
Topics []string `json:"invalidTopics"` | ||
Message string `json:"message"` | ||
Message string `json:"message"` | ||
InvalidTopics []string `json:"invalidTopics"` | ||
} | ||
|
||
//APIEmpty is an empty response | ||
|
@@ -122,9 +127,9 @@ func (ctx *APIContext) InternalServerError(err error) { | |
}) | ||
} | ||
|
||
var ( | ||
apiContextKey interface{} = "default_api_context" | ||
) | ||
type apiContextKeyType struct{} | ||
|
||
var apiContextKey = apiContextKeyType{} | ||
|
||
// WithAPIContext set up api context in request | ||
func WithAPIContext(req *http.Request, ctx *APIContext) *http.Request { | ||
|
@@ -351,7 +356,7 @@ func ReferencesGitRepo(allowEmpty bool) func(http.Handler) http.Handler { | |
// NotFound handles 404s for APIContext | ||
// String will replace message, errors will be added to a slice | ||
func (ctx *APIContext) NotFound(objs ...interface{}) { | ||
var message = "Not Found" | ||
var message = ctx.Tr("error.not_found") | ||
var errors []string | ||
for _, obj := range objs { | ||
// Ignore nil | ||
|
@@ -367,9 +372,9 @@ func (ctx *APIContext) NotFound(objs ...interface{}) { | |
} | ||
|
||
ctx.JSON(http.StatusNotFound, map[string]interface{}{ | ||
"message": message, | ||
"documentation_url": setting.API.SwaggerURL, | ||
"errors": errors, | ||
"message": message, | ||
"url": setting.API.SwaggerURL, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could this be seen as a breaking change? As it's changing a JSON field. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yep, it changes the field name (make the field name same as other API response). IMO no client could use this field to do anything useful, since it is just a URL for a API document, so it should be fine. |
||
"errors": errors, | ||
}) | ||
} | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be nice to pass here i18n error that issue/PR was not found
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason that I didn't introduce a new i18n translation is: I think the default "not_found" message is clear enough to most users, and we can keep our system small and simple without introducing new texts (and it reduces maintenance work in future).
I am fine with using either a specialized i18n text (new) or a general i18n text (as now).
Not sure how you think about this: either is fine? or should we introduce specialized i18n texts?