-
Notifications
You must be signed in to change notification settings - Fork 812
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.
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
Changes from 3 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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") | ||
| Store update(@PathVariable("storeId") Long storeId, Store store); | ||
|
|
||
| @DeleteMapping("/stores/{storeId:\\d+}") | ||
|
|
@@ -701,6 +701,36 @@ public interface DemoClient { | |
|
|
||
| You can also disable the feature via property `spring.cloud.openfeign.cache.enabled=false`. | ||
|
|
||
|
|
||
| [[spring-requestmapping-support]] | ||
| === Spring @RequestMapping Support | ||
|
|
||
| Spring Cloud OpenFeign provides support for the Spring `@RequestMapping` annotation and its derived annotations (such as `@GetMapping`, `@PostMapping`, etc.) support. | ||
|
||
| The attributes on the `@RequestMapping` annotation (including `value`, `method`, `params`, `headers`, `consumes`, and `produces`) will be parsed by `SpringMvcContract` as the content of the request. | ||
|
||
|
|
||
|
|
||
| For example: | ||
|
||
|
|
||
| Define an interface using the `params` attribute. | ||
|
|
||
| [source,java,indent=0] | ||
| ---- | ||
| @FeignClient("demo") | ||
| public interface DemoTemplate { | ||
|
|
||
| @PostMapping(value = "/stores/{storeId}", params = "mode=upsert") | ||
| Store update(@PathVariable("storeId") Long storeId, Store store); | ||
| } | ||
| ---- | ||
|
|
||
| In the above example, the request url will be resolved to `/stores/{storeId}?mode=upsert`. + | ||
|
||
| The params attribute also supports the use of multiple `key=value` or only one `key`. For example: + | ||
|
||
|
|
||
| - When `params = { "key1=v1", "key2=v2" }`, the request url will be parsed as `/stores/{storeId}?key1=v1&key2=v2`. | ||
|
||
| - When `params = "key"`, the request url will be parsed as `/stores/{storeId}?key`. | ||
|
||
|
|
||
|
|
||
|
|
||
| [[feign-querymap-support]] | ||
| === Feign @QueryMap support | ||
|
|
||
|
|
||
| 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. | ||
|
|
@@ -87,6 +87,7 @@ | |
| * @author Darren Foong | ||
| * @author Ram Anaswara | ||
| * @author Sam Kruglov | ||
| * @author Tang Xiong | ||
| */ | ||
| public class SpringMvcContract extends Contract.BaseContract implements ResourceLoaderAware { | ||
|
|
||
|
|
@@ -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<>()); | ||
| } | ||
|
|
||
|
|
@@ -354,6 +358,22 @@ private void parseHeaders(MethodMetadata md, Method method, RequestMapping annot | |
| } | ||
| } | ||
|
|
||
| private void parseParams(MethodMetadata data, Method method, RequestMapping methodMapping) { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please add documentation for this feature in There was a problem hiding this comment. Choose a reason for hiding this commentThe 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()) { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it's good. Fixed There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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<>(); | ||
|
|
@@ -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; | ||
| } | ||
|
|
||
| } | ||
|
|
||
| } | ||
| 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. | ||
|
|
@@ -82,6 +82,7 @@ | |
| * @author Szymon Linowski | ||
| * @author Sam Kruglov | ||
| * @author Bhavya Agrawal | ||
| * @author Tang Xiong | ||
| **/ | ||
|
|
||
| class SpringMvcContractTests { | ||
|
|
@@ -462,6 +463,68 @@ void testProcessAnnotations_MapParams() throws Exception { | |
| assertThat(data.queryMapIndex().intValue()).isEqualTo(0); | ||
| } | ||
|
|
||
| @Test | ||
| void testProcessAnnotations_ParseParams_SingleParam() throws Exception { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please add a tests to verify behaviour when both There was a problem hiding this comment. Choose a reason for hiding this commentThe 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); | ||
|
|
@@ -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") | ||
|
|
||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed