sis-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From desruisse...@apache.org
Subject svn commit: r1784553 - in /sis/branches/JDK8/core/sis-referencing-by-identifiers/src: main/java/org/apache/sis/internal/gazetteer/ main/java/org/apache/sis/referencing/gazetteer/ test/java/org/apache/sis/referencing/gazetteer/
Date Mon, 27 Feb 2017 12:44:57 GMT
Author: desruisseaux
Date: Mon Feb 27 12:44:57 2017
New Revision: 1784553

URL: http://svn.apache.org/viewvc?rev=1784553&view=rev
Log:
Add a check against infinite recursivity.

Modified:
    sis/branches/JDK8/core/sis-referencing-by-identifiers/src/main/java/org/apache/sis/internal/gazetteer/Resources.java
    sis/branches/JDK8/core/sis-referencing-by-identifiers/src/main/java/org/apache/sis/internal/gazetteer/Resources.properties
    sis/branches/JDK8/core/sis-referencing-by-identifiers/src/main/java/org/apache/sis/internal/gazetteer/Resources_fr.properties
    sis/branches/JDK8/core/sis-referencing-by-identifiers/src/main/java/org/apache/sis/referencing/gazetteer/AbstractLocationType.java
    sis/branches/JDK8/core/sis-referencing-by-identifiers/src/main/java/org/apache/sis/referencing/gazetteer/ModifiableLocationType.java
    sis/branches/JDK8/core/sis-referencing-by-identifiers/src/test/java/org/apache/sis/referencing/gazetteer/LocationTypeTest.java

Modified: sis/branches/JDK8/core/sis-referencing-by-identifiers/src/main/java/org/apache/sis/internal/gazetteer/Resources.java
URL: http://svn.apache.org/viewvc/sis/branches/JDK8/core/sis-referencing-by-identifiers/src/main/java/org/apache/sis/internal/gazetteer/Resources.java?rev=1784553&r1=1784552&r2=1784553&view=diff
==============================================================================
--- sis/branches/JDK8/core/sis-referencing-by-identifiers/src/main/java/org/apache/sis/internal/gazetteer/Resources.java
[UTF-8] (original)
+++ sis/branches/JDK8/core/sis-referencing-by-identifiers/src/main/java/org/apache/sis/internal/gazetteer/Resources.java
[UTF-8] Mon Feb 27 12:44:57 2017
@@ -62,6 +62,11 @@ public final class Resources extends Ind
         }
 
         /**
+         * Location type parent already has a child named “{0}”.
+         */
+        public static final short ChildAlreadyExists_1 = 8;
+
+        /**
          * “{0}” is not a valid grid coordinate.
          */
         public static final short IllegalGridCoordinate_1 = 1;
@@ -93,9 +98,24 @@ public final class Resources extends Ind
         public static final short InconsistentWithGZD_2 = 6;
 
         /**
+         * Location type “{0}” can not have itself as a children.
+         */
+        public static final short LocationTypeCycle_1 = 11;
+
+        /**
+         * No location type named “{0}” has been found.
+         */
+        public static final short LocationTypeNotFound_1 = 10;
+
+        /**
          * Can not determine dimension of “{0}” because of odd number of characters.
          */
         public static final short OddGridCoordinateLength_1 = 7;
