JSON-RPC Gadgets Handler

This is a new gadgets handler that can be batched with other json-rpc, jsonp, rest and other requests.  It inherits heavily from JsonRpcHandler and provides most of it's functionality.

[zhoresh, 2010-06-24 18:04:01]

It looks pretty good. But I still not sure about the structure.

Here are few things that we have in mind:
- Support additional flags for the iframe url. In our case we implemented test
harness for gadgets and a flag need to be added to the gadget iframe url for it.
Maybe it just overriding IframeUriManager

- Return additional iframe style data, for example to pass the scrollable
attribute from the gagdet spec to the caller so it can apply to the iframe html
node.

- We also want to support proxy uri metadata request, and probably same for js
uri. Is that in the same handler? if so it might get big, maybe we should break
it to sub classes.

- For caching benefit of the metadata response, we want to allow a parametrized
request. Basically return place holders in the url and let the caller put the
values in.

I like the fact that the API is basically implemented by annotating the classes.
The risk with it is that it is not always clear that a class is part of the API
and someone might change the class without realizing the effect on the API. A
good example of this is the UserPref class.
Can we mark all thos classes somehow to not be modified, maybe even put them is
separate library so it will be easy to figure out what is the api?

[Paul Lindner, 2010-06-24 18:51:17]

I think it's logical to couple the Gadget Spec parser/generator to the
metadata output and I do agree that this can cause issues.

- When people change those classes it can impact the output format.
- However if/when we support extended gadget data it's all in one place.

Luckily there already is a wrapper around the GadgetSpec that implements the
filtering.  If people do change method signatures it will affect
GadgetsHandler.

More comments inline:

On Thu, Jun 24, 2010 at 11:04 AM, <zhoresh@gmail.com> wrote:

> It looks pretty good. But I still not sure about the structure.
>
> Here are few things that we have in mind:
> - Support additional flags for the iframe url. In our case we
> implemented test harness for gadgets and a flag need to be added to the
> gadget iframe url for it. Maybe it just overriding IframeUriManager
>

How is this triggered?  Is it in response to something in the request?  If
so this could be set in the GadgetContext and then the iframeUriManager
could generate differing output for this.


> - Return additional iframe style data, for example to pass the
> scrollable attribute from the gagdet spec to the caller so it can apply
> to the iframe html node.
>

These are in the modulePrefs output as Scrolling/Scaling/Singleton


>
> - We also want to support proxy uri metadata request, and probably same
> for js uri. Is that in the same handler? if so it might get big, maybe
> we should break it to sub classes.
>

Can you elaborate on those types of requests?  The nice part about the
current structure is that you could batch those requests with the metadata
call.


> - For caching benefit of the metadata response, we want to allow a
> parametrized request. Basically return place holders in the url and let
> the caller put the values in.
>

This is already happening.  The iframe URLs being generated have %language%,
%country% and user prefs substitution variables. If you use REST or JSONP
you can also cache the responses to this API too -- however we may need to
add code that will deal with a 'refreshInterval' param...

[zhoresh, 2010-06-24 20:53:04]

On Thu, Jun 24, 2010 at 11:51 AM, Paul Lindner <lindner@inuus.com> wrote:

> I think it's logical to couple the Gadget Spec parser/generator to the
> metadata output and I do agree that this can cause issues.
>
> - When people change those classes it can impact the output format.
> - However if/when we support extended gadget data it's all in one place.
>
> Luckily there already is a wrapper around the GadgetSpec that implements
> the filtering.  If people do change method signatures it will affect
> GadgetsHandler.
>
> More comments inline:
>
> On Thu, Jun 24, 2010 at 11:04 AM, <zhoresh@gmail.com> wrote:
>
>> It looks pretty good. But I still not sure about the structure.
>>
>> Here are few things that we have in mind:
>> - Support additional flags for the iframe url. In our case we
>> implemented test harness for gadgets and a flag need to be added to the
>> gadget iframe url for it. Maybe it just overriding IframeUriManager
>>
>
> How is this triggered?  Is it in response to something in the request?  If
> so this could be set in the GadgetContext and then the iframeUriManager
> could generate differing output for this.
>

Think about the debug flag. It is passed as parameter in the metadata
request, and then returned in the iframe url.
The test flag is the same deal, need to pass it to the metadata request, and
then get it as part of the iframe url.
And I agree it sounds like iframeUriManager need to deal with it, and get
the flag from the gadget context.



>
>
>> - Return additional iframe style data, for example to pass the
>> scrollable attribute from the gagdet spec to the caller so it can apply
>> to the iframe html node.
>>
>
> These are in the modulePrefs output as Scrolling/Scaling/Singleton
>
>
>>
>> - We also want to support proxy uri metadata request, and probably same
>> for js uri. Is that in the same handler? if so it might get big, maybe
>> we should break it to sub classes.
>>
>
> Can you elaborate on those types of requests?  The nice part about the
> current structure is that you could batch those requests with the metadata
> call.
>

The big benefit of such metadata request is that you get the version as part
of the proxy or js url, just like in the render reply,
which then allow us to utilize the browser cache much more.
A cool example is the google home page background images.
The images are proxied through a shindig server, and the url for the image
is created using such a meta data request.
The url has a version attached and long ttl for browser cache.


>
>> - For caching benefit of the metadata response, we want to allow a
>> parametrized request. Basically return place holders in the url and let
>> the caller put the values in.
>>
>
> This is already happening.  The iframe URLs being generated have
> %language%, %country% and user prefs substitution variables. If you use REST
> or JSONP you can also cache the responses to this API too -- however we may
> need to add code that will deal with a 'refreshInterval' param...
>
>

[Paul Lindner, 2010-06-24 21:23:45]

On Thu, Jun 24, 2010 at 1:52 PM, Ziv Horesh <zhoresh@gmail.com> wrote:

> How is this triggered?  Is it in response to something in the request?  If
>> so this could be set in the GadgetContext and then the iframeUriManager
>> could generate differing output for this.
>>
>
> Think about the debug flag. It is passed as parameter in the metadata
> request, and then returned in the iframe url.
> The test flag is the same deal, need to pass it to the metadata request,
> and then get it as part of the iframe url.
> And I agree it sounds like iframeUriManager need to deal with it, and get
> the flag from the gadget context.
>

Ah, I see.  The new way of doing this is to put a placeholder parameter in
the iframe url and have the client substitute it.  You'll notice that we
return iframe urls with debug=%debug% and we expect the container client
side code to substitute.

This allows for caching of the gadget metadata.

Can you elaborate on those types of requests?  The nice part about the
>> current structure is that you could batch those requests with the metadata
>> call.
>>
>
> The big benefit of such metadata request is that you get the version as
> part of the proxy or js url, just like in the render reply,
> which then allow us to utilize the browser cache much more.
> A cool example is the google home page background images.
> The images are proxied through a shindig server, and the url for the image
> is created using such a meta data request.
> The url has a version attached and long ttl for browser cache.
>

Interesting.  What do the parameters and response of the request look like?
 I could see a general wrapper around a number of uri generators

osapi.gadgets.proxyurl({url: "http://foo.com/i.gif", refreshInterval: 1800})
osapi.gadgets.jsurl({features: ['core','pubsub'], refreshInterval: 1800});

  http://proxy.com/foo.com/i.gif?v=1234
  http://shindig/gadgets/js/core:pubsub.js?v=1234



>>
>>> - For caching benefit of the metadata response, we want to allow a
>>> parametrized request. Basically return place holders in the url and let
>>> the caller put the values in.
>>>
>>
>> This is already happening.  The iframe URLs being generated have
>> %language%, %country% and user prefs substitution variables. If you use REST
>> or JSONP you can also cache the responses to this API too -- however we may
>> need to add code that will deal with a 'refreshInterval' param...
>>
>>
>

[mhermanto, 2010-07-07 22:02:18]

Sorry for the tardy response, but this looks good. Agree with Ziv's comments. We
have an endpoint which clients talk to via protocol buffer. That data model is
unfortunately not quit the same as the one expressed here (MetadataGadgetSpec,
FilteringGadgetSpec), but since we build on top of Shindig, we should continue
use this data model. Let's have this submitted, and we'll make the
appropriate/further changes as we go through our exercise in converting these
model objects to ours. Thanks Paul for doing this.

http://codereview.appspot.com/1722041/diff/6001/7004
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandler.java
(right):

http://codereview.appspot.com/1722041/diff/6001/7004#newcode73
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandler.java:73:
@Operation(httpMethods = {"POST","GET"}, path = "/metadata/{view}")
FMI mostly, CC JS will call an endpoint using OSAPI lib, via a POST /api/rpc,
which is fixed. Can this path /metadata/{view} (which is seems to vary with
view) be expressed by that OSAPI call?

http://codereview.appspot.com/1722041/diff/6001/7004#newcode75
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandler.java:75:
Set<String> gadgetUrls = ImmutableSet.copyOf(request.getListParameter("ids"));
Is the order of the response preserved, since we're turning a list into a set?

