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 custom mappings between schema formats and Swift types #114

Closed
Tracked by #101
LePips opened this issue Aug 5, 2022 · 9 comments · Fixed by #142
Closed
Tracked by #101

Support custom mappings between schema formats and Swift types #114

LePips opened this issue Aug 5, 2022 · 9 comments · Fixed by #142
Milestone

Comments

@LePips
Copy link
Contributor

LePips commented Aug 5, 2022

Edit: differs from the original to a single global override instead of individual properties and I forgot parameter types. I think that we can start with a global override and somebody can request this fine tuning later.


Introduce a config option to override the Swift type when they are parsed and written:

# schema
Pet:
  properties:
    id:
      type: string
    tag:
      type: string
      format: uuid

# config
swiftTypeOverrides:
  UUID: String # any occurrences of UUID are replaced with String, so `Pet.tag: String`

Config name open for improvement. Obviously, a developer might override to a type that cannot be decoded from/encoded to the original type, like UUID: Int or a type that does not exist, like UUID: qwerty. So, it is a developer responsibility to make sure that overriding types can be properly decoded/encoded and that they are a proper Swift type.


This can be further enhanced with imports as we should be trivially able to set an imported type after #85 is figured out. Using NaiveDate as an example removing the useNaiveDate option, we include the package as an import in the config and then set the type override:

# schema
Pet:
  properties:
    birth_date:
      type: string
      format: date

# config
swiftTypeOverrides:
  Date: NaiveDate # any occurrences of Date are replaced with NaiveDate

Obviously, imported objects should be expected to properly decode/encode but that's an implementation detail of the imported type.

@LePips LePips changed the title Entity property type changes Entity property type overrides Aug 5, 2022
@LePips LePips changed the title Entity property type overrides Global and property type overrides Aug 6, 2022
@LePips LePips changed the title Global and property type overrides Global Swift type override Aug 6, 2022
@liamnichols
Copy link
Member

liamnichols commented Aug 13, 2022

To just clarify the scope here, I don't think we want to override the Swift types used, but more that we want the ability to customise how the format of a type should be interpreted?

If we summarise what we have today, its somewhat like the following:

string:
  date: Date
  date-time: NaiveDate # if useNaiveDate
  uuid: UUID
  uri: URL
  byte: Data
integer:
  int32: Int32 # if useFixWidthIntegers
  int64: Int64 # if useFixWidthIntegers
number:
  float: Float
  double: Double

With the current type/format -> Swift type behaviour, there are a few drawbacks:

  1. It's spread across multiple configurations
  2. We don't provide handling for even some of the defined formats (i.e password)
  3. There is no way to customise/override beyond useNaiveDate and useFixWidthIntegers

So what we want is for the generator to be able to use alternate mappings in addition to the default ones?

For example, a formats (or formatMappings or formatOverrides) property that is [String: [String: String]] ([SchemaType: [SchemaFormat: SwiftType]])?

So when inferring the Swift type of any given property in the generator, we can make some helper functions to share the lookups across Schemas and Path generation since it looks like there is some copy and paste going on:

private extension ConfigOptions {
    func integerType(for format: JSONTypeFormat.IntegerFormat) -> TypeIdentifier {
        let mappings = formats["integer"] ?? [:]

        switch (format, mappings[format.rawValue]) {
        case (.int32, nil):
            return .builtin("Int32")
        case (.int64, nil):
            return .builtin("Int64")

        case (_, let custom?):
            return .builtin(custom)

        case (_, nil):
            return .builtin("Int")
        }
    }

    func numberType(for format: JSONTypeFormat.NumberFormat) -> TypeIdentifier {
        let mappings = formats["number"] ?? [:]

        switch (format, mappings[format.rawValue]) {
        case (.double, nil):
            return .builtin("Double")
        case (.float, nil):
            return .builtin("Float")

        case (_, let custom?):
            return .builtin(custom)

        case (_, nil):
            return .builtin("Double")
        }
    }

    func stringType(for format: JSONTypeFormat.StringFormat) -> TypeIdentifier {
        let mappings = formats["string"] ?? [:]

        switch (format, mappings[format.rawValue]) {
        case (.dateTime, nil):
            return .builtin("Date")
        case (.date, nil):
            return .builtin("NaiveDate")
        case (.other("uri"), nil):
            return .builtin("URL")
        case (.other("uuid"), nil):
            return .builtin("UUID")
        case (.other("byte"), nil):
            return .builtin("Data")

        case (_, let custom?):
            return .builtin(custom)

        case (_, nil):
            return .builtin("String")
        }
    }
}

There might be a better way to write this, but the behaviour is as follows:

  1. If there wasn't a custom value and we know of a sensible default, use it
  2. If it was a format we are not aware of (i.e password) and a custom value, use it
  3. Otherwise use the default/fallback

This would allow us to make useFixWidthIntegers and useNaiveDate redundant, but still preserve their behaviours. It would require flipping the default behaviour of useFixWidthIntegers but it would be something like this:

Before

useFixWidthIntegers: false
useNaiveDate: false

After

formats:
  integer:
    int32: Int
    int64: Int
formats:
  string:
    date: String

It would require flipping the default behaviour of useFixWidthIntegers

I think this is ok because currently there is a discrepancy between how we handle string formats vs integer formats. For strings, we aim for the closest representation of a type by default but with ints, we don't. I don't see a signifiant harm in using fixed-width integers as a default if we have a suitable option to override that behaviour like the generic solution discussed here. If we don't want to change the default behaviour though, we could just set the default to Int and Int, but it feels wrong 😄

@LePips
Copy link
Contributor Author

LePips commented Aug 13, 2022

Yep! The 3 step behavior is what my current work does and I like the change in how the overrides are done in the config, so I'll switch to that. That also changes how I would set the types and clean things up a bit.

My PR also used checks the format as I didn't know about them being completely open when writing the issue.

useFixWidthIntegers

I'm okay for removing that and moving forward with the behavior you described. We can put that in the documentation as an example for convenience and clarity, as this would most likely be an override used by almost everybody. That's probably a reason to keep it but to be inline with this behavior they should specify it. After all, it would only be a few lines and would be easily documented and understandable.


For example, if you wanted to have Schema int32/int64 integers generate as Int:

... yaml example ...


@liamnichols
Copy link
Member

as this would most likely be an override used by almost everybody.

Hmm yeah this is a tough one 😄 If its complicating things for what the majority of users want, then maybe we should just default them both to Int.

My assumption is that if the schema is defining a fixed width, then that is for an important reason so we should respect that given that we can in Swift.

But being completely honest, I haven't seen enough schemas to know what the norm is 😄 I see that our Petstore schema does use fixed with format values but with a gigantic example like the App Store Connect API schema, they are not defined.

@LePips
Copy link
Contributor Author

LePips commented Aug 13, 2022

I think that the safest assumption would be that if the format isn't specified we leave it to Int and if there is we make the fixed width integers. After all, specifying fixed width is a specific system configuration and we can't know why a developer specified it (like they're possible making an API schema for use by embedded systems).

@liamnichols liamnichols mentioned this issue Aug 13, 2022
27 tasks
@LePips
Copy link
Contributor Author

LePips commented Aug 13, 2022

I also assume that "used by everyone" would only be as a result from auto-generated schemas from server documentation, which can specify the format as the type used by the server's implemented type. So, "used by everyone" might not necessarily be correct.

@liamnichols liamnichols changed the title Global Swift type override Support custom mappings between schema formats and Swift types Aug 13, 2022
@liamnichols liamnichols added this to the 0.1.0 milestone Aug 13, 2022
@LePips
Copy link
Contributor Author

LePips commented Aug 13, 2022

I like the change in how the overrides are done in the config

Actually thinking about my work again, I think that what I do in the PR is more powerful because they can also set a global type override. For int32/int64, it can just be declared that all OpenAPI integer data types will be represented as:

swiftTypeOverrides:
  integer: Int

This also parallels the way that we do the exclude/include, if you see what I mean.

@liamnichols
Copy link
Member

I see what you mean, but I don't think this should be supported.

I can understand use cases where you want to change the type based on the format, but I don't think you should ever be able to alter a primitive type (string, integer, number, boolean, array, object).

The only case where it might be useful is the one that you highlighted above so that we can disable fixed-sized integers in less config, but I don't think its worth it for the other weirdness that it has the potential to bring.

I think that we should keep this scoped as a 1:1 map between a types format and the resolved Swift type

@LePips
Copy link
Contributor Author

LePips commented Aug 13, 2022

I'm happy to implement only the formats, but why not allow the global override?

There can be cases where people need/want to set a primitive type but there's no format in the schema, so they can't set it and they would just get whatever we give them. People might need to have all integer in Int32 but there wouldn't be a format specified or people might want all string to be a String regardless of the provided format. This is the reasoning for the global override, the "disable fixed-sized integers in less config" is just a nice side effect.

The schema doesn't care about the primitive types implemented on a system, that's a developer concern. It's further a developer concern if they implement the wrong type that won't work within the context of their API, so the weirdness would be an error on their part. This would just allow full customization and I think that it would be more weird to only have formats allowed in this. This behavior can be easily documented.

@liamnichols
Copy link
Member

I wasn't particularly a fan of having a flat dictionary of schema-type.format: SwiftType entries to represent both global overrides as well as individual format specifiers. I feel that that it complicates the implementation (lookup is more complex) and I'm still not sure what value it brings.

But if we did want to support it, maybe we could do something like this then:

dataTypes:
  string:
    password: SecureString
    custom-thing: CustomThing
  integer: Int # applies regardless of the `format`
  number:
    decimal: NSDecimalNumber

Which would solve some concerns that I have, but I'm still not entirely convinced by the usefulness of it.

People might need to have all integer in Int32 but there wouldn't be a format specified

I'm having a hard time thinking of why you'd need to use Int32 across the board for all integer types.

or people might want all string to be a String regardless of the provided format.

That's achievable with the proposed solution (but again like fixed-width ints is a little more configuration).


If you want to support both approaches, please feel free to but I also think format-specific overrides would be good enough to begin with at least 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants