Skip to content

DATAJPA-1544 - Delete totals.size() == 1 condition in count method #388

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.

Sign up for GitHub

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

Closed
wants to merge 4 commits into from
Closed

DATAJPA-1544 - Delete totals.size() == 1 condition in count method #388

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f1d9288
DATAJPA-1544 - Delete totals.size() == 1 condition in count method in
Jul 1, 2019
7b7c497
DATAJPA-1544 - update count method in JpaQueryExecution.
Jul 11, 2019
502c9bd
DATAJPA-1544 - update count method and UTs.
Jul 11, 2019
0140f11
DATAJPA-1544 - Polishing.
Jul 11, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 29 additions & 3 deletions src/main/java/org/springframework/data/jpa/repository/query/JpaQueryExecution.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -56,6 +61,7 @@
* @author Christoph Strobl
* @author Nicolas Cirigliano
* @author Jens Schauder
* @author Chao Jiang
*/
public abstract class JpaQueryExecution {

Expand Down Expand Up @@ -204,13 +210,33 @@ protected Object doExecute(final AbstractJpaQuery repositoryQuery, final Object[
}

private long count(AbstractJpaQuery repositoryQuery, Object[] values) {

StringBuilder builder = new StringBuilder();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be honest I don't really grasp what is going on here.

I guess it starts with the previously existing version of the source code, which is bogus AFAIK since a count query must always return exactly one value.
Therefore this PR should be done on top of #400.

Any parsing of queries should happen where all the other parsing happens inside AstractJpaQuery derivatives.

And finally and possibly most crucial: The problem is that the generation of the count query is broken so that is the location that should get fixed.

String queryString;

List<?> totals = repositoryQuery.createCountQuery(values).getResultList();
return (totals.size() == 1 ? CONVERSION_SERVICE.convert(totals.get(0), Long.class) : totals.size());

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());
}
}
}

/**
/**
* Executes a {@link AbstractStringBasedJpaQuery} to return a single entity.
*/
static class SingleEntityExecution extends JpaQueryExecution {
Expand Down
8 changes: 8 additions & 0 deletions src/main/java/org/springframework/data/jpa/repository/query/NamedQuery.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@
* @author Oliver Gierke
* @author Thomas Darimont
* @author Mark Paluch
* @author Chao Jiang
*/
final class NamedQuery extends AbstractJpaQuery {

Expand Down Expand Up @@ -224,4 +225,11 @@ protected Optional<Class<?>> getTypeToRead(ReturnedType returnedType) {
? Optional.empty() //
: super.getTypeToRead(returnedType);
}

/**
* @return the query
*/
public DeclaredQuery getQuery() {
return declaredQuery;
}
}
38 changes: 38 additions & 0 deletions src/test/java/org/springframework/data/jpa/repository/query/JpaQueryExecutionUnitTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -49,6 +50,7 @@
* @author Mark Paluch
* @author Nicolas Cirigliano
* @author Jens Schauder
* @author Chao Jiang
*/
@RunWith(MockitoJUnitRunner.Silent.class)
public class JpaQueryExecutionUnitTests {
Expand All @@ -57,6 +59,7 @@ public class JpaQueryExecutionUnitTests {
@Mock AbstractStringBasedJpaQuery jpaQuery;
@Mock Query query;
@Mock JpaQueryMethod method;
@Mock DeclaredQuery declaredQuery;

@Mock TypedQuery<Long> countQuery;

Expand Down Expand Up @@ -147,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) });
Expand All @@ -168,6 +174,32 @@ 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));
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<Object> page = (Page<Object>) execution.doExecute(jpaQuery, new Object[] { PageRequest.of(0, 1) });

assertEquals(page.getTotalElements(), 20);

when(declaredQuery.getQueryString()).thenReturn("select count(1) from User u group by u.id");

page = (Page<Object>) execution.doExecute(jpaQuery, new Object[] { PageRequest.of(0, 1) });

assertEquals(page.getTotalElements(), 1);

}

@Test // DATAJPA-912
public void pagedExecutionShouldUseCountFromResultIfOffsetIsZeroAndResultsWithinPageSize() throws Exception {
Expand Down Expand Up @@ -205,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) });

Expand All @@ -221,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) });

Expand Down