spring-projects / spring-data-jpa

Simplifies the development of creating a JPA-based data access layer.
https://spring.io/projects/spring-data-jpa/
Apache License 2.0
3.01k stars 1.42k forks source link

When use specification with group by pagination is broken #2361

Closed pkuzmenkov closed 2 years ago

pkuzmenkov commented 2 years ago

Hi, guys, faced the same issue. Unfortunately, this "feature" totally breaks the pagination. The problem is when I'm using 'groupBy' than a number of returned elements is much less than the totalElements and as a result totalPages is wrong too.

List<Long> totals = query.getResultList();

in my case totals looks like

col_00

    2
    2
    1
    1

(4 rows) So, the content will have only 4 elements.

But in the end, total will be 6 for (Long element : totals) { total += element == null ? 0 : element; }

Originally posted by @pkuzmenkov in https://github.com/spring-projects/spring-data-jpa/issues/1034#issuecomment-971325551

My specification is a little bit complicated, so I can't use distinct(true). Generated query looks like this select count(presentati0_.id) as col_00 from presentation presentati0_ left outer join presentationlabel labels1 on presentati0.id=labels1.presentationid left outer join label label2 on labels1_.labelid=label2.id left outer join presentationlabel labels3 on presentati0.id=labels3.presentationid left outer join label label4 on labels3_.labelid=label4.id cross join users user5 where presentati0.userid=user5.id and totsvector(coalesce(presentati0.title, '')) || totsvector(coalesce(user5.name, '')) || totsvector(coalesce(presentati0.description, '')) || totsvector(coalesce(label4.title, '')) || totsvector(coalesce(presentati0.speaker_name, '')) @@ totsquery(?)=true group by presentati0.id , user5_.name

schauder commented 2 years ago

Specifications are intended as abstractions over WHERE clauses.

Therefore we are not considering this.

pkuzmenkov commented 2 years ago

Sorry, but it is very confusing. When the result has only 4 records, but total shows 6. From my point of view it doesn't matter at all how I've constructed the query. I'm talking about this code: https://github.com/spring-projects/spring-data-jpa/blob/3b78bbf2ffef10f462c22e4c294090ef0620c8b4/src/main/java/org/springframework/data/jpa/repository/support/SimpleJpaRepository.java#L586

In this case, the total could be much more than content.size

robin850 commented 2 years ago

Hi,

Chiming in just to give my two cents ; I can totally understand a "Yup, but we won't fix this" answer, that's fine. :-)

Actually, it really feels like I'm missing something or the Specification interface is a bit misleading. If the purpose of this interface is solely to provide WHERE clauses, why the contract doesn't reflect that ?

At least, the return type of the method should be something like a list of a class that can be then translated to a where clause. Or maybe method parameters should be something like a wrapping class around JPA classes but that'd be read-only.

Also, it really feels like this kind of thing doesn't fit with the fact that specifications are considered abstractions over WHERE clauses if their behavior depends partially on the SELECT one.

I can understand that you don't want to go down a rabbit hole where you'd have to manage all parts of the query but the ORDER BY clause is considered as well.

A warning at runtime or a substantial disclaimer in the documentation might be great as well.

Anyway, thanks for your time !

degr commented 2 years ago

Specifications are intended as abstractions over WHERE clauses. Therefore we are not considering this.

terrible response. It is high-level api, which allow to use group by without any warning or error, but returning wrong results.

It sounds like "you can use specification to obtain Pageable object, except it contains "group by", because it may return invalid totalCountOfElement. But no worry, it is how it intended to be, if you are doing "group by" and got incorrect results, it is not our mistake, it is your architecture lame".

Provide some kind of workaround, or throw warning at least

RuhulxD commented 1 year ago

It's frustrating that query is actually giving a wrong result. @schauder if you are not allowing this to be fixed then please add it the documentation and do not allow people to add group by or any other criteria to the query specification at first place. Now we've to look out for some workaround to fix those bug. 👎🏻

ankitpec72 commented 1 year ago

Same for us. The count query with group by clause does not work. @schauder please fix this issue for all