chore: migration script - owner_id add#2321
Conversation
Reviewer's GuideAdds a new PostgreSQL migration script to backfill system_platform.owner_id from inventory.hosts.system_profile and bumps the schema version to 164 to register the migration. Entity relationship diagram for owner_id migration between system_platform and inventory_hostserDiagram
system_platform {
uuid id PK
uuid inventory_id FK
uuid owner_id
}
inventory_hosts {
uuid id PK
jsonb system_profile
}
inventory_hosts ||--o| system_platform : inventory_id
Flow diagram for PostgreSQL owner_id backfill migration scriptflowchart TD
A[start DO block] --> B[Check if inventory.hosts view exists]
B -->|does not exist| C[Raise NOTICE inventory.hosts view does not exist, skipping migration]
C --> D[end DO block]
B -->|exists| E[Select rows where system_platform.owner_id is NULL and inventory.hosts.system_profile owner_id is not NULL]
E --> F{For each selected record}
F -->|record| G[Begin record update]
G --> H[Update system_platform set owner_id from ih_owner_id cast to UUID]
H --> I{Update succeeded}
I --> J[next record]
F -->|no more records| D
G --> K[On exception: raise WARNING with inventory_id, ih_owner_id, SQLERRM]
K --> J
J --> F
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
SC Environment Impact AssessmentOverall Impact: ⚪ NONE No SC Environment-specific impacts detected in this PR. What was checkedThis PR was automatically scanned for:
|
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The migration currently iterates row by row in a PL/pgSQL loop; this can be simplified and made more efficient with a single
UPDATE system_platform sp SET owner_id = ih.system_profile->>'owner_id'::uuid FROM inventory.hosts ih WHERE sp.inventory_id = ih.id AND sp.owner_id IS NULL AND ih.system_profile->>'owner_id' IS NOT NULLstatement while retaining exception handling if needed. - Consider narrowing the exception block to only catch UUID cast-related errors (e.g.,
invalid_text_representation) instead ofWHEN OTHERS, so that unexpected issues during the migration are not silently downgraded to warnings.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The migration currently iterates row by row in a PL/pgSQL loop; this can be simplified and made more efficient with a single `UPDATE system_platform sp SET owner_id = ih.system_profile->>'owner_id'::uuid FROM inventory.hosts ih WHERE sp.inventory_id = ih.id AND sp.owner_id IS NULL AND ih.system_profile->>'owner_id' IS NOT NULL` statement while retaining exception handling if needed.
- Consider narrowing the exception block to only catch UUID cast-related errors (e.g., `invalid_text_representation`) instead of `WHEN OTHERS`, so that unexpected issues during the migration are not silently downgraded to warnings.
## Individual Comments
### Comment 1
<location path="database/schema/upgrade_scripts/164-migrate-owner-id-to-system-platform.sql" line_range="28-29" />
<code_context>
+ UPDATE system_platform SET
+ owner_id = (rec.ih_owner_id)::UUID
+ WHERE id = rec.sp_id;
+ EXCEPTION WHEN OTHERS THEN
+ RAISE WARNING 'Migration failed for inventory_id=%, owner_id=%: %', rec.inventory_id, rec.ih_owner_id, SQLERRM;
+ END;
+ END LOOP;
</code_context>
<issue_to_address>
**issue (bug_risk):** Catching `WHEN OTHERS` globally can hide non-casting issues
`EXCEPTION WHEN OTHERS` turns all failures (including permissions, constraints, etc.) into warnings, so the migration can falsely appear successful. If you only expect cast issues, narrow this to specific errors like `invalid_text_representation` (and related cast errors) so unexpected problems still fail the migration while bad `owner_id` values are logged and handled explicitly.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| EXCEPTION WHEN OTHERS THEN | ||
| RAISE WARNING 'Migration failed for inventory_id=%, owner_id=%: %', rec.inventory_id, rec.ih_owner_id, SQLERRM; |
There was a problem hiding this comment.
issue (bug_risk): Catching WHEN OTHERS globally can hide non-casting issues
EXCEPTION WHEN OTHERS turns all failures (including permissions, constraints, etc.) into warnings, so the migration can falsely appear successful. If you only expect cast issues, narrow this to specific errors like invalid_text_representation (and related cast errors) so unexpected problems still fail the migration while bad owner_id values are logged and handled explicitly.
Secure Coding Practices Checklist GitHub Link
Secure Coding Checklist
Summary by Sourcery
Enhancements: