Fix isenum for referenced enums #1

LukeMarlin · 2020-07-03T20:35:43Z

Fixes OpenAPITools#5676
Referenced enums are now properly marked as isEnum = true

Had to fix kotlin spring generator. Tests go through as before, and I doubled checked that current code gives me the exact same models output for the sample that was related to this topic.

PR checklist

Read the contribution guidelines.
If contributing template-only or documentation-only changes which will change sample output, build the project beforehand.
Run the shell script ./bin/generate-samples.shto update all Petstore samples related to your fix. This is important, as CI jobs will verify all generator outputs of your HEAD commit as it would merge with master. These must match the expectations made by your contribution. You may regenerate an individual generator by passing the relevant config(s) as an argument to the script, for example ./bin/generate-samples.sh bin/config/java*. For Windows users, please run the script in Git BASH.
File the PR against the correct branch: master
Copy the technical committee to review the pull request if your PR is targeting a particular programming language.

While this is not targeting a specific generator, it did affect a kotlin spring test, so I'm summoning its technical comitee as my code in these files might not be OK: @jimschubert, @dr4ke616, @karismann, @Zomzog, @andrewemery, @4brunu, @yutaka0m

It also failed samples generation for haskell, so I patched it with a quick if, that should definitely be checked by the team: @jonschoning

Fixes OpenAPITools#5676 Referenced enums are now properly marked as isEnum = true

jimschubert

I'm running some tests locally, but left a couple comments.

There are a lot of changes to how enums are processed by other languages, which I'd need to understand better as it's considered a breaking change for these.

jimschubert · 2020-07-04T12:07:23Z

...pi-generator/src/main/java/org/openapitools/codegen/languages/KotlinSpringServerCodegen.java

