SpringMvcContract support parse params #1016

Puppy4C · 2024-04-04T06:58:21Z

Previously, SpringMvcContract did not support parsing the params parameter of @RequestMapping.

Example:

Define a rest interface that uses the params parameter：

@PutMapping(value = "/user", params = "action=disable")
public String disableUser() {
    return "user";
}

Define a FeignClient to call the previously declared interface：

@FeignClient("user-server")
static interface UserService {
    @PutMapping(value = "/user", params = "action=disable")
    String disableUser();
}

When sending this request, the request path will be resolved to /user instead of /user?action=disable, resulting in 400 errors.
Therefore, this PR provides the ability to parse params parameter.

pivotal-cla · 2024-04-04T06:58:25Z

@Puppy4C Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

pivotal-cla · 2024-04-04T06:59:21Z

@Puppy4C Thank you for signing the Contributor License Agreement!

OlgaMaciaszek

Thanks for the PR, @Puppy4C. Have added some comments - please take a look.

OlgaMaciaszek · 2024-04-18T15:20:06Z

...nfeign-core/src/main/java/org/springframework/cloud/openfeign/support/SpringMvcContract.java

@@ -354,6 +358,19 @@ private void parseHeaders(MethodMetadata md, Method method, RequestMapping annot
 		}
 	}

+	private void parseParams(MethodMetadata data, Method method, RequestMapping methodMapping) {


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

OlgaMaciaszek · 2024-04-18T15:20:44Z

...nfeign-core/src/main/java/org/springframework/cloud/openfeign/support/SpringMvcContract.java

+		NameValueResolver(String expression) {
+			int separator = expression.indexOf('=');
+			if (separator == -1) {
+				this.isNegated = expression.startsWith("!");


Please remove unnecessary this. keywords.

OlgaMaciaszek · 2024-04-18T15:22:00Z

...n-core/src/test/java/org/springframework/cloud/openfeign/support/SpringMvcContractTests.java

@@ -462,6 +463,60 @@ void testProcessAnnotations_MapParams() throws Exception {
 		assertThat(data.queryMapIndex().intValue()).isEqualTo(0);
 	}

+	@Test
+	void testProcessAnnotations_ParseParams_SingleParam() throws Exception {


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

OlgaMaciaszek · 2024-04-18T15:27:44Z

...nfeign-core/src/main/java/org/springframework/cloud/openfeign/support/SpringMvcContract.java

+		}
+		for (String param : params) {
+			NameValueResolver nameValueResolver = new NameValueResolver(param);
+			if (!nameValueResolver.isNegated()) {


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.

I think it's good. Fixed

OlgaMaciaszek

Thanks @Puppy4C. The implementation looks good, but the documentation should be slightly more robust. I have added a comment. Please address.

OlgaMaciaszek · 2024-04-22T13:07:26Z

docs/modules/ROOT/pages/spring-cloud-openfeign.adoc

@@ -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")


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

OlgaMaciaszek

Thanks @Puppy4C. LGTM.

OlgaMaciaszek · 2024-04-25T09:07:05Z

@Buzzardo could you please review the documentation changes?

Buzzardo · 2024-05-07T14:29:23Z

docs/modules/ROOT/pages/spring-cloud-openfeign.adoc

+[[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.


Change etc. to and others.

Done, thanks a lot!

Buzzardo · 2024-05-07T14:29:48Z

docs/modules/ROOT/pages/spring-cloud-openfeign.adoc

+=== 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.


Change will be to are.

Buzzardo · 2024-05-07T14:30:09Z

docs/modules/ROOT/pages/spring-cloud-openfeign.adoc

+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:


Change to Consider the following example:

Buzzardo · 2024-05-07T14:30:35Z

docs/modules/ROOT/pages/spring-cloud-openfeign.adoc

+}
+----
+
+In the above example, the request url will be resolved to `/stores/{storeId}?mode=upsert`. +


Change will be to is.

Buzzardo · 2024-05-07T14:31:09Z

docs/modules/ROOT/pages/spring-cloud-openfeign.adoc

+----
+
+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: +


Remove . For example

Buzzardo · 2024-05-07T14:31:27Z

docs/modules/ROOT/pages/spring-cloud-openfeign.adoc

+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`.


Change will be to is.

Buzzardo · 2024-05-07T14:31:42Z

docs/modules/ROOT/pages/spring-cloud-openfeign.adoc

+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`.


Change will be to is.

OlgaMaciaszek · 2024-05-09T10:16:07Z

Thanks a lot @Buzzardo. @Puppy4C please apply the documentation changes suggested by @Buzzardo.

Puppy4C · 2024-05-09T13:11:48Z

Hi @OlgaMaciaszek，Done

OlgaMaciaszek

Thanks @Puppy4C. LGTM.

spring-cloud-issues added the waiting-for-triage label Apr 4, 2024

OlgaMaciaszek requested changes Apr 18, 2024

View reviewed changes

OlgaMaciaszek added enhancement New feature or request and removed waiting-for-triage labels Apr 18, 2024

OlgaMaciaszek self-assigned this Apr 18, 2024

OlgaMaciaszek added this to the 4.1.2 milestone Apr 18, 2024

Puppy4C added 2 commits April 20, 2024 18:14

SpringMvcContract support parse params

724d632

Address PR Comments

c26d64e

Puppy4C force-pushed the SpringMvcContract-improve branch from ca111e1 to c26d64e Compare April 20, 2024 10:16

Puppy4C requested a review from OlgaMaciaszek April 20, 2024 10:21

OlgaMaciaszek requested changes Apr 22, 2024

View reviewed changes

Address PR Comments

2e44717

Puppy4C requested a review from OlgaMaciaszek April 24, 2024 13:25

OlgaMaciaszek approved these changes Apr 25, 2024

View reviewed changes

Buzzardo suggested changes May 7, 2024

View reviewed changes

Address PR Comments

8a9da4a

Puppy4C requested a review from OlgaMaciaszek May 9, 2024 14:00

OlgaMaciaszek approved these changes May 13, 2024

View reviewed changes

OlgaMaciaszek merged commit c276f1c into spring-cloud:main May 13, 2024
2 checks passed

natedanner pushed a commit to natedanner/spring-cloud__spring-cloud-openfeign that referenced this pull request May 20, 2024

SpringMvcContract support parse params (spring-cloud#1016)

a971264

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SpringMvcContract support parse params #1016

SpringMvcContract support parse params #1016

Puppy4C commented Apr 4, 2024 •

edited

pivotal-cla commented Apr 4, 2024

pivotal-cla commented Apr 4, 2024

OlgaMaciaszek left a comment

OlgaMaciaszek Apr 18, 2024

Puppy4C Apr 20, 2024

OlgaMaciaszek Apr 18, 2024

Puppy4C Apr 20, 2024

OlgaMaciaszek Apr 18, 2024

Puppy4C Apr 20, 2024

OlgaMaciaszek Apr 18, 2024

Puppy4C Apr 20, 2024

OlgaMaciaszek left a comment

OlgaMaciaszek Apr 22, 2024

Puppy4C Apr 24, 2024

OlgaMaciaszek left a comment

OlgaMaciaszek commented Apr 25, 2024

Buzzardo May 7, 2024

Puppy4C May 9, 2024

Buzzardo May 7, 2024

Puppy4C May 9, 2024

Buzzardo May 7, 2024

Puppy4C May 9, 2024

Buzzardo May 7, 2024

Puppy4C May 9, 2024

Buzzardo May 7, 2024

Puppy4C May 9, 2024

Buzzardo May 7, 2024

Puppy4C May 9, 2024

Buzzardo May 7, 2024

Puppy4C May 9, 2024

OlgaMaciaszek commented May 9, 2024

Puppy4C commented May 9, 2024

OlgaMaciaszek left a comment

		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:

SpringMvcContract support parse params #1016

SpringMvcContract support parse params #1016

Conversation

Puppy4C commented Apr 4, 2024 • edited

pivotal-cla commented Apr 4, 2024

pivotal-cla commented Apr 4, 2024

OlgaMaciaszek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OlgaMaciaszek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OlgaMaciaszek left a comment

Choose a reason for hiding this comment

OlgaMaciaszek commented Apr 25, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OlgaMaciaszek commented May 9, 2024

Puppy4C commented May 9, 2024

OlgaMaciaszek left a comment

Choose a reason for hiding this comment

Puppy4C commented Apr 4, 2024 •

edited