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/
