Skip to content

Fall back to canonical constructor in constructor resolution when using Java Records #2694

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@

<groupId>org.springframework.data</groupId>
<artifactId>spring-data-commons</artifactId>
<version>3.0.0-SNAPSHOT</version>
<version>3.0.0-GH-2625-SNAPSHOT</version>

<name>Spring Data Core</name>
<description>Core Spring concepts underpinning every Spring Data module.</description>
Expand Down
19 changes: 17 additions & 2 deletions src/main/asciidoc/object-mapping.adoc
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,8 @@ The resolution algorithm works as follows:
1. If there is a single static factory method annotated with `@PersistenceCreator` then it is used.
2. If there is a single constructor, it is used.
3. If there are multiple constructors and exactly one is annotated with `@PersistenceCreator`, it is used.
4. If there's a no-argument constructor, it is used.
4. If the type is a Java `Record` the canonical constructor is used.
5. If there's a no-argument constructor, it is used.
Other constructors will be ignored.

The value resolution assumes constructor/factory method argument names to match the property names of the entity, i.e. the resolution will be performed as if the property was to be populated, including all customizations in mapping (different datastore column or field name etc.).
Expand Down Expand Up @@ -295,9 +296,23 @@ Using the same field/column name for different values typically leads to corrupt

Spring Data adapts specifics of Kotlin to allow object creation and mutation.

[[mapping.kotlin.creation]]
=== Kotlin object creation

Kotlin classes are supported to be instantiated , all classes are immutable by default and require explicit property declarations to define mutable properties.
Kotlin classes are supported to be instantiated, all classes are immutable by default and require explicit property declarations to define mutable properties.

Spring Data automatically tries to detect a persistent entity's constructor to be used to materialize objects of that type.
The resolution algorithm works as follows:

1. If there is a constructor that is annotated with `@PersistenceCreator`, it is used.
2. If the type is a <<mapping.kotlin,Kotlin data cass>> the primary constructor is used.
3. If there is a single static factory method annotated with `@PersistenceCreator` then it is used.
4. If there is a single constructor, it is used.
5. If there are multiple constructors and exactly one is annotated with `@PersistenceCreator`, it is used.
6. If the type is a Java `Record` the canonical constructor is used.
7. If there's a no-argument constructor, it is used.
Other constructors will be ignored.

Consider the following `data` class `Person`:

====
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@

import java.lang.annotation.Annotation;
import java.lang.reflect.Constructor;
import java.lang.reflect.RecordComponent;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.List;
Expand All @@ -34,11 +35,11 @@
import org.springframework.data.mapping.PersistentEntity;
import org.springframework.data.mapping.PersistentProperty;
import org.springframework.data.mapping.PreferredConstructor;
import org.springframework.data.util.ClassTypeInformation;
import org.springframework.data.util.KotlinReflectionUtils;
import org.springframework.data.util.TypeInformation;
import org.springframework.lang.Nullable;
import org.springframework.util.Assert;
import org.springframework.util.ClassUtils;

