Add swift5 option for generating frozen enums by jarrodparkes · Pull Request #11013 · OpenAPITools/openapi-generator

jarrodparkes · 2021-12-01T19:26:16Z

Attempts to solve #1529

This PR adds a new swift5 option for generateFrozenEnums (default: true). If set to false, then qualifying enums (those with raw type of String or Int), will include case unknownDefault and be "non-frozen" — able to "support" cases not explicitly specified in the spec.

PR checklist

Read the contribution guidelines.
If contributing template-only or documentation-only changes which will change sample output, build the project before.
Run the shell script(s) under ./bin/ (or Windows batch scripts under.\bin\windows) to update Petstore samples related to your fix. This is important, as CI jobs will verify all generator outputs of your HEAD commit, and these must match the expectations made by your contribution. You only need to run ./bin/{LANG}-petstore.sh, ./bin/openapi3/{LANG}-petstore.sh if updating the code or mustache templates for a language ({LANG}) (e.g. php, ruby, python, etc).
File the PR against the correct branch: master, 4.3.x, 5.0.x. Default: master.
Copy the technical committee to review the pull request if your PR is targeting a particular programming language.

Core Team Members

@jgavris
@ehyche
@Edubits
@jaz-ah
@4brunu

4brunu · 2021-12-01T19:41:01Z

modules/openapi-generator/src/main/resources/swift5/modelEnum.mustache

 {{/allowableValues}}
