-
-
Notifications
You must be signed in to change notification settings - Fork 6.5k
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
Divide operations by content type #19473
base: master
Are you sure you want to change the base?
Divide operations by content type #19473
Conversation
@wing328 Could you review this PR? |
@cachescrubber (2022/02) @welshm (2022/02) @MelleD (2022/02) @atextor (2022/02) @manedev79 (2022/02) @javisst (2022/02) @borsch (2022/02) @banlevente (2022/02) @Zomzog (2022/09) @martin-mfg (2023/08) |
@wing328 @cachescrubber (2022/02) @welshm (2022/02) @MelleD (2022/02) @atextor (2022/02) @manedev79 (2022/02) @javisst (2022/02) @borsch (2022/02) @banlevente (2022/02) @Zomzog (2022/09) @martin-mfg (2023/08) ping |
thanks for the PR. general speaking i like the idea of dividing operations by content type since this PR updates both default codegen and generator, it may take more time to review at the first glance, my first feedback is to add more comments (e.g. docstring explaining what the function does, comments above code blocks to help other contributors easily understand what these do) |
7710ce5
to
a3a9fbb
Compare
@wing328 thanks for the answer! I had to make changes to the core because there is no way to do this at the level of a custom code generator, because I needed to change the logic of processing operations. So, the changes are quite simple, what I do:
I just did not see a more correct mechanism for how this could be built into the current code |
a3a9fbb
to
f9e5320
Compare
Hi @altro3, thanks for the PR! In my tests using the Java generator, I can't disable the new "divideOperationsByContentType" feature. Apparently because the below two new lines always enables it: Line 198 in f9e5320
Line 93 in f9e5320
It also seems you need to update the samples (following bullet point 3 in the PR checklist) again. Other than that this PR looks good to me. 👍 |
thanks for the PR! I have a question what about multiple content types for response body? example is here #17877 (comment) content:
application/json:
schema:
$ref: '#/components/schemas/coordinates'
application/xml:
schema:
$ref: '#/components/schemas/coordinates' if we define also such content types for response body, I think second function that is generated should have not only different params but also different response type, correct? now it's not working like that |
@martin-mfg @slabiakt Hi guys! Sorry for the long answer. Ok, about your questins:
I mean that no sane Java or Kotlin developer would turn off this feature, because otherwise the generated code would be incorrect from the point of view of Java or Kotlin frameworks. So my idea was to not give the user the ability to enable or disable this setting. And this flag is needed exclusively for communication between the DefaultCodegen and DefaultGenerator classes.
As far as I know, the content type of the response does not participate in routing, so the problem is that you have one method that can return responses with different content types. And you can't 100% correctly divide this logic into different methods. Of course, I may be wrong. In addition, the question arises, how will you correctly divide these methods if you simultaneously have an incoming request that can be xml or json, and at the same time the response can be xml or json? If you add division by response, then you should have 4 methods generated, not 2. But this is no longer a more correct generation, but an excessive generation. In my opinion, division only by incoming content type is quite sufficient |
f9e5320
to
127a9ae
Compare
@altro3 you have header "Accept" which is used for negotiation about response, in spring if you have such two endpoints: @RestController
@RequestMapping("/api/example")
public class ExampleController {
@GetMapping(produces = MediaType.APPLICATION_JSON_VALUE)
public ResponseEntity<MyDto> getJson() {
MyDto response = new MyDto();
response.setMessage("JSON response");
return ResponseEntity.ok(response);
}
@GetMapping(produces = MediaType.APPLICATION_XML_VALUE)
public ResponseEntity<MyDto> getXml() {
MyDto response = new MyDto();
response.setMessage("XML response");
return ResponseEntity.ok(response);
}
} depending what you will provide in Accept header, request will hit either getJson or getXml method. |
It's just that your solution only covers special cases. I'm not sure you're right about 90%... In general, the only solution I see for this case is to generate all possible combinations by default and add a setting - to compare the content of the type request and response. It seems to me that such a solution will cover 100% of implementations |
yes, I agree that it would be better to just generate all combinations 👍 |
127a9ae
to
92f38a8
Compare
@martin-mfg @slabiakt Ok, i've added two new options I think these options are enough. Maybe you should think about more readable method naming. Sample openapi spec: openapi: 3.0.3
info:
version: "1"
title: Multiple Content Types for same request
paths:
/multiplecontentpath:
post:
operationId: myOp
requestBody:
content:
application/json:
schema:
$ref: '#/components/schemas/coordinates'
application/xml:
schema:
$ref: '#/components/schemas/coordinates'
multipart/form-data:
schema:
type: object
properties:
coordinates:
$ref: '#/components/schemas/coordinates'
file:
type: string
format: binary
application/yaml:
schema:
$ref: '#/components/schemas/MySchema'
text/json:
schema:
$ref: '#/components/schemas/MySchema'
responses:
201:
description: Successfully created
headers:
Location:
schema:
type: string
content:
application/json:
schema:
$ref: '#/components/schemas/coordinates'
application/xml:
schema:
$ref: '#/components/schemas/coordinates'
text/json:
schema:
$ref: '#/components/schemas/MySchema'
components:
schemas:
coordinates:
type: object
required:
- lat
- long
properties:
lat:
type: number
long:
type: number
MySchema:
type: object
required:
- lat
properties:
lat:
type: number Micronaut samples (the same will be for spring)
@Post("/multiplecontentpath")
@Consumes("application/xml")
@Produces({"application/json", "application/xml"})
Mono<HttpResponse<@Valid Coordinates>> myOp(
@Body @Nullable @Valid Coordinates coordinates
);
@Post("/multiplecontentpath")
@Produces({"application/json", "application/xml"})
Mono<HttpResponse<@Valid Coordinates>> myOp_1(
@Body @Nullable @Valid Coordinates coordinates
);
@Post("/multiplecontentpath")
@Consumes({"application/json", "application/xml", "text/json"})
@Produces("multipart/form-data")
Mono<HttpResponse<@Valid Coordinates>> myOp_2(
@Nullable @Valid Coordinates coordinates,
@Nullable byte[] file
);
@Post("/multiplecontentpath")
@Consumes("text/json")
@Produces({"application/yaml", "text/json"})
Mono<HttpResponse<@Valid MySchema>> myOp_3(
@Body @Nullable @Valid MySchema mySchema
);
@Post("/multiplecontentpath")
Mono<HttpResponse<@Valid Coordinates>> myOp(
@Body @Nullable @Valid Coordinates coordinates
);
@Post("/multiplecontentpath")
@Consumes("application/xml")
@Produces("application/xml")
Mono<HttpResponse<@Valid Coordinates>> myOp_1(
@Body @Nullable @Valid Coordinates coordinates
);
@Post("/multiplecontentpath")
@Consumes({"application/json", "application/xml", "text/json"})
@Produces("multipart/form-data")
Mono<HttpResponse<@Valid Coordinates>> myOp_2(
@Nullable @Valid Coordinates coordinates,
@Nullable byte[] file
);
@Post("/multiplecontentpath")
@Consumes({"application/json", "application/xml", "text/json"})
@Produces("application/yaml")
Mono<HttpResponse<@Valid Coordinates>> myOp_3(
@Body @Nullable @Valid MySchema mySchema
);
@Post("/multiplecontentpath")
@Consumes("text/json")
@Produces("text/json")
Mono<HttpResponse<@Valid MySchema>> myOp_4(
@Body @Nullable @Valid MySchema mySchema
);
@Post("/multiplecontentpath")
@Consumes("application/xml")
@Produces({"application/json", "application/xml"})
Mono<HttpResponse<@Valid Coordinates>> myOp(
@Body @Nullable @Valid Coordinates coordinates
);
@Post("/multiplecontentpath")
@Produces({"application/json", "application/xml"})
Mono<HttpResponse<@Valid Coordinates>> myOp_1(
@Body @Nullable @Valid Coordinates coordinates
);
@Post("/multiplecontentpath")
@Consumes("text/json")
@Produces({"application/json", "application/xml"})
Mono<HttpResponse<@Valid MySchema>> myOp_2(
@Body @Nullable @Valid Coordinates coordinates
);
@Post("/multiplecontentpath")
@Consumes("application/xml")
@Produces("multipart/form-data")
Mono<HttpResponse<@Valid Coordinates>> myOp_3(
@Nullable @Valid Coordinates coordinates,
@Nullable byte[] file
);
@Post("/multiplecontentpath")
@Produces("multipart/form-data")
Mono<HttpResponse<@Valid Coordinates>> myOp_4(
@Nullable @Valid Coordinates coordinates,
@Nullable byte[] file
);
@Post("/multiplecontentpath")
@Consumes("text/json")
@Produces("multipart/form-data")
Mono<HttpResponse<@Valid MySchema>> myOp_5(
@Nullable @Valid Coordinates coordinates,
@Nullable byte[] file
);
@Post("/multiplecontentpath")
@Consumes("application/xml")
@Produces({"application/yaml", "text/json"})
Mono<HttpResponse<@Valid Coordinates>> myOp_6(
@Body @Nullable @Valid MySchema mySchema
);
@Post("/multiplecontentpath")
@Produces({"application/yaml", "text/json"})
Mono<HttpResponse<@Valid Coordinates>> myOp_7(
@Body @Nullable @Valid MySchema mySchema
);
@Post("/multiplecontentpath")
@Consumes("text/json")
@Produces({"application/yaml", "text/json"})
Mono<HttpResponse<@Valid MySchema>> myOp_8(
@Body @Nullable @Valid MySchema mySchema
);
@Post("/multiplecontentpath")
@Consumes("application/xml")
Mono<HttpResponse<@Valid Coordinates>> myOp(
@Body @Nullable @Valid Coordinates coordinates
);
@Post("/multiplecontentpath")
Mono<HttpResponse<@Valid Coordinates>> myOp_1(
@Body @Nullable @Valid Coordinates coordinates
);
@Post("/multiplecontentpath")
@Consumes("text/json")
Mono<HttpResponse<@Valid MySchema>> myOp_2(
@Body @Nullable @Valid Coordinates coordinates
);
@Post("/multiplecontentpath")
@Consumes("text/json")
@Produces("application/yaml")
Mono<HttpResponse<@Valid MySchema>> myOp_3(
@Body @Nullable @Valid MySchema mySchema
);
@Post("/multiplecontentpath")
@Consumes("application/xml")
@Produces("text/json")
Mono<HttpResponse<@Valid Coordinates>> myOp_4(
@Body @Nullable @Valid MySchema mySchema
);
@Post("/multiplecontentpath")
@Produces("text/json")
Mono<HttpResponse<@Valid Coordinates>> myOp_5(
@Body @Nullable @Valid MySchema mySchema
);
@Post("/multiplecontentpath")
@Consumes("text/json")
@Produces("text/json")
Mono<HttpResponse<@Valid MySchema>> myOp_6(
@Body @Nullable @Valid MySchema mySchema
);
@Post("/multiplecontentpath")
@Consumes("application/xml")
@Produces("application/xml")
Mono<HttpResponse<@Valid Coordinates>> myOp_7(
@Body @Nullable @Valid Coordinates coordinates
);
@Post("/multiplecontentpath")
@Produces("application/xml")
Mono<HttpResponse<@Valid Coordinates>> myOp_8(
@Body @Nullable @Valid Coordinates coordinates
);
@Post("/multiplecontentpath")
@Consumes("text/json")
@Produces("application/xml")
Mono<HttpResponse<@Valid MySchema>> myOp_9(
@Body @Nullable @Valid Coordinates coordinates
);
@Post("/multiplecontentpath")
@Consumes("application/xml")
@Produces("multipart/form-data")
Mono<HttpResponse<@Valid Coordinates>> myOp_10(
@Nullable @Valid Coordinates coordinates,
@Nullable byte[] file
);
@Post("/multiplecontentpath")
@Produces("multipart/form-data")
Mono<HttpResponse<@Valid Coordinates>> myOp_11(
@Nullable @Valid Coordinates coordinates,
@Nullable byte[] file
);
@Post("/multiplecontentpath")
@Consumes("text/json")
@Produces("multipart/form-data")
Mono<HttpResponse<@Valid MySchema>> myOp_12(
@Nullable @Valid Coordinates coordinates,
@Nullable byte[] file
);
@Post("/multiplecontentpath")
@Consumes("application/xml")
@Produces("application/yaml")
Mono<HttpResponse<@Valid Coordinates>> myOp_13(
@Body @Nullable @Valid MySchema mySchema
);
@Post("/multiplecontentpath")
@Produces("application/yaml")
Mono<HttpResponse<@Valid Coordinates>> myOp_14(
@Body @Nullable @Valid MySchema mySchema
); PS Where |
92f38a8
to
6ddd97b
Compare
Fixed #17877
OpenAPI allow set to one operation multiple content types for request body, but in main frameworks we need to divide this operation to multiple methods - with the same path, but different request body class and different consumed content type.
Enabled it for Java and Kotlin generators.
See isssue to details: #17877
PR checklist
Commit all changed files.
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/configs/java*
.IMPORTANT: Do NOT purge/delete any folders/files (e.g. tests) when regenerating the samples as manually written tests may be removed.
master
(upcoming 7.6.0 minor release - breaking changes with fallbacks),8.0.x
(breaking changes without fallbacks)