Skip to content
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

Support type: 'null' as OpenAPIValueContainer #557

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

brandonbloom
Copy link

Motivation

OpenAPIKit's handling of type: 'null' was problematic with older Yams versions. See https://github.com/mattpolzin/OpenAPIKit/pull/366/files for a test demonstrating that & dependency version bump.

Similarly, swift-openapi-generator explicitly does not "support" type: 'null'. As part of searching for something to address #513 I have added such support for explicit nulls. This gives one more sort of input schema where swift-openapi-generator produces some useful output instead of skipping a schema.

Modifications

  • Bumped minimum supported Yams version.
  • Returntrue from isSchemaSupported for type: 'null' schemas.
  • Defined .null to have a builtint ype of OpenAPIValueContainer

Result

Nothing should change for existing users because type: null schemas were previously unsupported. Going forward, usages of these schemas is supported.

Test Plan

New test included.

@brandonbloom brandonbloom changed the title Support type: 'null' as OpenAPIValueContainer Support type: 'null' as OpenAPIValueContainer Mar 31, 2024
@@ -289,6 +289,7 @@ struct TypeMatcher {
private static func _tryMatchBuiltinNonRecursive(for schema: JSONSchema.Schema) -> TypeUsage? {
let typeName: TypeName
switch schema {
case .null(_): typeName = TypeName.valueContainer
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm I'm still unsure whether we want to promote null to a JSON value here. Let's discuss in the other PR first, then we can come back to this.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also considered the singleton instance of NSNull, or some new enum type with one case, or () or something. It wasn't obvious if there was a more appropriate type for this in Swift.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm maybe we should introduce an typealias OpenAPINull = Void in the runtime library, and use that? What do you think? cc @simonjbeaumont

OpenAPIValueContainer isn't appropriate, I think, as it erases our knowledge of it being null, forcing the adopter to check again later. We should be able to avoid that.

@czechboy0
Copy link
Contributor

Based on the discussion in the other thread, I think we should not merge this PR, and instead we should simply filter out null from anyOf/allOf schemas, as OpenAPIKit will already represent the nullability on the parent schema.

@simonjbeaumont
Copy link
Collaborator

We had a report that this is blocking generation for the Figma API: https://github.com/figma/rest-api-spec/blob/main/openapi/openapi.yaml, which I can confirm doesn't compile as-is.

@Brett-Best
Copy link

It would be great to get this merged as it unblocks using the Figma API as Simon mentions.
Anything I can do to help?

@czechboy0
Copy link
Contributor

I think we can't merge it as-is, as it'd be an API break. Plus, see my earlier message, I think we should fix this slightly differently.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants