-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Add @QueryMap mapEncoder
attribute
#2098
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -16,6 +16,8 @@ | |
import java.lang.annotation.Retention; | ||
import java.util.List; | ||
import java.util.Map; | ||
import feign.querymap.BeanQueryMapEncoder; | ||
import feign.querymap.FieldQueryMapEncoder; | ||
import static java.lang.annotation.ElementType.PARAMETER; | ||
import static java.lang.annotation.RetentionPolicy.RUNTIME; | ||
|
||
|
@@ -70,4 +72,26 @@ | |
* @deprecated | ||
*/ | ||
boolean encoded() default false; | ||
|
||
/** | ||
* 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; | ||
} | ||
} | ||
Comment on lines
+76
to
+96
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The new 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this issue is correctly handled There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @pilak That's great to hear! It's crucial to handle potential null values to prevent unexpected NullPointerExceptions. If the |
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -108,26 +108,28 @@ public RequestTemplate create(Object[] argv) { | |
// add query map parameters after initial resolve so that they take | ||
// precedence over any predefined values | ||
Object value = argv[metadata.queryMapIndex()]; | ||
Map<String, Object> queryMap = toQueryMap(value); | ||
Map<String, Object> queryMap = toQueryMap(value, metadata.queryMapEncoder()); | ||
template = addQueryMapQueryParameters(queryMap, template); | ||
} | ||
|
||
if (metadata.headerMapIndex() != null) { | ||
// add header map parameters for a resolution of the user pojo object | ||
Object value = argv[metadata.headerMapIndex()]; | ||
Map<String, Object> headerMap = toQueryMap(value); | ||
Map<String, Object> headerMap = toQueryMap(value, metadata.queryMapEncoder()); | ||
template = addHeaderMapHeaders(headerMap, template); | ||
} | ||
|
||
return template; | ||
} | ||
|
||
private Map<String, Object> toQueryMap(Object value) { | ||
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); | ||
} | ||
Comment on lines
+125
to
135
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This change introduces a fallback mechanism for encoding values into a query map. If a specific - return queryMapEncoder.encode(value);
+ return queryMapEncoder != null ? queryMapEncoder.encode(value)
+ : this.queryMapEncoder.encode(value); There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what can I do here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @pilak: There are a few things you can do here:
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -16,6 +16,7 @@ | |
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; | ||
Comment on lines
16
to
22
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The import statement for - import feign.QueryMap.MapEncoder;
+ import feign.QueryMap.MapEncoder; |
||
|
@@ -924,14 +925,39 @@ public void queryMap_with_child_pojo() throws Exception { | |
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"); | ||
Comment on lines
925
to
+937
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This test case seems to be testing the query parameters generated when a |
||
} | ||
|
||
@Test | ||
public void queryMap_with_child_pojo_altered_by_getter_while_using_overriding_encoder() | ||
throws Exception { | ||
TestInterface api = new TestInterfaceBuilder() | ||
.queryMapEncoder(new FieldQueryMapEncoder()) | ||
.target("http://localhost:" + server.getPort()); | ||
|
||
ChildPojo childPojo = new ChildPojo(); | ||
childPojo.setChildPrivateProperty("first"); | ||
childPojo.setParentProtectedProperty("second"); | ||
childPojo.setParentPublicProperty("third"); | ||
childPojo.setParentPrivatePropertyAlteredByGetter("fourth"); | ||
|
||
server.enqueue(new MockResponse()); | ||
api.queryMapPropertyInheritenceWithBeanMapEncoder(childPojo); | ||
assertThat(server.takeRequest()) | ||
.hasQueryParams( | ||
"parentPublicProperty=third", | ||
"parentProtectedProperty=second", | ||
"childPrivateProperty=first", | ||
"parentPrivatePropertyAlteredByGetter=fourthFromGetter"); | ||
} | ||
|
||
@Test | ||
|
@@ -1210,6 +1236,10 @@ void queryMapWithQueryParams(@Param("name") String name, | |
@RequestLine("GET /") | ||
void queryMapPropertyPojo(@QueryMap PropertyPojo object); | ||
|
||
@RequestLine("GET /") | ||
void queryMapPropertyInheritenceWithBeanMapEncoder(@QueryMap( | ||
mapEncoder = MapEncoder.BEAN) ChildPojo object); | ||
|
||
@RequestLine("GET /") | ||
void queryMapPropertyInheritence(@QueryMap ChildPojo object); | ||
|
||
|
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.
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 potentialNullPointerException
s.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.
queryMapEncoder is nullable
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.
@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 avoidNullPointerException
s. Here's the updated code:Please make sure to handle the null case wherever this method is used.