Incomplete null handling in QueryDslJpaRepository [DATAJPA-585]

[spring-projects-issues]

Grigoris Dimoulas opened DATAJPA-585 and commented

In QueryDslJpaRepository, when findAll(Predicate, Pageable) is called with a null Pageable, a NPE is thrown after the code tries to access the offset. However, delegation to the Querydsl helper class has dealt with the null value properly two lines above.

---

Affects: 1.6.1 (Dijkstra SR1)

Referenced from: commits https://github.com/spring-projects/spring-data-jpa/commit/6c2b80a79af201d360a256e12f3813fc09e9f3aa, https://github.com/spring-projects/spring-data-jpa/commit/7fa997085d2493339b993815365d0881bcf17f7e, https://github.com/spring-projects/spring-data-jpa/commit/eeede28c9a8647619aad7600f7939de08383cb6b

Backported to: 1.9.1 (Gosling SR1), 1.8.3 (Fowler SR3)

[spring-projects-issues]

Thomas Darimont commented

Hello Grigoris,

thanks for reporting this - but I think this is fixed in the latest version of Dijkstra / Evans. In org.springframework.data.jpa.repository.query.JpaQueryExecution.PagedExecution.doExecute(AbstractJpaQuery, Object[]) the current check looks like:

List<Object> content = pageable == null || total > pageable.getOffset() ? query.getResultList() : Collections
              .emptyList();

So in this case pageable.getOffset() can only be called when pageable is not null. Would you mind given this a spring with a more recent version?

Cheers, Thomas

[spring-projects-issues]

Grigoris Dimoulas commented

Hi Thomas

Unfortunately, I am not currently working with the same team as back in August, so I don't have access to the code that caused the issue.

Cheers, Grigoris

[spring-projects-issues]

Thomas Darimont commented

Oliver Drotbohm - any thoughts on this? I think we can close this ticket

[spring-projects-issues]

Grigoris Dimoulas commented

It seems ok now, so yes, mark it as fixed

[spring-projects-issues]

Thomas Darimont commented

I think we can close this as well

[spring-projects-issues]

Steven Thomson commented

The NPE issue has returned in 1.9.0:

public Page<T> findAll(Predicate predicate, Pageable pageable) {
    ...
    List<T> content = pageable != null && total > pageable.getOffset() ? query.list(path) : Collections.<T> emptyList();
    ...
}

Adding the null check would fix the issue

[spring-projects-issues]

Oliver Drotbohm commented

I am not sure where the NPE can occur here. If pageable is null the latter part of the and-clause is not evaluated

[spring-projects-issues]

Steven Thomson commented

I added the "pageable != null &&" part. In the current code it does not exist. Sorry for the confusion, the code looks like this right now:

List<T> content = total > pageable.getOffset() ? query.list(path) : Collections.<T> emptyList();

I should have put the suggestion is a different section of the comment

[spring-projects-issues]

Oliver Drotbohm commented

Oh, jeez. Sorry for that. Should be an easy fix! :)

[spring-projects-issues]

Oliver Drotbohm commented

That's fixed in master and the bugfix branches for Gosling and Fowler. I've use pageable == null || … though as we have to trigger the query execution if no Pageable is given

[spring-projects-issues]

Steven Thomson commented

Great! Thank you for the quick turnaround