Server module refactoring

[James M Snell <ja...@gmail.com>]

Ok, given the discussions around Adapter/CollectionProvider and the 
complex way the existing server code has evolved, I decided to take a 
step back and look at the server module as a whole.  I believe I've come 
up with a refactoring that will greatly simplify things overall and make 
it easier to support not only the capabilities that our friends from 
google want, but also things like extension methods (e.g. patch), 
protocol extensions (e.g. opensearch), etc.

Note: the refactoring would actually take place above the functionality 
that the google feed-server provides, so there could potentially be very 
little impact on that work, but we'll address that shortly.

The refactoring that I have in mind changes everything from 
AbderaServlet up through CollectionProvider and is focused on reducing 
the number of moving parts and number of things a developer has to do 
when implementing an atompub server.

In this note, I want to provide a quick run through of the changes I 
have in mind.  I'll post some of the preliminary implementation ideas to 
a jira issue.

Structure:

Package: org.apache.abdera.protocol.server.refactor

   CollectionAdapter.java
   MediaCollectionAdapter.java
   Provider.java
   ProviderHelper.java
   Transactional.java
   WorkspaceManager.java

Package: org.apache.abdera.protocol.server.refactor.impl

   AbstractProvider.java
   AbstractWorkspaceManager.java
   AbstractWorkspaceManagerProvider.java


The code flow in the current implementation is:

   AbderaServlet -> RequestHandler -> Provider -> CollectionProvider

After refactoring, RequestHandler will be removed completely.  The 
Provider interface is simplified to a single request method.

There will be a single base abstract Provider implementation provided 
but advanced developers will be able to replace that implementation with 
their own.

The default provider implementation will extend 
AbstractWorkspaceManagerProvider.java.  This class implements boththe 
Provider and WorkspaceManager interfaces.

The WorkspaceManager is equivalent to the current WorkspaceInfo 
interface.  It's primary function is to lookup the appropriate 
CollectionAdapter instance.

CollectionAdapter is the equivalent to the current CollectionProvider 
interface.

MediaCollectionAdapter extends CollectionAdapter to provide methods for 
handling media resources.

A CollectionAdapter instance can choose to implement the Provider 
interface in order to support extension methods.

A CollectionAdapter instance can choose to implement the Transactional 
interface if it wishes to support begin/end/compensate semantics.

All of the methods on CollectionAdapter take RequestContext and return 
ResponseContext, maintaining the ability to parameterize requests and 
utilize streaming responses.

The flow is simple: request comes into the servlet, which dispatches to 
the Provider.  The Provider gets the WorkspaceManager and uses it to get 
the CollectionAdapter.  If the request is for a known method and a known 
target type, the provider dispatches to the appropriate method on 
CollectionAdapter (e.g. getFeed, deleteEntry, etc).  If the request is 
for an unknown method and an unknown target type, the provider checks to 
see if the CollectionAdapter implements the Provider interface, and if 
so, forwards the request on to it for processing, otherwise, an 
unsupported method response is returned.  If CollectionAdapter 
implements Transactional, all of this is preceeded by a 
Transactional.begin() and proceeded by Transactional.end().

A CollectionAdapter can be implemented that wraps the simplified Adapter 
interface used by feed-server.  Initially, this interface would not 
implement either Transactional or MediaCollectionAdapter keeping it in 
line with the current functionality offered by feed-server.

Additional refactorings would be required to implement this but overall 
the code structure would be greatly simplified, more logical, easier to 
extend, easier to maintain, and more accessible for end users.

Anyway, please take a look and let me know what you think.

- James

Re: Server module refactoring

[James M Snell <ja...@gmail.com>]

Dan Diephouse wrote:
> I just took a look at the refactoring. Overall I like it.
> 

Good to hear.

> Some comments:
> 
> Most of my comments have to do with the ext package. I like the 
> interfaces for the most part in the top level package. But I think to 
> some degree the separation which has occurred inside the ext/basic 
> packages is unecessary.
> 

Agreed.

> AbstractProvider already includes the concepts of CollectionAdapter & 
> WorkspaceManager. So why have so many derivative implementations? There 
> is AbstractProivder, AbstractWorkspaceManagerProvider, BasicProvider, 
> ExtendedProvider. They are all slight variations on the same thing and 
> should all be combined into one thing IMO. I was kind of on the fence 
> before, but after seeing the class proliferation, think its a bad idea 
> to have a provider implement CollectionAdapter & WorkspaceManager. It 
> creates an overly complex code base and will be confusing for users  - 
> i.e. which Provider do I use? Lets make the typical answer to that 
> question just one answer unless they have to override some advanced 
> functionality. Then the process of writing a server becomes conceptually 
> simpler:
> - Write a CollectionAdapter
> - If necessary write a workspace manager
> - Glue it together with a ServiceContext and a provider
> See how this eliminates a step and confusion from the process?
> 
> Along these lines:
> - ExtendedWorkspaceManager -> WorkspaceManagerImpl, SimpleWorkspaceInfo 
> -> WorkspaceImpl (I personally like Impl better, but whatever)
> - Collapse NamedCollectionAdapter into CollectionAdapter
> - Delete AbstractWorkspaceManagerProvider, 
> AbstractNamedCollectionAdapter, redundant Basic* stuff, etc
> 
> And see how many classes can be removed? :-)
> 

Again agreed. I would have combined much of this stuff already but I 
wanted to keep the various approaches separate in this first revision 
and then start iterating to simplify things.  Mainly, I'm still not 
convinced that there is a need for the BasicAdapter interface.  I 
understand what the feed-server stuff is doing but I think the 
additional interfaces causes more difficulty than it is worth.

> Other comments:
> - Can we please move to TestNG or JUnit4? The test code to deal with 
> servers is fairly hackish and this would alleviate that.
> - I find the code style a little unreadable. I'm hesitant to bring it up 
> though as its a neverending battle. If I had two wishes though they 
> would be that we don't break constrcutor/method lines unless necessary 
> and we move to 4 spaces :-)
> 
> I have more thoughts, but I need to hop on a plane. Nice work James and 
> looking forward to digging in more soon.
> - Dan
> 
> -- 
> Dan Diephouse
> MuleSource
> http://mulesource.com | http://netzooid.com/blog
> 
> 

Re: Server module refactoring

[Dan Diephouse <da...@mulesource.com>]

I'll take a look later tonight or early in the morning and let you know 
my thoughts. No need to rush to write something up for me as the code is 
probably OK for now.

- Dan

James M Snell wrote:
> Ok, many changes.  Later this evening (after the kids are in bed) I 
> will write up a note summarizing the changes and will take a first 
> stab at a brief server implementation guide based on the revised code.
>
> Please take a look and let me know your thoughts.
>
> - James
>
> Dan Diephouse wrote:
>> I just took a look at the refactoring. Overall I like it.
>>
>> Some comments:
>>
>> Most of my comments have to do with the ext package. I like the 
>> interfaces for the most part in the top level package. But I think to 
>> some degree the separation which has occurred inside the ext/basic 
>> packages is unecessary.
>>
>> AbstractProvider already includes the concepts of CollectionAdapter & 
>> WorkspaceManager. So why have so many derivative implementations? 
>> There is AbstractProivder, AbstractWorkspaceManagerProvider, 
>> BasicProvider, ExtendedProvider. They are all slight variations on 
>> the same thing and should all be combined into one thing IMO. I was 
>> kind of on the fence before, but after seeing the class 
>> proliferation, think its a bad idea to have a provider implement 
>> CollectionAdapter & WorkspaceManager. It creates an overly complex 
>> code base and will be confusing for users  - i.e. which Provider do I 
>> use? Lets make the typical answer to that question just one answer 
>> unless they have to override some advanced functionality. Then the 
>> process of writing a server becomes conceptually simpler:
>> - Write a CollectionAdapter
>> - If necessary write a workspace manager
>> - Glue it together with a ServiceContext and a provider
>> See how this eliminates a step and confusion from the process?
>>
>> Along these lines:
>> - ExtendedWorkspaceManager -> WorkspaceManagerImpl, 
>> SimpleWorkspaceInfo -> WorkspaceImpl (I personally like Impl better, 
>> but whatever)
>> - Collapse NamedCollectionAdapter into CollectionAdapter
>> - Delete AbstractWorkspaceManagerProvider, 
>> AbstractNamedCollectionAdapter, redundant Basic* stuff, etc
>>
>> And see how many classes can be removed? :-)
>>
>> Other comments:
>> - Can we please move to TestNG or JUnit4? The test code to deal with 
>> servers is fairly hackish and this would alleviate that.
>> - I find the code style a little unreadable. I'm hesitant to bring it 
>> up though as its a neverending battle. If I had two wishes though 
>> they would be that we don't break constrcutor/method lines unless 
>> necessary and we move to 4 spaces :-)
>>
>> I have more thoughts, but I need to hop on a plane. Nice work James 
>> and looking forward to digging in more soon.
>> - Dan
>>
>> -- 
>> Dan Diephouse
>> MuleSource
>> http://mulesource.com | http://netzooid.com/blog
>>
>>


-- 
Dan Diephouse
MuleSource
http://mulesource.com | http://netzooid.com/blog

Re: Server module refactoring

[James M Snell <ja...@gmail.com>]

Ok, many changes.  Later this evening (after the kids are in bed) I will 
write up a note summarizing the changes and will take a first stab at a 
brief server implementation guide based on the revised code.

Please take a look and let me know your thoughts.

- James

Dan Diephouse wrote:
> I just took a look at the refactoring. Overall I like it.
> 
> Some comments:
> 
> Most of my comments have to do with the ext package. I like the 
> interfaces for the most part in the top level package. But I think to 
> some degree the separation which has occurred inside the ext/basic 
> packages is unecessary.
> 
> AbstractProvider already includes the concepts of CollectionAdapter & 
> WorkspaceManager. So why have so many derivative implementations? There 
> is AbstractProivder, AbstractWorkspaceManagerProvider, BasicProvider, 
> ExtendedProvider. They are all slight variations on the same thing and 
> should all be combined into one thing IMO. I was kind of on the fence 
> before, but after seeing the class proliferation, think its a bad idea 
> to have a provider implement CollectionAdapter & WorkspaceManager. It 
> creates an overly complex code base and will be confusing for users  - 
> i.e. which Provider do I use? Lets make the typical answer to that 
> question just one answer unless they have to override some advanced 
> functionality. Then the process of writing a server becomes conceptually 
> simpler:
> - Write a CollectionAdapter
> - If necessary write a workspace manager
> - Glue it together with a ServiceContext and a provider
> See how this eliminates a step and confusion from the process?
> 
> Along these lines:
> - ExtendedWorkspaceManager -> WorkspaceManagerImpl, SimpleWorkspaceInfo 
> -> WorkspaceImpl (I personally like Impl better, but whatever)
> - Collapse NamedCollectionAdapter into CollectionAdapter
> - Delete AbstractWorkspaceManagerProvider, 
> AbstractNamedCollectionAdapter, redundant Basic* stuff, etc
> 
> And see how many classes can be removed? :-)
> 
> Other comments:
> - Can we please move to TestNG or JUnit4? The test code to deal with 
> servers is fairly hackish and this would alleviate that.
> - I find the code style a little unreadable. I'm hesitant to bring it up 
> though as its a neverending battle. If I had two wishes though they 
> would be that we don't break constrcutor/method lines unless necessary 
> and we move to 4 spaces :-)
> 
> I have more thoughts, but I need to hop on a plane. Nice work James and 
> looking forward to digging in more soon.
> - Dan
> 
> -- 
> Dan Diephouse
> MuleSource
> http://mulesource.com | http://netzooid.com/blog
> 
> 

Re: Server module refactoring

[Dan Diephouse <da...@mulesource.com>]

I just took a look at the refactoring. Overall I like it.

Some comments:

Most of my comments have to do with the ext package. I like the 
interfaces for the most part in the top level package. But I think to 
some degree the separation which has occurred inside the ext/basic 
packages is unecessary.

AbstractProvider already includes the concepts of CollectionAdapter & 
WorkspaceManager. So why have so many derivative implementations? There 
is AbstractProivder, AbstractWorkspaceManagerProvider, BasicProvider, 
ExtendedProvider. They are all slight variations on the same thing and 
should all be combined into one thing IMO. I was kind of on the fence 
before, but after seeing the class proliferation, think its a bad idea 
to have a provider implement CollectionAdapter & WorkspaceManager. It 
creates an overly complex code base and will be confusing for users  - 
i.e. which Provider do I use? Lets make the typical answer to that 
question just one answer unless they have to override some advanced 
functionality. Then the process of writing a server becomes conceptually 
simpler:
- Write a CollectionAdapter
- If necessary write a workspace manager
- Glue it together with a ServiceContext and a provider
See how this eliminates a step and confusion from the process?

Along these lines:
- ExtendedWorkspaceManager -> WorkspaceManagerImpl, SimpleWorkspaceInfo 
-> WorkspaceImpl (I personally like Impl better, but whatever)
- Collapse NamedCollectionAdapter into CollectionAdapter
- Delete AbstractWorkspaceManagerProvider, 
AbstractNamedCollectionAdapter, redundant Basic* stuff, etc

And see how many classes can be removed? :-)

Other comments:
- Can we please move to TestNG or JUnit4? The test code to deal with 
servers is fairly hackish and this would alleviate that.
- I find the code style a little unreadable. I'm hesitant to bring it up 
though as its a neverending battle. If I had two wishes though they 
would be that we don't break constrcutor/method lines unless necessary 
and we move to 4 spaces :-)

I have more thoughts, but I need to hop on a plane. Nice work James and 
looking forward to digging in more soon.
- Dan

--
Dan Diephouse
MuleSource
http://mulesource.com | http://netzooid.com/blog

Re: Server module refactoring

[James M Snell <ja...@gmail.com>]

Dan Diephouse wrote:
> I've commented on a couple things which I have other thoughts on, but 
> overall it looks good.
> 
> James M Snell wrote:
>>
>> The default provider implementation will extend 
>> AbstractWorkspaceManagerProvider.java.  This class implements boththe 
>> Provider and WorkspaceManager interfaces.
> I would also like to float the idea that the Provider should take a 
> WorkspaceManager as a property/constructor. This way I as a user just 
> implement WorkspaceManager which can look up collections from my 
> database and pass it in as an argument to some stock Provider.
>>
>> The WorkspaceManager is equivalent to the current WorkspaceInfo 
>> interface.  It's primary function is to lookup the appropriate 
>> CollectionAdapter instance.
> We still need a way to describe the workspaces though. A la 
> WorkspaceInfo.getName/getCollectionProviders. Are you sure we don't need 
> WorkspaceInfo still?
> 

WorkspaceInfo is still going to be there, it will be used for 
Provider/Adapter implentations that derived from what is currently your 
CollectionProvider code, which I am currently in the process of 
porting/adapting over the revised model.  Once I finish porting and you 
get a chance to review, I think it will make sense.

>>
>> A CollectionAdapter instance can choose to implement the Provider 
>> interface in order to support extension methods.
> I'm not sure that provider is the right interface to do this as it mixes 
> semantics a little bit. My alternative proposals would be to either:
> 1. have a fallback method built into CA like doOther(RequestContext) or 
> something like that.
> 2. Make the Provider responsible for choosing a method via some sort of 
> extensible dispatching mechanism

Yes, I'm not convinced Provider is right either.  For now, it suited the 
immediate need but I was planning to revisit it.

>>
>> A CollectionAdapter instance can choose to implement the Transactional 
>> interface if it wishes to support begin/end/compensate semantics.
> I think this might be going a little bit overboard here in terms of 
> interfaces. I would rather lean toward building this into the 
> AbstractCollectionAdapter itself. A la the code snippet I published 
> earlier which called begin/end inside.
> 
> BTW, I also think we'll need to have begin/end points which we can 
> intercept in the actual Provider implementation itself.
> 

Noted. Once I finish the initial port, I'll check the code into a branch 
so we can collectively review it and make whatever additional tweaks we 
need.

> Overall, I like the simplified flow and increased coherency!
> 

Excellent.

- James

> - Dan
> 

Re: Server module refactoring

[Dan Diephouse <da...@mulesource.com>]

I've commented on a couple things which I have other thoughts on, but 
overall it looks good.

James M Snell wrote:
>
> The default provider implementation will extend 
> AbstractWorkspaceManagerProvider.java.  This class implements boththe 
> Provider and WorkspaceManager interfaces.
I would also like to float the idea that the Provider should take a 
WorkspaceManager as a property/constructor. This way I as a user just 
implement WorkspaceManager which can look up collections from my 
database and pass it in as an argument to some stock Provider.
>
> The WorkspaceManager is equivalent to the current WorkspaceInfo 
> interface.  It's primary function is to lookup the appropriate 
> CollectionAdapter instance.
We still need a way to describe the workspaces though. A la 
WorkspaceInfo.getName/getCollectionProviders. Are you sure we don't need 
WorkspaceInfo still?

>
> A CollectionAdapter instance can choose to implement the Provider 
> interface in order to support extension methods.
I'm not sure that provider is the right interface to do this as it mixes 
semantics a little bit. My alternative proposals would be to either:
1. have a fallback method built into CA like doOther(RequestContext) or 
something like that.
2. Make the Provider responsible for choosing a method via some sort of 
extensible dispatching mechanism
>
> A CollectionAdapter instance can choose to implement the Transactional 
> interface if it wishes to support begin/end/compensate semantics.
I think this might be going a little bit overboard here in terms of 
interfaces. I would rather lean toward building this into the 
AbstractCollectionAdapter itself. A la the code snippet I published 
earlier which called begin/end inside.

BTW, I also think we'll need to have begin/end points which we can 
intercept in the actual Provider implementation itself.

Overall, I like the simplified flow and increased coherency!

- Dan

-- 
Dan Diephouse
MuleSource
http://mulesource.com | http://netzooid.com/blog