Skip to content

Fix sorting issue #39

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
Merged
lpandzic merged 4 commits into from
Sep 9, 2021
Merged

Fix sorting issue #39

lpandzic merged 4 commits into from
Sep 9, 2021
+293 −21

Conversation

@dirkluijk
Copy link
Contributor

@dirkluijk dirkluijk commented Sep 8, 2021

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

dirkluijk commented Sep 9, 2021

Will do!

@dirkluijk
Copy link
Contributor Author

dirkluijk commented Sep 9, 2021

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
return RepositoryFragment.implemented(simpleJPAQuerydslFragment);
}

@SuppressWarnings("unchecked")
Copy link
Member

@lpandzic lpandzic Sep 9, 2021

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