Skip to content
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

Fix: add support for Kotlin data classes without PersistenceConstructor #37

Merged
merged 5 commits into from
Sep 8, 2021

Conversation

dirkluijk
Copy link
Contributor

@dirkluijk dirkluijk commented Sep 7, 2021

When using a Kotlin data class without PersistenceConstructor, it picks the incorrect internal Kotlin constructor which leads to errors.

This improvement uses PreferredConstructorDiscoverer from Spring Data Common to find the preferred constructor, which supports Kotlin (source).

@lpandzic lpandzic self-requested a review September 8, 2021 06:25
lpandzic
lpandzic previously approved these changes Sep 8, 2021
@lpandzic
Copy link
Member

lpandzic commented Sep 8, 2021

Tests are failing...

@dirkluijk
Copy link
Contributor Author

Oops, I only tested it in my own project. I'll check it.

@dirkluijk
Copy link
Contributor Author

Okay, so this was a bit of a puzzle.

This is how Spring Data resolves constructors:

- If there is a single constructor, it is used.
- If there are multiple constructors and exactly one is annotated with @PersistenceConstructor, it is used.
- If there’s a no-argument constructor, it is used. Other constructors will be ignored.

(taken from reference docs)

With this logic now in place, there were two problems with NoArgsEntity and QueryDSL:

  1. first of all, it now wants to use the no-args constructor new NoArgsEntity(). However, this constructor is package-private and thus not detectable by QueryDQL. Making this constructor public fixed all tests in this project (except for one, see next comment).

  2. There was one other test failing: QuerydslJdbcRepositoryTest.shouldSupportMultipleConstructors(). For two reasons. First of all, the constructorExpression for a no-arg entityProjection creates an invalid query. It seems QueryDSLs AbstractSQLQuery does not work very well with no-arg constructors. And that makes sense, because the no-arg thingy in Spring Data is probably meant to support JPA, which does not use the AbstractSQLQuery in QueryDSL (I think?). Secondly, looking at that test, it expects all arguments to be set and that the constructor with all the arguments should be used. According to Spring Data JDBC, we should add the @PersistenceConstructor in that case. So that's what I did, and it fixed that test.

  3. Because of the @PersistenceConstructor, the no-args constructor doesn't have to be public anymore.

@lpandzic
Copy link
Member

lpandzic commented Sep 8, 2021

Tests pass and I agree with your reasoning and conclusion. I don't remember exactly why I implemented it the way I did other than for no arg entities support - I assume it's lack of time to research the underlying logic at the time.

@lpandzic lpandzic merged commit e6b32dc into infobip:master Sep 8, 2021
@dirkluijk
Copy link
Contributor Author

Cool, thanks! This allows me to use plain data classes in Kotlin without @PersistenceConstructor, which makes it a little bit cleaner. :)

I might have another contribution, I will look into it anytime soon.

@dirkluijk dirkluijk deleted the support-preferred-constructors branch September 8, 2021 11:47
@lpandzic
Copy link
Member

lpandzic commented Sep 8, 2021

FYI, 6.1.1 has been released.

@lpandzic
Copy link
Member

lpandzic commented Sep 8, 2021

Cool, thanks! This allows me to use plain data classes in Kotlin without @PersistenceConstructor, which makes it a little bit cleaner. :)

I might have another contribution, I will look into it anytime soon.

Thanks for contributing. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants