Using custom @ ids GroupIds throws IllegalArgumentException

If you try to create a GroupId with a non standard @ id an IllegalArgumentException will be thrown. This is because we default the type to ObjectId and @ is not a valid character in an ObjectId.

[rbaxter85, 2012-06-10 22:15:05]

Updated patch

[DouglasLDavies, 2012-06-11 04:23:39]

http://codereview.appspot.com/6305079/diff/1002/java/social-api/src/main/java...
File
java/social-api/src/main/java/org/apache/shindig/social/opensocial/spi/GroupId.java
(right):

http://codereview.appspot.com/6305079/diff/1002/java/social-api/src/main/java...
java/social-api/src/main/java/org/apache/shindig/social/opensocial/spi/GroupId.java:115:
String type = parseType(objectId);
I wonder if parseType could be smart enough to recognize a custom groupId that
starts with @ but isn't one of the spec defined ones?

Then you could just do

else if (type.equals("custom"))
    return Type.custom;

http://codereview.appspot.com/6305079/diff/1002/java/social-api/src/main/java...
java/social-api/src/main/java/org/apache/shindig/social/opensocial/spi/GroupId.java:122:
} else if(objectId instanceof String && ((String)objectId).startsWith("@")) {
Would a Type.unknown or Type.custom be better than a null?

[rbaxter85, 2012-06-11 13:31:05]

Updated based on Doug's suggestions.

[DouglasLDavies, 2012-06-11 13:48:55]

http://codereview.appspot.com/6305079/diff/6001/java/social-api/src/main/java...
File
java/social-api/src/main/java/org/apache/shindig/social/opensocial/spi/GroupId.java
(right):

http://codereview.appspot.com/6305079/diff/6001/java/social-api/src/main/java...
java/social-api/src/main/java/org/apache/shindig/social/opensocial/spi/GroupId.java:128:
// return null we don't know the type
change comment about null

http://codereview.appspot.com/6305079/diff/6001/java/social-api/src/test/java...
File
java/social-api/src/test/java/org/apache/shindig/social/opensocial/spi/GroupIdTest.java
(right):

http://codereview.appspot.com/6305079/diff/6001/java/social-api/src/test/java...
java/social-api/src/test/java/org/apache/shindig/social/opensocial/spi/GroupIdTest.java:43:
GroupId unknown = GroupId.fromJson("@foo");
Call the variable custom

[TheMistyMay, 2012-06-11 15:35:11]

Other than the null comment...LGTM

Applied patch and ran tests, all successful.

Ship it!

http://codereview.appspot.com/6305079/diff/6001/java/social-api/src/main/java...
File
java/social-api/src/main/java/org/apache/shindig/social/opensocial/spi/GroupId.java
(right):

http://codereview.appspot.com/6305079/diff/6001/java/social-api/src/main/java...
java/social-api/src/main/java/org/apache/shindig/social/opensocial/spi/GroupId.java:128:
// return null we don't know the type
+1

On 2012/06/11 13:48:55, DouglasLDavies wrote:
> change comment about null

http://codereview.appspot.com/6305079/diff/6001/java/social-api/src/test/java...
File
java/social-api/src/test/java/org/apache/shindig/social/opensocial/spi/GroupIdTest.java
(right):

http://codereview.appspot.com/6305079/diff/6001/java/social-api/src/test/java...
java/social-api/src/test/java/org/apache/shindig/social/opensocial/spi/GroupIdTest.java:43:
GroupId unknown = GroupId.fromJson("@foo");
I would disagree, seems misleading to call it @custom.  @foo demonstrates it can
be anything and it not just another hardcoded type.

Works either way but I think @foo is a better representation of the feature.

On 2012/06/11 13:48:55, DouglasLDavies wrote:
> Call the variable custom

[Stanton, 2012-06-11 19:53:57]

LGTM.

[rbaxter85, 2012-06-12 00:41:21]

Updated comments

[rbaxter85, 2012-06-12 00:41:36]

http://codereview.appspot.com/6305079/diff/6001/java/social-api/src/test/java...
File
java/social-api/src/test/java/org/apache/shindig/social/opensocial/spi/GroupIdTest.java
(right):

http://codereview.appspot.com/6305079/diff/6001/java/social-api/src/test/java...
java/social-api/src/test/java/org/apache/shindig/social/opensocial/spi/GroupIdTest.java:43:
GroupId unknown = GroupId.fromJson("@foo");
+1 Mike.

Doug is there any reason why you would want the test to use @custom?

On 2012/06/11 15:35:11, TheMistyMay wrote:
> I would disagree, seems misleading to call it @custom.  @foo demonstrates it
can
> be anything and it not just another hardcoded type.
> 
> Works either way but I think @foo is a better representation of the feature.
> 
> On 2012/06/11 13:48:55, DouglasLDavies wrote:
> > Call the variable custom
>

[DouglasLDavies, 2012-06-12 02:37:55]

http://codereview.appspot.com/6305079/diff/6001/java/social-api/src/test/java...
File
java/social-api/src/test/java/org/apache/shindig/social/opensocial/spi/GroupIdTest.java
(right):

http://codereview.appspot.com/6305079/diff/6001/java/social-api/src/test/java...
java/social-api/src/test/java/org/apache/shindig/social/opensocial/spi/GroupIdTest.java:43:
GroupId unknown = GroupId.fromJson("@foo");
Sorry. I meant the variable name. Call it custom instead of unknown.

[TheMistyMay, 2012-06-12 12:22:49]

http://codereview.appspot.com/6305079/diff/6001/java/social-api/src/test/java...
File
java/social-api/src/test/java/org/apache/shindig/social/opensocial/spi/GroupIdTest.java
(right):

http://codereview.appspot.com/6305079/diff/6001/java/social-api/src/test/java...
java/social-api/src/test/java/org/apache/shindig/social/opensocial/spi/GroupIdTest.java:43:
GroupId unknown = GroupId.fromJson("@foo");
Ahhh! I'd be fine with that.

+1

On 2012/06/12 02:37:55, DouglasLDavies wrote:

[rbaxter85, 2012-06-12 13:34:52]

Changed variable name

[rbaxter85, 2012-06-13 00:30:55]

On 2012/06/12 13:34:52, rbaxter85 wrote:
> Changed variable name

Committed