-
Notifications
You must be signed in to change notification settings - Fork 147
WIP: Support type: 'null'
with anyOf and oneOf
#831
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
base: main
Are you sure you want to change the base?
Conversation
|
||
func testPetstore() throws { try _test(referenceProject: .init(name: .petstore)) } | ||
|
||
func testExample() throws { try _test(referenceProject: .init(name: .todolist)) } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we need to add a whole new reference project, that's considerably expensive to maintain.
Could you instead look into the snippet tests we have? You can add test cases there, and those are more focused and easier to maintain.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I started creating some snippet tests, but I'm quickly noticing that snippet tests bypass the GeneratorPipeline
, which is where I currently hooked up the code modifying the parsed document. Thus, my code is not being triggered by those tests. Perhaps that is not the best place for my code to perform the function? Any suggestions?
alternately, I can modify the makeTypesTranslator()
in snippets to call the extension similar to how it is connected in the GeneratorPipeline
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah let's see, maybe move it to makeGeneratorPipeline, where we already have other pre-processing steps:
let validateDoc = { (doc: OpenAPI.Document) -> OpenAPI.Document in |
For testing, some unit tests validating that the stripping of the null
works correctly will be a great start, and then you can add a few examples of such schemas into the existing Petstore file-based reference tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just pushed an update to this PR, which I hope addresses your feedback. I feel pretty comfortable with the basic case of:
anyOf/oneOf:
- $ref: "#/components/schemas/B"
- type: null
but I personally have not worked with cases like:
anyOf/oneOf:
- $ref: "#/components/schemas/B1"
- $ref: "#/components/schemas/B2"
- type: null
To me, the output in the snippet tests looks about correct, but I would hope someone more familiar could verify it.
This is currently marked as Work-In-Progress (WIP) as I'm not fully confident in the approach with a limited understanding of the broader code base. It is fully functional, and perhaps WIP may not be the correct designation.
Motivation
Summary
This PR takes a different approach than the prior PRs mentioned below. It attempts to implement what @czechboy0 suggested by filtering/modifying the initial parsed openapi document at a point where nullablity can be handled and before the rest of the generator kicks in to begin producing an output.
Background
swift-openapi-generator
does not "support" type: 'null' when used withanyOf
andoneOf
. The problem became worse after with yams >=5.1. There are multiple issues filed on and related to this topic:#817
#419
#565
#513
#286
There are also older PRs from @brandonbloom:
#557
#558
But it seems not to be moving forward.
When looking over these issues I have seen:
{ "type": "null" }
from the anyOf and oneOf definitions. This is great if you are starting by defining your own OpenAPI document, but manually doing so is not maintainable when the document is generated and provided to you with regular updates.a) @czechboy0 suggested filtering out null Support
type: 'null'
asOpenAPIValueContainer
#557 (comment)b) @czechboy0 also called out that simply removing the null would be losy Improvements to nullable references. #558 (comment). In this case, we end up with the field being treated as required instead of optional.
c) it needs to work for both any/oneOf { $ref ..., null} and cases with { $ref ..., $ref ..., null} Improvements to nullable references. #558 (comment)
Modifications
Sources / _OpenAPIGeneratorCore / GeneratorPipline.swift
by adding several extensions to perform the schema filtering of null from anyOf and oneOf and if null is found, it marks the in memory schema context as nullable for that field.Tests / OpenAPIGeneratorReferenceTests / Resources
withDocs / totolist.json
(a json based openapi document containing oneOf and anyOf with null) and withReferenceSources / Todolist / (Types.swift, Client.swift, Server.swift)
Tests / OpenAPIGeneratorReferenceTests / FileBasedReferenceTests
to perform tests using the newDocs / totolist.json
Result
The resulting generated code seems to behave as expected.
Test Plan
So far I have run the tests as described in the modifications.