Re: svn commit: r991485 - /ofbiz/trunk/framework/base/src/org/ofbiz/base/util/UtilIO.java

[Adam Heath-2]

On 09/01/2010 04:15 AM, [hidden email] wrote:

> Author: jleroux
> Date: Wed Sep  1 09:15:14 2010
> New Revision: 991485
>
> URL: http://svn.apache.org/viewvc?rev=991485&view=rev
> Log:
> A patch from Liu Xiangqian "method writeObject(StringBuilder sb, Object value, boolean allowJsonResolve) in UtilIO always throw an IOException" (https://issues.apache.org/jira/browse/OFBIZ-3916) - OFBIZ-3916
>
> Tried to invoke method UtilIO.writeObject() but found the following code
>
> try {
> StringWriter writer = new StringWriter();
> if (encodeObject(writer, value, allowJsonResolve)) { sb.append(writer.toString()); return; }
> } catch (Exception e) {
> }
> throw new IOException("Can't write (" + value + ")");
>
> It caused the method to always throw an IOException.
>
> Modified:
>      ofbiz/trunk/framework/base/src/org/ofbiz/base/util/UtilIO.java

This patch needs to be reverted.  writeObject() calls encodeObject(),
which *always* returns true, unless an error occurrs.  This means the
true block of the if is run, which then returns, so the IOException
never gets thrown.

Randomly applying patches from jira, that only contain a description,
and no actual example, to code that you didn't originally write
yourself, should not be done.  People need to understand what they are
changing, and that wasn't done here.

Sometimes, the code is formatted a certain way to provide full
coverage; I'm not saying that is the case here, but just looking at
the method and code flow will show that IOException is not always
thrown as the reporter says.

>
> Modified: ofbiz/trunk/framework/base/src/org/ofbiz/base/util/UtilIO.java
> URL: http://svn.apache.org/viewvc/ofbiz/trunk/framework/base/src/org/ofbiz/base/util/UtilIO.java?rev=991485&r1=991484&r2=991485&view=diff
> ==============================================================================
> --- ofbiz/trunk/framework/base/src/org/ofbiz/base/util/UtilIO.java (original)
> +++ ofbiz/trunk/framework/base/src/org/ofbiz/base/util/UtilIO.java Wed Sep  1 09:15:14 2010
> @@ -399,7 +399,7 @@ public final class UtilIO {
>                   return;
>               }
>           } catch (Exception e) {
> +            throw new IOException("Can't write (" + value + ")");
>           }
> -        throw new IOException("Can't write (" + value + ")");
>       }
>   }
>
>

[Jacques Le Roux]

From: "Adam Heath" <[hidden email]>

> On 09/01/2010 04:15 AM, [hidden email] wrote:
>> Author: jleroux
>> Date: Wed Sep  1 09:15:14 2010
>> New Revision: 991485
>>
>> URL: http://svn.apache.org/viewvc?rev=991485&view=rev
>> Log:
>> A patch from Liu Xiangqian "method writeObject(StringBuilder sb, Object value, boolean allowJsonResolve) in UtilIO always throw
>> an IOException" (https://issues.apache.org/jira/browse/OFBIZ-3916) - OFBIZ-3916
>>
>> Tried to invoke method UtilIO.writeObject() but found the following code
>>
>> try {
>> StringWriter writer = new StringWriter();
>> if (encodeObject(writer, value, allowJsonResolve)) { sb.append(writer.toString()); return; }
>> } catch (Exception e) {
>> }
>> throw new IOException("Can't write (" + value + ")");
>>
>> It caused the method to always throw an IOException.
>>
>> Modified:
>>      ofbiz/trunk/framework/base/src/org/ofbiz/base/util/UtilIO.java
>
> This patch needs to be reverted.  writeObject() calls encodeObject(), which *always* returns true, unless an error occurrs.  This
> means the true block of the if is run, which then returns, so the IOException never gets thrown.

Well spotted! It's a pity though a comment did not explain this clearly in code. Empty catches are really the kind of things that
would support even a raw comment, and would avoid all this trouble...
I put it back and added a comment at r997526

Jacques

