Annotations based Request Handlers

[Ben Smith <be...@gmail.com>]

Hey all (and particularly @lryan),

I spent a lot of yesturday migrating our code to comply with the new 
Shindig annotations based handler and handler registry. I really like 
this way of organising things, but I did have quite a few problems while 
itegrating which I hope you can help us with.

*1. Registering a new Handler*

Up until yesturday we've been including the SocialApiGuiceModule in our 
web.xml and test-bootstraps. This is generally a good idea as we don't 
have to manually keep the bindings up-to-date.

Now that SocialApiGuiceModule registers a list of handlers:
bind(List.class).annotatedWith(Names.named("org.apache.shindig.handlers"))
        .toInstance(Lists.immutableList(ActivityHandler.class, 
AppDataHandler.class,
            PersonHandler.class, SampleContainerHandler.class));

And we extend the PersonHandler with, say, SuperAwesomeBBCPersonHandler, 
we now have to copy and paste all of SocialApiGuiceModule, so we can 
change that one registration. Even if we bind PersonHandler.class to 
SuperAwesomeBBCPersonHandler.class, it's PersonHandler that gets 
registered as a handler (unless I did something wrong when I tried 
this.. which is likely).

There's probably some way of achieving this that I missed yesturday, but 
as it stands this will increase our maintenance / tracking overhead.

*2. Modularising the Actions*

The major advantage to this system can be seen in the /@supportedFields 
as you can modularise the actions and not have to stuff them all in to 
one long function.

The problem I am having is creating seperate functions with paths that 
only change at the end.

