search

smallrye / smallrye-open-api

SmallRye implementation of Eclipse MicroProfile OpenAPI
Apache License 2.0
117 stars 89 forks source link

Enhancement Request: Provide a fixed order for responses #1802

Closed dgf closed 5 months ago

dgf commented 5 months ago

We've encountered some challenges while using definitions generated by Quarkus in our CI workflow. Constantly/randomly it changes the order of the responses in the generated OpenAPI JSON and YML files. This has various side effects and requires manual reviews in our processes.

The cause is the generation of the responses section itself. The function io.smallrye.openapi.runtime.io.response.ResponseWriter#writeAPIResponses loops over the entry set of responses without any sorting.

We fixed it for now in our env by deploying a SNAPSHOT that sorts the set with Map.Entry.comparingByKey()

Its tested and running well already for some dozen builds and we would like to see the stabilization for the export of definitions in the standard.

To do that we/you need to decide what should be the fixed order. For now I'm seeing two different ones in the tests. Some JSON based expectation files like core/src/test/resources/io/smallrye/openapi/runtime/io/_everything.json containing the responses in alphabetical order "default", "200", "404" and the some YML assertions like in the test io.smallrye.openapi.runtime.io.OpenApiParserAndSerializerTest#testParsingBigStaticYamlFile are reversed: "204", "200" and "404", "200"

What is the expected order? Are there any notes about in the specs or a best practice?

We can easily create a PR, as we have a fix ready (just need to decide if forward or backwards sorted).

phillip-kruger commented 5 months ago

Please provide that PR and we can discuss there. Thanks

dgf commented 5 months ago

oh dear, I've just discovered that the entire core package has been refactored. I created the fix based on 3.10.0 . So I'll have to start again, but first I'll have to familiarize myself with the new JSON handling.

MikeEdgar commented 5 months ago

Hi @dgf , how large were your changes? Even if you can post a link to a git commit here, we can at least discuss the approach before re-basing on the latest main branch.

dgf commented 5 months ago

I have rebuilt the changes to the current version. Unfortunately, I can't test this with Quarkus at the moment because the class references have changed. But that should be sufficient as a basis for further exchanges about that topic.

dgf commented 5 months ago

Solved on Quarkus itself by using a sorted map, see https://github.com/quarkusio/quarkus/issues/40222

In general, I still think it's a good idea to have a predefined order for responses, but no longer necessary, at least, for our specific case.

phillip-kruger commented 5 months ago

Closing here. Thanks