Skip to content
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

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

CTrando
Copy link

@CTrando CTrando commented Apr 3, 2022

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:

Account:
  id: readonly

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.

@CTrando CTrando changed the title Don't validate readonly field for write requests Don't validate readonly field for write requests or vice-versa Apr 3, 2022
Copy link
Collaborator

@fenollp fenollp left a 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

Field Name Type Description
readOnly boolean Relevant only for Schema "properties" definitions. Declares the property as "read only". This means that it MAY be sent as part of a response but SHOULD NOT be sent as part of the request. If the property is marked as readOnly being true and is in the required list, the required will take effect on the response only. A property MUST NOT be marked as both readOnly and writeOnly being true. Default value is false.
writeOnly boolean Relevant only for Schema "properties" definitions. Declares the property as "write only". Therefore, it MAY be sent as part of a request but SHOULD NOT be sent as part of the response. If the property is marked as writeOnly being true and is in the required list, the required will take effect on the request only. A property MUST NOT be marked as both readOnly and writeOnly being true. Default value is false.

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.

@codyaray
Copy link
Contributor

codyaray commented Apr 4, 2022

@CTrando can you share the exact error output you receive before this change? Just trying to make sure we have all the context here

Copy link
Contributor

@codyaray codyaray left a 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
Copy link
Contributor

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)
Copy link
Contributor

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)?

@fenollp
Copy link
Collaborator

fenollp commented Jun 1, 2022

@CTrando Kind greetings :) Is there anything you'd like to add to this PR / explain about the issue you're solving? Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants