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

Don't generate inline schemas when allOf only contains a single schema #278

Merged
merged 5 commits into from
Aug 5, 2021

Conversation

liamnichols
Copy link
Contributor

This relates to the patch applied in #269, which in turn relates to #267. cc @nicholascross, @ymhuang0808. Firstly please excuse me if i missed some important historical context but I'm just trying to solve a slightly different regression that I noticed in some of my own examples.

In #269, we reintroduced inline generation for allOf types however in turn this introduced this diff: https://github.com/yonaskolb/SwagGen/pull/269/files#diff-6582636d47b022b95a7a5dfd79ff1e767f5f862e5eba00c00eb46208c5e7aab1

I believe that this is separate to the original issue however since Zoo.AllOfDog and Zoo.InlineAnimal go unused and this is related to the issue that I am seeing with my schema.

If we take the following:

UserSubclass:
  allOf:
    - $ref: "#/components/schemas/User"
    - type: object
      properties:
        age:
          type: integer
        last_error:
          allOf:
            - $ref: "#/components/schemas/Error"
          nullable: true
          description: last error reported to user object, or null if they have not seen an error.

The intention of the last_error property is to reference an existing schema (Error) but to override its nullability and/or description when used within UserSubclass since its likely that these two properties on the schema (and possibly more) will vary. My understanding is that this approach is commonly used and i've seen other generators working with it.

In fact, SwagGen was already properly resolving the property types to Animal and Dog as expected (hence why the inline types went unused) thanks to this:

case let .group(groupSchema):
if groupSchema.schemas.count == 1, let singleGroupSchema = groupSchema.schemas.first {
//flatten group schemas with only one schema
return getSchemaType(name: name, schema: singleGroupSchema)
}
return escapeType(name.upperCamelCased())

As a result, my change ensures that inlineSchema is only returned if group.schemas.count > 1 and this means that the unused LastError inline type would be omitted again:

Before
diff --git a/Specs/TestSpec/generated/Swift/Sources/Models/UserSubclass.swift b/Specs/TestSpec/generated/Swift/Sources/Models/UserSubclass.swift
index d11ebc7..a6414ef 100644
--- a/Specs/TestSpec/generated/Swift/Sources/Models/UserSubclass.swift
+++ b/Specs/TestSpec/generated/Swift/Sources/Models/UserSubclass.swift
@@ -9,8 +9,33 @@ public class UserSubclass: User {
 
     public var age: Int?
 
-    public init(id: Int? = nil, name: String? = nil, age: Int? = nil) {
+    /** last error reported to user object, or null if they have not seen an error. */
+    public var lastError: ErrorType?
+
+    /** last error reported to user object, or null if they have not seen an error. */
+    public class LastError: ErrorType {
+
+        public override init(code: Int, message: String) {
+            super.init(code: code, message: message)
+        }
+
+        public required init(from decoder: Decoder) throws {
+            try super.init(from: decoder)
+        }
+
+        public override func encode(to encoder: Encoder) throws {
+            try super.encode(to: encoder)
+        }
+
+        override public func isEqual(to object: Any?) -> Bool {
+          guard object is LastError else { return false }
+          return super.isEqual(to: object)
+        }
+    }
+
+    public init(id: Int? = nil, name: String? = nil, age: Int? = nil, lastError: ErrorType? = nil) {
         self.age = age
+        self.lastError = lastError
         super.init(id: id, name: name)
     }
 
@@ -18,6 +43,7 @@ public class UserSubclass: User {
         let container = try decoder.container(keyedBy: StringCodingKey.self)
 
         age = try container.decodeIfPresent("age")
+        lastError = try container.decodeIfPresent("last_error")
         try super.init(from: decoder)
     }
 
@@ -25,12 +51,14 @@ public class UserSubclass: User {
         var container = encoder.container(keyedBy: StringCodingKey.self)
 
         try container.encodeIfPresent(age, forKey: "age")
+        try container.encodeIfPresent(lastError, forKey: "last_error")
         try super.encode(to: encoder)
     }
 
     override public func isEqual(to object: Any?) -> Bool {
       guard let object = object as? UserSubclass else { return false }
       guard self.age == object.age else { return false }
+      guard self.lastError == object.lastError else { return false }
       return super.isEqual(to: object)
     }
 }
After
diff --git a/Specs/TestSpec/generated/Swift/Sources/Models/UserSubclass.swift b/Specs/TestSpec/generated/Swift/Sources/Models/UserSubclass.swift
index d11ebc7..d34e692 100644
--- a/Specs/TestSpec/generated/Swift/Sources/Models/UserSubclass.swift
+++ b/Specs/TestSpec/generated/Swift/Sources/Models/UserSubclass.swift
@@ -9,8 +9,12 @@ public class UserSubclass: User {
 
     public var age: Int?
 
-    public init(id: Int? = nil, name: String? = nil, age: Int? = nil) {
+    /** last error reported to user object, or null if they have not seen an error. */
+    public var lastError: ErrorType?
+
+    public init(id: Int? = nil, name: String? = nil, age: Int? = nil, lastError: ErrorType? = nil) {
         self.age = age
+        self.lastError = lastError
         super.init(id: id, name: name)
     }
 
@@ -18,6 +22,7 @@ public class UserSubclass: User {
         let container = try decoder.container(keyedBy: StringCodingKey.self)
 
         age = try container.decodeIfPresent("age")
+        lastError = try container.decodeIfPresent("last_error")
         try super.init(from: decoder)
     }
 
@@ -25,12 +30,14 @@ public class UserSubclass: User {
         var container = encoder.container(keyedBy: StringCodingKey.self)
 
         try container.encodeIfPresent(age, forKey: "age")
+        try container.encodeIfPresent(lastError, forKey: "last_error")
         try super.encode(to: encoder)
     }
 
     override public func isEqual(to object: Any?) -> Bool {
       guard let object = object as? UserSubclass else { return false }
       guard self.age == object.age else { return false }
+      guard self.lastError == object.lastError else { return false }
       return super.isEqual(to: object)
     }
 }

As for the original issue reported in #267, I updated TestSpec in c10a5e6 to include the expected results described by @ymhuang0808 (at least I think I did 😅) so hopefully the updated fixtures describe fixes to both issues... Please confirm though!

Thanks 🙇

Copy link
Owner

@yonaskolb yonaskolb left a comment

Choose a reason for hiding this comment

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

Looks good to me, but would love to hear confirmation from @nicholascross and @ymhuang0808 if it still solves their issues

@yonaskolb yonaskolb merged commit 1a01b5f into yonaskolb:master Aug 5, 2021
@liamnichols liamnichols deleted the ln/inline-schema-bug branch August 5, 2021 08:23
kerrmarin-lvmh pushed a commit to kerrmarin-lvmh/SwagGen that referenced this pull request Sep 28, 2021
…ema (yonaskolb#278)

* Update TestSpec to include a scenario where allOf is used to change a referenced schemas nullability or description

* Update inlineSchema extension on Schema to ignore allOf groups where there is only a single child schema

* Update generated fixtures to match expected results after patch

* Update CHANGELOG.md

* Update TestSpec to include example demonstrated in yonaskolb#267
rusik pushed a commit to agorra/SwagGen that referenced this pull request Oct 14, 2021
…ema (yonaskolb#278)

* Update TestSpec to include a scenario where allOf is used to change a referenced schemas nullability or description

* Update inlineSchema extension on Schema to ignore allOf groups where there is only a single child schema

* Update generated fixtures to match expected results after patch

* Update CHANGELOG.md

* Update TestSpec to include example demonstrated in yonaskolb#267
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.

2 participants