Support type: 'null' with anyOf and oneOf

[dpasirst]

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 with anyOf and oneOf. 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:

1. @czechboy0 suggesting to modify the openapi document by removing the { "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.
2. In the PRs from @brandonbloom, there were challenges and suggestions highlighted
   a) @czechboy0 suggested filtering out null Support type: 'null' as OpenAPIValueContainer #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

1. Modified 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.
2. Added Tests to SnippetBasedReferenceTests
3. Modified SnippetBasedReferenceTests for makeTypesTranslator() to either removingNullFromAnyOfAndOneOf() or sanitizeSchemaNulls(document) to approximate processing that a file read and processed via GeneratorPipline would pass through.

Result

The resulting generated code seems to behave as expected.

Test Plan

Tests added to snippets, snippets modified to call the same function(s) used in the GeneratorPipeline.

[czechboy0]

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.

[dpasirst]

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

[czechboy0]

Yeah let's see, maybe move it to makeGeneratorPipeline, where we already have other pre-processing steps:

swift-openapi-generator/Sources/_OpenAPIGeneratorCore/GeneratorPipeline.swift

Line 111 in c6e8513

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.

[dpasirst]

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.

[dpasirst]

removed "WIP" from the title. I think it is ready for a real review.

[czechboy0]

Thanks @dpasirst - just a heads up that it'll take me some time to properly review, as once we make this change, it'd be a breaking change to revert it. I also want to see if the new OpenAPI versions 3.2.0 and 3.1.2 offer further clarification about semantics related to nullable values.

[kozzza]

Hey any updates on this?

[czechboy0]

Not yet. It will require substantial changes to solve this properly.

[kozzza]

As a temporary fix, I got my project working with this patch. Thought I would leave it here for anyone else using a generated schema from FastAPI.

from fastapi.openapi.utils import get_openapi

# your app
app = FastAPI(
    title="app",
    version="1.0.0",
    servers=[]
)

# 1. finds model fields with anyOf that has exactly 2 properties with one of them being 'null'
# 2. grabs the property that isn't null (this is the actual type that we care about)
# 3. rewrites the property in place to a format that parses to an optional in swift
def _rewrite_anyof_nullable(schema: dict) -> None:
    """
    mutate the schema in place:

    anyOf: [{type: "string"}, {type: "null"}]
      -> {type: "string", nullable: true}
    """

    components = schema.get("components", {})
    schemas = components.get("schemas", {})

    for _, model_schema in schemas.items():
        props = model_schema.get("properties", {})
        for _, prop_schema in props.items():
            any_of = prop_schema.get("anyOf")
            if not (isinstance(any_of, list) and len(any_of) == 2):
                continue

            # look for exactly one null type and one non-null
            has_null = any(isinstance(x, dict) and x.get("type") == "null" for x in any_of)
            if not has_null:
                continue

            non_null = next((x for x in any_of if x.get("type") != "null"), None)
            if not isinstance(non_null, dict):
                continue

            # replace property schema with non-null branch + nullable flag
            prop_schema.clear()
            prop_schema.update(non_null)
            prop_schema["nullable"] = True


# reassign to patched openapi schema
def custom_openapi():
    if app.openapi_schema:
        return app.openapi_schema

    openapi_schema = get_openapi(
        title=app.title,
        version=app.version,
        routes=app.routes,
        servers=app.servers,
    )

    _rewrite_anyof_nullable(openapi_schema)
    app.openapi_schema = openapi_schema
    return app.openapi_schema


app.openapi = custom_openapi