TO-404: destructive migrations - #843
Open
almeidaraul wants to merge 17 commits into
Open
Conversation
This reverts commit cf9032d.
Contributor
There was a problem hiding this comment.
Pull request overview
Implements the contract (destructive) half of TO-404 by removing legacy solution-specific “bundled builds” schema after the prior expand migration has been deployed, and adds migration tests to validate upgrade/downgrade behavior.
Changes:
- Adds Alembic migration
8bd1f5009f02to re-backfill leftover legacy bundled-build data intoartefact.attributes, swap theunique_solutionindex to(name, version), and dropbundled_builds_hash+artefact_bundled_builds_association. - Adds migration tests covering upgrade backfill, downgrade restoration, schema assertions, and fail-fast duplicate detection.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| backend/migrations/versions/2026_08_10_1716-8bd1f5009f02_drop_solution_specific_bundled_build_fields.py | Contract migration: re-copy legacy data into attributes, tighten unique_solution, then drop legacy column/table; includes downgrade restore. |
| backend/tests/migrations/test_8bd1f5009f02_drop_solution_specific_bundled_build_fields.py | Migration test suite validating upgrade/downgrade data movement, schema changes, and duplicate-key safeguards. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Suppressed comments (1)
backend/migrations/versions/2026_08_10_1716-8bd1f5009f02_drop_solution_specific_bundled_build_fields.py:190
- The guard
length(elem.value) <= 9is stricter than PostgresINTEGERrange and can silently skip validartefact_build.idvalues (e.g. 10-digit ids up to 2_147_483_647). That would lose bundled-build associations during downgrade even though the IDs are representable and should be restorable.
ON ab.id = CASE
WHEN elem.value ~ '^[0-9]+$' AND length(elem.value) <= 9
THEN elem.value::int
END
…fields' into TO-404/destructive-migration
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
Suppressed comments (2)
backend/migrations/versions/2026_08_10_1716-8bd1f5009f02_drop_solution_specific_bundled_build_fields.py:190
- The downgrade guard for
bundled_buildsIDs useslength(elem.value) <= 9as an “in-range” check before casting toint. PostgreSQLINTEGERsupports up to 2,147,483,647 (10 digits), so this will incorrectly skip legitimate 10-digit IDs (and the comment claims this is an in-range check). Consider a precise bound check that still avoids overflow during casting.
ON ab.id = CASE
WHEN elem.value ~ '^[0-9]+$' AND length(elem.value) <= 9
THEN elem.value::int
END
backend/migrations/versions/2026_08_10_1716-8bd1f5009f02_drop_solution_specific_bundled_build_fields.py:124
- When recreating
artefact_bundled_builds_associationin a downgrade, it’s better to useop.f(...)for constraint names so Alembic applies the project’s naming convention consistently. The original creation of this table inbackend/migrations/versions/2026_04_24_1401-717189ad8f3f_add_risk_to_artefact.pyusesop.f(...)for these same constraint names, so restoring with raw strings risks name mismatches across environments.
sa.ForeignKeyConstraint(
["artefact_build_id"],
["artefact_build.id"],
name="artefact_bundled_builds_association_id_fkey",
ondelete="CASCADE",
),
Comment on lines
+133
to
+148
| 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 | ||
| ) |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
Suppressed comments (1)
backend/migrations/env.py:39
_EXPAND_CONTRACT_IGNORED_*are currently initialized with{}, which creates empty dicts rather than empty sets. This works accidentally when empty, but it changes the intended types and can lead to subtle issues once values are added (and it’s inconsistent with the previous set-based usage). Useset()(optionally with type annotations) instead.
# Transitional expand/contract exclusions
# In expand/contract migrations it could happen that the ORM models temporarily
# don't reference all fields from the database (i.e. in an expand release).
# `alembic check` will notice that and fail in CI, so we need these exceptions
_EXPAND_CONTRACT_IGNORED_TABLES = {}
_EXPAND_CONTRACT_IGNORED_COLUMNS = {}
_EXPAND_CONTRACT_IGNORED_INDEXES = {}
This reverts commit cf9032d.
…est_observer into TO-404/destructive-migration
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 20 out of 20 changed files in this pull request and generated 1 comment.
Suppressed comments (2)
backend/migrations/env.py:39
- The transitional ignore collections are intended to be sets, but they’re currently initialized with
{}(an empty dict). That’s easy to misread and prevents using set operations if entries need to be added later. Useset()(optionally with type hints) for these.
_EXPAND_CONTRACT_IGNORED_TABLES = {}
_EXPAND_CONTRACT_IGNORED_COLUMNS = {}
_EXPAND_CONTRACT_IGNORED_INDEXES = {}
backend/tests/migrations/test_8bd1f5009f02_drop_solution_specific_bundled_build_fields.py:303
- This comment is outdated/misleading: the test now expects the downgrade to succeed without raising, so it shouldn’t state that a
DatabaseErroris raised.
# Raises a DatabaseError; after guarding the traversal it should complete cleanly.
Comment on lines
+65
to
+67
| def test_solution_unique_constraint_ignores_source_track_and_stage( | ||
| generator: DataGenerator, db_session: Session | ||
| ) -> None: |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 20 out of 20 changed files in this pull request and generated no new comments.
Suppressed comments (3)
frontend/lib/routing.dart:137
- These new /solutions routes add more artefact-family-specific routing and branching, which the repository explicitly calls out as technical debt and says new contributions must not introduce (see CLAUDE.md:148-158). To align with the project’s architecture, avoid adding new family-specific routes and instead route to a single generic dashboard/artefact page and drive the selected family via data/config (e.g., a query parameter or a path parameter) rather than hard-coded per-family GoRoutes.
GoRoute(
path: AppRoutes.solutions,
pageBuilder: (_, __) => const NoTransitionPage(
child: Dashboard(),
),
backend/test_observer/controllers/artefacts/artefacts.py:378
- Adding another FamilyName-specific branch here expands artefact-family-specific behavior in the backend. The repo’s architecture guidance states new contributions must not introduce/extend branching behavior based on artefact family (CLAUDE.md:148-158). Consider replacing the per-family enum casting with a generic, data-driven validation (e.g., a mapping of family->allowed stages in a single structure, or a DB/config-driven allowed-stage set) so adding new families doesn’t require code changes across controllers.
case FamilyName.image:
ImageStage(stage)
case FamilyName.solution:
SolutionStage(stage)
frontend/assets/config.yaml:7
- The PR description frames this as the destructive/contract migration half of #835, but this change introduces a new frontend tab (/solutions) and implies new user-facing navigation/routing work. Please either update the PR description to explicitly include the frontend feature scope, or split the frontend routing/tab changes into a dedicated PR so the migration can be reviewed/deployed independently.
- snaps
- debs
- charms
- images
- solutions
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 20 out of 20 changed files in this pull request and generated no new comments.
Suppressed comments (16)
backend/scripts/seed_data.py:479
- These seed requests use the deprecated
track/sourcefields on StartSolutionTestExecutionRequest. Prefer populatingattributesinstead so seed data exercises the current API shape and doesn't rely on legacy compatibility mapping.
StartSolutionTestExecutionRequest(
family=FamilyName.solution,
name="canonical-kubernetes",
version="1.32.1",
track="1.32",
source="different-sha",
arch="arm64",
execution_stage=SolutionStage.candidate,
backend/migrations/env.py:39
- These ignore-lists are currently initialised as
{}which is an empty dict, not an empty set. Membership tests happen to work, but this is a type/intent mismatch and makes later.add(...)style updates error-prone.
# Transitional expand/contract exclusions
# In expand/contract migrations it could happen that the ORM models temporarily
# don't reference all fields from the database (i.e. in an expand release).
# `alembic check` will notice that and fail in CI, so we need these exceptions
_EXPAND_CONTRACT_IGNORED_TABLES = {}
_EXPAND_CONTRACT_IGNORED_COLUMNS = {}
_EXPAND_CONTRACT_IGNORED_INDEXES = {}
frontend/lib/routing.dart:146
- This adds a new family-specific top-level route (
/solutions) and wires it into familyFromUri/isDashboardPage/isArtefactPage. CLAUDE.md (Core Design Principle, ~150-158) explicitly calls family-specific routes/branching technical debt and asks that new contributions not introduce more of it; please avoid expanding this pattern and prefer a family-agnostic artefact route (e.g. a single dashboard route with a family filter in query/state).
GoRoute(
path: AppRoutes.solutions,
pageBuilder: (_, __) => const NoTransitionPage(
child: Dashboard(),
),
),
GoRoute(
path: '${AppRoutes.solutions}/:artefactId',
pageBuilder: (context, state) => NoTransitionPage(
child: ArtefactPage(
artefactId: int.parse(state.pathParameters['artefactId']!),
),
),
),
frontend/assets/config.yaml:7
- Adding a new 'solutions' tab expands the family-specific tab surface area. Per CLAUDE.md Core Design Principle (~150-158), family-specific frontend routes/tabs are acknowledged technical debt and shouldn't be expanded in new work.
tabs:
- snaps
- debs
- charms
- images
- solutions
frontend/lib/ui/navbar.dart:44
- This introduces a dedicated navbar label for the new family-specific 'solutions' tab. CLAUDE.md Core Design Principle (~150-158) asks not to introduce new artefact-family-specific UI branches/routes; consider a family-agnostic navigation approach instead of adding another per-family tab.
String _tabDisplayName(String tab) {
return switch (tab) {
'snaps' => 'Snap Testing',
'debs' => 'Deb Testing',
'charms' => 'Charm Testing',
'images' => 'Image Testing',
'solutions' => 'Solution Testing',
_ => tab,
};
frontend/lib/ui/dashboard/dashboard_body/dashboard_body.dart:44
- This adds another explicit family branch in the dashboard body switch. CLAUDE.md Core Design Principle (~150-158) calls out artefact-family branching as technical debt and asks that new contributions not expand it.
builder: (_, viewMode) => switch ((family, viewMode)) {
(_, ViewModes.dashboard) => const ArtefactsColumnsView(),
(FamilyName.snap, ViewModes.list) => const ArtefactsListView.snaps(),
(FamilyName.deb, ViewModes.list) => const ArtefactsListView.debs(),
(FamilyName.charm, ViewModes.list) => const ArtefactsListView.charms(),
(FamilyName.image, ViewModes.list) => const ArtefactsListView.images(),
(FamilyName.solution, ViewModes.list) =>
const ArtefactsListView.solutions(),
},
frontend/lib/ui/dashboard/dashboard_body/artefacts_list_view/artefacts_list_view.dart:78
- This adds a solutions-specific list view constructor and builder, increasing the number of family-specific UI code paths. CLAUDE.md Core Design Principle (~150-158) asks that new contributions not expand family-specific frontend branching.
const ArtefactsListView.solutions({super.key})
: listHeader = const _Headers.solutions(key: PageStorageKey('Header')),
listItemBuilder = _solutionsListItemBuilder;
static Widget _solutionsListItemBuilder(Artefact artefact) {
return _Row.solution(key: PageStorageKey(artefact.id), artefact: artefact);
}
frontend/lib/ui/dashboard/dashboard_body/artefacts_list_view/row.dart:40
- This adds a solutions-specific row variant, further expanding the family-specific UI branching that CLAUDE.md (Core Design Principle ~150-158) identifies as technical debt.
const _Row.solution({super.key, required this.artefact})
: columnsMetaData = _solutionColumnsMetadata;
frontend/lib/ui/dashboard/dashboard_body/artefacts_list_view/headers.dart:34
- This adds a solutions-specific header variant, expanding the set of family-specific UI branches. CLAUDE.md Core Design Principle (~150-158) asks to avoid introducing new artefact-family-specific UI routing/branching.
const _Headers.solutions({super.key})
: columnsMetaData = _solutionColumnsMetadata;
frontend/lib/ui/dashboard/dashboard_body/artefacts_list_view/column_metadata.dart:296
- Introducing a dedicated
_solutionColumnsMetadataexpands the family-specific table schema in the UI. Per CLAUDE.md Core Design Principle (~150-158), this kind of per-family branching is technical debt and should not be expanded in new work.
const _solutionColumnsMetadata = <ColumnMetadata>[
(
name: 'Name',
queryParam: ArtefactSortingQuery.name,
flex: 2,
cellBuilder: _buildNameCell,
),
(
name: 'Version',
queryParam: ArtefactSortingQuery.version,
flex: 2,
cellBuilder: _buildVersionCell,
),
(
name: 'Due date',
queryParam: ArtefactSortingQuery.dueDate,
flex: 1,
cellBuilder: _buildDueDateCell,
),
(
name: 'Reviews remaining',
queryParam: ArtefactSortingQuery.reviewsRemaining,
flex: 1,
cellBuilder: _buildReviewsRemainingCell,
),
(
name: 'Status',
queryParam: ArtefactSortingQuery.status,
flex: 1,
cellBuilder: _buildStatusCell,
),
(
name: 'Reviewers',
queryParam: ArtefactSortingQuery.reviewer,
flex: 1,
cellBuilder: _buildReviewersCell,
),
];
frontend/lib/models/family_name.dart:16
- Adding a new enum member for a specific artefact family expands the codebase-wide family branching surface area (routing, filtering, UI switches, etc.). CLAUDE.md Core Design Principle (~150-158) requests avoiding new artefact-specific logic in new contributions.
enum FamilyName { snap, deb, charm, image, solution }
frontend/lib/models/stage_name.dart:68
- This adds another family-specific branch for stage lists. CLAUDE.md Core Design Principle (~150-158) calls out artefact-family branching as technical debt and asks not to expand it; consider deriving valid stages from backend capabilities/schema instead of hard-coding per-family lists.
case FamilyName.solution:
return [
StageName.edge,
StageName.beta,
StageName.candidate,
StageName.stable,
];
frontend/lib/filtering/artefact_filters.dart:53
- This introduces a solutions-specific filter set, expanding family-specific frontend branching that CLAUDE.md Core Design Principle (~150-158) identifies as technical debt.
FamilyName.solution => [
_artefactReviewerFilter,
_artefactStatusFilter,
_artefactDueDateFilter,
],
frontend/lib/ui/artefact_page/manual_testing_dialog.dart:185
- For solutions, this sends
track/sourceas top-level request fields. On the backend these are marked deprecated on StartSolutionTestExecutionRequest (they are meant to be folded intoattributes). Prefer sendingattributes: {track, source}and omitting the deprecated fields to avoid relying on legacy compatibility paths.
case FamilyName.solution:
fields['track'] = artefact.track;
fields['source'] = artefact.source;
fields['execution_stage'] = artefact.stage.name;
break;
backend/scripts/seed_data.py:467
- These seed requests use the deprecated
track/sourcefields on StartSolutionTestExecutionRequest. Prefer populatingattributesinstead so seed data exercises the current API shape and doesn't rely on legacy compatibility mapping.
This issue also appears on line 472 of the same file.
StartSolutionTestExecutionRequest(
family=FamilyName.solution,
name="canonical-kubernetes",
version="1.32.1",
track="1.32",
source="some-sha",
arch="amd64",
execution_stage=SolutionStage.candidate,
backend/test_observer/controllers/artefacts/artefacts.py:378
- This adds another artefact-family-specific branch in stage validation. CLAUDE.md Core Design Principle (~150-158) asks not to introduce new artefact-family branching; consider moving stage validation to a family-agnostic mechanism (e.g. a single stage enum + per-artefact allowed transitions/capabilities stored as data).
def _validate_artefact_stage(artefact: Artefact, stage: StageName) -> None:
try:
match artefact.family:
case FamilyName.snap:
SnapStage(stage)
case FamilyName.deb:
DebStage(stage)
case FamilyName.charm:
CharmStage(stage)
case FamilyName.image:
ImageStage(stage)
case FamilyName.solution:
SolutionStage(stage)
…tructive-migration
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Suppressed comments (2)
backend/tests/migrations/test_8bd1f5009f02_drop_solution_specific_bundled_build_fields.py:303
- This comment says the downgrade raises a DatabaseError, but the test expects the downgrade to succeed without raising. Update the comment to reflect the intended behavior (and optionally mention that it used to fail before adding guards).
# Raises a DatabaseError; after guarding the traversal it should complete cleanly.
backend/tests/migrations/test_8bd1f5009f02_drop_solution_specific_bundled_build_fields.py:322
unknown_build_idis actually an artefact id (returned by_insert_artefact), which makes the later loop harder to read and easy to misinterpret. Rename it to reflect what it stores and update the reference in the loop.
# bundled_builds references a build id that does not exist -> FK violation.
unknown_build_id = _insert_artefact(
conn, "solution-unknown-build", attributes='{"bundled_builds": [999999999]}'
)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Destructive part of #835
Resolved issues
Resolves TO-404
Documentation
Web service API changes
Tests