-
-
Notifications
You must be signed in to change notification settings - Fork 432
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
Don't validate readonly field for write requests or vice-versa #532
base: master
Are you sure you want to change the base?
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.
From https://github.com/OAI/OpenAPI-Specification/blob/main/versions/3.0.0.md#schema-object
cc #246 I'm not sure I understand your issue.
There's already VisitAsRequest
/ VisitAsResponse
options which you clone in this PR. Why not reuse them?
Your tests only mention openapi3filter.ValidateRequest
for which you introduced EndpointType
. Why not use ``openapi3filter.ValidateResponse` as well?
Also if your tests could reuse as much as possible of the same openapi document that'd be a great help to make sense of them please.
@CTrando can you share the exact error output you receive before this change? Just trying to make sure we have all the context here |
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.
I don't know the codebase at all, so just took a prelim look here
router, err := legacyrouter.NewRouter(doc) | ||
require.NoError(t, err) | ||
|
||
// Set no id because id is a required readonly field, but this is a write request |
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.
ID is marked writeOnly
in this test, not readOnly
...
Route: route, | ||
Options: &Options{EndpointType: ReadEndpoint}, | ||
}) | ||
require.Error(t, err) |
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.
wait, why would this fail due to "insufficient length ID" if this is a read endpoint and a read-only field (i.e., it shouldn't be validated)?
@CTrando Kind greetings :) Is there anything you'd like to add to this PR / explain about the issue you're solving? Thanks! |
What
Don't validate readonly field for write requests
Why
In my work, we were using the same struct for many of our openapi endpoints, and one of those struct fields was
readOnly
. We were getting validation errors whenever we tried to make a post request with that struct because we weren't filling in the field.For example, we were using the common struct:
For all our endpoints.
When we tried to create an account, we wouldn't fill in the ID, but we were still getting validation errors. The issue is because in the library there's no way (to my knowledge) to give the validator context on whether the endpoint is a read endpoint or a write endpoint. The behavior should be if the endpoint is a write endpoint, then we shouldn't validate a readonly field.
cc @codyaray who knows more about this than me.