/**
* Helper class to find a {@link PreferredConstructor}.
Expand All @@ -49,7 +50,7 @@
* @author Mark Paluch
* @author Xeno Amess
*/
public interface PreferredConstructorDiscoverer<T, P extends PersistentProperty<P>> {
public interface PreferredConstructorDiscoverer {

/**
* Discovers the {@link PreferredConstructor} for the given type.
Expand Down Expand Up @@ -124,12 +125,45 @@ <T, P extends PersistentProperty<P>> PreferredConstructor<T, P> discover(TypeInf
}
}

if (rawOwningType.isRecord() && (candidates.size() > 1 || (noArg != null && !candidates.isEmpty()))) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find the combination of || and && rather confusing, especially in combination with the negations.

Isn't this equivalent to rawOwningType.isRecord() && (candidates.size != 1 || noArg == null)?
Either way, I think we should simplify this expression and either add a comment or extract to a method to explain the reasoning behind this condition.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After thinking about it again: no my expression is wrong, which kind of proves the point that I consider the current version confusing.

I think in general the checks would be simpler if we would add the noargs constructor to the candidates as well.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thinking even more about it, wouldn't it make more sense to handle the cases in the following order (with noargs added to the candidates):

  • no candidate
  • single candidate
  • records

Also, the whole logic should really fail when there is more than one PreferredCreator annotation, shouldn't it? It currently it seems to just take the first.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Records resolve the no-candidate case by falling back to the constructor that is always there, we do the same for Kotlin data classes already so it makes sense to align. Selecting the no-args constructor for records is pointless with the idea to mimic with… style with our InstantiationAwarePropertyAccessor.

return RECORD.discover(type, entity);
}

if (noArg != null) {
return buildPreferredConstructor(noArg, type, entity);
}

return candidates.size() > 1 || candidates.isEmpty() ? null
: buildPreferredConstructor(candidates.iterator().next(), type, entity);
if (candidates.size() == 1) {
return buildPreferredConstructor(candidates.iterator().next(), type, entity);
}

return null;
}
},

/**
* Discovers the canonical constructor for Java Record types.
*
* @since 3.0
*/
RECORD {

@Nullable
@Override
<T, P extends PersistentProperty<P>> PreferredConstructor<T, P> discover(TypeInformation<T> type,
@Nullable PersistentEntity<T, P> entity) {

Class<?> rawOwningType = type.getType();

if (!rawOwningType.isRecord()) {
return null;
}

Class<?>[] paramTypes = Arrays.stream(rawOwningType.getRecordComponents()).map(RecordComponent::getType)
.toArray(Class<?>[]::new);
Constructor<?> canonicalConstructor = ClassUtils.getConstructorIfAvailable(rawOwningType, paramTypes);

return canonicalConstructor != null ? buildPreferredConstructor(canonicalConstructor, type, entity) : null;
}
},

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
import lombok.Value;

import org.junit.jupiter.api.Test;

import org.springframework.data.mapping.context.SampleMappingContext;
import org.springframework.data.mapping.context.SamplePersistentProperty;
import org.springframework.data.mapping.model.EntityInstantiators;
Expand All @@ -44,8 +45,7 @@ void shouldCreateNewInstance() {
var sample = new Sample("Dave", "Matthews", 42);

PersistentPropertyAccessor<Sample> wrapper = new InstantiationAwarePropertyAccessor<>(sample,
entity::getPropertyAccessor,
instantiators);
entity::getPropertyAccessor, instantiators);

wrapper.setProperty(entity.getRequiredPersistentProperty("firstname"), "Oliver August");

Expand All @@ -71,10 +71,38 @@ void shouldSetMultipleProperties() {
assertThat(wrapper.getBean()).isEqualTo(new Sample("Oliver August", "Heisenberg", 42));
}

@Test // GH-2625
void shouldSetPropertyOfRecordUsingCanonicalConstructor() {

var instantiators = new EntityInstantiators();
var context = new SampleMappingContext();

PersistentEntity<Object, SamplePersistentProperty> entity = context
.getRequiredPersistentEntity(WithSingleArgConstructor.class);

var bean = new WithSingleArgConstructor(42L, "Dave");

PersistentPropertyAccessor<WithSingleArgConstructor> wrapper = new InstantiationAwarePropertyAccessor<>(bean,
entity::getPropertyAccessor, instantiators);

wrapper.setProperty(entity.getRequiredPersistentProperty("name"), "Oliver August");
wrapper.setProperty(entity.getRequiredPersistentProperty("id"), 41L);

assertThat(wrapper.getBean()).isEqualTo(new WithSingleArgConstructor(41L, "Oliver August"));
}

@Value
static class Sample {

String firstname, lastname;
int age;
}

public record WithSingleArgConstructor(Long id, String name) {

public WithSingleArgConstructor(String name) {
this(null, name);
}
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
import org.junit.jupiter.api.Test;
import org.springframework.beans.factory.annotation.Value;
import org.springframework.data.annotation.PersistenceConstructor;
import org.springframework.data.annotation.PersistenceCreator;
import org.springframework.data.mapping.PreferredConstructorDiscovererUnitTests.Outer.Inner;
import org.springframework.data.mapping.model.BasicPersistentEntity;
import org.springframework.data.mapping.model.PreferredConstructorDiscoverer;
Expand Down Expand Up @@ -149,6 +150,34 @@ void detectsMetaAnnotatedValueAnnotation() {
});
}

@Test // GH-2332
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to see a test for the behavior of a private all args constructor for Records, because right now I can't easily tell how the code behaves.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Constructors that match the canonical constructor cannot be private. Also, a constructor matching the canonical makes Java backoff from its default constructor creation (same as with public no-arg constructors for regular classes).

void detectsCanonicalRecordConstructor() {

assertThat(PreferredConstructorDiscoverer.discover(RecordWithSingleArgConstructor.class)).satisfies(ctor -> {

assertThat(ctor.getParameters()).hasSize(2);
assertThat(ctor.getParameters().get(0).getRawType()).isEqualTo(Long.class);
assertThat(ctor.getParameters().get(1).getRawType()).isEqualTo(String.class);
});

assertThat(PreferredConstructorDiscoverer.discover(RecordWithNoArgConstructor.class)).satisfies(ctor -> {

assertThat(ctor.getParameters()).hasSize(2);
assertThat(ctor.getParameters().get(0).getRawType()).isEqualTo(Long.class);
assertThat(ctor.getParameters().get(1).getRawType()).isEqualTo(String.class);
});
}

@Test // GH-2332
void detectsAnnotatedRecordConstructor() {

assertThat(PreferredConstructorDiscoverer.discover(RecordWithPersistenceCreator.class)).satisfies(ctor -> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we make these either soft asserts, or separate tests?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For whatever reason, the entire test class is written in that way. While I find that style strange (likely that comes from our Optional experiments), we could apply some polishing post-RC1.


assertThat(ctor.getParameters()).hasSize(1);
assertThat(ctor.getParameters().get(0).getRawType()).isEqualTo(String.class);
});
}

static class SyntheticConstructor {
@PersistenceConstructor
private SyntheticConstructor(String x) {}
Expand Down Expand Up @@ -227,5 +256,28 @@ static class ClassWithMetaAnnotatedParameter {
@Target(ElementType.PARAMETER)
@Retention(RetentionPolicy.RUNTIME)
@Value("${hello-world}")
@interface MyValue {}
@interface MyValue {
}

public record RecordWithSingleArgConstructor(Long id, String name) {

public RecordWithSingleArgConstructor(String name) {
this(null, name);
}
}

public record RecordWithNoArgConstructor(Long id, String name) {

public RecordWithNoArgConstructor(String name) {
this(null, null);
}
}

public record RecordWithPersistenceCreator(Long id, String name) {

@PersistenceCreator
public RecordWithPersistenceCreator(String name) {
this(null, name);
}
}
}