Re: cvs commit:jakarta-commons/digester/src/java/org/apache/commons/digesterSetPropertiesRule.java

[Simon Kitching <si...@ecnetwork.co.nz>]

On Thu, 2004-09-23 at 09:24, robert burrell donkin wrote:
> hi craig
> 
> unfortunately (or not) SetPropertiesRules has used beanutils.properties  
> since you imported it from struts. i'd be very reluctant to change it  
> for fear of breaking backwards compatibility.

Yep. I agree that BeanUtils is the wrong class to be using, but that's
what it has always used and also think changing to PropertyUtils now
would be a bad idea (despite Han's comments that he couldn't find any
relevant behavioural differences).

I have recently started creating some code I hope to put forward for
consideration as a base for "digester2". I will try updating
SetPropertiesRule to use PropertyUtils...

> 
> the information from hans is quite useful since jelly has recently  
> revised the original choice (to use beanutils.properties) and there  
> don't seem to be any ill effects. maybe it we could factor out the  
> actual property setting leaving the existing situation as default (for  
> backwards compatibility).

Hmm..

Are you saying that we could add a switch for "beanutils" or
"propertyutils" property setting, and that if someone enables the "check
property exists" feature, that we then force "propertyutils" to be used?

It's quite a tempting idea. We get the old code working unchanged, but
the new feature works consistently. 

The downside is that when you turn on "check property exists" you could
potentially change the matching behaviour too. However Hans Gilde's very
useful comment indicates that this is not likely to cause a problem
though.

I'm tentatively -0 to the idea of building in a generic user-settable
property-setter strategy. I just think this is a feature that will never
be used.

Given Han's comments, I'm less worried about the patch now. So I would
be satisfied with either of the following:
(a) add a flag to control whether beanutils or propertyutils is used
  to set properties, and force propertyutils to be used if the
  "check property exists" functionality is enabled, or
(b) add a warning comment to the existing (patched) code to note that
   we are aware that the data paths are different, and that this is
   not desirable but is unlikely to cause problems.

I am happy to add the comment from (b) myself if people think this is
the right way to go.

As you say, this only introduces a risk for people using this new
feature, so no existing code can be broken by this new patch.

Regards,

Simon



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

Re: cvs commit:jakarta-commons/digester/src/java/org/apache/commons/digesterSetPropertiesRule.java

[Craig McClanahan <cr...@gmail.com>]

On Thu, 23 Sep 2004 11:50:57 +1200, Simon Kitching
<si...@ecnetwork.co.nz> wrote:
> [snip]
> Are you saying that we could add a switch for "beanutils" or
> "propertyutils" property setting, and that if someone enables the "check
> property exists" feature, that we then force "propertyutils" to be used?
> 

Aren't there two orthogonal issues here?  Checking for missing
properties versus using BeanUtils.setProperties() versus
BeanUtils.populate()?  If we go the configuration route, we should
provide configuration properties for both, with the defaults obviously
set to enable the current behavior.

I would not suggest that we use PropertyUtils.setProperties() for this
rule, because it would give up the very useful conversions that are
performed automatically -- for example, you would never be able to
configure an int or double property from the incoming data, which is
necessarily a String.

Craig

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