RFR 8071660 :URLPermission not handling empty method lists correctly

[Vyom Tewari]

Hi Pavel,

Please find the latest 
webrev(http://cr.openjdk.java.net/~vtewari/8071660/webrev0.4/index.html 
<http://cr.openjdk.java.net/%7Evtewari/8071660/webrev0.4/index.html>). I 
had incorporated the review comments.

Thanks,
Vyom


On Monday 20 June 2016 07:27 PM, Pavel Rappo wrote:
> Vyom, please consider the following changes:
>
> 1. Append 8071660 to the lists of tests here:
>
> 	* @bug 8010464 8027570 8027687 8029354 8071660
>                                                 ^
> 2. Reformat the code the way it's in the rest of the file:
>
> from:
>
>   266         URLPermission that = (URLPermission)p;
>   267
>   268         if(this.methods.isEmpty() && !that.methods.isEmpty())
>   269             return false;
>   270
>   271         if (!this.methods.isEmpty() && !this.methods.get(0).equals("*") &&
>   272                 Collections.indexOfSubList(this.methods, that.methods) == -1) {
>   273             return false;
>   274         }
>
> to:
>
>   266         URLPermission that = (URLPermission)p;
>   267
>   268         if (this.methods.isEmpty() && !that.methods.isEmpty()) {
>   269             return false;
>   270         }
>   271
>   272         if (!this.methods.isEmpty() &&
>   273             !this.methods.get(0).equals("*") &&
>   274             Collections.indexOfSubList(this.methods,
>   275                                        that.methods) == -1) {
>   276             return false;
>   277         }
>
> Other than these minor things, looks good.
>
> Thanks,
> -Pavel
>
>> On 20 Jun 2016, at 12:36, Daniel Fuchs <daniel.fuchs at oracle.com> wrote:
>>
>> On 17/06/16 15:09, Vyom Tewari wrote:
>>> Hi,
>>>
>>> Please find the latest
>>> webrev(http://cr.openjdk.java.net/~vtewari/8071660/webrev0.3/index.html
>>> <http://cr.openjdk.java.net/%7Evtewari/8071660/webrev0.3/index.html>). I
>>> fixed the typo along with spaces issue.
>> Looks good!
>>
>> -- daniel
>>
>>> Thanks,
>>> Vyom
>>>
>>>
>>> On Friday 17 June 2016 06:34 PM, Roger Riggs wrote:
>>>> Hi Vyom,
>>>>
>>>> URLPermissionTest.java:
>>>>
>>>> line 125:  it looks odd to assign the same variable twice with the
>>>> same value.
>>>>    Perhaps it should have been assigning this.url2 = url2.
>>>>
>>>> line 330-332: please add a space after "," in argument lists.
>>>>
>>>> Roger
>>>>
>>>>
>>>>
>>>>
>>>> On 6/17/2016 8:42 AM, Daniel Fuchs wrote:
>>>>> On 17/06/16 13:25, Vyom Tewari wrote:
>>>>>> Hi Daniel,
>>>>>>
>>>>>> thanks for review please find the latest
>>>>>> webrev(http://cr.openjdk.java.net/~vtewari/8071660/webrev0.2/index.html
>>>>>> <http://cr.openjdk.java.net/%7Evtewari/8071660/webrev0.2/index.html>) .
>>>>>> I added some more tests where base url is different.
>>>>> Thanks Vyom,
>>>>> Looks good to me.
>>>>>
>>>>> See if you can get someone more experienced in the area
>>>>> to give a thumb up too :-)
>>>>>
>>>>> best regards,
>>>>>
>>>>> -- daniel
>>>>>
>>>>>
>>>>>> Thanks,
>>>>>> Vyom
>>>>>>
>>>>>> On Friday 17 June 2016 04:57 PM, Daniel Fuchs wrote:
>>>>>>> On 17/06/16 09:21, Vyom Tewari wrote:
>>>>>>>> Hi All,
>>>>>>>>
>>>>>>>> Please find the new
>>>>>>>> webrev(http://cr.openjdk.java.net/~vtewari/8071660/webrev0.1/index.html
>>>>>>>>
>>>>>>>> <http://cr.openjdk.java.net/%7Evtewari/8071660/webrev0.1/index.html>).
>>>>>>>>
>>>>>>>> I  addressed the review comments given by Daniel.
>>>>>>> Hi Vyom,
>>>>>>>
>>>>>>> Looks good to me - but I'm a bit concerned that the previous
>>>>>>> mistake was not caught that by any test.
>>>>>>> Could you add a test that fails with you previous fix
>>>>>>> but passes with the new one?
>>>>>>>
>>>>>>> best regards,
>>>>>>>
>>>>>>> -- daniel
>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> Vyom
>>>>>>>>
>>>>>>>>
>>>>>>>> On Sunday 12 June 2016 02:04 PM, Daniel Fuchs wrote:
>>>>>>>>> Hi Vyom,
>>>>>>>>>
>>>>>>>>> This looks strange to me:
>>>>>>>>>
>>>>>>>>> 268         if(!this.methods.isEmpty() && that.methods.isEmpty())
>>>>>>>>> 269             return true;
>>>>>>>>> 270         if(this.methods.isEmpty() && !that.methods.isEmpty())
>>>>>>>>> 271             return false;
>>>>>>>>> 272         if(this.methods.isEmpty() && that.methods.isEmpty())
>>>>>>>>> 273             return true;
>>>>>>>>>
>>>>>>>>> Namely, lines 269 & 273 will return true before the URL part
>>>>>>>>> of the permission has been checked.
>>>>>>>>> Is that really the expected behavior?
>>>>>>>>>
>>>>>>>>> best regards,
>>>>>>>>>
>>>>>>>>> -- daniel
>>>>>>>>>
>>>>>>>>> On 11/06/16 05:50, vyom wrote:
>>>>>>>>>> Hi All,
>>>>>>>>>>
>>>>>>>>>> Please review the below fix.
>>>>>>>>>>
>>>>>>>>>> Bug       :         JDK-8071660 URLPermission not handling empty
>>>>>>>>>> method
>>>>>>>>>> lists correctly
>>>>>>>>>> Webrev :
>>>>>>>>>> http://cr.openjdk.java.net/~vtewari/8071660/webrev0.0/index.html
>>>>>>>>>> <http://cr.openjdk.java.net/%7Evtewari/8071660/webrev0.0/index.html>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Thanks,
>>>>>>>>>> Vyom