Allow substituting types

[simonbility]

Motivation

Picking up after some time from this issue #375

There are some usecases described here that i think could be addressed by this.

I suspect there are some "bigger picture" decisions (maybe proposals) needing to happen so i wanted to get the ball rolling :)

Modifications

I made a small change allowing to "swap in" any type instead of generating one, by using a vendor-extension (x-swift-open-api-substitute-type)

Result

The following spec

openapi: 3.1.0
info:
  title: api
  version: 1.0.0
components:
  schemas:
    MyCustomString:
      type: string
      x-swift-open-api-substitute-type: MyLibrary.MyCustomString

would generate code like this (abbreviated)

public enum Components {
    public enum Schemas {
        /// - Remark: Generated from `#/components/schemas/MyCustomString`.
        public typealias MyCustomString = MyLibrary.MyCustomString
    }
}

Test Plan

I did write a test but suspect theres, other parts affected that i missed

[simonjbeaumont]

Sorry this went unacknowledged until now, and thank you for taking the time to think about this.

I suspect there are some "bigger picture" decisions (maybe proposals) needing to happen so i wanted to get the ball rolling :)

I'm lightly in favour of this approach since, until we have a holistic solution, it provides an escape-hatch.

We've also previously mentioned that extensions might be a suitable solution to some of these problems: #375 (comment).

It definitely opens up a bunch of questions and caveats. I see this was motivated by the discussion on providing more strongly typed scalar types and agree this is tempting for things like using a UUID type, instead of a string. My biggest concern would be how this would be used with complex schemas and the burden this shifts to adopters for providing a type that satisfies the schema, especially when that schema is itself referenced in other schemas.

Relatedly there was some work in #627 to move targeted types over which is something I'm supportive of but was lacking an API evolution story. Potentially the config to opt-in might be good enough.

Do you see this feature only being valuable if it can provide substitution for richer types or is there value exploring something more targeted?

@czechboy0 WDYT to this too?

[simonbility]

Sorry this went unacknowledged until now, and thank you for taking the time to think about this.

No worries at all! i appreciate the response.

I also would classify it as a "escape-hatch" solution and it is inherently "unsafe" as you are relying on manually ensuring that it adheres to the schema...

and i would happily take that tradeoff (obviously 😅)

In our project, where we would like to migrate to generating our models, instead of hand-maintaining them.

The main reasons this escape-hatch would be welcomed are:

• strong-scalars (this is a pretty common and established pattern in our codebase)
• types from external libraries (we would deem it "good enough" to defer the en/de-coding to the library)

I can see this approach breaking down if the schema uses custom "format" without defining a custom named schema.

This would IMO something worth-wile to investigate as well.
In our case this isn't a issue since the spec is in our control, but could be problematic when using a spec vendored by a third-party.

 User:
    type: object
    properties:
      name:
        type: string
      avatar_url: # there is (currently) no way to swap out this type
        type: string
        format: uri

[simonjbeaumont]

In our project, where we would like to migrate to generating our models, instead of hand-maintaining them.

So you're seeing this as a tool to unlock incremental adoption. That's an interesting idea.

In our case this isn't a issue since the spec is in our control, but could be problematic when using a spec vendored by a third-party.

More generally, this approach only works by patching the OpenAPI document. Are there any other approaches that would allow for something similar but still be useful to adopters who consume a doc from a third-party? Maybe that's a step too far.

I'm still interested in what @czechboy0 thinks about this proposal. If it were targeted, well-tested, and documented as a only an escape-hatch, WDYT to us moving forward?

[czechboy0]

I think it's a reasonable approach, yeah. An important decision will be whether this would only support primitive type replacement, ie to solve the typesafe scalars problem, or if it'd be supported to replace even object and other composite types - would be nice, just a heads up that it might be more involved implementation-wise. It'll mean not generating inline types, not validating them, etc.

But I'm in support of this in principle - to use vendor extensions to override which type reference is generated.

[simonbility]

Great to hear the approach is good.

I would very much prefer to support object types.

I tried to have a deeper look into the implementation as you mentioned, specifically not generating inline types, not validating them.

Maybe a bit too optimistic, but i have the feeling that most of this is already accounted for.

Here is a example (the latest revision shows the change adding the vendor-extension on a object type)
https://gist.github.com/simonbility/d84fa79b59cf487d41d6364dc4078a55/revisions

But i am not super-familiar with the codebase and would appreciate any pointers to specific spots i could check out.

[czechboy0]

