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

[Pavel Rappo]

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
>