Improvements to nullable references.

[brandonbloom]

Motivation

A common pattern in OpenAPI schemas I have seen uses oneOf with references like the following:

oneOf:
  - $ref: '#/components/schemas/Thing'
  - type: 'null'

Prior to this patch, this returns a enum for the possible variants of this schema, but an Optional<T> would be more expected and ergonomic.

Modifications

In addition to the basic support for explicit nulls (#557, included in this PR too), I've introduced a notion of an "optional root", which is a schema / type usage in which the optionality occurs at the top level and not through some indirection of references. This undoes the prior behavior of propagating optionality all the way to the top, instead adding it only where needed.

This new model of optionality relies on relies on enhancements to TypeMatcher isOptional to look for explicit nulls and to support following references and dealing with potentially recursive schemas.

Finally, TypesFileTranslator detects the {oneOf: [{type: 'null'}, ...]} pattern and returns a Swift Optional<...remaining cases...> instead.

Result

The added testOneOfRefOrNull test shows the new behavior in action.

This would be a breaking change for anyone using this common pattern. A few places where enums were needed will need to be migrated to use optionals instead of the generated enums. It's a mechanical and straightforward migration, resulting in less and cleaner code.

Test Plan

Several new unit tests were added. See diff.

[czechboy0]

Hi @brandonbloom,

thanks for the PR! We might just need to split this up a bit before merging.

First thing - this is a breaking change, and if merged as-is, would require us to tag a 2.0. That's not really desirable at this stage, so I wonder what you suggest we do here.

Can you talk more about the reasoning of why you think the generated code needs to change? We've avoided special-casing specific patterns so far, as they don't evolve very well when a new case is added. Can you provide more details of why we should diverge from that approach here? (Check out the design principles here: https://swiftpackageindex.com/apple/swift-openapi-generator/1.2.1/documentation/swift-openapi-generator/project-scope-and-goals#Principle-Generate-code-that-evolves-with-the-OpenAPI-document)

[brandonbloom]

We might just need to split this up a bit before merging.

Understood. I broke up the steps as separate commits for that reason.

Can you talk more about the reasoning of why you think the generated code needs to change?

I found the behavior that exists prior to this change very confusing. The generated types don't match the schemas that they are generated from as it pertains to optionality. Instead, the optionality is bubbled up to the top level, and baked into the usage sites. This means that if you use a typealias for something that is expected to be T?, you actually just get a T.

However, the main justification is the {oneOf: [{type: 'null'}, ...]} pattern, which is used widely in a pair of OpenAPI specs I'm working with. In one case, it works way because the spec it itself generated from bespoke schema system which produces a referenceable schema for each named type and then uses the oneOf-null pattern everywhere. In the other case, the upstream schema used to use nullable: true keyword, but that's deprecated now in OpenAPI 3.1 and hence the move to the oneOf:null/... pattern.

We've avoided special-casing specific patterns so far, as they don't evolve very well when a new case is added. Can you provide more details of why we should diverge from that approach here?

That makes sense. Only null is handled specially in this change. I preserved the behavior that a single-case enum remains if null is not involved. I'm sensitive to the evolution argument, but I think that there are enough ergonomics for Optionals in Swift that it's worthwhile. With this change, my usage code has eliminated numerous case-lets and switches and other heavy syntax is favor of much lighter if-lets and ?. operators.

would require us to tag a 2.0. That's not really desirable at this stage, so I wonder what you suggest we do here.

There are two separate breaking changes. The "optional root" behavior and then the subsequent rule for simplifying the oneOf:null/... pattern. The former supports the latter, but the latter could be a setting or preference quite easily, if that's appropriate. Not sure if/how the former can be mitigated.

[czechboy0]

I found the behavior that exists prior to this change very confusing. The generated types don't match the schemas that they are generated from as it pertains to optionality. Instead, the optionality is bubbled up to the top level, and baked into the usage sites. This means that if you use a typealias for something that is expected to be T?, you actually just get a T.

Right, this is where mapping OpenAPI onto Swift 1:1 isn't possible, as Swift doesn't have a concept of optional base types. For example:

struct Foo {} // ✅
struct Foo? {} // ❌ not a thing in Swift, but is a thing in JSON Schema/OpenAPI

So the optionality of schemas in JSON Schema has to move somewhere in Swift. We chose to move it to the usage site, as it mapped best onto how Codable works and seemed to lead the least surprises. An alternative would have been to define a typealias for every nullable type like typealias Foo = _UnderlyingFoo?, which would have been pretty complicated and harder to read.

With the context above, I wonder how you think we should approach this.

Your motivating example is:

oneOf:
  - $ref: '#/components/schemas/Thing'
  - type: 'null'

How can we make sure that if you add a third case, and make it:

oneOf:
  - $ref: '#/components/schemas/Thing'
  - $ref: '#/components/schemas/AnotherThing'
  - type: 'null'

that the nature of the type doesn't completely change? (To keep with our principle of deterministic API evolution)

For example, I do not think the first example should translate to something like Thing?, because it collapses the oneOf, which goes directly against our guiding principles. However, it might be reasonable for it to appear as MyOneOf? where MyOneOf has a single case, Thing. That way, adding a third case just expands MyOneOf.

I realize your PR goes all the way to Thing?, but would preserving the enum still be acceptable to you? Basically, we'd "remove" the type: 'null' case from the oneOf and mark the oneOf itself as optional. Which reminds me, should this possible be done in OpenAPIKit instead, @mattpolzin?

[mattpolzin]

This reminds me a lot of this comment (and the preceeding/following one). Are we talking about a different situation in this case?

[mattpolzin]

If I recall correctly, one reason I want to avoid removing the .null() schema from the oneOf (and similar) is because it is lossy so OpenAPIKit no longer knows where to put the "nullability" when re-encoding (it could be a part of any of the subschemas).

[EDIT] not to derail this conversation, but I'm realizing that OpenAPIKit actually has what I consider buggy logic here at the moment. It treats oneOf with any nullable subschema as nullable; that makes sense for allOf/anyOf, but isn't quite right for oneOf where the nullability should only propagate for a subschema that is exactly type: null (I think). I'll fix that. In that specific case, I could drop the null subschema (but I still think not so for anyOf/allOf).

[czechboy0]

Good call, I forgot about that other issue. So a null case already gets propagated to the parent anyOf/allOf today? If so, does that mean we just need to filter out the null case in the generator and the rest should work?

[mattpolzin]

does that mean we just need to filter out the null case in the generator and the rest should work?

Correct; OpenAPIKit currently sets any/one/all-of schemas as nullable if any of the subschemas are nullable.

Also, I keep waking up more as the morning goes on and now I'm thinking there is no current bug with the way OpenAPIKit handles oneOf (I speculated there was in my previous comment's EDIT).

[czechboy0]

Ok great - @brandonbloom would you be interested in opening a PR for the filtering of nulls, which should hopefully improve how we handle these cases? It's my understanding we completely skip such anyOf/allOfs today, so they don't get generated. If we filter the null out, it should start getting generated as an optional anyOf/allOf, which I what I think we want. We can keep discussing the collapsing of that anyOf/allOf separately, but that'd be a feature flag anyway, as it'd be an API-break, so we shouldn't block this work for that.

[jmg-duarte]

@czechboy0 I'm currently stuck on a compilation error stating:

Schema "null" is not supported, reason: "schema type", skipping [context: foundIn=Components.Schemas.Body_authSignin

This goes on for a bit with a bunch of other endpoints.

I see that #557 is open so I assume some work is being done to address this, but is there anything I can do to work around this issue?

[czechboy0]

Hi @jmg-duarte - as a workaround, you could remove the anyOf that wraps your schema and a null. If the property isn't included in the object's required array, it'll be generated as optional anyway, so the extra wrapping in anyOf that also includes a null is not necessary, and in fact will make the generated code uglier in Swift.

[x128]

Why is this change necessary?
Let's say I have an optional string in my schema:

"line2": {
	"title": "Line2",
	"oneOf": [
		{
			"type": "string",
		},
		{
			"type": "null"
		}
	]
}

It's not marked as required. Full json is attached.

So now, with this change, it generates a nested optional type:

/// - Remark: Generated from `#/components/schemas/Address/line2`.
public typealias line2Payload = Swift.String?
/// - Remark: Generated from `#/components/schemas/Address/line2`.
public var line2: Components.Schemas.Address.line2Payload?

And if we have an optional struct, not a String, it also generates a nested optional, so we have to access its values using generatedObject??.property which is a bit weird.

Without this change, everything seems to work. I didn't find an answer in test cases as well. I was thinking about forking and undoing this change, but maybe I'm missing something, so what's behind that?

P.S. Great job @brandonbloom. I have no idea how much effort is needed to support anyOf/allOf correctly. But in our project we probably need only oneOf for optionals, so this PR is a really nice work.

[jasondiab]

Can this be merged given it is now approved? It would help us avoid using scattered workarounds to make this work 🙏 thanks.

Not a contributor.

[czechboy0]

Hi @jasondiab, this PR is not approved by any of the maintainers, and still needs work. If landed as-is, it'd be a major breaking change.

[colincornaby]

Could this become an opt in feature? It is breaking, but it (along with #557) are causing a lot of churn in having to hand tweak OpenAPI being ingested by the parser. A required opt in would prevent it from breaking semantic versioning.

[mattpolzin]

If I understand the above conversation fully then this PR might not represent the easiest solution which would hopefully simply involve filtering out .null() schemas before generating code. Food for thought if someone has time to try that.