From 363dc9975318b210cf35e62fced3e33903ce8877 Mon Sep 17 00:00:00 2001 From: Mark Paluch Date: Wed, 18 Oct 2017 13:48:15 +0200 Subject: [PATCH 1/4] DATACMNS-1200 - Prepare issue branch. --- pom.xml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pom.xml b/pom.xml index 2473b93f59..c06ded7c9f 100644 --- a/pom.xml +++ b/pom.xml @@ -5,7 +5,7 @@ org.springframework.data spring-data-commons - 2.1.0.BUILD-SNAPSHOT + 2.1.0.DATACMNS-1200-SNAPSHOT Spring Data Core From c5e8e627bbcd416f11c6663f696fa298836116d4 Mon Sep 17 00:00:00 2001 From: Mark Paluch Date: Wed, 18 Oct 2017 14:27:05 +0200 Subject: [PATCH 2/4] DATACMNS-1200 - Guard casts to primitive type in ClassGeneratingEntityInstantiator. We now insert assertions for primitive types before passing these to the actual constructor to prevent NullPointerExceptions. We also output the index/parameter name if the parameter was null. --- .../ClassGeneratingEntityInstantiator.java | 21 +++++++++++++++++++ ...GeneratingEntityInstantiatorUnitTests.java | 11 ++++++++++ 2 files changed, 32 insertions(+) diff --git a/src/main/java/org/springframework/data/convert/ClassGeneratingEntityInstantiator.java b/src/main/java/org/springframework/data/convert/ClassGeneratingEntityInstantiator.java index 144b3e005b..a2092ba2e3 100644 --- a/src/main/java/org/springframework/data/convert/ClassGeneratingEntityInstantiator.java +++ b/src/main/java/org/springframework/data/convert/ClassGeneratingEntityInstantiator.java @@ -23,6 +23,7 @@ import java.security.PrivilegedAction; import java.util.Arrays; import java.util.HashMap; +import java.util.List; import java.util.Map; import org.springframework.asm.ClassWriter; @@ -399,6 +400,7 @@ private void visitCreateMethod(ClassWriter cw, PersistentEntity entity, Constructor ctor = constructor.getConstructor(); Class[] parameterTypes = ctor.getParameterTypes(); + List> parameters = constructor.getParameters(); for (int i = 0; i < parameterTypes.length; i++) { @@ -409,6 +411,11 @@ private void visitCreateMethod(ClassWriter cw, PersistentEntity entity, mv.visitInsn(AALOAD); if (parameterTypes[i].isPrimitive()) { + + mv.visitInsn(DUP); + String parameterName = parameters.size() > i ? parameters.get(i).getName() : null; + + insertAssertNotNull(mv, parameterName == null ? String.format("at index %d", i) : parameterName); insertUnboxInsns(mv, Type.getType(parameterTypes[i]).toString().charAt(0), ""); } else { mv.visitTypeInsn(CHECKCAST, Type.getInternalName(parameterTypes[i])); @@ -438,6 +445,20 @@ private static void visitArrayIndex(MethodVisitor mv, int idx) { mv.visitLdcInsn(idx); } + /** + * Insert not {@literal null} assertion for a parameter. + * + * @param mv the method visitor into which instructions should be inserted + * @param parameterName name of the parameter to create the appropriate assertion message. + */ + private void insertAssertNotNull(MethodVisitor mv, String parameterName) { + + // Assert.notNull(property) + mv.visitLdcInsn(String.format("Parameter %s must not be null!", parameterName)); + mv.visitMethodInsn(INVOKESTATIC, "org/springframework/util/Assert", "notNull", + String.format("(%s%s)V", String.format("L%s;", JAVA_LANG_OBJECT), "Ljava/lang/String;"), false); + } + /** * Insert any necessary cast and value call to convert from a boxed type to a primitive value. *

diff --git a/src/test/java/org/springframework/data/convert/ClassGeneratingEntityInstantiatorUnitTests.java b/src/test/java/org/springframework/data/convert/ClassGeneratingEntityInstantiatorUnitTests.java index 242b1a7010..bb3f263cde 100755 --- a/src/test/java/org/springframework/data/convert/ClassGeneratingEntityInstantiatorUnitTests.java +++ b/src/test/java/org/springframework/data/convert/ClassGeneratingEntityInstantiatorUnitTests.java @@ -264,6 +264,17 @@ public void instantiateObjectCtor1ParamInt() { }); } + @Test // DATACMNS-1200 + public void instantiateObjectCtor1ParamIntWithoutValue() { + + doReturn(ObjectCtor1ParamInt.class).when(entity).getType(); + doReturn(PreferredConstructorDiscoverer.discover(ObjectCtor1ParamInt.class))// + .when(entity).getPersistenceConstructor(); + + assertThatThrownBy(() -> this.instance.createInstance(entity, provider)) // + .hasCauseInstanceOf(IllegalArgumentException.class); + } + @Test // DATACMNS-578, DATACMNS-1126 @SuppressWarnings("unchecked") public void instantiateObjectCtor7ParamsString5IntsString() { From 93c019bd6b965b5a1eda41a694dd8972a44feb90 Mon Sep 17 00:00:00 2001 From: Mark Paluch Date: Wed, 18 Oct 2017 14:48:56 +0200 Subject: [PATCH 3/4] DATACMNS-1200 - Polishing. Throw MappingInstantiationException from KotlinClassGeneratingEntityInstantiator if instantiation fails to align behavior with ClassGeneratingEntityInstantiator. Report Kotlin constructor instead of Java constructor if available. --- ...tlinClassGeneratingEntityInstantiator.java | 16 ++++++++++++-- .../model/MappingInstantiationException.java | 22 ++++++++++++++++++- ...ssGeneratingEntityInstantiatorUnitTests.kt | 19 ++++++++++++++++ 3 files changed, 54 insertions(+), 3 deletions(-) diff --git a/src/main/java/org/springframework/data/convert/KotlinClassGeneratingEntityInstantiator.java b/src/main/java/org/springframework/data/convert/KotlinClassGeneratingEntityInstantiator.java index 1bdb23a3ab..05b7d5cd3f 100644 --- a/src/main/java/org/springframework/data/convert/KotlinClassGeneratingEntityInstantiator.java +++ b/src/main/java/org/springframework/data/convert/KotlinClassGeneratingEntityInstantiator.java @@ -20,6 +20,7 @@ import kotlin.reflect.jvm.ReflectJvmMapping; import java.lang.reflect.Constructor; +import java.util.Arrays; import java.util.List; import java.util.stream.IntStream; @@ -27,6 +28,7 @@ import org.springframework.data.mapping.PersistentProperty; import org.springframework.data.mapping.PreferredConstructor; import org.springframework.data.mapping.PreferredConstructor.Parameter; +import org.springframework.data.mapping.model.MappingInstantiationException; import org.springframework.data.mapping.model.ParameterValueProvider; import org.springframework.data.util.ReflectionUtils; import org.springframework.lang.Nullable; @@ -197,7 +199,17 @@ static class DefaultingKotlinClassInstantiatorAdapter implements EntityInstantia public , P extends PersistentProperty

> T createInstance(E entity, ParameterValueProvider

provider) { - PreferredConstructor preferredConstructor = entity.getPersistenceConstructor(); + Object[] params = extractInvocationArguments(entity.getPersistenceConstructor(), provider); + + try { + return (T) instantiator.newInstance(params); + } catch (Exception e) { + throw new MappingInstantiationException(entity, Arrays.asList(params), e); + } + } + + private

, T> Object[] extractInvocationArguments( + @Nullable PreferredConstructor preferredConstructor, ParameterValueProvider

provider) { if (preferredConstructor == null) { throw new IllegalArgumentException("PreferredConstructor must not be null!"); @@ -237,7 +249,7 @@ public , P extends PersistentPrope params[userParameterCount + i] = defaulting[i]; } - return (T) instantiator.newInstance(params); + return params; } } } diff --git a/src/main/java/org/springframework/data/mapping/model/MappingInstantiationException.java b/src/main/java/org/springframework/data/mapping/model/MappingInstantiationException.java index 68e7a82998..37bbe99611 100644 --- a/src/main/java/org/springframework/data/mapping/model/MappingInstantiationException.java +++ b/src/main/java/org/springframework/data/mapping/model/MappingInstantiationException.java @@ -15,6 +15,9 @@ */ package org.springframework.data.mapping.model; +import kotlin.reflect.KFunction; +import kotlin.reflect.jvm.ReflectJvmMapping; + import java.lang.reflect.Constructor; import java.util.ArrayList; import java.util.List; @@ -22,6 +25,7 @@ import org.springframework.data.mapping.PersistentEntity; import org.springframework.data.mapping.PreferredConstructor; +import org.springframework.data.util.ReflectionUtils; import org.springframework.lang.Nullable; import org.springframework.util.ObjectUtils; @@ -88,12 +92,28 @@ private static String buildExceptionMessage(Optional> ent } return String.format(TEXT_TEMPLATE, it.getType().getName(), - constructor.map(c -> c.getConstructor().toString()).orElse("NO_CONSTRUCTOR"), // + constructor.map(c -> toString(c)).orElse("NO_CONSTRUCTOR"), // String.join(",", toStringArgs)); }).orElse(defaultMessage); } + private static String toString(PreferredConstructor preferredConstructor) { + + Constructor constructor = preferredConstructor.getConstructor(); + + if (ReflectionUtils.isSupportedKotlinClass(constructor.getDeclaringClass())) { + + KFunction kotlinFunction = ReflectJvmMapping.getKotlinFunction(constructor); + + if (kotlinFunction != null) { + return kotlinFunction.toString(); + } + } + + return constructor.toString(); + } + /** * Returns the type of the entity that was attempted to instantiate. * diff --git a/src/test/kotlin/org/springframework/data/convert/KotlinClassGeneratingEntityInstantiatorUnitTests.kt b/src/test/kotlin/org/springframework/data/convert/KotlinClassGeneratingEntityInstantiatorUnitTests.kt index 0c3f820c08..a2d05cb66b 100644 --- a/src/test/kotlin/org/springframework/data/convert/KotlinClassGeneratingEntityInstantiatorUnitTests.kt +++ b/src/test/kotlin/org/springframework/data/convert/KotlinClassGeneratingEntityInstantiatorUnitTests.kt @@ -25,8 +25,10 @@ import org.mockito.Mock import org.mockito.junit.MockitoJUnitRunner import org.springframework.data.mapping.PersistentEntity import org.springframework.data.mapping.context.SamplePersistentProperty +import org.springframework.data.mapping.model.MappingInstantiationException import org.springframework.data.mapping.model.ParameterValueProvider import org.springframework.data.mapping.model.PreferredConstructorDiscoverer +import java.lang.IllegalArgumentException /** * Unit tests for [KotlinClassGeneratingEntityInstantiator] creating instances using Kotlin data classes. @@ -79,6 +81,21 @@ class KotlinClassGeneratingEntityInstantiatorUnitTests { Assertions.assertThat(instance.prop34).isEqualTo("White") } + @Test // DATACMNS-1200 + fun `absent primitive value should cause MappingInstantiationException`() { + + val entity = this.entity as PersistentEntity + val constructor = PreferredConstructorDiscoverer.discover(WithBoolean::class.java) + + doReturn(constructor).whenever(entity).persistenceConstructor + doReturn(constructor.constructor.declaringClass).whenever(entity).type + + Assertions.assertThatThrownBy { KotlinClassGeneratingEntityInstantiator().createInstance(entity, provider) } // + .isInstanceOf(MappingInstantiationException::class.java) // + .hasMessageContaining("fun (kotlin.Boolean)") // + .hasCauseInstanceOf(IllegalArgumentException::class.java) + } + data class Contact(val firstname: String, val lastname: String) data class ContactWithDefaulting(val prop0: String, val prop1: String = "White", val prop2: String, @@ -94,5 +111,7 @@ class KotlinClassGeneratingEntityInstantiatorUnitTests { val prop30: String = "White", val prop31: String = "White", val prop32: String = "White", val prop33: String, val prop34: String = "White" ) + + data class WithBoolean(val state: Boolean) } From ba6c835544cbf985dd6d32fee8dd6049b33b6118 Mon Sep 17 00:00:00 2001 From: Mark Paluch Date: Wed, 18 Oct 2017 15:12:38 +0200 Subject: [PATCH 4/4] DATACMNS-1200 - Fix entity instantiation of Kotlin types using primitives with default values. We now determine initial values for primitive parameters in Kotlin constructors that are absent (null) and defaulted. We default all Java primitive types to their initial zero value to prevent possible NullPointerExceptions. Kotlin defaulting uses a bitmask to determine which parameter should be defaulted but still requires the appropriate type. Previously, null values were attempted to cast/unbox and caused NullPointerException even though they had default values through Kotlin assigned. --- ...tlinClassGeneratingEntityInstantiator.java | 49 +++++++++++++++++-- ...ssGeneratingEntityInstantiatorUnitTests.kt | 24 +++++++++ 2 files changed, 69 insertions(+), 4 deletions(-) diff --git a/src/main/java/org/springframework/data/convert/KotlinClassGeneratingEntityInstantiator.java b/src/main/java/org/springframework/data/convert/KotlinClassGeneratingEntityInstantiator.java index 05b7d5cd3f..9b35c17e13 100644 --- a/src/main/java/org/springframework/data/convert/KotlinClassGeneratingEntityInstantiator.java +++ b/src/main/java/org/springframework/data/convert/KotlinClassGeneratingEntityInstantiator.java @@ -229,15 +229,19 @@ private

, T> Object[] extractInvocationArguments int slot = i / 32; int offset = slot * 32; - Object param = provider.getParameterValue(parameters.get(i)); + Parameter parameter = parameters.get(i); + Class type = parameter.getType().getType(); + Object param = provider.getParameterValue(parameter); KParameter kParameter = kParameters.get(i); // what about null and parameter is mandatory? What if parameter is non-null? - if (kParameter.isOptional()) { + if (kParameter.isOptional() && param == null) { - if (param == null) { - defaulting[slot] = defaulting[slot] | (1 << (i - offset)); + defaulting[slot] = defaulting[slot] | (1 << (i - offset)); + + if (type.isPrimitive()) { + param = getPrimitiveDefault(type); } } @@ -251,5 +255,42 @@ private

, T> Object[] extractInvocationArguments return params; } + + private static Object getPrimitiveDefault(Class type) { + + if (type == Byte.TYPE || type == Byte.class) { + return (byte) 0; + } + + if (type == Short.TYPE || type == Short.class) { + return (short) 0; + } + + if (type == Integer.TYPE || type == Integer.class) { + return 0; + } + + if (type == Long.TYPE || type == Long.class) { + return 0L; + } + + if (type == Float.TYPE || type == Float.class) { + return 0F; + } + + if (type == Double.TYPE || type == Double.class) { + return 0D; + } + + if (type == Character.TYPE || type == Character.class) { + return '\u0000'; + } + + if (type == Boolean.TYPE) { + return Boolean.FALSE; + } + + throw new IllegalArgumentException(String.format("Primitive type %s not supported!", type)); + } } } diff --git a/src/test/kotlin/org/springframework/data/convert/KotlinClassGeneratingEntityInstantiatorUnitTests.kt b/src/test/kotlin/org/springframework/data/convert/KotlinClassGeneratingEntityInstantiatorUnitTests.kt index a2d05cb66b..14e0da6b44 100644 --- a/src/test/kotlin/org/springframework/data/convert/KotlinClassGeneratingEntityInstantiatorUnitTests.kt +++ b/src/test/kotlin/org/springframework/data/convert/KotlinClassGeneratingEntityInstantiatorUnitTests.kt @@ -96,6 +96,27 @@ class KotlinClassGeneratingEntityInstantiatorUnitTests { .hasCauseInstanceOf(IllegalArgumentException::class.java) } + @Test // DATACMNS-1200 + fun `should apply primitive defaulting for absent parameters`() { + + val entity = this.entity as PersistentEntity + val constructor = PreferredConstructorDiscoverer.discover(WithPrimitiveDefaulting::class.java) + + doReturn(constructor).whenever(entity).persistenceConstructor + doReturn(constructor.constructor.declaringClass).whenever(entity).type + + val instance: WithPrimitiveDefaulting = KotlinClassGeneratingEntityInstantiator().createInstance(entity, provider) + + Assertions.assertThat(instance.aByte).isEqualTo(0) + Assertions.assertThat(instance.aShort).isEqualTo(0) + Assertions.assertThat(instance.anInt).isEqualTo(0) + Assertions.assertThat(instance.aLong).isEqualTo(0L) + Assertions.assertThat(instance.aFloat).isEqualTo(0.0f) + Assertions.assertThat(instance.aDouble).isEqualTo(0.0) + Assertions.assertThat(instance.aChar).isEqualTo('a') + Assertions.assertThat(instance.aBool).isTrue() + } + data class Contact(val firstname: String, val lastname: String) data class ContactWithDefaulting(val prop0: String, val prop1: String = "White", val prop2: String, @@ -113,5 +134,8 @@ class KotlinClassGeneratingEntityInstantiatorUnitTests { ) data class WithBoolean(val state: Boolean) + + data class WithPrimitiveDefaulting(val aByte: Byte = 0, val aShort: Short = 0, val anInt: Int = 0, val aLong: Long = 0L, + val aFloat: Float = 0.0f, val aDouble: Double = 0.0, val aChar: Char = 'a', val aBool: Boolean = true) }