dev@javaserverfaces.java.net

[Review] Fix for RI issue 134, 162 and legacy resolver bug

From: Jayashri Visvanathan <Jayashri.Visvanathan_at_Sun.COM>
Date: Mon, 08 Aug 2005 18:40:39 -0700

M src/com/sun/faces/application/ApplicationAssociate.java
M src/com/sun/faces/application/ApplicationImpl.java
  - Fix for JSFRI issue 162. FacesException should be thrown if
    illegal ValueExpression is passed to createComponent().
  - When setPropertyResolver() or setVariableResolver is invoked,
    no manipulation is required to satisfied the requirements for
    JSF 1.2. It is required only if decorator pattern is followed.
  - JSF RI 134. Synchronize on Application or SessionMap instead of
    locking on ApplicationAssociate.
 
M src/com/sun/faces/config/ConfigureListener.java
  - Remove DummyProperty/VariableImpl placed at the beginning of
    the legacy chain. This was causing the legacy resolvers
    following the decorator pattern to be not invoked.

M src/com/sun/faces/util/Util.java
  log the exception when an adapter constructor is not found.

M test/com/sun/faces/TestOldVariableResolver.java
M test/com/sun/faces/TestPropertyResolver.java
M test/com/sun/faces/TestVariableResolver.java
M test/com/sun/faces/application/PropertyResolverTestImpl.java
M test/com/sun/faces/application/TestApplicationImpl.java
M web/test/WEB-INF/faces-config.xml
  Test case for decorator pattern and JSFRI 162.

Note: Sorry to combine to issues. Fix for JSFRI issue 134 and 162 are
also posted to the issues. I meant to work in a fresh workspace when I
started on the Legacy resolver issue bug by mistake worked in the same one.


Index: src/com/sun/faces/application/ApplicationAssociate.java
===================================================================
RCS file: /cvs/javaserverfaces-sources/jsf-ri/src/com/sun/faces/application/ApplicationAssociate.java,v
retrieving revision 1.17
diff -u -r1.17 ApplicationAssociate.java
--- src/com/sun/faces/application/ApplicationAssociate.java 29 Jul 2005 20:10:24 -0000 1.17
+++ src/com/sun/faces/application/ApplicationAssociate.java 9 Aug 2005 01:36:42 -0000
@@ -201,15 +201,7 @@
      * Application.getPropertyResolver()
      */
     public PropertyResolver getLegacyPropertyResolver(){
- // place DummyPropertyResolver at the chain that sets the
- // propertyResolved to true to satisfy the requirements of unified EL.
- if (legacyPropertyResolver != null) {
- Object instance = Util.createInstance(
- "com.sun.faces.el.DummyPropertyResolverImpl",
- PropertyResolver.class, legacyPropertyResolver);
- return ((PropertyResolver) instance);
- }
- return null;
+ return legacyPropertyResolver;
     }
     
     /**
@@ -225,15 +217,7 @@
      * Application.getVariableResolver()
      */
     public VariableResolver getLegacyVariableResolver(){
- // place DummyVariableResolver at the chain that sets the
- // propertyResolved to true to satisfy the requirements of new EL.
- if (legacyVariableResolver != null) {
- Object instance = Util.createInstance(
- "com.sun.faces.el.DummyVariableResolverImpl",
- VariableResolver.class, legacyVariableResolver);
- return ((VariableResolver) instance);
- }
- return null;
+ return legacyVariableResolver;
     }
     
 
