OpenSocial Message API changes

This implements the messaging API for 0.9

[rodrigo.gallardo, 2009-03-02 23:59:30]

Message templates are of reduced utility without template expansion as in
Activities, but we seem to have overlooked that during the spec proposal phase.

Should we request a spec clarification adding a templateParameters field, or
would that be better left as a proposal for the next spec version?

http://codereview.appspot.com/20106/diff/1/30
File features/src/main/javascript/features/opensocial-base/jsonmessage.js
(right):

http://codereview.appspot.com/20106/diff/1/30#newcode44
Line 44: //}
If this is not being used, why not remove it?

http://codereview.appspot.com/20106/diff/1/20
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ProxyBase.java
(right):

http://codereview.appspot.com/20106/diff/1/20#newcode73
Line 73: "url parameter is not a valid url: " + urlToValidate);
This probably belongs in a separate patch.

http://codereview.appspot.com/20106/diff/1/15
File
java/social-api/src/main/java/org/apache/shindig/social/opensocial/model/MediaItem.java
(right):

http://codereview.appspot.com/20106/diff/1/15#newcode145
Line 145: void setThumbnailUrl(String url);
Missing javadoc.

http://codereview.appspot.com/20106/diff/1/11
File
java/social-api/src/main/java/org/apache/shindig/social/opensocial/model/Person.java
(right):

http://codereview.appspot.com/20106/diff/1/11#newcode20
Line 20: import org.apache.shindig.social.opensocial.model.EnumUtil;
unused import?

http://codereview.appspot.com/20106/diff/1/11#newcode35
Line 35: import java.util.EnumSet;
unused imports?

http://codereview.appspot.com/20106/diff/1/11#newcode251
Line 251: return lookup.get(jsonString);
Does this belong in this patch?

http://codereview.appspot.com/20106/diff/1/18
File
java/social-api/src/main/java/org/apache/shindig/social/opensocial/spi/CollectionOptions.java
(right):

http://codereview.appspot.com/20106/diff/1/18#newcode50
Line 50: 
Empty diff.

http://codereview.appspot.com/20106/diff/1/17
File
java/social-api/src/main/java/org/apache/shindig/social/opensocial/spi/MessageService.java
(right):

http://codereview.appspot.com/20106/diff/1/17#newcode53
Line 53: Future<MessageCollection> createMessageCollection(UserId userId,
MessageCollection msgCollection, SecurityToken token);
Missing javadocs.

http://codereview.appspot.com/20106/diff/1/17#newcode55
Line 55: Future<Void> deleteMessageCollection(UserId userId, String msgCollId,
SecurityToken token);
Add throws SocialSpiException to all these.

http://codereview.appspot.com/20106/diff/1/17#newcode102
Line 102: Future<Void> modifyMessage(UserId userId, String msgCollId, String
messageId, Message message, SecurityToken token);
Missing javadoc.
Add throws SocialSpiException to method signature.

http://codereview.appspot.com/20106/diff/1/4
File
java/social-api/src/main/java/org/apache/shindig/social/sample/spi/JsonDbOpensocialService.java
(right):

http://codereview.appspot.com/20106/diff/1/4#newcode490
Line 490: return null;
All these unimplemented methods should throw a NOT_IMPLEMENTED
SocialSpiException, instead of silently doing nothing.

http://codereview.appspot.com/20106/diff/1/4#newcode552
Line 552: 
empty lines

[Paul Lindner, 2009-03-03 01:52:27]

updated

[Paul Lindner, 2009-03-03 01:52:53]

patch updated...

http://codereview.appspot.com/20106/diff/1/30
File features/src/main/javascript/features/opensocial-base/jsonmessage.js
(right):

http://codereview.appspot.com/20106/diff/1/30#newcode44
Line 44: //}
On 2009/03/02 23:59:30, rodrigo.gallardo wrote:
> If this is not being used, why not remove it?

Done.

http://codereview.appspot.com/20106/diff/1/20
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ProxyBase.java
(right):

http://codereview.appspot.com/20106/diff/1/20#newcode73
Line 73: "url parameter is not a valid url: " + urlToValidate);
On 2009/03/02 23:59:30, rodrigo.gallardo wrote:
> This probably belongs in a separate patch.

Done.

http://codereview.appspot.com/20106/diff/1/15
File
java/social-api/src/main/java/org/apache/shindig/social/opensocial/model/MediaItem.java
(right):

http://codereview.appspot.com/20106/diff/1/15#newcode145
Line 145: void setThumbnailUrl(String url);
On 2009/03/02 23:59:30, rodrigo.gallardo wrote:
> Missing javadoc.

Done.

http://codereview.appspot.com/20106/diff/1/11
File
java/social-api/src/main/java/org/apache/shindig/social/opensocial/model/Person.java
(right):

http://codereview.appspot.com/20106/diff/1/11#newcode20
Line 20: import org.apache.shindig.social.opensocial.model.EnumUtil;
On 2009/03/02 23:59:30, rodrigo.gallardo wrote:
> unused import?

Done.

http://codereview.appspot.com/20106/diff/1/11#newcode35
Line 35: import java.util.EnumSet;
On 2009/03/02 23:59:30, rodrigo.gallardo wrote:
> unused imports?

Done.

http://codereview.appspot.com/20106/diff/1/18
File
java/social-api/src/main/java/org/apache/shindig/social/opensocial/spi/CollectionOptions.java
(right):

http://codereview.appspot.com/20106/diff/1/18#newcode50
Line 50: 
On 2009/03/02 23:59:30, rodrigo.gallardo wrote:
> Empty diff.

Done.

http://codereview.appspot.com/20106/diff/1/17
File
java/social-api/src/main/java/org/apache/shindig/social/opensocial/spi/MessageService.java
(right):

http://codereview.appspot.com/20106/diff/1/17#newcode53
Line 53: Future<MessageCollection> createMessageCollection(UserId userId,
MessageCollection msgCollection, SecurityToken token);
On 2009/03/02 23:59:30, rodrigo.gallardo wrote:
> Missing javadocs.

Done.

http://codereview.appspot.com/20106/diff/1/17#newcode55
Line 55: Future<Void> deleteMessageCollection(UserId userId, String msgCollId,
SecurityToken token);
On 2009/03/02 23:59:30, rodrigo.gallardo wrote:
> Add throws SocialSpiException to all these.

Done.

http://codereview.appspot.com/20106/diff/1/17#newcode102
Line 102: Future<Void> modifyMessage(UserId userId, String msgCollId, String
messageId, Message message, SecurityToken token);
On 2009/03/02 23:59:30, rodrigo.gallardo wrote:
> Missing javadoc.
> Add throws SocialSpiException to method signature.

Done.

http://codereview.appspot.com/20106/diff/1/4
File
java/social-api/src/main/java/org/apache/shindig/social/sample/spi/JsonDbOpensocialService.java
(right):

http://codereview.appspot.com/20106/diff/1/4#newcode552
Line 552: 
On 2009/03/02 23:59:30, rodrigo.gallardo wrote:
> empty lines

Done.

[Paul Lindner, 2009-03-03 13:15:16]

On 2009/03/02 23:59:30, rodrigo.gallardo wrote:
> Message templates are of reduced utility without template expansion as in
> Activities, but we seem to have overlooked that during the spec proposal
phase.
> 
> Should we request a spec clarification adding a templateParameters field, or
> would that be better left as a proposal for the next spec version?

supporting standardized templating for message submission does seem like a good
idea.  We can raise this point with the spec list and see what the response is.

[louiscryan, 2009-03-03 19:24:30]

Looks mostly fine. A couple of nits and one comment on operation naming.

http://codereview.appspot.com/20106/diff/51/1026
File
java/social-api/src/main/java/org/apache/shindig/social/opensocial/model/Address.java
(right):

http://codereview.appspot.com/20106/diff/51/1026#newcode65
Line 65: private static final Map<String, Field> lookup = new HashMap<String,
Field>();
technically should be concurrent map.

http://codereview.appspot.com/20106/diff/51/1029
File
java/social-api/src/main/java/org/apache/shindig/social/opensocial/service/MessageHandler.java
(right):

http://codereview.appspot.com/20106/diff/51/1029#newcode88
Line 88: @Operation(httpMethods = "POST", bodyParam = "data")
is there a better name than "data" for the body?

http://codereview.appspot.com/20106/diff/51/1029#newcode89
Line 89: public Future<?> post(SocialRequestItem request) throws
SocialSpiException {
if the operations are more CRUD like I've been using create/update for the
method names instead of get/post which means they will be exposed to RPC as
messages.create and messages.update and by implication to oslite as such which
is more inline with the JS APIs. See ActivityHandler etc.

[Paul Lindner, 2009-03-03 21:42:10]

updates applied, going to commit to trunk.

http://codereview.appspot.com/20106/diff/51/1026
File
java/social-api/src/main/java/org/apache/shindig/social/opensocial/model/Address.java
(right):

http://codereview.appspot.com/20106/diff/51/1026#newcode65
Line 65: private static final Map<String, Field> lookup = new HashMap<String,
Field>();
On 2009/03/03 19:24:31, louiscryan wrote:
> technically should be concurrent map.

actually it's only modified at class instantiation, so should be fine as-is

http://codereview.appspot.com/20106/diff/51/1029
File
java/social-api/src/main/java/org/apache/shindig/social/opensocial/service/MessageHandler.java
(right):

http://codereview.appspot.com/20106/diff/51/1029#newcode88
Line 88: @Operation(httpMethods = "POST", bodyParam = "data")
On 2009/03/03 19:24:31, louiscryan wrote:
> is there a better name than "data" for the body?

entity?  hard to say since this data can be a message collection or a message...

http://codereview.appspot.com/20106/diff/51/1029#newcode89
Line 89: public Future<?> post(SocialRequestItem request) throws
SocialSpiException {
On 2009/03/03 19:24:31, louiscryan wrote:
> if the operations are more CRUD like I've been using create/update for the
> method names instead of get/post which means they will be exposed to RPC as
> messages.create and messages.update and by implication to oslite as such which
> is more inline with the JS APIs. See ActivityHandler etc.

agreed. done.