RFR JDK8015799

[Dmitry Samersoff]

Chris,

1. I'm not sure it's a correct to return null rather then empty value,
   but you understand better what is happening, so I'm leaving it up
   to you.

2. It might be better to move

  2805         if (value == null)
  2806             return null;

under if(SET_COOKIE ...), i.e. to ll. 2810

it doesn't change anything in practice - the methods continue returning
null for all cases where value is null -  but makes code better
understandable.

-Dmitry


On 2013-06-27 00:49, Chris Hegarty wrote:
> To link this email thread, both in the archives, and for others. The
> call for review on this bug started with:
>   http://mail.openjdk.java.net/pipermail/net-dev/2013-June/006607.html
> 
> On 06/26/2013 08:22 PM, Kurchi Hazra wrote:
>>
>> On 6/26/2013 12:17 PM, Kurchi Hazra wrote:
>>> Hi John,
>>>
>>>   Why not change lines 2810-2811 to:
>>>     if (value == null || value.length() == 0)
>>>         return value;
>> I meant return null. For other cookie-headers too, is there any reason
>> for us not returning null if the length of value is 0?
> 
> In the first webrev John had made this change, but I asked him to revert
> it and only change the Set-Cookie(2) headers.
> 
> "Since all header retrieval passes through filterHeaderField, in one way
> or another, I'm a little concerned about changing this. Also, as the
> only issue we know of is with Set-Cookie(2), maybe you could add the
> empty string check to these headers only? ( that is to say, move the
> 'value.length() == 0' check into the ' if
> (SET_COOKIE.equalsIgnoreCase(name)..... '  "
> 
> The difference is, currently if a header value is non-null and has a
> length of 0 ( i.e. empty string ), then empty string is returned. With
> the original change then null is returned.
> 
> We have been bitten by subtle changes in this area before. Returning
> null from such an API, URLConnection.getHeaderField(s), for cases where
> it did not return null before may lead to NPE's in some applications.
> 
> -Chris.
> 
>>>
>>> Also, lots of formatting issue in the test, especially in
>>> TestCookieHandler, try-catch block indentation is off in line 54.
>>>  Its also best to stop the server in a finally clause at line 58.
>>> Alternatively, I also liked Andreas' use of autocloseable in his test
>>> for 6563286. See [1].
> 
> Yes, please.
> 
> -Chris.
> 
>>>
>>> - Kurchi
>>>
>>> [1]
>>> http://cr.openjdk.java.net/~arieber/6563286/webrev.00/test/sun/net/www/http/HttpURLConnection/MalformedFollowRedirect.java.html
>>>
>>> <http://cr.openjdk.java.net/%7Earieber/6563286/webrev.00/test/sun/net/www/http/HttpURLConnection/MalformedFollowRedirect.java.html>
>>>
>>>
>>> On 6/26/2013 10:54 AM, John Zavgren wrote:
>>>> Please consider the following changes to the Java cookie code.
>>>>
>>>> http://cr.openjdk.java.net/~jzavgren/8015799/webrev.02/
>>>> <http://cr.openjdk.java.net/%7Ejzavgren/8015799/webrev.02/>
>>>>
>>>> The problem I fixed occurs when a server returns an array of cookies
>>>> that contains a null cookie.
>>>>
>>>> Thanks
>>>> John
>>>> -- 
>>>> John Zavgren
>>>> john.zavgren at oracle.com
>>>> 603-821-0904
>>>> US-Burlington-MA
>>>
>>
>> -- 
>> -Kurchi
>>


-- 
Dmitry Samersoff
Oracle Java development team, Saint Petersburg, Russia
* I would love to change the world, but they won't give me the sources.