SpringMvcContract support parse params by Puppy4C · Pull Request #1016 · spring-cloud/spring-cloud-openfeign

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

 		}
 	}

+	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

 	}

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

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!

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

 	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

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

		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:

Conversation

Puppy4C commented Apr 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pivotal-cla commented Apr 4, 2024

Uh oh!

pivotal-cla commented Apr 4, 2024

Uh oh!

OlgaMaciaszek left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

OlgaMaciaszek left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

OlgaMaciaszek left a comment

Choose a reason for hiding this comment

Uh oh!

OlgaMaciaszek commented Apr 25, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

OlgaMaciaszek commented May 9, 2024

Uh oh!

Puppy4C commented May 9, 2024

Uh oh!

OlgaMaciaszek left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Puppy4C commented Apr 4, 2024 •

edited

Loading