+
+        /**
+         * A location type parent named “{0}” already exists.
+         */
+        public static final short ParentAlreadyExists_1 = 9;
     }
 
     /**

Modified: sis/branches/JDK8/core/sis-referencing-by-identifiers/src/main/java/org/apache/sis/internal/gazetteer/Resources.properties
URL: http://svn.apache.org/viewvc/sis/branches/JDK8/core/sis-referencing-by-identifiers/src/main/java/org/apache/sis/internal/gazetteer/Resources.properties?rev=1784553&r1=1784552&r2=1784553&view=diff
==============================================================================
--- sis/branches/JDK8/core/sis-referencing-by-identifiers/src/main/java/org/apache/sis/internal/gazetteer/Resources.properties
[ISO-8859-1] (original)
+++ sis/branches/JDK8/core/sis-referencing-by-identifiers/src/main/java/org/apache/sis/internal/gazetteer/Resources.properties
[ISO-8859-1] Mon Feb 27 12:44:57 2017
@@ -25,4 +25,8 @@ IllegalSquareIdentification_1     = \u20
 IllegalUPSZone_1                  = \u201c{0}\u201d is not a valid Universal Polar Stereographic
(UPS) zone.
 IllegalUTMZone_1                  = \u201c{0}\u201d is not a valid Universal Transverse Mercator
(UTM) zone.
 InconsistentWithGZD_2             = Square identification in the \u201c{0}\u201d reference
is inconsistent with the grid zone designation. A consistent zone would be \u201c{1}\u201d.
+LocationTypeCycle_1               = Location type \u201c{0}\u201d can not have itself as
a children.
+LocationTypeNotFound_1            = No location type named \u201c{0}\u201d has been found.
 OddGridCoordinateLength_1         = Can not determine dimension of \u201c{0}\u201d because
of odd number of characters.
+ParentAlreadyExists_1             = A location type parent named \u201c{0}\u201d already
exists.
+ChildAlreadyExists_1              = Location type parent already has a child named \u201c{0}\u201d.

Modified: sis/branches/JDK8/core/sis-referencing-by-identifiers/src/main/java/org/apache/sis/internal/gazetteer/Resources_fr.properties
URL: http://svn.apache.org/viewvc/sis/branches/JDK8/core/sis-referencing-by-identifiers/src/main/java/org/apache/sis/internal/gazetteer/Resources_fr.properties?rev=1784553&r1=1784552&r2=1784553&view=diff
==============================================================================
--- sis/branches/JDK8/core/sis-referencing-by-identifiers/src/main/java/org/apache/sis/internal/gazetteer/Resources_fr.properties
[ISO-8859-1] (original)
+++ sis/branches/JDK8/core/sis-referencing-by-identifiers/src/main/java/org/apache/sis/internal/gazetteer/Resources_fr.properties
[ISO-8859-1] Mon Feb 27 12:44:57 2017
@@ -30,4 +30,8 @@ IllegalSquareIdentification_1     = \u00
 IllegalUPSZone_1                  = \u00ab\u202f{0}\u202f\u00bb n\u2019est pas une zone valide
de la projection Polaire St\u00e9r\u00e9ographique Universelle (UPS).
 IllegalUTMZone_1                  = \u00ab\u202f{0}\u202f\u00bb n\u2019est pas une zone valide
de la projection Transverse Mercator Universelle (UTM).
 InconsistentWithGZD_2             = L\u2019identification du carr\u00e9 dans la r\u00e9f\u00e9rence
\u00ab\u202f{0}\u202f\u00bb n\u2019est pas coh\u00e9rente avec la d\u00e9signation de la zone
de la grille. Une zone coh\u00e9rente serait \u00ab\u202f{1}\u202f\u00bb.
+LocationTypeCycle_1               = Le type de location \u00ab\u202f{0}\u202f\u00bb ne doit
pas appara\u00eetre comme un de ses propres enfants.
+LocationTypeNotFound_1            = Aucun type de location nomm\u00e9 \u00ab\u202f{0}\u202f\u00bb
n\u2019a \u00e9t\u00e9 trouv\u00e9.
 OddGridCoordinateLength_1         = Ne peut pas d\u00e9terminer la dimension de \u00ab\u202f{0}\u202f\u00bb
\u00e0 cause du nombre impair de caract\u00e8res.
+ParentAlreadyExists_1             = Un type de location parent nomm\u00e9 \u00ab\u202f{0}\u202f\u00bb
existe d\u00e9j\u00e0.
+ChildAlreadyExists_1              = Le type de location parent a d\u00e9j\u00e0 un enfant
nomm\u00e9 \u00ab\u202f{0}\u202f\u00bb.

Modified: sis/branches/JDK8/core/sis-referencing-by-identifiers/src/main/java/org/apache/sis/referencing/gazetteer/AbstractLocationType.java
URL: http://svn.apache.org/viewvc/sis/branches/JDK8/core/sis-referencing-by-identifiers/src/main/java/org/apache/sis/referencing/gazetteer/AbstractLocationType.java?rev=1784553&r1=1784552&r2=1784553&view=diff
==============================================================================
--- sis/branches/JDK8/core/sis-referencing-by-identifiers/src/main/java/org/apache/sis/referencing/gazetteer/AbstractLocationType.java
[UTF-8] (original)
+++ sis/branches/JDK8/core/sis-referencing-by-identifiers/src/main/java/org/apache/sis/referencing/gazetteer/AbstractLocationType.java
[UTF-8] Mon Feb 27 12:44:57 2017
@@ -20,8 +20,10 @@ import java.util.List;
 import java.util.Arrays;
 import java.util.Objects;
 import java.util.IdentityHashMap;
+import java.util.Map;
 import org.opengis.referencing.gazetteer.ReferenceSystemUsingIdentifiers;
 import org.opengis.referencing.gazetteer.LocationType;
+import org.apache.sis.internal.gazetteer.Resources;
 import org.apache.sis.util.collection.DefaultTreeTable;
 import org.apache.sis.util.collection.TableColumn;
 import org.apache.sis.util.collection.TreeTable;
@@ -29,7 +31,6 @@ import org.apache.sis.util.LenientCompar
 import org.apache.sis.util.ComparisonMode;
 import org.apache.sis.util.ArgumentChecks;
 import org.apache.sis.util.Utilities;
-import org.apache.sis.util.Debug;
 
 
 /**
@@ -64,7 +65,37 @@ abstract class AbstractLocationType impl
      */
     public static List<LocationType> snapshot(final ReferenceSystemUsingIdentifiers
rs, final LocationType... types) {
         ArgumentChecks.ensureNonNull("types", types);
-        return FinalLocationType.snapshot(Arrays.asList(types), rs, new IdentityHashMap<>());
+        final List<LocationType> snapshot = FinalLocationType.snapshot(Arrays.asList(types),
rs, new IdentityHashMap<>());
+        final Map<LocationType,Boolean> parents = new IdentityHashMap<>();
+        for (final LocationType type : snapshot) {
+            checkForCycles(type, parents);
+        }
+        return snapshot;
+    }
+
+    /**
+     * Implementation of {@link #verifyCycle()} to be invoked recursively for each children.
+     *
+     * @throws IllegalArgumentException if an infinite recursivity is detected.
+     */
+    private static void checkForCycles(final LocationType type, final Map<LocationType,Boolean>
parents) {
+        if (parents.put(type, Boolean.TRUE) != null) {
+            throw new IllegalArgumentException(Resources.format(Resources.Keys.LocationTypeCycle_1,
type.getName()));
+        }
+        for (final LocationType child : type.getChildren()) {
+            checkForCycles(child, parents);
+        }
+        parents.remove(type);
+    }
+
+    /**
+     * Verifies that there is not cycles in the children.
+     * This method should be invoked for validating a user argument.
+     *
+     * @throws IllegalArgumentException if an infinite recursivity is detected.
+     */
+    final void checkForCycles() {
+        checkForCycles(this, new IdentityHashMap<>());
     }
 
     /**
@@ -115,8 +146,8 @@ abstract class AbstractLocationType impl
                 if (Objects.equals(getName(), that.getName())) {
                     /*
                      * To be safe, we should apply some check against infinite recursivity
here.
-                     * We do not on the assumption that the constructor verified that we
do not
-                     * have any cycle.
+                     * We do not on the assumption that subclasses verified that we do not
have
+                     * any cycle.
                      */
                     return Utilities.deepEquals(getChildren(), that.getChildren(), mode);
                 }
