Clean up old/ugly code -- and get sqlalchemy v2 working

colandr/apis/api_v1.py

@@ -1,7 +1,24 @@
+# ruff: noqa: E402
 from flask_restx import Api
 
+
+api_v1 = Api(
+    version="1.0",
+    prefix="/api",  # NOTE: not using "/api/v1" here to maintain backwards compatibility
+    doc="/docs",
+    default_mediatype="application/json",
+    title="colandr",
+    description="REST API powering the colandr app",
+    authorizations={
+        "api_key": {"type": "apiKey", "in": "header", "name": "Authorization"}
+        # NOTE: below style is for OpenAPI v3, which flask-restx doesn't yet support :/
+        # "access_token": {"type": "http", "scheme": "bearer", "bearerFormat": "JWT"},
+    },
+    security="api_key",
+)
+
+
 from .auth import ns as ns_auth
-from .errors import ns as ns_errors
 from .health import ns as ns_health
 from .resources.citation_imports import ns as ns_citation_imports
 from .resources.citation_screenings import ns as ns_citation_screenings
@@ -23,23 +40,7 @@
 from .swagger import ns as ns_swagger
 
 
-api_v1 = Api(
-    version="1.0",
-    prefix="/api",  # NOTE: not using "/api/v1" here to maintain backwards compatibility
-    doc="/docs",
-    default_mediatype="application/json",
-    title="colandr",
-    description="REST API powering the colandr app",
-    authorizations={
-        "api_key": {"type": "apiKey", "in": "header", "name": "Authorization"}
-        # NOTE: below style is for OpenAPI v3, which flask-restx doesn't yet support :/
-        # "access_token": {"type": "http", "scheme": "bearer", "bearerFormat": "JWT"},
-    },
-    security="api_key",
-)
-
 api_v1.add_namespace(ns_auth)
-api_v1.add_namespace(ns_errors)
 api_v1.add_namespace(ns_health)
 api_v1.add_namespace(ns_swagger)
 api_v1.add_namespace(ns_users)

colandr/apis/errors.py

@@ -1,65 +1,59 @@
+from typing import Optional
+
 import webargs.core
 import webargs.flaskparser
-from flask import current_app, jsonify
-from flask_restx import Namespace
+from flask import current_app
 from werkzeug import exceptions
+from werkzeug.http import HTTP_STATUS_CODES
 
-
-ns = Namespace("errors")
+from .api_v1 import api_v1
 
 
-@ns.errorhandler
-def default_error(error):
-    message = f"an unhandled exception occurred: {error}"
+@api_v1.errorhandler(exceptions.HTTPException)
+def http_error(error):
+    message = error.description
     current_app.logger.exception(message)
-    response = jsonify({"message": message})
-    response.status_code = getattr(error, "code", 500)
-    return response
+    return _make_error_response(error.code, message)
 
 
-@ns.errorhandler(exceptions.BadRequest)
+@api_v1.errorhandler(exceptions.BadRequest)
 def bad_request_error(error):
     message = str(error)
     current_app.logger.error(message)
-    response = jsonify({"message": message})
-    response.status_code = 400
-    return response
+    return _make_error_response(400, message)
 
 
-@ns.errorhandler(exceptions.Unauthorized)
+@api_v1.errorhandler(exceptions.Unauthorized)
 def unauthorized_error(error):
     message = str(error)
     current_app.logger.error(message)
-    response = jsonify({"message": message})
-    response.status_code = 401
-    return response
+    return _make_error_response(401, message)
 
 
-@ns.errorhandler(exceptions.Forbidden)
+@api_v1.errorhandler(exceptions.Forbidden)
 def forbidden_error(error):
     message = str(error)
     current_app.logger.error(message)
-    response = jsonify({"message": message})
-    response.status_code = 403
-    return response
+    return _make_error_response(403, message)
 
 
-@ns.errorhandler(exceptions.NotFound)
+@api_v1.errorhandler(exceptions.NotFound)
 def not_found_error(error):
     message = str(error)
     current_app.logger.error(message)
-    response = jsonify({"message": message})
-    response.status_code = 404
-    return response
+    return _make_error_response(404, message)
 
 
-@ns.errorhandler(exceptions.InternalServerError)
+@api_v1.errorhandler(exceptions.InternalServerError)
 def internal_server_error(error):
     message = str(error)
+    current_app.logger.exception(message)
+    return _make_error_response(500, message)
+
+
+def db_integrity_error(message):
     current_app.logger.error(message)
-    response = jsonify({"message": message})
-    response.status_code = 500
-    return response
+    return _make_error_response(422, message)
 
 
 @webargs.flaskparser.parser.error_handler
