Skip to content

TO-404: remove solution-specific fields - #835

Open
almeidaraul wants to merge 23 commits into
mainfrom
TO-404/remove-solution-specific-fields
Open

TO-404: remove solution-specific fields#835
almeidaraul wants to merge 23 commits into
mainfrom
TO-404/remove-solution-specific-fields

Conversation

@almeidaraul

Copy link
Copy Markdown
Contributor

Description

  • The attributes field is added to artefacts
  • Solution-specific fields are removed from artefacts (and moved to attributes
  • The uniqueness constraint for solutions becomes a combination of name and version
  • Solution-related triggers for calculating the hash are removed
  • There are tests for the new field
  • The new field is added to the API (i.e. relevant endpoints)

Resolved issues

Resolves TO-404

Documentation

Web service API changes

Updated the API accordingly

Tests

There are new unit tests

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR implements TO-404 by moving solution-specific artefact metadata into a generic attributes JSONB column, removing the bundled-builds schema and API surface, and updating solution uniqueness to be based on (name, version).

Changes:

  • Add Artefact.attributes (JSONB) and expose it via artefact GET/PATCH and solution start-test requests.
  • Remove bundled-builds model/API (bundled_builds_hash, association table, and related response fields).
  • Change solution uniqueness constraint to (name, version) and update tests/migration coverage accordingly.

Reviewed changes

Copilot reviewed 17 out of 17 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
backend/tests/migrations/test_8202f7b5953e_replace_solution_specific_fields.py Adds migration tests covering copying bundled-build fields into attributes and schema upgrade/downgrade behavior.
backend/tests/data_generator.py Updates test data generator to support creating artefacts with attributes.
backend/tests/data_access/test_models.py Adds tests for the new solution uniqueness constraint semantics.
backend/tests/controllers/test_executions/test_start_test.py Updates solution start-test behavior/tests for attributes and new uniqueness rules.
backend/tests/controllers/test_executions/test_reruns.py Updates rerun-related expected artefact payload to include attributes and drop bundled-builds.
backend/tests/controllers/auth/test_saml.py Adjusts IdP public netloc port used by SAML tests.
backend/tests/controllers/artefacts/test_builds.py Updates artefact build response expectations after removing bundled-builds references.
backend/tests/controllers/artefacts/test_artefacts.py Adds tests for attributes in artefact GET/PATCH; removes bundled-builds tests and response expectations.
backend/test_observer/data_access/repository.py Removes eager-loading of bundled-builds from artefact listing query paths.
backend/test_observer/data_access/models.py Replaces bundled-builds schema/relationships with a JSONB attributes field and updates solution unique index.
backend/test_observer/controllers/test_executions/start_test.py Stops using bundled-builds hash for solutions; stores solution attributes on artefact creation.
backend/test_observer/controllers/test_executions/models.py Replaces solution track/source request fields with attributes; updates stage description.
backend/test_observer/controllers/artefacts/models.py Adds attributes to artefact responses and patch model; removes bundled-build fields/models.
backend/test_observer/controllers/artefacts/builds.py Removes bundled-in eager-loading from build retrieval.
backend/test_observer/controllers/artefacts/artefacts.py Removes bundled-build handling from artefact list/get/patch/history and versions query.
backend/schemata/openapi.json Updates OpenAPI schema to reflect new attributes field and removed bundled-builds fields.
backend/migrations/versions/2026_07_21_1311-8202f7b5953e_replace_solution_specific_fields_with_.py Adds attributes, changes unique_solution, migrates bundled-build data into attributes, and drops legacy schema.
Comments suppressed due to low confidence (1)

backend/migrations/versions/2026_07_21_1311-8202f7b5953e_replace_solution_specific_fields_with_.py:46

  • This migration both introduces the new attributes column and drops the legacy bundled_builds_hash column and artefact_bundled_builds_association table in the same deploy step. In rolling/HA deployments, this can break older application instances still expecting the old schema (expand/contract guideline).

Consider splitting into at least two migrations/PRs: (1) add & backfill attributes while keeping old columns/tables, deploy code that can read/write the new field, then (2) remove the legacy columns/tables in a later deploy.

def _remove_bundled_builds() -> None:
    _copy_bundled_builds_to_attributes()
    op.drop_table("artefact_bundled_builds_association")
    op.drop_column("artefact", "bundled_builds_hash")

Comment thread backend/tests/controllers/auth/test_saml.py Outdated
Comment thread backend/test_observer/data_access/models.py Outdated
Copilot AI review requested due to automatic review settings July 23, 2026 15:32

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 16 out of 16 changed files in this pull request and generated 2 comments.

Comments suppressed due to low confidence (2)

backend/migrations/versions/2026_07_21_1311-8202f7b5953e_replace_solution_specific_fields_with_.py:26

  • The new unique index on solutions is only (name, version). If the existing database contains multiple solution rows with the same name/version (previously allowed by including source/track/stage/bundled_builds_hash), op.create_index(..., unique=True) will fail during upgrade and block deployment. The migration should either reconcile duplicates (e.g. merge/archive) or proactively detect and raise a clear, actionable error before attempting to create the index.
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()

backend/test_observer/data_access/models.py:328

  • The comment suggests attributes is solution-specific, but the column is defined on the generic Artefact model and is returned by the generic artefact API response. This comment is misleading and makes the field look like a violation of the project’s family-agnostic design, even though the implementation is generic.
    # (for now) Solution specific field

Comment thread backend/test_observer/controllers/artefacts/artefacts.py
Copilot AI review requested due to automatic review settings July 23, 2026 16:04

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 16 out of 16 changed files in this pull request and generated 3 comments.

Comment thread backend/test_observer/controllers/test_executions/models.py
Comment thread backend/test_observer/controllers/artefacts/models.py
Comment thread backend/test_observer/data_access/repository.py
Copilot AI review requested due to automatic review settings July 23, 2026 16:12

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 16 out of 16 changed files in this pull request and generated 1 comment.

Comment thread backend/test_observer/controllers/artefacts/artefacts.py Outdated
Copilot AI review requested due to automatic review settings July 23, 2026 17:52

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 16 out of 16 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (1)

backend/test_observer/controllers/artefacts/models.py:94

  • The deprecated bundled_builds field now returns list[Any], which downgrades the OpenAPI schema (items become untyped {}) and is less backwards-compatible for clients that still use the old response shape. Since it is always empty, you can keep the previous element type to preserve the contract while still deprecating it.
    @computed_field(
        deprecated="bundled_builds is deprecated and always empty; solutions now use the generic "
        "attributes field instead.",
    )
    def bundled_builds(self) -> list[Any]:

Comment thread backend/test_observer/controllers/test_executions/models.py Outdated
Copilot AI review requested due to automatic review settings July 23, 2026 18:44

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 16 out of 16 changed files in this pull request and generated 3 comments.

Comment thread backend/test_observer/controllers/test_executions/models.py
Comment thread backend/test_observer/controllers/artefacts/models.py Outdated
Comment on lines +95 to +99
def _remove_bundled_builds() -> None:
_copy_bundled_builds_to_attributes()
op.drop_table("artefact_bundled_builds_association")
op.drop_column("artefact", "bundled_builds_hash")

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For any reviewer, please see migration tests

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added the Copilot instructions to recommend the expand-contract pattern. The issue is in an HA deployment, the upgrades can happen at different times, so one unit might still be using old code with the old schema while the migration has already changed the database to use the new schema, or vice versa, and this can cause errors.

I think this should still be split, possibly into three migrations

  1. The first migration adds the artefact.attributes column
  2. The second is a data-only migration that copies bundled builds to attributes for solutions
  3. The third changes the index and drops artefact.bundled_builds_hash and artefact_bundled_builds_association

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I updated this PR with the split, I'll make changes based on the next copilot review before splitting into two PRs

Copilot AI review requested due to automatic review settings July 24, 2026 12:10

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 16 out of 16 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (1)

backend/migrations/versions/2026_07_21_1311-8202f7b5953e_replace_solution_specific_fields_with_.py:42

  • This migration both expands (adds artefact.attributes + new unique index) and contracts (drops artefact_bundled_builds_association and artefact.bundled_builds_hash) in the same upgrade. In rolling/HA deployments, applying the migration while old app versions are still running can break them (they may still read/write the dropped table/column). Consider splitting into an expand migration (add attributes + copy data + keep old table/column) and a later contract migration (drop old structures) after the new code is fully deployed.
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'")
    )
    _remove_bundled_builds()

