Merge lp:~leonardr/lazr.restful/forbid-reference-to-entry-not-published-in-this-version into lp:lazr.restful

[Leonard Richardson]

This branch makes it impossible to define a Reference to an unpublished interface. This might be an interface that's not published *at all*, because you forgot export_as_webservice_entry(): a common mistake and one that I even found in our test suite. Or it might be an interface that is published in some versions but not others. If IFoo shows up starting in 2.0, you can't get IBar to publish a reference to an IFoo in 1.0.

The sanity checking happens after all the registrations are done. (This is necessary because IFoo may be defined after IBar, and the Reference schema patched.) I gather information about each published interface as it's registered. After they've all been registered, I get a list of all the IEntry registrations for different versions. (In practice, these two lists should be the same, but I don't know if I want to rely on that--an application might do some manual registrations or other trickery.)

Then, for each version, for each Reference field in a published interface, I see if the Reference points to some Interface that's registered for that version.

The sanity check code also keeps track of named operation registrations. This is for the next phase of the work, in which you will be prohibited from publishing a named operation in 1.0 if it has arguments or a return value of a type not published in 1.0.

I suspect I have not come up with all the tests necessary to show that this works in general; let me know if you can come up with more.

[Tim Penhey (thumper)]

A simple docstring for webservice_sanity_checks would be nice.

Perhaps a dict string mapping would be better here:
    raise ValueError(
        "In version %s, %s.%s is a Reference to %s, "
        "but version %s of the web service does not publish "
        "%s as an entry. (It may not be published "
        "at all.)" % (
            version, interface.__name__, field.__name__,
            referenced_interface.__name__, version,
            referenced_interface.__name__))
becomes
    raise ValueError(
        "In version %(version)s, %(interface)s.%(field)s is a Reference "
        "to %(reference)s, but version %(version)s of the web service "
        "does not publish %(reference)s as an entry. "
        "(It may not be published at all.)" % {
            'version': version,
            'field': field.__name__,
            'interface': interface.__name__,
            'reference': referenced_interface.__name__}

For your assert raises test, why not grab the copy from testtools?

    def assertRaises(self, excClass, callableObj, *args, **kwargs):
        """Fail unless an exception of class excClass is thrown
           by callableObj when invoked with arguments args and keyword
           arguments kwargs. If a different type of exception is
           thrown, it will not be caught, and the test case will be
           deemed to have suffered an error, exactly as for an
           unexpected exception.
        """
        try:
            ret = callableObj(*args, **kwargs)
        except excClass:
            return sys.exc_info()[1]
        else:
            excName = self._formatTypes(excClass)
            self.fail("%s not raised, %r returned instead." % (excName, ret))

You'd need this too:

    def _formatTypes(self, classOrIterable):
        """Format a class or a bunch of classes for display in an error."""
        className = getattr(classOrIterable, '__name__', None)
        if className is None:
            className = ', '.join(klass.__name__ for klass in classOrIterable)
        return className

test_reference_to_object_published_later_fails exception string test
will always pass as you are only checking that the string isn't empty.

On the whole I think the implementation is fine.

review: Needs Fixing

[lp:~leonardr/lazr.restful/forbid-reference-to-entry-not-published-in-this-version]

209. By Leonard Richardson on 2011-03-18

Replaced _assertRaises with a copy of assertRaises. Made a test pass for the right reasons. Change a complicated string interpolation to use a dict mapping.

210. By Leonard Richardson on 2011-03-18

Added explanation of why my 'success' test worked right away even though I didn't expect it to.

[Leonard Richardson (leonardr)]

I figured out why I was worried about not having all the tests. Basically, I didn't expect test_reference_to_object_published_earlier_succeeds() to work and I was surprised that it did. I didn't understand why the code I wrote would find IPublishedEarly in 2.0 when it was defined in 1.0 and not changed afterwards.

The answer is that lazr.restful creates and registers an IEntry adapter for each published entry for each version. It's not smart enough to see that an entry hasn't changed since the last version. Now that lack of optimization has paid off.

=== modified file 'src/lazr/restful/metazcml.py'
--- src/lazr/restful/metazcml.py	2011-03-10 14:07:30 +0000
+++ src/lazr/restful/metazcml.py	2011-03-18 12:22:30 +0000
@@ -9,24 +9,41 @@
 import inspect
 import itertools
 
-from zope.component import getUtility
+from zope.component import (
+    getGlobalSiteManager,
+    getUtility,
+    )
 from zope.component.zcml import handler
 from zope.configuration.fields import GlobalObject
 from zope.interface import Interface
 from zope.interface.interfaces import IInterface
 
-
 from lazr.restful.declarations import (
-    COLLECTION_TYPE, ENTRY_TYPE, LAZR_WEBSERVICE_EXPORTED, OPERATION_TYPES,
-    REMOVED_OPERATION_TYPE, generate_collection_adapter,
-    generate_entry_adapters, generate_entry_interfaces,
-    generate_operation_adapter)
+    COLLECTION_TYPE,
+    ENTRY_TYPE,
+    LAZR_WEBSERVICE_EXPORTED,
+    OPERATION_TYPES,
+    REMOVED_OPERATION_TYPE,
+    generate_collection_adapter,
+    generate_entry_adapters,
+    generate_entry_interfaces,
+    generate_operation_adapter,
+    )
 from lazr.restful.error import WebServiceExceptionView
 
 from lazr.restful.interfaces import (
-    ICollection, IEntry, IResourceDELETEOperation, IResourceGETOperation,
-    IResourcePOSTOperation, IWebServiceClientRequest,
-    IWebServiceConfiguration, IWebServiceVersion)
+    ICollection,
+    IEntry,
+    IReference,
+    IResourceDELETEOperation,
+    IResourceGETOperation,
+    IResourcePOSTOperation,
+    IWebServiceClientRequest,
+    IWebServiceConfiguration,
+    IWebServiceVersion,
+    )
+
+from lazr.restful.utils import VersionedObject
 
 
 class IRegisterDirective(Interface):
@@ -36,7 +53,8 @@
         title=u'Module which will be inspected for webservice declarations')
 
 
-def generate_and_register_entry_adapters(interface, info, contributors):
+def generate_and_register_entry_adapters(interface, info, contributors,
+                                         repository=None):
     """Generate an entry adapter for every version of the web service.
 
     This code generates an IEntry subinterface for every version, each
@@ -54,9 +72,9 @@
         interface, contributors, *versions)
     web_factories = generate_entry_adapters(
         interface, contributors, web_interfaces)
-    for i in range(0, len(web_interfaces)):
-        interface_version, web_interface = web_interfaces[i]
-        factory_version, factory = web_factories[i]
+    for index, (interface_version, web_interface) in (
+        enumerate(web_interfaces)):
+        factory_version, factory = web_factories[index]
         assert factory_version==interface_version, (
             "Generated interface and factory versions don't match up! "
             '%s vs. %s' % (factory_version, interface_version))
@@ -69,6 +87,11 @@
         register_adapter_for_version(factory, interface, interface_version,
                                      IEntry, '', info)
 
+        # If we were given a repository, add the interface and version
+        # to it.
+        if repository is not None:
+            repository.append(VersionedObject(interface_version, web_interface))
+
 
 def ensure_correct_version_ordering(name, version_list):
     """Make sure that a list mentions versions from earliest to latest.
@@ -146,6 +169,7 @@
     handler('registerAdapter', factory, (interface, marker),
             provides, name, info)
 
+
 def _is_exported_interface(member):
     """Helper for find_exported_interfaces; a predicate to inspect.getmembers.
 
@@ -163,6 +187,7 @@
             return True
     return False
 
+
 def find_exported_interfaces(module):
     """Find all the interfaces in a module marked for export.
 
@@ -173,6 +198,7 @@
     return (interface for name, interface
             in inspect.getmembers(module, _is_exported_interface))
 
+
 def find_interfaces_and_contributors(module):
     """Find the interfaces and its contributors marked for export.
 
@@ -254,6 +280,11 @@
                         module, type(module))
     interfaces_with_contributors = find_interfaces_and_contributors(module)
 
+    # Keep track of entry and operation registrations so we can
+    # sanity-check them later.
+    registered_entries = []
+    registered_operations = []
+
     for interface, contributors in interfaces_with_contributors.items():
         if issubclass(interface, Exception):
             register_exception_view(context, interface)
@@ -264,7 +295,8 @@
             context.action(
                 discriminator=('webservice entry interface', interface),
                     callable=generate_and_register_entry_adapters,
-                    args=(interface, context.info, contributors),
+                    args=(interface, context.info, contributors,
+                          registered_entries),
                     )
         elif tag['type'] == COLLECTION_TYPE:
             for version in tag['collection_default_content'].keys():
@@ -282,12 +314,74 @@
             raise AssertionError('Unknown export type: %s' % tag['type'])
         context.action(
             discriminator=('webservice versioned operations', interface),
-            args=(context, interface, contributors),
+            args=(context, interface, contributors, registered_operations),
             callable=generate_and_register_webservice_operations)
 
+    # Now that all the adapters and operations are registered, run a
+    # sanity check to make sure no version of the web service
+    # references a feature not available in that version
+    context.action(
+        discriminator=('webservice sanity checks'),
+        args=(registered_entries, registered_operations),
+        callable=webservice_sanity_checks)
+
+
+def webservice_sanity_checks(entries, operations):
+    """Ensure the web service contains no references to unpublished objects.
+
+    We are worried about fields that link to unpublished objects, and
+    operations that have arguments or return values that are
+    unpublished. An unpublished object may not be published at all, or
+    it may not be published in the same version in which it's
+    referenced.
+    """
+
+    # Create a mapping of marker interfaces to version names.
+    versions = getUtility(IWebServiceConfiguration).active_versions
+    version_for_marker = { IWebServiceClientRequest: versions[0] }
+    for version in versions:
+        marker_interface = getUtility(IWebServiceVersion, name=version)
+        version_for_marker[marker_interface] = version
+
+    # For each version, build a list of all of the IEntries published
+    # in that version's web service. This works because every IEntry
+    # is explicitly registered for every version, even if there's been
+    # no change since the last version. For the sake of performance,
+    # the list is stored as a set of VersionedObject 2-tuples.
+    available_registrations = set()
+    registrations = getGlobalSiteManager().registeredAdapters()
+    for registration in registrations:
+        if (not IInterface.providedBy(registration.provided)
+            or not registration.provided.isOrExtends(IEntry)):
+            continue
+        interface, version_marker = registration.required
+        available_registrations.add(VersionedObject(
+                version_for_marker[version_marker], interface))
+
+    # Check every Reference field in every IEntry interface, making
+    # sure that it's a Reference to another IEntry published in that
+    # version.
+    for version, interface in entries:
+        if version is None:
+            version = versions[0]
+        for name, field in interface.namesAndDescriptions():
+            if IReference.providedBy(field):
+                referenced_interface = field.schema
+                to_check = VersionedObject(version, referenced_interface)
+                if to_check not in available_registrations:
+                    raise ValueError(
+                        "In version %(version)s, %(interface)s.%(field)s "
+                        "is a Reference to %(reference)s, but version "
+                        "%(version)s of the web service does not publish "
+                        "%(reference)s as an entry. (It may not be published "
+                        "at all.)" % dict(
+                            version=version, interface=interface.__name__,
+                            field=field.__name__,
+                            reference=referenced_interface.__name__))
+
 
 def generate_and_register_webservice_operations(
-        context, interface, contributors):
+        context, interface, contributors, repository=None):
     """Create and register adapters for all exported methods.
 
     Different versions of the web service may publish the same
@@ -458,6 +552,8 @@
                         # the operation registration for this mutator
                         # will need to be blocked.
                         mutator_operation_needs_to_be_blocked = True
+                if repository is not None:
+                    repository.append(VersionedObject(version, method))
             previous_operation_name = operation_name
             previous_operation_provides = operation_provides
 
 
=== modified file 'src/lazr/restful/publisher.py'
--- src/lazr/restful/publisher.py	2011-01-26 19:32:05 +0000
+++ src/lazr/restful/publisher.py	2011-03-18 12:22:30 +0000
@@ -9,7 +9,6 @@
 __metaclass__ = type
 __all__ = [
     'browser_request_to_web_service_request',
-    'TraverseWithGet',
     'WebServicePublicationMixin',
     'WebServiceRequestTraversal',
     ]
 
=== modified file 'src/lazr/restful/testing/webservice.py'
--- src/lazr/restful/testing/webservice.py	2011-03-10 13:30:32 +0000
+++ src/lazr/restful/testing/webservice.py	2011-03-18 12:22:30 +0000
@@ -472,6 +472,34 @@
         sm.registerUtility(
             IWebServiceTestRequest20, IWebServiceVersion, name='2.0')
 
+    def assertRaises(self, excClass, callableObj, *args, **kwargs):
+        """Fail unless an exception of class excClass is thrown
+           by callableObj when invoked with arguments args and keyword
+           arguments kwargs. If a different type of exception is
+           thrown, it will not be caught, and the test case will be
+           deemed to have suffered an error, exactly as for an
+           unexpected exception.
+
+           This is a copy of assertRaises from testtools.
+        """
+        try:
+            ret = callableObj(*args, **kwargs)
+        except excClass:
+            return sys.exc_info()[1]
+        else:
+            excName = self._formatTypes(excClass)
+            self.fail("%s not raised, %r returned instead." % (excName, ret))
+
+    def _formatTypes(self, classOrIterable):
+        """Format a class or a bunch of classes for display in an error.
+
+           This is a copy of _formatTypes from testtools.
+        """
+        className = getattr(classOrIterable, '__name__', None)
+        if className is None:
+            className = ', '.join(klass.__name__ for klass in classOrIterable)
+        return className
+
     def fake_request(self, version):
         request = FakeRequest(version=version)
         alsoProvides(
 
=== modified file 'src/lazr/restful/tests/test_declarations.py'
--- src/lazr/restful/tests/test_declarations.py	2011-03-18 12:22:30 +0000
+++ src/lazr/restful/tests/test_declarations.py	2011-03-18 12:22:30 +0000
@@ -1,3 +1,4 @@
+from zope.configuration.config import ConfigurationExecutionError
 from zope.component import (
     adapts,
     getMultiAdapter,
@@ -102,7 +103,7 @@
         # with its original name whereas for the '2.0' version it's exported
         # as 'development_branch_20'.
         self.product._dev_branch = Branch('A product branch')
-        register_test_module('testmod', IProduct, IHasBranches)
+        register_test_module('testmod', IBranch, IProduct, IHasBranches)
         adapter = getMultiAdapter((self.product, self.one_zero_request), IEntry)
         self.assertEqual(adapter.development_branch, self.product._dev_branch)
 
@@ -116,7 +117,7 @@
         # development_branch field, but on version '2.0' that field can be
         # modified as we define a mutator for it.
         self.product._dev_branch = Branch('A product branch')
-        register_test_module('testmod', IProduct, IHasBranches)
+        register_test_module('testmod', IBranch, IProduct, IHasBranches)
         adapter = getMultiAdapter((self.product, self.one_zero_request), IEntry)
         try:
             adapter.development_branch = None
@@ -151,7 +152,8 @@
         # from both IHasBugs and IHasBranches.
         self.product._bug_count = 10
         self.product._dev_branch = Branch('A product branch')
-        register_test_module('testmod', IProduct, IHasBugs, IHasBranches)
+        register_test_module(
+            'testmod', IBranch, IProduct, IHasBugs, IHasBranches)
         adapter = getMultiAdapter((self.product, self.one_zero_request), IEntry)
         self.assertEqual(adapter.bug_count, 10)
         self.assertEqual(adapter.development_branch, self.product._dev_branch)
@@ -172,7 +174,8 @@
         # When looking up an entry's redacted_fields, we take into account the
         # interface where the field is defined and adapt the context to that
         # interface before accessing that field.
-        register_test_module('testmod', IProduct, IHasBugs, IHasBranches)
+        register_test_module(
+            'testmod', IBranch, IProduct, IHasBugs, IHasBranches)
         entry_resource = EntryResource(self.product, self.one_zero_request)
         self.assertEquals([], entry_resource.redacted_fields)
 
@@ -180,7 +183,8 @@
         # When looking up an entry's redacted_fields for an object which is
         # security proxied, we use the security checker for the interface
         # where the field is defined.
-        register_test_module('testmod', IProduct, IHasBugs, IHasBranches)
+        register_test_module(
+            'testmod', IBranch, IProduct, IHasBugs, IHasBranches)
         newInteraction()
         try:
             secure_product = ProxyFactory(
@@ -194,7 +198,8 @@
     def test_duplicate_contributed_attributes(self):
         # We do not allow a given attribute to be contributed to a given
         # interface by more than one contributing interface.
-        testmod = create_test_module('testmod', IProduct, IHasBugs, IHasBugs2)
+        testmod = create_test_module(
+            'testmod', IBranch, IProduct, IHasBugs, IHasBugs2)
         self.assertRaises(
             ConflictInContributingInterfaces,
             find_interfaces_and_contributors, testmod)
@@ -205,7 +210,7 @@
         class DummyHasBranches:
             implements(IHasBranches)
         dummy = DummyHasBranches()
-        register_test_module('testmod', IProduct, IHasBranches)
+        register_test_module('testmod', IBranch, IProduct, IHasBranches)
         self.assertRaises(
             ComponentLookupError,
             getMultiAdapter, (dummy, self.one_zero_request), IEntry)
@@ -318,6 +323,7 @@
 
 
 class IBranch(Interface):
+    export_as_webservice_entry()
     name = TextLine(title=u'The branch name')
 
 
@@ -552,16 +558,6 @@
         self.utility = getUtility(IWebServiceConfiguration)
         self.utility.require_explicit_versions = True
 
-    def _assertRaises(self, exception, func, *args, **kwargs):
-        # Approximate the behavior of testtools assertRaises, which
-        # returns the raised exception. I couldn't get
-        # testtools.TestCase to play nicely with zope's Cleanup class.
-        self.assertRaises(exception, func, *args, **kwargs)
-        try:
-            func(*args, **kwargs)
-        except Exception, e:
-            return e
-
     def test_entry_exported_with_as_of_succeeds(self):
         # An entry exported using as_of is present in the as_of_version
         # and in subsequent versions.
@@ -582,7 +578,7 @@
         self.assertEquals(interfaces[0].version, '2.0')
 
     def test_entry_exported_without_as_of_fails(self):
-        exception = self._assertRaises(
+        exception = self.assertRaises(
             ValueError, generate_entry_interfaces,
             IEntryExportedWithoutAsOf, [],
             *self.utility.active_versions)
@@ -618,7 +614,7 @@
     def test_field_not_exported_using_as_of_fails(self):
         # If you export a field without specifying as_of, you get an
         # error.
-        exception = self._assertRaises(
+        exception = self.assertRaises(
             ValueError, generate_entry_interfaces,
             IFieldExportedWithoutAsOf, [], *self.utility.active_versions)
         self.assertEquals(
@@ -631,7 +627,7 @@
     def test_field_cannot_be_both_exported_and_not_exported(self):
         # If you use the as_of keyword argument, you can't also set
         # the exported keyword argument to False.
-        exception = self._assertRaises(
+        exception = self.assertRaises(
             ValueError, exported, TextLine(), as_of='1.0', exported=False)
         self.assertEquals(
                 str(exception),
@@ -640,7 +636,7 @@
 
     def test_field_exported_as_of_nonexistent_version_fails(self):
         # You can't export a field as_of a nonexistent version.
-        exception = self._assertRaises(
+        exception = self.assertRaises(
             ValueError, generate_entry_interfaces,
             IFieldAsOfNonexistentVersion, [],
             *self.utility.active_versions)
@@ -652,7 +648,7 @@
     def test_field_exported_with_duplicate_attributes_fails(self):
         # You can't provide a dictionary of attributes for the
         # version specified in as_of.
-        exception = self._assertRaises(
+        exception = self.assertRaises(
             ValueError, generate_entry_interfaces,
             IFieldDoubleDefinition, [], *self.utility.active_versions)
         self.assertEquals(
@@ -663,7 +659,7 @@
     def test_field_with_annotations_that_precede_as_of_fails(self):
         # You can't provide a dictionary of attributes for a version
         # preceding the version specified in as_of.
-        exception = self._assertRaises(
+        exception = self.assertRaises(
             ValueError, generate_entry_interfaces,
             IFieldDefiningAttributesBeforeAsOf, [],
             *self.utility.active_versions)
@@ -682,7 +678,7 @@
         method = [method for name, method in
                   IFieldImplicitOperationDefinition.namesAndDescriptions()
                   if name == 'implicitly_in_10'][0]
-        exception = self._assertRaises(
+        exception = self.assertRaises(
             ValueError, generate_operation_adapter, method, None)
         self.assertEquals(
             str(exception),
@@ -694,7 +690,7 @@
 
     def test_operation_implicitly_exported_in_earliest_version_fails(self):
         # You can't implicitly define an operation for the earliest version.
-        exception = self._assertRaises(
+        exception = self.assertRaises(
             ValueError, generate_and_register_webservice_operations,
             None, IFieldImplicitOperationDefinition, [])
         self.assertEquals(
@@ -720,3 +716,66 @@
         self.assertEquals(
             adapter.__class__.__name__,
             'GET_IFieldExplicitOperationDefinition_explicitly_in_10_1_0')
+
+
+# Classes for TestSanityChecking
+
+class INotPublished(Interface):
+    pass
+
+
+class IReferencesUnpublishedObject(Interface):
+    export_as_webservice_entry()
+    field = exported(Reference(schema=INotPublished))
+
+
+class IPublishedTooLate(Interface):
+    export_as_webservice_entry(as_of='2.0')
+
+
+class IReferencesPublishedTooLate(Interface):
+    export_as_webservice_entry(as_of='1.0')
+    field = exported(Reference(schema=IPublishedTooLate))
+
+
+class IPublishedEarly(Interface):
+    export_as_webservice_entry(as_of='1.0')
+
+
+class IPublishedLate(Interface):
+    export_as_webservice_entry(as_of='2.0')
+    field = exported(Reference(schema=IPublishedEarly))
+
+
+class TestSanityChecking(TestCaseWithWebServiceFixtures):
+    """Test lazr.restful's sanity checking upon web service registration."""
+
+    def test_reference_to_unpublished_object_fails(self):
+        exception = self.assertRaises(
+            ConfigurationExecutionError, register_test_module, 'testmod',
+            INotPublished, IReferencesUnpublishedObject)
+        self.assertTrue(
+            ("In version 1.0, IReferencesUnpublishedObjectEntry_1_0.field "
+             "is a Reference to INotPublished, but version 1.0 of the web "
+             "service does not publish INotPublished as an entry. "
+             "(It may not be published at all.)") in str(exception))
+
+    def test_reference_to_object_published_later_fails(self):
+        exception = self.assertRaises(
+            ConfigurationExecutionError, register_test_module, 'testmod',
+            IPublishedTooLate, IReferencesPublishedTooLate)
+        self.assertTrue(
+            ("In version 1.0, IReferencesPublishedTooLateEntry_1_0.field is "
+             "a Reference to IPublishedTooLate, but version 1.0 of the web "
+             "service does not publish IPublishedTooLate as an entry. (It "
+             "may not be published at all.)") in str(exception))
+
+    def test_reference_to_object_published_earlier_succeeds(self):
+        # It's okay for an object defined in 2.0 to reference an
+        # object first defined in 1.0, so long as the referenced
+        # object is also present in 2.0.
+
+        # We'll call this test a success if it doesn't raise an exception.
+        module = register_test_module(
+            'testmod', IPublishedEarly, IPublishedLate)
+