Open ahmednfwela opened 2 years ago
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 required
/nullable
status
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 !required && isNullable
the problem isn't the nullability, it's the default value when a default value is absent and it's not required (the user doesn't have to enter it), the generator has to assume it but how do you assume it in this case? you are suggesting that we just assume it as null (option 2 i declared above)
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 they can either mark that specific parameter as nullable, required, or give it a default value
Can you give an example of this schema?
property has no default value, and is marked non-required and non-nullable
Isn't that just an optional property with a type defined?
nullable: false has no impact per:
A true value adds "null" to the allowed type specified by the type keyword, only if type is explicitly defined within the same Schema Object. Other Schema Object constraints retain their defined behavior, and therefore may disallow the use of null as a value. A false value leaves the specified or default type unmodified. The default value is false
Some examples here:
type: object
properties:
foo:
# required: false should not be here, this property should be omitted from the list of required property names
# nullable: false has no impact because false by default
type: string
required: []
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. foos value may not be null.
type: object
properties:
foo: {}
required: []
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 when a generator attempts to generate this schema, the property foo doesn't have a default value and isn't required so the generator needs to assume its default value. IF it was nullable the default value would be null, but since it isn't nullable, the generator has to do one of the 4 options I wrote above. generators do one of them implicitly anyway and the most common is to assume the property nullable
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. Below are some solutions that don't require your proposed solutions. @wing328
In python-experiental
Java, use JsonNullable per https://github.com/OpenAPITools/openapi-generator/issues/3435 Because one can store undefined (unset) values in it per: https://github.com/OpenAPITools/jackson-databind-nullable jersey2 uses this
Go-experimental nullable support: https://github.com/OpenAPITools/openapi-generator/pull/5414
@spacether having a "special" undefined
value is also another solution that's similar to assuming a default value.
this can be added as an option number 6
assumeDefaultIsUndefined
that is different from assumeDefault
in that it creates a special value called undefined
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:
omittedFromPayloadIfUnset
python-experimental does not use a sentinel value. I was just mentioning it as a possible solution.What does raiseError
mean?
raiseError
means the tooling (generator) would raise an error if a spec schema had this use case in it.
also by default, if a variable is marked as not required, it shouldn't be serialized if value == defaultValue
this applies to all generators, not just python
Thank you for explaining what would raise the error.
it shouldn't be serialized if value == defaultValue this applies to all generators, not just python
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 required
parameter was made for ?
this is already the default behavior in many generators including dart-dio-next
(now dart-dio
) and charp-netcore
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 where an omitted property will be assigned undefined unless specified otherwise.
giving a non-required property a default value should be equivalent to completely omitting it from the json
Agreed, that's why I described this as not being sent.
however this isn't true for all languages, e.g. js where an omitted property will be assigned undefined unless specified otherwise.
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 default
field (assuming you're the owner of the API). If you foresee the default value is going to be different for different programming languages, you may want to use extensions, e.g. x-java-default, x-csharp-default.
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 it's not required and the user didn't set it, just don't send it, who cares what you have on client side
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 undefined
if they were using js
or setting it to null
+ some annotations if they were using c#, or maybe wrapping every field in a class like java's JsonNullable
but OAS isn't just Json schema, it builds on top of it and provides more information per schema.
taken from the official docs:
default - The default value represents what would be assumed by the consumer of the input as the value of the schema if one is not provided. Unlike JSON Schema, the value MUST conform to the defined type for the Schema Object defined at the same level. For example, if type is string, then default can be "foo" but cannot be 1.
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
non-required
- might not be present when receiving or sending a jsonno default
- consumer doesn't have instructions on what to set it to when it's not presentnon-nullable
- doesn't accept nullsso my suggestion here is very simple:
give power to the developer to how to handle that specific case in ANY generator by using one of the approaches I wrote above, the only thing that needs to be handled per generator is the assumeDefault
way, since each language has their way of handling this
but OAS isn't just Json schema, it builds on top of it and provides more information per schema.
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 Unlike JSON Schema
part is no longer true in the latest version.
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 default
value in the OpenAPI spec/doc in such case.
I want to use https://github.com/OpenAPITools/openapi-generator/pull/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 handlingStrategyForNonNullableNonRequired
. No matter which one we choose, it may not be what the API owner have in mind.
In C#, there's something called DataMemberAttribute.EmitDefaultValue
. I think the option is somehow similar to what you're trying to do. I think your use case at this stage is the Dart client generator. What about adding an option to the Dart client generator (after discussion with the Dart community in this project) to start with instead of adding an option to ALL generators (100+), which is a lot of work?
visiting on this after this PR https://github.com/OpenAPITools/openapi-generator/pull/14172 we can add normalizers to handle all options mentioned above, what do you think ? @wing328
this is a problem that happens when a property has no default value, and is marked non-required and non-nullable related: https://github.com/OAI/OpenAPI-Specification/discussions/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