commons-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Simon Kitching <si...@ecnetwork.co.nz>
Subject Re: [Digester] Re: cvs commit: jakarta-commons/digester/src/java/org/apache/commons/digester SetPropertiesRule.java
Date Mon, 27 Sep 2004 08:35:30 GMT
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


Mime
View raw message