Use the @Key definition from the subclass

To fix http://code.google.com/p/google-http-java-client/issues/detail?id=17 : When a subclass with a field annotated with @Key hides a superclass field annotated with @Key, prefer the definition from the subclass.

[yanivi, 2012-07-30 21:35:36]

http://codereview.appspot.com/6312057/diff/4001/google-http-client/src/main/j...
File google-http-client/src/main/java/com/google/api/client/util/ClassInfo.java
(left):

http://codereview.appspot.com/6312057/diff/4001/google-http-client/src/main/j...
google-http-client/src/main/java/com/google/api/client/util/ClassInfo.java:190:
Preconditions.checkArgument(conflictingFieldInfo == null,
no, we still want this check.  It's just that we only want it for fields of the
same class.  So for example this is still bad:

class A {
  @Key String foo;
  @Key("foo") String foo2;
}

http://codereview.appspot.com/6312057/diff/4001/google-http-client/src/main/j...
File google-http-client/src/main/java/com/google/api/client/util/ClassInfo.java
(right):

http://codereview.appspot.com/6312057/diff/4001/google-http-client/src/main/j...
google-http-client/src/main/java/com/google/api/client/util/ClassInfo.java:190:
nameToFieldInfoMap.put(fieldName, fieldInfo);
please add a comment mentioning that we are intentionally allowing a field in a
subclass to override a field from a superclass

[rmistry, 2012-07-31 18:58:04]

http://codereview.appspot.com/6312057/diff/4001/google-http-client/src/main/j...
File google-http-client/src/main/java/com/google/api/client/util/ClassInfo.java
(left):

http://codereview.appspot.com/6312057/diff/4001/google-http-client/src/main/j...
google-http-client/src/main/java/com/google/api/client/util/ClassInfo.java:190:
Preconditions.checkArgument(conflictingFieldInfo == null,
On 2012/07/30 21:35:36, yanivi wrote:
> no, we still want this check.  It's just that we only want it for fields of
the
> same class.  So for example this is still bad:
> 
> class A {
>   @Key String foo;
>   @Key("foo") String foo2;
> }

Done and added a test case.

http://codereview.appspot.com/6312057/diff/4001/google-http-client/src/main/j...
File google-http-client/src/main/java/com/google/api/client/util/ClassInfo.java
(right):

http://codereview.appspot.com/6312057/diff/4001/google-http-client/src/main/j...
google-http-client/src/main/java/com/google/api/client/util/ClassInfo.java:190:
nameToFieldInfoMap.put(fieldName, fieldInfo);
On 2012/07/30 21:35:36, yanivi wrote:
> please add a comment mentioning that we are intentionally allowing a field in
a
> subclass to override a field from a superclass

Not sure if this is needed because of the two maps. Let me know what you think
about the two map design.

[yanivi, 2012-08-01 11:42:33]

http://codereview.appspot.com/6312057/diff/7002/google-http-client/src/main/j...
File google-http-client/src/main/java/com/google/api/client/util/ClassInfo.java
(right):

http://codereview.appspot.com/6312057/diff/7002/google-http-client/src/main/j...
google-http-client/src/main/java/com/google/api/client/util/ClassInfo.java:179:
if (superClass != null) {
Maybe a slightly better fix is to move lines 179-183 to after the for-loop (to
right before 205)?  then you won't need superNameToFieldInfoMap, though of
course you'd need to iterate over the super fields to only put those that aren't
already in nameToFieldInfoMap.

[rmistry, 2012-08-01 12:49:25]

http://codereview.appspot.com/6312057/diff/7002/google-http-client/src/main/j...
File google-http-client/src/main/java/com/google/api/client/util/ClassInfo.java
(right):

http://codereview.appspot.com/6312057/diff/7002/google-http-client/src/main/j...
google-http-client/src/main/java/com/google/api/client/util/ClassInfo.java:179:
if (superClass != null) {
On 2012/08/01 11:42:33, yanivi wrote:
> Maybe a slightly better fix is to move lines 179-183 to after the for-loop (to
> right before 205)?  then you won't need superNameToFieldInfoMap, though of
> course you'd need to iterate over the super fields to only put those that
aren't
> already in nameToFieldInfoMap.

Done.

[yanivi, 2012-08-01 15:38:57]

http://codereview.appspot.com/6312057/diff/13002/google-http-client/src/main/...
File google-http-client/src/main/java/com/google/api/client/util/ClassInfo.java
(right):

http://codereview.appspot.com/6312057/diff/13002/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/util/ClassInfo.java:195:
for (Field field : superClass.getDeclaredFields()) {
why such complicated logic?  why not something simpler like:

    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());
        }
      }
    }

[rmistry, 2012-08-01 16:16:16]

http://codereview.appspot.com/6312057/diff/13002/google-http-client/src/main/...
File google-http-client/src/main/java/com/google/api/client/util/ClassInfo.java
(right):

http://codereview.appspot.com/6312057/diff/13002/google-http-client/src/main/...
google-http-client/src/main/java/com/google/api/client/util/ClassInfo.java:195:
for (Field field : superClass.getDeclaredFields()) {
On 2012/08/01 15:38:57, yanivi wrote:
> why such complicated logic?  why not something simpler like:
> 
>     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());
>         }
>       }
>     }

Done.

[yanivi, 2012-08-01 16:27:05]

LGTM