[CXF-7493] Add a default ClassUnwrapper for CDI integration. #313

johnament · 2017-09-18T10:56:41Z

This is to avoid issues with proxies and class metadata.

rmannibucau · 2017-09-18T13:09:37Z

integration/cdi/src/main/java/org/apache/cxf/cdi/CdiClassUnwrapper.java

+import org.apache.cxf.common.util.ClassUnwrapper;
+
+class CdiClassUnwrapper implements ClassUnwrapper {
+    private static final List<String> UNWRAPPED_MARKERS = asList("OwbNormalScopeProxy", "WeldClientProxy");


OWB has other suffixes, why not using $$ as in https://github.com/apache/meecrowave/blob/7bd58d4a254c0d40fb1a2ab0d19b24dbab4014ca/meecrowave-core/src/main/java/org/apache/meecrowave/cxf/MeecrowaveBus.java#L54 ? Would also handle TomEE proxies (EJB) BTW. Also any way it just gets merge with the default unwrapper? I don't see any reason to not have it out of the box since it doesn't depend on anything.

I have only seen the one suffix from OWB. Where can I find the list of suffixes? I saw what you did in TomEE, and I think using $$ could be problematic for non-CDI frameworks, so I wanted to explicitly list out the known suffixes.

I'm fine with putting something like this in core, and explicitly activating it in CDI, but would be concerned with being over eager with the swap out.

For owb you need to check AbstractProxyFactory subclasses out, without more checking I had in mind $$OwbSubClass. In TomEE we also have $$LocalBeanProxy which would break probably, and a lot of frameworks use $$ for that purpose. That is why we use $$ and not something more precise which will always hit a pitfall. Do you know any framework using $$ and would break cxf unwrapping the class? If so we can surely just make the suffixes configurable. Thoughts?

@johnament @rmannibucau Was going through this discussion (https://issues.jboss.org/browse/CDI-10), seems to be a known problem people struggle with. Not sure if it is better or not, what if we detect the CDI implementation at runtime (class probing) and delegate the unwrapping to the framework itself? F.e. in case of OWB, we could useNormalScopeProxyFactory.unwrapInstance(o), something similar should be available for Weld as well. Not ideal either, but more reliable may be? Thoughts?

Well, I don't think Weld has a behavior like unwrapInstance but it could be done by us if its Weld. I also figured that checking by name would be easier since it avoid any compile time dependencies.

It is definitely the easiest / simplest way to do that, not sure how reliable it is though ... May be we could have some regex, like ^.+$$.+Proxy$, the $$ usually depict a generated class, so we would cover Xxx$$OwbNormalScopeProxy, Xxx$$WeldClientProxy as well as Xxx$$LocalBeanProxy like @rmannibucau pointed out as well. Should be pretty good heuristic, what do you think?

johnament · 2017-09-19T22:49:11Z

integration/cdi/src/main/java/org/apache/cxf/cdi/CdiClassUnwrapper.java

+import org.apache.cxf.common.util.ClassUnwrapper;
+
+class CdiClassUnwrapper implements ClassUnwrapper {
+    private static final Pattern PROXY_PATTERN = Pattern.compile(".+\\$\\$.+Proxy");


@reta @rmannibucau ok, this uses a pattern based on the discussion. Do we still want to move outside the CDI module?

Thanks @johnament, I don't think it should be moved outside CDI integration, @rmannibucau any particular reasons to do that?

Yep the reason is to ensure it is used by all integrations. Very few server uses cdi extension to comply to spec but all use proxies. Since the perf and behavior shouldnt hurt it can be added to the default IMO

Hi guys, but the problem is only showing itself on the CDI path ?

Not really. it has nothing to do with CDI actually except CDI uses proxies. It will happen each time a lib is using subclassing proxies, CDI is one big consumer but there are a lot of other ways to get it even in plain standalone.

Works for me

Sounds like a good plan, @sberyozkin are we good to merge?

It's not clear what the plan is, just leave it?

Hi Andriy, if all 3 of you are happy, then yes, please merge :-), I did not really contribute myself

@johnament Sorry, was not clear, I was referring to @sberyozkin suggestion for 2 stages, let us start from CDI first, and than make a decision if it worth going to core or not. Make sense?

reta · 2017-09-20T11:33:54Z

integration/cdi/src/main/java/org/apache/cxf/cdi/CdiBusBean.java

@@ -63,6 +65,9 @@ public String getName() {
    @Override
    public ExtensionManagerBus create(final CreationalContext< ExtensionManagerBus > ctx) {
        final ExtensionManagerBus instance = injectionTarget.produce(ctx);
+        if ("true".equals(SystemPropertyAction.getProperty("org.apache.cxf.cdi.useCdiUnwrapper", "true"))) {


@johnament pretty minor, before merging could you please rename property org.apache.cxf.cdi.useCdiUnwrapper to org.apache.cxf.cdi.unwrap.proxies (or similar) so it kind of explains its purpose.

This is to avoid issues with proxies and class metadata.

johnament · 2017-09-20T22:44:08Z

Sure sounds good. Feel free to merge if good.

…

On Sep 20, 2017 6:41 PM, "Andriy Redko" ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In integration/cdi/src/main/java/org/apache/cxf/cdi/CdiClassUnwrapper.java <#313 (comment)>: > + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.cxf.cdi; + +import java.util.regex.Pattern; + +import org.apache.cxf.common.util.ClassUnwrapper; + +class CdiClassUnwrapper implements ClassUnwrapper { + private static final Pattern PROXY_PATTERN = Pattern.compile(".+\\$\\$.+Proxy"); @johnament <https://github.com/johnament> Sorry, was not clear, I was referring to @sberyozkin <https://github.com/sberyozkin> suggestion for 2 stages, let us start from CDI first, and than make a decision if it worth going to core or not. Make sense? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#313 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAGmh5HMBhr0c0Q4hzG8G44zuxOsC72Cks5skZSDgaJpZM4Pau6x> .

reta · 2017-09-21T00:01:59Z

Thanks @johnament !

[CXF-7493] Add a default ClassUnwrapper for CDI integration.

Ggrzybek alignment1

rmannibucau reviewed Sep 18, 2017

View reviewed changes

johnament force-pushed the CXF-7493 branch from 719ffff to 6b20079 Compare September 19, 2017 22:48

johnament commented Sep 19, 2017

View reviewed changes

reta reviewed Sep 20, 2017

View reviewed changes

[CXF-7493] Add a default ClassUnwrapper for CDI integration.

42efe37

This is to avoid issues with proxies and class metadata.

johnament force-pushed the CXF-7493 branch from 6b20079 to 42efe37 Compare September 20, 2017 11:43

reta merged commit 160eebb into apache:master Sep 21, 2017

reta added a commit that referenced this pull request Oct 8, 2017

Merge pull request #313 from johnament/CXF-7493

f150b0d

[CXF-7493] Add a default ClassUnwrapper for CDI integration.

andymc12 pushed a commit to andymc12/cxf that referenced this pull request Oct 25, 2017

Merge pull request apache#313 from johnament/CXF-7493

6899dda

[CXF-7493] Add a default ClassUnwrapper for CDI integration.

rnetuka pushed a commit to rnetuka/cxf that referenced this pull request Oct 3, 2022

Merge pull request apache#313 from jboss-fuse/ggrzybek-alignment1

4f09cb9

Ggrzybek alignment1

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CXF-7493] Add a default ClassUnwrapper for CDI integration. #313

[CXF-7493] Add a default ClassUnwrapper for CDI integration. #313

johnament commented Sep 18, 2017

rmannibucau Sep 18, 2017

johnament Sep 18, 2017

rmannibucau Sep 18, 2017 •

edited

reta Sep 18, 2017

johnament Sep 19, 2017

reta Sep 19, 2017

johnament Sep 19, 2017

reta Sep 20, 2017

rmannibucau Sep 20, 2017

sberyozkin Sep 20, 2017

rmannibucau Sep 20, 2017

rmannibucau Sep 20, 2017

reta Sep 20, 2017

johnament Sep 20, 2017

sberyozkin Sep 20, 2017

reta Sep 20, 2017

reta Sep 20, 2017

johnament Sep 20, 2017

johnament commented Sep 20, 2017 via email

reta commented Sep 21, 2017

[CXF-7493] Add a default ClassUnwrapper for CDI integration. #313

[CXF-7493] Add a default ClassUnwrapper for CDI integration. #313

Conversation

johnament commented Sep 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rmannibucau Sep 18, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnament commented Sep 20, 2017 via email

reta commented Sep 21, 2017

rmannibucau Sep 18, 2017 •

edited