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/migrations/env.py b/backend/migrations/env.py index 1f288e70b..affd1d7e2 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: 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 + 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(): 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/schemata/openapi.json b/backend/schemata/openapi.json index fd9da3427..1d369e462 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", @@ -8518,20 +8479,41 @@ "const": "solution", "title": "Family" }, + "attributes": { + "additionalProperties": true, + "type": "object", + "title": "Attributes" + }, + "execution_stage": { + "$ref": "#/components/schemas/SolutionStage", + "description": "Promotion stage of the solution being tested." + }, "track": { - "type": "string", + "anyOf": [ + { + "type": "string" + }, + { + "type": "null" + } + ], "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." + "description": "Legacy field. Merged into attributes['track'] when attributes['track'] is not provided.", + "deprecated": true }, "source": { - "type": "string", - "maxLength": 200, + "anyOf": [ + { + "type": "string", + "maxLength": 200 + }, + { + "type": "null" + } + ], "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'." - }, - "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": "Legacy field. Merged into attributes['source'] when attributes['source'] is not provided.", + "deprecated": true } }, "type": "object", @@ -8542,8 +8524,6 @@ "environment", "test_plan", "family", - "track", - "source", "execution_stage" ], "title": "StartSolutionTestExecutionRequest" 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" diff --git a/backend/test_observer/controllers/artefacts/artefacts.py b/backend/test_observer/controllers/artefacts/artefacts.py index 5838c7be7..8e9ffff0d 100644 --- a/backend/test_observer/controllers/artefacts/artefacts.py +++ b/backend/test_observer/controllers/artefacts/artefacts.py @@ -87,7 +87,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: @@ -96,7 +95,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, ) @@ -194,7 +192,6 @@ def get_artefact_history( .offset(offset) .options( selectinload(Artefact.builds).selectinload(ArtefactBuild.test_executions), - selectinload(Artefact.bundled_builds), ) ) @@ -227,7 +224,6 @@ def get_artefact( artefact: Artefact = Depends( ArtefactRetriever( selectinload(Artefact.builds).selectinload(ArtefactBuild.environment_reviews), - selectinload(Artefact.bundled_builds), ) ), ): @@ -247,7 +243,6 @@ def patch_artefact( artefact: Artefact = Depends( ArtefactRetriever( selectinload(Artefact.builds).selectinload(ArtefactBuild.environment_reviews), - selectinload(Artefact.bundled_builds), ) ), ): @@ -265,6 +260,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: + # 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 @@ -341,30 +339,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: @@ -415,12 +389,13 @@ 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( 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) @@ -428,8 +403,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..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).selectinload(ArtefactBuild.bundled_in), ) ), ): diff --git a/backend/test_observer/controllers/artefacts/models.py b/backend/test_observer/controllers/artefacts/models.py index e4198831d..75b887aa6 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 @@ -79,7 +80,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 +147,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 +155,7 @@ 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", - ) + attributes: dict[str, Any] | None = None assignee_id: int | None = Field( default=None, deprecated=True, @@ -215,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 diff --git a/backend/test_observer/controllers/test_executions/models.py b/backend/test_observer/controllers/test_executions/models.py index fc01f4ee8..ea9c05bc0 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,25 +285,44 @@ 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." + 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 = Field( + source: str | None = Field( + default=None, 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." + 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. An explicit null for + # attributes is left untouched so it falls through to normal (non-nullable) validation. + if "attributes" not in data: + attributes: dict[str, Any] = {} + else: + attributes = data["attributes"] + if attributes is None or not isinstance(attributes, dict): + return data + + 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): PASS = "pass" diff --git a/backend/test_observer/controllers/test_executions/start_test.py b/backend/test_observer/controllers/test_executions/start_test.py index f66c44fb4..707447333 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 @@ -348,15 +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 - filter_kwargs["bundled_builds_hash"] = calculate_bundled_builds_hash([]) - 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/test_observer/data_access/models.py b/backend/test_observer/data_access/models.py index fd095409b..b726ebe5c 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 @@ -345,14 +327,11 @@ class Artefact(Base): # 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" ) + 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 @@ -397,11 +376,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 +403,6 @@ def __repr__(self) -> str: "due_date", "status", "archived", - "bundled_builds_hash", ) @hybrid_property @@ -454,34 +428,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 +438,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..99d0ebce7 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]: """ @@ -40,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: @@ -136,9 +137,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 ce24eb548..2755f00ae 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 @@ -230,6 +229,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) @@ -332,6 +343,69 @@ 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_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) @@ -1048,6 +1122,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, @@ -1189,6 +1284,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), @@ -1197,7 +1293,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"] = [ @@ -1479,289 +1574,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..822262268 100644 --- a/backend/tests/controllers/test_executions/test_reruns.py +++ b/backend/tests/controllers/test_executions/test_reruns.py @@ -166,13 +166,13 @@ 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] 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/controllers/test_executions/test_start_test.py b/backend/tests/controllers/test_executions/test_start_test.py index 872a85150..4aa5d7376 100644 --- a/backend/tests/controllers/test_executions/test_start_test.py +++ b/backend/tests/controllers/test_executions/test_start_test.py @@ -121,19 +121,124 @@ 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 + + +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", + } + + +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 + + +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, 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 +1000,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 +1096,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 +1152,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..1a332635d 100644 --- a/backend/tests/data_access/test_models.py +++ b/backend/tests/data_access/test_models.py @@ -16,7 +16,8 @@ import pytest 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 +from tests.data_generator import DataGenerator @pytest.mark.parametrize( @@ -52,3 +53,41 @@ def test_issue_url( assert expected is None else: assert result == expected + + +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_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 fcdc29898..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,7 +148,7 @@ def gen_artefact( bug_link: str = "", due_date: date | None = None, reviewers: list[User] | None = None, - bundled_builds: list[ArtefactBuild] | None = None, + attributes: dict[str, Any] | None = None, ) -> Artefact: family = FamilyName(family) @@ -163,7 +164,7 @@ def gen_artefact( created_at = created_at or datetime.utcnow() reviewers = reviewers or [] - bundled_builds = bundled_builds or [] + attributes = attributes or {} artefact = Artefact( name=name, @@ -182,7 +183,7 @@ def gen_artefact( bug_link=bug_link, due_date=due_date, reviewers=reviewers, - bundled_builds=bundled_builds, + attributes=attributes, ) self._add_object(artefact) return artefact 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