You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@ofbiz.apache.org by Adrian Crum <ad...@hlmksw.com> on 2010/03/01 17:00:53 UTC
Re: svn commit: r917376 - /ofbiz/trunk/framework/base/src/org/ofbiz/base/util/ObjectType.java
doogie@apache.org wrote:
> Author: doogie
> Date: Mon Mar 1 05:06:16 2010
> New Revision: 917376
>
> URL: http://svn.apache.org/viewvc?rev=917376&view=rev
> Log:
> BUG FIX: Move the check that tests whether we are converting to the
> passed object to before the loadClass call.
>
> Modified:
> ofbiz/trunk/framework/base/src/org/ofbiz/base/util/ObjectType.java
>
> Modified: ofbiz/trunk/framework/base/src/org/ofbiz/base/util/ObjectType.java
> URL: http://svn.apache.org/viewvc/ofbiz/trunk/framework/base/src/org/ofbiz/base/util/ObjectType.java?rev=917376&r1=917375&r2=917376&view=diff
> ==============================================================================
> --- ofbiz/trunk/framework/base/src/org/ofbiz/base/util/ObjectType.java (original)
> +++ ofbiz/trunk/framework/base/src/org/ofbiz/base/util/ObjectType.java Mon Mar 1 05:06:16 2010
> @@ -484,6 +484,9 @@
> return obj.toString();
> }
> Class<?> sourceClass = obj.getClass();
> + if (sourceClass.getName().equals(type)) {
> + return obj;
> + }
> if ("Object".equals(type) || "java.lang.Object".equals(type)) {
> return obj;
> }
Are you sure this will work? Take a look at the following if statement.
Java Object Type Conversion (was: svn commit: r917376 - /ofbiz/trunk/framework/base/src/org/ofbiz/base/util/ObjectType.java)
Posted by Adrian Crum <ad...@hlmksw.com>.
Adam Heath wrote:
> Now, if we want to discuss removing these extra changes, then we also
> need to take it upon ourselves to fix any uses that required the code
> to function in this manner.
I guess the community needs to discuss (and agree upon) a set of rules
that Java object type conversion should follow.
That would be a discussion worth having - just like the TimeZone issue
David brought up earlier.
Sometimes code just gets thrown into the project without a lot of
planning, so it can be helpful to have the community agree on a design.
Then we can change the code to follow that design.
I admit I made a couple of code behavior changes while introducing the
conversion framework. For the most part I kept things the same - to
ensure backward compatibility. But I also changed some things because
the original conversions didn't seem to follow any rules - they didn't
make sense . In hindsight, I should have started a thread on the subject
before changing it.
-Adrian
Re: svn commit: r917376 - /ofbiz/trunk/framework/base/src/org/ofbiz/base/util/ObjectType.java
Posted by Adam Heath <do...@brainfood.com>.
Adrian Crum wrote:
> Adam Heath wrote:
>> Adrian Crum wrote:
>>> Adam Heath wrote:
>>>> Adrian Crum wrote:
>>>>> doogie@apache.org wrote:
>>>>>> Author: doogie
>>>>>> Date: Mon Mar 1 05:06:16 2010
>>>>>> New Revision: 917376
>>>>>>
>>>>>> URL: http://svn.apache.org/viewvc?rev=917376&view=rev
>>>>>> Log:
>>>>>> BUG FIX: Move the check that tests whether we are converting to the
>>>>>> passed object to before the loadClass call.
>>>>>>
>>>>>> Modified:
>>>>>>
>>>>>> ofbiz/trunk/framework/base/src/org/ofbiz/base/util/ObjectType.java
>>>>>>
>>>>>> Modified:
>>>>>> ofbiz/trunk/framework/base/src/org/ofbiz/base/util/ObjectType.java
>>>>>> URL:
>>>>>> http://svn.apache.org/viewvc/ofbiz/trunk/framework/base/src/org/ofbiz/base/util/ObjectType.java?rev=917376&r1=917375&r2=917376&view=diff
>>>>>>
>>>>>>
>>>>>>
>>>>>> ==============================================================================
>>>>>>
>>>>>>
>>>>>>
>>>>>> ---
>>>>>> ofbiz/trunk/framework/base/src/org/ofbiz/base/util/ObjectType.java
>>>>>> (original)
>>>>>> +++
>>>>>> ofbiz/trunk/framework/base/src/org/ofbiz/base/util/ObjectType.java
>>>>>> Mon Mar 1 05:06:16 2010
>>>>>> @@ -484,6 +484,9 @@
>>>>>> return obj.toString();
>>>>>> }
>>>>>> Class<?> sourceClass = obj.getClass();
>>>>>> + if (sourceClass.getName().equals(type)) {
>>>>>> + return obj;
>>>>>> + }
>>>>>> if ("Object".equals(type) ||
>>>>>> "java.lang.Object".equals(type)) {
>>>>>> return obj;
>>>>>> }
>>>>> Are you sure this will work? Take a look at the following if
>>>>> statement.
>>>> Yes, due a combination of things.
>>>>
>>>> If the object was null, return null.
>>>>
>>>> If converting to the same exact class as the incoming obj, return with
>>>> no change.
>>>>
>>>> If the incoming object is a string, and it is empty, return null.
>>>>
>>>> The above 3 constraints were pulled from the old version of
>>>> simpleTypeConvert.
>>>>
>>>> Without all three, if you tried to convert an empty String to a
>>>> String, you'd get null.
>>> I think you missed my point. Shouldn't the test be something like:
>>>
>>> if (sourceClass.getName().equals(type) ||
>>> sourceClass.getSimpleName().equals(type)) {
>>> return obj;
>>> }
>>>
>>> ?
>>>
>>> In other words, you can't count on type being "java.util.Date" - it
>>> might be simply "Date".
>>
>> Except always testing against simpleName isn't right, as only certain
>> classes had simpleName tests from the previous version.
>>
>> I see your point, however, and will fix it how you want, but only
>> insomuch that it matches the old version.
>
> I didn't care for the simple name test in the original code. Maybe
> that's why I changed it to be more reliable.
>
> Like you said in your summary, there were things in the original code
> that didn't work or didn't make sense.
Ok, no, I will not be implementing the change you suggest. The
original code did not check the short name there. And yes, that is
weird, but this series of fixes was to only make the new version
function exactly like the old, so that existing code wouldn't break.
Now, if we want to discuss removing these extra changes, then we also
need to take it upon ourselves to fix any uses that required the code
to function in this manner.
>
Re: svn commit: r917376 - /ofbiz/trunk/framework/base/src/org/ofbiz/base/util/ObjectType.java
Posted by Adrian Crum <ad...@hlmksw.com>.
Adam Heath wrote:
> Adrian Crum wrote:
>> Adam Heath wrote:
>>> Adrian Crum wrote:
>>>> doogie@apache.org wrote:
>>>>> Author: doogie
>>>>> Date: Mon Mar 1 05:06:16 2010
>>>>> New Revision: 917376
>>>>>
>>>>> URL: http://svn.apache.org/viewvc?rev=917376&view=rev
>>>>> Log:
>>>>> BUG FIX: Move the check that tests whether we are converting to the
>>>>> passed object to before the loadClass call.
>>>>>
>>>>> Modified:
>>>>> ofbiz/trunk/framework/base/src/org/ofbiz/base/util/ObjectType.java
>>>>>
>>>>> Modified:
>>>>> ofbiz/trunk/framework/base/src/org/ofbiz/base/util/ObjectType.java
>>>>> URL:
>>>>> http://svn.apache.org/viewvc/ofbiz/trunk/framework/base/src/org/ofbiz/base/util/ObjectType.java?rev=917376&r1=917375&r2=917376&view=diff
>>>>>
>>>>>
>>>>> ==============================================================================
>>>>>
>>>>>
>>>>> --- ofbiz/trunk/framework/base/src/org/ofbiz/base/util/ObjectType.java
>>>>> (original)
>>>>> +++ ofbiz/trunk/framework/base/src/org/ofbiz/base/util/ObjectType.java
>>>>> Mon Mar 1 05:06:16 2010
>>>>> @@ -484,6 +484,9 @@
>>>>> return obj.toString();
>>>>> }
>>>>> Class<?> sourceClass = obj.getClass();
>>>>> + if (sourceClass.getName().equals(type)) {
>>>>> + return obj;
>>>>> + }
>>>>> if ("Object".equals(type) ||
>>>>> "java.lang.Object".equals(type)) {
>>>>> return obj;
>>>>> }
>>>> Are you sure this will work? Take a look at the following if statement.
>>> Yes, due a combination of things.
>>>
>>> If the object was null, return null.
>>>
>>> If converting to the same exact class as the incoming obj, return with
>>> no change.
>>>
>>> If the incoming object is a string, and it is empty, return null.
>>>
>>> The above 3 constraints were pulled from the old version of
>>> simpleTypeConvert.
>>>
>>> Without all three, if you tried to convert an empty String to a
>>> String, you'd get null.
>> I think you missed my point. Shouldn't the test be something like:
>>
>> if (sourceClass.getName().equals(type) ||
>> sourceClass.getSimpleName().equals(type)) {
>> return obj;
>> }
>>
>> ?
>>
>> In other words, you can't count on type being "java.util.Date" - it
>> might be simply "Date".
>
> Except always testing against simpleName isn't right, as only certain
> classes had simpleName tests from the previous version.
>
> I see your point, however, and will fix it how you want, but only
> insomuch that it matches the old version.
I didn't care for the simple name test in the original code. Maybe
that's why I changed it to be more reliable.
Like you said in your summary, there were things in the original code
that didn't work or didn't make sense.
Re: svn commit: r917376 - /ofbiz/trunk/framework/base/src/org/ofbiz/base/util/ObjectType.java
Posted by Adam Heath <do...@brainfood.com>.
Adrian Crum wrote:
> Adam Heath wrote:
>> Adrian Crum wrote:
>>> doogie@apache.org wrote:
>>>> Author: doogie
>>>> Date: Mon Mar 1 05:06:16 2010
>>>> New Revision: 917376
>>>>
>>>> URL: http://svn.apache.org/viewvc?rev=917376&view=rev
>>>> Log:
>>>> BUG FIX: Move the check that tests whether we are converting to the
>>>> passed object to before the loadClass call.
>>>>
>>>> Modified:
>>>> ofbiz/trunk/framework/base/src/org/ofbiz/base/util/ObjectType.java
>>>>
>>>> Modified:
>>>> ofbiz/trunk/framework/base/src/org/ofbiz/base/util/ObjectType.java
>>>> URL:
>>>> http://svn.apache.org/viewvc/ofbiz/trunk/framework/base/src/org/ofbiz/base/util/ObjectType.java?rev=917376&r1=917375&r2=917376&view=diff
>>>>
>>>>
>>>> ==============================================================================
>>>>
>>>>
>>>> --- ofbiz/trunk/framework/base/src/org/ofbiz/base/util/ObjectType.java
>>>> (original)
>>>> +++ ofbiz/trunk/framework/base/src/org/ofbiz/base/util/ObjectType.java
>>>> Mon Mar 1 05:06:16 2010
>>>> @@ -484,6 +484,9 @@
>>>> return obj.toString();
>>>> }
>>>> Class<?> sourceClass = obj.getClass();
>>>> + if (sourceClass.getName().equals(type)) {
>>>> + return obj;
>>>> + }
>>>> if ("Object".equals(type) ||
>>>> "java.lang.Object".equals(type)) {
>>>> return obj;
>>>> }
>>> Are you sure this will work? Take a look at the following if statement.
>>
>> Yes, due a combination of things.
>>
>> If the object was null, return null.
>>
>> If converting to the same exact class as the incoming obj, return with
>> no change.
>>
>> If the incoming object is a string, and it is empty, return null.
>>
>> The above 3 constraints were pulled from the old version of
>> simpleTypeConvert.
>>
>> Without all three, if you tried to convert an empty String to a
>> String, you'd get null.
>
> I think you missed my point. Shouldn't the test be something like:
>
> if (sourceClass.getName().equals(type) ||
> sourceClass.getSimpleName().equals(type)) {
> return obj;
> }
>
> ?
>
> In other words, you can't count on type being "java.util.Date" - it
> might be simply "Date".
Except always testing against simpleName isn't right, as only certain
classes had simpleName tests from the previous version.
I see your point, however, and will fix it how you want, but only
insomuch that it matches the old version.
Re: svn commit: r917376 - /ofbiz/trunk/framework/base/src/org/ofbiz/base/util/ObjectType.java
Posted by Adam Heath <do...@brainfood.com>.
Adrian Crum wrote:
> Adam Heath wrote:
>> Adrian Crum wrote:
>>> doogie@apache.org wrote:
>>>> Author: doogie
>>>> Date: Mon Mar 1 05:06:16 2010
>>>> New Revision: 917376
>>>>
>>>> URL: http://svn.apache.org/viewvc?rev=917376&view=rev
>>>> Log:
>>>> BUG FIX: Move the check that tests whether we are converting to the
>>>> passed object to before the loadClass call.
>>>>
>>>> Modified:
>>>> ofbiz/trunk/framework/base/src/org/ofbiz/base/util/ObjectType.java
>>>>
>>>> Modified:
>>>> ofbiz/trunk/framework/base/src/org/ofbiz/base/util/ObjectType.java
>>>> URL:
>>>> http://svn.apache.org/viewvc/ofbiz/trunk/framework/base/src/org/ofbiz/base/util/ObjectType.java?rev=917376&r1=917375&r2=917376&view=diff
>>>>
>>>>
>>>> ==============================================================================
>>>>
>>>>
>>>> --- ofbiz/trunk/framework/base/src/org/ofbiz/base/util/ObjectType.java
>>>> (original)
>>>> +++ ofbiz/trunk/framework/base/src/org/ofbiz/base/util/ObjectType.java
>>>> Mon Mar 1 05:06:16 2010
>>>> @@ -484,6 +484,9 @@
>>>> return obj.toString();
>>>> }
>>>> Class<?> sourceClass = obj.getClass();
>>>> + if (sourceClass.getName().equals(type)) {
>>>> + return obj;
>>>> + }
>>>> if ("Object".equals(type) ||
>>>> "java.lang.Object".equals(type)) {
>>>> return obj;
>>>> }
>>> Are you sure this will work? Take a look at the following if statement.
>>
>> Yes, due a combination of things.
>>
>> If the object was null, return null.
>>
>> If converting to the same exact class as the incoming obj, return with
>> no change.
>>
>> If the incoming object is a string, and it is empty, return null.
>>
>> The above 3 constraints were pulled from the old version of
>> simpleTypeConvert.
>>
>> Without all three, if you tried to convert an empty String to a
>> String, you'd get null.
>
> I think you missed my point. Shouldn't the test be something like:
>
> if (sourceClass.getName().equals(type) ||
> sourceClass.getSimpleName().equals(type)) {
> return obj;
> }
>
> ?
>
> In other words, you can't count on type being "java.util.Date" - it
> might be simply "Date".
Another issue, is you are converting from a non-null String, that is
empty, then even if the class is not found, it still needs to return
null. That makes it difficult to place this particular test after the
loadClass call.
Re: svn commit: r917376 - /ofbiz/trunk/framework/base/src/org/ofbiz/base/util/ObjectType.java
Posted by Adrian Crum <ad...@hlmksw.com>.
Adam Heath wrote:
> Adrian Crum wrote:
>> doogie@apache.org wrote:
>>> Author: doogie
>>> Date: Mon Mar 1 05:06:16 2010
>>> New Revision: 917376
>>>
>>> URL: http://svn.apache.org/viewvc?rev=917376&view=rev
>>> Log:
>>> BUG FIX: Move the check that tests whether we are converting to the
>>> passed object to before the loadClass call.
>>>
>>> Modified:
>>> ofbiz/trunk/framework/base/src/org/ofbiz/base/util/ObjectType.java
>>>
>>> Modified:
>>> ofbiz/trunk/framework/base/src/org/ofbiz/base/util/ObjectType.java
>>> URL:
>>> http://svn.apache.org/viewvc/ofbiz/trunk/framework/base/src/org/ofbiz/base/util/ObjectType.java?rev=917376&r1=917375&r2=917376&view=diff
>>>
>>> ==============================================================================
>>>
>>> --- ofbiz/trunk/framework/base/src/org/ofbiz/base/util/ObjectType.java
>>> (original)
>>> +++ ofbiz/trunk/framework/base/src/org/ofbiz/base/util/ObjectType.java
>>> Mon Mar 1 05:06:16 2010
>>> @@ -484,6 +484,9 @@
>>> return obj.toString();
>>> }
>>> Class<?> sourceClass = obj.getClass();
>>> + if (sourceClass.getName().equals(type)) {
>>> + return obj;
>>> + }
>>> if ("Object".equals(type) || "java.lang.Object".equals(type)) {
>>> return obj;
>>> }
>> Are you sure this will work? Take a look at the following if statement.
>
> Yes, due a combination of things.
>
> If the object was null, return null.
>
> If converting to the same exact class as the incoming obj, return with
> no change.
>
> If the incoming object is a string, and it is empty, return null.
>
> The above 3 constraints were pulled from the old version of
> simpleTypeConvert.
>
> Without all three, if you tried to convert an empty String to a
> String, you'd get null.
I think you missed my point. Shouldn't the test be something like:
if (sourceClass.getName().equals(type) ||
sourceClass.getSimpleName().equals(type)) {
return obj;
}
?
In other words, you can't count on type being "java.util.Date" - it
might be simply "Date".
Re: svn commit: r917376 - /ofbiz/trunk/framework/base/src/org/ofbiz/base/util/ObjectType.java
Posted by Adam Heath <do...@brainfood.com>.
Adrian Crum wrote:
> doogie@apache.org wrote:
>> Author: doogie
>> Date: Mon Mar 1 05:06:16 2010
>> New Revision: 917376
>>
>> URL: http://svn.apache.org/viewvc?rev=917376&view=rev
>> Log:
>> BUG FIX: Move the check that tests whether we are converting to the
>> passed object to before the loadClass call.
>>
>> Modified:
>> ofbiz/trunk/framework/base/src/org/ofbiz/base/util/ObjectType.java
>>
>> Modified:
>> ofbiz/trunk/framework/base/src/org/ofbiz/base/util/ObjectType.java
>> URL:
>> http://svn.apache.org/viewvc/ofbiz/trunk/framework/base/src/org/ofbiz/base/util/ObjectType.java?rev=917376&r1=917375&r2=917376&view=diff
>>
>> ==============================================================================
>>
>> --- ofbiz/trunk/framework/base/src/org/ofbiz/base/util/ObjectType.java
>> (original)
>> +++ ofbiz/trunk/framework/base/src/org/ofbiz/base/util/ObjectType.java
>> Mon Mar 1 05:06:16 2010
>> @@ -484,6 +484,9 @@
>> return obj.toString();
>> }
>> Class<?> sourceClass = obj.getClass();
>> + if (sourceClass.getName().equals(type)) {
>> + return obj;
>> + }
>> if ("Object".equals(type) || "java.lang.Object".equals(type)) {
>> return obj;
>> }
>
> Are you sure this will work? Take a look at the following if statement.
Yes, due a combination of things.
If the object was null, return null.
If converting to the same exact class as the incoming obj, return with
no change.
If the incoming object is a string, and it is empty, return null.
The above 3 constraints were pulled from the old version of
simpleTypeConvert.
Without all three, if you tried to convert an empty String to a
String, you'd get null.