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

Improve schema building, bug fixes #1012

Merged
merged 2 commits into from
May 15, 2023

Conversation

altro3
Copy link
Collaborator

@altro3 altro3 commented May 9, 2023

Main changes:

  • Added correct processing for optional path variables (routing like this: "/test{/pathVar1,pathVar2}")
  • Fixes process oneOf block on field level
  • Added unwrapping allOf block when it possible
  • Now the in field for parameters without annotation is determined automatically (path in all cases, query for get requests)
  • Fixed duplicate elements in oneOf and anyOf blocks

Code changes

  • Now all tests check final openApi object for more correct checks (found some incorrect tests after it)
  • Now all modifications for the OpenAPI object after the analysis of the class structure are moved to the method postProcessOpenApi
  • Some minor code fixes

Copy link
Contributor

@dstepanov dstepanov left a comment

Choose a reason for hiding this comment

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

It's hard to understand what is fixed/changed. Can you keep the old tests as they are if modification is not needed and add new ones? Or please comment on every test change and describe why it changed. Thanks!

((MapSchema) petSchema.properties['freeForm']).description == "A free-form object"
((MapSchema) petSchema.properties['freeForm']).getAdditionalProperties() == true
petSchema.properties['freeForm'].type == "object"
petSchema.properties['freeForm'].description == "A free-form object"
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Now we have just instance of Schema class

@@ -21,7 +21,7 @@ import io.swagger.v3.oas.annotations.Operation;
class OpenApiController {

@Operation(summary = "Update tag", description = "Updates an existing tag", tags = "users_tag")
@Post("/tags/{tagId: \\\\d+}/{path:.*}{.ext}/update{/id:[a-zA-Z]+}/{+path}{?max,offset}")
@Post("/tags/{tagId: \\\\d+}/{path:.*}{.ext}/update/{+path}{?max,offset}{/id:[a-zA-Z]+}")
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

optional path variable must be last

@Controller("/path")
class OpenApiController {

@Patch
public HttpResponse<String> processSync4(@Parameter(schema = @Schema(type = "string", format = "uuid")) String param) {
public HttpResponse<String> processSync4(@Parameter(schema = @Schema(type = "string", format = "uuid")) @QueryValue String param) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added lost annotation @QueryValue

Copy link
Contributor

Choose a reason for hiding this comment

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

We support no-annotation parameters, which can also be query value, but for proper OpenApi it might be needed

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@dstepanov I didn't test it for Patch method, can tell you what I know:

  1. For Get method - it is true. And I've add support it in openapi
  2. For Post method, if you dont's set annotation, parameters are automatically treated as wrapped, i.e. they must be passed in the request body (by the way, I don’t understand at all why the logic is different for different methods. This is not at all obvious), or as path variables (it's supported too)
    Which logic for other methods - I didn't test

Copy link
Collaborator Author

@altro3 altro3 May 9, 2023

Choose a reason for hiding this comment

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

In this test before my changes in field in was null value

return HttpResponse.ok();
}

@Put
public HttpResponse<String> processSync3(
@Parameter(schema = @Schema(ref = "#/components/schemas/MyParamSchema")) Optional<Period> period) {
@Parameter(schema = @Schema(ref = "#/components/schemas/MyParamSchema")) @QueryValue Optional<Period> period) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added lost @QueryValue annotation

String test7(String someId, @Nullable String someNotRequired, java.util.Optional<String> someNotRequired2, HttpRequest req, Principal principal, @Body Greeting myBody);

@Get("/test8{/pathVar1,pathVar2}")
String test8(String pathVar1, String pathVar2, String queryVar, @Nullable String name);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

new test case for optional path vars

@@ -918,7 +899,6 @@ class MyBean {}
''')
then: "the state is correct"
!Utils.testReference
!Utils.testReferenceAfterPlaceholders
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Utils.testReferenceAfterPlaceholders - this filed deleted and we need to check testReference always


import io.swagger.v3.oas.annotations.media.Schema;

public class MyJaxbElement4 {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@Schema(oneOf = {DiscountSizeOpenApi.class, DiscountFixedOpenApi.class, MultiplierSizeOpenApi.class})
Test for this block. Fixed processing in this PR

@@ -84,7 +83,6 @@ class MyBean {}
then:
openAPI.components.schemas
openAPI.components.schemas.size() == 1
openAPI.components.schemas['Person'].name == 'Person'
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

schema name always null. Schema name contains in schemas map

@@ -36,7 +36,7 @@ class OpenApiController {
description = "This is description",
tags = "Normalize",
responses = {
@ApiResponse(responseCode = "200", description = "Desc1", content = @Content(mediaType = MediaType.ALL, schema = @Schema(type = "blob", format = "binary"))),
@ApiResponse(responseCode = "200", description = "Desc1", content = @Content(mediaType = MediaType.ALL, schema = @Schema(type = "string", format = "binary"))),
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

we can't use custom values for type field

@@ -35,6 +35,7 @@ class MyDto {
private JAXBElement<? extends XmlElement> xmlElement;
private JAXBElement<? extends XmlElement2> xmlElement2;
public JAXBElement<? extends XmlElement3> xmlElement3;
public JAXBElement<? extends XmlElement4> xmlElement4;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

new test case for oneOf block

@altro3
Copy link
Collaborator Author

altro3 commented May 9, 2023

@dstepanov added comments to most of changes in tests. Need to double check to places (without comments now, I don't remember why I did these changes). You can check places with comments

@dstepanov dstepanov requested a review from graemerocher May 9, 2023 18:35
withRef.openIdConnectUrl == null
withRef.bearerFormat == null
withRef.flows == null
withRef.scheme == null
Copy link
Contributor

Choose a reason for hiding this comment

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

Why was this deleted?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@graemerocher Because the swagger doesn't really support links to other security schemes. Therefore, when you try to deserialize a swagger file with such a scheme, it will be ignored, because. type is not specified. See class SecuritySchemeDeserializer:
изображение
That is, this test case is not correct.

@altro3 altro3 force-pushed the improve_schema_building branch from 48a4288 to 061f1ce Compare May 10, 2023 16:29
@altro3 altro3 requested a review from graemerocher May 12, 2023 22:04
@graemerocher graemerocher merged commit df1792b into micronaut-projects:4.9.x May 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants