Make optionals with converters better supported

Mats-SX · Mats-SX · commit b13c445b1c9d · 2022-12-12T08:59:25.000+01:00
The generated @configuration code will now inspect the converter in/out types and do Optional wrapping/unwrapping automatically. This also includes the builder methods.
diff --git a/algo/src/main/java/org/neo4j/gds/embeddings/hashgnn/HashGNNConfig.java b/algo/src/main/java/org/neo4j/gds/embeddings/hashgnn/HashGNNConfig.java
@@ -51,16 +51,12 @@ default boolean heterogeneous() {
     }
 
     @Configuration.ToMapValue("org.neo4j.gds.embeddings.hashgnn.HashGNNConfig#toMapGenerateFeaturesConfig")
-    @Configuration.ConvertWith(method = "org.neo4j.gds.embeddings.hashgnn.HashGNNConfig#parseGenerateFeaturesConfig", inverse = Configuration.ConvertWith.INVERSE_IS_TO_MAP)
-    default Optional<GenerateFeaturesConfig> generateFeatures() {
-        return Optional.empty();
-    }
+    @Configuration.ConvertWith(method = "parseGenerateFeaturesConfig", inverse = Configuration.ConvertWith.INVERSE_IS_TO_MAP)
+    Optional<GenerateFeaturesConfig> generateFeatures();
 
     @Configuration.ToMapValue("org.neo4j.gds.embeddings.hashgnn.HashGNNConfig#toMapBinarizationConfig")
-    @Configuration.ConvertWith(method = "org.neo4j.gds.embeddings.hashgnn.HashGNNConfig#parseBinarizationConfig", inverse = Configuration.ConvertWith.INVERSE_IS_TO_MAP)
-    default Optional<BinarizeFeaturesConfig> binarizeFeatures() {
-        return Optional.empty();
-    }
+    @Configuration.ConvertWith(method = "parseBinarizationConfig", inverse = Configuration.ConvertWith.INVERSE_IS_TO_MAP)
+    Optional<BinarizeFeaturesConfig> binarizeFeatures();
 
     @Value.Check
     default void validate() {
@@ -73,28 +69,22 @@ default void validate() {
         }
     }
 
