refactor: Migrate remaining ExpectedMachine BeginTx sites to WithTx#557
refactor: Migrate remaining ExpectedMachine BeginTx sites to WithTx#557chet wants to merge 1 commit into
Conversation
Summary by CodeRabbit
WalkthroughThe PR refactors batch create and batch update handlers for ExpectedMachine to decouple validation reads (site resolution, MAC/serial unicity, SKU existence) from transactional writes. Validation queries run outside transactions; DB writes and Temporal workflow scheduling occur inside cdb.WithTxResult callbacks. Workflow IDs now use UUID suffixes and correlation uses ExpectedMachineID. ChangesExpectedMachine Batch Transaction Refactor
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
🔐 TruffleHog Secret Scan✅ No secrets or credentials found! Your code has been scanned for 700+ types of secrets and credentials. All clear! 🎉 🕐 Last updated: 2026-05-21 19:29:34 UTC | Commit: c8defbc |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
api/pkg/api/handler/expectedmachine.go (1)
1031-1038:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't cap the SKU lookup to the request count unless the query is filtered by those SKU IDs.
SkuDAO.GetAllhere is only scoped bySiteIDs, soLimit: len(requestedSkuIDs)can return an arbitrary subset of the site's SKUs. On sites with more SKUs than the batch references, validskuIDvalues can be rejected as missing.Proposed fix
- existingSkus, _, err := skuDAO.GetAll(ctx, nil, cdbm.SkuFilterInput{ - SiteIDs: []uuid.UUID{siteID}, - }, paginator.PageInput{ - Limit: cdb.GetIntPtr(len(requestedSkuIDs)), - }) + existingSkus, _, err := skuDAO.GetAll(ctx, nil, cdbm.SkuFilterInput{ + SiteIDs: []uuid.UUID{siteID}, + }, paginator.PageInput{ + // We are not filtering by requested SKU IDs here, so we need the full site set. + Limit: cdb.GetIntPtr(paginator.TotalLimit), + })Apply the same change in both batch-create and batch-update paths. If
cdbm.SkuFilterInputsupports filtering by specific SKU IDs, that would be even better than loading the full site set.Also applies to: 1436-1443
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@api/pkg/api/handler/expectedmachine.go` around lines 1031 - 1038, The SKU lookup is incorrectly limited by Limit: cdb.GetIntPtr(len(requestedSkuIDs)) while SkuDAO.GetAll is only filtered by SiteIDs, which can return a subset and cause missing validations; update the calls around SkuDAO.GetAll (used in the batch-create and batch-update paths) to remove or increase the Limit so you fetch all SKUs for the site, or—preferably—add filtering by the specific SKU IDs via cdbm.SkuFilterInput (e.g., include SKUIDs: requestedSkuIDs) so SkuDAO.GetAll returns precisely the referenced SKUs before comparing against requestedSkuIDs (apply the same change at both occurrences).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@api/pkg/api/handler/expectedmachine.go`:
- Around line 1179-1182: The code currently only warns when
workflowResult.GetResults() length differs from len(createdMachines) and still
commits; change this to fail the transaction and return an error when
AcceptPartialResults is false so the operation is all-or-nothing: after
receiving workflowResult in the create path (where createdMachines is used)
check if len(workflowResult.GetResults()) != len(createdMachines) and if
AcceptPartialResults == false, abort the transaction and return an error
(instead of logger.Warn()), and make the identical change in the batch-update
branch (the other branch handling the update at the referenced lines) to ensure
both handlers enforce the same all-or-nothing behavior.
---
Outside diff comments:
In `@api/pkg/api/handler/expectedmachine.go`:
- Around line 1031-1038: The SKU lookup is incorrectly limited by Limit:
cdb.GetIntPtr(len(requestedSkuIDs)) while SkuDAO.GetAll is only filtered by
SiteIDs, which can return a subset and cause missing validations; update the
calls around SkuDAO.GetAll (used in the batch-create and batch-update paths) to
remove or increase the Limit so you fetch all SKUs for the site,
or—preferably—add filtering by the specific SKU IDs via cdbm.SkuFilterInput
(e.g., include SKUIDs: requestedSkuIDs) so SkuDAO.GetAll returns precisely the
referenced SKUs before comparing against requestedSkuIDs (apply the same change
at both occurrences).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 789ac409-e7e6-4306-9324-d3a899290ef3
📒 Files selected for processing (1)
api/pkg/api/handler/expectedmachine.go
🔍 Container Scan Summary
Per-CVE detail lives in the per-service |
Picks up the remaining `BeginTx` sites left over in `expectedmachine.go` after the original migration (Create and Update flows). In this PR: - Both call sites use `WithTxResult` naturally. - Moved Create validation reads out of the tx (for `existingMachinesOnSite` and `existingSkus`). - Moved Update validation reads out of the tx (for `requestedExpectedMachine`, `expectedMachinesOnSite`, and `skus`). - Errors switched to `NewAPIError`. Signed-off-by: Chet Nichols III <chetn@nvidia.com>
chet
force-pushed
the
with-tx-expectedmachine
branch
from
c8defbc to
0048c34
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
api/pkg/api/handler/expectedmachine.go (1)
1076-1076:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winError message incorrectly references "update" in the create handler.
This error message states
"Failed to validate Expected Machine update data"but resides withinCreateExpectedMachinesHandler. The message should reference "create" or "creation" for consistency and to avoid confusion during debugging.Proposed fix
- return cutil.NewAPIErrorResponse(c, http.StatusBadRequest, "Failed to validate Expected Machine update data", validationErrors) + return cutil.NewAPIErrorResponse(c, http.StatusBadRequest, "Failed to validate Expected Machine create data", validationErrors)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@api/pkg/api/handler/expectedmachine.go` at line 1076, The error message in CreateExpectedMachinesHandler incorrectly says "Failed to validate Expected Machine update data"; update the message passed to cutil.NewAPIErrorResponse in CreateExpectedMachinesHandler to reference "create" or "creation" (e.g., "Failed to validate Expected Machine creation data") so it accurately reflects the handler's purpose while leaving the validationErrors payload unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@api/pkg/api/handler/expectedmachine.go`:
- Line 1076: The error message in CreateExpectedMachinesHandler incorrectly says
"Failed to validate Expected Machine update data"; update the message passed to
cutil.NewAPIErrorResponse in CreateExpectedMachinesHandler to reference "create"
or "creation" (e.g., "Failed to validate Expected Machine creation data") so it
accurately reflects the handler's purpose while leaving the validationErrors
payload unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 5eb483ca-8236-430a-9dca-637423e9386f
📒 Files selected for processing (1)
api/pkg/api/handler/expectedmachine.go
1 participant
Description
Picks up the remaining
BeginTxsites left over inexpectedmachine.goafter the original migration (Create and Update flows).In this PR:
WithTxResultnaturally.existingMachinesOnSiteandexistingSkus).requestedExpectedMachine,expectedMachinesOnSite, andskus).NewAPIError.Signed-off-by: Chet Nichols III chetn@nvidia.com
Type of Change
Services Affected
Related Issues (Optional)
Breaking Changes
Testing
Additional Notes