Fall back to canonical constructor in constructor resolution when using Java Records #2694
Conversation
…ng Java Records. We now fall back to the canonical constructor of records when we cannot disambiguate which constructor to use. Also, reflect the Kotlin persistence creator mechanism. Closes #2625
| @@ -124,12 +125,45 @@ <T, P extends PersistentProperty<P>> PreferredConstructor<T, P> discover(TypeInf | |||
| } | |||
| } | |||
|
|
|||
| if (rawOwningType.isRecord() && (candidates.size() > 1 || (noArg != null && !candidates.isEmpty()))) { | |||
There was a problem hiding this comment.
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.
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.
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.
There was a problem hiding this comment.
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.
| @Test // GH-2332 | ||
| void detectsAnnotatedRecordConstructor() { | ||
|
|
||
| assertThat(PreferredConstructorDiscoverer.discover(RecordWithPersistenceCreator.class)).satisfies(ctor -> { |
There was a problem hiding this comment.
Can we make these either soft asserts, or separate tests?
There was a problem hiding this comment.
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.
| @@ -149,6 +150,34 @@ void detectsMetaAnnotatedValueAnnotation() { | |||
| }); | |||
| } | |||
|
|
|||
| @Test // GH-2332 | |||
There was a problem hiding this comment.
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.
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).
We now fall back to the canonical constructor of records when we cannot disambiguate which constructor to use.
Also, reflect the Kotlin persistence creator mechanism.
Closes #2625