-    static Optional<BinarizeFeaturesConfig> parseBinarizationConfig(Object o) {
-        if (o instanceof Optional) {
-            return (Optional<BinarizeFeaturesConfig>) o;
-        }
-        var cypherMapWrapper = CypherMapWrapper.create((Map<String, Object>) o);
+    static BinarizeFeaturesConfig parseBinarizationConfig(Map<String, Object> parameter) {
+        var cypherMapWrapper = CypherMapWrapper.create(parameter);
         var binarizeFeaturesConfig = new BinarizeFeaturesConfigImpl(cypherMapWrapper);
         cypherMapWrapper.requireOnlyKeysFrom(binarizeFeaturesConfig.configKeys());
-        return Optional.of(binarizeFeaturesConfig);
+        return binarizeFeaturesConfig;
     }
 
     static Map<String, Object> toMapBinarizationConfig(BinarizeFeaturesConfig config) {
         return config.toMap();
     }
 
-    static Optional<GenerateFeaturesConfig> parseGenerateFeaturesConfig(Object o) {
-        if (o instanceof Optional) {
-            return (Optional<GenerateFeaturesConfig>) o;
-        }
-        var cypherMapWrapper = CypherMapWrapper.create((Map<String, Object>) o);
+    static GenerateFeaturesConfig parseGenerateFeaturesConfig(Map<String, Object> parameter) {
+        var cypherMapWrapper = CypherMapWrapper.create(parameter);
         var generateFeaturesConfig = new GenerateFeaturesConfigImpl(cypherMapWrapper);
         cypherMapWrapper.requireOnlyKeysFrom(generateFeaturesConfig.configKeys());
-        return Optional.of(generateFeaturesConfig);
+        return generateFeaturesConfig;
     }
 
     static Map<String, Object> toMapGenerateFeaturesConfig(GenerateFeaturesConfig config) {
diff --git a/config-api/src/main/java/org/neo4j/gds/config/BaseConfig.java b/config-api/src/main/java/org/neo4j/gds/config/BaseConfig.java
@@ -60,7 +60,7 @@ default Map<String, Object> toMap() {
         return new HashMap<>();
     }
 
-    static Optional<String> trim(Optional<String> input) {
-        return input.map(String::trim).filter(s -> !s.isEmpty());
+    static String trim(String input) {
+        return input.trim();
     }
 }
diff --git a/config-generator/src/main/java/org/neo4j/gds/proc/GenerateConfiguration.java b/config-generator/src/main/java/org/neo4j/gds/proc/GenerateConfiguration.java
@@ -44,7 +44,6 @@
 import javax.lang.model.element.Modifier;
 import javax.lang.model.element.PackageElement;
 import javax.lang.model.element.TypeElement;
-import javax.lang.model.element.VariableElement;
 import javax.lang.model.type.DeclaredType;
 import javax.lang.model.type.TypeMirror;
 import javax.lang.model.util.Elements;
@@ -454,7 +453,7 @@ private Optional<MemberDefinition> memberDefinition(NameAllocator names, ConfigP
         TypeMirror targetType = method.getReturnType();
         ConvertWith convertWith = method.getAnnotation(ConvertWith.class);
         if (convertWith == null) {
-            return memberDefinition(names, member, targetType);
+            return memberDefinition(names, member, targetType, Optional.empty());
         }
 
         String converter = convertWith.method().trim();
@@ -555,9 +554,22 @@ private Optional<MemberDefinition> memberDefinition(
 
             if (validCandidates.size() == 1) {
                 ExecutableElement candidate = validCandidates.get(0);
-                VariableElement parameter = candidate.getParameters().get(0);
                 TypeMirror currentType = currentClass.asType();
-                return memberDefinition(names, member, parameter.asType())
+                TypeMirror converterInputType = candidate.getParameters().get(0).asType();
+                if (isTypeOf(Optional.class, targetType)) {
+                    return memberDefinition(names, member, targetType, Optional.of(converterInputType))
+                        .map(d -> ImmutableMemberDefinition.builder()
+                            .from(d)
+                            .addConverter(c -> CodeBlock.of(
+                                "$L.map($T::$N)",
+                                c,
+                                currentType,
+                                candidate.getSimpleName().toString()
+                            ))
+                            .build()
+                        );
+                }
+                return memberDefinition(names, member, converterInputType, Optional.empty())
                     .map(d -> ImmutableMemberDefinition.builder()
                         .from(d)
                         .addConverter(c -> CodeBlock.of(
@@ -612,6 +624,9 @@ private void validateCandidateModifiers(
         if (!(candidate.getParameters().size() == 1)) {
             invalidCandidates.add(InvalidCandidate.of(candidate, "May only accept one parameter"));
         }
+        if (isTypeOf(Optional.class, targetType)) {
+            targetType = ((DeclaredType) targetType).getTypeArguments().get(0);
+        }
         if (!typeUtils.isAssignable(candidate.getReturnType(), targetType)) {
             invalidCandidates.add(InvalidCandidate.of(
                 candidate,
@@ -624,7 +639,8 @@ private void validateCandidateModifiers(
     private Optional<MemberDefinition> memberDefinition(
         NameAllocator names,
         ConfigParser.Member member,
-        TypeMirror targetType
+        TypeMirror targetType,
+        Optional<TypeMirror> converterInputType
     ) {
         ImmutableMemberDefinition.Builder builder = ImmutableMemberDefinition
             .builder()
@@ -680,7 +696,16 @@ private Optional<MemberDefinition> memberDefinition(
                         }
                     }
 
-                    if (maybeInnerType.isPresent()) {
+                    if (converterInputType.isPresent()) {
+                        TypeMirror expectedType = typeUtils.erasure(converterInputType.get());
+                        var x = typeUtils.getDeclaredType(elementUtils.getTypeElement("java.util.Optional"), expectedType);
+                        builder
+                            .methodPrefix("get")
+                            .methodName("Optional")
+                            .expectedType(CodeBlock.of("$T.class", expectedType))
+                            .expectedTypeRaw(expectedType)
+                            .expectedTypeRawWrappedInOptional(x);
+                    } else if (maybeInnerType.isPresent()) {
                         builder
                             .methodPrefix("get")
                             .methodName("Optional")
@@ -760,6 +785,10 @@ interface MemberDefinition {
 
         Optional<CodeBlock> expectedType();
 
+        Optional<TypeMirror> expectedTypeRaw();
+
+        Optional<TypeMirror> expectedTypeRawWrappedInOptional();
+
         List<UnaryOperator<CodeBlock>> converters();
     }
 
diff --git a/config-generator/src/main/java/org/neo4j/gds/proc/GenerateConfigurationBuilder.java b/config-generator/src/main/java/org/neo4j/gds/proc/GenerateConfigurationBuilder.java
@@ -212,10 +212,11 @@ private static List<MethodSpec> defineConfigMapEntrySetters(
             .flatMap(implMember -> {
                 var setterMethods = Stream.<MethodSpec>builder();
                 String configKeyName = implMember.member().methodName();
+                TypeMirror parameterType = implMember.parameterType();
 
                 MethodSpec.Builder setMethodBuilder = MethodSpec.methodBuilder(configKeyName)
                     .addModifiers(Modifier.PUBLIC)
-                    .addParameter(unpackedType(implMember.parameterType()), configKeyName)
+                    .addParameter(unpackedType(implMember.expectedTypeRaw().orElse(parameterType)), configKeyName)
                     .returns(builderClassName)
                     .addCode(CodeBlock.builder()
                         .addStatement(
@@ -230,12 +231,12 @@ private static List<MethodSpec> defineConfigMapEntrySetters(
 
                 setterMethods.add(setMethodBuilder.build());
 
-                if (isTypeOf(Optional.class, implMember.parameterType())) {
+                if (isTypeOf(Optional.class, parameterType)) {
                     String lambdaVarName = "actual" + configKeyName;
 
                     var optionalSetterBuilder = MethodSpec.methodBuilder(configKeyName)
                         .addModifiers(Modifier.PUBLIC)
-                        .addParameter(TypeName.get(implMember.parameterType()), configKeyName)
+                        .addParameter(TypeName.get(implMember.expectedTypeRawWrappedInOptional().orElse(parameterType)), configKeyName)
                         .returns(builderClassName)
                         .addStatement(
                             "$1N.ifPresent($2N -> this.$3N.put(\"$4L\", $2N))",
diff --git a/config-generator/src/test/resources/expected/Conversions.java b/config-generator/src/test/resources/expected/Conversions.java
@@ -22,6 +22,7 @@
 import java.util.ArrayList;
 import java.util.HashMap;
 import java.util.Map;
+import java.util.Optional;
 import java.util.stream.Collectors;
 import javax.annotation.processing.Generated;
 import org.jetbrains.annotations.NotNull;
@@ -38,6 +39,8 @@ public final class ConversionsConfig implements Conversions.MyConversion {
 
     private String referenceTypeAsResult;
 
+    private Optional<Conversions.Foo> optional;
+
     public ConversionsConfig(@NotNull CypherMapAccess config) {
         ArrayList<IllegalArgumentException> errors = new ArrayList<>();
         try {
@@ -63,6 +66,11 @@ public ConversionsConfig(@NotNull CypherMapAccess config) {
         } catch (IllegalArgumentException e) {
             errors.add(e);
         }
+        try {
+            this.optional = CypherMapAccess.failOnNull("optional", config.getOptional("optional", Map.class).map(Conversions.MyConversion::toFoo));
+        } catch (IllegalArgumentException e) {
+            errors.add(e);
+        }
         if (!errors.isEmpty()) {
             if (errors.size() == 1) {
                 throw errors.get(0);
@@ -101,6 +109,11 @@ public String referenceTypeAsResult() {
         return this.referenceTypeAsResult;
     }
 
+    @Override
+    public Optional<Conversions.Foo> optional() {
+        return this.optional;
+    }
+
     public static ConversionsConfig.Builder builder() {
         return new ConversionsConfig.Builder();
     }
@@ -118,6 +131,7 @@ public static ConversionsConfig.Builder from(Conversions.MyConversion baseConfig
             builder.inheritedMethod(String.valueOf(baseConfig.inheritedMethod()));
             builder.qualifiedMethod(String.valueOf(baseConfig.qualifiedMethod()));
             builder.referenceTypeAsResult(positive.Conversions.MyConversion.remove42(baseConfig.referenceTypeAsResult()));
+            builder.optional(baseConfig.optional().map(v -> positive.Conversions.MyConversion.fromFoo(v)));
             return builder;
         }
 
@@ -141,6 +155,16 @@ public ConversionsConfig.Builder referenceTypeAsResult(String referenceTypeAsRes
             return this;
         }
 
+        public ConversionsConfig.Builder optional(Map optional) {
+            this.config.put("optional", optional));
+            return this;
+        }
+
+        public ConversionsConfig.Builder optional(Optional<Map> optional) {
+            optional.ifPresent(actualoptional -> this.config.put("optional", actualoptional));
+            return this;
+        }
+
         public Conversions.MyConversion build() {
             CypherMapWrapper config = CypherMapWrapper.create(this.config);
             return new ConversionsConfig(config);
diff --git a/config-generator/src/test/resources/positive/Conversions.java b/config-generator/src/test/resources/positive/Conversions.java
@@ -21,6 +21,8 @@
 
 import org.neo4j.gds.annotation.Configuration;
 
+import java.util.Map;
+import java.util.Optional;
 
 public interface Conversions {
 
@@ -51,6 +53,9 @@ interface MyConversion extends BaseConversion {
         @Configuration.ConvertWith(method = "add42", inverse = "positive.Conversions.MyConversion#remove42")
         String referenceTypeAsResult();
 
+        @Configuration.ConvertWith(method = "toFoo", inverse = "positive.Conversions.MyConversion#fromFoo")
+        Optional<Foo> optional();
+
         static int toInt(String input) {
             return Integer.parseInt(input);
         }
@@ -62,5 +67,14 @@ static String add42(String input) {
         static String remove42(String input) {
             return input.substring(0, input.length() - 2);
         }
+
+        static Foo toFoo(Map<String, Object> parameter) {
+            return new Foo() {};
+        }
+        static Map<String, Object> fromFoo(Foo f) {
+            return Map.of();
+        }
     }
+
+    interface Foo {}
 }

Original file line number	Diff line number	Diff line change
`@@ -60,7 +60,7 @@ default Map<String, Object> toMap() {`
`60`	`60`	`return new HashMap<>();`
`61`	`61`	`}`
`62`	`62`
`63`		`- static Optional<String> trim(Optional<String> input) {`
`64`		`- return input.map(String::trim).filter(s -> !s.isEmpty());`
	`63`	`+ static String trim(String input) {`
	`64`	`+ return input.trim();`
`65`	`65`	`}`
`66`	`66`	`}`