-
-
Notifications
You must be signed in to change notification settings - Fork 6.6k
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] Add parameter to standardize handling null types in all language #12257
Comments
Is this issue regarding whether a property is nullable? Required only defines the nullability in regards to a codegen parameter. When dealing with a codegen property, use isNullable instead. |
@devhl-labs that's not how the openapi spec works, it doesn't differentiate between a codegen parameter and a property, they are both part of the schema and they both have |
To be clear, your concern is what we set isNullable to when a property/parameter is both non-required and non-nullable? I also had this concern, and I toyed with using setting the nullability to |
the problem isn't the nullability, it's the default value but shouldn't the user be able to customize this behavior ? |
The problem is that in the C# RestSharp templates, the parameters all get a default value. This is a mistake in my opinion, and it is one that I have fixed in my generichost library. If the definition states that a field is required and not nullable and no default value, then the user must provide a value. The mustache template should validate the input and throw where appropriate. If the property is nullable and the user provides no value, then use the default value. If the default value is null, then it's null. |
but why "force" users to do anything? users should be able to customize the behavior they want, which is my suggestion here |
If there is a method, get_by_id, that takes an required non nullable with no default, and the user tries to run it without providing a value, the only reasonable thing to do is throw. Don't even send the request to the server. The spec says the parameter is required and not nullable. The user must obey that. If there is a default then the spec is wrong. Update the spec. |
what we are talking about here is non-required non-nullable params with no default value |
Then you would mark the parameter as nullable and only add the value to the request when not null. |
as I have said multiple times, I am giving the user an option to do what they want |
Can you give an example of this schema?
Isn't that just an optional property with a type defined? Some examples here: 1
the object can have an optional key foo with a string value. The key value pair for foo may be included or it may be omitted. 2
the object can have an optional key foo with a value containing any json type (string/int/float/null/array/object). The key value pair for foo may be included or it may be omitted. If an optional value is omitted from an object client side it should not be sent to the server. Likewise if a server is sending back an object that lacks the optional value, the key value pair should not be included. |
lets take the first case |
the problem is that this specific case itself is illegal, since non-required means the server might not send it, but at the same time, it does not tell the generator what to do when the server does not send it (by not specifying a default value). if it was just validating an object against a schema that would be fine, but openapi handles generating the object as well. |
So what you have described sounds like it comes from the context of a compiled language. Is that correct? What you have said is not true for all generators or all compiled languages. InterpretedIn python-experiental
CompiledJava, use JsonNullable per #3435 Go-experimental nullable support: |
@spacether having a "special" this can be added as an option number 6
However this won't be supported by all languages (especially strictly typed ones) |
If this is a feature that we add, then python-experimental would need an option like:
What does
|
also by default, if a variable is marked as not required, it shouldn't be serialized if |
Thank you for explaining what would raise the error.
That sounds like an implementation decision. I disagree that it should be done that way. If the defaultValue is allowed, then I think that the client should also be able to send it if a developer manually specifies it. As an implementation one could choose not to send the value when it is == to defaultValue; one can do it that way if one wants. |
but isn't that what the
this is already the default behavior in many generators including what we are discussing, is what to do if the spec doesn't provide a default value, and the property is neither nullable (so we can't assume the default value to be null) nor required (so we can't force the user or the server to give us a value) |
Null can be a valid value for an optional variable that needs to be sent to the server. In use case 2, foo can have the value null which should be sent to the server. |
@spacether in that case, it should be marked required and nullable with a default: null (implicit) |
The default is for if the value is not sent. What if it is optional, nullable int with a default of 2. It is still optional and null should be sent. It is not required. Values could not be sent. |
according to the spec, a required property means that it must exist in the json, with no regard to its actual value. giving a non-required property a default value should be equivalent to completely omitting it from the json however this isn't true for all languages, e.g. js |
Agreed, that's why I described this as not being sent.
That’s an implementation detail. Just because our generator does something doesn't mean that it conforms to the spec. We should strive to do the best job that we can in the generators and conform to the spec. All languages support maps/dicts. Optional properties could be stored in them if they exist and omitted if they do not. I think go or go-experimental does that for additionalProperties which are optional. |
the problem is, the spec doesn't explain what to do in this scenario, which is why my suggestion here to clarify and standardize that |
@ahmednfwela first of all, thanks for the questions/suggestions. To answer your question, I would suggest we first imagine there's no such thing called OpenAPI and back to days when the APIs are described by HTML documentation. Let' consider https://developer.twitter.com/en/docs/twitter-api/tweets/timelines/api-reference/get-users-id-tweets#tab0. The response has some optional fields which do not specify the default value. Would that result in confusions to developers who create SDKS in a particular programming language (e.g. Java, C#)? I don't think so as I've seen many API clients in different programming languages created for Twitter API. My understanding is that the owner of the API (Twitter API in this case) doesn't care how the optional fields are stored in the client SDKs (e.g. nil, undef, null, etc). For the request body sent to the API server, it checks to see if it contains the required fields (or the "Default" fields in the Twitter API documentation), e.g. https://developer.twitter.com/en/docs/twitter-api/tweets/manage-tweets/api-reference/post-tweets#tab2. Notice that some optional fields are omitted in request body (JSON) sent to the server. For your use cases in which you would like to explain clearly how to store the optional fields in the client side, you can specify the For the situation in which we're the consumer of the API (not owner), we only need to ensure the auto-generated client is doing the right thing by ensuing the payload does not contain optional fields that are not set by the users of the client. I hope this answer your question regarding "when a property has no default value, and is marked non-required and non-nullable" |
@wing328 thanks for the great reply! I understand the general consensus of
if we are talking about Json schemas that would be perfectly fine, since the server doesn't see nor control the consumer's behavior for handling non-required fields, maybe consumer is storing it in RAM as but OAS isn't just Json schema, it builds on top of it and provides more information per schema. taken from the official docs:
so now we are telling the consumer what to do when an optional field is not provided and it's not just set it to null, something Json schema didn't do before. so the only problem here is the case
so my suggestion here is very simple: |
That's referring to 3.0.3 spec. The latest 3.1.0 now supports 100% compatibility with the latest draft (2020-12) of JSON Schema: https://www.openapis.org/blog/2021/02/18/openapi-specification-3-1-released so the I used JSON payload as an example but my understanding of the default value is independent of the payload format (XML, protobuf, etc). Imagine there's a payload format in this world in which the default value is crucial for "non-required and non-nullable" property and without it the API functionality may break, then the owner of the API must provide the I want to use #10913 to explain how I view implementation in the client side in particular. As you can see, the users do not want snake_case in the method naming in the Python code but snake_case is the official method naming method in the style guide. Clearly he's the right to do so. The code still works with non-snake case but if we accept such "enhancement", we need to provide a lot of options down the road. The PR author in this case (or other users have similar need) can maintain a customized version of openapi-generator to meet his unique requirement. In your original post, you listed our 5 possible values for the option In C#, there's something called |
this is a problem that happens when a property has no default value, and is marked non-required and non-nullable
related: OAI/OpenAPI-Specification#2807
Describe the solution you'd like
handlingStrategyForNonNullableNonRequired
)FALLBACK
old behavior so we won't cause a breaking changeassumeNullable
assume that it'snullable
assumeRequired
, assume that it'srequired
assumeDefault
, assumes adefault
value based on the clr (e.g. in c# that would bedefault(T)
, in js it would beundefined
)raiseError
prevents handling this case altogetherFEATURE SET
what of the 5 values they support (All languages will supportfallback
,raiseError
,assumeNullable
,assumeRequired
by default, since they can be handled globally)I can start working on the implmentation if someone can point me in the right direction
@wing328
The text was updated successfully, but these errors were encountered: