fix(rest-api): Prevent user from updating ExpectedRack rackId after creation - #4461
fix(rest-api): Prevent user from updating ExpectedRack rackId after creation#4461dev-tnsq wants to merge 5 commits into
Conversation
…IDIA#4365) Make rackId immutable on PATCH /expected-rack/{id}. A changed value is rejected with 400 before any database mutation or workflow trigger, mirroring the ExpectedMachine BMC MAC identity boundary. Omitted or identical values remain a no-op, and RackID is no longer passed into the DAO update input so the update path is structurally incapable of a rename. OpenAPI marks rackId deprecated in the update request schema; sdk/standard regenerated. Signed-off-by: Tanishq <tanishq160405@gmail.com>
|
Important Review skippedReview was skipped as selected files did not have any reviewable changes. 💤 Files selected but had no reviewable changes (1)
⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
You can disable this status message by setting the Use the checkbox below for a quick retry:
Summary by CodeRabbit
WalkthroughThe Expected Rack PATCH handler rejects changed ChangesExpected Rack ID Immutability
Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
thossain-nv
added
the
rest-api
ChatGPT Codex Connector
There was a problem hiding this comment.
Pull request overview
This PR closes the gap where PATCH /expected-rack/{id} would accept a changed rackId (attempting an unsupported rename), mutate the Cloud record, and then fail downstream because Core/Flow treat rackId as the identity key. The new behavior rejects rackId changes early (after site-access checks) and makes the update path structurally incapable of performing a rename.
Changes:
- Enforce
rackIdimmutability in the ExpectedRack PATCH handler and stop passingrackIdthrough the DAO update input. - Update OpenAPI and regenerate the Go SDK to document/reflect the immutability and deprecation on the update request schema.
- Expand unit tests to cover rename rejection, compatibility for identical/omitted
rackId, and ordering relative to site-access checks (plus a dedicated “no mutation / no workflow” assertion for rename attempts).
Reviewed changes
Copilot reviewed 5 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| rest-api/api/pkg/api/handler/expectedrack.go | Rejects rackId changes with 400 and removes rackId from DAO update input. |
| rest-api/api/pkg/api/handler/expectedrack_test.go | Adds coverage for rename rejection, identical-id compatibility, and pre-mutation/pre-workflow guarantees. |
| rest-api/api/pkg/api/model/expectedrack.go | Clarifies API update-model semantics in comments for rackId immutability. |
| rest-api/api/pkg/api/model/expectedrack_test.go | Updates validation test wording to reflect handler-enforced immutability. |
| rest-api/openapi/spec.yaml | Documents rackId immutability for PATCH and marks rackId deprecated in the update request schema. |
| rest-api/sdk/standard/api_expected_rack.go | Propagates PATCH documentation update into the generated SDK client. |
| rest-api/sdk/standard/model_expected_rack_update_request.go | Propagates schema deprecation + immutability notes into the generated SDK model. |
Files not reviewed (2)
- rest-api/sdk/standard/api_expected_rack.go: Generated file
- rest-api/sdk/standard/model_expected_rack_update_request.go: Generated file
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![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
🤖 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 `@rest-api/openapi/spec.yaml`:
- Around line 24397-24399: Update the rackId schema description associated with
the deprecated field to document that providing JSON null also preserves the
stored rackId, alongside the existing omit-or-existing-value compatibility
behavior.
🪄 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: d3b74c68-d01b-45e3-a6c5-6fffbb6abe98
⛔ Files ignored due to path filters (2)
rest-api/sdk/standard/api_expected_rack.gois excluded by!rest-api/sdk/standard/api_*.gorest-api/sdk/standard/model_expected_rack_update_request.gois excluded by!rest-api/sdk/standard/model_*.go
📒 Files selected for processing (5)
rest-api/api/pkg/api/handler/expectedrack.gorest-api/api/pkg/api/handler/expectedrack_test.gorest-api/api/pkg/api/model/expectedrack.gorest-api/api/pkg/api/model/expectedrack_test.gorest-api/openapi/spec.yaml
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> Signed-off-by: Tanishq <139997424+dev-tnsq@users.noreply.github.com>
|
@chet @thossain-nv , can u have a look at this |
NVIDIA#4365) The rackId property in ExpectedRackUpdateRequest accepts null; document that null preserves the stored value, matching the immutable-identity contract. Regenerates sdk/standard. Signed-off-by: Tanishq <tanishq160405@gmail.com>
thossain-nv
changed the title
|
|
||
| For single updates (PATCH /expected-rack/{id}), the `id` field is optional in the body and will be ignored if provided (the `id` from the URL path is used). | ||
|
|
||
| The `rackId` field is immutable on update — omit it or provide the existing value. Renaming an Expected Rack is not supported. |
There was a problem hiding this comment.
I would omit the follow up Renaming an Expected Rack is not supported. In NICo REST, rename indicates changing the name attribute of a resource.
There was a problem hiding this comment.
thanks for the review, @thossain-nv! You're right that in NICo REST, 'rename' refers to the name attribute, so that phrasing was misleading. I've omitted it in the latest commit (b262c12).
…ption (NVIDIA#4365) In NICo REST, rename refers to changing the name attribute, not rackId. Remove the phrase from the request schema description and regenerate the SDK. Signed-off-by: Tanishq <tanishq160405@gmail.com>
|
/ok to test b262c12 |
|
🌿 Preview your docs: https://nvidia-preview-pull-request-4461.docs.buildwithfern.com/infra-controller |
🔐 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-08-03 18:12:56 UTC | Commit: b262c12 |
…VIDIA#4365) The ExpectedRackUpdateRequest rackId property description changed in the spec; rebuild rest-api/docs/index.html via make publish-openapi. Signed-off-by: Tanishq <tanishq160405@gmail.com>
Related issues
fix(rest-api): stop accepting unsupported ExpectedRack ID renamesType of Change
Breaking Changes
Testing
Additional Notes
Problem
PATCH /expected-rack/{id}accepted a newrackId, wrote it to Cloud, then ran theUpdateExpectedRackworkflow — which fails in Core because Core identifies expected racks byrack_id. The rename could never complete, and the Cloud row was mutated (then rolled back) before the failure surfaced.Solution
Treat
rackIdas immutable on update, following the same boundary already applied to ExpectedMachine BMC MAC identity (expectedmachine.go). A changed value is rejected with400(validation.Errors{"rackId": "RackID cannot be changed after creation"}) after site-access checks and before any database write or workflow trigger. Omitted or identical values remain a no-op, andRackIDis no longer passed into the DAO update input, so the update path is structurally incapable of a rename.Only single PATCH ever accepted a rename; site-inventory-sync (NICo as source of truth) and the
PUT /expected-rack/allReplaceAll endpoint are unaffected.Docs / generated code
OpenAPI updated (
rackIdmarked deprecated in the update request schema, PATCH operation description) andsdk/standardregenerated.Tests
Table-driven handler tests cover changed → 400, changed-to-existing → 400, identical → 200 (stored value preserved), and site-access-checked-before-identity → 403. A dedicated handler test asserts rejection happens without any DB mutation or workflow trigger. Model, DAO, and inventory-sync activity suites pass.