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

[CXF-8361] Finalized support for Jakarta NS #746

Merged
merged 2 commits into from
Mar 2, 2021

Conversation

ropalka
Copy link
Contributor

@ropalka ropalka commented Feb 16, 2021

https://issues.apache.org/jira/browse/CXF-8361

  • introduced new org.apache.cxf.jaxws.handler.jakarta_types package for JAXB de/serialization
  • added new methods in HandlerChainBuilder
  • enhanced handlers de/serialization tests

@reta
Copy link
Member

reta commented Feb 19, 2021

Thanks for the PR, @ropalka, I think we got to the point when it is probably make sense to completely separate Jakartaee and Javaee name handler chains (I think you would agree). I suggest to:

  1. Introduce org.apache.cxf.jaxws.handler.jakartaee and org.apache.cxf.jaxws.handler.javaee packages
  2. Introduce JakartaeeHandlerChainBuilder and JavaeeHandlerChainBuilder
  3. Basically, I think it safe to assume that either JakartaEE or JavaEE namespace is going to be used, so in this case AnnotationHandlerChainBuilder could inspect the namespace, instantiate JakartaeeHandlerChainBuilder orJavaeeHandlerChainBuilder, and delegate to one of those. In this case, the conditionals should be gone and implementation becomes much cleaner.

What do you think?

@ropalka
Copy link
Contributor Author

ropalka commented Feb 24, 2021

Hi @reta . It makes sense to me. What I am worry about is backward compatibility.
Isn't jaxws frontend considered to be a public API? In other words if I would refactor package
from org.apache.cxf.jaxws.handler.types to org.apache.cxf.jaxws.handler.javaee
I would break compatibility.

@reta
Copy link
Member

reta commented Feb 24, 2021

Thanks for the reply @ropalka. We won't be changing the public API, which is outlined by AnnotationHandlerChainBuilder. The JakartaeeHandlerChainBuilder and JavaeeHandlerChainBuilder would encapsulate the implementation specifics to each namespace, and are not supposed to be used outside AnnotationHandlerChainBuilder. All the existing classes, under org.apache.cxf.jaxws.handler.types fe, could stay were they are. What do you think?

@ropalka
Copy link
Contributor Author

ropalka commented Feb 26, 2021

Hi @reta . The problem with your proposal is AnnotationHandlerChainBuilder is extending HandlerChainBuilder class.
If I would introduce both JakartaeeHandlerChainBuilder and JavaeeHandlerChain builders I would need to drop
"AnnotationHandlerChainBuilder extends HandlerChainBuilder" relationship which is again not public API backward compatible change. The proposal PR I sent is 100% backward compatible.

@ropalka
Copy link
Contributor Author

ropalka commented Feb 26, 2021

I enhanced this PR to be as much backward compatible as possible @reta.
I introduced new org.apache.cxf.jaxws.handler.jakartaee package as you proposed.
I left org.apache.cxf.jaxws.handler.types package untouched because of BC.
I introduced JAXB deserialization adaptors to be able to eliminate HandlerChainBuilder
'jakartaee' methods I indroduced in previous pull request proposal. WDYT about it now?

 * introduced new org.apache.cxf.jaxws.handler.jakartaee package for JAXB de/serialization
 * introduced JakartaEE to JavaEE adaptors for JAXB types
 * enhanced handlers de/serialization tests
@reta
Copy link
Member

reta commented Feb 27, 2021

I enhanced this PR to be as much backward compatible as possible @reta.
I introduced new org.apache.cxf.jaxws.handler.jakartaee package as you proposed.
I left org.apache.cxf.jaxws.handler.types package untouched because of BC.
I introduced JAXB deserialization adaptors to be able to eliminate HandlerChainBuilder
'jakartaee' methods I indroduced in previous pull request proposal. WDYT about it now?

@ropalka thank you for the effort, it definitely looks better but the main problem is still unsolved: the processing of the JavaEE and JakartaEE namespaces is tangled. May I ask you please to take at look at #756, it is based of master (without your changes) but illustrates the idea of how the processing could be separated. AFAIK it does not change or alter any public APIs, only the implementation details have been moved around. Would appreciate to hear any concerns, thank you.

@ropalka
Copy link
Contributor Author

ropalka commented Mar 1, 2021

I updated this PR as you suggested @reta, please review.

@reta
Copy link
Member

reta commented Mar 2, 2021

@ropalka thank you, LGTM, I have a few minor comments which I could address myself if you prefer, otherwise - I think we are good to go

@reta
Copy link
Member

reta commented Mar 2, 2021

Thanks again for your work, @ropalka

@reta reta merged commit b18b665 into apache:3.3.x-fixes Mar 2, 2021
reta pushed a commit that referenced this pull request Mar 3, 2021
* [CXF-8361] Finalized support for Jakarta NS
 * introduced new org.apache.cxf.jaxws.handler.jakartaee package for JAXB de/serialization
 * introduced JakartaEE to JavaEE adaptors for JAXB types
 * enhanced handlers de/serialization tests

* [CXF-8361] Separating Jakarta EE & Java EE handler chain implementation details
reta pushed a commit that referenced this pull request Mar 3, 2021
* [CXF-8361] Finalized support for Jakarta NS
 * introduced new org.apache.cxf.jaxws.handler.jakartaee package for JAXB de/serialization
 * introduced JakartaEE to JavaEE adaptors for JAXB types
 * enhanced handlers de/serialization tests

* [CXF-8361] Separating Jakarta EE & Java EE handler chain implementation details
@ropalka ropalka deleted the CXF-8361 branch March 3, 2021 13:40
@lulseged
Copy link

lulseged commented Apr 20, 2021

I have tried 3.5.0-SNAPSHOT and miss a lot of jakarta.* packages. When do you plan to move to jakarta namespaces?
The change will not be backward compatible. What is the use of making it backward compatible?
I would create 4.0.0-SNAPSHOT and start jakarta namespace from there and keep 3.x.x javax namespace.

I am getting java.lang.NoClassDefFoundError: javax.annotation.Resource when using 3.5.0-SNAPSHOT.

@reta
Copy link
Member

reta commented Apr 20, 2021

@lulseged please follow #737

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.

4 participants