done.
On Thu, Mar 31, 2011 at 3:17 PM, <[email protected]> wrote: > One small thing, we should probably take out the TODO comment in > GadgetsHandlerApi now that we are handling multimap. > > On 2011/03/31 16:08:13, zhoresh wrote: > >> On Thu, Mar 31, 2011 at 8:54 AM, Igor Belakovskiy >> > <mailto:[email protected]> wrote: > > > A MultiMap just comes back as: >> > >> > "features":{"example-feature":{* >> > "params":{"param-name":["param-value1","param-value2"]}* >> > ,"name":"example-feature","required":false}, >> > >> > "core":{*"params":{}*,"name":"core","required":true}}} > >> > >> > >> > So accessible as *param-name[i]* in js, and easy enough for the >> > client to > >> > grab the first param value, if it's single valued. >> > >> > If you return a list of param pairs, you'd have to potentially >> > iterate over > >> > the entire list, to find the parameter name/value pair, right? Seems >> > like > >> > the Multimap representation is cleaner. >> > >> > > Sounds right to me, thanks for testing. >> > > > > >> > The tests should into >> > org.apache.shindig.gadgets.servlet.GadgetsHandlerServiceTest, >> > correct? > >> > >> > > Yes, and also please add a test in GadgetHandlerTest that verify the >> > json > >> object. >> Thanks for the patch! >> > > > > >> > On Thu, Mar 31, 2011 at 2:06 AM, Ziv Horesh >> > <mailto:[email protected]> wrote: > >> > > >> > > 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, <mailto:[email protected]> wrote: >> > >> >> > >> Reviewers: http://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; >> > >> } >> > >> >> > >> >> > > >> > >> > >> > > > > http://codereview.appspot.com/4314055/ > -- Paul Lindner -- [email protected] -- linkedin.com/in/plindner
