Step 2 in GadgetHandler restructure

The change break GadgetHandler class to two.
One class to process the data nd one to handle provide the json api.
The goal is to be able to export the same interface in other means then json rpc.

The change also extract the input parameters into the Api class, 
Note that the code take advantage of the BeanDelegator also for the input parameters, but there is no good verification in this case (because not all is needed in this case).

[zhoresh, 2010-08-10 18:27:36]

Update Token class passing and rename @Unflitered annotation

[johnfargo, 2010-08-12 08:13:49]

http://codereview.appspot.com/1925042/diff/7001/8005
File
java/common/src/main/java/org/apache/shindig/protocol/conversion/BeanDelegator.java
(right):

http://codereview.appspot.com/1925042/diff/7001/8005#newcode121
java/common/src/main/java/org/apache/shindig/protocol/conversion/BeanDelegator.java:121:
public Object createDelegator(Object source, Class<?> apiInterface) {
For better type-safety, we can do:
public T createDelegator(Object source, Class<T> apiInterface)

http://codereview.appspot.com/1925042/diff/7001/8001
File
java/common/src/test/java/org/apache/shindig/protocol/conversion/BeanDelegatorTest.java
(right):

http://codereview.appspot.com/1925042/diff/7001/8001#newcode133
java/common/src/test/java/org/apache/shindig/protocol/conversion/BeanDelegatorTest.java:133:
}
seems like it could make sense, perhaps for a new GadgetsHandlerApiTest class,
to test the actual uses of beanDelegator as applied to each Api class, with
corresponding concrete classes as source. WDYT?

http://codereview.appspot.com/1925042/diff/7001/8004
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandler.java
(right):

http://codereview.appspot.com/1925042/diff/7001/8004#newcode77
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandler.java:77:
ImmutableMap.<Enum<?>, Enum<?>>of());
We should just support a beanDelegator constructor w/o ctor args as a helper.

http://codereview.appspot.com/1925042/diff/7001/8004#newcode174
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandler.java:174:
};
nit: abstract into helper method.

http://codereview.appspot.com/1925042/diff/7001/8004#newcode194
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandler.java:194:
};
and use here

http://codereview.appspot.com/1925042/diff/7001/8007
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerApi.java
(right):

http://codereview.appspot.com/1925042/diff/7001/8007#newcode44
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerApi.java:44:
public int getModuleId();
ModuleId is largely deprecated -- we could probably get rid of it.

http://codereview.appspot.com/1925042/diff/7001/8002
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerService.java
(right):

http://codereview.appspot.com/1925042/diff/7001/8002#newcode49
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerService.java:49:
public class GadgetsHandlerService {
does this class have a test of its own, aside from being transitively tested in
GadgetsHandlerTest?

http://codereview.appspot.com/1925042/diff/7001/8002#newcode102
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerService.java:102:
public GadgetsHandlerApi.MetadataResponse
getMetadata(GadgetsHandlerApi.MetadataRequest request)
Each Request/Response method should have some JavaDoc.

http://codereview.appspot.com/1925042/diff/7001/8002#newcode257
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerService.java:257:
}
whole lotta boilerplate here. It would be nice if we could find a way to
construct an object of an interfaced-type out of multiple inputs rather than
just one, and let the Delegator do it.

One idea: delegator.createDelegator(Object source, Map<String, Object>
extraFields, Class<T> api)
--> the Map provides a way to delegate to "url", for instance, for APIs
"getAppId" and "getAppUrl".

http://codereview.appspot.com/1925042/diff/7001/8002#newcode286
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerService.java:286:
protected static class MetadataResponseData extends BaseResponseData {
these two classes already implement the Api interfaces - how about just saying
they do and avoid the beanDelegation?

[zhoresh, 2010-08-13 00:16:52]

Updated according to John comments

[zhoresh, 2010-08-13 00:19:06]

John thanks for the review.
Great idea to add the field map to the delegator!
It saved a good chunk of code. 
I wish java reflection proxy would also work for classes, then I caould have
change the GadgetContext as well.

http://codereview.appspot.com/1925042/diff/7001/8005
File
java/common/src/main/java/org/apache/shindig/protocol/conversion/BeanDelegator.java
(right):

http://codereview.appspot.com/1925042/diff/7001/8005#newcode121
java/common/src/main/java/org/apache/shindig/protocol/conversion/BeanDelegator.java:121:
public Object createDelegator(Object source, Class<?> apiInterface) {
On 2010/08/12 08:13:49, johnfargo wrote:
> For better type-safety, we can do:
> public T createDelegator(Object source, Class<T> apiInterface)

Done.

http://codereview.appspot.com/1925042/diff/7001/8001
File
java/common/src/test/java/org/apache/shindig/protocol/conversion/BeanDelegatorTest.java
(right):

http://codereview.appspot.com/1925042/diff/7001/8001#newcode133
java/common/src/test/java/org/apache/shindig/protocol/conversion/BeanDelegatorTest.java:133:
}
On 2010/08/12 08:13:49, johnfargo wrote:
> seems like it could make sense, perhaps for a new GadgetsHandlerApiTest class,
> to test the actual uses of beanDelegator as applied to each Api class, with
> corresponding concrete classes as source. WDYT?

This class only specify api not implementation.
The implementation class GadgetHandlerService is the one that introduce the
mapping and should have the tests.

http://codereview.appspot.com/1925042/diff/7001/8004
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandler.java
(right):

http://codereview.appspot.com/1925042/diff/7001/8004#newcode77
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandler.java:77:
ImmutableMap.<Enum<?>, Enum<?>>of());
On 2010/08/12 08:13:49, johnfargo wrote:
> We should just support a beanDelegator constructor w/o ctor args as a helper.

Done.

http://codereview.appspot.com/1925042/diff/7001/8004#newcode174
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandler.java:174:
};
On 2010/08/12 08:13:49, johnfargo wrote:
> nit: abstract into helper method.

Done.

http://codereview.appspot.com/1925042/diff/7001/8004#newcode194
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandler.java:194:
};
On 2010/08/12 08:13:49, johnfargo wrote:
> and use here

Done.

http://codereview.appspot.com/1925042/diff/7001/8007
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerApi.java
(right):

http://codereview.appspot.com/1925042/diff/7001/8007#newcode44
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerApi.java:44:
public int getModuleId();
On 2010/08/12 08:13:49, johnfargo wrote:
> ModuleId is largely deprecated -- we could probably get rid of it.

Done.

http://codereview.appspot.com/1925042/diff/7001/8002
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerService.java
(right):

http://codereview.appspot.com/1925042/diff/7001/8002#newcode49
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerService.java:49:
public class GadgetsHandlerService {
On 2010/08/12 08:13:49, johnfargo wrote:
> does this class have a test of its own, aside from being transitively tested
in
> GadgetsHandlerTest?

Added test to verify the delegation mapping, but no processing unit tests yet.
All functionality is tested by the Handler tests.

http://codereview.appspot.com/1925042/diff/7001/8002#newcode102
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerService.java:102:
public GadgetsHandlerApi.MetadataResponse
getMetadata(GadgetsHandlerApi.MetadataRequest request)
On 2010/08/12 08:13:49, johnfargo wrote:
> Each Request/Response method should have some JavaDoc.

Done.

http://codereview.appspot.com/1925042/diff/7001/8002#newcode257
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerService.java:257:
}
On 2010/08/12 08:13:49, johnfargo wrote:
> whole lotta boilerplate here. It would be nice if we could find a way to
> construct an object of an interfaced-type out of multiple inputs rather than
> just one, and let the Delegator do it.
> 
> One idea: delegator.createDelegator(Object source, Map<String, Object>
> extraFields, Class<T> api)
> --> the Map provides a way to delegate to "url", for instance, for APIs
> "getAppId" and "getAppUrl".

Great idea! done

http://codereview.appspot.com/1925042/diff/7001/8002#newcode286
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerService.java:286:
protected static class MetadataResponseData extends BaseResponseData {
On 2010/08/12 08:13:49, johnfargo wrote:
> these two classes already implement the Api interfaces - how about just saying
> they do and avoid the beanDelegation?

The problem is that the fields are still the concrete and not the interface
classes. But with the new way of specifying fields map, it can be basically be a
delegator! :-)

