Fix bugs, improve safety, further decouple modules, add tests

Author: agrgr

.editorconfig

@@ -0,0 +1,19 @@
+# top-most EditorConfig file
+root = true
+
+# Unix-style newlines with a newline ending every file
+[*]
+charset = utf-8
+end_of_line = lf
+
+[*.java]
+indent_style = space
+indent_size = 4
+insert_final_newline = true
+max_line_length = 120
+ij_java_wrap_long_lines = true
+ij_java_wrap_comments = true
+ij_java_method_call_chain_wrap = normal
+ij_java_blank_lines_after_class_header = 1
+ij_java_class_count_to_use_import_on_demand = 10
+ij_java_names_count_to_use_import_on_demand = 10

core/src/main/java/com/aerospike/mapper/tools/ClassCache.java

@@ -8,16 +8,15 @@
 import lombok.Getter;
 
 import javax.validation.constraints.NotNull;
-import java.util.HashMap;
-import java.util.Map;
+import java.util.concurrent.ConcurrentHashMap;
 
 public class ClassCache {
 
     @Getter
     private static final ClassCache instance = new ClassCache();
-    private final Map<Class<?>, ClassCacheEntry<?>> cacheMap = new HashMap<>();
-    private final Map<String, ClassConfig> classesConfig = new HashMap<>();
-    private final Map<String, ClassCacheEntry<?>> storedNameToCacheEntry = new HashMap<>();
+    private final ConcurrentHashMap<Class<?>, ClassCacheEntry<?>> cacheMap = new ConcurrentHashMap<>();
+    private final ConcurrentHashMap<String, ClassConfig> classesConfig = new ConcurrentHashMap<>();
+    private final ConcurrentHashMap<String, ClassCacheEntry<?>> storedNameToCacheEntry = new ConcurrentHashMap<>();
     private final Object lock = new Object();
 
     private ClassCache() {
@@ -29,7 +28,6 @@ public <T> ClassCacheEntry<T> loadClass(@NotNull Class<T> clazz, IObjectMapper m
 
     @SuppressWarnings("unchecked")
     public <T> ClassCacheEntry<T> loadClass(@NotNull Class<T> clazz, IObjectMapper mapper, boolean requireRecord) {
-        // Clazz can be null if an interface is passed
         if (clazz == null || clazz.isPrimitive() || clazz.equals(Object.class) || clazz.equals(String.class)
                 || clazz.equals(Character.class) || Number.class.isAssignableFrom(clazz)) {
             return null;
@@ -41,14 +39,6 @@ public <T> ClassCacheEntry<T> loadClass(@NotNull Class<T> clazz, IObjectMapper m
                 entry = (ClassCacheEntry<T>) cacheMap.get(clazz);
                 if (entry == null) {
                     try {
-                        // Construct a class cache entry. This must be done in 2 steps, one creating the entry
-                        // and the other finalizing construction of it.
-                        // This is to cater for classes which recursively refer to themselves, such as
-                        // 	public static class A {
-                        //      @AerospikeKey
-                        //      public int id;
-                        //      public A a;
-                        //  }
                         entry = new ClassCacheEntry<>(clazz, mapper, getClassConfig(clazz), requireRecord);
                     } catch (NotAnnotatedClass nae) {
                         return null;
@@ -69,7 +59,6 @@ public <T> ClassCacheEntry<T> loadClass(@NotNull Class<T> clazz, IObjectMapper m
         return entry;
     }
 
-    // package visibility
     void setStoredName(@NotNull ClassCacheEntry<?> entry, @NotNull String name) {
         ClassCacheEntry<?> existingEntry = storedNameToCacheEntry.get(name);
         if (existingEntry != null && !(existingEntry.equals(entry))) {
@@ -89,14 +78,13 @@ public boolean hasClass(Class<?> clazz) {
         return cacheMap.containsKey(clazz);
     }
 
-    /**
-     * This method is typically only used for testing
-     */
     public void clear() {
-        this.cacheMap.clear();
-        this.classesConfig.clear();
-        TypeUtils.clear();
-        this.storedNameToCacheEntry.clear();
+        synchronized (lock) {
+            this.cacheMap.clear();
+            this.classesConfig.clear();
+            TypeUtils.clear();
+            this.storedNameToCacheEntry.clear();
+        }
     }
 
     public void addConfiguration(@NotNull Configuration configuration) {
@@ -105,6 +93,7 @@ public void addConfiguration(@NotNull Configuration configuration) {
         }
     }
 
+    @SuppressWarnings("unused")
     public ClassConfig getClassConfig(String className) {
         return classesConfig.get(className);
     }
@@ -113,19 +102,12 @@ public ClassConfig getClassConfig(Class<?> clazz) {
         return classesConfig.get(clazz.getName());
     }
 
+    @SuppressWarnings("unused")
     public boolean hasClassConfig(String className) {
         return classesConfig.containsKey(className);
     }
 
     public boolean hasClassConfig(Class<?> clazz) {
         return classesConfig.containsKey(clazz.getName());
     }
-
-    public enum PolicyType {
-        READ,
-        WRITE,
-        BATCH,
-        SCAN,
-        QUERY
-    }
 }

core/src/main/java/com/aerospike/mapper/tools/ClassCacheEntry.java

@@ -113,7 +113,7 @@ private enum FactoryMethodType {
         this.config = config;
     }
 
-    public ClassCacheEntry<T> construct() {
+    public void construct() {
         if (config != null) {
             config.validate();
             this.overrideSettings(config);
@@ -146,7 +146,6 @@ public ClassCacheEntry<T> construct() {
 
         this.checkRecordSettingsAgainstSuperClasses();
         constructed = true;
-        return this;
     }
 
     public boolean isNotConstructed() {
@@ -378,7 +377,7 @@ private Method findConstructorFactoryMethod() {
                         String.format("Missing factoryClass definition when factoryMethod is specified on class %s",
                                 clazz.getSimpleName()));
             }
-            if (StringUtils.isBlank(this.factoryClass)) {
+            if (StringUtils.isBlank(this.factoryMethod)) {
                 throw new AerospikeMapperException(
                         String.format("Missing factoryMethod definition when factoryClass is specified on class %s",
                                 clazz.getSimpleName()));
@@ -806,21 +805,25 @@ private void mapField(Field thisField, BinConfig thisBin, boolean isKey) {
 
     private Method findMethodWithNameAndParams(String name, Class<?>... params) {
         try {
-            Method method = this.clazz.getDeclaredMethod(name, params);
             // TODO: Should this ascend the inheritance hierarchy using getMethod on superclasses?
-            return method;
+            return this.clazz.getDeclaredMethod(name, params);
         } catch (NoSuchMethodException nsme) {
             return null;
         }
     }
 
-    private Method findMethodWithNameAndParamByTypeName(String name, Class<?> firstParam, String secondParamTypeName) {
+    /**
+     * Searches for a 2-param setter whose second parameter is a recognized client Key or Value type, as determined by
+     * the mapper's {@link SetterParamTypeResolver}.
+     */
+    private Method findSetterWithClientParamType(String setterName, Class<?> firstParam) {
+        SetterParamTypeResolver resolver = this.mapper.getSetterParamTypeResolver();
         for (Method m : this.clazz.getDeclaredMethods()) {
-            if (m.getName().equals(name)) {
+            if (m.getName().equals(setterName)) {
                 Class<?>[] paramTypes = m.getParameterTypes();
                 if (paramTypes.length == 2
                         && paramTypes[0].isAssignableFrom(firstParam)
-                        && secondParamTypeName.equals(paramTypes[1].getName())) {
+                    && resolver.resolve(paramTypes[1].getName()) != PropertyDefinition.SetterParamType.NONE) {
                     return m;
                 }
             }
@@ -844,10 +847,7 @@ private void validateAccessorsForField(String binName, Field thisField) {
 
         Method setter = findMethodWithNameAndParams(setterName, thisField.getType());
         if (setter == null) {
-            setter = findMethodWithNameAndParamByTypeName(setterName, thisField.getType(), "com.aerospike.client.Key");
-        }
-        if (setter == null) {
-            setter = findMethodWithNameAndParamByTypeName(setterName, thisField.getType(), "com.aerospike.client.Value");
+            setter = findSetterWithClientParamType(setterName, thisField.getType());
         }
         if (setter == null) {
             throw new AerospikeMapperException(String.format(
@@ -917,11 +917,6 @@ public Integer getTtl() {
         return ttl;
     }
 
-    /** Returns whether the key field is stored as a regular bin (true) or only in key metadata (false). */
-    public boolean isKeyStoredAsBin() {
-        return keyAsBin;
-    }
-
     /**
      * Traverses the class hierarchy to find the field name of the key (may be in a superclass).
      * Returns null if no key field is found.
@@ -976,21 +971,6 @@ public Integer getGenerationValue(Object instance) {
         }
     }
 
-    private boolean contains(String[] names, String thisName) {
-        if (names == null || names.length == 0) {
-            return true;
-        }
-        if (thisName == null) {
-            return false;
-        }
-        for (String aName : names) {
-            if (thisName.equals(aName)) {
-                return true;
-            }
-        }
-        return false;
-    }
-
     public Map<String, Object> getMap(Object instance, boolean needsType) {
         try {
             Map<String, Object> results = new HashMap<>();
@@ -1023,8 +1003,8 @@ private void addDataFromValueName(String name, Object instance, ClassCacheEntry<
         }
     }
 
-    private boolean isKeyField(String name) {
-        return keyName != null && keyName.equals(name);
+    private boolean isNotKeyField(String name) {
+        return keyName == null || !keyName.equals(name);
     }
 
     public List<Object> getList(Object instance, boolean skipKey, boolean needsType) {
@@ -1039,14 +1019,14 @@ public List<Object> getList(Object instance, boolean skipKey, boolean needsType)
                 if (thisClass.ordinals != null) {
                     for (int i = 1; i <= thisClass.ordinals.size(); i++) {
                         String name = thisClass.ordinals.get(i);
-                        if (!skipKey || !isKeyField(name)) {
+                        if (!skipKey || isNotKeyField(name)) {
                             addDataFromValueName(name, instance, thisClass, results);
                         }
                     }
                 }
                 for (String name : thisClass.values.keySet()) {
                     if (thisClass.fieldsWithOrdinals == null || !thisClass.fieldsWithOrdinals.contains(name)) {
-                        if (!skipKey || !isKeyField(name)) {
+                        if (!skipKey || isNotKeyField(name)) {
                             addDataFromValueName(name, instance, thisClass, results);
                         }
                     }
@@ -1237,15 +1217,15 @@ public T constructAndHydrate(List<Object> list, boolean skipKey) {
                     if (thisClass.ordinals != null) {
                         for (int i = 1; i <= thisClass.ordinals.size(); i++) {
                             String name = thisClass.ordinals.get(i);
-                            if (!skipKey || !isKeyField(name)) {
+                            if (!skipKey || isNotKeyField(name)) {
                                 index = thisClass.setValueByField(name, objectVersion, recordVersion, null, index, list,
                                         valueMap);
                             }
                         }
                     }
                     for (String name : thisClass.values.keySet()) {
                         if (thisClass.fieldsWithOrdinals == null || !thisClass.fieldsWithOrdinals.contains(name)) {
-                            if (!skipKey || !isKeyField(name)) {
+                            if (!skipKey || isNotKeyField(name)) {
                                 index = thisClass.setValueByField(name, objectVersion, recordVersion, null, index, list,
                                         valueMap);
                             }
@@ -1286,15 +1266,15 @@ public void hydrateFromList(List<Object> list, Object instance, boolean skipKey)
                     if (ordinals != null) {
                         for (int i = 1; i <= ordinals.size(); i++) {
                             String name = ordinals.get(i);
-                            if (!skipKey || !isKeyField(name)) {
+                            if (!skipKey || isNotKeyField(name)) {
                                 index = setValueByField(name, objectVersion, recordVersion, instance, index, list,
                                         null);
                             }
                         }
                     }
                     for (String name : this.values.keySet()) {
                         if (this.fieldsWithOrdinals == null || !thisClass.fieldsWithOrdinals.contains(name)) {
-                            if (!skipKey || !isKeyField(name)) {
+                            if (!skipKey || isNotKeyField(name)) {
                                 index = setValueByField(name, objectVersion, recordVersion, instance, index, list,
                                         null);
                             }

core/src/main/java/com/aerospike/mapper/tools/IObjectMapper.java

@@ -8,4 +8,12 @@ public interface IObjectMapper {
     IRecordConverter getMappingConverter();
 
     RecordLoader getRecordLoader();
+
+    /**
+     * Returns a resolver that maps setter parameter type names to {@link PropertyDefinition.SetterParamType} values for
+     * the client library used by this mapper.
+     */
+    default SetterParamTypeResolver getSetterParamTypeResolver() {
+        return SetterParamTypeResolver.DEFAULT;
+    }
 }

core/src/main/java/com/aerospike/mapper/tools/PropertyDefinition.java

@@ -11,7 +11,6 @@
 import java.lang.annotation.Annotation;
 import java.lang.reflect.Method;
 import java.lang.reflect.Parameter;
-import java.lang.reflect.Type;
 
 public class PropertyDefinition {
 
@@ -50,10 +49,6 @@ public Annotation[] getAnnotations() {
         return getter != null ? getter.getAnnotations() : setter.getAnnotations();
     }
 
-    public Type getGenericType() {
-        return this.getter.getGenericReturnType();
-    }
-
     /**
      * Validate that this is a valid property
      */
@@ -79,11 +74,9 @@ public void validate(String className, ClassConfig config, boolean allowNoSetter
             if (setter.getParameterCount() == 2) {
                 Parameter param = setter.getParameters()[1];
                 String paramTypeName = param.getType().getName();
-                if ("com.aerospike.client.Key".equals(paramTypeName)) {
-                    this.setterParamType = SetterParamType.KEY;
-                } else if ("com.aerospike.client.Value".equals(paramTypeName)) {
-                    this.setterParamType = SetterParamType.VALUE;
-                } else {
+                SetterParamTypeResolver resolver = mapper.getSetterParamTypeResolver();
+                this.setterParamType = resolver.resolve(paramTypeName);
+                if (this.setterParamType == SetterParamType.NONE) {
                     throw new AerospikeMapperException(String.format("Property %s on class %s has a setter with 2 arguments," +
                             " but the second one is neither a Key nor a Value", this.name, className));
                 }

core/src/main/java/com/aerospike/mapper/tools/RecordMetaConfig.java

@@ -0,0 +1,25 @@
+package com.aerospike.mapper.tools;
+
+import com.aerospike.mapper.tools.PropertyDefinition.SetterParamType;
+
+/**
+ * Resolves the {@link SetterParamType} for a setter's second parameter based on the parameter's fully-qualified type
+ * name. This decouples core from specific client libraries.
+ */
+@FunctionalInterface
+public interface SetterParamTypeResolver {
+
+    /**
+     * Determines if the given parameter type name corresponds to a Key or Value type in the client library used by this
+     * mapper.
+     *
+     * @param paramTypeName fully-qualified class name of the setter's second parameter
+     * @return the resolved {@link SetterParamType}, or {@link SetterParamType#NONE} if unrecognized
+     */
+    SetterParamType resolve(String paramTypeName);
+
+    /**
+     * Default resolver that does not recognize any client-specific parameter types.
+     */
+    SetterParamTypeResolver DEFAULT = paramTypeName -> SetterParamType.NONE;
+}