Another patch for SHINDIG-912

[Paul Lindner, 2009-02-13 19:21:48]

Hi,

I added in code that caches the results of calls to getMethods() in the older
version of BeanJsonConverter.  Any idea what happened to that code?

See SHINDIG-633 or r700626 in svn

http://codereview.appspot.com/14088/diff/1/6
File
java/common/src/main/java/org/apache/shindig/protocol/conversion/BeanJsonConverter.java
(right):

http://codereview.appspot.com/14088/diff/1/6#newcode117
Line 117: 
The ConcurrentHashMap for getters/setters seems to have disappeared. This is
very, very important as calls to getMethods() are synchronized per-class.

[etnu00, 2009-03-17 00:30:25]

Updated for bit rot

[awiner, 2009-03-17 15:09:44]

http://codereview.appspot.com/14088/diff/2001/3006
File
java/common/src/main/java/org/apache/shindig/protocol/conversion/BeanJsonConverter.java
(right):

http://codereview.appspot.com/14088/diff/2001/3006#newcode77
Line 77: // Ensure consistent method ordering by using a linked hash map.
comment doesn't match code (didn't before, either)

http://codereview.appspot.com/14088/diff/2001/3006#newcode91
Line 91: private static String getPropertyName(Method method) {
method -> setter?  Clearer that this only works with set() methods.

http://codereview.appspot.com/14088/diff/2001/3006#newcode94
Line 94: String name = method.getName();
this includes static methods as well as non-static;  and methods with multiple
args or no args.  Shouldn't it be a bit more restrictive (at least for the
no-annotation codepath)?

http://codereview.appspot.com/14088/diff/2001/3006#newcode139
Line 139: return value;
return Boolean.TRUE.equals(value)?  Or "true".equals(String.valueOf(type));

http://codereview.appspot.com/14088/diff/2001/3006#newcode175
Line 175: } else if (Collection.class.isAssignableFrom(clazz)) {
List<T> is missing

http://codereview.appspot.com/14088/diff/2001/3006#newcode216
Line 216: for (Object o : type.getEnumConstants()) {
could be just Enum.valueOf(type, value);

http://codereview.appspot.com/14088/diff/2001/3006#newcode240
Line 240: Object out = injector.getInstance(Key.get(type));
injector.getInstance(type);

[etnu00, 2009-03-17 17:25:54]

http://codereview.appspot.com/14088/diff/2001/3006
File
java/common/src/main/java/org/apache/shindig/protocol/conversion/BeanJsonConverter.java
(right):

http://codereview.appspot.com/14088/diff/2001/3006#newcode77
Line 77: // Ensure consistent method ordering by using a linked hash map.
On 2009/03/17 15:09:44, awiner wrote:
> comment doesn't match code (didn't before, either)

Done.

http://codereview.appspot.com/14088/diff/2001/3006#newcode94
Line 94: String name = method.getName();
On 2009/03/17 15:09:44, awiner wrote:
> this includes static methods as well as non-static;  and methods with multiple
> args or no args.  Shouldn't it be a bit more restrictive (at least for the
> no-annotation codepath)?

Possibly, though I didn't think it would make much of a difference one way or
another, and the code is more complex to support the extra level of filtering.

http://codereview.appspot.com/14088/diff/2001/3006#newcode139
Line 139: return value;
On 2009/03/17 15:09:44, awiner wrote:
> return Boolean.TRUE.equals(value)?  Or "true".equals(String.valueOf(type));

Done.

http://codereview.appspot.com/14088/diff/2001/3006#newcode175
Line 175: } else if (Collection.class.isAssignableFrom(clazz)) {
On 2009/03/17 15:09:44, awiner wrote:
> List<T> is missing

How so? Collection.class.isAssignableFrom should cover any collection type,
including lists. Set is explicit because it's meaningfully different when
requested, but if it wasn't requested explicitly, a List is the most accurate
fall back type.

http://codereview.appspot.com/14088/diff/2001/3006#newcode216
Line 216: for (Object o : type.getEnumConstants()) {
On 2009/03/17 15:09:44, awiner wrote:
> could be just Enum.valueOf(type, value);

This doesn't work for the enums that have overridden toString though, which
happens in the Java -> JSON mappings.

http://codereview.appspot.com/14088/diff/2001/3006#newcode240
Line 240: Object out = injector.getInstance(Key.get(type));
On 2009/03/17 15:09:44, awiner wrote:
> injector.getInstance(type);

getInstance doesn't work on Type, it works on Class<?>, or Key. I'd have to cast
to Class<?>, which would make parameterized type population break.

[etnu00, 2009-03-17 17:50:30]

Minor tweaks

[awiner, 2009-03-17 20:24:04]

http://codereview.appspot.com/14088/diff/2001/3006
File
java/common/src/main/java/org/apache/shindig/protocol/conversion/BeanJsonConverter.java
(right):

http://codereview.appspot.com/14088/diff/2001/3006#newcode175
Line 175: } else if (Collection.class.isAssignableFrom(clazz)) {
On 2009/03/17 17:25:54, etnu00 wrote:
> On 2009/03/17 15:09:44, awiner wrote:
> > List<T> is missing
> 
> How so? Collection.class.isAssignableFrom should cover any collection type,
> including lists. Set is explicit because it's meaningfully different when
> requested, but if it wasn't requested explicitly, a List is the most accurate
> fall back type.

Good point.

http://codereview.appspot.com/14088/diff/2001/3006#newcode240
Line 240: Object out = injector.getInstance(Key.get(type));
On 2009/03/17 17:25:54, etnu00 wrote:
> On 2009/03/17 15:09:44, awiner wrote:
> > injector.getInstance(type);
> 
> getInstance doesn't work on Type, it works on Class<?>, or Key. I'd have to
cast
> to Class<?>, which would make parameterized type population break.

See line 239: this is a Class<?> here, not a Type.