launchpad-reviewers team mailing list archive

[Gavin Panella <gavin.panella@xxxxxxxxxxxxx>]

Gavin Panella has proposed merging lp:~allenap/maas/pserv-xmlrpc-cobblerclient into lp:maas.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~allenap/maas/pserv-xmlrpc-cobblerclient/+merge/91630

This ensures that CobblerObject.get_values() and get_all_values() only ever return attributes mentioned in cls.known_attributes. This way we help avoid getting dependent on Cobbler's internal data without being fully aware of what we're doing.
-- 
https://code.launchpad.net/~allenap/maas/pserv-xmlrpc-cobblerclient/+merge/91630
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~allenap/maas/pserv-xmlrpc-cobblerclient into lp:maas.

=== modified file 'src/provisioningserver/cobblerclient.py'
--- src/provisioningserver/cobblerclient.py	2012-02-03 12:20:34 +0000
+++ src/provisioningserver/cobblerclient.py	2012-02-06 10:43:50 +0000
@@ -325,6 +325,14 @@
         returnValue([cls(session, name) for name in result])
 
     @classmethod
+    def _trim_attributes(cls, attributes):
+        """Return a dict containing only keys from `known_attributes`."""
+        return {
+            name: value for name, value in attributes.iteritems()
+            if name in cls.known_attributes
+            }
+
+    @classmethod
     @inlineCallbacks
     def get_all_values(cls, session):
         """Load the attributes for all objects of this type.
@@ -333,8 +341,9 @@
             to dicts containing their respective attributes.
         """
         method = cls._name_method("get_%s", plural=True)
-        result = yield session.call(method)
-        returnValue(dict((obj['name'], obj) for obj in result))
+        results = yield session.call(method)
+        results = (cls._trim_attributes(result) for result in results)
+        returnValue(dict((obj['name'], obj) for obj in results))
 
     def get_values(self):
         """Load the object's attributes as a dict.
@@ -343,6 +352,7 @@
             attribute names and values.
         """
         d = self.session.call(self._name_method("get_%s"), self.name)
+        d.addCallback(self._trim_attributes)
         return d
 
     @classmethod
@@ -361,14 +371,14 @@
         else:
             attributes['name'] = name
         missing_attributes = (
-            set(cls.required_attributes) - set(attributes.keys()))
+            set(cls.required_attributes).difference(attributes))
         assert len(missing_attributes) == 0, (
             "Required attributes for %s missing: %s"
             % (cls.object_type, missing_attributes))
 
         args = dict(
             (cls._normalize_attribute(key), value)
-            for key, value in attributes.items())
+            for key, value in attributes.iteritems())
 
         # Overwrite any existing object of the same name.  Unfortunately
         # this parameter goes into the "attributes," and seems to be

=== modified file 'src/provisioningserver/tests/test_cobblersession.py'
--- src/provisioningserver/tests/test_cobblersession.py	2012-02-03 10:58:38 +0000
+++ src/provisioningserver/tests/test_cobblersession.py	2012-02-06 10:43:50 +0000
@@ -375,9 +375,11 @@
         return assert_fails_with(d, defer.CancelledError)
 
 
-class CobblerObject(TestCase):
+class TestCobblerObject(TestCase):
     """Tests for the `CobblerObject` classes."""
 
+    run_tests_with = AsynchronousDeferredRunTest
+
     def test_name_method_inserts_type_name(self):
         self.assertEqual(
             'foo_system_bar',
@@ -395,3 +397,90 @@
         with ExpectedException(AssertionError):
             yield cobblerclient.CobblerSystem.new(
                 session, 'incomplete_system', {})
+
+    @inlineCallbacks
+    def test_get_values_returns_only_known_attributes(self):
+        session = make_recording_session()
+        # Create a new CobblerDistro. The True return value means the faked
+        # call to xapi_object_edit was successful.
+        session.proxy.set_return_values([True])
+        distro = yield cobblerclient.CobblerDistro.new(
+            session, name="fred", attributes={
+                "initrd": "an_initrd", "kernel": "a_kernel"})
+        # Fake that Cobbler holds the following attributes about the distro
+        # just created.
+        values_stored = {
+            "clobber": True,
+            "initrd": "an_initrd",
+            "kernel": "a_kernel",
+            "likes": u"cabbage",
+            "name": u"fred",
+            }
+        session.proxy.set_return_values([values_stored])
+        # However, CobblerObject.get_values() only returns attributes that are
+        # in known_attributes.
+        values_expected = {
+            "initrd": "an_initrd",
+            "kernel": "a_kernel",
+            "name": u"fred",
+            }
+        values_observed = yield distro.get_values()
+        self.assertEqual(values_expected, values_observed)
+
+    @inlineCallbacks
+    def test_get_all_values_returns_only_known_attributes(self):
+        session = make_recording_session()
+        # Create some new CobblerDistros. The True return values mean the
+        # faked calls to xapi_object_edit were successful.
+        session.proxy.set_return_values([True, True, True])
+        yield cobblerclient.CobblerDistro.new(
+            session, name="alice", attributes={
+                "initrd": "an_initrd", "kernel": "a_kernel"})
+        yield cobblerclient.CobblerDistro.new(
+            session, name="bob", attributes={
+                "initrd": "an_initrd", "kernel": "a_kernel"})
+        yield cobblerclient.CobblerDistro.new(
+            session, name="carol", attributes={
+                "initrd": "an_initrd", "kernel": "a_kernel"})
+        # Fake that Cobbler holds the following attributes about the distros
+        # just created.
+        values_stored = [
+            {"clobber": True,
+             "initrd": "an_initrd",
+             "kernel": "a_kernel",
+             "likes": u"cabbage",
+             "name": u"alice"},
+            {"clobber": True,
+             "initrd": "an_initrd",
+             "kernel": "a_kernel",
+             "eats": u"fish",
+             "name": u"bob"},
+            {"clobber": True,
+             "initrd": "an_initrd",
+             "kernel": "a_kernel",
+             "buys": u"too much",
+             "name": u"carol"},
+            ]
+        session.proxy.set_return_values([values_stored])
+        # However, CobblerObject.get_all_values() only returns attributes that
+        # are in known_attributes.
+        values_expected = {
+            "alice": {
+                "initrd": "an_initrd",
+                "kernel": "a_kernel",
+                "name": u"alice",
+                },
+            "bob": {
+                "initrd": "an_initrd",
+                "kernel": "a_kernel",
+                "name": u"bob",
+                },
+            "carol": {
+                "initrd": "an_initrd",
+                "kernel": "a_kernel",
+                "name": u"carol",
+                },
+            }
+        values_observed = yield (
+            cobblerclient.CobblerDistro.get_all_values(session))
+        self.assertEqual(values_expected, values_observed)