> Randomly applying patches from jira, that only contain a description, and no actual example, to code that you didn't originally
> write yourself, should not be done.  People need to understand what they are changing, and that wasn't done here.
>
> Sometimes, the code is formatted a certain way to provide full coverage; I'm not saying that is the case here, but just looking at
> the method and code flow will show that IOException is not always thrown as the reporter says.
>
>
>
>
>>
>> Modified: ofbiz/trunk/framework/base/src/org/ofbiz/base/util/UtilIO.java
>> URL:
>> http://svn.apache.org/viewvc/ofbiz/trunk/framework/base/src/org/ofbiz/base/util/UtilIO.java?rev=991485&r1=991484&r2=991485&view=diff
>> ==============================================================================
>> --- ofbiz/trunk/framework/base/src/org/ofbiz/base/util/UtilIO.java (original)
>> +++ ofbiz/trunk/framework/base/src/org/ofbiz/base/util/UtilIO.java Wed Sep  1 09:15:14 2010
>> @@ -399,7 +399,7 @@ public final class UtilIO {
>>                   return;
>>               }
>>           } catch (Exception e) {
>> +            throw new IOException("Can't write (" + value + ")");
>>           }
>> -        throw new IOException("Can't write (" + value + ")");
>>       }
>>   }
>>
>>
>

[Adam Heath-2]

On 09/15/2010 05:22 PM, Jacques Le Roux wrote:

> From: "Adam Heath" <[hidden email]>
>> On 09/01/2010 04:15 AM, [hidden email] wrote:
>>> Author: jleroux
>>> Date: Wed Sep 1 09:15:14 2010
>>> New Revision: 991485
>>>
>>> URL: http://svn.apache.org/viewvc?rev=991485&view=rev
>>> Log:
>>> A patch from Liu Xiangqian "method writeObject(StringBuilder sb,
>>> Object value, boolean allowJsonResolve) in UtilIO always throw an
>>> IOException" (https://issues.apache.org/jira/browse/OFBIZ-3916) -
>>> OFBIZ-3916
>>>
>>> Tried to invoke method UtilIO.writeObject() but found the following code
>>>
>>> try {
>>> StringWriter writer = new StringWriter();
>>> if (encodeObject(writer, value, allowJsonResolve)) {
>>> sb.append(writer.toString()); return; }
>>> } catch (Exception e) {
>>> }
>>> throw new IOException("Can't write (" + value + ")");
>>>
>>> It caused the method to always throw an IOException.
>>>
>>> Modified:
>>> ofbiz/trunk/framework/base/src/org/ofbiz/base/util/UtilIO.java
>>
>> This patch needs to be reverted. writeObject() calls encodeObject(),
>> which *always* returns true, unless an error occurrs. This means the
>> true block of the if is run, which then returns, so the IOException
>> never gets thrown.
>
> Well spotted! It's a pity though a comment did not explain this clearly
> in code. Empty catches are really the kind of things that would support
> even a raw comment, and would avoid all this trouble...
> I put it back and added a comment at r997526

Er, yeah, you're right about a comment needs to be here.  I'll add
that to my long todo list.

Restating what I said: this patch filed in jira did not contain an
example that showed a problem.  It only contained a description of the
code flow.  Without such an example, it's would be hard to know if the
patch actually fixed anything, or causes further problems.  Such
issues that get filed should not be acted upon, until more is known.

Btw, did you reopen the issue, and ask for further clarification?
Code reviews are fine, but, as is the case here, sometimes others
don't fully understand what is actually happening.

>
> Jacques
>
>> Randomly applying patches from jira, that only contain a description,
>> and no actual example, to code that you didn't originally write
>> yourself, should not be done. People need to understand what they are
>> changing, and that wasn't done here.
>>
>> Sometimes, the code is formatted a certain way to provide full
>> coverage; I'm not saying that is the case here, but just looking at
>> the method and code flow will show that IOException is not always
>> thrown as the reporter says.
>>
>>
>>
>>
>>>
>>> Modified: ofbiz/trunk/framework/base/src/org/ofbiz/base/util/UtilIO.java
>>> URL:
>>> http://svn.apache.org/viewvc/ofbiz/trunk/framework/base/src/org/ofbiz/base/util/UtilIO.java?rev=991485&r1=991484&r2=991485&view=diff
>>>
>>> ==============================================================================
>>>
>>> --- ofbiz/trunk/framework/base/src/org/ofbiz/base/util/UtilIO.java
>>> (original)
>>> +++ ofbiz/trunk/framework/base/src/org/ofbiz/base/util/UtilIO.java
>>> Wed Sep 1 09:15:14 2010
>>> @@ -399,7 +399,7 @@ public final class UtilIO {
>>> return;
>>> }
>>> } catch (Exception e) {
>>> + throw new IOException("Can't write (" + value + ")");
>>> }
>>> - throw new IOException("Can't write (" + value + ")");
>>> }
>>> }
>>>
>>>
>>
>
>

