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

enum unknown values #130

Open
LePips opened this issue Aug 8, 2022 · 16 comments · May be fixed by #143
Open

enum unknown values #130

LePips opened this issue Aug 8, 2022 · 16 comments · May be fixed by #143

Comments

@LePips
Copy link
Contributor

LePips commented Aug 8, 2022

Forced unknown

Since we are creating API interfaces, new cases may be created from the server that aren't available on a current version of a developer's SDK or even their available schema. Only Apple can create nonfrozen enums by my understanding so developers cannot use @unknown default. So, if an app with an out-of-date API schema attempts to decode an unknown case for a property or return type that cannot be optional, it will crash.

To fix this issue, create a new config option enumForceUnknown: true that adds a new case to every enum which represents an unknown case. We would decode to this unknown case if there is no properly expected case currently defined. The name is open for improvement, but I think that it should stand out from typical Swift formatting in a way that ensures it will be different from any case in the schema and tells the developer that this is strongly unknown.

enum MyTypes {
    case foo
    case bar
    case forced_unknown // added unknown case
}

Organization

Smaller pitch for an organization option to put enums in a separate folder: Enums. Just as an organizational measure this can help a lot to separate the struct/class entities from other Swift types to the developer. This would only apply to the split generation and remain as an option. Or, could also apply to merged generation with a separate Enums.swift file.

MyAPI
  - Entities/
  - Paths/
  - Enums/
  AnyJson.swift

Potentially split generation might have more customization options, so we create a new section in the config for each generation type. This would help with #47.

split:
  - enumFolder: true
  // other options

There would also be a separate section for merged with whatever options it would potentially have. The existence of these two sections together would throw an incorrect config error. This was just a loose idea, needs much improvement.

@liamnichols
Copy link
Member

Only Apple can create nonfrozen enums by my understanding so developers cannot use @unknown default.

