Update GadgetHandlerApi and improve error handling

The change include improving error handling by the GadgetHandler and don't fail the all batch request if only one item fails.
Support filtering of the rul (it is already the key in the reply so no need to put twice in result)

Also Updated API:
- Update error to include a message and an error code
- Move url to specific request (js request will not have url)
- Add duration to token requests (TODO actually use it...)
- Add Expiration time and response time so cache in CC can be implemented correctly (TODO actually provide correct time)

And to support the change it also update the CC JS code

[mhermanto, 2010-09-24 22:34:57]

http://codereview.appspot.com/2253044/diff/1/features/src/main/javascript/fea...
File features/src/main/javascript/features/container/service.js (right):

http://codereview.appspot.com/2253044/diff/1/features/src/main/javascript/fea...
features/src/main/javascript/features/container/service.js:143: var message = [
'Server failure to fetch token for gadget ' + id + '.' ].join('');
While you are in this file, please change the '+' to ','. Nit mistake on my
part.

http://codereview.appspot.com/2253044/diff/1/features/src/main/javascript/fea...
features/src/main/javascript/features/container/service.js:144:
finalResponse[id] = { 'error' : { 'message' : message } };
Also have a reasonable error.errorCode. It was originally, 'NOLOAD' (not quite
an HTTP error code). I asked this to consistently have a code and a message in
an error, whether it be an error returned by server or generated by client
(this).

http://codereview.appspot.com/2253044/diff/1/java/gadgets/src/main/java/org/a...
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandler.java
(right):

http://codereview.appspot.com/2253044/diff/1/java/gadgets/src/main/java/org/a...
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandler.java:126:
class CallableData extends Pair<String, GadgetsHandlerApi.BaseResponse> {
Why not just internally use individual members -- String id, GHA.BR data. It
will sound more welcoming for new member additions than Pair. Or, alternatively,
have a member Pair<> internally abstracted, without having the class extending
one.

http://codereview.appspot.com/2253044/diff/1/java/gadgets/src/main/java/org/a...
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandler.java:263:
this.durationSeconds = durationSeconds;
What is this field for? Token duration should be returned based on the gadget,
not requested by user.

http://codereview.appspot.com/2253044/diff/1/java/gadgets/src/main/java/org/a...
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerApi.java
(right):

http://codereview.appspot.com/2253044/diff/1/java/gadgets/src/main/java/org/a...
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerApi.java:50:
/** Url of the request, optional (for example for bad url error) */
Does optional mean it can be null, or an incorrect Uri?

http://codereview.appspot.com/2253044/diff/1/java/gadgets/src/main/java/org/a...
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerApi.java:53:
@Unfiltered
Necessary annotation? How about on getUrl()?

http://codereview.appspot.com/2253044/diff/1/java/gadgets/src/main/java/org/a...
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerApi.java:62:
public Uri getUrl();
Need this to be declared? It's already declared in BaseResponse.

http://codereview.appspot.com/2253044/diff/1/java/gadgets/src/main/java/org/a...
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerApi.java:169:
public Long getDurationSeconds();
Nit, getDurationInSeconds().

http://codereview.appspot.com/2253044/diff/1/java/gadgets/src/main/java/org/a...
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerService.java
(right):

http://codereview.appspot.com/2253044/diff/1/java/gadgets/src/main/java/org/a...
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerService.java:140:
private boolean fieldsInclude(Set<String> fields, String name) {
Nit, prefer rename to question-form doFieldsInclude() or includeFields(), much
like Java isEmpty(), contains(), etc.

[zhoresh, 2010-09-25 00:20:48]

Mike thanks for the review, reply below, new patch next.

http://codereview.appspot.com/2253044/diff/1/features/src/main/javascript/fea...
File features/src/main/javascript/features/container/service.js (right):

http://codereview.appspot.com/2253044/diff/1/features/src/main/javascript/fea...
features/src/main/javascript/features/container/service.js:143: var message = [
'Server failure to fetch token for gadget ' + id + '.' ].join('');
On 2010/09/24 22:34:57, mhermanto wrote:
> While you are in this file, please change the '+' to ','. Nit mistake on my
> part.

Removed

http://codereview.appspot.com/2253044/diff/1/features/src/main/javascript/fea...
features/src/main/javascript/features/container/service.js:144:
finalResponse[id] = { 'error' : { 'message' : message } };
On 2010/09/24 22:34:57, mhermanto wrote:
> Also have a reasonable error.errorCode. It was originally, 'NOLOAD' (not quite
> an HTTP error code). I asked this to consistently have a code and a message in
> an error, whether it be an error returned by server or generated by client
> (this).

Actually we can just pas on the main error which use same structure

http://codereview.appspot.com/2253044/diff/1/java/gadgets/src/main/java/org/a...
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandler.java
(right):

http://codereview.appspot.com/2253044/diff/1/java/gadgets/src/main/java/org/a...
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandler.java:126:
class CallableData extends Pair<String, GadgetsHandlerApi.BaseResponse> {
On 2010/09/24 22:34:57, mhermanto wrote:
> Why not just internally use individual members -- String id, GHA.BR data. It
> will sound more welcoming for new member additions than Pair. Or,
alternatively,
> have a member Pair<> internally abstracted, without having the class extending
> one.

It started as pair, and then to save typing I created "typedef". 
Changed to use simple class instead.

http://codereview.appspot.com/2253044/diff/1/java/gadgets/src/main/java/org/a...
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandler.java:263:
this.durationSeconds = durationSeconds;
On 2010/09/24 22:34:57, mhermanto wrote:
> What is this field for? Token duration should be returned based on the gadget,
> not requested by user.

I can see cases where a container might want to control this. Remove and added a
TODO in API if someone want it later.

http://codereview.appspot.com/2253044/diff/1/java/gadgets/src/main/java/org/a...
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerApi.java
(right):

http://codereview.appspot.com/2253044/diff/1/java/gadgets/src/main/java/org/a...
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerApi.java:50:
/** Url of the request, optional (for example for bad url error) */
On 2010/09/24 22:34:57, mhermanto wrote:
> Does optional mean it can be null, or an incorrect Uri?

Yes, it will be null if user provide bad url that cannot be parsed to Uri.

http://codereview.appspot.com/2253044/diff/1/java/gadgets/src/main/java/org/a...
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerApi.java:53:
@Unfiltered
On 2010/09/24 22:34:57, mhermanto wrote:
> Necessary annotation? How about on getUrl()?

The error should never be filtered out (prevent user error), while the url of
the request is not always needed since it is also the key of the return hash -
so it is redundent in that case.

http://codereview.appspot.com/2253044/diff/1/java/gadgets/src/main/java/org/a...
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerApi.java:62:
public Uri getUrl();
On 2010/09/24 22:34:57, mhermanto wrote:
> Need this to be declared? It's already declared in BaseResponse.

It is in base response but not base requests. I have second thought about this
and moved it back to base request as well. Since it can be null (ommited), it
should be ok if we will add requests later that does not need it.

http://codereview.appspot.com/2253044/diff/1/java/gadgets/src/main/java/org/a...
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerApi.java:169:
public Long getDurationSeconds();
On 2010/09/24 22:34:57, mhermanto wrote:
> Nit, getDurationInSeconds().

Commented out for now

http://codereview.appspot.com/2253044/diff/1/java/gadgets/src/main/java/org/a...
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerService.java
(right):

http://codereview.appspot.com/2253044/diff/1/java/gadgets/src/main/java/org/a...
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerService.java:140:
private boolean fieldsInclude(Set<String> fields, String name) {
On 2010/09/24 22:34:57, mhermanto wrote:
> Nit, prefer rename to question-form doFieldsInclude() or includeFields(), much
> like Java isEmpty(), contains(), etc.

Done.

[zhoresh, 2010-09-25 00:21:21]

Update patch according to Mike comments

[mhermanto, 2010-09-25 00:46:48]

Fix that and LGTM++.

http://codereview.appspot.com/2253044/diff/14001/features/src/main/javascript...
File features/src/main/javascript/features/container/service.js (right):

http://codereview.appspot.com/2253044/diff/14001/features/src/main/javascript...
features/src/main/javascript/features/container/service.js:106:
finalResponse[id] = { 'error' : error };
You'll want response.error.

[zhoresh, 2010-09-25 01:00:26]

On 2010/09/25 00:46:48, mhermanto wrote:
> Fix that and LGTM++.
> 
>
http://codereview.appspot.com/2253044/diff/14001/features/src/main/javascript...
> File features/src/main/javascript/features/container/service.js (right):
> 
>
http://codereview.appspot.com/2253044/diff/14001/features/src/main/javascript...
> features/src/main/javascript/features/container/service.js:106:
> finalResponse[id] = { 'error' : error };
> You'll want response.error.

Good catch, thanks!

Fixed and committed as 1001115