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 count query with different parameters than main query #1883

Open
wants to merge 3 commits into
base: 4.7.x
Choose a base branch
from

Conversation

radovanradic
Copy link
Contributor

Fixes #1773

@sonarcloud
Copy link

sonarcloud bot commented Nov 25, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

100.0% 100.0% Coverage
0.0% 0.0% Duplication

@dstepanov
Copy link
Contributor

Changes look fine, but IMHO different parameters count might indicate an error. We might want to fail at the compilation time.

@radovanradic
Copy link
Contributor Author

Changes look fine, but IMHO different parameters count might indicate an error. We might want to fail at the compilation time.

Right, I was not sure if that was valid use case and should we support it. Maybe it is and we merge this fix now?

@dstepanov
Copy link
Contributor

@graemerocher WDYT?

@graemerocher
Copy link
Contributor

@dstepanov what type of errors do you foresee?

@dstepanov
Copy link
Contributor

Well, the example added is incorrect and will lead to the count being different than the pagination.

@radovanradic
Copy link
Contributor Author

If we allow users to specify countQuery then I guess it's their responsibility if it does not return count as the main query number of items. That's what this user expects at least.
If you think it's not right, then we can remove this fix and maybe throw the error if parameters are not the same.

@radovanradic
Copy link
Contributor Author

Do we have decision how to handle this, either fix it like in this PR or throw an error if query params and count query params don't match? I guess the user who posted this issue expects fix, but not sure which is better approach.

@safarmer
Copy link
Contributor

safarmer commented Jan 13, 2023

To add some context to the specific use case: we have some overly complicated queries to map older data to our new data structure projections and we need some query parameters to generate data for the result-set that does not affect the number of rows returned.

As an example, we have a query that returns a list of item summaries which includes a left join on the current user associated state (if they have purchased, pinned, saved, etc the item). This leads to a query like the following:

select item_.*, save_.*
from item item_ left join save save_ on item_.id = save_.item and save_.user = :viewer
where item_.published_by = :publisher

with the corresponding count query:

select count(*) from item where published_by = :publisher

This works as the item to save relationship is essentially an optional 1:1 mapping. The existence of a save record for the current user does not change the number of published items.

@safarmer
Copy link
Contributor

@radovanradic @dstepanov would it be helpful to have a test case that was a valid example? I know that using a projection makes it easier to exercise the use case in a way that would not invalidate the results.

Something like this perhaps:

    @Query(value = "select age + :years as age from person person_ where person_.name like :name",
        countQuery = "select count(*) from person person_ where person_.name like :name")
    Page<PersonDto> findFutureAgeByNameLike(String name, int years, Pageable pageable);
    void "test pageable findBy and count using different params"() {
        given:
        def person1 = new Person(name: "Jon", age: 72)
        def person2 = new Person(name: "Jonas")
        def person3 = new Person(name: "Joanna", age: 51)
        personRepository.saveAll(Arrays.asList(person1, person2, person3))
        when: "People are searched by name"
        def pageable = Pageable.from(0, 10)
        Page<PersonDto> page = personRepository.findFutureAgeByNameLike("Jon%", 10, pageable)

        then: "The page returns correct results and count"
        page.offset == 0
        page.pageNumber == 0
        page.totalSize == 2
        page.content
        page.content.size() == 2
    }

@CLAassistant
Copy link

CLAassistant commented Feb 7, 2024

CLA assistant check
All committers have signed the CLA.

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.

Pageable JDBC query with extra parameters not used in countQuery fails
5 participants