SpringBeanELResolver - setValue throws PropertyNotWritableException [SPR-11502]

[spring-projects-issues]

Amit opened SPR-11502 and commented

I am trying to upgrade spring from 3.2.1 to 4.0.2 which requires us to use <el-resolver> instead of <variable-resolver> (it was deprecated from quite sometime). The upgrade throws an PropertyNotWritableException after using SpringBeanFacesELResolver. The exception is thrown by the setValue() method which checks if the requested bean is present in the BeanFactory. If found, a PropertyNotWritableException is thrown. I would like to understand the root cause of the exception which is not clear from its implementation.

public void setValue(ELContext elContext, Object base, Object property, Object value) throws ELException {
		if (base == null) {
			String beanName = property.toString();
			BeanFactory bf = getBeanFactory(elContext);
			if (bf.containsBean(beanName)) {
				throw new PropertyNotWritableException(
						"Variable '" + beanName + "' refers to a Spring bean which by definition is not writable");
			}
		}
	}

The setValue() implementation doesn't set the value but just do a check. On debugging I found the value of beanName, value and property refer to the same backing bean. Does that cause the issue? If so why?

---

Affects: 3.2.2

Backported to: 3.2.9

[spring-projects-issues]

Amit commented

@Juergen Hoeller - I noticed that you marked this issue as a bug. It would be nice if you could share the details about the issue.

[spring-projects-issues]

Juergen Hoeller commented

ELResolvers are getting chained, so all we have to do here is to identify a (base, property) pair that is meant to be handled by us, and to react accordingly. It seems that we are mis-identifying a pair as ours there when it is rather meant to be handled by a different ELResolver...

Could you please share details about when this is being invoked like that? Which expression triggers a setValue call with those parameter values?

Juergen

[spring-projects-issues]

Amit commented

We have a composite component defined that requires an handle to the parent backing bean. By parent backing bean I mean the backing bean of the page in which the composite component is rendered. The exception is seen for the expression where we inject the parent backing bean in the composite component backing bean.

[spring-projects-issues]

Amit commented

Did the above comment help?

[spring-projects-issues]

Juergen Hoeller commented

I have yet to dive further into the EL API here... It's still unclear to me why we get a setValue call with those parameter values in such a scenario.

So in the debugger, you've seen that the 'value' parameter is identical to beanName? I suppose we could explicitly skip that case through a beanName.equals(value) check...

Juergen

[spring-projects-issues]

Amit commented

I tried jumping into the composite component code to get more details (sorry for not doing this earlier).

We have overridden the processUpdates method of the UIComponentBase class in our composite component. We fetch the bindings and verify if any of the TagValueExpression's are readOnly(). If not, we try to set the value on it. The strange part is isReadOnly() returns true while setValue() throws an exception (both of them have similar method implementation in SpringBeanELResolver.

Below is the composite component code.

@Override
    public void processUpdates(FacesContext context) {
        pushComponentToEL(context, this);
        super.processUpdates(context);
        HashMap<String, ?> bindings = getBindings();
        if (bindings != null) {
            Map<String, ValueExpression> outParameters = getOutParameters(bindings, context);
            if (!outParameters.isEmpty())
                exportOutParameterValues(outParameters, context);
        }
        popComponentFromEL(context);
    }

    private void exportOutParameterValues(Map<String, ValueExpression> outParameters, FacesContext context) {
        String backingBeanName = getBackingBeanName();
        Object value;
        ValueExpression out;
        for (Map.Entry<String, ValueExpression> entry : outParameters.entrySet()) {
            value = getOutParameterValue(backingBeanName, entry.getKey(), context);
            out = entry.getValue();
            out.setValue(context.getELContext(), value);                //Here is the call to setValue() that breaks.
        }
    }

    private Map<String, ValueExpression> getOutParameters(HashMap<String, ?> bindings, FacesContext context) {
        Map<String, ValueExpression> outParameters = new HashMap<>();
        ELContext elContext = context.getELContext();
        for (Map.Entry<String, ?> entry : bindings.entrySet()) {
            if (!(entry.getValue() instanceof TagValueExpression)) {
                continue;
            }

            TagValueExpression ve = (TagValueExpression) entry.getValue();
            try {
                if (ve.isReadOnly(elContext)) {
                    continue;
                }
            } catch (javax.el.PropertyNotFoundException e) {
                logger.debug("The property {} is not a bean value, is a method.", entry.getKey());
                continue;

            }

            outParameters.put(entry.getKey(), ve);
        }
        return outParameters;
    }

    private Object getOutParameterValue(String backingBeanName, String property, FacesContext context) {
        String exprString = "#{" + backingBeanName + "." + property + "}";
        ValueExpression val = ELUtils.createValueExpression(exprString);
        return val.getValue(context.getELContext());
    }

    public HashMap<String, ?> getBindings() {
        Class[] classes = javax.faces.component.UIComponent.class.getDeclaredClasses();
        Class propertyKeys = null;
        for (Class c : classes) {
            if (c.getSimpleName().equals(PROPERTY_KEYS_ENUM_NAME)) {
                propertyKeys = c;
                break;
            }
        }
        if (propertyKeys == null) {
            throw new IllegalStateException();
        }
        Field bindings = null;
        for (Field f : propertyKeys.getDeclaredFields()) {
            if (f.getName().equals(BINDINGS_KEY)) {
                bindings = f;
                break;
            }
        }
        if (bindings == null) {
            throw new IllegalStateException();
        }
        bindings.setAccessible(true);
        Object key;
        try {
            key = bindings.get(this);
        } catch (IllegalAccessException e) {
            throw new RuntimeException(e);
        }
        return (HashMap<String, ?>) getStateHelper().get((Serializable) key);
    }

About your question on debugging, yes the 'value' parameter refers to the backing bean instance which is identical to the string 'beanName'. I didn't completely follow what you meant by "explicitly skip that case through a beanName.equals(value) check". Do you refer to modifying setValue() method implementation?

[spring-projects-issues]

Juergen Hoeller commented

Indeed, I was referring to relaxing setValue's assertion a bit. The general contract for an ELResolver requires us to find out whether the current resolver is applicable to the given (base, property) pair and to react accordingly. If we relax our assertion, i.e. don't throw an exception and let the call be propagated to the next ELResolver, we can't do much harm except for making debugging harder if the call fails later on. For that reason, we could selectively relax it for specific incoming parameters, along the lines of:

if (bf.containsBean(beanName) && !beanName.equals(value)) {
    throw new PropertyNotWritableException(...);
}

or, preventing propagation to further ELResolvers:

if (bf.containsBean(beanName)) {
    if (beanName.equals(value) {
        elContext.setPropertyResolved(true);
    }
    else {
        throw new PropertyNotWritableException(...);
    }
}

So it looks like you're checking isReadOnly() there first. However, if this returns true, that's exactly where setValue is supposed to throw a PropertyNotWritableException subsequently. Which makes me wonder: What exactly is that setValue call with property=beanName and value=beanName supposed to achieve? Technically, it suggests to set the beanName variable to itself? Is this simply a side effect of a general iteration algorithm and can effectively be ignored like in the second code sketch above?

Juergen

[spring-projects-issues]

Amit commented

• In your code snippet, you compare beanName with the value. Here value is the instance of the backing bean. I didn't quite get how would the equals check work for the object instance and the beanName string.

• Yes its a generic code. In the exportOutParameterValues() from above did you mean I should be checking

     private void exportOutParameterValues(Map<String, ValueExpression> outParameters, FacesContext context) {
    String backingBeanName = getBackingBeanName();
    Object value;
    ValueExpression out;
    for (Map.Entry<String, ValueExpression> entry : outParameters.entrySet()) {
        value = getOutParameterValue(backingBeanName, entry.getKey(), context);
        out = entry.getValue();
        if(!(out.getValue(context.getELContext()).equals(value))) {              //A new check to check if both instances are same. Would it be a right check when there are multiple such composite components on the same page?
            out.setValue(context.getELContext(), value);
        }
    }
}

• Also I didn't understood it clearly why does the setValue method throw a exception when the beanFactory contains the requested bean? Why does that get interpreted as a property not being writable?

[spring-projects-issues]

Juergen Hoeller commented

OK, so the 'value' is the bean instance for the given bean name... We'd have to call getBean and check for equality against the instance then.

The reason why setValue throws an exception is that we can't change the bean instance that a Spring bean name points to. So if a different value comes in for a Spring bean name here, we have to raise an exception. However, if the same instance is being specified for the bean name, we can effectively ignore the setValue call, since no actual state change is requested...

Juergen

[spring-projects-issues]

Amit commented

Ok, so would it be possible for you to add the above check in Spring ELResolver and provide a snapshot build that could be tested?

[spring-projects-issues]

Juergen Hoeller commented

Alright, I've committed this to master, comparing the provided value to the current bean instance and not throwing a PropertyNotWritableException if they are identical. This will be available in the next 4.0.3 snapshot (see http://projects.spring.io/spring-framework/ for Maven coordinates).

Juergen