landing_job: add landed_revisions field (bug 1834885) (#318)

- add diff_id to revision_landing_job table
- add method to set diff IDs and call it at landing time
- add landed_revisions helper method to fetch revision_to_diff_ids
- add a test that lands two versions of a revision
- fix variable names in transplants.warning_previously_landed

landoapi/api/transplants.py

@@ -401,6 +401,7 @@ def post(phab: PhabricatorClient, data: dict):
 
     # Submit landing job.
     job.status = LandingJobStatus.SUBMITTED
+    job.set_landed_revision_diffs()
     db.session.commit()
 
     logger.info(f"New landing job {job.id} created for {landing_repo.tree} repo.")

landoapi/models/landing_job.py

@@ -135,8 +135,20 @@ class LandingJob(Base):
     )
 
     @property
-    def landing_path(self) -> list[tuple[int, int]]:
-        return [(revision.revision_id, revision.diff_id) for revision in self.revisions]
+    def landed_revisions(self) -> dict:
+        """Return revision and diff ID mapping associated with the landing job."""
+        revision_ids = [revision.id for revision in self.revisions]
+        revision_to_diff_ids_query = (
+            revision_landing_job.select()
+            .join(Revision)
+            .where(
+                revision_landing_job.c.revision_id.in_(revision_ids),
+                revision_landing_job.c.landing_job_id == self.id,
+            )
+            .with_only_columns(Revision.revision_id, revision_landing_job.c.diff_id)
+            .order_by(revision_landing_job.c.index)
+        )
+        return dict(list(db.session.execute(revision_to_diff_ids_query)))
 
     @property
     def serialized_landing_path(self):
@@ -147,7 +159,7 @@ def serialized_landing_path(self):
                     "revision_id": "D{}".format(revision_id),
                     "diff_id": diff_id,
                 }
-                for revision_id, diff_id in self.landing_path
+                for revision_id, diff_id in self.landed_revisions.items()
             ]
         else:
             return [
@@ -249,6 +261,19 @@ def sort_revisions(self, revisions: list[Revision]):
                 .values(index=index)
             )
 
+    def set_landed_revision_diffs(self):
+        """Assign diff_ids, if available, to each association row."""
+        # Update association table records with current diff_id values.
+        for revision in self.revisions:
+            db.session.execute(
+                revision_landing_job.update()
+                .where(
+                    revision_landing_job.c.landing_job_id == self.id,
+                    revision_landing_job.c.revision_id == revision.id,
+                )
+                .values(diff_id=revision.diff_id)
+            )
+
     def transition_status(
         self,
         action: LandingJobAction,

landoapi/models/revisions.py

@@ -36,11 +36,13 @@ class DiffWarningGroup(enum.Enum):
 
 
 # Association table with custom "index" column to guarantee sorting of revisions.
+# The diff_id column is used as a transaction record of the diff_id at landing time.
 revision_landing_job = db.Table(
     "revision_landing_job",
     db.Column("landing_job_id", db.ForeignKey("landing_job.id")),
     db.Column("revision_id", db.ForeignKey("revision.id")),
     db.Column("index", db.Integer),
+    db.Column("diff_id", db.Integer, nullable=True),
 )
 
 

landoapi/transplants.py

@@ -224,13 +224,11 @@ def warning_previously_landed(*, revision, diff, **kwargs):
     if job is None:
         return None
 
-    revision_to_diff_id = {
-        revision.revision_id: revision.diff_id for revision in job.revisions
-    }
+    revision_to_diff_id = job.landed_revisions
     if job.revision_to_diff_id:
         legacy_data = {
-            int(revision_id): int(diff_id)
-            for revision_id, diff_id in job.revision_to_diff_id.items()
+            int(legacy_revision_id): int(legacy_diff_id)
+            for legacy_revision_id, legacy_diff_id in job.revision_to_diff_id.items()
         }
         revision_to_diff_id.update(legacy_data)
     landed_diff_id = revision_to_diff_id[revision_id]

migrations/versions/50ffadceca83_add_revision_landing_job_diff_id.py

@@ -0,0 +1,30 @@
+"""add revision_landing_job.diff_id
+
+Revision ID: 50ffadceca83
+Revises: a8cd4d43c4c3
+Create Date: 2023-05-25 01:50:36.755884
+
+"""
+from alembic import op
+import sqlalchemy as sa
+
+
+# revision identifiers, used by Alembic.
+revision = "50ffadceca83"
+down_revision = "a8cd4d43c4c3"
+branch_labels = None
+depends_on = None
+
+
+def upgrade():
+    # ### commands auto generated by Alembic - please adjust! ###
+    op.add_column(
+        "revision_landing_job", sa.Column("diff_id", sa.Integer(), nullable=True)
+    )
+    # ### end Alembic commands ###
+
+
+def downgrade():
+    # ### commands auto generated by Alembic - please adjust! ###
+    op.drop_column("revision_landing_job", "diff_id")
+    # ### end Alembic commands ###

tests/test_transplants.py

@@ -711,6 +711,106 @@ def test_integrated_transplant_simple_stack_saves_data_in_db(
         (r3["id"], d3["id"]),
     ]
     assert job.status == LandingJobStatus.SUBMITTED
+    assert job.landed_revisions == {1: 1, 2: 2, 3: 3}
+
+
+def test_integrated_transplant_updated_diff_id_reflected_in_landed_revisions(
+    db,
+    client,
+    phabdouble,
+    auth0_mock,
+    release_management_project,
+    register_codefreeze_uri,
+):
+    """
+    Perform a simple test but with two landing jobs for the same revision.
+
+    The test is similar to the one in
+    test_integrated_transplant_simple_stack_saves_data_in_db but submits an additional
+    landing job for an updated revision diff.
+    """
+    repo = phabdouble.repo()
+    user = phabdouble.user(username="reviewer")
+
+    d1a = phabdouble.diff()
+    r1 = phabdouble.revision(diff=d1a, repo=repo)
+    phabdouble.reviewer(r1, user)
+
+    response = client.post(
+        "/transplants",
+        json={
+            "landing_path": [
+                {"revision_id": "D{}".format(r1["id"]), "diff_id": d1a["id"]},
+            ]
+        },
+        headers=auth0_mock.mock_headers,
+    )
+    assert response.status_code == 202
+    assert response.content_type == "application/json"
+    assert "id" in response.json
+    job_1_id = response.json["id"]
+
+    # Ensure DB access isn't using uncommitted data.
+    db.session.close()
+
+    # Get LandingJob object by its id.
+    job = LandingJob.query.get(job_1_id)
+    assert job.id == job_1_id
+    assert [(revision.revision_id, revision.diff_id) for revision in job.revisions] == [
+        (r1["id"], d1a["id"]),
+    ]
+    assert job.status == LandingJobStatus.SUBMITTED
+    assert job.landed_revisions == {r1["id"]: d1a["id"]}
+
+    # Cancel job.
+    response = client.put(
+        f"/landing_jobs/{job.id}",
+        json={"status": "CANCELLED"},
+        headers=auth0_mock.mock_headers,
+    )
+
+    job = LandingJob.query.get(job_1_id)
+    assert job.status == LandingJobStatus.CANCELLED
+
+    d1b = phabdouble.diff(revision=r1)
+    phabdouble.reviewer(r1, user)
+    response = client.post(
+        "/transplants",
+        json={
+            "landing_path": [
+                {"revision_id": "D{}".format(r1["id"]), "diff_id": d1b["id"]},
+            ]
+        },
+        headers=auth0_mock.mock_headers,
+    )
+
+    job_2_id = response.json["id"]
+
+    # Ensure DB access isn't using uncommitted data.
+    db.session.close()
+
+    # Get LandingJob objects by their ids.
+    job_1 = LandingJob.query.get(job_1_id)
+    job_2 = LandingJob.query.get(job_2_id)
+
+    # The Revision objects always track the latest revisions.
+    assert [
+        (revision.revision_id, revision.diff_id) for revision in job_1.revisions
+    ] == [
+        (r1["id"], d1b["id"]),
+    ]
+
+    assert [
+        (revision.revision_id, revision.diff_id) for revision in job_2.revisions
+    ] == [
+        (r1["id"], d1b["id"]),
+    ]
+
+    assert job_1.status == LandingJobStatus.CANCELLED
+    assert job_2.status == LandingJobStatus.SUBMITTED
+
+    assert job_1.landed_revisions == {r1["id"]: d1a["id"]}
+    assert job_2.landed_revisions == {r1["id"]: d1b["id"]}
 
 
 def test_integrated_transplant_with_flags(