Copilot AI review requested due to automatic review settings August 11, 2026 20:48

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 18 out of 18 changed files in this pull request and generated 1 comment.

Copilot AI review requested due to automatic review settings August 12, 2026 16:01

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 18 out of 18 changed files in this pull request and generated 1 comment.

Suppressed comments (1)

backend/test_observer/data_access/models.py:380

  • unique_solution is updated in the ORM metadata to only cover (name, version), but the migrations in this PR do not swap the database index accordingly (the new migration is explicitly an expand step that leaves the legacy unique_solution index in place). This schema drift can both break alembic check/autogenerate and leave production DBs upgraded via migrations enforcing a different uniqueness rule than the ORM/runtime code assumes.

Either keep the legacy index/columns represented in Base.metadata until the contract migration is applied, or add the contract migration in this PR (swap unique_solution in the DB, then drop the legacy column/table only after rollout).

        Index(
            "unique_solution",
            "name",
            "version",
            postgresql_where=column("family") == FamilyName.solution.name,

Comment thread backend/test_observer/controllers/test_executions/start_test.py
Copilot AI review requested due to automatic review settings August 12, 2026 16:48

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 18 out of 18 changed files in this pull request and generated no new comments.

Suppressed comments (2)

backend/test_observer/controllers/test_executions/start_test.py:353

  • For solutions, create_artefact() now calls get_or_create() with filter_kwargs limited to (name, version, family). In a database that already contains multiple solution rows for the same name/version (possible under the current unique_solution definition, and during rollout), get_or_create() will return an arbitrary match (.first()), so new test executions may attach to the wrong artefact. Either enforce the new (name, version) uniqueness at the DB level before using this lookup, or make the lookup deterministic / detect duplicates and fail fast.
            case StartSolutionTestExecutionRequest():
                creation_kwargs["attributes"] = self.request.attributes

        self.artefact = get_or_create(self.db, Artefact, filter_kwargs=filter_kwargs, creation_kwargs=creation_kwargs)

backend/test_observer/data_access/models.py:380

  • Artefact model metadata now defines unique_solution as unique on (name, version) only, but the only migration in this PR (2026_07_21_1311-8202f7b5953e_add_artefact_attributes_and_backfill.py) does not recreate unique_solution (and explicitly documents keeping the old schema for rolling upgrades). This mismatch will make alembic check / autogenerate report pending schema changes, and it undermines the “expand”/rolling-upgrade contract described by the migration tests.
        Index(
            "unique_solution",
            "name",
            "version",
            postgresql_where=column("family") == FamilyName.solution.name,

Copilot AI review requested due to automatic review settings August 12, 2026 18:00

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 19 out of 19 changed files in this pull request and generated no new comments.

Copilot AI review requested due to automatic review settings August 12, 2026 18:06

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 19 out of 19 changed files in this pull request and generated no new comments.

Suppressed comments (1)

backend/test_observer/controllers/test_executions/models.py:324

  • map_legacy_track_and_source mutates the incoming attributes dict (and the data dict) in-place. Because this is a mode="before" validator, the input object may be reused by the caller (e.g., unit tests or internal Python callers), and in-place mutation can lead to surprising side effects. Safer to copy attributes (and return a new data dict) before adding legacy track/source.
            attributes = data["attributes"]
            if attributes is None or not isinstance(attributes, dict):
                return data

@almeidaraul
almeidaraul requested a review from wctaylor August 12, 2026 18:33
Copilot AI review requested due to automatic review settings August 12, 2026 19:06

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 19 out of 19 changed files in this pull request and generated no new comments.

Suppressed comments (3)

backend/migrations/env.py:25

  • typing.Tuple is only used for _EXPAND_CONTRACT_IGNORED_COLUMNS. Prefer the builtin tuple (PEP 585) and drop the extra import (this also avoids import-order/isort complaints).
# for 'autogenerate' support
target_metadata = Base.metadata

backend/tests/data_generator.py:168

  • attributes = attributes or {} treats an explicitly provided empty dict as "not provided" and replaces it with a new dict. Use an explicit None check so callers passing {} keep their object/value unchanged.
        created_at = created_at or datetime.utcnow()
        reviewers = reviewers or []
        attributes = attributes or {}

backend/migrations/env.py:52

  • The include_object callback parameter name object shadows the Python builtin object and may be flagged by Ruff/flake8-builtins. Renaming it also makes it clearer this is an Alembic/SQLAlchemy object.
        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

Copilot AI review requested due to automatic review settings August 12, 2026 19:13

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 19 out of 19 changed files in this pull request and generated no new comments.

Suppressed comments (2)

backend/test_observer/controllers/artefacts/models.py:75

  • Removing the previously-exposed bundled_builds field from the /v1/artefacts/* response is a backward-incompatible API change for existing clients. If clients still depend on this field, consider either (a) keeping it temporarily as a deprecated field (e.g. defaulting to an empty list) or (b) introducing a versioned response (/v2) for the breaking change.
    stage: str
    family: str
    status: ArtefactStatus
    comment: str
    attributes: dict[str, Any]
    archived: bool

backend/test_observer/controllers/artefacts/models.py:151

  • Removing the bundled_in field from ArtefactBuildResponse is a backward-incompatible change for clients consuming build details. If this field has active consumers, consider keeping it temporarily as a deprecated field (even if it always returns []) or moving the change behind a versioned API response.
    model_config = ConfigDict(from_attributes=True)

    id: int
    architecture: str
    revision: int | None
    test_executions: list[TestExecutionResponse]


Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants