Skip to content

Conversation

@cavallium
Copy link
Contributor

The internal __absolute_uri__ flag is breaking the code that checks if the model is empty

Motivation:

OpenAPI 3.1 specifications do not work because vert.x populates the model with its own fields, then checks if the model is empty

Conformance:

You should have signed the Eclipse Contributor Agreement as explained in https://github.com/eclipse/vert.x/blob/master/CONTRIBUTING.md
Please also make sure you adhere to the code style guidelines: https://github.com/vert-x3/wiki/wiki/Vert.x-code-style-guidelines

The internal __absolute_uri__ flag is breaking the code that checks if the model is empty
@pk-work
Copy link
Contributor

pk-work commented Dec 2, 2024

Hi @cavallium thanks for finding this bug! Could you also provide a test case to ensure that we don't run into this issue again?

@cavallium
Copy link
Contributor Author

A test can't guarantee this with the current code, since that variable can contain fields that do not exist on the defined specification.
I suggest to merge this temporarily, and then refactor the class in a way that makes it possible to check if the original unmodified model is really empty.

@pk-work
Copy link
Contributor

pk-work commented Dec 5, 2024

I am convinced that we can write a test that will prevent exactly this problem from occurring again. We can also directly write a test, to ensure that the other two annotations __absolute_recursive_ref__ and __absolute_ref__ [1] will not result in the same problem.

I will not merge a fix without a test, because otherwise nobody will add a test later. MediaTypeImplTest would be a good place for testing this issue.

And yes, long term we should fix it in vertx-json-schema.

[1] https://github.com/eclipse-vertx/vertx-json-schema/blob/1ad80a82191aa3e6184ac642e3eaf9cc3aaec686/src/main/java/io/vertx/json/schema/impl/JsonObjectSchema.java#L22

@cavallium
Copy link
Contributor Author

I am convinced that we can write a test that will prevent exactly this problem from occurring again. We can also directly write a test, to ensure that the other two annotations __absolute_recursive_ref__ and __absolute_ref__ [1] will not result in the same problem.

I will not merge a fix without a test, because otherwise nobody will add a test later. MediaTypeImplTest would be a good place for testing this issue.

And yes, long term we should fix it in vertx-json-schema.

[1] https://github.com/eclipse-vertx/vertx-json-schema/blob/1ad80a82191aa3e6184ac642e3eaf9cc3aaec686/src/main/java/io/vertx/json/schema/impl/JsonObjectSchema.java#L22

Should MediaType#getSchema() return the "__*" annotations?

@cavallium
Copy link
Contributor Author

cavallium commented Dec 5, 2024

@pk-work I have fixed the existing test instead of creating a new one, since it was already checking for that behavior, but it wasn't checking for internal json-schema annotations.

@pk-work
Copy link
Contributor

pk-work commented Dec 5, 2024

Cool! I think we could improve the readability of the code and avoid code duplication, be re-using (copying) the created JsonObjects. Instead of writing multipel times new JsonObject().put(ABS_URI, DUMMY_REF_VALUE) you could create a JsonObejct schemaWithABSUri and re-use it. In case you use merge call copy()before.

I think this will reduce the code in testGetters() a lot.

@cavallium
Copy link
Contributor Author

Cool! I think we could improve the readability of the code and avoid code duplication, be re-using (copying) the created JsonObjects. Instead of writing multipel times new JsonObject().put(ABS_URI, DUMMY_REF_VALUE) you could create a JsonObejct schemaWithABSUri and re-use it. In case you use merge call copy()before.

I think this will reduce the code in testGetters() a lot.

Done, thanks for your suggestion, it's much better now

@pk-work pk-work merged commit 92bac72 into eclipse-vertx:main Dec 9, 2024
5 checks passed
@pk-work
Copy link
Contributor

pk-work commented Dec 9, 2024

Thank you very much for your contribution and your patience to add the best possible test!

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.

2 participants