Skip to content

Conversation

@dkswnkk
Copy link
Contributor

@dkswnkk dkswnkk commented May 9, 2025

Summary

This PR addresses issue #1191 by adding a warning when a @FeignClient method is declared as GET but has one or more parameters that are not annotated with any supported HTTP binding annotations (e.g., @RequestParam, @RequestHeader, @PathVariable, @SpringQueryMap, etc.).

Context

When no annotations are present, Feign may attempt to treat those parameters as a request body. For GET methods, this leads to a body-less POST due to underlying HTTP client behavior (e.g., HttpURLConnection), which causes a discrepancy:

  • The client logs show the request as GET
  • The server receives the request as a POST
  • No request body is actually sent (Content-Length: 0)

This is difficult to debug and confusing in real-world applications.

Related core issue: OpenFeign/feign#2872

Implementation Details

  • Added logic in SpringMvcContract#parseAndValidateMetadata() to check for:
    • Method is declared as GET
    • Method has parameters
    • None of the parameters have HTTP-related annotations
  • If so, a warning is logged via SLF4J:
[OpenFeign Warning] Feign method 'public abstract Response com.example.Client.getSomething(java.lang.String)' is declared as GET with parameters, but none of the parameters are annotated (e.g. @RequestParam, @RequestHeader, @PathVariable, etc). This may result in fallback to POST at runtime. Consider explicitly annotating parameters.

Copy link
Collaborator

@OlgaMaciaszek OlgaMaciaszek left a comment

Choose a reason for hiding this comment

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

Hello @dkswnkk, thanks for submitting the PR. Have added a comment. Please address it.

MethodMetadata metadata = super.parseAndValidateMetadata(targetType, method);

if (isGetMethod(metadata) && method.getParameterCount() > 0 && !hasHttpAnnotations(method)) {
LOG.warn(String.format(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please use if (LOG.isWarnEnabled())[...]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for your feedback.
I've updated the code to include the LOG.isWarnEnabled() check as you suggested.
Let me know if you have any further comments!

@OlgaMaciaszek OlgaMaciaszek self-assigned this May 21, 2025
@OlgaMaciaszek OlgaMaciaszek added this to the 4.3.0 milestone May 21, 2025
@OlgaMaciaszek OlgaMaciaszek moved this to In Progress in 2025.0.0 May 21, 2025
@OlgaMaciaszek OlgaMaciaszek removed this from 2025.0.0 May 21, 2025
@OlgaMaciaszek OlgaMaciaszek removed this from the 4.3.0 milestone May 21, 2025
Copy link
Collaborator

@OlgaMaciaszek OlgaMaciaszek left a comment

Choose a reason for hiding this comment

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

LGTM.

@OlgaMaciaszek OlgaMaciaszek merged commit deabba6 into spring-cloud:main May 23, 2025
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Suggestion: Warn or fail when GET method in @FeignClient has unannotated parameters

3 participants