[Proposal] SOAR-0012: Generate enums for server variables

Server Development
1

Hi all,

The proposal SOAR-0012: Generate enums for server variables for Swift OpenAPI Generator is now up and In Review:

The review period will run until October 1st, 2024 - please post your feedback either as a reply to this thread, or on the pull request.

Thanks!

cc @theoriginalbit

4 Likes
2

First off, thanks @theoriginalbit for opening this proposal! :pray:

I'm supportive of anywhere we can provide greater compile-time guarantees for our adopters by e.g. using stronger-typed values for generated properties.

I also appreciate the consideration given to ensuring the backwards compatibility of the generated code, which, in this case, is achieved by this only being opt-in with a feature flag.

However, I'd love for us to explore ways we can make these sorts of improvements more readily discoverable and/or available to casual adopters. Especially for these such improvements which, if we were ever to mint a new major, would just be the default.

I'd love to open this up for discussion as I imagine there are a bunch of options here. and I'll start with some ideas of my own:

  1. Simply to surface these feature-flags more prominently in the documentation; maybe a recommended set of feature flags that represent the best practice: better than nothing, but won't be very visible.
  2. Generate both the more– and less–type-safe variants where we can preserve API backwards compatibility and...
    a. Generate an @available deprecation annotation on the less type-safe and use the message to point to the feature flag (also add a config option to suppress this).
    b. Generate a build time #warning for folks not using feature-flags that enable better safety (also add a config option to suppress this).
    c. Provide an option to opt-out of generating the old thing.

Turning our attention to this proposal specifically, would we be able to generate both the old version (with String parameters) and the new version (with enum parameters) as an overload? Potentially this requires the use of @_disfavoredOverload. I'm not completely sure it would work or that it's a good idea but I'd like to discuss the option here.

If not, could we consider generating the newly-proposed API with a new name if the feature flag isn't present and use the "normal" name if it is present?

To reiterate, I'm in favour of us moving to more strongly typed generated code and, while I don't want to sidetrack this proposal, it's the first of its kind since we committed to the stable generated API, so I'd like us to take this opportunity to do it well, and establish the right pattern going forward for other such changes—especially since there are requests for similar, e.g. generating typed properties for different format: options in the OAS.

3 Likes
3

Looking at the @ _disfavoredOverload approach it would work, though I'm unsure how/if its possible to disambiguate the function in the renamed: parameter so Xcode can offer a fixit.

enum Servers {
    enum Variables {
        enum Server1 {
            enum Environment {
                // ...
            }
        }
    }
    
    @available(*, deprecated, renamed: "???", message: "Consider upgrading to use the type-safe version.")
    @_disfavoredOverload static func server1(environment: Swift.String = "production") throws -> Foundation.URL {
        // ...
    }
    
    static func server1(environment: Variables.Server1.Environment = .default) throws -> Foundation.URL {
        // ...
    }
}

If this approach were to be taken then I believe it should remove the need for the feature flag as adopters that were previously using the default parameters effectively see no change, let url = try Servers.server1() just uses the new API, and other adopters receive deprecation warnings signalling a coming change. I do think this approach would be easier to maintain than the approach requiring new and "normal" names.

One thought I've now had, that would also allow the deprecation approach but remove the need for @ _disfavoredOverload, is a relocation of the static function. Currently the adopter would do let url = try Servers.server1() but the new API could be changed let url = try Servers.Server1.url() or let url = try Servers.Server1.serverURL() (with the function name of the latter option matching the parameter name on the generated Client type's initialiser.) The advantage to this approach is there would be no need for the disfavouring annotation and means the fixit doesn't require disambiguation either.

enum Servers {
    enum Server1 {
        enum Environment {
            // ...
        }
        
        static func url(environment: Environment = .default) throws -> Foundation.URL {
            // ...
        }
    }
    
    @available(*, deprecated, renamed: "Servers.Server1.url(environment:)", message: "Consider upgrading to use the type-safe version.")
    static func server1(environment: Swift.String = "production") throws -> Foundation.URL {
        // ...
    }
}

The above approach does also remove a level of nesting, no longer using the Variables namespace the proposal contains; which is also something the current proposal could consider doing anyway.

4

I like where this is going. I'm also interested in us exploring a way to not require the feature flag and get more users opted into this type-safe behavior by default while maintaining API stability.

I do think @_disfavoredOverload should work here, and I don't think we need the renamed: parameter - as the Fix-It wouldn't be able to mechanically fix the issue anyway for the adopter. If the adopter sees the deprecation warning, it's because they're explicitly passing in a String parameter in a place that now has an enum variant. They'll need to e.g. change "staging" to .staging in their code to switch to the new variant. I'm happy for the warning to guide them to it by being descriptive.

I also quite like the idea around having new namespaced methods, i.e. Servers.Server1.url() - it nicely namespaces the variables, but even without variables, I quite like how it reads. I'm curious what others think.

5

I don't mind us exploring relocation options and, if it results in a better generated API, then we should take that route. :slight_smile:

However, I don't want us to only do the relocation because it sidesteps the API evolution question because, while it might be acceptable here, there are plenty of cases where we will need a strategy to generate type-safe API where we currently don't.

I'd love for us to try and generalise a strategy for how we could do this without relocation, even if we don't take that approach for the server URLs.

6

I think relocation actually results in a better API here overall, but even the other strategy that was outlined above (generating both methods, one with @_disfavoredOverload) would have worked.

Coming up with a strategy for generating more type-safe APIs in other places might make sense to do with some of the other UUID/URL discussions we're having in the other issues on GitHub. Here we already seem to have a way to avoid the feature flag, which I believe was the main sticking point.

1 Like
7

Hm true, though I was thinking that the compile error should be pretty clear for the adopters that were providing strings, but for adopters using defaults the Fix-It makes changing super frictionless.

Genuinely was not my intention, was merely thinking of alternatives that could address the feature flag and in turn make adoption easier. At the time I didn’t have anything to contribute on the evolution side, though now I do so to put in my 2 cents, I think that ultimately there are multiple categories of evolution at play, and the two I immediately think of here are

  1. aims to improve API ergonomics and type-safety, like this proposal, and now that it’s brought up deprecation does seem like it would likely always be the easier general approach than a feature flag.
  2. aims to add conveniences to the API, so the likes of new format: options. Adopters can always do this themselves, but adding common ones (like say UUID and URL) becomes a convenience. I don’t think this can have the same deprecation approach, which lines it up best to feature flags.

As for the discovery of feature flags… my educated guess would be that casual adopters are the biggest hurdle and likely no one approach will work. Basic examples will be the first point of call and those exceed the scope of feature flags, except maybe to demo that it’s possible?
A documentation on “supported types” that also lists types behind feature flags is probably going to be the best bet on that front; I’m thinking of other tools like linters and formatters that have a doc that lists their features. In addition to that I do like the idea of a build warning from the generator when it is processing a format that isn’t supported, which could be extended to specify that it is supported behind a feature flag (perhaps even with a Fit-It to enable the flag in the config file?)

I agree, I do think it’s a nicer API than what I originally designed in the proposal. It provides a better purpose for the namespace.

Do we wait for others to weigh in first, or should I update the proposal to make the current approach a considered alternative? What’s the typical process of this swift-lite evo?