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

ReadOnly / WriteOnly clarification toward required field #101

Open
LasneF opened this issue Jan 31, 2024 · 8 comments
Open

ReadOnly / WriteOnly clarification toward required field #101

LasneF opened this issue Jan 31, 2024 · 8 comments

Comments

@LasneF
Copy link
Member

LasneF commented Jan 31, 2024

It would be interesting to have a statement on the precedence of readOnly rules vs required rules

given the specification here
small.json

the object contains Id , name , password , both are required ... in differents scenario

id is set to ReadOnly
password as WriteOnly

use case is legitimate, for instance you want to have a single model for an Object (here Pet) , and the id beeing mandatory in read , but not in write (ie POST)

should this schema be valid or not ?

it so it would be good to add a line when we set OAS is leveraging JSON schema for validation .

discussing with JSON schema team the readOnly / writeOnly flag are just meta data and are not taken into account in the validation, it is up to the upper layer to take those points in consideration

to me we have 2 ways of handling this ,

  • Either we consider as invalid OAS , this would mean that a required field cannot be in a readOnly , writeOnly (in some circunstances cf sample)
  • Either it is a valid use OAS and so this means that the readOnly , writeOnly , over defined (override by reducing ) the required field , more or less , depending the context

as this it is not the Json schema policy to defines this behaviour , would be good to have a precision in the OAS Spec

notice than looking on linter , most of them consider the spec as invalid ... my guess because leveraging raw json schema library without pre processing the schema with the context of usage

@philsturgeon
Copy link
Collaborator

Different applications will do different things with the readOnly/writeOnly metadata.

  • Documentation will often strip readOnly properties out of request bodies and writeOnly out of response bodies.
  • Mocking may do the same.
  • SDK Generators might create getters and setters depending on which flags are enabled.
  • Linters might have a moan if the example for a request body contains a read only property.
  • Validation middleware and contract testing tools might return a validation error if a request body contains a read only property.

This is not in the spec, but they are conventions that good tools use. A huge part of my brain worries when I hear statements like that, because that means inconsistent experiences across different tools, but only for the rare few people making their OAS available in multiple documentation tools, or using multiple linters, generally thats not going to be an issue, and the majority of tooling is moving towards these sensible standards because everyone expects those tools to wokr that way and request it when missing.

@LasneF
Copy link
Member Author

LasneF commented Jan 31, 2024

but only for the rare few people making their OAS available in multiple documentation tools, or using multiple linters,

The issue is that you have a lot of case where the OAS spec is involved by several layers , and several tooling especially ,
so may be not multiple linter, or documentation but differents stacks like

Linter , Mock server , Proxy validator (including API gateway , or just tooling like prism) , as well as code generator with validation enabled . Having a consistent , pre determined behaviour and standardized is usefull here

I understand that readOnly is meta data for JsonSchema (to me it should even be dropped from it) , but as it is touching the payload validation in OAS context having it clarified would be good thing , long time ago it was even part from OAS

@philsturgeon
Copy link
Collaborator

philsturgeon commented Jan 31, 2024

Yes I'm very aware the same OpenAPI could be put through a linter, mock server, proxy, validater, etc, I've worked on all these tools for years.

I'm saying that JSON Schema makes the choice of considering this metadata to be used by the application in ways it sees fit for the same reasons that OpenAPI also does: it's impossible to know what its being used for so trying to clamp down on exactly how this should be used when the same spec is used by so many different tools is difficult and probably counter productive.

You don't want the spec venturing off into guidance of how linters should handle this or how validators should handle this or how SDKs should handle this.

What I'm saying is, I do not think this is a problem, and I do not think anything here needs solving. I think it's ok to leave this as metadata, and tooling should be aware of the context in an appropriate way.

I've noticed you talking in Vaccum issues about this and I have noticed Vacuum handles required wrong with readOnly/writeOnly, that's an issue for Vacuum and I'll talk to David about it, but I don't see that as a reason to try and fix this in the spec because I don't think it can be fixed in the spec.

@handrews
Copy link
Member

handrews commented Feb 1, 2024

@LasneF you can combine readOnlyand required in either direction (see OAI/OpenAPI-Specification#2110) as round-tripping such a field unchanged through GET/modify/PUT should work. "writing" means "changed" not just "took a scenic vacation through the client" 🙁 There's no need to actually write the value unless it changed.

Combining required and writeOnly for GET is going to cause trouble (whether any validation code notices it or not) because forbidding reading literally forbids including the field. Otherwise you want format: password (for strings... format is such a mess). Defining writeOnly fields requires being careful about the semantics of omitting the field in a PUT, since the client can't be expected to guess the current back-end value and include it.

This is all rather involved to include in the spec, but might make for a great page on the Learn site.

@handrews
Copy link
Member

@OAI/tsc review request: I don't think there's anything to do for the spec here, as there are too many possibilities that might "look" wrong but actually have value. This is not entirely unrelated to the question of how strict to make the schemas that came up in OAI/OpenAPI-Specification#3837 (comment)

Unless we want to change something in the spec we should close this (and maybe open an issue on the Learn site, as it would be worthwhile to have a more in-depth discussion of this complex topic).

@LasneF
Copy link
Member Author

LasneF commented May 31, 2024

thanks for the tips OAI/OpenAPI-Specification#2110 , is clear as well as the "old" specification in 3.0 was clear
https://github.com/OAI/OpenAPI-Specification/blob/main/versions/3.0.2.md#user-content-schemareadonly

" If the property is marked as readOnly being true and is in the required list, the required will take effect on the response only."

with the switch to JSON schema this rules becomes less clear, but the spirit is there.

as it is clarified, i am closing this ticket ... i can open a ticket on my favorite linters 😅

@handrews
Copy link
Member

If the property is marked as readOnly being true and is in the required list, the required will take effect on the response only."

@LasneF this guidance was removed intentionally. It is NOT the spirit in which the keyword should be used in 3.1, and a failure to enforce required based on context would be an error.

@handrews
Copy link
Member

@LasneF I'm going to re-open this and transfer it to the Learn site because you assuming that you can apply the 3.0 to the 3.1 text when we intentionally removed that text in 3.1 shows that we need to do something about it.

@handrews handrews reopened this May 31, 2024
@handrews handrews transferred this issue from OAI/OpenAPI-Specification May 31, 2024
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

No branches or pull requests

3 participants