@@ -422,30 +406,48 @@
         if ((scopeIsApplication =
             scope.equalsIgnoreCase(RIConstants.APPLICATION)) ||
             (scopeIsSession = scope.equalsIgnoreCase(RIConstants.SESSION))) {
- synchronized (this) {
- try {
- bean = managedBean.newInstance(context);
- if (logger.isLoggable(Level.FINE)) {
- logger.fine("Created bean " + managedBeanName
- + " successfully ");
+ if (scopeIsApplication) {
+ Map applicationMap = context.getExternalContext().
+ getApplicationMap();
+ synchronized (applicationMap) {
+ try {
+ bean = managedBean.newInstance(context);
+ if (logger.isLoggable(Level.FINE)) {
+ logger.fine("Created application scoped bean " +
+ managedBeanName + " successfully ");
+ }
+ } catch (Exception ex) {
+ Object[] params = {managedBeanName};
+ if (logger.isLoggable(Level.SEVERE)) {
+ logger.log(Level.SEVERE,
+ "jsf.managed_bean_creation_error", params);
+ }
+ throw new FacesException(ex);
                     }
- } catch (Exception ex) {
- Object[] params = {managedBeanName};
- if (logger.isLoggable(Level.SEVERE)) {
- logger.log(Level.SEVERE,
- "jsf.managed_bean_creation_error", params);
+ //add bean to appropriate scope
+ applicationMap.put(managedBeanName, bean);
+ }
+ } else {
+ Map sessionMap = Util.getSessionMap(context);
+ synchronized (sessionMap) {
+ try {
+ bean = managedBean.newInstance(context);
+ if (logger.isLoggable(Level.FINE)) {
+ logger.fine("Created session scoped bean "
+ + managedBeanName + " successfully ");
+ }
+ } catch (Exception ex) {
+ Object[] params = {managedBeanName};
+ if (logger.isLoggable(Level.SEVERE)) {
+ logger.log(Level.SEVERE,
+ "jsf.managed_bean_creation_error", params);
+ }
+ throw new FacesException(ex);
                     }
- throw new FacesException(ex);
- }
- //add bean to appropriate scope
- if (scopeIsApplication) {
- context.getExternalContext().
- getApplicationMap().put(managedBeanName, bean);
+ //add bean to appropriate scope
+ sessionMap.put(managedBeanName, bean);
                 }
- if (scopeIsSession) {
- Util.getSessionMap(context).put(managedBeanName, bean);
- }
- }
+ }
         } else {
             scopeIsRequest = scope.equalsIgnoreCase(RIConstants.REQUEST);
             try {
Index: src/com/sun/faces/application/ApplicationImpl.java
===================================================================
RCS file: /cvs/javaserverfaces-sources/jsf-ri/src/com/sun/faces/application/ApplicationImpl.java,v
retrieving revision 1.65
diff -u -r1.65 ApplicationImpl.java
--- src/com/sun/faces/application/ApplicationImpl.java 21 Jul 2005 00:56:39 -0000 1.65
+++ src/com/sun/faces/application/ApplicationImpl.java 9 Aug 2005 01:36:42 -0000
@@ -183,14 +183,15 @@
                 result = this.createComponent(componentType);
                 componentExpression.setValue((context.getELContext()), result);
             }
- } catch (ELException elex) {
- throw new FacesException(elex);
+ } catch (Exception ex) {
+ throw new FacesException(ex);
         }
 
         return (UIComponent) result;
     }
 
     public ELResolver getELResolver() {
+
         if (compositeELResolver != null) {
             return compositeELResolver;
         }
@@ -537,17 +538,26 @@
             message = message +" componentType " + componentType;
             throw new NullPointerException(message);
         }
- UIComponent returnVal = (UIComponent) newThing(componentType,
- componentMap);
+ UIComponent returnVal = null;
+ try {
+ returnVal = (UIComponent) newThing(componentType, componentMap);
+ } catch (Exception ex) {
+ if (logger.isLoggable(Level.WARNING)) {
+ logger.log(Level.WARNING,
+ "jsf.cannot_instantiate_component_error", ex);
+ }
+ throw new FacesException(ex);
+ }
         if (returnVal == null) {
             Object[] params = {componentType};
             if (logger.isLoggable(Level.SEVERE)) {
- logger.log(Level.SEVERE,
- "jsf.cannot_instantiate_component_error", params);
+ logger.log(Level.SEVERE,
+ "jsf.cannot_instantiate_component_error", params);
             }
             throw new FacesException(Util.getExceptionMessageString(
- Util.NAMED_OBJECT_NOT_FOUND_ERROR_MESSAGE_ID, params));
+ Util.NAMED_OBJECT_NOT_FOUND_ERROR_MESSAGE_ID, params));
         }
+
         if (logger.isLoggable(Level.FINE)) {
             logger.log(Level.FINE, "Created component " + componentType);
         }
@@ -570,17 +580,20 @@
 
         Object result = null;
         boolean createOne = false;
-
- if (null != (result = componentBinding.getValue(context))) {
- // if the result is not an instance of UIComponent
- createOne = (!(result instanceof UIComponent));
- // we have to create one.
- }
- if (null == result || createOne) {
- result = this.createComponent(componentType);
- componentBinding.setValue(context, result);
+ try {
+ if (null != (result = componentBinding.getValue(context))) {
+ // if the result is not an instance of UIComponent
+ createOne = (!(result instanceof UIComponent));
+ // we have to create one.
+ }
+
+ if (null == result || createOne) {
+ result = this.createComponent(componentType);
+ componentBinding.setValue(context, result);
+ }
+ } catch (Exception ex) {
+ throw new FacesException(ex);
         }
-
         return (UIComponent) result;
     }
 
@@ -925,10 +938,8 @@
         Object result = null;
         Class clazz = null;
         Object value = null;
-
         synchronized (this) {
             value = map.get(key);
-
             if (value == null) {
                 return null;
             }
@@ -950,11 +961,8 @@
             result = clazz.newInstance();
         } catch (Throwable t) {
             Object[] params = {clazz.getName()};
- if (logger.isLoggable(Level.SEVERE)) {
- logger.log(Level.SEVERE, t.getMessage(), t);
- }
- throw new FacesException(Util.getExceptionMessageString(
- Util.CANT_INSTANTIATE_CLASS_ERROR_MESSAGE_ID, params));
+ throw new FacesException((Util.getExceptionMessageString(
+ Util.CANT_INSTANTIATE_CLASS_ERROR_MESSAGE_ID, params)), t);
         }
         return result;
     }
Index: src/com/sun/faces/config/ConfigureListener.java
===================================================================
RCS file: /cvs/javaserverfaces-sources/jsf-ri/src/com/sun/faces/config/ConfigureListener.java,v
retrieving revision 1.45
diff -u -r1.45 ConfigureListener.java
--- src/com/sun/faces/config/ConfigureListener.java 3 Aug 2005 02:24:26 -0000 1.45
+++ src/com/sun/faces/config/ConfigureListener.java 9 Aug 2005 01:36:42 -0000
@@ -480,7 +480,7 @@
         ApplicationAssociate associate =
                 ApplicationAssociate.getInstance(getExternalContextDuringInitialize());
         
- Object instance;
+ Object instance = null;
         Object prevInChain = null;
         String value;
         String[] values;
@@ -542,23 +542,21 @@
         values = config.getPropertyResolvers();
         if ((values != null) && (values.length > 0)) {
             // initialize the prevInChain to DummyPropertyResolver instance
- // to satisfy decorator pattern
+ // to satisfy decorator pattern as well as satisfy the requirements
+ // of unified EL. This resolver sets propertyResolved to false on
+ // invocation of its method, so that variable resolution can move
+ // further in the chain.
             prevInChain = new DummyPropertyResolverImpl();
             for (int i = 0; i < values.length; i++) {
                 if (logger.isLoggable(Level.FINER)) {
                     logger.finer("setPropertyResolver(" + values[i] + ')');
                 }
                 instance = Util.createInstance(values[i],
- PropertyResolver.class, prevInChain);
+ PropertyResolver.class, ((PropertyResolver)prevInChain));
                 prevInChain = instance;
             }
- // place DummyPropertyResolver at the chain that sets the
- // propertyResolved to true to satisfy the requirements of new EL.
- instance = Util.createInstance(
- "com.sun.faces.el.DummyPropertyResolverImpl",
- PropertyResolver.class, prevInChain);
- legacyPRChainHead = (PropertyResolver) instance;
- associate.setLegacyPRChainHead(legacyPRChainHead);
+ legacyPRChainHead = (PropertyResolver) instance;
+ associate.setLegacyPRChainHead(legacyPRChainHead);
         }
         
         // process custom el-resolver elements if any