Might be easiest for you to try to create a draft PR, while ensuring all tests still pass? Will be easier to iterate there. Once you have confidence that your approach works, writing a lightweight proposal would be great (we have a template for those). Thanks for looking into this!

[simonbility]

This PR already contains a POC version of the proposed changes, and the tests pass.

Should i create a dedicated github-issue for this as well? (and a additional PR introducing the evolution proposal)

[czechboy0]

I see, would you be able to add more tests, for other schema types? Especially: object, allOf/oneOf/anyOf (including nesting), nested object types, things like that. I'd also propose a slightly different name: x-swift-openapi-name-override (and we can further refine that during the proposal review).

[simonbility]

Hi finally had the time to look into this again.

I hope you don't mind that the time i can contribute to this can be a bit sporadic, with full-time job and family.

I do want to give an update on my progress so far:

• Added a Example Project (this 8646df8 might be of interest, as it shows the diff of applying the vendor-extensions)
• Allow replacements in inline schema definitions and added some more tests

I do have some concerns that x-swift-openapi-name-override could be interpreted as "only" a mechanism to control the name of the generated types, but does not clearly communicate that it actually prevents generating the type completely.

This i think would also be a useful, but separate, feature.

I used x-swift-open-api-replace-type in the meantime, and would be interested in more feedback on this from the evolution process.

Still on my todo-list:

• Write the Proposal document
• support/test replacing types and their interaction with default-values
• support/test replacing types in additionalProperties
• look into a potential bug in OpenAPIKit (seems vendor-extensions are not parsed for anyOf, allOf, oneOf) more here

  swift-openapi-generator/Tests/OpenAPIGeneratorCoreTests/Translator/TypesTranslator/Test_typeSubstitutions.swift

  Lines 79 to 117 in 8646df8

  	try _test(
  	schema: """
  	anyOf:
  	- type: string
  	- type: integer
  	x-swift-open-api-replace-type: MyLibrary.MyCustomType
  	""",
  	expectedType: .member(["MyLibrary", "MyCustomType"])
  	)
  	try _test(
  	schema: """
  	allOf:
  	- type: object
  	properties:
  	foo:
  	type: string
  	- type: object
  	properties:
  	bar:
  	type: string
  	x-swift-open-api-replace-type: MyLibrary.MyCustomType
  	""",
  	expectedType: .member(["MyLibrary", "MyCustomType"])
  	)
  	try _test(
  	schema: """
  	oneOf:
  	- type: object
  	properties:
  	foo:
  	type: string
  	- type: object
  	properties:
  	bar:
  	type: string
  	x-swift-open-api-replace-type: MyLibrary.MyCustomType
  	""",
  	expectedType: .member(["MyLibrary", "MyCustomType"])
  	)

[simonbility]

@simonjbeaumont @czechboy0 I added some more tests and also opened a PR on OpenAPIKit to address a issue preventing this from working on any/all/one-Of types mattpolzin/OpenAPIKit#409

I also have written a draft of an evolution proposal (currently this is in this PR and i can move that to a separate PR)
https://github.com/apple/swift-openapi-generator/blob/108386aa79dcc00dfaec78505da335d00613bbee/Sources/swift-openapi-generator/Documentation.docc/Proposals/SOAR-0014.md

In writing this i am reconsidering a bit if vendor-extensions are the best approach to this as the config approach in "Alternatives Considered" also seems quite straightforward, while also being easily usable with third-party open-api documents

Any Feedback on the implementation or the proposal would be welcom

[czechboy0]

This is great progress, @simonbility!

Since this might end up being a pretty large change, once all said and done, I wonder if we could consider scoping the first iteration down a bit, and extending it later in a separate PR/proposal.

Specifically, I wonder if the most constrained version that still fulfills a big chunk of the requirement could look like:

1. Only support replacing top level types in #/components/schemas, so postpone supporting schemas anywhere else till later.
2. Use a config mapping from the original schema name to a replacement Swift type, rather than a vendor extension.

While that might require some OpenAPI docs to get changed, it'd just be to move some inline types to be named top level types. Otherwise no change needed, and it'd be config driven.

I suspect that'll end up a much smaller implementation, because only the generation of Types.swift needs to be changed, and only in the one place where we iterate through top level types. It should also play nicely with both filtering and cycle detection.

If we can deliver this successfully, that'll prove out the concept without closing any doors to future enhancements. WDYT, @simonbility @simonjbeaumont?

[simonjbeaumont]

I like the idea of the reduced scope but IIUC from the above comments it would only have limited value for the adopter.

Maybe it's a good step in the right direction though.

[simonbility]

Im perfectly happy to keep this scoped to top-level types with the config based mapping.

Thats fairly close to my initial version where only later on i added inline properties,… for "completeness" sake.

Ill make the adaptations for the scoped down version at some point in the following days.

[simonbility]

I made the modifications and also updated the Proposal document https://github.com/apple/swift-openapi-generator/blob/2130376fb452a278ceec897963f3da2fa6de1be0/Sources/swift-openapi-generator/Documentation.docc/Proposals/SOAR-0014.md

Some questions/topics i wanted to bring up.

Should this be constrained to #/components/schemas?

In the current version this will override #/components/schemas/OldType

typeOverrides:
   OldType: NewType

The mappings could conceivably also be applied to other places (i.e. #/parameters)
Im leaning towards Yes, since for example when converting to a schema to query-parameters the generator needs to be "fully aware" of the schema (and cannot delegate it to Codable and assume it will be compliant)

If this should support all locations in the document i would opt to use the full path instead of just the name

typeOverrides:
  UUID: Foundation.UUID
# vs
  '#/components/schemas/UUID': UUID

Configuration approach

Since custom formats for strings/numbers are often requested features, they might get some support in the future.

I can imagine this support also falling into the "type overrides" category, so i want to be a bit careful to not "claim" the term in the config, and make evolving this in the future harder.

Just a throwaway idea how this could look like

typeOverrides:
  schemas:
    OldName: NewName
  formats:
    string:
      uuid: Foundation.UUID
      uri: Foundation.URL
    number:
      integer

[czechboy0]

for example when converting to a schema to query-parameters the generator needs to be "fully aware" of the schema (and cannot delegate it to Codable and assume it will be compliant)

Can you elaborate on this point? My assumption was that the adopter is responsible for making sure that their replacement type is indeed compatible with the replaced type. For example, if you replace an object with a string, the generator will let you do it (because it doesn't know what the type will be at runtime that you're replacing it with), but it likely won't work.

What's the awareness that you think the generator should have here?

[simonbility]

for example when converting to a schema to query-parameters the generator needs to be "fully aware" of the schema (and cannot delegate it to Codable and assume it will be compliant)

Can you elaborate on this point? My assumption was that the adopter is responsible for making sure that their replacement type is indeed compatible with the replaced type. For example, if you replace an object with a string, the generator will let you do it (because it doesn't know what the type will be at runtime that you're replacing it with), but it likely won't work.

What's the awareness that you think the generator should have here?

I assumed wronglythe code converting a type to query parameters was generated by the generator, but i now see that its actually based on top of Codable types.
Sorry for the confusion

[czechboy0]

No worries, sounds good.

[simonbility]

I would still have a question about how to approach the configuration.

Short JSON Name

typeOverrides:
  UUID: Foundation.UUID

Fully qualified JSON Path

typeOverrides:
  '#/components/schemas/UUID': UUID

The fully qualified JSON Path theoretically allows overriding not only #/comonents/schemas.
But i am doubtful that actually makes sense.
Most of the other components have some extra fields in them (for instance. responses have the contentTypes dict, ...) and usually wrap a schema in some form.

Based on that my approach would be to keep this scoped to '#/components/schemas' and go with the Short JSON Name approach.

This is also whats implemented at the moment.

Do you agree with that?

[czechboy0]

Mostly yes, I do wonder if we can make more room for others with:

typeOverrides:
  schemas:
    Foo: Bar.Baz

And in the future, if we support inline schemas, we'll recognize it from the key being a full JSON pointer, rather than just a single word.

[simonbility]

I like that approach 👍

Allows this to stay narrowly focused on schemas while not closing any doors for future extensions.

Also makes it a bit more obvious that this is replacing schemas

[simonbility]

Made the adaptations here f14f710

[czechboy0]

Thanks @simonbility - I think the proposal is ready to be reviewed by the community. Could you open a separate PR with just the proposal, and we'll kick off the review process? Thanks for working on this! 🙏

[czechboy0]

This might need updating, since this example doesn't currently have any executable targets.

[czechboy0]

Suggested change

[simonbility]

This one actually was on purpose.
The intent was to add a linebreak after TypeOverrides and then have all the overrides (only schemas at the moment) indented under it.

- TypeOverrides:<-this is the linebreak
  - Schemas: "Old"->"New" 

Without it would look like this

- TypeOverrides:  - Schemas: "Old"->"New" 

[simonbility]

Addressed all the Feedback, and added a separate PR with only the Proposal here #774