Add @QueryMap mapEncoder attribute #1013
Conversation
|
hello @OlgaMaciaszek , Can this PR be merged before the next release version? This feature allows for more flexible transformer DTO. |
|
Thanks for the PR, @galaxy-sea. We will definitely review it before the next release (not coming till mid May). |
OlgaMaciaszek
added
enhancement
There was a problem hiding this comment.
Hi @galaxy-sea, thanks for submitting the PR. There's a cosmetic issue with the licese doc dating, but mainly I'm not sure if we need another interface on top of QueryMapEncoder. Please provide more details regarding this design choice.
| @@ -74,6 +75,7 @@ | |||
| * @author Olga Maciaszek-Sharma | |||
| * @author Hyeonmin Park | |||
| * @author Yanming Zhou | |||
| * @author changjin wei(魏昌进) | |||
There was a problem hiding this comment.
Please update the year in license comments of all the modified files to -2024.
| * @author changjin wei(魏昌进) | ||
| * @see org.springframework.cloud.openfeign.SpringQueryMap | ||
| */ | ||
| public interface SpringMapEncoder { |
There was a problem hiding this comment.
Not sure why add another interface instead of just looking for QueryMapEncoder beans available in the context; what was the reasoning behind this?
There was a problem hiding this comment.
If using QueryMapEncoder, it will cause FeignClientFactoryBean to be unable to obtain the global QueryMapEncoder
There was a problem hiding this comment.
Sure, I'm thinking we could have the users mark that one as @Primary if there's more than one, but that would be a braking change, so for the 2025.0.x release, but adding another interface feels redundant. Will discuss it with the team.
There was a problem hiding this comment.
2025.0.x is also too far away😭. Still hope to merge PR as soon as possible
There was a problem hiding this comment.
After some discussion with the team, we would not like to add an additional interface. I think we should add a boolean opt-in flag and an additional annotation such as @BaseQueryMapEncoder to set the base builder one. Like this, if someone does not turn on the flag, things work for them as they did before; if they turn on the flag and have a bean annotated as @BaseQueryMapEncoder, that one is going to be used in the base builder; if they have beans without that annotation, those will only be used as possible to pick with the annotation, regardless of whether there's one or more. If you have a different idea, but without adding a wrapping interface, feel free to suggest it here.
There was a problem hiding this comment.
hi @OlgaMaciaszek , I seem to have encountered some trouble.
I am unable to filter out bean that use @BaseQueryMapEncoder in multiple beans.
Map<String, QueryMapEncoder> queryMapEncoders = getInheritedAwareInstances(context, QueryMapEncoder.class);
QueryMapEncoder queryMapEncoder = queryMapEncoders.values()
.stream()
.filter(encoder -> {
// TODO: Filter QueryMapEncoders that use @BaseQueryMapEncoder
})
.findAny()
.orElseGet(() -> getInheritedAwareOptional(context, QueryMapEncoder.class));There was a problem hiding this comment.
Yes, this will require adding a change in SC Commons as well; Have created a PR: spring-cloud/spring-cloud-commons#1352, and then you'll need to add a method to handle the "inherited" flag, as in getInheritedAwareInstances method.
|
Make sure to document your feature in |
|
hello @OlgaMaciaszek , I will change the implementation to |
There was a problem hiding this comment.
Thanks, @galaxy-sea. I think we're still missing the opt-in property flag. Make sure to implement the entire behaviour discussed here and include all the information on it in the documentation. Let me know when it's ready for another review.
hi @OlgaMaciaszek , I don't think it's necessary to add opt in property flags, just using bean annotated as @ BaseQueryMapEncoder would be more reasonable |
|
@galaxy-sea the idea is to maintain it backwards compatible since we're not waiting for the major - the behaviour should not cause any breaking changes for existing users, hence the flag. |
|
Hello @galaxy-sea, please let us know if you're planning to continue working on this PR and add requested changes? |
Referenced feign#2098.♥️ .
I like this feature