From 2e79681670f7b8e6843abf29d6e95ffda0c3147c Mon Sep 17 00:00:00 2001
From: Victor Stinner <vstinner@redhat.com>
Date: Thu, 25 Jun 2015 11:13:16 +0200
Subject: Fix sqlalchemy.ModelBase.__contains__() behaviour

Currently, sqlalchemy.ModelBase.__contains__() catches any exception,
and by doing that, it hides real bugs. For example, a Nova unit test
raises the following error, but __contains__() simply returns False:

    sqlalchemy.orm.exc.DetachedInstanceError: Parent instance
    <InstanceExtra at 0x7fea635bc5f8> is not bound to a Session;
    deferred load operation of attribute 'pci_requests' cannot proceed

On Python 3, hasattr() calls getattr(): it returns True if getattr()
succeeds, False if getattr() raises an AttributeError, or passes
through the exception, if getattr() failed for a different reason.

On Python 2, hasattr() also calls getattr(), but it catches *any*
exception.

This change replaces hasattr() with getattr(), and it only catches
AttributeError as Python 3, so passes through sqlalchemy exceptions.

Add an unit test to test the new behaviour.

Closes-Bug: #1469225

Change-Id: If9c3ccc03f1dc9746936b0b83ea132508491e577
---
 oslo_db/sqlalchemy/models.py            | 10 +++++++++-
 oslo_db/tests/sqlalchemy/test_models.py | 12 ++++++++++++
 2 files changed, 21 insertions(+), 1 deletion(-)

diff --git a/oslo_db/sqlalchemy/models.py b/oslo_db/sqlalchemy/models.py
index 4293c9d..0c4e0de 100644
--- a/oslo_db/sqlalchemy/models.py
+++ b/oslo_db/sqlalchemy/models.py
@@ -54,7 +54,15 @@ class ModelBase(six.Iterator):
         return getattr(self, key)
 
     def __contains__(self, key):
-        return hasattr(self, key)
+        # Don't use hasattr() because hasattr() catches any exception, not only
+        # AttributeError. We want to passthrough SQLAlchemy exceptions
+        # (ex: sqlalchemy.orm.exc.DetachedInstanceError).
+        try:
+            getattr(self, key)
+        except AttributeError:
+            return False
+        else:
+            return True
 
     def get(self, key, default=None):
         return getattr(self, key, default)
diff --git a/oslo_db/tests/sqlalchemy/test_models.py b/oslo_db/tests/sqlalchemy/test_models.py
index 525573a..279c5c3 100644
--- a/oslo_db/tests/sqlalchemy/test_models.py
+++ b/oslo_db/tests/sqlalchemy/test_models.py
@@ -70,6 +70,18 @@ class ModelBaseTest(test_base.DbTestCase):
 
         self.assertFalse('non-existent-key' in mb)
 
+    def test_modelbase_contains_exc(self):
+        class ErrorModel(models.ModelBase):
+            @property
+            def bug(self):
+                raise ValueError
+
+        model = ErrorModel()
+        model.update({'attr': 5})
+
+        self.assertTrue('attr' in model)
+        self.assertRaises(ValueError, lambda: 'bug' in model)
+
     def test_modelbase_items_iteritems(self):
         h = {'a': '1', 'b': '2'}
         expected = {
-- 
cgit v1.2.1