Generate enums for server variables #618

theoriginalbit · 2024-09-07T02:16:00Z

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 · 2024-09-07T08:54:59Z

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 · 2024-09-08T15:24:38Z

This is great, @theoriginalbit!

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

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.
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 · 2024-09-08T23:23:18Z

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?

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 · 2024-09-09T14:14:48Z

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 · 2024-09-09T14:37:35Z

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 · 2024-09-09T19:00:31Z

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 · 2024-09-12T11:29:40Z

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 🙂

Sources/_OpenAPIGeneratorCore/FeatureFlags.swift

Tests/OpenAPIGeneratorReferenceTests/FileBasedReferenceTests.swift

Co-authored-by: Honza Dvorsky <[email protected]>

Tests/OpenAPIGeneratorReferenceTests/SnippetBasedReferenceTests.swift

czechboy0 requested changes Sep 18, 2024

View reviewed changes

Sources/_OpenAPIGeneratorCore/FeatureFlags.swift Outdated Show resolved Hide resolved

Tests/OpenAPIGeneratorReferenceTests/FileBasedReferenceTests.swift Outdated Show resolved Hide resolved

theoriginalbit mentioned this pull request Sep 18, 2024

[Proposal] SOAR-0012 Generate enums for server variables #629

Open

theoriginalbit and others added 4 commits September 18, 2024 22:56

Generate enums for server variables

a04abcc

Generated enums for server variables are now feature flagged

cf3b952

Update Sources/_OpenAPIGeneratorCore/FeatureFlags.swift

ba26b65

Co-authored-by: Honza Dvorsky <[email protected]>

Revert addition of file-based reference test

78bf584

theoriginalbit force-pushed the refactor/generate-server-variable-enums branch from 0a752ea to 78bf584 Compare September 19, 2024 10:19

czechboy0 reviewed Sep 19, 2024

View reviewed changes

Tests/OpenAPIGeneratorReferenceTests/SnippetBasedReferenceTests.swift Outdated Show resolved Hide resolved

Introduce snippet-based reference tests for server variable translation

6ad525a

theoriginalbit force-pushed the refactor/generate-server-variable-enums branch from 0504c42 to 6ad525a Compare September 19, 2024 22:49

theoriginalbit requested a review from czechboy0 September 19, 2024 22:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Generate enums for server variables #618

Generate enums for server variables #618

theoriginalbit commented Sep 7, 2024 •

edited

Loading

simonjbeaumont commented Sep 7, 2024

czechboy0 commented Sep 8, 2024

theoriginalbit commented Sep 8, 2024 •

edited

Loading

simonjbeaumont commented Sep 9, 2024

simonjbeaumont commented Sep 9, 2024

theoriginalbit commented Sep 9, 2024

theoriginalbit commented Sep 12, 2024

czechboy0 left a comment

Generate enums for server variables #618

Are you sure you want to change the base?

Generate enums for server variables #618

Conversation

theoriginalbit commented Sep 7, 2024 • edited Loading

Motivation

Modifications

Result

Result

Test Plan

simonjbeaumont commented Sep 7, 2024

czechboy0 commented Sep 8, 2024

theoriginalbit commented Sep 8, 2024 • edited Loading

simonjbeaumont commented Sep 9, 2024

simonjbeaumont commented Sep 9, 2024

theoriginalbit commented Sep 9, 2024

theoriginalbit commented Sep 12, 2024

czechboy0 left a comment

Choose a reason for hiding this comment

theoriginalbit commented Sep 7, 2024 •

edited

Loading

theoriginalbit commented Sep 8, 2024 •

edited

Loading