fix(database): replace type: ignore with assert for type narrowing in LegacyDataMigrator
CI / lint (pull_request) Failing after 36s
CI / quality (pull_request) Successful in 45s
CI / security (pull_request) Successful in 52s
CI / typecheck (pull_request) Successful in 53s
CI / coverage (pull_request) Has been skipped
CI / build (pull_request) Successful in 21s
CI / helm (pull_request) Successful in 27s
CI / push-validation (pull_request) Successful in 27s
CI / e2e_tests (pull_request) Successful in 3m17s
CI / integration_tests (pull_request) Failing after 3m56s
CI / unit_tests (pull_request) Successful in 5m22s
CI / docker (pull_request) Has been skipped
CI / status-check (pull_request) Failing after 1s
CI / benchmark-publish (pull_request) Has been skipped
CI / benchmark-regression (pull_request) Has been skipped

Remove the unsafe `# type: ignore` suppression on line 111 of
`legacy_migrator.py` and replace it with an explicit
`assert existing_plan.id is not None` statement. This provides
proper type narrowing to the type checker while preserving the
logical correctness guaranteed by the preceding `if existing_plan:`
guard.

Also adds a new BDD scenario in `legacy_migrator_coverage.feature`
that explicitly exercises the code path where an existing plan with
a non-None id is found during migration, verifying the assert-based
type narrowing works correctly end-to-end.

ISSUES CLOSED: #3051
This commit is contained in:
2026-04-05 08:20:27 +00:00
committed by CleverThis
parent a33b6caa7e
commit 3d535c3c0e
3 changed files with 25 additions and 1 deletions
@@ -204,9 +204,17 @@ Feature: Legacy Data Migrator Coverage
Given I have a legacy project with read-only JSON files
When I run the legacy data migration
Then the migration should handle backup errors gracefully
Scenario: get_all_for_project is called only once for multiple plans
Given I have a legacy project with multiple plans to migrate efficiently
When I run the legacy data migration with query tracking
Then get_all_for_project should be called exactly once
And the migration should return true
And all plans should be migrated correctly
Scenario: Map existing plan IDs correctly during migration
Given I have a legacy project with plans already in database
When I run the legacy data migration
Then existing plans should not be duplicated
And the migration should return true
And the existing plan id should be mapped without type suppression
+15
View File
@@ -1085,3 +1085,18 @@ def step_check_all_plans_migrated(context: Context) -> None:
assert plan_names == expected_names, (
f"Expected plans {expected_names}, got {plan_names}"
)
@then("the existing plan id should be mapped without type suppression")
def step_check_existing_plan_id_mapped(context: Context) -> None:
"""Check that existing plan id was mapped correctly via assert type narrowing."""
with context.unit_of_work.transaction() as ctx:
project = ctx.projects.get_by_name("existing_plans")
assert project is not None, "Project 'existing_plans' should exist in database"
plans = ctx.plans.get_all_for_project(project.id)
main_plans = [p for p in plans if p.name == "main"]
assert len(main_plans) == 1, "Should have exactly one 'main' plan"
assert main_plans[0].id is not None, (
"Existing plan id must be non-None (assert narrowing validates this)"
)
@@ -110,7 +110,8 @@ class LegacyDataMigrator:
(p for p in all_plans if p.name == plan_name), None
)
if existing_plan:
plan_id_map[plan_name] = existing_plan.id # type: ignore
assert existing_plan.id is not None
plan_id_map[plan_name] = existing_plan.id
continue
# Create new plan