models: add table lock and translate old db usage (bug 1886854) (#45)

- add table lock functionality
- translate old db queries and ORM usage
- fix few incorrect imports
- add one_or_none method to base model
- remove unneeded tests
- remove db fixture

Author: zzzeid

src/lando/api/legacy/api/diff_warnings.py

@@ -15,7 +15,6 @@
 
 from lando.api.legacy.decorators import require_phabricator_api_key
 from lando.main.models.revision import DiffWarning, DiffWarningStatus
-from lando.api.legacy.storage import db
 
 logger = logging.getLogger(__name__)
 
@@ -41,15 +40,14 @@ def post(data: dict):
             type="https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/400",
         )
     warning = DiffWarning(**data)
-    db.session.add(warning)
-    db.session.commit()
+    warning.save()
     return warning.serialize(), 201
 
 
 @require_phabricator_api_key(provide_client=False)
 def delete(pk: str):
     """Archive a `DiffWarning` based on provided pk."""
-    warning = DiffWarning.query.get(pk)
+    warning = DiffWarning.objects.get(pk=pk)
     if not warning:
         return problem(
             400,
@@ -58,17 +56,17 @@ def delete(pk: str):
             type="https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/400",
         )
     warning.status = DiffWarningStatus.ARCHIVED
-    db.session.commit()
+    warning.save()
     return warning.serialize(), 200
 
 
 @require_phabricator_api_key(provide_client=False)
 def get(revision_id: str, diff_id: str, group: str):
     """Return a list of active revision diff warnings, if any."""
