From a20d89bf9a28eaf09653c22d28bb28f3976c3bc7 Mon Sep 17 00:00:00 2001 From: Mark Paluch Date: Thu, 4 Oct 2018 14:19:55 +0200 Subject: [PATCH] DATACMNS-1402 - Fix invocation of default Kotlin constructor. We now correctly calculate the number of defaulting masks used to represent constructor arguments. Previously, we've been one off which caused that Kotlin classes with 32/33 parameters weren't able to be instantiated. We also now reuse KotlinDefaultMask to apply defaulting calculation and removed code duplicates. Original pull request: #317. --- ...tlinClassGeneratingEntityInstantiator.java | 35 ++-- .../data/mapping/model/KotlinDefaultMask.java | 100 ++++++++++++ ...ameterizedKotlinInstantiatorUnitTests.java | 152 ++++++++++++++++++ .../data/mapping/model/With32Args.kt | 23 +++ .../data/mapping/model/With33Args.kt | 23 +++ 5 files changed, 317 insertions(+), 16 deletions(-) create mode 100644 src/main/java/org/springframework/data/mapping/model/KotlinDefaultMask.java create mode 100644 src/test/java/org/springframework/data/convert/ParameterizedKotlinInstantiatorUnitTests.java create mode 100644 src/test/kotlin/org/springframework/data/mapping/model/With32Args.kt create mode 100644 src/test/kotlin/org/springframework/data/mapping/model/With33Args.kt diff --git a/src/main/java/org/springframework/data/convert/KotlinClassGeneratingEntityInstantiator.java b/src/main/java/org/springframework/data/convert/KotlinClassGeneratingEntityInstantiator.java index 2f7811dc7..a7750f52d 100644 --- a/src/main/java/org/springframework/data/convert/KotlinClassGeneratingEntityInstantiator.java +++ b/src/main/java/org/springframework/data/convert/KotlinClassGeneratingEntityInstantiator.java @@ -28,6 +28,7 @@ import org.springframework.data.mapping.PersistentEntity; 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.KotlinDefaultMask; import org.springframework.data.mapping.model.MappingInstantiationException; import org.springframework.data.mapping.model.ParameterValueProvider; import org.springframework.data.util.ReflectionUtils; @@ -114,7 +115,7 @@ public class KotlinClassGeneratingEntityInstantiator extends ClassGeneratingEnti // candidates must contain at least two additional parameters (int, DefaultConstructorMarker). // Number of defaulting masks derives from the original constructor arg count - int syntheticParameters = (constructor.getParameterCount() / Integer.SIZE) + 1 + int syntheticParameters = KotlinDefaultMask.getMaskCount(constructor.getParameterCount()) + /* DefaultConstructorMarker */ 1; if (constructor.getParameterCount() + syntheticParameters != candidate.getParameterCount()) { @@ -172,6 +173,7 @@ public class KotlinClassGeneratingEntityInstantiator extends ClassGeneratingEnti static class DefaultingKotlinClassInstantiatorAdapter implements EntityInstantiator { private final ObjectInstantiator instantiator; + private final KFunction constructor; private final List kParameters; private final Constructor synthetic; @@ -185,6 +187,7 @@ public class KotlinClassGeneratingEntityInstantiator extends ClassGeneratingEnti } this.instantiator = instantiator; + this.constructor = kotlinConstructor; this.kParameters = kotlinConstructor.getParameters(); this.synthetic = constructor.getConstructor(); } @@ -214,10 +217,8 @@ public class KotlinClassGeneratingEntityInstantiator extends ClassGeneratingEnti throw new IllegalArgumentException("PreferredConstructor must not be null!"); } - int[] defaulting = new int[(synthetic.getParameterCount() / 32) + 1]; - Object[] params = allocateArguments( - synthetic.getParameterCount() + defaulting.length + /* DefaultConstructorMarker */1); + synthetic.getParameterCount() + KotlinDefaultMask.getMaskCount(synthetic.getParameterCount()) + /* DefaultConstructorMarker */1); int userParameterCount = kParameters.size(); List> parameters = preferredConstructor.getParameters(); @@ -225,28 +226,30 @@ public class KotlinClassGeneratingEntityInstantiator extends ClassGeneratingEnti // Prepare user-space arguments for (int i = 0; i < userParameterCount; i++) { - int slot = i / 32; - int offset = slot * 32; - Parameter parameter = parameters.get(i); - Class type = parameter.getType().getType(); - Object param = provider.getParameterValue(parameter); + params[i] = provider.getParameterValue(parameter); + } - KParameter kParameter = kParameters.get(i); + KotlinDefaultMask defaultMask = KotlinDefaultMask.from(constructor, it -> { - // what about null and parameter is mandatory? What if parameter is non-null? - if (kParameter.isOptional() && param == null) { + int index = kParameters.indexOf(it); - defaulting[slot] = defaulting[slot] | (1 << (i - offset)); + Parameter parameter = parameters.get(index); + Class type = parameter.getType().getType(); + if (it.isOptional() && params[index] == null) { if (type.isPrimitive()) { - param = getPrimitiveDefault(type); + + // apply primitive defaulting to prevent NPE on primitive downcast + params[index] = getPrimitiveDefault(type); } + return false; } - params[i] = param; - } + return true; + }); + int[] defaulting = defaultMask.getDefaulting(); // append nullability masks to creation arguments for (int i = 0; i < defaulting.length; i++) { params[userParameterCount + i] = defaulting[i]; diff --git a/src/main/java/org/springframework/data/mapping/model/KotlinDefaultMask.java b/src/main/java/org/springframework/data/mapping/model/KotlinDefaultMask.java new file mode 100644 index 000000000..60ff8bbaf --- /dev/null +++ b/src/main/java/org/springframework/data/mapping/model/KotlinDefaultMask.java @@ -0,0 +1,100 @@ +/* + * Copyright 2018 the original author or authors. + * + * 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.springframework.data.mapping.model; + +import java.util.ArrayList; +import java.util.List; +import java.util.function.IntConsumer; +import java.util.function.Predicate; + +import kotlin.reflect.KFunction; +import kotlin.reflect.KParameter; +import kotlin.reflect.KParameter.Kind; +import lombok.AccessLevel; +import lombok.Getter; +import lombok.RequiredArgsConstructor; + +/** + * Value object representing defaulting masks used for Kotlin methods applying parameter defaulting. + * + * @author Mark Paluch + * @since 2.0.11 + */ +@RequiredArgsConstructor(access = AccessLevel.PRIVATE) +public class KotlinDefaultMask { + + @Getter + private final int[] defaulting; + + /** + * Callback method to notify {@link IntConsumer} for each defaulting mask. + * + * @param maskCallback must not be {@literal null}. + */ + public void forEach(IntConsumer maskCallback) { + + for (int i : defaulting) { + maskCallback.accept(i); + } + } + + /** + * Return the number of defaulting masks required to represent the number of {@code arguments}. + * + * @param arguments number of method arguments. + * @return the number of defaulting masks required. + */ + public static int getMaskCount(int arguments) { + return ((arguments - 1) / Integer.SIZE) + 1; + } + + /** + * Creates defaulting mask(s) used to invoke Kotlin {@literal default} methods that conditionally apply parameter + * values. + * + * @param function the {@link KFunction} that should be invoked. + * @param isPresent {@link Predicate} for the presence/absence of parameters. + * @return {@link KotlinDefaultMask}. + */ + public static KotlinDefaultMask from(KFunction function, Predicate isPresent) { + + List masks = new ArrayList<>(); + int index = 0; + int mask = 0; + + List parameters = function.getParameters(); + + for (KParameter parameter : parameters) { + + if (index != 0 && index % Integer.SIZE == 0) { + masks.add(mask); + mask = 0; + } + + if (parameter.isOptional() && !isPresent.test(parameter)) { + mask = mask | (1 << (index % Integer.SIZE)); + } + + if (parameter.getKind() == Kind.VALUE) { + index++; + } + } + + masks.add(mask); + + return new KotlinDefaultMask(masks.stream().mapToInt(i -> i).toArray()); + } +} diff --git a/src/test/java/org/springframework/data/convert/ParameterizedKotlinInstantiatorUnitTests.java b/src/test/java/org/springframework/data/convert/ParameterizedKotlinInstantiatorUnitTests.java new file mode 100644 index 000000000..d07fa11c0 --- /dev/null +++ b/src/test/java/org/springframework/data/convert/ParameterizedKotlinInstantiatorUnitTests.java @@ -0,0 +1,152 @@ +/* + * Copyright 2018 the original author or authors. + * + * 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.springframework.data.convert; + +import static org.assertj.core.api.Assertions.*; + +import java.util.ArrayList; +import java.util.List; +import java.util.stream.Collectors; +import java.util.stream.IntStream; + +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.Parameterized; +import org.junit.runners.Parameterized.Parameters; +import org.springframework.data.mapping.PersistentEntity; +import org.springframework.data.mapping.PreferredConstructor.Parameter; +import org.springframework.data.mapping.context.SampleMappingContext; +import org.springframework.data.mapping.context.SamplePersistentProperty; +import org.springframework.data.mapping.model.BasicPersistentEntity; +import org.springframework.data.mapping.model.ParameterValueProvider; +import org.springframework.data.mapping.model.With32Args; +import org.springframework.data.mapping.model.With33Args; +import org.springframework.test.util.ReflectionTestUtils; + +/** + * Unit test to verify correct object instantiation using Kotlin defaulting via {@link KotlinClassGeneratingEntityInstantiator}. + * + * @author Mark Paluch + */ +@RunWith(Parameterized.class) +public class ParameterizedKotlinInstantiatorUnitTests { + + private final String valueToSet = "THE VALUE"; + private final PersistentEntity entity; + private final int propertyCount; + private final int propertyUnderTestIndex; + private final String propertyUnderTestName; + private final EntityInstantiator entityInstantiator; + + public ParameterizedKotlinInstantiatorUnitTests(PersistentEntity entity, int propertyCount, int propertyUnderTestIndex, String propertyUnderTestName, EntityInstantiator entityInstantiator, String label) { + this.entity = entity; + this.propertyCount = propertyCount; + this.propertyUnderTestIndex = propertyUnderTestIndex; + this.propertyUnderTestName = propertyUnderTestName; + this.entityInstantiator = entityInstantiator; + } + + @Parameters(name = "{5}") + public static List parameters() { + + SampleMappingContext context = new SampleMappingContext(); + + KotlinClassGeneratingEntityInstantiator generatingInstantiator = new KotlinClassGeneratingEntityInstantiator(); + ReflectionEntityInstantiator reflectionInstantiator = ReflectionEntityInstantiator.INSTANCE; + + List fixtures = new ArrayList<>(); + fixtures.addAll(createFixture(context, With32Args.class, 32, generatingInstantiator)); + fixtures.addAll(createFixture(context, With32Args.class, 32, reflectionInstantiator)); + fixtures.addAll(createFixture(context, With33Args.class, 33, generatingInstantiator)); + fixtures.addAll(createFixture(context, With33Args.class, 33, reflectionInstantiator)); + + return fixtures; + } + + private static List createFixture(SampleMappingContext context, Class entityType, int propertyCount, EntityInstantiator entityInstantiator) { + + BasicPersistentEntity persistentEntity = context.getPersistentEntity(entityType); + + return IntStream.range(0, propertyCount).mapToObj(i -> { + + return new Object[]{persistentEntity, propertyCount, i, Integer.toString(i), entityInstantiator, String.format("Property %d for %s using %s", i, entityType.getSimpleName(), entityInstantiator.getClass().getSimpleName())}; + }).collect(Collectors.toList()); + } + + @Test // DATACMNS-1402 + public void shouldCreateInstanceWithSinglePropertySet() { + + Object instance = entityInstantiator.createInstance(entity, new SingleParameterValueProvider()); + + for (int i = 0; i < propertyCount; i++) { + + Object value = ReflectionTestUtils.getField(instance, Integer.toString(i)); + + if (propertyUnderTestIndex == i) { + assertThat(value).describedAs("Property " + i + " of " + entity).isEqualTo(valueToSet); + } else { + assertThat(value).describedAs("Property " + i + " of " + entity).isEqualTo(""); + } + } + } + + @Test // DATACMNS-1402 + public void shouldCreateInstanceWithAllExceptSinglePropertySet() { + + Object instance = entityInstantiator.createInstance(entity, new AllButParameterValueProvider()); + + for (int i = 0; i < propertyCount; i++) { + + Object value = ReflectionTestUtils.getField(instance, Integer.toString(i)); + + if (propertyUnderTestIndex == i) { + assertThat(value).describedAs("Property " + i + " of " + entity).isEqualTo(""); + } else { + assertThat(value).describedAs("Property " + i + " of " + entity).isEqualTo(Integer.toString(i)); + } + } + } + + /** + * Return the value to set for the property to test. + */ + class SingleParameterValueProvider implements ParameterValueProvider { + + @Override + public T getParameterValue(Parameter parameter) { + + if (parameter.getName().equals(propertyUnderTestName)) { + return (T) valueToSet; + } + return null; + } + } + + /** + * Return the property name as value for all properties except the one to test. + */ + class AllButParameterValueProvider implements ParameterValueProvider { + + @Override + public T getParameterValue(Parameter parameter) { + + if (!parameter.getName().equals(propertyUnderTestName)) { + return (T) parameter.getName(); + } + return null; + } + } +} diff --git a/src/test/kotlin/org/springframework/data/mapping/model/With32Args.kt b/src/test/kotlin/org/springframework/data/mapping/model/With32Args.kt new file mode 100644 index 000000000..09cff72a4 --- /dev/null +++ b/src/test/kotlin/org/springframework/data/mapping/model/With32Args.kt @@ -0,0 +1,23 @@ +/* + * Copyright 2018 the original author or authors. + * + * 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.springframework.data.mapping.model + +data class With32Args(val `0`: String = "", val `1`: String = "", val `2`: String = "", val `3`: String = "", val `4`: String = "", val `5`: String = "", + val `6`: String = "", val `7`: String = "", val `8`: String = "", val `9`: String = "", val `10`: String = "", val `11`: String = "", val `12`: String = "", + val `13`: String = "", val `14`: String = "", val `15`: String = "", val `16`: String = "", val `17`: String = "", val `18`: String = "", val `19`: String = "", + val `20`: String = "", val `21`: String = "", val `22`: String = "", val `23`: String = "", val `24`: String = "", val `25`: String = "", val `26`: String = "", + val `27`: String = "", val `28`: String = "", val `29`: String = "", val `30`: String = "", val `31`: String = "" +) diff --git a/src/test/kotlin/org/springframework/data/mapping/model/With33Args.kt b/src/test/kotlin/org/springframework/data/mapping/model/With33Args.kt new file mode 100644 index 000000000..36be17a25 --- /dev/null +++ b/src/test/kotlin/org/springframework/data/mapping/model/With33Args.kt @@ -0,0 +1,23 @@ +/* + * Copyright 2018 the original author or authors. + * + * 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.springframework.data.mapping.model + +data class With33Args(val `0`: String = "", val `1`: String = "", val `2`: String = "", val `3`: String = "", val `4`: String = "", val `5`: String = "", + val `6`: String = "", val `7`: String = "", val `8`: String = "", val `9`: String = "", val `10`: String = "", val `11`: String = "", val `12`: String = "", + val `13`: String = "", val `14`: String = "", val `15`: String = "", val `16`: String = "", val `17`: String = "", val `18`: String = "", val `19`: String = "", + val `20`: String = "", val `21`: String = "", val `22`: String = "", val `23`: String = "", val `24`: String = "", val `25`: String = "", val `26`: String = "", + val `27`: String = "", val `28`: String = "", val `29`: String = "", val `30`: String = "", val `31`: String = "", val `32`: String = "" +)