Skip to content

Fix typescript generator for parameter collectionFormat for pipes ssv#5000

Closed
DavidBiesack wants to merge 22 commits intoOpenAPITools:masterfrom
DavidBiesack:bugfix/4999
Closed

Fix typescript generator for parameter collectionFormat for pipes ssv#5000
DavidBiesack wants to merge 22 commits intoOpenAPITools:masterfrom
DavidBiesack:bugfix/4999

Conversation

@DavidBiesack
Copy link
Contributor

Fix #4999 as described in that issue report.

PR checklist

  • [ x] Read the contribution guidelines.
  • [x ] If contributing template-only or documentation-only changes which will change sample output, build the project before.
  • [x ] 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).
  • [x ] File the PR against the correct branch: master, 4.3.x, 5.0.x. Default: master.
  • [ x] Copy the technical committee to review the pull request if your PR is targeting a particular programming language.

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

Copy link
Member

@macjohnny macjohnny left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should rather be changed in

export const COLLECTION_FORMATS = {
csv: ",",
ssv: " ",
tsv: "\t",
pipes: "|",
};

@DavidBiesack
Copy link
Contributor Author

@macjohnny I don't see how that fixes this; the definition of COLLECTION_FORMATS is already fine and matches the collectionFormat values in OpenAPI 2.0 , using pipes and ssv. Are you saying change this to use pipe and space instead? As I noted, the problem is in all typescript-* generators. so in addition to openapi-generator/src/main/resources/typescript-axios/baseApi.mustache, all these files need updating:

$ find typescript-* -name '*.mustache' -exec grep -Hn 'const COLLECTION_FORMAT' \{\} \;
typescript-angular/variables.mustache:4:export const COLLECTION_FORMATS = {
typescript-axios/baseApi.mustache:15:export const COLLECTION_FORMATS = {
typescript-fetch/runtime.mustache:110:export const COLLECTION_FORMATS = {
typescript-inversify/variables.mustache:1:export const COLLECTION_FORMATS = {
typescript-jquery/variables.mustache:2:export const COLLECTION_FORMATS = {
typescript-redux-query/runtime.mustache:27:export const COLLECTION_FORMATS = {
typescript-rxjs/runtime.mustache:133:export const COLLECTION_FORMATS = {

as they all define COLLECTION_FORMATS.pipes and COLLECTION_FORMATS.ssv
Likewise, many other generators use/assumes pipes, for example

./src/main/templates/Java/ApiClient.mustache:499:    } else if ("pipes".equals(collectionFormat)) {

I think the fix I submitted is the correct one: use the same tokens used in OpenAPI 2.0 (`pipes`, `ssv`)
in `modules/openapi-generator/src/main/java/org/openapitools/codegen/DefaultCodegen.java`
 

@macjohnny
Copy link
Member

@wing328 @jimschubert can you help here?

@jimschubert
Copy link
Member

jimschubert commented Jan 23, 2020

@macjohnny collectionFormat is definitely only pipes and ssv rather than pipe and space. In version 3.0 of the spec, these should be the parameter style used in this method. I think we'll need to see if anyone is concatenating the previous value to create OpenAPI 3.0 style values (pipeDelimited and spaceDelimited), for example in the openapi generator. If there's none doing this, then I think this is the appropriate bug fix, and we should get it into 4.2.3.

This probably hasn't come up before because of how uncommon these formats are.

@macjohnny
Copy link
Member

@DavidBiesack @jimschubert would you like to move forward with this to have it released in 4.2.3 on 2020-01-31?

@macjohnny
Copy link
Member

@DavidBiesack can you please update with the latest master?

@DavidBiesack
Copy link
Contributor Author

@DavidBiesack can you please update with the latest master?

@macjohnny Updated!

@macjohnny
Copy link
Member

some samples are not up to date anymore, according to https://circleci.com/gh/OpenAPITools/openapi-generator/12497#tests/containers/2

@jimschubert can you confirm this should be merged as-is, after the samples are updated?

@DavidBiesack
Copy link
Contributor Author

Please confirm - this update requires running all the scripts in bin and bin/openapi3 and committing all the updated files?

@macjohnny
Copy link
Member

@DavidBiesack please dont run the bin/openapi3 scripts, just /bin

@macjohnny
Copy link
Member

@DavidBiesack can you please merge the most recent master and re-generate the samples?

@DavidBiesack
Copy link
Contributor Author

@DavidBiesack can you please merge the most recent master and re-generate the samples?

@macjohnny I'll give it a try. merged master into my local branch and it build cleanly; regenerating petstore samples now.

I do get some errors:

[main] ERROR o.o.c.languages.ElmClientCodegen - Error running the command (/usr/bin/env elm-format --elm-version=0.19 --yes /Users/david.biesack/dev/tools/openapi-generator/samples/client/petstore/elm/src/DateTime.elm). Exit code: 127

and similar; I do not have elm-format installed so I'm trying what I found in CI/circle_parallel.sh:

   npm install -g elm-format

and that let me rerun bin/elm-petstore-all.sh cleanly
but then things failed on

Running ./bin/meta-codegen-kotlin.sh (output to /dev/null)
Error: Could not find or load main class org.gradle.wrapper.GradleWrapperMain
Caused by: java.lang.ClassNotFoundException: org.gradle.wrapper.GradleWrapperMain
Error: Could not find or load main class org.gradle.wrapper.GradleWrapperMain
Caused by: java.lang.ClassNotFoundException: org.gradle.wrapper.GradleWrapperMain
Can't load config class with name 'myClientCodegen'
Available:
ada
ada-server
android
apache2
apex
aspnetcore
avro-schema
bash
c
clojure
cwiki
cpp-qt5-client
cpp-qt5-qhttpengine-server
cpp-pistache-server
cpp-restbed-server
cpp-restsdk
cpp-tizen
csharp
csharp-netcore
csharp-dotnet2
csharp-nancyfx
dart
dart-dio
dart-jaguar
eiffel
elixir
elm
erlang-client
erlang-proper
erlang-server
flash
fsharp-giraffe-server
go
go-experimental
go-server
go-gin-server
graphql-schema
graphql-nodejs-express-server
groovy
kotlin
kotlin-server
kotlin-spring
kotlin-vertx
haskell-http-client
haskell
java
jaxrs-cxf-client
java-inflector
java-msf4j
java-pkmst
java-play-framework
java-undertow-server
java-vertx
java-vertx-web
jaxrs-cxf
jaxrs-cxf-extended
jaxrs-cxf-cdi
jaxrs-jersey
jaxrs-resteasy
jaxrs-resteasy-eap
jaxrs-spec
javascript
javascript-flowtyped
javascript-closure-angular
jmeter
k6
lua
mysql-schema
nim
nodejs-server-deprecated
nodejs-express-server
objc
ocaml
openapi
openapi-yaml
perl
php
php-laravel
php-lumen
php-slim-deprecated
php-slim4
php-silex
php-symfony
php-ze-ph
powershell
powershell-experimental
protobuf-schema
python
python-experimental
python-flask
python-aiohttp
python-blueplanet
r
ruby
ruby-on-rails
ruby-sinatra
rust
rust-server
scalatra
scala-akka
scala-finch
scala-httpclient-deprecated
scala-gatling
scala-lagom-server
scala-play-server
scalaz
spring
dynamic-html
html
html2
swift2-deprecated
swift3-deprecated
swift4
swift5
typescript-angular
typescript-angularjs
typescript-aurelia
typescript-axios
typescript-fetch
typescript-inversify
typescript-jquery
typescript-node
typescript-redux-query
typescript-rxjs
asciidoc
fsharp-functions
markdown
scala-sttp

[error] Check the spelling of the generator's name and try again.
ERROR!! FAILED TO RUN ./bin/meta-codegen-kotlin.sh

and


Running ./bin/python-server-blueplanet-petstore.sh (output to /dev/null)
rm: samples/server/petstore/python-blueplanet/test-requirements.txt: No such file or directory
Exception in thread "main" java.lang.RuntimeException: Could not generate model 'ApiResponse'
	at org.openapitools.codegen.DefaultGenerator.generateModels(DefaultGenerator.java:539)
	at org.openapitools.codegen.DefaultGenerator.generate(DefaultGenerator.java:1005)
	at org.openapitools.codegen.cmd.Generate.run(Generate.java:426)
	at org.openapitools.codegen.OpenAPIGenerator.main(OpenAPIGenerator.java:61)
Caused by: java.nio.file.NoSuchFileException: /Users/david/biesack/dev/tools/openapi-generator/samples/server/petstore/python-blueplanet/model-definitions/types/tosca/openapi_server/ApiResponse_ResourceType.tosca
	at java.base/sun.nio.fs.UnixException.translateToIOException(UnixException.java:92)
	at java.base/sun.nio.fs.UnixException.rethrowAsIOException(UnixException.java:111)
	at java.base/sun.nio.fs.UnixException.rethrowAsIOException(UnixException.java:116)
	at java.base/sun.nio.fs.UnixFileSystemProvider.newByteChannel(UnixFileSystemProvider.java:215)
	at java.base/java.nio.file.spi.FileSystemProvider.newOutputStream(FileSystemProvider.java:434)
	at java.base/java.nio.file.Files.newOutputStream(Files.java:218)
	at java.base/java.nio.file.Files.write(Files.java:3355)
	at org.openapitools.codegen.AbstractGenerator.writeToFileRaw(AbstractGenerator.java:111)
	at org.openapitools.codegen.AbstractGenerator.writeToFile(AbstractGenerator.java:96)
	at org.openapitools.codegen.DefaultGenerator.writeToFile(DefaultGenerator.java:1513)
	at org.openapitools.codegen.AbstractGenerator.writeToFile(AbstractGenerator.java:57)
	at org.openapitools.codegen.DefaultGenerator.processTemplateToFile(DefaultGenerator.java:1080)
	at org.openapitools.codegen.DefaultGenerator.generateModelDocumentation(DefaultGenerator.java:348)
	at org.openapitools.codegen.DefaultGenerator.generateModels(DefaultGenerator.java:536)
	... 3 more
ERROR!! FAILED TO RUN ./bin/python-server-blueplanet-petstore.sh

after updating petstore, I ran:

mvn package

again and that failed


[INFO] Reactor Summary:
[INFO] 
[INFO] openapi-generator-project .......................... SUCCESS [  1.492 s]
[INFO] openapi-generator-core ............................. SUCCESS [  4.283 s]
[INFO] openapi-generator (core library) ................... SUCCESS [01:08 min]
[INFO] openapi-generator (executable) ..................... FAILURE [  9.511 s]
[INFO] openapi-generator (maven-plugin) ................... SKIPPED
[INFO] openapi-generator-gradle-plugin (maven wrapper) .... SKIPPED
[INFO] openapi-generator-online ........................... SKIPPED
[INFO] ------------------------------------------------------------------------
[INFO] BUILD FAILURE
[INFO] ------------------------------------------------------------------------
[INFO] Total time: 01:24 min
[INFO] Finished at: 2020-03-20T09:25:56-04:00
[INFO] Final Memory: 31M/114M
[INFO] ------------------------------------------------------------------------
[ERROR] Failed to execute goal org.apache.maven.plugins:maven-shade-plugin:3.2.0:shade (process-resources) on project openapi-generator-cli: Error creating shaded jar: /Users/david.biesack/dev/tools/openapi-generator/modules/openapi-generator/target/classes (Is a directory) -> [Help 1]
[ERROR] 
[ERROR] To see the full stack trace of the errors, re-run Maven with the -e switch.
[ERROR] Re-run Maven using the -X switch to enable full debug logging.
[ERROR] 
[ERROR] For more information about the errors and possible solutions, please read the following articles:
[ERROR] [Help 1] http://cwiki.apache.org/confluence/display/MAVEN/MojoExecutionException
[ERROR] 
[ERROR] After correcting the problems, you can resume the build with the command
[ERROR]   mvn <goals> -rf :openapi-generator-cli

Should I add all the updated petstore samples to this PR and try to commit/push the PR?

@DavidBiesack
Copy link
Contributor Author

This does not look right -- too many diffs. I thought I'd merged from master but the changes in samples look like not everything is up-to-date

@macjohnny
Copy link
Member

please run mvn clean package after merging the master and before running the /bin/... scripts

@DavidBiesack
Copy link
Contributor Author

please run mvn clean package after merging the master and before running the /bin/... scripts

that is what I did. I'll merge master again, mvn clean package and run the scripts again, and update the PR one more time.

@DavidBiesack
Copy link
Contributor Author

PR updated

wing328 and others added 8 commits April 27, 2020 07:57
* fix php tests

* fix scala tests

* update ts angular v6 rest

* fix user create test

* fix spring cloud test

* comment out user delete

* fix angular v7 tests

* fix user test

* fix tests

* fix go exp tests

* commented out delete user tests

* comment out delete user tests in go openapi 3

* fix clojure tests
…C" (#5624)

* Removed stray "printf"s in
modules/openapi-generator/src/main/java/org/openapitools/codegen/languages/CLibcurlClientCodegen.java

* Support for booleans in C client

* Update README.md

* Change to C API mustache files to solve issue #5623

* Debugging of C's modle-body.mustache, as suggested by ityuhui

* Final changes suggested by ityuhui
* fix map type

* remove output type, fix appveyor

* test macos

* comment out failing scala test

* fix typo: configuration

* Revert "comment out failing scala test"

This reverts commit 1dcf84f.
…c has circular references (#5646)

* protects against stackoverflow when OAS spec has circular references

* protects against stackoverflow when OAS spec has circular references
@macjohnny
Copy link
Member

@DavidBiesack can you please allow pushes to your branch / PR from maintainers? I merged the most recent master, resolved the conflicts, and re-generated all necessary samples with the following command:

mvn clean package -DskipTests=true
./bin/utils/ensure-up-to-date --batch

@DavidBiesack
Copy link
Contributor Author

@macjohnny I sent collaborator requests to you and @jimschubert

@jimschubert jimschubert self-assigned this May 22, 2020
@jimschubert
Copy link
Member

@macjohnny sorry, I had things get really hectic and I fell way behind on issues/prs. I'm assuming since you were trying to get master merged up and samples regenerated that your "requested changes" approval status is just stale?

Once I regenerate samples and CI passes, we can merge this.

@jimschubert
Copy link
Member

See #6553 for updated branch and regenerated samples.

@jimschubert
Copy link
Member

@DavidBiesack I don't know if GitHub notifies this in a meaningful way, but your changes have been merged via #6553 (didn't want you to think this PR was just closed).

Sorry for the delay but I'm a couple months behind on things.

@DavidBiesack
Copy link
Contributor Author

Thanks, @jimschubert - I git notices via 4999/5000. Thanks for letting me contribute.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG][typescript-axios] emits wrong collection format code for parameter

6 participants