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

Fix bug where object schemas representing an allOf type with more than one schema but only one property were inferred as the wrong type #181

Merged
merged 2 commits into from Feb 22, 2023

Conversation

liamnichols
Copy link
Member

@liamnichols liamnichols commented Feb 21, 2023

Background

It's a bit of a mouthful, but we ran into an issue with a very specific schema definition:

components:
  schemas:
    A:
      type: object
      required:
      - kind
      properties:
        kind:
          type: string
    Four:
      allOf:
      - $ref: "#/components/schemas/A"
      - type: object

The above schema describes two types, type A, which has a single property called kind which is a String and type Four, which is actually identical to A.

It's a unique situation, but when we attempt to use Four in a polymorphic schema that uses a discriminator, it's type is incorrectly inferred as demonstrated in 0d04732:

AnotherContainer:
type: object
required:
- content
properties:
content:
oneOf:
- $ref: "#/components/schemas/One"
- $ref: "#/components/schemas/Two"
- $ref: "#/components/schemas/Three"
- $ref: "#/components/schemas/Four"
discriminator:
propertyName: kind
mapping:
one: "#/components/schemas/One"
two: "#/components/schemas/Two"
three: "#/components/schemas/Three"
four: "#/components/schemas/Four"

public struct AnotherContainer: Codable {
public var content: Content
public enum Content: Codable {
case a(A)
case three(Three)
case string(String)
public init(from decoder: Decoder) throws {
struct Discriminator: Decodable {
let kind: String
}
let container = try decoder.singleValueContainer()
let discriminatorValue = try container.decode(Discriminator.self).kind
switch discriminatorValue {
case "one": self = .a(try container.decode(A.self))
case "two": self = .a(try container.decode(A.self))
case "three": self = .three(try container.decode(Three.self))
case "four": self = .string(try container.decode(String.self))
default:
throw DecodingError.dataCorruptedError(
in: container,
debugDescription: "Discriminator value '\(discriminatorValue)' does not match any expected values (one, two, three, four)."
)
}
}

Instead of the AnotherContainer type containing a four(Four) case, it ends up with string(String), which is absolutely not what we want.

We traced it back to this code:

// TODO: Improve this and adopt for other types (see Zoom spec)
if properties.count == 1 {
var property = properties[0]
if let nested = property.nested as? EntityDeclaration, nested.name.rawValue == "Object" {
nested.name = name
property.nested = nested
property.type = .userDefined(name: name)
}
return TypealiasDeclaration(name: name, type: property.type, nested: property.nested)
}

Changes

It's not entirely clear what the special Zoom code was trying to achieve, but when running CreateAPI against the Zoom API spec again, it turns out that the presence of this code makes no difference to the output. For that reason, I can only assume that it's now redundant and given that its presence causes a bug in other usage, we've decided to remove it.

If we do for some reason realise that we need this code back, it could always return as a vendor extension, but realistically I don't think that will happen. I'd also rather look into the exact motivation and figure out how to support that in other ways anyway.

After removing this code, I am able to generate the expected representation of AnotherContainer which is committed as part of this Pull Request.

Note: I noticed an unrelated bug when adding this snapshot configuration where AnyJSON has been generated when it shouldn't have been. It has been reported separately as #182

@liamnichols liamnichols self-assigned this Feb 21, 2023
@liamnichols liamnichols marked this pull request as ready for review February 21, 2023 15:47
@liamnichols liamnichols merged commit 0ba6538 into main Feb 22, 2023
@liamnichols liamnichols deleted the fix-bug-with-discriminator branch February 22, 2023 09:26
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.

None yet

1 participant