Say I've got an operation that creates a Person:
@Operation(httpMethods = "POST")
public Future<?> create(RequestItem request) throws SocialSpiException {

And an operation that creates a person's Igloos:
@Operation(httpMethods = "POST", path = "/{userId}+/@self/igloos")
public Future<?> createIgloo(RequestItem request) throws 
SocialSpiException {

The paths aren't different enough for the DefaultHandlerRegistry and the 
two functions conflict. Am I doing something wrong or does the Hash for 
Rest operations in the DefaultHandlerRegistry need to create cleverer 
keys for this to work?


Hope you can help me with this. Overall, nice work, it could be made 
just a bit easier for extending parties.

Cheers,
Ben Smith
BBC

Re: Annotations based Request Handlers

[Adam Winer <aw...@google.com>]

On Wed, Feb 4, 2009 at 9:06 AM, Louis Ryan <lr...@google.com> wrote:
> Ben
>
> Glad you like it overall. This piece is definitely something of a work in
> progress but the intent is to support the things you want to do Some notes
> inline.
>
> On Wed, Feb 4, 2009 at 1:53 AM, Ben Smith <be...@gmail.com> wrote:
>
>> Hey all (and particularly @lryan),
>>
>> I spent a lot of yesturday migrating our code to comply with the new
>> Shindig annotations based handler and handler registry. I really like this
>> way of organising things, but I did have quite a few problems while
>> itegrating which I hope you can help us with.
>>
>> *1. Registering a new Handler*
>>
>> Up until yesturday we've been including the SocialApiGuiceModule in our
>> web.xml and test-bootstraps. This is generally a good idea as we don't have
>> to manually keep the bindings up-to-date.
>>
>> Now that SocialApiGuiceModule registers a list of handlers:
>> bind(List.class).annotatedWith(Names.named("org.apache.shindig.handlers"))
>>       .toInstance(Lists.immutableList(ActivityHandler.class,
>> AppDataHandler.class,
>>           PersonHandler.class, SampleContainerHandler.class));
>>
>> And we extend the PersonHandler with, say, SuperAwesomeBBCPersonHandler, we
>> now have to copy and paste all of SocialApiGuiceModule, so we can change
>> that one registration. Even if we bind PersonHandler.class to
>> SuperAwesomeBBCPersonHandler.class, it's PersonHandler that gets registered
>> as a handler (unless I did something wrong when I tried this.. which is
>> likely).
>>
>> There's probably some way of achieving this that I missed yesturday, but as
>> it stands this will increase our maintenance / tracking overhead.
>
>
> Yup, definitely an issue and something Ill fix. If we were to move to Guice
> 2 this problem would disappear as you could simply override the binding.
> Unfortunately its not released yet so Ill need to resort to other means to
> make this easier

Could you change this from a List to a Set?  On that ever-distant day
when Guice 2.0 is released, we could switch this to a Multibinding,
which would let you contribute new handlers to the base instead of
just overriding.

>
>>
>>
>> *2. Modularising the Actions*
>>
>> The major advantage to this system can be seen in the /@supportedFields as
>> you can modularise the actions and not have to stuff them all in to one long
>> function.
>>
>> The problem I am having is creating seperate functions with paths that only
>> change at the end.
>>
>> Say I've got an operation that creates a Person:
>> @Operation(httpMethods = "POST")
>> public Future<?> create(RequestItem request) throws SocialSpiException {
>>
>> And an operation that creates a person's Igloos:
>> @Operation(httpMethods = "POST", path = "/{userId}+/@self/igloos")
>> public Future<?> createIgloo(RequestItem request) throws SocialSpiException
>> {
>>
>> The paths aren't different enough for the DefaultHandlerRegistry and the
>> two functions conflict. Am I doing something wrong or does the Hash for Rest
>> operations in the DefaultHandlerRegistry need to create cleverer keys for
>> this to work?
>
>
> Yes, this is a known issue with the patch matching. It basically searches
> using a longest-prefix match and only matches constant parts of the path.
> Let me take a look at doing something a bit more refined.
>
>
>>
>>
>> Hope you can help me with this. Overall, nice work, it could be made just a
>> bit easier for extending parties.
>>
>> Cheers,
>> Ben Smith
>> BBC
>>
>

Re: Annotations based Request Handlers

[Ben Smith <be...@gmail.com>]

On 4/2/09 17:06, Louis Ryan wrote:
> Ben
>
> Glad you like it overall. This piece is definitely something of a work in
> progress but the intent is to support the things you want to do Some notes
> inline.
>    
Cheers for this Louis. Let me know if you want a hand when you get round 
to your next iteration.

Re: Annotations based Request Handlers

[Louis Ryan <lr...@google.com>]

Ben

Glad you like it overall. This piece is definitely something of a work in
progress but the intent is to support the things you want to do Some notes
inline.

On Wed, Feb 4, 2009 at 1:53 AM, Ben Smith <be...@gmail.com> wrote:

> Hey all (and particularly @lryan),
>
> I spent a lot of yesturday migrating our code to comply with the new
> Shindig annotations based handler and handler registry. I really like this
> way of organising things, but I did have quite a few problems while
> itegrating which I hope you can help us with.
>
> *1. Registering a new Handler*
>
> Up until yesturday we've been including the SocialApiGuiceModule in our
> web.xml and test-bootstraps. This is generally a good idea as we don't have
> to manually keep the bindings up-to-date.
>
> Now that SocialApiGuiceModule registers a list of handlers:
> bind(List.class).annotatedWith(Names.named("org.apache.shindig.handlers"))
>       .toInstance(Lists.immutableList(ActivityHandler.class,
> AppDataHandler.class,
>           PersonHandler.class, SampleContainerHandler.class));
>
> And we extend the PersonHandler with, say, SuperAwesomeBBCPersonHandler, we
> now have to copy and paste all of SocialApiGuiceModule, so we can change
> that one registration. Even if we bind PersonHandler.class to
> SuperAwesomeBBCPersonHandler.class, it's PersonHandler that gets registered
> as a handler (unless I did something wrong when I tried this.. which is
> likely).
>
> There's probably some way of achieving this that I missed yesturday, but as
> it stands this will increase our maintenance / tracking overhead.


Yup, definitely an issue and something Ill fix. If we were to move to Guice
2 this problem would disappear as you could simply override the binding.
Unfortunately its not released yet so Ill need to resort to other means to
make this easier

>
>
> *2. Modularising the Actions*
>
> The major advantage to this system can be seen in the /@supportedFields as
> you can modularise the actions and not have to stuff them all in to one long
> function.
>
> The problem I am having is creating seperate functions with paths that only
> change at the end.
>
> Say I've got an operation that creates a Person:
> @Operation(httpMethods = "POST")
> public Future<?> create(RequestItem request) throws SocialSpiException {
>
> And an operation that creates a person's Igloos:
> @Operation(httpMethods = "POST", path = "/{userId}+/@self/igloos")
> public Future<?> createIgloo(RequestItem request) throws SocialSpiException
> {
>
> The paths aren't different enough for the DefaultHandlerRegistry and the
> two functions conflict. Am I doing something wrong or does the Hash for Rest
> operations in the DefaultHandlerRegistry need to create cleverer keys for
> this to work?


Yes, this is a known issue with the patch matching. It basically searches
using a longest-prefix match and only matches constant parts of the path.
Let me take a look at doing something a bit more refined.


>
>
> Hope you can help me with this. Overall, nice work, it could be made just a
> bit easier for extending parties.
>
> Cheers,
> Ben Smith
> BBC
>