You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@commons.apache.org by Simon Kitching <si...@ecnetwork.co.nz> on 2004/09/27 08:51:38 UTC
[Digester] Re: cvs commit:
jakarta-commons/digester/src/java/org/apache/commons/digester
SetPropertiesRule.java
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.
As no-one objected, I have done so (see below).
Regards,
Simon
On Mon, 2004-09-27 at 18:20, skitching@apache.org wrote:
> skitching 2004/09/26 23:20:38
>
> Modified: digester/src/java/org/apache/commons/digester
> SetPropertiesRule.java
> Log:
> Added some comments about the use of PropertyUtils to check for
> xml attributes with no corresponding bean setter methods.
>
> Revision Changes Path
> 1.21 +20 -1 jakarta-commons/digester/src/java/org/apache/commons/digester/SetPropertiesRule.java
>
> Index: SetPropertiesRule.java
> ===================================================================
> RCS file: /home/cvs/jakarta-commons/digester/src/java/org/apache/commons/digester/SetPropertiesRule.java,v
> retrieving revision 1.20
> retrieving revision 1.21
> diff -u -r1.20 -r1.21
> --- SetPropertiesRule.java 20 Sep 2004 21:59:23 -0000 1.20
> +++ SetPropertiesRule.java 27 Sep 2004 06:20:38 -0000 1.21
> @@ -206,6 +206,25 @@
> }
>
> if ((!ignoreMissingProperty) && (name != null)) {
> + // The BeanUtils.populate method silently ignores items in
> + // the map (ie xml entities) which have no corresponding
> + // setter method, so here we check whether each xml attribute
> + // does have a corresponding property before calling the
> + // BeanUtils.populate method.
> + //
> + // Yes having the test and set as separate steps is ugly and
> + // inefficient. But BeanUtils.populate doesn't provide the
> + // functionality we need here, and changing the algorithm which
> + // determines the appropriate setter method to invoke is
> + // considered too risky.
> + //
> + // Using two different classes (PropertyUtils vs BeanUtils) to
> + // do the test and the set is also ugly; the codepaths
> + // are different which could potentially lead to trouble.
> + // However the BeanUtils/ProperyUtils code has been carefully
> + // compared and the PropertyUtils functionality does appear
> + // compatible so we'll accept the risk here.
> +
> Object top = digester.peek();
> boolean test = PropertyUtils.isWriteable(top, name);
> if (!test)
>
>
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
> For additional commands, e-mail: commons-dev-help@jakarta.apache.org
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: commons-dev-help@jakarta.apache.org
Re: [Digester] Re: cvs commit: jakarta-commons/digester/src/java/org/apache/commons/digester SetPropertiesRule.java
Posted by robert burrell donkin <ro...@blueyonder.co.uk>.
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
Re: [Digester] Re: cvs commit:
jakarta-commons/digester/src/java/org/apache/commons/digester
SetPropertiesRule.java
Posted by Simon Kitching <si...@ecnetwork.co.nz>.
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).
Any other opinions?
Regards,
Simon
---------------------------------------------------------------------
To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: commons-dev-help@jakarta.apache.org