RFR 8114860: Behavior of java.net.URLPermission.getActions() contradicts spec

[Vyom Tewari]

Hi Pavel,

thanks for review, even i did not understand why test case is comparing 
the hashcode with the hard coded values that's why at first place i did 
not removed it. As you mentioned   value doesn't matter i will modify 
the hashcode test and will send the modified webrev again. I was not 
expecting that hashcodetest is testing NPE by comparing the 
*"hardcoded"* hash values.

Thanks,
Vyom

On Monday 20 June 2016 08:17 PM, Pavel Rappo wrote:
> Vyom,
>
> You've noticed the bug in the code I proposed: "b =" instead of "b +=". Good!
> (Sorry about that.)
>
> When I asked about the strings that were commented out, I meant we'd better do
> something about it. First of all, because it's almost always not a good idea to
> *push a commented out code* without yet another comment explaining why we're
> doing so. Otherwise it confuses people and wastes their precious attention
> later.
>
> Secondly, commented out or removed, it doesn't really matter in this particular
> case. As I understand correctly these lines were there to test
>
>      8027570: NullPointerException in URLPermission.hashCode()
>
> If we remove these lines, this issue will no longer be tested. What was the
> problem with these hashCodes? Did they become different after the fix? If so,
> let's address it. As I understand the original issue, values do not matter, what
> matters is that there's no NPE thrown in these cases.
>
> Thanks,
> -Pavel
>
>> On 20 Jun 2016, at 14:51, Vyom Tewari <vyom.tewari at oracle.com> wrote:
>>
>> Hi,
>>
>> Please find the updated webrev(http://cr.openjdk.java.net/~vtewari/8114860/webrev0.1/index.html <http://cr.openjdk.java.net/%7Evtewari/8114860/webrev0.1/index.html>). I incorporated the review comments and remove the "HashCodeTest" where it compares the hashcode with hard coded hashcode.
>>
>> Thanks,
>> Vyom
>>
>>
>> On Monday 20 June 2016 03:57 PM, Pavel Rappo wrote:
>>> Hi Vyom, thanks for looking into this!
>>>
>>> 0.  * Copyright (c) 2013,2016 Oracle and/or its affiliates. All rights reserved.
>>>
>>> If I understand correctly, we're not required to update years in the header
>>> manually. But if you decided to do so, please follow the established pattern (in
>>> both files you've changed):
>>>
>>>      * Copyright (c) 2013, 2016, Oracle and/or its affiliates. All rights reserved.
>>>                           ^    ^
>>>
>>> 1. I wonder if we could use String convenience methods instead of bringing
>>> dependency on java.util.stream.Collector. Could something like this be a bit
>>> better both in terms of dependencies and readability?
>>>
>>>      private String actions() {
>>>          String b = String.join(",", methods);
>>>          if (!requestHeaders.isEmpty()) {
>>>              b = ":" + String.join(",", requestHeaders);
>>>          }
>>>          return b;
>>>      }
>>>
>>> 2. Is there a particular reason for these parentheses?
>>>
>>>      150             return (expectedActions.equals(urlp.getActions()));
>>>                             ^                                         ^
>>>
>>> 3. Why are these lines commented out?
>>>
>>>      259     static Test[] hashTests = {
>>>      260         //hashtest("http://www.foo.com/path", "GET:X-Foo", 388644203),
>>>      261         //hashtest("http:*", "*:*", 3255810)
>>>      262     };
>>>      IMO they either have to be updates or removed altogether.
>>>
>>> Thanks,
>>> -Pavel
>>>
>>>> On 20 Jun 2016, at 08:44, Vyom Tewari <vyom.tewari at oracle.com> wrote:
>>>>
>>>> ping!
>>>>
>>>> On Saturday 11 June 2016 10:34 AM, vyom wrote:
>>>>> Hi All,
>>>>>
>>>>> Please review the below fix.
>>>>>
>>>>> Bug               : JDK-8114860 Behavior of java.net.URLPermission.getActions() contradicts spec
>>>>> Webrev         : http://cr.openjdk.java.net/~vtewari/8114860/webrev0.0/index.html <http://cr.openjdk.java.net/%7Evtewari/8114860/webrev0.0/index.html>
>>>>>
>>>>> Thanks,
>>>>> Vyom

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/net-dev/attachments/20160620/8ccd0bd2/attachment.html>