-
Notifications
You must be signed in to change notification settings - Fork 784
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
SpringMvcContract support parse params #1016
SpringMvcContract support parse params #1016
Conversation
@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. |
@Puppy4C Thank you for signing the Contributor License Agreement! |
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.
Thanks for the PR, @Puppy4C. Have added some comments - please take a look.
@@ -354,6 +358,19 @@ 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 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
.
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
NameValueResolver(String expression) { | ||
int separator = expression.indexOf('='); | ||
if (separator == -1) { | ||
this.isNegated = expression.startsWith("!"); |
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 remove unnecessary this.
keywords.
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
@@ -462,6 +463,60 @@ 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 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.
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
} | ||
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 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.
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.
I think it's good. Fixed
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.
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!
ca111e1
to
c26d64e
Compare
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.
Thanks @Puppy4C. The implementation looks good, but the documentation should be slightly more robust. I have added a comment. Please address.
@@ -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") |
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
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.
Thanks @Puppy4C. LGTM.
@Buzzardo could you please review the documentation changes? |
[[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. |
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.
Change etc.
to and others
.
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.
Done, thanks a lot!
=== 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. |
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.
Change will be
to are
.
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.
Done
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: |
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.
Change to Consider the following example:
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.
Done
} | ||
---- | ||
|
||
In the above example, the request url will be resolved to `/stores/{storeId}?mode=upsert`. + |
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.
Change will be
to is
.
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.
Done
---- | ||
|
||
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: + |
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.
Remove . For example
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.
Done
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`. |
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.
Change will be
to is
.
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.
Done
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`. |
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.
Change will be
to is
.
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.
Done
Hi @OlgaMaciaszek,Done |
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.
Thanks @Puppy4C. LGTM.
Previously, SpringMvcContract did not support parsing the
params
parameter of@RequestMapping
.Example:
params
parameter: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.