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

Ensure that jakarta json types can be deserialized in native mode #45097

Merged
merged 1 commit into from
Dec 13, 2024

Conversation

This comment has been minimized.

@geoand geoand requested a review from gastaldi December 13, 2024 04:26
Copy link
Member

@gsmet gsmet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added some questions.

I'm curious though, how can this lead to the method not being registered in native?

Comment on lines 34 to 44
// @Override
// public boolean isReadable(Class<?> type, Type genericType, ResteasyReactiveResourceInfo lazyMethod,
// MediaType mediaType) {
// return JsonObject.class.isAssignableFrom(type);
// }
//
// @Override
// public JsonObject readFrom(Class<JsonObject> type, Type genericType, MediaType mediaType,
// ServerRequestContext context) throws WebApplicationException, IOException {
// return JsonpUtil.reader(context.getInputStream(), mediaType).readObject();
// }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why this commented out code?

Copy link
Contributor Author

@geoand geoand Dec 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By mistake, I'll fix it

Comment on lines +31 to +42
RestAssured
.given()
.contentType("application/json")
.body("""
{
"name": "foo"
}
""")
.when().post("/json")
.then()
.statusCode(200)
.body(containsString("foo"));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we add the test in something more core than the Qute ITs. Reasoning is that if at some point we drop Qute, we will lose these tests which are related to basic core features.

(Yeah, sorry for being annoying!)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I looked for another place and didn't find a good candidate.

Agreed it's not a good place, but I have no better candidates in mind

@geoand
Copy link
Contributor Author

geoand commented Dec 13, 2024

I'm curious though, how can this lead to the method not being registered in native?

In Quarkus REST the built in readers and writers don't need access to the annotations, and using our custom interfaces ensures that

Copy link

quarkus-bot bot commented Dec 13, 2024

Status for workflow Quarkus CI

This is the status report for running Quarkus CI on commit 0879ce4.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

You can consult the Develocity build scans.

@geoand geoand merged commit 3caeb08 into quarkusio:main Dec 13, 2024
46 checks passed
@quarkus-bot quarkus-bot bot added this to the 3.18 - main milestone Dec 13, 2024
@geoand geoand deleted the #45084 branch December 13, 2024 12:21
@gsmet gsmet modified the milestones: 3.18 - main, 3.17.5 Dec 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants