Simplify join to rbac_entries for subnets

The subnet table was joined to the rbac table via an association proxy
which required a bunch of hacking in the model_query setup to get it
to work right.

This patch simplifies it quiet a bit by just using a direct relationship
between the subnets and networkrbacs tables joined via the subnet's
networkid.

It also unrolls an API test that was really difficult to debug because
it was hard to tell which iteration it was on.

Change-Id: I4da85218158aae624835b97053da9fbb6fb154ef

neutron/db/common_db_mixin.py

@@ -129,9 +129,8 @@ class CommonDbMixin(object):
         query_filter = None
         if self.model_query_scope(context, model):
             if hasattr(model, 'rbac_entries'):
-                rbac_model, join_params = self._get_rbac_query_params(
-                    model)[:2]
-                query = query.outerjoin(*join_params)
+                query = query.outerjoin(model.rbac_entries)
+                rbac_model = model.rbac_entries.property.mapper.class_
                 query_filter = (
                     (model.tenant_id == context.tenant_id) |
                     ((rbac_model.action == 'access_as_shared') &
@@ -184,27 +183,6 @@ class CommonDbMixin(object):
         query = self._model_query(context, model)
         return query.filter(model.id == id).one()
 
-    @staticmethod
-    def _get_rbac_query_params(model):
-        """Return the parameters required to query an model's RBAC entries.
-
-        Returns a tuple of 3 containing:
-        1. the relevant RBAC model for a given model
-        2. the join parameters required to query the RBAC entries for the model
-        3. the ID column of the passed in model that matches the object_id
-           in the rbac entries.
-        """
-        try:
-            cls = model.rbac_entries.property.mapper.class_
-            return (cls, (cls, ), model.id)
-        except AttributeError:
-            # an association proxy is being used (e.g. subnets
-            # depends on network's rbac entries)
-            rbac_model = (model.rbac_entries.target_class.
-                          rbac_entries.property.mapper.class_)
-            return (rbac_model, model.rbac_entries.attr,
-                    model.rbac_entries.remote_attr.class_.id)
-
     def _apply_filters_to_query(self, query, model, filters, context=None):
         if filters:
             for key, value in six.iteritems(filters):
@@ -222,9 +200,8 @@ class CommonDbMixin(object):
                 elif key == 'shared' and hasattr(model, 'rbac_entries'):
                     # translate a filter on shared into a query against the
                     # object's rbac entries
-                    rbac, join_params, oid_col = self._get_rbac_query_params(
-                        model)
-                    query = query.outerjoin(*join_params, aliased=True)
+                    query = query.outerjoin(model.rbac_entries)
+                    rbac = model.rbac_entries.property.mapper.class_
                     matches = [rbac.target_tenant == '*']
                     if context:
                         matches.append(rbac.target_tenant == context.tenant_id)
@@ -240,6 +217,12 @@ class CommonDbMixin(object):
                         # because that will still give us a network shared to
                         # our tenant (or wildcard) if it's shared to another
                         # tenant.
+                        # This is the column joining the table to rbac via
+                        # the object_id. We can't just use model.id because
+                        # subnets join on network.id so we have to inspect the
+                        # relationship.
+                        join_cols = model.rbac_entries.property.local_columns
+                        oid_col = list(join_cols)[0]
                         is_shared = ~oid_col.in_(
                             query.session.query(rbac.object_id).
                             filter(is_shared)

neutron/db/models_v2.py

@@ -14,7 +14,6 @@
 #    under the License.
 
 import sqlalchemy as sa
-from sqlalchemy.ext.associationproxy import association_proxy
 from sqlalchemy import orm
 
 from neutron.api.v2 import attributes as attr
@@ -205,7 +204,12 @@ class Subnet(model_base.BASEV2, HasId, HasTenant):
                                   constants.DHCPV6_STATEFUL,
                                   constants.DHCPV6_STATELESS,
                                   name='ipv6_address_modes'), nullable=True)
-    rbac_entries = association_proxy('networks', 'rbac_entries')
+    # subnets don't have their own rbac_entries, they just inherit from
+    # the network rbac entries
+    rbac_entries = orm.relationship(
+        rbac_db_models.NetworkRBAC, lazy='joined',
+        foreign_keys='Subnet.network_id',
+        primaryjoin='Subnet.network_id==NetworkRBAC.object_id')
 
 
 class SubnetPoolPrefix(model_base.BASEV2):

neutron/tests/api/admin/test_shared_network_extension.py

@@ -347,16 +347,20 @@ class RBACSharedNetworksTest(base.BaseAdminNetworkTest):
     def test_filtering_works_with_rbac_records_present(self):
         resp = self._make_admin_net_and_subnet_shared_to_tenant_id(
             self.client.tenant_id)
-        net = resp['network']
-        sub = resp['subnet']
+        net = resp['network']['id']
+        sub = resp['subnet']['id']
         self.admin_client.create_rbac_policy(
-            object_type='network', object_id=net['id'],
+            object_type='network', object_id=net,
             action='access_as_shared', target_tenant='*')
-        for state, assertion in ((False, self.assertNotIn),
-                                 (True, self.assertIn)):
-            nets = [n['id'] for n in
-                    self.admin_client.list_networks(shared=state)['networks']]
-            assertion(net['id'], nets)
-            subs = [s['id'] for s in
-                    self.admin_client.list_subnets(shared=state)['subnets']]
-            assertion(sub['id'], subs)
+        self._assert_shared_object_id_listing_presence('subnets', False, sub)
+        self._assert_shared_object_id_listing_presence('subnets', True, sub)
+        self._assert_shared_object_id_listing_presence('networks', False, net)
+        self._assert_shared_object_id_listing_presence('networks', True, net)
+
+    def _assert_shared_object_id_listing_presence(self, resource, shared, oid):
+        lister = getattr(self.admin_client, 'list_%s' % resource)
+        objects = [o['id'] for o in lister(shared=shared)[resource]]
+        if shared:
+            self.assertIn(oid, objects)
+        else:
+            self.assertNotIn(oid, objects)