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

Allow extensions to exclude classes to be found in the JAXB context #31651

Closed
wants to merge 1 commit into from

Conversation

Sgitario
Copy link
Contributor

@Sgitario Sgitario commented Mar 7, 2023

Fix #31646

@Sgitario Sgitario requested a review from ppalaga March 7, 2023 11:27
@quarkus-bot quarkus-bot bot added the area/jaxb label Mar 7, 2023
@gsmet
Copy link
Member

gsmet commented Mar 7, 2023

Hmmm, sorry but I will have to ask candid questions: I don't understand why we are trying to push all the classes to a unique JAXBContext.

Can't we imagine that an extension is creating a JAXBContext with the classes that are actually needed and this subset wouldn't contain any collisions?

I wasn't entirely sure about this collision thing when the original PR was created and I have even more doubts now.

Note that I'm seeing that from very far as it has been a long time since I played with this extension but I would appreciate if we could discuss this and if you could explain me why I'm all wrong :). Thanks!

@quarkus-bot
Copy link

quarkus-bot bot commented Mar 7, 2023

✔️ 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.

@Sgitario
Copy link
Contributor Author

Sgitario commented Mar 7, 2023

The idea was to ease the usage of JAXB to be similar to Jackson, where you can use the same ObjectMapper instance to serialize/deserialize objects into JSON.

Having said that, I'm not even sure if I was who introduced this :/, but to me, it makes sense how it works.

@gsmet
Copy link
Member

gsmet commented Mar 7, 2023

I don't know. I'm not convinced it is a good idea given how many problems it causes.

Maybe it should be done the other way and have the extensions actually register the classes, instead of excluding them? But even so, I'm not sure if it's a good idea to have a unique JAXBContext with everything.

@Sgitario
Copy link
Contributor Author

Sgitario commented Mar 7, 2023

I don't know. I'm not convinced it is a good idea given how many problems it causes.

Maybe it should be done the other way and have the extensions actually register the classes, instead of excluding them? But even so, I'm not sure if it's a good idea to have a unique JAXBContext with everything.

I'm not aware of how many problems this change is causing (but the issue with the BuildIT and the camel extensions, though these two issues were due to transitive dependencies that ran into conflicts in JAXB).

Maybe, instead of trying to see if this is done for good or the opposite, we should analyze or clarify why this extension is being used and its expectations. As far as I see, this extension is doing two things:

  1. Adapt JAXB types to be compatible with native
  2. Discover all the JAXB types and bind the discovered JAXB types to a generated JAXBContext

At the moment and as far as I know, the generated JAXBContext is used by the Resteasy Reactive and the Resteasy Reactive REST Client extensions.

Therefore, maybe and as @ppalaga suggested, it makes sense to split this extension into one for point 1. and keep the jaxb extension for point 2.

In the future, we could also refine the logic that includes all the discovered JAXB types to the JAXBContext to be less intrusive (I'm not sure how we could do this, since we don't know if one class is coming from a direct dependency or a transitive dependency, maybe does the Index help with this?).

@Sgitario
Copy link
Contributor Author

Sgitario commented Mar 8, 2023

Closing as the linked issue has been fixed by #31666.
If needed, we can always include this in the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/jaxb triage/invalid This doesn't seem right
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow third party extensions to avoid validating the default JAXB context
2 participants