[zhoresh, 2010-08-20 18:06:03]

+jtarrio

Jacobo, please help in the review now that John is off for a while

Thanks
Ziv


On Thu, Aug 12, 2010 at 5:19 PM, <zhoresh@gmail.com> wrote:

> John thanks for the review.
> Great idea to add the field map to the delegator!
> It saved a good chunk of code.
> I wish java reflection proxy would also work for classes, then I caould
> have change the GadgetContext as well.
>
>
>
>
> http://codereview.appspot.com/1925042/diff/7001/8005
> File
>
>
java/common/src/main/java/org/apache/shindig/protocol/conversion/BeanDelegator.java
> (right):
>
> http://codereview.appspot.com/1925042/diff/7001/8005#newcode121
>
>
java/common/src/main/java/org/apache/shindig/protocol/conversion/BeanDelegator.java:121:
> public Object createDelegator(Object source, Class<?> apiInterface) {
> On 2010/08/12 08:13:49, johnfargo wrote:
>
>> For better type-safety, we can do:
>> public T createDelegator(Object source, Class<T> apiInterface)
>>
>
> Done.
>
>
> http://codereview.appspot.com/1925042/diff/7001/8001
> File
>
>
java/common/src/test/java/org/apache/shindig/protocol/conversion/BeanDelegatorTest.java
> (right):
>
> http://codereview.appspot.com/1925042/diff/7001/8001#newcode133
>
>
java/common/src/test/java/org/apache/shindig/protocol/conversion/BeanDelegatorTest.java:133:
> }
> On 2010/08/12 08:13:49, johnfargo wrote:
>
>> seems like it could make sense, perhaps for a new
>>
> GadgetsHandlerApiTest class,
>
>> to test the actual uses of beanDelegator as applied to each Api class,
>>
> with
>
>> corresponding concrete classes as source. WDYT?
>>
>
> This class only specify api not implementation.
> The implementation class GadgetHandlerService is the one that introduce
> the mapping and should have the tests.
>
>
> http://codereview.appspot.com/1925042/diff/7001/8004
> File
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandler.java
> (right):
>
> http://codereview.appspot.com/1925042/diff/7001/8004#newcode77
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandler.java:77:
> ImmutableMap.<Enum<?>, Enum<?>>of());
> On 2010/08/12 08:13:49, johnfargo wrote:
>
>> We should just support a beanDelegator constructor w/o ctor args as a
>>
> helper.
>
> Done.
>
>
> http://codereview.appspot.com/1925042/diff/7001/8004#newcode174
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandler.java:174:
> };
> On 2010/08/12 08:13:49, johnfargo wrote:
>
>> nit: abstract into helper method.
>>
>
> Done.
>
>
> http://codereview.appspot.com/1925042/diff/7001/8004#newcode194
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandler.java:194:
> };
> On 2010/08/12 08:13:49, johnfargo wrote:
>
>> and use here
>>
>
> Done.
>
>
> http://codereview.appspot.com/1925042/diff/7001/8007
> File
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerApi.java
> (right):
>
> http://codereview.appspot.com/1925042/diff/7001/8007#newcode44
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerApi.java:44:
> public int getModuleId();
> On 2010/08/12 08:13:49, johnfargo wrote:
>
>> ModuleId is largely deprecated -- we could probably get rid of it.
>>
>
> Done.
>
>
> http://codereview.appspot.com/1925042/diff/7001/8002
> File
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerService.java
> (right):
>
> http://codereview.appspot.com/1925042/diff/7001/8002#newcode49
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerService.java:49:
> public class GadgetsHandlerService {
> On 2010/08/12 08:13:49, johnfargo wrote:
>
>> does this class have a test of its own, aside from being transitively
>>
> tested in
>
>> GadgetsHandlerTest?
>>
>
> Added test to verify the delegation mapping, but no processing unit
> tests yet. All functionality is tested by the Handler tests.
>
>
> http://codereview.appspot.com/1925042/diff/7001/8002#newcode102
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerService.java:102:
> public GadgetsHandlerApi.MetadataResponse
> getMetadata(GadgetsHandlerApi.MetadataRequest request)
> On 2010/08/12 08:13:49, johnfargo wrote:
>
>> Each Request/Response method should have some JavaDoc.
>>
>
> Done.
>
>
> http://codereview.appspot.com/1925042/diff/7001/8002#newcode257
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerService.java:257:
> }
> On 2010/08/12 08:13:49, johnfargo wrote:
>
>> whole lotta boilerplate here. It would be nice if we could find a way
>>
> to
>
>> construct an object of an interfaced-type out of multiple inputs
>>
> rather than
>
>> just one, and let the Delegator do it.
>>
>
>  One idea: delegator.createDelegator(Object source, Map<String, Object>
>> extraFields, Class<T> api)
>> --> the Map provides a way to delegate to "url", for instance, for
>>
> APIs
>
>> "getAppId" and "getAppUrl".
>>
>
> Great idea! done
>
>
> http://codereview.appspot.com/1925042/diff/7001/8002#newcode286
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerService.java:286:
> protected static class MetadataResponseData extends BaseResponseData {
> On 2010/08/12 08:13:49, johnfargo wrote:
>
>> these two classes already implement the Api interfaces - how about
>>
> just saying
>
>> they do and avoid the beanDelegation?
>>
>
> The problem is that the fields are still the concrete and not the
> interface classes. But with the new way of specifying fields map, it can
> be basically be a delegator! :-)
>
>
> http://codereview.appspot.com/1925042/show
>

[jtarrio, 2010-08-20 22:18:24]

lgtm

[johnfargo, 2010-08-21 17:44:39]

On Thu, Aug 12, 2010 at 5:19 PM, <zhoresh@gmail.com> wrote:

> John thanks for the review.
> Great idea to add the field map to the delegator!
> It saved a good chunk of code.
> I wish java reflection proxy would also work for classes, then I caould
> have change the GadgetContext as well.
>
>
>
>
> http://codereview.appspot.com/1925042/diff/7001/8005
> File
>
>
java/common/src/main/java/org/apache/shindig/protocol/conversion/BeanDelegator.java
> (right):
>
> http://codereview.appspot.com/1925042/diff/7001/8005#newcode121
>
>
java/common/src/main/java/org/apache/shindig/protocol/conversion/BeanDelegator.java:121:
> public Object createDelegator(Object source, Class<?> apiInterface) {
> On 2010/08/12 08:13:49, johnfargo wrote:
>
>> For better type-safety, we can do:
>> public T createDelegator(Object source, Class<T> apiInterface)
>>
>
> Done.
>
>
> http://codereview.appspot.com/1925042/diff/7001/8001
> File
>
>
java/common/src/test/java/org/apache/shindig/protocol/conversion/BeanDelegatorTest.java
> (right):
>
> http://codereview.appspot.com/1925042/diff/7001/8001#newcode133
>
>
java/common/src/test/java/org/apache/shindig/protocol/conversion/BeanDelegatorTest.java:133:
> }
> On 2010/08/12 08:13:49, johnfargo wrote:
>
>> seems like it could make sense, perhaps for a new
>>
> GadgetsHandlerApiTest class,
>
>> to test the actual uses of beanDelegator as applied to each Api class,
>>
> with
>
>> corresponding concrete classes as source. WDYT?
>>
>
> This class only specify api not implementation.
> The implementation class GadgetHandlerService is the one that introduce
> the mapping and should have the tests.


Seems reasonable.


