-
-
Notifications
You must be signed in to change notification settings - Fork 434
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
Add validation handler and error encoder #197
Conversation
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.
Requesting some small changes mainly to ensure properties of the API can be held with time.
In future PRs we can touch on:
- producing correct JSON pointers (so no patching is later needed)
- matching errors from either
var Err...
(when possible) or with type switches (otherwise): I'd prefer there'd be no error string matching, so let's export some errors/error types :)
|
||
// Error implements the error interface. | ||
func (e *ValidationError) Error() string { | ||
b := new(bytes.Buffer) |
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.
return b.String() | ||
} | ||
|
||
// StatusCode implements the StatusCoder interface for DefaultErrorEncoder |
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.
Enforce this with var _ StatusCoder = ...
Encoder ErrorEncoder | ||
} | ||
|
||
// Encode implements the ErrorEncoder interface for encoding ValidationErrors |
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.
Also enforce this statement with a var _ ...
compile time check.
h.ErrorEncoder(r.Context(), err, w) | ||
return | ||
} | ||
// TODO: validateResponse |
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.
Indeed
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.
This can come later as another PR.
Could you look at the WIP new router at #210? |
This looks pretty close, anything i can do to help? |
@fenollp thanks for merging. I agree that I really don't like matching on error strings. That was the only option given what we have at the moment though. Sorry I missed your follow-up questions. What did you mean by "producing correct JSON pointers" ? I assume you mean this kinda stuff? https://github.com/getkin/kin-openapi/pull/197/files#diff-ee2d579818e5124734fb3ce2cd7e87acR155 |
Implement #194 and #195
This is what I had to add on top of kin-openapi in order to provide openapi validation errors in my API service. More discussion at #194 (comment)