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.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? > 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? > 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? > Once we register ContactCollection as the ICollection implementation, > we can adapt the ContactSet object to a web service ICollection. > > +<<<<<<< TREE > >>> for version in ['beta', 'dev', '1.0']: > ... sm.registerAdapter( > ... ContactCollection, provided=ICollection, name=version) Conflict marker. > + > +Using the web service > +===================== > + > +Now that we can create versioned web service requests, let's try out > +the different versions of the web service. > + > +Beta > +---- > + > +Here's the service root resource. > + > + >>> import simplejson > + >>> request = create_web_service_request('/beta/') > + >>> resource = request.traverse(None) > + >>> body = simplejson.loads(resource()) > + >>> print sorted(body.keys()) > + ['contacts_collection_link', 'resource_type_link'] > + > + >>> print body['contacts_collection_link'] > + http://api.multiversion.dev/beta/contact_list > + > +Here's the contact list. > + > + >>> request = create_web_service_request('/beta/contact_list') > + >>> resource = request.traverse(None) > + >>> print absoluteURL(contact_set, request) > + http://api.multiversion.dev/beta/contact_list > + > + >>> body = simplejson.loads(resource()) > + >>> body['total_size'] > + 2 > + >>> for link in sorted( > + ... [contact['self_link'] for contact in body['entries']]): > + ... print link > + http://api.multiversion.dev/beta/contact_list/Cleo%20Python > + http://api.multiversion.dev/beta/contact_list/Oliver%20Bluth > + > +We can traverse through the collection to an entry. > + > + >>> request_beta = create_web_service_request( > + ... '/beta/contact_list/Cleo Python') > + >>> resource = request_beta.traverse(None) > + >>> print absoluteURL(C1, request_beta) I don't understand what C1 is here. That's what you presume is the resource context? Would be better to use resource.context explicitely, no? You'll want to apply any changes also to the other version examples. > === modified file 'src/lazr/restful/docs/webservice.txt' > +We also need to define a marker interface for each version of the web > +service, so that incoming requests can be marked with the appropriate > +version string. The configuration above defines two versions, 'beta' > +and 'devel'. > + > + >>> from lazr.restful.interfaces import ( > + ... IVersionedClientRequestImplementation, IWebServiceClientRequest) > + >>> class IWebServiceRequestBeta(IWebServiceClientRequest): > + ... pass > + > + >>> class IWebServiceRequestDevel(IWebServiceClientRequest): > + ... pass > + > + >>> versions = ((IWebServiceRequestBeta, 'beta'), > + ... (IWebServiceRequestDevel, 'devel')) > + > + >>> from zope.interface import alsoProvides > + >>> for cls, version in versions: > + ... alsoProvides(cls, IVersionedClientRequestImplementation) > + ... sm.registerUtility(cls, IVersionedClientRequestImplementation, > + ... name=version) > + > + Since you do that in quite a few places, you might want to provide a helper in lazr.restful.testing to do that kind of registration more expressively? > === modified file 'src/lazr/restful/interfaces/_rest.py' > +class IRequestAwareLocation(Interface): > + """An ILocation-like interface for resources that change location. > + > + Versioning is the most common reason why a resource would change > + location depending on the request. A resource may have one name or > + parent in version N, and another name or parent in version N+1. > + """ > + > + def __parent__(request): > + """The parent in the location hierarchy.""" > + > + def __name__(request): > + """The name within the parent.""" > + > + Why is this interface needed? And why keep the attribute names of ILocation when you are actually transforming this into methods? Why not use simply define an ILocation multi-adapter. getMultiAdapter((ob, request), ILocation). And __parent__ and __name__ could still be attributes? > === modified file 'src/lazr/restful/publisher.py' > --- src/lazr/restful/publisher.py 2010-01-06 21:52:12 +0000 > +++ src/lazr/restful/publisher.py 2010-01-06 21:52:12 +0000 > @@ -62,8 +62,8 @@ > # handle traversing to the scoped collection itself. > if len(request.getTraversalStack()) == 0: > try: > - entry = IEntry(ob) > - except TypeError: > + entry = getMultiAdapter((ob, request), IEntry) > + except ComponentLookupError: > pass Do you need to check that ob provides IEntry? > ======= > # Find the version-specific interface this request should > # provide, and provide it. > - try: > - to_provide = getUtility(IVersionedClientRequestImplementation, > - name=version) > - alsoProvides(self, to_provide) > - except ComponentLookupError: > - # XXX leonardr 2009-11-23 This stops single-version tests > - # from breaking. I'll probably remove it once the > - # necessary version-specific interfaces are automatically > - # generated. > - pass > + to_provide = getUtility(IVersionedClientRequestImplementation, > + name=version) > + alsoProvides(self, to_provide) > self.version = version With my previous name suggestion, this would become: version_interface = getUtility(IWebClientRequest, name=version) alsoProvides(self, version_interface) > === modified file 'src/lazr/restful/simple.py' > +class AbsoluteURL(ZopeAbsoluteURL): > + """An IAbsoluteURL implementation for IRequestAwareLocation objects. > + > + This code is a copy of Zope's AbsoluteURL implementation, except > + that it deals with IRequestAwareLocation objects rather than > + ILocation objects. In IRequestAwareLocation objects, __name__ and > + __parent__ are not attributes. They're methods that take the > + request object as an argument. > + """ > + def __str__(self): > + context = self.context > + request = self.request > + > + # The application URL contains all the namespaces that are at the > + # beginning of the URL, such as skins, virtual host specifications and > + # so on. > + if (context is None > + or sameProxiedObjects(context, request.getVirtualHostRoot())): > + return request.getApplicationURL() > + > + context = IRequestAwareLocation(context) > + container = context.__parent__(request) > + if container is None: > + raise TypeError(_insufficientContext) > + > + url = str(getMultiAdapter((container, request), IAbsoluteURL)) > + name = context.__name__(request) > + if name is None: > + raise TypeError(_insufficientContext) > + > + if name: > + url += '/' + urllib.quote(name.encode('utf-8'), _safe) > + > + return url > + > + __call__ = __str__ > + I don't understand the need for this class? > === modified file 'src/lazr/restful/tests/test_navigation.py' > class NavigationTestCase(unittest.TestCase): > > + def setUp(self): > + # Register ChildEntry as the IEntry implementation for IChild. > + sm = getSiteManager() > + sm.registerAdapter( > + ChildEntry, [IChild, IWebServiceClientRequest], provided=IEntry) > + > + # Register ParentEntry as the IEntry implementation for IParent. > + sm.registerAdapter( > + ParentEntry, [IParent, IWebServiceClientRequest], provided=IEntry) > + You probably want to add a tearDown calling zope.testing.cleanup.cleanUp to make sure these registrations are undone. -- Francis J. Lacoste