apple / swift-openapi-generator

Generate Swift client and server code from an OpenAPI document.
https://swiftpackageindex.com/apple/swift-openapi-generator/documentation
Apache License 2.0
1.45k stars 121 forks source link

How to define custom type name for inline defined type? #617

Closed teameh closed 2 months ago

teameh commented 2 months ago

Question

I'm trying to switch from OpenAPITools/openapi-generator to apple/swift-openapi-generator and I can't get my inline schema objects to be named how I would want them.

When using this custom scheme:

MyObject: 
  type: object
  properties:
    id:
      type: string
    foo: 
      type: object
      title: Foo           <-- custom title property
      properties:
        bar:
          type: string
        baz:
          type: string

I would expect Components.Schemes.MyObject.Foo to be generated. But instead Components.Schemas.MyObject.fooPayload is generated.

Is there a way to customise the type name for this object?

simonjbeaumont commented 2 months ago

Thanks for taking the time to file this issue.

The naming here is unfortunate but I thought this was the best we could do given our principle of not generating code that doesn't compile, when presented with a valid OpenAPI document.

There are a number of places where this occurs. For example, a valid schema can have two properties with names that differ only in their casing. This is why we do not adjust the casing to match any Swift idioms: because it can produce a conflict in the generated code when there was no real issue in the OpenAPI document. It's also why we have numbered enum cases.

I think this is another case where the OpenAPI spec does not help us here.

For example, this is valid...

# This is a valid openapi.yaml šŸ˜Ž
components:
  schemas:
    MyObject: 
      type: object
      properties:
        id:
          type: string
        foo: 
          type: string
          title: Foo  # <-- custom title property

...but so is this...

# This is also a valid openapi.yaml šŸ˜³
components:
  schemas:
    MyObject: 
      type: object
      properties:
        id:
          type: string
          title: Foo  # <-- not unique among properties
        foo: 
          type: string
          title: Foo  # <-- not unique among properties

You can see that both of these validate fine here: https://apitools.dev/swagger-parser/online/.

This puts us in an awkward situation. There are lots of heuristics one could apply here. For example we could use title when it is unique among peers, and add disambiguation when necessary, but this is a complex API evolution problem. Because we may change the code we generate for a property, just because a peer is added in the OpenAPI document and that would be backwards incompatible.

The relevant section of the OpenAPI specification states:

4.8.24.1 Properties

The OpenAPI Schema Object dialect is defined as requiring the OAS base vocabulary, in addition to the vocabularies as specified in the JSON Schema draft 2020-12 general purpose meta-schema.

The OpenAPI Schema Object dialect for this version of the specification is identified by the URI https://spec.openapis.org/oas/3.1/dialect/base (the ā€œOAS dialect schema idā€).

The following properties are taken from the JSON Schema specification but their definitions have been extended by the OAS:

description - [CommonMark] syntax MAY be used for rich text representation. format - See Data Type Formats for further details. While relying on JSON Schemaā€™s defined formats, the OAS offers a few additional predefined formats. In addition to the JSON Schema properties comprising the OAS dialect, the Schema Object supports keywords from any other vocabularies, or entirely arbitrary properties.

ā€” source: https://spec.openapis.org/oas/latest.html#schema-object

We've considered using description too, but this is both (1) not required, and (2) does not need to be unique.

The OAS defers to JSON Schema Specification Draft 2020-12, which does make use of title: in some places and, from my read, these appear to be annotations, which, too, don't appear to have any uniqueness constraints.

I hope that clears up the why things are the way they are.

To whether we could do better, I do wonder if there's space for an opt-in mode where we do use title:.

We'd need to understand how widespread its use is and the conventions for how it's used.

teameh commented 2 months ago

Thanks for your elaborate answer!

...but so is this...

Haha horrible šŸ˜…

This is why we do not adjust the casing to match any Swift idioms: because it can produce a conflict in the generated code when there was no real issue in the OpenAPI document.

Check. I already stumbled upon that when trying to generate .rectangle(Rectangle) but got .Rectangle(Rectangle) from:

Shape:
  oneOf:
    - $ref: '#/components/schemas/Rectangle'
    - $ref: '#/components/schemas/Circle'  
  discriminator:
    propertyName: type
    mapping:
      rectangle: '#/components/schemas/Rectangle'
      circle: '#/components/schemas/Circle'

The naming here is unfortunate but I thought this was the best we could do given our principle of not generating code that doesn't compile, when presented with a valid OpenAPI document.

That is indeed important. But it also means you'll always get less nice code compared to when you would assume a bit more strict schema without weird quirks..

To whether we could do better, I do wonder if there's space for an opt-in mode where we do use title:.

Yeah, I think in general, the usability of this generator would benefit a from from more options to tweak the generated output. When trying to move over from the swift5 generator from OpenApiTools I noticed that there are a lot less options here compared to that generator. On top of the options, users can tweak the templates of the other generator themselves, giving them even more freedom.

That does not necessarily have to be a problem, a strict set op options and an opinionated output can be good, but I can imagine that there are lot of folks who would like to try out this generator, but can't get it to behave how they would like to. And that's a shame, because the protocol based output, and the server stubs kick ass!

Perhaps some additional options would indeed improve this!

simonjbeaumont commented 2 months ago

The naming here is unfortunate but I thought this was the best we could do given our principle of not generating code that doesn't compile, when presented with a valid OpenAPI document.

That is indeed important. But it also means you'll always get less nice code compared to when you would assume a bit more strict schema without weird quirks..

You're completely correct and this is the line we have to tread. IMO the generator should produce compiling code wherever it can. Even when presented with a quirky, but correct OpenAPI document. The motivation for this is that consumers of the OpenAPI document are often not the authorsā€”for every service, there are many clientsā€”and we want to be able to cope with the (sometimes frustrating) expressivity of an OpenAPI document.

To whether we could do better, I do wonder if there's space for an opt-in mode where we do use title:.

Yeah, I think in general, the usability of this generator would benefit a from more options to tweak the generated output. When trying to move over from ... there are a lot less options here compared to that generator. On top of the options, users can tweak the templates of the other generator themselves, giving them even more freedom.

You're also right that this generator is less customisable than others. The fact that it is not template-based, we consider to be a strength, since we're building a syntax tree, which allows us to be more precise in what we generate and be more confident that it will compile.

That does not necessarily have to be a problem, a strict set op options and an opinionated output can be good, but I can imagine that there are lot of folks who would like to try out this generator, but can't get it to behave how they would like to. And that's a shame, because the protocol based output, and the server stubs kick ass!

Perhaps some additional options would indeed improve this!

It's great that you find the protocol-based output compelling; we've tried to create a Swift-first implementation here, but within the (lack of) constraints of the OpenAPI document.

I'm sympathetic to the desire for more options, but every option we have adds complexity to the generator and multiplies the test space.

We've usually tried to position this tool as generating a lower-level implementation of the documented APIā€”one that is is as faithful as possible to the OpenAPI documentā€”and encourage users to wrap it where they have opinions and idioms they would like to follow.

That being said, we're conscious to understand adopters' preferences, especially if they are adoption blockers. We've typically tried to prioritise missing OpenAPI features over subjective discussions about the generated code or its format and, because we do not recommend vending the generated code as an API to any downstream project without curation, we hadn't seen the names of generated properties to be big deal.

We could consider an option which continues to generate the properties as named today, but also generates a computed property for every property that has a title:. Although, we'd probably want to do a bit more research to understand the implications of such a feature.

czechboy0 commented 2 months ago

Another option is to move the schema out into a named schema under #/components/schemas, and then you can give it any name you want, and the generator will respect it.

simonjbeaumont commented 2 months ago

@czechboy0 is correct. If you check out the hello-world example in the repo, you'll see this is done for the Greeting type.

teameh commented 2 months ago

Thanks. Yeah that all makes sense and I definitely consider being not template based a strength as well!

We've usually tried to position this tool as generating a lower-level implementation of the documented APIā€”one that is is as faithful as possible to the OpenAPI documentā€”and encourage users to wrap it where they have opinions and idioms they would like to follow.

Yeah you both iterated on that in https://github.com/apple/swift-openapi-generator/issues/375#issuecomment-1809919354 as well and I get it.

But I can also imagine that a lot of users would just like to use the generated code directly without wrapping it. It's just so convenient to not having to write all these boilerplate objects If they are already generated. And if you have a lot of objects, but the rest of your project is sufficiently small or there are other reasons why you want to use these objects directly then having to rename or change some properties when the schema or the implementation of the generator changes might be the lesser evil compared to writing out the boilerplate that comes with wrapping them. (Yeah, I did read the "API stability of generated code" docs).

But I guess you're right and these users should just create named schemas in this case. I do feel that doing this decreases readability of the spec a bit, but I guess you can't have it all ĀÆ\_(惄)_/ĀÆ.

simonjbeaumont commented 2 months ago

users should just create named schemas in this case. I do feel that doing this decreases readability of the spec a bit,

Interesting take. I realise that it's subjective, but I find specs structured like this both (1) more readable and (2) more usefulā€”because you can reuse the types in multiple places.

I hadn't considered using the title: annotation. Needs a bit more thought but, depending on its use, there might be a case for using it, like I said before. Do you happen to have some example OpenAPI documents that are public you could point to that make use of it?

teameh commented 2 months ago

For me it depends on the complexity of the scheme and indeed wether or not the scheme reused. But I can also imagine users inlining a response when it's sufficiently simple, to prevent having to scroll down to check out the response when reading the spec?

paths:
  /logos:
    get:
      operationId: getLogos
      responses:
        '200':
          description: get all available logos
          content:
            application/json:
              schema:
                title: LogosResponse            <- title
                type: object
                required: [logos, modifiedAt]
                properties:
                  logos:
                    type: array
                    items:
                      $ref: '#/components/schemas/Logo'
                  modifiedAt:
                    type: string
                    format: date-time
                    description: The timestamp of the last modification to the logos

It depends on the complexity of the scheme. Perhaps you would rather inline the image urls here to prevent having to scroll too look them up when reading the scheme?

Logo:
  type: object
  required: [id, name, urls, tags]
  properties:
    id:
      type: string
      description: Unique identifier for the logo
    name:
      type: string
      description: Name of the logo
    images:
      type: object
      title: LogoImages                                     <-- title
      description: Images of this logo
      required:
        - large
        - medium
        - small
      properties:
        large:
          type: string
          format: uri
        medium:
          type: string
          format: uri
        small:
          type: string
          format: uri
    tags:
      type: array
      items:
        type: string
      description: Tags associated with the logo
  examples:
    - id: "nba_black"
      name: "NBA"
      urls:
        large: "http://localhost/logos/nba_123891723_large.png"
        medium: "http://localhost/logos/nba_123891723_medium.png"
        small: "http://localhost/logos/nba_123891723_small.png"
      tags: ["Red", "2024"]

Instead of defining them separately if they are only used once?

Logo:
  type: object
  required: [id, name, urls, tags]
  properties:
    id:
      type: string
      description: Unique identifier for the logo
    name:
      type: string
      description: Name of the logo
    images:
      $ref: '#/components/schemas/LogoImages'
    tags:
      type: array
      items:
        type: string
      description: Tags associated with the logo
  examples:
    - id: "nba_black"
      name: "NBA"
      urls:
        large: "http://localhost/logos/nba_123891723_large.png"
        medium: "http://localhost/logos/nba_123891723_medium.png"
        small: "http://localhost/logos/nba_123891723_small.png"
      tags: ["Red", "2024"]

LogoImages:
  type: object
  description: Images of this logo
  required:
    - large
    - medium
    - small
  properties:
    large:
      type: string
      format: uri
    medium:
      type: string
      format: uri
    small:
      type: string
      format: uri

But I agree, this is completely subjective šŸ˜Š.

czechboy0 commented 2 months ago

As discussed earlier, the major difference here is that the keys in #/components/schemas must be unique. But title: doesn't have to be.

That's why only the former can be reliably used to derive a unique name, but the latter can't.

And as Si mentioned, conditionally using a title for a name, depending on whether it's globally unique, produces "spooky-action-at-a-distance" effects that we've really tried to avoid. Changing one schema simply shouldn't lead to an unrelated schema changing its name in the generated code (but that'd happen if we ever pick generated names based on uniqueness, such as by using title).

While I empathize and agree that it's not ideal, we generally recommend that folks either live with the generated names, even when not pretty, or that they wrap the generated code in a hand-written API, or break out subschemas into reusable schemas and give them the desired names.

teameh commented 2 months ago

I think that summarises it nicely, and it makes sense. I'll close this ticket.

Thanks, both, for taking the time to discuss this at length here. I hope this thread serves anyone with a similar question in the future šŸ˜Š.

simonjbeaumont commented 2 months ago

Youā€™re welcome. Thanks for your input. And should a future version OpenAPI specification afford us something better, weā€™ll be sure to make use of it.