>
>
> http://codereview.appspot.com/1925042/diff/7001/8004
> File
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandler.java
> (right):
>
> http://codereview.appspot.com/1925042/diff/7001/8004#newcode77
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandler.java:77:
> ImmutableMap.<Enum<?>, Enum<?>>of());
> On 2010/08/12 08:13:49, johnfargo wrote:
>
>> We should just support a beanDelegator constructor w/o ctor args as a
>>
> helper.
>
> Done.
>
>
> http://codereview.appspot.com/1925042/diff/7001/8004#newcode174
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandler.java:174:
> };
> On 2010/08/12 08:13:49, johnfargo wrote:
>
>> nit: abstract into helper method.
>>
>
> Done.
>
>
> http://codereview.appspot.com/1925042/diff/7001/8004#newcode194
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandler.java:194:
> };
> On 2010/08/12 08:13:49, johnfargo wrote:
>
>> and use here
>>
>
> Done.
>
>
> http://codereview.appspot.com/1925042/diff/7001/8007
> File
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerApi.java
> (right):
>
> http://codereview.appspot.com/1925042/diff/7001/8007#newcode44
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerApi.java:44:
> public int getModuleId();
> On 2010/08/12 08:13:49, johnfargo wrote:
>
>> ModuleId is largely deprecated -- we could probably get rid of it.
>>
>
> Done.
>
>
> http://codereview.appspot.com/1925042/diff/7001/8002
> File
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerService.java
> (right):
>
> http://codereview.appspot.com/1925042/diff/7001/8002#newcode49
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerService.java:49:
> public class GadgetsHandlerService {
> On 2010/08/12 08:13:49, johnfargo wrote:
>
>> does this class have a test of its own, aside from being transitively
>>
> tested in
>
>> GadgetsHandlerTest?
>>
>
> Added test to verify the delegation mapping, but no processing unit
> tests yet. All functionality is tested by the Handler tests.


SGTM also.


>
>
> http://codereview.appspot.com/1925042/diff/7001/8002#newcode102
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerService.java:102:
> public GadgetsHandlerApi.MetadataResponse
> getMetadata(GadgetsHandlerApi.MetadataRequest request)
> On 2010/08/12 08:13:49, johnfargo wrote:
>
>> Each Request/Response method should have some JavaDoc.
>>
>
> Done.
>
>
> http://codereview.appspot.com/1925042/diff/7001/8002#newcode257
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerService.java:257:
> }
> On 2010/08/12 08:13:49, johnfargo wrote:
>
>> whole lotta boilerplate here. It would be nice if we could find a way
>>
> to
>
>> construct an object of an interfaced-type out of multiple inputs
>>
> rather than
>
>> just one, and let the Delegator do it.
>>
>
>  One idea: delegator.createDelegator(Object source, Map<String, Object>
>> extraFields, Class<T> api)
>> --> the Map provides a way to delegate to "url", for instance, for
>>
> APIs
>
>> "getAppId" and "getAppUrl".
>>
>
> Great idea! done


Excellent! Looks much cleaner. I suspect this will help in other places as
well.


>
>
> http://codereview.appspot.com/1925042/diff/7001/8002#newcode286
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerService.java:286:
> protected static class MetadataResponseData extends BaseResponseData {
> On 2010/08/12 08:13:49, johnfargo wrote:
>
>> these two classes already implement the Api interfaces - how about
>>
> just saying
>
>> they do and avoid the beanDelegation?
>>
>
> The problem is that the fields are still the concrete and not the
> interface classes. But with the new way of specifying fields map, it can
> be basically be a delegator! :-)


Looks sooo much better.


>


>
> http://codereview.appspot.com/1925042/show
>

[johnfargo, 2010-08-21 17:45:13]

LGTM!

Just in time for vacation. A few tiny little cleanup notes, nothing of
consequence.

http://codereview.appspot.com/1925042/diff/23001/24006
File
java/common/src/main/java/org/apache/shindig/protocol/conversion/BeanDelegator.java
(right):

http://codereview.appspot.com/1925042/diff/23001/24006#newcode52
java/common/src/main/java/org/apache/shindig/protocol/conversion/BeanDelegator.java:52:
public static final String NULL = "NULL";
micronit: suggest "<NULL sentinel>" in case it shows up in debugging here and
there

http://codereview.appspot.com/1925042/diff/23001/24006#newcode67
java/common/src/main/java/org/apache/shindig/protocol/conversion/BeanDelegator.java:67:
ImmutableMap.<Enum<?>, Enum<?>>of());
nanonit: 1 additional space indent

http://codereview.appspot.com/1925042/diff/23001/24006#newcode162
java/common/src/main/java/org/apache/shindig/protocol/conversion/BeanDelegator.java:162:
Preconditions.checkNotNull(extraFields);
could just support null and set extraFields to EXTRA_FIELDS