Uploaded image for project: 'CDI Specification Issues'
  1. CDI Specification Issues
  2. CDI-527

allow proxying of classes with non-private final methods

    • Icon: Feature Request Feature Request
    • Resolution: Done
    • Icon: Major Major
    • 2.0 .Final
    • 1.2.Final
    • Beans

      Currently we explicitly disallow proxying of classes with non-private final methods.

      EJB does allow this. And there are a few final methods in the JDK and other libs. E.g. HashMap#initHashSeedAsNeeded. Currently we cannot have a producer method for it.

      We might rethink our decision and allow it. Probably with an own annotation like @AllowProxying which disables this check for certain cases (subclass managed-beans or producers).

            [CDI-527] allow proxying of classes with non-private final methods

            In F2F meeting we decided to add "ignoreFinalMethod" method to ProcessBeanAttribute.

            Adding support for @WithAnnotations on the ProcessBeanAttributes (CDI-270) will also help to create team specific annotation allow this.

            Antoine Sabot-Durand (Inactive) added a comment - - edited In F2F meeting we decided to add "ignoreFinalMethod" method to ProcessBeanAttribute . Adding support for @WithAnnotations on the ProcessBeanAttributes ( CDI-270 ) will also help to create team specific annotation allow this.

            +1 for IgnoreUnproxyableMethod*s* to be more generic.

            Thomas Andraschko (Inactive) added a comment - +1 for IgnoreUnproxyableMethod*s* to be more generic.

            Good point manovotn

            +1 for IgnoreFinalMethodProxying

            Antoine Sabot-Durand (Inactive) added a comment - Good point manovotn +1 for IgnoreFinalMethodProxying

            +1 for IgnoreFinalMethodProxying

            Reasons:

            • there is no real proxying going on for final methods, so word like ignore is better than allow
            • mentions that this annotation relates to final methods, making it clear
            • another good fit is IgnoreUnproxyableMethod, though I deem IgnoreFinalMethodProxying better because it underlines that you aim to ignore the process of proxy creation (rather than ignoring the method)

            Matěj Novotný added a comment - +1 for IgnoreFinalMethodProxying Reasons: there is no real proxying going on for final methods, so word like ignore is better than allow mentions that this annotation relates to final methods, making it clear another good fit is IgnoreUnproxyableMethod , though I deem IgnoreFinalMethodProxying better because it underlines that you aim to ignore the process of proxy creation (rather than ignoring the method)

            List of proposed names for the annotation so far:

            • AllowProxying
            • Passthrough
            • SupressUnproxyAbleResolution
            • ForceProxy
            • SuppressUnproxyableResolutionException
            • AllowFinalMethodProxying
            • SkipFinalMethodProxying
            • IgnoreFinalMethodProxying
            • SkipUnproxyableMethod
            • IgnoreUnproxyableMethod
            • PassOverUnproxyable
            • ByPassUnproxyable

            Antoine Sabot-Durand (Inactive) added a comment - - edited List of proposed names for the annotation so far: AllowProxying Passthrough SupressUnproxyAbleResolution ForceProxy SuppressUnproxyableResolutionException AllowFinalMethodProxying SkipFinalMethodProxying IgnoreFinalMethodProxying SkipUnproxyableMethod IgnoreUnproxyableMethod PassOverUnproxyable ByPassUnproxyable

            Suggestion: @Passthrough

            Romain Manni-Bucau (Inactive) added a comment - Suggestion: @Passthrough

            To sum up. We need to find the best name for the annotation (suggestions welcome) and add the SPI part for this feature. struberg what is your proposed roadmap on the PR update?

            Antoine Sabot-Durand (Inactive) added a comment - - edited To sum up. We need to find the best name for the annotation (suggestions welcome) and add the SPI part for this feature. struberg what is your proposed roadmap on the PR update?

            mkouba@redhat.com it is doable as generic as you mention of with a hardcoded list for known types depending the type of application. The main point is to allow this type of usage and to allow to solve the broken applications without modifying the existing code. The SPI solves the second point and the annotation the first one so sounds like a good compromise for past and future to me.

            Romain Manni-Bucau (Inactive) added a comment - mkouba@redhat.com it is doable as generic as you mention of with a hardcoded list for known types depending the type of application. The main point is to allow this type of usage and to allow to solve the broken applications without modifying the existing code. The SPI solves the second point and the annotation the first one so sounds like a good compromise for past and future to me.

            rmannibucau@gmail.com Ok, so we would need a configurable extension which would detect problematic AnnotatedTypes and add this annotation where appropriate. Sounds doable. Also I'm aware of pros and cons of portable and non-portable solutions.

            Martin Kouba added a comment - rmannibucau@gmail.com Ok, so we would need a configurable extension which would detect problematic AnnotatedTypes and add this annotation where appropriate. Sounds doable. Also I'm aware of pros and cons of portable and non-portable solutions.

            mkouba@redhat.com annotation => spi so we can write an extension solving issues for legacy apps so this is fine IMO. Big advantage is to allow a portable solution which is reliable on the long term (opposed to impl solutions)

            Romain Manni-Bucau (Inactive) added a comment - mkouba@redhat.com annotation => spi so we can write an extension solving issues for legacy apps so this is fine IMO. Big advantage is to allow a portable solution which is reliable on the long term (opposed to impl solutions)

            Correct me if I'm wrong, but if the only way of using this "feature" would be declaring an annotation than we will not be able to help legacy apps (compiled against older CDI API, no source available, etc.) - this was one of the requirements often cited in previous comments. Also it might be problematic to integrate a class we're not able to modify. Just for completeness - I'm ok with this as both impls provide a non-portable solution already.

            Martin Kouba added a comment - Correct me if I'm wrong, but if the only way of using this "feature" would be declaring an annotation than we will not be able to help legacy apps (compiled against older CDI API, no source available, etc.) - this was one of the requirements often cited in previous comments. Also it might be problematic to integrate a class we're not able to modify . Just for completeness - I'm ok with this as both impls provide a non-portable solution already.

            This feature was voted by the EG on Feb 15th 2016, but a majority of users expressed preference for the introduction of a dedicated annotation to support this advanced feature.
            @AllowProxy was the name proposed by struberg but we could also find a less low level name based on Business Method concept. suggestion are welcome.

            As we are about to introduce a new annotation related to proxy behavior, I thought interesting to think about using it for other requested feature. See for example my comment in CDI-414

            Antoine Sabot-Durand (Inactive) added a comment - This feature was voted by the EG on Feb 15th 2016, but a majority of users expressed preference for the introduction of a dedicated annotation to support this advanced feature. @AllowProxy was the name proposed by struberg but we could also find a less low level name based on Business Method concept. suggestion are welcome. As we are about to introduce a new annotation related to proxy behavior, I thought interesting to think about using it for other requested feature. See for example my comment in CDI-414

            beans_2_0.xsd added and pull request pushed.

            Mark Struberg (Inactive) added a comment - beans_2_0.xsd added and pull request pushed.

            first draft https://github.com/struberg/cdi/tree/CDI-527
            Github pull requests to the official repo is broken currently. Not sure why. See mail on the list.

            Mark Struberg (Inactive) added a comment - first draft https://github.com/struberg/cdi/tree/CDI-527 Github pull requests to the official repo is broken currently. Not sure why. See mail on the list.

            Martin, to come back to your argument that it won't help for legacy apps. It would not help if you have a sealed WAR/EAR anymore which you cannot change. But you can add a simple small jar to them which contains just the beans.xml with those additional settings almost always. You can even put this into the shared classpath without changing your EAR/WAR...
            So I think it is the closest we can get. Again: happy for any other good solution. But doing nothing is not an option in my opinion.

            Mark Struberg (Inactive) added a comment - Martin, to come back to your argument that it won't help for legacy apps. It would not help if you have a sealed WAR/EAR anymore which you cannot change. But you can add a simple small jar to them which contains just the beans.xml with those additional settings almost always. You can even put this into the shared classpath without changing your EAR/WAR... So I think it is the closest we can get. Again: happy for any other good solution. But doing nothing is not an option in my opinion.

            This has exactly NOTHING to do with merging info from beans.xml. Nor has @Alternative any problems with merging beans.xml btw. The ONLY case where this might make problems is for decorators and interceptors!

            Any other arguments? Because this is not a valid argument so far.

            Mark Struberg (Inactive) added a comment - This has exactly NOTHING to do with merging info from beans.xml. Nor has @Alternative any problems with merging beans.xml btw. The ONLY case where this might make problems is for decorators and interceptors! Any other arguments? Because this is not a valid argument so far.

            as I already said many weeks ago...

            Sometimes there is no good solution. Also for legacy applications this beans.xml approach wouldn't help either - if it is possible to change the app, then in most cases the "interface workaround" mentioned in A. might be used.

            And no, as discussed and agreed many times...

            Not agreed. However, this issue is not a good place to discuss the problem. One of the things which is not "perfectly covered" is merging of beans.xml within an existing EE module.

            Martin Kouba added a comment - as I already said many weeks ago... Sometimes there is no good solution. Also for legacy applications this beans.xml approach wouldn't help either - if it is possible to change the app, then in most cases the "interface workaround" mentioned in A. might be used. And no, as discussed and agreed many times... Not agreed. However, this issue is not a good place to discuss the problem. One of the things which is not "perfectly covered" is merging of beans.xml within an existing EE module.

            as I already said many weeks ago: give us a better solution and we will think about it. But blocking all solutions for a REAL issue is contraproductive...

            And no, as discussed and agreed many times (also by JBoss), both interpretations of the term BDA are perfectly covered by the spec.

            Mark Struberg (Inactive) added a comment - as I already said many weeks ago: give us a better solution and we will think about it. But blocking all solutions for a REAL issue is contraproductive... And no, as discussed and agreed many times (also by JBoss), both interpretations of the term BDA are perfectly covered by the spec.

            Hi Mark, I don't like this solution. beans.xml is per bean archive and "chapter 5 sense of BDA" is just an OWB interpretation (and wrong imho) - so nothing we can build other solutions on.

            Martin Kouba added a comment - Hi Mark, I don't like this solution. beans.xml is per bean archive and "chapter 5 sense of BDA" is just an OWB interpretation (and wrong imho) - so nothing we can build other solutions on.

            Oh and if you like this solution then we might also think about introducing a new allowProxying(String classname) or Class to BeforeBeanDiscovery

            Mark Struberg (Inactive) added a comment - Oh and if you like this solution then we might also think about introducing a new allowProxying(String classname) or Class to BeforeBeanDiscovery

            Happy new year!

            I think there is an agreement that we shall not remove the non-proxyable rules in general (Option B).
            Option A. is purely an educational thing.
            We might write some blog about it but we should not pollute the spec with long explanations.
            Those should go into a side letter instead.

            But I also grok Emilys statement and have a possible solution for it.
            We could enhance the beans.xml.

            <beans>
              <allowProxying>
                <class&gt;com.acme.MyClassWithFinalMethods</class&gt;
              </allowProxying>
            </beans>
            

            It would be in the chapter 5 sense of BDA of course.
            Otherwise it would make no sense at all as y'ou would have to re-package the jar again.
            Means having a jar with the above in an EARs /lib folder would allow it for the whole EAR.
            If it's just in an WAR WEB-INF/lib/ jar inside an EAR then it would only allow it for this very WAR in the EAR.

            The container could log out a WARNING if it finds such entries.
            Something like "Class XxxYyyy explicitly declared as proxyable despite having final methods!"

            Mark Struberg (Inactive) added a comment - Happy new year! I think there is an agreement that we shall not remove the non-proxyable rules in general (Option B). Option A. is purely an educational thing. We might write some blog about it but we should not pollute the spec with long explanations. Those should go into a side letter instead. But I also grok Emilys statement and have a possible solution for it. We could enhance the beans.xml. <beans> <allowProxying> < class& gt;com.acme.MyClassWithFinalMethods</ class& gt; </allowProxying> </beans> It would be in the chapter 5 sense of BDA of course. Otherwise it would make no sense at all as y'ou would have to re-package the jar again. Means having a jar with the above in an EARs /lib folder would allow it for the whole EAR. If it's just in an WAR WEB-INF/lib/ jar inside an EAR then it would only allow it for this very WAR in the EAR. The container could log out a WARNING if it finds such entries. Something like "Class XxxYyyy explicitly declared as proxyable despite having final methods!"

            Sorry, typo... You were right this initHashSeedAsNeeded was introduced to jdk7. I meant say their app will be broken once they move up to java7.

            Emily Jiang (Inactive) added a comment - Sorry, typo... You were right this initHashSeedAsNeeded was introduced to jdk7. I meant say their app will be broken once they move up to java7.

            But this final method is already in HashMap as of Java7. This is nothing which only got introduced with Java8...

            Mark Struberg (Inactive) added a comment - But this final method is already in HashMap as of Java7. This is nothing which only got introduced with Java8...

            >>Not sure about this. Most of this classes already have final protected methods as of Java7. Which class do you have in mind?
            HashMap#initHashSeedAsNeeded, as mentioned in your description.

            Emily Jiang (Inactive) added a comment - >>Not sure about this. Most of this classes already have final protected methods as of Java7. Which class do you have in mind? HashMap#initHashSeedAsNeeded, as mentioned in your description.

            french_c Imo a perfectly valid use case. In the case you described only the option with the excludes or with the special Annotation would be possible.

            emijiang6] Not sure about this. Most of this classes already have final protected methods as of Java7. Which class do you have in mind?

            Mark Struberg (Inactive) added a comment - french_c Imo a perfectly valid use case. In the case you described only the option with the excludes or with the special Annotation would be possible. emijiang6 ] Not sure about this. Most of this classes already have final protected methods as of Java7. Which class do you have in mind?

            >>I have never had an issue with java.* or javax.* being Unproxyable so far.
            I know a customer has hit this problem after moving up to Java8.

            Some customers use third party libraries and they don't have source code, so they cannot easily update the code. We can easily say "go to the open source community....". The reality is that it takes time and the customers' applications are not functioning and they cannot operate their business.

            Someone somehow has to offer help. I think we can address this problem with some careful design.

            Emily Jiang (Inactive) added a comment - >>I have never had an issue with java.* or javax.* being Unproxyable so far. I know a customer has hit this problem after moving up to Java8. Some customers use third party libraries and they don't have source code, so they cannot easily update the code. We can easily say "go to the open source community....". The reality is that it takes time and the customers' applications are not functioning and they cannot operate their business. Someone somehow has to offer help. I think we can address this problem with some careful design.

            The only situation where I have been running into this issue was integrating CDI in an existing framework, where the entry point to CDI should be as integrated as possible and therefore required extending a framework class with protected final methods. In other words: If you work with framework X you have to extend base class Y (e.g. a web framework action). To make most out of CDI I would love to have an CDI bean extending Y which failed due to the UnproxyableResolutionException.

            The biggest difference to what you discussed above is: I have to deal with lots of legacy code that should be migrated to a newer stack (slowly). Moving to CDI and CDI interceptors (@Transactional) requires to work with 10+ years old code, that uses base classes and protected final methods a lot. I have never had an issue with java.* or javax.* being Unproxyable so far.

            Jens Schumann (Inactive) added a comment - The only situation where I have been running into this issue was integrating CDI in an existing framework, where the entry point to CDI should be as integrated as possible and therefore required extending a framework class with protected final methods. In other words: If you work with framework X you have to extend base class Y (e.g. a web framework action). To make most out of CDI I would love to have an CDI bean extending Y which failed due to the UnproxyableResolutionException. The biggest difference to what you discussed above is: I have to deal with lots of legacy code that should be migrated to a newer stack (slowly). Moving to CDI and CDI interceptors (@Transactional) requires to work with 10+ years old code, that uses base classes and protected final methods a lot. I have never had an issue with java.* or javax.* being Unproxyable so far.

            Think we should stay close to java there and don't abuse of bytecode generation since we can end up in a state were we break java (extending classes and proxying final method is already). Proxying types need a no-op constructor for instance even if protected and this one is far more a real constraint when coding than final methods IMHO so users already know what is the technical impl (and implied constraints) so staying aligned on java keeps things understandable I think.

            More specifically on the new API: this proxying is not always possible for java.* cause some API are not "exposed enough" so it is a super risky hyposthesis and even if the new API is not used on producer code itself the user would need to flag it anyway so the portability of legacy applications is already not free and suppose you hit this issue, what's the fastest? Modifying your code with a Ctrl+R (replace) or checking the spec and implementation for a feature which is maybe not reliable?

            Romain Manni-Bucau (Inactive) added a comment - Think we should stay close to java there and don't abuse of bytecode generation since we can end up in a state were we break java (extending classes and proxying final method is already). Proxying types need a no-op constructor for instance even if protected and this one is far more a real constraint when coding than final methods IMHO so users already know what is the technical impl (and implied constraints) so staying aligned on java keeps things understandable I think. More specifically on the new API: this proxying is not always possible for java.* cause some API are not "exposed enough" so it is a super risky hyposthesis and even if the new API is not used on producer code itself the user would need to flag it anyway so the portability of legacy applications is already not free and suppose you hit this issue, what's the fastest? Modifying your code with a Ctrl+R (replace) or checking the spec and implementation for a feature which is maybe not reliable?

            Romain, the new API is used by CDI integrators (e.g. application servers) to set whether they would like to support this proxying on nonproxyable classes. Weld/OWB should provide the capability of proxying the unproxyable java. or javax. classes.

            Emily Jiang (Inactive) added a comment - Romain, the new API is used by CDI integrators (e.g. application servers) to set whether they would like to support this proxying on nonproxyable classes. Weld/OWB should provide the capability of proxying the unproxyable java. or javax. classes.

            emijiang6 any new API doesn't solve your issue since the applications are by hypothesis "legacy".

            Romain Manni-Bucau (Inactive) added a comment - emijiang6 any new API doesn't solve your issue since the applications are by hypothesis "legacy".

            Option A is easy but it does not help the customers with legacy applications. It is not their fault in the first place. The JDK change is beyond their control. If we want to make CDI popular, we should help the customers who are stuck if they are unable to update their legacy applications.

            Option D: Support proxy on all java.x or javax... classes. Since the system property does not sell well, how about introduce a method setAllowProxyingOnUnProxyable(boolean allowProxying) on CDI.java? If not set, false is the default.

            Emily Jiang (Inactive) added a comment - Option A is easy but it does not help the customers with legacy applications. It is not their fault in the first place. The JDK change is beyond their control. If we want to make CDI popular, we should help the customers who are stuck if they are unable to update their legacy applications. Option D: Support proxy on all java.x or javax... classes. Since the system property does not sell well, how about introduce a method setAllowProxyingOnUnProxyable(boolean allowProxying) on CDI.java? If not set, false is the default.

            +1 for A
            -1 for introducing a system property

            From the user point of view - creating a producer for a third-party class is always risky (and it's not only about final methods, consider for example passivation scopes). For now there are two workarounds for JDK's HashMap/ConcurrentHashMap and similar use-cases:

            1. use the Map/ConcurrentMap interface as the bean class field/return type - bean author
            2. inject the Map/ConcurrentMap interface - bean consumer (see also my comment https://issues.jboss.org/browse/CDI-527?focusedCommentId=13120531&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-13120531)

            We need to provide a way for the applications to continue working.

            Emily, I understand your point. But I don't think it's a good solution to transfer responsibility to CDI.

            Martin Kouba added a comment - +1 for A -1 for introducing a system property From the user point of view - creating a producer for a third-party class is always risky (and it's not only about final methods, consider for example passivation scopes). For now there are two workarounds for JDK's HashMap/ConcurrentHashMap and similar use-cases: use the Map/ConcurrentMap interface as the bean class field/return type - bean author inject the Map/ConcurrentMap interface - bean consumer (see also my comment https://issues.jboss.org/browse/CDI-527?focusedCommentId=13120531&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-13120531 ) We need to provide a way for the applications to continue working. Emily, I understand your point. But I don't think it's a good solution to transfer responsibility to CDI.

            +1 for A. As pointed out by rmannibucau@gmail.com, I cannot really imagine an application where this would be a blocker.

            While C sounds tempting at first, I am afraid of the level of complexity it would bring into the application. Especially for anyone who isn't the original author of the code in question.

            Matěj Novotný added a comment - +1 for A. As pointed out by rmannibucau@gmail.com , I cannot really imagine an application where this would be a blocker. While C sounds tempting at first, I am afraid of the level of complexity it would bring into the application. Especially for anyone who isn't the original author of the code in question.

            emijiang6 Also remember that CDI-1.2 is only specified for Java7. Also note that EJB doesn't support this neither for NIVs...
            How would D look like in practice? System properties and excludes can be done in a non-portable way anyway. But adding system properties to any spec is in 99% a sign of a broken design.

            -1 for B. Just added it for completeness

            We also could have another option
            E.) introduce some class excludes via beans.xml

            But that would have the same issues like D imo.

            Imo C is the most explicit and easiest for users to grasp.

            Mark Struberg (Inactive) added a comment - emijiang6 Also remember that CDI-1 .2 is only specified for Java7. Also note that EJB doesn't support this neither for NIVs... How would D look like in practice? System properties and excludes can be done in a non-portable way anyway. But adding system properties to any spec is in 99% a sign of a broken design. -1 for B. Just added it for completeness We also could have another option E.) introduce some class excludes via beans.xml But that would have the same issues like D imo. Imo C is the most explicit and easiest for users to grasp.

            emijiang6 never has really been supported on CDI side, JDK broke some API (in a more vicious way than this jira) but doesnt mean CDi has to do anything. Do you have an example of a broken application cause of application code (and not JVM)?

            Romain Manni-Bucau (Inactive) added a comment - emijiang6 never has really been supported on CDI side, JDK broke some API (in a more vicious way than this jira) but doesnt mean CDi has to do anything. Do you have an example of a broken application cause of application code (and not JVM)?

            I am thinking from customers' view. For legacy applications falling into this category, they suddenly stopped working with the UnproxyableResolutionException exception throwing at runtime. Sometimes it is not realistic to update the applications. We need to provide a way for the applications to continue working.

            I think D is good and we can introduce a system property to disable this by default and only enable the support when the property is explicitly set.

            Emily Jiang (Inactive) added a comment - I am thinking from customers' view. For legacy applications falling into this category, they suddenly stopped working with the UnproxyableResolutionException exception throwing at runtime. Sometimes it is not realistic to update the applications. We need to provide a way for the applications to continue working. I think D is good and we can introduce a system property to disable this by default and only enable the support when the property is explicitly set.

            Tomas Remes added a comment -

            Yes +1 for A. I consider C as a bit of overkill too. It seems to me that it would bring rather confusion than clarification.

            Tomas Remes added a comment - Yes +1 for A. I consider C as a bit of overkill too. It seems to me that it would bring rather confusion than clarification.

            Think D is just not realistic enough to be reliable on the long term (+ better is the spec doesnt get 100 pages of whitelist/blackist )
            C sounds overkill for this particular and precise need

            I hesitate between A and B cause B gives the ability to just code "java" which is nice. Now I never saw an application seeing this as a real issue/blocker (means when it happens the reaction is: "ah? not allowed, ok let's do this other way") so A would work fine with a very low cost.

            Romain Manni-Bucau (Inactive) added a comment - Think D is just not realistic enough to be reliable on the long term (+ better is the spec doesnt get 100 pages of whitelist/blackist ) C sounds overkill for this particular and precise need I hesitate between A and B cause B gives the ability to just code "java" which is nice. Now I never saw an application seeing this as a real issue/blocker (means when it happens the reaction is: "ah? not allowed, ok let's do this other way") so A would work fine with a very low cost.

            I wrote down a summary of the problem and possible solutions for proxying with final fields:

            Java8 introduced a few final methods to existing classes.
            This now leads to a javax.enterprise.inject.UnproxyableResolutionException at bootstrap if you have a producer method or field for those classes.

            EJB also seemingly allows this.
            But it's specced on a completely different level.
            EJB disallows final 'business methods'.
            That means that whenever you have an interface only those methods will get proxied.
            The methods of the implementation do not matter at all.
            With NIV all methods are 'business methods' and must be proxyable imo (can someone verify this?).

            I personally like the non-proxyable rule.
            It really prevents tons o user errors.
            As you all know CDI heavily relies on subclassing proxies.
            Means we override all methods from the proxied type.
            But we obviously cannot override (and thus proxy) static, private and final methods.
            While static and private methods do not really matter, all non-private non-static final methods do.
            The problem appears when people invoke such a final method on the proxy.
            In that case they will just hit the proxy and not the Contextual Instance.
            To prevent the user from making this error we throw the UnproxyableResolutionException at bootstrap.

            We now have a few options to deal with that:

            A.) no it's not a problem at all.

            • We often got the report for ConcurrentHashMap.
              But people can already just use the ConcurrentMap interface as return type in their producers.
            • Also the report that this only makes a problem since Java8 is plain wrong imo.
              ConcurrentHashMap has the following method since at least Java7:
              final V put(K key, int hash, V value, boolean onlyIfAbsent)
            • There are a few situations where it hurts though.
              E.g. extending ConcurrentHashMap with own functionality.
              This would require introducing an own interface for this new functionality.

            B.) Generally allow proxying of beans with final methods.

            • I'm rather -1 for this.
              The current behaviour was introduced exactly because people made too many errors and blew up at runtime.
              Removing this rule would really make things worse.
              Because people might then add new final methods.
              And if you call those then you will only hit the proxy class and not the Contextual Instance.

            C.) Introduce an annotation to explicitly declare Beans to be proxyable

            • Something like @AllowProxying
              Can be applied to producer methods, producer fields and managed beans.
            • That would make it explicit for programmers that they leave the sunshine path.
              People will blow up with the UnproxyableResolutionException and can then decide whether they want to provide that bean anyway or not.
            • Con: Only the author of the Producer or managed bean knows that he added it.
              Thus other users will first need to look at the producer to become aware.
              But that might not be so much of a problem in most cases as it's only a matter of JavaDoc.

            D.) Introduce an exclude list for knowingly 'bad' classes.

            • We could e.g. introduce that all classes from java.* and javax.* are proxyable by definition.
            • Con: hard to know what's going on and why the code doesn't work if a user calls such a final method.

            Mark Struberg (Inactive) added a comment - I wrote down a summary of the problem and possible solutions for proxying with final fields: Java8 introduced a few final methods to existing classes. This now leads to a javax.enterprise.inject.UnproxyableResolutionException at bootstrap if you have a producer method or field for those classes. EJB also seemingly allows this. But it's specced on a completely different level. EJB disallows final 'business methods'. That means that whenever you have an interface only those methods will get proxied. The methods of the implementation do not matter at all. With NIV all methods are 'business methods' and must be proxyable imo (can someone verify this?). I personally like the non-proxyable rule. It really prevents tons o user errors. As you all know CDI heavily relies on subclassing proxies. Means we override all methods from the proxied type. But we obviously cannot override (and thus proxy) static, private and final methods. While static and private methods do not really matter, all non-private non-static final methods do. The problem appears when people invoke such a final method on the proxy. In that case they will just hit the proxy and not the Contextual Instance. To prevent the user from making this error we throw the UnproxyableResolutionException at bootstrap. We now have a few options to deal with that: A.) no it's not a problem at all. We often got the report for ConcurrentHashMap. But people can already just use the ConcurrentMap interface as return type in their producers. Also the report that this only makes a problem since Java8 is plain wrong imo. ConcurrentHashMap has the following method since at least Java7: final V put(K key, int hash, V value, boolean onlyIfAbsent) There are a few situations where it hurts though. E.g. extending ConcurrentHashMap with own functionality. This would require introducing an own interface for this new functionality. B.) Generally allow proxying of beans with final methods. I'm rather -1 for this. The current behaviour was introduced exactly because people made too many errors and blew up at runtime. Removing this rule would really make things worse. Because people might then add new final methods. And if you call those then you will only hit the proxy class and not the Contextual Instance. C.) Introduce an annotation to explicitly declare Beans to be proxyable Something like @AllowProxying Can be applied to producer methods, producer fields and managed beans. That would make it explicit for programmers that they leave the sunshine path. People will blow up with the UnproxyableResolutionException and can then decide whether they want to provide that bean anyway or not. Con: Only the author of the Producer or managed bean knows that he added it. Thus other users will first need to look at the producer to become aware. But that might not be so much of a problem in most cases as it's only a matter of JavaDoc. D.) Introduce an exclude list for knowingly 'bad' classes. We could e.g. introduce that all classes from java.* and javax.* are proxyable by definition. Con: hard to know what's going on and why the code doesn't work if a user calls such a final method.

            struberg Yes, it is about bean types. But the validation requirement is clearly defined:

            A bean type must be proxyable if an injection point resolves to a bean... Otherwise, the container automatically detects the problem, and treats it as a deployment problem.

            For extensions, EL, etc. - you get UnproxyableResolutionException at runtime. Also note that a contextual reference is not required to implement all bean types of the bean. Only the required bean type and all interfaces (see 6.5.3. Contextual reference for a bean).

            Martin Kouba added a comment - struberg Yes, it is about bean types. But the validation requirement is clearly defined: A bean type must be proxyable if an injection point resolves to a bean... Otherwise, the container automatically detects the problem, and treats it as a deployment problem. For extensions, EL, etc. - you get UnproxyableResolutionException at runtime. Also note that a contextual reference is not required to implement all bean types of the bean. Only the required bean type and all interfaces (see 6.5.3. Contextual reference for a bean).

            tremes I think this wording only got introduced pretty recently. Afaik it was not there in EJB-3.1. So I assume they 'fixed' it into the wrong direction. And I fear it's an illegal non-backward compatible change in the EJB spec...

            mkouba@redhat.com No, I think you are not right. Read the paragraph 3.15. It is all about unproxyable BEAN TYPES. Not the Injection Points, it is BEAN TYPES. got me?
            Only looking at injection points would also trash Extensions, EL and dynamic resolving (BeanManager#getBean by name or type).

            Mark Struberg (Inactive) added a comment - tremes I think this wording only got introduced pretty recently. Afaik it was not there in EJB-3.1. So I assume they 'fixed' it into the wrong direction. And I fear it's an illegal non-backward compatible change in the EJB spec... mkouba@redhat.com No, I think you are not right. Read the paragraph 3.15. It is all about unproxyable BEAN TYPES. Not the Injection Points, it is BEAN TYPES. got me? Only looking at injection points would also trash Extensions, EL and dynamic resolving (BeanManager#getBean by name or type).

            I am not sure I follow on this. On the NIV's it's explicitly stated in 4.9.8 Session Bean’s No-Interface View that:

            Only private methods of the bean class and any superclasses except java.lang.Object
            may be declared final.

            There is no problem from CDI point of view AFAIK. In the case of interface view types I can't see any problem as well because the proxy instance is created for the interface type (not impl) which has all methods public and not final. What's the reason for CDI to behave differently? In fact I think we can't supress EJB validations anyway.

            Tomas Remes added a comment - I am not sure I follow on this. On the NIV's it's explicitly stated in 4.9.8 Session Bean’s No-Interface View that: Only private methods of the bean class and any superclasses except java.lang.Object may be declared final. There is no problem from CDI point of view AFAIK. In the case of interface view types I can't see any problem as well because the proxy instance is created for the interface type (not impl) which has all methods public and not final. What's the reason for CDI to behave differently? In fact I think we can't supress EJB validations anyway.

            For the record - the spec only requires the injection points to be validated, or rather the required bean types to be proxyable. So for the HashMap from JDK7 a simple workaround would be to inject/use the Map interface.

            -1 for introducing a new annotation - this wouldn't help for third-party bean classes, e.g. the JDK7's HashMap example.

            Martin Kouba added a comment - For the record - the spec only requires the injection points to be validated, or rather the required bean types to be proxyable. So for the HashMap from JDK7 a simple workaround would be to inject/use the Map interface. -1 for introducing a new annotation - this wouldn't help for third-party bean classes, e.g. the JDK7's HashMap example.

            @Jozef: just use a parent class with a protected final method. This is not forbidden. That said 3.2 should remove this constraint since that's a regression from 3.1.

            last point: why do we care about EJB at all? CDi just allow an integration with EJB but EJB shouldn't be the first model (in particular because it defines this contract definition which is not the case in CDI where internal methods are seen as well).

            Romain Manni-Bucau (Inactive) added a comment - @Jozef: just use a parent class with a protected final method. This is not forbidden. That said 3.2 should remove this constraint since that's a regression from 3.1. last point: why do we care about EJB at all? CDi just allow an integration with EJB but EJB shouldn't be the first model (in particular because it defines this contract definition which is not the case in CDI where internal methods are seen as well).

            I still fail to see how EJB "allows this". Can you provide a code sample of a session bean with non-private final method that is valid according to EJB 3.2?

            Jozef Hartinger added a comment - I still fail to see how EJB "allows this". Can you provide a code sample of a session bean with non-private final method that is valid according to EJB 3.2?

            No, rather the opposite. EJB allows it. But most probably purely as side effect. Up to and including EJB-3.1 all non 'Business Methods' can be final. That includes package and protected methods for @Local and NIVs or even public methods if you have an EJB API (and the final public method in question is not in that set of API methods)! EJB really did only care about the proxying of their 'Business Methods'. From this pov CDI is really the same, BUT there is one huge difference: In CDI we treat much more methods as 'Business Methods'. E.g. it is perfectly fine to do a @Inject SelfClass self and invoke a protected or package scoped method on it. So for CDI all non-private, non-static methods are 'Business Methods' and can get intercepted, decorated, and proxied.

            Only in EJB-3.2 the NIV case was 'fixed'. Or rather got broken in hindsight of backward compat... I already had a talk with one from the EJB EG who agreed that this was most likely an unintentional backward incompatible change. It also makes zero sense for NIV EJBs as they explicitely declare that still only public methods are 'Business Methods'.

            Mark Struberg (Inactive) added a comment - - edited No, rather the opposite. EJB allows it. But most probably purely as side effect. Up to and including EJB-3.1 all non 'Business Methods' can be final. That includes package and protected methods for @Local and NIVs or even public methods if you have an EJB API (and the final public method in question is not in that set of API methods)! EJB really did only care about the proxying of their 'Business Methods'. From this pov CDI is really the same, BUT there is one huge difference: In CDI we treat much more methods as 'Business Methods'. E.g. it is perfectly fine to do a @Inject SelfClass self and invoke a protected or package scoped method on it. So for CDI all non-private, non-static methods are 'Business Methods' and can get intercepted, decorated, and proxied. Only in EJB-3.2 the NIV case was 'fixed'. Or rather got broken in hindsight of backward compat... I already had a talk with one from the EJB EG who agreed that this was most likely an unintentional backward incompatible change. It also makes zero sense for NIV EJBs as they explicitely declare that still only public methods are 'Business Methods'.

            OK, so EJB does not allow it either, correct?

            Jozef Hartinger added a comment - OK, so EJB does not allow it either, correct?

            rhn-engineering-jharting I digged a bit deeper. Seems EJB-3.2 introduced this (non backward compat) restriction. But only for NIVs!
            Previously this restriction was only for 'Business Methods'. Which always have been only public methods (this is what is different to CDI).

            Mark Struberg (Inactive) added a comment - rhn-engineering-jharting I digged a bit deeper. Seems EJB-3.2 introduced this (non backward compat) restriction. But only for NIVs! Previously this restriction was only for 'Business Methods'. Which always have been only public methods (this is what is different to CDI).

            I remember a discussion back then with Marina and Linda. And what what I remember the outcome was that it is depending on the term 'Business Method' where final is forbidden. And a 'Business Method' in EJBs must be public. See EJB spec 4.6.5.

            Mark Struberg (Inactive) added a comment - I remember a discussion back then with Marina and Linda. And what what I remember the outcome was that it is depending on the term 'Business Method' where final is forbidden. And a 'Business Method' in EJBs must be public. See EJB spec 4.6.5.

            it does

            EJB does allow this.

            Can you elaborate on how EJB allows that? Based on the following:

            Only private methods of the bean class and any superclasses except java.lang.Object
            may be declared final

            I interpret the EJB spec as explicitly prohibiting this.

            Jozef Hartinger added a comment - EJB does allow this. Can you elaborate on how EJB allows that? Based on the following: Only private methods of the bean class and any superclasses except java.lang.Object may be declared final I interpret the EJB spec as explicitly prohibiting this.

            I personally like the restriction in many cases. People should really be aware that they need to be careful when they proxy such classes. Although the people writing the producer might not be the people who consume it...

            Mark Struberg (Inactive) added a comment - I personally like the restriction in many cases. People should really be aware that they need to be careful when they proxy such classes. Although the people writing the producer might not be the people who consume it...

            +1 without any new API, this was just a technical constraint I guess which doesnt have real any sense in practise

            Romain Manni-Bucau (Inactive) added a comment - +1 without any new API, this was just a technical constraint I guess which doesnt have real any sense in practise

            +1.

            This would simplify CDI integration in a lot of cases. Do we have the same issue with / should talk about interceptors too?

            Jens Schumann (Inactive) added a comment - +1. This would simplify CDI integration in a lot of cases. Do we have the same issue with / should talk about interceptors too?

              asabotdu@redhat.com Antoine Sabot-Durand (Inactive)
              struberg Mark Struberg (Inactive)
              Votes:
              0 Vote for this issue
              Watchers:
              10 Start watching this issue

                Created:
                Updated:
                Resolved: