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 recursive types #70

Closed
scsinke opened this issue Jun 16, 2023 · 22 comments · Fixed by #330
Closed

Support recursive types #70

scsinke opened this issue Jun 16, 2023 · 22 comments · Fixed by #330
Assignees
Labels
area/generator Affects: plugin, CLI, config file. area/openapi Adding/updating a feature defined in OpenAPI. kind/enhacement Improvements to existing feature. size/M Medium task. (A couple of days of work.)
Milestone

Comments

@scsinke
Copy link

scsinke commented Jun 16, 2023

Currently the package does not support recursive types. Should this be possible? Maybe I'm doing something wrong. otherwise, I would propose to add this constraint in the docs. I have this use case due to Server Driven UI. This results in components or actions that can be embedded in each other.

Errors:

  • Value type 'x' cannot have a stored property that recursively contains it
  • Value type 'x' has infinite size
@czechboy0 czechboy0 added area/generator Affects: plugin, CLI, config file. area/openapi Adding/updating a feature defined in OpenAPI. kind/enhacement Improvements to existing feature. labels Jun 16, 2023
@czechboy0
Copy link
Collaborator

Thanks for filing the issue, we'd certainly accept a contribution of support of recursive types.

Could you please add a snippet of OpenAPI that uses it in YAML, and how you imagine the generated code would change? That'll help us discuss the desired solution.

@knellr
Copy link

knellr commented Jun 16, 2023

We are looking at migrating from our own hand-rolled code generation to this library, and for some of our APIs we are also encountering this issue.

Here's an example schema with recursion (involving only one model):

Recursion:
  properties:
    recursion:
      "$ref": "#/components/schemas/Recursion"
  type: object

In our existing code generator we break out of the dependency cycles by wrapping the property in a Box:

    public final class Box<Wrapped> {
        public var value: Wrapped
        public init(_ value: Wrapped) {
            self.value = value
        }
    }

    extension Box: Encodable where Wrapped: Encodable {
        public func encode(to encoder: Encoder) throws {
            try value.encode(to: encoder)
        }
    }

    extension Box: Decodable where Wrapped: Decodable {
        public convenience init(from decoder: Decoder) throws {
            let value = try Wrapped(from: decoder)
            self.init(value)
        }
    }

    public struct Recursion: Codable, Hashable {
        public let recursion: Box<Recursion>?
    }

@scsinke
Copy link
Author

scsinke commented Jun 21, 2023

I don't know if this can solve the issue. But maybe we can make use of the indirect keyword? https://www.hackingwithswift.com/example-code/language/what-are-indirect-enums

@czechboy0
Copy link
Collaborator

Seems that indirect can only be used on enums, not structs.

Before we consider the explicit Box approach, I'd like to see if there's another recommended way in Swift to solve this.

@scsinke
Copy link
Author

scsinke commented Jun 21, 2023

Hereby a simplified spec we use. For the full openAPI spec we use. check this link.

{
  "openapi": "3.0.2",
  "components": {
    "schemas": {
      "Action": {
        "anyOf": [
          {
            "$ref": "#/components/schemas/PromptAction"
          }
        ]
      },
      "PromptAction": {
        "type": "object",
        "properties": {
          "type": {
            "type": "string",
            "enum": [
              "PROMPT"
            ]
          },
          "prompt": {
            "$ref": "#/components/schemas/Prompt"
          }
        },
        "required": [
          "type",
          "prompt"
        ]
      },
      "Prompt": {
        "anyOf": [
          {
            "$ref": "#/components/schemas/DialogPrompt"
          }
        ]
      },
      "DialogPrompt": {
        "type": "object",
        "properties": {
          "type": {
            "type": "string",
            "enum": [
              "DIALOG"
            ]
          },
          "title": {
            "type": "string"
          },
          "message": {
            "type": "string"
          },
          "confirmButtonTitle": {
            "type": "string"
          },
          "cancelButonTitle": {
            "type": "string"
          },
          "confirmAction": {
            "$ref": "#/components/schemas/Action"
          }
        },
        "required": [
          "type",
          "title",
          "message",
          "confirmButtonTitle",
          "cancelButonTitle",
          "confirmAction"
        ]
      }
    }
  }
}

@scsinke
Copy link
Author

scsinke commented Jun 21, 2023

I think there are three options as far as I can tell.

  • Make it a class. But this does make it a bit less swifty because we're not using structs
class Recursive {
    let value: Recursive
}
  • Make an indirect enum.
enum Recursive<Element> {
  indirect case value(Recursive<Element>)
}
  • Use the box as stated above. So it can be used inside a struct. This can be a bit simplified by maybe adding a property wrapper.

@czechboy0
Copy link
Collaborator

Yeah, that sounds about right. Ideally, we could constrain this as much as possible, to:

  • direct nesting
  • the property must be optional

And we could use a property wrapper.

