Skip to content

SpringMvcContract support parse params #1016

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

Merged
merged 4 commits into from
May 13, 2024
Merged

SpringMvcContract support parse params #1016

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
724d632
SpringMvcContract support parse params
Apr 4, 2024
c26d64e
Address PR Comments
Apr 20, 2024
2e44717
Address PR Comments
Apr 24, 2024
8a9da4a
Address PR Comments
May 9, 2024
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
2 changes: 1 addition & 1 deletion docs/modules/ROOT/pages/spring-cloud-openfeign.adoc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ public interface StoreClient {
@GetMapping("/stores")
Page<Store> getStores(Pageable pageable);

@PostMapping(value = "/stores/{storeId}", consumes = "application/json")
@PostMapping(value = "/stores/{storeId}", consumes = "application/json", params = "mode=upsert")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please add a description of the feature, including information on what syntax is allowed and to what resulting query params it will be resolved.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

Store update(@PathVariable("storeId") Long storeId, Store store);

@DeleteMapping("/stores/{storeId:\\d+}")
Expand Down
58 changes: 57 additions & 1 deletion ...ign-core/src/main/java/org/springframework/cloud/openfeign/support/SpringMvcContract.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright 2013-2023 the original author or authors.
* Copyright 2013-2024 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.
Expand Down Expand Up @@ -87,6 +87,7 @@
* @author Darren Foong
* @author Ram Anaswara
* @author Sam Kruglov
* @author Tang Xiong
*/
public class SpringMvcContract extends Contract.BaseContract implements ResourceLoaderAware {

Expand Down Expand Up @@ -244,6 +245,9 @@ protected void processAnnotationOnMethod(MethodMetadata data, Annotation methodA
// headers
parseHeaders(data, method, methodMapping);

// params
parseParams(data, method, methodMapping);

data.indexToExpander(new LinkedHashMap<>());
}

Expand Down Expand Up @@ -354,6 +358,22 @@ private void parseHeaders(MethodMetadata md, Method method, RequestMapping annot
}
}

private void parseParams(MethodMetadata data, Method method, RequestMapping methodMapping) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please add documentation for this feature in spring-cloud-openfeign.adoc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

String[] params = methodMapping.params();
if (params == null || params.length == 0) {
return;
}
for (String param : params) {
NameValueResolver nameValueResolver = new NameValueResolver(param);
if (!nameValueResolver.isNegated()) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I know it was handled for headers, but it actually does not make much sense to have negated values on client side; I think we should not handle them at all (i.e. throw an exception), because adding them on client side indicates the client-side API has not been thought through.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it's good. Fixed

Copy link

Choose a reason for hiding this comment

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

I know it was handled for headers, but it actually does not make much sense to have negated values on client side; I think we should not handle them at all (i.e. throw an exception), because adding them on client side indicates the client-side API has not been thought through.

Hi @OlgaMaciaszek ,

Is it possible to change the access modifier of the parseParams method (and other parse* methods) in this Contract from private to protected? This would allow for greater accessibility and potential subclassing, enabling us to enrich the Contract.

Note: We use generated APIs from the same OpenAPI specifications for intercommunication between our Spring-based applications, which use the same interfaces for both the Feign client and REST controller.

Thank you!

data.template().query(resolve(nameValueResolver.getName()), resolve(nameValueResolver.getValue()));
}
else {
throw new IllegalArgumentException("Negated params are not supported: " + param);
}
}
}

private Map<Class<? extends Annotation>, AnnotatedParameterProcessor> toAnnotatedArgumentProcessorMap(
List<AnnotatedParameterProcessor> processors) {
Map<Class<? extends Annotation>, AnnotatedParameterProcessor> result = new HashMap<>();
Expand Down Expand Up @@ -465,4 +485,40 @@ public Collection<String> setTemplateParameter(String name, Collection<String> r

}

private static class NameValueResolver {

private final String name;

private final String value;

private final boolean isNegated;

NameValueResolver(String expression) {
int separator = expression.indexOf('=');
if (separator == -1) {
isNegated = expression.startsWith("!");
name = (isNegated ? expression.substring(1) : expression);
value = null;
}
else {
isNegated = (separator > 0) && (expression.charAt(separator - 1) == '!');
name = (isNegated ? expression.substring(0, separator - 1) : expression.substring(0, separator));
value = expression.substring(separator + 1);
}
}

public String getName() {
return name;
}

public String getValue() {
return value;
}

public boolean isNegated() {
return isNegated;
}

}

}
90 changes: 89 additions & 1 deletion ...ore/src/test/java/org/springframework/cloud/openfeign/support/SpringMvcContractTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright 2013-2023 the original author or authors.
* Copyright 2013-2024 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.
Expand Down Expand Up @@ -82,6 +82,7 @@
* @author Szymon Linowski
* @author Sam Kruglov
* @author Bhavya Agrawal
* @author Tang Xiong
**/

