Skip to content

Commit

Permalink
Block clas-level request mapping on Feign clients.
Browse files Browse the repository at this point in the history
  • Loading branch information
@OlgaMaciaszek
OlgaMaciaszek committed Oct 21, 2021
1 parent 1275875 commit d6783a6
Show file tree
Hide file tree
Showing 4 changed files with 41 additions and 117 deletions.
20 changes: 3 additions & 17 deletions README.adoc
View file
Expand Up @@ -66,23 +66,9 @@ the `.mvn` configuration, so if you find you have to do it to make a
build succeed, please raise a ticket to get the settings added to
source control.

For hints on how to build the project look in `.travis.yml` if there
is one. There should be a "script" and maybe "install" command. Also
look at the "services" section to see if any services need to be
running locally (e.g. mongo or rabbit). Ignore the git-related bits
that you might find in "before_install" since they're related to setting git
credentials and you already have those.

The projects that require middleware generally include a
`docker-compose.yml`, so consider using
https://docs.docker.com/compose/[Docker Compose] to run the middeware servers
in Docker containers. See the README in the
https://github.com/spring-cloud-samples/scripts[scripts demo
repository] for specific instructions about the common cases of mongo,
rabbit and redis.

NOTE: If all else fails, build with the command from `.travis.yml` (usually
`./mvnw install`).
The projects that require middleware (i.e. Redis) for testing generally
require that a local instance of [Docker](https://www.docker.com/get-started) is installed and running.


=== Documentation

Expand Down
5 changes: 1 addition & 4 deletions docs/src/main/asciidoc/spring-cloud-openfeign.adoc
View file
Expand Up @@ -523,10 +523,7 @@ public interface UserClient extends UserService {
}
----

NOTE: It is generally not advisable to share an interface between a
server and a client. It introduces tight coupling, and also actually
doesn't work with Spring MVC in its current form (method parameter
mapping is not inherited).
WARNING: `@FeignClient` interfaces should not be shared between server and client and annotating `@FeignClient` interfaces with `@RequestMapping` on class level is no longer supported.

=== Feign request/response compression

Expand Down
43 changes: 9 additions & 34 deletions ...ign-core/src/main/java/org/springframework/cloud/openfeign/support/SpringMvcContract.java
View file
Expand Up @@ -35,6 +35,8 @@
import feign.MethodMetadata;
import feign.Param;
import feign.Request;
import org.apache.commons.logging.Log;
import org.apache.commons.logging.LogFactory;

