Annotation based handler dispatch for REST/RPC protocols

Use annotations to describe the services exported by a protocol handler class and their parameterization. 
Additional services can be exported by creating Guice bindings. 
RPC handlers can now support more operations than just the same list that REST supports.
Added "system" endpoint to introspect the set of available endpoints, this will be used for the os-lite implementation.
Fixed many, many API tests

The next phase of this work is to migrate all the basic protocol handling into common so it can be used to implement makeRequest in terms of our standard protocols.

[louiscryan, 2009-01-28 23:21:20]

Use annotations to describe the services exported by a protocol handler class
and their parameterization. 
Additional services can be exported by creating Guice bindings. 
RPC handlers can now support more operations than just the same list that REST
supports.
Added "system" endpoint to introspect the set of available endpoints, this will
be used for the os-lite implementation.
Fixed many, many API tests

The next phase of this work is to migrate all the basic protocol handling into
common so it can be used to implement makeRequest in terms of our standard
protocols.

[etnu00, 2009-01-29 03:52:19]

http://codereview.appspot.com/12646/diff/1/30
File
java/social-api/src/main/java/org/apache/shindig/social/core/config/SocialApiGuiceModule.java
(right):

http://codereview.appspot.com/12646/diff/1/30#newcode24
Line 24: import com.google.inject.name.Names;
Your import order is wrong.

http://codereview.appspot.com/12646/diff/1/27
File
java/social-api/src/main/java/org/apache/shindig/social/opensocial/service/ActivityHandler.java
(right):

http://codereview.appspot.com/12646/diff/1/27#newcode142
Line 142: String container =
Objects.firstNonNull(request.getToken().getContainer(), "default");
Use ContainerConfig.DEFAULT_CONTAINER

http://codereview.appspot.com/12646/diff/1/15
File
java/social-api/src/main/java/org/apache/shindig/social/opensocial/service/BasicHandlerRegistry.java
(right):

