add BeanQueryMapEncoder (#802)

* changed default query encoder result from POJO field to getter property

* changed default query encoder result from POJO field to getter property

* reset mistakenly deleted file

* Create PropertyQueryMapEncoder and extract QueryMapEncoder.Default to FieldQueryMapEncoder

* rename PropertyQueryMapEncoder to BeanQueryMapEncoder and add README

* fix README

* add comments to QueryMapEncoder and remove deprecation on Default

* rename test name

* rename package name queryMap to querymap

* format code

README.md

@@ -690,6 +690,18 @@ public class Example {
 }
 ```
 
+When annotating objects with @QueryMap, the default encoder uses reflection to inspect provided objects Fields to expand the objects values into a query string. If you prefer that the query string be built using getter and setter methods, as defined in the Java Beans API, please use the BeanQueryMapEncoder
+
+```java
+public class Example {
+  public static void main(String[] args) {
+    MyApi myApi = Feign.builder()
+                 .queryMapEncoder(new BeanQueryMapEncoder())
+                 .target(MyApi.class, "https://api.hostname.com");
+  }
+}
+```
+
 ### Error Handling
 If you need more control over handling unexpected responses, Feign instances can
 register a custom `ErrorDecoder` via the builder.

core/src/main/java/feign/QueryMapEncoder.java

@@ -13,16 +13,16 @@
  */
 package feign;
 
-import feign.codec.EncodeException;
-import java.lang.reflect.Field;
-import java.util.ArrayList;
-import java.util.Collections;
-import java.util.HashMap;
-import java.util.List;
+import feign.querymap.FieldQueryMapEncoder;
+import feign.querymap.BeanQueryMapEncoder;
 import java.util.Map;
 
 /**
  * A QueryMapEncoder encodes Objects into maps of query parameter names to values.
+ *
+ * @see FieldQueryMapEncoder
+ * @see BeanQueryMapEncoder
+ *
  */
 public interface QueryMapEncoder {
 
@@ -34,55 +34,12 @@ public interface QueryMapEncoder {
    */
   Map<String, Object> encode(Object object);
 
-  class Default implements QueryMapEncoder {
-
-    private final Map<Class<?>, ObjectParamMetadata> classToMetadata =
-        new HashMap<Class<?>, ObjectParamMetadata>();
-
-    @Override
-    public Map<String, Object> encode(Object object) throws EncodeException {
-      try {
-        ObjectParamMetadata metadata = getMetadata(object.getClass());
-        Map<String, Object> fieldNameToValue = new HashMap<String, Object>();
-        for (Field field : metadata.objectFields) {
-          Object value = field.get(object);
-          if (value != null && value != object) {
-            fieldNameToValue.put(field.getName(), value);
-          }
-        }
-        return fieldNameToValue;
-      } catch (IllegalAccessException e) {
-        throw new EncodeException("Failure encoding object into query map", e);
-      }
-    }
-
-    private ObjectParamMetadata getMetadata(Class<?> objectType) {
-      ObjectParamMetadata metadata = classToMetadata.get(objectType);
-      if (metadata == null) {
-        metadata = ObjectParamMetadata.parseObjectType(objectType);
-        classToMetadata.put(objectType, metadata);
-      }
-      return metadata;
-    }
-
-    private static class ObjectParamMetadata {
-
-      private final List<Field> objectFields;
-
-      private ObjectParamMetadata(List<Field> objectFields) {
-        this.objectFields = Collections.unmodifiableList(objectFields);
-      }
-
-      private static ObjectParamMetadata parseObjectType(Class<?> type) {
-        List<Field> fields = new ArrayList<Field>();
-        for (Field field : type.getDeclaredFields()) {
-          if (!field.isAccessible()) {
-            field.setAccessible(true);
-          }
-          fields.add(field);
-        }
-        return new ObjectParamMetadata(fields);
-      }
-    }
+  /**
+   * @deprecated use {@link BeanQueryMapEncoder} instead. default encoder uses reflection to inspect
+   *             provided objects Fields to expand the objects values into a query string. If you
+   *             prefer that the query string be built using getter and setter methods, as defined
+   *             in the Java Beans API, please use the {@link BeanQueryMapEncoder}
+   */
+  class Default extends FieldQueryMapEncoder {
   }
 }

core/src/main/java/feign/querymap/BeanQueryMapEncoder.java

@@ -0,0 +1,85 @@
+/**
+ * Copyright 2012-2018 The Feign Authors
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except
+ * in compliance with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
+ * or implied. See the License for the specific language governing permissions and limitations under
+ * the License.
+ */
+package feign.querymap;
+
+import feign.QueryMapEncoder;
+import feign.codec.EncodeException;
+import java.beans.IntrospectionException;
+import java.beans.Introspector;
+import java.beans.PropertyDescriptor;
+import java.lang.reflect.InvocationTargetException;
+import java.util.*;
+
+/**
+ * the query map will be generated using java beans accessible getter property as query parameter
+ * names.
+ *
+ * eg: "/uri?name={name}&number={number}"
+ *
+ * order of included query parameters not guaranteed, and as usual, if any value is null, it will be
+ * left out
+ */
+public class BeanQueryMapEncoder implements QueryMapEncoder {
+  private final Map<Class<?>, ObjectParamMetadata> classToMetadata =
+      new HashMap<Class<?>, ObjectParamMetadata>();
+
+  @Override
+  public Map<String, Object> encode(Object object) throws EncodeException {
+    try {
+      ObjectParamMetadata metadata = getMetadata(object.getClass());
+      Map<String, Object> propertyNameToValue = new HashMap<String, Object>();
+      for (PropertyDescriptor pd : metadata.objectProperties) {
+        Object value = pd.getReadMethod().invoke(object);
+        if (value != null && value != object) {
+          propertyNameToValue.put(pd.getName(), value);
+        }
+      }
+      return propertyNameToValue;
+    } catch (IllegalAccessException | IntrospectionException | InvocationTargetException e) {
+      throw new EncodeException("Failure encoding object into query map", e);
+    }
+  }
+
+  private ObjectParamMetadata getMetadata(Class<?> objectType) throws IntrospectionException {
+    ObjectParamMetadata metadata = classToMetadata.get(objectType);
+    if (metadata == null) {
+      metadata = ObjectParamMetadata.parseObjectType(objectType);
+      classToMetadata.put(objectType, metadata);
+    }
+    return metadata;
+  }
+
+  private static class ObjectParamMetadata {
+
+    private final List<PropertyDescriptor> objectProperties;
+
+    private ObjectParamMetadata(List<PropertyDescriptor> objectProperties) {
+      this.objectProperties = Collections.unmodifiableList(objectProperties);
+    }
+
+    private static ObjectParamMetadata parseObjectType(Class<?> type)
+        throws IntrospectionException {
+      List<PropertyDescriptor> properties = new ArrayList<PropertyDescriptor>();
+
+      for (PropertyDescriptor pd : Introspector.getBeanInfo(type).getPropertyDescriptors()) {
+        boolean isGetterMethod = pd.getReadMethod() != null && !"class".equals(pd.getName());
+        if (isGetterMethod) {
+          properties.add(pd);
+        }
+      }
+
+      return new ObjectParamMetadata(properties);
+    }
+  }
+}

core/src/main/java/feign/querymap/FieldQueryMapEncoder.java

@@ -0,0 +1,79 @@
+/**
+ * Copyright 2012-2018 The Feign Authors
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except
+ * in compliance with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
+ * or implied. See the License for the specific language governing permissions and limitations under
+ * the License.
+ */
+package feign.querymap;
+
+import feign.QueryMapEncoder;
+import feign.codec.EncodeException;
+import java.lang.reflect.Field;
+import java.util.*;
+
+/**
+ * the query map will be generated using member variable names as query parameter names.
+ *
+ * eg: "/uri?name={name}&number={number}"
+ *
+ * order of included query parameters not guaranteed, and as usual, if any value is null, it will be
+ * left out
+ */
+public class FieldQueryMapEncoder implements QueryMapEncoder {
+
+  private final Map<Class<?>, ObjectParamMetadata> classToMetadata =
+      new HashMap<Class<?>, ObjectParamMetadata>();
+
+  @Override
+  public Map<String, Object> encode(Object object) throws EncodeException {
+    try {
+      ObjectParamMetadata metadata = getMetadata(object.getClass());
+      Map<String, Object> fieldNameToValue = new HashMap<String, Object>();
+      for (Field field : metadata.objectFields) {
+        Object value = field.get(object);
+        if (value != null && value != object) {
+          fieldNameToValue.put(field.getName(), value);
+        }
+      }
+      return fieldNameToValue;
+    } catch (IllegalAccessException e) {
+      throw new EncodeException("Failure encoding object into query map", e);
+    }
+  }
+
+  private ObjectParamMetadata getMetadata(Class<?> objectType) {
+    ObjectParamMetadata metadata = classToMetadata.get(objectType);
+    if (metadata == null) {
+      metadata = ObjectParamMetadata.parseObjectType(objectType);
+      classToMetadata.put(objectType, metadata);
+    }
+    return metadata;
+  }
+
+  private static class ObjectParamMetadata {
+
+    private final List<Field> objectFields;
+
+    private ObjectParamMetadata(List<Field> objectFields) {
+      this.objectFields = Collections.unmodifiableList(objectFields);
+    }
+
+    private static ObjectParamMetadata parseObjectType(Class<?> type) {
+      List<Field> fields = new ArrayList<Field>();
+      for (Field field : type.getDeclaredFields()) {
+        if (!field.isAccessible()) {
+          field.setAccessible(true);
+        }
+        fields.add(field);
+      }
+      return new ObjectParamMetadata(fields);
+    }
+  }
+}

core/src/test/java/feign/DefaultQueryMapEncoderTest.java

@@ -13,11 +13,11 @@
  */
 package feign;
 
-import java.util.HashMap;
-import java.util.Map;
 import org.junit.Rule;
 import org.junit.Test;
 import org.junit.rules.ExpectedException;
+import java.util.HashMap;
+import java.util.Map;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertTrue;
 

core/src/test/java/feign/FeignTest.java

@@ -15,37 +15,23 @@ package feign;
 
 import com.google.gson.Gson;
 import com.google.gson.reflect.TypeToken;
+import feign.Feign.ResponseMappingDecoder;
 import feign.Request.HttpMethod;
+import feign.Target.HardCodedTarget;
+import feign.codec.*;
+import feign.querymap.BeanQueryMapEncoder;
 import okhttp3.mockwebserver.MockResponse;
-import okhttp3.mockwebserver.SocketPolicy;
 import okhttp3.mockwebserver.MockWebServer;
-import java.util.Collection;
-import java.util.Collections;
-import java.util.HashMap;
-import java.util.LinkedHashMap;
+import okhttp3.mockwebserver.SocketPolicy;
 import okio.Buffer;
-import org.assertj.core.data.MapEntry;
 import org.junit.Rule;
 import org.junit.Test;
 import org.junit.rules.ExpectedException;
 import java.io.IOException;
 import java.lang.reflect.Type;
 import java.net.URI;
-import java.util.ArrayList;
-import java.util.Arrays;
-import java.util.Date;
-import java.util.List;
-import java.util.Map;
-import java.util.NoSuchElementException;
+import java.util.*;
 import java.util.concurrent.atomic.AtomicReference;
-import feign.Target.HardCodedTarget;
-import feign.codec.DecodeException;
-import feign.codec.Decoder;
-import feign.codec.EncodeException;
-import feign.codec.Encoder;
-import feign.codec.ErrorDecoder;
-import feign.codec.StringDecoder;
-import feign.Feign.ResponseMappingDecoder;
 import static feign.Util.UTF_8;
 import static feign.assertj.MockWebServerAssertions.assertThat;
 import static org.assertj.core.data.MapEntry.entry;
@@ -779,6 +765,50 @@ public class FeignTest {
     assertEquals(api.post(), "RESPONSE!");
   }
 
+  @Test
+  public void beanQueryMapEncoderWithPrivateGetterIgnored() throws Exception {
+    TestInterface api = new TestInterfaceBuilder().queryMapEndcoder(new BeanQueryMapEncoder())
+        .target("http://localhost:" + server.getPort());
+
+    PropertyPojo.ChildPojoClass propertyPojo = new PropertyPojo.ChildPojoClass();
+    propertyPojo.setPrivateGetterProperty("privateGetterProperty");
+    propertyPojo.setName("Name");
+    propertyPojo.setNumber(1);
+
+    server.enqueue(new MockResponse());
+    api.queryMapPropertyPojo(propertyPojo);
+    assertThat(server.takeRequest())
+        .hasQueryParams(Arrays.asList("name=Name", "number=1"));
+  }
+
+  @Test
+  public void beanQueryMapEncoderWithNullValueIgnored() throws Exception {
+    TestInterface api = new TestInterfaceBuilder().queryMapEndcoder(new BeanQueryMapEncoder())
+        .target("http://localhost:" + server.getPort());
+
+    PropertyPojo.ChildPojoClass propertyPojo = new PropertyPojo.ChildPojoClass();
+    propertyPojo.setName(null);
+    propertyPojo.setNumber(1);
+
+    server.enqueue(new MockResponse());
+    api.queryMapPropertyPojo(propertyPojo);
+    assertThat(server.takeRequest())
+        .hasQueryParams("number=1");
+  }
+
+  @Test
+  public void beanQueryMapEncoderWithEmptyParams() throws Exception {
+    TestInterface api = new TestInterfaceBuilder().queryMapEndcoder(new BeanQueryMapEncoder())
+        .target("http://localhost:" + server.getPort());
+
+    PropertyPojo.ChildPojoClass propertyPojo = new PropertyPojo.ChildPojoClass();
+
+    server.enqueue(new MockResponse());
+    api.queryMapPropertyPojo(propertyPojo);
+    assertThat(server.takeRequest())
+        .hasQueryParams("/");
+  }
+
   interface TestInterface {
 
     @RequestLine("POST /")
@@ -852,6 +882,9 @@ public class FeignTest {
     @RequestLine("GET /")
     void queryMapPojo(@QueryMap CustomPojo object);
 
+    @RequestLine("GET /")
+    void queryMapPropertyPojo(@QueryMap PropertyPojo object);
+
     class DateToMillis implements Param.Expander {
 
       @Override
@@ -957,6 +990,11 @@ public class FeignTest {
       return this;
     }
 
+    TestInterfaceBuilder queryMapEndcoder(QueryMapEncoder queryMapEncoder) {
+      delegate.queryMapEncoder(queryMapEncoder);
+      return this;
+    }
+
     TestInterface target(String url) {
       return delegate.target(TestInterface.class, url);
     }

core/src/test/java/feign/PropertyPojo.java

@@ -0,0 +1,50 @@
+/**
+ * Copyright 2012-2018 The Feign Authors
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except
+ * in compliance with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
+ * or implied. See the License for the specific language governing permissions and limitations under
+ * the License.
+ */
+package feign;
+
+public class PropertyPojo {
+
+  private String name;
+
+  public static class ChildPojoClass extends PropertyPojo {
+    private Integer number;
+
+    private String privateGetterProperty;
+
+    public Integer getNumber() {
+      return number;
+    }
+
+    public void setNumber(Integer number) {
+      this.number = number;
+    }
+
+    public void setPrivateGetterProperty(String privateGetterProperty) {
+      this.privateGetterProperty = privateGetterProperty;
+    }
+
+    private String getPrivateGetterProperty() {
+      return privateGetterProperty;
+    }
+  }
+
+  public String getName() {
+    return name;
+  }
+
+  public void setName(String name) {
+    this.name = name;
+  }
+
+}

core/src/test/java/feign/querymap/PropertyQueryMapEncoderTest.java

@@ -0,0 +1,130 @@
+/**
+ * Copyright 2012-2018 The Feign Authors
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except
+ * in compliance with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
+ * or implied. See the License for the specific language governing permissions and limitations under
+ * the License.
+ */
+package feign.querymap;
+
+import feign.QueryMapEncoder;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.rules.ExpectedException;
+import java.util.HashMap;
+import java.util.Map;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertTrue;
+
+public class PropertyQueryMapEncoderTest {
+
+  @Rule
+  public final ExpectedException thrown = ExpectedException.none();
+
+  private final QueryMapEncoder encoder = new BeanQueryMapEncoder();
+
+  @Test
+  public void testDefaultEncoder_normalClassWithValues() {
+    Map<String, Object> expected = new HashMap<>();
+    expected.put("foo", "fooz");
+    expected.put("bar", "barz");
+    expected.put("fooAppendBar", "foozbarz");
+    NormalObject normalObject = new NormalObject("fooz", "barz");
+
+    Map<String, Object> encodedMap = encoder.encode(normalObject);
+
+    assertEquals("Unexpected encoded query map", expected, encodedMap);
+  }
+
+  @Test
+  public void testDefaultEncoder_normalClassWithOutValues() {
+    NormalObject normalObject = new NormalObject(null, null);
+
+    Map<String, Object> encodedMap = encoder.encode(normalObject);
+
+    assertTrue("Non-empty map generated from null getter: " + encodedMap, encodedMap.isEmpty());
+  }
+
+  @Test
+  public void testDefaultEncoder_haveSuperClass() {
+    Map<String, Object> expected = new HashMap<>();
+    expected.put("page", 1);
+    expected.put("size", 10);
+    expected.put("query", "queryString");
+    SubClass subClass = new SubClass();
+    subClass.setPage(1);
+    subClass.setSize(10);
+    subClass.setQuery("queryString");
+
+    Map<String, Object> encodedMap = encoder.encode(subClass);
+
+    assertEquals("Unexpected encoded query map", expected, encodedMap);
+  }
+
+
+  class NormalObject {
+
+    private NormalObject(String foo, String bar) {
+      this.foo = foo;
+      this.bar = bar;
+    }
+
+    private String foo;
+    private String bar;
+
+    public String getFoo() {
+      return foo;
+    }
+
+    public String getBar() {
+      return bar;
+    }
+
+    public String getFooAppendBar() {
+      if (foo != null && bar != null) {
+        return foo + bar;
+      }
+      return null;
+    }
+  }
+
+  class SuperClass {
+    private int page;
+    private int size;
+
+    public int getPage() {
+      return page;
+    }
+
+    public void setPage(int page) {
+      this.page = page;
+    }
+
+    public int getSize() {
+      return size;
+    }
+
+    public void setSize(int size) {
+      this.size = size;
+    }
+  }
+
+  class SubClass extends SuperClass {
+
+    private String query;
+
+    public String getQuery() {
+      return query;
+    }
+
+    public void setQuery(String query) {
+      this.query = query;
+    }
+  }
+}