Generate reflection declaration from MP OpenAPI schema definition #1389
Conversation
|
Could you please provide a descriptive PR summary? It's OK to have "Fixes..." in the body but the title should say what the PR is actually for. |
machi1990
changed the title
|
@dmlloyd I have renamed the PR. Should the same be done for the commit message? |
Yes please. |
There was a problem hiding this comment.
@machi1990 I added another round of comments. We're nearly there!
...teasy/deployment/src/main/java/io/quarkus/resteasy/deployment/ResteasyScanningProcessor.java
Outdated
Show resolved
Hide resolved
...teasy/deployment/src/main/java/io/quarkus/resteasy/deployment/ResteasyScanningProcessor.java
Outdated
Show resolved
Hide resolved
integration-tests/main/src/main/java/io/quarkus/example/rest/TestResource.java
Outdated
Show resolved
Hide resolved
integration-tests/main/src/main/java/io/quarkus/example/rest/TestResource.java
Outdated
Show resolved
Hide resolved
integration-tests/main/src/main/java/io/quarkus/example/rest/TestResource.java
Show resolved
Hide resolved
|
@machi1990 I let you finish this? Looks like we just need some minor adjustments to the constants and the tests, now. |
@gsmet looks like we are good. I do not understand why the build is failing on the CI though. |
machi1990
force-pushed
the
fix-745
branch
2 times, most recently
from
b5b6795
to
8f5e644
Compare
|
machi1990
force-pushed
the
fix-745
branch
2 times, most recently
from
80268ef
to
016f870
Compare
|
@gsmet I have finished integrating the requested changes and rebased the branch with master. This is ready for another round of review. |
|
Thanks, it's on my TODO list for today. |
|
@machi1990 I just force pushed a rebase to hopefully get CI working correctly. Review planned for later today. |
| public void testOpenApiSchemaResponse() { | ||
| RestAssured.when().get("/test/openapi/responses").then() | ||
| .body("name", is("my openapi entity name")); | ||
| } |
There was a problem hiding this comment.
You're not testing the "/openapi/responses/{version}" path.
Otherwise, it looks good.
Could you rebase and take care of it?
There was a problem hiding this comment.
Great. I have rebased the master and force pushed to the branch.
|
Thanks to you too. I can contribute to something else if need be. |
|
Coming late to this, but wondering whether it makes sense for this to be done as part of the Mostly because if you're using MP OpenAPI annotations, without adding that dependency yourself you're only going to have that if the |
|
@kenfinnigan Aaaah... sorry, just saw your message when I clicked the Merge button as I was waiting for this to go green. So the rationale was to try to centralize the serialization reflection registration so that we have a global view of what's going on in the RESTEasy extension. That's something we can discuss for sure, we can move the code if needed. |
|
Just seems odd for RESTEasy extension to need to know about OpenAPI, and potentially lots of other things |
|
Oops, I too moved a bit fast and deleted my local branch. I think @kenfinnigan is making a interesting point, and thanks to @gsmet for giving an element of response to the case. I took this approach for the reason stated above. Should we decide to move the code to |
|
@machi1990 Yeah, maybe let's do that. It shouldn't be too much of a hassle: you should move the code in the processor (and use the annotation classes as they will be in the classpath now). The tests can stay as they are. |
cescoffier
changed the title
Fixes #745.
The main idea is to scan for SCHEMA annotation and mark them as ReflectiveHierarchy
-Scan APIResponse annotation for schema declaration and mark the results (
implementation , not, oneOf, anyOf, allOf values) for reflection.-Scan APIResponses annotation and do as above.