@@ -596,8 +594,11 @@
         prevInChain = null;
         values = config.getVariableResolvers();
         if ((values != null) && (values.length > 0)) {
- // initialize the prevInChain DummyVariableResolver instance to
- // satisfy decorator pattern
+ // initialize the prevInChain to DummyVariableResolver instance
+ // to satisfy decorator pattern as well as satisfy the requirements
+ // of unified EL. This resolver sets propertyResolved to false on
+ // invocation of its method, so that variable resolution can move
+ // further in the chain.
             prevInChain = new DummyVariableResolverImpl();
             for (int i = 0; i < values.length; i++) {
                 if (logger.isLoggable(Level.FINER)) {
@@ -607,13 +608,8 @@
                         (values[i], VariableResolver.class, prevInChain);
                 prevInChain = instance;
             }
- // place DummyVariableResolver at the chain that sets the
- // propertyResolved to true to satisfy the requirements of new EL.
- instance = Util.createInstance(
- "com.sun.faces.el.DummyVariableResolverImpl",
- VariableResolver.class, prevInChain);
- legacyVRChainHead = (VariableResolver) instance;
- associate.setLegacyVRChainHead(legacyVRChainHead);
+ legacyVRChainHead = (VariableResolver) instance;
+ associate.setLegacyVRChainHead(legacyVRChainHead);
         }
 
         values = config.getViewHandlers();
Index: src/com/sun/faces/util/Util.java
===================================================================
RCS file: /cvs/javaserverfaces-sources/jsf-ri/src/com/sun/faces/util/Util.java,v
retrieving revision 1.167
diff -u -r1.167 Util.java
--- src/com/sun/faces/util/Util.java 22 Jul 2005 16:58:21 -0000 1.167
+++ src/com/sun/faces/util/Util.java 9 Aug 2005 01:36:42 -0000
@@ -1309,8 +1309,11 @@
                                 clazz.getConstructor(parameterTypes);
                             Object[] parameters = new Object[]{root};
                             returnObject = construct.newInstance(parameters);
- } catch (NoSuchMethodException nsme) {
+ } catch (Exception ex) {
                             // OK - there's no adapter constructor
+ if ( logger.isLoggable(Level.INFO)) {
+ logger.log(Level.INFO, ex.getMessage(), ex);
+ }
                         }
                     }
 