We already have the concept of a TypeUsage that represents a type being wrapped in an optional, an array, etc. I guess we could think of an "indirection box" as just another type of wrapping here, make it Sendable, Codable, etc.

It's important we limit how many places in the generator have to make explicit decisions based on this.

Might be worth for someone to prototype this and see what else might get hit.

@scsinke
Copy link
Author

scsinke commented Jun 26, 2023

@czechboy0 If you can give me some pointers on how and where to start prototyping. Will try to play around with it.

@czechboy0
Copy link
Collaborator

Hi @scsinke, sure!

TypeUsage is defined here:

The need for boxing could be represented as another case in the type usage internal enum.

The need for it would be calculated for the property schema, probably somewhere around

, if the parent type and the property type are the same.

Then, we'd probably need to adjust the struct generation logic to include the extra boxing when requested:

Would be great if the box could be represented as a property wrapper somehow.

But that's just one idea, you'll see what looks good when you're prototyping it, feel free to tag me on a PR with a proof of concept, and we can discuss more.

czechboy0 added a commit that referenced this issue Jul 21, 2023
…#130)

Validate incoming OpenAPI docs using OpenAPIKit's built-in validation

### Motivation

When provided with an OpenAPI document that has recursion, the generator either crashes, or produces Swift code that doesn't compile. We should catch recursion earlier in the process, and emit a descriptive error.

### Modifications

This PR adds two validation steps:
- OpenAPIKit's `validate` method catches some structural issues in the OpenAPI doc.
- OpenAPIKit's `locallyDereferenced()` method is a great way to ensure no reference cycles appear in the document.

Catching reference cycles earlier in the process is great, as even if we generate the Swift code, it won't compile until we have some explicit support for recursive types (tracked by #70).

### Result

Now, when an OpenAPI document with recursion is provided, instead of crashing or producing non-compiling Swift code, it prints a descriptive error and returns a non-0 code.

### Test Plan

Tested manually on the CLI with purposefully malformed documents. But since this isn't our code, I don't want to add detailed tests of the validation details.


Reviewed by: simonjbeaumont

Builds:
     ✔︎ pull request validation (5.8) - Build finished. 
     ✔︎ pull request validation (5.9) - Build finished. 
     ✔︎ pull request validation (docc test) - Build finished. 
     ✔︎ pull request validation (integration test) - Build finished. 
     ✔︎ pull request validation (nightly) - Build finished. 
     ✔︎ pull request validation (soundness) - Build finished. 

#130
@czechboy0
Copy link
Collaborator

Another way to implement this would be:

  • The type that needs to be "boxed" (aka is part of a cycle) would have a slightly different implementation under the hood: it'd still be a struct publicly, but internally would hold a class and use CoW. So the adopter would not know whether it's a plain old struct, or this "boxed" struct, but it'd break the cycle and allow for both direct and transitive recursive types.
  • Deciding which type needs to be boxed is a separate algorithm, ideally we want to box as few types as needed to break all the cycles in a single document, but an algorithm that boxes all types (func needsBoxing(...) -> Bool { true }) would be an okay starting point, and try to make it more efficient from there.

If someone wants to pick this up, it could make sense to PR these bits independently, e.g. the algorithm first, then the changes to the generator.

@czechboy0 czechboy0 changed the title Recursive types not supported Support recursive types Jul 25, 2023
@czechboy0 czechboy0 added the size/M Medium task. (A couple of days of work.) label Aug 23, 2023
@czechboy0 czechboy0 modified the milestones: Post-1.0, 1.0 Aug 25, 2023
@czechboy0
Copy link
Collaborator

This also blocks generating the App Store Connect OpenAPI document: https://developer.apple.com/sample-code/app-store-connect/app-store-connect-openapi-specification.zip

The schema DiagnosticLogCallStackNode is recursive there.

@herrernst
Copy link

We have lots of cycles in the API we want to use, so fixing this would be crucial for us. Thanks in advance.

@czechboy0
Copy link
Collaborator

Thanks @herrernst for letting us know. Without going into any confidential specifics, what's your use case of recursive schemas? We've seen a representation of a file system hierarchy, which is inherently recursive, and in general graph representations. What's yours?

@yanniks
Copy link
Contributor

yanniks commented Oct 7, 2023

Also very interested in this. We also have the use-case of server-driven UI.

@herrernst
Copy link

Thanks @herrernst for letting us know. Without going into any confidential specifics, what's your use case of recursive schemas? We've seen a representation of a file system hierarchy, which is inherently recursive, and in general graph representations. What's yours?

Just basic things like a Person having a partner that is also a Person or having friends that are Persons.

@czechboy0 czechboy0 self-assigned this Oct 10, 2023
@MattNewberry
Copy link

MattNewberry commented Oct 10, 2023

In our use case, we have a data type which represents a polymorphic wrapper which encapsulates nested data types of the same wrapper. In short, we have Content which contains an array of Content.

For example:

[
    {
        "type": "box",
        "value": [ // Polymorphic envelope
            {
                "type": "text",
                "value": [ // Polymorphic envelope
                    {
                        "text": "hello world"
                    }
                ]
            }
        ]
    }
]

(We're also anxiously awaiting recursive types to be supported)

@czechboy0
Copy link
Collaborator

Thanks for the info, everyone, helps as I'm working on the design for this.

czechboy0 added a commit to apple/swift-openapi-runtime that referenced this issue Oct 19, 2023
[Runtime] Support recursive types

### Motivation

Runtime changes for apple/swift-openapi-generator#70.

### Modifications

Introduce a new wrapper type `CopyOnWriteBox` as a reference type with copy-on-write semantics, to be used by the generated code to hide a ref type inside a struct that needs boxing. (Details on the logic will be in the generator PR description.)

### Result

A new type available for the generated code to use for breaking reference cycles.

### Test Plan

Added unit tests for the type.


Reviewed by: dnadoba

Builds:
     ✔︎ pull request validation (5.10) - Build finished. 
     ✔︎ pull request validation (5.8) - Build finished. 
     ✔︎ pull request validation (5.9) - Build finished. 
     ✔︎ pull request validation (api breakage) - Build finished. 
     ✔︎ pull request validation (docc test) - Build finished. 
     ✔︎ pull request validation (integration test) - Build finished. 
     ✔︎ pull request validation (nightly) - Build finished. 
     ✔︎ pull request validation (soundness) - Build finished. 

#58
czechboy0 added a commit that referenced this issue Oct 19, 2023
Support recursive types

### Motivation

Fixes #70.

### Modifications

To start, read the new docc [article](https://github.com/apple/swift-openapi-generator/blob/ceb51fa1a4f1f858590b22da75162c4bf999719b/Sources/swift-openapi-generator/Documentation.docc/Development/Supporting-recursive-types.md) about how reference types are implemented, then most of the PR should make sense.

As suggested by @simonjbeaumont, boxing of recursive types happens on the Swift representation, as opposed to my original approach, which tried to do this early in the translation layer. This massively simplified the problem and definitely seems like the better way to do it.

Highlights:
- In `validateDoc`, removed the dereferencing, which we previously used to catch cycles early and emit descriptive errors.
- Introduced an efficient stack type caller `ReferenceStack` that makes checking if an item is present in the stack fast, on top of being a stack (represented as an array).
- Helper methods like `isSchemaSupported` and `isKeyValuePair` gained an inout parameter of the stack, to allow it to break infinite recursion.
- The actual algorithm for walking the graph, detecting cycles, and deciding which types to box is implemented in `RecursionDetector`, which is a generic algorithm on top of nodes with edges.
- Then `DeclarationRecursionDetector` provides concrete types that glue it with our structured Swift representation's `Declaration`.
- The algorithm runs in `translateSchemas` where we're iterating over the items in `#/components/schemas`, as those are the only ones that can produce a cycle (as schemas in other parts of the document can refer to items in `#/components/schemas`, but not the other way around: items in `#/components/schemas` cannot refer to schemas outside of it.)

### Result

OpenAPI documents with recursive schemas are now supported.

### Test Plan

- Added unit tests for the recursion detector.
- Adapted other tests, of `isSchemaSupported` and `isKeyValuePair`.
- Added examples to `petstore.yaml`, as this one introduces quite a lot of new code that we want to make sure compiles without warnings.
- Also added examples to snippet tests, to allow us to expand those later with edge cases we haven't thought about yet.



Reviewed by: dnadoba

Builds:
     ✔︎ pull request validation (5.10) - Build finished. 
     ✔︎ pull request validation (5.8) - Build finished. 
     ✔︎ pull request validation (5.9) - Build finished. 
     ✔︎ pull request validation (compatibility test) - Build finished. 
     ✔︎ pull request validation (docc test) - Build finished. 
     ✔︎ pull request validation (integration test) - Build finished. 
     ✔︎ pull request validation (nightly) - Build finished. 
     ✔︎ pull request validation (soundness) - Build finished. 

#330
@czechboy0
Copy link
Collaborator

Landed in main, will get released in 0.3.1.

@czechboy0
Copy link
Collaborator

@herrernst
Copy link

Thank you, I can confirm it's working for my schema!

@czechboy0
Copy link
Collaborator

Glad to hear that. Just FYI for anyone else following up, I did find a scenario under which the cycle detector doesn't work correctly in a complex graph and am investigating it. If you hit an issue where the generated code doesn't compile, complaining about "infinite size" structs, please let me know here.

@czechboy0
Copy link
Collaborator

Here's the fix: #335

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/generator Affects: plugin, CLI, config file. area/openapi Adding/updating a feature defined in OpenAPI. kind/enhacement Improvements to existing feature. size/M Medium task. (A couple of days of work.)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants