[rfc][icedtea-web] PolicyEditor sorted entries list
Andrew Azores
aazores at redhat.com
Mon Aug 17 19:57:47 UTC 2015
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.
--
Thanks,
Andrew Azores
-------------- next part --------------
A non-text attachment was scrubbed...
Name: sorted-policyeditor-list-2.patch
Type: text/x-patch
Size: 15744 bytes
Desc: not available
URL: <http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20150817/bc8fd894/sorted-policyeditor-list-2.patch>
More information about the distro-pkg-dev
mailing list