+{{^generateFrozenEnums}}
+{{#vendorExtensions.x-non-frozen-enum-capable}}
+    case unknown


@jarrodparkes I'm afraid this will conflict with an Enum case that is already unknown, for example, in my project some of the Enum's already have a unknown case.
Here are some alternative names:

unknownDefault (@unknown default)

openApiUnknown

{projectName}Unknown

Do you have other suggestions?

@4brunu good call. of your options, I like unknownDefault personally

@4brunu updated

Sorry for didn't put more thought into it, but since I discover that the raw value name is the case name, maybe we should name the enum something a bit more complex to be extra sure that we avoid conflicts.

What do you think of the following suggestions?

{projectName}UnknownDefault

unknownDefault{projectName}

openApiUnknownDefault

unknownDefaultOpenApi

Do you have other suggestions?

I also like the unknownDefault because of the pattern in Swift @unknown default, but maybe we should try harder to avoid conflicts?

Yeh, probably so. I like unknownDefaultOpenApi. And now that I think about it, enum cases that are multiword strings, normally use snake_case. With that in mind, I can also set String raw value to "unknown_default_open_api"

Looks good to me 👍

4brunu · 2021-12-01T19:41:12Z

modules/openapi-generator/src/main/resources/swift5/modelInlineEnumDeclaration.mustache

        {{/allowableValues}}
+        {{^generateFrozenEnums}}
+        {{#vendorExtensions.x-non-frozen-enum-capable}}
+        case unknown


@jarrodparkes same here

4brunu · 2021-12-01T19:56:10Z

Here are some experiences that I was doing

public enum EnumString: String, Codable, CaseIterable {
    case upper = "UPPER"
    case lower = "lower"
    case empty = ""
    case unknown1 = "unknown"
    case unknown // error: Raw value for enum case is not unique - the default case is the name of the enum
}
public enum EnumStringRequired: String, Codable, CaseIterable {
    case upper = "UPPER"
    case lower = "lower"
    case empty = ""
    case unknown1 = "unknown"
    case unknown // error: Raw value for enum case is not unique - the default case is the name of the enum
}
public enum EnumInteger: Int, Codable, CaseIterable {
    case _1 = 1
    case number1 = -1
    case number0 = 0
    case unknown // error: Raw value for enum case is not unique - the default value for Int is 0
}

jarrodparkes · 2021-12-01T20:03:02Z

^ @4brunu the updated unknownDefault should solve those problems for EnumString and EnumStringRequired, yeh?

As for EnumInteger, I thought the compiler would pick back up and assigned unknown/unknownDefault the rawValue of 2, but that doesn't seem to be the case. Not sure what would be the best practice here.

We could make Int types non-qualifying for non-frozen?
We could use a highly unlikely rawValue? -1529 (this issue's number 😉) or 192 (the Swift Evolution issue number for frozen/non-frozen enums)

4brunu · 2021-12-01T20:05:39Z

What about something like Int.max? Or to make it more random Int.max/192?

jarrodparkes · 2021-12-01T20:11:55Z

@4brunu It has to be a literal value. Int.max isn't a literal. Maybe something like -152915291529?

4brunu · 2021-12-01T20:18:48Z

.../openapi-generator/src/main/java/org/openapitools/codegen/languages/Swift5ClientCodegen.java

+
+        if (property.isEnum) {
+            // A non-frozen (with the "unknownDefault" case) enum is only possible with String or Int raw types.
+            if (property.dataType.equals("String") || property.dataType.equals("Int")) {


Since now we are going to assign values to the enums, can we remove this?

Maybe. Let me push the next update. I think we'll want to know the enum's type in order to assign a default value that is correct.

Is there any enum type that we don't support?
I think there is an easier way to do this, by removing this block of code and do this type check in the mustache files.
I think there are checks like isString, isInt, isDouble, isNumber, but I need to check tomorrow, because I'm not 100% sure.

@4brunu You are correct. I had tried this earlier, but was using the mustache clauses incorrectly. I made the change to remove the custom vendor extensions and use the built-in type checks. I think we should be good now

4brunu · 2021-12-01T20:23:38Z

That number doesn't fit inside the Int range.
Maybe -11184809 that is Int.min/192?

jarrodparkes · 2021-12-01T20:27:50Z

@4brunu alright, take a peek now 👀 and let me know what you think

jarrodparkes · 2021-12-01T22:01:04Z

Fix #1529

4brunu · 2021-12-01T22:13:15Z

modules/openapi-generator/src/main/resources/swift5/Protocols.mustache

+// https://openapi-generator.tech
+//
+
+/// An enum where the last case value can be used as a default catch-all.


There are some protocols already defined in the models.mustache files, I think, so I'm not sure if we should create a new file or put this one with the others that already exist.
Although this is not a very important topic, and it's subjective, but I'm just trying to keep things simple and coherent.

Here is an example

openapi-generator/modules/openapi-generator/src/main/resources/swift5/Models.mustache

Lines 9 to 15 in 9280e55

protocol JSONEncodable {

func encodeToJSON() -> Any

}

extension JSONEncodable {

func encodeToJSON() -> Any { self }

}

that's fair — I wasn't really sure what was recommended or preferred. I don't have a preference, so I'm fine to move it — will take care of shortly

4brunu · 2021-12-01T22:28:17Z

modules/openapi-generator/src/main/resources/swift5/modelInlineEnumDeclaration.mustache

        {{/allowableValues}}
+        {{^generateFrozenEnums}}
+        {{#vendorExtensions.x-non-frozen-enum-capable}}
+        case unknownDefaultOpenApi{{#vendorExtensions.x-non-frozen-enum-int}} = -11184809 // -(Int.min / 192){{/vendorExtensions.x-non-frozen-enum-int}}{{#vendorExtensions.x-non-frozen-enum-string}} = "unknown_default_open_api"{{/vendorExtensions.x-non-frozen-enum-string}}


I like that you put the comment here, and I think we should expand it a bit so that it's explicite on why this case is here.
And could you please also add a comment on the case above please?
The comment could maybe be something like:

This enum case is a fallback generated by OpenAPI, in case the server adds new enum cases that are unknown by old versions of the client. The raw value of this case is a dummy and it tries to use a value that avoids collisions.

And if its a number, put the precious message and also add the following:

The formula used to generate it is Int.min/192 (the Swift Evolution issue number for frozen/non-frozen enums)

Note: English is not my main language, and this message ended up being very big, so fell free to improve it, shorten it or write a completely different one.
This is just something that I thought that may help the users of this library understand this new enum case.

Or maybe this is overkill and unnecessary?

What do you think?

I like it, will add shortly 😄

4brunu · 2021-12-02T09:49:19Z

modules/openapi-generator/src/main/resources/swift5/modelEnum.mustache

 {{/enumVars}}
 {{/allowableValues}}
+{{^generateFrozenEnums}}
+{{#isString}}


@jarrodparkes should we support more types? Maybe Double, Float, and more?

now that we are specifying the raw values, yes 😄 — will add shortly

4brunu · 2021-12-02T09:57:30Z

By the way, thanks for creating this PR, this is a big improvement on the Swift Generator 👍
The PR was very solid, with an option to enable this feature and it's disabled by default, so great work 🙂

4brunu · 2021-12-02T14:07:23Z

I forgot one thing, since you added a new sample project, please make the following changes:

add your project to the spm build list

openapi-generator/samples/client/petstore/swift5/swift5_test_all.sh

Line 16 in 9280e55

# spm build
copy those two files to the sample project directory
1 - https://github.com/OpenAPITools/openapi-generator/blob/9280e55888e1ed56753ce475095bc25eed6a5ada/samples/client/petstore/swift5/deprecated/pom.xml
2 - https://github.com/OpenAPITools/openapi-generator/blob/9280e55888e1ed56753ce475095bc25eed6a5ada/samples/client/petstore/swift5/deprecated/run_spmbuild.sh

This will add the sample project to the CI pipeline.

jarrodparkes · 2021-12-02T14:39:06Z

@4brunu

I think that covers your suggestions from this morning. The one change I felt iffy about was the use of {{#isContainer}}{{/isContainer}} as an additional check in the modelInlineEnumDeclaration.mustache file. When testing locally, if I did not include that check, then certain enums (with String raw values) that were eligible for the .unknownDefaultOpenApi case would not receive it.

It was as-if some enums with String raw values were captured with the {{#isString}} check while others were captured with the {{#isContainer}} check. For example,

    // NOTICE: This was marked as {{#isString}}
    public enum JustSymbol: String, Codable, CaseIterable, CaseIterableDefaultsLast {
        case greaterThanOrEqualTo = ">="
        case dollar = "$"
        // This case is a catch-all generated by OpenAPI such that the enum is "non-frozen".
        // If new enum cases are added that are unknown to the spec/client, they are safely
        // decoded to this case. The raw value of this case is a dummy value that attempts
        // to avoids collisions with previously specified cases.
        case unknownDefaultOpenApi = "unknown_default_open_api"
    }
    // NOTICE: This was marked as {{#isContainer}}
    public enum ArrayEnum: String, Codable, CaseIterable, CaseIterableDefaultsLast {
        case fish = "fish"
        case crab = "crab"
        // This case is a catch-all generated by OpenAPI such that the enum is "non-frozen".
        // If new enum cases are added that are unknown to the spec/client, they are safely
        // decoded to this case. The raw value of this case is a dummy value that attempts
        // to avoids collisions with previously specified cases.
        case unknownDefaultOpenApi = "unknown_default_open_api"
    }

4brunu · 2021-12-02T14:41:21Z

Hum, I see, I don't have a better solution for that :/

jarrodparkes · 2021-12-02T14:45:50Z

@4brunu

I guess the greater question is...

Is using the {{#isContainer}}{{/isContainer}} correct? From my tests, I believe this to be true. I was hoping you might be able to confirm its correctness, and then if a better solution presents itself later, we could improve it.

4brunu

I think this is the last small nits

4brunu · 2021-12-02T14:45:03Z

modules/openapi-generator/src/main/resources/swift5/modelEnum.mustache

+    // decoded to this case. The raw value of this case is a dummy value that attempts
+    // to avoids collisions with previously specified cases.
+{{#isString}}
+    case unknownDefaultOpenApi = "unknown_default_open_api"


@jarrodparkes this is my fault, and sorry for that, but it should be unknownDefaultOpenAPI, of OpenAPI

4brunu · 2021-12-02T14:45:50Z

modules/openapi-generator/src/main/resources/swift5/modelEnum.mustache

+    //
+{{/isNumeric}}
+{{#isInteger}}
+    case unknownDefaultOpenApi = -11184809 // -(Int.min / 192)


@jarrodparkes typo here, the formula is Int.min / 192, and not -(Int.min / 192)

4brunu · 2021-12-02T14:47:01Z

modules/openapi-generator/src/main/resources/swift5/modelEnum.mustache

+    case unknownDefaultOpenApi = -11184809 // -(Int.min / 192)
+{{/isInteger}}
+{{#isDouble}}
+    case unknownDefaultOpenApi = -0.016362461737446838 // -(Double.pi / 192)


@jarrodparkes maybe it's a good idea to use always the same number -11184809 in all the cases?

4brunu · 2021-12-02T14:47:10Z

modules/openapi-generator/src/main/resources/swift5/modelEnum.mustache

+    case unknownDefaultOpenApi = -0.016362461737446838 // -(Double.pi / 192)
+{{/isDouble}}
+{{#isFloat}}
+    case unknownDefaultOpenApi = -0.01636246 // -(Float.pi / 192)


@jarrodparkes same here

4brunu · 2021-12-02T14:47:43Z

modules/openapi-generator/src/main/resources/swift5/modelInlineEnumDeclaration.mustache

        case {{{name}}} = {{{value}}}
        {{/enumVars}}
        {{/allowableValues}}
+        {{^generateFrozenEnums}}


@jarrodparkes please apply all the feedback from modelEnum.mustache also to this class

4brunu · 2021-12-02T14:54:13Z

@jarrodparkes looking at the modelInlineEnumDeclaration.mustache, looks to me that the use of {{#isContainer}}{{/isContainer}} is correct

openapi-generator/modules/openapi-generator/src/main/resources/swift5/modelInlineEnumDeclaration.mustache

Line 1 in 0e873a9

    
           {{#nonPublicApi}}internal{{/nonPublicApi}}{{^nonPublicApi}}public{{/nonPublicApi}} enum {{enumName}}: {{^isContainer}}{{dataType}}{{/isContainer}}{{#isContainer}}String{{/isContainer}}, {{#useVapor}}Content, Hashable{{/useVapor}}{{^useVapor}}Codable{{/useVapor}}, CaseIterable{{^generateFrozenEnums}}{{#isInteger}}, CaseIterableDefaultsLast{{/isInteger}}{{#isFloat}}, CaseIterableDefaultsLast{{/isFloat}}{{#isDouble}}, CaseIterableDefaultsLast{{/isDouble}}{{#isString}}, CaseIterableDefaultsLast{{/isString}}{{#isContainer}}, CaseIterableDefaultsLast{{/isContainer}}{{/generateFrozenEnums}} {

4brunu · 2021-12-02T14:59:45Z

modules/openapi-generator/src/main/resources/swift5/modelInlineEnumDeclaration.mustache

+        {{#isString}}
+        case unknownDefaultOpenApi = "unknown_default_open_api"
+        {{/isString}}
+        {{#isContainer}}


@jarrodparkes should this logic of the {{#isContainer}} be added to the file modelEnum.mustache?

@4brunu

I don't believe so — the only thing that gave me the hunch to use {{#isContainer}} on modelInlineEnumDeclaration.mustache was the fact that I already saw that clause used earlier in the file to help construct the enum's raw type. If you look at modelEnum.mustache, no such clause exists.

You are right 👍

jarrodparkes · 2021-12-02T15:03:47Z

@4brunu made the other changes 👍

4brunu

Looks good to me 👍
Thanks, this e a great improvement to the OpenAPI Swift Generator 🙂

4brunu · 2021-12-09T11:17:46Z

@jarrodparkes This PR was awesome, so I created #11078 that makes the enum unknown case available to all generators.
If you want to review it, you are welcome 👍

add swift5 option for generating frozen enums

0a6d4da

4brunu reviewed Dec 1, 2021

View reviewed changes

jarrodparkes added 2 commits December 1, 2021 13:51

use case unknownDefault to avoid conflicts

bd8cba2

update comments to reflect unknownDefault case

96a8b4b

4brunu reviewed Dec 1, 2021

View reviewed changes

set default values for unknown case to avoid conflict

1d6efb3

dont need vendor extensions to detect enum raw data type

9280e55

4brunu reviewed Dec 2, 2021

View reviewed changes

move CaseIterableDefaultsLast into models mustache template

2950d0d

jarrodparkes added 4 commits December 2, 2021 08:22

comment catch all case and add support for other types

219e3d5

add frozen enums to ci pipeline

8113ed9

remove extraneous edit to extensions template

869499e

remove left over protocols files

0e873a9

4brunu reviewed Dec 2, 2021

View reviewed changes

small comment and case adjustments

6eab5c3

4brunu approved these changes Dec 2, 2021

View reviewed changes

wing328 added Client: Swift Enhancement: Feature Feature: Enum labels Dec 4, 2021

wing328 added this to the 5.3.1 milestone Dec 4, 2021

wing328 merged commit 15f4571 into OpenAPITools:master Dec 4, 2021

jarrodparkes deleted the feat-1529/swift-frozen-enum-option branch December 4, 2021 11:27

This was referenced Dec 8, 2021

[REQ][Swift] Enum Cater for new enum Values e.g. Enum changes #10876

Closed

[Swift] Safer handling of enums #1529

Closed

[DefaultCodegen] generate unknown default case #11078

Merged

wing328 changed the title ~~add swift5 option for generating frozen enums~~ Add swift5 option for generating frozen enums Dec 19, 2021

	protocol JSONEncodable {
	func encodeToJSON() -> Any
	}

	extension JSONEncodable {
	func encodeToJSON() -> Any { self }
	}

Uh oh!

Conversation

jarrodparkes commented Dec 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR checklist

Core Team Members

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

4brunu commented Dec 1, 2021

Uh oh!

jarrodparkes commented Dec 1, 2021

Uh oh!

4brunu commented Dec 1, 2021

Uh oh!

jarrodparkes commented Dec 1, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

4brunu commented Dec 1, 2021

Uh oh!

jarrodparkes commented Dec 1, 2021

Uh oh!

jarrodparkes commented Dec 1, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

4brunu commented Dec 2, 2021

Uh oh!

4brunu commented Dec 2, 2021

Uh oh!

jarrodparkes commented Dec 2, 2021

Uh oh!

4brunu commented Dec 2, 2021

Uh oh!

jarrodparkes commented Dec 2, 2021

Uh oh!

4brunu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jarrodparkes commented Dec 1, 2021 •

edited

Loading