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.

Sign up for GitHub

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

Jump to bottom

Detect MessageBodyReader/Writer from META-INF/services/javax.ws.rs.ext.Providers #27981

Merged
merged 2 commits into from Sep 16, 2022
Merged

Detect MessageBodyReader/Writer from META-INF/services/javax.ws.rs.ext.Providers #27981

merged 2 commits into from Sep 16, 2022

Conversation

geoand
Copy link
Contributor

@geoand geoand commented Sep 16, 2022 •
edited

Resolves: #27970

P.S I haven't added a test for this because it requires us creating an extra module, but I can do that if we really think it's necessary

@geoand geoand mentioned this pull request Sep 16, 2022
Closed
Sgitario
Sgitario reviewed Sep 16, 2022
View reviewed changes
Copy link
Contributor

@Sgitario Sgitario left a comment

Choose a reason for hiding this comment

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

Even though it would require an extra module, I think these changes are quite important to cover and to ensure the functionality now and in the future.

.../src/main/java/io/quarkus/resteasy/reactive/server/deployment/ResteasyReactiveProcessor.java
} else {
builder.setMediaTypeStrings(Collections.singletonList(MediaType.WILDCARD_TYPE.toString()));
}
messageBodyWriterProducer.produce(builder.build()); // TODO: does it make sense to limit these to the Server?
Copy link
Contributor

Choose a reason for hiding this comment

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

do you mean to do the same for REST Client reactive? If so, I think it would make sense.

.../src/main/java/io/quarkus/resteasy/reactive/server/deployment/ResteasyReactiveProcessor.java Show resolved Hide resolved
@geoand
Copy link
Contributor Author

geoand commented Sep 16, 2022

Even though it would require an extra module, I think these changes are quite important to cover and to ensure the functionality now and in the future.

Do we have any such 'common' modules used in testing extension?

@geoand
Copy link
Contributor Author

geoand commented Sep 16, 2022

Nevermind, I see we have something called shared-library

@geoand
Copy link
Contributor Author

geoand commented Sep 16, 2022

Test added

Sgitario
Sgitario approved these changes Sep 16, 2022
View reviewed changes
Copy link
Contributor

@Sgitario Sgitario left a comment

Choose a reason for hiding this comment

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

lgtm. If we implement the same for REST Client reactive in the future, we could move some logic in the common module. For now, it's ok as it is. Thanks!

@sberyozkin sberyozkin mentioned this pull request Sep 16, 2022
Closed
@gsmet
Copy link
Member

gsmet commented Sep 16, 2022

A bit unrelated but not so much: I think this makes this more important to throw errors when both RESTEasy Reactive and RESTEasy Classic are around. Or RESTEasy Reactive will try to register the ones coming from Classic.
I thought we had something to detect this and throw errors but I saw several cases already when both are mixed and no errors are reported.

Not saying it should be done in the same PR but that's something that should be tackled.

@quarkus-bot

This comment has been minimized.

Sign in to view
@geoand
Copy link
Contributor Author

geoand commented Sep 16, 2022

I thought we had something to detect this and throw errors but I saw several cases already when both are mixed and no errors are reported.

We did, not sure what's going on now

@geoand
Copy link
Contributor Author

geoand commented Sep 16, 2022

So in this case there is not much we can do as it's not two conflicting extensions, it's a matter of RESTEasy being on the classpath. #27998 should fix it

@geoand
Copy link
Contributor Author

geoand commented Sep 16, 2022 •
edited

In this case, the failing test was a good thing as it allowed me to make the type resolution far more resilient.

@geoand geoand added the triage/waiting-for-ci Ready to merge when CI successfully finishes label Sep 16, 2022
@quarkus-bot
Copy link

quarkus-bot bot commented Sep 16, 2022

Failing Jobs - Building a1d000a

Status Name Step Failures Logs Raw logs
✖ Quickstarts Compilation - JDK 17 Compile Quickstarts Failures Logs Raw logs

Failures

⚙️ Quickstarts Compilation - JDK 17 #

- Failing: optaplanner-quickstart

📦 optaplanner-quickstart

✖ Failed to execute goal io.quarkus:quarkus-maven-plugin:999-SNAPSHOT:build (default) on project optaplanner-quickstart: Failed to build quarkus application

@geoand geoand merged commit 731a8f8 into quarkusio:main Sep 16, 2022
@quarkus-bot quarkus-bot bot added this to the 2.14 - main milestone Sep 16, 2022
@geoand geoand deleted the #27970 branch September 16, 2022 13:20
@quarkus-bot quarkus-bot bot added kind/bugfix and removed triage/waiting-for-ci Ready to merge when CI successfully finishes labels Sep 16, 2022
@michalvavrik michalvavrik mentioned this pull request Sep 18, 2022
Closed
@geoand geoand mentioned this pull request Sep 19, 2022
Merged
@gsmet gsmet removed this from the 2.14 - main milestone Sep 20, 2022
@gsmet gsmet added this to the 2.13.0.Final milestone Sep 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Sgitario Sgitario Sgitario approved these changes

Assignees
No one assigned
Labels
area/resteasy-reactive kind/bugfix
Projects
None yet
Milestone
2.13.0.Final
Development

Successfully merging this pull request may close these issues.

CloudEvent deserialization does not work with quarkus-resteasy-reactive
3 participants
@geoand @gsmet @Sgitario