Merge lp:~leonardr/lazr.restful/operation-sanity-check into lp:lazr.restful

[Leonard Richardson]

This branch beefs up our post-web-service-definition sanity check, and the tests for it. Both the code and the tests have also been heavily refactored to avoid massive repetition.

Previously, the sanity check only looked at fields that were References to unpublished entries. We had a wide variety of tests for what counts as an unpublished entry (never published, published and removed, published too late), but there are lots of places other than Reference fields where unpublished entries might show up:

1. A field might be a *collection* of unpublished entries.
2. A named operation might *return* an unpublished entry (or a collection of them).
3. A named operation might *accept* an unpublished entry (or a collection of them) as a parameter.

[Leonard Richardson (leonardr)]

Some background: in the currently released version of lazr.restful, an interface that's got export_as_webservice_entry() is published in *every single version* of the web service. Since then, I've landed code that lets you publish entries themselves on the same terms as you publish their fields and named operations. For instance, you can publish IFoo in 1.0 and get rid of it in 2.0, or you can start publishing IBar in 2.0 and then give it a new human-readable name in 3.0.

The problem (and the reason I didn't do this to start with) is that this creates potential data integrity problems. What if IFoo references IBar in 1.0, but IBar itself isn't published in 2.0? The web service definition for 1.0 is incoherent. The job of the sanity checker is to go through the web service definition and make sure version X contains no references to entries not published in version X.

[Leonard Richardson (leonardr)]

s/IBar itself isn't published in 2.0/IBar itself isn't published until 2.0/

[Abel Deuring (adeuring)]

The code looks good. Thanks for these really useful additional consistency checks!

review: Approve (code)

=== modified file 'src/lazr/restful/metazcml.py'
--- src/lazr/restful/metazcml.py	2011-03-18 12:13:42 +0000
+++ src/lazr/restful/metazcml.py	2011-03-18 15:37:24 +0000
@@ -33,6 +33,7 @@
 
 from lazr.restful.interfaces import (
     ICollection,
+    ICollectionField,
     IEntry,
     IReference,
     IResourceDELETEOperation,
@@ -358,26 +359,92 @@
         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.
+    # Check every Reference and CollectionField field in every IEntry
+    # interface, making sure that they only reference entries that are
+    # 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__))
+            referenced_interface, what = _extract_reference_type(field)
+            if referenced_interface is not None:
+                _assert_interface_registered_for_version(
+                    version, referenced_interface, available_registrations,
+                    ("%(interface)s.%(field)s is %(what)s"
+                     % dict(interface=interface.__name__,
+                            field=field.__name__, what=what)))
+
+    # For every version, check the return value and arguments to every
+    # named operation published in that version, making sure that
+    # there are no references to IEntries not published in that
+    # version.
+    for version, method in operations:
+        tags = method.getTaggedValue(LAZR_WEBSERVICE_EXPORTED)
+        return_type = tags.get('return_type')
+        referenced_interface, what = _extract_reference_type(return_type)
+        if referenced_interface is not None:
+            _assert_interface_registered_for_version(
+                version, referenced_interface, available_registrations,
+                "named operation %s returns %s" % (tags['as'], what))
+
+        if not 'params' in tags:
+            continue
+
+        for name, field in tags['params'].items():
+            referenced_interface, what = _extract_reference_type(field)
+            if referenced_interface is not None:
+                _assert_interface_registered_for_version(
+                    version, referenced_interface, available_registrations,
+                    "named operation %s accepts %s" % (tags['as'], what))
+
+def _extract_reference_type(field):
+    """Determine what kind of object the given field is a reference to.
+
+    This is a helper method used by the sanity checker.
+
+    :return: A 2-tuple (reference_type, human_readable): If
+    `field` is a reference to a scalar entry, human_readable is
+    the name of the interface. If `field` is a collection of
+    entries, human_readable is the string "a collection of
+    [interface name]". If `field` is not a reference to an
+    entry, both values are None.
+    """
+    if IReference.providedBy(field):
+        schema = field.schema
+        return (schema, schema.__name__)
+    elif ICollectionField.providedBy(field):
+        schema = field.value_type.schema
+        return (schema, "a collection of %s" % schema.__name__)
+    else:
+        return (None, None)
+
+
+def _assert_interface_registered_for_version(
+    version, interface, available_registrations, error_message_insert):
+    """A helper method for the sanity checker.
+
+    See if the given entry interface is published in the given version
+    (as determined by the contents of `available_registrations`), and
+    if it's not, raise an exception.
+
+    :param version: The version in which `interface` should be published.
+    :param interface: The interface that ought to be published.
+    :param available_registrations: A set of (version, interface) pairs
+        to check.
+    :param error_message_insert: A snippet of text explaining the significance
+        if `interface`, eg. "IMyInterface.myfield is IOughtToBePublished" or
+        "named operation my_named_operation returns IOughtToBePublished".
+    """
+
+    if version is None:
+        version = getUtility(IWebServiceConfiguration).active_versions[0]
+
+    to_check = VersionedObject(version, interface)
+    if to_check not in available_registrations:
+        raise ValueError(
+            "In version %(version)s, %(problem)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, problem=error_message_insert,
+                reference=interface.__name__))
 
 
 def generate_and_register_webservice_operations(

=== modified file 'src/lazr/restful/tests/test_declarations.py'
--- src/lazr/restful/tests/test_declarations.py	2011-03-18 12:43:45 +0000
+++ src/lazr/restful/tests/test_declarations.py	2011-03-18 15:37:24 +0000
@@ -1,3 +1,7 @@
+# Copyright 2008-2011 Canonical Ltd.  All rights reserved.
+
+"""Unit tests for the conversion of interfaces into a web service."""
+
 from zope.configuration.config import ConfigurationExecutionError
 from zope.component import (
     adapts,
@@ -37,8 +41,13 @@
     mutator_for,
     operation_for_version,
     operation_parameters,
-    )
-from lazr.restful.fields import Reference
+    operation_returns_collection_of,
+    operation_returns_entry,
+    )
+from lazr.restful.fields import (
+    CollectionField,
+    Reference,
+    )
 from lazr.restful.interfaces import (
     IEntry,
     IResourceGETOperation,
@@ -725,11 +734,54 @@
     pass
 
 
-class IReferencesUnpublishedObject(Interface):
+class IReferencesNotPublished(Interface):
     export_as_webservice_entry()
     field = exported(Reference(schema=INotPublished))
 
 
+class IReferencesCollectionOfNotPublished(Interface):
+    export_as_webservice_entry()
+    field = exported(
+        CollectionField(value_type=Reference(schema=INotPublished)))
+
+
+class IOperationReturnsNotPublished(Interface):
+    export_as_webservice_entry()
+
+    @operation_returns_entry(INotPublished)
+    @export_read_operation()
+    def get_impossible_object():
+        pass
+
+
+class IOperationReturnsNotPublishedCollection(Interface):
+    export_as_webservice_entry()
+
+    @operation_returns_collection_of(INotPublished)
+    @export_read_operation()
+    def get_impossible_objects():
+        pass
+
+
+class IOperationAcceptsNotPublished(Interface):
+    export_as_webservice_entry()
+
+    @operation_parameters(arg=Reference(schema=INotPublished))
+    @export_write_operation()
+    def use_impossible_object(arg):
+        pass
+
+
+class IOperationAcceptsCollectionOfNotPublished(Interface):
+    export_as_webservice_entry()
+
+    @operation_parameters(
+        arg=CollectionField(value_type=Reference(schema=INotPublished)))
+    @export_write_operation()
+    def use_impossible_objects(arg):
+        pass
+
+
 class IPublishedTooLate(Interface):
     export_as_webservice_entry(as_of='2.0')
 
@@ -761,41 +813,62 @@
 class TestSanityChecking(TestCaseWithWebServiceFixtures):
     """Test lazr.restful's sanity checking upon web service registration."""
 
+    def _test_fails_sanity_check(
+        self, expect_failure_in_version, expect_failure_for_reason,
+        expect_failure_due_to_interface, *classes):
+        """Verify that the given interfaces can't become a web service.
+
+        The given set of interfaces are expected to fail the sanity
+        check because they include an annotation that makes some
+        version of the web service reference an entry not defined in
+        that version (or at all).
+
+        :param expect_failure_in_version: Which version of the web
+           service will fail the sanity check.
+        :param expect_failure_for_reason: The reason that will be given
+           for failing the sanity check.
+        :param expect_failure_due_to_interface: The interface that will
+           cause the failure due to not being published in
+           `expect_failure_in_version`.
+        :param classes: The interfaces to attempt to publish as a web
+            service. `expect_failure_due_to_interface` will be added to
+            this list, so there's no need to specify it again.
+        """
+        exception = self.assertRaises(
+            ConfigurationExecutionError, register_test_module, 'testmod',
+            *(list(classes) + [expect_failure_due_to_interface]))
+        expected_message = (
+            "In version %(version)s, %(reason)s, but version %(version)s "
+            "of the web service does not publish %(interface)s as an entry. "
+             "(It may not be published at all.)" % dict(
+                version=expect_failure_in_version,
+                reason=expect_failure_for_reason,
+                interface=expect_failure_due_to_interface.__name__))
+        self.assertTrue(expected_message in str(exception))
+
     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))
+        self._test_fails_sanity_check(
+            '1.0',
+            ("IReferencesNotPublishedEntry_1_0.field is INotPublished"),
+            INotPublished, IReferencesNotPublished)
 
     def test_reference_to_object_published_later_fails(self):
-        exception = self.assertRaises(
-            ConfigurationExecutionError, register_test_module, 'testmod',
+        self._test_fails_sanity_check(
+            '1.0',
+            ("IReferencesPublishedTooLateEntry_1_0.field is "
+             "IPublishedTooLate"),
             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_and_then_removed_fails(self):
         # This setup is acceptable in version 1.0, but in version 2.0
         # IPublishedAndThenRemoved is gone. This puts
         # IReferencesPublishedAndThenRemoved in violation of the
         # sanity check in 2.0, even though it hasn't changed since 1.0.
-        exception = self.assertRaises(
-            ConfigurationExecutionError, register_test_module,
-            'testmod', IPublishedAndThenRemoved,
-            IReferencesPublishedAndThenRemoved)
-        self.assertTrue(
-            ("In version 2.0, "
-             "IReferencesPublishedAndThenRemovedEntry_2_0.field is a "
-             "Reference to IPublishedAndThenRemoved, but version 2.0 of the "
-             "web service does not publish IPublishedAndThenRemoved as an "
-             "entry. (It may not be published at all.)"))
+        self._test_fails_sanity_check(
+            '2.0',
+            ("IReferencesPublishedAndThenRemovedEntry_2_0.field is "
+             "IPublishedAndThenRemoved"),
+            IPublishedAndThenRemoved, IReferencesPublishedAndThenRemoved)
 
     def test_reference_to_object_published_earlier_succeeds(self):
         # It's okay for an object defined in 2.0 to reference an
@@ -806,3 +879,51 @@
         module = register_test_module(
             'testmod', IPublishedEarly, IPublishedLate)
 
+    # At this point we've tested all the ways entries might not be
+    # published in a given version: they might never be published,
+    # they might be published later, or they might be published
+    # earlier and then removed.
+
+    # Now we need to test all the places an unpublished entry might
+    # show up, not counting 'as the referent of a Reference field',
+    # which we've already tested.
+
+    def test_reference_to_collection_of_unpublished_objects_fails(self):
+        # An entry may define a field that's a scoped collection of
+        # unpublished entries.
+        self._test_fails_sanity_check(
+            '1.0',
+            ("IReferencesCollectionOfNotPublishedEntry_1_0.field is a "
+             "collection of INotPublished"),
+            INotPublished, IReferencesCollectionOfNotPublished)
+
+    def test_operation_returning_unpublished_object_fails(self):
+        # An operation may return an unpublished entry.
+        self._test_fails_sanity_check(
+            '1.0',
+            ("named operation get_impossible_object returns INotPublished"),
+            INotPublished, IOperationReturnsNotPublished)
+
+    def test_operation_returning_collection_of_unpublished_object_fails(self):
+        # An operation may return a collection of unpublished entries.
+        self._test_fails_sanity_check(
+            '1.0',
+            ("named operation get_impossible_objects returns a collection "
+             "of INotPublished"),
+            INotPublished, IOperationReturnsNotPublishedCollection)
+
+    def test_operation_taking_unpublished_argument_fails(self):
+        # An operation may take an unpublished entry as an argument.
+        self._test_fails_sanity_check(
+            '1.0',
+            ("named operation use_impossible_object accepts INotPublished"),
+            INotPublished, IOperationAcceptsNotPublished)
+
+    def test_operation_taking_unpublished_collection_argument_fails(self):
+        # An operation may take a collection of unpublished entries as
+        # an argument.
+        self._test_fails_sanity_check(
+            '1.0',
+            ("named operation use_impossible_objects accepts a collection "
+             "of INotPublished"),
+            INotPublished, IOperationAcceptsCollectionOfNotPublished)