Fix Union Types Import Issue #6789

FrankEssenberger · 2020-06-26T09:29:41Z

This is an attempt to fix the bug reported here: #5467
The problem was that the union types in typescrit e.g. TypeA | TypeB represented by the oneOf keyword in the service definition lead to not valid code:

import{TypeA | TypeB} from '../models'

you can either use a comma in the import or split into separate imports. I did the latter one because I thought this could be more solid for models in different files and paths potentially. If they are not all exported from the same path.

I adjusted it for three typescript generators:

axios (this I where I was effected)
node
angular

Then I stopped and thought it is a good point to start the discussion before I adjust it for all the other Typescript generators.

Since I am new I would highly appreciate some help regarding the following issues:

How to run the code formatter. When I run mvn formatter:format I get hundreds of code changes.
How to run the tests. Is mvn test fine?
Not sure about the locations where I put the tests and so on. Perhaps this kind of test has to go somewhere else.

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.

FrankEssenberger · 2020-06-26T09:31:33Z

@TiFu (2017/07) @taxpon (2017/07) @sebastianhaas (2017/07) @kenisteward (2017/07) @Vrolijkx (2017/09) @macjohnny (2018/01) @topce (2018/10) @akehir (2019/07) @petejohansonxo (2019/11) @amakhrov (2020/02)

...erator/src/main/java/org/openapitools/codegen/languages/AbstractTypeScriptClientCodegen.java

macjohnny · 2020-06-26T16:17:04Z

...erator/src/main/java/org/openapitools/codegen/languages/AbstractTypeScriptClientCodegen.java

+        return objsUnderProcess;
+    }
+
+    protected List<Object> splitUniontypeImports(List<Map<String,String>> imports){


@amakhrov @wing328 do you think such a complicated logic is necessary, or could this be solved elsewhere?

It looks quite similar to the existing logic in TypeScriptAngularClientCodegen (see postProcessAllModels and parseImports)

Maybe we should lift the logic from AngularClient to AbstractTypescriptClient instead of pretty much repeating it?

The root problem, as I see it, is that toModelImport takes a string as an argument rather than a model and returns a string instead of a list. It makes it very limited in terms of handling composite models. But changing that would require a change in Generator and CodegenConfig interfaces. And generally means more work, of course.

I totally agree on your analysis regarding the signature of the toModelImport(). I did not want to change that. I have also missed the implementation already present in the AngularClient. I will try to move everything up to the parent class.

macjohnny · 2020-06-29T13:06:38Z

@FrankEssenberger thanks for your contribution!

FrankEssenberger · 2020-07-01T13:06:40Z

I adjusted my approach and created a second method Map<String,String> toModelImportMap(String input) which will give the map between the fully quantified model for import and the initial name. For most cases this map will have only a single pair. However, for the typescript case if a type like "classA | classB" is found it will have two entries.

This seemed much cleaner then to fiddle around with the imports in a post processing step.

FrankEssenberger · 2020-07-01T13:11:53Z

I would like to run the formatter so that the diff looks better. Please let me know how to execute this and I am also happy for further feedback.

wing328 · 2020-07-02T07:40:55Z

cc @OpenAPITools/generator-core-team as the change impacts default codegen, generator

…rator

TiFu

Just a brief question about this PR.

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

macjohnny · 2020-07-10T07:34:50Z

@FrankEssenberger could you please merge the current master and re-generate the samples?

FrankEssenberger · 2020-07-10T13:58:43Z

I have done a ./bin/generate-samples.sh and merged master. However, there were a lot of code changes in the samples which I commited. Not sure if this is correct?

macjohnny · 2020-07-10T14:10:11Z

@FrankEssenberger did you

merge master
mvn clean package
re-generate the samples
?

FrankEssenberger · 2020-07-10T16:29:42Z

yes this is what I did. If I do this on the not forked repo I do not get the changes in the samples. Also the changes are reordering of dependencies for example:

#include "ApiResponse.h"	
#include "HttpContent.h"	
#include "Pet.h"	#include "Pet.h"
#include <cpprest/details/basic_types.h>

changes to

#include "Pet.h"	#include "Pet.h"
#include <cpprest/details/basic_types.h>
#include "ApiResponse.h"	
#include "HttpContent.h"

For whatever reason. Is there a formatter task present?

macjohnny · 2020-07-10T16:38:07Z

Could it be your IDE that performs code cleanup when committing?

FrankEssenberger · 2020-07-10T16:39:21Z

ah, I just realized that with me map introduction I changed the order of imports. Most likely the hashmap has not a alphabetic ordering when I loop over the keys. I will fix this that the imports are order alphabetically.

amakhrov · 2020-08-04T03:12:45Z

...erator/src/main/java/org/openapitools/codegen/languages/AbstractTypeScriptClientCodegen.java

+            return  result;
+        }
+        return Collections.singletonMap(toModelImport(name),name);
+    }


These two methods (toModelImportMap and toModelImport) look very similar (split by |, remove spaces, process each part). Should one method just delegate to the other? E.g. toModelImport could call toModelImportMap and then collect the resulting map back into a string.

sure good point. I introduced small helper methods with a more descriptive name and used the map method inside the single entry one.

FrankEssenberger · 2020-08-04T12:53:00Z

The pipeline seems a bit flaky. I see in the ci build the error:

[error] sbt.librarymanagement.ResolveException: Error downloading com.softwaremill.sttp.client:json4s_2.13:2.2.0
[error]   Not found
[error]   Not found
[error]   not found: /home/circleci/.ivy2/local/com.softwaremill.sttp.client/json4s_2.13/2.2.0/ivys/ivy.xml
[error]   download error: Caught java.net.ConnectException: Connection timed out (Connection timed out) (Connection timed out (Connection timed out)) while downloading https://repo1.maven.org/maven2/com/softwaremill/sttp/client/json4s_2.13/2.2.0/json4s_2.13-2.2.0.pom
[error] 	at lmcoursier.CoursierDependencyResolution.unresolvedWarningOrThrow(CoursierDependencyResolution.scala:249)

FrankEssenberger · 2020-08-04T13:05:57Z

And for the travis-ci I see the errorr:

 1) Isomorphic Fetch
       GET-Request:
     Error: Timeout of 10000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves. (/home/travis/build/OpenAPITools/openapi-generator/samples/openapi3/client/petstore/typescript/tests/default/test/http/isomorphic-fetch.test.ts)

However, if I run the test locally it runs through so perhaps the timeout is reached for some strange reason in the pipeline.

macjohnny · 2020-08-04T13:15:17Z

@FrankEssenberger I re-started the CI builds.

FrankEssenberger · 2020-08-04T13:50:38Z

Cool, your restart helped. You have the magic green finger :-).

macjohnny · 2020-08-04T13:57:29Z

cc @OpenAPITools/generator-core-team for final approval, as the change impacts default codegen, generator

FrankEssenberger · 2020-08-11T16:53:02Z

merged master. Do you need something from my side?

macjohnny · 2020-08-12T06:16:52Z

@FrankEssenberger we need to wait for a final review a a member of the @OpenAPITools/generator-core-team

wing328 · 2020-08-12T08:01:28Z

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

@@ -1115,6 +1099,40 @@ private static String generateParameterId(Parameter parameter) {
        return operations;
    }

+    private Map<String,String> getAllImportsMapppings(Set<String> allImports){


Can you please add docstrings to explain what these new (private) functions do (e.g. avoid duplicates)?

We want to document all methods in the default codegen/generator class to make code easier to understand for new contributors. (I do not expect you to document all methods missing the docstring. The community can help with separate PRs)

I added some doc and used a Set to better represent that the entries are unique for the other method.

macjohnny · 2020-08-17T09:17:10Z

@wing328 can we merge this?

felschr · 2020-08-19T17:37:29Z

typescript-fetch has the same issue. Would be nice to fix it as well.
UPDATE: Looking at the changes it seems that it should already affect all typescript generators. Never mind then :)

FrankEssenberger · 2020-08-20T07:09:05Z

Yes, I put the change in the AbstractTypeScriptClientCodegen.java which should be the parent for all typescript generators I guess. In the test I piked three axios, node and angular and for all the union types were correctly handled.

macjohnny · 2020-08-20T07:13:44Z

@FrankEssenberger thanks again for your contribution.
I hope we can merge it soon

wing328

LGTM. Thanks for the PR.

* master: (720 commits) [docs] Update README badges (#7276) Update apiInvoker.mustache and sample file for akka-scala client for issue #7258 fix (#7259) [Dart] Get all enum values in a list (#7166) Update .gitattributes [ci] Set ubuntu workflow verification to autoclrf=true, safeclrf=false Update check-supported-versions.yaml [ci] Update gitattributes and allow skipping docs generation for Windows CI workflows (#7273) [core][bug] FILES is now path relative with no prefixes (#7271) Update check-supported-versions.yaml Update check-supported-versions.yaml (#7268) [Java][jersey2] Add jersey injection dependencies (#7240) [C][Clang Static Analyzer] Remove the useless variable when assembling URL (#7255) Date format dart (#6389) minor enhancement to java client generator (#7253) typescript: Fix Union Types Import Issue (#6789) Modifying the es5 and es6 templates for javascript to handle default values (#6649) [python-exp] simplify examples (#7157) Support for KumuluzEE microprofile runtime (#5944) [C#][netcore] minor improvements and bug fixes (#7244) Deprecate Flash (ActionScript) client generator (#7231) ...

* master: (219 commits) [java] Appropriate instantiation of model with dynamic properties (OpenAPITools#6052) [docs] Update README badges (OpenAPITools#7276) Update apiInvoker.mustache and sample file for akka-scala client for issue OpenAPITools#7258 fix (OpenAPITools#7259) [Dart] Get all enum values in a list (OpenAPITools#7166) Update .gitattributes [ci] Set ubuntu workflow verification to autoclrf=true, safeclrf=false Update check-supported-versions.yaml [ci] Update gitattributes and allow skipping docs generation for Windows CI workflows (OpenAPITools#7273) [core][bug] FILES is now path relative with no prefixes (OpenAPITools#7271) Update check-supported-versions.yaml Update check-supported-versions.yaml (OpenAPITools#7268) [Java][jersey2] Add jersey injection dependencies (OpenAPITools#7240) [C][Clang Static Analyzer] Remove the useless variable when assembling URL (OpenAPITools#7255) Date format dart (OpenAPITools#6389) minor enhancement to java client generator (OpenAPITools#7253) typescript: Fix Union Types Import Issue (OpenAPITools#6789) Modifying the es5 and es6 templates for javascript to handle default values (OpenAPITools#6649) [python-exp] simplify examples (OpenAPITools#7157) Support for KumuluzEE microprofile runtime (OpenAPITools#5944) [C#][netcore] minor improvements and bug fixes (OpenAPITools#7244) ...

FrankEssenberger added 2 commits June 26, 2020 11:17

First approach for discussion

54a529c

typo

234d2bd

macjohnny reviewed Jun 26, 2020

View reviewed changes

FrankEssenberger added 2 commits July 1, 2020 14:55

add addiotional method

74f3286

polish a bit

598ec7b

remove call of super method

57e9f2e

fix javadoc error

a5f5021

macjohnny requested review from jimschubert and wing328 July 1, 2020 13:50

Merge branch 'master' of https://github.com/OpenAPITools/openapi-gene…

5f12d9a

…rator

TiFu reviewed Jul 2, 2020

View reviewed changes

modules/openapi-generator/src/main/java/org/openapitools/codegen/DefaultGenerator.java Outdated Show resolved Hide resolved

FrankEssenberger added 3 commits July 3, 2020 09:08

com.google.common.collect.

53d6b3c

Merge remote-tracking branch 'upstream/master'

d0f83e3

Merge remote-tracking branch 'upstream/master'

8f4d3b7

FrankEssenberger added 2 commits July 10, 2020 15:17

Merge remote-tracking branch 'upstream/master'

2529f40

merge master regenerate samples

cbb8701

FrankEssenberger added 3 commits July 10, 2020 20:18

sort imports alphabetically

b3fc17e

sort imports alphabetically with right key

bcfdc66

typo

51b911c

amakhrov reviewed Aug 4, 2020

View reviewed changes

FrankEssenberger added 5 commits August 4, 2020 09:36

remark amakhrov

1aff13b

Merge remote-tracking branch 'upstream/master'

88ded7d

check in one generated file to test

1fb99be

adjust call super

24dc8ce

Merge remote-tracking branch 'upstream/master'

c83d6b4

Merge remote-tracking branch 'upstream/master'

5f376de

wing328 reviewed Aug 12, 2020

View reviewed changes

add comment use set.

d076160

wing328 approved these changes Aug 20, 2020

View reviewed changes

wing328 added Client: TypeScript Issue: Bug labels Aug 20, 2020

macjohnny merged commit d3017ff into OpenAPITools:master Aug 20, 2020

macjohnny mentioned this pull request Aug 20, 2020

[BUG][typescript-angular] generator generates invalid TypeScript for polymorphic types #5467

Closed

6 tasks

macjohnny added this to the 5.0.0 milestone Aug 20, 2020

felschr mentioned this pull request Aug 20, 2020

[BUG] Generation fails "the allOf schemas must use $ref" #7262

Closed

6 tasks

felschr mentioned this pull request Sep 7, 2020

[BUG][typescript-fetch] Further union issues #7367

Open

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Union Types Import Issue #6789

Fix Union Types Import Issue #6789

FrankEssenberger commented Jun 26, 2020 •

edited

Loading

FrankEssenberger commented Jun 26, 2020

macjohnny Jun 26, 2020

amakhrov Jun 26, 2020

FrankEssenberger Jun 29, 2020

macjohnny commented Jun 29, 2020

FrankEssenberger commented Jul 1, 2020

FrankEssenberger commented Jul 1, 2020

wing328 commented Jul 2, 2020

TiFu left a comment

macjohnny commented Jul 10, 2020

FrankEssenberger commented Jul 10, 2020

macjohnny commented Jul 10, 2020

FrankEssenberger commented Jul 10, 2020

macjohnny commented Jul 10, 2020

FrankEssenberger commented Jul 10, 2020

amakhrov Aug 4, 2020

FrankEssenberger Aug 4, 2020

FrankEssenberger commented Aug 4, 2020

FrankEssenberger commented Aug 4, 2020

macjohnny commented Aug 4, 2020

FrankEssenberger commented Aug 4, 2020

macjohnny commented Aug 4, 2020

FrankEssenberger commented Aug 11, 2020

macjohnny commented Aug 12, 2020

wing328 Aug 12, 2020

FrankEssenberger Aug 13, 2020

macjohnny commented Aug 17, 2020

felschr commented Aug 19, 2020 •

edited

Loading

FrankEssenberger commented Aug 20, 2020

macjohnny commented Aug 20, 2020

wing328 left a comment

Fix Union Types Import Issue #6789

Fix Union Types Import Issue #6789

Conversation

FrankEssenberger commented Jun 26, 2020 • edited Loading

PR checklist

FrankEssenberger commented Jun 26, 2020

macjohnny Jun 26, 2020

Choose a reason for hiding this comment

amakhrov Jun 26, 2020

Choose a reason for hiding this comment

FrankEssenberger Jun 29, 2020

Choose a reason for hiding this comment

macjohnny commented Jun 29, 2020

FrankEssenberger commented Jul 1, 2020

FrankEssenberger commented Jul 1, 2020

wing328 commented Jul 2, 2020

TiFu left a comment

Choose a reason for hiding this comment

macjohnny commented Jul 10, 2020

FrankEssenberger commented Jul 10, 2020

macjohnny commented Jul 10, 2020

FrankEssenberger commented Jul 10, 2020

macjohnny commented Jul 10, 2020

FrankEssenberger commented Jul 10, 2020

amakhrov Aug 4, 2020

Choose a reason for hiding this comment

FrankEssenberger Aug 4, 2020

Choose a reason for hiding this comment

FrankEssenberger commented Aug 4, 2020

FrankEssenberger commented Aug 4, 2020

macjohnny commented Aug 4, 2020

FrankEssenberger commented Aug 4, 2020

macjohnny commented Aug 4, 2020

FrankEssenberger commented Aug 11, 2020

macjohnny commented Aug 12, 2020

wing328 Aug 12, 2020

Choose a reason for hiding this comment

FrankEssenberger Aug 13, 2020

Choose a reason for hiding this comment

macjohnny commented Aug 17, 2020

felschr commented Aug 19, 2020 • edited Loading

FrankEssenberger commented Aug 20, 2020

macjohnny commented Aug 20, 2020

wing328 left a comment

Choose a reason for hiding this comment

FrankEssenberger commented Jun 26, 2020 •

edited

Loading

felschr commented Aug 19, 2020 •

edited

Loading