[Spring] Add an option to return success code #1197

ackintosh · 2018-10-08T13:42:25Z

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: master, 3.4.x, 4.0.x. Default: master.
Copied the technical committee to review the pull request if your PR is targeting a particular programming language.

Description of the PR

~~This PR makes the stub server returns status code defined at Response Object instead of 501(Not implemented).~~

This PR adds an option to make the stub server return success code. ( #1197 (comment) )

Remove the spacer "{{#async}} ... {{/async}}" "{{^async}} ... {{/async}}"

ackintosh · 2018-10-08T14:41:18Z

@bbdouglas (2017/07) @JFCote (2017/08) @sreeshas (2017/08) @jfiala (2017/08) @lukoyanov (2017/09) @cbornet (2017/09) @jeff9finger (2018/01)

cbornet · 2018-10-08T16:31:08Z

samples/client/petstore/spring-stubs/src/main/java/org/openapitools/api/StoreApi.java

-                    ApiUtil.setExampleResponse(request, "application/xml", "<Order>  <id>123456789</id>  <petId>123456789</petId>  <quantity>123</quantity>  <shipDate>2000-01-23T04:56:07.000Z</shipDate>  <status>aeiou</status>  <complete>true</complete></Order>");
-                    break;
-                }
+        for (MediaType mediaType: MediaType.parseMediaTypes(request.getHeader("Accept"))) {


cbornet · 2018-10-08T16:31:30Z

modules/openapi-generator/src/main/resources/JavaSpring/methodBody.mustache

-        {{#jdk8}}
-            {{#async}}
+{{#-first}}
+{{#jdk8}}


Indentation is broken

cbornet · 2018-10-08T16:38:01Z

I'm not sure about this one : why would we pick a specific status code over another ?
At least 501 describes the situation quite well : the endpoint was not implemented yet.

ackintosh · 2018-10-09T09:42:51Z

Thanks for your review!
I'm considering that the case of frontend development (e.g. SPA, mobile app) with a stub server.
In that case, a stub server returns a value defined at spec, but a client interprets the response as an error as the response code is 501.

cbornet · 2018-10-09T09:48:54Z

Then I guess you would rather have a 200 for all endpoints in that case, no ?

cbornet · 2018-10-09T09:49:54Z

We could have an application property where you set the default HTTP code you want to get.

ackintosh · 2018-10-09T10:09:40Z

The following is the spec that I'm working on. The each endpoint differently have the HTTP code a stub server should returns. (GET /posts -> 200, DELETE /posts/{postId} -> 204)
So I think we need to pick a specific status defined at responses.

openapi: 3.0.1
info:
  title: 匿名掲示板API
  version: 1.0.0
paths:
  /posts:
    get:
      description: 投稿をすべて取得する
      responses:
        '200':
          description: 投稿の配列
          content:
            application/json:
              schema:
                type: array
                items:
                  $ref: '#/components/schemas/Post'
    post:
...
      responses:
        '201':
          description: 作成した投稿
          content:
            application/json:
              schema:
                $ref: '#/components/schemas/Post'
        '422':
          $ref: '#/components/responses/UnprocessableEntity'
  /posts/{postId}:
    delete:
      description: 投稿を削除する
      parameters:
        - $ref: '#/components/parameters/postIdParam'
      responses:
        '204':
          description: 投稿の削除に成功
        '404':
          $ref: '#/components/responses/NotFound'
...

cbornet · 2018-10-09T10:43:43Z

Is it really important for your client ? Clients should accept any 2xx codes as successful responses (as per Postel's law)

ackintosh · 2018-10-09T11:46:48Z

As you say from the view of client, it may not important. Client can accept 2xx codes as successful responses.
On the other hand, from the view of a stub server, I think we should return properly codes as possible as.

cbornet · 2018-10-09T12:56:41Z

Ok. Then we would need some kind of "success code" resolution. I wonder if we don't already have that for some gens.

ackintosh · 2018-10-11T05:05:38Z

I've fixed the broken indentation. Please have a look when you have time.

ackintosh · 2018-10-26T06:11:53Z

ping @cbornet 😉

cbornet · 2018-10-26T07:26:07Z

I still think we shouldn't send codes like 400 by default. As I see it, OAI response codes are mostly hints for special codes that you want to highlight. We could however do something smarter about 2xx codes. But in any case sending a successful or not 5xx error code should be at most an option (not necessarily at generation, could be an app option).

cbornet · 2018-10-26T07:27:58Z

modules/openapi-generator/src/main/resources/JavaSpring/methodBody.mustache

@@ -1,35 +1,38 @@
 {{^reactive}}
+int statusCode = {{#responses}}{{#-first}}{{{code}}}{{/-first}}{{/responses}};


This could be inlined in the ResponseEntity which would make the PR much simpler.

ackintosh · 2018-11-15T14:10:45Z

I still think we shouldn't send codes like 400 by default. As I see it, OAI response codes are mostly hints for special codes that you want to highlight.

That's a thought.

I've noticed two use cases of the generated server codes:

Scaffolding

I understand that it is the use case you mentioned in [Spring] Add an option to return success code #1197 (comment) and [Spring] Add an option to return success code #1197 (comment) .
- Is my understanding correct?
The generated codes will be modified to implement application logic and will be able to return correct status code.
- so generated codes returning 501 Not Implemented is reasonable.

Just for stubbing

The use case mentioned in this comment.
The generated codes are used for just stubbing in frontend development.
- When server-side development has finished the stub is replaced with the concrete application.
- This contract-first approach proceeds frontend and server-side parallely
In this use case the generated server should return a status code defined in the OpenAPI spec.
- I think 400 is not problem in this use case if 400 is defined at the spec.
- If 500/400 is inconvenient we can put 200 at top of responses definition.

cbornet · 2018-11-15T16:03:46Z

Yes, for scaffolding 501 is better, for stubbing I guess 2xx would be better with resolution of the code if there are codes different than 200 defined in the OAI.

ackintosh · 2018-11-16T11:33:38Z

for stubbing I guess 2xx would be better with resolution of the code if there are codes different than 200 defined in the OAI.

Good idea. 💡

👇In my mind, mustache file will be updated like below. Are we on the same page?

MethodBody.mustache

-    return new ResponseEntity<>(HttpStatus.valueOf(HttpStatus.NOT_IMPLEMENTED));
+    return new ResponseEntity<>(HttpStatus.valueOf(
+        ({{#responses}}{{#-first}}{{{code}}}{{/-first}}{{/responses}} >= 400) ? 200 : {{#responses}}{{#-first}}{{{code}}}{{/-first}}{{/responses}}
+    ));

cbornet · 2018-11-16T12:38:44Z

I think we could do the resolution in the Java gen (postProcessOperation ?)

ackintosh · 2018-11-16T13:09:53Z

Ah 💡 Thanks!

* Revert "Fix broken indentation" * This reverts commit 95b6a00. * Revert "Tweak indent" * This reverts commit ba2cedc. * Revert "Returns status code which defined at Response Object" * This reverts commit f676a89.

…e-properly

ackintosh · 2018-11-16T14:35:39Z

Hmm... ideally example should have its status code. example has its "content-type" and "response body".
(related class: ExampleGenerator.java)

./bin/spring-all-pestore.sh

ackintosh · 2018-11-17T16:02:27Z

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

-                    op.examples = new ExampleGenerator(schemas, openAPI).generateFromResponseSchema(responseSchema, getProducesInfo(openAPI, operation));
+                    String exampleStatusCode = "200";
+                    for (String key : operation.getResponses().keySet()) {
+                        if (operation.getResponses().get(key) == methodResponse && !key.equals("default")) {


methodResponse is "2xx" or "default" so exampleStatusCode will be 2xx.

ackintosh · 2018-11-18T04:30:45Z

Please review the changes when you have time. 🙏

ackintosh · 2018-11-28T02:53:58Z

As be commented at gitter, I'll make this changes an option. Making the changes an option makes sense to preserve backward compat and covers the two use cases.

cc @cbornet

ackintosh · 2018-11-28T03:05:07Z

We have 2 ways how make the changes an option: Generation or Spring config. I'm trying to make a "Generation" option as I'm not familiar with Spring. 🤔💦

ackintosh · 2018-11-28T04:26:59Z

Changed the PR title.

ackintosh · 2018-11-28T08:32:50Z

Test `--additional-properties returnSuccessCode=true`

$ bin/springboot-petstore-server.sh --additional-properties returnSuccessCode=true
...
...
$ git diff

↓↓

ackintosh · 2018-11-28T08:38:56Z

@cbornet I've updated this PR, make success code optional. Generated server returns 501 as default. Please review 🙏

cbornet · 2018-11-28T10:13:04Z

modules/openapi-generator/src/main/resources/JavaSpring/methodBody.mustache

@@ -25,14 +25,14 @@ return CompletableFuture.supplyAsync(()-> {
    {{/-last}}
 {{/examples}}
 {{^examples}}
-return {{#jdk8}}{{#async}}CompletableFuture.completedFuture({{/async}}{{/jdk8}}new ResponseEntity<>(HttpStatus.NOT_IMPLEMENTED){{#jdk8}}{{#async}}){{/async}}{{/jdk8}};
+return {{#jdk8}}{{#async}}CompletableFuture.completedFuture({{/async}}{{/jdk8}}new ResponseEntity<>({{#returnSuccessCode}}HttpStatus.OK{{/returnSuccessCode}}{{^returnSuccessCode}}HttpStatus.NOT_IMPLEMENTED{{/returnSuccessCode}}){{#jdk8}}{{#async}}){{/async}}{{/jdk8}};


Shouldn't it be HttpStatus.valueOf({{{statusCode}}}) instead of HttpStatus.OK

cbornet · 2018-11-28T10:13:59Z

modules/openapi-generator/src/main/resources/JavaSpring/methodBody.mustache

@@ -43,5 +43,8 @@ exchange.getResponse().setStatusCode(HttpStatus.NOT_IMPLEMENTED);
        }
        {{/-last}}
    {{/examples}}
+{{^examples}}
+        exchange.getResponse().setStatusCode({{#returnSuccessCode}}HttpStatus.OK{{/returnSuccessCode}}{{^returnSuccessCode}}HttpStatus.NOT_IMPLEMENTED{{/returnSuccessCode}});


Shouldn't it be HttpStatus.valueOf({{{statusCode}}}) instead of HttpStatus.OK ?

Yes, it should be HttpStatus.OK as the status code is contained by example. The code above is in {{^examples}} section.

cbornet · 2018-11-28T10:22:40Z

LGTM 👍

ackintosh · 2018-11-29T02:42:50Z

Thank you. ✨

ackintosh · 2018-11-29T02:49:30Z

To be clear the purpose, I've updated the description of this PR. 😌
#1197 (comment)

* Returns status code which defined at Response Object * Tweak indent Remove the spacer "{{#async}} ... {{/async}}" "{{^async}} ... {{/async}}" * Update samples^ * Fix broken indentation * Update samples * Revert methodBody.mustache * Revert "Fix broken indentation" * This reverts commit 95b6a00. * Revert "Tweak indent" * This reverts commit ba2cedc. * Revert "Returns status code which defined at Response Object" * This reverts commit f676a89. * Example contains status code * Update samples ./bin/spring-all-pestore.sh * Fix syntax error * Update samples * Run bin/utils/ensure-up-to-date * Make the changes an option `returnSuccessCode` * Run bin/spring-all-pestore.sh to update samples * Run ./bin/utils/export_docs_generators.sh

ackintosh added 3 commits October 8, 2018 21:58

Returns status code which defined at Response Object

f676a89

Tweak indent

ba2cedc

Remove the spacer "{{#async}} ... {{/async}}" "{{^async}} ... {{/async}}"

Update samples^

acff51c

cbornet reviewed Oct 8, 2018

View reviewed changes

ackintosh added 2 commits October 11, 2018 13:26

Fix broken indentation

95b6a00

Update samples

df17007

cbornet reviewed Oct 26, 2018

View reviewed changes

ackintosh added 2 commits November 16, 2018 22:25

Revert methodBody.mustache

d3731ee

* Revert "Fix broken indentation" * This reverts commit 95b6a00. * Revert "Tweak indent" * This reverts commit ba2cedc. * Revert "Returns status code which defined at Response Object" * This reverts commit f676a89.

Merge remote-tracking branch 'upstream/master' into return-status-cod…

27a4e2b

…e-properly

ackintosh added 2 commits November 18, 2018 00:57

Example contains status code

2e4f484

Update samples

4ade38a

./bin/spring-all-pestore.sh

ackintosh commented Nov 17, 2018

View reviewed changes

ackintosh added 3 commits November 18, 2018 01:23

Fix syntax error

a1b793b

Update samples

a758b1d

Run bin/utils/ensure-up-to-date

bd7ee46

ackintosh force-pushed the return-status-code-properly branch from dc7087a to bd7ee46 Compare November 18, 2018 03:13

ackintosh added 2 commits November 28, 2018 13:16

Make the changes an option returnSuccessCode

73a88d8

Run bin/spring-all-pestore.sh to update samples

048fcf1

ackintosh changed the title ~~[Spring] Return status code properly~~ [Spring] Add an option to return success code Nov 28, 2018

ackintosh added 2 commits November 28, 2018 14:47

Merge branch 'master' into return-status-code-properly

4af665d

Run ./bin/utils/export_docs_generators.sh

8dedb43

cbornet reviewed Nov 28, 2018

View reviewed changes

ackintosh added Enhancement: Feature Server: Spring labels Nov 29, 2018

ackintosh added this to the 3.3.4 milestone Nov 29, 2018

ackintosh merged commit f7c857c into OpenAPITools:master Nov 29, 2018

ackintosh deleted the return-status-code-properly branch November 29, 2018 02:51

		@@ -1,35 +1,38 @@
		{{^reactive}}
		int statusCode = {{#responses}}{{#-first}}{{{code}}}{{/-first}}{{/responses}};

Uh oh!

[Spring] Add an option to return success code #1197

[Spring] Add an option to return success code #1197

Uh oh!

Conversation

ackintosh commented Oct 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR checklist

Description of the PR

Uh oh!

ackintosh commented Oct 8, 2018

Uh oh!

cbornet Oct 8, 2018

Choose a reason for hiding this comment

Uh oh!

cbornet Oct 8, 2018

Choose a reason for hiding this comment

Uh oh!

cbornet commented Oct 8, 2018

Uh oh!

ackintosh commented Oct 9, 2018

Uh oh!

cbornet commented Oct 9, 2018

Uh oh!

cbornet commented Oct 9, 2018

Uh oh!

ackintosh commented Oct 9, 2018

Uh oh!

cbornet commented Oct 9, 2018

Uh oh!

ackintosh commented Oct 9, 2018

Uh oh!

cbornet commented Oct 9, 2018

Uh oh!

ackintosh commented Oct 11, 2018

Uh oh!

ackintosh commented Oct 26, 2018

Uh oh!

cbornet commented Oct 26, 2018

Uh oh!

cbornet Oct 26, 2018

Choose a reason for hiding this comment

Uh oh!

ackintosh commented Nov 15, 2018

Uh oh!

cbornet commented Nov 15, 2018

Uh oh!

ackintosh commented Nov 16, 2018

Uh oh!

cbornet commented Nov 16, 2018

Uh oh!

ackintosh commented Nov 16, 2018

Uh oh!

ackintosh commented Nov 16, 2018

Uh oh!

ackintosh Nov 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ackintosh commented Nov 18, 2018

Uh oh!

ackintosh commented Nov 28, 2018

Uh oh!

ackintosh commented Nov 28, 2018

Uh oh!

ackintosh commented Nov 28, 2018

Uh oh!

ackintosh commented Nov 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test --additional-properties returnSuccessCode=true

Uh oh!

ackintosh commented Nov 28, 2018

Uh oh!

cbornet Nov 28, 2018

Choose a reason for hiding this comment

Uh oh!

cbornet Nov 28, 2018

Choose a reason for hiding this comment

Uh oh!

ackintosh Nov 29, 2018

ackintosh commented Oct 8, 2018 •

edited

Loading

ackintosh Nov 17, 2018 •

edited

Loading

ackintosh commented Nov 28, 2018 •

edited

Loading

Test `--additional-properties returnSuccessCode=true`