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

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'),

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)).\

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',

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

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)'
     )