http://codereview.appspot.com/1722041/diff/6001/7004#newcode96
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandler.java:96:
throw new ProtocolException(HttpServletResponse.SC_INTERNAL_SERVER_ERROR,
"Processing interrupted", e);
Instead of throwing one error due to error from subset of gadget requests, let's
throw per-gadget error. This will allow common container code to only render
gadget that work successfully (should be better than failing altogether). It
seems like you're already doing this for ExecutionException below.

http://codereview.appspot.com/1722041/diff/6001/7004#newcode116
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandler.java:116:
return ALL_METADATA_FIELDS;
What's the use case for this?

http://codereview.appspot.com/1722041/diff/6001/7004#newcode168
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandler.java:168:
this.ignoreCache = Boolean.valueOf(request.getParameter("ignoreCache"));
Rename to nocache to keep it consistent with the iframe query param.

http://codereview.appspot.com/1722041/diff/6001/7004#newcode169
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandler.java:169:
this.debug = Boolean.valueOf(request.getParameter("debug"));
This should be fine, but CC will not need to call this endpoint upon variables
that don't require server-generated URI, and save HTTP request.

http://codereview.appspot.com/1722041/diff/6001/7004#newcode215
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandler.java:215:
// TODO
TODO: extract from request?

http://codereview.appspot.com/1722041/diff/6001/7009
File
java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/FakeIframeUriManager.java
(right):

http://codereview.appspot.com/1722041/diff/6001/7009#newcode27
java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/FakeIframeUriManager.java:27:
public class FakeIframeUriManager implements IframeUriManager {
Is this used by anything? If only by one class, just have it as an inner class.

[johnfargo, 2010-07-14 18:32:51]

Hey Paul:

Just committed this CL on your behalf, since A) it's new and thus won't break
anyone, B) we'd like to iterate on it re: comments from Ziv and Michael, as well
as other shared ideas; C) we're testing it out in our installation and wanted to
use the Shindig version ASAP.

Cheers,
John

