update javadoc and suggest areas for improvement

core/src/main/java/org/jclouds/rest/internal/AsyncRestClientProxy.java

@@ -18,11 +18,7 @@
  */
 package org.jclouds.rest.internal;
 
-/**
+
- * Generates RESTful clients from appropriately annotated interfaces.
- * 
- * @author Adrian Cole
- */
 import java.lang.annotation.Annotation;
 import java.lang.reflect.InvocationHandler;
 import java.lang.reflect.Method;
@@ -60,6 +56,30 @@ import com.google.inject.Provides;
 import com.google.inject.ProvisionException;
 import com.google.inject.TypeLiteral;
 
+/**
+ * Generates RESTful clients from appropriately annotated interfaces.
+ * <p/>
+ * Particularly, this code delegates calls to other things.
+ * <ol>
+ * <li>if the method has a {@link Provides} annotation, it responds via a
+ * {@link Injector} lookup</li>
+ * <li>if the method has a {@link Delegate} annotation, it responds with an
+ * instance of interface set in returnVal, adding the current JAXrs annotations
+ * to whatever are on that class.</li>
+ * <ul>
+ * <li>ex. if the method with {@link Delegate} has a {@link Path} annotation,
+ * and the returnval interface also has {@link Path}, these values are combined.
+ * </li>
+ * </ul>
+ * <li>if {@link RestAnnotationProcessor#delegationMap} contains a mapping for
+ * this, and the returnVal is properly assigned as a {@link ListenableFuture},
+ * it responds with an http implementation.</li>
+ * <li>otherwise a RuntimeException is thrown with a message including:
+ * {@code method is intended solely to set constants}</li>
+ * </ol>
+ * 
+ * @author Adrian Cole
+ */
 @Singleton
 public class AsyncRestClientProxy<T> implements InvocationHandler {
    private final Injector injector;
@@ -106,31 +126,43 @@ public class AsyncRestClientProxy<T> implements InvocationHandler {
       } else if (method.getName().equals("hashCode")) {
          return this.hashCode();
       } else if (method.isAnnotationPresent(Provides.class)) {
-         try {
+         return lookupValueFromGuice(method);
-            try {
-               Annotation qualifier = Iterables.find(ImmutableList.copyOf(method.getAnnotations()), isQualifierPresent);
-               return injector.getInstance(Key.get(method.getGenericReturnType(), qualifier));
-            } catch (NoSuchElementException e) {
-               return injector.getInstance(Key.get(method.getGenericReturnType()));
-            }
-         } catch (ProvisionException e) {
-            AuthorizationException aex = Throwables2.getFirstThrowableOfType(e, AuthorizationException.class);
-            if (aex != null)
-               throw aex;
-            throw e;
-         }
       } else if (method.isAnnotationPresent(Delegate.class)) {
-         return delegateMap.get(new ClassMethodArgs(method.getReturnType(), method, args));
+         return propagateContextToDelegate(method, args);
-      } else if (annotationProcessor.getDelegateOrNull(method) != null
+      } else if (isRestCall(method)) {
-               && ListenableFuture.class.isAssignableFrom(method.getReturnType())) {
+         return createListenableFutureForHttpRequestMappedToMethodAndArgs(method, args);
-         return createListenableFuture(method, args);
       } else {
          throw new RuntimeException("method is intended solely to set constants: " + method);
       }
    }
 
+   public boolean isRestCall(Method method) {
+      return annotationProcessor.getDelegateOrNull(method) != null
+               && ListenableFuture.class.isAssignableFrom(method.getReturnType());
+   }
+
+   public Object propagateContextToDelegate(Method method, Object[] args) throws ExecutionException {
+      return delegateMap.get(new ClassMethodArgs(method.getReturnType(), method, args));
+   }
+
+   public Object lookupValueFromGuice(Method method) {
+      try {
+         try {
+            Annotation qualifier = Iterables.find(ImmutableList.copyOf(method.getAnnotations()), isQualifierPresent);
+            return injector.getInstance(Key.get(method.getGenericReturnType(), qualifier));
+         } catch (NoSuchElementException e) {
+            return injector.getInstance(Key.get(method.getGenericReturnType()));
+         }
+      } catch (ProvisionException e) {
+         AuthorizationException aex = Throwables2.getFirstThrowableOfType(e, AuthorizationException.class);
+         if (aex != null)
+            throw aex;
+         throw e;
+      }
+   }
+
    @SuppressWarnings( { "unchecked", "rawtypes" })
-   private ListenableFuture<?> createListenableFuture(Method method, Object[] args) throws ExecutionException {
+   private ListenableFuture<?> createListenableFutureForHttpRequestMappedToMethodAndArgs(Method method, Object[] args) throws ExecutionException {
       method = annotationProcessor.getDelegateOrNull(method);
       logger.trace("Converting %s.%s", declaring.getSimpleName(), method.getName());
       Function<Exception, ?> exceptionParser = annotationProcessor

core/src/main/java/org/jclouds/rest/internal/RestAnnotationProcessor.java

@@ -184,6 +184,17 @@ public class RestAnnotationProcessor<T> {
    static final LoadingCache<Method, LoadingCache<Integer, Set<Annotation>>> methodToIndexOfParamToPostParamAnnotations = createMethodToIndexOfParamToAnnotation(PayloadParam.class);
    static final LoadingCache<Method, LoadingCache<Integer, Set<Annotation>>> methodToIndexOfParamToPartParamAnnotations = createMethodToIndexOfParamToAnnotation(PartParam.class);
    static final LoadingCache<Method, LoadingCache<Integer, Set<Annotation>>> methodToIndexOfParamToParamParserAnnotations = createMethodToIndexOfParamToAnnotation(ParamParser.class);
+   
+   /**
+    * Shared for all types of rest clients. this is read-only in this class, and
+    * currently populated only by {@link SeedAnnotationCache}
+    * 
+    * @see SeedAnnotationCache
+    */
+   // TODO: change this to a private final LoadingCache<MethodKey, Method>
+   // supplied by guice. The CacheLoader<MethodKey, Method> can be refactored
+   // out from SeedAnnotationCache. Potentially, preseed the cache, but only if
+   // this is uncomplicated.
    static final Map<MethodKey, Method> delegationMap = newHashMap();
 
    static LoadingCache<Method, LoadingCache<Integer, Set<Annotation>>> createMethodToIndexOfParamToAnnotation(
@@ -681,6 +692,7 @@ public class RestAnnotationProcessor<T> {
       }
    }
 
+   //TODO: change to LoadingCache<Method, List<HttpRequestFilter>> and move this logic to the CacheLoader.
    @VisibleForTesting
    List<HttpRequestFilter> getFiltersIfAnnotated(Method method) {
       List<HttpRequestFilter> filters = Lists.newArrayList();
@@ -703,6 +715,7 @@ public class RestAnnotationProcessor<T> {
       return filters;
    }
 
+   //TODO: change to LoadingCache<ClassMethodArgs, Optional<URI>> and move this logic to the CacheLoader.
    @VisibleForTesting
    public static URI getEndpointInParametersOrNull(Method method, final Object[] args, Injector injector)
          throws ExecutionException {
@@ -746,6 +759,7 @@ public class RestAnnotationProcessor<T> {
       return null;
    }
 
+   //TODO: change to LoadingCache<ClassMethodArgs, URI> and move this logic to the CacheLoader.
    public static URI getEndpointFor(Method method, Object[] args, Injector injector) throws ExecutionException {
       URI endpoint = getEndpointInParametersOrNull(method, args, injector);
       if (endpoint == null) {
@@ -784,6 +798,7 @@ public class RestAnnotationProcessor<T> {
    public static final TypeLiteral<ListenableFuture<HttpResponse>> futureHttpResponseLiteral = new TypeLiteral<ListenableFuture<HttpResponse>>() {
    };
 
+   //TODO: change to LoadingCache<Method, Key<? extends Function<HttpResponse, ?>>> and move this logic to the CacheLoader.
    @SuppressWarnings({ "unchecked", "rawtypes" })
    public static Key<? extends Function<HttpResponse, ?>> getParserOrThrowException(Method method) {
       ResponseParser annotation = method.getAnnotation(ResponseParser.class);
@@ -864,6 +879,7 @@ public class RestAnnotationProcessor<T> {
       return null;
    }
 
+   //TODO: change to LoadingCache<ClassMethodArgs, Optional<MapBinder>> and move this logic to the CacheLoader.
    public org.jclouds.rest.MapBinder getMapPayloadBinderOrNull(Method method, Object... args) {
       if (args != null) {
          for (Object arg : args) {
@@ -1020,7 +1036,8 @@ public class RestAnnotationProcessor<T> {
       return indexToPayloadAnnotation;
    }
 
-   private HttpRequestOptions findOptionsIn(Method method, Object... args) throws ExecutionException {
+  //TODO: change to LoadingCache<ClassMethodArgs, HttpRequestOptions and move this logic to the CacheLoader.
+  private HttpRequestOptions findOptionsIn(Method method, Object... args) throws ExecutionException {
       for (int index : methodToIndexesOfOptions.get(method)) {
          if (args.length >= index + 1) {// accomodate varargs
             if (args[index] instanceof Object[]) {
@@ -1155,6 +1172,7 @@ public class RestAnnotationProcessor<T> {
       return null;
    }
 
+   //TODO: change to LoadingCache<ClassMethodArgs, Multimap<String,String> and move this logic to the CacheLoader.
    private Multimap<String, String> getPathParamKeyValues(Method method, Object... args) throws ExecutionException {
       Multimap<String, String> pathParamValues = LinkedHashMultimap.create();
       LoadingCache<Integer, Set<Annotation>> indexToPathParam = methodToIndexOfParamToPathParamAnnotations.get(method);
@@ -1192,6 +1210,7 @@ public class RestAnnotationProcessor<T> {
       return encoded;
    }
 
+   //TODO: change to LoadingCache<ClassMethodArgs, Multimap<String,String> and move this logic to the CacheLoader.
    private Multimap<String, String> getMatrixParamKeyValues(Method method, Object... args) throws ExecutionException {
       Multimap<String, String> matrixParamValues = LinkedHashMultimap.create();
       LoadingCache<Integer, Set<Annotation>> indexToMatrixParam = methodToIndexOfParamToMatrixParamAnnotations.get(method);
@@ -1221,6 +1240,8 @@ public class RestAnnotationProcessor<T> {
       return matrixParamValues;
    }
 
+   //TODO: change to LoadingCache<ClassMethodArgs, Multimap<String,String> and move this logic to the CacheLoader.
+   //take care to manage size of this cache
    private Multimap<String, String> getFormParamKeyValues(Method method, Object... args) throws ExecutionException {
       Multimap<String, String> formParamValues = LinkedHashMultimap.create();
       LoadingCache<Integer, Set<Annotation>> indexToFormParam = methodToIndexOfParamToFormParamAnnotations.get(method);
@@ -1252,6 +1273,7 @@ public class RestAnnotationProcessor<T> {
       return formParamValues;
    }
 
+   //TODO: change to LoadingCache<ClassMethodArgs, Multimap<String,String> and move this logic to the CacheLoader.
    private Multimap<String, String> getQueryParamKeyValues(Method method, Object... args) throws ExecutionException {
       Multimap<String, String> queryParamValues = LinkedHashMultimap.create();
       LoadingCache<Integer, Set<Annotation>> indexToQueryParam = methodToIndexOfParamToQueryParamAnnotations.get(method);
@@ -1281,6 +1303,8 @@ public class RestAnnotationProcessor<T> {
       return queryParamValues;
    }
 
+   //TODO: change to LoadingCache<ClassMethodArgs, Map<String,String> and move this logic to the CacheLoader.
+   //take care to manage size of this cache
    private Map<String, String> buildPostParams(Method method, Object... args) throws ExecutionException {
       Map<String, String> postParams = newHashMap();
       LoadingCache<Integer, Set<Annotation>> indexToPathParam = methodToIndexOfParamToPostParamAnnotations.get(method);

core/src/main/java/org/jclouds/rest/internal/SeedAnnotationCache.java

@@ -58,11 +58,13 @@ import com.google.inject.Key;
 import com.google.inject.Provides;
 
 /**
- * seeds the annotation cache
+ * seeds the annotation cache located at
+ * {@link RestAnnotationProcessor#delegationMap}. Note this call is only
+ * intended to be called once per {@link RestContext} and avoids expensive
+ * lookups on each call.
  * 
  * @author Adrian Cole
  */
-
 @Singleton
 public class SeedAnnotationCache extends CacheLoader<Class<?>, Boolean> {
    @Resource