Do not call `to_dict` outside of a session context

Do not call `to_dict` outside of a session context as if to_dict
interacts with lazy-loaded relationships it can cause errors. For
the most part these are edge-cases and unlikely to happen.

A couple FIXMEs were added to restructure the calls to allow for
`to_dict` to be moved into a method that will act within a session
context.

Change-Id: I769c2cdea1b08a780093d27cdc70bce9f004017b

keystone/catalog/backends/sql.py

@@ -81,6 +81,14 @@ class Endpoint(sql.ModelBase, sql.DictBase):
                          server_default=sqlalchemy.sql.expression.true())
     extra = sql.Column(sql.JsonBlob())
 
+    @classmethod
+    def from_dict(cls, endpoint_dict):
+        """Override from_dict to set enabled if missing."""
+        new_dict = endpoint_dict.copy()
+        if new_dict.get('enabled') is None:
+            new_dict['enabled'] = True
+        return super(Endpoint, cls).from_dict(new_dict)
+
 
 @dependency.requires('catalog_api')
 class Catalog(base.CatalogDriverBase):
@@ -213,11 +221,11 @@ class Catalog(base.CatalogDriverBase):
             return ref.to_dict()
 
     # Endpoints
-    def create_endpoint(self, endpoint_id, endpoint_ref):
-        new_endpoint = Endpoint.from_dict(endpoint_ref)
+    def create_endpoint(self, endpoint_id, endpoint):
         with sql.session_for_write() as session:
-            session.add(new_endpoint)
-        return new_endpoint.to_dict()
+            endpoint_ref = Endpoint.from_dict(endpoint)
+            session.add(endpoint_ref)
+            return endpoint_ref.to_dict()
 
     def delete_endpoint(self, endpoint_id):
         with sql.session_for_write() as session:

keystone/identity/backends/sql.py

@@ -57,6 +57,7 @@ class Identity(base.IdentityDriverBase):
         with sql.session_for_read() as session:
             try:
                 user_ref = self._get_user(session, user_id)
+                user_dict = base.filter_user(user_ref.to_dict())
             except exception.UserNotFound:
                 raise AssertionError(_('Invalid user / password'))
         if self._is_account_locked(user_id, user_ref):
@@ -71,7 +72,7 @@ class Identity(base.IdentityDriverBase):
         # successful auth, reset failed count if present
         if user_ref.local_user.failed_auth_count:
             self._reset_failed_auth(user_id)
-        return base.filter_user(user_ref.to_dict())
+        return user_dict
 
     def _is_account_locked(self, user_id, user_ref):
         """Check if the user account is locked.

keystone/identity/shadow_backends/sql.py

@@ -42,6 +42,13 @@ class ShadowUsers(base.ShadowUsersDriverBase):
             return identity_base.filter_user(user_ref.to_dict())
 
     def get_federated_user(self, idp_id, protocol_id, unique_id):
+        # NOTE(notmorgan): Open a session here to ensure .to_dict is called
+        # within an active session context. This will prevent lazy-load
+        # relationship failure edge-cases
+        # FIXME(notmorgan): Eventually this should not call `to_dict` here and
+        # rely on something already in the session context to perform the
+        # `to_dict` call.
+        with sql.session_for_read():
             user_ref = self._get_federated_user(idp_id, protocol_id, unique_id)
             return identity_base.filter_user(user_ref.to_dict())
 

keystone/revoke/controllers.py

@@ -31,6 +31,12 @@ class RevokeController(controller.V3Controller):
             except ValueError:
                 raise exception.ValidationError(
                     message=_('invalid date format %s') % since)
+        # FIXME(notmorgan): The revocation events cannot have resource options
+        # added to them or lazy-loaded relationships as long as to_dict
+        # is called outside of an active session context. This API is unused
+        # and should be deprecated in the near future. Fix this before adding
+        # resource_options or any lazy-loaded relationships to the revocation
+        # events themselves.
         events = self.revoke_api.list_events(last_fetch=last_fetch)
         # Build the links by hand as the standard controller calls require ids
         response = {'events': [event.to_dict() for event in events],

keystone/tests/unit/common/test_sql_core.py

@@ -49,4 +49,6 @@ class TestModelDictMixin(unit.BaseTestCase):
         expected = {'id': utils.new_uuid(), 'text': utils.new_uuid()}
         m = TestModel(id=expected['id'], text=expected['text'])
         m.extra = 'this should not be in the dictionary'
+        # NOTE(notmorgan): This is currently explicitly harmless as this does
+        # not actually use SQL-Alchemy.
         self.assertEqual(expected, m.to_dict())

keystone/tests/unit/identity/test_backends.py

@@ -1407,6 +1407,8 @@ class ShadowUsersTests(object):
             federated_dict["protocol_id"],
             federated_dict["unique_id"],
             new_display_name)
+        # NOTE(notmorgan): to_dict not called here, an explicit session context
+        # is not needed.
         user_ref = self.shadow_users_api._get_federated_user(
             federated_dict["idp_id"],
             federated_dict["protocol_id"],

keystone/tests/unit/test_backend_id_mapping_sql.py

@@ -326,6 +326,10 @@ class SqlIDMapping(test_backend_sql.SqlTests):
         local_entity5['public_id'] = self.id_mapping_api.create_id_mapping(
             local_entity5)
 
+        # NOTE(notmorgan): Always call to_dict in an active session context to
+        # ensure that lazy-loaded relationships succeed. Edge cases could cause
+        # issues especially in attribute mappers.
+        with sql.session_for_read():
             # list mappings for domainA
             domain_a_mappings = self.id_mapping_api.get_domain_mapping_list(
                 self.domainA['id'])