Index: test/com/sun/faces/TestOldVariableResolver.java
===================================================================
RCS file: /cvs/javaserverfaces-sources/jsf-ri/test/com/sun/faces/TestOldVariableResolver.java,v
retrieving revision 1.5
diff -u -r1.5 TestOldVariableResolver.java
--- test/com/sun/faces/TestOldVariableResolver.java 6 May 2005 22:02:03 -0000 1.5
+++ test/com/sun/faces/TestOldVariableResolver.java 9 Aug 2005 01:36:43 -0000
@@ -17,8 +17,9 @@
 
 public class TestOldVariableResolver extends VariableResolver {
    
- public TestOldVariableResolver() {
-
+ VariableResolver resolver = null;
+ public TestOldVariableResolver(VariableResolver variableResolver) {
+ this.resolver = variableResolver;
     }
     
     //
@@ -28,7 +29,10 @@
     // Specified by javax.faces.el.VariableResolver.resolveVariable()
     public Object resolveVariable(FacesContext context, String name)
             throws EvaluationException {
- return null;
+ if (name.equals("customVRTest2")) {
+ return "TestOldVariableResolver";
+ }
+ return resolver.resolveVariable(context, name);
     }
 
 }
Index: test/com/sun/faces/TestPropertyResolver.java
===================================================================
RCS file: /cvs/javaserverfaces-sources/jsf-ri/test/com/sun/faces/TestPropertyResolver.java,v
retrieving revision 1.5
diff -u -r1.5 TestPropertyResolver.java
--- test/com/sun/faces/TestPropertyResolver.java 6 May 2005 22:02:03 -0000 1.5
+++ test/com/sun/faces/TestPropertyResolver.java 9 Aug 2005 01:36:43 -0000
@@ -17,50 +17,58 @@
 
 public class TestPropertyResolver extends PropertyResolver {
    
+ PropertyResolver root = null;
     public TestPropertyResolver(PropertyResolver propertyResolver) {
+ root = propertyResolver;
     }
-
-
- // Specified by javax.faces.el.PropertyResolver.getType(Object,int)
- public Class getType(Object base, int index)
- throws EvaluationException, PropertyNotFoundException{
- return null;
+
+ // Specified by javax.faces.el.PropertyResolver.getValue(Object,String)
+ public Object getValue(Object base, Object property) {
+ if (property.equals("customPRTest1")) {
+ return "TestPropertyResolver";
+ }
+ return root.getValue(base, property);
     }
 
- // Specified by javax.faces.el.PropertyResolver.getType(Object,String)
- public Class getType(Object base, Object property) {
- return null;
+ public Object getValue(Object base, int index)
+ throws EvaluationException, PropertyNotFoundException {
+ return root.getValue(base, index);
     }
 
- // Specified by javax.faces.el.PropertyResolver.getValue(Object,int)
- public Object getValue(Object base, int index) {
- return null;
+
+ public void setValue(Object base, Object name, Object value)
+ throws EvaluationException, PropertyNotFoundException {
+ root.setValue(base, name, value);
     }
 
- // Specified by javax.faces.el.PropertyResolver.getValue(Object,String)
- public Object getValue(Object base, Object property) {
- Object result = null;
- return result;
+
+ public void setValue(Object base, int index, Object value)
+ throws EvaluationException, PropertyNotFoundException {
+ root.setValue(base, index, value);
     }
 
- // Specified by javax.faces.el.PropertyResolver.isReadOnly(Object,int)
- public boolean isReadOnly(Object base, int index) {
- return false;
+
+ public boolean isReadOnly(Object base, Object name)
+ throws PropertyNotFoundException {
+ return root.isReadOnly(base, name);
     }
 
- // Specified by javax.faces.el.PropertyResolver.isReadOnly(Object,String)
- public boolean isReadOnly(Object base, Object property) {
- return false;
+
+ public boolean isReadOnly(Object base, int index)
+ throws PropertyNotFoundException {
+ return root.isReadOnly(base, index);
     }
 
- // Specified by javax.faces.el.PropertyResolver.setValue(Object,int,Object)
- public void setValue(Object base, int index, Object value) {
-
+
+ public Class getType(Object base, Object name)
+ throws PropertyNotFoundException {
+ return root.getType(base, name);
     }
 
- // Specified by
- // javax.faces.el.PropertyResolver.setValue(Object,String,Object)
- public void setValue(Object base, Object property, Object value) {
+
+ public Class getType(Object base, int index)
+ throws PropertyNotFoundException {
+ return root.getType(base, index);
     }
     
 }
Index: test/com/sun/faces/TestVariableResolver.java
===================================================================
RCS file: /cvs/javaserverfaces-sources/jsf-ri/test/com/sun/faces/TestVariableResolver.java,v
retrieving revision 1.5
diff -u -r1.5 TestVariableResolver.java
--- test/com/sun/faces/TestVariableResolver.java 6 May 2005 22:02:03 -0000 1.5
+++ test/com/sun/faces/TestVariableResolver.java 9 Aug 2005 01:36:43 -0000
@@ -17,8 +17,9 @@
 
 public class TestVariableResolver extends VariableResolver {
    
+ VariableResolver resolver = null;
     public TestVariableResolver(VariableResolver variableResolver) {
-
+ this.resolver = variableResolver;
     }
     
     //
@@ -28,7 +29,10 @@
     // Specified by javax.faces.el.VariableResolver.resolveVariable()
     public Object resolveVariable(FacesContext context, String name)
             throws EvaluationException {
- return null;
+ if (name.equals("customVRTest1")) {
+ return "TestVariableResolver";
+ }
+ return resolver.resolveVariable(context, name);
     }
 
 }
Index: test/com/sun/faces/application/PropertyResolverTestImpl.java
===================================================================
RCS file: /cvs/javaserverfaces-sources/jsf-ri/test/com/sun/faces/application/PropertyResolverTestImpl.java,v
retrieving revision 1.5
diff -u -r1.5 PropertyResolverTestImpl.java
--- test/com/sun/faces/application/PropertyResolverTestImpl.java 6 May 2005 22:02:03 -0000 1.5
+++ test/com/sun/faces/application/PropertyResolverTestImpl.java 9 Aug 2005 01:36:43 -0000
@@ -14,8 +14,18 @@
 
 public class PropertyResolverTestImpl extends TestPropertyResolver{
 
+ PropertyResolver root = null;
+
     public PropertyResolverTestImpl(PropertyResolver root) {
- super(root);
+ super(root);
+ this.root = root;
+ }
+
+ public Object getValue(Object base, Object property) {
+ if (property.equals("customPRTest2")) {
+ return "PropertyResolverTestImpl";
+ }
+ return root.getValue(base, property);
     }
 
 }
Index: test/com/sun/faces/application/TestApplicationImpl.java
===================================================================
RCS file: /cvs/javaserverfaces-sources/jsf-ri/test/com/sun/faces/application/TestApplicationImpl.java,v
retrieving revision 1.25
diff -u -r1.25 TestApplicationImpl.java
--- test/com/sun/faces/application/TestApplicationImpl.java 19 Jul 2005 19:33:18 -0000 1.25
+++ test/com/sun/faces/application/TestApplicationImpl.java 9 Aug 2005 01:36:43 -0000
@@ -426,6 +426,21 @@
             exceptionThrown = true;
         }
         assertTrue(exceptionThrown);
+
+ // make sure FacesException is thrown when a bogus ValueExpression is
+ // passed to createComponent. JSF RI Issue 162
+ assertTrue(null != (valueBinding =
+ application.getExpressionFactory().createValueExpression(
+ getFacesContext().getELContext(), "#{a.b}", Object.class)));
+
+ try {
+ result = application.createComponent(valueBinding, getFacesContext(),
+ "notreached");
+ assertTrue(false);
+ } catch (FacesException e) {
+ exceptionThrown = true;
+ }
+ assertTrue(exceptionThrown);
     }
 
 
@@ -508,6 +523,36 @@
         value = rb.getString("label");
         assertEquals("Abflug", value);
         
+ }
+
+ public void testLegacyPropertyResolversWithUnifiedEL() {
+
+ ValueExpression ve1 = application.getExpressionFactory().
+ createValueExpression(getFacesContext().getELContext(),
+ "#{mixedBean.customPRTest1}", Object.class);
+ Object result = ve1.getValue(getFacesContext().getELContext());
+ assertTrue(result.equals("TestPropertyResolver"));
+
+ ValueExpression ve2 = application.getExpressionFactory().
+ createValueExpression(getFacesContext().getELContext(),
+ "#{mixedBean.customPRTest2}", Object.class);
+ result = ve2.getValue(getFacesContext().getELContext());
+ assertTrue(result.equals("PropertyResolverTestImpl"));
+ }
+
+ public void testLegacyVariableResolversWithUnifiedEL() {
+
+ ValueExpression ve1 = application.getExpressionFactory().
+ createValueExpression(getFacesContext().getELContext(),
+ "#{customVRTest1}", Object.class);
+ Object result = ve1.getValue(getFacesContext().getELContext());
+ assertTrue(result.equals("TestVariableResolver"));
+
+ ValueExpression ve2 = application.getExpressionFactory().
+ createValueExpression(getFacesContext().getELContext(),
+ "#{customVRTest2}", Object.class);
+ result = ve2.getValue(getFacesContext().getELContext());
+ assertTrue(result.equals("TestOldVariableResolver"));
     }
     
     public static void clearResourceBundlesFromAssociate(ApplicationImpl application) {
Index: web/test/WEB-INF/faces-config.xml
===================================================================
RCS file: /cvs/javaserverfaces-sources/jsf-ri/web/test/WEB-INF/faces-config.xml,v
retrieving revision 1.26
diff -u -r1.26 faces-config.xml
--- web/test/WEB-INF/faces-config.xml 19 Jul 2005 19:33:19 -0000 1.26
+++ web/test/WEB-INF/faces-config.xml 9 Aug 2005 01:36:43 -0000
@@ -103,6 +103,7 @@
       com.sun.faces.TestNavigationHandler
     </navigation-handler>
     <property-resolver>com.sun.faces.TestPropertyResolver</property-resolver>
+ <property-resolver>com.sun.faces.application.PropertyResolverTestImpl</property-resolver>
     <variable-resolver>com.sun.faces.TestVariableResolver</variable-resolver>
     <view-handler>com.sun.faces.TestViewHandler</view-handler>