Drop limit columns

The service_id, region_id and resource_name columns in limit table
is useless now. These info can be got by 'registered_limit_id'
column from registered limit table. So there is no data migration
needed.

Since this is a silent change for end users, there is no need for
a release note as well.

Related-Bug: #1777892
Change-Id: Ibf4aa81dad7c5ebbd5599a55237cc5a658223432

5 files changed, 104 insertions, 90 deletions

diff --git a/keystone/common/sql/contract_repo/versions/063_contract_drop_limit_columns.py b/keystone/common/sql/contract_repo/versions/063_contract_drop_limit_columns.py
new file mode 100644
index 000000000..8858ba916
--- /dev/null
+++ b/keystone/common/sql/contract_repo/versions/063_contract_drop_limit_columns.py
@@ -0,0 +1,23 @@
+#    Licensed under the Apache License, Version 2.0 (the "License"); you may
+#    not use this file except in compliance with the License. You may obtain
+#    a copy of the License at
+#
+#         http://www.apache.org/licenses/LICENSE-2.0
+#
+#    Unless required by applicable law or agreed to in writing, software
+#    distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+#    WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+#    License for the specific language governing permissions and limitations
+#    under the License.
+
+import sqlalchemy as sql
+
+
+def upgrade(migrate_engine):
+    meta = sql.MetaData()
+    meta.bind = migrate_engine
+
+    limit_table = sql.Table('limit', meta, autoload=True)
+    limit_table.c.service_id.drop()
+    limit_table.c.region_id.drop()
+    limit_table.c.resource_name.drop()
diff --git a/keystone/common/sql/data_migration_repo/versions/063_migrate_drop_limit_columns.py b/keystone/common/sql/data_migration_repo/versions/063_migrate_drop_limit_columns.py
new file mode 100644
index 000000000..8aa15c1ef
--- /dev/null
+++ b/keystone/common/sql/data_migration_repo/versions/063_migrate_drop_limit_columns.py
@@ -0,0 +1,15 @@
+#    Licensed under the Apache License, Version 2.0 (the "License"); you may
+#    not use this file except in compliance with the License. You may obtain
+#    a copy of the License at
+#
+#         http://www.apache.org/licenses/LICENSE-2.0
+#
+#    Unless required by applicable law or agreed to in writing, software
+#    distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+#    WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+#    License for the specific language governing permissions and limitations
+#    under the License.
+
+
+def upgrade(migrate_engine):
+    pass
diff --git a/keystone/common/sql/expand_repo/versions/063_expand_drop_limit_columns.py b/keystone/common/sql/expand_repo/versions/063_expand_drop_limit_columns.py
new file mode 100644
index 000000000..8aa15c1ef
--- /dev/null
+++ b/keystone/common/sql/expand_repo/versions/063_expand_drop_limit_columns.py
@@ -0,0 +1,15 @@
+#    Licensed under the Apache License, Version 2.0 (the "License"); you may
+#    not use this file except in compliance with the License. You may obtain
+#    a copy of the License at
+#
+#         http://www.apache.org/licenses/LICENSE-2.0
+#
+#    Unless required by applicable law or agreed to in writing, software
+#    distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+#    WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+#    License for the specific language governing permissions and limitations
+#    under the License.
+
+
+def upgrade(migrate_engine):
+    pass
diff --git a/keystone/limit/backends/sql.py b/keystone/limit/backends/sql.py
index 3d32c8da7..b2ce6c13b 100644
--- a/keystone/limit/backends/sql.py
+++ b/keystone/limit/backends/sql.py
@@ -69,15 +69,10 @@ class LimitModel(sql.ModelBase, sql.ModelDictMixin):
         'registered_limit_id'
     ]
 
-    # TODO(wxy): Drop "service_id", "region_id" and "resource_name" columns
-    # in T release.
     internal_id = sql.Column(sql.Integer, primary_key=True, nullable=False)
     id = sql.Column(sql.String(length=64), nullable=False, unique=True)
     project_id = sql.Column(sql.String(64))
     domain_id = sql.Column(sql.String(64))
-    _service_id = sql.Column('service_id', sql.String(255))
-    _region_id = sql.Column('region_id', sql.String(64), nullable=True)
-    _resource_name = sql.Column('resource_name', sql.String(255))
     resource_limit = sql.Column(sql.Integer, nullable=False)
     description = sql.Column(sql.Text())
     registered_limit_id = sql.Column(sql.String(64),
@@ -89,29 +84,21 @@ class LimitModel(sql.ModelBase, sql.ModelDictMixin):
     def service_id(self):
         if self.registered_limit:
             return self.registered_limit.service_id
-        return self._service_id
-
-    @service_id.setter
-    def service_id(self, value):
-        self._service_id = value
+        return None
 
     @service_id.expression
     def service_id(self):
-        return LimitModel._service_id
+        return RegisteredLimitModel.service_id
 
     @hybrid_property
     def region_id(self):
         if self.registered_limit:
             return self.registered_limit.region_id
-        return self._region_id
-
-    @region_id.setter
-    def region_id(self, value):
-        self._region_id = value
+        return None
 
     @region_id.expression
     def region_id(self):
-        return LimitModel._region_id
+        return RegisteredLimitModel.region_id
 
     @hybrid_property
     def resource_name(self):
@@ -119,25 +106,16 @@ class LimitModel(sql.ModelBase, sql.ModelDictMixin):
             return self.registered_limit.resource_name
         return self._resource_name
 
-    @resource_name.setter
-    def resource_name(self, value):
-        self._resource_name = value
-
     @resource_name.expression
     def resource_name(self):
-        return LimitModel._resource_name
-
-    @classmethod
-    def from_dict(cls, limit):
-        obj = super(LimitModel, cls).from_dict(limit)
-        with sql.session_for_read() as session:
-            query = session.query(RegisteredLimitModel).filter_by(
-                id=obj.registered_limit_id)
-            obj.registered_limit = query.first()
-        return obj
+        return RegisteredLimitModel.resource_name
 
     def to_dict(self):
         ref = super(LimitModel, self).to_dict()
+        if self.registered_limit:
+            ref['service_id'] = self.registered_limit.service_id
+            ref['region_id'] = self.registered_limit.region_id
+            ref['resource_name'] = self.registered_limit.resource_name
         ref.pop('internal_id')
         ref.pop('registered_limit_id')
         return ref
@@ -151,16 +129,18 @@ class UnifiedLimit(base.UnifiedLimitDriverBase):
         # current reference between registered limit and limit. i.e. We should
         # ensure that there is no duplicate entry.
         hints = driver_hints.Hints()
-        hints.add_filter('service_id', unified_limit['service_id'])
-        hints.add_filter('resource_name', unified_limit['resource_name'])
-        hints.add_filter('region_id', unified_limit.get('region_id'))
         if is_registered_limit:
+            hints.add_filter('service_id', unified_limit['service_id'])
+            hints.add_filter('resource_name', unified_limit['resource_name'])
+            hints.add_filter('region_id', unified_limit.get('region_id'))
             with sql.session_for_read() as session:
                 query = session.query(RegisteredLimitModel)
                 unified_limits = sql.filter_limit_query(RegisteredLimitModel,
                                                         query,
                                                         hints).all()
         else:
+            hints.add_filter('registered_limit_id',
+                             unified_limit['registered_limit_id'])
             is_project_limit = (True if unified_limit.get('project_id')
                                 else False)
             if is_project_limit:
@@ -169,27 +149,8 @@ class UnifiedLimit(base.UnifiedLimitDriverBase):
                 hints.add_filter('domain_id', unified_limit['domain_id'])
             with sql.session_for_read() as session:
                 query = session.query(LimitModel)
-                old_unified_limits = sql.filter_limit_query(LimitModel,
-                                                            query,
-                                                            hints).all()
-                query = session.query(
-                    LimitModel).outerjoin(RegisteredLimitModel)
-                query = query.filter(
-                    RegisteredLimitModel.service_id ==
-                    unified_limit['service_id'],
-                    RegisteredLimitModel.region_id ==
-                    unified_limit.get('region_id'),
-                    RegisteredLimitModel.resource_name ==
-                    unified_limit['resource_name'])
-                if is_project_limit:
-                    query = query.filter(
-                        LimitModel.project_id == unified_limit['project_id'])
-                else:
-                    query = query.filter(
-                        LimitModel.domain_id == unified_limit['domain_id'])
-                new_unified_limits = query.all()
-
-                unified_limits = old_unified_limits + new_unified_limits
+                unified_limits = sql.filter_limit_query(LimitModel, query,
+                                                        hints).all()
 
         if unified_limits:
             msg = _('Duplicate entry')
@@ -296,12 +257,16 @@ class UnifiedLimit(base.UnifiedLimitDriverBase):
             with sql.session_for_write() as session:
                 new_limits = []
                 for limit in limits:
-                    self._check_unified_limit_unique(limit,
-                                                     is_registered_limit=False)
                     target = self._check_and_fill_registered_limit_id(limit)
+                    self._check_unified_limit_unique(target,
+                                                     is_registered_limit=False)
                     ref = LimitModel.from_dict(target)
                     session.add(ref)
-                    new_limits.append(ref.to_dict())
+                    new_limit = ref.to_dict()
+                    new_limit['service_id'] = limit['service_id']
+                    new_limit['region_id'] = limit.get('region_id')
+                    new_limit['resource_name'] = limit['resource_name']
+                    new_limits.append(new_limit)
                 return new_limits
         except db_exception.DBReferenceError:
             raise exception.NoLimitReference()
@@ -310,43 +275,18 @@ class UnifiedLimit(base.UnifiedLimitDriverBase):
     def update_limit(self, limit_id, limit):
         with sql.session_for_write() as session:
             ref = self._get_limit(session, limit_id)
-            old_dict = ref.to_dict()
-            old_dict.update(limit)
-            new_limit = LimitModel.from_dict(old_dict)
-            ref.resource_limit = new_limit.resource_limit
-            ref.description = new_limit.description
+            if limit.get('resource_limit'):
+                ref.resource_limit = limit['resource_limit']
+            if limit.get('description'):
+                ref.description = limit['description']
             return ref.to_dict()
 
     @driver_hints.truncated
     def list_limits(self, hints):
-        hint_copy = copy.deepcopy(hints)
-        new_format_data = []
         with sql.session_for_read() as session:
-            query = session.query(LimitModel)
-            limits = sql.filter_limit_query(LimitModel,
-                                            query,
-                                            hints)
-            old_format_data = [s.to_dict() for s in limits]
-            project_filter = hint_copy.get_exact_filter_by_name('project_id')
-            domain_filter = hint_copy.get_exact_filter_by_name('domain_id')
-            if hint_copy.filters and (not (project_filter or domain_filter)
-                                      or len(hint_copy.filters) > 1):
-                # If the hints contain "service_id", "region_id" or
-                # "resource_name", we should combine the registered_limit table
-                # first to fetch these information.
-                query_new = session.query(
-                    LimitModel).outerjoin(RegisteredLimitModel)
-                limits = sql.filter_limit_query(RegisteredLimitModel,
-                                                query_new,
-                                                hint_copy)
-                if project_filter:
-                    limits = limits.filter(
-                        LimitModel.project_id == project_filter['value'])
-                elif domain_filter:
-                    limits = limits.filter(
-                        LimitModel.domain_id == domain_filter['value'])
-                new_format_data = [s.to_dict() for s in limits]
-            return old_format_data + new_format_data
+            query = session.query(LimitModel).outerjoin(RegisteredLimitModel)
+            limits = sql.filter_limit_query(LimitModel, query, hints)
+            return [limit.to_dict() for limit in limits]
 
     def _get_limit(self, session, limit_id):
         query = session.query(LimitModel).filter_by(id=limit_id)
diff --git a/keystone/tests/unit/test_sql_upgrade.py b/keystone/tests/unit/test_sql_upgrade.py
index fe1f5b230..cfa65cf06 100644
--- a/keystone/tests/unit/test_sql_upgrade.py
+++ b/keystone/tests/unit/test_sql_upgrade.py
@@ -3366,6 +3366,27 @@ class FullMigration(SqlMigrateBase, unit.TestCase):
         self.assertEqual(trust['redelegation_count'],
                          upgraded_trust.redelegation_count)
 
+    def test_migration_063_drop_limit_columns(self):
+        self.expand(62)
+        self.migrate(62)
+        self.contract(62)
+
+        limit_table = 'limit'
+        self.assertTableColumns(
+            limit_table,
+            ['id', 'project_id', 'service_id', 'region_id', 'resource_name',
+             'resource_limit', 'description', 'internal_id',
+             'registered_limit_id', 'domain_id'])
+
+        self.expand(63)
+        self.migrate(63)
+        self.contract(63)
+
+        self.assertTableColumns(
+            limit_table,
+            ['id', 'project_id', 'resource_limit', 'description',
+             'internal_id', 'registered_limit_id', 'domain_id'])
+
 
 class MySQLOpportunisticFullMigration(FullMigration):
     FIXTURE = db_fixtures.MySQLOpportunisticFixture