From f1d9288e781ed61f6f05f70ee76480bd26759dd2 Mon Sep 17 00:00:00 2001 From: jiangchao1 Date: Mon, 1 Jul 2019 20:25:57 +0800 Subject: [PATCH 1/4] DATAJPA-1544 - Delete totals.size() == 1 condition in count method in class JpaQueryExecution. The totals.get(0) cannot always get the correct count result no matter whether there is a group by clause or not. There is also misunderstanding when we use getTotalElements method for paging. --- .../repository/query/JpaQueryExecution.java | 3 ++- .../query/JpaQueryExecutionUnitTests.java | 26 +++++++++++++++++++ 2 files changed, 28 insertions(+), 1 deletion(-) diff --git a/src/main/java/org/springframework/data/jpa/repository/query/JpaQueryExecution.java b/src/main/java/org/springframework/data/jpa/repository/query/JpaQueryExecution.java index 3d94aa6167..221bbd4484 100644 --- a/src/main/java/org/springframework/data/jpa/repository/query/JpaQueryExecution.java +++ b/src/main/java/org/springframework/data/jpa/repository/query/JpaQueryExecution.java @@ -56,6 +56,7 @@ * @author Christoph Strobl * @author Nicolas Cirigliano * @author Jens Schauder + * @author Chao Jiang */ public abstract class JpaQueryExecution { @@ -206,7 +207,7 @@ protected Object doExecute(final AbstractJpaQuery repositoryQuery, final Object[ private long count(AbstractJpaQuery repositoryQuery, Object[] values) { List totals = repositoryQuery.createCountQuery(values).getResultList(); - return (totals.size() == 1 ? CONVERSION_SERVICE.convert(totals.get(0), Long.class) : totals.size()); + return totals.size(); } } diff --git a/src/test/java/org/springframework/data/jpa/repository/query/JpaQueryExecutionUnitTests.java b/src/test/java/org/springframework/data/jpa/repository/query/JpaQueryExecutionUnitTests.java index 572f174e6c..bd6b115e34 100644 --- a/src/test/java/org/springframework/data/jpa/repository/query/JpaQueryExecutionUnitTests.java +++ b/src/test/java/org/springframework/data/jpa/repository/query/JpaQueryExecutionUnitTests.java @@ -34,6 +34,7 @@ import org.mockito.Mock; import org.mockito.Mockito; import org.mockito.junit.MockitoJUnitRunner; +import org.springframework.data.domain.Page; import org.springframework.data.domain.PageRequest; import org.springframework.data.domain.Pageable; import org.springframework.data.jpa.repository.query.JpaQueryExecution.ModifyingExecution; @@ -49,6 +50,7 @@ * @author Mark Paluch * @author Nicolas Cirigliano * @author Jens Schauder + * @author Chao Jiang */ @RunWith(MockitoJUnitRunner.Silent.class) public class JpaQueryExecutionUnitTests { @@ -168,6 +170,30 @@ public void pagedExecutionShouldNotGenerateCountQueryIfQueryReportedNoResults() verify(countQuery, times(0)).getResultList(); verify(jpaQuery, times(0)).createCountQuery((Object[]) any()); } + + @Test // DATAJPA-1544 + public void pagedExecutionShouldUseTotalSizeInCount() throws Exception { + + Parameters parameters = new DefaultParameters(getClass().getMethod("sampleMethod", Pageable.class)); + when(jpaQuery.createCountQuery(Mockito.any(Object[].class))).thenReturn(countQuery); + when(jpaQuery.createQuery(Mockito.any(Object[].class))).thenReturn(query); + when(countQuery.getResultList()).thenReturn(Arrays.asList(20L)); + when(query.getResultList()).thenReturn(Arrays.asList(20L)); + + PagedExecution execution = new PagedExecution(parameters); + Page largePage = (Page) execution.doExecute(jpaQuery, new Object[] { PageRequest.of(0, 10) }); + Page equalPage = (Page) execution.doExecute(jpaQuery, new Object[] { PageRequest.of(0, 1) }); + + assertEquals(largePage.getTotalElements(), 1); + assertEquals(equalPage.getTotalElements(), 1); + + when(countQuery.getResultList()).thenReturn(Arrays.asList(20L, 20L)); + when(query.getResultList()).thenReturn(Arrays.asList(20L, 20L)); + + Page smallPage = (Page) execution.doExecute(jpaQuery, new Object[] { PageRequest.of(0, 1) }); + + assertEquals(smallPage.getTotalElements(), 2); + } @Test // DATAJPA-912 public void pagedExecutionShouldUseCountFromResultIfOffsetIsZeroAndResultsWithinPageSize() throws Exception { From 7b7c4973e71f4fced47fcfb34875856fb092958f Mon Sep 17 00:00:00 2001 From: jiangchao1 Date: Thu, 11 Jul 2019 11:20:28 +0800 Subject: [PATCH 2/4] DATAJPA-1544 - update count method in JpaQueryExecution. --- .../repository/query/JpaQueryExecution.java | 21 +++++++++++++++--- .../query/JpaQueryExecutionUnitTests.java | 22 +++++++++---------- 2 files changed, 29 insertions(+), 14 deletions(-) diff --git a/src/main/java/org/springframework/data/jpa/repository/query/JpaQueryExecution.java b/src/main/java/org/springframework/data/jpa/repository/query/JpaQueryExecution.java index 221bbd4484..90ff2e5da2 100644 --- a/src/main/java/org/springframework/data/jpa/repository/query/JpaQueryExecution.java +++ b/src/main/java/org/springframework/data/jpa/repository/query/JpaQueryExecution.java @@ -15,10 +15,15 @@ */ package org.springframework.data.jpa.repository.query; +import static java.util.regex.Pattern.CASE_INSENSITIVE; +import static java.util.regex.Pattern.compile; + import java.lang.reflect.Method; import java.util.Collection; import java.util.List; import java.util.Optional; +import java.util.regex.Matcher; +import java.util.regex.Pattern; import javax.persistence.EntityManager; import javax.persistence.NoResultException; @@ -205,13 +210,23 @@ protected Object doExecute(final AbstractJpaQuery repositoryQuery, final Object[ } private long count(AbstractJpaQuery repositoryQuery, Object[] values) { - + StringBuilder builder = new StringBuilder(); + builder.append("(.*)?(group\\s+by\\s+).*"); + Pattern GROUP_MATCH = compile(builder.toString(), CASE_INSENSITIVE); + List totals = repositoryQuery.createCountQuery(values).getResultList(); - return totals.size(); + + Matcher matcher = GROUP_MATCH.matcher(repositoryQuery.getQueryMethod().getCountQuery()); + if(matcher.matches()) { + return totals.size(); + } else { + return (totals.size() == 1 ? CONVERSION_SERVICE.convert(totals.get(0), Long.class) : totals.size()); + } + } } - /** + /** * Executes a {@link AbstractStringBasedJpaQuery} to return a single entity. */ static class SingleEntityExecution extends JpaQueryExecution { diff --git a/src/test/java/org/springframework/data/jpa/repository/query/JpaQueryExecutionUnitTests.java b/src/test/java/org/springframework/data/jpa/repository/query/JpaQueryExecutionUnitTests.java index bd6b115e34..b3779b2837 100644 --- a/src/test/java/org/springframework/data/jpa/repository/query/JpaQueryExecutionUnitTests.java +++ b/src/test/java/org/springframework/data/jpa/repository/query/JpaQueryExecutionUnitTests.java @@ -68,6 +68,7 @@ public void setUp(){ when(query.executeUpdate()).thenReturn(0); when(jpaQuery.createQuery(Mockito.any(Object[].class))).thenReturn(query); when(jpaQuery.getQueryMethod()).thenReturn(method); + when(method.getCountQuery()).thenReturn("select count(1) from User u"); } @Test(expected = IllegalArgumentException.class) @@ -179,20 +180,19 @@ public void pagedExecutionShouldUseTotalSizeInCount() throws Exception { when(jpaQuery.createQuery(Mockito.any(Object[].class))).thenReturn(query); when(countQuery.getResultList()).thenReturn(Arrays.asList(20L)); when(query.getResultList()).thenReturn(Arrays.asList(20L)); - + when(method.getCountQuery()).thenReturn("select count(1) from User u"); + PagedExecution execution = new PagedExecution(parameters); - Page largePage = (Page) execution.doExecute(jpaQuery, new Object[] { PageRequest.of(0, 10) }); - Page equalPage = (Page) execution.doExecute(jpaQuery, new Object[] { PageRequest.of(0, 1) }); - - assertEquals(largePage.getTotalElements(), 1); - assertEquals(equalPage.getTotalElements(), 1); - - when(countQuery.getResultList()).thenReturn(Arrays.asList(20L, 20L)); - when(query.getResultList()).thenReturn(Arrays.asList(20L, 20L)); + Page page = (Page) execution.doExecute(jpaQuery, new Object[] { PageRequest.of(0, 1) }); + + assertEquals(page.getTotalElements(), 20); + + when(method.getCountQuery()).thenReturn("select count(1) from User u group by u.id"); + + page = (Page) execution.doExecute(jpaQuery, new Object[] { PageRequest.of(0, 1) }); - Page smallPage = (Page) execution.doExecute(jpaQuery, new Object[] { PageRequest.of(0, 1) }); + assertEquals(page.getTotalElements(), 1); - assertEquals(smallPage.getTotalElements(), 2); } @Test // DATAJPA-912 From 502c9bdb4f1e96c42af5dc1724b8828259d228dd Mon Sep 17 00:00:00 2001 From: jiangchao1 Date: Thu, 11 Jul 2019 16:50:23 +0800 Subject: [PATCH 3/4] DATAJPA-1544 - update count method and UTs. --- .../repository/query/JpaQueryExecution.java | 18 ++++++++++++++---- .../data/jpa/repository/query/NamedQuery.java | 8 ++++++++ .../query/JpaQueryExecutionUnitTests.java | 18 +++++++++++++++--- 3 files changed, 37 insertions(+), 7 deletions(-) diff --git a/src/main/java/org/springframework/data/jpa/repository/query/JpaQueryExecution.java b/src/main/java/org/springframework/data/jpa/repository/query/JpaQueryExecution.java index 90ff2e5da2..f66d2acf6a 100644 --- a/src/main/java/org/springframework/data/jpa/repository/query/JpaQueryExecution.java +++ b/src/main/java/org/springframework/data/jpa/repository/query/JpaQueryExecution.java @@ -211,18 +211,28 @@ protected Object doExecute(final AbstractJpaQuery repositoryQuery, final Object[ private long count(AbstractJpaQuery repositoryQuery, Object[] values) { StringBuilder builder = new StringBuilder(); - builder.append("(.*)?(group\\s+by\\s+).*"); - Pattern GROUP_MATCH = compile(builder.toString(), CASE_INSENSITIVE); + String queryString; List totals = repositoryQuery.createCountQuery(values).getResultList(); - Matcher matcher = GROUP_MATCH.matcher(repositoryQuery.getQueryMethod().getCountQuery()); + if(repositoryQuery instanceof AbstractStringBasedJpaQuery) { + queryString = ((AbstractStringBasedJpaQuery)repositoryQuery).getQuery().getQueryString(); + } else if(repositoryQuery instanceof NamedQuery) { + queryString = ((NamedQuery)repositoryQuery).getQuery().getQueryString(); + } else { + //OartTreeJpaQuery, StoredProcedureJpaQuery, etc. + queryString = ""; + } + + builder.append("(.*)?(group\\s+by\\s+).*"); + Pattern GROUP_MATCH = compile(builder.toString(), CASE_INSENSITIVE); + Matcher matcher = GROUP_MATCH.matcher(queryString); + if(matcher.matches()) { return totals.size(); } else { return (totals.size() == 1 ? CONVERSION_SERVICE.convert(totals.get(0), Long.class) : totals.size()); } - } } diff --git a/src/main/java/org/springframework/data/jpa/repository/query/NamedQuery.java b/src/main/java/org/springframework/data/jpa/repository/query/NamedQuery.java index 73528f24a8..7d9476b9a9 100644 --- a/src/main/java/org/springframework/data/jpa/repository/query/NamedQuery.java +++ b/src/main/java/org/springframework/data/jpa/repository/query/NamedQuery.java @@ -41,6 +41,7 @@ * @author Oliver Gierke * @author Thomas Darimont * @author Mark Paluch + * @author Chao Jiang */ final class NamedQuery extends AbstractJpaQuery { @@ -224,4 +225,11 @@ protected Optional> getTypeToRead(ReturnedType returnedType) { ? Optional.empty() // : super.getTypeToRead(returnedType); } + + /** + * @return the query + */ + public DeclaredQuery getQuery() { + return declaredQuery; + } } diff --git a/src/test/java/org/springframework/data/jpa/repository/query/JpaQueryExecutionUnitTests.java b/src/test/java/org/springframework/data/jpa/repository/query/JpaQueryExecutionUnitTests.java index b3779b2837..885fafad3f 100644 --- a/src/test/java/org/springframework/data/jpa/repository/query/JpaQueryExecutionUnitTests.java +++ b/src/test/java/org/springframework/data/jpa/repository/query/JpaQueryExecutionUnitTests.java @@ -59,6 +59,7 @@ public class JpaQueryExecutionUnitTests { @Mock AbstractStringBasedJpaQuery jpaQuery; @Mock Query query; @Mock JpaQueryMethod method; + @Mock DeclaredQuery declaredQuery; @Mock TypedQuery countQuery; @@ -68,7 +69,6 @@ public void setUp(){ when(query.executeUpdate()).thenReturn(0); when(jpaQuery.createQuery(Mockito.any(Object[].class))).thenReturn(query); when(jpaQuery.getQueryMethod()).thenReturn(method); - when(method.getCountQuery()).thenReturn("select count(1) from User u"); } @Test(expected = IllegalArgumentException.class) @@ -150,6 +150,9 @@ public void pagedExecutionRetrievesObjectsForPageableOutOfRange() throws Excepti when(jpaQuery.createCountQuery(Mockito.any(Object[].class))).thenReturn(countQuery); when(jpaQuery.createQuery(Mockito.any(Object[].class))).thenReturn(query); when(countQuery.getResultList()).thenReturn(Arrays.asList(20L)); + + when(jpaQuery.getQuery()).thenReturn(declaredQuery); + when(declaredQuery.getQueryString()).thenReturn("select count(1) from User u"); PagedExecution execution = new PagedExecution(parameters); execution.doExecute(jpaQuery, new Object[] { PageRequest.of(2, 10) }); @@ -182,13 +185,16 @@ public void pagedExecutionShouldUseTotalSizeInCount() throws Exception { when(query.getResultList()).thenReturn(Arrays.asList(20L)); when(method.getCountQuery()).thenReturn("select count(1) from User u"); + when(jpaQuery.getQuery()).thenReturn(declaredQuery); + when(declaredQuery.getQueryString()).thenReturn("select count(1) from User u"); + PagedExecution execution = new PagedExecution(parameters); Page page = (Page) execution.doExecute(jpaQuery, new Object[] { PageRequest.of(0, 1) }); assertEquals(page.getTotalElements(), 20); - when(method.getCountQuery()).thenReturn("select count(1) from User u group by u.id"); - + when(declaredQuery.getQueryString()).thenReturn("select count(1) from User u group by u.id"); + page = (Page) execution.doExecute(jpaQuery, new Object[] { PageRequest.of(0, 1) }); assertEquals(page.getTotalElements(), 1); @@ -231,6 +237,9 @@ public void pagedExecutionShouldUseRequestCountFromResultWithOffsetAndResultsHit when(jpaQuery.createCountQuery(Mockito.any(Object[].class))).thenReturn(query); when(countQuery.getResultList()).thenReturn(Arrays.asList(20L)); + when(jpaQuery.getQuery()).thenReturn(declaredQuery); + when(declaredQuery.getQueryString()).thenReturn("select count(1) from User u"); + PagedExecution execution = new PagedExecution(parameters); execution.doExecute(jpaQuery, new Object[] { PageRequest.of(4, 4) }); @@ -247,6 +256,9 @@ public void pagedExecutionShouldUseRequestCountFromResultWithOffsetAndResultsHit when(jpaQuery.createCountQuery(Mockito.any(Object[].class))).thenReturn(query); when(countQuery.getResultList()).thenReturn(Arrays.asList(20L)); + when(jpaQuery.getQuery()).thenReturn(declaredQuery); + when(declaredQuery.getQueryString()).thenReturn("select count(1) from User u"); + PagedExecution execution = new PagedExecution(parameters); execution.doExecute(jpaQuery, new Object[] { PageRequest.of(4, 4) }); From 0140f11289136e48f12876c9017a8cb6693ee085 Mon Sep 17 00:00:00 2001 From: jiangchao1 Date: Thu, 11 Jul 2019 17:00:00 +0800 Subject: [PATCH 4/4] DATAJPA-1544 - Polishing. --- .../data/jpa/repository/query/JpaQueryExecutionUnitTests.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/test/java/org/springframework/data/jpa/repository/query/JpaQueryExecutionUnitTests.java b/src/test/java/org/springframework/data/jpa/repository/query/JpaQueryExecutionUnitTests.java index 885fafad3f..384c0bcf9e 100644 --- a/src/test/java/org/springframework/data/jpa/repository/query/JpaQueryExecutionUnitTests.java +++ b/src/test/java/org/springframework/data/jpa/repository/query/JpaQueryExecutionUnitTests.java @@ -191,13 +191,13 @@ public void pagedExecutionShouldUseTotalSizeInCount() throws Exception { PagedExecution execution = new PagedExecution(parameters); Page page = (Page) execution.doExecute(jpaQuery, new Object[] { PageRequest.of(0, 1) }); - assertEquals(page.getTotalElements(), 20); + assertEquals(page.getTotalElements(), 20); when(declaredQuery.getQueryString()).thenReturn("select count(1) from User u group by u.id"); page = (Page) execution.doExecute(jpaQuery, new Object[] { PageRequest.of(0, 1) }); - assertEquals(page.getTotalElements(), 1); + assertEquals(page.getTotalElements(), 1); }