Merge ~sylvain-pineau/checkbox-ng:fix-1898900 into checkbox-ng:master

[Sylvain Pineau]

resource: Return an empty string when trying to access non-existent resource attributes

Instead of raising AttributeError which makes the full expression line to fail even is some sub expressions could pass.

Fixes the linked bug by making an invalid getattr expression non fatal, i.e not raising an AttributeError, which makes the full resource expression to evaluate to False even if some sub parts could return true.

Example:

package.foo == 'bar' or snap.name == 'core'

foo does not exist and even is the core snap is present, the requirement will fail.

Proposed solution is to return an empty string when trying to access non existent attr to properly fail the sub-expression.

Tested with:
https://git.launchpad.net/plainbox-provider-checkbox/tree/units/usb/usb.pxu#n246

[Jonathan Cave (jocave)]

This seems logical to me.

I'm trying to think of situations where it might fall down, the only situation I can think of is if a job was expecting a resource to be present but empty i.e.

something.a_field == ''

i.e. if 'a_field' was missing this would evaluate to True. However, I'm not aware that any resource would do this and the requirement really makes little practical sense

review: Approve

[Sylvain Pineau (sylvain-pineau)]

Mute the warning about "Ignoring %s with missing template parameter"

review: Needs Resubmitting

[Jonathan Cave (jocave)]

Looks even better to me.

review: Approve

diff --git a/plainbox/impl/ctrl.py b/plainbox/impl/ctrl.py
index 801eccd..5facd64 100644
--- a/plainbox/impl/ctrl.py
+++ b/plainbox/impl/ctrl.py
@@ -317,7 +317,7 @@ class CheckBoxSessionStateController(ISessionStateController):
                     try:
                         check_result = new_unit.check()
                     except MissingParam as m:
-                        logger.warning(_("Ignoring %s with missing "
+                        logger.debug(_("Ignoring %s with missing "
                                          "template parameter %s"),
                                          new_unit._raw_data.get('id'),
                                          m.parameter)
diff --git a/plainbox/impl/resource.py b/plainbox/impl/resource.py
index 38f3d59..9b094bb 100644
--- a/plainbox/impl/resource.py
+++ b/plainbox/impl/resource.py
@@ -116,7 +116,7 @@ class Resource:
         if attr in data:
             return data[attr]
         else:
-            raise AttributeError(attr, "don't poke at %r" % attr)
+            return ''
 
     def __getattribute__(self, attr):
         if attr != "_data":
diff --git a/plainbox/impl/test_ctrl.py b/plainbox/impl/test_ctrl.py
index cfcbc84..ce026cc 100644
--- a/plainbox/impl/test_ctrl.py
+++ b/plainbox/impl/test_ctrl.py
@@ -341,7 +341,7 @@ class CheckBoxSessionStateControllerTests(TestCase):
         session_state = SessionState([template, job])
         self.ctrl.observe_result(session_state, job, result)
         # Ensure that a warning was logged
-        mock_logger.warning.assert_called_with(
+        mock_logger.debug.assert_called_with(
             "Ignoring %s with missing template parameter %s",
             "foo-{missing}", "missing")
 
diff --git a/plainbox/impl/test_resource.py b/plainbox/impl/test_resource.py
index 18a5275..178f3b8 100644
--- a/plainbox/impl/test_resource.py
+++ b/plainbox/impl/test_resource.py
@@ -78,7 +78,6 @@ class ResourceTests(TestCase):
 
     def test_private_data_is_somewhat_protected(self):
         res = Resource()
-        self.assertRaises(AttributeError, getattr, res, "_data")
         self.assertRaises(AttributeError, delattr, res, "_data")
         self.assertRaises(AttributeError, setattr, res, "_data", None)
 
@@ -91,7 +90,7 @@ class ResourceTests(TestCase):
 
     def test_getattr(self):
         res = Resource()
-        self.assertRaises(AttributeError, getattr, res, "attr")
+        self.assertEqual(getattr(res, 'attr'), '')
         res = Resource({'attr': 'value'})
         self.assertEqual(getattr(res, 'attr'), 'value')
 
@@ -120,8 +119,8 @@ class ResourceTests(TestCase):
         self.assertRaises(AttributeError, delattr, res, "attr")
         res = Resource({'attr': 'value'})
         del res.attr
-        self.assertRaises(AttributeError, getattr, res, "attr")
-        self.assertRaises(AttributeError, lambda res: res.attr, res)
+        self.assertEqual(getattr(res, 'attr'), '')
+        self.assertEqual(res.attr, '')
 
     def test_delitem(self):
         res = Resource()