import org.springframework.cloud.openfeign.AnnotatedParameterProcessor;
import org.springframework.cloud.openfeign.CollectionFormat;
Expand Down Expand Up @@ -83,6 +85,8 @@
public class SpringMvcContract extends Contract.BaseContract
implements ResourceLoaderAware {

private static final Log LOG = LogFactory.getLog(SpringMvcContract.class);

private static final String ACCEPT = "Accept";

private static final String CONTENT_TYPE = "Content-Type";
Expand Down Expand Up @@ -181,49 +185,20 @@ public void setResourceLoader(ResourceLoader resourceLoader) {

@Override
protected void processAnnotationOnClass(MethodMetadata data, Class<?> clz) {
if (clz.getInterfaces().length == 0) {
RequestMapping classAnnotation = findMergedAnnotation(clz,
RequestMapping.class);
if (classAnnotation != null) {
// Prepend path from class annotation if specified
if (classAnnotation.value().length > 0) {
String pathValue = emptyToNull(classAnnotation.value()[0]);
pathValue = resolve(pathValue);
if (!pathValue.startsWith("/")) {
pathValue = "/" + pathValue;
}
data.template().uri(pathValue);
if (data.template().decodeSlash() != decodeSlash) {
data.template().decodeSlash(decodeSlash);
}
}
LOG.error("Cannot process class: " + clz.getName()
+ ". @RequestMapping annotation is not allowed on @FeignClient interfaces.");
throw new IllegalArgumentException(
"@RequestMapping annotation not allowed on @FeignClient interfaces");
}
}
}

@Override
public MethodMetadata parseAndValidateMetadata(Class<?> targetType, Method method) {
processedMethods.put(Feign.configKey(targetType, method), method);
MethodMetadata md = super.parseAndValidateMetadata(targetType, method);

RequestMapping classAnnotation = findMergedAnnotation(targetType,
RequestMapping.class);
if (classAnnotation != null) {
// produces - use from class annotation only if method has not specified this
if (!md.template().headers().containsKey(ACCEPT)) {
parseProduces(md, method, classAnnotation);
}

// consumes -- use from class annotation only if method has not specified this
if (!md.template().headers().containsKey(CONTENT_TYPE)) {
parseConsumes(md, method, classAnnotation);
}

// headers -- class annotation is inherited to methods, always write these if
// present
parseHeaders(md, method, classAnnotation);
}
return md;
return super.parseAndValidateMetadata(targetType, method);
}

@Override
Expand Down
90 changes: 28 additions & 62 deletions ...ore/src/test/java/org/springframework/cloud/openfeign/support/SpringMvcContractTests.java
View file
Expand Up @@ -62,6 +62,7 @@
import static com.fasterxml.jackson.annotation.JsonAutoDetect.Visibility.NONE;
import static feign.CollectionFormat.SSV;
import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.assertThatIllegalArgumentException;
import static org.junit.Assume.assumeTrue;

/**
Expand Down Expand Up @@ -182,51 +183,30 @@ public void testProcessAnnotations_SimpleGetMapping() throws Exception {
}

@Test
public void testProcessAnnotations_Class_AnnotationsGetSpecificTest()
throws Exception {
Method method = TestTemplate_Class_Annotations.class
.getDeclaredMethod("getSpecificTest", String.class, String.class);
MethodMetadata data = contract
.parseAndValidateMetadata(method.getDeclaringClass(), method);

assertThat(data.template().url()).isEqualTo("/prepend/{classId}/test/{testId}");
assertThat(data.template().method()).isEqualTo("GET");

assertThat(data.indexToName().get(0).iterator().next()).isEqualTo("classId");
assertThat(data.indexToName().get(1).iterator().next()).isEqualTo("testId");
public void testProcessAnnotations_Class_Annotations_RequestMapping() {
assertThatIllegalArgumentException()
.isThrownBy(() -> {
Method method = TestTemplate_Class_RequestMapping.class
.getDeclaredMethod("getSpecificTest", String.class, String.class);
contract.parseAndValidateMetadata(method.getDeclaringClass(), method);
});
}

@Test
public void testProcessAnnotations_Class_AnnotationsGetAllTests() throws Exception {
Method method = TestTemplate_Class_Annotations.class
.getDeclaredMethod("getAllTests", String.class);
.getDeclaredMethod("getAllTests", String.class);
MethodMetadata data = contract
.parseAndValidateMetadata(method.getDeclaringClass(), method);
.parseAndValidateMetadata(method.getDeclaringClass(), method);

assertThat(data.template().url()).isEqualTo("/prepend/{classId}");
assertThat(data.template().url()).isEqualTo("/");
assertThat(data.template().method()).isEqualTo("GET");

assertThat(data.indexToName().get(0).iterator().next()).isEqualTo("classId");

assertThat(data.template().decodeSlash()).isTrue();
}

@Test
public void testProcessAnnotations_Class_AnnotationsGetAllTests_EncodeSlash()
throws Exception {
contract = new SpringMvcContract(Collections.emptyList(), getConversionService(),
false);

Method method = TestTemplate_Class_Annotations.class
.getDeclaredMethod("getAllTests", String.class);
MethodMetadata data = contract
.parseAndValidateMetadata(method.getDeclaringClass(), method);

assertThat(data.template().url()).isEqualTo("/prepend/{classId}");

assertThat(data.template().decodeSlash()).isFalse();
}

@Test
public void testProcessAnnotations_ExtendedInterface() throws Exception {
Method extendedMethod = TestTemplate_Extended.class.getMethod("getAllTests",
Expand All @@ -249,27 +229,6 @@ public void testProcessAnnotations_ExtendedInterface() throws Exception {
assertThat(data.template().decodeSlash()).isTrue();
}

@Test
public void testProcessAnnotations_ExtendedInterface_EncodeSlash() throws Exception {
contract = new SpringMvcContract(Collections.emptyList(), getConversionService(),
false);

Method extendedMethod = TestTemplate_Extended.class.getMethod("getAllTests",
String.class);
MethodMetadata extendedData = contract.parseAndValidateMetadata(
extendedMethod.getDeclaringClass(), extendedMethod);

Method method = TestTemplate_Class_Annotations.class
.getDeclaredMethod("getAllTests", String.class);
MethodMetadata data = contract
.parseAndValidateMetadata(method.getDeclaringClass(), method);

assertThat(data.template().url()).isEqualTo(extendedData.template().url());
assertThat(data.template().method()).isEqualTo(extendedData.template().method());

assertThat(data.template().decodeSlash()).isFalse();
}

@Test
public void testProcessAnnotations_SimplePost() throws Exception {
Method method = TestTemplate_Simple.class.getDeclaredMethod("postTest",
Expand Down Expand Up @@ -306,7 +265,7 @@ public void testProcessAnnotationsOnMethod_Advanced() throws Exception {
.parseAndValidateMetadata(method.getDeclaringClass(), method);

assertThat(data.template().url())
.isEqualTo("/advanced/test/{id}?amount=" + "{amount}");
.isEqualTo("/test/{id}?amount=" + "{amount}");
assertThat(data.template().method()).isEqualTo("PUT");
assertThat(data.template().headers().get("Accept").iterator().next())
.isEqualTo(MediaType.APPLICATION_JSON_VALUE);
Expand Down Expand Up @@ -342,7 +301,7 @@ public void testProcessAnnotations_Advanced() throws Exception {
.parseAndValidateMetadata(method.getDeclaringClass(), method);

assertThat(data.template().url())
.isEqualTo("/advanced/test/{id}?amount=" + "{amount}");
.isEqualTo("/test/{id}?amount=" + "{amount}");
assertThat(data.template().method()).isEqualTo("PUT");
assertThat(data.template().headers().get("Accept").iterator().next())
.isEqualTo(MediaType.APPLICATION_JSON_VALUE);
Expand All @@ -367,7 +326,7 @@ public void testProcessAnnotations_Aliased() throws Exception {
.parseAndValidateMetadata(method.getDeclaringClass(), method);

assertThat(data.template().url())
.isEqualTo("/advanced/test2?amount=" + "{amount}");
.isEqualTo("/test2?amount=" + "{amount}");
assertThat(data.template().method()).isEqualTo("PUT");
assertThat(data.template().headers().get("Accept").iterator().next())
.isEqualTo(MediaType.APPLICATION_JSON_VALUE);
Expand Down Expand Up @@ -427,12 +386,12 @@ public void testProcessAnnotations_NumberFormatParam() throws Exception {
public void testProcessAnnotations_Advanced2() throws Exception {
Method method = TestTemplate_Advanced.class.getDeclaredMethod("getTest");
MethodMetadata data = contract
.parseAndValidateMetadata(method.getDeclaringClass(), method);
.parseAndValidateMetadata(method.getDeclaringClass(), method);

assertThat(data.template().url()).isEqualTo("/advanced");
assertThat(data.template().url()).isEqualTo("/");
assertThat(data.template().method()).isEqualTo("GET");
assertThat(data.template().headers().get("Accept").iterator().next())
.isEqualTo(MediaType.APPLICATION_JSON_VALUE);
.isEqualTo(MediaType.APPLICATION_JSON_VALUE);
}

@Test
Expand Down Expand Up @@ -539,7 +498,7 @@ public void testProcessAnnotations_Fallback() throws Exception {
.parseAndValidateMetadata(method.getDeclaringClass(), method);

assertThat(data.template().url())
.isEqualTo("/advanced/testfallback/{id}?amount=" + "{amount}");
.isEqualTo("/testfallback/{id}?amount=" + "{amount}");
assertThat(data.template().method()).isEqualTo("PUT");
assertThat(data.template().headers().get("Accept").iterator().next())
.isEqualTo(MediaType.APPLICATION_JSON_VALUE);
Expand Down Expand Up @@ -699,7 +658,7 @@ public interface TestTemplate_Simple {
ResponseEntity<TestObject> getMappingTest(@PathVariable("id") String id);

@RequestMapping(method = RequestMethod.POST,
produces = MediaType.APPLICATION_JSON_VALUE)
produces = MediaType.APPLICATION_JSON_VALUE)
TestObject postTest(@RequestBody TestObject object);

@PostMapping(produces = MediaType.APPLICATION_JSON_VALUE)
Expand All @@ -708,11 +667,19 @@ public interface TestTemplate_Simple {
}

@RequestMapping("/prepend/{classId}")
public interface TestTemplate_Class_RequestMapping {

@RequestMapping(value = "/test/{testId}", method = RequestMethod.GET)
TestObject getSpecificTest(@PathVariable("classId") String classId,
@PathVariable("testId") String testId);

}

public interface TestTemplate_Class_Annotations {

@RequestMapping(value = "/test/{testId}", method = RequestMethod.GET)
TestObject getSpecificTest(@PathVariable("classId") String classId,
@PathVariable("testId") String testId);
@PathVariable("testId") String testId);

@RequestMapping(method = RequestMethod.GET)
TestObject getAllTests(@PathVariable("classId") String classId);
Expand Down Expand Up @@ -812,7 +779,6 @@ public interface TestTemplate_MatrixVariable {
}

@JsonAutoDetect
@RequestMapping("/advanced")
public interface TestTemplate_Advanced {

@CollectionFormat(SSV)
Expand Down

0 comments on commit d6783a6

Please sign in to comment.