Repository: jdbi/jdbi
Issue: vavr argument factory should check type rather than `instanceof`

I think this is wrong:
https://github.com/jdbi/jdbi/blob/master/vavr/src/main/java/org/jdbi/v3/vavr/VavrValueArgumentFactory.java#L41

This ends up giving the wrong type for `null` values.
Ref: https://groups.google.com/forum/#!topic/jdbi/piSOY5yB320


Changed files:
vavr/src/main/java/org/jdbi/v3/vavr/VavrValueArgumentFactory.java, vavr/src/test/java/org/jdbi/v3/vavr/TestVavrValueArgumentFactory.java, core/src/main/java/org/jdbi/v3/core/argument/NullArgument.java

diff --git a/core/src/main/java/org/jdbi/v3/core/argument/NullArgument.java b/core/src/main/java/org/jdbi/v3/core/argument/NullArgument.java
index 411e13ce4..50b6cd86a 100644
--- a/core/src/main/java/org/jdbi/v3/core/argument/NullArgument.java
+++ b/core/src/main/java/org/jdbi/v3/core/argument/NullArgument.java
@@ -49,6 +49,14 @@ public class NullArgument implements Argument {
         }
     }
 
+    /**
+     * @return the SQL type of the null
+     * @see java.sql.Types
+     */
+    public Integer getSqlType() {
+        return sqlType;
+    }
+
     @Override
     public String toString() {
         return "NULL";
diff --git a/vavr/src/main/java/org/jdbi/v3/vavr/VavrValueArgumentFactory.java b/vavr/src/main/java/org/jdbi/v3/vavr/VavrValueArgumentFactory.java
index addc95d4e..ab05234a5 100644
--- a/vavr/src/main/java/org/jdbi/v3/vavr/VavrValueArgumentFactory.java
+++ b/vavr/src/main/java/org/jdbi/v3/vavr/VavrValueArgumentFactory.java
@@ -14,7 +14,10 @@
 package org.jdbi.v3.vavr;
 
 import java.lang.reflect.Type;
+import java.util.Arrays;
+import java.util.HashSet;
 import java.util.Optional;
+import java.util.Set;
 
 import io.vavr.Lazy;
 import io.vavr.Value;
@@ -26,6 +29,7 @@ import org.jdbi.v3.core.argument.Argument;
 import org.jdbi.v3.core.argument.ArgumentFactory;
 import org.jdbi.v3.core.argument.Arguments;
 import org.jdbi.v3.core.config.ConfigRegistry;
+import org.jdbi.v3.core.generic.GenericTypes;
 
 import static org.jdbi.v3.core.generic.GenericTypes.findGenericParameter;
 
@@ -35,27 +39,26 @@ import static org.jdbi.v3.core.generic.GenericTypes.findGenericParameter;
  * if there is no such value (Try-Failed, Either-Left...) a "null" value will be applied as argument value
  */
 class VavrValueArgumentFactory implements ArgumentFactory {
+    private static final Set<Class<?>> VALUE_CLASSES = new HashSet<>(Arrays.asList(Option.class, Lazy.class, Try.class, Either.class, Validation.class));
 
     @Override
     public Optional<Argument> build(Type type, Object value, ConfigRegistry config) {
-        if (value instanceof Option || value instanceof Lazy || value instanceof Try || value instanceof Either || value instanceof Validation) {
+        Class<?> rawType = GenericTypes.getErasedType(type);
+
+        if (VALUE_CLASSES.stream().anyMatch(vc -> vc.isAssignableFrom(rawType))) {
             return buildValueArgument(type, config, (Value) value);
         }
 
         return Optional.empty();
     }
 
-    private Optional<Argument> buildValueArgument(Type type, ConfigRegistry config, Value<?> value) {
+    private static Optional<Argument> buildValueArgument(Type type, ConfigRegistry config, Value<?> value) {
         Type nestedType = findGenericParameter(type, Value.class).orElseGet(() -> extractTypeOfValue(value));
-        Object nestedValue = value.getOrNull();
-        return resolveNestedFromConfigured(config, nestedType, nestedValue);
-    }
-
-    Optional<Argument> resolveNestedFromConfigured(ConfigRegistry config, Type nestedType, Object nestedValue) {
+        Object nestedValue = value == null ? null : value.getOrNull();
         return config.get(Arguments.class).findFor(nestedType, nestedValue);
     }
 
-    private Type extractTypeOfValue(Value<?> value) {
+    private static Type extractTypeOfValue(Value<?> value) {
         Value<Class<?>> classOfValue = value.map(Object::getClass);
         return classOfValue.getOrElse(Object.class);
     }
diff --git a/vavr/src/test/java/org/jdbi/v3/vavr/TestVavrValueArgumentFactory.java b/vavr/src/test/java/org/jdbi/v3/vavr/TestVavrValueArgumentFactory.java
index c40292a1b..60062d12e 100644
--- a/vavr/src/test/java/org/jdbi/v3/vavr/TestVavrValueArgumentFactory.java
+++ b/vavr/src/test/java/org/jdbi/v3/vavr/TestVavrValueArgumentFactory.java
@@ -14,67 +14,60 @@
 package org.jdbi.v3.vavr;
 
 import java.lang.reflect.Type;
+import java.sql.Types;
 import java.util.Optional;
 
 import io.vavr.Lazy;
-import io.vavr.NotImplementedError;
 import io.vavr.control.Either;
 import io.vavr.control.Option;
 import io.vavr.control.Try;
 import io.vavr.control.Validation;
 import org.jdbi.v3.core.argument.Argument;
+import org.jdbi.v3.core.argument.NullArgument;
 import org.jdbi.v3.core.config.ConfigRegistry;
 import org.jdbi.v3.core.generic.GenericType;
-import org.junit.Before;
 import org.junit.Test;
 
 import static org.assertj.core.api.Assertions.assertThat;
 import static org.assertj.core.api.Assertions.assertThatThrownBy;
 
 public class TestVavrValueArgumentFactory {
+    private static final Type TRY_INTEGER = new GenericType<Try<Integer>>() {}.getType();
+    private static final Type OPTION_INTEGER = new GenericType<Option<Integer>>() {}.getType();
+    private static final Type LAZY_INTEGER = new GenericType<Lazy<Integer>>() {}.getType();
+    private static final Type LAZY_WILDCARD = new GenericType<Lazy<?>>() {}.getType();
+    private static final Type EITHER_STRING_INTEGER = new GenericType<Either<String, Integer>>() {}.getType();
+    private static final Type EITHER_WILDCARD = new GenericType<Either<?, ?>>() {}.getType();
+    private static final Type VALIDATION_STRING_INT = new GenericType<Validation<String, Integer>>() {}.getType();
 
-    private static final Argument MOCK_ARGUMENT = ((position, statement, ctx) -> new NotImplementedError());
+    private ConfigRegistry configRegistry = new ConfigRegistry();
 
-    private VavrValueArgumentFactory unit;
-
-    @Before
-    public void setUp() {
-        unit = new VavrValueArgumentFactory() {
-            @Override
-            Optional<Argument> resolveNestedFromConfigured(ConfigRegistry config, Type nestedType, Object nestedValue) {
-                return Optional.of(MOCK_ARGUMENT);
-            }
-        };
-    }
+    private VavrValueArgumentFactory unit = new VavrValueArgumentFactory();
 
     @Test
     public void testGetNonValueArgumentShouldNotBeEmpty() {
-        Optional<Argument> arg = unit.build(new GenericType<Option<Integer>>() {}.getType(),
-                Option.of(1), null);
+        Optional<Argument> arg = unit.build(OPTION_INTEGER, Option.of(1), configRegistry);
 
         assertThat(arg).isNotEmpty();
     }
 
     @Test
     public void testGetArgumentOfNoneShouldNotBeEmpty() {
-        Optional<Argument> arg = unit.build(new GenericType<Option<Integer>>() {}.getType(),
-                Option.none(), null);
+        Optional<Argument> arg = unit.build(OPTION_INTEGER, Option.none(), configRegistry);
 
         assertThat(arg).isNotEmpty();
     }
 
     @Test
     public void testGetLazyArgumentShouldNotBeEmpty() {
-        Optional<Argument> arg = unit.build(new GenericType<Lazy<Integer>>() {}.getType(),
-                Lazy.of(() -> 1), null);
+        Optional<Argument> arg = unit.build(LAZY_INTEGER, Lazy.of(() -> 1), configRegistry);
 
         assertThat(arg).isNotEmpty();
     }
 
     @Test
     public void testGetLazyArgumentInferredShouldNotBeEmpty() {
-        Optional<Argument> arg = unit.build(new GenericType<Lazy<?>>() {}.getType(),
-                Lazy.of(() -> 1), null);
+        Optional<Argument> arg = unit.build(LAZY_WILDCARD, Lazy.of(() -> 1), configRegistry);
 
         assertThat(arg).isNotEmpty();
     }
@@ -85,85 +78,75 @@ public class TestVavrValueArgumentFactory {
             throw new TestSpecificException();
         });
 
-        assertThatThrownBy(() -> unit.build(new GenericType<Lazy<Integer>>() {}.getType(),
-                badEvaluatingLazy, null))
+        assertThatThrownBy(() -> unit.build(LAZY_INTEGER, badEvaluatingLazy, configRegistry))
                 .isInstanceOf(TestSpecificException.class);
     }
 
     @Test
     public void testGetFailedTryArgumentShouldNotBeEmpty() {
-        Optional<Argument> arg = unit.build(new GenericType<Try<Integer>>() {}.getType(),
-                Try.failure(new TestSpecificException()), null);
+        Optional<Argument> arg = unit.build(TRY_INTEGER, Try.failure(new TestSpecificException()), configRegistry);
 
         assertThat(arg).isNotEmpty();
     }
 
     @Test
     public void testGetSuccessTryArgumentShouldNotBeEmpty() {
-        Optional<Argument> arg = unit.build(new GenericType<Try<Integer>>() {}.getType(),
-                Try.failure(new TestSpecificException()), null);
+        Optional<Argument> arg = unit.build(TRY_INTEGER, Try.failure(new TestSpecificException()), configRegistry);
 
         assertThat(arg).isNotEmpty();
     }
 
     @Test
     public void testGetLeftEitherArgumentShouldNotBeEmpty() {
-        Optional<Argument> arg = unit.build(new GenericType<Either<String, Integer>>() {}.getType(),
-                Either.left("error"), null);
+        Optional<Argument> arg = unit.build(EITHER_STRING_INTEGER, Either.left("error"), configRegistry);
 
         assertThat(arg).isNotEmpty();
     }
 
     @Test
     public void testGetRightEitherArgumentShouldNotBeEmpty() {
-        Optional<Argument> arg = unit.build(new GenericType<Either<String, Integer>>() {}.getType(),
-                Either.right(1), null);
+        Optional<Argument> arg = unit.build(EITHER_STRING_INTEGER, Either.right(1), configRegistry);
 
         assertThat(arg).isNotEmpty();
     }
 
     @Test
     public void testGetRightEitherArgumentInferredShouldNotBeEmpty() {
-        Optional<Argument> arg = unit.build(new GenericType<Either<?, ?>>() {}.getType(),
-                Either.right(1), null);
+        Optional<Argument> arg = unit.build(EITHER_WILDCARD, Either.right(1), configRegistry);
 
         assertThat(arg).isNotEmpty();
     }
 
     @Test
     public void testGetValidValidationArgumentShouldNotBeEmpty() {
-        Optional<Argument> arg =
-                unit.build(new GenericType<Validation<String, Integer>>() {}.getType(),
-                        Validation.valid(1), null);
+        Optional<Argument> arg = unit.build(VALIDATION_STRING_INT, Validation.valid(1), configRegistry);
 
         assertThat(arg).isNotEmpty();
     }
 
     @Test
     public void testGetInvalidValidationArgumentShouldNotBeEmpty() {
-        Optional<Argument> arg =
-                unit.build(new GenericType<Validation<String, Integer>>() {}.getType(),
-                        Validation.invalid("error"), null);
+        Optional<Argument> arg = unit.build(VALIDATION_STRING_INT, Validation.invalid("error"), configRegistry);
 
         assertThat(arg).isNotEmpty();
     }
 
     @Test
-    public void testGetArgumentNotPartOfFactoryShouldBeEmpty() {
-        Optional<Argument> arg = unit.build(new GenericType<Option<Integer>>() {}.getType(),
-                1, null);
+    public void testGetArgumentForNull() {
+        Optional<Argument> arg = unit.build(OPTION_INTEGER, null, configRegistry);
 
-        assertThat(arg).isEmpty();
+        assertThat(((NullArgument) arg.get()).getSqlType())
+            .isEqualTo(Types.INTEGER);
     }
 
     @Test
-    public void testGetArgumentNotPartOfFactory2ShouldBeEmpty() {
-        Optional<Argument> arg = unit.build(new GenericType<Integer>() {}.getType(),
-                null, null);
+    public void testGetArgumentNotPartOfFactoryShouldBeEmpty() {
+        Optional<Argument> arg = unit.build(new GenericType<Integer>() {}.getType(), null, configRegistry);
 
         assertThat(arg).isEmpty();
     }
 
-    private static class TestSpecificException extends RuntimeException {}
-
+    private static class TestSpecificException extends RuntimeException {
+        private static final long serialVersionUID = 1L;
+    }
 }