Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Comments

Close side panel

Fix allOf decoding issue#27

Merged
liamnichols merged 8 commits intoCreateAPI:mainCreateAPI/CreateAPI:mainfrom
imjn:imjn/fix-allOf-decoding-issueimjn/CreateAPI:imjn/fix-allOf-decoding-issueCopy head branch name to clipboard
Jun 10, 2022
Merged

Fix allOf decoding issue#27
liamnichols merged 8 commits intoCreateAPI:mainCreateAPI/CreateAPI:mainfrom
imjn:imjn/fix-allOf-decoding-issueimjn/CreateAPI:imjn/fix-allOf-decoding-issueCopy head branch name to clipboard

Conversation

@imjn
Copy link
Member

@imjn imjn commented Mar 10, 2022 •
edited
Loading

WHAT

When we have this kind OpenAPI schema ↓

"Dog": {
  "allOf": [
    {
      "type": "object",
      "additionalProperties": false,
      "required": ["type"],
      "properties": {
        "type": {
          "type": "string",
          "enum": ["dog"]
        }
      }
    },
    {
      "$ref": "#/components/schemas/BaseDog"
    }
  ]
}

it fails to decode type because currently, CreateAPI generates ↓

public init(from decoder: Decoder) throws {
    self.type = try `Type`(from: decoder) # ← THIS IS WRONG
    self.baseDog = try BaseDog(from: decoder)
}

It should be

public init(from decoder: Decoder) throws {
    let values = try decoder.container(keyedBy: StringCodingKey.self)
    self.type = try values.decode(`Type`.self, forKey: "type") # ← THIS IS CORRECT
    self.baseDog = try BaseDog(from: decoder)
}

In this PR, I've added isInlineed to Property in order to differentiate Type and BaseDog generated codes in the example above.

Also, when an object in allOf has a reference type property, it fails because currently it is generated as

# spec
Dog:
    allOf:
      - $ref: "#/components/schemas/Animal"
      - type: object
        properties:
          breed:
            type: string
          image:
            $ref: "#/components/schemas/Image"
public struct Dog: Codable {
    public var animal: Animal
    public var breed: String?
    public var image: Image?

    public init(animal: Animal, breed: String? = nil, image: Image? = nil) {
        self.animal = animal
        self.breed = breed
        self.image = image
    }

    public init(from decoder: Decoder) throws {
        let values = try decoder.container(keyedBy: StringCodingKey.self)
        self.animal = try Animal(from: decoder)
        self.breed = try values.decodeIfPresent(String.self, forKey: "breed")
        self.image = try Image(from: decoder) # ← THIS IS WRONG
    }

    public func encode(to encoder: Encoder) throws {
        var values = encoder.container(keyedBy: StringCodingKey.self)
        try values.encode(animal, forKey: "animal")
        try values.encodeIfPresent(breed, forKey: "breed")
        try values.encodeIfPresent(image, forKey: "image")
    }
}

but should be

public struct Dog: Codable {
    public var animal: Animal
    public var breed: String?
    public var image: Image?

    public init(animal: Animal, breed: String? = nil, image: Image? = nil) {
        self.animal = animal
        self.breed = breed
        self.image = image
    }

    public init(from decoder: Decoder) throws {
        let values = try decoder.container(keyedBy: StringCodingKey.self)
        self.animal = try Animal(from: decoder)
        self.breed = try values.decodeIfPresent(String.self, forKey: "breed")
        self.image = try values.decodeIfPresent(Image.self, forKey: "image") # ← THIS IS CORRECT
    }

    public func encode(to encoder: Encoder) throws {
        var values = encoder.container(keyedBy: StringCodingKey.self)
        try values.encode(animal, forKey: "animal")
        try values.encodeIfPresent(breed, forKey: "breed")
        try values.encodeIfPresent(image, forKey: "image")
    }
}

HOW

  • Added some tests
  • Added isInlined to Property.

@imjn imjn marked this pull request as ready for review March 10, 2022 16:31
@imjn imjn marked this pull request as draft March 14, 2022 15:30
@imjn
Copy link
Member Author

imjn commented Mar 14, 2022

I found more to fix so marked this PR as draft now 🙏🏽

@kean
Copy link
Member

kean commented Mar 14, 2022

Thanks for working on this. I merged the other two MRs and also fixed the remaining tests in the main branch.

@imjn imjn force-pushed the imjn/fix-allOf-decoding-issue branch 10 times, most recently from 9428021 to 5f532ad Compare March 28, 2022 11:10
imjn
imjn commented Mar 28, 2022
View reviewed changes
Tests/CreateAPITests/Specs/edgecases.yaml
properties:
breed:
type: string
image:
Copy link
Member Author

@imjn imjn Mar 28, 2022

Choose a reason for hiding this comment

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

This is the test case I wanted to pass by this PR's changes.

@imjn imjn marked this pull request as ready for review March 28, 2022 14:22
liamnichols
liamnichols reviewed Apr 11, 2022
View reviewed changes
Copy link
Member

@liamnichols liamnichols left a comment

Choose a reason for hiding this comment

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

This looks great @imjn 💪

Did you also want to replicate the first issue that you described in the PR description within the specs/tests? Or do you think just adding Image to Animal is sufficient like you have done already? I'm just trying to understand if the current tests are covering all of the behavioural changes 🙏

Tests/CreateAPITests/Specs/edgecases.yaml Outdated Show resolved Hide resolved
Sources/CreateAPI/Generator/Generator+Render.swift Outdated Show resolved Hide resolved
Sources/CreateAPI/Generator/Declarations.swift Show resolved Hide resolved
@imjn imjn force-pushed the imjn/fix-allOf-decoding-issue branch from ea379ef to 6ebb9f5 Compare May 11, 2022 11:16
@imjn
Copy link
Member Author

imjn commented May 11, 2022

Added test coverage for the first issue that I described in the description 6ebb9f5.
This is for Enum decoding issue in allOf.

@imjn imjn requested a review from liamnichols May 11, 2022 11:23
@imjn imjn self-assigned this May 11, 2022
@imjn imjn force-pushed the imjn/fix-allOf-decoding-issue branch from 6ebb9f5 to b7d9bfb Compare June 9, 2022 13:40
liamnichols
liamnichols reviewed Jun 9, 2022
View reviewed changes
Copy link
Member

@liamnichols liamnichols left a comment

Choose a reason for hiding this comment

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

Looking great @imjn! Thanks again for solving this 💪 Just one tweak to the schema and then I think its good to go 🚀

Tests/CreateAPITests/Expected/edgecases-yaml-config/Sources/Entities.swift Outdated Show resolved Hide resolved
Tests/CreateAPITests/Specs/edgecases.yaml Outdated Show resolved Hide resolved
@imjn imjn force-pushed the imjn/fix-allOf-decoding-issue branch from eba3b81 to e1cd392 Compare June 10, 2022 10:03
liamnichols
liamnichols approved these changes Jun 10, 2022
View reviewed changes
Copy link
Member

@liamnichols liamnichols left a comment

Choose a reason for hiding this comment

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

Great! Lets merge this 🚀

@liamnichols liamnichols merged commit 1df9afe into CreateAPI:main Jun 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@liamnichols liamnichols liamnichols approved these changes

Assignees

@imjn imjn

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@imjn @kean @liamnichols
Morty Proxy This is a proxified and sanitized view of the page, visit original site.