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
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:
@@ -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
|
||||
|
||||
@@ -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
|
||||
|
||||
Reference in New Issue
Block a user