@@ -72,14 +66,23 @@ def validation_error(error, req, schema, *, error_status_code, error_headers):
     See: webargs/issues/181
     """
     status_code = error_status_code or webargs.core.DEFAULT_VALIDATION_STATUS
-    webargs.flaskparser.abort(
-        status_code,
-        exc=error,
-        messages=error.messages,
-    )
-
-
-def db_integrity_error(message):
-    response = jsonify({"message": message})
-    response.status_code = 422
-    return response
+    webargs.flaskparser.abort(status_code, exc=error, messages=error.messages)
+
+
+def _make_error_response(
+    status_code: int, message: Optional[str] = None
+) -> tuple[dict[str, str], int]:
+    data = {"error": HTTP_STATUS_CODES.get(status_code, "Unknown error")}
+    if message:
+        data["message"] = message
+    return (data, status_code)
+
+
+# TODO: do we need this?
+# @api_v1.errorhandler
+# def default_error(error):
+#     message = f"an unhandled exception occurred: {error}"
+#     current_app.logger.exception(message)
+#     response = jsonify({"message": message})
+#     response.status_code = getattr(error, "code", 500)
+#     return response

colandr/apis/resources/citation_screenings.py

@@ -12,11 +12,11 @@
 from ...extensions import db
 from ...lib import constants
 from ...models import Citation, CitationScreening, Fulltext, Review, Study, User
+from ...utils import assign_status
 from .. import auth
 from ..errors import bad_request_error, forbidden_error, not_found_error
 from ..schemas import ScreeningSchema
 from ..swagger import screening_model
-from ..utils import assign_status
 
 
 ns = Namespace(

colandr/apis/resources/data_extractions.py

@@ -158,9 +158,7 @@ def put(self, args, id):
             is None
         ):
             return forbidden_error(
-                "{} forbidden to modify extracted data for this study".format(
-                    current_user
-                )
+                f"{current_user} forbidden to modify extracted data for this study"
             )
         study = db.session.get(Study, id)
         if study.data_extraction_status == "finished":
@@ -219,9 +217,7 @@ def put(self, args, id):
                 validated_value = sanitizers.sanitize_type(value, type_)
                 if validated_value is None:
                     return bad_request_error(
-                        'value "{}" for label "{}" invalid; must be {}'.format(
-                            value, label, field_type
-                        )
+                        f"value '{value} for label '{label}' invalid; must be {field_type}"
                     )
             elif field_type == "select_one":
                 if value not in allowed_values:

colandr/apis/resources/fulltext_screenings.py

@@ -11,10 +11,10 @@
 from ...extensions import db
 from ...lib import constants
 from ...models import DataExtraction, Fulltext, FulltextScreening, Review, Study, User
+from ...utils import assign_status
 from ..errors import bad_request_error, forbidden_error, not_found_error
 from ..schemas import ScreeningSchema
 from ..swagger import screening_model
-from ..utils import assign_status
 
 
 ns = Namespace(
@@ -63,7 +63,7 @@ def get(self, id, fields):
         # check current user authorization
         fulltext = db.session.get(Fulltext, id)
         if not fulltext:
-            return not_found_error("<Fulltext(id={})> not found".format(id))
+            return not_found_error(f"<Fulltext(id={id})> not found")
         if (
             current_user.is_admin is False
             and current_user.review_user_assoc.filter_by(
@@ -72,9 +72,7 @@ def get(self, id, fields):
             is None
         ):
             return forbidden_error(
-                "{} forbidden to get fulltext screenings for this review".format(
-                    current_user
-                )
+                f"{current_user} forbidden to get fulltext screenings for this review"
             )
         return ScreeningSchema(many=True, only=fields).dump(fulltext.screenings)
 
@@ -100,24 +98,20 @@ def delete(self, id):
         # check current user authorization
         fulltext = db.session.get(Fulltext, id)
         if not fulltext:
-            return not_found_error("<Fulltext(id={})> not found".format(id))
+            return not_found_error(f"<Fulltext(id={id})> not found")
         if (
             current_user.review_user_assoc.filter_by(
                 review_id=fulltext.review_id
             ).one_or_none()
             is None
         ):
             return forbidden_error(
-                "{} forbidden to delete fulltext screening for this review".format(
-                    current_user
-                )
+                f"{current_user} forbidden to delete fulltext screening for this review"
             )
         screening = fulltext.screenings.filter_by(user_id=current_user.id).one_or_none()
         if not screening:
             return forbidden_error(
-                "{} has not screened {}, so nothing to delete".format(
-                    current_user, fulltext
-                )
+                f"{current_user} has not screened {fulltext}, so nothing to delete"
             )
         db.session.delete(screening)
         db.session.commit()
@@ -149,7 +143,7 @@ def post(self, args, id):
         # check current user authorization
         fulltext = db.session.get(Fulltext, id)
         if not fulltext:
-            return not_found_error("<Fulltext(id={})> not found".format(id))
+            return not_found_error(f"<Fulltext(id={id})> not found")
         if (
             current_user.is_admin is False
             and current_user.review_user_assoc.filter_by(
@@ -158,13 +152,11 @@ def post(self, args, id):
             is None
         ):
             return forbidden_error(
-                "{} forbidden to screen fulltexts for this review".format(current_user)
+                f"{current_user} forbidden to screen fulltexts for this review"
             )
         if fulltext.filename is None:
             return forbidden_error(
-                "user can't screen {} without first having uploaded its content".format(
-                    fulltext
-                )
+                f"user can't screen {fulltext} without first having uploaded its content"
             )
         # validate and add screening
         if args["status"] == "excluded" and not args["exclude_reasons"]:
@@ -182,9 +174,7 @@ def post(self, args, id):
             fulltext.review_id, user_id, id, args["status"], args["exclude_reasons"]
         )
         if fulltext.screenings.filter_by(user_id=current_user.id).one_or_none():
-            return forbidden_error(
-                "{} has already screened {}".format(current_user, fulltext)
-            )
+            return forbidden_error(f"{current_user} has already screened {fulltext}")
         fulltext.screenings.append(screening)
         db.session.commit()
         current_app.logger.info("inserted %s", screening)
@@ -220,7 +210,7 @@ def put(self, args, id):
         current_user = jwtext.get_current_user()
         fulltext = db.session.get(Fulltext, id)
         if not fulltext:
-            return not_found_error("<Fulltext(id={})> not found".format(id))
+            return not_found_error(f"<Fulltext(id={id})> not found")
         if current_user.is_admin is True and "user_id" in args:
             screening = fulltext.screenings.filter_by(
                 user_id=args["user_id"]
@@ -230,9 +220,7 @@ def put(self, args, id):
                 user_id=current_user.id
             ).one_or_none()
         if not screening:
-            return not_found_error(
-                "{} has not screened this fulltext".format(current_user)
-            )
+            return not_found_error(f"{current_user} has not screened this fulltext")
         if args["status"] == "excluded" and not args["exclude_reasons"]:
             return bad_request_error("screenings that exclude must provide a reason")
         for key, value in args.items():
@@ -310,9 +298,7 @@ def get(self, fulltext_id, user_id, review_id, status_counts):
             # check user authorization
             fulltext = db.session.get(Fulltext, fulltext_id)
             if not fulltext:
-                return not_found_error(
-                    "<Fulltext(id={})> not found".format(fulltext_id)
-                )
+                return not_found_error(f"<Fulltext(id={fulltext_id})> not found")
             if (
                 current_user.is_admin is False
                 and fulltext.review.review_user_assoc.filter_by(
@@ -321,30 +307,28 @@ def get(self, fulltext_id, user_id, review_id, status_counts):
                 is None
             ):
                 return forbidden_error(
-                    "{} forbidden to get screenings for {}".format(
-                        current_user, fulltext
-                    )
+                    f"{current_user} forbidden to get screenings for {fulltext}"
                 )
             query = query.filter_by(fulltext_id=fulltext_id)
         if user_id is not None:
             # check user authorization
             user = db.session.get(User, user_id)
             if not user:
-                return not_found_error("<User(id={})> not found".format(user_id))
+                return not_found_error(f"<User(id={user_id})> not found")
             if current_user.is_admin is False and not any(
                 user_id == user.id
                 for review in current_user.reviews
                 for user in review.users
             ):
                 return forbidden_error(
-                    "{} forbidden to get screenings for {}".format(current_user, user)
+                    f"{current_user} forbidden to get screenings for {user}"
                 )
             query = query.filter_by(user_id=user_id)
         if review_id is not None:
             # check user authorization
             review = db.session.get(Review, review_id)
             if not review:
-                return not_found_error("<Review(id={})> not found".format(review_id))
+                return not_found_error(f"<Review(id={review_id})> not found")
             if (
                 current_user.is_admin is False
                 and review.review_user_assoc.filter_by(
@@ -353,7 +337,7 @@ def get(self, fulltext_id, user_id, review_id, status_counts):
                 is None
             ):
                 return forbidden_error(
-                    "{} forbidden to get screenings for {}".format(current_user, review)
+                    f"{current_user} forbidden to get screenings for {review}"
                 )
             query = query.filter_by(review_id=review_id)
         if status_counts is True:
@@ -407,7 +391,7 @@ def post(self, args, review_id, user_id):
         # check current user authorization
         review = db.session.get(Review, review_id)
         if not review:
-            return not_found_error("<Review(id={})> not found".format(review_id))
+            return not_found_error(f"<Review(id={review_id})> not found")
         # bulk insert fulltext screenings
         screener_user_id = user_id or current_user.id
         screenings_to_insert = []