From a54540f64eb563eedd594b9c8a8e7095b67789d9 Mon Sep 17 00:00:00 2001 From: Raul Almeida Date: Tue, 21 Jul 2026 10:47:16 -0300 Subject: [PATCH 01/21] feat: add attributes JSONB to artefacts and remove solution-specific fields --- ..._replace_solution_specific_fields_with_.py | 116 +++++++ .../controllers/artefacts/artefacts.py | 33 +- .../controllers/artefacts/builds.py | 2 +- .../controllers/artefacts/models.py | 6 - .../controllers/test_executions/start_test.py | 2 - backend/test_observer/data_access/models.py | 69 +---- .../test_observer/data_access/repository.py | 4 - .../controllers/artefacts/test_artefacts.py | 288 ------------------ .../controllers/artefacts/test_builds.py | 4 - .../test_executions/test_reruns.py | 1 - backend/tests/data_generator.py | 3 - 11 files changed, 125 insertions(+), 403 deletions(-) create mode 100644 backend/migrations/versions/2026_07_21_1311-8202f7b5953e_replace_solution_specific_fields_with_.py diff --git a/backend/migrations/versions/2026_07_21_1311-8202f7b5953e_replace_solution_specific_fields_with_.py b/backend/migrations/versions/2026_07_21_1311-8202f7b5953e_replace_solution_specific_fields_with_.py new file mode 100644 index 000000000..f8543a503 --- /dev/null +++ b/backend/migrations/versions/2026_07_21_1311-8202f7b5953e_replace_solution_specific_fields_with_.py @@ -0,0 +1,116 @@ +"""Replace solution-specific fields with attributes + +Revision ID: 8202f7b5953e +Revises: eba1d1c92dba +Create Date: 2026-07-21 13:11:39.128081+00:00 + +""" + +import sqlalchemy as sa +from alembic import op +from sqlalchemy.dialects import postgresql + +# revision identifiers, used by Alembic. +revision = "8202f7b5953e" +down_revision = "eba1d1c92dba" +branch_labels = None +depends_on = None + + +def upgrade() -> None: + op.add_column("artefact", sa.Column("attributes", postgresql.JSONB(), server_default="{}", nullable=False)) + op.drop_index("unique_solution", table_name="artefact", postgresql_where="(family = 'solution'::familyname)") + op.create_index( + "unique_solution", "artefact", ["name", "version"], unique=True, postgresql_where=sa.text("family = 'solution'") + ) + _remove_bundled_builds() + + +def downgrade() -> None: + _add_bundled_builds() + op.drop_index("unique_solution", table_name="artefact", postgresql_where=sa.text("family = 'solution'")) + op.create_index( + "unique_solution", + "artefact", + ["name", "source", "version", "track", "stage", "bundled_builds_hash"], + unique=True, + postgresql_where="(family = 'solution'::familyname)", + ) + op.drop_column("artefact", "attributes") + + +def _remove_bundled_builds() -> None: + _copy_bundled_builds_to_attributes() + op.drop_table("artefact_bundled_builds_association") + op.drop_column("artefact", "bundled_builds_hash") + + +def _add_bundled_builds() -> None: + op.add_column( + "artefact", sa.Column("bundled_builds_hash", sa.VARCHAR(length=64), autoincrement=False, nullable=True) + ) + op.create_table( + "artefact_bundled_builds_association", + sa.Column("artefact_id", sa.INTEGER(), autoincrement=False, nullable=False), + sa.Column("artefact_build_id", sa.INTEGER(), autoincrement=False, nullable=False), + sa.ForeignKeyConstraint( + ["artefact_build_id"], + ["artefact_build.id"], + name="artefact_bundled_builds_association_id_fkey", + ondelete="CASCADE", + ), + sa.ForeignKeyConstraint( + ["artefact_id"], ["artefact.id"], name="artefact_bundled_builds_artefact_id_fkey", ondelete="CASCADE" + ), + sa.PrimaryKeyConstraint("artefact_id", "artefact_build_id", name="artefact_bundled_builds_association_pkey"), + ) + _restore_bundled_builds_from_attributes() + + +def _copy_bundled_builds_to_attributes() -> None: + op.execute( + """ + UPDATE artefact AS a + SET attributes = a.attributes + || jsonb_strip_nulls( + jsonb_build_object('bundled_builds_hash', a.bundled_builds_hash) + ) + || COALESCE( + ( + SELECT jsonb_build_object( + 'bundled_builds', + jsonb_agg(assoc.artefact_build_id ORDER BY assoc.artefact_build_id) + ) + FROM artefact_bundled_builds_association assoc + WHERE assoc.artefact_id = a.id + HAVING count(*) > 0 + ), + '{}'::jsonb + ) + WHERE a.bundled_builds_hash IS NOT NULL + OR EXISTS ( + SELECT 1 + FROM artefact_bundled_builds_association assoc + WHERE assoc.artefact_id = a.id + ) + """ + ) + + +def _restore_bundled_builds_from_attributes() -> None: + op.execute( + """ + UPDATE artefact AS a + SET bundled_builds_hash = a.attributes ->> 'bundled_builds_hash' + WHERE a.attributes ? 'bundled_builds_hash' + """ + ) + op.execute( + """ + INSERT INTO artefact_bundled_builds_association (artefact_id, artefact_build_id) + SELECT a.id, elem::int + FROM artefact a, + jsonb_array_elements_text(a.attributes -> 'bundled_builds') AS elem + WHERE a.attributes ? 'bundled_builds' + """ + ) diff --git a/backend/test_observer/controllers/artefacts/artefacts.py b/backend/test_observer/controllers/artefacts/artefacts.py index 7d5cb2de3..d00228c88 100644 --- a/backend/test_observer/controllers/artefacts/artefacts.py +++ b/backend/test_observer/controllers/artefacts/artefacts.py @@ -86,7 +86,6 @@ def get_artefacts(family: FamilyName | None = None, db: Session = Depends(get_db db, family, load_environment_reviews=True, - load_bundled_builds=True, order_by_columns=order_by, ) else: @@ -95,7 +94,6 @@ def get_artefacts(family: FamilyName | None = None, db: Session = Depends(get_db db, family, load_environment_reviews=True, - load_bundled_builds=True, order_by_columns=order_by, ) @@ -193,7 +191,6 @@ def get_artefact_history( .offset(offset) .options( selectinload(Artefact.builds).selectinload(ArtefactBuild.test_executions), - selectinload(Artefact.bundled_builds), ) ) @@ -226,7 +223,6 @@ def get_artefact( artefact: Artefact = Depends( ArtefactRetriever( selectinload(Artefact.builds).selectinload(ArtefactBuild.environment_reviews), - selectinload(Artefact.bundled_builds), ) ), ): @@ -246,7 +242,6 @@ def patch_artefact( artefact: Artefact = Depends( ArtefactRetriever( selectinload(Artefact.builds).selectinload(ArtefactBuild.environment_reviews), - selectinload(Artefact.bundled_builds), ) ), ): @@ -340,30 +335,6 @@ def patch_artefact( artefact, NotificationType.USER_ASSIGNED_ARTEFACT_REVIEW, ) - - # Handle bundled_builds - if "bundled_builds" in request.model_fields_set: - if request.bundled_builds is None: - artefact.bundled_builds = [] - elif len(request.bundled_builds) != len(set(request.bundled_builds)): - raise HTTPException( - status_code=422, - detail="Duplicate build ids are not allowed in bundled_builds", - ) - else: - builds = db.scalars(select(ArtefactBuild).where(ArtefactBuild.id.in_(request.bundled_builds))).all() - builds_by_id = {build.id: build for build in builds} - bundled_builds = [] - for build_id in request.bundled_builds: - build = builds_by_id.get(build_id) - if build is None: - raise HTTPException( - status_code=422, - detail=f"ArtefactBuild with id {build_id} not found", - ) - bundled_builds.append(build) - artefact.bundled_builds = bundled_builds - db.commit() if len(newly_assigned_reviewers) > 0 and artefact.jira_issue is not None: @@ -412,7 +383,7 @@ def _validate_artefact_stage(artefact: Artefact, stage: StageName) -> None: dependencies=[Security(permission_checker, scopes=[Permission.view_artefact])], ) def get_artefact_versions( - artefact: Artefact = Depends(ArtefactRetriever(selectinload(Artefact.bundled_builds))), + artefact: Artefact = Depends(ArtefactRetriever()), db: Session = Depends(get_db), ): return db.scalars( @@ -425,8 +396,6 @@ def get_artefact_versions( .where(Artefact.os == artefact.os) .where(Artefact.release == artefact.release) .where(Artefact.source == artefact.source) - .where(Artefact.bundled_builds_hash == artefact.bundled_builds_hash) - .options(selectinload(Artefact.bundled_builds)) .order_by(Artefact.id.desc()) ) diff --git a/backend/test_observer/controllers/artefacts/builds.py b/backend/test_observer/controllers/artefacts/builds.py index e0ac22fab..3dfd35189 100644 --- a/backend/test_observer/controllers/artefacts/builds.py +++ b/backend/test_observer/controllers/artefacts/builds.py @@ -43,7 +43,7 @@ def get_artefact_builds( artefact: Artefact = Depends( ArtefactRetriever( selectinload(Artefact.builds).selectinload(ArtefactBuild.test_executions).options(*TEST_EXECUTION_OPTIONS), - selectinload(Artefact.builds).selectinload(ArtefactBuild.bundled_in), + selectinload(Artefact.builds), ) ), ): diff --git a/backend/test_observer/controllers/artefacts/models.py b/backend/test_observer/controllers/artefacts/models.py index e4198831d..9a8f45aed 100644 --- a/backend/test_observer/controllers/artefacts/models.py +++ b/backend/test_observer/controllers/artefacts/models.py @@ -79,7 +79,6 @@ class ArtefactResponse(BaseModel): jira_issue: str | None all_environment_reviews_count: int completed_environment_reviews_count: int - bundled_builds: list["ArtefactBuildMinimalResponse"] = Field(default_factory=list) @computed_field( description=("Backward-compatible assignee field. Populated from the first entry in reviewers when present.") @@ -147,7 +146,6 @@ class ArtefactBuildResponse(BaseModel): architecture: str revision: int | None test_executions: list[TestExecutionResponse] - bundled_in: list["ArtefactMinimalResponse"] = Field(default_factory=list) class ArtefactPatch(BaseModel): @@ -156,10 +154,6 @@ class ArtefactPatch(BaseModel): stage: StageName | None = None comment: str | None = None jira_issue: str | None = None - bundled_builds: list[int] | None = Field( - default=None, - description="List of ArtefactBuild IDs to bundle with this artefact", - ) assignee_id: int | None = Field( default=None, deprecated=True, diff --git a/backend/test_observer/controllers/test_executions/start_test.py b/backend/test_observer/controllers/test_executions/start_test.py index f66c44fb4..f2d39c42b 100644 --- a/backend/test_observer/controllers/test_executions/start_test.py +++ b/backend/test_observer/controllers/test_executions/start_test.py @@ -38,7 +38,6 @@ TestExecution, TestPlan, User, - calculate_bundled_builds_hash, ) from test_observer.data_access.models_enums import NotificationType from test_observer.data_access.queries import match_artefact_considering_specificity @@ -355,7 +354,6 @@ def create_artefact(self) -> None: filter_kwargs["track"] = self.request.track filter_kwargs["source"] = self.request.source filter_kwargs["stage"] = self.request.execution_stage - filter_kwargs["bundled_builds_hash"] = calculate_bundled_builds_hash([]) creation_kwargs = {} self.artefact = get_or_create(self.db, Artefact, filter_kwargs=filter_kwargs, creation_kwargs=creation_kwargs) diff --git a/backend/test_observer/data_access/models.py b/backend/test_observer/data_access/models.py index fd095409b..b1442d055 100644 --- a/backend/test_observer/data_access/models.py +++ b/backend/test_observer/data_access/models.py @@ -13,7 +13,6 @@ # SPDX-FileCopyrightText: Copyright 2023 Canonical Ltd. # SPDX-License-Identifier: AGPL-3.0-only -import hashlib import secrets from collections import defaultdict from datetime import date, datetime, timedelta @@ -34,16 +33,15 @@ case, column, desc, - event, exists, select, ) -from sqlalchemy.dialects.postgresql import ARRAY +from sqlalchemy.dialects.postgresql import ARRAY, JSONB from sqlalchemy.ext.hybrid import hybrid_property +from sqlalchemy.ext.mutable import MutableDict from sqlalchemy.orm import ( DeclarativeBase, Mapped, - Session, column_property, foreign, mapped_column, @@ -117,22 +115,6 @@ def data_model_repr(obj: DataModel, *keys: str) -> str: ) -artefact_bundled_builds_association = Table( - "artefact_bundled_builds_association", - Base.metadata, - Column( - "artefact_id", - ForeignKey("artefact.id", ondelete="CASCADE"), - primary_key=True, - ), - Column( - "artefact_build_id", - ForeignKey("artefact_build.id", ondelete="CASCADE"), - primary_key=True, - ), -) - - environment_review_reviewers_association = Table( "environment_review_reviewers_association", Base.metadata, @@ -326,14 +308,14 @@ class Artefact(Base): # Snap and Charm specific field branch: Mapped[str] = mapped_column(String(200), default="") - # Snap, Charm and Solution specific field + # Snap and Charm specific field track: Mapped[str] = mapped_column(default="") # Deb specific fields series: Mapped[str] = mapped_column(default="") repo: Mapped[str] = mapped_column(default="") - # Deb and Solution specific field + # Deb specific field source: Mapped[str] = mapped_column(String(200), default="") # Image specific fields @@ -343,12 +325,11 @@ class Artefact(Base): owner: Mapped[str] = mapped_column(String(200), default="") image_url: Mapped[str] = mapped_column(String(200), default="") + # (for now) Solution specific field + attributes: Mapped[dict[str, Any]] = mapped_column(MutableDict.as_mutable(JSONB), default=dict, server_default="{}") + # Relationships builds: Mapped[list["ArtefactBuild"]] = relationship(back_populates="artefact", cascade="all, delete") - bundled_builds: Mapped[list["ArtefactBuild"]] = relationship( - secondary=artefact_bundled_builds_association, back_populates="bundled_in" - ) - bundled_builds_hash: Mapped[str | None] = mapped_column(String(64), default=None) reviewers: Mapped[list[User]] = relationship( secondary=artefact_reviewers_association, back_populates="artefact_reviews" ) @@ -397,11 +378,7 @@ def architectures(self) -> set[str]: Index( "unique_solution", "name", - "source", "version", - "track", - "stage", - "bundled_builds_hash", postgresql_where=column("family") == FamilyName.solution.name, unique=True, ), @@ -428,7 +405,6 @@ def __repr__(self) -> str: "due_date", "status", "archived", - "bundled_builds_hash", ) @hybrid_property @@ -454,34 +430,6 @@ def completed_environment_reviews_count(self) -> int: return sum(len([er for er in ab.environment_reviews if er.review_decision]) for ab in self.latest_builds) -def calculate_bundled_builds_hash(build_ids: list[int]) -> str: - ordered_ids = ",".join(str(build_id) for build_id in sorted(build_ids)) - return hashlib.sha256(ordered_ids.encode()).hexdigest() - - -def refresh_artefact_hash(artefact: Artefact) -> None: - if artefact.family == FamilyName.solution: - build_ids = [b.id for b in artefact.bundled_builds if b.id] - artefact.bundled_builds_hash = calculate_bundled_builds_hash(build_ids) - - -@event.listens_for(Session, "before_flush") -def receive_before_flush(session: Session, *args: Any) -> None: # noqa: ANN401, ARG001 - for obj in session.new | session.dirty: - if isinstance(obj, Artefact) and obj.family == FamilyName.solution: - refresh_artefact_hash(obj) - - -@event.listens_for(Artefact.bundled_builds, "append") -def bundle_append(target: Artefact, *args: Any) -> None: # noqa: ANN401, ARG001 - refresh_artefact_hash(target) - - -@event.listens_for(Artefact.bundled_builds, "remove") -def bundle_remove(target: Artefact, *args: Any) -> None: # noqa: ANN401, ARG001 - refresh_artefact_hash(target) - - class ArtefactBuild(Base): """A model to represent specific builds of artefact (e.g. arm64 revision 2)""" @@ -492,9 +440,6 @@ class ArtefactBuild(Base): # Relationships artefact_id: Mapped[int] = mapped_column(ForeignKey("artefact.id", ondelete="CASCADE"), index=True) artefact: Mapped[Artefact] = relationship(back_populates="builds", foreign_keys=[artefact_id]) - bundled_in: Mapped[list["Artefact"]] = relationship( - secondary=artefact_bundled_builds_association, back_populates="bundled_builds" - ) test_executions: Mapped[list["TestExecution"]] = relationship( back_populates="artefact_build", cascade="all, delete" ) diff --git a/backend/test_observer/data_access/repository.py b/backend/test_observer/data_access/repository.py index aeb46bf3d..65783e0ed 100644 --- a/backend/test_observer/data_access/repository.py +++ b/backend/test_observer/data_access/repository.py @@ -32,7 +32,6 @@ def get_artefacts_by_family( family: FamilyName, load_environment_reviews: bool = False, load_builds: bool = False, - load_bundled_builds: bool = False, order_by_columns: Iterable[Any] | None = None, ) -> list[Artefact]: """ @@ -136,9 +135,6 @@ def get_artefacts_by_family( elif load_builds: query = query.options(joinedload(Artefact.builds)) - if load_bundled_builds: - query = query.options(joinedload(Artefact.bundled_builds)) - if order_by_columns: query = query.order_by(*order_by_columns) diff --git a/backend/tests/controllers/artefacts/test_artefacts.py b/backend/tests/controllers/artefacts/test_artefacts.py index 31608b5fa..90f5156b1 100644 --- a/backend/tests/controllers/artefacts/test_artefacts.py +++ b/backend/tests/controllers/artefacts/test_artefacts.py @@ -20,7 +20,6 @@ import pytest from fastapi.testclient import TestClient -from sqlalchemy.exc import IntegrityError from sqlalchemy.orm import Session from test_observer.common.enums import Permission @@ -1167,7 +1166,6 @@ def _assert_get_artefact_response(response: dict[str, Any], artefact: Artefact) "all_environment_reviews_count": artefact.all_environment_reviews_count, "completed_environment_reviews_count": artefact.completed_environment_reviews_count, # noqa: E501 "created_at": artefact.created_at.isoformat(), - "bundled_builds": [], } if artefact.reviewers: expected["reviewers"] = [ @@ -1449,289 +1447,3 @@ def test_patch_artefact_with_ignore_permissions_allowed( assert response.json()["comment"] == "Updated despite no permissions" finally: del app.dependency_overrides[get_current_user] - - -def test_solution_artefacts_with_same_builds_in_different_order_cannot_be_created(generator: DataGenerator): - """Test that two solution artefacts with identical bundled builds set, but listed in a different order, cannot be - created.""" - # GIVEN a solution was created - charm = generator.gen_artefact(family=FamilyName.charm, name="my-charm", version="1.0", track="latest") - build1 = generator.gen_artefact_build(charm, architecture="amd64") - build2 = generator.gen_artefact_build(charm, architecture="arm64") - - generator.gen_artefact( - name="my-solution", - family=FamilyName.solution, - stage=StageName.stable, - version="1.0", - track="latest", - source="my-source", - bundled_builds=[build2, build1], - ) - - # WHEN we attempt to create another identical solution - # THEN then unique constraint prevents the second solution from being created - with pytest.raises(IntegrityError): - generator.gen_artefact( - name="my-solution", - family=FamilyName.solution, - stage=StageName.stable, - version="1.0", - track="latest", - source="my-source", - bundled_builds=[build1, build2], - ) - - -def test_solution_artefacts_with_different_builds_are_created(generator: DataGenerator): - """Test that two solution artefacts with different bundled builds can be created.""" - # GIVEN two different builds and a solution using one of them - charm = generator.gen_artefact(family=FamilyName.charm, name="my-charm", version="1.0", track="latest") - build1 = generator.gen_artefact_build(charm, architecture="amd64") - build2 = generator.gen_artefact_build(charm, architecture="arm64") - - generator.gen_artefact( - name="my-solution", - family=FamilyName.solution, - stage=StageName.stable, - version="1.0", - track="latest", - source="my-source", - bundled_builds=[build1], - ) - - # WHEN we attempt to create another solution with a different build - # THEN the unique constraint does not block this operation - generator.gen_artefact( - name="my-solution", - family=FamilyName.solution, - stage=StageName.stable, - version="1.0", - track="latest", - source="my-source", - bundled_builds=[build2], - ) - - -def test_updating_solution_to_have_same_bundled_builds_as_another_is_blocked_by_unique_constraint( - generator: DataGenerator, -): - """Test that updating a solution artefact to have the same bundled builds as another solution is blocked by the - unique constraint.""" - # GIVEN two solutions with different builds - charm = generator.gen_artefact(family=FamilyName.charm, name="my-charm", version="1.0", track="latest") - build1 = generator.gen_artefact_build(charm, architecture="amd64") - build2 = generator.gen_artefact_build(charm, architecture="arm64") - - generator.gen_artefact( - name="my-solution", - family=FamilyName.solution, - stage=StageName.stable, - version="1.0", - track="latest", - source="my-source", - bundled_builds=[build1], - ) - - solution2 = generator.gen_artefact( - name="my-solution", - family=FamilyName.solution, - stage=StageName.stable, - version="1.0", - track="latest", - source="my-source", - bundled_builds=[build2], - ) - - # WHEN we attempt to update solution2 to have the same bundled build as solution1 - # THEN the unique constraint prevents this update from succeeding - with pytest.raises(IntegrityError): - solution2.bundled_builds = [build1] - generator._add_object(solution2) - - -def test_patch_artefact_bundled_builds_set_valid_builds(generator: DataGenerator, test_client: TestClient): - """Test that patching an artefact with valid bundled_builds returns 200.""" - # GIVEN a solution artefact and some builds to bundle - solution = generator.gen_artefact( - family=FamilyName.solution, - name="my-solution", - version="1.0", - track="latest", - source="my-source", - ) - charm = generator.gen_artefact(family=FamilyName.charm, name="my-charm", version="1.0", track="latest") - build1 = generator.gen_artefact_build(charm, architecture="amd64") - build2 = generator.gen_artefact_build(charm, architecture="arm64") - - # WHEN patching with valid bundled builds - response = make_authenticated_request( - lambda: test_client.patch( - f"/v1/artefacts/{solution.id}", - json={"bundled_builds": [build1.id, build2.id]}, - ), - Permission.change_artefact, - ) - - # THEN it succeeds with 200 - assert response.status_code == 200 - data = response.json() - assert len(data["bundled_builds"]) == 2 - bundled_ids = {b["id"] for b in data["bundled_builds"]} - assert bundled_ids == {build1.id, build2.id} - - -def test_patch_artefact_bundled_builds_clear_to_empty(generator: DataGenerator, test_client: TestClient): - """Test that patching an artefact with null bundled_builds clears them.""" - # GIVEN a solution artefact with bundled builds - charm = generator.gen_artefact(family=FamilyName.charm, name="my-charm", version="1.0", track="latest") - build1 = generator.gen_artefact_build(charm, architecture="amd64") - solution = generator.gen_artefact( - family=FamilyName.solution, - name="my-solution", - version="1.0", - track="latest", - source="my-source", - bundled_builds=[build1], - ) - - # Verify it has builds - response = make_authenticated_request( - lambda: test_client.get(f"/v1/artefacts/{solution.id}"), - Permission.view_artefact, - ) - assert len(response.json()["bundled_builds"]) == 1 - - # WHEN patching with null bundled_builds - response = make_authenticated_request( - lambda: test_client.patch( - f"/v1/artefacts/{solution.id}", - json={"bundled_builds": None}, - ), - Permission.change_artefact, - ) - - # THEN it succeeds with 200 and bundled_builds is empty - assert response.status_code == 200 - data = response.json() - assert len(data["bundled_builds"]) == 0 - - -def test_patch_artefact_bundled_builds_duplicate_ids_returns_422(generator: DataGenerator, test_client: TestClient): - """Test that patching with duplicate build IDs returns 422.""" - # GIVEN a solution artefact and a build - solution = generator.gen_artefact( - family=FamilyName.solution, - name="my-solution", - version="1.0", - track="latest", - source="my-source", - ) - charm = generator.gen_artefact(family=FamilyName.charm, name="my-charm", version="1.0", track="latest") - build = generator.gen_artefact_build(charm, architecture="amd64") - - # WHEN patching with duplicate build IDs - response = make_authenticated_request( - lambda: test_client.patch( - f"/v1/artefacts/{solution.id}", - json={"bundled_builds": [build.id, build.id]}, - ), - Permission.change_artefact, - ) - - # THEN it returns 422 with appropriate error message - assert response.status_code == 422 - data = response.json() - assert "Duplicate build ids are not allowed in bundled_builds" in data["detail"] - - -def test_patch_artefact_bundled_builds_unknown_id_returns_422(generator: DataGenerator, test_client: TestClient): - """Test that patching with unknown build IDs returns 422.""" - # GIVEN a solution artefact - solution = generator.gen_artefact( - family=FamilyName.solution, - name="my-solution", - version="1.0", - track="latest", - source="my-source", - ) - charm = generator.gen_artefact(family=FamilyName.charm, name="my-charm", version="1.0", track="latest") - build = generator.gen_artefact_build(charm, architecture="amd64") - - # Use a non-existent build ID (assuming IDs are sequential and large number doesn't exist) - unknown_build_id = 999999 - - # WHEN patching with unknown build ID - response = make_authenticated_request( - lambda: test_client.patch( - f"/v1/artefacts/{solution.id}", - json={"bundled_builds": [build.id, unknown_build_id]}, - ), - Permission.change_artefact, - ) - - # THEN it returns 422 with appropriate error message - assert response.status_code == 422 - data = response.json() - assert f"ArtefactBuild with id {unknown_build_id} not found" in data["detail"] - - -def test_patch_artefact_bundled_builds_empty_list(generator: DataGenerator, test_client: TestClient): - """Test that patching with an empty bundled_builds list works.""" - # GIVEN a solution artefact with bundled builds - charm = generator.gen_artefact(family=FamilyName.charm, name="my-charm", version="1.0", track="latest") - build1 = generator.gen_artefact_build(charm, architecture="amd64") - solution = generator.gen_artefact( - family=FamilyName.solution, - name="my-solution", - version="1.0", - track="latest", - source="my-source", - bundled_builds=[build1], - ) - - # WHEN patching with empty list - response = make_authenticated_request( - lambda: test_client.patch( - f"/v1/artefacts/{solution.id}", - json={"bundled_builds": []}, - ), - Permission.change_artefact, - ) - - # THEN it succeeds with 200 and bundled_builds is empty - assert response.status_code == 200 - data = response.json() - assert len(data["bundled_builds"]) == 0 - - -def test_patch_artefact_bundled_builds_preserves_order(generator: DataGenerator, test_client: TestClient): - """Test that patching preserves the order of bundled builds.""" - # GIVEN a solution artefact and multiple builds - solution = generator.gen_artefact( - family=FamilyName.solution, - name="my-solution", - version="1.0", - track="latest", - source="my-source", - ) - charm = generator.gen_artefact(family=FamilyName.charm, name="my-charm", version="1.0", track="latest") - build1 = generator.gen_artefact_build(charm, architecture="amd64") - build2 = generator.gen_artefact_build(charm, architecture="arm64") - build3 = generator.gen_artefact_build(charm, architecture="ppc64el") - - # WHEN patching with builds in a specific order - response = make_authenticated_request( - lambda: test_client.patch( - f"/v1/artefacts/{solution.id}", - json={"bundled_builds": [build3.id, build1.id, build2.id]}, - ), - Permission.change_artefact, - ) - - # THEN the order is preserved - assert response.status_code == 200 - data = response.json() - assert len(data["bundled_builds"]) == 3 - returned_ids = [b["id"] for b in data["bundled_builds"]] - assert returned_ids == [build3.id, build1.id, build2.id] diff --git a/backend/tests/controllers/artefacts/test_builds.py b/backend/tests/controllers/artefacts/test_builds.py index 1e332e732..6ed97b32e 100644 --- a/backend/tests/controllers/artefacts/test_builds.py +++ b/backend/tests/controllers/artefacts/test_builds.py @@ -38,7 +38,6 @@ def test_get_artefact_builds(test_client: TestClient, generator: DataGenerator): "id": ab.id, "revision": ab.revision, "architecture": ab.architecture, - "bundled_in": [], "test_executions": [ { "id": te.id, @@ -81,7 +80,6 @@ def test_get_artefact_builds_sorts_test_executions_by_environment_name( "id": ab.id, "revision": ab.revision, "architecture": ab.architecture, - "bundled_in": [], "test_executions": [ { "id": te1.id, @@ -140,7 +138,6 @@ def test_get_artefact_builds_only_latest(test_client: TestClient, generator: Dat "id": artefact_build2.id, "revision": artefact_build2.revision, "architecture": artefact_build2.architecture, - "bundled_in": [], "test_executions": [], } ] @@ -164,7 +161,6 @@ def test_get_artefact_builds_with_rerun_requested(test_client: TestClient, gener "id": ab.id, "revision": ab.revision, "architecture": ab.architecture, - "bundled_in": [], "test_executions": [ { "id": te.id, diff --git a/backend/tests/controllers/test_executions/test_reruns.py b/backend/tests/controllers/test_executions/test_reruns.py index 3953c68db..aa0aca933 100644 --- a/backend/tests/controllers/test_executions/test_reruns.py +++ b/backend/tests/controllers/test_executions/test_reruns.py @@ -172,7 +172,6 @@ def test_execution_to_pending_rerun(test_execution: TestExecution, priority: int if test_execution.artefact_build.artefact.reviewers else None ), - "bundled_builds": [], "reviewers": test_execution.artefact_build.artefact.reviewers, "due_date": test_execution.artefact_build.artefact.due_date, "bug_link": test_execution.artefact_build.artefact.bug_link, diff --git a/backend/tests/data_generator.py b/backend/tests/data_generator.py index fcdc29898..2ce9621e0 100644 --- a/backend/tests/data_generator.py +++ b/backend/tests/data_generator.py @@ -147,7 +147,6 @@ def gen_artefact( bug_link: str = "", due_date: date | None = None, reviewers: list[User] | None = None, - bundled_builds: list[ArtefactBuild] | None = None, ) -> Artefact: family = FamilyName(family) @@ -163,7 +162,6 @@ def gen_artefact( created_at = created_at or datetime.utcnow() reviewers = reviewers or [] - bundled_builds = bundled_builds or [] artefact = Artefact( name=name, @@ -182,7 +180,6 @@ def gen_artefact( bug_link=bug_link, due_date=due_date, reviewers=reviewers, - bundled_builds=bundled_builds, ) self._add_object(artefact) return artefact From a631e113f9d6e44803b93fa13ba2c4db8d833c92 Mon Sep 17 00:00:00 2001 From: Raul Almeida Date: Thu, 23 Jul 2026 11:34:16 -0300 Subject: [PATCH 02/21] tests: solutions request behaviour --- backend/schemata/openapi.json | 81 +---- .../controllers/artefacts/artefacts.py | 2 + .../controllers/artefacts/models.py | 2 + .../controllers/test_executions/models.py | 22 +- .../controllers/test_executions/start_test.py | 8 +- .../controllers/artefacts/test_artefacts.py | 52 +++ backend/tests/controllers/auth/test_saml.py | 2 +- .../test_executions/test_reruns.py | 1 + .../test_executions/test_start_test.py | 129 ++++--- backend/tests/data_access/test_models.py | 75 +++- backend/tests/data_generator.py | 4 + ...b5953e_replace_solution_specific_fields.py | 341 ++++++++++++++++++ 12 files changed, 560 insertions(+), 159 deletions(-) create mode 100644 backend/tests/migrations/test_8202f7b5953e_replace_solution_specific_fields.py diff --git a/backend/schemata/openapi.json b/backend/schemata/openapi.json index fd9da3427..076cb028f 100644 --- a/backend/schemata/openapi.json +++ b/backend/schemata/openapi.json @@ -5668,13 +5668,6 @@ }, "type": "array", "title": "Test Executions" - }, - "bundled_in": { - "items": { - "$ref": "#/components/schemas/ArtefactMinimalResponse" - }, - "type": "array", - "title": "Bundled In" } }, "type": "object", @@ -6299,34 +6292,6 @@ "title": "ArtefactMatchingRuleResponse", "description": "Artefact matching rule with associated teams" }, - "ArtefactMinimalResponse": { - "properties": { - "id": { - "type": "integer", - "title": "Id" - }, - "name": { - "type": "string", - "title": "Name" - }, - "version": { - "type": "string", - "title": "Version" - }, - "family": { - "type": "string", - "title": "Family" - } - }, - "type": "object", - "required": [ - "id", - "name", - "version", - "family" - ], - "title": "ArtefactMinimalResponse" - }, "ArtefactPatch": { "properties": { "status": { @@ -6382,20 +6347,17 @@ ], "title": "Jira Issue" }, - "bundled_builds": { + "attributes": { "anyOf": [ { - "items": { - "type": "integer" - }, - "type": "array" + "additionalProperties": true, + "type": "object" }, { "type": "null" } ], - "title": "Bundled Builds", - "description": "List of ArtefactBuild IDs to bundle with this artefact" + "title": "Attributes" }, "assignee_id": { "anyOf": [ @@ -6530,6 +6492,11 @@ "type": "string", "title": "Comment" }, + "attributes": { + "additionalProperties": true, + "type": "object", + "title": "Attributes" + }, "archived": { "type": "boolean", "title": "Archived" @@ -6581,13 +6548,6 @@ "type": "integer", "title": "Completed Environment Reviews Count" }, - "bundled_builds": { - "items": { - "$ref": "#/components/schemas/ArtefactBuildMinimalResponse" - }, - "type": "array", - "title": "Bundled Builds" - }, "assignee": { "anyOf": [ { @@ -6621,6 +6581,7 @@ "family", "status", "comment", + "attributes", "archived", "reviewers", "due_date", @@ -7875,8 +7836,8 @@ "anyOf": [ { "type": "integer", - "maximum": 1000000, - "minimum": -1000000 + "maximum": 1000000.0, + "minimum": -1000000.0 }, { "type": "null" @@ -8518,20 +8479,14 @@ "const": "solution", "title": "Family" }, - "track": { - "type": "string", - "title": "Track", - "description": "Solution release track being tested. Tracks represent different versions or streams of the solution. Examples: 'latest' (default), version-based tracks like '1.0', '2.0'. Use 'latest' if unsure." - }, - "source": { - "type": "string", - "maxLength": 200, - "title": "Source", - "description": "Source identifier for the solution. This identifies the packaging source or origin of the solution. Examples: 'ppa:team/ppa-name', 'custom-repo', 'internal-source'." + "attributes": { + "additionalProperties": true, + "type": "object", + "title": "Attributes" }, "execution_stage": { "$ref": "#/components/schemas/SolutionStage", - "description": "Distribution channel/risk level of the solution being tested. Options: 'edge' (cutting-edge updates), 'beta' (pre-release), 'candidate' (release candidate), 'stable' (production). Choose based on where the solution currently resides in the release pipeline." + "description": "Promotion stage of the solution being tested." } }, "type": "object", @@ -8542,8 +8497,6 @@ "environment", "test_plan", "family", - "track", - "source", "execution_stage" ], "title": "StartSolutionTestExecutionRequest" diff --git a/backend/test_observer/controllers/artefacts/artefacts.py b/backend/test_observer/controllers/artefacts/artefacts.py index d00228c88..273044c24 100644 --- a/backend/test_observer/controllers/artefacts/artefacts.py +++ b/backend/test_observer/controllers/artefacts/artefacts.py @@ -259,6 +259,8 @@ def patch_artefact( artefact.comment = request.comment if "jira_issue" in request.model_fields_set: artefact.jira_issue = request.jira_issue + if "attributes" in request.model_fields_set and request.attributes is not None: + artefact.attributes = request.attributes reviewer_ids_set = hasattr(request, "reviewer_ids") and "reviewer_ids" in request.model_fields_set reviewer_emails_set = hasattr(request, "reviewer_emails") and "reviewer_emails" in request.model_fields_set diff --git a/backend/test_observer/controllers/artefacts/models.py b/backend/test_observer/controllers/artefacts/models.py index 9a8f45aed..d71ebc70a 100644 --- a/backend/test_observer/controllers/artefacts/models.py +++ b/backend/test_observer/controllers/artefacts/models.py @@ -71,6 +71,7 @@ class ArtefactResponse(BaseModel): family: str status: ArtefactStatus comment: str + attributes: dict[str, Any] archived: bool reviewers: list[ReviewerResponse] due_date: date | None @@ -154,6 +155,7 @@ class ArtefactPatch(BaseModel): stage: StageName | None = None comment: str | None = None jira_issue: str | None = None + attributes: dict[str, Any] | None = None assignee_id: int | None = Field( default=None, deprecated=True, diff --git a/backend/test_observer/controllers/test_executions/models.py b/backend/test_observer/controllers/test_executions/models.py index fc01f4ee8..9c5ac944b 100644 --- a/backend/test_observer/controllers/test_executions/models.py +++ b/backend/test_observer/controllers/test_executions/models.py @@ -15,7 +15,7 @@ from datetime import datetime from enum import StrEnum -from typing import Annotated, ClassVar, Literal, Self +from typing import Annotated, Any, ClassVar, Literal, Self from pydantic import ( AliasPath, @@ -285,24 +285,8 @@ class StartImageTestExecutionRequest(_StartTestExecutionRequest): class StartSolutionTestExecutionRequest(_StartTestExecutionRequest): family: Literal[FamilyName.solution] - track: str = Field( - description="Solution release track being tested. " - "Tracks represent different versions or streams of the solution. " - "Examples: 'latest' (default), version-based tracks like '1.0', '2.0'. " - "Use 'latest' if unsure." - ) - source: str = Field( - max_length=200, - description="Source identifier for the solution. " - "This identifies the packaging source or origin of the solution. " - "Examples: 'ppa:team/ppa-name', 'custom-repo', 'internal-source'.", - ) - execution_stage: SolutionStage = Field( - description="Distribution channel/risk level of the solution being tested. " - "Options: 'edge' (cutting-edge updates), 'beta' (pre-release), " - "'candidate' (release candidate), 'stable' (production). " - "Choose based on where the solution currently resides in the release pipeline." - ) + attributes: dict[str, Any] = Field(default_factory=dict) + execution_stage: SolutionStage = Field(description="Promotion stage of the solution being tested.") class C3TestResultStatus(StrEnum): diff --git a/backend/test_observer/controllers/test_executions/start_test.py b/backend/test_observer/controllers/test_executions/start_test.py index f2d39c42b..707447333 100644 --- a/backend/test_observer/controllers/test_executions/start_test.py +++ b/backend/test_observer/controllers/test_executions/start_test.py @@ -347,14 +347,8 @@ def create_artefact(self) -> None: "image_url": str(self.request.image_url), } - # In other families, a single artefact will progress through stages, - # i.e. move from edge to stable. Solutions are different. Different stages are treated - # as different artefacts for solutions. case StartSolutionTestExecutionRequest(): - filter_kwargs["track"] = self.request.track - filter_kwargs["source"] = self.request.source - filter_kwargs["stage"] = self.request.execution_stage - creation_kwargs = {} + creation_kwargs["attributes"] = self.request.attributes self.artefact = get_or_create(self.db, Artefact, filter_kwargs=filter_kwargs, creation_kwargs=creation_kwargs) diff --git a/backend/tests/controllers/artefacts/test_artefacts.py b/backend/tests/controllers/artefacts/test_artefacts.py index 90f5156b1..339a4da65 100644 --- a/backend/tests/controllers/artefacts/test_artefacts.py +++ b/backend/tests/controllers/artefacts/test_artefacts.py @@ -228,6 +228,18 @@ def test_get_artefact(test_client: TestClient, generator: DataGenerator): _assert_get_artefact_response(response.json(), a) +def test_get_artefact_includes_attributes(test_client: TestClient, generator: DataGenerator): + artefact = generator.gen_artefact(attributes={"foo": "bar"}) + + response = make_authenticated_request( + lambda: test_client.get(f"/v1/artefacts/{artefact.id}"), + Permission.view_artefact, + ) + + assert response.status_code == 200 + assert response.json()["attributes"] == {"foo": "bar"} + + def test_get_artefact_environment_reviews_counts_only_latest_build(test_client: TestClient, generator: DataGenerator): a = generator.gen_artefact(StageName.beta) ab = generator.gen_artefact_build(artefact=a, revision=1) @@ -330,6 +342,45 @@ def test_artefact_signoff_disallow_reject(test_client: TestClient, test_executio assert response.status_code == 400 +def test_patch_artefact_updates_attributes(test_client: TestClient, generator: DataGenerator): + artefact = generator.gen_artefact(attributes={"old": "value"}) + + response = make_authenticated_request( + lambda: test_client.patch( + f"/v1/artefacts/{artefact.id}", + json={"attributes": {"new": "value", "nested": {"key": "value"}}}, + ), + Permission.change_artefact, + ) + + assert response.status_code == 200 + assert response.json()["attributes"] == {"new": "value", "nested": {"key": "value"}} + + get_response = make_authenticated_request( + lambda: test_client.get(f"/v1/artefacts/{artefact.id}"), + Permission.view_artefact, + ) + assert get_response.json()["attributes"] == {"new": "value", "nested": {"key": "value"}} + + +def test_patch_artefact_without_attributes_preserves_existing_attributes( + test_client: TestClient, + generator: DataGenerator, +): + artefact = generator.gen_artefact(attributes={"keep": "me"}) + + response = make_authenticated_request( + lambda: test_client.patch( + f"/v1/artefacts/{artefact.id}", + json={"comment": "updated comment"}, + ), + Permission.change_artefact, + ) + + assert response.status_code == 200 + assert response.json()["attributes"] == {"keep": "me"} + + def test_artefact_signoff_ignore_old_build_on_approve(test_client: TestClient, generator: DataGenerator): artefact = generator.gen_artefact(StageName.candidate) build1 = generator.gen_artefact_build(artefact, revision=1) @@ -1158,6 +1209,7 @@ def _assert_get_artefact_response(response: dict[str, Any], artefact: Artefact) "comment": artefact.comment, "archived": artefact.archived, "family": artefact.family, + "attributes": artefact.attributes, "assignee": assignee, "reviewers": [], "due_date": (artefact.due_date.strftime("%Y-%m-%d") if artefact.due_date else None), diff --git a/backend/tests/controllers/auth/test_saml.py b/backend/tests/controllers/auth/test_saml.py index 85d70a6be..e96c1032e 100644 --- a/backend/tests/controllers/auth/test_saml.py +++ b/backend/tests/controllers/auth/test_saml.py @@ -37,7 +37,7 @@ # The SP redirects to the IdP's publicly configured URL (SSP_BASE_URL_PATH). # That address is reachable when tests run on the host, but not from inside the # api container (where the IdP is the 'saml-idp' compose service on port 80). -IDP_PUBLIC_NETLOC = "localhost:8080" +IDP_PUBLIC_NETLOC = "localhost:8081" IDP_INTERNAL_URL = "http://saml-idp" diff --git a/backend/tests/controllers/test_executions/test_reruns.py b/backend/tests/controllers/test_executions/test_reruns.py index aa0aca933..822262268 100644 --- a/backend/tests/controllers/test_executions/test_reruns.py +++ b/backend/tests/controllers/test_executions/test_reruns.py @@ -166,6 +166,7 @@ def test_execution_to_pending_rerun(test_execution: TestExecution, priority: int "stage": test_execution.artefact_build.artefact.stage, "status": test_execution.artefact_build.artefact.status.name, "comment": test_execution.artefact_build.artefact.comment, + "attributes": test_execution.artefact_build.artefact.attributes, "archived": test_execution.artefact_build.artefact.archived, "assignee": ( test_execution.artefact_build.artefact.reviewers[0] diff --git a/backend/tests/controllers/test_executions/test_start_test.py b/backend/tests/controllers/test_executions/test_start_test.py index 872a85150..b9ed2c761 100644 --- a/backend/tests/controllers/test_executions/test_start_test.py +++ b/backend/tests/controllers/test_executions/test_start_test.py @@ -121,19 +121,66 @@ def execute_helper(data: dict[str, Any]) -> Response: "family": "solution", "name": "ubuntu-pro-fips", "version": "1.2.3", - "track": "22.04", - "source": "ppa:ubuntu-pro/fips", "arch": "amd64", - "execution_stage": SolutionStage.stable, + "execution_stage": SolutionStage.beta, "environment": "test-lab", "ci_link": "http://localhost", "test_plan": "fips test plan", } +def test_start_test_creates_new_solution_with_attributes( + execute: Execute, + db_session: Session, +) -> None: + attributes = {"foo": "bar", "nested": {"key": "value"}} + + response = execute({**solution_test_request, "attributes": attributes}) + + assert response.status_code == 200 + test_execution = db_session.get(TestExecution, response.json()["id"]) + assert test_execution is not None + assert test_execution.artefact_build.artefact.attributes == attributes + + +def test_start_test_without_attributes_creates_new_solution_with_empty_attributes( + execute: Execute, + db_session: Session, +) -> None: + response = execute(solution_test_request) + + assert response.status_code == 200 + test_execution = db_session.get(TestExecution, response.json()["id"]) + assert test_execution is not None + assert test_execution.artefact_build.artefact.attributes == {} + + +def test_start_test_on_existing_solution_does_not_change_attributes( + execute: Execute, + db_session: Session, +) -> None: + """Attributes are applied only on creation; resubmitting for an existing solution + with different attributes leaves the stored attributes unchanged.""" + original = {"foo": "bar"} + response = execute({**solution_test_request, "attributes": original}) + assert response.status_code == 200 + te1 = db_session.get(TestExecution, response.json()["id"]) + assert te1 is not None + artefact_id = te1.artefact_build.artefact_id + + response = execute({**solution_test_request, "attributes": {"foo": "changed"}, "ci_link": "http://localhost/other"}) + assert response.status_code == 200 + te2 = db_session.get(TestExecution, response.json()["id"]) + assert te2 is not None + db_session.expire_all() + + assert te2.artefact_build.artefact_id == artefact_id + assert te2.artefact_build.artefact.attributes == original + + @pytest.mark.parametrize( "start_request", - [snap_test_request, deb_test_request, charm_test_request, image_test_request, solution_test_request], + [snap_test_request, deb_test_request, charm_test_request, image_test_request], ) class TestFamilyIndependentTests: def test_starts_a_test(self, execute: Execute, start_request: dict[str, Any]): @@ -895,10 +942,7 @@ def test_image_same_sha_reuses_existing_artefact(execute: Execute, db_session: S [ "name", "version", - "track", - "source", "arch", - "execution_stage", "environment", "test_plan", ], @@ -994,16 +1038,6 @@ def test_validates_stage_for_charms(execute: Execute, invalid_stage: StageName): assert response.status_code == 422 -@pytest.mark.parametrize( - "invalid_stage", - set(StageName) - set(SolutionStage), -) -def test_validates_stage_for_solutions(execute: Execute, invalid_stage: StageName): - response = execute({**solution_test_request, "execution_stage": invalid_stage}) - - assert response.status_code == 422 - - def test_snap_branch_is_part_of_uniqueness(execute: Execute, db_session: Session): response = execute(snap_test_request) te1 = db_session.get(TestExecution, response.json()["id"]) @@ -1060,63 +1094,24 @@ def test_deb_with_source_and_stage_fails(execute: Execute): assert response.status_code == 422 -def test_solution_includes_track_source_and_stage(execute: Execute, db_session: Session): - """Verify that a solution test execution creates an artefact with track, source, and stage.""" - response = execute(solution_test_request) - assert response.status_code == 200 - - test_execution = db_session.get(TestExecution, response.json()["id"]) - assert test_execution - - artefact = test_execution.artefact_build.artefact - assert artefact.name == solution_test_request["name"] - assert artefact.version == solution_test_request["version"] - assert artefact.track == solution_test_request["track"] - assert artefact.source == solution_test_request["source"] - assert artefact.stage == solution_test_request["execution_stage"] - assert artefact.family == FamilyName.solution - - -def test_solution_track_source_stage_are_part_of_uniqueness(execute: Execute, db_session: Session): - """Verify that changing track, source, or stage creates a different artefact.""" +def test_solution_same_name_and_version_reuses_artefact(execute: Execute, db_session: Session): + """Solutions are identified by (name, version); resubmitting the same name and + version resolves to the same artefact, while a new version creates a new one.""" response = execute(solution_test_request) te1 = db_session.get(TestExecution, response.json()["id"]) + assert te1 is not None - # Different track should create a new artefact - request_different_track = { - **solution_test_request, - "track": "24.04", - "ci_link": "http://localhost/1", - } - response = execute(request_different_track) + response = execute({**solution_test_request, "ci_link": "http://localhost/1"}) te2 = db_session.get(TestExecution, response.json()["id"]) + assert te2 is not None + assert te2.artefact_build.artefact_id == te1.artefact_build.artefact_id - assert te1 and te2 - assert te1.artefact_build.artefact_id != te2.artefact_build.artefact_id - - # Different source should create a new artefact - request_different_source = { - **solution_test_request, - "source": "ppa:ubuntu-pro/other", - "ci_link": "http://localhost/2", - } - response = execute(request_different_source) + response = execute({**solution_test_request, "version": "2.0.0", "ci_link": "http://localhost/2"}) te3 = db_session.get(TestExecution, response.json()["id"]) + assert te3 is not None + assert te3.artefact_build.artefact_id != te1.artefact_build.artefact_id - assert te1 and te3 - assert te1.artefact_build.artefact_id != te3.artefact_build.artefact_id - - # Different stage should create a new artefact - request_different_stage = { - **solution_test_request, - "execution_stage": "beta", - "ci_link": "http://localhost/3", - } - response = execute(request_different_stage) - te4 = db_session.get(TestExecution, response.json()["id"]) - - assert te1 and te4 - assert te1.artefact_build.artefact_id != te4.artefact_build.artefact_id + assert db_session.query(Artefact).filter(Artefact.name == solution_test_request["name"]).count() == 2 def test_charm_assigned_to_charm_team_reviewer(db_session: Session, execute: Execute, generator: DataGenerator): diff --git a/backend/tests/data_access/test_models.py b/backend/tests/data_access/test_models.py index c259eb08d..f4740a80e 100644 --- a/backend/tests/data_access/test_models.py +++ b/backend/tests/data_access/test_models.py @@ -14,9 +14,12 @@ # SPDX-License-Identifier: AGPL-3.0-only import pytest +from sqlalchemy.exc import IntegrityError +from sqlalchemy.orm import Session from test_observer.data_access.models import Issue -from test_observer.data_access.models_enums import IssueSource +from test_observer.data_access.models_enums import FamilyName, IssueSource, StageName +from tests.data_generator import DataGenerator @pytest.mark.parametrize( @@ -52,3 +55,73 @@ def test_issue_url( assert expected is None else: assert result == expected + + +def test_solutions_with_same_name_and_version_are_unique(generator: DataGenerator, db_session: Session) -> None: + generator.gen_artefact(family=FamilyName.solution, name="solution", version="1.0") + + with pytest.raises(IntegrityError): + generator.gen_artefact(family=FamilyName.solution, name="solution", version="1.0") + db_session.rollback() + + +def test_solutions_with_same_name_and_different_versions_are_allowed(generator: DataGenerator) -> None: + first = generator.gen_artefact(family=FamilyName.solution, name="solution", version="1.0") + second = generator.gen_artefact(family=FamilyName.solution, name="solution", version="2.0") + + assert first.id != second.id + + +def test_solutions_with_same_version_and_different_names_are_allowed(generator: DataGenerator) -> None: + first = generator.gen_artefact(family=FamilyName.solution, name="solution-a", version="1.0") + second = generator.gen_artefact(family=FamilyName.solution, name="solution-b", version="1.0") + + assert first.id != second.id + + +def test_solutions_with_different_name_and_version_are_allowed(generator: DataGenerator) -> None: + first = generator.gen_artefact(family=FamilyName.solution, name="solution-a", version="1.0") + second = generator.gen_artefact(family=FamilyName.solution, name="solution-b", version="2.0") + + assert first.id != second.id + + +def test_solution_unique_constraint_ignores_source_track_and_stage( + generator: DataGenerator, db_session: Session +) -> None: + generator.gen_artefact( + family=FamilyName.solution, + name="solution", + version="1.0", + source="first-source", + track="first-track", + stage=StageName.beta, + ) + + with pytest.raises(IntegrityError): + generator.gen_artefact( + family=FamilyName.solution, + name="solution", + version="1.0", + source="second-source", + track="second-track", + stage=StageName.stable, + ) + db_session.rollback() + + +def test_snap_name_and_version_duplicates_are_not_blocked_by_solution_constraint(generator: DataGenerator) -> None: + first = generator.gen_artefact( + family=FamilyName.snap, + name="core", + version="1.0", + track="latest", + ) + second = generator.gen_artefact( + family=FamilyName.snap, + name="core", + version="1.0", + track="other-track", + ) + + assert first.id != second.id diff --git a/backend/tests/data_generator.py b/backend/tests/data_generator.py index 2ce9621e0..bf3b95b10 100644 --- a/backend/tests/data_generator.py +++ b/backend/tests/data_generator.py @@ -14,6 +14,7 @@ # SPDX-License-Identifier: AGPL-3.0-only from datetime import date, datetime +from typing import Any from sqlalchemy.orm import Session @@ -147,6 +148,7 @@ def gen_artefact( bug_link: str = "", due_date: date | None = None, reviewers: list[User] | None = None, + attributes: dict[str, Any] | None = None, ) -> Artefact: family = FamilyName(family) @@ -162,6 +164,7 @@ def gen_artefact( created_at = created_at or datetime.utcnow() reviewers = reviewers or [] + attributes = attributes or {} artefact = Artefact( name=name, @@ -180,6 +183,7 @@ def gen_artefact( bug_link=bug_link, due_date=due_date, reviewers=reviewers, + attributes=attributes, ) self._add_object(artefact) return artefact diff --git a/backend/tests/migrations/test_8202f7b5953e_replace_solution_specific_fields.py b/backend/tests/migrations/test_8202f7b5953e_replace_solution_specific_fields.py new file mode 100644 index 000000000..d8bed2eec --- /dev/null +++ b/backend/tests/migrations/test_8202f7b5953e_replace_solution_specific_fields.py @@ -0,0 +1,341 @@ +# Copyright 2026 Canonical Ltd. +# +# This program is free software: you can redistribute it and/or modify +# it under the terms of the GNU Affero General Public License version 3, as +# published by the Free Software Foundation. +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU Affero General Public License for more details. +# You should have received a copy of the GNU Affero General Public License +# along with this program. If not, see . +# +# SPDX-FileCopyrightText: Copyright 2026 Canonical Ltd. +# SPDX-License-Identifier: AGPL-3.0-only + +"""Tests for replacing bundled build fields with generic artefact attributes.""" + +from collections.abc import Generator +from urllib.parse import urlparse, urlunparse + +import pytest +from alembic import command +from alembic.config import Config +from sqlalchemy import Engine, create_engine, text +from sqlalchemy.engine import Connection +from sqlalchemy_utils import create_database, database_exists, drop_database # type: ignore[import-untyped] + +PREVIOUS_REV = "eba1d1c92dba" +TARGET_REV = "8202f7b5953e" + + +@pytest.fixture +def migration_context(db_url: str) -> Generator[tuple[Engine, Config], None, None]: + parsed = urlparse(db_url) + test_db_url = urlunparse( + (parsed.scheme, parsed.netloc, "/test_migration_solution_attrs", parsed.params, parsed.query, parsed.fragment) + ) + + if database_exists(test_db_url): + drop_database(test_db_url) + + create_database(test_db_url) + + try: + engine = create_engine(test_db_url) + alembic_config = Config("alembic.ini") + alembic_config.set_main_option("sqlalchemy.url", test_db_url) + + yield engine, alembic_config + finally: + engine.dispose() + if database_exists(test_db_url): + drop_database(test_db_url) + + +def _insert_artefact( + conn: Connection, + name: str, + bundled_builds_hash: str | None = None, + attributes: str | None = None, +) -> int: + optional_column = ", attributes" if attributes is not None else ", bundled_builds_hash" + optional_value = ", CAST(:attributes AS jsonb)" if attributes is not None else ", :bundled_builds_hash" + result = conn.execute( + text(f""" + INSERT INTO artefact ( + name, version, stage, family, status, archived, bug_link, comment, + store, branch, track, series, repo, source, os, release, sha256, owner, image_url, + created_at, updated_at{optional_column} + ) + VALUES ( + :name, '1.0', 'stable', 'solution', 'UNDECIDED', false, '', '', + '', '', 'latest', '', '', 'source', '', '', '', '', '', + NOW(), NOW(){optional_value} + ) + RETURNING id + """), + { + "name": name, + "bundled_builds_hash": bundled_builds_hash, + "attributes": attributes, + }, + ) + return result.scalar_one() + + +def _insert_artefact_build(conn: Connection, artefact_id: int, architecture: str = "amd64") -> int: + result = conn.execute( + text(""" + INSERT INTO artefact_build (architecture, revision, artefact_id, created_at, updated_at) + VALUES (:architecture, NULL, :artefact_id, NOW(), NOW()) + RETURNING id + """), + {"architecture": architecture, "artefact_id": artefact_id}, + ) + return result.scalar_one() + + +def _insert_association(conn: Connection, artefact_id: int, artefact_build_id: int) -> None: + conn.execute( + text(""" + INSERT INTO artefact_bundled_builds_association (artefact_id, artefact_build_id) + VALUES (:artefact_id, :artefact_build_id) + """), + {"artefact_id": artefact_id, "artefact_build_id": artefact_build_id}, + ) + + +def _attribute_text(engine: Engine, artefact_id: int, key: str) -> str | None: + with engine.connect() as conn: + return conn.execute( + text("SELECT attributes::jsonb ->> :key FROM artefact WHERE id = :artefact_id"), + {"artefact_id": artefact_id, "key": key}, + ).scalar_one() + + +def _attribute_key_exists(engine: Engine, artefact_id: int, key: str) -> bool: + with engine.connect() as conn: + return conn.execute( + text("SELECT attributes::jsonb ? :key FROM artefact WHERE id = :artefact_id"), + {"artefact_id": artefact_id, "key": key}, + ).scalar_one() + + +def _bundled_build_ids(engine: Engine, artefact_id: int) -> list[int]: + with engine.connect() as conn: + return list( + conn.execute( + text(""" + SELECT jsonb_array_elements_text(attributes::jsonb -> 'bundled_builds')::int + FROM artefact + WHERE id = :artefact_id + """), + {"artefact_id": artefact_id}, + ).scalars() + ) + + +def test_upgrade_preserves_empty_attributes(migration_context: tuple[Engine, Config]) -> None: + engine, alembic_config = migration_context + command.upgrade(alembic_config, PREVIOUS_REV) + with engine.begin() as conn: + artefact_id = _insert_artefact(conn, "solution-empty") + + command.upgrade(alembic_config, TARGET_REV) + + with engine.connect() as conn: + assert ( + conn.execute( + text("SELECT attributes::jsonb FROM artefact WHERE id = :id"), {"id": artefact_id} + ).scalar_one() + == {} + ) + + +def test_upgrade_copies_hash_and_associations(migration_context: tuple[Engine, Config]) -> None: + engine, alembic_config = migration_context + command.upgrade(alembic_config, PREVIOUS_REV) + with engine.begin() as conn: + artefact_id = _insert_artefact(conn, "solution-with-both", bundled_builds_hash="hash-value") + build_id = _insert_artefact_build(conn, artefact_id) + _insert_association(conn, artefact_id, build_id) + + command.upgrade(alembic_config, TARGET_REV) + + assert _attribute_text(engine, artefact_id, "bundled_builds_hash") == "hash-value" + assert _bundled_build_ids(engine, artefact_id) == [build_id] + + +def test_upgrade_copies_only_hash_when_no_associations(migration_context: tuple[Engine, Config]) -> None: + engine, alembic_config = migration_context + command.upgrade(alembic_config, PREVIOUS_REV) + with engine.begin() as conn: + artefact_id = _insert_artefact(conn, "solution-with-hash", bundled_builds_hash="hash-only") + + command.upgrade(alembic_config, TARGET_REV) + + assert _attribute_text(engine, artefact_id, "bundled_builds_hash") == "hash-only" + assert not _attribute_key_exists(engine, artefact_id, "bundled_builds") + + +def test_upgrade_copies_only_associations_when_hash_null(migration_context: tuple[Engine, Config]) -> None: + engine, alembic_config = migration_context + command.upgrade(alembic_config, PREVIOUS_REV) + with engine.begin() as conn: + artefact_id = _insert_artefact(conn, "solution-with-assoc") + build_id = _insert_artefact_build(conn, artefact_id) + _insert_association(conn, artefact_id, build_id) + + command.upgrade(alembic_config, TARGET_REV) + + assert not _attribute_key_exists(engine, artefact_id, "bundled_builds_hash") + assert _bundled_build_ids(engine, artefact_id) == [build_id] + + +def test_upgrade_copies_multiple_bundled_builds_in_ascending_order(migration_context: tuple[Engine, Config]) -> None: + engine, alembic_config = migration_context + command.upgrade(alembic_config, PREVIOUS_REV) + with engine.begin() as conn: + artefact_id = _insert_artefact(conn, "solution-with-many") + first_build_id = _insert_artefact_build(conn, artefact_id, architecture="amd64") + second_build_id = _insert_artefact_build(conn, artefact_id, architecture="arm64") + _insert_association(conn, artefact_id, second_build_id) + _insert_association(conn, artefact_id, first_build_id) + + command.upgrade(alembic_config, TARGET_REV) + + assert _bundled_build_ids(engine, artefact_id) == [first_build_id, second_build_id] + + +def test_downgrade_restores_empty_attributes(migration_context: tuple[Engine, Config]) -> None: + engine, alembic_config = migration_context + command.upgrade(alembic_config, TARGET_REV) + with engine.begin() as conn: + artefact_id = _insert_artefact(conn, "solution-downgrade-empty", attributes="{}") + + command.downgrade(alembic_config, PREVIOUS_REV) + + with engine.connect() as conn: + bundled_hash = conn.execute( + text("SELECT bundled_builds_hash FROM artefact WHERE id = :id"), + {"id": artefact_id}, + ).scalar_one() + association_count = conn.execute( + text("SELECT count(*) FROM artefact_bundled_builds_association WHERE artefact_id = :id"), + {"id": artefact_id}, + ).scalar_one() + assert bundled_hash is None + assert association_count == 0 + + +def test_downgrade_restores_hash_and_associations(migration_context: tuple[Engine, Config]) -> None: + engine, alembic_config = migration_context + command.upgrade(alembic_config, TARGET_REV) + with engine.begin() as conn: + artefact_id = _insert_artefact(conn, "solution-downgrade-both", attributes="{}") + first_build_id = _insert_artefact_build(conn, artefact_id, architecture="amd64") + second_build_id = _insert_artefact_build(conn, artefact_id, architecture="arm64") + conn.execute( + text(""" + UPDATE artefact + SET attributes = jsonb_build_object( + 'bundled_builds_hash', 'restored-hash', + 'bundled_builds', jsonb_build_array(CAST(:first_build_id AS int), CAST(:second_build_id AS int)) + ) + WHERE id = :artefact_id + """), + { + "artefact_id": artefact_id, + "first_build_id": first_build_id, + "second_build_id": second_build_id, + }, + ) + + command.downgrade(alembic_config, PREVIOUS_REV) + + with engine.connect() as conn: + bundled_hash = conn.execute( + text("SELECT bundled_builds_hash FROM artefact WHERE id = :id"), + {"id": artefact_id}, + ).scalar_one() + association_ids = list( + conn.execute( + text(""" + SELECT artefact_build_id + FROM artefact_bundled_builds_association + WHERE artefact_id = :id + ORDER BY artefact_build_id + """), + {"id": artefact_id}, + ).scalars() + ) + assert bundled_hash == "restored-hash" + assert association_ids == [first_build_id, second_build_id] + + +def test_upgrade_schema_changes(migration_context: tuple[Engine, Config]) -> None: + engine, alembic_config = migration_context + command.upgrade(alembic_config, TARGET_REV) + + with engine.connect() as conn: + attributes_column = conn.execute( + text(""" + SELECT is_nullable, column_default + FROM information_schema.columns + WHERE table_name = 'artefact' AND column_name = 'attributes' + """) + ).fetchone() + association_table = conn.execute( + text(""" + SELECT table_name + FROM information_schema.tables + WHERE table_name = 'artefact_bundled_builds_association' + """) + ).fetchone() + bundled_hash_column = conn.execute( + text(""" + SELECT column_name + FROM information_schema.columns + WHERE table_name = 'artefact' AND column_name = 'bundled_builds_hash' + """) + ).fetchone() + + assert attributes_column is not None + assert attributes_column[0] == "NO" + assert "'{}'" in attributes_column[1] + assert association_table is None + assert bundled_hash_column is None + + +def test_downgrade_schema_changes(migration_context: tuple[Engine, Config]) -> None: + engine, alembic_config = migration_context + command.upgrade(alembic_config, TARGET_REV) + command.downgrade(alembic_config, PREVIOUS_REV) + + with engine.connect() as conn: + attributes_column = conn.execute( + text(""" + SELECT column_name + FROM information_schema.columns + WHERE table_name = 'artefact' AND column_name = 'attributes' + """) + ).fetchone() + association_table = conn.execute( + text(""" + SELECT table_name + FROM information_schema.tables + WHERE table_name = 'artefact_bundled_builds_association' + """) + ).fetchone() + bundled_hash_column = conn.execute( + text(""" + SELECT column_name + FROM information_schema.columns + WHERE table_name = 'artefact' AND column_name = 'bundled_builds_hash' + """) + ).fetchone() + + assert attributes_column is None + assert association_table is not None + assert bundled_hash_column is not None From c54b77888b6f90e1887d74c283c79daeb325bad7 Mon Sep 17 00:00:00 2001 From: Raul Almeida Date: Thu, 23 Jul 2026 12:08:52 -0300 Subject: [PATCH 03/21] enh: copilot review --- backend/test_observer/controllers/artefacts/builds.py | 1 - backend/test_observer/controllers/artefacts/models.py | 9 --------- 2 files changed, 10 deletions(-) diff --git a/backend/test_observer/controllers/artefacts/builds.py b/backend/test_observer/controllers/artefacts/builds.py index 3dfd35189..b9d162d0b 100644 --- a/backend/test_observer/controllers/artefacts/builds.py +++ b/backend/test_observer/controllers/artefacts/builds.py @@ -43,7 +43,6 @@ def get_artefact_builds( artefact: Artefact = Depends( ArtefactRetriever( selectinload(Artefact.builds).selectinload(ArtefactBuild.test_executions).options(*TEST_EXECUTION_OPTIONS), - selectinload(Artefact.builds), ) ), ): diff --git a/backend/test_observer/controllers/artefacts/models.py b/backend/test_observer/controllers/artefacts/models.py index d71ebc70a..75b887aa6 100644 --- a/backend/test_observer/controllers/artefacts/models.py +++ b/backend/test_observer/controllers/artefacts/models.py @@ -211,15 +211,6 @@ class ArtefactBuildMinimalResponse(BaseModel): revision: int | None -class ArtefactMinimalResponse(BaseModel): - model_config = ConfigDict(from_attributes=True) - - id: int - name: str - version: str - family: str - - class ArtefactSearchResponse(BaseModel): artefacts: list[str] count: int From 7f249eeb99614e737aa2f2d2b784cc3baa82a558 Mon Sep 17 00:00:00 2001 From: Raul Almeida Date: Thu, 23 Jul 2026 12:32:00 -0300 Subject: [PATCH 04/21] fix: undo local-only change to test_saml --- backend/tests/controllers/auth/test_saml.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/backend/tests/controllers/auth/test_saml.py b/backend/tests/controllers/auth/test_saml.py index e96c1032e..85d70a6be 100644 --- a/backend/tests/controllers/auth/test_saml.py +++ b/backend/tests/controllers/auth/test_saml.py @@ -37,7 +37,7 @@ # The SP redirects to the IdP's publicly configured URL (SSP_BASE_URL_PATH). # That address is reachable when tests run on the host, but not from inside the # api container (where the IdP is the 'saml-idp' compose service on port 80). -IDP_PUBLIC_NETLOC = "localhost:8081" +IDP_PUBLIC_NETLOC = "localhost:8080" IDP_INTERNAL_URL = "http://saml-idp" From e831c14001be095f1a49e3998ef5443cad159689 Mon Sep 17 00:00:00 2001 From: Raul Almeida Date: Thu, 23 Jul 2026 13:02:41 -0300 Subject: [PATCH 05/21] enh: copilot review --- ..._replace_solution_specific_fields_with_.py | 38 +++++++++++++ .../controllers/artefacts/artefacts.py | 1 + backend/test_observer/data_access/models.py | 4 +- .../controllers/artefacts/test_artefacts.py | 21 +++++++ ...b5953e_replace_solution_specific_fields.py | 55 ++++++++++++++++++- 5 files changed, 113 insertions(+), 6 deletions(-) diff --git a/backend/migrations/versions/2026_07_21_1311-8202f7b5953e_replace_solution_specific_fields_with_.py b/backend/migrations/versions/2026_07_21_1311-8202f7b5953e_replace_solution_specific_fields_with_.py index f8543a503..6126c9e64 100644 --- a/backend/migrations/versions/2026_07_21_1311-8202f7b5953e_replace_solution_specific_fields_with_.py +++ b/backend/migrations/versions/2026_07_21_1311-8202f7b5953e_replace_solution_specific_fields_with_.py @@ -19,6 +19,7 @@ def upgrade() -> None: op.add_column("artefact", sa.Column("attributes", postgresql.JSONB(), server_default="{}", nullable=False)) + _assert_no_duplicate_solutions(["name", "version"]) op.drop_index("unique_solution", table_name="artefact", postgresql_where="(family = 'solution'::familyname)") op.create_index( "unique_solution", "artefact", ["name", "version"], unique=True, postgresql_where=sa.text("family = 'solution'") @@ -28,6 +29,10 @@ def upgrade() -> None: def downgrade() -> None: _add_bundled_builds() + _assert_no_duplicate_solutions( + ["name", "source", "version", "track", "stage", "bundled_builds_hash"], + nullable_columns=["bundled_builds_hash"], + ) op.drop_index("unique_solution", table_name="artefact", postgresql_where=sa.text("family = 'solution'")) op.create_index( "unique_solution", @@ -39,6 +44,39 @@ def downgrade() -> None: op.drop_column("artefact", "attributes") +def _assert_no_duplicate_solutions(key_columns: list[str], nullable_columns: list[str] | None = None) -> None: + """Fail fast with a clear error if applying a unique index on ``key_columns`` (scoped to + solution artefacts) would violate uniqueness, instead of letting index creation fail with an + opaque database error. + + Postgres unique indexes treat NULL as distinct from any other value (including another NULL), + so columns listed in ``nullable_columns`` are excluded from the duplicate search whenever they + are NULL, matching the semantics of the index we're about to create. + """ + nullable_columns = nullable_columns or [] + columns_sql = ", ".join(key_columns) + not_null_clauses = " AND ".join(f"{column} IS NOT NULL" for column in nullable_columns) + where_clause = f"family = 'solution' AND {not_null_clauses}" if not_null_clauses else "family = 'solution'" + + conn = op.get_bind() + duplicates = conn.execute( + sa.text(f""" + SELECT {columns_sql}, COUNT(*) AS duplicate_count + FROM artefact + WHERE {where_clause} + GROUP BY {columns_sql} + HAVING COUNT(*) > 1 + LIMIT 5 + """) # noqa: S608 - key_columns/nullable_columns are fixed, developer-controlled constants + ).fetchall() + + if duplicates: + raise RuntimeError( + f"Cannot create unique index on solutions ({columns_sql}): found existing duplicate rows " + f"(showing up to 5): {duplicates}. Resolve these duplicates manually before running this migration." + ) + + def _remove_bundled_builds() -> None: _copy_bundled_builds_to_attributes() op.drop_table("artefact_bundled_builds_association") diff --git a/backend/test_observer/controllers/artefacts/artefacts.py b/backend/test_observer/controllers/artefacts/artefacts.py index 273044c24..405c9a8d3 100644 --- a/backend/test_observer/controllers/artefacts/artefacts.py +++ b/backend/test_observer/controllers/artefacts/artefacts.py @@ -391,6 +391,7 @@ def get_artefact_versions( return db.scalars( select(Artefact) .where(Artefact.name == artefact.name) + .where(Artefact.family == artefact.family) .where(Artefact.track == artefact.track) .where(Artefact.branch == artefact.branch) .where(Artefact.series == artefact.series) diff --git a/backend/test_observer/data_access/models.py b/backend/test_observer/data_access/models.py index b1442d055..b726ebe5c 100644 --- a/backend/test_observer/data_access/models.py +++ b/backend/test_observer/data_access/models.py @@ -325,15 +325,13 @@ class Artefact(Base): owner: Mapped[str] = mapped_column(String(200), default="") image_url: Mapped[str] = mapped_column(String(200), default="") - # (for now) Solution specific field - attributes: Mapped[dict[str, Any]] = mapped_column(MutableDict.as_mutable(JSONB), default=dict, server_default="{}") - # Relationships builds: Mapped[list["ArtefactBuild"]] = relationship(back_populates="artefact", cascade="all, delete") reviewers: Mapped[list[User]] = relationship( secondary=artefact_reviewers_association, back_populates="artefact_reviews" ) + attributes: Mapped[dict[str, Any]] = mapped_column(MutableDict.as_mutable(JSONB), default=dict, server_default="{}") jira_issue: Mapped[str | None] = mapped_column(default=None) @property diff --git a/backend/tests/controllers/artefacts/test_artefacts.py b/backend/tests/controllers/artefacts/test_artefacts.py index 339a4da65..8cf5346fc 100644 --- a/backend/tests/controllers/artefacts/test_artefacts.py +++ b/backend/tests/controllers/artefacts/test_artefacts.py @@ -1068,6 +1068,27 @@ def test_get_artefact_versions(test_client: TestClient, generator: DataGenerator assert response.json() == [{"version": "3", "artefact_id": artefact3.id}] +def test_get_artefact_versions_does_not_mix_families(test_client: TestClient, generator: DataGenerator): + """Artefacts of different families sharing the same name (and otherwise-blank family-specific + fields) must not be mixed together in each other's version history.""" + snap = generator.gen_artefact(family=FamilyName.snap, name="shared-name", version="1") + solution = generator.gen_artefact(family=FamilyName.solution, name="shared-name", version="2") + + response = make_authenticated_request( + lambda: test_client.get(f"/v1/artefacts/{snap.id}/versions"), + Permission.view_artefact, + ) + assert response.status_code == 200 + assert response.json() == [{"version": "1", "artefact_id": snap.id}] + + response = make_authenticated_request( + lambda: test_client.get(f"/v1/artefacts/{solution.id}/versions"), + Permission.view_artefact, + ) + assert response.status_code == 200 + assert response.json() == [{"version": "2", "artefact_id": solution.id}] + + def test_get_artefact_history_default_filters(test_client: TestClient, generator: DataGenerator): charm_latest_1 = generator.gen_artefact( family=FamilyName.charm, diff --git a/backend/tests/migrations/test_8202f7b5953e_replace_solution_specific_fields.py b/backend/tests/migrations/test_8202f7b5953e_replace_solution_specific_fields.py index d8bed2eec..638488ae7 100644 --- a/backend/tests/migrations/test_8202f7b5953e_replace_solution_specific_fields.py +++ b/backend/tests/migrations/test_8202f7b5953e_replace_solution_specific_fields.py @@ -41,6 +41,7 @@ def migration_context(db_url: str) -> Generator[tuple[Engine, Config], None, Non create_database(test_db_url) + engine: Engine | None = None try: engine = create_engine(test_db_url) alembic_config = Config("alembic.ini") @@ -48,7 +49,8 @@ def migration_context(db_url: str) -> Generator[tuple[Engine, Config], None, Non yield engine, alembic_config finally: - engine.dispose() + if engine is not None: + engine.dispose() if database_exists(test_db_url): drop_database(test_db_url) @@ -58,6 +60,10 @@ def _insert_artefact( name: str, bundled_builds_hash: str | None = None, attributes: str | None = None, + version: str = "1.0", + track: str = "latest", + source: str = "source", + stage: str = "stable", ) -> int: optional_column = ", attributes" if attributes is not None else ", bundled_builds_hash" optional_value = ", CAST(:attributes AS jsonb)" if attributes is not None else ", :bundled_builds_hash" @@ -69,14 +75,18 @@ def _insert_artefact( created_at, updated_at{optional_column} ) VALUES ( - :name, '1.0', 'stable', 'solution', 'UNDECIDED', false, '', '', - '', '', 'latest', '', '', 'source', '', '', '', '', '', + :name, :version, :stage, 'solution', 'UNDECIDED', false, '', '', + '', '', :track, '', '', :source, '', '', '', '', '', NOW(), NOW(){optional_value} ) RETURNING id """), { "name": name, + "version": version, + "stage": stage, + "track": track, + "source": source, "bundled_builds_hash": bundled_builds_hash, "attributes": attributes, }, @@ -274,6 +284,45 @@ def test_downgrade_restores_hash_and_associations(migration_context: tuple[Engin assert association_ids == [first_build_id, second_build_id] +def test_upgrade_fails_fast_on_duplicate_name_and_version(migration_context: tuple[Engine, Config]) -> None: + """Pre-migration schema allows several solutions sharing (name, version) as long as track/source + differ; the upgrade must refuse to create the tighter (name, version) unique index rather than + fail with an opaque database error.""" + engine, alembic_config = migration_context + command.upgrade(alembic_config, PREVIOUS_REV) + with engine.begin() as conn: + _insert_artefact(conn, "dup-solution", version="1.0", track="track-a", source="source-a") + _insert_artefact(conn, "dup-solution", version="1.0", track="track-b", source="source-b") + + with pytest.raises(RuntimeError, match="Cannot create unique index"): + command.upgrade(alembic_config, TARGET_REV) + + # The failed migration must not have partially applied its schema changes. + with engine.connect() as conn: + attributes_column = conn.execute( + text(""" + SELECT column_name + FROM information_schema.columns + WHERE table_name = 'artefact' AND column_name = 'attributes' + """) + ).fetchone() + assert attributes_column is None + + +def test_downgrade_fails_fast_on_duplicate_widened_key(migration_context: tuple[Engine, Config]) -> None: + """Defends the downgrade's wider unique index the same way, in case data ever ends up violating + it (e.g. the (name, version) index was bypassed or dropped out-of-band).""" + engine, alembic_config = migration_context + command.upgrade(alembic_config, TARGET_REV) + with engine.begin() as conn: + conn.execute(text("DROP INDEX unique_solution")) + _insert_artefact(conn, "dup-solution", attributes='{"bundled_builds_hash": "hash-x"}') + _insert_artefact(conn, "dup-solution", attributes='{"bundled_builds_hash": "hash-x"}') + + with pytest.raises(RuntimeError, match="Cannot create unique index"): + command.downgrade(alembic_config, PREVIOUS_REV) + + def test_upgrade_schema_changes(migration_context: tuple[Engine, Config]) -> None: engine, alembic_config = migration_context command.upgrade(alembic_config, TARGET_REV) From 25a5d31d411d9b51dc5cf39631dd4c514d0f7f96 Mon Sep 17 00:00:00 2001 From: Raul Almeida Date: Thu, 23 Jul 2026 13:07:47 -0300 Subject: [PATCH 06/21] fix: licensing in migration --- ...953e_replace_solution_specific_fields_with_.py | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/backend/migrations/versions/2026_07_21_1311-8202f7b5953e_replace_solution_specific_fields_with_.py b/backend/migrations/versions/2026_07_21_1311-8202f7b5953e_replace_solution_specific_fields_with_.py index 6126c9e64..b54b3008e 100644 --- a/backend/migrations/versions/2026_07_21_1311-8202f7b5953e_replace_solution_specific_fields_with_.py +++ b/backend/migrations/versions/2026_07_21_1311-8202f7b5953e_replace_solution_specific_fields_with_.py @@ -1,3 +1,18 @@ +# Copyright 2026 Canonical Ltd. +# +# This program is free software: you can redistribute it and/or modify +# it under the terms of the GNU Affero General Public License version 3, as +# published by the Free Software Foundation. +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU Affero General Public License for more details. +# You should have received a copy of the GNU Affero General Public License +# along with this program. If not, see . +# +# SPDX-FileCopyrightText: Copyright 2026 Canonical Ltd. +# SPDX-License-Identifier: AGPL-3.0-only + """Replace solution-specific fields with attributes Revision ID: 8202f7b5953e From ae13ae2a934dc7b80fc5a53c2609c65fe38fed75 Mon Sep 17 00:00:00 2001 From: Raul Almeida Date: Thu, 23 Jul 2026 14:51:50 -0300 Subject: [PATCH 07/21] enh: copilot review --- backend/schemata/openapi.json | 36 +++++++++++++++- .../controllers/artefacts/artefacts.py | 5 ++- .../controllers/artefacts/models.py | 7 ++++ .../controllers/test_executions/models.py | 31 ++++++++++++++ .../test_observer/data_access/repository.py | 4 +- .../controllers/artefacts/test_artefacts.py | 25 +++++++++++ .../test_executions/test_reruns.py | 1 + .../test_executions/test_start_test.py | 42 +++++++++++++++++++ 8 files changed, 147 insertions(+), 4 deletions(-) diff --git a/backend/schemata/openapi.json b/backend/schemata/openapi.json index 076cb028f..5ecfd89e3 100644 --- a/backend/schemata/openapi.json +++ b/backend/schemata/openapi.json @@ -6559,6 +6559,13 @@ ], "description": "Backward-compatible assignee field. Populated from the first entry in reviewers when present.", "readOnly": true + }, + "bundled_builds": { + "items": {}, + "type": "array", + "title": "Bundled Builds", + "deprecated": true, + "readOnly": true } }, "type": "object", @@ -6590,7 +6597,8 @@ "jira_issue", "all_environment_reviews_count", "completed_environment_reviews_count", - "assignee" + "assignee", + "bundled_builds" ], "title": "ArtefactResponse" }, @@ -8487,6 +8495,32 @@ "execution_stage": { "$ref": "#/components/schemas/SolutionStage", "description": "Promotion stage of the solution being tested." + }, + "track": { + "anyOf": [ + { + "type": "string" + }, + { + "type": "null" + } + ], + "title": "Track", + "description": "Legacy field. Merged into attributes['track'] when attributes['track'] is not provided.", + "deprecated": true + }, + "source": { + "anyOf": [ + { + "type": "string" + }, + { + "type": "null" + } + ], + "title": "Source", + "description": "Legacy field. Merged into attributes['source'] when attributes['source'] is not provided.", + "deprecated": true } }, "type": "object", diff --git a/backend/test_observer/controllers/artefacts/artefacts.py b/backend/test_observer/controllers/artefacts/artefacts.py index 405c9a8d3..ec8cc91f0 100644 --- a/backend/test_observer/controllers/artefacts/artefacts.py +++ b/backend/test_observer/controllers/artefacts/artefacts.py @@ -259,8 +259,9 @@ def patch_artefact( artefact.comment = request.comment if "jira_issue" in request.model_fields_set: artefact.jira_issue = request.jira_issue - if "attributes" in request.model_fields_set and request.attributes is not None: - artefact.attributes = request.attributes + if "attributes" in request.model_fields_set: + # attributes is non-nullable in the DB; an explicit null in the request clears it to {}. + artefact.attributes = request.attributes if request.attributes is not None else {} reviewer_ids_set = hasattr(request, "reviewer_ids") and "reviewer_ids" in request.model_fields_set reviewer_emails_set = hasattr(request, "reviewer_emails") and "reviewer_emails" in request.model_fields_set diff --git a/backend/test_observer/controllers/artefacts/models.py b/backend/test_observer/controllers/artefacts/models.py index 75b887aa6..a737e77dc 100644 --- a/backend/test_observer/controllers/artefacts/models.py +++ b/backend/test_observer/controllers/artefacts/models.py @@ -87,6 +87,13 @@ class ArtefactResponse(BaseModel): def assignee(self) -> ReviewerResponse | None: return self.reviewers[0] if self.reviewers else None + @computed_field( + deprecated="bundled_builds is deprecated and always empty; solutions now use the generic " + "attributes field instead.", + ) + def bundled_builds(self) -> list[Any]: + return [] + class EnvironmentResponse(BaseModel): model_config = ConfigDict(from_attributes=True) diff --git a/backend/test_observer/controllers/test_executions/models.py b/backend/test_observer/controllers/test_executions/models.py index 9c5ac944b..81d04556c 100644 --- a/backend/test_observer/controllers/test_executions/models.py +++ b/backend/test_observer/controllers/test_executions/models.py @@ -287,6 +287,37 @@ class StartSolutionTestExecutionRequest(_StartTestExecutionRequest): family: Literal[FamilyName.solution] attributes: dict[str, Any] = Field(default_factory=dict) execution_stage: SolutionStage = Field(description="Promotion stage of the solution being tested.") + track: str | None = Field( + default=None, + deprecated=True, + description="Legacy field. Merged into attributes['track'] when attributes['track'] is not provided.", + ) + source: str | None = Field( + default=None, + deprecated=True, + description="Legacy field. Merged into attributes['source'] when attributes['source'] is not provided.", + ) + + @model_validator(mode="before") + @classmethod + def map_legacy_track_and_source(cls, data: object) -> object: + if not isinstance(data, dict): + return data + + # Backwards compatibility: fold the legacy track/source fields into attributes, + # without clobbering values explicitly set in attributes. + attributes = data.get("attributes") or {} + if not isinstance(attributes, dict): + return data + attributes = dict(attributes) + + if data.get("track") is not None and "track" not in attributes: + attributes["track"] = data["track"] + if data.get("source") is not None and "source" not in attributes: + attributes["source"] = data["source"] + + data["attributes"] = attributes + return data class C3TestResultStatus(StrEnum): diff --git a/backend/test_observer/data_access/repository.py b/backend/test_observer/data_access/repository.py index 65783e0ed..99d0ebce7 100644 --- a/backend/test_observer/data_access/repository.py +++ b/backend/test_observer/data_access/repository.py @@ -39,7 +39,9 @@ def get_artefacts_by_family( :session: DB session :family: name of the family - :load_stage: whether to eagerly load stage object in all artefacts + :load_environment_reviews: whether to eagerly load each build's environment reviews + :load_builds: whether to eagerly load each artefact's builds + :order_by_columns: optional columns to order the results by :return: list of Artefacts """ if family == FamilyName.charm: diff --git a/backend/tests/controllers/artefacts/test_artefacts.py b/backend/tests/controllers/artefacts/test_artefacts.py index 8cf5346fc..b80a5ee4a 100644 --- a/backend/tests/controllers/artefacts/test_artefacts.py +++ b/backend/tests/controllers/artefacts/test_artefacts.py @@ -381,6 +381,30 @@ def test_patch_artefact_without_attributes_preserves_existing_attributes( assert response.json()["attributes"] == {"keep": "me"} +def test_patch_artefact_with_explicit_null_attributes_clears_them( + test_client: TestClient, + generator: DataGenerator, +): + artefact = generator.gen_artefact(attributes={"old": "value"}) + + response = make_authenticated_request( + lambda: test_client.patch( + f"/v1/artefacts/{artefact.id}", + json={"attributes": None}, + ), + Permission.change_artefact, + ) + + assert response.status_code == 200 + assert response.json()["attributes"] == {} + + get_response = make_authenticated_request( + lambda: test_client.get(f"/v1/artefacts/{artefact.id}"), + Permission.view_artefact, + ) + assert get_response.json()["attributes"] == {} + + def test_artefact_signoff_ignore_old_build_on_approve(test_client: TestClient, generator: DataGenerator): artefact = generator.gen_artefact(StageName.candidate) build1 = generator.gen_artefact_build(artefact, revision=1) @@ -1233,6 +1257,7 @@ def _assert_get_artefact_response(response: dict[str, Any], artefact: Artefact) "attributes": artefact.attributes, "assignee": assignee, "reviewers": [], + "bundled_builds": [], "due_date": (artefact.due_date.strftime("%Y-%m-%d") if artefact.due_date else None), "bug_link": artefact.bug_link, "jira_issue": artefact.jira_issue, diff --git a/backend/tests/controllers/test_executions/test_reruns.py b/backend/tests/controllers/test_executions/test_reruns.py index 822262268..bb5f8883f 100644 --- a/backend/tests/controllers/test_executions/test_reruns.py +++ b/backend/tests/controllers/test_executions/test_reruns.py @@ -168,6 +168,7 @@ def test_execution_to_pending_rerun(test_execution: TestExecution, priority: int "comment": test_execution.artefact_build.artefact.comment, "attributes": test_execution.artefact_build.artefact.attributes, "archived": test_execution.artefact_build.artefact.archived, + "bundled_builds": [], "assignee": ( test_execution.artefact_build.artefact.reviewers[0] if test_execution.artefact_build.artefact.reviewers diff --git a/backend/tests/controllers/test_executions/test_start_test.py b/backend/tests/controllers/test_executions/test_start_test.py index b9ed2c761..d5f736c9f 100644 --- a/backend/tests/controllers/test_executions/test_start_test.py +++ b/backend/tests/controllers/test_executions/test_start_test.py @@ -178,6 +178,48 @@ def test_start_test_on_existing_solution_does_not_change_attributes( assert te2.artefact_build.artefact.attributes == original +def test_start_test_legacy_track_and_source_are_merged_into_attributes( + execute: Execute, + db_session: Session, +) -> None: + """The legacy track/source fields are deprecated but still accepted for backward + compatibility, and are folded into attributes for existing clients that haven't + migrated to the attributes field yet.""" + response = execute({**solution_test_request, "track": "22.04", "source": "ppa:ubuntu-pro/fips"}) + + assert response.status_code == 200 + test_execution = db_session.get(TestExecution, response.json()["id"]) + assert test_execution is not None + assert test_execution.artefact_build.artefact.attributes == { + "track": "22.04", + "source": "ppa:ubuntu-pro/fips", + } + + +def test_start_test_legacy_track_and_source_do_not_override_attributes( + execute: Execute, + db_session: Session, +) -> None: + """When attributes explicitly sets track/source, the legacy fields must not clobber them.""" + response = execute( + { + **solution_test_request, + "track": "legacy-track", + "source": "legacy-source", + "attributes": {"track": "explicit-track", "other": "value"}, + } + ) + + assert response.status_code == 200 + test_execution = db_session.get(TestExecution, response.json()["id"]) + assert test_execution is not None + assert test_execution.artefact_build.artefact.attributes == { + "track": "explicit-track", + "other": "value", + "source": "legacy-source", + } + + @pytest.mark.parametrize( "start_request", [snap_test_request, deb_test_request, charm_test_request, image_test_request], From c27112ebe18a4960fa74e6711010294a9315d6e7 Mon Sep 17 00:00:00 2001 From: Raul Almeida Date: Thu, 23 Jul 2026 15:44:15 -0300 Subject: [PATCH 08/21] enh: copilot review --- .../controllers/test_executions/models.py | 14 +++++++++----- .../controllers/test_executions/test_start_test.py | 8 ++++++++ 2 files changed, 17 insertions(+), 5 deletions(-) diff --git a/backend/test_observer/controllers/test_executions/models.py b/backend/test_observer/controllers/test_executions/models.py index 81d04556c..d8f0a3c3a 100644 --- a/backend/test_observer/controllers/test_executions/models.py +++ b/backend/test_observer/controllers/test_executions/models.py @@ -305,11 +305,15 @@ def map_legacy_track_and_source(cls, data: object) -> object: return data # Backwards compatibility: fold the legacy track/source fields into attributes, - # without clobbering values explicitly set in attributes. - attributes = data.get("attributes") or {} - if not isinstance(attributes, dict): - return data - attributes = dict(attributes) + # without clobbering values explicitly set in attributes. An explicit null for + # attributes is left untouched so it falls through to normal (non-nullable) validation. + if "attributes" not in data: + attributes: Any = {} + else: + attributes = data["attributes"] + if attributes is None or not isinstance(attributes, dict): + return data + attributes = dict(attributes) if data.get("track") is not None and "track" not in attributes: attributes["track"] = data["track"] diff --git a/backend/tests/controllers/test_executions/test_start_test.py b/backend/tests/controllers/test_executions/test_start_test.py index d5f736c9f..fc8e2cbe4 100644 --- a/backend/tests/controllers/test_executions/test_start_test.py +++ b/backend/tests/controllers/test_executions/test_start_test.py @@ -220,6 +220,14 @@ def test_start_test_legacy_track_and_source_do_not_override_attributes( } +def test_start_test_explicit_null_attributes_is_rejected(execute: Execute) -> None: + """attributes is non-nullable; an explicit null must fail validation rather than being + silently treated as an empty dict.""" + response = execute({**solution_test_request, "attributes": None}) + + assert response.status_code == 422 + + @pytest.mark.parametrize( "start_request", [snap_test_request, deb_test_request, charm_test_request, image_test_request], From 18a624632310184b5261cb9f74ce5bc2ceca9e27 Mon Sep 17 00:00:00 2001 From: Raul Almeida Date: Fri, 24 Jul 2026 09:10:32 -0300 Subject: [PATCH 09/21] enh: copilot review --- backend/schemata/openapi.json | 7 +++++-- backend/test_observer/controllers/artefacts/models.py | 2 +- .../test_observer/controllers/test_executions/models.py | 1 + 3 files changed, 7 insertions(+), 3 deletions(-) diff --git a/backend/schemata/openapi.json b/backend/schemata/openapi.json index 5ecfd89e3..8ff877d78 100644 --- a/backend/schemata/openapi.json +++ b/backend/schemata/openapi.json @@ -6561,7 +6561,9 @@ "readOnly": true }, "bundled_builds": { - "items": {}, + "items": { + "$ref": "#/components/schemas/ArtefactBuildMinimalResponse" + }, "type": "array", "title": "Bundled Builds", "deprecated": true, @@ -8512,7 +8514,8 @@ "source": { "anyOf": [ { - "type": "string" + "type": "string", + "maxLength": 200 }, { "type": "null" diff --git a/backend/test_observer/controllers/artefacts/models.py b/backend/test_observer/controllers/artefacts/models.py index a737e77dc..0edced1e4 100644 --- a/backend/test_observer/controllers/artefacts/models.py +++ b/backend/test_observer/controllers/artefacts/models.py @@ -91,7 +91,7 @@ def assignee(self) -> ReviewerResponse | None: deprecated="bundled_builds is deprecated and always empty; solutions now use the generic " "attributes field instead.", ) - def bundled_builds(self) -> list[Any]: + def bundled_builds(self) -> list["ArtefactBuildMinimalResponse"]: return [] diff --git a/backend/test_observer/controllers/test_executions/models.py b/backend/test_observer/controllers/test_executions/models.py index d8f0a3c3a..052b3e1fa 100644 --- a/backend/test_observer/controllers/test_executions/models.py +++ b/backend/test_observer/controllers/test_executions/models.py @@ -294,6 +294,7 @@ class StartSolutionTestExecutionRequest(_StartTestExecutionRequest): ) source: str | None = Field( default=None, + max_length=200, deprecated=True, description="Legacy field. Merged into attributes['source'] when attributes['source'] is not provided.", ) From ff46d9d16edaeeb824f80d9778d67430e6707d5d Mon Sep 17 00:00:00 2001 From: Raul Almeida Date: Fri, 24 Jul 2026 09:36:03 -0300 Subject: [PATCH 10/21] enh: copilot review (2) --- .../tests/controllers/test_executions/test_start_test.py | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/backend/tests/controllers/test_executions/test_start_test.py b/backend/tests/controllers/test_executions/test_start_test.py index fc8e2cbe4..4aa5d7376 100644 --- a/backend/tests/controllers/test_executions/test_start_test.py +++ b/backend/tests/controllers/test_executions/test_start_test.py @@ -228,6 +228,14 @@ def test_start_test_explicit_null_attributes_is_rejected(execute: Execute) -> No assert response.status_code == 422 +def test_start_test_legacy_source_over_max_length_is_rejected(execute: Execute) -> None: + """The legacy source field must keep the same 200-char limit as the underlying + artefact.source column, so oversized values fail validation (422) instead of a DB error.""" + response = execute({**solution_test_request, "source": "x" * 201}) + + assert response.status_code == 422 + + @pytest.mark.parametrize( "start_request", [snap_test_request, deb_test_request, charm_test_request, image_test_request], From 956db974a514e383fb3894acc9ff2a61914a469f Mon Sep 17 00:00:00 2001 From: Raul Almeida Date: Fri, 24 Jul 2026 17:15:18 -0300 Subject: [PATCH 11/21] fix: failing CI due to diff in schema types (thx copilot) --- .github/workflows/test_backend.yml | 6 +++++- backend/schemata/openapi.json | 4 ++-- backend/scripts/fetch_openapi_schema.sh | 7 ++++++- 3 files changed, 13 insertions(+), 4 deletions(-) diff --git a/.github/workflows/test_backend.yml b/.github/workflows/test_backend.yml index a498ad750..1fd7b967e 100644 --- a/.github/workflows/test_backend.yml +++ b/.github/workflows/test_backend.yml @@ -57,6 +57,10 @@ jobs: - name: Check if alembic migrations are up to date run: docker compose exec test-observer-api alembic check - name: Compare schema with repository - run: diff <(curl -s http://localhost:30000/openapi.json | jq) schemata/openapi.json + # Normalise integer-valued floats so the comparison is stable across jq + # versions (jq 1.6 on jammy canonicalises 1000000.0 -> 1000000 while jq + # 1.7 preserves the literal). Keep this filter in sync with + # scripts/fetch_openapi_schema.sh. + run: diff <(curl -s http://localhost:30000/openapi.json | jq 'walk(if type == "number" and floor == . then floor else . end)') schemata/openapi.json - name: Ensure all endpoints are secured run: ./scripts/check_endpoint_permissions.sh schemata/openapi.json diff --git a/backend/schemata/openapi.json b/backend/schemata/openapi.json index 8ff877d78..cadf12e02 100644 --- a/backend/schemata/openapi.json +++ b/backend/schemata/openapi.json @@ -7846,8 +7846,8 @@ "anyOf": [ { "type": "integer", - "maximum": 1000000.0, - "minimum": -1000000.0 + "maximum": 1000000, + "minimum": -1000000 }, { "type": "null" diff --git a/backend/scripts/fetch_openapi_schema.sh b/backend/scripts/fetch_openapi_schema.sh index ee6d2195e..0617140a2 100755 --- a/backend/scripts/fetch_openapi_schema.sh +++ b/backend/scripts/fetch_openapi_schema.sh @@ -23,8 +23,13 @@ SCRIPT_DIR="$( cd "$( dirname "${BASH_SOURCE[0]}" )" && pwd )" cd "$SCRIPT_DIR/.." tmpfile=$(mktemp) +# Normalise integer-valued floats (e.g. 1000000.0 -> 1000000) so the committed +# schema is stable across jq versions. jq 1.7 preserves number literals while +# jq 1.6 (used on the jammy CI runner) canonicalises them, which otherwise +# causes spurious diffs in the "Compare schema with repository" CI step. +normalise='walk(if type == "number" and floor == . then floor else . end)' if curl --silent --fail "http://localhost:30000/openapi.json" -o "$tmpfile"; then - jq < "$tmpfile" > schemata/openapi.json + jq "$normalise" < "$tmpfile" > schemata/openapi.json echo "OpenAPI schema fetched and written to schemata/openapi.json" else echo "Failed to fetch openapi.json" From f4b129b843ea0dabda1a9be89dd2ef124a4633f5 Mon Sep 17 00:00:00 2001 From: Raul Almeida Date: Mon, 10 Aug 2026 15:45:01 -0300 Subject: [PATCH 12/21] enh: small review points from Will --- backend/test_observer/controllers/artefacts/models.py | 7 ------- .../test_observer/controllers/test_executions/models.py | 3 +-- 2 files changed, 1 insertion(+), 9 deletions(-) diff --git a/backend/test_observer/controllers/artefacts/models.py b/backend/test_observer/controllers/artefacts/models.py index 0edced1e4..75b887aa6 100644 --- a/backend/test_observer/controllers/artefacts/models.py +++ b/backend/test_observer/controllers/artefacts/models.py @@ -87,13 +87,6 @@ class ArtefactResponse(BaseModel): def assignee(self) -> ReviewerResponse | None: return self.reviewers[0] if self.reviewers else None - @computed_field( - deprecated="bundled_builds is deprecated and always empty; solutions now use the generic " - "attributes field instead.", - ) - def bundled_builds(self) -> list["ArtefactBuildMinimalResponse"]: - return [] - class EnvironmentResponse(BaseModel): model_config = ConfigDict(from_attributes=True) diff --git a/backend/test_observer/controllers/test_executions/models.py b/backend/test_observer/controllers/test_executions/models.py index 052b3e1fa..ea9c05bc0 100644 --- a/backend/test_observer/controllers/test_executions/models.py +++ b/backend/test_observer/controllers/test_executions/models.py @@ -309,12 +309,11 @@ def map_legacy_track_and_source(cls, data: object) -> object: # without clobbering values explicitly set in attributes. An explicit null for # attributes is left untouched so it falls through to normal (non-nullable) validation. if "attributes" not in data: - attributes: Any = {} + attributes: dict[str, Any] = {} else: attributes = data["attributes"] if attributes is None or not isinstance(attributes, dict): return data - attributes = dict(attributes) if data.get("track") is not None and "track" not in attributes: attributes["track"] = data["track"] From 87def61926619541a842895b738d5143da36e1ce Mon Sep 17 00:00:00 2001 From: Raul Almeida Date: Tue, 11 Aug 2026 10:36:18 -0300 Subject: [PATCH 13/21] enh: split into 2 migrations (to-do: split into 2 PRs) --- ...3e_add_artefact_attributes_and_backfill.py | 83 +++++ ...solution_specific_bundled_build_fields.py} | 38 ++- ...202f7b5953e_add_attributes_and_backfill.py | 294 ++++++++++++++++++ ...solution_specific_bundled_build_fields.py} | 166 +++++----- 4 files changed, 473 insertions(+), 108 deletions(-) create mode 100644 backend/migrations/versions/2026_07_21_1311-8202f7b5953e_add_artefact_attributes_and_backfill.py rename backend/migrations/versions/{2026_07_21_1311-8202f7b5953e_replace_solution_specific_fields_with_.py => 2026_08_10_1716-8bd1f5009f02_drop_solution_specific_bundled_build_fields.py} (85%) create mode 100644 backend/tests/migrations/test_8202f7b5953e_add_attributes_and_backfill.py rename backend/tests/migrations/{test_8202f7b5953e_replace_solution_specific_fields.py => test_8bd1f5009f02_drop_solution_specific_bundled_build_fields.py} (71%) diff --git a/backend/migrations/versions/2026_07_21_1311-8202f7b5953e_add_artefact_attributes_and_backfill.py b/backend/migrations/versions/2026_07_21_1311-8202f7b5953e_add_artefact_attributes_and_backfill.py new file mode 100644 index 000000000..892081468 --- /dev/null +++ b/backend/migrations/versions/2026_07_21_1311-8202f7b5953e_add_artefact_attributes_and_backfill.py @@ -0,0 +1,83 @@ +# Copyright 2026 Canonical Ltd. +# +# This program is free software: you can redistribute it and/or modify +# it under the terms of the GNU Affero General Public License version 3, as +# published by the Free Software Foundation. +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU Affero General Public License for more details. +# You should have received a copy of the GNU Affero General Public License +# along with this program. If not, see . +# +# SPDX-FileCopyrightText: Copyright 2026 Canonical Ltd. +# SPDX-License-Identifier: AGPL-3.0-only + +"""Add artefact.attributes and backfill bundled builds into it + +This is the non-destructive (expand) half of adding the ``attributes`` column +to artefacts and removing solution-specific fields. + +It only adds the new column and copies existing data into it, leaving +``bundled_builds_hash`` and ``artefact_bundled_builds_association`` in place +so that code running against the old schema keeps working during a rolling +upgrade. + +The destructive (contract) half - swapping the ``unique_solution`` index and +dropping the old column/table - lives in a separate, later migration that must +be released only after this one has been fully rolled out. + +Revision ID: 8202f7b5953e +Revises: eba1d1c92dba +Create Date: 2026-07-21 13:11:39.128081+00:00 + +""" + +import sqlalchemy as sa +from alembic import op +from sqlalchemy.dialects import postgresql + +# revision identifiers, used by Alembic. +revision = "8202f7b5953e" +down_revision = "eba1d1c92dba" +branch_labels = None +depends_on = None + + +def upgrade() -> None: + op.add_column("artefact", sa.Column("attributes", postgresql.JSONB(), server_default="{}", nullable=False)) + _copy_bundled_builds_to_attributes() + + +def downgrade() -> None: + op.drop_column("artefact", "attributes") + + +def _copy_bundled_builds_to_attributes() -> None: + op.execute( + """ + UPDATE artefact AS a + SET attributes = a.attributes + || jsonb_strip_nulls( + jsonb_build_object('bundled_builds_hash', a.bundled_builds_hash) + ) + || COALESCE( + ( + SELECT jsonb_build_object( + 'bundled_builds', + jsonb_agg(assoc.artefact_build_id ORDER BY assoc.artefact_build_id) + ) + FROM artefact_bundled_builds_association assoc + WHERE assoc.artefact_id = a.id + HAVING count(*) > 0 + ), + '{}'::jsonb + ) + WHERE a.bundled_builds_hash IS NOT NULL + OR EXISTS ( + SELECT 1 + FROM artefact_bundled_builds_association assoc + WHERE assoc.artefact_id = a.id + ) + """ + ) diff --git a/backend/migrations/versions/2026_07_21_1311-8202f7b5953e_replace_solution_specific_fields_with_.py b/backend/migrations/versions/2026_08_10_1716-8bd1f5009f02_drop_solution_specific_bundled_build_fields.py similarity index 85% rename from backend/migrations/versions/2026_07_21_1311-8202f7b5953e_replace_solution_specific_fields_with_.py rename to backend/migrations/versions/2026_08_10_1716-8bd1f5009f02_drop_solution_specific_bundled_build_fields.py index b54b3008e..2b574ae9b 100644 --- a/backend/migrations/versions/2026_07_21_1311-8202f7b5953e_replace_solution_specific_fields_with_.py +++ b/backend/migrations/versions/2026_08_10_1716-8bd1f5009f02_drop_solution_specific_bundled_build_fields.py @@ -13,33 +13,46 @@ # SPDX-FileCopyrightText: Copyright 2026 Canonical Ltd. # SPDX-License-Identifier: AGPL-3.0-only -"""Replace solution-specific fields with attributes +"""Drop solution-specific bundled build fields -Revision ID: 8202f7b5953e -Revises: eba1d1c92dba -Create Date: 2026-07-21 13:11:39.128081+00:00 +This is the destructive (contract) half of adding the ``attributes`` field +to artefacts and removing the solution-specific fields. +It swaps the ``unique_solution`` index to ``(name, version)`` and drops +``artefact.bundled_builds_hash`` and ``artefact_bundled_builds_association``. + +For a safe rolling upgrade this migration must only be deployed *after* the +expand migration (``8202f7b5953e``) and the code that stops using the old fields +have been fully rolled out. It re-runs the backfill first so that any rows +written by not-yet-upgraded units during that rollout (which populate the old +columns but not ``attributes``) are copied over before the columns are dropped. + +Revision ID: 8bd1f5009f02 +Revises: 8202f7b5953e +Create Date: 2026-08-10 17:16:00.000000+00:00 """ import sqlalchemy as sa from alembic import op -from sqlalchemy.dialects import postgresql # revision identifiers, used by Alembic. -revision = "8202f7b5953e" -down_revision = "eba1d1c92dba" +revision = "8bd1f5009f02" +down_revision = "8202f7b5953e" branch_labels = None depends_on = None def upgrade() -> None: - op.add_column("artefact", sa.Column("attributes", postgresql.JSONB(), server_default="{}", nullable=False)) + # Catch leftover data written by old code during the rollout of the expand + # migration before the source columns/table are removed. + _copy_bundled_builds_to_attributes() _assert_no_duplicate_solutions(["name", "version"]) op.drop_index("unique_solution", table_name="artefact", postgresql_where="(family = 'solution'::familyname)") op.create_index( "unique_solution", "artefact", ["name", "version"], unique=True, postgresql_where=sa.text("family = 'solution'") ) - _remove_bundled_builds() + op.drop_table("artefact_bundled_builds_association") + op.drop_column("artefact", "bundled_builds_hash") def downgrade() -> None: @@ -56,7 +69,6 @@ def downgrade() -> None: unique=True, postgresql_where="(family = 'solution'::familyname)", ) - op.drop_column("artefact", "attributes") def _assert_no_duplicate_solutions(key_columns: list[str], nullable_columns: list[str] | None = None) -> None: @@ -92,12 +104,6 @@ def _assert_no_duplicate_solutions(key_columns: list[str], nullable_columns: lis ) -def _remove_bundled_builds() -> None: - _copy_bundled_builds_to_attributes() - op.drop_table("artefact_bundled_builds_association") - op.drop_column("artefact", "bundled_builds_hash") - - def _add_bundled_builds() -> None: op.add_column( "artefact", sa.Column("bundled_builds_hash", sa.VARCHAR(length=64), autoincrement=False, nullable=True) diff --git a/backend/tests/migrations/test_8202f7b5953e_add_attributes_and_backfill.py b/backend/tests/migrations/test_8202f7b5953e_add_attributes_and_backfill.py new file mode 100644 index 000000000..34395c4d8 --- /dev/null +++ b/backend/tests/migrations/test_8202f7b5953e_add_attributes_and_backfill.py @@ -0,0 +1,294 @@ +# Copyright 2026 Canonical Ltd. +# +# This program is free software: you can redistribute it and/or modify +# it under the terms of the GNU Affero General Public License version 3, as +# published by the Free Software Foundation. +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU Affero General Public License for more details. +# You should have received a copy of the GNU Affero General Public License +# along with this program. If not, see . +# +# SPDX-FileCopyrightText: Copyright 2026 Canonical Ltd. +# SPDX-License-Identifier: AGPL-3.0-only + +"""Tests for the expand half: adding artefact.attributes and backfilling it. + +This migration only adds the ``attributes`` column and copies existing bundled +build data into it. The old ``bundled_builds_hash`` column and +``artefact_bundled_builds_association`` table remain in place so old code keeps +working during a rolling upgrade; they are removed by a later migration. +""" + +from collections.abc import Generator +from urllib.parse import urlparse, urlunparse + +import pytest +from alembic import command +from alembic.config import Config +from sqlalchemy import Engine, create_engine, text +from sqlalchemy.engine import Connection +from sqlalchemy_utils import create_database, database_exists, drop_database # type: ignore[import-untyped] + +PREVIOUS_REV = "eba1d1c92dba" +TARGET_REV = "8202f7b5953e" + + +@pytest.fixture +def migration_context(db_url: str) -> Generator[tuple[Engine, Config], None, None]: + parsed = urlparse(db_url) + test_db_url = urlunparse( + (parsed.scheme, parsed.netloc, "/test_migration_solution_attrs", parsed.params, parsed.query, parsed.fragment) + ) + + if database_exists(test_db_url): + drop_database(test_db_url) + + create_database(test_db_url) + + engine: Engine | None = None + try: + engine = create_engine(test_db_url) + alembic_config = Config("alembic.ini") + alembic_config.set_main_option("sqlalchemy.url", test_db_url) + + yield engine, alembic_config + finally: + if engine is not None: + engine.dispose() + if database_exists(test_db_url): + drop_database(test_db_url) + + +def _insert_artefact( + conn: Connection, + name: str, + bundled_builds_hash: str | None = None, + attributes: str | None = None, + version: str = "1.0", + track: str = "latest", + source: str = "source", + stage: str = "stable", +) -> int: + optional_column = ", attributes" if attributes is not None else ", bundled_builds_hash" + optional_value = ", CAST(:attributes AS jsonb)" if attributes is not None else ", :bundled_builds_hash" + result = conn.execute( + text(f""" + INSERT INTO artefact ( + name, version, stage, family, status, archived, bug_link, comment, + store, branch, track, series, repo, source, os, release, sha256, owner, image_url, + created_at, updated_at{optional_column} + ) + VALUES ( + :name, :version, :stage, 'solution', 'UNDECIDED', false, '', '', + '', '', :track, '', '', :source, '', '', '', '', '', + NOW(), NOW(){optional_value} + ) + RETURNING id + """), + { + "name": name, + "version": version, + "stage": stage, + "track": track, + "source": source, + "bundled_builds_hash": bundled_builds_hash, + "attributes": attributes, + }, + ) + return result.scalar_one() + + +def _insert_artefact_build(conn: Connection, artefact_id: int, architecture: str = "amd64") -> int: + result = conn.execute( + text(""" + INSERT INTO artefact_build (architecture, revision, artefact_id, created_at, updated_at) + VALUES (:architecture, NULL, :artefact_id, NOW(), NOW()) + RETURNING id + """), + {"architecture": architecture, "artefact_id": artefact_id}, + ) + return result.scalar_one() + + +def _insert_association(conn: Connection, artefact_id: int, artefact_build_id: int) -> None: + conn.execute( + text(""" + INSERT INTO artefact_bundled_builds_association (artefact_id, artefact_build_id) + VALUES (:artefact_id, :artefact_build_id) + """), + {"artefact_id": artefact_id, "artefact_build_id": artefact_build_id}, + ) + + +def _attribute_text(engine: Engine, artefact_id: int, key: str) -> str | None: + with engine.connect() as conn: + return conn.execute( + text("SELECT attributes::jsonb ->> :key FROM artefact WHERE id = :artefact_id"), + {"artefact_id": artefact_id, "key": key}, + ).scalar_one() + + +def _attribute_key_exists(engine: Engine, artefact_id: int, key: str) -> bool: + with engine.connect() as conn: + return conn.execute( + text("SELECT attributes::jsonb ? :key FROM artefact WHERE id = :artefact_id"), + {"artefact_id": artefact_id, "key": key}, + ).scalar_one() + + +def _bundled_build_ids(engine: Engine, artefact_id: int) -> list[int]: + with engine.connect() as conn: + return list( + conn.execute( + text(""" + SELECT jsonb_array_elements_text(attributes::jsonb -> 'bundled_builds')::int + FROM artefact + WHERE id = :artefact_id + """), + {"artefact_id": artefact_id}, + ).scalars() + ) + + +def test_upgrade_preserves_empty_attributes(migration_context: tuple[Engine, Config]) -> None: + engine, alembic_config = migration_context + command.upgrade(alembic_config, PREVIOUS_REV) + with engine.begin() as conn: + artefact_id = _insert_artefact(conn, "solution-empty") + + command.upgrade(alembic_config, TARGET_REV) + + with engine.connect() as conn: + assert ( + conn.execute( + text("SELECT attributes::jsonb FROM artefact WHERE id = :id"), {"id": artefact_id} + ).scalar_one() + == {} + ) + + +def test_upgrade_copies_hash_and_associations(migration_context: tuple[Engine, Config]) -> None: + engine, alembic_config = migration_context + command.upgrade(alembic_config, PREVIOUS_REV) + with engine.begin() as conn: + artefact_id = _insert_artefact(conn, "solution-with-both", bundled_builds_hash="hash-value") + build_id = _insert_artefact_build(conn, artefact_id) + _insert_association(conn, artefact_id, build_id) + + command.upgrade(alembic_config, TARGET_REV) + + assert _attribute_text(engine, artefact_id, "bundled_builds_hash") == "hash-value" + assert _bundled_build_ids(engine, artefact_id) == [build_id] + + +def test_upgrade_copies_only_hash_when_no_associations(migration_context: tuple[Engine, Config]) -> None: + engine, alembic_config = migration_context + command.upgrade(alembic_config, PREVIOUS_REV) + with engine.begin() as conn: + artefact_id = _insert_artefact(conn, "solution-with-hash", bundled_builds_hash="hash-only") + + command.upgrade(alembic_config, TARGET_REV) + + assert _attribute_text(engine, artefact_id, "bundled_builds_hash") == "hash-only" + assert not _attribute_key_exists(engine, artefact_id, "bundled_builds") + + +def test_upgrade_copies_only_associations_when_hash_null(migration_context: tuple[Engine, Config]) -> None: + engine, alembic_config = migration_context + command.upgrade(alembic_config, PREVIOUS_REV) + with engine.begin() as conn: + artefact_id = _insert_artefact(conn, "solution-with-assoc") + build_id = _insert_artefact_build(conn, artefact_id) + _insert_association(conn, artefact_id, build_id) + + command.upgrade(alembic_config, TARGET_REV) + + assert not _attribute_key_exists(engine, artefact_id, "bundled_builds_hash") + assert _bundled_build_ids(engine, artefact_id) == [build_id] + + +def test_upgrade_copies_multiple_bundled_builds_in_ascending_order(migration_context: tuple[Engine, Config]) -> None: + engine, alembic_config = migration_context + command.upgrade(alembic_config, PREVIOUS_REV) + with engine.begin() as conn: + artefact_id = _insert_artefact(conn, "solution-with-many") + first_build_id = _insert_artefact_build(conn, artefact_id, architecture="amd64") + second_build_id = _insert_artefact_build(conn, artefact_id, architecture="arm64") + _insert_association(conn, artefact_id, second_build_id) + _insert_association(conn, artefact_id, first_build_id) + + command.upgrade(alembic_config, TARGET_REV) + + assert _bundled_build_ids(engine, artefact_id) == [first_build_id, second_build_id] + + +def test_upgrade_schema_changes(migration_context: tuple[Engine, Config]) -> None: + """The expand migration adds attributes but must leave the old column/table in place.""" + engine, alembic_config = migration_context + command.upgrade(alembic_config, TARGET_REV) + + with engine.connect() as conn: + attributes_column = conn.execute( + text(""" + SELECT is_nullable, column_default + FROM information_schema.columns + WHERE table_name = 'artefact' AND column_name = 'attributes' + """) + ).fetchone() + association_table = conn.execute( + text(""" + SELECT table_name + FROM information_schema.tables + WHERE table_name = 'artefact_bundled_builds_association' + """) + ).fetchone() + bundled_hash_column = conn.execute( + text(""" + SELECT column_name + FROM information_schema.columns + WHERE table_name = 'artefact' AND column_name = 'bundled_builds_hash' + """) + ).fetchone() + + assert attributes_column is not None + assert attributes_column[0] == "NO" + assert "'{}'" in attributes_column[1] + # Old schema must still be present after the expand migration. + assert association_table is not None + assert bundled_hash_column is not None + + +def test_downgrade_schema_changes(migration_context: tuple[Engine, Config]) -> None: + """Downgrading the expand migration only removes the attributes column.""" + engine, alembic_config = migration_context + command.upgrade(alembic_config, TARGET_REV) + command.downgrade(alembic_config, PREVIOUS_REV) + + with engine.connect() as conn: + attributes_column = conn.execute( + text(""" + SELECT column_name + FROM information_schema.columns + WHERE table_name = 'artefact' AND column_name = 'attributes' + """) + ).fetchone() + association_table = conn.execute( + text(""" + SELECT table_name + FROM information_schema.tables + WHERE table_name = 'artefact_bundled_builds_association' + """) + ).fetchone() + bundled_hash_column = conn.execute( + text(""" + SELECT column_name + FROM information_schema.columns + WHERE table_name = 'artefact' AND column_name = 'bundled_builds_hash' + """) + ).fetchone() + + assert attributes_column is None + assert association_table is not None + assert bundled_hash_column is not None diff --git a/backend/tests/migrations/test_8202f7b5953e_replace_solution_specific_fields.py b/backend/tests/migrations/test_8bd1f5009f02_drop_solution_specific_bundled_build_fields.py similarity index 71% rename from backend/tests/migrations/test_8202f7b5953e_replace_solution_specific_fields.py rename to backend/tests/migrations/test_8bd1f5009f02_drop_solution_specific_bundled_build_fields.py index 638488ae7..8172e8a31 100644 --- a/backend/tests/migrations/test_8202f7b5953e_replace_solution_specific_fields.py +++ b/backend/tests/migrations/test_8bd1f5009f02_drop_solution_specific_bundled_build_fields.py @@ -13,7 +13,15 @@ # SPDX-FileCopyrightText: Copyright 2026 Canonical Ltd. # SPDX-License-Identifier: AGPL-3.0-only -"""Tests for replacing bundled build fields with generic artefact attributes.""" +"""Tests for the contract half: dropping the solution-specific bundled build fields. + +This migration swaps the ``unique_solution`` index to ``(name, version)`` and +drops ``artefact.bundled_builds_hash`` and ``artefact_bundled_builds_association``. +It re-runs the backfill first so rows written by not-yet-upgraded units during +the expand rollout are copied into ``attributes`` before the columns are dropped. + +``PREVIOUS_REV`` is the expand migration (attributes already added and backfilled). +""" from collections.abc import Generator from urllib.parse import urlparse, urlunparse @@ -25,15 +33,17 @@ from sqlalchemy.engine import Connection from sqlalchemy_utils import create_database, database_exists, drop_database # type: ignore[import-untyped] -PREVIOUS_REV = "eba1d1c92dba" -TARGET_REV = "8202f7b5953e" +# The expand migration (adds + backfills attributes; old column/table still present). +PREVIOUS_REV = "8202f7b5953e" +# The contract migration under test (drops old column/table, swaps index). +TARGET_REV = "8bd1f5009f02" @pytest.fixture def migration_context(db_url: str) -> Generator[tuple[Engine, Config], None, None]: parsed = urlparse(db_url) test_db_url = urlunparse( - (parsed.scheme, parsed.netloc, "/test_migration_solution_attrs", parsed.params, parsed.query, parsed.fragment) + (parsed.scheme, parsed.netloc, "/test_migration_drop_bundled", parsed.params, parsed.query, parsed.fragment) ) if database_exists(test_db_url): @@ -55,29 +65,29 @@ def migration_context(db_url: str) -> Generator[tuple[Engine, Config], None, Non drop_database(test_db_url) -def _insert_artefact( +def _insert_legacy_artefact( conn: Connection, name: str, bundled_builds_hash: str | None = None, - attributes: str | None = None, version: str = "1.0", track: str = "latest", source: str = "source", stage: str = "stable", ) -> int: - optional_column = ", attributes" if attributes is not None else ", bundled_builds_hash" - optional_value = ", CAST(:attributes AS jsonb)" if attributes is not None else ", :bundled_builds_hash" + """Insert a solution artefact at the expand revision, where both the ``attributes`` + column and the legacy ``bundled_builds_hash`` column exist. ``attributes`` is left at + its server default ({}) to simulate rows written by not-yet-upgraded code.""" result = conn.execute( - text(f""" + text(""" INSERT INTO artefact ( name, version, stage, family, status, archived, bug_link, comment, store, branch, track, series, repo, source, os, release, sha256, owner, image_url, - created_at, updated_at{optional_column} + created_at, updated_at, bundled_builds_hash ) VALUES ( :name, :version, :stage, 'solution', 'UNDECIDED', false, '', '', '', '', :track, '', '', :source, '', '', '', '', '', - NOW(), NOW(){optional_value} + NOW(), NOW(), :bundled_builds_hash ) RETURNING id """), @@ -88,6 +98,42 @@ def _insert_artefact( "track": track, "source": source, "bundled_builds_hash": bundled_builds_hash, + }, + ) + return result.scalar_one() + + +def _insert_artefact( + conn: Connection, + name: str, + attributes: str = "{}", + version: str = "1.0", + track: str = "latest", + source: str = "source", + stage: str = "stable", +) -> int: + """Insert a solution artefact at the contracted revision, where only the ``attributes`` + column exists (``bundled_builds_hash`` has been dropped).""" + result = conn.execute( + text(""" + INSERT INTO artefact ( + name, version, stage, family, status, archived, bug_link, comment, + store, branch, track, series, repo, source, os, release, sha256, owner, image_url, + created_at, updated_at, attributes + ) + VALUES ( + :name, :version, :stage, 'solution', 'UNDECIDED', false, '', '', + '', '', :track, '', '', :source, '', '', '', '', '', + NOW(), NOW(), CAST(:attributes AS jsonb) + ) + RETURNING id + """), + { + "name": name, + "version": version, + "stage": stage, + "track": track, + "source": source, "attributes": attributes, }, ) @@ -124,14 +170,6 @@ def _attribute_text(engine: Engine, artefact_id: int, key: str) -> str | None: ).scalar_one() -def _attribute_key_exists(engine: Engine, artefact_id: int, key: str) -> bool: - with engine.connect() as conn: - return conn.execute( - text("SELECT attributes::jsonb ? :key FROM artefact WHERE id = :artefact_id"), - {"artefact_id": artefact_id, "key": key}, - ).scalar_one() - - def _bundled_build_ids(engine: Engine, artefact_id: int) -> list[int]: with engine.connect() as conn: return list( @@ -146,28 +184,13 @@ def _bundled_build_ids(engine: Engine, artefact_id: int) -> list[int]: ) -def test_upgrade_preserves_empty_attributes(migration_context: tuple[Engine, Config]) -> None: - engine, alembic_config = migration_context - command.upgrade(alembic_config, PREVIOUS_REV) - with engine.begin() as conn: - artefact_id = _insert_artefact(conn, "solution-empty") - - command.upgrade(alembic_config, TARGET_REV) - - with engine.connect() as conn: - assert ( - conn.execute( - text("SELECT attributes::jsonb FROM artefact WHERE id = :id"), {"id": artefact_id} - ).scalar_one() - == {} - ) - - -def test_upgrade_copies_hash_and_associations(migration_context: tuple[Engine, Config]) -> None: +def test_upgrade_backfills_leftovers_before_dropping(migration_context: tuple[Engine, Config]) -> None: + """A row written by not-yet-upgraded code (legacy columns set, attributes still empty) + after the expand migration must be copied into attributes before the columns are dropped.""" engine, alembic_config = migration_context command.upgrade(alembic_config, PREVIOUS_REV) with engine.begin() as conn: - artefact_id = _insert_artefact(conn, "solution-with-both", bundled_builds_hash="hash-value") + artefact_id = _insert_legacy_artefact(conn, "solution-leftover", bundled_builds_hash="hash-value") build_id = _insert_artefact_build(conn, artefact_id) _insert_association(conn, artefact_id, build_id) @@ -177,47 +200,6 @@ def test_upgrade_copies_hash_and_associations(migration_context: tuple[Engine, C assert _bundled_build_ids(engine, artefact_id) == [build_id] -def test_upgrade_copies_only_hash_when_no_associations(migration_context: tuple[Engine, Config]) -> None: - engine, alembic_config = migration_context - command.upgrade(alembic_config, PREVIOUS_REV) - with engine.begin() as conn: - artefact_id = _insert_artefact(conn, "solution-with-hash", bundled_builds_hash="hash-only") - - command.upgrade(alembic_config, TARGET_REV) - - assert _attribute_text(engine, artefact_id, "bundled_builds_hash") == "hash-only" - assert not _attribute_key_exists(engine, artefact_id, "bundled_builds") - - -def test_upgrade_copies_only_associations_when_hash_null(migration_context: tuple[Engine, Config]) -> None: - engine, alembic_config = migration_context - command.upgrade(alembic_config, PREVIOUS_REV) - with engine.begin() as conn: - artefact_id = _insert_artefact(conn, "solution-with-assoc") - build_id = _insert_artefact_build(conn, artefact_id) - _insert_association(conn, artefact_id, build_id) - - command.upgrade(alembic_config, TARGET_REV) - - assert not _attribute_key_exists(engine, artefact_id, "bundled_builds_hash") - assert _bundled_build_ids(engine, artefact_id) == [build_id] - - -def test_upgrade_copies_multiple_bundled_builds_in_ascending_order(migration_context: tuple[Engine, Config]) -> None: - engine, alembic_config = migration_context - command.upgrade(alembic_config, PREVIOUS_REV) - with engine.begin() as conn: - artefact_id = _insert_artefact(conn, "solution-with-many") - first_build_id = _insert_artefact_build(conn, artefact_id, architecture="amd64") - second_build_id = _insert_artefact_build(conn, artefact_id, architecture="arm64") - _insert_association(conn, artefact_id, second_build_id) - _insert_association(conn, artefact_id, first_build_id) - - command.upgrade(alembic_config, TARGET_REV) - - assert _bundled_build_ids(engine, artefact_id) == [first_build_id, second_build_id] - - def test_downgrade_restores_empty_attributes(migration_context: tuple[Engine, Config]) -> None: engine, alembic_config = migration_context command.upgrade(alembic_config, TARGET_REV) @@ -285,28 +267,28 @@ def test_downgrade_restores_hash_and_associations(migration_context: tuple[Engin def test_upgrade_fails_fast_on_duplicate_name_and_version(migration_context: tuple[Engine, Config]) -> None: - """Pre-migration schema allows several solutions sharing (name, version) as long as track/source - differ; the upgrade must refuse to create the tighter (name, version) unique index rather than - fail with an opaque database error.""" + """The expand-revision schema still allows several solutions sharing (name, version) as long as + track/source differ; the contract upgrade must refuse to create the tighter (name, version) + unique index rather than fail with an opaque database error, and must not partially apply.""" engine, alembic_config = migration_context command.upgrade(alembic_config, PREVIOUS_REV) with engine.begin() as conn: - _insert_artefact(conn, "dup-solution", version="1.0", track="track-a", source="source-a") - _insert_artefact(conn, "dup-solution", version="1.0", track="track-b", source="source-b") + _insert_legacy_artefact(conn, "dup-solution", version="1.0", track="track-a", source="source-a") + _insert_legacy_artefact(conn, "dup-solution", version="1.0", track="track-b", source="source-b") with pytest.raises(RuntimeError, match="Cannot create unique index"): command.upgrade(alembic_config, TARGET_REV) - # The failed migration must not have partially applied its schema changes. + # The failed migration must not have dropped the legacy column. with engine.connect() as conn: - attributes_column = conn.execute( + bundled_hash_column = conn.execute( text(""" SELECT column_name FROM information_schema.columns - WHERE table_name = 'artefact' AND column_name = 'attributes' + WHERE table_name = 'artefact' AND column_name = 'bundled_builds_hash' """) ).fetchone() - assert attributes_column is None + assert bundled_hash_column is not None def test_downgrade_fails_fast_on_duplicate_widened_key(migration_context: tuple[Engine, Config]) -> None: @@ -330,7 +312,7 @@ def test_upgrade_schema_changes(migration_context: tuple[Engine, Config]) -> Non with engine.connect() as conn: attributes_column = conn.execute( text(""" - SELECT is_nullable, column_default + SELECT column_name FROM information_schema.columns WHERE table_name = 'artefact' AND column_name = 'attributes' """) @@ -351,8 +333,6 @@ def test_upgrade_schema_changes(migration_context: tuple[Engine, Config]) -> Non ).fetchone() assert attributes_column is not None - assert attributes_column[0] == "NO" - assert "'{}'" in attributes_column[1] assert association_table is None assert bundled_hash_column is None @@ -385,6 +365,8 @@ def test_downgrade_schema_changes(migration_context: tuple[Engine, Config]) -> N """) ).fetchone() - assert attributes_column is None + # Downgrading the contract migration restores the old schema but keeps attributes + # (attributes is removed only by downgrading the expand migration). + assert attributes_column is not None assert association_table is not None assert bundled_hash_column is not None From 402191386e6171b9bfae3ebafaf8284076c5c6e9 Mon Sep 17 00:00:00 2001 From: Raul Almeida Date: Tue, 11 Aug 2026 16:56:23 -0300 Subject: [PATCH 14/21] fix: failing test --- backend/tests/controllers/artefacts/test_artefacts.py | 1 - backend/tests/controllers/test_executions/test_reruns.py | 1 - 2 files changed, 2 deletions(-) diff --git a/backend/tests/controllers/artefacts/test_artefacts.py b/backend/tests/controllers/artefacts/test_artefacts.py index b80a5ee4a..e0be898ac 100644 --- a/backend/tests/controllers/artefacts/test_artefacts.py +++ b/backend/tests/controllers/artefacts/test_artefacts.py @@ -1257,7 +1257,6 @@ def _assert_get_artefact_response(response: dict[str, Any], artefact: Artefact) "attributes": artefact.attributes, "assignee": assignee, "reviewers": [], - "bundled_builds": [], "due_date": (artefact.due_date.strftime("%Y-%m-%d") if artefact.due_date else None), "bug_link": artefact.bug_link, "jira_issue": artefact.jira_issue, diff --git a/backend/tests/controllers/test_executions/test_reruns.py b/backend/tests/controllers/test_executions/test_reruns.py index bb5f8883f..822262268 100644 --- a/backend/tests/controllers/test_executions/test_reruns.py +++ b/backend/tests/controllers/test_executions/test_reruns.py @@ -168,7 +168,6 @@ def test_execution_to_pending_rerun(test_execution: TestExecution, priority: int "comment": test_execution.artefact_build.artefact.comment, "attributes": test_execution.artefact_build.artefact.attributes, "archived": test_execution.artefact_build.artefact.archived, - "bundled_builds": [], "assignee": ( test_execution.artefact_build.artefact.reviewers[0] if test_execution.artefact_build.artefact.reviewers From 08b4e9eea377ea475a27aea6e3b6ddc6a6e108ce Mon Sep 17 00:00:00 2001 From: Raul Almeida Date: Tue, 11 Aug 2026 17:14:20 -0300 Subject: [PATCH 15/21] fix: schema --- backend/schemata/openapi.json | 12 +----------- 1 file changed, 1 insertion(+), 11 deletions(-) diff --git a/backend/schemata/openapi.json b/backend/schemata/openapi.json index cadf12e02..1d369e462 100644 --- a/backend/schemata/openapi.json +++ b/backend/schemata/openapi.json @@ -6559,15 +6559,6 @@ ], "description": "Backward-compatible assignee field. Populated from the first entry in reviewers when present.", "readOnly": true - }, - "bundled_builds": { - "items": { - "$ref": "#/components/schemas/ArtefactBuildMinimalResponse" - }, - "type": "array", - "title": "Bundled Builds", - "deprecated": true, - "readOnly": true } }, "type": "object", @@ -6599,8 +6590,7 @@ "jira_issue", "all_environment_reviews_count", "completed_environment_reviews_count", - "assignee", - "bundled_builds" + "assignee" ], "title": "ArtefactResponse" }, From cf9032d731bf95c9a63359cc4ed160331e901656 Mon Sep 17 00:00:00 2001 From: Raul Almeida Date: Tue, 11 Aug 2026 17:48:09 -0300 Subject: [PATCH 16/21] chore: remove destructive migration from this branch --- ..._solution_specific_bundled_build_fields.py | 175 -------- ..._solution_specific_bundled_build_fields.py | 372 ------------------ 2 files changed, 547 deletions(-) delete mode 100644 backend/migrations/versions/2026_08_10_1716-8bd1f5009f02_drop_solution_specific_bundled_build_fields.py delete mode 100644 backend/tests/migrations/test_8bd1f5009f02_drop_solution_specific_bundled_build_fields.py diff --git a/backend/migrations/versions/2026_08_10_1716-8bd1f5009f02_drop_solution_specific_bundled_build_fields.py b/backend/migrations/versions/2026_08_10_1716-8bd1f5009f02_drop_solution_specific_bundled_build_fields.py deleted file mode 100644 index 2b574ae9b..000000000 --- a/backend/migrations/versions/2026_08_10_1716-8bd1f5009f02_drop_solution_specific_bundled_build_fields.py +++ /dev/null @@ -1,175 +0,0 @@ -# Copyright 2026 Canonical Ltd. -# -# This program is free software: you can redistribute it and/or modify -# it under the terms of the GNU Affero General Public License version 3, as -# published by the Free Software Foundation. -# This program is distributed in the hope that it will be useful, -# but WITHOUT ANY WARRANTY; without even the implied warranty of -# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the -# GNU Affero General Public License for more details. -# You should have received a copy of the GNU Affero General Public License -# along with this program. If not, see . -# -# SPDX-FileCopyrightText: Copyright 2026 Canonical Ltd. -# SPDX-License-Identifier: AGPL-3.0-only - -"""Drop solution-specific bundled build fields - -This is the destructive (contract) half of adding the ``attributes`` field -to artefacts and removing the solution-specific fields. -It swaps the ``unique_solution`` index to ``(name, version)`` and drops -``artefact.bundled_builds_hash`` and ``artefact_bundled_builds_association``. - -For a safe rolling upgrade this migration must only be deployed *after* the -expand migration (``8202f7b5953e``) and the code that stops using the old fields -have been fully rolled out. It re-runs the backfill first so that any rows -written by not-yet-upgraded units during that rollout (which populate the old -columns but not ``attributes``) are copied over before the columns are dropped. - -Revision ID: 8bd1f5009f02 -Revises: 8202f7b5953e -Create Date: 2026-08-10 17:16:00.000000+00:00 - -""" - -import sqlalchemy as sa -from alembic import op - -# revision identifiers, used by Alembic. -revision = "8bd1f5009f02" -down_revision = "8202f7b5953e" -branch_labels = None -depends_on = None - - -def upgrade() -> None: - # Catch leftover data written by old code during the rollout of the expand - # migration before the source columns/table are removed. - _copy_bundled_builds_to_attributes() - _assert_no_duplicate_solutions(["name", "version"]) - op.drop_index("unique_solution", table_name="artefact", postgresql_where="(family = 'solution'::familyname)") - op.create_index( - "unique_solution", "artefact", ["name", "version"], unique=True, postgresql_where=sa.text("family = 'solution'") - ) - op.drop_table("artefact_bundled_builds_association") - op.drop_column("artefact", "bundled_builds_hash") - - -def downgrade() -> None: - _add_bundled_builds() - _assert_no_duplicate_solutions( - ["name", "source", "version", "track", "stage", "bundled_builds_hash"], - nullable_columns=["bundled_builds_hash"], - ) - op.drop_index("unique_solution", table_name="artefact", postgresql_where=sa.text("family = 'solution'")) - op.create_index( - "unique_solution", - "artefact", - ["name", "source", "version", "track", "stage", "bundled_builds_hash"], - unique=True, - postgresql_where="(family = 'solution'::familyname)", - ) - - -def _assert_no_duplicate_solutions(key_columns: list[str], nullable_columns: list[str] | None = None) -> None: - """Fail fast with a clear error if applying a unique index on ``key_columns`` (scoped to - solution artefacts) would violate uniqueness, instead of letting index creation fail with an - opaque database error. - - Postgres unique indexes treat NULL as distinct from any other value (including another NULL), - so columns listed in ``nullable_columns`` are excluded from the duplicate search whenever they - are NULL, matching the semantics of the index we're about to create. - """ - nullable_columns = nullable_columns or [] - columns_sql = ", ".join(key_columns) - not_null_clauses = " AND ".join(f"{column} IS NOT NULL" for column in nullable_columns) - where_clause = f"family = 'solution' AND {not_null_clauses}" if not_null_clauses else "family = 'solution'" - - conn = op.get_bind() - duplicates = conn.execute( - sa.text(f""" - SELECT {columns_sql}, COUNT(*) AS duplicate_count - FROM artefact - WHERE {where_clause} - GROUP BY {columns_sql} - HAVING COUNT(*) > 1 - LIMIT 5 - """) # noqa: S608 - key_columns/nullable_columns are fixed, developer-controlled constants - ).fetchall() - - if duplicates: - raise RuntimeError( - f"Cannot create unique index on solutions ({columns_sql}): found existing duplicate rows " - f"(showing up to 5): {duplicates}. Resolve these duplicates manually before running this migration." - ) - - -def _add_bundled_builds() -> None: - op.add_column( - "artefact", sa.Column("bundled_builds_hash", sa.VARCHAR(length=64), autoincrement=False, nullable=True) - ) - op.create_table( - "artefact_bundled_builds_association", - sa.Column("artefact_id", sa.INTEGER(), autoincrement=False, nullable=False), - sa.Column("artefact_build_id", sa.INTEGER(), autoincrement=False, nullable=False), - sa.ForeignKeyConstraint( - ["artefact_build_id"], - ["artefact_build.id"], - name="artefact_bundled_builds_association_id_fkey", - ondelete="CASCADE", - ), - sa.ForeignKeyConstraint( - ["artefact_id"], ["artefact.id"], name="artefact_bundled_builds_artefact_id_fkey", ondelete="CASCADE" - ), - sa.PrimaryKeyConstraint("artefact_id", "artefact_build_id", name="artefact_bundled_builds_association_pkey"), - ) - _restore_bundled_builds_from_attributes() - - -def _copy_bundled_builds_to_attributes() -> None: - op.execute( - """ - UPDATE artefact AS a - SET attributes = a.attributes - || jsonb_strip_nulls( - jsonb_build_object('bundled_builds_hash', a.bundled_builds_hash) - ) - || COALESCE( - ( - SELECT jsonb_build_object( - 'bundled_builds', - jsonb_agg(assoc.artefact_build_id ORDER BY assoc.artefact_build_id) - ) - FROM artefact_bundled_builds_association assoc - WHERE assoc.artefact_id = a.id - HAVING count(*) > 0 - ), - '{}'::jsonb - ) - WHERE a.bundled_builds_hash IS NOT NULL - OR EXISTS ( - SELECT 1 - FROM artefact_bundled_builds_association assoc - WHERE assoc.artefact_id = a.id - ) - """ - ) - - -def _restore_bundled_builds_from_attributes() -> None: - op.execute( - """ - UPDATE artefact AS a - SET bundled_builds_hash = a.attributes ->> 'bundled_builds_hash' - WHERE a.attributes ? 'bundled_builds_hash' - """ - ) - op.execute( - """ - INSERT INTO artefact_bundled_builds_association (artefact_id, artefact_build_id) - SELECT a.id, elem::int - FROM artefact a, - jsonb_array_elements_text(a.attributes -> 'bundled_builds') AS elem - WHERE a.attributes ? 'bundled_builds' - """ - ) diff --git a/backend/tests/migrations/test_8bd1f5009f02_drop_solution_specific_bundled_build_fields.py b/backend/tests/migrations/test_8bd1f5009f02_drop_solution_specific_bundled_build_fields.py deleted file mode 100644 index 8172e8a31..000000000 --- a/backend/tests/migrations/test_8bd1f5009f02_drop_solution_specific_bundled_build_fields.py +++ /dev/null @@ -1,372 +0,0 @@ -# Copyright 2026 Canonical Ltd. -# -# This program is free software: you can redistribute it and/or modify -# it under the terms of the GNU Affero General Public License version 3, as -# published by the Free Software Foundation. -# This program is distributed in the hope that it will be useful, -# but WITHOUT ANY WARRANTY; without even the implied warranty of -# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the -# GNU Affero General Public License for more details. -# You should have received a copy of the GNU Affero General Public License -# along with this program. If not, see . -# -# SPDX-FileCopyrightText: Copyright 2026 Canonical Ltd. -# SPDX-License-Identifier: AGPL-3.0-only - -"""Tests for the contract half: dropping the solution-specific bundled build fields. - -This migration swaps the ``unique_solution`` index to ``(name, version)`` and -drops ``artefact.bundled_builds_hash`` and ``artefact_bundled_builds_association``. -It re-runs the backfill first so rows written by not-yet-upgraded units during -the expand rollout are copied into ``attributes`` before the columns are dropped. - -``PREVIOUS_REV`` is the expand migration (attributes already added and backfilled). -""" - -from collections.abc import Generator -from urllib.parse import urlparse, urlunparse - -import pytest -from alembic import command -from alembic.config import Config -from sqlalchemy import Engine, create_engine, text -from sqlalchemy.engine import Connection -from sqlalchemy_utils import create_database, database_exists, drop_database # type: ignore[import-untyped] - -# The expand migration (adds + backfills attributes; old column/table still present). -PREVIOUS_REV = "8202f7b5953e" -# The contract migration under test (drops old column/table, swaps index). -TARGET_REV = "8bd1f5009f02" - - -@pytest.fixture -def migration_context(db_url: str) -> Generator[tuple[Engine, Config], None, None]: - parsed = urlparse(db_url) - test_db_url = urlunparse( - (parsed.scheme, parsed.netloc, "/test_migration_drop_bundled", parsed.params, parsed.query, parsed.fragment) - ) - - if database_exists(test_db_url): - drop_database(test_db_url) - - create_database(test_db_url) - - engine: Engine | None = None - try: - engine = create_engine(test_db_url) - alembic_config = Config("alembic.ini") - alembic_config.set_main_option("sqlalchemy.url", test_db_url) - - yield engine, alembic_config - finally: - if engine is not None: - engine.dispose() - if database_exists(test_db_url): - drop_database(test_db_url) - - -def _insert_legacy_artefact( - conn: Connection, - name: str, - bundled_builds_hash: str | None = None, - version: str = "1.0", - track: str = "latest", - source: str = "source", - stage: str = "stable", -) -> int: - """Insert a solution artefact at the expand revision, where both the ``attributes`` - column and the legacy ``bundled_builds_hash`` column exist. ``attributes`` is left at - its server default ({}) to simulate rows written by not-yet-upgraded code.""" - result = conn.execute( - text(""" - INSERT INTO artefact ( - name, version, stage, family, status, archived, bug_link, comment, - store, branch, track, series, repo, source, os, release, sha256, owner, image_url, - created_at, updated_at, bundled_builds_hash - ) - VALUES ( - :name, :version, :stage, 'solution', 'UNDECIDED', false, '', '', - '', '', :track, '', '', :source, '', '', '', '', '', - NOW(), NOW(), :bundled_builds_hash - ) - RETURNING id - """), - { - "name": name, - "version": version, - "stage": stage, - "track": track, - "source": source, - "bundled_builds_hash": bundled_builds_hash, - }, - ) - return result.scalar_one() - - -def _insert_artefact( - conn: Connection, - name: str, - attributes: str = "{}", - version: str = "1.0", - track: str = "latest", - source: str = "source", - stage: str = "stable", -) -> int: - """Insert a solution artefact at the contracted revision, where only the ``attributes`` - column exists (``bundled_builds_hash`` has been dropped).""" - result = conn.execute( - text(""" - INSERT INTO artefact ( - name, version, stage, family, status, archived, bug_link, comment, - store, branch, track, series, repo, source, os, release, sha256, owner, image_url, - created_at, updated_at, attributes - ) - VALUES ( - :name, :version, :stage, 'solution', 'UNDECIDED', false, '', '', - '', '', :track, '', '', :source, '', '', '', '', '', - NOW(), NOW(), CAST(:attributes AS jsonb) - ) - RETURNING id - """), - { - "name": name, - "version": version, - "stage": stage, - "track": track, - "source": source, - "attributes": attributes, - }, - ) - return result.scalar_one() - - -def _insert_artefact_build(conn: Connection, artefact_id: int, architecture: str = "amd64") -> int: - result = conn.execute( - text(""" - INSERT INTO artefact_build (architecture, revision, artefact_id, created_at, updated_at) - VALUES (:architecture, NULL, :artefact_id, NOW(), NOW()) - RETURNING id - """), - {"architecture": architecture, "artefact_id": artefact_id}, - ) - return result.scalar_one() - - -def _insert_association(conn: Connection, artefact_id: int, artefact_build_id: int) -> None: - conn.execute( - text(""" - INSERT INTO artefact_bundled_builds_association (artefact_id, artefact_build_id) - VALUES (:artefact_id, :artefact_build_id) - """), - {"artefact_id": artefact_id, "artefact_build_id": artefact_build_id}, - ) - - -def _attribute_text(engine: Engine, artefact_id: int, key: str) -> str | None: - with engine.connect() as conn: - return conn.execute( - text("SELECT attributes::jsonb ->> :key FROM artefact WHERE id = :artefact_id"), - {"artefact_id": artefact_id, "key": key}, - ).scalar_one() - - -def _bundled_build_ids(engine: Engine, artefact_id: int) -> list[int]: - with engine.connect() as conn: - return list( - conn.execute( - text(""" - SELECT jsonb_array_elements_text(attributes::jsonb -> 'bundled_builds')::int - FROM artefact - WHERE id = :artefact_id - """), - {"artefact_id": artefact_id}, - ).scalars() - ) - - -def test_upgrade_backfills_leftovers_before_dropping(migration_context: tuple[Engine, Config]) -> None: - """A row written by not-yet-upgraded code (legacy columns set, attributes still empty) - after the expand migration must be copied into attributes before the columns are dropped.""" - engine, alembic_config = migration_context - command.upgrade(alembic_config, PREVIOUS_REV) - with engine.begin() as conn: - artefact_id = _insert_legacy_artefact(conn, "solution-leftover", bundled_builds_hash="hash-value") - build_id = _insert_artefact_build(conn, artefact_id) - _insert_association(conn, artefact_id, build_id) - - command.upgrade(alembic_config, TARGET_REV) - - assert _attribute_text(engine, artefact_id, "bundled_builds_hash") == "hash-value" - assert _bundled_build_ids(engine, artefact_id) == [build_id] - - -def test_downgrade_restores_empty_attributes(migration_context: tuple[Engine, Config]) -> None: - engine, alembic_config = migration_context - command.upgrade(alembic_config, TARGET_REV) - with engine.begin() as conn: - artefact_id = _insert_artefact(conn, "solution-downgrade-empty", attributes="{}") - - command.downgrade(alembic_config, PREVIOUS_REV) - - with engine.connect() as conn: - bundled_hash = conn.execute( - text("SELECT bundled_builds_hash FROM artefact WHERE id = :id"), - {"id": artefact_id}, - ).scalar_one() - association_count = conn.execute( - text("SELECT count(*) FROM artefact_bundled_builds_association WHERE artefact_id = :id"), - {"id": artefact_id}, - ).scalar_one() - assert bundled_hash is None - assert association_count == 0 - - -def test_downgrade_restores_hash_and_associations(migration_context: tuple[Engine, Config]) -> None: - engine, alembic_config = migration_context - command.upgrade(alembic_config, TARGET_REV) - with engine.begin() as conn: - artefact_id = _insert_artefact(conn, "solution-downgrade-both", attributes="{}") - first_build_id = _insert_artefact_build(conn, artefact_id, architecture="amd64") - second_build_id = _insert_artefact_build(conn, artefact_id, architecture="arm64") - conn.execute( - text(""" - UPDATE artefact - SET attributes = jsonb_build_object( - 'bundled_builds_hash', 'restored-hash', - 'bundled_builds', jsonb_build_array(CAST(:first_build_id AS int), CAST(:second_build_id AS int)) - ) - WHERE id = :artefact_id - """), - { - "artefact_id": artefact_id, - "first_build_id": first_build_id, - "second_build_id": second_build_id, - }, - ) - - command.downgrade(alembic_config, PREVIOUS_REV) - - with engine.connect() as conn: - bundled_hash = conn.execute( - text("SELECT bundled_builds_hash FROM artefact WHERE id = :id"), - {"id": artefact_id}, - ).scalar_one() - association_ids = list( - conn.execute( - text(""" - SELECT artefact_build_id - FROM artefact_bundled_builds_association - WHERE artefact_id = :id - ORDER BY artefact_build_id - """), - {"id": artefact_id}, - ).scalars() - ) - assert bundled_hash == "restored-hash" - assert association_ids == [first_build_id, second_build_id] - - -def test_upgrade_fails_fast_on_duplicate_name_and_version(migration_context: tuple[Engine, Config]) -> None: - """The expand-revision schema still allows several solutions sharing (name, version) as long as - track/source differ; the contract upgrade must refuse to create the tighter (name, version) - unique index rather than fail with an opaque database error, and must not partially apply.""" - engine, alembic_config = migration_context - command.upgrade(alembic_config, PREVIOUS_REV) - with engine.begin() as conn: - _insert_legacy_artefact(conn, "dup-solution", version="1.0", track="track-a", source="source-a") - _insert_legacy_artefact(conn, "dup-solution", version="1.0", track="track-b", source="source-b") - - with pytest.raises(RuntimeError, match="Cannot create unique index"): - command.upgrade(alembic_config, TARGET_REV) - - # The failed migration must not have dropped the legacy column. - with engine.connect() as conn: - bundled_hash_column = conn.execute( - text(""" - SELECT column_name - FROM information_schema.columns - WHERE table_name = 'artefact' AND column_name = 'bundled_builds_hash' - """) - ).fetchone() - assert bundled_hash_column is not None - - -def test_downgrade_fails_fast_on_duplicate_widened_key(migration_context: tuple[Engine, Config]) -> None: - """Defends the downgrade's wider unique index the same way, in case data ever ends up violating - it (e.g. the (name, version) index was bypassed or dropped out-of-band).""" - engine, alembic_config = migration_context - command.upgrade(alembic_config, TARGET_REV) - with engine.begin() as conn: - conn.execute(text("DROP INDEX unique_solution")) - _insert_artefact(conn, "dup-solution", attributes='{"bundled_builds_hash": "hash-x"}') - _insert_artefact(conn, "dup-solution", attributes='{"bundled_builds_hash": "hash-x"}') - - with pytest.raises(RuntimeError, match="Cannot create unique index"): - command.downgrade(alembic_config, PREVIOUS_REV) - - -def test_upgrade_schema_changes(migration_context: tuple[Engine, Config]) -> None: - engine, alembic_config = migration_context - command.upgrade(alembic_config, TARGET_REV) - - with engine.connect() as conn: - attributes_column = conn.execute( - text(""" - SELECT column_name - FROM information_schema.columns - WHERE table_name = 'artefact' AND column_name = 'attributes' - """) - ).fetchone() - association_table = conn.execute( - text(""" - SELECT table_name - FROM information_schema.tables - WHERE table_name = 'artefact_bundled_builds_association' - """) - ).fetchone() - bundled_hash_column = conn.execute( - text(""" - SELECT column_name - FROM information_schema.columns - WHERE table_name = 'artefact' AND column_name = 'bundled_builds_hash' - """) - ).fetchone() - - assert attributes_column is not None - assert association_table is None - assert bundled_hash_column is None - - -def test_downgrade_schema_changes(migration_context: tuple[Engine, Config]) -> None: - engine, alembic_config = migration_context - command.upgrade(alembic_config, TARGET_REV) - command.downgrade(alembic_config, PREVIOUS_REV) - - with engine.connect() as conn: - attributes_column = conn.execute( - text(""" - SELECT column_name - FROM information_schema.columns - WHERE table_name = 'artefact' AND column_name = 'attributes' - """) - ).fetchone() - association_table = conn.execute( - text(""" - SELECT table_name - FROM information_schema.tables - WHERE table_name = 'artefact_bundled_builds_association' - """) - ).fetchone() - bundled_hash_column = conn.execute( - text(""" - SELECT column_name - FROM information_schema.columns - WHERE table_name = 'artefact' AND column_name = 'bundled_builds_hash' - """) - ).fetchone() - - # Downgrading the contract migration restores the old schema but keeps attributes - # (attributes is removed only by downgrading the expand migration). - assert attributes_column is not None - assert association_table is not None - assert bundled_hash_column is not None From 19079e84721ed92a7e0a5e435224e6d275aa613b Mon Sep 17 00:00:00 2001 From: Raul Almeida Date: Wed, 12 Aug 2026 12:56:14 -0300 Subject: [PATCH 17/21] chore: remove test that only belongs to next PR --- backend/tests/data_access/test_models.py | 32 ------------------------ 1 file changed, 32 deletions(-) diff --git a/backend/tests/data_access/test_models.py b/backend/tests/data_access/test_models.py index f4740a80e..d55c24c2b 100644 --- a/backend/tests/data_access/test_models.py +++ b/backend/tests/data_access/test_models.py @@ -57,14 +57,6 @@ def test_issue_url( assert result == expected -def test_solutions_with_same_name_and_version_are_unique(generator: DataGenerator, db_session: Session) -> None: - generator.gen_artefact(family=FamilyName.solution, name="solution", version="1.0") - - with pytest.raises(IntegrityError): - generator.gen_artefact(family=FamilyName.solution, name="solution", version="1.0") - db_session.rollback() - - def test_solutions_with_same_name_and_different_versions_are_allowed(generator: DataGenerator) -> None: first = generator.gen_artefact(family=FamilyName.solution, name="solution", version="1.0") second = generator.gen_artefact(family=FamilyName.solution, name="solution", version="2.0") @@ -86,30 +78,6 @@ def test_solutions_with_different_name_and_version_are_allowed(generator: DataGe assert first.id != second.id -def test_solution_unique_constraint_ignores_source_track_and_stage( - generator: DataGenerator, db_session: Session -) -> None: - generator.gen_artefact( - family=FamilyName.solution, - name="solution", - version="1.0", - source="first-source", - track="first-track", - stage=StageName.beta, - ) - - with pytest.raises(IntegrityError): - generator.gen_artefact( - family=FamilyName.solution, - name="solution", - version="1.0", - source="second-source", - track="second-track", - stage=StageName.stable, - ) - db_session.rollback() - - def test_snap_name_and_version_duplicates_are_not_blocked_by_solution_constraint(generator: DataGenerator) -> None: first = generator.gen_artefact( family=FamilyName.snap, From 94cf63ec5cc204fde628ac40f59bd5beb18d5234 Mon Sep 17 00:00:00 2001 From: Raul Almeida Date: Wed, 12 Aug 2026 13:48:04 -0300 Subject: [PATCH 18/21] chore: linting --- backend/tests/data_access/test_models.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/backend/tests/data_access/test_models.py b/backend/tests/data_access/test_models.py index d55c24c2b..1a332635d 100644 --- a/backend/tests/data_access/test_models.py +++ b/backend/tests/data_access/test_models.py @@ -14,11 +14,9 @@ # SPDX-License-Identifier: AGPL-3.0-only import pytest -from sqlalchemy.exc import IntegrityError -from sqlalchemy.orm import Session from test_observer.data_access.models import Issue -from test_observer.data_access.models_enums import FamilyName, IssueSource, StageName +from test_observer.data_access.models_enums import FamilyName, IssueSource from tests.data_generator import DataGenerator From 58ffaa1f1b1584c3f0fb5a9b72f81f590870bb34 Mon Sep 17 00:00:00 2001 From: Raul Almeida Date: Wed, 12 Aug 2026 15:00:44 -0300 Subject: [PATCH 19/21] fix: alembic check --- backend/migrations/env.py | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/backend/migrations/env.py b/backend/migrations/env.py index 1f288e70b..65352a6d8 100644 --- a/backend/migrations/env.py +++ b/backend/migrations/env.py @@ -30,6 +30,26 @@ fileConfig(config.config_file_name) +# Note: transitional expand/contract exclusions +# `alembic check` complains that the ORM models don't reference legacy solution- +# specific fields that were replaced by the `attributes` field. This happens +# because we are doing expand/contract migrations (for two separate releases); +# the following release will remove these exceptions. +_EXPAND_CONTRACT_IGNORED_TABLES = {"artefact_bundled_builds_association"} +_EXPAND_CONTRACT_IGNORED_COLUMNS = {("artefact", "bundled_builds_hash")} +_EXPAND_CONTRACT_IGNORED_INDEXES = {"unique_solution"} + + +def include_object(object, name, type_, reflected, compare_to): # noqa: ANN001, ANN201, ARG001 + if type_ == "table" and name in _EXPAND_CONTRACT_IGNORED_TABLES: + return False + if type_ == "column": + table_name = object.table.name if object.table is not None else None + if (table_name, name) in _EXPAND_CONTRACT_IGNORED_COLUMNS: + return False + return not (type_ == "index" and name in _EXPAND_CONTRACT_IGNORED_INDEXES) + + # Don't overwrite value if set by tests if config.get_main_option("sqlalchemy.url") is None: config.set_main_option("sqlalchemy.url", DB_URL) @@ -52,6 +72,7 @@ def run_migrations_offline() -> None: target_metadata=target_metadata, literal_binds=True, dialect_opts={"paramstyle": "named"}, + include_object=include_object, ) with context.begin_transaction(): @@ -76,6 +97,7 @@ def run_migrations_online() -> None: connection=connection, target_metadata=target_metadata, transaction_per_migration=True, + include_object=include_object, ) with context.begin_transaction(): From 04f869461133bdeb71bd1df1356352f811701d90 Mon Sep 17 00:00:00 2001 From: Raul Almeida Date: Wed, 12 Aug 2026 16:05:46 -0300 Subject: [PATCH 20/21] fix: mypy --- backend/migrations/env.py | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/backend/migrations/env.py b/backend/migrations/env.py index 65352a6d8..0248724cf 100644 --- a/backend/migrations/env.py +++ b/backend/migrations/env.py @@ -21,6 +21,8 @@ from test_observer.data_access import Base from test_observer.data_access.setup import DB_URL +from typing import Tuple + # for 'autogenerate' support target_metadata = Base.metadata @@ -35,9 +37,9 @@ # specific fields that were replaced by the `attributes` field. This happens # because we are doing expand/contract migrations (for two separate releases); # the following release will remove these exceptions. -_EXPAND_CONTRACT_IGNORED_TABLES = {"artefact_bundled_builds_association"} -_EXPAND_CONTRACT_IGNORED_COLUMNS = {("artefact", "bundled_builds_hash")} -_EXPAND_CONTRACT_IGNORED_INDEXES = {"unique_solution"} +_EXPAND_CONTRACT_IGNORED_TABLES: set[str] = {"artefact_bundled_builds_association"} +_EXPAND_CONTRACT_IGNORED_COLUMNS: set[Tuple[str, str]] = {("artefact", "bundled_builds_hash")} +_EXPAND_CONTRACT_IGNORED_INDEXES: set[str] = {"unique_solution"} def include_object(object, name, type_, reflected, compare_to): # noqa: ANN001, ANN201, ARG001 From a4cb61a465ffaadf1a36111f402783ec87bd3f7d Mon Sep 17 00:00:00 2001 From: Raul Almeida Date: Wed, 12 Aug 2026 16:07:35 -0300 Subject: [PATCH 21/21] fix: linting --- backend/migrations/env.py | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/backend/migrations/env.py b/backend/migrations/env.py index 0248724cf..affd1d7e2 100644 --- a/backend/migrations/env.py +++ b/backend/migrations/env.py @@ -21,8 +21,6 @@ from test_observer.data_access import Base from test_observer.data_access.setup import DB_URL -from typing import Tuple - # for 'autogenerate' support target_metadata = Base.metadata @@ -37,8 +35,8 @@ # specific fields that were replaced by the `attributes` field. This happens # because we are doing expand/contract migrations (for two separate releases); # the following release will remove these exceptions. -_EXPAND_CONTRACT_IGNORED_TABLES: set[str] = {"artefact_bundled_builds_association"} -_EXPAND_CONTRACT_IGNORED_COLUMNS: set[Tuple[str, str]] = {("artefact", "bundled_builds_hash")} +_EXPAND_CONTRACT_IGNORED_TABLES: set[str] = {"artefact_bundled_builds_association"} +_EXPAND_CONTRACT_IGNORED_COLUMNS: set[tuple[str, str]] = {("artefact", "bundled_builds_hash")} _EXPAND_CONTRACT_IGNORED_INDEXES: set[str] = {"unique_solution"}