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

sort responses alphabetically #1803

Closed
wants to merge 1 commit into from
Closed

sort responses alphabetically #1803

wants to merge 1 commit into from

Conversation

dgf
Copy link

@dgf dgf commented Apr 18, 2024

No description provided.

@dgf
Copy link
Author

dgf commented Apr 18, 2024

refs #1802

@phillip-kruger
Copy link
Member

@MikeEdgar This seems ok to me. w.d.y.t ?

@MikeEdgar
Copy link
Member

One thing to think about with doing it this way is that it affects the responses object no matter how it was created. For example, users may set some custom order in a static file or filter today (using a TreeMap or some custom insertion-order LinkedHashMap) and this overrides that.

I am not sure how likely that is. Generally, I would say that stuff that changes the model would be better placed outside of the serializer, and keep that concerned with just dumping whatever the model already is.

Maybe the sorting of APIResponses keys could be done in the annotation scanner after all the paths are scanned? That would limit the impact to just scanned responses. I think we do something like this for tag names already.

Thoughts?

@phillip-kruger
Copy link
Member

Mmm, yes. I wonder should all of this not be done in a Filter ? We can provide the filter but let users opt in to use it ?

@MikeEdgar
Copy link
Member

Mmm, yes. I wonder should all of this not be done in a Filter ? We can provide the filter but let users opt in to use it ?

Maybe something like the io.smallrye.openapi.api.util.UnusedSchemaFilter

@dgf
Copy link
Author

dgf commented Apr 19, 2024

Just to add some context.

Our specific issue is caused by the way how Quarkus is adding the default security responses for authenticated endpoints. This results in random outputs of the response entries in generated files, depending on the machine or time of the day or whatever is seeding the loop access of maps in Java.

Quarkus creates a Map of additional responses: https://github.com/quarkusio/quarkus/blob/37fbf8acd65ad2ae1884272d60e8f0408d3f7bb6/extensions/smallrye-openapi/deployment/src/main/java/io/quarkus/smallrye/openapi/deployment/filter/AutoRolesAllowedFilter.java#L148

and merges it with the existing ones: https://github.com/quarkusio/quarkus/blob/37fbf8acd65ad2ae1884272d60e8f0408d3f7bb6/extensions/smallrye-openapi/deployment/src/main/java/io/quarkus/smallrye/openapi/deployment/filter/AutoRolesAllowedFilter.java#L139

Additional note / finding: the order is constant in the generated responses per run, so e.g. for the simple cases all responses are "200" "401" "403" or "200" "403" "401"

@MikeEdgar so if I'm getting you right we could fix that also on Quarkus side by using a LinkedHashMap?

@phillip-kruger
Copy link
Member

Yea this should be easy to fix in the Quarkus auto security filter

@MikeEdgar
Copy link
Member

Closing for now since it was fixed in Quarkus. If we want to revisit again here, let's consider doing it as a filter or some other approach that users can opt-in to enable, but have it disabled by default.

@MikeEdgar MikeEdgar closed this Apr 24, 2024
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