OAI / learn.openapis.org

OpenAPI - Getting started, and the specification explained
https://learn.openapis.org/
Creative Commons Attribution 4.0 International
119 stars 58 forks source link

ReadOnly / WriteOnly clarification toward required field #101

Open LasneF opened 8 months ago

LasneF commented 8 months ago

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 ,

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 commented 8 months ago

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

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 commented 8 months ago

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 commented 8 months ago

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 commented 8 months ago

@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 commented 4 months ago

@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 https://github.com/OAI/OpenAPI-Specification/pull/3837#issuecomment-2127675043

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 commented 4 months ago

thanks for the tips https://github.com/OAI/OpenAPI-Specification/issues/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 commented 4 months ago

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 commented 4 months ago

@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.