JpaSpecificationExecutor.findBy inserts wrong limit/offset into SQL Query

[MartinHaeusler]

I have a Spring Data JPA repository like this:

public interface MyRepository extends
    JpaRepository<MyEntity, UUID>,
    JpaSpecificationExecutor<MyEntity>

... and I'm using a Spring Data Specification object (i.e. criteria API) to query it like so in my service:

@Transactional(readOnly = true)
public List<MyEntity> find(Specification specification, int limit, int offset){
    return this.repository.findBy(specification, entry -> 
            entry.sortBy(Sort.by("timestamp").descending())
                .limit(limit)
                .scroll(ScrollPosition.offset(offset))
                .toList()
        );
}

My JUnit test calls this with limit of 100 and an offset of 0. If I enable Hibernate logging and look at the actual SQL queries being produced (PostGreSQL 15, if it matters), I get the following picture:

Spring Data Version	Limit in .limit(...) call	Limit in SQL	Offset in .offset(...) call	Offset in SQL
3.2.5	100	101 ❌	0	0 ✔️
3.2.6	100	101 ❌	0	0 ✔️
3.2.7	100	101 ❌	0	0 ✔️
3.3.0	100	101 ❌	0	1 ❌
3.3.1	100	101 ❌	0	1 ❌

Apart from the Spring Data JPA version, the codebase was unchanged.

This is really bad news. In 3.2.5 (which I'm using currently) the limit was already off-by-one (it queries one row in addition to what the input parameter to .limit(...) says) but that flew under the radar. I started investigating today because said unit test started to fail after I attempted an upgrade to 3.3.1. Well it turns out: now the offset(...) is also off by 1.

Finding this took me quite a while. I would highly appreciate a fix.

[quaff]

It's a bit weird but intentional, you should provide a failing test case if you think it's a bug.

[MartinHaeusler]

I'm sorry but there's no way that this was intentional. Here's the standalone reproducer project:

https://github.com/MartinHaeusler/springDataLimitOffsetReproducer

It contains a full setup with two JUnit tests. They pass using Spring Boot 3.2.5 and they fail in 3.3.1.

[quaff]

FYI, There is a internal change since v3.3.0, it caused offset 1 instead 0
see 0206de8

[MartinHaeusler]

So... this change introduced the bug?

[quaff]

So... this change introduced the bug?

I think it's an intentional breaking change, you could fix tests by using offset == 0 ? ScrollPosition.offset() : ScrollPosition.offset(offset) .

[MartinHaeusler]

I could, but I think we can agree that this is the opposite of a clean API... The ScrollPosition.offset(int) method should handle the zero case properly internally. Not doing so creates a huge pitfall for developers. How about doing the check for offset == 0 in the ScrollPosition.offset(int) method?

[quaff]

How about doing the check for offset == 0 in the ScrollPosition.offset(int) method?

ScrollPosition.offset() is similar to ScrollPosition.offset(-1) which is not valid offset, It's not same as ScrollPosition.offset(0).
You should check out linked issue for more background.

[MartinHaeusler]

I don't really care about the background, this is a crystal clear regression bug. I even delivered a reproducer that showcases this. There's nothing left to discuss here as far as I'm concerned.

[christophstrobl]

@MartinHaeusler thank you for getting in touch - we'd like to remind you of our code of conduct and kindly ask you to respect everyone trying to help. As @quaff already mentioned, the change was intentional and is documented in the release wiki.
Going forward spring-projects/spring-data-commons#3071 will allow defining in-/exclusion of the ScrollPosition itself.