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.

Sign up for GitHub

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

Jump to bottom

Spring Data Repository nested camel-case properties resolution fix #13312

Merged
merged 1 commit into from
Nov 23, 2020
Merged

Spring Data Repository nested camel-case properties resolution fix #13312

merged 1 commit into from
Nov 23, 2020

Conversation

renegrob
Copy link
Contributor

@renegrob renegrob commented Nov 16, 2020 •
edited
Loading

Fix #13067.
I re-implemented the resolution of nested properties now they should be resolved as described here https://docs.spring.io/spring-data/jpa/docs/current/reference/html/#repositories.query-methods.query-property-expressions. I also widened the allowed types for the resolved properties to the types supported by JPA.
The property resolution is now done iteratively and tries to find the longest match (but at most to the next underscore because the underscore is treated as reserved character according to the Spring Data JPA documentation).

@ghost
Copy link

ghost commented Nov 16, 2020

Thanks for your pull request!

The title of your pull request does not follow our editorial rules. Could you have a look?

This message is automatically generated by a bot.

@ghost ghost added the area/spring Issues relating to the Spring integration label Nov 16, 2020
@renegrob renegrob changed the title Spring Data Repository nested camel-case properties Issue #13067 Spring Data Repository nested camel-case properties resolution fix Nov 16, 2020
@geoand
Copy link
Contributor

geoand commented Nov 17, 2020

Thanks for this!

I think we really need some tests covering the issues the PR addresses

@renegrob renegrob force-pushed the quarkus-issue-13067 branch 2 times, most recently from e9b9d1d to 93be872 Compare November 19, 2020 14:12
@renegrob
Copy link
Contributor Author

@geoand I added some unit tests (and found and fixed some issues). I think the coverage should be okay now.

@geoand
Copy link
Contributor

geoand commented Nov 19, 2020

Thanks a lot!

I'll take a closer look tomorrow or Monday.
Can you please take the PR out of draft so we can get a CI run?

@renegrob renegrob force-pushed the quarkus-issue-13067 branch from 93be872 to 2a3f633 Compare November 19, 2020 18:38
@renegrob renegrob marked this pull request as ready for review November 19, 2020 18:41
@renegrob renegrob force-pushed the quarkus-issue-13067 branch from 2a3f633 to 73509a2 Compare November 20, 2020 07:09
geoand
geoand reviewed Nov 20, 2020
View reviewed changes
Copy link
Contributor

@geoand geoand left a comment

Choose a reason for hiding this comment

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

This is great work, thanks a lot!

I added a couple of tiny comments, can you please address them so we can then merge this?

@renegrob renegrob force-pushed the quarkus-issue-13067 branch from 73509a2 to 689aa25 Compare November 20, 2020 15:39
geoand
geoand approved these changes Nov 20, 2020
View reviewed changes
Copy link
Contributor

@geoand geoand left a comment

Choose a reason for hiding this comment

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

👍

@geoand geoand added this to the 1.11 - master milestone Nov 20, 2020
@geoand geoand added the triage/waiting-for-ci Ready to merge when CI successfully finishes label Nov 20, 2020
@renegrob renegrob force-pushed the quarkus-issue-13067 branch from 689aa25 to 58bbe33 Compare November 21, 2020 16:53
@renegrob
Copy link
Contributor Author

@geoand My branch fails but I don't see how the test failures are related to my changes. I saw that other pull requests seem to have similar problems. The branches run against an isolated repository, don't they? (Unfortunately maven does not support hash based build caches.) Do the builds share any other resources?

@geoand
Copy link
Contributor

geoand commented Nov 21, 2020

They don't share anything.

The failure is likely just a flake. If that is the case, I'll merge your PR anyway.

I'll check on Monday.

@geoand
Copy link
Contributor

geoand commented Nov 23, 2020

Can you please rebase onto the latest master in order to pick up the CI fix?

@renegrob renegrob force-pushed the quarkus-issue-13067 branch from 58bbe33 to 2873086 Compare November 23, 2020 06:58
@renegrob
Copy link
Contributor Author

@geoand Thanks for helping fixing the CI!

@geoand
Copy link
Contributor

geoand commented Nov 23, 2020

Don't thank me, thank @gsmet 😉

@gsmet gsmet merged commit de07180 into quarkusio:master Nov 23, 2020
@gsmet
Copy link
Member

gsmet commented Nov 23, 2020

Thanks!

@geoand is this something we want to backport to 1.10.1? If so, please add the label, thanks!

@geoand
Copy link
Contributor

geoand commented Nov 23, 2020

Yeah, I guess we can

@gsmet gsmet modified the milestones: 1.11 - master, 1.10.1.Final Nov 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@geoand geoand geoand approved these changes

Assignees
No one assigned
Labels
area/spring Issues relating to the Spring integration triage/waiting-for-ci Ready to merge when CI successfully finishes
Projects
None yet
Milestone
1.10.1.Final
Development

Successfully merging this pull request may close these issues.

Spring Data Repository with nested camel-case properties does not work with Quarkus
3 participants
@renegrob @geoand @gsmet