From 96e049783b94841582f191856c03e62176d4164f Mon Sep 17 00:00:00 2001 From: Valeriy Ponomaryov Date: Fri, 3 Apr 2015 19:02:37 +0300 Subject: [PATCH] Fix fields 'deleted' in various DB models for PostgreSQL compatibility Our migrations set field 'deleted' as Boolean for table 'share_type_extra_specs', but model 'ShareTypeExtraSpecs' treats it as Integer. Same for several other models. It works using MySQL as far it uses 'tinyint' storing `1` or `0` there. So, in case of MySQL it is always Integer. But PostgreSQL fails to write any records there because it distinguishes Boolean from Integer. So, fix DB migrations, DB methods and models to make mentioned field compatible for PostreSQL backend too. Also, reuse func 'model_query' from oslo_db to remove errors with queries that contain aggregate functions. Change-Id: I0ca00ce86adad893ac786b10324a934d372e2a04 Closes-Bug: #1440089 --- ...621e5a_change_volume_type_to_share_type.py | 11 ++- .../59eb64046740_add_required_extra_spec.py | 4 +- .../ef0c02b4366_add_share_type_projects.py | 3 +- manila/db/sqlalchemy/api.py | 80 +++++++++---------- manila/db/sqlalchemy/models.py | 6 +- 5 files changed, 52 insertions(+), 52 deletions(-) diff --git a/manila/db/migrations/alembic/versions/38e632621e5a_change_volume_type_to_share_type.py b/manila/db/migrations/alembic/versions/38e632621e5a_change_volume_type_to_share_type.py index f0bef270..0d0c8fb5 100644 --- a/manila/db/migrations/alembic/versions/38e632621e5a_change_volume_type_to_share_type.py +++ b/manila/db/migrations/alembic/versions/38e632621e5a_change_volume_type_to_share_type.py @@ -29,6 +29,7 @@ from alembic import op from manila.i18n import _LI from manila.openstack.common import log as logging +from oslo_utils import strutils import sqlalchemy as sa from sqlalchemy.sql import table @@ -54,7 +55,7 @@ def upgrade(): sa.Column('created_at', sa.DateTime), sa.Column('updated_at', sa.DateTime), sa.Column('deleted_at', sa.DateTime), - sa.Column('deleted', sa.Boolean), + sa.Column('deleted', sa.Integer), sa.Column('id', sa.Integer, primary_key=True, nullable=False), sa.Column('share_type_id', sa.String(length=36), sa.ForeignKey('share_types.id', name="st_id_fk"), @@ -115,7 +116,7 @@ def _copy_records(destination_table, up_migration=True): sa.Column('created_at', sa.DateTime), sa.Column('updated_at', sa.DateTime), sa.Column('deleted_at', sa.DateTime), - sa.Column('deleted', sa.Boolean), + sa.Column('deleted', sa.Integer if up_migration else sa.Boolean), sa.Column('id', sa.Integer, primary_key=True, nullable=False), sa.Column(data_from[0] + '_type_id', sa.String(length=36)), sa.Column(data_from[1] + 'key', sa.String(length=255)), @@ -123,11 +124,15 @@ def _copy_records(destination_table, up_migration=True): extra_specs = [] for es in op.get_bind().execute(from_table.select()): + if up_migration: + deleted = strutils.int_from_bool_as_string(es.deleted) + else: + deleted = strutils.bool_from_string(es.deleted) extra_specs.append({ 'created_at': es.created_at, 'updated_at': es.updated_at, 'deleted_at': es.deleted_at, - 'deleted': es.deleted, + 'deleted': deleted, 'id': es.id, data_to[0] + '_type_id': getattr(es, data_from[0] + '_type_id'), data_to[1] + 'key': getattr(es, data_from[1] + 'key'), diff --git a/manila/db/migrations/alembic/versions/59eb64046740_add_required_extra_spec.py b/manila/db/migrations/alembic/versions/59eb64046740_add_required_extra_spec.py index 672921eb..a87e5c2b 100644 --- a/manila/db/migrations/alembic/versions/59eb64046740_add_required_extra_spec.py +++ b/manila/db/migrations/alembic/versions/59eb64046740_add_required_extra_spec.py @@ -46,12 +46,14 @@ def upgrade(): sa.Column('deleted', sa.Integer), sa.Column('id', sa.Integer)) + # NOTE(vponomaryov): field 'deleted' is integer here. existing_required_extra_specs = session.query(es_table).\ filter(es_table.c.spec_key == 'driver_handles_share_servers').\ - filter(es_table.c.deleted.in_(('0', False))).\ + filter(es_table.c.deleted == 0).\ all() exclude_st_ids = [es.share_type_id for es in existing_required_extra_specs] + # NOTE(vponomaryov): field 'deleted' is string here. share_types = session.query(st_table).\ filter(st_table.c.deleted.in_(('0', 'False', ))).\ filter(st_table.c.id.notin_(exclude_st_ids)).\ diff --git a/manila/db/migrations/alembic/versions/ef0c02b4366_add_share_type_projects.py b/manila/db/migrations/alembic/versions/ef0c02b4366_add_share_type_projects.py index 9167a417..85480e20 100644 --- a/manila/db/migrations/alembic/versions/ef0c02b4366_add_share_type_projects.py +++ b/manila/db/migrations/alembic/versions/ef0c02b4366_add_share_type_projects.py @@ -55,8 +55,7 @@ def upgrade(): sql.Column('share_type_id', sql.String(36), sql.ForeignKey('share_types.id', name="stp_id_fk")), sql.Column('project_id', sql.String(length=255)), - sql.Column('deleted', sql.Boolean(create_constraint=True, - name=None)), + sql.Column('deleted', sql.Integer), sql.UniqueConstraint('share_type_id', 'project_id', 'deleted', name="stp_project_id_uc"), mysql_engine='InnoDB', diff --git a/manila/db/sqlalchemy/api.py b/manila/db/sqlalchemy/api.py index 0d74e44d..19b2f741 100644 --- a/manila/db/sqlalchemy/api.py +++ b/manila/db/sqlalchemy/api.py @@ -27,6 +27,7 @@ from oslo_config import cfg from oslo_db import exception as db_exception from oslo_db import options as db_options from oslo_db.sqlalchemy import session +from oslo_db.sqlalchemy import utils as db_utils from oslo_log import log from oslo_utils import timeutils import six @@ -178,6 +179,7 @@ def model_query(context, model, *args, **kwargs): """Query helper that accounts for context's `read_deleted` field. :param context: context to query under + :param model: model to query. Must be a subclass of ModelBase. :param session: if present, the session to use :param read_deleted: if present, overrides context's read_deleted field. :param project_only: if present and context is user-type, then restrict @@ -186,24 +188,17 @@ def model_query(context, model, *args, **kwargs): session = kwargs.get('session') or get_session() read_deleted = kwargs.get('read_deleted') or context.read_deleted project_only = kwargs.get('project_only') + kwargs = dict() - query = session.query(model, *args) + if project_only and not context.is_admin: + kwargs['project_id'] = context.project_id + if read_deleted in ('no', 'n', False): + kwargs['deleted'] = False + elif read_deleted in ('yes', 'y', True): + kwargs['deleted'] = True - default_deleted_value = str(model.__mapper__.c.deleted.default.arg) - if read_deleted == 'no': - query = query.filter(model.deleted == default_deleted_value) - elif read_deleted == 'yes': - pass # omit the filter to include deleted and active - elif read_deleted == 'only': - query = query.filter(model.deleted != default_deleted_value) - else: - raise Exception(_("Unrecognized read_deleted value '%s'") - % read_deleted) - - if project_only and is_user_context(context): - query = query.filter_by(project_id=context.project_id) - - return query + return db_utils.model_query( + model=model, session=session, args=args, **kwargs) def exact_filter(query, model, filters, legal_keys): @@ -993,7 +988,7 @@ def reservation_commit(context, reservations, project_id=None, user_id=None): if reservation.delta >= 0: usage.reserved -= reservation.delta usage.in_use += reservation.delta - reservation_query.update({'deleted': True, + reservation_query.update({'deleted': 1, 'deleted_at': timeutils.utcnow(), 'updated_at': literal_column('updated_at')}, synchronize_session=False) @@ -1010,7 +1005,7 @@ def reservation_rollback(context, reservations, project_id=None, user_id=None): usage = usages[reservation.resource] if reservation.delta >= 0: usage.reserved -= reservation.delta - reservation_query.update({'deleted': True, + reservation_query.update({'deleted': 1, 'deleted_at': timeutils.utcnow(), 'updated_at': literal_column('updated_at')}, synchronize_session=False) @@ -1024,7 +1019,7 @@ def quota_destroy_all_by_project_and_user(context, project_id, user_id): read_deleted="no").\ filter_by(project_id=project_id).\ filter_by(user_id=user_id).\ - update({'deleted': True, + update({'deleted': 1, 'deleted_at': timeutils.utcnow(), 'updated_at': literal_column('updated_at')}, synchronize_session=False) @@ -1033,7 +1028,7 @@ def quota_destroy_all_by_project_and_user(context, project_id, user_id): session=session, read_deleted="no").\ filter_by(project_id=project_id).\ filter_by(user_id=user_id).\ - update({'deleted': True, + update({'deleted': 1, 'deleted_at': timeutils.utcnow(), 'updated_at': literal_column('updated_at')}, synchronize_session=False) @@ -1042,7 +1037,7 @@ def quota_destroy_all_by_project_and_user(context, project_id, user_id): session=session, read_deleted="no").\ filter_by(project_id=project_id).\ filter_by(user_id=user_id).\ - update({'deleted': True, + update({'deleted': 1, 'deleted_at': timeutils.utcnow(), 'updated_at': literal_column('updated_at')}, synchronize_session=False) @@ -1055,7 +1050,7 @@ def quota_destroy_all_by_project(context, project_id): model_query(context, models.Quota, session=session, read_deleted="no").\ filter_by(project_id=project_id).\ - update({'deleted': True, + update({'deleted': 1, 'deleted_at': timeutils.utcnow(), 'updated_at': literal_column('updated_at')}, synchronize_session=False) @@ -1063,7 +1058,7 @@ def quota_destroy_all_by_project(context, project_id): model_query(context, models.ProjectUserQuota, session=session, read_deleted="no").\ filter_by(project_id=project_id).\ - update({'deleted': True, + update({'deleted': 1, 'deleted_at': timeutils.utcnow(), 'updated_at': literal_column('updated_at')}, synchronize_session=False) @@ -1071,7 +1066,7 @@ def quota_destroy_all_by_project(context, project_id): model_query(context, models.QuotaUsage, session=session, read_deleted="no").\ filter_by(project_id=project_id).\ - update({'deleted': True, + update({'deleted': 1, 'deleted_at': timeutils.utcnow(), 'updated_at': literal_column('updated_at')}, synchronize_session=False) @@ -1079,7 +1074,7 @@ def quota_destroy_all_by_project(context, project_id): model_query(context, models.Reservation, session=session, read_deleted="no").\ filter_by(project_id=project_id).\ - update({'deleted': True, + update({'deleted': 1, 'deleted_at': timeutils.utcnow(), 'updated_at': literal_column('updated_at')}, synchronize_session=False) @@ -1099,7 +1094,7 @@ def reservation_expire(context): reservation.usage.reserved -= reservation.delta session.add(reservation.usage) - reservation_query.update({'deleted': True, + reservation_query.update({'deleted': 1, 'deleted_at': timeutils.utcnow(), 'updated_at': literal_column('updated_at')}, synchronize_session=False) @@ -1157,7 +1152,7 @@ def share_data_get_for_project(context, project_id, user_id, session=None): else: result = query.first() - return (result[1] or 0, result[2] or 0) + return (result[0] or 0, result[1] or 0) @require_context @@ -1318,7 +1313,7 @@ def share_delete(context, share_id): 'status': 'deleted'}) session.query(models.ShareMetadata).\ filter_by(share_id=share_id).\ - update({'deleted': True, + update({'deleted': 1, 'deleted_at': timeutils.utcnow(), 'updated_at': literal_column('updated_at')}) session.query(models.ShareExportLocations).\ @@ -1383,7 +1378,7 @@ def share_access_delete(context, access_id): with session.begin(): session.query(models.ShareAccessMapping).\ filter_by(id=access_id).\ - update({'deleted': True, + update({'deleted': access_id, 'deleted_at': timeutils.utcnow(), 'updated_at': literal_column('updated_at'), 'state': models.ShareAccessMapping.STATE_DELETED}) @@ -1424,15 +1419,14 @@ def snapshot_data_get_for_project(context, project_id, user_id, session=None): func.sum(models.ShareSnapshot.size), read_deleted="no", session=session).\ - filter_by(project_id=project_id).\ - options(joinedload('share')) + filter_by(project_id=project_id) if user_id: result = query.filter_by(user_id=user_id).first() else: result = query.first() - return (result[1] or 0, result[2] or 0) + return (result[0] or 0, result[1] or 0) @require_context @@ -1442,7 +1436,7 @@ def share_snapshot_destroy(context, snapshot_id): session.query(models.ShareSnapshot).\ filter_by(id=snapshot_id).\ update({'status': 'deleted', - 'deleted': True, + 'deleted': snapshot_id, 'deleted_at': timeutils.utcnow(), 'updated_at': literal_column('updated_at')}) @@ -1550,7 +1544,6 @@ def share_snapshot_data_get_for_project(context, project_id, session=None): read_deleted="no", session=session).\ filter_by(project_id=project_id).\ - options(joinedload('share')).\ first() # NOTE(vish): convert None to 0 @@ -1582,7 +1575,7 @@ def share_metadata_get(context, share_id): def share_metadata_delete(context, share_id, key): _share_metadata_get_query(context, share_id).\ filter_by(key=key).\ - update({'deleted': True, + update({'deleted': 1, 'deleted_at': timeutils.utcnow(), 'updated_at': literal_column('updated_at')}) @@ -1628,7 +1621,7 @@ def _share_metadata_update(context, share_id, metadata, delete, session=None): meta_ref = _share_metadata_get_item(context, share_id, meta_key, session=session) - meta_ref.update({'deleted': True}) + meta_ref.update({'deleted': 1}) meta_ref.save(session=session) meta_ref = None @@ -1726,7 +1719,7 @@ def share_export_locations_update(context, share_id, export_locations, delete): updated_at = indexed_update_time[location['path']] location.update({ 'updated_at': updated_at, - 'deleted': False, + 'deleted': 0, }) location.save(session=session) @@ -1742,7 +1735,7 @@ def share_export_locations_update(context, share_id, export_locations, delete): 'path': path, 'share_id': share_id, 'updated_at': indexed_update_time[path], - 'deleted': False, + 'deleted': 0, }) location_ref.save(session=session) @@ -1893,7 +1886,7 @@ def share_network_get_all_by_security_service(context, security_service_id): join(models.ShareNetworkSecurityServiceAssociation, models.ShareNetwork.id == models.ShareNetworkSecurityServiceAssociation.share_network_id).\ - filter_by(security_service_id=security_service_id, deleted=False).\ + filter_by(security_service_id=security_service_id, deleted=0).\ options(joinedload('share_servers')).all() @@ -2232,7 +2225,8 @@ def share_type_get_all(context, inactive=False, filters=None): if filters['is_public'] and context.project_id is not None: projects_attr = getattr(models. ShareTypes, 'projects') the_filter.extend([ - projects_attr.any(project_id=context.project_id, deleted=False) + projects_attr.any( + project_id=context.project_id, deleted=0) ]) if len(the_filter) > 1: query = query.filter(or_(*the_filter)) @@ -2327,7 +2321,7 @@ def share_type_destroy(context, id): raise exception.ShareTypeInUse(share_type_id=id) model_query(context, models.ShareTypeExtraSpecs, session=session).\ filter_by(share_type_id=id).update( - {'deleted': True, + {'deleted': 1, 'deleted_at': timeutils.utcnow(), 'updated_at': literal_column('updated_at')}) model_query(context, models.ShareTypes, session=session).\ @@ -2427,7 +2421,7 @@ def share_type_extra_specs_delete(context, share_type_id, key): _share_type_extra_specs_get_item(context, share_type_id, key, session) _share_type_extra_specs_query(context, share_type_id, session).\ filter_by(key=key).\ - update({'deleted': True, + update({'deleted': 1, 'deleted_at': timeutils.utcnow(), 'updated_at': literal_column('updated_at')}) @@ -2460,7 +2454,7 @@ def share_type_extra_specs_update_or_create(context, share_type_id, specs): spec_ref = models.ShareTypeExtraSpecs() spec_ref.update({"key": key, "value": value, "share_type_id": share_type_id, - "deleted": False}) + "deleted": 0}) spec_ref.save(session=session) return specs diff --git a/manila/db/sqlalchemy/models.py b/manila/db/sqlalchemy/models.py index 694ac135..a6a1dff2 100644 --- a/manila/db/sqlalchemy/models.py +++ b/manila/db/sqlalchemy/models.py @@ -194,7 +194,7 @@ class Share(BASE, ManilaBase): lazy='immediate', primaryjoin='and_(' 'Share.id == ShareExportLocations.share_id, ' - 'ShareExportLocations.deleted == False)') + 'ShareExportLocations.deleted == 0)') share_network_id = Column(String(36), ForeignKey('share_networks.id'), nullable=True) share_type_id = Column(String(36), ForeignKey('share_types.id'), @@ -247,7 +247,7 @@ class ShareTypeProjects(BASE, ManilaBase): foreign_keys=share_type_id, primaryjoin='and_(' 'ShareTypeProjects.share_type_id == ShareTypes.id,' - 'ShareTypeProjects.deleted == False)') + 'ShareTypeProjects.deleted == 0)') class ShareTypeExtraSpecs(BASE, ManilaBase): @@ -264,7 +264,7 @@ class ShareTypeExtraSpecs(BASE, ManilaBase): foreign_keys=share_type_id, primaryjoin='and_(' 'ShareTypeExtraSpecs.share_type_id == ShareTypes.id,' - 'ShareTypeExtraSpecs.deleted == False)' + 'ShareTypeExtraSpecs.deleted == 0)' )