commons-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From robert burrell donkin <robertburrelldon...@blueyonder.co.uk>
Subject Re: [Digester] Re: cvs commit: jakarta-commons/digester/src/java/org/apache/commons/digester SetPropertiesRule.java
Date Mon, 27 Sep 2004 21:58:48 GMT
On 27 Sep 2004, at 09:35, Simon Kitching wrote:

> On Mon, 2004-09-27 at 18:51, Simon Kitching wrote:
>> A few days ago I proposed adding a comment to the SetPropertiesRule 
>> file
>> summarising the results of the email discussion we had on this list.
>
> Hmm... I've just (re)discovered three places in the existing code where
> we check whether a bean has a property or not; they all do it the same
> way (but different from the code just added to SetPropertiesRule).
>
> BeanPropertySetterRule, SetPropertyRule and SetNestedPropertiesRule all
> use identical code as follows:
>
> <code>
>   // Force an exception if the property does not exist
>   // (BeanUtils.setProperty() silently returns in this case)
>   if (top instanceof DynaBean) {
>       DynaProperty desc =
>           ((DynaBean) top).getDynaClass().getDynaProperty(property);
>       if (desc == null) {
>           throw new NoSuchMethodException
>               ("Bean has no property named " + property);
>       }
>   } else /* this is a standard JavaBean */ {
>       PropertyDescriptor desc =
>           PropertyUtils.getPropertyDescriptor(top, property);
>       if (desc == null) {
>           throw new NoSuchMethodException
>               ("Bean has no property named " + property);
>       }
>   }
>
>   // Set the property (with conversion as necessary)
>   BeanUtils.setProperty(top, property, bodyText);
> </code>
>
> I've compared the code above to the PropertyUtils.isWriteable() method,
> and found them very very similar.
>
> The only difference is that the above code only checks for the
> *existence* of a property, and will then let BeanUtils.setProperty fail
> silently if it is not writeable whereas PropertyUtils.isWriteable will
> return false (and thereby cause an error to be reported) if there *is*
> such a property but it is read-only.
>
> Personally, I think that PropertyUtils.isWriteable() is the right 
> method
> to be calling; we *do* really want to raise an alert if the property
> isn't writeable.
>
> However changing the code in BeanPropertySetterRule etc is probably not
> advisable for compatibility reasons.
>
> So my opinion is that we don't need to do anything; the new check in
> SetPropertiesRule *should* use PropertyUtils.isWriteable instead of
> copying the code from BeanPropertySettterRule/SetPropertyRule because
> it's "the right thing to do". And we should leave the (slightly broken)
> code in BeanPropertySetterRule etc. alone to avoid the possibility of
> breakage (well, I might add a warning comment).

+1

thanks for raising the original issue and investigating it. i'm not 
really sure that it's really solved but i'm glad that the code 
committed before isn't positively harmful. i'd like to think that we'll 
come back to this issue but maybe it's more productive to look 
forward...

- robert


---------------------------------------------------------------------
To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: commons-dev-help@jakarta.apache.org


Mime
View raw message