MyFaces Core
  1. MyFaces Core
  2. MYFACES-2621

BeanValidation does not work with Unified EL 2.2

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.0.0-beta-3
    • Fix Version/s: 2.0.0
    • Component/s: JSR-314
    • Labels:
      None
    • Environment:
      bean validation 1.0.0.GA, unified el 2.2

      Description

      When using the new Unified EL 2.2, BeanValidation does not work, because _BeanValidatorUELUtils.getUELValueReferenceWrapper() always returns null.

      See also the related thread on the mailing list: http://www.mail-archive.com/users@myfaces.apache.org/msg55250.html

        Activity

        Hide
        Jakob Korherr added a comment -

        OK thanks, Jan-Kees. I just added volatile to all fields.

        Show
        Jakob Korherr added a comment - OK thanks, Jan-Kees. I just added volatile to all fields.
        Hide
        Jan-Kees van Andel added a comment -

        Looks good, you only need to declare the fields volatile, to ensure safe publication of the values.

        Performance impact of a volatile read is no issue.

        Show
        Jan-Kees van Andel added a comment - Looks good, you only need to declare the fields volatile, to ensure safe publication of the values. Performance impact of a volatile read is no issue.
        Hide
        Jakob Korherr added a comment -

        I removed synchronized from all ExternalSpecifications methods, because this is not needed. This issue is now fixed!

        Show
        Jakob Korherr added a comment - I removed synchronized from all ExternalSpecifications methods, because this is not needed. This issue is now fixed!
        Hide
        Martin Marinschek added a comment -

        Sounds like a totally sure case for removing synchronized

        best regards,

        Martin

        Show
        Martin Marinschek added a comment - Sounds like a totally sure case for removing synchronized best regards, Martin
        Hide
        Jakob Korherr added a comment -

        So this works now perfectly

        The only thing which I really don't like now is that we are calling ExternalSpecifications.isUnifiedELAvailable() very often (for every ValueExpression in TagAttributeImpl at least once) and this method is synchronized. And frankly, I really don't understand why this one has to be synchronized. It is null at first and then it changes to Boolean.TRUE or Boolean.FALSE and is never changed again, as Leonardo wrote before. When we're removing synchronized the only thing which can happen is that more than one thread sets it from null to true or false, but this really is no problem, because every thread will come up with the same result here!

        So removing synchronized should not cause any problems here or am I missing something? What were your concerns, Jan-Kees?

        Show
        Jakob Korherr added a comment - So this works now perfectly The only thing which I really don't like now is that we are calling ExternalSpecifications.isUnifiedELAvailable() very often (for every ValueExpression in TagAttributeImpl at least once) and this method is synchronized. And frankly, I really don't understand why this one has to be synchronized. It is null at first and then it changes to Boolean.TRUE or Boolean.FALSE and is never changed again, as Leonardo wrote before. When we're removing synchronized the only thing which can happen is that more than one thread sets it from null to true or false, but this really is no problem, because every thread will come up with the same result here! So removing synchronized should not cause any problems here or am I missing something? What were your concerns, Jan-Kees?
        Hide
        Jakob Korherr added a comment -

        Looking at all the different ValueExpression wrappers I figured out the following:

        TagValueExpression - needed for bean validation: created TagValueExpressionUEL
        LocationValueExpression - needed for bean validation: created LocationValueExpressionUEL
        ValueBindingToValueExpression - not needed: only used in ApplicationImpl to create a component with a binding
        MappedValueExpression, IndexedValueExpression, IteratedValueExpression - not needed: only used for c:forEach and bean validation does not work with properties of Map, List, array... because they can't be annotated
        LiteralValueExpression - not needed: bean validation only works for "real" properties of a class

        So I'll commit the solution with TagValueExpressionUEL and LocationValueExpressionUEL.

        Show
        Jakob Korherr added a comment - Looking at all the different ValueExpression wrappers I figured out the following: TagValueExpression - needed for bean validation: created TagValueExpressionUEL LocationValueExpression - needed for bean validation: created LocationValueExpressionUEL ValueBindingToValueExpression - not needed: only used in ApplicationImpl to create a component with a binding MappedValueExpression, IndexedValueExpression, IteratedValueExpression - not needed: only used for c:forEach and bean validation does not work with properties of Map, List, array... because they can't be annotated LiteralValueExpression - not needed: bean validation only works for "real" properties of a class So I'll commit the solution with TagValueExpressionUEL and LocationValueExpressionUEL.
        Hide
        Jakob Korherr added a comment -

        Thinking more about the problem with the wrappers I came up with the following solution: just create a sub-class of every wrapper that overrides ValueReference correctly and choose at creation time whether to use the "original" wrapper or the sub-class. I also already tried it out with TagValueExpression and it works just fine!

        Simple, easy, fast. Any objections to this solution?

        Show
        Jakob Korherr added a comment - Thinking more about the problem with the wrappers I came up with the following solution: just create a sub-class of every wrapper that overrides ValueReference correctly and choose at creation time whether to use the "original" wrapper or the sub-class. I also already tried it out with TagValueExpression and it works just fine! Simple, easy, fast. Any objections to this solution?
        Hide
        Jakob Korherr added a comment -

        I committed a first solution to this problem. The code tries to use getValueReference() if el-api 2.2 is available and falls back to the "normal" way if this returns null.

        This works for all scenarios, except that getValueReference() always returns null for our wrapped ValueExpressions. I added a todo for that on the code and I will work on that tomorrow!

        Show
        Jakob Korherr added a comment - I committed a first solution to this problem. The code tries to use getValueReference() if el-api 2.2 is available and falls back to the "normal" way if this returns null. This works for all scenarios, except that getValueReference() always returns null for our wrapped ValueExpressions. I added a todo for that on the code and I will work on that tomorrow!
        Hide
        Jakob Korherr added a comment -

        We need the ValueReference because it contains the base object and the property name to which the ValueExpression resolves. This is needed in BeanValidator, because we need to get the class of the managed bean and the right property name to get the right annotations out of it!

        Now with el-api 2.1 there is the trick to resolve the ValueExpression with a decorated ELContext and a special ELResolver to get both base object and property name (see class BeanValidator for the exact implementation). The new el-api however provides a method for this case called getValueReference() and the spec says that if it is available, we must use it instead of the trick.

        I'll try some things here, reflection and/or custom wrappers and figure out what is the best!

        Show
        Jakob Korherr added a comment - We need the ValueReference because it contains the base object and the property name to which the ValueExpression resolves. This is needed in BeanValidator, because we need to get the class of the managed bean and the right property name to get the right annotations out of it! Now with el-api 2.1 there is the trick to resolve the ValueExpression with a decorated ELContext and a special ELResolver to get both base object and property name (see class BeanValidator for the exact implementation). The new el-api however provides a method for this case called getValueReference() and the spec says that if it is available, we must use it instead of the trick. I'll try some things here, reflection and/or custom wrappers and figure out what is the best!
        Hide
        Leonardo Uribe added a comment -

        I miss one wrapper:

        org.apache.myfaces.view.facelets.tag.jstl.core.IteratedValueExpression

        In the case of LocationValueExpression, IndexedValueExpression and IteratedValueExpression we can use two wrappers hack. I'm just exploring ideas, to see which solution looks better.

        Really I don't like to change the signature of a method. If the method does not compile with el-api 2.2, that says something is wrong. In my personal opinion, the fact that it works, but does not compile with el-api 2.2 makes it not viable, because we could expect in the future (other versions of jsf) to upgrade to el-api 2.2, and in that case our code will break.

        Other thing we can do is when ValueReference returns null, do the same that we do when el 2.2 is not available, but I ignore the implications of do this. All depends of why and what we do with ValueReference in bean validation. Note I don't know all details behind current bean validation implementation.

        Show
        Leonardo Uribe added a comment - I miss one wrapper: org.apache.myfaces.view.facelets.tag.jstl.core.IteratedValueExpression In the case of LocationValueExpression, IndexedValueExpression and IteratedValueExpression we can use two wrappers hack. I'm just exploring ideas, to see which solution looks better. Really I don't like to change the signature of a method. If the method does not compile with el-api 2.2, that says something is wrong. In my personal opinion, the fact that it works, but does not compile with el-api 2.2 makes it not viable, because we could expect in the future (other versions of jsf) to upgrade to el-api 2.2, and in that case our code will break. Other thing we can do is when ValueReference returns null, do the same that we do when el 2.2 is not available, but I ignore the implications of do this. All depends of why and what we do with ValueReference in bean validation. Note I don't know all details behind current bean validation implementation.
        Hide
        Jakob Korherr added a comment -

        I totally agree with you Leonardo here! Also I want to be able to set those field using reflection for automated tests, so a big -1 from me on reverting ExternalSpecifications!!!!

        We can use Object getValueReference(ELContext context) here, because MyFaces is compiled with the el-api 2.1 dependency and so it does not know the newer method from the super class with return value ValueReference. At first I was also not sure if this works, but I tested it and it did. Of course, it won't compile with el-api 2.2, but the only thing to change here would be the return type...

        To your suggestion with FacesWrapper, Leonardo: we can't do this, because there are wrappers that have to do some work before and after every method of the wrapped ValueExpression is called (e.g. LocationValueExpression for ValueExpressions containing #

        {cc}

        ). Accessing the wrapper directly would change the expected behavior.

        Show
        Jakob Korherr added a comment - I totally agree with you Leonardo here! Also I want to be able to set those field using reflection for automated tests, so a big -1 from me on reverting ExternalSpecifications!!!! We can use Object getValueReference(ELContext context) here, because MyFaces is compiled with the el-api 2.1 dependency and so it does not know the newer method from the super class with return value ValueReference. At first I was also not sure if this works, but I tested it and it did. Of course, it won't compile with el-api 2.2, but the only thing to change here would be the return type... To your suggestion with FacesWrapper, Leonardo: we can't do this, because there are wrappers that have to do some work before and after every method of the wrapped ValueExpression is called (e.g. LocationValueExpression for ValueExpressions containing # {cc} ). Accessing the wrapper directly would change the expected behavior.
        Hide
        Leonardo Uribe added a comment -

        I have this list of wrappers on ValueExpression:

        javax.faces.component._ValueBindingToValueExpression (not used anymore, and in this case not relevant)
        org.apache.myfaces.view.facelets.tag.jstl.core.IndexedValueExpression
        org.apache.myfaces.view.facelets.el.TagValueExpression
        org.apache.myfaces.view.facelets.el.LocationValueExpression
        org.apache.myfaces.view.facelets.el.ELText.LiteralValueExpression

        Show
        Leonardo Uribe added a comment - I have this list of wrappers on ValueExpression: javax.faces.component._ValueBindingToValueExpression (not used anymore, and in this case not relevant) org.apache.myfaces.view.facelets.tag.jstl.core.IndexedValueExpression org.apache.myfaces.view.facelets.el.TagValueExpression org.apache.myfaces.view.facelets.el.LocationValueExpression org.apache.myfaces.view.facelets.el.ELText.LiteralValueExpression
        Hide
        Leonardo Uribe added a comment -

        I think we don't really need synchronize methods on ExternalSpecifications. The reason is the variables:

        private static Boolean beanValidationAvailable;
        private static Boolean unifiedELAvailable;

        only changes from null to Boolean.TRUE or from null to Boolean.FALSE, and after they are initialized, it never changes. So is multiple threads set this variable at the same time, everyone will set it with the same value (true or false, but never null). Ok, if we want to prevent that why don't use:

        private static volatile Boolean beanValidationAvailable;
        private static volatile Boolean unifiedELAvailable;

        that is "one time safe publication" pattern.

        Show
        Leonardo Uribe added a comment - I think we don't really need synchronize methods on ExternalSpecifications. The reason is the variables: private static Boolean beanValidationAvailable; private static Boolean unifiedELAvailable; only changes from null to Boolean.TRUE or from null to Boolean.FALSE, and after they are initialized, it never changes. So is multiple threads set this variable at the same time, everyone will set it with the same value (true or false, but never null). Ok, if we want to prevent that why don't use: private static volatile Boolean beanValidationAvailable; private static volatile Boolean unifiedELAvailable; that is "one time safe publication" pattern.
        Hide
        Leonardo Uribe added a comment -

        Use reflection has a performance hit. If we use duplicate wrappers, the only problem is the synchronized methods used for check if EL is available, but since this code is called many times, use those methods are not a good idea. We have to check which solution has better performance.

        Other idea we could use is make TagValueExpression implements FacesWrapper, so we can get the inner ValueExpression and call it directly.

        If there exists other ValueExpression wrappers that need to be handled, we need a list of the ones required to change in myfaces.

        The signature of ValueExpression.getValueReference is this:

        ValueReference getValueReference(ELContext context)

        My question is: Does really match the method with the proposed signature?

        Object getValueReference(ELContext context)

        If so, maybe use reflection is the way to go, but we have to try.

        Show
        Leonardo Uribe added a comment - Use reflection has a performance hit. If we use duplicate wrappers, the only problem is the synchronized methods used for check if EL is available, but since this code is called many times, use those methods are not a good idea. We have to check which solution has better performance. Other idea we could use is make TagValueExpression implements FacesWrapper, so we can get the inner ValueExpression and call it directly. If there exists other ValueExpression wrappers that need to be handled, we need a list of the ones required to change in myfaces. The signature of ValueExpression.getValueReference is this: ValueReference getValueReference(ELContext context) My question is: Does really match the method with the proposed signature? Object getValueReference(ELContext context) If so, maybe use reflection is the way to go, but we have to try.
        Hide
        Jan-Kees van Andel added a comment -

        My first implementation of ExternalSpecifications was purely designed for performance, relying on the ClassLoader to safely publish the variables. Testability was the reason to refactor it into synchronized methods.
        If you guys don't mind about less testability, we can always revert it to the original implementation which is probably the fastest possible implementation (no volatile/synchronized needed).

        A Double Checked Locking (DCL) implementation would also be a valid choice, since we depend on Java 5+ and the Java 5 JMM guarantees DCL correctness. That way, the first access requires an exclusive lock and consecutive calls only do volatile reads. This is still pretty fast without compromising testability.

        I would say, revert ExternalSpecifications to its original form and then look at Leonardo's suggestion.

        Show
        Jan-Kees van Andel added a comment - My first implementation of ExternalSpecifications was purely designed for performance, relying on the ClassLoader to safely publish the variables. Testability was the reason to refactor it into synchronized methods. If you guys don't mind about less testability, we can always revert it to the original implementation which is probably the fastest possible implementation (no volatile/synchronized needed). A Double Checked Locking (DCL) implementation would also be a valid choice, since we depend on Java 5+ and the Java 5 JMM guarantees DCL correctness. That way, the first access requires an exclusive lock and consecutive calls only do volatile reads. This is still pretty fast without compromising testability. I would say, revert ExternalSpecifications to its original form and then look at Leonardo's suggestion.
        Hide
        Jakob Korherr added a comment -

        Totally right!

        Of course, we could check if unified el 2.2 is available before the creation of the TagValueExpression and use an el-2.2 aware implementation of TagValueExpression instead. But this would lead to lots of duplicate code, because TagValueExpression is not the only ValueExpression wrapper. There are some more.

        Furthermore I don't think that this is that slow when using reflection. Also a big advantage here is for future versions of MyFaces that have el-2.2 as a requirement that we just have to change this one method to not use reflection in every wrapper.

        I am prefering the reflection way, but if you think that this is just too slow, we can also have the duplicate wrappers

        Show
        Jakob Korherr added a comment - Totally right! Of course, we could check if unified el 2.2 is available before the creation of the TagValueExpression and use an el-2.2 aware implementation of TagValueExpression instead. But this would lead to lots of duplicate code, because TagValueExpression is not the only ValueExpression wrapper. There are some more. Furthermore I don't think that this is that slow when using reflection. Also a big advantage here is for future versions of MyFaces that have el-2.2 as a requirement that we just have to change this one method to not use reflection in every wrapper. I am prefering the reflection way, but if you think that this is just too slow, we can also have the duplicate wrappers
        Hide
        Leonardo Uribe added a comment -

        org.apache.myfaces.view.facelets.el.TagValueExpression is created only from org.apache.myfaces.view.facelets.tag.TagAttributeImpl, and it is used as a wrapper from the real ValueExpression, right?

        Other alternative could be do something on TagAttributeImpl, so it uses another class like TagValueExpression, but that one override getValueReference, passing the call to the original one. The problem with this one is that this code is called many, many times, so it is critical to have an optimal implementation, and the current implementation of ExternalSpecifications has some synchronized methods that doesn't look good from my point of view. This code was let as is to allow run test configurations easily, but in this case I think it is more important code performace.

        Show
        Leonardo Uribe added a comment - org.apache.myfaces.view.facelets.el.TagValueExpression is created only from org.apache.myfaces.view.facelets.tag.TagAttributeImpl, and it is used as a wrapper from the real ValueExpression, right? Other alternative could be do something on TagAttributeImpl, so it uses another class like TagValueExpression, but that one override getValueReference, passing the call to the original one. The problem with this one is that this code is called many, many times, so it is critical to have an optimal implementation, and the current implementation of ExternalSpecifications has some synchronized methods that doesn't look good from my point of view. This code was let as is to allow run test configurations easily, but in this case I think it is more important code performace.
        Hide
        Jakob Korherr added a comment -

        We could do something like this in all our ValueExpression wrapper classes:

        public Object getValueReference(ELContext context)
        {
        try

        { Method method = ValueExpression.class.getMethod("getValueReference", ELContext.class); return method.invoke(this.orig, context); }

        catch (Exception e)

        { return null; }

        }

        I just tested it and it works great. What do you think?

        Show
        Jakob Korherr added a comment - We could do something like this in all our ValueExpression wrapper classes: public Object getValueReference(ELContext context) { try { Method method = ValueExpression.class.getMethod("getValueReference", ELContext.class); return method.invoke(this.orig, context); } catch (Exception e) { return null; } } I just tested it and it works great. What do you think?
        Hide
        Jakob Korherr added a comment -

        OK, change of plan. The javadoc of BeanValidator really says "If this application is running in an environment with a Unified EL Implementation for Java EE6 or later, obtain the ValueReference from valueExpression...", so we have to find a way to make this work correctly with wrappers.

        Show
        Jakob Korherr added a comment - OK, change of plan. The javadoc of BeanValidator really says "If this application is running in an environment with a Unified EL Implementation for Java EE6 or later, obtain the ValueReference from valueExpression...", so we have to find a way to make this work correctly with wrappers.
        Hide
        Jakob Korherr added a comment -

        The problem here is that the ValueExpression on which we invoke getValueReference() via reflection is a TagValueExpression and not directly a ValueExpression implementation of the unified el 2.2. So getValueReference() always returns null here, because it is not implemented.

        We now have two opportunities:

        1) Just don't care if el-2.2 is available and get the right base and property the old fashioned way
        2) Try to extract the real ValueExpression out of a series of wrappers and invoke getValueReference() on it

        I tend to 1) here, because a) it will always work correctly and b) we won't be much slower as with 2) here, because we need to use reflection here a couple of times in order to make it work and I don't think that this is very fast.

        Anyway, I will do some performance tests and find out what is faster.

        Does anyone know if there is a scenario where our "old-fashioned" way to retrieve the base and the property of the ValueExpression will break with the new unified el?

        Show
        Jakob Korherr added a comment - The problem here is that the ValueExpression on which we invoke getValueReference() via reflection is a TagValueExpression and not directly a ValueExpression implementation of the unified el 2.2. So getValueReference() always returns null here, because it is not implemented. We now have two opportunities: 1) Just don't care if el-2.2 is available and get the right base and property the old fashioned way 2) Try to extract the real ValueExpression out of a series of wrappers and invoke getValueReference() on it I tend to 1) here, because a) it will always work correctly and b) we won't be much slower as with 2) here, because we need to use reflection here a couple of times in order to make it work and I don't think that this is very fast. Anyway, I will do some performance tests and find out what is faster. Does anyone know if there is a scenario where our "old-fashioned" way to retrieve the base and the property of the ValueExpression will break with the new unified el?

          People

          • Assignee:
            Jakob Korherr
            Reporter:
            Jakob Korherr
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development