[rfc][icedtea-web] PolicyEditor sorted entries list
Jiri Vanek
jvanek at redhat.com
Mon Aug 17 05:46:04 UTC 2015
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?
> 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() ?-)
> + 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.
More information about the distro-pkg-dev
mailing list