Generate enums for server variables

[theoriginalbit]

Motivation

Refer to proposal #629

PR description prior to raising proposal

### Motivation

Recently in a project I was using a spec which defined variables similar to below

servers:
  - url: https://{environment}.example.com/api/{version}
    variables:
      environment:
        default: prod
        enum:
          - prod
          - staging
          - dev
      version:
        default: v1

The generated code to create the default server URL was easy enough being able to utilise the default parameters

let serverURL = try Servers.server1()

But when I wanted to use a different variable I noticed that the parameter was generated as a string and it didn't expose the other allowed values that were defined in the OpenAPI document. It generated the following code:

/// Server URLs defined in the OpenAPI document.
internal enum Servers {
    ///
    /// - Parameters:
    ///   - environment:
    ///   - version:
    internal static func server1(
        environment: Swift.String = "prod",
        version: Swift.String = "v1"
    ) throws -> Foundation.URL {
        try Foundation.URL(
            validatingOpenAPIServerURL: "https://{environment}.example.com/api/{version}",
            variables: [
                .init(
                    name: "environment",
                    value: environment,
                    allowedValues: [
                        "prod",
                        "staging",
                        "dev"
                    ]
                ),
                .init(
                    name: "version",
                    value: version
                )
            ]
        )
    }
}

This meant usage needed to involve runtime checks whether the supplied variable was valid and if the OpenAPI document were to ever remove an option it could only be discovered at runtime.

let serverURL = try Servers.server1(environment: "stg") // might be a valid environment, might not

Looking into the OpenAPI spec for server templating and the implementation of the extension URL.init(validatingOpenAPIServerURL:variables:) I realised that the variables could very easily be represented by an enum in the generated code. By doing so it would also provide a compiler checked way to use a non-default variable.

Modifications

I have introduced a new set of types translator functions in the file translateServersVariables.swift which can create the enum declarations for the variables. If there are no variables defined then no declaration is generated.

Each variable defined in the OpenAPI document is generated as an enum with a case that represents each enum in the document. Each enum is also generated with a static computed property with the name default which returns the default value as required by the OpenAPI spec. These individual variable enums are then namespaced according to the server they are applicable for, for example Server1, allowing servers to have identically named variables with different enum values. Finally each of the server namespace enums are members of a final namespace, Variables, which exists as a member of the pre-existing Servers namespace. A truncated example:

enum Servers { // enum generated prior to this PR
  enum Variables {
    enum Server1 {
      enum VariableName1 {
        // ...
      }
      enum VariableName2 {
        // ...
      }
    }
  }
  static func server1(/* ... */) throws -> Foundation.URL { /* declaration prior to this PR */ }
}

To use the new translator functions the translateServers function has been modified to call the translateServersVariables function and insert the declarations as a member alongside the existing static functions for each of the servers. The translateServer(index:server:) function was also edited to make use of the generated variable enums, and the code which generated the string array for allowedValues has been removed; runtime validation should no longer be required, as the rawValue of a variable enum is the value defined in the OpenAPI document.

Result

The following spec

servers:
  - url: https://{environment}.example.com/api/
    variables:
      environment:
        default: prod
        enum:
          - prod
          - staging
          - dev

Would currently generate to the output

/// Server URLs defined in the OpenAPI document.
internal enum Servers {
    ///
    /// - Parameters:
    ///   - environment:
    internal static func server1(environment: Swift.String = "prod") throws -> Foundation.URL {
        try Foundation.URL(
            validatingOpenAPIServerURL: "https://{environment}.example.com/api/",
            variables: [
                .init(
                    name: "environment",
                    value: environment,
                    allowedValues: [
                        "prod",
                        "staging",
                        "dev"
                    ]
                )
            ]
        )
    }
}

But with this PR it would generate to be

/// Server URLs defined in the OpenAPI document.
internal enum Servers {
    /// Server URL variables defined in the OpenAPI document.
    internal enum Variables {
        /// The variables for Server1 defined in the OpenAPI document.
        internal enum Server1 {
            /// The "environment" variable defined in the OpenAPI document.
            ///
            /// The default value is "prod".
            internal enum Environment: Swift.String {
                case prod
                case staging
                case dev
                /// The default variable.
                internal static var `default`: Environment {
                    return Environment.prod
                }
            }
        }
    }
    ///
    /// - Parameters:
    ///   - environment:
    internal static func server1(environment: Variables.Server1.Environment = Variables.Server1.Environment.default) throws -> Foundation.URL {
        try Foundation.URL(
            validatingOpenAPIServerURL: "https://{environment}.example.com/api/",
            variables: [
                .init(
                    name: "environment",
                    value: environment.rawValue
                )
            ]
        )
    }
}

Now when it comes to usage

let url = try Servers.server1() // ✅ works

let url = try Servers.server1(environment: .default)  // ✅ works

let url = try Servers.server1(environment: .staging)  // ✅ works

let url = try Servers.server1(environment: .stg)  // ❌ compiler error, stg not defined on the enum

// some time later staging gets removed from OpenAPI document
let url = try Servers.server1(environment: . staging)  // ❌ compiler error, staging not defined on the enum

If the document does not define enum values for the variable, an enum is still generated with a single member (the default required by the spec).

servers:
  - url: https://example.com/api/{version}
    variables:
      version:
        default: v1

Before this PR:

/// Server URLs defined in the OpenAPI document.
internal enum Servers {
    ///
    /// - Parameters:
    ///   - version:
    internal static func server1(version: Swift.String = "v1") throws -> Foundation.URL {
        try Foundation.URL(
            validatingOpenAPIServerURL: "https://example.com/api/{version}",
            variables: [
                .init(
                    name: "version",
                    value: version
                )
            ]
        )
    }
}

With this PR:

/// Server URLs defined in the OpenAPI document.
internal enum Servers {
    /// Server URL variables defined in the OpenAPI document.
    internal enum Variables {
        /// The variables for Server1 defined in the OpenAPI document.
        internal enum Server1 {
            /// The "version" variable defined in the OpenAPI document.
            ///
            /// The default value is "v1".
            internal enum Version: Swift.String {
                case v1
                /// The default variable.
                internal static var `default`: Version {
                    return Version.v1
                }
            }
        }
    }
    ///
    /// - Parameters:
    ///   - version:
    internal static func server1(version: Variables.Server1.Version = Variables.Server1.Version.default) throws -> Foundation.URL {
        try Foundation.URL(
            validatingOpenAPIServerURL: "https://example.com/api/{version}",
            variables: [
                .init(
                    name: "version",
                    value: version.rawValue
                )
            ]
        )
    }
}

Result

Refer to #618 (comment)

Test Plan

I have updated the petstore unit tests to reflect the changes made in this PR, see diff.

[simonjbeaumont]

Thanks @theoriginalbit!. This is just a courtesy note to acknowledge I’ve seen it but I’ll probably get to it early next week.

[czechboy0]

This is great, @theoriginalbit!

I only skimmed it, but two things I suspect we need to change before landing it:

1. This is an API breaking change, so it needs to be hidden behind a feature flag (see FeatureFlags.swift), and will need to be opt-in. As existing generated code that adopters are already calling today cannot change its API in a breaking way.
2. The enum should only be generated for variables that actually have "allowedValues" defined, but other raw strings should continue to be just strings.

Thanks again! I'm sure we'll be able to land this with a few tweaks.

[theoriginalbit]

Thanks so much for taking a look and providing feedback @czechboy0.

When I was writing the PR description I did wonder about how this breaking change would be handled, I totally forgot to add the question to the description.

I will definitely take a look at the FeatureFlags and add one in for this, is there a particular convention for naming the feature flags that would be appropriate to enable this generation? And is there a unit testing approach already set up for testing feature flags?

2. The enum should only be generated for variables that actually have "allowedValues" defined, but other raw strings should continue to be just strings.

So just to make sure I'm understanding correctly, in the scenario of the OpenAPI document

servers:
  - url: https://{environment}.example.com/api/{version}
    variables:
      environment:
        default: prod
        enum:
          - prod
          - staging
          - dev
      version:
        default: v1

You'd like to see it output as the following generated code?

/// Server URLs defined in the OpenAPI document.
internal enum Servers {
    /// Server URL variables defined in the OpenAPI document.
    internal enum Variables {
        /// The variables for Server1 defined in the OpenAPI document.
        internal enum Server1 {
            /// The "environment" variable defined in the OpenAPI document.
            ///
            /// The default value is "prod".
            internal enum Environment: Swift.String {
                case prod
                case staging
                case dev
                /// The default variable.
                internal static var `default`: Environment {
                    return Environment.prod
                }
            }
        }
    }
    ///
    /// - Parameters:
    ///   - version:
    internal static func server1(
        environment: Variables.Server1.Environment = Variables.Server1.Environment.default,
        version: Swift.String = "v1"
    ) throws -> Foundation.URL {
        try Foundation.URL(
            validatingOpenAPIServerURL: "https://example.com/api/{version}",
            variables: [
                .init(
                    name: "environment",
                    value: environment.rawValue
                ),
                .init(
                    name: "version",
                    value: version
                )
            ]
        )
    }
}

I'm very happy to make this change, though one thing I want to confirm with you all first since perhaps my understanding of the OpenAPI spec was wrong.

My understanding of the spec is that a default-only variable, version in this case, is defined because the provider knows at some point there will be additional values but there are none to yet define in an enum property. So the following two configurations would be functionally equivalent

# Syntax sugar
servers:
  - url: https://example.com/api/{version}
    variables:
      version:
        default: v1

# Verbose
servers:
  - url: https://example.com/api/{version}
    variables:
      version:
        default: v1
        enum:
          - v1

If that is the case, then I believe generating a default-only variable as an enum with a single case would be better for the consumer as if the provider later adds extra enum values then updating to that spec doesn't introduce a breaking change (it still uses the default value).

Also, perhaps it better communicates that there is only one available option? I could see with the above document a consumer invoking as such simply because it's a string

let url = try Servers.server1(version: "v2")

Which wouldn't be a runtime issue, even in the latest released implementation, since default-only variables don't get allowedValues passed as a parameter. My thinking with this is if it were to be an enum with a single case the compiler would enforce that the consumer is only able to provide v1 and not any other undocumented value.

What are your thoughts on this, have I misunderstood the purpose of default-only variables? Should they be open and unrestricted strings?

[simonjbeaumont]

Quick (half-baked) thought on how we could do this in a non-API-breaking way so as to be able to land this without a feature flag or major version: could we make these enums ExpressibleByStringLiteral? IIUC this would allow the newly generated code to continue to work, where folks are passing in string literals for the parameters, but bring in the compile-time guarantees for those that want them.

Whether this is something we can do will depend on how we interpret the OpenAPI spec w.r.t. whether values outside of those defined in the document are permitted.

[simonjbeaumont]

With regard to the OAS:

Field Name	Type	Description
enum	[string]	An enumeration of string values to be used if the substitution options are from a limited set. The array MUST NOT be empty.
default	string	REQUIRED. The default value to use for substitution, which SHALL be sent if an alternate value is not supplied. Note this behavior is different than the Schema Object’s treatment of default values, because in those cases parameter values are optional. If the enum is defined, the value MUST exist in the enum’s values.
description	string	An optional description for the server variable. [CommonMark] syntax MAY be used for rich text representation.

— source: https://spec.openapis.org/oas/latest.html#server-variable-object

An enumeration of string values to be used if the substitution options are from a limited set. The array MUST NOT be empty.

I take this to mean that, if an enum is provided, then it is a limited set.

[theoriginalbit]

Ah yeah, I totally misread and misunderstood the OAS, that wording definitely suggests that if there is no enum values defined then it's an open field. I'll update the PR later today to revert those specific definitions to be open Strings, and only apply the enum generation to variables defined with an enum field.

Using ExpressibleByStringLiteral is an interesting thought 🤔 Though the only way I think I see that working would be that each enum needs to have an undocumented(String) case generated; at runtime that case would throw the same existing RuntimeError that was thrown when validating the allowed values. Since the OAS does specify "limited set" I imagine that's why the current latest release performs the runtime checks and throws an error if the value is outside of what was documented. It feels to me that this better suits a breaking (but feature flagged) change so that going forward there is no option to provide a string which then requires runtime checks. Thoughts?

[theoriginalbit]

Apologies, I ran out of time to address the feedback the other day.

The changes I've just pushed fixes both of the issues raised. I've introduced a feature flag, ServerVariablesEnums, and it will only generate enums for variables that have enum defined in the OpenAPI Document.

So given the following definition

servers:
  - url: https://example.com/api
    description: Example service deployment.
    variables:
      environment:
        description: Server environment.
        default: prod
        enum:
          - prod
          - staging
          - dev
      version:
        default: v1

With the ServerVariablesEnums feature flag not defined the output would be

/// Server URLs defined in the OpenAPI document.
internal enum Servers {
    /// Example service deployment.
    ///
    /// - Parameters:
    ///   - environment: Server environment.
    ///   - version: The overall API version
    internal static func server1(
        environment: Swift.String = "prod",
        version: Swift.String = "v1"
    ) throws -> Foundation.URL {
        try Foundation.URL(
            validatingOpenAPIServerURL: "https://example.com/api",
            variables: [
                .init(
                    name: "environment",
                    value: environment,
                    allowedValues: [
                        "prod",
                        "staging",
                        "dev"
                    ]
                ),
                .init(
                    name: "version",
                    value: version
                )
            ]
        )
    }
}

