From adee3045a1c37d2bce66175480cabc93cd07e2b1 Mon Sep 17 00:00:00 2001 From: Thomas Darimont Date: Mon, 10 Nov 2014 16:15:41 +0100 Subject: [PATCH] DATAJPA-624 - Fix SonarQube warnings as far as possible/reasonable. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Added private constructor to Jpa21Utils to prevent instantiation. Removed unused parameter query in Querydsl.toOrderSpecifier(…). Inline currently unnecessary computeParameterValue(…) method in ParameterBinder. Return cached crudMethodMetadata instead of always creating and caching a new instance in CrudMethodMetadataPostProcessor. Removed unnecessary final modifier from DefaultCrudMethodMetadata. Introduced constant for error message in SimpleJpaRepository as well as for join alias group index in QueryUtils. Prevent instantiation of BeanDefinitionNames by making it an interface instead of a class. Reduced the usage of EvaluationContextProvider in JpaQueryLookupStrategy implementation. Removed unnecessary fixed sized set initialization in JpaQueryMethod static initializer block. Made JpaResultConverters final since it is not meant to be sub-classed. Avoid potential unsafe reference based equality check in StringQuery/hasPosition(..). Removed dead store in ClasspathScanningPersistenceUnitPostProessor.scanForMappingFileLocations. Encapsulated access to fields in EntityManagerFactoryBeanDefinition. Original pull request: #114. --- .../config/BeanDefinitionNames.java | 2 +- .../data/jpa/repository/query/Jpa21Utils.java | 4 ++ .../query/JpaQueryLookupStrategy.java | 11 +++-- .../jpa/repository/query/JpaQueryMethod.java | 2 +- .../repository/query/JpaResultConverters.java | 2 +- .../jpa/repository/query/ParameterBinder.java | 2 +- .../data/jpa/repository/query/QueryUtils.java | 7 ++- .../jpa/repository/query/StringQuery.java | 2 +- .../CrudMethodMetadataPostProcessor.java | 10 ++--- ...rBeanDefinitionRegistrarPostProcessor.java | 14 +++--- .../JpaMetamodelEntityInformation.java | 16 ++++--- .../data/jpa/repository/support/Querydsl.java | 4 +- .../support/SimpleJpaRepository.java | 10 +++-- ...hScanningPersistenceUnitPostProcessor.java | 2 +- .../data/jpa/util/BeanDefinitionUtils.java | 43 ++++++++++++++++--- 15 files changed, 89 insertions(+), 42 deletions(-) diff --git a/src/main/java/org/springframework/data/jpa/repository/config/BeanDefinitionNames.java b/src/main/java/org/springframework/data/jpa/repository/config/BeanDefinitionNames.java index 499ba44b0..2fa664a98 100644 --- a/src/main/java/org/springframework/data/jpa/repository/config/BeanDefinitionNames.java +++ b/src/main/java/org/springframework/data/jpa/repository/config/BeanDefinitionNames.java @@ -20,7 +20,7 @@ package org.springframework.data.jpa.repository.config; * * @author Oliver Gierke */ -class BeanDefinitionNames { +interface BeanDefinitionNames { public static final String JPA_MAPPING_CONTEXT_BEAN_NAME = "jpaMapppingContext"; } diff --git a/src/main/java/org/springframework/data/jpa/repository/query/Jpa21Utils.java b/src/main/java/org/springframework/data/jpa/repository/query/Jpa21Utils.java index 787c36599..d4b929d55 100644 --- a/src/main/java/org/springframework/data/jpa/repository/query/Jpa21Utils.java +++ b/src/main/java/org/springframework/data/jpa/repository/query/Jpa21Utils.java @@ -47,6 +47,10 @@ public class Jpa21Utils { } } + private Jpa21Utils() { + // prevent instantiation + } + /** * Adds a JPA 2.1 fetch-graph or load-graph hint to the given {@link Query} if running under JPA 2.1. * diff --git a/src/main/java/org/springframework/data/jpa/repository/query/JpaQueryLookupStrategy.java b/src/main/java/org/springframework/data/jpa/repository/query/JpaQueryLookupStrategy.java index 29c72b0c4..9880228c1 100644 --- a/src/main/java/org/springframework/data/jpa/repository/query/JpaQueryLookupStrategy.java +++ b/src/main/java/org/springframework/data/jpa/repository/query/JpaQueryLookupStrategy.java @@ -50,7 +50,7 @@ public final class JpaQueryLookupStrategy { private final EntityManager em; private final QueryExtractor provider; - protected final EvaluationContextProvider evaluationContextProvider; + private final EvaluationContextProvider evaluationContextProvider; /** * Creates a new {@link AbstractQueryLookupStrategy}. @@ -81,6 +81,10 @@ public final class JpaQueryLookupStrategy { } protected abstract RepositoryQuery resolveQuery(JpaQueryMethod method, EntityManager em, NamedQueries namedQueries); + + protected EvaluationContextProvider getEvaluationContextProvider() { + return evaluationContextProvider; + } } /** @@ -107,6 +111,7 @@ public final class JpaQueryLookupStrategy { method.toString()), e); } } + } /** @@ -134,7 +139,7 @@ public final class JpaQueryLookupStrategy { @Override protected RepositoryQuery resolveQuery(JpaQueryMethod method, EntityManager em, NamedQueries namedQueries) { - RepositoryQuery query = JpaQueryFactory.INSTANCE.fromQueryAnnotation(method, em, this.evaluationContextProvider); + RepositoryQuery query = JpaQueryFactory.INSTANCE.fromQueryAnnotation(method, em, getEvaluationContextProvider()); if (null != query) { return query; @@ -149,7 +154,7 @@ public final class JpaQueryLookupStrategy { String name = method.getNamedQueryName(); if (namedQueries.hasQuery(name)) { return JpaQueryFactory.INSTANCE.fromMethodWithQueryString(method, em, namedQueries.getQuery(name), - this.evaluationContextProvider); + getEvaluationContextProvider()); } query = NamedQuery.lookupFrom(method, em); diff --git a/src/main/java/org/springframework/data/jpa/repository/query/JpaQueryMethod.java b/src/main/java/org/springframework/data/jpa/repository/query/JpaQueryMethod.java index a3e89423e..da219fe6b 100644 --- a/src/main/java/org/springframework/data/jpa/repository/query/JpaQueryMethod.java +++ b/src/main/java/org/springframework/data/jpa/repository/query/JpaQueryMethod.java @@ -55,7 +55,7 @@ public class JpaQueryMethod extends QueryMethod { static { - Set> types = new HashSet>(4); + Set> types = new HashSet>(); types.add(byte[].class); types.add(Byte[].class); types.add(char[].class); diff --git a/src/main/java/org/springframework/data/jpa/repository/query/JpaResultConverters.java b/src/main/java/org/springframework/data/jpa/repository/query/JpaResultConverters.java index 8c80105c7..ac6b0653d 100644 --- a/src/main/java/org/springframework/data/jpa/repository/query/JpaResultConverters.java +++ b/src/main/java/org/springframework/data/jpa/repository/query/JpaResultConverters.java @@ -32,7 +32,7 @@ import org.springframework.util.StreamUtils; * @author Thomas Darimont * @since 1.6 */ -class JpaResultConverters { +final class JpaResultConverters { /** * {@code private} to prevent instantiation. diff --git a/src/main/java/org/springframework/data/jpa/repository/query/ParameterBinder.java b/src/main/java/org/springframework/data/jpa/repository/query/ParameterBinder.java index 370d215aa..3c104346e 100644 --- a/src/main/java/org/springframework/data/jpa/repository/query/ParameterBinder.java +++ b/src/main/java/org/springframework/data/jpa/repository/query/ParameterBinder.java @@ -106,7 +106,7 @@ public class ParameterBinder { if (canBindParameter(parameter)) { - Object value = computeParameterValue(parameter, values[methodParameterPosition], values); + Object value = values[methodParameterPosition]; bind(query, parameter, value, queryParameterPosition++); } diff --git a/src/main/java/org/springframework/data/jpa/repository/query/QueryUtils.java b/src/main/java/org/springframework/data/jpa/repository/query/QueryUtils.java index 3ed81b3c1..f48d088f0 100644 --- a/src/main/java/org/springframework/data/jpa/repository/query/QueryUtils.java +++ b/src/main/java/org/springframework/data/jpa/repository/query/QueryUtils.java @@ -92,6 +92,9 @@ public abstract class QueryUtils { private static final Map> ASSOCIATION_TYPES; + private static final int QUERY_JOIN_ALIAS_GROUP_INDEX = 3; + private static final int VARIABLE_NAME_GROUP_INDEX = 4; + static { StringBuilder builder = new StringBuilder(); @@ -254,7 +257,7 @@ public abstract class QueryUtils { while (matcher.find()) { - String alias = matcher.group(3); + String alias = matcher.group(QUERY_JOIN_ALIAS_GROUP_INDEX); if (StringUtils.hasText(alias)) { result.add(alias); } @@ -358,7 +361,7 @@ public abstract class QueryUtils { if (countProjection == null) { - String variable = matcher.matches() ? matcher.group(4) : null; + String variable = matcher.matches() ? matcher.group(VARIABLE_NAME_GROUP_INDEX) : null; boolean useVariable = variable != null && StringUtils.hasText(variable) && !variable.startsWith("new") && !variable.startsWith("count(") && !variable.contains(","); diff --git a/src/main/java/org/springframework/data/jpa/repository/query/StringQuery.java b/src/main/java/org/springframework/data/jpa/repository/query/StringQuery.java index 24603753e..7946257eb 100644 --- a/src/main/java/org/springframework/data/jpa/repository/query/StringQuery.java +++ b/src/main/java/org/springframework/data/jpa/repository/query/StringQuery.java @@ -423,7 +423,7 @@ class StringQuery { * @return */ public boolean hasPosition(Integer position) { - return position != null && this.name == null && this.position == position; + return position != null && this.name == null && position.equals(this.position); } /** diff --git a/src/main/java/org/springframework/data/jpa/repository/support/CrudMethodMetadataPostProcessor.java b/src/main/java/org/springframework/data/jpa/repository/support/CrudMethodMetadataPostProcessor.java index b5a31f884..5c524c48d 100644 --- a/src/main/java/org/springframework/data/jpa/repository/support/CrudMethodMetadataPostProcessor.java +++ b/src/main/java/org/springframework/data/jpa/repository/support/CrudMethodMetadataPostProcessor.java @@ -98,7 +98,7 @@ enum CrudMethodMetadataPostProcessor implements RepositoryProxyPostProcessor { public Object invoke(MethodInvocation invocation) throws Throwable { Method method = invocation.getMethod(); - Object metadata = TransactionSynchronizationManager.getResource(method); + CrudMethodMetadata metadata = (CrudMethodMetadata) TransactionSynchronizationManager.getResource(method); if (metadata != null) { return invocation.proceed(); @@ -112,7 +112,7 @@ enum CrudMethodMetadataPostProcessor implements RepositoryProxyPostProcessor { CrudMethodMetadata tmp = metadataCache.putIfAbsent(method, methodMetadata); if (tmp != null) { - metadata = tmp; + methodMetadata = tmp; } } @@ -152,20 +152,20 @@ enum CrudMethodMetadataPostProcessor implements RepositoryProxyPostProcessor { this.entityGraph = findEntityGraph(method); } - private static final JpaEntityGraph findEntityGraph(Method method) { + private static JpaEntityGraph findEntityGraph(Method method) { EntityGraph entityGraphAnnotation = AnnotationUtils.findAnnotation(method, EntityGraph.class); return entityGraphAnnotation == null ? null : new JpaEntityGraph(entityGraphAnnotation.value(), entityGraphAnnotation.type()); } - private static final LockModeType findLockModeType(Method method) { + private static LockModeType findLockModeType(Method method) { Lock annotation = AnnotationUtils.findAnnotation(method, Lock.class); return annotation == null ? null : (LockModeType) AnnotationUtils.getValue(annotation); } - private static final Map findQueryHints(Method method) { + private static Map findQueryHints(Method method) { Map queryHints = new HashMap(); QueryHints queryHintsAnnotation = AnnotationUtils.findAnnotation(method, QueryHints.class); diff --git a/src/main/java/org/springframework/data/jpa/repository/support/EntityManagerBeanDefinitionRegistrarPostProcessor.java b/src/main/java/org/springframework/data/jpa/repository/support/EntityManagerBeanDefinitionRegistrarPostProcessor.java index 11c5cc740..622ca8d0d 100644 --- a/src/main/java/org/springframework/data/jpa/repository/support/EntityManagerBeanDefinitionRegistrarPostProcessor.java +++ b/src/main/java/org/springframework/data/jpa/repository/support/EntityManagerBeanDefinitionRegistrarPostProcessor.java @@ -51,25 +51,25 @@ public class EntityManagerBeanDefinitionRegistrarPostProcessor implements BeanFa @Override public void postProcessBeanFactory(ConfigurableListableBeanFactory beanFactory) throws BeansException { - for (EntityManagerFactoryBeanDefinition definitions : getEntityManagerFactoryBeanDefinitions(beanFactory)) { + for (EntityManagerFactoryBeanDefinition definition : getEntityManagerFactoryBeanDefinitions(beanFactory)) { - if (!(definitions.beanFactory instanceof BeanDefinitionRegistry)) { + if (!(definition.getBeanFactory() instanceof BeanDefinitionRegistry)) { continue; } BeanDefinitionBuilder builder = BeanDefinitionBuilder .rootBeanDefinition("org.springframework.orm.jpa.SharedEntityManagerCreator"); builder.setFactoryMethod("createSharedEntityManager"); - builder.addConstructorArgReference(definitions.beanName); + builder.addConstructorArgReference(definition.getBeanName()); AbstractBeanDefinition emBeanDefinition = builder.getRawBeanDefinition(); - emBeanDefinition.addQualifier(new AutowireCandidateQualifier(Qualifier.class, definitions.beanName)); - emBeanDefinition.setScope(definitions.beanDefinition.getScope()); - emBeanDefinition.setSource(definitions.beanDefinition.getSource()); + emBeanDefinition.addQualifier(new AutowireCandidateQualifier(Qualifier.class, definition.getBeanName())); + emBeanDefinition.setScope(definition.getBeanDefinition().getScope()); + emBeanDefinition.setSource(definition.getBeanDefinition().getSource()); BeanDefinitionReaderUtils.registerWithGeneratedName(emBeanDefinition, - (BeanDefinitionRegistry) definitions.beanFactory); + (BeanDefinitionRegistry) definition.getBeanFactory()); } } } diff --git a/src/main/java/org/springframework/data/jpa/repository/support/JpaMetamodelEntityInformation.java b/src/main/java/org/springframework/data/jpa/repository/support/JpaMetamodelEntityInformation.java index 531d7dd4c..a2e6586be 100644 --- a/src/main/java/org/springframework/data/jpa/repository/support/JpaMetamodelEntityInformation.java +++ b/src/main/java/org/springframework/data/jpa/repository/support/JpaMetamodelEntityInformation.java @@ -207,6 +207,7 @@ public class JpaMetamodelEntityInformation extends J * Simple value object to encapsulate id specific metadata. * * @author Oliver Gierke + * @author Thomas Darimont */ private static class IdMetadata implements Iterable> { @@ -232,18 +233,21 @@ public class JpaMetamodelEntityInformation extends J return idType; } - Class idType; + // lazy initialization of idType field with tolerable benign data-race + this.idType = tryExtractIdTypeWithFallbackToIdTypeLookup(); + + return this.idType; + } + + private Class tryExtractIdTypeWithFallbackToIdTypeLookup() { try { Type idType2 = type.getIdType(); - idType = idType2 == null ? fallbackIdTypeLookup(type) : idType2.getJavaType(); + return idType2 == null ? fallbackIdTypeLookup(type) : idType2.getJavaType(); } catch (IllegalStateException e) { // see https://hibernate.onjira.com/browse/HHH-6951 - idType = fallbackIdTypeLookup(type); + return fallbackIdTypeLookup(type); } - - this.idType = idType; - return idType; } private static Class fallbackIdTypeLookup(IdentifiableType type) { diff --git a/src/main/java/org/springframework/data/jpa/repository/support/Querydsl.java b/src/main/java/org/springframework/data/jpa/repository/support/Querydsl.java index dd920481c..c46f153ef 100644 --- a/src/main/java/org/springframework/data/jpa/repository/support/Querydsl.java +++ b/src/main/java/org/springframework/data/jpa/repository/support/Querydsl.java @@ -164,7 +164,7 @@ public class Querydsl { Assert.notNull(query, "Query must not be null!"); for (Order order : sort) { - query.orderBy(toOrderSpecifier(order, query)); + query.orderBy(toOrderSpecifier(order)); } return query; @@ -177,7 +177,7 @@ public class Querydsl { * @return */ @SuppressWarnings({ "rawtypes", "unchecked" }) - private OrderSpecifier toOrderSpecifier(Order order, JPQLQuery query) { + private OrderSpecifier toOrderSpecifier(Order order) { return new OrderSpecifier(order.isAscending() ? com.mysema.query.types.Order.ASC : com.mysema.query.types.Order.DESC, buildOrderPropertyPathFrom(order), diff --git a/src/main/java/org/springframework/data/jpa/repository/support/SimpleJpaRepository.java b/src/main/java/org/springframework/data/jpa/repository/support/SimpleJpaRepository.java index e289ebd3c..e90f4bcd4 100644 --- a/src/main/java/org/springframework/data/jpa/repository/support/SimpleJpaRepository.java +++ b/src/main/java/org/springframework/data/jpa/repository/support/SimpleJpaRepository.java @@ -65,6 +65,8 @@ import org.springframework.util.Assert; public class SimpleJpaRepository implements JpaRepository, JpaSpecificationExecutor { + private static final String ID_MUST_NOT_BE_NULL = "The given id must not be null!"; + private final JpaEntityInformation entityInformation; private final EntityManager em; private final PersistenceProvider provider; @@ -132,7 +134,7 @@ public class SimpleJpaRepository implements JpaRepos @Transactional public void delete(ID id) { - Assert.notNull(id, "The given id must not be null!"); + Assert.notNull(id, ID_MUST_NOT_BE_NULL); T entity = findOne(id); @@ -213,7 +215,7 @@ public class SimpleJpaRepository implements JpaRepos */ public T findOne(ID id) { - Assert.notNull(id, "The given id must not be null!"); + Assert.notNull(id, ID_MUST_NOT_BE_NULL); Class domainType = getDomainClass(); @@ -234,7 +236,7 @@ public class SimpleJpaRepository implements JpaRepos @Override public T getOne(ID id) { - Assert.notNull(id, "The given id must not be null!"); + Assert.notNull(id, ID_MUST_NOT_BE_NULL); return em.getReference(getDomainClass(), id); } @@ -244,7 +246,7 @@ public class SimpleJpaRepository implements JpaRepos */ public boolean exists(ID id) { - Assert.notNull(id, "The given id must not be null!"); + Assert.notNull(id, ID_MUST_NOT_BE_NULL); if (entityInformation.getIdAttribute() == null) { return findOne(id) != null; diff --git a/src/main/java/org/springframework/data/jpa/support/ClasspathScanningPersistenceUnitPostProcessor.java b/src/main/java/org/springframework/data/jpa/support/ClasspathScanningPersistenceUnitPostProcessor.java index 9e92d94e6..0ab2a8f4d 100644 --- a/src/main/java/org/springframework/data/jpa/support/ClasspathScanningPersistenceUnitPostProcessor.java +++ b/src/main/java/org/springframework/data/jpa/support/ClasspathScanningPersistenceUnitPostProcessor.java @@ -134,7 +134,7 @@ public class ClasspathScanningPersistenceUnitPostProcessor implements Persistenc String path = ResourcePatternResolver.CLASSPATH_ALL_URL_PREFIX + basePackagePathComponent + slash + mappingFileNamePattern; Set mappingFileUris = new HashSet(); - Resource[] scannedResources = new Resource[0]; + Resource[] scannedResources; try { scannedResources = resolver.getResources(path); diff --git a/src/main/java/org/springframework/data/jpa/util/BeanDefinitionUtils.java b/src/main/java/org/springframework/data/jpa/util/BeanDefinitionUtils.java index 98e29636d..9d8f5606b 100644 --- a/src/main/java/org/springframework/data/jpa/util/BeanDefinitionUtils.java +++ b/src/main/java/org/springframework/data/jpa/util/BeanDefinitionUtils.java @@ -83,10 +83,10 @@ public class BeanDefinitionUtils { String transformedName = transformedBeanName(name); - EntityManagerFactoryBeanDefinition definition = new EntityManagerFactoryBeanDefinition(); - definition.beanName = transformedName; - definition.beanFactory = beanFactory; - definition.beanDefinition = beanFactory.getBeanDefinition(transformedName); + EntityManagerFactoryBeanDefinition definition = new EntityManagerFactoryBeanDefinition( // + transformedName, // + beanFactory, // + beanFactory.getBeanDefinition(transformedName)); definitions.add(definition); } @@ -125,10 +125,39 @@ public class BeanDefinitionUtils { } } + /** + * @author Oliver Gierke + * @author Thomas Darimont + */ public static class EntityManagerFactoryBeanDefinition { - public String beanName; - public BeanDefinition beanDefinition; - public BeanFactory beanFactory; + private final String beanName; + private final BeanFactory beanFactory; + private final BeanDefinition beanDefinition; + + /** + * Creates a new {@link EntityManagerFactoryBeanDefinition}. + * + * @param beanName + * @param beanFactory + * @param beanDefinition + */ + public EntityManagerFactoryBeanDefinition(String beanName, BeanFactory beanFactory, BeanDefinition beanDefinition) { + this.beanName = beanName; + this.beanFactory = beanFactory; + this.beanDefinition = beanDefinition; + } + + public String getBeanName() { + return beanName; + } + + public BeanFactory getBeanFactory() { + return beanFactory; + } + + public BeanDefinition getBeanDefinition() { + return beanDefinition; + } } }