Syncronize our SQLAlchemy type hints with db schema (#2197)

Make sure what what is defined in our SQLAlchemy schema is accuratly reflected in the type hints:
- If a `relationship` is not a collection, and its foreign key column is nullable make sure the annotation is Optional
  - There were some cases where this caused a large number of mypy failures, since we were widely assuming that these relationships were not optional. In these cases I queried all our production instances, and if the the foreign key was never null, I added a `nullable=False` to the foreign key column.
- Appropriately set the columns annotation to not Optional if:
  - `nullable=False`
  - `primary_key=True`
- Add `nullable=False` and appropriately set annotation for columns that have `default` set

alembic/versions/20241127_8dde64eab209_update_database_nullable_constraints.py

@@ -0,0 +1,326 @@
+"""Update database nullable constraints
+
+Revision ID: 8dde64eab209
+Revises: c3458e1ef9aa
+Create Date: 2024-11-27 19:04:42.562571+00:00
+
+"""
+
+import sqlalchemy as sa
+from alembic import op
+from sqlalchemy.dialects import postgresql
+
+# revision identifiers, used by Alembic.
+revision = "8dde64eab209"
+down_revision = "c3458e1ef9aa"
+branch_labels = None
+depends_on = None
+
+
+def upgrade() -> None:
+    # We add a nullable=False constraint to these columns because they previously had a
+    # non-null default value but still allowed null values. None of them actually contained
+    # null values in production, so setting the constraint ensures that nulls cannot be
+    # introduced in the future.
+    op.alter_column("annotations", "active", existing_type=sa.BOOLEAN(), nullable=False)
+    op.alter_column(
+        "collections", "marked_for_deletion", existing_type=sa.BOOLEAN(), nullable=False
+    )
+    op.alter_column(
+        "contributors",
+        "extra",
+        existing_type=postgresql.JSON(astext_type=sa.Text()),
+        nullable=False,
+    )
+    op.alter_column(
+        "customlists", "auto_update_enabled", existing_type=sa.BOOLEAN(), nullable=False
+    )
+    op.alter_column(
+        "customlists",
+        "auto_update_status",
+        existing_type=postgresql.ENUM(
+            "init", "updated", "repopulate", name="auto_update_status"
+        ),
+        nullable=False,
+    )
+    op.alter_column(
+        "datasources", "offers_licenses", existing_type=sa.BOOLEAN(), nullable=False
+    )
+    op.alter_column(
+        "datasources",
+        "extra",
+        existing_type=postgresql.JSON(astext_type=sa.Text()),
+        nullable=False,
+    )
+    op.alter_column(
+        "deliverymechanisms",
+        "default_client_can_fulfill",
+        existing_type=sa.BOOLEAN(),
+        nullable=False,
+    )
+    op.alter_column(
+        "editions",
+        "extra",
+        existing_type=postgresql.JSON(astext_type=sa.Text()),
+        nullable=False,
+    )
+    op.alter_column("equivalents", "votes", existing_type=sa.INTEGER(), nullable=False)
+    op.alter_column(
+        "equivalents", "enabled", existing_type=sa.BOOLEAN(), nullable=False
+    )
+    op.alter_column(
+        "libraries", "is_default", existing_type=sa.BOOLEAN(), nullable=False
+    )
+    op.alter_column(
+        "licensepools", "superceded", existing_type=sa.BOOLEAN(), nullable=False
+    )
+    op.alter_column(
+        "licensepools", "suppressed", existing_type=sa.BOOLEAN(), nullable=False
+    )
+    op.alter_column(
+        "measurements",
+        "weight",
+        existing_type=postgresql.DOUBLE_PRECISION(precision=53),
+        nullable=False,
+    )
+    op.alter_column(
+        "playtime_entries", "processed", existing_type=sa.BOOLEAN(), nullable=False
+    )
+    op.alter_column(
+        "playtime_summaries",
+        "total_seconds_played",
+        existing_type=sa.INTEGER(),
+        nullable=False,
+    )
+    op.alter_column(
+        "resources",
+        "voted_quality",
+        existing_type=postgresql.DOUBLE_PRECISION(precision=53),
+        nullable=False,
+    )
+    op.alter_column(
+        "resources", "votes_for_quality", existing_type=sa.INTEGER(), nullable=False
+    )
+    op.alter_column(
+        "resourcetransformations",
+        "settings",
+        existing_type=postgresql.JSON(astext_type=sa.Text()),
+        nullable=False,
+    )
+    op.alter_column("subjects", "locked", existing_type=sa.BOOLEAN(), nullable=False)
+    op.alter_column("subjects", "checked", existing_type=sa.BOOLEAN(), nullable=False)
+    op.alter_column(
+        "workgenres",
+        "affinity",
+        existing_type=postgresql.DOUBLE_PRECISION(precision=53),
+        nullable=False,
+    )
+    op.alter_column(
+        "works", "presentation_ready", existing_type=sa.BOOLEAN(), nullable=False
+    )
+
+    # These columns previously had type hints indicating they were not nullable, but the database
+    # schema allowed null values. We set the nullable=False constraint to match the type hint.
+    op.alter_column("lanes", "display_name", existing_type=sa.VARCHAR(), nullable=False)
+    op.alter_column(
+        "licensepools", "licenses_owned", existing_type=sa.INTEGER(), nullable=False
+    )
+    op.alter_column(
+        "licensepools", "licenses_available", existing_type=sa.INTEGER(), nullable=False
+    )
+    op.alter_column(
+        "licensepools", "licenses_reserved", existing_type=sa.INTEGER(), nullable=False
+    )
+    op.alter_column(
+        "licensepools",
+        "patrons_in_hold_queue",
+        existing_type=sa.INTEGER(),
+        nullable=False,
+    )
+
+    # These columns had foreign key constraints that allowed null values. After verifying that they
+    # are never null in production, we set the nullable=False constraint to ensure that SQLAlchemy
+    # relationships will always return an object, rather than None.
+    op.alter_column(
+        "editions", "data_source_id", existing_type=sa.INTEGER(), nullable=False
+    )
+    op.alter_column(
+        "editions", "primary_identifier_id", existing_type=sa.INTEGER(), nullable=False
+    )
+    op.alter_column(
+        "equivalents", "input_id", existing_type=sa.INTEGER(), nullable=False
+    )
+    op.alter_column(
+        "equivalentscoveragerecords",
+        "equivalency_id",
+        existing_type=sa.INTEGER(),
+        nullable=False,
+    )
+    op.alter_column("holds", "patron_id", existing_type=sa.INTEGER(), nullable=False)
+    op.alter_column(
+        "holds", "license_pool_id", existing_type=sa.INTEGER(), nullable=False
+    )
+    op.alter_column(
+        "licensepools", "data_source_id", existing_type=sa.INTEGER(), nullable=False
+    )
+    op.alter_column(
+        "licensepools", "identifier_id", existing_type=sa.INTEGER(), nullable=False
+    )
+    op.alter_column(
+        "licenses", "license_pool_id", existing_type=sa.INTEGER(), nullable=False
+    )
+    op.alter_column("loans", "patron_id", existing_type=sa.INTEGER(), nullable=False)
+    op.alter_column(
+        "loans", "license_pool_id", existing_type=sa.INTEGER(), nullable=False
+    )
+    op.alter_column(
+        "workgenres", "genre_id", existing_type=sa.INTEGER(), nullable=False
+    )
+    op.alter_column("workgenres", "work_id", existing_type=sa.INTEGER(), nullable=False)
+
+
+def downgrade() -> None:
+    op.alter_column(
+        "works", "presentation_ready", existing_type=sa.BOOLEAN(), nullable=True
+    )
+    op.alter_column(
+        "workgenres",
+        "affinity",
+        existing_type=postgresql.DOUBLE_PRECISION(precision=53),
+        nullable=True,
+    )
+    op.alter_column("workgenres", "work_id", existing_type=sa.INTEGER(), nullable=True)
+    op.alter_column("workgenres", "genre_id", existing_type=sa.INTEGER(), nullable=True)
+    op.alter_column("subjects", "checked", existing_type=sa.BOOLEAN(), nullable=True)
+    op.alter_column("subjects", "locked", existing_type=sa.BOOLEAN(), nullable=True)
+    op.alter_column(
+        "resourcetransformations",
+        "settings",
+        existing_type=postgresql.JSON(astext_type=sa.Text()),
+        nullable=True,
+    )
+    op.alter_column(
+        "resources", "votes_for_quality", existing_type=sa.INTEGER(), nullable=True
+    )
+    op.alter_column(
+        "resources",
+        "voted_quality",
+        existing_type=postgresql.DOUBLE_PRECISION(precision=53),
+        nullable=True,
+    )
+    op.alter_column(
+        "playtime_summaries",
+        "total_seconds_played",
+        existing_type=sa.INTEGER(),
+        nullable=True,
+    )
+    op.alter_column(
+        "playtime_entries", "processed", existing_type=sa.BOOLEAN(), nullable=True
+    )
+    op.alter_column(
+        "measurements",
+        "weight",
+        existing_type=postgresql.DOUBLE_PRECISION(precision=53),
+        nullable=True,
+    )
+    op.alter_column(
+        "loans", "license_pool_id", existing_type=sa.INTEGER(), nullable=True
+    )
+    op.alter_column("loans", "patron_id", existing_type=sa.INTEGER(), nullable=True)
+    op.alter_column(
+        "licenses", "license_pool_id", existing_type=sa.INTEGER(), nullable=True
+    )
+    op.alter_column(
+        "licensepools",
+        "patrons_in_hold_queue",
+        existing_type=sa.INTEGER(),
+        nullable=True,
+    )
+    op.alter_column(
+        "licensepools", "licenses_reserved", existing_type=sa.INTEGER(), nullable=True
+    )
+    op.alter_column(
+        "licensepools", "licenses_available", existing_type=sa.INTEGER(), nullable=True
+    )
+    op.alter_column(
+        "licensepools", "licenses_owned", existing_type=sa.INTEGER(), nullable=True
+    )
+    op.alter_column(
+        "licensepools", "suppressed", existing_type=sa.BOOLEAN(), nullable=True
+    )
+    op.alter_column(
+        "licensepools", "superceded", existing_type=sa.BOOLEAN(), nullable=True
+    )
+    op.alter_column(
+        "licensepools", "identifier_id", existing_type=sa.INTEGER(), nullable=True
+    )
+    op.alter_column(
+        "licensepools", "data_source_id", existing_type=sa.INTEGER(), nullable=True
+    )
+    op.alter_column(
+        "libraries", "is_default", existing_type=sa.BOOLEAN(), nullable=True
+    )
+    op.alter_column("lanes", "display_name", existing_type=sa.VARCHAR(), nullable=True)
+    op.alter_column(
+        "holds", "license_pool_id", existing_type=sa.INTEGER(), nullable=True
+    )
+    op.alter_column("holds", "patron_id", existing_type=sa.INTEGER(), nullable=True)
+    op.alter_column(
+        "equivalentscoveragerecords",
+        "equivalency_id",
+        existing_type=sa.INTEGER(),
+        nullable=True,
+    )
+    op.alter_column("equivalents", "enabled", existing_type=sa.BOOLEAN(), nullable=True)
+    op.alter_column("equivalents", "votes", existing_type=sa.INTEGER(), nullable=True)
+    op.alter_column(
+        "equivalents", "input_id", existing_type=sa.INTEGER(), nullable=True
+    )
+    op.alter_column(
+        "editions",
+        "extra",
+        existing_type=postgresql.JSON(astext_type=sa.Text()),
+        nullable=True,
+    )
+    op.alter_column(
+        "editions", "primary_identifier_id", existing_type=sa.INTEGER(), nullable=True
+    )
+    op.alter_column(
+        "editions", "data_source_id", existing_type=sa.INTEGER(), nullable=True
+    )
+    op.alter_column(
+        "deliverymechanisms",
+        "default_client_can_fulfill",
+        existing_type=sa.BOOLEAN(),
+        nullable=True,
+    )
+    op.alter_column(
+        "datasources",
+        "extra",
+        existing_type=postgresql.JSON(astext_type=sa.Text()),
+        nullable=True,
+    )
+    op.alter_column(
+        "datasources", "offers_licenses", existing_type=sa.BOOLEAN(), nullable=True
+    )
+    op.alter_column(
+        "customlists",
+        "auto_update_status",
+        existing_type=postgresql.ENUM(
+            "init", "updated", "repopulate", name="auto_update_status"
+        ),
+        nullable=True,
+    )
+    op.alter_column(
+        "customlists", "auto_update_enabled", existing_type=sa.BOOLEAN(), nullable=True
+    )
+    op.alter_column(
+        "contributors",
+        "extra",
+        existing_type=postgresql.JSON(astext_type=sa.Text()),
+        nullable=True,
+    )
+    op.alter_column(
+        "collections", "marked_for_deletion", existing_type=sa.BOOLEAN(), nullable=True
+    )
+    op.alter_column("annotations", "active", existing_type=sa.BOOLEAN(), nullable=True)

src/palace/manager/api/admin/controller/individual_admin_settings.py

@@ -418,7 +418,10 @@ def handle_password(self, password, admin: Admin, is_new, settingUp):
                     # libraries then they should not be able to change the password
                     for role in admin.roles:
                         if not user.is_library_manager(role.library):
-                            message = f"User is part of '{role.library.name}', you are not authorized to change their password."
+                            library_name = (
+                                role.library.name if role.library else "unknown"
+                            )
+                            message = f"User is part of '{library_name}', you are not authorized to change their password."
                             break
                     else:
                         can_change_pw = True

src/palace/manager/api/admin/controller/integration_settings.py

@@ -98,15 +98,6 @@ def configured_service_info(
         """This is the default implementation for getting details about a configured integration.
          It can be overridden by implementations that need to add additional information to the
         service info dict that gets returned to the admin UI."""
-
-        if service.goal is None:
-            # We should never get here, since we only query for services with a goal, and goal
-            # is a required field, but for mypy and safety, we check for it anyway.
-            self.log.warning(
-                f"IntegrationConfiguration {service.name}({service.id}) has no goal set. Skipping."
-            )
-            return None
-
         if service.protocol is None or service.protocol not in self.registry:
             self.log.warning(
                 f"Unknown protocol: {service.protocol} for goal {self.registry.goal}"

src/palace/manager/api/admin/controller/patron_auth_services.py

@@ -1,6 +1,6 @@
 from __future__ import annotations
 
-from typing import Any, cast
+from typing import Any
 
 import flask
 from flask import Response
@@ -193,7 +193,4 @@ def get_library_configuration(
         if not (library_configurations := integration.library_configurations):
             return None
         # We sort by library id to ensure that the result is predictable.
-        # We cast the library id to `int`, since mypy doesn't understand the relationship.
-        return sorted(
-            library_configurations, key=lambda config: cast(int, config.library_id)
-        )[0]
+        return sorted(library_configurations, key=lambda config: config.library_id)[0]

src/palace/manager/api/admin/dashboard_stats.py

@@ -122,7 +122,7 @@ def _authorized_collections(
         self,
         admin: Admin,
         authorized_libraries: list[Library],
-    ) -> tuple[list[_Collection], dict[str | None, list[_Collection]], bool]:
+    ) -> tuple[list[_Collection], dict[str, list[_Collection]], bool]:
         authorized_collections: set[_Collection] = set()
         authorized_collections_by_library = {}