Commits

Michael Ludwig committed 834ef13

Fix invalid isShared detection for object properties.

  • Participants
  • Parent commits 828face

Comments (0)

Files changed (3)

src/main/java/com/lhkbob/entreri/PropertySpecification.java

 /**
  *
  */
+@SuppressWarnings("unchecked")
 final class PropertySpecification implements Comparable<PropertySpecification> {
     private final String name;
     private final PropertyFactory<?> factory;
 
     private final Class<? extends Property> propertyType;
 
-    PropertySpecification(String name, PropertyFactory<?> factory, Method getter,
-                          Method setter, int setterParameter) {
+    private PropertySpecification(String name, PropertyFactory<?> factory, Method getter,
+                                  Method setter, int setterParameter) {
         this.name = name;
         this.factory = factory;
         this.getter = getter;
 
     private static void processSetter(Method m, Map<String, Method> setters,
                                       Map<String, Integer> parameters) {
+        Class<? extends Component> cType = (Class<? extends Component>) m
+                .getDeclaringClass();
         if (!m.getReturnType().equals(m.getDeclaringClass()) &&
             !m.getReturnType().equals(void.class)) {
-            throw new IllegalComponentDefinitionException(
-                    (Class<? extends Component>) m.getDeclaringClass(),
-                    "Setter method must have a return type equal to its declaring class, or return void: " +
-                    m.getName());
+            throw new IllegalComponentDefinitionException(cType,
+                                                          "Setter method must have a return type equal to its declaring class, or return void: " +
+                                                          m.getName());
         }
         if (m.getParameterTypes().length == 0) {
-            throw new IllegalComponentDefinitionException(
-                    (Class<? extends Component>) m.getDeclaringClass(),
-                    "Setter method must have at least 1 parameter: " + m.getName());
+            throw new IllegalComponentDefinitionException(cType,
+                                                          "Setter method must have at least 1 parameter: " +
+                                                          m.getName());
         }
 
         if (m.getParameterTypes().length == 1) {
             if (name != null) {
                 // verify absence of @Named on actual setter
                 if (m.getAnnotation(Named.class) != null) {
-                    throw new IllegalComponentDefinitionException(
-                            (Class<? extends Component>) m.getDeclaringClass(),
-                            "@Named cannot be on both parameter and method: " +
-                            m.getName());
+                    throw new IllegalComponentDefinitionException(cType,
+                                                                  "@Named cannot be on both parameter and method: " +
+                                                                  m.getName());
                 }
             } else {
                 name = getName(m, "set");
             }
 
             if (setters.containsKey(name)) {
-                throw new IllegalComponentDefinitionException(
-                        (Class<? extends Component>) m.getDeclaringClass(),
-                        "Property name collision: " + name);
+                throw new IllegalComponentDefinitionException(cType,
+                                                              "Property name collision: " +
+                                                              name);
             }
             setters.put(name, m);
             parameters.put(name, 0);
         } else {
             // verify absence of @Named on actual setter
             if (m.getAnnotation(Named.class) != null) {
-                throw new IllegalComponentDefinitionException(
-                        (Class<? extends Component>) m.getDeclaringClass(),
-                        "@Named cannot be applied to setter method with multiple parameters: " +
-                        m.getName());
+                throw new IllegalComponentDefinitionException(cType,
+                                                              "@Named cannot be applied to setter method with multiple parameters: " +
+                                                              m.getName());
             }
 
             int numP = m.getGenericParameterTypes().length;
             for (int i = 0; i < numP; i++) {
                 String name = getNameFromParameter(m, i);
                 if (name == null) {
-                    throw new IllegalComponentDefinitionException(
-                            (Class<? extends Component>) m.getDeclaringClass(),
-                            "@Named must be applied to each parameter for setter methods with multiple parameters: " +
-                            m.getName());
+                    throw new IllegalComponentDefinitionException(cType,
+                                                                  "@Named must be applied to each parameter for setter methods with multiple parameters: " +
+                                                                  m.getName());
                 }
 
                 if (setters.containsKey(name)) {
-                    throw new IllegalComponentDefinitionException(
-                            (Class<? extends Component>) m.getDeclaringClass(),
-                            "Property name collision: " + name);
+                    throw new IllegalComponentDefinitionException(cType,
+                                                                  "Property name collision: " +
+                                                                  name);
                 }
 
                 setters.put(name, m);
 
     private static void processGetter(Method m, String prefix,
                                       Map<String, Method> getters) {
+        Class<? extends Component> cType = (Class<? extends Component>) m
+                .getDeclaringClass();
         String name = getName(m, prefix);
         if (getters.containsKey(name)) {
-            throw new IllegalComponentDefinitionException(
-                    (Class<? extends Component>) m.getDeclaringClass(),
-                    "Property name collision: " + name);
+            throw new IllegalComponentDefinitionException(cType,
+                                                          "Property name collision: " +
+                                                          name);
         }
         if (m.getParameterTypes().length != 0) {
-            throw new IllegalComponentDefinitionException(
-                    (Class<? extends Component>) m.getDeclaringClass(),
-                    "Getter method cannot take arguments: " + m.getName());
+            throw new IllegalComponentDefinitionException(cType,
+                                                          "Getter method cannot take arguments: " +
+                                                          m.getName());
         }
         if (m.getReturnType().equals(void.class)) {
-            throw new IllegalComponentDefinitionException(
-                    (Class<? extends Component>) m.getDeclaringClass(),
-                    "Getter method must have a non-void return type: " + m.getName());
+            throw new IllegalComponentDefinitionException(cType,
+                                                          "Getter method must have a non-void return type: " +
+                                                          m.getName());
         }
 
         getters.put(name, m);
         }
 
         // verify contract of property
-        // FIXME we need to handle parameterized property types a bit better
         if (propertyType.equals(ObjectProperty.class)) {
-            return;
-        }
-
-        try {
-            Method g = propertyType.getMethod("get", int.class);
-            if (!g.getReturnType().equals(baseType)) {
+            // special case for ObjectProperty to support more permissive assignments
+            // (which to record requires a similar special case in the code generation)
+            if (isShared) {
                 throw new IllegalComponentDefinitionException(forType,
-                                                              "Does not implement required get() method for type " +
-                                                              baseType);
+                                                              "Property cannot be used with @SharedInstance: " +
+                                                              propertyType);
+            } else if (baseType.isPrimitive()) {
+                throw new IllegalComponentDefinitionException(forType,
+                                                              "ObjectProperty cannot be used with primitive types");
             }
-            // FIXME switch back to int, type method but then we have to update all the property defs
-            Method s = propertyType.getMethod("set", baseType, int.class);
-            if (!s.getReturnType().equals(void.class)) {
-                throw new IllegalComponentDefinitionException(forType,
-                                                              " does not implement required set() method for type " +
-                                                              baseType);
-            }
-        } catch (NoSuchMethodException e) {
-            throw new IllegalComponentDefinitionException(forType,
-                                                          " does not implement required get() or set() method for type " +
-                                                          baseType);
-        }
-
-        if (isShared) {
-            if (!ShareableProperty.class.isAssignableFrom(propertyType)) {
-                throw new IllegalComponentDefinitionException(
-                        (Class<? extends Component>) getter.getDeclaringClass(),
-                        "Property cannot be used with @SharedInstance: " + propertyType);
+            // else we know ObjectProperty is defined correctly because its part of the core library
+        } else {
+            try {
+                Method g = propertyType.getMethod("get", int.class);
+                if (!g.getReturnType().equals(baseType)) {
+                    throw new IllegalComponentDefinitionException(forType, "Property " +
+                                                                           propertyType +
+                                                                           " does not implement required get() method for type " +
+                                                                           baseType);
+                }
+                // FIXME switch back to int, type method but then we have to update all the property defs
+                Method s = propertyType.getMethod("set", baseType, int.class);
+                if (!s.getReturnType().equals(void.class)) {
+                    throw new IllegalComponentDefinitionException(forType, "Property " +
+                                                                           propertyType +
+                                                                           " does not implement required set() method for type " +
+                                                                           baseType);
+                }
+            } catch (NoSuchMethodException e) {
+                throw new IllegalComponentDefinitionException(forType, "Property " +
+                                                                       propertyType +
+                                                                       " does not implement required get() or set() method for type " +
+                                                                       baseType);
             }
 
-            // verify additional shareable property contract
-            try {
-                Method sg = propertyType.getMethod("get", int.class, baseType);
-                if (!sg.getReturnType().equals(void.class)) {
+            if (isShared) {
+                if (!ShareableProperty.class.isAssignableFrom(propertyType)) {
+                    throw new IllegalComponentDefinitionException(forType, "Property " +
+                                                                           propertyType +
+                                                                           " cannot be used with @SharedInstance: " +
+                                                                           propertyType);
+                }
+
+                // verify additional shareable property contract
+                try {
+                    Method sg = propertyType.getMethod("get", int.class, baseType);
+                    if (!sg.getReturnType().equals(void.class)) {
+                        throw new IllegalComponentDefinitionException(forType,
+                                                                      "Property " +
+                                                                      propertyType +
+                                                                      " does not implement shareable get() for " +
+                                                                      baseType);
+                    }
+                    Method creator = propertyType.getMethod("createShareableInstance");
+                    if (!creator.getReturnType().equals(baseType)) {
+                        throw new IllegalComponentDefinitionException(forType,
+                                                                      "Property " +
+                                                                      propertyType +
+                                                                      " does not implement createShareableInstance() for " +
+                                                                      baseType);
+                    }
+                } catch (NoSuchMethodException e) {
                     throw new RuntimeException(propertyType +
-                                               " does not implement required shared get() method for type " +
-                                               baseType);
+                                               " does not implement required shareable methods for type " +
+                                               baseType, e);
                 }
-            } catch (NoSuchMethodException e) {
-                throw new RuntimeException(propertyType +
-                                           " does not implement required shared get() method for type " +
-                                           baseType, e);
             }
         }
     }

src/test/java/com/lhkbob/entreri/PropertySpecificationTest.java

         doInvalidComponentDefinitionTest(MismatchedTypeComponent.class);
         doInvalidComponentDefinitionTest(NonBeanComponent.class);
         doInvalidComponentDefinitionTest(InvalidPropertyComponent.class);
+        doInvalidComponentDefinitionTest(IllegalSharedPropertyComponent.class);
+
+        // FIXME additional tests
+        // 1. invalid shared instance property on a primitive property type (or anything other than ObjectProperty)
+        // 2. properties that are missing a setter or getter
+        // 3. shareable properties that are missing the extra getter or the create method
     }
 
     @SuppressWarnings({ "rawtypes", "unchecked" })

src/test/java/com/lhkbob/entreri/components/IllegalSharedPropertyComponent.java

+package com.lhkbob.entreri.components;
+
+import com.lhkbob.entreri.Component;
+import com.lhkbob.entreri.SharedInstance;
+
+/**
+ * Invalid component definition that double checks that we fail when using a shared
+ * instance annotation with an incompatible type
+ */
+public interface IllegalSharedPropertyComponent extends Component {
+    @SharedInstance
+    public Object getValue();
+
+    public void setValue(Object v);
+}