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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 7 commits into from
Oct 11, 2024
Merged

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

merged 7 commits into from
Oct 11, 2024

Conversation

theoriginalbit
Copy link
Contributor

Motivation

As requested by @czechboy0 in #618 I have created this proposal for community feedback.

Modifications

Added the proposal.

Also fixed a typo in the document for the proposal process.

Result

N/A

Test Plan

N/A

@czechboy0
Copy link
Contributor

Whoops, SOAR-0011 is already taken, please use SOAR-0012 :) #626

@czechboy0
Copy link
Contributor

Looks good overall, almost ready to go into review. Could you just slightly elaborate on the behavior with the feature flag enabled/disabled in the API stability section? Right now it sounds like it could be API breaking even for folks who don't opt-in, which isn't the case.

@theoriginalbit theoriginalbit force-pushed the proposal/generate-server-variable-enums branch from 9abfdfd to 663dbcd Compare September 18, 2024 22:53
@theoriginalbit
Copy link
Contributor Author

Whoops, SOAR-0011 is already taken, please use SOAR-0012 :) #626

Fixed :)

@theoriginalbit theoriginalbit changed the title [Proposal] SOAR-0011 Generate enums for server variables [Proposal] SOAR-0012 Generate enums for server variables Sep 18, 2024
@theoriginalbit
Copy link
Contributor Author

Looks good overall, almost ready to go into review. Could you just slightly elaborate on the behavior with the feature flag enabled/disabled in the API stability section? Right now it sounds like it could be API breaking even for folks who don't opt-in, which isn't the case.

Done :)

@theoriginalbit theoriginalbit mentioned this pull request Sep 18, 2024
Merged
czechboy0
czechboy0 requested changes Sep 19, 2024
View reviewed changes
Copy link
Contributor

@czechboy0 czechboy0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two more nits, after that I'm happy to send this into review.

Sources/swift-openapi-generator/Documentation.docc/Proposals/SOAR-0012.md Outdated Show resolved Hide resolved
Sources/swift-openapi-generator/Documentation.docc/Proposals/SOAR-0012.md Outdated Show resolved Hide resolved
@czechboy0 czechboy0 added the kind/proposal Proposals for review. label Sep 19, 2024
@czechboy0
Copy link
Contributor

This proposal is now In Review, forums thread: https://forums.swift.org/t/proposal-soar-0012-generate-enums-for-server-variables/74737

@theoriginalbit theoriginalbit force-pushed the proposal/generate-server-variable-enums branch from 8960a5a to 777db9c Compare October 1, 2024 10:42
czechboy0 added a commit that referenced this pull request Oct 11, 2024
@theoriginalbit @czechboy0
Generate enums for server variables (#618)
ef6d07f 
### Motivation

Refer to proposal
#629

<details>
<summary>PR description prior to raising proposal</summary>
### Motivation

Recently in a project I was using a spec which defined variables similar
to below
```yaml
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
```swift
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:
```swift
/// 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.

```swift
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:

```swift
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
```yaml
servers:
  - url: https://{environment}.example.com/api/
    variables:
      environment:
        default: prod
        enum:
          - prod
          - staging
          - dev
```
Would currently generate to the output
```swift
/// 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
```swift
/// 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
```swift
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).

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

Before this PR:
```swift
/// 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:
```swift
/// 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
                )
            ]
        )
    }
}
```

</details>

### Result

Refer to
#618 (comment)

### Test Plan

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

---------

Co-authored-by: Honza Dvorsky <[email protected]>
@czechboy0 czechboy0 enabled auto-merge (squash) October 11, 2024 18:08
@czechboy0 czechboy0 merged commit dffd9d1 into apple:main Oct 11, 2024
25 checks passed
@theoriginalbit theoriginalbit deleted the proposal/generate-server-variable-enums branch October 11, 2024 21:46
@czechboy0 czechboy0 added semver/none No version bump required. and removed kind/proposal Proposals for review. labels Oct 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@czechboy0 czechboy0 czechboy0 approved these changes

Assignees
No one assigned
Labels
semver/none No version bump required.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@theoriginalbit @czechboy0