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

[REQ] OAS 3.0 support of readOnly/writeOnly modeling #4190

Open
jimschubert opened this issue Oct 20, 2019 · 23 comments
Open

[REQ] OAS 3.0 support of readOnly/writeOnly modeling #4190

jimschubert opened this issue Oct 20, 2019 · 23 comments

Comments

@jimschubert
Copy link
Member

jimschubert commented Oct 20, 2019

Is your feature request related to a problem? Please describe.

Yes, missing support for OAS 3.0 feature of readOnly and writeOnly properties within a model.

Discussion came up on Slack, and I'm documenting it here since we need to implement this for full OAS 3.0 support.

Describe the solution you'd like

Provide a writeOnlyVars to match other vars in our codegen model. Support differentiation between request models and response models.

Describe alternatives you've considered

Early discussions about writeOnly on OAI suggest domain modeling around usage of allOf. See OAI/OpenAPI-Specification/issues/425.

Additional context

OAS 3.x Schema Object supports two properties, readOnly and writeOnly. These properties are defined as:

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.

We should validate that a property is not both readOnly=true and writeOnly=true. This validation should be excluded from strict spec skipping as having a property both readOnly and writeOnly should be a logical error with undefined behavior.

The use of SHOULD NOT in the spec is defined via rfc2119:

SHOULD NOT This phrase, or the phrase "NOT RECOMMENDED" mean that
there may exist valid reasons in particular circumstances when the
particular behavior is acceptable or even useful, but the full
implications should be understood and the case carefully weighed
before implementing any behavior described with this label.

So, we're not technically out of compliance with the spec in relation to readOnly and writeOnly since our use case to support OpenAPI 2.0 and OpenAPI 3.0 functionality in the same generator + template combinations could be considered a "particular circumstance". However, the we are out of compliance with the clause regarding required because this makes the property required only in a request or response structure, and not both.

My proposal is to do multiple passes on models defined in the OpenAPI result object and sort these into an array of "RequestModels" and an array of "ResponseModels", leaving our current "models" collection as a raw collection for backward compatibility. This would allow us to have the same schema definition defined appropriately (according to the specification), without hacky workarounds like adding arbitrary prefixes or suffixes to differentiate.

We may be tempted to suggest that users split their schemas, similar to the recommendation linked above (OAI/OpenAPI-Specification/issues/425), but this will only address the issue for those users who own/manage their specifications. To allow code generation from external systems which follow OpenAPI 3.0 specification, we'd need to support this use case.

@jimschubert jimschubert changed the title [REQ] Feature Request Description [REQ] OAS 3.0 support of readOnly/writeOnly modeling Oct 20, 2019
@bodograumann
Copy link
Contributor

I would also love to see this feature.
From the typescript perspective we are currently simply exporting all models. So if there will be one “RequestModel” and one “ResponseModel” (which I think makes sense), the exports will have to be handled differently.

We will definitely first have to rewrite the typescript generator, in order not to have to implement it in all of them.

@sebastien-rosset
Copy link
Contributor

sebastien-rosset commented Mar 11, 2020

FYI, the python-experimental client does serialize read-only properties. I think it should skip serialization of the readOnly properties.
@spacether, what do you think?

  1. Add readOnly attribute to the generated model code, i.e. modules/openapi-generator/src/main/resources/python/python-experimental/model_templates/
  2. During serialization, skip properties that have readOnly: True

@spacether
Copy link
Contributor

spacether commented Mar 11, 2020

@sebastien-rosset this could be added to python-experimental the following way:

  • at the model level add a list or set of readonly_vars
  • at the model level add a list or set of writeonly_vars
  • when setting values we already pass in the boolean from_server so use it to check the readonly and writeonly conditions and throw errors accordingly. For example an error should be thrown if a writeonly variable is in the response. An error should be thrown if a readonly variable is set when from_server=False (a model is made for a request).
  • since these properties are required but may now be omitted from the request or response, these properties with readonly and writeonly must be made into named arguments with the default value of nulltype.Null. Our mustache template will change these variables to named arguments if we set a default value. We already set null defaults in the java method addNullDefaultToOneOfAnyOfReqProps. When the value is Null they will not be assigned. If it is anything else, then it will be assigned.

I have a general question though. Don't codegen parameters include the readonly and writeonly properties? Why do we need the new lists in Codegenmodel if we already have this info?

@sebastien-rosset
Copy link
Contributor

One consideration is attributes beyond read only and write-only, maybe through vendor extensions. For example, we use read-on-create for cases such as when the backend generates an API key secret and returns the value only when the resource is created. We also have create-only for properties that can be set during creation but cannot subsequently be modified.

Instead of creating lists of readonly_vars and writeonly_vars, can this be done using meta-data for each property? I.e. in the case of python, you already generate the "openapitype" meta-data for each property. Couldn't we add one more type of meta-data there?

@spacether
Copy link
Contributor

spacether commented Mar 11, 2020

Thanks for sharing that context. I wasn't aware of those use cases.

So far each CodegenProperty metadata has been stored separately. A helpful refactor in python-experimental would be to create some class like CodegenProperty and gather all the data for each parameter there. Then in ModelNormal/ModelComposed we can store a dict where the key is the variable name and the value is the CodegenProperty class instance. One could also do this with just a dict to begin with.

Wouldn't that read on create and create only vendor extension need to be individually handled by generators? I am worried about us adding too many lists of vars in CodegenModel.

@spacether
Copy link
Contributor

Though looking at CodegenModel we already have readOnlyVars, so sure, let's add writeOnlyVars if we have already started going down this path.
https://github.com/OpenAPITools/openapi-generator/blob/master/modules/openapi-generator/src/main/java/org/openapitools/codegen/CodegenModel.java#L61

@sebastien-rosset
Copy link
Contributor

Thanks for sharing that context. I wasn't aware of those use cases.

So far each codegenproperty Metadata has been stored separately. A helpful refractor in python-experimental would be to create some class like codegenproperty and gather all the data for each parameter there. Then in ModelNormal/Modelcomposed we can store a dict where the key is the variable name and the value is the codegenproperty class instance. One could also do this with just a dict to begin with.

I like this.

Wouldn't that read on create and create only vendor extension need to be individually handled by generators? I am worriedabout us adding too many lists of vars in Codegenmodel.

It seems that instead of maintaining separate lists, each property could have a list of meta-data attributes, and the generated code could look up the meta-data. For example, it could decide what to do for the serialization. If we want to get fancy, we could also expose a callback to the application such that app developers can insert their custom serialization logic.

{
readOnly: False,
writeOnly: False,
x-read-on-create: True,
...
}

@last-partizan
Copy link

Is there any roadmap for this issue?

I'm using typescript-axios generator and it generates invalid code, which requires read-only params for request.

@MLeipziger
Copy link

Is there any update on this issue? As mentioned by @last-partizan this generates invalid code.

@rdelcampog
Copy link

Need this :(

@edy-rodrigues
Copy link

I need this too!

@spacether
Copy link
Contributor

spacether commented Apr 15, 2022

One can also do this with composition instead of readonly/writeOnly.
Define one schema with shared properties.
Define another for readonly and allOf include the shared schema and define readonly additions.
Define another for the writeOnly schema in the same way. This way it is deterministic which properties are in the readonly and writeOnly schemas.

One can have non deterministic behavior by using readonly. Here is an example:

A:
Properties:
a:
ReadOnly

B:
properties:
a:
writeOnly

C:
AllOf:

  • A
  • B

Is C.a readonly or writeOnly?

@bodograumann
Copy link
Contributor

Readonly and writeonly flags are now implemented for properties with simple types like string, but not when a property has a custom model as its scheme.
So we only need it for the latter as well?

@creyD
Copy link

creyD commented Nov 19, 2022

@bodograumann I believe this would be crucial for many projects as well.

@naoyahieda
Copy link

This will be helpful for many developers!!

@UnleashSpirit
Copy link
Contributor

Damn, thought it was already good and found this issue :(
Need it for spring generator

@Steeve55
Copy link

Need this too please :)

@s-jepsen
Copy link
Contributor

Any progress on this issue?

@jorgerod
Copy link
Contributor

jorgerod commented Jan 31, 2024

I think it is a very important feature.

Is this on the roadmap? @wing328 @jimschubert

@bejvisek
Copy link

Any news on this issue?
Honestly, I don't see a way to use the API without this.
How do you handle field "ID" - it's always returned, but typically not send in "request", because it's created by the server.

@daniel17903
Copy link

Any progress on this issue?

@subhanshu-shukla-ril
Copy link

Any progress on this issue?

@awais786
Copy link

I am new to this and I have a question. I have writeonly field in schema. but generated model is doing validation on it and throwing error any ideas or any flag to skip this ?

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

No branches or pull requests