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

Fix sorting issue #39

Merged
merged 4 commits into from
Sep 9, 2021
Merged

Fix sorting issue #39

merged 4 commits into from
Sep 9, 2021

Conversation

dirkluijk
Copy link
Contributor

Fixes #38

First I created a test that replicates the issue, and added a possible fix in a separate commit.
Do I need to add the same tests for the other packages (JPA/R2DBC)?

@dirkluijk dirkluijk mentioned this pull request Sep 8, 2021
Closed
@dirkluijk
Copy link
Contributor Author

dirkluijk commented Sep 8, 2021 •
edited
Loading

It took me a while to figure out what's going on.

It seems that the sorting currently does not take the mapping into account. However, with the PascalNamingStrategy this bug is hard to spot because otherField and OtherField are considered the same column if your DBMS is case-insensitive (which seems the case in the tests).

However, with custom column names, it breaks.

In the fix I pass the RelationalPathBase (which contains the column mapping) to QueryDSL and convert the Sort to a QSort using the columns.

I just pushed a new fix which makes use of the RelationalPersistentProperty, which is closer to the default Spring behaviour.

lpandzic
lpandzic previously approved these changes Sep 9, 2021
View reviewed changes
@lpandzic
Copy link
Member

lpandzic commented Sep 9, 2021

You don't have to add tests for JPA as there's no custom logic for it in this project.
Please do add tests for R2DBC.

@dirkluijk
Copy link
Contributor Author

Will do!

@dirkluijk
Copy link
Contributor Author

Done!

@lpandzic lpandzic merged commit 8967499 into infobip:master Sep 9, 2021
lpandzic
lpandzic reviewed Sep 9, 2021
View reviewed changes
...dbc-querydsl/src/main/java/com/infobip/spring/data/r2dbc/QuerydslR2dbcRepositoryFactory.java
@@ -85,9 +88,15 @@ public QuerydslR2dbcRepositoryFactory(R2dbcEntityOperations operations,
return RepositoryFragment.implemented(simpleJPAQuerydslFragment);
}

@SuppressWarnings("unchecked")
Copy link
Member

Choose a reason for hiding this comment

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

this can be moved above line 97 to minimize the scope (since it's merged I'll do it on master)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lpandzic lpandzic lpandzic left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

How to apply Sort instead of QSort
2 participants
@dirkluijk @lpandzic