-
-
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
[BUG][codegen] Nullable references, nullable primitive types and complex types #5180
Comments
Summary of the proposal:
OptionalAddress:
oneOf:
- type: 'null'
- $ref: "#/definitions/Address" I am primarily interested in Java, Go-experimental and python-experimental, so I will focus on these, but the same constructs apply to every language. |
It's also the tricky thing in C# 8.0 where there is a semantic separation for nullable and non-nullable reference types. Please see the following bug. |
Hey, I was the one that originally sent in this request a few months ago. I was wondering what the status of this ticket is, and if there is any chance of this being fixed in the foreseeable future. When trying to generate a client for the following JSON it still gives errors:
The generated code:
Do you think there is any way this will be fixed soon? I imagine a lot of people are using NestJS to generate their swagger. |
Which request? Can you send a link?
Which error specifically?
I don't think this JSON achieves what you intended. Unless I am mistaken, I think what you intended is the value of partner is either a UserMvoDTO (which presumably is not nullable) or the null value. I think you should be using oneOf.
It would be good to understand the specific error. One aspect is to understand whether the issue is specific to a language generator (e.g. C#, go, java...) or if it is language independent (codegen). If it is language-specific, you should open an issue specific to that language (and maybe you have already done it). |
I made the request in your slack channel and sadly don't have a link. After some discussion by you guys you created this particular issue to deal with the problem.
Yes that does make sense, you are correct in your assumption of what we are trying to achieve. However, we use the NestJS framework and this is what the code generates. We will try to look into why the JSON is being generated like that, as that seems to be a NestJS framework issue. Thanks for pointing me in the right direction.
I have not done that yet, as I specified the language and frameworks we were using before this issue was created and I assumed it was taken into account when you guys created the issue. We will first look into the NestJS problem, and if generating the correct JSON fixes the issue I won't have to open another issue for this repo. Thanks for your reply, I'll update you on whether we fix the problem. |
We did some testing by manually changing the JSON to oneOf, but sadly now it gives a different issue.
The generated code:
As you can see, now it's | object, while it should be | null. Perhaps the "nullable": true is incorrect? This seems to be according to specifications and the way NestJS uses it in it's output. Is this something you can help me with here, or should I make a specific issue for Typescript/Nodejs? |
Hi Both, Thanks for the engagement.
In my opinion, it's correct, from C# perspective at least, so you can handle as a specific issue for Typescript/Nodejs. However, do you really think it's the right thing? oneOf should be used for handling the inheritance, so I don't fully get why the nullable prop cannot be used the same way as with the primitive types? e.g.
Do you know if there is any update on that?
|
No, because 'Nullable' is not the same thing as the null value. If on the other hand, you had the OAS document below, it would be exactly UserMvoDTO or the null value.
Note that "type": "null" is introduced in OAS 3.1. But that's not what you wrote. What you wrote is "nullable": true, without any other OAS attribute. That means it can literally be any JSON document. The following documents will pass the validation for nullable: true
|
I think you may be confusing "nullable": true with type: 'null'.
|
@sebastien-rosset I don't think, it's confusing with type: 'null'. According to the OpenApi Specs, the Nullable param can coexist together with type: 'null'. See more details below. OAI/OpenAPI-Specification@3cb92bd More discussion: |
Alright, well at this point I've tried both of your solutions but neither of them generated valid code. When I use the following JSON: I get the following code: And then in the return value the following code has errors:
At this point I'm not sure what I should do. The JSON we had originally was what NestJS generated using the Swagger plugin. NestJS is a pretty popular framework, so while I can believe they've made mistakes generating the Swagger JSON, it seems strange that we're the only one with this issue. Before making an issue at the NestJS repository, I'd like to at least have a working example of what the JSON should look like. The requirements are pretty simple in my opinion, I want a nullable custom type. It works for Nullable primitive types, just not for custom types. If you could be so kind as to give me an example of a nullable custom type that actually works in your generator for Typescript, I'd be very much obliged. Thanks for the effort you guys have already put in. |
I tend to focus on generators for Java, go, powershell and Python, and I will say upfront I don't know about the generators for NetJS. I can tell you what my experience has been for Java, go, powershell and Python, maybe this will be applicable to NetJS. I have used OpenAPITools to generate SDKs for Java, go, powershell and Python. It is an awesome tool, and the community is great. That being said, it is an open-source community, so when facing an issue, I knew we would have to dig into the code to understand the issues, and in some cases submit pull requests. |
Yes, I wasn't saying they cannot coexist. I was saying that if you write "nullable": true, you should not be surprised to see the following code being generated:
That's because nulllable: true means it can be any arbitrary JSON document. |
Also, when I wrote this issue, I was thinking about language-independent modeling, i.e. codegen. I did merge #5290 to support this, so I think I can close this issue. Then I opened specific issues and PRs for go, Java and python. It appears the issue you are reporting is specific to a particular language, so it would make sense to open a separate issue, and ideally submit a pull request. |
Alright, thanks for your feedback and help. I will see if I can report a language specific issue. |
Thanks for such a great summary of the problem @sebastien-rosset .
|
This is working in the |
Bug Report Checklist
Description
There is inconsistent support for nullable types across language generators. Some language generators (e.g. "go") are lenient and allow null values even if the OAS spec does explicitly list nullable types. Other language generators (e.g. Python) do strict validation and raise an Exception if the server returns a null value that wasn't explicitly allowed in the OAS spec.
To complicate matters, there are multiple ways to specify nullable values in the OAS spec, and there are changes between 3.0 and 3.1:
openapi-generator version
4.2.3
OpenAPI declaration file content or url
Suppose an OAS spec defines #/definitions/Address of type: object:
In OAS 3.0, Address can be marked as nullable:
Then other types can reference "Address", such as an "Order" type that has a "billTo" property, and "billTo" can have a null value:
One issue is that at least one language generator (python-experimental) is ignoring the "nullable" attribute. I.e. in the case of Python, when the server returns a null value for "billTo", the client raises an exception because it did not expect a null value.
On the other hand, the "go-experimental" code generator accepts null values, even if "nullable: true" wasn't specified in the OAS spec.
Another way to specify a nullable property is to use the vendor extension "x-nullable". A grep shows "x-nullable" is still used in a few OpenAPITool files. Is this still needed? I think this is a remnant of pre-3.0 OAS, before the "nullable" attribute existed.
In addition, "nullable" brings other issues that are described in Clarify Semantics of nullable in OpenAPI, hence it looks like the OAS TSR is deprecating "nullable" in 3.1.
Another problem with the above YAML is that it's not possible to selectively specify which properties of type "address" are nullable or not. For example, suppose the "Order" type has two properties, "shipTo" and "billTo":
Let's assume for the sake of this argument that "shipTo" must be set to a non-null value, and "billTo" can have a null value. Using "nullable" inside "Address" would not work, or at least the same "Address" type could not be reused for both the "shipTo" and "billTo" properties.
With the above, both "shipTo" and "billTo" can have a null value, which is not what was intended.
To address this issue, one may be tempted to use the nullable attribute as a sibling of $ref. Instead of specifying the nullable attribute in "Address", "nullable" would be specified in the property, as shown below.
However, the above is not supported in 3.0, the spec explicitly states all sibling attributes of "$ref" are ignored, and that seems to be ignored by the OpenApiTools code generators as well.
Note: the OAS TSR is considering (but nothing is decided) supporting siblings attributes of "$ref" and the associated merge semantic. However it's unlikely this will apply to "nullable" because "nullable" is deprecated anyway.
One way to workaround the above issue is to define two types, "Address" and "OptionalAddress":
Then the "Order" type can be changed as below:
However this has two problems: the OAS spec becomes verbose (maybe the OAS ends up specifying XYZ types and OptionalXYZ type for everything), and anyway "nullable: false" is a no-op.
Maybe a bigger issue is that "nullable" is being deprecated in 3.1, so authors should stop using nullable and use other constructs to specify nullable properties.
Besides the "nullable" attribute, another way to specify nullable properties is to use "oneOf":
This seems to be the standard, future-proof way to specify nullable properties for complex types.
Note: type: 'null' is not supported in OAS 3.0.x. It is being introduced in OAS 3.1.
From the perspective of the OpenAPITool code generator, there are a few problems:
It would be really nice if the generated code is simple, and looks the same whether it was written using "oneOf" or nullable. However, this does not seem very simple to achieve because the OAS spec could have more than oneOf types, such as below:
Finally, for properties that are of a primitive type, and the type should accept a null value, it's possible to use type arrays in OAS 3.1. This is the equivalent of "oneOf" for primitive types:
which is equivalent to:
Command line used for generation
Steps to reproduce
Related issues/PRs
Suggest a fix
The text was updated successfully, but these errors were encountered: