Merge lp:~leonardr/lazr.restful/multiversion-collection into lp:lazr.restful

This branch makes it possible to serve multiple versions of a web service from the same underlying dataset. The multiversion.txt test shows how it's done. Everything else in this branch is code to make existing lazr.restful code version-aware without breaking the existing tests.

As per the 'version-specific-request-interface' branch, every incoming request is associated with some version of the web service using a marker interface. To make lazr.restful code version-aware I made it request-aware. Instead of ILocation I now use IRequestAwareLocation. IEntry and ICollection lookups are now multi-adapter lookups: previously you could just write "IEntry(data_model_object)". Now you must write "getMultiAdapter((data_model_object, request), IEntry)" -- depending on the marker interface attached to the request object, you may get a different IEntry implementation back.

I had to add some more setup to the unit tests. Previously they got along without defining any configuration or request objects. Now that IEntry lookups require a request object, I had to define fake requests and minimal configuration objects for the unit tests.

There are a few TK places I need to look into. I also use code like this several times:

+ request_interface = getUtility(
+ IVersionedClientRequestImplementation,
+ name=self.request.version)
+ return getGlobalSiteManager().adapters.lookup(
+ (model_schema, request_interface), IEntry).schema

It might be worth writing a helper function.

I'm going through the code now and I'll comment on anything else strange I find.

I fixed all but one of the XXX sections (the remaining XXX can't be removed until we can generate multiple versions from declarations) and removed some useless code.

On January 7, 2010, Leonard Richardson wrote:
> I fixed all but one of the XXX sections (the remaining XXX can't be removed
> until we can generate multiple versions from declarations) and removed
> some useless code.
>
Hi Leonard,

This is really taking shape! I have a few questions and suggestions, so
setting this to

    review needsfixing code

> === modified file 'src/lazr/restful/_resource.py'
> class EntryFieldURL(AbsoluteURL):
> """An IAbsoluteURL adapter for EntryField objects."""
> component.adapts(EntryField, IHTTPRequest)
> @@ -1243,7 +1246,7 @@
> def __init__(self, context, request):
> """Associate this resource with a specific object and request."""
> super(EntryResource, self).__init__(context, request)
> - self.entry = IEntry(context)
> + self.entry = getMultiAdapter((context, request), IEntry)
>

Not that if context is already providing IEntry, you'll get an error. I don't
know if this is a plausible case, but if it is, you'll want to only query the
adapter, if context doesn't provide IEntry.

> # We are given a model schema (IFoo). Look up the
> # corresponding entry schema (IFooEntry).
> model_schema = self.relationship.value_type.schema
> - return getGlobalSiteManager().adapters.lookup1(
> - model_schema, IEntry).schema
> + request_interface = getUtility(
> + IVersionedClientRequestImplementation,
> + name=self.request.version)
> + return getGlobalSiteManager().adapters.lookup(
> + (model_schema, request_interface), IEntry).schema

In my previous review, I suggested I...Factory instead of
IVersionedClientRequestImplementation, here I see that it's not a factory.
Sine you are using the name, why do you need a separate interface. Why not
simply ask for the IWebClientRequest with the specific name? Since all
versioned interface should extend that one, it should work.

> === modified file 'src/lazr/restful/directives/__init__.py'

> + utility = cls()
> + sm = getSiteManager()
> + sm.registerUtility(utility, IWebServiceConfiguration)
> +
> + # Create and register marker interfaces for request objects.
> + for version in set(
> + utility.active_versions + [utility.latest_version_uri_prefix]):
> + classname = ("IWebServiceClientRequestVersion" +
> + version.replace('.', '_'))

There are probably other characters that need to be squashed '-' is likely to
be used.

> + marker_interface = InterfaceClass(
> + classname, (IWebServiceClientRequest,), {})
> + alsoProvides(
> + marker_interface, IVersionedClientRequestImplementation)
> + sm.registerUtility(
> + marker_interface, IVersionedClientRequestImplementation,
> + name=version)
> return True

I suggest writing a unit test for that part that would cover the mangling.

> === modified file 'src/lazr/restful/docs/multiversion.txt'
> +URL generation
> +--------------
> +
> + >>> from zope.component import getSiteManager
> + >>> from zope.traversing.browser...

Thanks for your thorough review. I'm going through it slowly. I'll probably have many comments in reply.

> > # We are given a model schema (IFoo). Look up the
> > # corresponding entry schema (IFooEntry).
> > model_schema = self.relationship.value_type.schema
> > - return getGlobalSiteManager().adapters.lookup1(
> > - model_schema, IEntry).schema
> > + request_interface = getUtility(
> > + IVersionedClientRequestImplementation,
> > + name=self.request.version)
> > + return getGlobalSiteManager().adapters.lookup(
> > + (model_schema, request_interface), IEntry).schema
>
> In my previous review, I suggested I...Factory instead of
> IVersionedClientRequestImplementation, here I see that it's not a factory.
> Sine you are using the name, why do you need a separate interface. Why not
> simply ask for the IWebClientRequest with the specific name? Since all
> versioned interface should extend that one, it should work.

I don't do that because request objects shouldn't be utilities. There's not one request object for every version, but one object for every request. I could create a dummy request object and register that as a utility, but I think that would be really confusing. What I have is also confusing, but it can be explained in an internally consistent way:

1. We need a way to look up a web service description given a version number.
2. These lookups don't operate by version number; they take a marker interface that's different for every version.
3. So we create a way to look up the marker interface given the version number.
4. We implement this by registering the marker interface objects as named utilities.

Does this make sense?

I believe in all these cases we have a specific request object whose .version we look up. If there was some way of extracting from the request object the marker interface that identified the version, we could write a property method WebServiceClientRequest.marker_interface. Is that what you meant? (I could write this property method now, with the utility lookup, and it would clean up the code a bit.)

On January 11, 2010, Leonard Richardson wrote:
> Thanks for your thorough review. I'm going through it slowly. I'll probably
> have many comments in reply.
>
> > > # We are given a model schema (IFoo). Look up the
> > > # corresponding entry schema (IFooEntry).
> > > model_schema = self.relationship.value_type.schema
> > > - return getGlobalSiteManager().adapters.lookup1(
> > > - model_schema, IEntry).schema
> > > + request_interface = getUtility(
> > > + IVersionedClientRequestImplementation,
> > > + name=self.request.version)
> > > + return getGlobalSiteManager().adapters.lookup(
> > > + (model_schema, request_interface), IEntry).schema
> >
> > In my previous review, I suggested I...Factory instead of
> > IVersionedClientRequestImplementation, here I see that it's not a
> > factory. Sine you are using the name, why do you need a separate
> > interface. Why not simply ask for the IWebClientRequest with the specific
> > name? Since all versioned interface should extend that one, it should
> > work.
>
> I don't do that because request objects shouldn't be utilities. There's not
> one request object for every version, but one object for every request. I
> could create a dummy request object and register that as a utility, but I
> think that would be really confusing. What I have is also confusing, but
> it can be explained in an internally consistent way:
>
> 1. We need a way to look up a web service description given a version
> number. 2. These lookups don't operate by version number; they take a
> marker interface that's different for every version. 3. So we create a way
> to look up the marker interface given the version number. 4. We implement
> this by registering the marker interface objects as named utilities.
>
> Does this make sense?

Yes, this does make sense. And I do think you should be registering the marker
interface themselve. Remember that a class (or an interface) is an object in
itself. It's already a pattern in zope, the ZCA registers all interfaces as
utility already.

You are also right that what I suggest doesn't work, since the
IWebServiceRequest subclass don't provide IWebServiceRequest themselves. So
what you are doing is the proper way to do. I think I just don't like the name
(it's too long, and the utility you are retrieving isn't an implementation.)
How about IWebServiceVersion ?

>
> I believe in all these cases we have a specific request object whose
> .version we look up. If there was some way of extracting from the request
> object the marker interface that identified the version, we could write a
> property method WebServiceClientRequest.marker_interface. Is that what you
> meant? (I could write this property method now, with the utility lookup,
> and it would clean up the code a bit.)
>

--
Francis J. Lacoste
<email address hidden>

> > === modified file 'src/lazr/restful/_resource.py'
> > class EntryFieldURL(AbsoluteURL):
> > """An IAbsoluteURL adapter for EntryField objects."""
> > component.adapts(EntryField, IHTTPRequest)
> > @@ -1243,7 +1246,7 @@
> > def __init__(self, context, request):
> > """Associate this resource with a specific object and request."""
> > super(EntryResource, self).__init__(context, request)
> > - self.entry = IEntry(context)
> > + self.entry = getMultiAdapter((context, request), IEntry)
> >
>
> Not that if context is already providing IEntry, you'll get an error. I don't
> know if this is a plausible case, but if it is, you'll want to only query the
> adapter, if context doesn't provide IEntry.

I could go either way on this, but I kind of consider it an error to pass an Entry object directly into EntryResource. I think allowing that would encourage sloppy thinking and cause lazr.restful developers to confuse entries and data model objects. What do you think?

> > + # Create and register marker interfaces for request objects.
> > + for version in set(
> > + utility.active_versions + [utility.latest_version_uri_prefix]):
> > + classname = ("IWebServiceClientRequestVersion" +
> > + version.replace('.', '_'))
>
> There are probably other characters that need to be squashed '-' is likely to
> be used.

I was lazy. I've added a make_identifier_safe function to lazr.restful.utils and a doctest for it.

> > === modified file 'src/lazr/restful/docs/multiversion.txt'
> > +URL generation
> > +--------------
> > +
> > + >>> from zope.component import getSiteManager
> > + >>> from zope.traversing.browser.interfaces import IAbsoluteURL
> > + >>> from lazr.restful.interfaces import (
> > + ... IRequestAwareLocation, IWebServiceLayer)
> > + >>> from lazr.restful.simple import AbsoluteURL
> > + >>> sm = getSiteManager()
> > + >>> sm.registerAdapter(
> > + ... AbsoluteURL, (IRequestAwareLocation, IWebServiceLayer),
> > + ... provided=IAbsoluteURL)
>
> Can you add a paragraph explaining what the above does?

OK.

> > Here's the interface for the 'set' object that manages the contacts.
> >
> > + >>> from zope.interface import implements
> > >>> from lazr.restful.interfaces import ITraverseWithGet
> > >>> class IContactSet(ITestDataObject, ITraverseWithGet):
> > - ... def getAll(self):
> > + ... def getAllContacts():
> > ... "Get all contacts."
> > ...
> > - ... def get(self, name):
> > + ... def get(request, name):
> > ... "Retrieve a single contact by name."
>
> Why is request part of the interface signature?

That method doesn't need to be defined in IContactSet at all, because it's inherited from ITraverseWithGet. ITraverseWithGet.get() needs to take a request as an argument because the traversal rules can change from one version to another. But they don't change in this case. I've removed the redundant definition.

> > Here's a simple ContactSet with a predefined list of contacts.
> >
> > + >>> from zope.publisher.interfaces.browser...

> You are also right that what I suggest doesn't work, since the
> IWebServiceRequest subclass don't provide IWebServiceRequest themselves. So
> what you are doing is the proper way to do. I think I just don't like the name
> (it's too long, and the utility you are retrieving isn't an implementation.)
> How about IWebServiceVersion ?

OK, I've renamed it to IWebServiceVersion.

Apropos getting rid of IRequestAwareLocation: I think you were envisioning some code like this:

    >>> class ContactSetLocation(object):
    ... """An ILocation implementation for ContactSet objects."""
    ... implements(ILocation)
    ...
    ... def __init__(self, context, request):
    ... self.context = contact_set
    ... self.request = request
    ...
    ... def __parent__(self, request):
    ... return getUtility(IServiceRootResource, name=request.version)
    ...
    ... def __name__(self, request):
    ... if request.version == 'beta':
    ... return 'contact_list'
    ... return 'contacts'

    >>> from zope.traversing.browser import AbsoluteURL as ZopeAbsoluteURL
    >>> sm.registerAdapter(
    ... ZopeAbsoluteURL, (ILocation, IWebServiceLayer),
    ... provided=IAbsoluteURL)

    >>> sm.registerAdapter(
    ... ContactSetLocation, (IContactSet, IWebServiceLayer),
    ... provided=ILocation)

Unfortunately this doesn't work with Zope's AbsoluteURL implementation, which contains this line:

        context = ILocation(context)

I never get a chance to convert ContactSet into a ContactSetLocation.

I can modify my alternate implementation of AbsoluteURL to try a multi-adapter lookup before trying a simple cast to ILocation. I'll still need the alternate AbsoluteURL but I'd be able to get rid of the IRequestAwareLocation interface. Does this make sense? Am I missing something?

On January 11, 2010, Leonard Richardson wrote:
> > > === modified file 'src/lazr/restful/_resource.py'
> > > class EntryFieldURL(AbsoluteURL):
> > > """An IAbsoluteURL adapter for EntryField objects."""
> > > component.adapts(EntryField, IHTTPRequest)
> > > @@ -1243,7 +1246,7 @@
> > > def __init__(self, context, request):
> > > """Associate this resource with a specific object and
> > > request.""" super(EntryResource, self).__init__(context, request) -
> > > self.entry = IEntry(context)
> > > + self.entry = getMultiAdapter((context, request), IEntry)
> >
> > Not that if context is already providing IEntry, you'll get an error. I
> > don't know if this is a plausible case, but if it is, you'll want to only
> > query the adapter, if context doesn't provide IEntry.
>
> I could go either way on this, but I kind of consider it an error to pass
> an Entry object directly into EntryResource. I think allowing that would
> encourage sloppy thinking and cause lazr.restful developers to confuse
> entries and data model objects. What do you think?

Sure.

> > > Here's a simple ContactSet with a predefined list of contacts.
> > >
> > > + >>> from zope.publisher.interfaces.browser import IBrowserRequest
> > > + >>> from lazr.restful.interfaces import IServiceRootResource
> > >
> > > >>> from lazr.restful.simple import TraverseWithGet
> > >
> > > - >>> from zope.publisher.interfaces.browser import IBrowserRequest
> > >
> > > >>> class ContactSet(TraverseWithGet):
> > >
> > > ... implements(IContactSet)
> > > - ... path = "contacts"
> > > ...
> > > ... def __init__(self):
> > > ... self.contacts = CONTACTS
> > > ...
> > > - ... def get(self, name):
> > > - ... contacts = [contact for contacts in self.contacts
> > > - ... if pair.name == name]
> > > + ... def get(self, request, name):
> > > + ... contacts = [contact for contact in self.contacts
> > > + ... if contact.name == name]
> > > ... if len(contacts) == 1:
> > > ... return contacts[0]
> > > ... return None
> >
> > From the implementation, I don't see why request is required?
>
> As before, it's inherited from ITraverseWithGet.

Ok.

--
Francis J. Lacoste
<email address hidden>

On January 11, 2010, Leonard Richardson wrote:
> Apropos getting rid of IRequestAwareLocation: I think you were envisioning
some code like this:
> >>> class ContactSetLocation(object):
>
> ... """An ILocation implementation for ContactSet objects."""
> ... implements(ILocation)
> ...
> ... def __init__(self, context, request):
> ... self.context = contact_set
> ... self.request = request
> ...
> ... def __parent__(self, request):
> ... return getUtility(IServiceRootResource,
> name=request.version) ...
> ... def __name__(self, request):
> ... if request.version == 'beta':
> ... return 'contact_list'
> ... return 'contacts'
>

I'd expect __parent__ and __name__ to be property. You don't need the request
parameter since you have it from the constructor.

> >>> from zope.traversing.browser import AbsoluteURL as ZopeAbsoluteURL
> >>> sm.registerAdapter(
>
> ... ZopeAbsoluteURL, (ILocation, IWebServiceLayer),
> ... provided=IAbsoluteURL)
>
> >>> sm.registerAdapter(
>
> ... ContactSetLocation, (IContactSet, IWebServiceLayer),
> ... provided=ILocation)
>
> Unfortunately this doesn't work with Zope's AbsoluteURL implementation,
> which contains this line:
>
> context = ILocation(context)
>
> I never get a chance to convert ContactSet into a ContactSetLocation.

I see. I suggest then that you simply provide an ILocation adapter and use
get_current_browser_request() to get the request parameter.

>
> I can modify my alternate implementation of AbsoluteURL to try a
> multi-adapter lookup before trying a simple cast to ILocation. I'll still
> need the alternate AbsoluteURL but I'd be able to get rid of the
> IRequestAwareLocation interface. Does this make sense? Am I missing
> something?
>

--
Francis J. Lacoste
<email address hidden>

If I use get_current_browser_request() I shouldn't need an ILocation adapter at all. (Though one is still possible.) I might as well make the data model objects implement ILocation directly.

On that note, I remember what I was doing with the IRequestAwareLocation thing. We can always implement a standard ILocation adapter that grabs the request and branches based on request.version. But with IRequestAwareLocation we can define totally different adapters for different versions. BetaContactLocation can provide the location for version 'beta' and DevContactLocation can provide the location for version 'dev'.

We don't really need that to get a basic system working, but it might be useful in the future. What do you think? I'll work on taking the code out for now.

On January 12, 2010, Leonard Richardson wrote:
> If I use get_current_browser_request() I shouldn't need an ILocation
> adapter at all. (Though one is still possible.) I might as well make the
> data model objects implement ILocation directly.
>
> On that note, I remember what I was doing with the IRequestAwareLocation
> thing. We can always implement a standard ILocation adapter that grabs the
> request and branches based on request.version. But with
> IRequestAwareLocation we can define totally different adapters for
> different versions. BetaContactLocation can provide the location for
> version 'beta' and DevContactLocation can provide the location for version
> 'dev'.
>
> We don't really need that to get a basic system working, but it might be
> useful in the future. What do you think? I'll work on taking the code out
> for now.
>

We shouldn't introduce other abstraction layers until they are needed :-) I
call YAGNI for now.

--
Francis J. Lacoste
<email address hidden>

OK, I've made all the changes you requested. The complete diff of my changes since your review is here:

http://pastebin.ubuntu.com/355558/

On January 12, 2010, Leonard Richardson wrote:
> OK, I've made all the changes you requested. The complete diff of my
> changes since your review is here:
>
> http://pastebin.ubuntu.com/355558/
>

That's great, thanks. My only comment is about the NEWS.txt file:

> === modified file 'src/lazr/restful/NEWS.txt'
> --- src/lazr/restful/NEWS.txt 2009-11-16 14:49:53 +0000
> +++ src/lazr/restful/NEWS.txt 2010-01-12 15:17:51 +0000
> @@ -9,10 +9,11 @@
> changes. You *must* change your configuration object to get your code
> to work in this version! See "active_versions" below.
>
> -Added the precursor of a versioning system for web services. Clients
> -can now request the "trunk" of a web service as well as one published
> -version. Apart from the URIs served, the two web services are exactly
> -the same.
> +Added a versioning system for web services. Clients can now request
> +the "trunk" of a web service as well as one published version. Apart
> +from the URIs served, the two web services are exactly the same. There
> +is no way to serve two different versions of a web service without
> +defining both versions from scratch.

You might want to make this less harsh by stating that a next version will add
annotations to make that easy to do.

  review approve code
  status approved

--
Francis J. Lacoste
<email address hidden>