GH-846 - Fix determination of target graph property type.

* Fix determination of target graph property type.

The algorithm that determines the graph property type is faulty: It is not enough to retrieve the declared methods of a class and filter out the synthetic ones to get the most specific method implementation or overwrite.

The main goal of that code in FieldInfo is about getting the actual return type without trying to figure this out from generic parameters. This is done now in multiple steps:

If we find more than one but one none synthetic, we use this (same as before).
If all we finde are synthetic, we keep those declared on the actual converter class.
If those are more than one, we take the one not returning generic Object.

This fixes #845.

core/src/main/java/org/neo4j/ogm/metadata/FieldInfo.java

@@ -27,8 +27,10 @@
 import java.lang.reflect.Type;
 import java.security.AccessController;
 import java.security.PrivilegedAction;
+import java.util.HashSet;
 import java.util.Map;
 import java.util.Optional;
+import java.util.Set;
 import java.util.function.Predicate;
 
 import org.apache.commons.lang3.StringUtils;
@@ -79,6 +81,12 @@ public class FieldInfo {
      */
     private CompositeAttributeConverter<?> compositeConverter;
 
+    /**
+     * A computed, optional type in case this field has custom converters.
+     * It is lazily computed, depending on the converters that have been set.
+     */
+    private volatile Optional<Class<?>> convertedType;
+
     /**
      * Constructs a new {@link FieldInfo} based on the given arguments.
      *
@@ -308,24 +316,66 @@ public String getTypeDescriptor() {
         return typeParameterDescriptor;
     }
 
+    /**
+     * @return The type that is stored in the graph.
+     */
     public Class<?> convertedType() {
+
+        Optional<Class<?>> loadedConvertedType = this.convertedType;
+        if (loadedConvertedType == null) {
+            synchronized (this) {
+                loadedConvertedType = this.convertedType;
+                if (loadedConvertedType == null) {
+                    this.convertedType = computeConvertedType();
+                    loadedConvertedType = this.convertedType;
+                }
+            }
+        }
+        return loadedConvertedType.orElse(null);
+    }
+
+    private Optional<Class<?>> computeConvertedType() {
+
         if (hasPropertyConverter() || hasCompositeConverter()) {
             Class converterClass = hasPropertyConverter() ?
                 getPropertyConverter().getClass() : getCompositeConverter().getClass();
             String methodName = hasPropertyConverter() ? "toGraphProperty" : "toGraphProperties";
-
             try {
-                for (Method method : converterClass.getDeclaredMethods()) {
-                    //we don't want the method on the AttributeConverter interface
-                    if (method.getName().equals(methodName) && !method.isSynthetic()) {
-                        return method.getReturnType();
+                Set<Method> methodsFound = new HashSet<>();
+                for (Method method : converterClass.getMethods()) {
+                    if (method.getName().equals(methodName)) {
+                        methodsFound.add(method);
+                    }
+                }
+
+                // We found more than one method. Can be either one synthetic and one concrete method (interface + implementation),
+                // or concrete class and base implementation, or multiple synthetic methods (nested inheritance).
+                // If there is at least one none synthetic method, we pick this, otherwise we choose the one declared
+                // on the the concrete class.
+                if (methodsFound.size() > 1) {
+
+                    // if we find one none synthetic method, we take this
+                    for (Method m : methodsFound) {
+                        if (!m.isSynthetic()) {
+                            return Optional.of(m.getReturnType());
+                        }
+                    }
+
+                    // if they all are synthetic, we remove all methods declared on classes
+                    // not being the converter class
+                    methodsFound.removeIf(m -> m.getDeclaringClass() != converterClass);
+
+                    // If still more than one, remove the ones returning generic objects
+                    if (methodsFound.size() > 1) {
+                        methodsFound.removeIf(m -> m.getReturnType() == Object.class);
                     }
                 }
+                return methodsFound.stream().findFirst().map(Method::getReturnType);
             } catch (Exception e) {
                 throw new RuntimeException(e);
             }
         }
-        return null;
+        return Optional.empty();
     }
 
     /**
@@ -397,9 +447,9 @@ public void writeDirect(Object instance, Object value) {
      * @return The field type (may be List, while the mapped type is retrievable from {@link #getTypeDescriptor()} ()}).
      */
     public Class<?> type() {
-        Class convertedType = convertedType();
-        if (convertedType != null) {
-            return convertedType;
+        Class returnedTyped = convertedType();
+        if (returnedTyped != null) {
+            return returnedTyped;
         }
         return fieldType;
     }

neo4j-ogm-tests/neo4j-ogm-integration-tests/src/test/java/org/neo4j/ogm/domain/convertible/numbers/AbstractListConverter.java

@@ -0,0 +1,78 @@
+/*
+ * Copyright (c) 2002-2020 "Neo4j,"
+ * Neo4j Sweden AB [http://neo4j.com]
+ *
+ * This file is part of Neo4j.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.neo4j.ogm.domain.convertible.numbers;
+
+import java.util.Arrays;
+import java.util.List;
+import java.util.function.Function;
+import java.util.stream.Collectors;
+
+import org.neo4j.ogm.typeconversion.AttributeConverter;
+
+/**
+ * An abstract base class for converters converting list of things to a string.
+ *
+ * @param <T> The type of one thing
+ * @author Michael J. Simons
+ */
+abstract class AbstractListConverter<T> implements AttributeConverter<List<T>, String> {
+
+    private final Function<T, String> writeFunction;
+    private final Function<String, T> readFunction;
+
+    AbstractListConverter(Function<T, String> writeFunction,
+        Function<String, T> readFunction) {
+        this.writeFunction = writeFunction;
+        this.readFunction = readFunction;
+    }
+
+    @Override
+    public String toGraphProperty(List<T> value) {
+        return value == null ? null : value.stream().map(writeFunction).collect(Collectors.joining(","));
+    }
+
+    @Override public List<T> toEntityAttribute(String value) {
+        return value == null ? null : Arrays.stream(value.split(",")).map(readFunction).collect(Collectors.toList());
+    }
+
+    protected static abstract class AbstractIntegerListConverter extends AbstractListConverter<Integer> {
+
+        AbstractIntegerListConverter(int radix) {
+            super(
+                i -> Integer.toString(i, radix),
+                s -> Integer.valueOf(s, radix)
+            );
+        }
+    }
+
+    public static class Base36NumberConverter extends AbstractIntegerListConverter {
+
+        public Base36NumberConverter() {
+            super(36);
+        }
+    }
+
+    public static class FoobarListConverter extends AbstractListConverter<Foobar> {
+
+        public FoobarListConverter() {
+            super(Foobar::getValue, Foobar::new);
+        }
+    }
+}
+

neo4j-ogm-tests/neo4j-ogm-integration-tests/src/test/java/org/neo4j/ogm/domain/convertible/numbers/Account.java

@@ -22,12 +22,14 @@
 import java.math.BigInteger;
 import java.util.List;
 
+import org.neo4j.ogm.annotation.typeconversion.Convert;
 import org.neo4j.ogm.annotation.typeconversion.NumberString;
 
 /**
  * @author Vince Bickers
  * @author Luanne Misquitta
  * @author Gerrit Meier
+ * @author Michael J. Simons
  */
 public class Account {
 
@@ -42,6 +44,23 @@ public class Account {
     @NumberString(value = Integer.class, lenient = true)
     private Integer futureBalanceLenient;
 
+    @Convert(AbstractListConverter.Base36NumberConverter.class)
+    private List<Integer> valueA;
+
+    @Convert(HexadecimalNumberConverter.class)
+    private List<Integer> valueB;
+
+    @Convert(FoobarListConverter.class)
+    private List<Foobar> listOfFoobars;
+
+    @Convert(AbstractListConverter.FoobarListConverter.class)
+    private List<Foobar> anotherListOfFoobars;
+
+    private Integer notConverter;
+
+    @Convert(FoobarConverter.class)
+    private Foobar foobar;
+
     private List<BigInteger> loans;
     private short code;
     private Float limit;
@@ -121,4 +140,52 @@ public void setLimit(Float limit) {
     public Long getId() {
         return id;
     }
+
+    public List<Integer> getValueA() {
+        return valueA;
+    }
+
+    public void setValueA(List<Integer> valueA) {
+        this.valueA = valueA;
+    }
+
+    public List<Integer> getValueB() {
+        return valueB;
+    }
+
+    public void setValueB(List<Integer> valueB) {
+        this.valueB = valueB;
+    }
+
+    public List<Foobar> getListOfFoobars() {
+        return listOfFoobars;
+    }
+
+    public void setListOfFoobars(List<Foobar> listOfFoobars) {
+        this.listOfFoobars = listOfFoobars;
+    }
+
+    public Integer getNotConverter() {
+        return notConverter;
+    }
+
+    public void setNotConverter(Integer notConverter) {
+        this.notConverter = notConverter;
+    }
+
+    public Foobar getFoobar() {
+        return foobar;
+    }
+
+    public void setFoobar(Foobar foobar) {
+        this.foobar = foobar;
+    }
+
+    public List<Foobar> getAnotherListOfFoobars() {
+        return anotherListOfFoobars;
+    }
+
+    public void setAnotherListOfFoobars(List<Foobar> anotherListOfFoobars) {
+        this.anotherListOfFoobars = anotherListOfFoobars;
+    }
 }

neo4j-ogm-tests/neo4j-ogm-integration-tests/src/test/java/org/neo4j/ogm/domain/convertible/numbers/Foobar.java

@@ -0,0 +1,38 @@
+/*
+ * Copyright (c) 2002-2020 "Neo4j,"
+ * Neo4j Sweden AB [http://neo4j.com]
+ *
+ * This file is part of Neo4j.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.neo4j.ogm.domain.convertible.numbers;
+
+/**
+ * An arbitrary object that is not an entity and collections of it should not be treated as
+ * assocations as they are annoated with {@link org.neo4j.ogm.annotation.typeconversion.Convert @Convert}.
+ *
+ * @author Michael J. Simons
+ */
+public final class Foobar {
+
+    private final String value;
+
+    public Foobar(String value) {
+        this.value = value;
+    }
+
+    public String getValue() {
+        return value;
+    }
+}

neo4j-ogm-tests/neo4j-ogm-integration-tests/src/test/java/org/neo4j/ogm/domain/convertible/numbers/FoobarConverter.java

@@ -0,0 +1,37 @@
+/*
+ * Copyright (c) 2002-2020 "Neo4j,"
+ * Neo4j Sweden AB [http://neo4j.com]
+ *
+ * This file is part of Neo4j.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.neo4j.ogm.domain.convertible.numbers;
+
+import org.neo4j.ogm.typeconversion.AttributeConverter;
+
+/**
+ * @author Michael J. Simons
+ */
+public class FoobarConverter implements AttributeConverter<Foobar, String> {
+
+    @Override
+    public String toGraphProperty(Foobar value) {
+        return value == null ? null : value.getValue();
+    }
+
+    @Override
+    public Foobar toEntityAttribute(String value) {
+        return value == null ? null : new Foobar(value);
+    }
+}

neo4j-ogm-tests/neo4j-ogm-integration-tests/src/test/java/org/neo4j/ogm/domain/convertible/numbers/FoobarListConverter.java

@@ -0,0 +1,32 @@
+/*
+ * Copyright (c) 2002-2020 "Neo4j,"
+ * Neo4j Sweden AB [http://neo4j.com]
+ *
+ * This file is part of Neo4j.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.neo4j.ogm.domain.convertible.numbers;
+
+/**
+ * The way methods are synthesized is different with public inner classes
+ * (compare to {@link org.neo4j.ogm.domain.convertible.numbers.AbstractListConverter.FoobarListConverter}).
+ *
+ * @author Michael J. Simons
+ */
+public class FoobarListConverter extends AbstractListConverter<Foobar> {
+
+    public FoobarListConverter() {
+        super(Foobar::getValue, Foobar::new);
+    }
+}