Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Scala] Fix async helper methods when body is optional #7274

Merged
merged 2 commits into from
Jan 9, 2018

Conversation

gmarz
Copy link
Contributor

@gmarz gmarz commented Dec 29, 2017

Closes #7272

PR checklist

  • Read the contribution guidelines.
  • Ran the shell script under ./bin/ to update Petstore sample so that CIs can verify the change. (For instance, only need to run ./bin/{LANG}-petstore.sh and ./bin/security/{LANG}-petstore.sh if updating the {LANG} (e.g. php, ruby, python, etc) code generator or {LANG} client's mustache templates). Windows batch files can be found in .\bin\windows\.
  • Filed the PR against the correct branch: 3.0.0 branch for changes related to OpenAPI spec 3.0. Default: master.
  • Copied the technical committee to review the pull request if your PR is targeting a particular programming language. (/cc @clasnake @jimschubert)

Description of the PR

Closes #7272

Copy link
Contributor

@jimschubert jimschubert left a comment

Choose a reason for hiding this comment

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

This change is most likely fine.

I wanted to comment on the "body is optional" comment, though.

I believe None would get serialized as null here, and not an empty body. The "required" property of a parameter for Swagger/OpenAPI 2.0 specs says:

Determines whether this parameter is mandatory. If the parameter is in "path", this property is required and its value MUST be true. Otherwise, the property MAY be included and its default value is false.

It's been a few years since I've used json4s, but I believe the way to truly support required=false for body parameters per swagger spec would be to implement a custom serializer per type and only write when the Option value is defined.

This isn't a deficiency in your PR, it's a confusion of the 2.0 specification, in which required refers to the json structure and not the data within the structure. This is fixed in Open API 3.0.1 for schema objects using the nullable attribute (see https://github.com/OAI/OpenAPI-Specification/blob/master/versions/3.0.1.md#schemaNullable).

@gmarz
Copy link
Contributor Author

gmarz commented Jan 3, 2018

I believe None would get serialized as null here, and not an empty body.

I might be wrong (haven't actually tested this yet), but it looks like the async methods will write an empty body.

@wing328
Copy link
Contributor

wing328 commented Jan 9, 2018

CircleCI reported the following errors:

[ERROR] /home/ubuntu/swagger-codegen/samples/client/petstore/scala/src/main/scala/io/swagger/client/api/PetApi.scala:56: error: not found: type JValue
[ERROR]   implicit val jvalueReader: ClientResponseReader[JValue] = ClientResponseReaders.JValueReader
[ERROR]                                                   ^
[ERROR] /home/ubuntu/swagger-codegen/samples/client/petstore/scala/src/main/scala/io/swagger/client/api/StoreApi.scala:54: error: not found: type JValue
[ERROR]   implicit val jvalueReader: ClientResponseReader[JValue] = ClientResponseReaders.JValueReader
[ERROR]                                                   ^
[ERROR] /home/ubuntu/swagger-codegen/samples/client/petstore/scala/src/main/scala/io/swagger/client/api/UserApi.scala:54: error: not found: type JValue
[ERROR]   implicit val jvalueReader: ClientResponseReader[JValue] = ClientResponseReaders.JValueReader
[ERROR]                                                   ^
[ERROR] three errors found
[ERROR] Failed to execute goal net.alchim31.maven:scala-maven-plugin:3.1.5:compile (scala-compile-first) on project swagger-scala-client: wrap: org.apache.commons.exec.ExecuteException: Process exited with an error: 1 (Exit value: 1) -> [Help 1]

I wonder if you can take a look when you've time.

UPDATE: I merged with master locally and no longer can repeat the error. I've pushed the branch to CircleCI to verify again. Let's see how it goes.

@wing328
Copy link
Contributor

wing328 commented Jan 9, 2018

CircleCI tests passed via https://circleci.com/gh/swagger-api/swagger-codegen/3321

@wing328 wing328 merged commit 7cc738a into swagger-api:master Jan 9, 2018
jimschubert added a commit to jimschubert/swagger-codegen that referenced this pull request Jan 10, 2018
* master: (26 commits)
  [Scala] Fix async helper methods when body is optional (swagger-api#7274)
  [Rust] Recommend style based on 'rustfmt' defaults (swagger-api#7335)
  [Java:vertx] Initialize router in init method and re-use router member to create S… (swagger-api#7234)
  [Scala] Fix missing json4s import (swagger-api#7271)
  deploy snapshot version 2.3.1
  [Ada] Add Ada support for server code generator swagger-api#6680 (swagger-api#7256)
  add shijinkui to scala technical committee
  Generate swagger yaml for go client (swagger-api#7281)
  use openjdk7 in travis to ensure it works with jdk7
  docs(readme): update link to contributing guid (swagger-api#7332)
  Fix a regression bug that was introduce in a recent commit. Removed the tabs that were causing error in Play Framework (swagger-api#7241)
  Fix issue swagger-api#7262 with the parameter name in the path. The problem was that camelCase naming was forced only in this part of the code when everywhere else it is configurable. (swagger-api#7313)
  Java8 fix (swagger-api#7260)
  update to 2.3.1-SNAPSHOT
  fix typo, update 2017 to 2018
  [Doc] add huawei cloud to companies list swagger-api#7308 (swagger-api#7309)
  Adding Peatio opensource as reference project (swagger-api#7267)
  Update README.md (swagger-api#7298)
  Update README.md (swagger-api#7299)
  [all] sys props in CodegenConstants
  ...
wing328 pushed a commit that referenced this pull request Jan 11, 2018
* [Scala] Fix async helper methods when body is optional (#7274)

* [Scala] Fix async helper methods when body is optional

Closes #7272

* Update petstore sample

* Standardize all isRestfulxxx methods.

Change 'equals' method to 'equalsIgnoreCase' for isRestfulIndex method in Operations.  This matches isRestfulCreate, isRestfulShow, isRestfulDestroy
chiman-jang pushed a commit to chiman-jang/swagger-codegen that referenced this pull request Jan 16, 2018
* [Scala] Fix async helper methods when body is optional (swagger-api#7274)

* [Scala] Fix async helper methods when body is optional

Closes swagger-api#7272

* Update petstore sample

* Standardize all isRestfulxxx methods.

Change 'equals' method to 'equalsIgnoreCase' for isRestfulIndex method in Operations.  This matches isRestfulCreate, isRestfulShow, isRestfulDestroy
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.

None yet

3 participants