@@ -162,11 +193,20 @@ abstract class AbstractLocationType impl
 
     /**
      * Returns a string representation of this location type and all its children.
+     * Current implementation formats a tree with the {@linkplain ModifiableLocationType#getName()
name}
+     * and {@linkplain ModifiableLocationType#getDefinition() definition} of each type, like
below:
+     *
+     * {@preformat text
+     *   administrative area………………… area of responsibility of highest level
local authority
+     *     ├─town……………………………………………… city or town
+     *     │   └─street……………………………… thoroughfare providing
access to properties
+     *     └─street………………………………………… thoroughfare providing
access to properties
+     * }
+     *
      * The string representation is mostly for debugging purpose and may change in any future
SIS version.
      *
      * @return a string representation of this location type.
      */
-    @Debug
     @Override
     public String toString() {
         final DefaultTreeTable table = new DefaultTreeTable(TableColumn.NAME, TableColumn.VALUE_AS_TEXT);
@@ -176,6 +216,9 @@ abstract class AbstractLocationType impl
 
     /**
      * Invoked recursively for formatting the given type in the given tree.
+     * This method does not perform any check against infinite recursivity
+     * on the assumption that subclasses verified this constraint by calls
+     * to {@link #checkForCycles()}.
      */
     private static void format(final LocationType type, final TreeTable.Node node) {
         node.setValue(TableColumn.NAME, type.getName());

Modified: sis/branches/JDK8/core/sis-referencing-by-identifiers/src/main/java/org/apache/sis/referencing/gazetteer/ModifiableLocationType.java
URL: http://svn.apache.org/viewvc/sis/branches/JDK8/core/sis-referencing-by-identifiers/src/main/java/org/apache/sis/referencing/gazetteer/ModifiableLocationType.java?rev=1784553&r1=1784552&r2=1784553&view=diff
==============================================================================
--- sis/branches/JDK8/core/sis-referencing-by-identifiers/src/main/java/org/apache/sis/referencing/gazetteer/ModifiableLocationType.java
[UTF-8] (original)
+++ sis/branches/JDK8/core/sis-referencing-by-identifiers/src/main/java/org/apache/sis/referencing/gazetteer/ModifiableLocationType.java
[UTF-8] Mon Feb 27 12:44:57 2017
@@ -27,6 +27,7 @@ import org.opengis.metadata.extent.Geogr
 import org.opengis.referencing.gazetteer.ReferenceSystemUsingIdentifiers;
 import org.apache.sis.metadata.iso.extent.DefaultGeographicDescription;
 import org.apache.sis.metadata.iso.citation.DefaultOrganisation;
+import org.apache.sis.internal.gazetteer.Resources;
 import org.apache.sis.util.CorruptedObjectException;
 import org.apache.sis.util.resources.Errors;
 import org.apache.sis.util.ArgumentChecks;
@@ -402,15 +403,31 @@ public class ModifiableLocationType exte
      */
     public void addParent(final ModifiableLocationType parent) {
         ArgumentChecks.ensureNonNull("parent", parent);
-        final String key = parent.name.toString();
-        if (parents.putIfAbsent(key, parent) != null) {
-            throw new IllegalStateException(Errors.format(Errors.Keys.ElementAlreadyPresent_1,
key));
+        final String parentName = parent.name.toString();
+        if (parents.putIfAbsent(parentName, parent) != null) {
+            throw new IllegalStateException(Resources.format(Resources.Keys.ParentAlreadyExists_1,
parentName));
         }
-        if (parent.children.putIfAbsent(name.toString(), this) != null) {
-            if (parents.remove(key) != parent) {                            // Paranoiac
check.
-                throw new ConcurrentModificationException();
+        final String childName = name.toString();
+        if (parent.children.putIfAbsent(childName, this) != null) {
+            if (parents.remove(parentName) != parent) {
+                throw new ConcurrentModificationException();                    // Paranoiac
check.
             }
-            throw new IllegalArgumentException("Child already present");    // TODO: localize
+            throw new IllegalArgumentException(Resources.format(Resources.Keys.ChildAlreadyExists_1,
parentName));
+        }
+        /*
+         * Following verification is not very efficient (lot of verifications are repeated
every time that a new
+         * parent is added), and is redundant with the verification performed by snapshot(…)
method.  For now we
+         * perform this verification here on the assumption that the tree is small, and that
reporting errors
+         * early make development easier. However if performance become a concern, we could
remove this code
+         * and perform the verification when first needed in getParents() and getChildren(),
or make equals(…)
+         * and toString() methods tolerance to cycles.
+         */
+        try {
+            parent.checkForCycles();
+        } catch (IllegalArgumentException e) {
+            parent.children.remove(childName);                                  // Rollback
+            parents.remove(parentName);
+            throw e;
         }
     }
 
@@ -425,7 +442,7 @@ public class ModifiableLocationType exte
         final String key = parent.name.toString();
         final ModifiableLocationType removed = parents.remove(key);
         if (removed == null) {
-            throw new IllegalArgumentException(Errors.format(Errors.Keys.ElementNotFound_1,
key));
+            throw new IllegalArgumentException(Resources.format(Resources.Keys.LocationTypeNotFound_1,
key));
         }
         if (removed.children.remove(name.toString()) != this || removed != parent) {
             throw new CorruptedObjectException();

Modified: sis/branches/JDK8/core/sis-referencing-by-identifiers/src/test/java/org/apache/sis/referencing/gazetteer/LocationTypeTest.java
URL: http://svn.apache.org/viewvc/sis/branches/JDK8/core/sis-referencing-by-identifiers/src/test/java/org/apache/sis/referencing/gazetteer/LocationTypeTest.java?rev=1784553&r1=1784552&r2=1784553&view=diff
==============================================================================
--- sis/branches/JDK8/core/sis-referencing-by-identifiers/src/test/java/org/apache/sis/referencing/gazetteer/LocationTypeTest.java
[UTF-8] (original)
+++ sis/branches/JDK8/core/sis-referencing-by-identifiers/src/test/java/org/apache/sis/referencing/gazetteer/LocationTypeTest.java
[UTF-8] Mon Feb 27 12:44:57 2017
@@ -188,6 +188,15 @@ public final strictfp class LocationType
     @Test
     @DependsOnMethod("testInheritance")
     public void testToString() {
+        verifyToString(create(true)[0]);
+    }
+
+    /**
+     * Verify the string representation of "administrative area" location type.
+     * This is the body of {@link #testToString()} method, but is also shared by other tests
+     * as a convenient way to verify that a {@code ModifiableLocationType} did not changed.
+     */
+    private static void verifyToString(final ModifiableLocationType area) {
         assertMultilinesEquals(
                 "administrative area………………… area of responsibility of highest
level local authority\n" +
                 "  ├─town……………………………………………… city
or town\n" +
@@ -197,14 +206,14 @@ public final strictfp class LocationType
                 "  │   └─street……………………………… thoroughfare
providing access to properties\n" +
                 "  │       └─property……………… land use\n" +
                 "  └─street………………………………………… thoroughfare
providing access to properties\n" +
-                "      └─property………………………… land use\n", create(true)[0].toString());
+                "      └─property………………………… land use\n", area.toString());
     }
 
     /**
      * Tests the equality and hash code value computation.
      */
     @Test
-    @DependsOnMethod("testToString")
+    @DependsOnMethod("testToString")                // Because in case of failure, JUnit
invokes toString().
     public void testEquals() {
         final ModifiableLocationType t1 = create(false)[0];
         final ModifiableLocationType t2 = create(true )[0];
@@ -222,4 +231,29 @@ public final strictfp class LocationType
     public void testSerialization() {
         assertSerializedEquals(ModifiableLocationType.snapshot(null, create(true)));
     }
+
+    /**
+     * Tests the safety against infinite recursivity.
+     * This method attempts to add "town" as a child of "street".
+     */
+    @Test
+    @DependsOnMethod("testToString")
+    public void testCheckForCycles() {
+        final ModifiableLocationType[] types  = create(true);
+        final ModifiableLocationType   town   = types[1];
+        final ModifiableLocationType   street = types[3];
+        try {
+            town.addParent(street);
+            fail("Shall not accept to add town as a child of street.");
+        } catch (IllegalArgumentException e) {
+            final String message = e.getMessage();
+            assertTrue(message, message.contains("street"));
+        }
+        /*
+         * Verify the string representation as a way to verify that parent addition
+         * has been properly rolled back. If not, we may have an infinite loop here
+         * until a StackOverflowError or OutOfMemoryError occurs.
+         */
+        verifyToString(types[0]);
+    }
 }



Mime
View raw message