camel-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Claus Ibsen <claus.ib...@gmail.com>
Subject Re: svn commit: r1339962 - /camel/trunk/camel-core/src/main/java/org/apache/camel/util/ObjectHelper.java
Date Tue, 22 May 2012 08:03:01 GMT
Hi

Thanks for adding the unit test. We should have these tests, that
helps against regressions.

XML nodes with and without attributes. I dont see an optional solution
that fits all use-cases.

I suggest to add a note in the Camel 2.10 release notes about this change.
And then keep the old behavior in the 2.9 and 2.8 patch releases. Then
we dont introduce any surprises for people in production etc.
And for ppl upgrading to 2.10 they would be more okay with us
introducing this change in a minor release. But not in a patch
release.



On Mon, May 21, 2012 at 4:05 PM, Willem Jiang <willem.jiang@gmail.com> wrote:
> Sorry, I didn't bake the idea with enough tests.
>
> I dig the first change of ObjectHelper about checking the empty DOM[1]
> But I don't think the change on the ObjectHelper is related to the patch as
> when I revert the patch on the ObjectHelper, the XPathContentBasedRouterTest
> is passed.
>
> I just committed a patch[2] of the unit test to show an interesting example
>
> <person name="claus"/>
> <person>claus</person>
>
> I think the below two element is represent same person but the first one
> will be treat as false without applying the patch.
>
> Any thought?
>
>
> [1]https://issues.apache.org/jira/browse/CAMEL-3531
> [2]http://svn.apache.org/viewvc?rev=1341029&view=rev
>
>
> On Sat May 19 17:22:25 2012, Claus Ibsen wrote:
>>
>> Hi
>>
>> 1)
>> There *should* be an unit test when we change behavior in a *core*
>> feature such as this.
>>
>> 2)
>> A lot of Camel users parses XML files and whatnot, and evaluates
>> predicates and expressions.
>> So this is a change in behavior. And this behavior has been running
>> for this for a long time.
>>
>> I do not like that this is backported to patch releases.
>> The patch releases *must* be stable and rely on current behavior.
>>
>> 3)
>> And I think this should be disussed a bit more before committing a
>> fix. As its a *core* change.
>> Why is an attribute on a root tag suddenly matter if its regarded as
>> true or false?
>>
>> If you have tags as follows.
>>
>> a)
>> <users>
>> </users>
>>
>> b)
>> <users type="admin">
>> </users>
>>
>> In my mind both of these is empty, and therefore false.
>>
>> I think the old behavior is the correct. This is how Camel have run
>> all the time.
>>
>> Personally I think we should consider reverting and keeping the old
>> behavior.
>> Unless there is a good use-case saying otherwise.
>> I have not yet seen this.
>>
>>
>>
>>
>> On Fri, May 18, 2012 at 5:13 AM,<ningjiang@apache.org>  wrote:
>>>
>>> Author: ningjiang
>>> Date: Fri May 18 03:13:35 2012
>>> New Revision: 1339962
>>>
>>> URL: http://svn.apache.org/viewvc?rev=1339962&view=rev
>>> Log:
>>> CAMEL-5276 Fix the issue that the ObjectHelper will return false for a
>>> node without children
>>>
>>> Modified:
>>>
>>>  camel/trunk/camel-core/src/main/java/org/apache/camel/util/ObjectHelper.java
>>>
>>> Modified:
>>> camel/trunk/camel-core/src/main/java/org/apache/camel/util/ObjectHelper.java
>>> URL:
>>> http://svn.apache.org/viewvc/camel/trunk/camel-core/src/main/java/org/apache/camel/util/ObjectHelper.java?rev=1339962&r1=1339961&r2=1339962&view=diff
>>>
>>> ==============================================================================
>>> ---
>>> camel/trunk/camel-core/src/main/java/org/apache/camel/util/ObjectHelper.java
>>> (original)
>>> +++
>>> camel/trunk/camel-core/src/main/java/org/apache/camel/util/ObjectHelper.java
>>> Fri May 18 03:13:35 2012
>>> @@ -1227,7 +1227,10 @@ public final class ObjectHelper {
>>>                 return false;
>>>             }
>>>         } else if (value instanceof NodeList) {
>>> -            // is it an empty dom
>>> +            // is it an empty dom with empty attributes
>>> +            if (value instanceof Node&&  ((Node)value).hasAttributes())
>>> {
>>>
>>> +                return true;
>>> +            }
>>>             NodeList list = (NodeList) value;
>>>             return list.getLength()>  0;
>>>         } else if (value instanceof Collection) {
>>>
>>>
>>
>>
>>
>
>
> --
> Willem
> ----------------------------------
>
> CamelOne 2012 Conference, May 15-16, 2012: http://camelone.com
> FuseSource
> Web: http://www.fusesource.com
> Blog:    http://willemjiang.blogspot.com (English)
>         http://jnn.javaeye.com (Chinese)
> Twitter: willemjiang
> Weibo: willemjiang



-- 
Claus Ibsen
-----------------
CamelOne 2012 Conference, May 15-16, 2012: http://camelone.com
FuseSource
Email: cibsen@fusesource.com
Web: http://fusesource.com
Twitter: davsclaus, fusenews
Blog: http://davsclaus.blogspot.com/
Author of Camel in Action: http://www.manning.com/ibsen/

Mime
View raw message