From 6b61d2b0269804ab1bba0938d48e84fcf11e459b Mon Sep 17 00:00:00 2001 From: Oliver Gierke Date: Mon, 15 Jan 2018 15:08:45 +0100 Subject: [PATCH] DATAREST-1176 - Polishing. Moved the flag to decide whether to expose repository methods by default to the RepositoryDetectionStrategy interface, so that it can be directly accessed and the test on the particular enum value is not needed anymore and thus also not duplicated into different parts of the codebase. Added more tests to actually verify behavior on CrudMethodsSupportedHttpMethods. DefaultExposureAwareCrudMethods uses @RequiredArgumentConstructor again. --- .../CrudMethodsSupportedHttpMethods.java | 16 ++++----- .../RepositoryAwareResourceMetadata.java | 3 +- .../mapping/RepositoryDetectionStrategy.java | 30 +++++++++++++++-- .../RepositoryMethodResourceMapping.java | 15 +++------ .../mapping/RepositoryResourceMappings.java | 18 +++++++--- ...dMethodsSupportedHttpMethodsUnitTests.java | 33 +++++++++++++++++-- ...epositoryDetectionStrategiesUnitTests.java | 4 ++- ...ositoryMethodResourceMappingUnitTests.java | 7 ++-- 8 files changed, 88 insertions(+), 38 deletions(-) diff --git a/spring-data-rest-core/src/main/java/org/springframework/data/rest/core/mapping/CrudMethodsSupportedHttpMethods.java b/spring-data-rest-core/src/main/java/org/springframework/data/rest/core/mapping/CrudMethodsSupportedHttpMethods.java index 62c400036..b649b7f1f 100644 --- a/spring-data-rest-core/src/main/java/org/springframework/data/rest/core/mapping/CrudMethodsSupportedHttpMethods.java +++ b/spring-data-rest-core/src/main/java/org/springframework/data/rest/core/mapping/CrudMethodsSupportedHttpMethods.java @@ -19,6 +19,7 @@ import static org.springframework.data.rest.core.mapping.ResourceType.*; import static org.springframework.http.HttpMethod.*; import lombok.NonNull; +import lombok.RequiredArgsConstructor; import java.lang.reflect.Method; import java.util.Collections; @@ -47,15 +48,14 @@ public class CrudMethodsSupportedHttpMethods implements SupportedHttpMethods { * Creates a new {@link CrudMethodsSupportedHttpMethods} for the given {@link CrudMethods}. * * @param crudMethods must not be {@literal null}. + * @param methodsExposedByDefault whether repository methods should be considered exposed by default or need to be + * annotated with {@link RestResource} to really be visible. */ - public CrudMethodsSupportedHttpMethods(CrudMethods crudMethods, RepositoryResourceMappings provider) { + public CrudMethodsSupportedHttpMethods(CrudMethods crudMethods, boolean methodsExposedByDefault) { Assert.notNull(crudMethods, "CrudMethods must not be null!"); - boolean exportedDefault = provider.getRepositoryDetectionStrategy() - != RepositoryDetectionStrategy.RepositoryDetectionStrategies.EXPLICIT_METHOD_ANNOTATED; - - this.exposedMethods = new DefaultExposureAwareCrudMethods(crudMethods, exportedDefault); + this.exposedMethods = new DefaultExposureAwareCrudMethods(crudMethods, methodsExposedByDefault); } /* @@ -141,16 +141,12 @@ public class CrudMethodsSupportedHttpMethods implements SupportedHttpMethods { /** * @author Oliver Gierke */ + @RequiredArgsConstructor private static class DefaultExposureAwareCrudMethods implements ExposureAwareCrudMethods { private final @NonNull CrudMethods crudMethods; private final boolean exportedDefault; - DefaultExposureAwareCrudMethods(CrudMethods crudMethods, boolean exportedDefault) { - this.crudMethods = crudMethods; - this.exportedDefault = exportedDefault; - } - /* * (non-Javadoc) * @see org.springframework.data.rest.core.mapping.ExposureAwareCrudMethods#exposesSave() diff --git a/spring-data-rest-core/src/main/java/org/springframework/data/rest/core/mapping/RepositoryAwareResourceMetadata.java b/spring-data-rest-core/src/main/java/org/springframework/data/rest/core/mapping/RepositoryAwareResourceMetadata.java index 4bb9c7d37..a1b1173f4 100644 --- a/spring-data-rest-core/src/main/java/org/springframework/data/rest/core/mapping/RepositoryAwareResourceMetadata.java +++ b/spring-data-rest-core/src/main/java/org/springframework/data/rest/core/mapping/RepositoryAwareResourceMetadata.java @@ -57,7 +57,8 @@ class RepositoryAwareResourceMetadata implements ResourceMetadata { this.mapping = mapping; this.provider = provider; this.repositoryMetadata = repositoryMetadata; - this.crudMethodsSupportedHttpMethods = new CrudMethodsSupportedHttpMethods(repositoryMetadata.getCrudMethods(), provider); + this.crudMethodsSupportedHttpMethods = new CrudMethodsSupportedHttpMethods(repositoryMetadata.getCrudMethods(), + provider.exposeMethodsByDefault()); } /** diff --git a/spring-data-rest-core/src/main/java/org/springframework/data/rest/core/mapping/RepositoryDetectionStrategy.java b/spring-data-rest-core/src/main/java/org/springframework/data/rest/core/mapping/RepositoryDetectionStrategy.java index 7209b1442..049b735f3 100644 --- a/spring-data-rest-core/src/main/java/org/springframework/data/rest/core/mapping/RepositoryDetectionStrategy.java +++ b/spring-data-rest-core/src/main/java/org/springframework/data/rest/core/mapping/RepositoryDetectionStrategy.java @@ -26,6 +26,7 @@ import org.springframework.data.rest.core.annotation.RestResource; * The strategy to determine whether a given repository is to be exported by Spring Data REST. * * @author Oliver Gierke + * @author Tobias Weiß * @since 2.5 * @soundtrack Katinka - Ausverkauf */ @@ -39,6 +40,17 @@ public interface RepositoryDetectionStrategy { */ boolean isExported(RepositoryMetadata metadata); + /** + * Returns whether to expose repository methods by default, i.e. without the need to explicitly annotate them with + * {@link RestResource}. + * + * @return + * @since 3.1 + */ + default boolean exposeMethodsByDefault() { + return true; + } + /** * A variety of strategies to determine repository exposure. * @@ -99,15 +111,27 @@ public interface RepositoryDetectionStrategy { }, /** - * Behaves like the annotated strategy on repository level. But it does not export all methods - * of an exported Repository. The methods have to be annotated explicitly too. + * Behaves like the {@link RepositoryDetectionStrategies#ANNOTATED} strategy on repository level, but only exports + * the methods of the repository that have been explicitly annotated with {@link RestResource}. CRUD methods need to + * be annotated, too, for the default collection resource exposure to be applied. + * + * @since 3.1 */ - EXPLICIT_METHOD_ANNOTATED { + EXPLICITLY_ANNOTATED { @Override public boolean isExported(RepositoryMetadata metadata) { return isExplicitlyExported(metadata.getRepositoryInterface(), false); } + + /* + * (non-Javadoc) + * @see org.springframework.data.rest.core.mapping.RepositoryDetectionStrategy#exposeMethodsByDefault() + */ + @Override + public boolean exposeMethodsByDefault() { + return false; + } }; /** diff --git a/spring-data-rest-core/src/main/java/org/springframework/data/rest/core/mapping/RepositoryMethodResourceMapping.java b/spring-data-rest-core/src/main/java/org/springframework/data/rest/core/mapping/RepositoryMethodResourceMapping.java index 9fdb368d5..3e867ea23 100644 --- a/spring-data-rest-core/src/main/java/org/springframework/data/rest/core/mapping/RepositoryMethodResourceMapping.java +++ b/spring-data-rest-core/src/main/java/org/springframework/data/rest/core/mapping/RepositoryMethodResourceMapping.java @@ -42,7 +42,6 @@ import org.springframework.util.StringUtils; */ class RepositoryMethodResourceMapping implements MethodResourceMapping { - @SuppressWarnings("unchecked") // private static final Collection> IMPLICIT_PARAMETER_TYPES = Arrays.asList(Pageable.class, Sort.class); private static final AnnotationAttribute PARAM_VALUE = new AnnotationAttribute(Param.class); @@ -61,9 +60,11 @@ class RepositoryMethodResourceMapping implements MethodResourceMapping { * * @param method must not be {@literal null}. * @param resourceMapping must not be {@literal null}. + * @param metadata can be {@literal null}. + * @param whether the methods are supposed to be exported by default. */ public RepositoryMethodResourceMapping(Method method, ResourceMapping resourceMapping, RepositoryMetadata metadata, - RepositoryDetectionStrategy strategy) { + boolean exposeMethodsByDefault) { Assert.notNull(method, "Method must not be null!"); Assert.notNull(resourceMapping, "ResourceMapping must not be null!"); @@ -71,7 +72,7 @@ class RepositoryMethodResourceMapping implements MethodResourceMapping { RestResource annotation = AnnotationUtils.findAnnotation(method, RestResource.class); String resourceRel = resourceMapping.getRel(); - this.isExported = determineIsExported(strategy, annotation); + this.isExported = annotation != null ? annotation.exported() : exposeMethodsByDefault; this.rel = annotation == null || !StringUtils.hasText(annotation.rel()) ? method.getName() : annotation.rel(); this.path = annotation == null || !StringUtils.hasText(annotation.path()) ? new Path(method.getName()) : new Path(annotation.path()); @@ -85,14 +86,6 @@ class RepositoryMethodResourceMapping implements MethodResourceMapping { this.metadata = metadata; } - private boolean determineIsExported(RepositoryDetectionStrategy strategy, RestResource annotation) { - - boolean exportedDefault = strategy - != RepositoryDetectionStrategy.RepositoryDetectionStrategies.EXPLICIT_METHOD_ANNOTATED; - - return annotation != null ? annotation.exported() : exportedDefault; - } - private static final List discoverParameterMetadata(Method method, String baseRel) { List result = new ArrayList(); diff --git a/spring-data-rest-core/src/main/java/org/springframework/data/rest/core/mapping/RepositoryResourceMappings.java b/spring-data-rest-core/src/main/java/org/springframework/data/rest/core/mapping/RepositoryResourceMappings.java index 1ceb3123c..6f2ce6499 100644 --- a/spring-data-rest-core/src/main/java/org/springframework/data/rest/core/mapping/RepositoryResourceMappings.java +++ b/spring-data-rest-core/src/main/java/org/springframework/data/rest/core/mapping/RepositoryResourceMappings.java @@ -26,6 +26,7 @@ import org.springframework.data.mapping.PersistentProperty; import org.springframework.data.mapping.context.PersistentEntities; import org.springframework.data.repository.core.RepositoryInformation; import org.springframework.data.repository.support.Repositories; +import org.springframework.data.rest.core.annotation.RestResource; import org.springframework.data.rest.core.config.RepositoryRestConfiguration; import org.springframework.hateoas.RelProvider; import org.springframework.hateoas.core.EvoInflectorRelProvider; @@ -65,8 +66,8 @@ public class RepositoryResourceMappings extends PersistentEntitiesResourceMappin * @param strategy must not be {@literal null}. * @param relProvider must not be {@literal null}. */ - public RepositoryResourceMappings(Repositories repositories, PersistentEntities entities, RepositoryDetectionStrategy strategy, - RelProvider relProvider) { + public RepositoryResourceMappings(Repositories repositories, PersistentEntities entities, + RepositoryDetectionStrategy strategy, RelProvider relProvider) { super(entities); @@ -120,7 +121,7 @@ public class RepositoryResourceMappings extends PersistentEntitiesResourceMappin if (resourceMapping.isExported()) { for (Method queryMethod : repositoryInformation.getQueryMethods()) { RepositoryMethodResourceMapping methodMapping = new RepositoryMethodResourceMapping(queryMethod, - resourceMapping, repositoryInformation, strategy); + resourceMapping, repositoryInformation, exposeMethodsByDefault()); if (methodMapping.isExported()) { mappings.add(methodMapping); } @@ -159,7 +160,14 @@ public class RepositoryResourceMappings extends PersistentEntitiesResourceMappin return repositories.hasRepositoryFor(property.getActualType()) && super.isMapped(property); } - public RepositoryDetectionStrategy getRepositoryDetectionStrategy() { - return strategy; + /** + * Returns whether to expose repository methods by default, i.e. without the need to explicitly annotate them with + * {@link RestResource}. + * + * @since 3.1 + * @see RepositoryDetectionStrategy#exposeMethodsByDefault() + */ + public boolean exposeMethodsByDefault() { + return strategy.exposeMethodsByDefault(); } } diff --git a/spring-data-rest-core/src/test/java/org/springframework/data/rest/core/mapping/CrudMethodsSupportedHttpMethodsUnitTests.java b/spring-data-rest-core/src/test/java/org/springframework/data/rest/core/mapping/CrudMethodsSupportedHttpMethodsUnitTests.java index a7f9c637a..d39f42c89 100755 --- a/spring-data-rest-core/src/test/java/org/springframework/data/rest/core/mapping/CrudMethodsSupportedHttpMethodsUnitTests.java +++ b/spring-data-rest-core/src/test/java/org/springframework/data/rest/core/mapping/CrudMethodsSupportedHttpMethodsUnitTests.java @@ -16,6 +16,7 @@ package org.springframework.data.rest.core.mapping; import static org.assertj.core.api.Assertions.*; +import static org.mockito.Mockito.*; import static org.springframework.data.rest.core.mapping.ResourceType.*; import static org.springframework.http.HttpMethod.*; @@ -23,6 +24,7 @@ import java.util.List; import java.util.Optional; import java.util.Set; +import org.junit.Before; import org.junit.Test; import org.junit.runner.RunWith; import org.mockito.Mock; @@ -48,8 +50,12 @@ import org.springframework.http.HttpMethod; @RunWith(MockitoJUnitRunner.class) public class CrudMethodsSupportedHttpMethodsUnitTests { - @Mock - private RepositoryResourceMappings mappings; + @Mock RepositoryResourceMappings mappings; + + @Before + public void setUp() { + when(mappings.exposeMethodsByDefault()).thenReturn(true); + } @Test // DATACMNS-589, DATAREST-409 public void doesNotSupportAnyHttpMethodForEmptyRepository() { @@ -122,12 +128,24 @@ public class CrudMethodsSupportedHttpMethodsUnitTests { assertMethodsSupported(getSupportedHttpMethodsFor(NoFindOne.class), ITEM, false, DELETE); } + @Test // DATAREST-1176 + public void onlyExposesExplicitlyAnnotatedMethodsIfConfigured() { + + reset(mappings); + when(mappings.exposeMethodsByDefault()).thenReturn(false); + + assertMethodsSupported(getSupportedHttpMethodsFor(MethodsExplicitlyExportedRepository.class), COLLECTION, true, + POST, OPTIONS); + assertMethodsSupported(getSupportedHttpMethodsFor(MethodsExplicitlyExportedRepository.class), ITEM, true, OPTIONS, + PUT, PATCH); + } + private SupportedHttpMethods getSupportedHttpMethodsFor(Class repositoryInterface) { RepositoryMetadata metadata = new DefaultRepositoryMetadata(repositoryInterface); CrudMethods crudMethods = new DefaultCrudMethods(metadata); - return new CrudMethodsSupportedHttpMethods(crudMethods, mappings); + return new CrudMethodsSupportedHttpMethods(crudMethods, mappings.exposeMethodsByDefault()); } private static void assertMethodsSupported(SupportedHttpMethods methods, ResourceType type, boolean supported, @@ -165,6 +183,15 @@ public class CrudMethodsSupportedHttpMethodsUnitTests { interface EntityRepository extends CrudRepository {} + interface MethodsExplicitlyExportedRepository extends Repository { + + @RestResource + S save(S entity); + + @RestResource + void delete(Object entity); + } + class Entity { Entity embedded; diff --git a/spring-data-rest-core/src/test/java/org/springframework/data/rest/core/mapping/RepositoryDetectionStrategiesUnitTests.java b/spring-data-rest-core/src/test/java/org/springframework/data/rest/core/mapping/RepositoryDetectionStrategiesUnitTests.java index b270b3134..c5c285438 100755 --- a/spring-data-rest-core/src/test/java/org/springframework/data/rest/core/mapping/RepositoryDetectionStrategiesUnitTests.java +++ b/spring-data-rest-core/src/test/java/org/springframework/data/rest/core/mapping/RepositoryDetectionStrategiesUnitTests.java @@ -92,7 +92,7 @@ public class RepositoryDetectionStrategiesUnitTests { @Test // DATAREST-1176 public void onlyExplicitAnnotatedMethodsAreExposed() { - assertExposures(EXPLICIT_METHOD_ANNOTATED, new HashMap, Boolean>() { + assertExposures(EXPLICITLY_ANNOTATED, new HashMap, Boolean>() { { put(AnnotatedRepository.class, true); put(HiddenRepository.class, false); @@ -100,6 +100,8 @@ public class RepositoryDetectionStrategiesUnitTests { put(PackageProtectedRepository.class, false); } }); + + assertThat(EXPLICITLY_ANNOTATED.exposeMethodsByDefault()).isFalse(); } private static void assertExposures(RepositoryDetectionStrategy strategy, Map, Boolean> expected) { diff --git a/spring-data-rest-core/src/test/java/org/springframework/data/rest/core/mapping/RepositoryMethodResourceMappingUnitTests.java b/spring-data-rest-core/src/test/java/org/springframework/data/rest/core/mapping/RepositoryMethodResourceMappingUnitTests.java index 425579469..d4cf1cc0e 100755 --- a/spring-data-rest-core/src/test/java/org/springframework/data/rest/core/mapping/RepositoryMethodResourceMappingUnitTests.java +++ b/spring-data-rest-core/src/test/java/org/springframework/data/rest/core/mapping/RepositoryMethodResourceMappingUnitTests.java @@ -38,9 +38,9 @@ import org.springframework.data.rest.core.mapping.RepositoryDetectionStrategy.Re */ public class RepositoryMethodResourceMappingUnitTests { + RepositoryDetectionStrategy strategy = RepositoryDetectionStrategies.DEFAULT; RepositoryMetadata metadata = new DefaultRepositoryMetadata(PersonRepository.class); - RepositoryCollectionResourceMapping resourceMapping = new RepositoryCollectionResourceMapping(metadata, - RepositoryDetectionStrategies.DEFAULT); + RepositoryCollectionResourceMapping resourceMapping = new RepositoryCollectionResourceMapping(metadata, strategy); @Test public void defaultsMappingToMethodName() throws Exception { @@ -131,8 +131,7 @@ public class RepositoryMethodResourceMappingUnitTests { } private RepositoryMethodResourceMapping getMappingFor(Method method) { - RepositoryDetectionStrategy strategy = RepositoryDetectionStrategies.DEFAULT; - return new RepositoryMethodResourceMapping(method, resourceMapping, metadata, strategy); + return new RepositoryMethodResourceMapping(method, resourceMapping, metadata, strategy.exposeMethodsByDefault()); } static class Person {}