RHINENG-29747: add workspace_id not null constraint - #2329
Open
Dugowitch wants to merge 2 commits into
Open
Conversation
WorkspaceID will not be nil after the migration in previous commit. Co-Authored-By: Gemini <gemini@google.com>
Reviewer's GuideThe PR makes workspace_id mandatory end to end: migration 167 changes the database constraint and advisory-cache SQL, Go models and API representations use non-null UUIDs, and listener ingestion rejects missing or malformed workspace data while downstream events and responses assume a valid workspace. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="database_admin/migrations/167_add_workspace_id_not_null.up.sql" line_range="1" />
<code_context>
+ALTER TABLE system_inventory ALTER COLUMN workspace_id SET NOT NULL;
+
+CREATE OR REPLACE FUNCTION refresh_account_advisory_caches_multi(advisory_ids_in INTEGER[] DEFAULT NULL,
</code_context>
<issue_to_address>
**issue (bug_risk):** The migration fails at `SET NOT NULL` when any existing `system_inventory.workspace_id` row is NULL. The migration does not backfill or delete those rows before applying the constraint, and the preceding schema explicitly allowed NULL values.
**Triggers:** When upgrading a database containing inventories created before this constraint.
**Suggested fix:** Backfill valid workspace IDs or otherwise handle NULL rows before setting the column to NOT NULL, ideally with an explicit validation step.
</issue_to_address>
### Comment 2
<location path="listener/upload.go" line_range="369-372" />
<code_context>
- )
- }
+ l := len(host.Groups)
+ if l == 0 {
+ utils.LogError("inventoryID", inventoryID, "workspace UUID missing for system")
+ return nil, errors.New("workspace UUID missing for system")
+ }
+ workspace := host.Groups[0]
+ workspaceID, err := uuid.Parse(workspace.ID)
</code_context>
<issue_to_address>
**issue (bug_risk):** Uploads for hosts with no workspace group now return an error instead of storing the inventory. The existing code accepted such hosts and persisted a NULL workspace ID, so any still-valid host event without `Groups[0]` is rejected and its upload transaction fails.
**Triggers:** When an inventory upload contains an empty `host.Groups` list.
**Suggested fix:** Ensure the event pipeline supplies a valid workspace before this function, or define and use a non-NULL fallback workspace value instead of rejecting the upload.
</issue_to_address>Sourcery assessment
Needs a human reviewer. 2 findings to address first, and if existing rows have a null workspace_id, the migration can fail, and hosts without a workspace will stop being ingested or updated. A wrongly assigned workspace UUID would be persisted and could misgroup advisory caches and API results; reverting the code does not automatically repair those records, though they can be corrected by recomputation or update.
Blocking findings: database_admin/migrations/167_add_workspace_id_not_null.up.sql:1, listener/upload.go:372
Comment on lines
+369
to
+372
| if l == 0 { | ||
| utils.LogError("inventoryID", inventoryID, "workspace UUID missing for system") | ||
| return nil, errors.New("workspace UUID missing for system") | ||
| } |
There was a problem hiding this comment.
issue (bug_risk): Uploads for hosts with no workspace group now return an error instead of storing the inventory. The existing code accepted such hosts and persisted a NULL workspace ID, so any still-valid host event without Groups[0] is rejected and its upload transaction fails.
Triggers: When an inventory upload contains an empty host.Groups list.
Suggested fix: Ensure the event pipeline supplies a valid workspace before this function, or define and use a non-NULL fallback workspace value instead of rejecting the upload.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2329 +/- ##
==========================================
- Coverage 59.06% 58.98% -0.09%
==========================================
Files 150 150
Lines 9615 9608 -7
==========================================
- Hits 5679 5667 -12
- Misses 3342 3346 +4
- Partials 594 595 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Contributor
Author
|
Is it better to error out, if there is no workspace id in the message, or to use the zero UUID? |
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
2 participants
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.
Secure Coding Practices Checklist GitHub Link
Secure Coding Checklist
Summary by Sourcery
Enforce workspace association for all system inventory records and propagate the required workspace identity consistently across the application.
Bug Fixes:
Enhancements:
Tests: