[rfc][icedtea-web] PolicyEditor sorted entries list
Andrew Azores
aazores at redhat.com
Mon Aug 17 21:08:47 UTC 2015
On 17/08/15 03:57 PM, Andrew Azores wrote:
> On 17/08/15 01:46 AM, Jiri Vanek wrote:
>> On 07/30/2015 10:42 PM, Andrew Azores wrote:
>>> Hi,
>>>
>>> This patch makes it so that the list of entries/identifiers in
>>> PolicyEditor is sorted, mainly so
>>> that entries will appear with a consistent ordering. This also
>>> guarantees that the
>>> ALL_APPLETS_IDENTIFIER is always the least element and thus always
>>> appears consistently at the top
>>
>> Good nit ;)
>>
>>> of the list.
>>>
>>> --
>>> Thanks,
>>>
>>> Andrew Azores
>>>
>>>
>>> sorted-policyeditor-list.patch
>>>
>>>
>>> # HG changeset patch
>>> # User Andrew Azores<aazores at redhat.com>
>>> # Date 1438288913 14400
>>> # Thu Jul 30 16:41:53 2015 -0400
>>> # Node ID bce05dbb84e73eff67dc8ac6af9509a5eaf36b8a
>>> # Parent 09792ebffb27b328f47e5ab5f5e075f2c2fe0a18
>>> Sort identifiers list in PolicyEditor
>>>
>>> * NEWS: add note about sorting
>>> * netx/net/sourceforge/jnlp/security/policyeditor/PolicyEditor.java
>>> (addNewEntry): clear listModel before adding new identifiers from
>>> policyEditorController. Controller handles sorting results for us,
>>> so we
>>> just clear and repopulate the UI.
>>> *
>>> netx/net/sourceforge/jnlp/security/policyeditor/PolicyEditorController.java
>>> (getIdentifiers): return TreeSet rather than HashSet because of TreeSet
>>> providing sorting
>>> * netx/net/sourceforge/jnlp/security/policyeditor/PolicyFileModel.java
>>> (getIdentifiers): likewise
>>> * netx/net/sourceforge/jnlp/security/policyeditor/PolicyIdentifier.java
>>> (compareTo, compare): new methods for implementing Comparable
>>>
>>> diff --git a/ChangeLog b/ChangeLog
>>> --- a/ChangeLog
>>> +++ b/ChangeLog
>>> @@ -1,3 +1,19 @@
>>> +2015-07-30 Andrew Azores<aazores at redhat.com>
>>> +
>>> + Sort identifiers list in PolicyEditor
>>> + * NEWS: add note about sorting
>>> + *
>>> netx/net/sourceforge/jnlp/security/policyeditor/PolicyEditor.java
>>> + (addNewEntry): clear listModel before adding new identifiers from
>>> + policyEditorController. Controller handles sorting results for
>>> us, so we
>>> + just clear and repopulate the UI.
>>> + *
>>> netx/net/sourceforge/jnlp/security/policyeditor/PolicyEditorController.java
>>> + (getIdentifiers): return TreeSet rather than HashSet because of
>>> TreeSet
>>> + providing sorting
>>> + *
>>> netx/net/sourceforge/jnlp/security/policyeditor/PolicyFileModel.java
>>> + (getIdentifiers): likewise
>>> + *
>>> netx/net/sourceforge/jnlp/security/policyeditor/PolicyIdentifier.java
>>> + (compareTo, compare): new methods for implementing Comparable
>>> +
>>> 2015-07-30 Andrew Azores<aazores at redhat.com>
>>>
>>> Add tests for PolicyEditor.getFilePathArgument
>>> diff --git a/NEWS b/NEWS
>>> --- a/NEWS
>>> +++ b/NEWS
>>> @@ -29,6 +29,7 @@
>>> - fixed issues with -html shortcuts
>>> - fixed issue with -html receiving garbage in width and height
>>> * PolicyEditor
>>> + - Entry list is sorted, entries will appear with consistent ordering
>>
>> I would probably not include this to NEWS as it is small
>> implementation detail. But Its up to you,
>>
>>> - file flag made to work when used standalone
>>> - file flag cannot be used in combination with main argument
>>> - defaultfile flag added
>>> diff --git
>>> a/netx/net/sourceforge/jnlp/security/policyeditor/PolicyEditor.java
>>> b/netx/net/sourceforge/jnlp/security/policyeditor/PolicyEditor.java
>>> --- a/netx/net/sourceforge/jnlp/security/policyeditor/PolicyEditor.java
>>> +++ b/netx/net/sourceforge/jnlp/security/policyeditor/PolicyEditor.java
>>> @@ -840,6 +840,7 @@
>>> invokeRunnableOrEnqueueLater(new Runnable() {
>>> @Override
>>> public void run() {
>>> + listModel.clear();
>>
>> Why the clear was not necessary before?
>
> Since this is being done on an addition operation (addNewEntry), the
> listModel is expected to already contain most of the PolicyIdentifiers
> which policyEditorController.getIdentifiers() returns to us. We
> iterate over that collection and add any missing elements into the
> list in the order we come across them. This no longer works, however,
> if we expect that policyEditorController.getIdentifiers() is returning
> to us a *sorted* collection, because listModel.addElement() will just
> append whichever new items we find to the end of the list, so the
> sorting is not preserved in the UI. Rather than performing a sort on
> the listModel (which there isn't even a very nice way to do), it made
> more sense to me to just clear the listModel and re-add everything
> that the PolicyEditorController gives back, since that's supposed to
> be the canonical source of information anyway.
>
> This also goes along with the old task of reducing the statefulness of
> the UI and doing more MVC refactoring work to PolicyEditor, which I
> never got around to way back when. I have been chipping away at that
> lately, though. Not sure when that patch might come out.
>
>>
>>> for (final PolicyIdentifier identifier :
>>> policyEditorController.getIdentifiers()) {
>>> if (!listModel.contains(identifier)) {
>>> listModel.addElement(identifier);
>>> diff --git
>>> a/netx/net/sourceforge/jnlp/security/policyeditor/PolicyEditorController.java
>>> b/netx/net/sourceforge/jnlp/security/policyeditor/PolicyEditorController.java
>>>
>>> ---
>>> a/netx/net/sourceforge/jnlp/security/policyeditor/PolicyEditorController.java
>>> +++
>>> b/netx/net/sourceforge/jnlp/security/policyeditor/PolicyEditorController.java
>>> @@ -48,6 +48,8 @@
>>> import java.util.HashSet;
>>> import java.util.Map;
>>> import java.util.Set;
>>> +import java.util.SortedSet;
>>> +import java.util.TreeSet;
>>>
>>> import net.sourceforge.jnlp.util.logging.OutputController;
>>> import sun.security.provider.PolicyParser;
>>> @@ -102,8 +104,8 @@
>>> policyFile.removeIdentifier(identifier);
>>> }
>>>
>>> - public Set<PolicyIdentifier> getIdentifiers() {
>>> - return new HashSet<>(policyFile.getIdentifiers());
>>> + public SortedSet<PolicyIdentifier> getIdentifiers() {
>>> + return new TreeSet<>(policyFile.getIdentifiers());
>>
>> Good idea!
>>
>>> }
>>>
>>> public Map<PolicyIdentifier, Map<PolicyEditorPermissions,
>>> Boolean>> getCopyOfPermissions() {
>>> diff --git
>>> a/netx/net/sourceforge/jnlp/security/policyeditor/PolicyFileModel.java
>>> b/netx/net/sourceforge/jnlp/security/policyeditor/PolicyFileModel.java
>>> ---
>>> a/netx/net/sourceforge/jnlp/security/policyeditor/PolicyFileModel.java
>>> +++
>>> b/netx/net/sourceforge/jnlp/security/policyeditor/PolicyFileModel.java
>>> @@ -48,6 +48,8 @@
>>> import java.util.Map;
>>> import java.util.Objects;
>>> import java.util.Set;
>>> +import java.util.SortedSet;
>>> +import java.util.TreeSet;
>>>
>>> import net.sourceforge.jnlp.util.FileUtils;
>>> import net.sourceforge.jnlp.util.MD5SumWatcher;
>>> @@ -201,8 +203,8 @@
>>> return fileWatcher != null && fileWatcher.update();
>>> }
>>>
>>> - synchronized Set<PolicyIdentifier> getIdentifiers() {
>>> - return new HashSet<>(permissionsMap.keySet());
>>> + synchronized SortedSet<PolicyIdentifier> getIdentifiers() {
>>> + return new TreeSet<>(permissionsMap.keySet());
>>> }
>>>
>>> synchronized KeystoreInfo getKeystoreInfo() {
>>> diff --git
>>> a/netx/net/sourceforge/jnlp/security/policyeditor/PolicyIdentifier.java
>>> b/netx/net/sourceforge/jnlp/security/policyeditor/PolicyIdentifier.java
>>> ---
>>> a/netx/net/sourceforge/jnlp/security/policyeditor/PolicyIdentifier.java
>>> +++
>>> b/netx/net/sourceforge/jnlp/security/policyeditor/PolicyIdentifier.java
>>> @@ -43,10 +43,12 @@
>>> import java.util.ArrayList;
>>> import java.util.Collection;
>>> import java.util.Collections;
>>> +import java.util.LinkedHashSet;
>>> import java.util.List;
>>> +import java.util.Set;
>>>
>>> //http://docs.oracle.com/javase/7/docs/technotes/guides/security/PolicyFiles.html
>>>
>>> -public class PolicyIdentifier implements Serializable {
>>> +public class PolicyIdentifier implements
>>> Comparable<PolicyIdentifier>, Serializable {
>>>
>>> public static final PolicyIdentifier ALL_APPLETS_IDENTIFIER =
>>> new PolicyIdentifier(null,
>>> Collections.<PolicyParser.PrincipalEntry>emptySet(), null) {
>>> @Override
>>> @@ -56,7 +58,7 @@
>>> };
>>>
>>> private final String signedBy;
>>> - private final List<PolicyParser.PrincipalEntry> principals =
>>> new ArrayList<>();
>>> + private final LinkedHashSet<PolicyParser.PrincipalEntry>
>>> principals = new LinkedHashSet<>();
>>> private final String codebase;
>>>
>>> public PolicyIdentifier(final String signedBy, final
>>> Collection<PolicyParser.PrincipalEntry> principals, final String
>>> codebase) {
>>> @@ -77,7 +79,7 @@
>>> return signedBy;
>>> }
>>>
>>> - public List<PolicyParser.PrincipalEntry> getPrincipals() {
>>> + public Set<PolicyParser.PrincipalEntry> getPrincipals() {
>>> return principals;
>>> }
>>>
>>> @@ -144,4 +146,39 @@
>>> result = 31 * result + (codebase.hashCode());
>>> return result;
>>> }
>>> +
>>> + @Override
>>> + public int compareTo(PolicyIdentifier policyIdentifier) {
>>> + if (this.equals(ALL_APPLETS_IDENTIFIER) &&
>>> policyIdentifier.equals(ALL_APPLETS_IDENTIFIER)) {
>>> + return 0;
>>> + } else if (this.equals(ALL_APPLETS_IDENTIFIER) &&
>>> !policyIdentifier.equals(ALL_APPLETS_IDENTIFIER)) {
>>> + return -1;
>>> + } else if (!this.equals(ALL_APPLETS_IDENTIFIER) &&
>>> policyIdentifier.equals(ALL_APPLETS_IDENTIFIER)) {
>>> + return 1;
>>> + }
>>> +
>>> + final int codebaseComparison = compare(this.getCodebase(),
>>> policyIdentifier.getCodebase());
>>> + if (codebaseComparison != 0) {
>>> + return codebaseComparison;
>>> + }
>>> +
>>> + final int signedByComparison = compare(this.getSignedBy(),
>>> policyIdentifier.getSignedBy());
>>> + if (signedByComparison != 0) {
>>> + return signedByComparison;
>>> + }
>>> +
>>> + return Integer.compare(this.getPrincipals().hashCode(),
>>> policyIdentifier.getPrincipals().hashCode());
>>> + }
>>> +
>>
>> Well ths one looks scary :D
>> I would change changelog a bit, as this method do not implement compare.
>> Also I woudl rename it - like comapreComaprable() ?-)
>
> What do you mean... ? This patch makes it so that PolicyIdentifier
> implements Comparable<PolicyIdentifier> and this 'int
> compareTo(PolicyIdentifier policyIdentifier)' is there to fulfill that
> contract. The static helper method 'compare(T a, T b)' is just there
> to simplify null-checking and comparison of two Comparables, like
> codebases and signedBys.
>
> The first part of it is just guaranteeing that ALL_APPLETS_IDENTIFIER
> is always less than any other PolicyIdentifier, other than itself. The
> following parts just sort the identifiers normally with codebase being
> the primary sort attribute, then signedBy, and finally principals.
>
>
>>
>>> + private static <T extends Comparable<T>> int compare(T a, T b) {
>>> + if (a == null && b != null) {
>>> + return 1;
>>> + } else if (a != null && b == null) {
>>> + return -1;
>>> + } else if (a == b) {
>>> + return 0;
>>> + } else {
>>> + return a.compareTo(b);
>>> + }
>>> + }
>>> }
>>>
>>
>> Good to go after reconsidering above three nits.
>>
>> Thanx!
>> J.
>>
>
>>
>>> Hi,
>>>
>>> This patch makes it so that the list of entries/identifiers in
>>> PolicyEditor is sorted, mainly so
>>> that entries will appear with a consistent ordering. This also
>>> guarantees that the
>>> ALL_APPLETS_IDENTIFIER is always the least element and thus always
>>> appears consistently at the top
>>> of the list.
>>>
>> Also please add unittests so we can see what order is actually
>> expected....
>>
>> Thanx!
>>
>>
>> J,
>
> Tests added, new patch attached.
>
Ha! Sorry, new patch attached. The license header in
PolicyIdentifierTest was the wrong one :) no changes otherwise.
--
Thanks,
Andrew Azores
-------------- next part --------------
A non-text attachment was scrubbed...
Name: sorted-policyeditor-list-2.patch
Type: text/x-patch
Size: 15705 bytes
Desc: not available
URL: <http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20150817/11300c95/sorted-policyeditor-list-2-0001.patch>
More information about the distro-pkg-dev
mailing list