class SpringMvcContractTests {
Expand Down Expand Up @@ -462,6 +463,68 @@ void testProcessAnnotations_MapParams() throws Exception {
assertThat(data.queryMapIndex().intValue()).isEqualTo(0);
}

@Test
void testProcessAnnotations_ParseParams_SingleParam() throws Exception {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please add a tests to verify behaviour when both params= and @RequestParam annotation present.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

Method method = TestTemplate_ParseParams.class.getDeclaredMethod("singleParam");
MethodMetadata data = contract.parseAndValidateMetadata(method.getDeclaringClass(), method);

assertThat(data.template().url()).isEqualTo("/test?p1=1");
assertThat(data.template().method()).isEqualTo("GET");
}

@Test
void testProcessAnnotations_ParseParams_MultipleParams() throws Exception {
Method method = TestTemplate_ParseParams.class.getDeclaredMethod("multipleParams");
MethodMetadata data = contract.parseAndValidateMetadata(method.getDeclaringClass(), method);

assertThat(data.template().url()).isEqualTo("/test?p1=1&p2=2");
assertThat(data.template().method()).isEqualTo("GET");
}

@Test
void testProcessAnnotations_ParseParams_MixParams() throws Exception {
Method method = TestTemplate_ParseParams.class.getDeclaredMethod("mixParams");
MethodMetadata data = contract.parseAndValidateMetadata(method.getDeclaringClass(), method);

assertThat(data.template().url()).isEqualTo("/test?p1=1&p2");
assertThat(data.template().method()).isEqualTo("GET");
}

@Test
void testProcessAnnotations_ParseParams_SingleParamWithoutValue() throws Exception {
Method method = TestTemplate_ParseParams.class.getDeclaredMethod("singleParamWithoutValue");
MethodMetadata data = contract.parseAndValidateMetadata(method.getDeclaringClass(), method);

assertThat(data.template().url()).isEqualTo("/test?p1");
assertThat(data.template().method()).isEqualTo("GET");
}

@Test
void testProcessAnnotations_ParseParams_MultipleParamsWithoutValue() throws Exception {
Method method = TestTemplate_ParseParams.class.getDeclaredMethod("multipleParamsWithoutValue");
MethodMetadata data = contract.parseAndValidateMetadata(method.getDeclaringClass(), method);

assertThat(data.template().url()).isEqualTo("/test?p1&p2");
assertThat(data.template().method()).isEqualTo("GET");
}

@Test
void testProcessAnnotations_ParseParams_NotEqualParams() throws Exception {
assertThatIllegalArgumentException().isThrownBy(() -> {
Method method = TestTemplate_ParseParams.class.getDeclaredMethod("notEqualParams");
contract.parseAndValidateMetadata(method.getDeclaringClass(), method);
});
}

@Test
void testProcessAnnotations_ParseParams_ParamsAndRequestParam() throws Exception {
Method method = TestTemplate_ParseParams.class.getDeclaredMethod("paramsAndRequestParam", String.class);
MethodMetadata data = contract.parseAndValidateMetadata(method.getDeclaringClass(), method);

assertThat(data.template().url()).isEqualTo("/test?p1=1&p2={p2}");
assertThat(data.template().method()).isEqualTo("GET");
}

@Test
void testProcessHeaders() throws Exception {
Method method = TestTemplate_Headers.class.getDeclaredMethod("getTest", String.class);
Expand Down Expand Up @@ -750,6 +813,31 @@ public interface TestTemplate_MapParams {

}

public interface TestTemplate_ParseParams {

@GetMapping(value = "test", params = "p1=1")
ResponseEntity<TestObject> singleParam();

@GetMapping(value = "test", params = { "p1=1", "p2=2" })
ResponseEntity<TestObject> multipleParams();

@GetMapping(value = "test", params = { "p1" })
ResponseEntity<TestObject> singleParamWithoutValue();

@GetMapping(value = "test", params = { "p1", "p2" })
ResponseEntity<TestObject> multipleParamsWithoutValue();

@GetMapping(value = "test", params = { "p1=1", "p2" })
ResponseEntity<TestObject> mixParams();

@GetMapping(value = "test", params = { "p1!=1" })
ResponseEntity<TestObject> notEqualParams();

@GetMapping(value = "test", params = { "p1=1" })
ResponseEntity<TestObject> paramsAndRequestParam(@RequestParam("p2") String p2);

}

public interface TestTemplate_HeaderMap {

@GetMapping("/headerMap")
Expand Down