-
Notifications
You must be signed in to change notification settings - Fork 682
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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; | ||
|
@@ -149,6 +150,34 @@ void detectsMetaAnnotatedValueAnnotation() { | |
}); | ||
} | ||
|
||
@Test // GH-2332 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 -> { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we make these either soft asserts, or separate tests? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
|
||
assertThat(ctor.getParameters()).hasSize(1); | ||
assertThat(ctor.getParameters().get(0).getRawType()).isEqualTo(String.class); | ||
}); | ||
} | ||
|
||
static class SyntheticConstructor { | ||
@PersistenceConstructor | ||
private SyntheticConstructor(String x) {} | ||
|
@@ -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); | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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):
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.There was a problem hiding this comment.
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 ourInstantiationAwarePropertyAccessor
.