Cleanup fallthrough in FilePermission and PropertyPermission was: Request for Review of 7116890 (Warning Cleanup java.io)

[Sebastian Sickelmann]

Am 05.12.2011 12:53, schrieb Weijun Wang:
>
>
> On 12/03/2011 06:12 AM, Stuart Marks wrote:
>> I'm adding Weijun (Max) Wang to this thread.
>>
>> The same "ackbarfaccept" code had come up a third time when I was
>> reviewing some of Max's changes. The code in question all has to do with
>> permissions, and Max is in the security group, so he might have a better
>> insight whether doing a refactoring is appropriate and how to approach
>> doing it.
>
> Maybe we can group all permissions with a target and a predefined set 
> of actions under a new abstract class.

This sounds like the best idea from the implementors point of view. I 
would prepare a patch as suggestion for this.
Regarding to my own question i would say that there isn't any additional 
jigsaw related problem here by
introducing a common abstract class. They all rely on 
java.security.Permission, so java.security seems the correct
package for the new abstract class.

Any suggestions for the name? PermissionWithActions???? AbstractPermission??

If i see it right there is the following class hierarchy:

j.s.Permission implements Serializable
+j.i.FilePermission
+j.n.SocketPermission
+jx.m.MBeanPermission
+jx.s.a.k.ServicePermission
+j.s.BasicPermission
  +j.u.PropertyPermission

What happens to serialization when the new hierarchy would look like this:

j.s.Permission implements Serializable
+j.s.AbstractPermission
+j.i.FilePermission
+j.n.SocketPermission
  +jx.m.MBeanPermission
+jx.s.a.k.ServicePermission
  +j.s.BasicPermission
   +j.u.PropertyPermission

All the classes have a serialVerionUID so i think serialisation is no 
problem, or is there
a problem i i don't see here?

-- Sebastian
> -Max
>
>>
>> Some searching around reveals that "ackbarfaccept" appears in the
>> PlayerPermission class of the Java ME Mobile Media API (not in any of
>> the OpenJDK repositories) so it looks like the code has been copied even
>> more times than are visible here. Perhaps a public permissions parsing
>> utility method is called for.
>>
>> s'marks
>>
>> On 12/2/11 9:02 AM, Sebastian Sickelmann wrote:
>>> Am 02.12.2011 16:27, schrieb Brandon Passanisi:
>>>> Hi Sebastian. I'm not sure if you had seen the e-mail from Stuart 
>>>> Marks
>>>> regarding this, but Stuart was able to find more instances of the
>>>> similar
>>>> block of "fallthrough" code. I can volunteer to apply your upcoming
>>>> change
>>>> to FilePermission to the other files if you wish. Or, you can try
>>>> applying
>>>> the change to the other files, but if you don't have time I don't
>>>> mind doing
>>>> it. Here's the section of Stuart's e-mail on this topic:
>>>>
>>>> ------------------------------------------------------------------------------------- 
>>>>
>>>>
>>>> (Incidentally, this is the third time I've reviewed code today that
>>>> looks exactly like this. The other cases are in java.io.FilePermission
>>>> and java.util.PropertyPermission. They each have the /*FALLTHROUGH*/
>>>> into a set of cases that do nothing but break; and they have similar
>>>> ("ackbarfaccept") comments. It would be nice if these chunks of code
>>>> could be unified, but they differ in a number of fiddly details.)
>>>>
>>>> (The string "ackbarfaccept" occurs in the following files:
>>>> 1. java/io/FilePermission.java
>>>> 2. java/net/SocketPermission.java
>>>> 3. java/util/PropertyPermission.java
>>>> 4. javax/management/MBeanPermission.java
>>>> 5. javax/security/auth/kerberos/ServicePermission.java
>>>> Hmmm.)
>>>> ------------------------------------------------------------------------------------- 
>>>>
>>>>
>>>>
>>>> Thanks.
>>>
>>> Hi Brandon,
>>> Hi Stuart,
>>>
>>> i had a look at all those classes and it seems to be the same
>>> algorithm. In an
>>> normal project (not the jdk) i would suggest to completely refactor it
>>> and make
>>> some code de-duplication cleanup. But if i thinks on projects like
>>> jigsaw this
>>> can easily get a real problem. What do you think, should we try to
>>> cleanup them
>>> all. Or should we try to make some de-duplication/code-reuse 
>>> refactoring.
>>>
>>>
>>> -- Sebastian
>>>>
>>>> On 12/1/2011 10:18 PM, Sebastian Sickelmann wrote:
>>>>> Hi Brandon,
>>>>>
>>>>> i will try to work out a fix for both and cc the review request to 
>>>>> you.
>>>>>
>>>>> -- Sebastian
>>>>>
>>>>> Am 01.12.2011 23:54, schrieb Brandon Passanisi:
>>>>>> Hi Sebastian. I was speaking with Stuart Marks earlier today and he
>>>>>> mentioned that the "fallthrough" code in FilePermission.java also
>>>>>> exists in
>>>>>> java.util.PropertyPermission.java. Maybe the code author had done 
>>>>>> some
>>>>>> copy/paste when working on these files. Stuart had said that you
>>>>>> might be
>>>>>> planning on doing some work on this after the warnings cleanup work.
>>>>>> If/when you do this work, maybe you can let me know so that I can
>>>>>> sync the
>>>>>> same changes you apply to FilePermission.java to
>>>>>> PropertyPermission.java?
>>>>>> Or, maybe you can apply the same changes yourself to
>>>>>> PropertyPermission.java?