Chris, Looks good for me.
Thank you for doing it. -Dmitry On 2013-06-28 13:36, Chris Hegarty wrote: > The latest webrev is > http://cr.openjdk.java.net/~jzavgren/8015799/webrev.03/ > > We end up with: > > private String filterHeaderField(String name, String value) { > if (value == null) > return null; > > if (SET_COOKIE.equalsIgnoreCase(name) || > SET_COOKIE2.equalsIgnoreCase(name)) { > > // Filtering only if there is a cookie handler. [Assumption: > the > // cookie handler will store/retrieve the HttpOnly cookies] > if (cookieHandler == null || value.length() == 0) > return value; > .... > > So return value is not changed. BTW. I agree with your comments. > > -Chris. > > > On 06/27/2013 06:13 PM, Dmitry Samersoff wrote: >> 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.zavg...@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.