Any vendor can compile their library using Library evolution using BUILD_LIBRARY_FOR_DISTRIBUTION (although I'm not sure how to do this with swift packages).

Using such library would mean that by default any enum within that module will be treated as unfrozen which would require the @unknown default statement in switch cases unless the enum was annotated with @frozen.

But then again, it's probably unlikely that anybody is ever going to use library evolution.

So, if an app with an out-of-date API schema attempts to decode an unknown case for a property or return type that cannot be optional, it will crash.

This to be honest sounds like a problem outside of CreateAPI because the API being consumed isn't being versioned properly. The JSON Schema/OpenAPI specification does not support any differentiation between frozen or unfrozen enums and technically when you are using a different schema, it should be versioned differently.

We have this discussion a lot in my company and make it clear to api engineers that enums should be considered frozen for the current API version so adding cases is considered a breaking change.


That being said, if we really did want to support flexibility then instead I recommend generating RawRepresentable structs instead of enums. For example:

enum MyTypes: String {
    case foo
    case bar
}

// usage

switch value {
case .foo:
    print("Do foo")
case .bar:
    print("Do bar")
}

would become this:

struct MyTypes: RawRepresentable {
    let rawValue: String
    
    init(rawValue: String) {
        self.rawValue = rawValue
    }

    static let foo = Self(rawValue: "foo")
    static let bar = Self(rawValue: "bar")
}

// usage

switch value {
case .foo:
    print("Do foo")
case .bar:
    print("Do bar")
case MyType(rawValue: "baz"):
    print("Do undocumented baz")
default:
    print("Do something else")
}

The generateEnums option could instead become something like enumDecoration with accepted values of none, enum and rawRepresentable. Maybe we could come up with something better, but something along those lines.

@LePips
Copy link
Contributor Author

LePips commented Aug 9, 2022

The JSON Schema/OpenAPI specification does not support any differentiation between frozen or unfrozen enums and technically when you are using a different schema, it should be versioned differently.

No, that's because this lands in the territory of how a Swift developer uses this schema. This isn't about frozen/unfrozen enums within the same version of the schema but across different schema versions. I still very much consider this "unfrozen" as an API is very much open to evolution. It's somewhat obvious that an application should not crash due to unexpected responses from an API, at least as much as it can and unexpected enum cases are easy to account for.


Basic example, if Pet.type was required and is defined by the cases .dog, .cat in V1 and then V1.1 added .bird to the available cases:

1 - if the developer didn't update the app's API to V1.1 it would crash for everyone
2 - if the developer did update the app's API but people haven't updated the app, it will crash for them

Instead the application just probably wouldn't show these unknown types and they can only be shown to people who update their app.


We don't necessarily have the responsibility to handle this, after all that is what the schema explicitly states, but it would definitely help developers use an API. This kind of addition (among all the other options like exclusions and renamings I argue) is strictly Swift developer facing and does not actually go against the schema, it just helps us work with safety and convenience.

RawRepresentable instead of enum

I think this can be problematic because then one would have to switch against String, which would always require a default case.* I was thinking something with the decoding that could fallback to the unknown case if that's included, hence why we should choose a name that will probably not conflict with existing cases but that can also be customizable.

*Nevermind, I kind of see where you were going with this (but the crossed out point portion still applies). I still think that it would be a lot simpler to just provide an extra enum case as any arbitrary MyType can be set which isn't what I was recommending, just an unknown value. Back to the example, the developer should not necessarily be allowed to receive .bird as it hasn't updated the API. It should just be unknown to the developer.

@marcprux
Copy link

marcprux commented Aug 9, 2022

I just ran into this issue in a project that uses CreateAPI to access Apple's App Store Connect API: AvdLee/appstoreconnect-swift-sdk#197 (comment).

I think the RawRepresentable String wrapper solution is better than the forced_unknown enum case, since client code will still be able to access the rawValue of the enumeration, and it lends itself to adding undocumented cases using extensions, like so:

struct BundleIdPlatform : Codable, RawRepresentable, Equatable { 
    var rawValue: String
    static let ios = Self(rawValue: "IOS")
    static let macos = Self(rawValue: "MAC_OS")
}

extension BundleIdPlatform {
    // unhandled cases can be manually added with an extension
    static let universal = Self(rawValue: "UNIVERSAL")
}

But I also think there are some cases where an enum might make more sense, such as when the API documents an explicit guarantee that the enumerated values will never change. So perhaps this could be added as a configuration attribute like entities.openEnumerations listing the type names that should be instead implemented as a RawRepresentable struct.

Either way, the current implementation is a ticking time bomb. I'm surprised that more users of APIs like this are not experiencing mysterious parse errors from the lack of forward-compatibility for enums that add an additional case.

@LePips
Copy link
Contributor Author

LePips commented Aug 9, 2022

I see value now in creating the extensions from a RawRepresentable. That sounds good 👍

@marcprux
Copy link

marcprux commented Aug 9, 2022

While I'm spitballing ideas, maybe there could be a entities.openEnumerationsByDefault property that defaults to false in order to continue generating the same code as before (and thus not break client code that might be relying on switch statements). When entities.openEnumerationsByDefault is true, it would behave as if all of the types were listed in the entities.openEnumerations property, and so generate the RawRepresentable implementations by default.

This would enable projects to easily choose between enabling open enumerations by default, or else progressively opt-in to using extensible enumerations with the entities.openEnumerations.

I suppose, for completeness, there should also be a entities.frozenEnumerations list that allows a user to override the entities.openEnumerationsByDefault = true behavior for a set of specific types. This could be useful when an API contains many open enumerations, but has a few that are documented to never change (e.g., some logically-frozen enum like cardinalDirection = ["north", "south", "east", "west"]).

@LePips
Copy link
Contributor Author

LePips commented Aug 9, 2022

With #111 we can now condense the options into two:

  • entities.openEnumerations
  • entities.frozenEnumerations

We would of course just include the checks that neither of these are true or false at the same time, they must independent binary values/cannot have conflicting enums. Haven't thought through the fine details but that sounds good as well

@liamnichols
Copy link
Member

I don't think #111 will help in this instance actually because that only applies when using a Set of values that are CaseIterable (like mutableProperties).

We can however probably follow the same pattern that we do with entities.defaultType and entities.typeOverrides.

Something like defaultEnumBehaviour and enumBehaviourOverrides?

Disable

Before

isGeneratingEnums: false

After

enumOutput: string

Closed/Frozen/Normal

Before

isGeneratingEnums: true

After

enumOutput: enum

Open/RawRepresentable

enumOutput: rawRepresentable

Overrides

enumOutput: enum
enumOutputOverrides:
  FooEnum: rawRepresentable
  BarEnum: string

I'm still not convinced about the naming that I've suggested above, but having the two options like this seems to make the most sense I think? It also covers all the bases described above.

The one problem that I can think of though is how to reference an enum in enumOutputOverrides since enums aren't defined as schemas. I'm not completely sure how CreateAPI goes about nesting them/generating their names? Maybe it's derived from the property name and then nested within another entity? Does that always happen?

Do we want to try and make sure that we only reference types in the context of the spec? In that case we might need to use the FooType.barProperty syntax to reference the property that is the enum in the schema?

@liamnichols
Copy link
Member

Oh I was wrong, you can define enums in components.schemas as part of the spec - https://swagger.io/docs/specification/data-models/enums/

@liamnichols
Copy link
Member

It turns out that enums can be integer and other types as well so using string to represent isGeneratingEnums: false isn't quite right

@LePips
Copy link
Contributor Author

LePips commented Aug 9, 2022

Mirroring the type overrides sounds a lot neater.

@LePips
Copy link
Contributor Author

LePips commented Aug 12, 2022

Building off of the earlier comment, I think we can make a separate section for enum generation just like paths and entities. They're technically a different kind of object than an entity.

enums:
  defaultType: enum || static
  typeOverrides:
    myEnum: enum || static
  # Would probably copy a lot of `entities` options, but filter out
  # what doesn't match for enums.
  # Sort alphabetically, inits, etc.
  # Could make enum specific ones like caseiterable option
  # and providing the equivalent for `static` generation

This would overall generalize the work for this.

@liamnichols
Copy link
Member

Interesting. With that in mind, does it also mean that we should include enums in the new generate (#134) option instead of the generateEnums option?

generate: [entities, enums]
enums:
  defaultType: struct # (== RawRepresentable)
  typeOverrides:
    FrozenOption: enum
  # out of scope, but as an example:
  protocols:
  - CaseIterable

@liamnichols
Copy link
Member

It feels like it would make sense to incorporate enums under generate regardless to if we bring in more options or not actually. I'll probably go ahead and do that in my change anyway unless there is a good reason not to?

@LePips
Copy link
Contributor Author

LePips commented Aug 13, 2022

Sensible to me

@liamnichols
Copy link
Member

📝 Looks like this could have the potential to provide a workaround for AvdLee/appstoreconnect-swift-sdk#197 as well 👍

@nickasd
Copy link

nickasd commented Sep 24, 2023

This would also be very much welcome for App Store Connect API. Currently it contains several enums representing software platforms, screenshot types and so on that can potentially change whenever an Apple product with a new operating system or screen size is launched.

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 a pull request may close this issue.

4 participants