-
Notifications
You must be signed in to change notification settings - Fork 91
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
Found several bugs in the OpenAPI generation results #2062
Comments
Hi @MikeEdgar thanks, I will checkout the changes. What is the best way to pickup these changes, point to Quarkus 999-SNAPSHOT? Also what about the #4 issue, Furthermore we have observed that @JSONVIEW is not respected at all if there is a @Schema annotation. This is one of the biggest bugs we found but didn't see that listed as fixed. |
@dhoffer I believe that should be handled by this change [1] where previously the presence of a |
To answer your other question, this is not yet incorporated in Quarkus, so you would need to build it locally and add the |
Hi @MikeEdgar, we have tested your changes and have some feedback. We are almost there but not quite, let me explain. First I found I had to add these dependencies to pickup your changes: Of my reported issues, #1, 2 & 4 are fixed. However in our testing #3 still fails (it is still being applied globally). However now that we have thought about this more, we think from an OpenAPI perspective you should ignore the @JsonIgnoreProperties annotation completely. The reason we think you should just ignore this property is that you have no way to know what to call the modified model for the specific use case. Note there could be lots of usages of @JsonIgnoreProperties and you would have to have different models for each combination. We now don't think OpenAPI should care about this annotation as this is really just a Jackson serialization issue. We welcome your thoughts on this. |
@dhoffer I think With PR #2063, the model referenced by a field annotated with Lines 5 to 45 in 458fb2f
Let me know if this addresses your concern. Did you see different behavior when you tried the changes in 4.0.3-SNAPSHOT? |
Hi @MikeEdgar but you are also removing the String description from all of the Role models:
Where is this part?
|
It's not in the unit test, but if I do add the @Path("/role")
class RoleResource {
@GET
@Produces(MediaType.APPLICATION_JSON)
@JsonView(Views.Full.class)
@APIResponse(responseCode = "200", content = @Content(schema = @Schema(implementation = Role.class)))
public Response getRole() {
return null;
}
@POST
public Response post(@RequestBody @JsonView(Views.Ingest.class) Role role) {
return null;
}
} The resulting schemas are like this (which I see a bug where description should not be present in the "Role_Full" : {
"type" : "object",
"properties" : {
"id" : {
"type" : "string",
"format" : "uuid",
"pattern" : "[a-fA-F0-9]{8}-[a-fA-F0-9]{4}-[a-fA-F0-9]{4}-[a-fA-F0-9]{4}-[a-fA-F0-9]{12}"
},
"name" : {
"type" : "string"
},
"createdAt" : {
"$ref" : "#/components/schemas/LocalDateTime"
},
"description" : {
"type" : "string",
"title" : "Title of description"
}
}
},
"Role_Ingest" : {
"type" : "object",
"properties" : {
"name" : {
"type" : "string"
},
"description" : {
"type" : "string",
"title" : "Title of description"
}
}
} |
@MikeEdgar The problem with that test is it's not applying what the Group processing is doing first. I will attach the full OpenAPI of this sample project. Also I don't see any problem with Role_Ingest where you mentioned you did see a bug. The problem I see is that Role_Full is missing description because of the @JsonIgnoreProperties("description") in Group on the Roles list. This is with running the sample app using your SNAPSHOTs and then downloading the openapi it generates. |
@MikeEdgar One more thought on this. I know you said you feel @JsonIgnoreProperties is important and should be supported to allow precise inline OpenAPI models and we are fine with that but I'd like to see this be configurable. One idea would be for smallrye openapi to allow users to specify the list of annotations that smallrye openapi should ignore. Our current/prior approach that used Swagger libraries skipped @JsonIgnoreProperties for OpenAPI generation and that was fine for us. We would actually prefer it that way just so there is no change with this upgrade to use smallrye openapi, so having it configurable would be ideal. |
@dhoffer I'm not opposed to making things configurable, but I'd like to understand the use case better. I'm struggling to follow why |
@MikeEdgar In our case we really aren't trying to hide anything, it was added to break a circular recursion issue when the data was serialized by Jackson. That wouldn't be a problem in the sample reproducer project but it is in our production system. I believe that is the only reason we have to occasionally use @JsonIgnoreProperties. |
Ok, I see. Either way the result is that the JSON is missing a property that is documented by the OpenAPI schema, no? I'm trying to understand if there is a better/different way to make things work for you. |
Well, lets back up and first cover the remaining bug. Then we can discuss the idea of it being configurable. The current code does not work because @JsonIgnoreProperties is still being applied globally and it should just be applied to the inline model. That is the breaking part. Then yes being able to configure smallrye openapi to skip @JsonIgnoreProperties would be nice for our use case as we are fine with it always being in the model but no data exists (it doesn't exist in a deep nested level so users don't want the data at that level anyway). I hope that makes sense. |
@MikeEdgar The issue we see with In the reproducer, the issue occurs since the scanner resolves Without
With
|
Thanks @JakeH10 , I see the issue and I'll look into the fix. |
I'm not able to pull down the PR branch for some reason. Are you able to merge so we can test using main branch? |
The branch is in my fork. I can go ahead and merge and we'll address anything new in a separate change. |
@MikeEdgar Fields are not being hidden when in-line, i.e. when
The
The actual
|
Yeah, I think the logger should have been ignored from that annotation. Just curious, why not make the logger |
In production we have hundreds of entities and its just easier to have this in the base class. @Schema(hidden = true) Also we use @Schema(hidden = true) quite extensively to hide things from the generated OpenAPI schema so its critical that works. |
@dhoffer the third linked PR (now merged to |
@MikeEdgar We have been testing and this looks good. Thank you for all your help. We will let you know if we find anything. How soon could you make the next release with these fixes? We could use that asap. |
I'll cut release 4.0.3 now. |
We are trying to upgrade a large JAX-RS REST app to use the quarkus-smallrye-openapi library instead of Swagger libraries but we have run into some blocking issues.
List of issues:
I have created workarounds for 1 & 2 in our OASFilter but it is not possible to fix 3 & 4 in the filter as the schema is completely broken before it reaches the filter.
I have attached a reproducer project.
We are using Quarkus 3.16.2. This is a blocking issue for us so can't use quarkus-smallrye-openapi library until we have fixes or workarounds for all of these issues.
json-view-schema-example.zip
The text was updated successfully, but these errors were encountered: