-
-
Notifications
You must be signed in to change notification settings - Fork 6.6k
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
Remove wrong request mapping for feign clients #13546
Remove wrong request mapping for feign clients #13546
Conversation
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.
Looks good to me!
@@ -491,6 +493,8 @@ public void processOpts() { | |||
additionalProperties.put(SINGLE_CONTENT_TYPES, "true"); | |||
this.setSingleContentTypes(true); | |||
} | |||
additionalProperties.put(USE_FEIGN_CLIENT, "true"); |
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.
Ah I guess this didn't exist, it just relied on importing the correct apiClient.mustache
instead
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.
Yes correct
@MelleD do you think this is good to merge? The Feing issue is gaining more traction, would be nice to get a fix in the next patch release 🙏 |
Yes for me it’s finish and mergeable |
@wing328 I think this is good to go, probably fixes some of the Feign issues |
* Remove request mapping * Fix bug for feign clients * Fix test * Fix test files * Rebuild * Revert change
@MelleD I just stumbled upon the @RequestMapping issue. IIRC especially when using the spring-cloud feign client, one should not annotate the interface with the @RequestMapping annotation. See spring-cloud/spring-cloud-netflix#466. But the |
Sorry @MelleD, the logic behind useFeignClient seems right. I still have breaking builds where |
@cachescrubber with 6.2.1 a regression has been brought into the spring generator. It now - again - places @NotNull validations even with useBeanValidation=false and - worse - it forgets to add the import into the generated interface |
PR checklist
This is important, as CI jobs will verify all generator outputs of your HEAD commit as it would merge with master.
These must match the expectations made by your contribution.
You may regenerate an individual generator by passing the relevant config(s) as an argument to the script, for example
./bin/generate-samples.sh bin/configs/java*
.For Windows users, please run the script in Git BASH.
master