Skip to content

Conversation

@chameleon82
Copy link
Contributor

@chameleon82 chameleon82 commented Jan 25, 2020

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)

@chameleon82 chameleon82 changed the title fix akka-scala-client template compilation warnings [scala] fix akka-scala-client template compilation warnings Jan 26, 2020
@wing328
Copy link
Member

wing328 commented Jan 29, 2020

Thanks for the PR but your commit (as shown in the Commits tab) is not linked to your Github account, which means this PR won't count as your contribution in https://github.com/OpenAPITools/openapi-generator/graphs/contributors.

Let me know if you need help fixing it.

Ref: https://github.com/OpenAPITools/openapi-generator/wiki/FAQ#how-can-i-update-commits-that-are-not-linked-to-my-github-account

add COOKIE location authorization key
regenerate template
@chameleon82 chameleon82 force-pushed the scala-akka-fix-legacy branch from 90df525 to 6bfd5b2 Compare January 29, 2020 14:51
@wing328
Copy link
Member

wing328 commented Jan 30, 2020

@chameleon82 chameleon82 force-pushed the scala-akka-fix-legacy branch from 974f08f to ef3ac57 Compare January 31, 2020 15:52
$ref: '#/components/schemas/Pet'
application/json:
schema:
$ref: '#/components/schemas/Pet'
Copy link
Member

Choose a reason for hiding this comment

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

@chameleon82 are these a must? In other words, would the change still work without documenting the HTTP 200 response in the spec?

We prefer not to update petstore.yaml to keep it as original as possible.

Copy link
Member

Choose a reason for hiding this comment

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

@wing328 Thanks for calling this out. When you say original, what are we keeping it original compared to? This spec isn't the "official" petstore.yaml spec from OpenAPI Specification 3.0 examples.

I'd have some concerns about not including these changes. It would mean that the PUT and POST methods are only documented to ever return a client error. From https://github.com/OAI/OpenAPI-Specification/blob/master/versions/3.0.2.md#responsesObject:

A container for the expected responses of an operation. The container maps a HTTP response code to the expected response.

The documentation is not necessarily expected to cover all possible HTTP response codes because they may not be known in advance. However, documentation is expected to cover a successful operation response and any known errors.

The default MAY be used as a default response object for all HTTP codes that are not covered individually by the specification.

The Responses Object MUST contain at least one response code, and it SHOULD be the response for a successful operation call.

Although the line "documentation is expected to cover a successful operation response" doesn't use the specification terminology MUST, MAY or SHOULD, I think our test yaml breaking 'expectations' could be confusing especially since there is no 'default' response documented.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@wing328 @jimschubert I've checked than one https://petstore3.swagger.io/api/v3/openapi.yaml which currently has those specified, openApi specs also have specified successful response here https://github.com/OAI/OpenAPI-Specification/blob/master/examples/v3.0/petstore.yaml and here https://github.com/OAI/OpenAPI-Specification/blob/master/examples/v3.0/petstore-expanded.yaml
However https://petstore.swagger.io/v2/swagger.yaml has not specified even default one.
Seems that case must be clear in OpenApi Specification.
I suppose SHOULD for successful operation means that client has another way to check result. For example, by calling GET /pet/123 after calling POST /pet {id = 123, ... }
For the current case i have added this to validate successful scenarios in the unit tests

Copy link
Member

Choose a reason for hiding this comment

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

You're right that's probably why they've used SHOULD. However, the common way to do that without a returned body is to use links in the header of a successful response. I think your changes to include 2xx type responses makes sense. If we don't include positive responses for these endpoints, I think we should remove the endpoints entirely, because no realistic API returns only client errors for a path.

@wing328 are you cool with keeping these changes? We should probably also review all our go-to specs and make sure they represent real-world designs.

Copy link
Member

Choose a reason for hiding this comment

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

Totally agreed with reviewing the spec to ensure it's correct. Let's go with this PR for the time being.

Copy link
Member

@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 looks good, and only a minor concern about the removal of the UUID mapping (users can always manually apply this import mapping at generation time).

{{/imports}}
import {{mainPackage}}.core.ApiModel
import org.joda.time.DateTime
import java.util.UUID
Copy link
Member

Choose a reason for hiding this comment

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

This will be a potential breaking change unless this is added to the constructor:

importMapping.put("UUID", "java.util.UUID");

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it is already there in ScalaAkkaClientCodegen extends AbstractScalaCodegen extends DefaultCodegen base class.
I will add unit test to check there is no breaking changes for the further versions

$ref: '#/components/schemas/Pet'
application/json:
schema:
$ref: '#/components/schemas/Pet'
Copy link
Member

Choose a reason for hiding this comment

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

@wing328 Thanks for calling this out. When you say original, what are we keeping it original compared to? This spec isn't the "official" petstore.yaml spec from OpenAPI Specification 3.0 examples.

I'd have some concerns about not including these changes. It would mean that the PUT and POST methods are only documented to ever return a client error. From https://github.com/OAI/OpenAPI-Specification/blob/master/versions/3.0.2.md#responsesObject:

A container for the expected responses of an operation. The container maps a HTTP response code to the expected response.

The documentation is not necessarily expected to cover all possible HTTP response codes because they may not be known in advance. However, documentation is expected to cover a successful operation response and any known errors.

The default MAY be used as a default response object for all HTTP codes that are not covered individually by the specification.

The Responses Object MUST contain at least one response code, and it SHOULD be the response for a successful operation call.

Although the line "documentation is expected to cover a successful operation response" doesn't use the specification terminology MUST, MAY or SHOULD, I think our test yaml breaking 'expectations' could be confusing especially since there is no 'default' response documented.

@jimschubert jimschubert self-assigned this Feb 6, 2020
@jimschubert jimschubert added this to the 4.3.0 milestone Feb 6, 2020
@wing328
Copy link
Member

wing328 commented Feb 6, 2020

For some reasons, I can't comment/reply to #5106 (comment). Agreed with revising the "original" as it may not be perfectly accurate. I'll create a separate issue to track it.

@wing328 wing328 merged commit 1bba3a5 into OpenAPITools:master Feb 6, 2020
@chameleon82 chameleon82 deleted the scala-akka-fix-legacy branch February 6, 2020 14:46
@chameleon82 chameleon82 mentioned this pull request Feb 14, 2020
6 tasks
MikailBag pushed a commit to MikailBag/openapi-generator that referenced this pull request Mar 23, 2020
…ools#5106)

* fix akka-scala-client template compilation warnings

add COOKIE location authorization key
regenerate template

* fix scala akka-http client model imports

* fix OpenAPITools#4004 custom package name in reference.conf

* fix scaka-akka codegen test
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][Scala-AKKA] The custom package name is being overwritten in reference.conf file

3 participants