[Issue 17] When a subclass with a field annotated with @key hides a superclass field annotated with @key, prefer the definition from the subclass.

http://codereview.appspot.com/6312057/

Author: rmistry

google-http-client/src/main/java/com/google/api/client/util/ClassInfo.java

@@ -169,13 +169,6 @@ public int compare(String s0, String s1) {
         return s0 == s1 ? 0 : s0 == null ? -1 : s1 == null ? 1 : s0.compareTo(s1);
       }
     });
-    // inherit from super class
-    Class<?> superClass = srcClass.getSuperclass();
-    if (superClass != null) {
-      ClassInfo superClassInfo = ClassInfo.of(superClass, ignoreCase);
-      nameToFieldInfoMap.putAll(superClassInfo.nameToFieldInfoMap);
-      nameSet.addAll(superClassInfo.names);
-    }
     // iterate over declared fields
     for (Field field : srcClass.getDeclaredFields()) {
       FieldInfo fieldInfo = FieldInfo.of(field);
@@ -196,6 +189,18 @@ public int compare(String s0, String s1) {
       nameToFieldInfoMap.put(fieldName, fieldInfo);
       nameSet.add(fieldName);
     }
+    // inherit from super class and add only fields not already in nameToFieldInfoMap
+    Class<?> superClass = srcClass.getSuperclass();
+    if (superClass != null) {
+      ClassInfo superClassInfo = ClassInfo.of(superClass, ignoreCase);
+      nameSet.addAll(superClassInfo.names);
+      for (Map.Entry<String, FieldInfo> e : superClassInfo.nameToFieldInfoMap.entrySet()) {
+        String name = e.getKey();
+        if (!nameToFieldInfoMap.containsKey(name)) {
+          nameToFieldInfoMap.put(name, e.getValue());
+        }
+      }
+    }
     names = nameSet.isEmpty() ? Collections.<String>emptyList() : Collections.unmodifiableList(
         new ArrayList<String>(nameSet));
   }

google-http-client/src/test/java/com/google/api/client/http/HttpResponseTest.java

@@ -15,10 +15,14 @@
 package com.google.api.client.http;
 
 import com.google.api.client.http.LogContentTest.Recorder;
+import com.google.api.client.json.JsonObjectParser;
+import com.google.api.client.json.JsonString;
+import com.google.api.client.json.jackson.JacksonFactory;
 import com.google.api.client.testing.http.HttpTesting;
 import com.google.api.client.testing.http.MockHttpTransport;
 import com.google.api.client.testing.http.MockLowLevelHttpRequest;
 import com.google.api.client.testing.http.MockLowLevelHttpResponse;
+import com.google.api.client.util.GenericData;
 import com.google.api.client.util.Key;
 import com.google.api.client.util.StringUtils;
 
@@ -389,4 +393,63 @@ public LowLevelHttpResponse execute() throws IOException {
     response.parseAsString();
     recorder.assertMessages(expectedMessages);
   }
+
+  public static class A extends GenericData {
+    @Key
+    String foo;
+
+    public A() {
+      super();
+    }
+  }
+
+  public static class B extends A {
+    @SuppressWarnings("hiding")
+    @Key
+    @JsonString
+    Integer foo;
+
+    public B() {
+      super();
+    }
+  }
+
+  public static class C extends B {
+    @SuppressWarnings("hiding")
+    @Key
+    String foo;
+
+    public C() {
+      super();
+    }
+  }
+
+  public void testParseWithOverridenKey() throws IOException {
+    GenericData bData = new B();
+    bData.set("foo", 1);
+    GenericData cData = new C();
+    cData.set("foo", "1");
+
+    HttpTransport transport = new MockHttpTransport() {
+      @Override
+      public LowLevelHttpRequest buildGetRequest(String url) throws IOException {
+        return new MockLowLevelHttpRequest() {
+          @Override
+          public LowLevelHttpResponse execute() throws IOException {
+            MockLowLevelHttpResponse result = new MockLowLevelHttpResponse();
+            result.setContentType("application/json; charset=UTF-8");
+            result.setContent("{\"foo\": \"1\"}");
+            return result;
+          }
+        };
+      }
+    };
+    HttpRequest request =
+        transport.createRequestFactory().buildGetRequest(HttpTesting.SIMPLE_GENERIC_URL);
+    request.setParser(new JsonObjectParser(new JacksonFactory()));
+    HttpResponse response = request.execute();
+    assertEquals(bData, response.parseAs(B.class));
+    response = request.execute();
+    assertEquals(cData, response.parseAs(C.class));
+  }
 }

google-http-client/src/test/java/com/google/api/client/util/ClassInfoTest.java

@@ -71,6 +71,13 @@ public class C extends B {
     String abc2;
   }
 
+  public class A1 {
+    @Key
+    String foo;
+    @Key("foo")
+    String foo2;
+  }
+
   public void testNames() {
     assertEquals(ImmutableList.of("AbC", "b", "oc"), ClassInfo.of(A.class).names);
     assertEquals(ImmutableList.of("AbC", "b", "e", "oc"), ClassInfo.of(B.class).names);
@@ -88,11 +95,14 @@ public void testNames_enum() {
 
   public void testOf() {
     try {
-      ClassInfo.of(C.class, true);
+      ClassInfo.of(A1.class, true);
       fail("expected " + IllegalArgumentException.class);
     } catch (IllegalArgumentException e) {
       // expected
     }
+
+    ClassInfo.of(C.class, true);
+
     try {
       ClassInfo.of(E.class, true);
       fail("expected " + IllegalArgumentException.class);