sis-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From desruisse...@apache.org
Subject [sis] 01/03: `CompoundFormat.getFormat(Class)` should choose `Quantity` formatter before `Number` formatter.
Date Thu, 25 Jun 2020 14:23:22 GMT
This is an automated email from the ASF dual-hosted git repository.

desruisseaux pushed a commit to branch geoapi-4.0
in repository https://gitbox.apache.org/repos/asf/sis.git

commit e402a7439ab5c8b1d3562fbac2127043eaef6d79
Author: Martin Desruisseaux <martin.desruisseaux@geomatys.com>
AuthorDate: Thu Jun 25 11:57:48 2020 +0200

    `CompoundFormat.getFormat(Class)` should choose `Quantity` formatter before `Number` formatter.
---
 .../java/org/apache/sis/io/CompoundFormat.java     | 35 +++++++++++++++-------
 .../src/main/java/org/apache/sis/util/Classes.java | 29 +++++++++++++++---
 .../test/java/org/apache/sis/util/ClassesTest.java | 34 +++++++++++++++------
 3 files changed, 75 insertions(+), 23 deletions(-)

diff --git a/core/sis-utility/src/main/java/org/apache/sis/io/CompoundFormat.java b/core/sis-utility/src/main/java/org/apache/sis/io/CompoundFormat.java
index 78f919d..45e54eb 100644
--- a/core/sis-utility/src/main/java/org/apache/sis/io/CompoundFormat.java
+++ b/core/sis-utility/src/main/java/org/apache/sis/io/CompoundFormat.java
@@ -403,18 +403,33 @@ public abstract class CompoundFormat<T> extends Format implements
Localized {
         if (format == null && !formats.containsKey(valueType)) {
             format = createFormat(valueType);
             if (format == null) {
-                Class<?>   type = valueType;
-                Class<?>[] interfaces = null;
-                for (int i=-1;;) {
-                    if (i >= 0 || (type = type.getSuperclass()) == null) {      // Try
parent classes first.
-                        if (interfaces == null) {
-                            interfaces = Classes.getAllInterfaces(valueType);   // Try interfaces
after we tried all parent classes.
+                /*
+                 * We tried the given class directly. If it didn't worked, try the interfaces
before
+                 * to try the parent class. The reason is that we may have for example:
+                 *
+                 *     interface Length extends Quantity;                   // From JSR-363.
+                 *
+                 *     class MyLength extends Number implements Length;
+                 *
+                 * If we were looking for parent classes first, we would get a formatter
for Number.
+                 * But instead we want a formatter for Quantity, which is a (Number + Unit)
tuple.
+                 * Note that looking for directly declared interfaces first is not sufficient;
+                 * we need to look for parent of Length so we can find Quantity before Number.
+                 */
+                final Class<?>[] interfaces = Classes.getAllInterfaces(valueType);
+                Class<?> type = null;
+                for (int i=0; ; i++) {
+                    if (i < interfaces.length) {
+                        type = interfaces[i];               // Try interfaces first.
+                    } else {
+                        if (i == interfaces.length) {       // Try parent classes after we
tried all interfaces.
+                            type = valueType;
                         }
-                        if (++i >= interfaces.length) break;                    // No
format found - stop the search with format = null.
-                        type = interfaces[i];
+                        type = type.getSuperclass();
+                        if (type == null) break;            // No format found - stop the
search with format = null.
                     }
                     format = formats.get(type);
-                    if (format != null) break;                                  // Intentionally
no formats.containsKey(type) check here.
+                    if (format != null) break;              // Intentionally no formats.containsKey(type)
check here.
                     format = createFormat(type);
                     if (format != null) {
                         formats.put(type, format);
@@ -422,7 +437,7 @@ public abstract class CompoundFormat<T> extends Format implements
Localized {
                     }
                 }
             }
-            formats.put(valueType, format);                                     // Store
result even null.
+            formats.put(valueType, format);                 // Store result even null.
         }
         return format;
     }
diff --git a/core/sis-utility/src/main/java/org/apache/sis/util/Classes.java b/core/sis-utility/src/main/java/org/apache/sis/util/Classes.java
index 66d0163..4cfa397 100644
--- a/core/sis-utility/src/main/java/org/apache/sis/util/Classes.java
+++ b/core/sis-utility/src/main/java/org/apache/sis/util/Classes.java
@@ -53,7 +53,7 @@ import static org.apache.sis.internal.system.Modules.INTERNAL_CLASSNAME_PREFIX;
  * </ul>
  *
  * @author  Martin Desruisseaux (IRD, Geomatys)
- * @version 1.0
+ * @version 1.1
  * @since   0.3
  * @module
  */
@@ -313,6 +313,10 @@ public final class Classes extends Static {
      * the returned set will contain {@link java.util.List} (which is implemented directly)
      * together with its parent interfaces {@link Collection} and {@link Iterable}.
      *
+     * <h4>Elements ordering</h4>
+     * All interfaces implemented directly by the given type are first and in the order they
are declared
+     * in the {@code implements} or {@code extends} clause. Parent interfaces are next.
+     *
      * @param  <T>   the compile-time type of the {@code Class} argument.
      * @param  type  the class or interface for which to get all implemented interfaces.
      * @return all implemented interfaces (not including the given {@code type} if it was
an interface),
@@ -339,11 +343,13 @@ public final class Classes extends Static {
      *       while they can not cast {@code Set<Class<? super T>>} to {@code
Set<Class<?>>}.</li>
      * </ul>
      *
+     * See {@link #getInterfaceSet(Class, Set)} javadoc for a note about elements order.
+     *
      * @param  type  the class or interface for which to get all implemented interfaces.
      * @return all implemented interfaces (not including the given {@code type} if it was
an interface),
      *         or {@code null} if none. Callers can freely modify the returned set.
      */
-    static Set<Class<?>> getInterfaceSet(Class<?> type) {
+    private static Set<Class<?>> getInterfaceSet(Class<?> type) {
         Set<Class<?>> interfaces = null;
         while (type != null) {
             interfaces = getInterfaceSet(type, interfaces);
@@ -354,6 +360,16 @@ public final class Classes extends Static {
 
     /**
      * Adds to the given set every interfaces implemented by the given class or interface.
+     * This method invokes itself recursively for adding parent interfaces.
+     * The given type is <em>not</em> added to the set.
+     *
+     * <h4>Elements ordering</h4>
+     * All interfaces directly implemented by the given type are added first. Then parent
interfaces
+     * are added recursively. The goal is to increase the chances to have the most specific
types first.
+     * Example: suppose a class implementing two interfaces: {@code A extends C} and {@code
B extends C}.
+     * If the parents of A were added immediately after A, we would get {A, C, B} order.
+     * But if instead we add all directly implemented interfaces before to add parents,
+     * then we get {A, B, C} order, which is better.
      *
      * @param  type   the type for which to add the interfaces in the given set.
      * @param  addTo  the set where to add interfaces, or {@code null} if not yet created.
@@ -362,11 +378,16 @@ public final class Classes extends Static {
      */
     private static Set<Class<?>> getInterfaceSet(final Class<?> type, Set<Class<?>>
addTo) {
         final Class<?>[] interfaces = type.getInterfaces();
-        for (final Class<?> candidate : interfaces) {
+        for (int i=0; i<interfaces.length; i++) {
             if (addTo == null) {
                 addTo = new LinkedHashSet<>(hashMapCapacity(interfaces.length));
             }
-            if (addTo.add(candidate)) {
+            if (!addTo.add(interfaces[i])) {
+                interfaces[i] = null;           // Remember that this interface is already
present.
+            }
+        }
+        for (final Class<?> candidate : interfaces) {
+            if (candidate != null) {
                 getInterfaceSet(candidate, addTo);
             }
         }
diff --git a/core/sis-utility/src/test/java/org/apache/sis/util/ClassesTest.java b/core/sis-utility/src/test/java/org/apache/sis/util/ClassesTest.java
index 7c7c000..7eb8316 100644
--- a/core/sis-utility/src/test/java/org/apache/sis/util/ClassesTest.java
+++ b/core/sis-utility/src/test/java/org/apache/sis/util/ClassesTest.java
@@ -47,9 +47,13 @@ import java.awt.geom.Point2D;
 import org.opengis.util.InternationalString;
 import org.opengis.metadata.extent.Extent;
 import org.opengis.referencing.IdentifiedObject;
+import org.opengis.referencing.ReferenceSystem;
+import org.opengis.referencing.cs.EllipsoidalCS;
+import org.opengis.referencing.cs.CoordinateSystem;
+import org.opengis.referencing.crs.CoordinateReferenceSystem;
 import org.opengis.referencing.crs.SingleCRS;
+import org.opengis.referencing.crs.GeodeticCRS;
 import org.opengis.referencing.crs.GeographicCRS;
-import org.opengis.referencing.crs.CoordinateReferenceSystem;
 import org.opengis.referencing.operation.Transformation;
 import org.opengis.referencing.operation.CoordinateOperation;
 
@@ -58,7 +62,7 @@ import org.opengis.referencing.operation.CoordinateOperation;
  * Tests the {@link Classes} static methods.
  *
  * @author  Martin Desruisseaux (Geomatys)
- * @version 1.0
+ * @version 1.1
  * @since   0.3
  * @module
  */
@@ -90,14 +94,26 @@ public final strictfp class ClassesTest extends TestCase {
      * Tests {@link Classes#getAllInterfaces(Class)}.
      */
     @Test
+    @SuppressWarnings("")
     public void testGetAllInterfaces() {
-        final Set<Class<?>> interfaces = getInterfaceSet(ArrayList.class);
-        assertTrue(interfaces.contains(List        .class));
-        assertTrue(interfaces.contains(Collection  .class));
-        assertTrue(interfaces.contains(Iterable    .class));
-        assertTrue(interfaces.contains(RandomAccess.class));
-        assertTrue(interfaces.contains(Serializable.class));
-        assertTrue(interfaces.contains(Cloneable   .class));
+        assertArrayEquals(new Class[] {
+            GeographicCRS.class,
+            EllipsoidalCS.class,                // Shall be before parent types listed below.
+            GeodeticCRS.class,
+            SingleCRS.class,
+            CoordinateReferenceSystem.class,
+            ReferenceSystem.class,
+            IdentifiedObject.class,
+            CoordinateSystem.class
+        }, getAllInterfaces(MixedImpl.class));
+    }
+
+    /**
+     * A dummy class which implements two interfaces having a common parent.
+     * The intent is to verify that explicitly declared interfaces are listed
+     * before parent interfaces in {@link #testGetAllInterfaces()}.
+     */
+    private static abstract class MixedImpl implements GeographicCRS, EllipsoidalCS {
     }
 
     /**


Mime
View raw message