Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add @QueryMap mapEncoder attribute #2098

Merged
merged 2 commits into from
Oct 4, 2023
Merged

Add @QueryMap mapEncoder attribute #2098

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8a2c298
Add @QueryMap `mapEncoder` attribute
Jun 21, 2023
f9a898d
Merge branch 'master' into query-map-encoder-choice
velo Oct 2, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion core/src/main/java/feign/BaseBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ public abstract class BaseBuilder<B extends BaseBuilder<B, T>, T> implements Clo
protected Decoder decoder = new Decoder.Default();
protected boolean closeAfterDecode = true;
protected boolean decodeVoid = false;
protected QueryMapEncoder queryMapEncoder = new FieldQueryMapEncoder();
protected QueryMapEncoder queryMapEncoder = QueryMap.MapEncoder.FIELD.instance();
protected ErrorDecoder errorDecoder = new ErrorDecoder.Default();
protected Options options = new Options();
protected InvocationHandlerFactory invocationHandlerFactory =
Expand Down
1 change: 1 addition & 0 deletions core/src/main/java/feign/Contract.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -315,6 +315,7 @@ public Default() {
checkState(data.queryMapIndex() == null,
"QueryMap annotation was present on multiple parameters.");
data.queryMapIndex(paramIndex);
data.queryMapEncoder(queryMap.mapEncoder().instance());
});
super.registerParameterAnnotation(HeaderMap.class, (queryMap, data, paramIndex) -> {
checkState(data.headerMapIndex() == null,
Expand Down
10 changes: 10 additions & 0 deletions core/src/main/java/feign/MethodMetadata.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ public final class MethodMetadata implements Serializable {
private Integer bodyIndex;
private Integer headerMapIndex;
private Integer queryMapIndex;
private QueryMapEncoder queryMapEncoder;
private boolean alwaysEncodeBody;
private transient Type bodyType;
private final RequestTemplate template = new RequestTemplate();
Expand Down Expand Up @@ -109,6 +110,15 @@ public MethodMetadata queryMapIndex(Integer queryMapIndex) {
return this;
}

public QueryMapEncoder queryMapEncoder() {
return queryMapEncoder;
}

public MethodMetadata queryMapEncoder(QueryMapEncoder queryMapEncoder) {
this.queryMapEncoder = queryMapEncoder;
return this;
}
Comment on lines +113 to +120
Copy link

@coderabbitai coderabbitai bot Sep 7, 2023 •
edited
Loading

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 potential NullPointerExceptions.

public MethodMetadata queryMapEncoder(QueryMapEncoder queryMapEncoder) {
+   if (queryMapEncoder == null) {
+       throw new IllegalArgumentException("QueryMapEncoder cannot be null");
+   }
    this.queryMapEncoder = queryMapEncoder;
    return this;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

queryMapEncoder is nullable

Copy link

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 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.


@Experimental
public boolean alwaysEncodeBody() {
return alwaysEncodeBody;
Expand Down
24 changes: 24 additions & 0 deletions core/src/main/java/feign/QueryMap.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -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
Copy link

@coderabbitai coderabbitai bot Sep 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this issue is correctly handled

Copy link

Choose a reason for hiding this comment

The 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 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!

}
10 changes: 6 additions & 4 deletions core/src/main/java/feign/RequestTemplateFactoryResolver.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link

@coderabbitai coderabbitai bot Sep 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

The 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 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);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

what can I do here?

Copy link

Choose a reason for hiding this comment

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

@pilak: There are a few things you can do here:

  1. 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?

  2. 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.

  3. Understand the context: Try to understand why these changes were made. What problem is this solving? How does it fit into the larger project?

  4. 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.

  5. Approve or request changes: Based on your review, you can approve the changes if everything looks good, or request further changes if necessary.

Expand Down
9 changes: 9 additions & 0 deletions core/src/test/java/feign/ChildPojo.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,15 @@
class ParentPojo {
public String parentPublicProperty;
protected String parentProtectedProperty;
private String parentPrivatePropertyAlteredByGetter;

public String getParentPrivatePropertyAlteredByGetter() {
return parentPrivatePropertyAlteredByGetter + "FromGetter";
}

public void setParentPrivatePropertyAlteredByGetter(String parentPrivatePropertyAlteredByGetter) {
this.parentPrivatePropertyAlteredByGetter = parentPrivatePropertyAlteredByGetter;
}

public String getParentPublicProperty() {
return parentPublicProperty;
Expand Down
32 changes: 31 additions & 1 deletion core/src/test/java/feign/FeignTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link

Choose a reason for hiding this comment

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

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;

Expand Down Expand Up @@ -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
Copy link

Choose a reason for hiding this comment

The 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 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.

}

@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
Expand Down Expand Up @@ -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);

Expand Down