http://codereview.appspot.com/12646/diff/1/15#newcode48
Line 48: public class BasicHandlerRegistry implements HandlerRegistry {
Suggest DefaultHandlerRegistry. This implementation seems perfectly suitable for
many production deployments.

http://codereview.appspot.com/12646/diff/1/15#newcode50
Line 50: private final Map<String, RestHandler> httpOperations =
Maps.newHashMap();
restOperations? Both RPC and REST are HTTP...

http://codereview.appspot.com/12646/diff/1/15#newcode53
Line 53: private Injector injector;
final

http://codereview.appspot.com/12646/diff/1/15#newcode116
Line 116: public void addHandler(Object handler) {
Why not make the public signature take varargs and eliminate this?

http://codereview.appspot.com/12646/diff/1/15#newcode218
Line 218: public Future execute(String path, Map parameters, Reader body,
Why the missing type arguments on some of these parameters (Map, Future, etc.)?

http://codereview.appspot.com/12646/diff/1/33
File
java/social-api/src/main/java/org/apache/shindig/social/opensocial/service/HandlerPreconditions.java
(right):

http://codereview.appspot.com/12646/diff/1/33#newcode40
Line 40: public static void requireEmpty(Collection<?> list, String message)
throws SocialSpiException {
Why is the arg named list here and coll in the other methods?

http://codereview.appspot.com/12646/diff/1/3
File
java/social-api/src/main/java/org/apache/shindig/social/opensocial/service/HandlerRegistry.java
(right):

http://codereview.appspot.com/12646/diff/1/3#newcode28
Line 28: * registers the three standard Shindig handlers.
Documentation doesn't reflect what BasicHandlerRegistry is doing, since it has
handlers injected.

http://codereview.appspot.com/12646/diff/1/16
File
java/social-api/src/main/java/org/apache/shindig/social/opensocial/service/Operation.java
(right):

http://codereview.appspot.com/12646/diff/1/16#newcode36
Line 36: String[] httpMethods();
Could we just call this "methods"?

http://codereview.appspot.com/12646/diff/1/16#newcode39
Line 39: * The parameter name to bind the body content to
The documentation here isn't clear. Is this referring to the result of the
operation?

http://codereview.appspot.com/12646/diff/1/16#newcode44
Line 44: * The path to match for the operation ot override the service
typo

http://codereview.appspot.com/12646/diff/1/12
File
java/social-api/src/main/java/org/apache/shindig/social/sample/service/SampleContainerHandler.java
(right):

http://codereview.appspot.com/12646/diff/1/12#newcode49
Line 49: * We don't distinguish between put and post for these urls.
The documentation contradicts the annotation.

[bobevans, 2009-01-30 22:17:31]

LGTM

http://codereview.appspot.com/12646/diff/1/15
File
java/social-api/src/main/java/org/apache/shindig/social/opensocial/service/BasicHandlerRegistry.java
(right):

http://codereview.appspot.com/12646/diff/1/15#newcode123
Line 123: if (handler instanceof Class) {
Micro-ordering issue:
Should the case of getting an instance from a class that was passed in happen
before the Class handlerType = handler.getClass() line? It seems like a prior
case to just getting the class of the handler.

http://codereview.appspot.com/12646/diff/1/15#newcode134
Line 134: for (Method m : handlerType.getMethods()) {
It would be good to extract some methods here, like,
mapServicesExposedBy(Handler) or something like that.

http://codereview.appspot.com/12646/diff/1/15#newcode143
Line 143: for (String httpMethod : op.httpMethods()) {
Same here, this looks like a discrete method, e.g., mapHttpMethods

http://codereview.appspot.com/12646/diff/1/15#newcode227
Line 227: String[] expectedUrl = service.path().split("/");
micro-optimization: this never changes right? Should it be created at
construction time?

http://codereview.appspot.com/12646/diff/1/15#newcode229
Line 229: for (int i = 1; i < actualUrl.length; i++) {
Another extractable method, e.g., extractParametersFromUrl

http://codereview.appspot.com/12646/diff/1/23
File
java/social-api/src/main/java/org/apache/shindig/social/opensocial/service/DataServiceServlet.java
(left):

http://codereview.appspot.com/12646/diff/1/23#oldcode56
Line 56: private static final Map<String, String> SERVICE_TO_SUPPORTED_FIELD_MAP
=
Shouldn't we just make the config match?

http://codereview.appspot.com/12646/diff/1/23#oldcode119
Line 119: BeanConverter converter) throws IOException {
Would it be cleaner if the converters were made into filters on entry and exit
to the handler instead of being passed in?

http://codereview.appspot.com/12646/diff/1/23
File
java/social-api/src/main/java/org/apache/shindig/social/opensocial/service/DataServiceServlet.java
(right):

http://codereview.appspot.com/12646/diff/1/23#newcode113
Line 113: Reader bodyReader = null;
Extract method

http://codereview.appspot.com/12646/diff/1/24
File
java/social-api/src/main/java/org/apache/shindig/social/opensocial/service/PersonHandler.java
(left):

http://codereview.appspot.com/12646/diff/1/24#oldcode75
Line 75: throw new IllegalArgumentException("Cannot fetch personIds for multiple
userIds");
I agree that this request is weird, but is it error-worthy? Should we just
convert it into a getPeople request?

http://codereview.appspot.com/12646/diff/1/17
File
java/social-api/src/main/java/org/apache/shindig/social/opensocial/service/Service.java
(right):

http://codereview.appspot.com/12646/diff/1/17#newcode45
Line 45: * missing path segmenets are bound to null or empty sets
typo

http://codereview.appspot.com/12646/diff/1/17#newcode56
Line 56: String path() default "";
It seems like creating an empty string default will increase the likelihood of
errors. What happens if multiple services map the same url path? Perhaps leaving
out a default would be better because there would not be a default option of
conflict?

[louiscryan, 2009-01-30 23:17:44]

http://codereview.appspot.com/12646/diff/1/30
File
java/social-api/src/main/java/org/apache/shindig/social/core/config/SocialApiGuiceModule.java
(right):

http://codereview.appspot.com/12646/diff/1/30#newcode24
Line 24: import com.google.inject.name.Names;
On 2009/01/29 03:52:19, etnu00 wrote:
> Your import order is wrong.

Done.

http://codereview.appspot.com/12646/diff/1/27
File
java/social-api/src/main/java/org/apache/shindig/social/opensocial/service/ActivityHandler.java
(right):

http://codereview.appspot.com/12646/diff/1/27#newcode142
Line 142: String container =
Objects.firstNonNull(request.getToken().getContainer(), "default");
On 2009/01/29 03:52:19, etnu00 wrote:
> Use ContainerConfig.DEFAULT_CONTAINER

Done.

http://codereview.appspot.com/12646/diff/1/15
File
java/social-api/src/main/java/org/apache/shindig/social/opensocial/service/BasicHandlerRegistry.java
(right):

http://codereview.appspot.com/12646/diff/1/15#newcode48
Line 48: public class BasicHandlerRegistry implements HandlerRegistry {
On 2009/01/29 03:52:19, etnu00 wrote:
> Suggest DefaultHandlerRegistry. This implementation seems perfectly suitable
for
> many production deployments.

Done.

http://codereview.appspot.com/12646/diff/1/15#newcode50
Line 50: private final Map<String, RestHandler> httpOperations =
Maps.newHashMap();
On 2009/01/29 03:52:19, etnu00 wrote:
> restOperations? Both RPC and REST are HTTP...

Done.

http://codereview.appspot.com/12646/diff/1/15#newcode53
Line 53: private Injector injector;
On 2009/01/29 03:52:19, etnu00 wrote:
> final

Done.

http://codereview.appspot.com/12646/diff/1/15#newcode116
Line 116: public void addHandler(Object handler) {
On 2009/01/29 03:52:19, etnu00 wrote:
> Why not make the public signature take varargs and eliminate this?

Done.

http://codereview.appspot.com/12646/diff/1/15#newcode123
Line 123: if (handler instanceof Class) {
On 2009/01/30 22:17:31, bobevans wrote:
> Micro-ordering issue:
> Should the case of getting an instance from a class that was passed in happen
> before the Class handlerType = handler.getClass() line? It seems like a prior
> case to just getting the class of the handler.

Done.

http://codereview.appspot.com/12646/diff/1/15#newcode134
Line 134: for (Method m : handlerType.getMethods()) {
On 2009/01/30 22:17:31, bobevans wrote:
> It would be good to extract some methods here, like,
> mapServicesExposedBy(Handler) or something like that.

Factored out creation of REST and RPC handlers

http://codereview.appspot.com/12646/diff/1/15#newcode143
Line 143: for (String httpMethod : op.httpMethods()) {
On 2009/01/30 22:17:31, bobevans wrote:
> Same here, this looks like a discrete method, e.g., mapHttpMethods 

Done.

http://codereview.appspot.com/12646/diff/1/15#newcode218
Line 218: public Future execute(String path, Map parameters, Reader body,
On 2009/01/29 03:52:19, etnu00 wrote:
> Why the missing type arguments on some of these parameters (Map, Future,
etc.)?

Done.

http://codereview.appspot.com/12646/diff/1/15#newcode227
Line 227: String[] expectedUrl = service.path().split("/");
On 2009/01/30 22:17:31, bobevans wrote:
> micro-optimization: this never changes right? Should it be created at
> construction time?

Done.

http://codereview.appspot.com/12646/diff/1/15#newcode229
Line 229: for (int i = 1; i < actualUrl.length; i++) {
On 2009/01/30 22:17:31, bobevans wrote:
> Another extractable method, e.g., extractParametersFromUrl

Done.

http://codereview.appspot.com/12646/diff/1/23
File
java/social-api/src/main/java/org/apache/shindig/social/opensocial/service/DataServiceServlet.java
(left):

http://codereview.appspot.com/12646/diff/1/23#oldcode56
Line 56: private static final Map<String, String> SERVICE_TO_SUPPORTED_FIELD_MAP
=
On 2009/01/30 22:17:31, bobevans wrote:
> Shouldn't we just make the config match?

Container config? Yes. Added comment in appropriate handlers

http://codereview.appspot.com/12646/diff/1/23#oldcode119
Line 119: BeanConverter converter) throws IOException {
On 2009/01/30 22:17:31, bobevans wrote:
> Would it be cleaner if the converters were made into filters on entry and exit
> to the handler instead of being passed in?

Doing so on entry is a little tricky because of the delayed parameter parsing. A
refactoring of this nature may be valuable though. Something to look at later.

http://codereview.appspot.com/12646/diff/1/23
File
java/social-api/src/main/java/org/apache/shindig/social/opensocial/service/DataServiceServlet.java
(right):

http://codereview.appspot.com/12646/diff/1/23#newcode113
Line 113: Reader bodyReader = null;
On 2009/01/30 22:17:31, bobevans wrote:
> Extract method

Hmm, theres no other re-use of this.

http://codereview.appspot.com/12646/diff/1/33
File
java/social-api/src/main/java/org/apache/shindig/social/opensocial/service/HandlerPreconditions.java
(right):

http://codereview.appspot.com/12646/diff/1/33#newcode40
Line 40: public static void requireEmpty(Collection<?> list, String message)
throws SocialSpiException {
On 2009/01/29 03:52:19, etnu00 wrote:
> Why is the arg named list here and coll in the other methods?

Done.

http://codereview.appspot.com/12646/diff/1/3
File
java/social-api/src/main/java/org/apache/shindig/social/opensocial/service/HandlerRegistry.java
(right):

http://codereview.appspot.com/12646/diff/1/3#newcode28
Line 28: * registers the three standard Shindig handlers.
On 2009/01/29 03:52:19, etnu00 wrote:
> Documentation doesn't reflect what BasicHandlerRegistry is doing, since it has
> handlers injected.

Done.

http://codereview.appspot.com/12646/diff/1/16
File
java/social-api/src/main/java/org/apache/shindig/social/opensocial/service/Operation.java
(right):

http://codereview.appspot.com/12646/diff/1/16#newcode36
Line 36: String[] httpMethods();
On 2009/01/29 03:52:19, etnu00 wrote:
> Could we just call this "methods"? 

Seems a little ambiguous given the context of annotating methods?

http://codereview.appspot.com/12646/diff/1/16#newcode39
Line 39: * The parameter name to bind the body content to
On 2009/01/29 03:52:19, etnu00 wrote:
> The documentation here isn't clear. Is this referring to the result of the
> operation?

Done.

http://codereview.appspot.com/12646/diff/1/16#newcode44
Line 44: * The path to match for the operation ot override the service
On 2009/01/29 03:52:19, etnu00 wrote:
> typo

Done.

http://codereview.appspot.com/12646/diff/1/24
File
java/social-api/src/main/java/org/apache/shindig/social/opensocial/service/PersonHandler.java
(left):

http://codereview.appspot.com/12646/diff/1/24#oldcode75
Line 75: throw new IllegalArgumentException("Cannot fetch personIds for multiple
userIds");
On 2009/01/30 22:17:31, bobevans wrote:
> I agree that this request is weird, but is it error-worthy? Should we just
> convert it into a getPeople request?

I think it is better to error for this. If the optionalPersonId isnt available
for one of the userIds what would the right semantic be? I think we can afford
to be quite strict.

http://codereview.appspot.com/12646/diff/1/17
File
java/social-api/src/main/java/org/apache/shindig/social/opensocial/service/Service.java
(right):

http://codereview.appspot.com/12646/diff/1/17#newcode45
Line 45: * missing path segmenets are bound to null or empty sets
On 2009/01/30 22:17:31, bobevans wrote:
> typo
> 

Done.

http://codereview.appspot.com/12646/diff/1/17#newcode56
Line 56: String path() default "";
On 2009/01/30 22:17:31, bobevans wrote:
> It seems like creating an empty string default will increase the likelihood of
> errors. What happens if multiple services map the same url path? Perhaps
leaving
> out a default would be better because there would not be a default option of
> conflict?

Its a little difficult to leave out though. It is an optional feature of the
annotation but if you remove the default you require it to be specified
everywhere its used which isnt exactly friendly and you cant use null either.

http://codereview.appspot.com/12646/diff/1/12
File
java/social-api/src/main/java/org/apache/shindig/social/sample/service/SampleContainerHandler.java
(right):

http://codereview.appspot.com/12646/diff/1/12#newcode49
Line 49: * We don't distinguish between put and post for these urls.
On 2009/01/29 03:52:19, etnu00 wrote:
> The documentation contradicts the annotation.

Actually we dont. They do the same thing

[louiscryan, 2009-01-30 23:19:35]

Updates per comments.