You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@commons.apache.org by Hans Gilde <hg...@earthlink.net> on 2004/09/22 05:58:34 UTC
RE: cvs commit:jakarta-commons/digester/src/java/org/apache/commons/digesterSetPropertiesRule.java
FTI, Jelly has a feature like just SetPropertiesRule in a tag called
useBean.
I converted this tag from BeanUtils.populate to a combination of isWritable
and copyProperty/copyProperties and all unit tests ran and several big Maven
script also worked. Although I haven't uploaded the patch for this change.
Maybe all of commons should release this conversion at once, so that people
are simply forced to adapt to it across the board. :)
Seriously, though, I really think that it'll work fine for you. I walked
through the populate code and all I could find was stuff that honestly
doesn't apply to XML processing. For instance, populate will handle a
attribute with a "." in the name: <tag foo.bar="baz" /> will set the bean
property .getFoo().setBar("baz") where "baz" is converted to the proper
argument type. This is obviously meant for HTTP variable processing, not XML
attributes.
Do you really want this feature? If not, I'd say you should try
copyProperties with isWritable.
Hans
-----Original Message-----
From: Simon Kitching [mailto:simon@ecnetwork.co.nz]
Sent: Tuesday, September 21, 2004 12:06 AM
To: Jakarta Commons Developers List
Subject: Re: cvs
commit:jakarta-commons/digester/src/java/org/apache/commons/digesterSetPrope
rtiesRule.java
On Tue, 2004-09-21 at 09:59, rdonkin@apache.org wrote:
> rdonkin 2004/09/20 14:59:23
>
> Modified: digester build.xml
> digester/src/java/org/apache/commons/digester
> SetPropertiesRule.java
> Log:
> Allows exception throwing for mismatches to be switch on. Patch
contributed by Gabriele Carcassi.
I'm worried about this patch.
The existing code to actually do the assignment to properties calls
BeanUtils.populate(...) which calls BeanUtilsBean.populate(...) which
calls BeanUtilsBean.setProperty(...).
This patch however uses PropertyUtils.isWriteable to check for the
existence of the property.
The two code paths are completely different, and I am concerned that
they may consider different things to be valid properties.
I would prefer to see either:
(a) BeanUtils.populate modified to optionally throw an exception if a
matching property can't be found, or
(b) SetPropertiesRule be modified to use PropertyUtils methods to do the
property setting rather than BeanUtils.populate.
The latter is probably too risky a change to make; too much chance of
breaking existing digester users like Struts.
While I very much like this new feature, I am not convinced using
PropertyUtils methods to implement it is safe.
Regards,
Simon
---------------------------------------------------------------------
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: cvs commit:jakarta-commons/digester/src/java/org/apache/commons/digesterSetPropertiesRule.java
Posted by 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
Re: cvs
commit:jakarta-commons/digester/src/java/org/apache/commons/digesterSetPropertiesRule.java
Posted by 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
Posted by robert burrell donkin <ro...@blueyonder.co.uk>.
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.
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).
- robert
On 22 Sep 2004, at 21:37, Craig McClanahan wrote:
> In case it is not obvious enough from the blatant comments in the
> Javadocs, BeanUtils.populate() is ABSOLUTELY, POSITIVELY not
> appropriate for general XML processing. It is finely tuned to dealing
> with typical issues you see in processing request parameters in a web
> application (which is where this code was originally used in Struts).
>
> If you need property copying with conversion, use
> BeanUtils.copyProperties(). If you don't need the conversions,
> PropertyUtils.copyProperties() will run substantially faster.
>
> Craig McClanahan
>
>
> On Tue, 21 Sep 2004 23:58:34 -0400, Hans Gilde
> <hg...@earthlink.net> wrote:
>> FTI, Jelly has a feature like just SetPropertiesRule in a tag called
>> useBean.
>>
>> I converted this tag from BeanUtils.populate to a combination of
>> isWritable
>> and copyProperty/copyProperties and all unit tests ran and several
>> big Maven
>> script also worked. Although I haven't uploaded the patch for this
>> change.
>>
>> Maybe all of commons should release this conversion at once, so that
>> people
>> are simply forced to adapt to it across the board. :)
>>
>> Seriously, though, I really think that it'll work fine for you. I
>> walked
>> through the populate code and all I could find was stuff that honestly
>> doesn't apply to XML processing. For instance, populate will handle a
>> attribute with a "." in the name: <tag foo.bar="baz" /> will set the
>> bean
>> property .getFoo().setBar("baz") where "baz" is converted to the
>> proper
>> argument type. This is obviously meant for HTTP variable processing,
>> not XML
>> attributes.
>>
>> Do you really want this feature? If not, I'd say you should try
>> copyProperties with isWritable.
>>
>> Hans
>>
>> -----Original Message-----
>> From: Simon Kitching [mailto:simon@ecnetwork.co.nz]
>> Sent: Tuesday, September 21, 2004 12:06 AM
>> To: Jakarta Commons Developers List
>> Subject: Re: cvs
>> commit:jakarta-commons/digester/src/java/org/apache/commons/
>> digesterSetPrope
>> rtiesRule.java
>>
>> On Tue, 2004-09-21 at 09:59, rdonkin@apache.org wrote:
>>> rdonkin 2004/09/20 14:59:23
>>>
>>> Modified: digester build.xml
>>> digester/src/java/org/apache/commons/digester
>>> SetPropertiesRule.java
>>> Log:
>>> Allows exception throwing for mismatches to be switch on. Patch
>> contributed by Gabriele Carcassi.
>>
>> I'm worried about this patch.
>>
>> The existing code to actually do the assignment to properties calls
>> BeanUtils.populate(...) which calls BeanUtilsBean.populate(...) which
>> calls BeanUtilsBean.setProperty(...).
>>
>> This patch however uses PropertyUtils.isWriteable to check for the
>> existence of the property.
>>
>> The two code paths are completely different, and I am concerned that
>> they may consider different things to be valid properties.
>>
>> I would prefer to see either:
>> (a) BeanUtils.populate modified to optionally throw an exception if a
>> matching property can't be found, or
>> (b) SetPropertiesRule be modified to use PropertyUtils methods to do
>> the
>> property setting rather than BeanUtils.populate.
>>
>> The latter is probably too risky a change to make; too much chance of
>> breaking existing digester users like Struts.
>>
>> While I very much like this new feature, I am not convinced using
>> PropertyUtils methods to implement it is safe.
>>
>> Regards,
>>
>> Simon
>>
>> ---------------------------------------------------------------------
>> 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
>>
>>
>
> ---------------------------------------------------------------------
> 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: cvs commit:jakarta-commons/digester/src/java/org/apache/commons/digesterSetPropertiesRule.java
Posted by Craig McClanahan <cr...@gmail.com>.
In case it is not obvious enough from the blatant comments in the
Javadocs, BeanUtils.populate() is ABSOLUTELY, POSITIVELY not
appropriate for general XML processing. It is finely tuned to dealing
with typical issues you see in processing request parameters in a web
application (which is where this code was originally used in Struts).
If you need property copying with conversion, use
BeanUtils.copyProperties(). If you don't need the conversions,
PropertyUtils.copyProperties() will run substantially faster.
Craig McClanahan
On Tue, 21 Sep 2004 23:58:34 -0400, Hans Gilde
<hg...@earthlink.net> wrote:
> FTI, Jelly has a feature like just SetPropertiesRule in a tag called
> useBean.
>
> I converted this tag from BeanUtils.populate to a combination of isWritable
> and copyProperty/copyProperties and all unit tests ran and several big Maven
> script also worked. Although I haven't uploaded the patch for this change.
>
> Maybe all of commons should release this conversion at once, so that people
> are simply forced to adapt to it across the board. :)
>
> Seriously, though, I really think that it'll work fine for you. I walked
> through the populate code and all I could find was stuff that honestly
> doesn't apply to XML processing. For instance, populate will handle a
> attribute with a "." in the name: <tag foo.bar="baz" /> will set the bean
> property .getFoo().setBar("baz") where "baz" is converted to the proper
> argument type. This is obviously meant for HTTP variable processing, not XML
> attributes.
>
> Do you really want this feature? If not, I'd say you should try
> copyProperties with isWritable.
>
> Hans
>
> -----Original Message-----
> From: Simon Kitching [mailto:simon@ecnetwork.co.nz]
> Sent: Tuesday, September 21, 2004 12:06 AM
> To: Jakarta Commons Developers List
> Subject: Re: cvs
> commit:jakarta-commons/digester/src/java/org/apache/commons/digesterSetPrope
> rtiesRule.java
>
> On Tue, 2004-09-21 at 09:59, rdonkin@apache.org wrote:
> > rdonkin 2004/09/20 14:59:23
> >
> > Modified: digester build.xml
> > digester/src/java/org/apache/commons/digester
> > SetPropertiesRule.java
> > Log:
> > Allows exception throwing for mismatches to be switch on. Patch
> contributed by Gabriele Carcassi.
>
> I'm worried about this patch.
>
> The existing code to actually do the assignment to properties calls
> BeanUtils.populate(...) which calls BeanUtilsBean.populate(...) which
> calls BeanUtilsBean.setProperty(...).
>
> This patch however uses PropertyUtils.isWriteable to check for the
> existence of the property.
>
> The two code paths are completely different, and I am concerned that
> they may consider different things to be valid properties.
>
> I would prefer to see either:
> (a) BeanUtils.populate modified to optionally throw an exception if a
> matching property can't be found, or
> (b) SetPropertiesRule be modified to use PropertyUtils methods to do the
> property setting rather than BeanUtils.populate.
>
> The latter is probably too risky a change to make; too much chance of
> breaking existing digester users like Struts.
>
> While I very much like this new feature, I am not convinced using
> PropertyUtils methods to implement it is safe.
>
> Regards,
>
> Simon
>
> ---------------------------------------------------------------------
> 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
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: commons-dev-help@jakarta.apache.org