[Jacques Le Roux]

From: "Adam Heath" <[hidden email]>

> On 09/15/2010 05:22 PM, Jacques Le Roux wrote:
>> From: "Adam Heath" <[hidden email]>
>>> On 09/01/2010 04:15 AM, [hidden email] wrote:
>>>> Author: jleroux
>>>> Date: Wed Sep 1 09:15:14 2010
>>>> New Revision: 991485
>>>>
>>>> URL: http://svn.apache.org/viewvc?rev=991485&view=rev
>>>> Log:
>>>> A patch from Liu Xiangqian "method writeObject(StringBuilder sb,
>>>> Object value, boolean allowJsonResolve) in UtilIO always throw an
>>>> IOException" (https://issues.apache.org/jira/browse/OFBIZ-3916) -
>>>> OFBIZ-3916
>>>>
>>>> Tried to invoke method UtilIO.writeObject() but found the following code
>>>>
>>>> try {
>>>> StringWriter writer = new StringWriter();
>>>> if (encodeObject(writer, value, allowJsonResolve)) {
>>>> sb.append(writer.toString()); return; }
>>>> } catch (Exception e) {
>>>> }
>>>> throw new IOException("Can't write (" + value + ")");
>>>>
>>>> It caused the method to always throw an IOException.
>>>>
>>>> Modified:
>>>> ofbiz/trunk/framework/base/src/org/ofbiz/base/util/UtilIO.java
>>>
>>> This patch needs to be reverted. writeObject() calls encodeObject(),
>>> which *always* returns true, unless an error occurrs. This means the
>>> true block of the if is run, which then returns, so the IOException
>>> never gets thrown.
>>
>> Well spotted! It's a pity though a comment did not explain this clearly
>> in code. Empty catches are really the kind of things that would support
>> even a raw comment, and would avoid all this trouble...
>> I put it back and added a comment at r997526
>
> Er, yeah, you're right about a comment needs to be here.  I'll add that to my long todo list.

No needs, if you read the line above, I said "and added a comment"

> Restating what I said: this patch filed in jira did not contain an example that showed a problem.  It only contained a description
> of the code flow.  Without such an example, it's would be hard to know if the patch actually fixed anything, or causes further
> problems.  Such issues that get filed should not be acted upon, until more is known.

Yes I agree, especially for things that touch the Framework it should be the way to go and I will care about that in the future!

> Btw, did you reopen the issue, and ask for further clarification? Code reviews are fine, but, as is the case here, sometimes
> others don't fully understand what is actually happening.

Good point, I will put a note there also (a least now there is a comment in code ;o)

Jacques

>>
>> Jacques
>>
>>> Randomly applying patches from jira, that only contain a description,
>>> and no actual example, to code that you didn't originally write
>>> yourself, should not be done. People need to understand what they are
>>> changing, and that wasn't done here.
>>>
>>> Sometimes, the code is formatted a certain way to provide full
>>> coverage; I'm not saying that is the case here, but just looking at
>>> the method and code flow will show that IOException is not always
>>> thrown as the reporter says.
>>>
>>>
>>>
>>>
>>>>
>>>> Modified: ofbiz/trunk/framework/base/src/org/ofbiz/base/util/UtilIO.java
>>>> URL:
>>>> http://svn.apache.org/viewvc/ofbiz/trunk/framework/base/src/org/ofbiz/base/util/UtilIO.java?rev=991485&r1=991484&r2=991485&view=diff
>>>>
>>>> ==============================================================================
>>>>
>>>> --- ofbiz/trunk/framework/base/src/org/ofbiz/base/util/UtilIO.java
>>>> (original)
>>>> +++ ofbiz/trunk/framework/base/src/org/ofbiz/base/util/UtilIO.java
>>>> Wed Sep 1 09:15:14 2010
>>>> @@ -399,7 +399,7 @@ public final class UtilIO {
>>>> return;
>>>> }
>>>> } catch (Exception e) {
>>>> + throw new IOException("Can't write (" + value + ")");
>>>> }
>>>> - throw new IOException("Can't write (" + value + ")");
>>>> }
>>>> }
>>>>
>>>>
>>>
>>
>>
>

[Jacques Le Roux]

From: "Jacques Le Roux" <[hidden email]>

