OFBiz › OFBiz - Dev

Re: svn commit: r917376 - /ofbiz/trunk/framework/base/src/org/ofbiz/base/util/ObjectType.java

_‹ Previous Topic Next Topic _›

Classic

List

Threaded

8 messages Options

Adrian Crum

Re: svn commit: r917376 - /ofbiz/trunk/framework/base/src/org/ofbiz/base/util/ObjectType.java

[hidden email] 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.

Adam Heath-2

Re: svn commit: r917376 - /ofbiz/trunk/framework/base/src/org/ofbiz/base/util/ObjectType.java

Adrian Crum wrote:

> [hidden email] 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.

Adrian Crum

Re: svn commit: r917376 - /ofbiz/trunk/framework/base/src/org/ofbiz/base/util/ObjectType.java

Adam Heath wrote:

> Adrian Crum wrote:
>> [hidden email] 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".

Adam Heath-2

Re: svn commit: r917376 - /ofbiz/trunk/framework/base/src/org/ofbiz/base/util/ObjectType.java

Adrian Crum wrote:

> Adam Heath wrote:
>> Adrian Crum wrote:
>>> [hidden email] 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.

Adam Heath-2

Re: svn commit: r917376 - /ofbiz/trunk/framework/base/src/org/ofbiz/base/util/ObjectType.java

In reply to this post by Adrian Crum

Adrian Crum wrote:

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.

Adrian Crum

Re: svn commit: r917376 - /ofbiz/trunk/framework/base/src/org/ofbiz/base/util/ObjectType.java

In reply to this post by Adam Heath-2

Adam Heath wrote:

> Adrian Crum wrote:
>> Adam Heath wrote:
>>> Adrian Crum wrote:
>>>> [hidden email] 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.

Adam Heath-2

Re: svn commit: r917376 - /ofbiz/trunk/framework/base/src/org/ofbiz/base/util/ObjectType.java

Adrian Crum wrote:

> Adam Heath wrote:
>> Adrian Crum wrote:
>>> Adam Heath wrote:
>>>> Adrian Crum wrote:
>>>>> [hidden email] 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.

>

Adrian Crum

Java Object Type Conversion (was: svn commit: r917376 - /ofbiz/trunk/framework/base/src/org/ofbiz/base/util/ObjectType.java)

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