From c585ed3c3b9c4fb91c9abc5bc7a7b80ea41d6cb1 Mon Sep 17 00:00:00 2001 From: Thomas Darimont Date: Thu, 23 Oct 2014 14:58:36 +0200 Subject: [PATCH] DATAJPA-617 - Avoid potential transaction rollbacks during named query lookups. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We now use a dedicated EntityManager for the named query lookup in NamedQuery.hasNamedQuery(…). Previously we used the given EntityManager to lookup the named query which will cause the transaction to be rolled back due to the Exception thrown in the case that the named query couldn't be found. Added fast path to NamedQuery.lookupFrom(…) that now uses NamedQuery.hasNamedQuery(…) to test if a query exists or not. Pulled the check for the presence of a named count query from doCreateCountQuery(…) up to the NamedCountQuery constructor. This makes sure that the check is only performed once. Original pull request: #110. --- .../data/jpa/repository/query/NamedQuery.java | 28 +++++++++++++------ .../CrudMethodMetadataIntegrationTests.java | 6 ++++ .../repository/query/NamedQueryUnitTests.java | 17 ++++++----- .../JpaRepositoryFactoryUnitTests.java | 8 +++++- 4 files changed, 43 insertions(+), 16 deletions(-) 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 0829e4b05..180c80c46 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 @@ -1,5 +1,5 @@ /* - * Copyright 2008-2013 the original author or authors. + * Copyright 2008-2014 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -29,6 +29,7 @@ import org.springframework.data.repository.query.RepositoryQuery; * Implementation of {@link RepositoryQuery} based on {@link javax.persistence.NamedQuery}s. * * @author Oliver Gierke + * @author Thomas Darimont */ final class NamedQuery extends AbstractJpaQuery { @@ -43,6 +44,7 @@ final class NamedQuery extends AbstractJpaQuery { private final String countQueryName; private final String countProjection; private final QueryExtractor extractor; + private final boolean namedCountQueryIsPresent; /** * Creates a new {@link NamedQuery}. @@ -58,16 +60,14 @@ final class NamedQuery extends AbstractJpaQuery { Parameters parameters = method.getParameters(); - // Let's see if the referenced named query exists - em.createNamedQuery(queryName); - if (parameters.hasSortParameter()) { throw new IllegalStateException(String.format("Finder method %s is backed " + "by a NamedQuery and must " + "not contain a sort parameter as we cannot modify the query! Use @Query instead!", method)); } - boolean weNeedToCreateCountQuery = !hasNamedQuery(em, countQueryName) - && method.getParameters().hasPageableParameter(); + this.namedCountQueryIsPresent = hasNamedQuery(em, countQueryName); + + boolean weNeedToCreateCountQuery = !namedCountQueryIsPresent && method.getParameters().hasPageableParameter(); boolean cantExtractQuery = !this.extractor.canExtractQuery(); if (weNeedToCreateCountQuery && cantExtractQuery) { @@ -88,12 +88,20 @@ final class NamedQuery extends AbstractJpaQuery { */ private static boolean hasNamedQuery(EntityManager em, String queryName) { + /* + * @see DATAJPA-617 + * we have to use a dedicated em for the lookups to avoid a potential rollback of the running tx. + */ + EntityManager lookupEm = em.getEntityManagerFactory().createEntityManager(); + try { - em.createNamedQuery(queryName); + lookupEm.createNamedQuery(queryName); return true; } catch (IllegalArgumentException e) { LOG.debug("Did not find named query {}", queryName); return false; + } finally { + lookupEm.close(); } } @@ -109,6 +117,10 @@ final class NamedQuery extends AbstractJpaQuery { LOG.debug("Looking up named query {}", queryName); + if (!hasNamedQuery(em, queryName)) { + return null; + } + try { RepositoryQuery query = new NamedQuery(method, em); LOG.debug("Found named query {}!", queryName); @@ -139,7 +151,7 @@ final class NamedQuery extends AbstractJpaQuery { EntityManager em = getEntityManager(); TypedQuery countQuery = null; - if (hasNamedQuery(em, countQueryName)) { + if (namedCountQueryIsPresent) { countQuery = em.createNamedQuery(countQueryName, Long.class); } else { Query query = createQuery(values); diff --git a/src/test/java/org/springframework/data/jpa/repository/CrudMethodMetadataIntegrationTests.java b/src/test/java/org/springframework/data/jpa/repository/CrudMethodMetadataIntegrationTests.java index 0bb0f3f03..8fe76aa15 100644 --- a/src/test/java/org/springframework/data/jpa/repository/CrudMethodMetadataIntegrationTests.java +++ b/src/test/java/org/springframework/data/jpa/repository/CrudMethodMetadataIntegrationTests.java @@ -23,6 +23,7 @@ import java.util.Collections; import java.util.Map; import javax.persistence.EntityManager; +import javax.persistence.EntityManagerFactory; import javax.persistence.LockModeType; import javax.persistence.TypedQuery; import javax.persistence.criteria.CriteriaBuilder; @@ -44,11 +45,13 @@ import org.springframework.data.jpa.repository.support.JpaRepositoryFactory; * Integratio test for lock support. * * @author Oliver Gierke + * @author Thomas Darimont */ @RunWith(MockitoJUnitRunner.class) public class CrudMethodMetadataIntegrationTests { @Mock EntityManager em; + @Mock EntityManagerFactory emf; @Mock CriteriaBuilder builder; @Mock CriteriaQuery criteriaQuery; @Mock JpaEntityInformation information; @@ -62,6 +65,9 @@ public class CrudMethodMetadataIntegrationTests { when(information.getJavaType()).thenReturn(Role.class); + when(em.getEntityManagerFactory()).thenReturn(emf); + when(emf.createEntityManager()).thenReturn(em); + JpaRepositoryFactory factory = new JpaRepositoryFactory(em) { @Override @SuppressWarnings("unchecked") diff --git a/src/test/java/org/springframework/data/jpa/repository/query/NamedQueryUnitTests.java b/src/test/java/org/springframework/data/jpa/repository/query/NamedQueryUnitTests.java index 260a3bafa..3b6b7a1db 100644 --- a/src/test/java/org/springframework/data/jpa/repository/query/NamedQueryUnitTests.java +++ b/src/test/java/org/springframework/data/jpa/repository/query/NamedQueryUnitTests.java @@ -1,5 +1,5 @@ /* - * Copyright 2008-2011 the original author or authors. + * Copyright 2008-2014 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -21,6 +21,7 @@ import static org.mockito.Mockito.*; import java.lang.reflect.Method; import javax.persistence.EntityManager; +import javax.persistence.EntityManagerFactory; import org.junit.Before; import org.junit.Test; @@ -36,16 +37,15 @@ import org.springframework.data.repository.query.QueryCreationException; * Unit tests for {@link NamedQuery}. * * @author Oliver Gierke + * @author Thomas Darimont */ @RunWith(MockitoJUnitRunner.class) public class NamedQueryUnitTests { - @Mock - RepositoryMetadata metadata; - @Mock - QueryExtractor extractor; - @Mock - EntityManager em; + @Mock RepositoryMetadata metadata; + @Mock QueryExtractor extractor; + @Mock EntityManager em; + @Mock EntityManagerFactory emf; Method method; @@ -56,6 +56,9 @@ public class NamedQueryUnitTests { method = SampleRepository.class.getMethod("foo", Pageable.class); when(metadata.getDomainType()).thenReturn((Class) String.class); when(metadata.getReturnedDomainClass(method)).thenReturn((Class) String.class); + + when(em.getEntityManagerFactory()).thenReturn(emf); + when(emf.createEntityManager()).thenReturn(em); } @Test(expected = QueryCreationException.class) diff --git a/src/test/java/org/springframework/data/jpa/repository/support/JpaRepositoryFactoryUnitTests.java b/src/test/java/org/springframework/data/jpa/repository/support/JpaRepositoryFactoryUnitTests.java index dc342c1ff..e70767c99 100644 --- a/src/test/java/org/springframework/data/jpa/repository/support/JpaRepositoryFactoryUnitTests.java +++ b/src/test/java/org/springframework/data/jpa/repository/support/JpaRepositoryFactoryUnitTests.java @@ -1,5 +1,5 @@ /* - * Copyright 2008-2011 the original author or authors. + * Copyright 2008-2014 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -23,6 +23,7 @@ import java.io.IOException; import java.io.Serializable; import javax.persistence.EntityManager; +import javax.persistence.EntityManagerFactory; import org.junit.Before; import org.junit.Test; @@ -43,6 +44,7 @@ import org.springframework.transaction.annotation.Transactional; * Unit test for {@code JpaRepositoryFactory}. * * @author Oliver Gierke + * @author Thomas Darimont */ @RunWith(MockitoJUnitRunner.class) public class JpaRepositoryFactoryUnitTests { @@ -51,6 +53,7 @@ public class JpaRepositoryFactoryUnitTests { @Mock EntityManager entityManager; @Mock @SuppressWarnings("rawtypes") JpaEntityInformation metadata; + @Mock EntityManagerFactory emf; @Before public void setUp() { @@ -67,6 +70,9 @@ public class JpaRepositoryFactoryUnitTests { }; factory.setQueryLookupStrategyKey(Key.CREATE_IF_NOT_FOUND); + + when(entityManager.getEntityManagerFactory()).thenReturn(emf); + when(emf.createEntityManager()).thenReturn(entityManager); } /**