scala set and default values for set and array. change in array type #4872

fokot · 2019-12-27T10:50:16Z

Changes:

if uniqueItems: true then generate Set otherwise Seq
changed default implementation of Seq from ListBuffer to List
default value for set is Set.empy and for array List.empy

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.

@clasnake (2017/07), @jimschubert (2017/09) ❤️, @shijinkui (2018/01), @ramzimaalej (2018/03)

…r to List

fokot · 2019-12-27T12:15:00Z

I touched only Scala and there are failing tests in different languages which were failing even before. Is it ok?

jimschubert · 2019-12-27T12:25:15Z

...enapi-generator/src/main/java/org/openapitools/codegen/languages/ScalaAkkaClientCodegen.java

@@ -113,7 +113,7 @@ public ScalaAkkaClientCodegen() {
        typeMapping.put("binary", "File");
        typeMapping.put("number", "Double");

-        instantiationTypes.put("array", "ListBuffer");
+        instantiationTypes.put("array", "List");


This is a breaking change, and won't work because templates expect mutation.

When do we access generated array in templates? Do you mean tests? Or am I missing something?

@fokot "templates" here doesn't just refer to the built-in templates under resources. openapi-generator supports user customized templates, so changing the above will break expectations of anyone with customized templates who expect mutability. Sorry to not be more clear about that.

Also, instantiationTypes really only makes sense as something which can instantiate an object. You can see the difference in your test modifications, for example:

- Assert.assertEquals(property1.defaultValue, "new ListBuffer[String]() "); + Assert.assertEquals(property1.defaultValue, "List.empty ");

This might be used in a class. Now, rather than an immutable property reference with a mutable data structure:

prop: ListBuffer[String] = new ListBuffer[String]()

We have an immutable type:

prop: List[String] = List.empty

while I agree that the latter is more idiomatic, and you can just use the copy method as case classes intend, anyone who has previously generated against openapi-generator will have to change their client call sites or communicate the change to client SDK users.

If possible, could we instead rollback this change in this PR and open another PR against the 5.0 branch for this change? There is a workaround in 4.x to specify the instantiation types and type mappings manually for anyone who wants the functionality you've implemented here. Unfortunately, doing the right think now means introducing a breaking change for those using the template.

jimschubert · 2019-12-27T12:31:05Z

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

@@ -187,7 +187,9 @@ public String getTypeDeclaration(Schema p) {
        if (ModelUtils.isArraySchema(p)) {
            ArraySchema ap = (ArraySchema) p;
            Schema inner = ap.getItems();
-            return getSchemaType(p) + "[" + getTypeDeclaration(inner) + "]";
+            boolean isSet = ModelUtils.isSet(ap);


I feel like this should be done for List as well.

jimschubert · 2019-12-27T12:33:18Z

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

@@ -386,6 +386,11 @@ public static boolean isArraySchema(Schema schema) {
        return (schema instanceof ArraySchema);
    }

+    public static boolean isSet(ArraySchema schema) {


I think this needs to consider List as well.

jimschubert · 2019-12-27T12:36:41Z

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

@@ -76,8 +76,6 @@ public ScalazClientCodegen() {
        importMapping.remove("List");
        importMapping.remove("Set");
        importMapping.remove("Map");
-
-        importMapping.put("ListBuffer", "scala.collection.mutable.ListBuffer");


This is a ~~badass~~ breaking change and shouldn't be removed.

Edit: all your changes are badass, but I meant "breaking".

jimschubert · 2019-12-27T12:47:57Z

Thanks for the contribution. Don't worry about the failures for now, we can restart the jobs once master is green. Also... sorry for the individual comments on the review, but GitHub kept crashing Chrome and Firefox submitted then all as regular comments.

My main concern is the change from ListBuffer to List. The templates are written in a way that these almost always expect mutation, and List doesn't have an .add function. Once master is stable and we restart the jobs in this PR, we should see compilation errors in Scala samples because of this. I had a couple other questions, like whether we should be considering array and list for conversion to set (in one place, you use List.empy for array instantiation which isn't the same)?

wing328 · 2019-12-27T13:00:12Z

I'm looking into the CI failure in the master branch...

fokot · 2019-12-27T13:28:20Z

@jimschubert I changed the default values to List.empty as we see them as Seq (scala.collection.Seq) fields which are immutable. If we want mutation, templates should import scala.collection.mutable.Seq. Or is it meant, that people should cast in Scala?

jimschubert · 2019-12-27T18:40:39Z

I changed the default values to List.empty as we see them as Seq (scala.collection.Seq) fields which are immutable.

@fokot Seq and List are completely different, though (think List vs LinkedList in Java). Seq is also a trait which can represent non-List collections, while List is very limiting on data type to only types which implement Seq. Why not use Seq.empty?

fokot · 2019-12-27T23:00:59Z

@jimschubert now I know the misunderstanding. You must be using pre 2.13 Scala. They changed Seq to immutable.Seq in 2.13 https://github.com/scala/scala/releases/tag/v2.13.0

jimschubert · 2019-12-29T02:54:09Z

@fokot I'm confused by your comment. The things I've mentioned are:

We can't change from ListBuffer to List without breaking clients generated with openapi-generator, and should probably hold off until 5.0 for that change
Array generates as Seq, I'm not following why we use Seq/Set in some places, but the default instantiation is List.empty rather than Seq.empty (are you simply banking on List implements Seq here?)
Why do we convert only array with uniqueItems=true to a Set and not list? I think it's correct to apply this logic using ArraySchema, but what happens if someone sets a type mapping from array to list? I think ModelUtils.isSet would still return as expected but that some of the ternary conditions wouldn't return the target list type.

I'll try to take some time tomorrow and provide more details if needed. However, I don't think we can merge with the current ListBuffer change.

jimschubert · 2020-01-05T14:23:06Z

@fokot I included your contribution and made adjustments based on my comments above. I opened #4926, which has been merged into master. Please take a look and let me know if you any additional concerns that I might have missed in the above discussion.

fokot added 2 commits December 27, 2019 11:10

scala set and default values for set and array. change from ListBuffe…

c47da0d

…r to List

run generators from bin

87bbaab

jimschubert reviewed Dec 27, 2019

View reviewed changes

jimschubert mentioned this pull request Jan 4, 2020

[scala] Support for Set when array has uniqueItems=true #4926

Merged

5 tasks

jimschubert closed this in #4926 Jan 5, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

scala set and default values for set and array. change in array type #4872

scala set and default values for set and array. change in array type #4872

Uh oh!

fokot commented Dec 27, 2019 •

edited

Loading

Uh oh!

fokot commented Dec 27, 2019

Uh oh!

jimschubert Dec 27, 2019

Uh oh!

fokot Dec 27, 2019

Uh oh!

jimschubert Dec 27, 2019

Uh oh!

jimschubert Dec 27, 2019

Uh oh!

jimschubert Dec 27, 2019

Uh oh!

jimschubert Dec 27, 2019

Uh oh!

jimschubert commented Dec 27, 2019

Uh oh!

wing328 commented Dec 27, 2019

Uh oh!

fokot commented Dec 27, 2019

Uh oh!

jimschubert commented Dec 27, 2019

Uh oh!

fokot commented Dec 27, 2019

Uh oh!

jimschubert commented Dec 29, 2019

Uh oh!

jimschubert commented Jan 5, 2020

Uh oh!

Uh oh!

Uh oh!

scala set and default values for set and array. change in array type #4872

scala set and default values for set and array. change in array type #4872

Uh oh!

Conversation

fokot commented Dec 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR checklist

Uh oh!

fokot commented Dec 27, 2019

Uh oh!

jimschubert Dec 27, 2019

Choose a reason for hiding this comment

Uh oh!

fokot Dec 27, 2019

Choose a reason for hiding this comment

Uh oh!

jimschubert Dec 27, 2019

Choose a reason for hiding this comment

Uh oh!

jimschubert Dec 27, 2019

Choose a reason for hiding this comment

Uh oh!

jimschubert Dec 27, 2019

Choose a reason for hiding this comment

Uh oh!

jimschubert Dec 27, 2019

Choose a reason for hiding this comment

Uh oh!

jimschubert commented Dec 27, 2019

Uh oh!

wing328 commented Dec 27, 2019

Uh oh!

fokot commented Dec 27, 2019

Uh oh!

jimschubert commented Dec 27, 2019

Uh oh!

fokot commented Dec 27, 2019

Uh oh!

jimschubert commented Dec 29, 2019

Uh oh!

jimschubert commented Jan 5, 2020

Uh oh!

Uh oh!

fokot commented Dec 27, 2019 •

edited

Loading