The issue I concern with this change is the json representation of the data. You can easily represent map or list, but not sure how mutimap will be represented. Please add tests with multiple items with same key. My preferred solution here would be to return list of pairs and let the client generate a map if needed. (Basically add something like the next function to Feature class: List<FeatureParam> getParamPairs())
On Wed, Mar 30, 2011 at 7:00 PM, <[email protected]> wrote: > Reviewers: dev_shindig.apache.org, > > Description: > Currently when the container receives gadget metadata, that metadata > does not include the feature parameters. This prevents container-side > features that rely on gadget supplied parameters from being able to use > them. The patch adds getParams to the Feature interface in > org.apache.shindig.gadgets.servlet.GadgetsHandlerApi and adds Multimap > support to org.apache.shindig.protocol.conversion.BeanDelegator. The > resulting metadata includes feature params. > > Please review this at http://codereview.appspot.com/4314055/ > > Affected files: > > > java/common/src/main/java/org/apache/shindig/protocol/conversion/BeanDelegator.java > > > java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerApi.java > > > ### Eclipse Workspace Patch 1.0 > #P shindig-project > Index: > java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerApi.java > =================================================================== > --- > java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerApi.java > (revision 4252) > +++ > java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerApi.java > (working copy) > @@ -20,6 +20,8 @@ > > import org.apache.shindig.common.uri.Uri; > import org.apache.shindig.protocol.conversion.BeanFilter.Unfiltered; > + > +import com.google.common.collect.Multimap; > // Keep imports clean, so it is clear what is used by API > > import java.util.List; > @@ -163,7 +165,7 @@ > public String getName(); > public boolean getRequired(); > // TODO: Handle multi map if params are needed > - // public Multimap<String, String> getParams(); > + public Multimap<String, String> getParams(); > } > > public interface LinkSpec { > Index: > java/common/src/main/java/org/apache/shindig/protocol/conversion/BeanDelegator.java > =================================================================== > --- > java/common/src/main/java/org/apache/shindig/protocol/conversion/BeanDelegator.java > (revision 4252) > +++ > java/common/src/main/java/org/apache/shindig/protocol/conversion/BeanDelegator.java > (working copy) > @@ -21,8 +21,10 @@ > import com.google.common.base.Preconditions; > import com.google.common.collect.ImmutableList; > import com.google.common.collect.ImmutableMap; > +import com.google.common.collect.ImmutableMultimap; > import com.google.common.collect.ImmutableSet; > import com.google.common.collect.Maps; > +import com.google.common.collect.Multimap; > > import org.apache.commons.lang.StringUtils; > import org.apache.shindig.common.uri.Uri; > @@ -161,6 +163,21 @@ > } > } > > + // Proxy each item in a map (map key is not proxied) > + if (source instanceof Multimap<?, ?>) { > + Multimap<?, ?> mapSource = (Multimap<?, ?>) source; > + if (!mapSource.isEmpty() && delegatedClasses.containsKey( > + mapSource.values().iterator().next().getClass())) { > + // Convert Map: > + ImmutableMultimap.Builder<Object, Object> mapBuilder = > ImmutableMultimap.builder(); > + for (Map.Entry<?, ?> entry : mapSource.entries()) { > + mapBuilder.put(entry.getKey(), createDelegator(entry.getValue(), > apiInterface)); > + } > + return (T) mapBuilder.build(); > + } else { > + return (T) source; > + } > + } > // Proxy each item in a list > if (source instanceof List<?>) { > List<?> listSource = (List<?>) source; > @@ -250,6 +267,8 @@ > type = paramType.getActualTypeArguments()[0]; > } else if (Map.class.isAssignableFrom((Class<?>) > paramType.getRawType())) { > type = paramType.getActualTypeArguments()[1]; > + } else if (Multimap.class.isAssignableFrom((Class<?>) > paramType.getRawType())) { > + type = paramType.getActualTypeArguments()[1]; > } > } > return (Class<?>) type; > @@ -313,6 +332,18 @@ > interfaceType = interfaceParamType.getActualTypeArguments()[1]; > return validateTypes(dataType, interfaceType); > } > + > + if (Multimap.class.isAssignableFrom((Class<?>) > dataParamType.getRawType()) && > + Multimap.class.isAssignableFrom((Class<?>) > interfaceParamType.getRawType())) { > + Type dataKeyType = dataParamType.getActualTypeArguments()[0]; > + Type interfaceKeyType = > interfaceParamType.getActualTypeArguments()[0]; > + if (dataKeyType != interfaceKeyType || > !PRIMITIVE_TYPE_CLASSES.contains(dataKeyType)) { > + return false; > + } > + dataType = dataParamType.getActualTypeArguments()[1]; > + interfaceType = > interfaceParamType.getActualTypeArguments()[1]; > + return validateTypes(dataType, interfaceType); > + } > // Only support Map and List generics > return false; > } > > >
