Add @QueryMap mapEncoder attribute
#2098
Conversation
pilak
force-pushed
the
query-map-encoder-choice
branch
3 times, most recently
from
e318408
to
a56de3c
Compare
pilak
force-pushed
the
query-map-encoder-choice
branch
from
a56de3c
to
6975a20
Compare
|
Can you give project team edit access so I can rebase this branch and get CI rinnung? |
I guess this is done |
pilak
force-pushed
the
query-map-encoder-choice
branch
3 times, most recently
from
09c5852
to
a77998b
Compare
pilak
force-pushed
the
query-map-encoder-choice
branch
from
a77998b
to
b0506d3
Compare
pilak
force-pushed
the
query-map-encoder-choice
branch
from
b0506d3
to
3ad949c
Compare
* use `mapEncoder` attribute at method level for what encoder to use * still use builder `QueryMapEncoder` if no attribute specified
pilak
force-pushed
the
query-map-encoder-choice
branch
from
3ad949c
to
8a2c298
Compare
WalkthroughThis pull request introduces a flexible way to encode query parameters in Feign. It adds a new Changes
TipsChat with CodeRabbit Bot (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 3
Files selected for processing (7)
- core/src/main/java/feign/BaseBuilder.java (1 hunks)
- core/src/main/java/feign/Contract.java (1 hunks)
- core/src/main/java/feign/MethodMetadata.java (2 hunks)
- core/src/main/java/feign/QueryMap.java (2 hunks)
- core/src/main/java/feign/RequestTemplateFactoryResolver.java (1 hunks)
- core/src/test/java/feign/ChildPojo.java (1 hunks)
- core/src/test/java/feign/FeignTest.java (3 hunks)
Additional comments (Suppressed): 12
core/src/test/java/feign/ChildPojo.java (1)
- 16-30: The new private property
parentPrivatePropertyAlteredByGetterand its getter method could potentially introduce a side effect when accessed, as it appends "FromGetter" to the returned value. Ensure that this behavior is intended and does not cause unexpected results in the rest of your codebase.- return parentPrivatePropertyAlteredByGetter + "FromGetter"; + return parentPrivatePropertyAlteredByGetter;core/src/main/java/feign/BaseBuilder.java (1)
- 47-47: The default
QueryMapEncoderhas been changed fromFieldQueryMapEncodertoQueryMap.MapEncoder.FIELD.instance(). Ensure that this change does not affect the existing functionality whereFieldQueryMapEncoderwas expected as the default encoder.- protected QueryMapEncoder queryMapEncoder = new FieldQueryMapEncoder(); + protected QueryMapEncoder queryMapEncoder = QueryMap.MapEncoder.FIELD.instance();core/src/main/java/feign/Contract.java (2)
315-318: The new code introduces a call to
queryMap.mapEncoder().instance(). Ensure that themapEncoder()method and theinstance()method are properly defined and return the expected types. Also, verify that theinstance()method does not have any side effects that could affect the program's behavior.320-321: No issues found in these lines. The changes seem to be consistent with the existing codebase and follow the same pattern as previous implementations.
core/src/main/java/feign/MethodMetadata.java (1)
- 29-35: The addition of
QueryMapEncoder queryMapEncoder;is a good approach to allow more granular control over query map encoding strategies. However, please ensure that this new field is properly serialized and deserialized if theMethodMetadataobject needs to be passed across different JVMs or stored for later use, as the class implementsSerializable.core/src/main/java/feign/QueryMap.java (1)
- 72-74: The
encodedattribute is marked as deprecated. Ensure that this change does not break existing functionality where theencodedattribute is used.core/src/main/java/feign/RequestTemplateFactoryResolver.java (2)
108-112: The changes here are introducing a new way to encode the query map parameters using the
queryMapEncoderfrom the method metadata. This allows for more granular control over the encoding strategy at the method level.115-119: Similar to the query map parameters, the header map parameters are also being encoded using the
queryMapEncoderfrom the method metadata. However, it seems odd to use aqueryMapEncoderfor headers. Please verify if this is intended or if a separate encoder should be used for headers.core/src/test/java/feign/FeignTest.java (4)
16-22: The import statement for
feign.QueryMap.MapEncoderis added to support the new feature of specifying aQueryMapEncoderat the method level. This change aligns with the goals of the PR.921-933: This test case verifies that the query parameters are correctly formed when using a
ChildPojoobject with thequeryMapPropertyInheritencemethod. The expected behavior is that all properties (public, protected, and private) should be included in the query parameters. The test case seems to cover this scenario adequately.936-957: This test case checks the functionality of the new attribute
mapEncoderin the@QueryMapannotation. It uses aFieldQueryMapEncoderas the encoder and expects the query parameters to include all properties (public, protected, and private), with the private property altered by its getter method. This test case provides good coverage for the new feature introduced in this PR.1232-1241: These changes reflect the addition of a new method
queryMapPropertyInheritenceWithBeanMapEncoderin theTestInterface. This method uses the newmapEncoderattribute in the@QueryMapannotation to specify aBEANencoder. This change is consistent with the PR's goal of allowing more granular control over query map encoding strategies.
| public QueryMapEncoder queryMapEncoder() { | ||
| return queryMapEncoder; | ||
| } | ||
|
|
||
| public MethodMetadata queryMapEncoder(QueryMapEncoder queryMapEncoder) { | ||
| this.queryMapEncoder = queryMapEncoder; | ||
| return this; | ||
| } |
There was a problem hiding this comment.
The getter and setter methods for queryMapEncoder are correctly implemented. However, it would be beneficial to add null checks in the setter method to prevent potential NullPointerExceptions.
public MethodMetadata queryMapEncoder(QueryMapEncoder queryMapEncoder) {
+ if (queryMapEncoder == null) {
+ throw new IllegalArgumentException("QueryMapEncoder cannot be null");
+ }
this.queryMapEncoder = queryMapEncoder;
return this;
}There was a problem hiding this comment.
queryMapEncoder is nullable
There was a problem hiding this comment.
@pilak Thank you for your clarification. If queryMapEncoder is indeed nullable, then the null check isn't necessary. However, it's important to ensure that any code using this setter method handles the potential null value appropriately to avoid NullPointerExceptions. Here's the updated code:
public MethodMetadata queryMapEncoder(QueryMapEncoder queryMapEncoder) {
this.queryMapEncoder = queryMapEncoder;
return this;
}Please make sure to handle the null case wherever this method is used.
| /** | ||
| * Specifies the QueryMapEncoder implementation to use to transform DTO into query map. | ||
| * | ||
| * @return the enum containing the instance of QueryMapEncoder | ||
| */ | ||
| MapEncoder mapEncoder() default MapEncoder.DEFAULT; | ||
|
|
||
| public enum MapEncoder { | ||
| // the latter DEFAULT will use BaseBuilder instance | ||
| BEAN(new BeanQueryMapEncoder()), FIELD(new FieldQueryMapEncoder()), DEFAULT(null); | ||
|
|
||
| private QueryMapEncoder mapEncoder; | ||
|
|
||
| private MapEncoder(QueryMapEncoder mapEncoder) { | ||
| this.mapEncoder = mapEncoder; | ||
| } | ||
|
|
||
| public QueryMapEncoder instance() { | ||
| return mapEncoder; | ||
| } | ||
| } |
There was a problem hiding this comment.
The new mapEncoder attribute and MapEncoder enum are introduced to specify the QueryMapEncoder implementation at the method level. This is a good improvement for more granular control over query map encoding strategies.
MapEncoder mapEncoder() default MapEncoder.DEFAULT;
public enum MapEncoder {
BEAN(new BeanQueryMapEncoder()), FIELD(new FieldQueryMapEncoder()), DEFAULT(null);
private QueryMapEncoder mapEncoder;
private MapEncoder(QueryMapEncoder mapEncoder) {
this.mapEncoder = mapEncoder;
}
public QueryMapEncoder instance() {
return mapEncoder;
}
}However, it's important to ensure that the DEFAULT value of MapEncoder is handled correctly in the codebase where mapEncoder().instance() is called, as it could potentially return null.
There was a problem hiding this comment.
this issue is correctly handled
There was a problem hiding this comment.
@pilak That's great to hear! It's crucial to handle potential null values to prevent unexpected NullPointerExceptions. If the DEFAULT case is properly handled in the codebase, this change should provide a more flexible and granular control over query map encoding strategies. Good job on addressing this concern!
| private Map<String, Object> toQueryMap(Object value, QueryMapEncoder queryMapEncoder) { | ||
| if (value instanceof Map) { | ||
| return (Map<String, Object>) value; | ||
| } | ||
| try { | ||
| return queryMapEncoder.encode(value); | ||
| // encode with @QueryMap annotation if exists otherwise with the one from this resolver | ||
| return queryMapEncoder != null ? queryMapEncoder.encode(value) | ||
| : this.queryMapEncoder.encode(value); | ||
| } catch (EncodeException e) { | ||
| throw new IllegalStateException(e); | ||
| } |
There was a problem hiding this comment.
This change introduces a fallback mechanism for encoding values into a query map. If a specific QueryMapEncoder is provided in the method metadata, it will be used; otherwise, the default encoder from the resolver will be used. This provides flexibility and allows for different encoding strategies on a per-method basis.
- return queryMapEncoder.encode(value);
+ return queryMapEncoder != null ? queryMapEncoder.encode(value)
+ : this.queryMapEncoder.encode(value);There was a problem hiding this comment.
@pilak: There are a few things you can do here:
-
Review the code: Look at the changes made in this pull request and provide feedback. Are there any potential issues or improvements that could be made? Do you understand the purpose of the changes?
-
Test the changes: If possible, test the new functionality to ensure it works as expected. This could involve writing unit tests or manually testing the feature.
-
Understand the context: Try to understand why these changes were made. What problem is this solving? How does it fit into the larger project?
-
Ask questions: If anything is unclear, ask for clarification. This could be about the code itself, the reasoning behind it, or how it's supposed to work.
-
Approve or request changes: Based on your review, you can approve the changes if everything looks good, or request further changes if necessary.
|
@velo do I have to resolve rabbitai reviews? |
not really. I'm thinking about removing it, as it's not that useful |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 2
Files selected for processing (1)
- core/src/test/java/feign/FeignTest.java (3 hunks)
Additional comments (Suppressed): 2
core/src/test/java/feign/FeignTest.java (2)
940-961: This new test case
queryMap_with_child_pojo_altered_by_getter_while_using_overriding_encoder()appears to be testing the functionality of using an overriding encoder (FieldQueryMapEncoder) with thequeryMapPropertyInheritenceWithBeanMapEncodermethod. The test case looks well-written and covers the necessary assertions. Good job on adding this test case.1236-1245: The
TestInterfaceinterface has been updated with a new methodqueryMapPropertyInheritenceWithBeanMapEncoder. This method uses the@QueryMapannotation with themapEncoderattribute set toMapEncoder.BEAN. Ensure that all calls to this method throughout the codebase have been updated to match the new signature.
| import com.google.gson.Gson; | ||
| import com.google.gson.reflect.TypeToken; | ||
| import feign.Feign.ResponseMappingDecoder; | ||
| import feign.QueryMap.MapEncoder; | ||
| import feign.Request.HttpMethod; | ||
| import feign.Target.HardCodedTarget; | ||
| import feign.codec.DecodeException; |
There was a problem hiding this comment.
The import statement for feign.QueryMap.MapEncoder is not properly formatted. It should be aligned with the other import statements.
- import feign.QueryMap.MapEncoder;
+ import feign.QueryMap.MapEncoder;| childPojo.setChildPrivateProperty("first"); | ||
| childPojo.setParentProtectedProperty("second"); | ||
| childPojo.setParentPublicProperty("third"); | ||
| childPojo.setParentPrivatePropertyAlteredByGetter("fourth"); | ||
|
|
||
| server.enqueue(new MockResponse()); | ||
| api.queryMapPropertyInheritence(childPojo); | ||
| assertThat(server.takeRequest()) | ||
| .hasQueryParams( | ||
| "parentPublicProperty=third", | ||
| "parentProtectedProperty=second", | ||
| "childPrivateProperty=first"); | ||
| "childPrivateProperty=first", | ||
| "parentPrivatePropertyAlteredByGetter=fourth"); |
There was a problem hiding this comment.
This test case seems to be testing the query parameters generated when a ChildPojo object is passed to the queryMapPropertyInheritence method. The expected query parameters are correctly listed in the assertion. However, it would be beneficial to add some comments explaining what this test case is specifically testing for future reference.
yeah awesome but a tiny bit intrusive 😄 |
mapEncoderattribute at method level for what encoder to useQueryMapEncoderif no attribute specifiedHello,
Working on a project where we are using FeignBuilder in a separate library that is used like a client factory. When using some particular DTO request object, with
@QueryMapannotation, we want to pass through the POJO getters. Unfortunately, we have to change the Builder instance ofQueryMapEncoderfromFieldQueryMapEncodertoBeanQueryMapEncoderto do so, and the consequence is that this implementation will be used in every client that will be built by this factory.Also we are wondering why the choice of this encoder implementation is not closer to the annocation itself.
Here's a PR that makes possible to add the choice of the
QueryMapEncoderimplementation in a@QueryMapannotation attribute.Doing so it is possible to change a QueryMapEncoder only of a specific method in the same Client
Summary by CodeRabbit
Release Notes:
BEAN,FIELD, andDEFAULTencoding strategies.BaseBuilder,Contract,MethodMetadata, andRequestTemplateFactoryResolverclasses to support the new query map encoding feature.FeignTestto verify the functionality of the new query map encoding feature and its impact on child POJOs altered by getters.Please note that these changes may require updates to your existing code if you are directly using any of the modified classes or methods.