Issue 4314055: Feature params are not returned as part of gadget metadata
|
Created:
13 years, 1 month ago by igor1x Modified:
13 years, 1 month ago Base URL:
http://svn.apache.org/repos/asf/incubator/shindig/trunk/trunk/ Visibility:
Public. |
DescriptionCurrently 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.
Patch Set 1 #
MessagesTotal messages: 6
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, <igor1x@gmail.com> 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; > } > > >
Sign in to reply to this message.
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.
The tests should into
org.apache.shindig.gadgets.servlet.GadgetsHandlerServiceTest, correct?
On Thu, Mar 31, 2011 at 2:06 AM, Ziv Horesh <zhoresh@gmail.com> 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, <igor1x@gmail.com> 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;
>> }
>>
>>
>
Sign in to reply to this message.
On Thu, Mar 31, 2011 at 8:54 AM, Igor Belakovskiy <igor1x@gmail.com> 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 <zhoresh@gmail.com> 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, <igor1x@gmail.com> 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; > >> } > >> > >> > > > >
Sign in to reply to this message.
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:igor1x@gmail.com>
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:zhoresh@gmail.com>
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:igor1x@gmail.com> 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;
> > >> }
> > >>
> > >>
> > >
> >
> >
Sign in to reply to this message.
done. On Thu, Mar 31, 2011 at 3:17 PM, <rbaxter85@gmail.com> 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:igor1x@gmail.com> 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:zhoresh@gmail.com> 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:igor1x@gmail.com> 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 -- lindner@inuus.com -- linkedin.com/in/plindner
Sign in to reply to this message.
|
|||||||||||||||||||||||||||||||||||||||||||||||