On 2010/07/07 22:02:18, mhermanto wrote:
> Sorry for the tardy response, but this looks good. Agree with Ziv's comments.
We
> have an endpoint which clients talk to via protocol buffer. That data model is
> unfortunately not quit the same as the one expressed here (MetadataGadgetSpec,
> FilteringGadgetSpec), but since we build on top of Shindig, we should continue
> use this data model. Let's have this submitted, and we'll make the
> appropriate/further changes as we go through our exercise in converting these
> model objects to ours. Thanks Paul for doing this.
> 
> http://codereview.appspot.com/1722041/diff/6001/7004
> File
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandler.java
> (right):
> 
> http://codereview.appspot.com/1722041/diff/6001/7004#newcode73
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandler.java:73:
> @Operation(httpMethods = {"POST","GET"}, path = "/metadata/{view}")
> FMI mostly, CC JS will call an endpoint using OSAPI lib, via a POST /api/rpc,
> which is fixed. Can this path /metadata/{view} (which is seems to vary with
> view) be expressed by that OSAPI call?
> 
> http://codereview.appspot.com/1722041/diff/6001/7004#newcode75
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandler.java:75:
> Set<String> gadgetUrls = ImmutableSet.copyOf(request.getListParameter("ids"));
> Is the order of the response preserved, since we're turning a list into a set?
> 
> http://codereview.appspot.com/1722041/diff/6001/7004#newcode96
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandler.java:96:
> throw new ProtocolException(HttpServletResponse.SC_INTERNAL_SERVER_ERROR,
> "Processing interrupted", e);
> Instead of throwing one error due to error from subset of gadget requests,
let's
> throw per-gadget error. This will allow common container code to only render
> gadget that work successfully (should be better than failing altogether). It
> seems like you're already doing this for ExecutionException below.
> 
> http://codereview.appspot.com/1722041/diff/6001/7004#newcode116
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandler.java:116:
> return ALL_METADATA_FIELDS;
> What's the use case for this?
> 
> http://codereview.appspot.com/1722041/diff/6001/7004#newcode168
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandler.java:168:
> this.ignoreCache = Boolean.valueOf(request.getParameter("ignoreCache"));
> Rename to nocache to keep it consistent with the iframe query param.
> 
> http://codereview.appspot.com/1722041/diff/6001/7004#newcode169
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandler.java:169:
> this.debug = Boolean.valueOf(request.getParameter("debug"));
> This should be fine, but CC will not need to call this endpoint upon variables
> that don't require server-generated URI, and save HTTP request.
> 
> http://codereview.appspot.com/1722041/diff/6001/7004#newcode215
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandler.java:215:
> // TODO
> TODO: extract from request?
> 
> http://codereview.appspot.com/1722041/diff/6001/7009
> File
>
java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/FakeIframeUriManager.java
> (right):
> 
> http://codereview.appspot.com/1722041/diff/6001/7009#newcode27
>
java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/FakeIframeUriManager.java:27:
> public class FakeIframeUriManager implements IframeUriManager {
> Is this used by anything? If only by one class, just have it as an inner
class.

[Paul Lindner, 2010-07-14 23:48:05]

not a problem.  thanks!

Starting to catch up with a big backlog now.


On Wed, Jul 14, 2010 at 11:32 AM, <johnfargo@gmail.com> wrote:

> Hey Paul:
>
> Just committed this CL on your behalf, since A) it's new and thus won't
> break anyone, B) we'd like to iterate on it re: comments from Ziv and
> Michael, as well as other shared ideas; C) we're testing it out in our
> installation and wanted to use the Shindig version ASAP.
>
> Cheers,
> John
>
>
> On 2010/07/07 22:02:18, mhermanto wrote:
>
>> Sorry for the tardy response, but this looks good. Agree with Ziv's
>>
> comments. We
>
>> have an endpoint which clients talk to via protocol buffer. That data
>>
> model is
>
>> unfortunately not quit the same as the one expressed here
>>
> (MetadataGadgetSpec,
>
>> FilteringGadgetSpec), but since we build on top of Shindig, we should
>>
> continue
>
>> use this data model. Let's have this submitted, and we'll make the
>> appropriate/further changes as we go through our exercise in
>>
> converting these
>
>> model objects to ours. Thanks Paul for doing this.
>>
>
>  http://codereview.appspot.com/1722041/diff/6001/7004
>> File
>>
>
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandler.java
>
>> (right):
>>
>
>  http://codereview.appspot.com/1722041/diff/6001/7004#newcode73
>>
>
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandler.java:73:
>
>> @Operation(httpMethods = {"POST","GET"}, path = "/metadata/{view}")
>> FMI mostly, CC JS will call an endpoint using OSAPI lib, via a POST
>>
> /api/rpc,
>
>> which is fixed. Can this path /metadata/{view} (which is seems to vary
>>
> with
>
>> view) be expressed by that OSAPI call?
>>
>
>  http://codereview.appspot.com/1722041/diff/6001/7004#newcode75
>>
>
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandler.java:75:
>
>> Set<String> gadgetUrls =
>>
> ImmutableSet.copyOf(request.getListParameter("ids"));
>
>> Is the order of the response preserved, since we're turning a list
>>
> into a set?
>
>  http://codereview.appspot.com/1722041/diff/6001/7004#newcode96
>>
>
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandler.java:96:
>
>> throw new
>>
> ProtocolException(HttpServletResponse.SC_INTERNAL_SERVER_ERROR,
>
>> "Processing interrupted", e);
>> Instead of throwing one error due to error from subset of gadget
>>
> requests, let's
>
>> throw per-gadget error. This will allow common container code to only
>>
> render
>
>> gadget that work successfully (should be better than failing
>>
> altogether). It
>
>> seems like you're already doing this for ExecutionException below.
>>
>
>  http://codereview.appspot.com/1722041/diff/6001/7004#newcode116
>>
>
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandler.java:116:
>
>> return ALL_METADATA_FIELDS;
>> What's the use case for this?
>>
>
>  http://codereview.appspot.com/1722041/diff/6001/7004#newcode168
>>
>
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandler.java:168:
>
>> this.ignoreCache =
>>
> Boolean.valueOf(request.getParameter("ignoreCache"));
>
>> Rename to nocache to keep it consistent with the iframe query param.
>>
>
>  http://codereview.appspot.com/1722041/diff/6001/7004#newcode169
>>
>
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandler.java:169:
>
>> this.debug = Boolean.valueOf(request.getParameter("debug"));
>> This should be fine, but CC will not need to call this endpoint upon
>>
> variables
>
>> that don't require server-generated URI, and save HTTP request.
>>
>
>  http://codereview.appspot.com/1722041/diff/6001/7004#newcode215
>>
>
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandler.java:215:
>
>> // TODO
>> TODO: extract from request?
>>
>
>  http://codereview.appspot.com/1722041/diff/6001/7009
>> File
>>
>
>
>
java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/FakeIframeUriManager.java
>
>> (right):
>>
>
>  http://codereview.appspot.com/1722041/diff/6001/7009#newcode27
>>
>
>
>
java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/FakeIframeUriManager.java:27:
>
>> public class FakeIframeUriManager implements IframeUriManager {
>> Is this used by anything? If only by one class, just have it as an
>>
> inner class.
>
>
>
> http://codereview.appspot.com/1722041/show
>