Merge pull request #469 from metabrainz/sqlalchemy-warnings

Upgrade SQLAlchemy and enable 2.0 warnings

critiquebrainz/data/utils.py

@@ -14,7 +14,7 @@
 
 
 def create_all():
-    db.run_sql_script(os.path.join(ADMIN_SQL_DIR, 'create_extensions.sql'))
+    db.run_sql_script_without_transaction(os.path.join(ADMIN_SQL_DIR, 'create_extensions.sql'))
     db.run_sql_script(os.path.join(ADMIN_SQL_DIR, 'create_types.sql'))
     db.run_sql_script(os.path.join(ADMIN_SQL_DIR, 'create_tables.sql'))
     db.run_sql_script(os.path.join(ADMIN_SQL_DIR, 'create_primary_keys.sql'))

critiquebrainz/db/__init__.py

@@ -1,4 +1,5 @@
-from sqlalchemy import create_engine
+from flask_debugtoolbar.panels import sqlalchemy
+from sqlalchemy import create_engine, text
 from sqlalchemy.pool import NullPool
 
 # This value must be incremented after schema changes on exported tables!
@@ -22,7 +23,22 @@ def init_db_engine(connect_str):
 
 
 def run_sql_script(sql_file_path):
-    with open(sql_file_path) as sql:
-        connection = engine.connect()
-        connection.execute(sql.read())
-        connection.close()
+    with open(sql_file_path) as sql, engine.begin() as connection:
+        connection.execute(text(sql.read()))
+
+
+def run_sql_script_without_transaction(sql_file_path):
+    with open(sql_file_path) as sql, engine.connect().execution_options(isolation_level="AUTOCOMMIT") as connection:
+        lines = sql.read().splitlines()
+        try:
+            for line in lines:
+                # TODO: Not a great way of removing comments. The alternative is to catch
+                #  the exception sqlalchemy.exc.ProgrammingError "can't execute an empty query"
+                if line and not line.startswith("--"):
+                    connection.execute(text(line))
+        except sqlalchemy.exc.ProgrammingError as e:
+            print("Error: {}".format(e))
+            return False
+        finally:
+            connection.close()
+        return True

critiquebrainz/db/avg_rating.py

@@ -42,11 +42,12 @@ def update(entity_id, entity_type):
 
     # Calculate average rating and update it
     sum, count = row[0], row[1]
+
     if count == 0:
         delete(entity_id, entity_type)
         return
     avg_rating = int(sum / count + 0.5)
-    with db.engine.connect() as connection:
+    with db.engine.begin() as connection:
         connection.execute(sqlalchemy.text("""
             INSERT INTO avg_rating(entity_id, entity_type, rating, count)
                  VALUES (:entity_id, :entity_type, :rating, :count)
@@ -70,7 +71,7 @@ def delete(entity_id, entity_type):
         entity_id (uuid): ID of the entity
         entity_type (str): Type of the entity
     """
-    with db.engine.connect() as connection:
+    with db.engine.begin() as connection:
         connection.execute(sqlalchemy.text("""
             DELETE
               FROM avg_rating
@@ -111,7 +112,7 @@ def get(entity_id, entity_type):
             "entity_type": entity_type
         })
 
-        avg_rating = result.fetchone()
+        avg_rating = result.mappings().first()
         if not avg_rating:
             raise db_exceptions.NoDataFoundException("""No rating for the entity with ID: {id} and Type: {type}""".
                                                      format(id=entity_id, type=entity_type))

critiquebrainz/db/comment.py

@@ -36,7 +36,7 @@ def create(*, user_id, text, review_id, is_draft=False):
     Returns:
         the newly created comment in dict form
     """
-    with db.engine.connect() as connection:
+    with db.engine.begin() as connection:
         result = connection.execute(sqlalchemy.text("""
             INSERT INTO comment (user_id, review_id, is_draft)
                  VALUES (:user_id, :review_id, :is_draft)
@@ -46,8 +46,8 @@ def create(*, user_id, text, review_id, is_draft=False):
                 'review_id': review_id,
                 'is_draft': is_draft,
                 })
-        comment_id = result.fetchone()['id']
-        db_comment_revision.create(comment_id, text)
+        comment_id = result.fetchone().id
+        db_comment_revision.create(connection, comment_id, text)
     return get_by_id(comment_id)
 
 
@@ -100,7 +100,7 @@ def get_by_id(comment_id):
                 'comment_id': comment_id,
                 })
 
-        comment = result.fetchone()
+        comment = result.mappings().first()
         if not comment:
             raise db_exceptions.NoDataFoundException('Can\'t find comment with ID: {id}'.format(id=comment_id))
 
@@ -199,7 +199,7 @@ def list_comments(*, review_id=None, user_id=None, limit=20, offset=0, inc_hidde
             OFFSET :offset
             """.format(where_clause=filterstr)), query_vals)
 
-        rows = [dict(row) for row in result.fetchall()]
+        rows = [dict(row) for row in result.mappings()]
         for row in rows:
             row['last_revision'] = {
                 'id': row.pop('last_revision_id'),
@@ -225,7 +225,7 @@ def delete(comment_id):
     Args:
         comment_id (uuid): the ID of the comment to be deleted.
     """
-    with db.engine.connect() as connection:
+    with db.engine.begin() as connection:
         connection.execute(sqlalchemy.text("""
             DELETE
               FROM comment
@@ -266,15 +266,15 @@ def update(comment_id, *, text=None, is_draft=None, is_hidden=None):
 
     setstr = ', '.join(updates)
     update_data['comment_id'] = comment_id
-    with db.engine.connect() as connection:
+    with db.engine.begin() as connection:
         connection.execute(sqlalchemy.text("""
                 UPDATE comment
                    SET {setstr}
                  WHERE id = :comment_id
             """.format(setstr=setstr)), update_data)
 
-    if text is not None:
-        db_comment_revision.create(comment_id, text)
+        if text is not None:
+            db_comment_revision.create(connection, comment_id, text)
 
 
 def count_comments(*, review_id=None, user_id=None, is_hidden=None, is_draft=None):
@@ -317,4 +317,4 @@ def count_comments(*, review_id=None, user_id=None, is_hidden=None, is_draft=Non
               FROM comment
             {where_condition}
             """.format(where_condition=filterstr)), filter_data)
-        return result.fetchone()[0]
+        return result.fetchone().count

critiquebrainz/db/comment_revision.py

@@ -21,7 +21,7 @@
 import critiquebrainz.db as db
 
 
-def create(comment_id, text):
+def create(conn, comment_id, text):
     """ Create a new revision for comment with specified ID.
 
     Args:
@@ -31,14 +31,13 @@ def create(comment_id, text):
     Returns:
         int: the ID of the new revision created.
     """
-    with db.engine.connect() as connection:
-        result = connection.execute(sqlalchemy.text("""
-            INSERT INTO comment_revision (comment_id, text)
-                 VALUES (:comment_id, :text)
-              RETURNING id
-            """), {
-                'comment_id': comment_id,
-                'text': text,
-                })
+    result = conn.execute(sqlalchemy.text("""
+        INSERT INTO comment_revision (comment_id, text)
+             VALUES (:comment_id, :text)
+          RETURNING id
+        """), {
+            'comment_id': comment_id,
+            'text': text,
+            })
 
-        return result.fetchone()['id']
+    return result.fetchone().id

critiquebrainz/db/license.py

@@ -23,7 +23,7 @@ def create(*, id, full_name, info_url=None):
         "full_name": full_name,
         "info_url": info_url,
     }
-    with db.engine.connect() as connection:
+    with db.engine.begin() as connection:
         connection.execute(sqlalchemy.text("""
             INSERT INTO license(id, full_name, info_url)
                  VALUES (:id, :full_name, :info_url)
@@ -37,7 +37,7 @@ def delete(*, id):
     Args:
         id (str): ID of the license.
     """
-    with db.engine.connect() as connection:
+    with db.engine.begin() as connection:
         connection.execute(sqlalchemy.text("""
             DELETE
               FROM license
@@ -59,7 +59,7 @@ def get_licenses_list(connection):
           FROM license
     """)
     results = connection.execute(query)
-    return [dict(row) for row in results.fetchall()]
+    return [dict(row) for row in results.mappings()]
 
 
 def list_licenses():

critiquebrainz/db/moderation_log.py

@@ -38,7 +38,7 @@ def create(*, admin_id, review_id=None, user_id=None,
         raise ValueError("No review ID or user ID specified.")
     if action not in AdminActions.get_all_actions():
         raise ValueError("Please specify a valid action.")
-    with db.engine.connect() as connection:
+    with db.engine.begin() as connection:
         connection.execute(sqlalchemy.text("""
             INSERT INTO moderation_log(admin_id, user_id, review_id, action, timestamp, reason)
                  VALUES (:admin_id, :user_id, :review_id, :action, :timestamp, :reason)
@@ -81,7 +81,7 @@ def list_logs(*, admin_id=None, limit=None, offset=None):
     """.format(where_clause=where_clause))
     with db.engine.connect() as connection:
         result = connection.execute(query, filter_data)
-        count = result.fetchone()[0]
+        count = result.fetchone().count
 
     filter_data["limit"] = limit
     filter_data["offset"] = offset
@@ -142,7 +142,7 @@ def list_logs(*, admin_id=None, limit=None, offset=None):
     with db.engine.connect() as connection:
         result = connection.execute(query, filter_data)
         logs = []
-        for log in result.fetchall():
+        for log in result.mappings():
             log = dict(log)
             log["user"] = {
                 "id": log["user_id"],

critiquebrainz/db/oauth_client.py

@@ -29,7 +29,7 @@ def create(*, user_id, name, desc, website, redirect_uri):
             "redirect_uri": redirect_uri,
         }
     """
-    with db.engine.connect() as connection:
+    with db.engine.begin() as connection:
         row = connection.execute(sqlalchemy.text("""
             INSERT INTO oauth_client (client_id, client_secret, redirect_uri,
                         user_id, name, "desc", website)
@@ -45,7 +45,7 @@ def create(*, user_id, name, desc, website, redirect_uri):
             "website": website,
             "desc": desc,
         })
-        return dict(row.fetchone())
+        return dict(row.mappings().first())
 
 
 def update(*, client_id, name=None, desc=None, website=None, redirect_uri=None):
@@ -83,7 +83,7 @@ def update(*, client_id, name=None, desc=None, website=None, redirect_uri=None):
     """.format(update_str=update_str))
 
     if update_str:
-        with db.engine.connect() as connection:
+        with db.engine.begin() as connection:
             connection.execute(query, update_data)
 
 
@@ -93,7 +93,7 @@ def delete(client_id):
     Args:
         client_id(str): ID of the OAuth Client.
     """
-    with db.engine.connect() as connection:
+    with db.engine.begin() as connection:
         connection.execute(sqlalchemy.text("""
             DELETE
               FROM oauth_client
@@ -134,7 +134,7 @@ def get_client(client_id):
         """), {
             "client_id": client_id,
         })
-        row = result.fetchone()
+        row = result.mappings().first()
         if not row:
             raise db_exceptions.NoDataFoundException("Can't find OAuth client with ID: {id}".format(id=client_id))
-    return dict(row)
+        return dict(row)

critiquebrainz/db/oauth_grant.py

@@ -35,13 +35,13 @@ def create(*, client_id, scopes, code, expires, redirect_uri, user_id):
         "user_id": user_id,
         "scopes": scopes,
     }
-    with db.engine.connect() as connection:
+    with db.engine.begin() as connection:
         result = connection.execute(sqlalchemy.text("""
             INSERT INTO oauth_grant(client_id, code, expires, redirect_uri, scopes, user_id)
                  VALUES (:client_id, :code, :expires, :redirect_uri, :scopes, :user_id)
               RETURNING id
         """), grant)
-        grant["id"] = result.fetchone()[0]
+        grant["id"] = result.first().id
     return grant
 
 
@@ -97,8 +97,8 @@ def list_grants(*, client_id=None, code=None, limit=1, offset=None):
              LIMIT :limit
             OFFSET :offset
         """.format(where_clause=filterstr)), filter_data)
-        rows = results.fetchall()
-    return [dict(row) for row in rows]
+        rows = results.mappings()
+        return [dict(row) for row in rows]
 
 
 def delete(*, client_id, code):
@@ -108,7 +108,7 @@ def delete(*, client_id, code):
         client_id (str): ID of the OAuth Client.
         code (str): The authorization code returned from authorization request.
     """
-    with db.engine.connect() as connection:
+    with db.engine.begin() as connection:
         connection.execute(sqlalchemy.text("""
             DELETE
               FROM oauth_grant
@@ -136,9 +136,9 @@ def get_scopes(grant_id):
         """), {
             "grant_id": grant_id,
         })
-        scopes = result.fetchone()
+        scopes = result.mappings().first()
     if not scopes:
         raise db_exceptions.NoDataFoundException("No grant exists with ID: {grant_id}".format(grant_id=grant_id))
-    if scopes[0] is None:
+    if scopes["scopes"] is None:
         return list()
-    return scopes[0].split()
+    return scopes["scopes"].split()

critiquebrainz/db/oauth_token.py

@@ -35,13 +35,13 @@ def create(*, client_id, scopes, access_token, refresh_token, expires, user_id):
         "user_id": user_id,
         "scopes": scopes,
     }
-    with db.engine.connect() as connection:
+    with db.engine.begin() as connection:
         result = connection.execute(sqlalchemy.text("""
             INSERT INTO oauth_token(client_id, access_token, refresh_token, expires, scopes, user_id)
                  VALUES (:client_id, :access_token, :refresh_token, :expires, :scopes, :user_id)
               RETURNING id
         """), token)
-        token["id"] = result.fetchone()[0]
+        token["id"] = result.first().id
     return token
 
 
@@ -107,8 +107,8 @@ def list_tokens(*, client_id=None, refresh_token=None, access_token=None, limit=
              LIMIT :limit
             OFFSET :offset
         """.format(where_clause=filterstr)), filter_data)
-        rows = results.fetchall()
-    return [dict(row) for row in rows]
+        rows = results.mappings()
+        return [dict(row) for row in rows]
 
 
 def delete(*, client_id=None, refresh_token=None, user_id=None):
@@ -128,7 +128,7 @@ def delete(*, client_id=None, refresh_token=None, user_id=None):
         filters.append("user_id = :user_id")
         filter_data["user_id"] = user_id
     filterstr = " AND ".join(filters)
-    with db.engine.connect() as connection:
+    with db.engine.begin() as connection:
         connection.execute(sqlalchemy.text("""
             DELETE
               FROM oauth_token