> From: "Adam Heath" <[hidden email]>
>> On 09/15/2010 05:22 PM, Jacques Le Roux wrote:
>>> From: "Adam Heath" <[hidden email]>
>>>> On 09/01/2010 04:15 AM, [hidden email] wrote:
>>>>> Author: jleroux
>>>>> Date: Wed Sep 1 09:15:14 2010
>>>>> New Revision: 991485
>>>>>
>>>>> URL: http://svn.apache.org/viewvc?rev=991485&view=rev
>>>>> Log:
>>>>> A patch from Liu Xiangqian "method writeObject(StringBuilder sb,
>>>>> Object value, boolean allowJsonResolve) in UtilIO always throw an
>>>>> IOException" (https://issues.apache.org/jira/browse/OFBIZ-3916) -
>>>>> OFBIZ-3916
>>>>>
>>>>> Tried to invoke method UtilIO.writeObject() but found the following code
>>>>>
>>>>> try {
>>>>> StringWriter writer = new StringWriter();
>>>>> if (encodeObject(writer, value, allowJsonResolve)) {
>>>>> sb.append(writer.toString()); return; }
>>>>> } catch (Exception e) {
>>>>> }
>>>>> throw new IOException("Can't write (" + value + ")");
>>>>>
>>>>> It caused the method to always throw an IOException.
>>>>>
>>>>> Modified:
>>>>> ofbiz/trunk/framework/base/src/org/ofbiz/base/util/UtilIO.java
>>>>
>>>> This patch needs to be reverted. writeObject() calls encodeObject(),
>>>> which *always* returns true, unless an error occurrs. This means the
>>>> true block of the if is run, which then returns, so the IOException
>>>> never gets thrown.
>>>
>>> Well spotted! It's a pity though a comment did not explain this clearly
>>> in code. Empty catches are really the kind of things that would support
>>> even a raw comment, and would avoid all this trouble...
>>> I put it back and added a comment at r997526
>>
>> Er, yeah, you're right about a comment needs to be here.  I'll add that to my long todo list.
>
> No needs, if you read the line above, I said "and added a comment"
>
>> Restating what I said: this patch filed in jira did not contain an example that showed a problem.  It only contained a
>> description of the code flow.  Without such an example, it's would be hard to know if the patch actually fixed anything, or
>> causes further problems.  Such issues that get filed should not be acted upon, until more is known.
>
> Yes I agree, especially for things that touch the Framework it should be the way to go and I will care about that in the future!
>
>> Btw, did you reopen the issue, and ask for further clarification? Code reviews are fine, but, as is the case here, sometimes
>> others don't fully understand what is actually happening.
>
> Good point, I will put a note there also (a least now there is a comment in code ;o)

Done
Jacques

> Jacques
>
>>>
>>> Jacques
>>>
>>>> Randomly applying patches from jira, that only contain a description,
>>>> and no actual example, to code that you didn't originally write
>>>> yourself, should not be done. People need to understand what they are
>>>> changing, and that wasn't done here.
>>>>
>>>> Sometimes, the code is formatted a certain way to provide full
>>>> coverage; I'm not saying that is the case here, but just looking at
>>>> the method and code flow will show that IOException is not always
>>>> thrown as the reporter says.
>>>>
>>>>
>>>>
>>>>
>>>>>
>>>>> Modified: ofbiz/trunk/framework/base/src/org/ofbiz/base/util/UtilIO.java
>>>>> URL:
>>>>> http://svn.apache.org/viewvc/ofbiz/trunk/framework/base/src/org/ofbiz/base/util/UtilIO.java?rev=991485&r1=991484&r2=991485&view=diff
>>>>>
>>>>> ==============================================================================
>>>>>
>>>>> --- ofbiz/trunk/framework/base/src/org/ofbiz/base/util/UtilIO.java
>>>>> (original)
>>>>> +++ ofbiz/trunk/framework/base/src/org/ofbiz/base/util/UtilIO.java
>>>>> Wed Sep 1 09:15:14 2010
>>>>> @@ -399,7 +399,7 @@ public final class UtilIO {
>>>>> return;
>>>>> }
>>>>> } catch (Exception e) {
>>>>> + throw new IOException("Can't write (" + value + ")");
>>>>> }
>>>>> - throw new IOException("Can't write (" + value + ")");
>>>>> }
>>>>> }
>>>>>
>>>>>
>>>>
>>>
>>>
>>
>
>