-    warnings = DiffWarning.query.filter(
-        DiffWarning.revision_id == revision_id,
-        DiffWarning.diff_id == diff_id,
-        DiffWarning.status == DiffWarningStatus.ACTIVE,
-        DiffWarning.group == group,
+    warnings = DiffWarning.objects.filter(
+        revision_id=revision_id,
+        diff_id=diff_id,
+        status=DiffWarningStatus.ACTIVE,
+        group=group,
     )
     return [w.serialize() for w in warnings], 200

src/lando/api/legacy/api/landing_jobs.py

@@ -8,7 +8,6 @@
 
 from lando.api import auth
 from lando.main.models.landing_job import LandingJob, LandingJobAction, LandingJobStatus
-from lando.api.legacy.storage import db
 
 logger = logging.getLogger(__name__)
 
@@ -32,6 +31,7 @@ def put(landing_job_id: str, data: dict):
             updated (for example, when trying to cancel a job that is already in
             progress).
     """
+    # TODO: fix this based on locks/etc.
     landing_job = LandingJob.query.with_for_update().get(landing_job_id)
 
     if not landing_job:
@@ -61,7 +61,7 @@ def put(landing_job_id: str, data: dict):
 
     if landing_job.status in (LandingJobStatus.SUBMITTED, LandingJobStatus.DEFERRED):
         landing_job.transition_status(LandingJobAction.CANCEL)
-        db.session.commit()
+        landing_job.save()
         return {"id": landing_job.id}, 200
     else:
         raise ProblemException(

src/lando/api/legacy/api/stacks.py

@@ -119,9 +119,7 @@ def get(phab: PhabricatorClient, revision_id: str):
 
     revisions_response = []
     for _phid, phab_revision in stack_data.revisions.items():
-        lando_revision = Revision.query.filter(
-            Revision.revision_id == phab_revision["id"]
-        ).one_or_none()
+        lando_revision = Revision.one_or_none(revision_id=phab_revision["id"])
         revision_phid = PhabricatorClient.expect(phab_revision, "phid")
         fields = PhabricatorClient.expect(phab_revision, "fields")
         diff_phid = PhabricatorClient.expect(fields, "diffPHID")

src/lando/api/legacy/api/transplants.py

@@ -55,7 +55,6 @@
     get_landable_repos_for_revision_data,
     request_extended_revision_data,
 )
-from lando.api.legacy.storage import db
 from lando.api.legacy.tasks import admin_remove_phab_project
 from lando.api.legacy.transplants import (
     TransplantAssessment,
@@ -354,10 +353,8 @@ def post(phab: PhabricatorClient, data: dict):
         lando_revision = Revision.get_from_revision_id(revision_id)
         if not lando_revision:
             lando_revision = Revision(revision_id=revision_id)
-            db.session.add(lando_revision)
-
         lando_revision.diff_id = diff_id
-        db.session.commit()
+        lando_revision.save()
 
         revision_reviewers[lando_revision.id] = get_approved_by_ids(
             phab,
@@ -373,7 +370,7 @@ def post(phab: PhabricatorClient, data: dict):
 
         raw_diff = phab.call_conduit("differential.getrawdiff", diffID=diff["id"])
         lando_revision.set_patch(raw_diff, patch_data)
-        db.session.commit()
+        lando_revision.save()
         lando_revisions.append(lando_revision)
 
     ldap_username = g.auth0_user.email
@@ -384,8 +381,7 @@ def post(phab: PhabricatorClient, data: dict):
         )
     )
     stack_ids = [revision.revision_id for revision in lando_revisions]
-    with db.session.begin_nested():
-        LandingJob.lock_table()
+    with LandingJob.lock_table():
         if (
             LandingJob.revisions_query(stack_ids)
             .filter(
@@ -412,7 +408,7 @@ def post(phab: PhabricatorClient, data: dict):
     # Submit landing job.
     job.status = LandingJobStatus.SUBMITTED
     job.set_landed_revision_diffs()
-    db.session.commit()
+    job.save()
 
     logger.info(f"New landing job {job.id} created for {landing_repo.tree} repo.")
 

src/lando/api/legacy/app.py

@@ -20,7 +20,6 @@
 from lando.api.legacy.repos import repo_clone_subsystem
 from lando.api.legacy.sentry import sentry_subsystem
 from lando.api.legacy.smtp import smtp_subsystem
-from lando.api.legacy.storage import db_subsystem
 from lando.api.legacy.systems import Subsystem
 from lando.api.legacy.treestatus import treestatus_subsystem
 from lando.api.legacy.ui import lando_ui_subsystem
@@ -36,7 +35,6 @@
     auth0_subsystem,
     cache_subsystem,
     celery_subsystem,
-    db_subsystem,
     lando_ui_subsystem,
     phabricator_subsystem,
     smtp_subsystem,
@@ -54,8 +52,6 @@ def load_config() -> dict[str, Any]:
         "MAIL_SUPPRESS_SEND": bool(os.getenv("MAIL_SUPPRESS_SEND")),
         "MAIL_USE_SSL": bool(os.getenv("MAIL_USE_SSL")),
         "MAIL_USE_TLS": bool(os.getenv("MAIL_USE_TLS")),
-        "SQLALCHEMY_DATABASE_URI": os.getenv("DATABASE_URL"),
-        "SQLALCHEMY_TRACK_MODIFICATIONS": False,
         "VERSION": version(),
     }
 

src/lando/api/legacy/cli.py

@@ -82,9 +82,6 @@ def landing_worker():
 @cli.command(name="run-pre-deploy-sequence")
 def run_pre_deploy_sequence():
     """Runs the sequence of commands required before a deployment."""
-    from lando.api.legacy.storage import db_subsystem
-
-    db_subsystem.ensure_ready()
     ConfigurationVariable.set(
         ConfigurationKey.API_IN_MAINTENANCE, VariableType.BOOL, "1"
     )
@@ -96,9 +93,6 @@ def run_pre_deploy_sequence():
 @cli.command(name="run-post-deploy-sequence")
 def run_post_deploy_sequence():
     """Runs the sequence of commands required after a deployment."""
-    from lando.api.legacy.storage import db_subsystem
-
-    db_subsystem.ensure_ready()
     ConfigurationVariable.set(
         ConfigurationKey.API_IN_MAINTENANCE, VariableType.BOOL, "0"
     )

src/lando/api/legacy/storage.py

@@ -303,10 +303,10 @@ def warning_revision_missing_testing_tag(
 
 @RevisionWarningCheck(6, "Revision has a diff warning.", True)
 def warning_diff_warning(*, revision, diff, **kwargs):
-    warnings = DiffWarning.query.filter(
-        DiffWarning.revision_id == revision["id"],
-        DiffWarning.diff_id == diff["id"],
-        DiffWarning.status == DiffWarningStatus.ACTIVE,
+    warnings = DiffWarning.objects.filter(
+        revision_id=revision["id"],
+        diff_id=diff["id"],
+        status=DiffWarningStatus.ACTIVE,
     )
     if warnings.count():
         return [w.data for w in warnings]

src/lando/api/legacy/transplants.py

@@ -26,8 +26,8 @@
     TreeApprovalRequired,
     TreeClosed,
 )
-from lando.api.legacy.models.configuration import ConfigurationKey
-from lando.api.legacy.models.landing_job import LandingJob, LandingJobAction, LandingJobStatus
+from lando.main.models.configuration import ConfigurationKey
+from lando.main.models.landing_job import LandingJob, LandingJobAction, LandingJobStatus
 from lando.api.legacy.notifications import (
     notify_user_of_bug_update_failure,
     notify_user_of_landing_failure,
@@ -36,7 +36,6 @@
     Repo,
     repo_clone_subsystem,
 )
-from lando.api.legacy.storage import SQLAlchemy, db
 from lando.api.legacy.tasks import phab_trigger_repo_update
 from lando.api.legacy.treestatus import (
     TreeStatus,
@@ -51,7 +50,7 @@
 
 
 @contextmanager
-def job_processing(worker: LandingWorker, job: LandingJob, db: SQLAlchemy):
+def job_processing(worker: LandingWorker, job: LandingJob):
     """Mutex-like context manager that manages job processing miscellany.
 
     This context manager facilitates graceful worker shutdown, tracks the duration of
@@ -60,14 +59,13 @@ def job_processing(worker: LandingWorker, job: LandingJob, db: SQLAlchemy):
     Args:
         worker: the landing worker that is processing jobs
         job: the job currently being processed
-        db: active database session
     """
     start_time = datetime.now()
     try:
         yield
     finally:
         job.duration_seconds = (datetime.now() - start_time).seconds
-        db.session.commit()
+        job.save()
 
 
 class LandingWorker(Worker):

src/lando/api/legacy/workers/landing_worker.py

@@ -14,7 +14,6 @@
 import redis
 import requests
 import requests_mock
-import sqlalchemy
 from flask import current_app
 from pytest_flask.plugin import JSONResponse
 
@@ -259,22 +258,6 @@ def app(versionfile, docker_env_vars, disable_migrations, mocked_repo_config):
     return flask_app
 
 
-@pytest.fixture
-def db(app):
-    """Reset database for each test."""
-    try:
-        _db.engine.connect()
-    except sqlalchemy.exc.OperationalError:
-        if EXTERNAL_SERVICES_SHOULD_BE_PRESENT:
-            raise
-        else:
-            pytest.skip("Could not connect to PostgreSQL")
-    _db.create_all()
-    yield _db
-    _db.session.remove()
-    _db.drop_all()
-
-
 @pytest.fixture
 def jwks(monkeypatch):
     monkeypatch.setattr("landoapi.auth.get_jwks", lambda *args, **kwargs: TEST_JWKS)

src/lando/api/tests/conftest.py

@@ -4,7 +4,7 @@
 
 import pytest
 
-from lando.api.legacy.models.revisions import (
+from lando.main.models.revision import (
     DiffWarning,
     DiffWarningGroup,
     DiffWarningStatus,
@@ -57,7 +57,7 @@ def test_diff_warning_create(db, client, diff_warning_data, phab_header):
     assert "id" in response.json
 
     pk = response.json["id"]
-    warning = DiffWarning.query.get(pk)
+    warning = DiffWarning.objects.get(pk=pk)
     assert warning.group == DiffWarningGroup.LINT
     assert warning.revision_id == 1
     assert warning.diff_id == 1
@@ -79,7 +79,7 @@ def test_diff_warning_delete(db, client, diff_warning_data, phab_header):
     )
     assert response.status_code == 201
     pk = response.json["id"]
-    warning = DiffWarning.query.get(pk)
+    warning = DiffWarning.objects.get(pk=pk)
     assert warning.status == DiffWarningStatus.ACTIVE
 
     response = client.delete(
@@ -89,7 +89,7 @@ def test_diff_warning_delete(db, client, diff_warning_data, phab_header):
 
     assert response.status_code == 200
 
-    warning = DiffWarning.query.get(pk)
+    warning = DiffWarning.objects.get(pk=pk)
     assert warning.status == DiffWarningStatus.ARCHIVED
 
 

src/lando/api/tests/test_diff_warnings.py

@@ -5,24 +5,10 @@
 from unittest.mock import Mock
 
 import redis
-from sqlalchemy.exc import SQLAlchemyError
 
 from lando.api.legacy.auth import auth0_subsystem
 from lando.api.legacy.cache import cache_subsystem
 from lando.api.legacy.phabricator import PhabricatorAPIException, phabricator_subsystem
-from lando.api.legacy.storage import db_subsystem
-
-
-def test_database_healthy(db):
-    assert db_subsystem.healthy() is True
-
-
-def test_database_unhealthy(db, monkeypatch):
-    mock_db = Mock(db)
-    monkeypatch.setattr("landoapi.storage.db", mock_db)
-
-    mock_db.engine.connect.side_effect = SQLAlchemyError
-    assert db_subsystem.healthy() is not True
 
 
 def test_phabricator_healthy(app, phabdouble):

src/lando/api/tests/test_health.py

@@ -4,7 +4,7 @@
 
 import pytest
 
-from lando.api.legacy.models.landing_job import LandingJob, LandingJobStatus
+from lando.main.models.landing_job import LandingJob, LandingJobStatus
 
 
 @pytest.fixture

src/lando/api/tests/test_landing_job.py

@@ -9,12 +9,12 @@
 import pytest
 
 from lando.api.legacy.hg import AUTOFORMAT_COMMIT_MESSAGE, HgRepo
-from lando.api.legacy.models.landing_job import (
+from lando.main.models.landing_job import (
     LandingJob,
     LandingJobStatus,
     add_job_with_revisions,
 )
-from lando.api.legacy.models.revisions import Revision
+from lando.main.models.revision import Revision
 from lando.api.legacy.repos import SCM_LEVEL_3, Repo
 from lando.api.legacy.workers.landing_worker import LandingWorker
 
@@ -878,5 +878,5 @@ def test_landing_job_revisions_sorting(
     new_ordering = [revisions[2], revisions[0], revisions[1]]
     job.sort_revisions(new_ordering)
     db.session.commit()
-    job = LandingJob.query.get(job.id)
+    job = LandingJob.objects.get(id=job.id)
     assert job.revisions == new_ordering