+                for (CodegenProperty prop : model.allVars) {
+                    //If a property is an enum and a primitive type, it is an embedded enum
+                    if (prop.isEnum && prop.isPrimitiveType) {
+                        model.imports.add("JsonValue");


This will have already occurred on line 519.

Not what I witnessed: if the model itself is not an enum, but has variables that are enums, then kotlin generator need to have that import.

... Now that I reread, I see it's hasEnums so you should be right. Two possibilities: my code here changed nothing so I'll remove it, or hasEnumwas false and I need to check out why. I'll try to do that on monday and report back

@jimschubert Just took a look at it again, line 519 adds JsonProperty if a model is not an enum, because it will have properties.
My code replaces the previous line 521 that was adding JsonValue whenever a model has enums by a code that adds it only if one of the enum is embedded, as what was done previously by that generator. I did it to have the same result as master, although the JsonValue import doesn't seem to be used at all...

jimschubert · 2020-07-04T12:10:18Z

modules/openapi-generator/src/main/resources/kotlin-spring/dataClass.mustache

@@ -20,7 +20,7 @@
        {{>interfaceOptVar}}
    {{/optionalVars}}
 {{/discriminator}}
-{{#hasEnums}}{{#vars}}{{#isEnum}}
+{{#hasEnums}}{{#vars}}{{#isEnum}}{{#isPrimitiveType}}


Interesting. I started looking at this a bit after our Slack conversation, but hadn't thought about how isPrimitiveType might fit into things.

One issue with considering enums as primitive is that $ref doesn't mean "This is a class/model", it means "I want to reuse this bit of schema in more than one place". So, if an enum which is inline is a primitive type, an enum that is $ref should also be a primitive type.

I'll have to think about this a little more and maybe discuss with the core team.

Yes, the kotlin test allowed me to notice that an inline enum is basically this in core:

isEnum = True

isPrimitive = True

isString = True

But when you have a referenced enum (with my change), you get:

isEnum = True

and that's it.

So relying on isPrimitive would be the only way for generators to distinguish between the two cases (as far as I've noticed).
It does seem a bit brittle though. I guess isString might be ok, but I'm unsure it would cover all cases.

jonschoning · 2020-07-04T15:18:22Z

modules/openapi-generator/src/main/java/org/openapitools/codegen/DefaultCodegen.java

@@ -5257,6 +5258,9 @@ public void setParameterBooleanFlagWithCodegenProperty(CodegenParameter paramete
     * @param var list of CodegenProperty
     */
    public void updateCodegenPropertyEnum(CodegenProperty var) {
+        if (!var.isPrimitiveType) {
+            return;


What is the rationale for returning early here?

This prevents the allowableValues/EnumVars from being constructed at all, which I think you'd want to do if it's an Enum in any way.

Ah sorry, I should have removed the summoning from description. I gave a bit more context to Jim about that base PR.
first is the actual intended change, the second is a lame attempt at fixing all the changes that result from it. So there is no rationale behind this change, I was just trying stuff out.

So yes, you are absolutely right here. My bad!

jonschoning · 2020-07-04T15:19:38Z

samples/client/petstore/haskell-http-client/lib/OpenAPIPetstore/Model.hs

-  | E'EnumString'Lower -- ^ @"lower"@
-  | E'EnumString'Empty -- ^ @""@
-  deriving (P.Show, P.Eq, P.Typeable, P.Ord, P.Bounded, P.Enum)
+  = 


the generated enum properties are missing here now

This reverts commit 2430619.

LukeMarlin · 2020-07-08T09:59:39Z

Reverted the commit that was a lame attempt at keeping backward compatibility, it was just noise to the matter at hand.

jimschubert · 2020-08-18T01:36:14Z

I wonder if in the 14 places where we add + "Enum" to enum names, whether we should do this only if the text doesn't end in Enum already. Maybe @wing328 will have a thought on it.

There are a ton of changes in this PR that change previous OuterEnum to OuterEnumEnum, which isn't ideal.

There are times when an enum may be a $ref just for reusability and the user may not want it to be generated as a model. I think in those cases people may need to change from components/schema to another yaml/json node which doesn't have a specific meaning in the OAI spec.

LukeMarlin · 2020-08-19T08:34:59Z

I wonder if in the 14 places where we add + "Enum" to enum names, whether we should do this only if the text doesn't end in Enum already. Maybe @wing328 will have a thought on it.

I could add it if it's needed, I think it makes sense.

There are times when an enum may be a $ref just for reusability and the user may not want it to be generated as a model. I think in those cases people may need to change from components/schema to another yaml/json node which doesn't have a specific meaning in the OAI spec.

Unsure whether I understand, TBH :)

We'd "preserve" the old behaviour by telling people to move their enum to a non openapi section if they want to avoid having a model generated (due to now's true value of isEnum)? That's still quite a breaking change right?

jimschubert · 2020-08-19T12:35:13Z

I think it's correct to always generate a top-level type for $ref enum when supported by the generator.

It would be better if swagger-parser supported yaml fully and allowed anchors and node references, then we wouldn't need to worry about the edge case of people using $ref for reuse rather than model definition.

jimschubert · 2020-09-06T12:45:22Z

@LukeMarlin I couldn't tell if you were continuing to work on this or not.

Whenever you're ready to contribute this upstream, please open the PR against openapi-generator repo (rather than your fork).

LukeMarlin · 2020-09-07T14:45:18Z

Hi @jimschubert ! I didn't do anything new on that PR because I have no idea in which direction I should go.
This is a breaking change, and based on last comments I couldn't tell if it was a desirable one or not.

Considering only the first commit, do you think I should open the PR with it, or do you see changes to be made first?

wing328 · 2022-12-28T08:40:02Z

closed via OpenAPITools#13880

LukeMarlin added 2 commits July 3, 2020 21:28

Fix isEnum for referenced enums

d11d6b7

Fixes OpenAPITools#5676 Referenced enums are now properly marked as isEnum = true

Keep backward compatible behaviour

2430619

jimschubert reviewed Jul 4, 2020

View reviewed changes

jonschoning reviewed Jul 4, 2020

View reviewed changes

Revert "Keep backward compatible behaviour"

3b6fc9a

This reverts commit 2430619.

rivancic mentioned this pull request Aug 19, 2022

Fix isEnum for referenced enums OpenAPITools/openapi-generator#9526

Closed

5 tasks

wing328 mentioned this pull request Nov 2, 2022

Add isEnumRef, isEnumOrRef to CodegenProperty OpenAPITools/openapi-generator#13880

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix isenum for referenced enums #1

Fix isenum for referenced enums #1

LukeMarlin commented Jul 3, 2020

jimschubert left a comment

jimschubert Jul 4, 2020

LukeMarlin Jul 4, 2020

LukeMarlin Jul 8, 2020

jimschubert Jul 4, 2020

LukeMarlin Jul 4, 2020 •

edited

Loading

jonschoning Jul 4, 2020

LukeMarlin Jul 4, 2020

jonschoning Jul 4, 2020

LukeMarlin commented Jul 8, 2020

jimschubert commented Aug 18, 2020

LukeMarlin commented Aug 19, 2020

jimschubert commented Aug 19, 2020

jimschubert commented Sep 6, 2020

LukeMarlin commented Sep 7, 2020

wing328 commented Dec 28, 2022

Fix isenum for referenced enums #1

Are you sure you want to change the base?

Fix isenum for referenced enums #1

Conversation

LukeMarlin commented Jul 3, 2020

PR checklist

jimschubert left a comment

Choose a reason for hiding this comment

jimschubert Jul 4, 2020

Choose a reason for hiding this comment

LukeMarlin Jul 4, 2020

Choose a reason for hiding this comment

LukeMarlin Jul 8, 2020

Choose a reason for hiding this comment

jimschubert Jul 4, 2020

Choose a reason for hiding this comment

LukeMarlin Jul 4, 2020 • edited Loading

Choose a reason for hiding this comment

jonschoning Jul 4, 2020

Choose a reason for hiding this comment

LukeMarlin Jul 4, 2020

Choose a reason for hiding this comment

jonschoning Jul 4, 2020

Choose a reason for hiding this comment

LukeMarlin commented Jul 8, 2020

jimschubert commented Aug 18, 2020

LukeMarlin commented Aug 19, 2020

jimschubert commented Aug 19, 2020

jimschubert commented Sep 6, 2020

LukeMarlin commented Sep 7, 2020

wing328 commented Dec 28, 2022

LukeMarlin Jul 4, 2020 •

edited

Loading