With the ServerVariablesEnums feature flag provided the output would be

/// Server URLs defined in the OpenAPI document.
internal enum Servers {
    /// Server URL variables defined in the OpenAPI document.
    internal enum Variables {
        /// The variables for Server1 defined in the OpenAPI document.
        internal enum Server1 {
            /// Server environment.
            ///
            /// The "environment" variable defined in the OpenAPI document. The default value is "prod".
            internal enum Environment: Swift.String {
                case prod
                case staging
                case dev
                /// The default variable.
                internal static var `default`: Environment {
                    return Environment.prod
                }
            }
        }
    }
    /// Example service deployment.
    ///
    /// - Parameters:
    ///   - environment: Server environment.
    ///   - version:
    internal static func server1(
        environment: Variables.Server1.Environment = Variables.Server1.Environment.default,
        version: Swift.String = "v1"
    ) throws -> Foundation.URL {
        try Foundation.URL(
            validatingOpenAPIServerURL: "https://example.com/api",
            variables: [
                .init(
                    name: "environment",
                    value: environment.rawValue
                ),
                .init(
                    name: "version",
                    value: version
                )
            ]
        )
    }
}

I've also added a new test case to validate the output when the feature flag is enabled.

[czechboy0]

Overall this looks like a good direction, but since this is introducing new generated API, I'd like us to run this through our lightweight proposal process: https://swiftpackageindex.com/apple/swift-openapi-generator/1.3.0/documentation/swift-openapi-generator/proposals

I think some of the names could use community feedback. The proposal doesn't need to be long, much of what you wrote up in the PR description can probably be lifted over to the proposal already.

Some things I'm curious to hear other people's feedback on is:

• the names of the generated types Variables
• whether we need a dedicated default static var, or if the default should be generated inline in the server3(...) function

Let us know if you have any questions - thanks again, this will be a great addition 🙂

[czechboy0]

Okay the implementation looks good, now let's wait for the review to run its course and we'll finalize that once accepted - thanks again! 🙏

[theoriginalbit]

Okay the implementation looks good, now let's wait for the review to run its course and we'll finalize that once accepted - thanks again! 🙏

Awesome, no worries at all. Hopefully some people can provide feedback on the namespace names, otherwise looking forward to this going through the finalising steps.

[czechboy0]

A few minor change requests, but looks good overall! Thanks

[czechboy0]

Oh, one more thing:

• please find and replace all references to Servers.server1() (and similar) in this repo with the new syntax, especially in documentation and examples, to make sure that out of the box, folks use the new syntax and don't see the warning

[theoriginalbit]

I've addressed the latest round of feedback @czechboy0 :)

• please find and replace all references to Servers.server1() (and similar) in this repo with the new syntax, especially in documentation and examples, to make sure that out of the box, folks use the new syntax and don't see the warning

None of the examples appeared to use the generated servers URL functions.

I also opted to keep one test assertion, for the sake of regression testing, which tested the deprecated API threw if an invalid variable was supplied. Once the deprecated functions are no longer being generated I figured the assertion could be removed, but at least for now it serves a purpose.

[czechboy0]

• please find and replace all references to Servers.server1() (and similar) in this repo with the new syntax, especially in documentation and examples, to make sure that out of the box, folks use the new syntax and don't see the warning

None of the examples appeared to use the generated servers URL functions.

There are places in Examples and IntegrationTest that'll need updating, but we'll need to do that as a follow-up, once this change lands and gets released. All the other places you've already updated, thank you 🙂

[czechboy0]

One small request, otherwise looks great! Kicking off CI.

[czechboy0]

lgtm, we just need to get the CI green. The format check just requires you to run e.g. act --bind workflow_call -j soundness --input format_check_enabled=true and commit the changes.

[czechboy0]

Oh and you'll need to move the nested protocol out, as we still support 5.9:

/__w/swift-openapi-generator/swift-openapi-generator/Sources/_OpenAPIGeneratorCore/Translator/TypesTranslator/translateServersVariables.swift:51:14: error: protocol 'ServerVariableGenerator' cannot be nested inside another declaration

[theoriginalbit]

Sorry took a bit, had to download act and Docker; then the inevitable debugging to get them to actually work.

Hopefully all fixed now, @czechboy0.

[czechboy0]

Thank you for the great contribution, @theoriginalbit! 👏