chore(rest-api): MIgrate Provider and Status Details DAOs to use struct params#2759
Open
nvlitagaki wants to merge 6 commits into
Open
chore(rest-api): MIgrate Provider and Status Details DAOs to use struct params#2759nvlitagaki wants to merge 6 commits into
nvlitagaki wants to merge 6 commits into
Conversation
Migrate StatusDetail, InfrastructureProvider, MachineInstanceType, and remaining DAOs to param-struct signatures and update all callers. Signed-off-by: Leah Itagaki <litagaki@nvidia.com>
Contributor
WalkthroughThe PR replaces positional DAO helper calls with typed input structs and ChangesDAO API Modernization: Typed Inputs and Unified Pagination
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
🔍 Container Scan Summary
Per-CVE detail lives in the per-service |
🔐 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-06-22 18:52:58 UTC | Commit: 5b2aa6f |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
rest-api/api/pkg/api/handler/dpuextensionservice.go (1)
310-314:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard against nil before dereferencing
statusDetailin the ready-status append path.Line 310 returns a pointer, but Line 314 dereferences it unconditionally. If
sdDAO.Createever returnsnil, nil, this path panics after a successful create flow.Suggested fix
- statusDetail, serr := sdDAO.Create(ctx, nil, cdbm.StatusDetailCreateInput{EntityID: dpuExtensionService.ID.String(), Status: cdbm.DpuExtensionServiceStatusReady, Message: cutil.GetPtr("DPU Extension Service is ready for deployment")}) + statusDetail, serr := sdDAO.Create(ctx, nil, cdbm.StatusDetailCreateInput{EntityID: dpuExtensionService.ID.String(), Status: cdbm.DpuExtensionServiceStatusReady, Message: cutil.GetPtr("DPU Extension Service is ready for deployment")}) if serr != nil { logger.Error().Err(serr).Msg("error creating Status Detail DB entry") + } else if statusDetail == nil { + logger.Error().Msg("Status Detail DB entry not returned from Create") } else { statusDetails = append(statusDetails, *statusDetail) }🤖 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 `@rest-api/api/pkg/api/handler/dpuextensionservice.go` around lines 310 - 314, The code dereferences the statusDetail pointer returned from sdDAO.Create without verifying it is not nil, which will cause a panic if the pointer is nil even when serr is nil. In the else block where statusDetails is appended, add a nil check to verify statusDetail is not nil before dereferencing it with the asterisk operator on the line that appends to the statusDetails slice.rest-api/db/pkg/db/model/statusdetail.go (1)
219-274:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUpdate the selected status row by
id, not byentity_id.Line 231 loads one row via
input.StatusDetailID, but Line 274 updates every row sharing the sameEntityID. For entities with status history, this can overwrite multiple historical rows and their counts.Proposed fix
- _, err = db.GetIDB(tx, sdd.dbSession).NewUpdate().Model(upsd).Column(updatedFields...).Where("entity_id = ?", sd.EntityID).Exec(ctx) + _, err = db.GetIDB(tx, sdd.dbSession).NewUpdate().Model(upsd).Column(updatedFields...).Where("id = ?", input.StatusDetailID).Exec(ctx)🤖 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 `@rest-api/db/pkg/db/model/statusdetail.go` around lines 219 - 274, The Update method in StatusDetailSQLDAO loads a specific StatusDetail row using id via input.StatusDetailID in the NewSelect call, but then incorrectly updates using entity_id in the final NewUpdate call. This causes all StatusDetail rows sharing the same EntityID to be updated instead of just the one that was loaded. Change the Where clause in the db.GetIDB(tx, sdd.dbSession).NewUpdate().Model(upsd).Column(updatedFields...).Where(...).Exec(ctx) statement to use Where("id = ?", sd.ID) instead of Where("entity_id = ?", sd.EntityID) to ensure only the specific row is updated.
🧹 Nitpick comments (3)
rest-api/api/pkg/api/handler/util/common/common.go (1)
360-360: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winUse zero-limit pagination for count-only DAO calls.
This helper only uses
tot;cdbp.PageInput{}can still fetch a default page of rows. SetLimitto0to avoid unnecessary scan work.Suggested patch
- _, tot, err := mitDAO.GetAll(ctx, tx, cdbm.MachineInstanceTypeFilterInput{InstanceTypeIDs: []uuid.UUID{instanceTypeID}}, cdbp.PageInput{}, nil) + _, tot, err := mitDAO.GetAll( + ctx, + tx, + cdbm.MachineInstanceTypeFilterInput{InstanceTypeIDs: []uuid.UUID{instanceTypeID}}, + cdbp.PageInput{Limit: cutil.GetPtr(0)}, + nil, + )🤖 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 `@rest-api/api/pkg/api/handler/util/common/common.go` at line 360, The mitDAO.GetAll call in the common.go helper function only uses the total count (tot) but passes an empty cdbp.PageInput{} which applies a default page limit causing unnecessary row scanning. Modify the PageInput argument in the mitDAO.GetAll call to explicitly set Limit to 0, changing cdbp.PageInput{} to cdbp.PageInput{Limit: 0}, so that only the count is fetched without scanning unnecessary rows.rest-api/api/pkg/api/handler/vpcprefix.go (1)
857-857: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winMove response-only status reads out of the transaction.
Line 857 fetches status history only for response construction, but it runs inside
cdb.WithTxbefore a synchronous workflow wait. That unnecessarily extends transaction/connection hold time.♻️ Suggested refactor
@@ - fetchedSSDs, _, derr := sdDAO.GetAllByEntityID(ctx, tx, updated.ID.String(), cdbp.PageInput{Limit: cutil.GetPtr(pagination.MaxPageSize)}) - if derr != nil { - logger.Error().Err(derr).Msg("error retrieving Status Details for VPC prefix from DB") - return cutil.NewAPIError(http.StatusInternalServerError, "Failed to retrieve Status Details for VPC prefix", nil) - } - ssds = fetchedSSDs @@ if timeoutResp != nil { return timeoutResp() } + + fetchedSSDs, _, err := sdDAO.GetAllByEntityID( + ctx, + nil, + updatedVpcPrefix.ID.String(), + cdbp.PageInput{Limit: cutil.GetPtr(pagination.MaxPageSize)}, + ) + if err != nil { + logger.Error().Err(err).Msg("error retrieving Status Details for VPC prefix from DB") + return cutil.NewAPIErrorResponse(c, http.StatusInternalServerError, "Failed to retrieve Status Details for VPC prefix", nil) + } + ssds = fetchedSSDsAs per coding guidelines: “Reads belong outside the transaction unless they need to be inside.”
🤖 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 `@rest-api/api/pkg/api/handler/vpcprefix.go` at line 857, The call to sdDAO.GetAllByEntityID that retrieves fetchedSSDs on line 857 is currently executing inside the cdb.WithTx transaction block, but this data is only used for response construction and does not require transaction consistency. Move this sdDAO.GetAllByEntityID call outside the cdb.WithTx block to execute after the transaction completes and before constructing the response. This reduces unnecessary transaction and connection hold time in alignment with the coding guideline that reads should occur outside transactions unless they require transactional guarantees.Source: Coding guidelines
rest-api/api/pkg/api/handler/machineinstancetype.go (1)
221-221: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winBound existence checks to a single row.
Lines 221 and 605-608 only need “exists/first match”, but
paginator.PageInput{}allows larger scans than necessary. SettingLimit: 1avoids extra DB work on hot paths.⚡ Suggested change
- emits, _, derr := mitDAO.GetAll(ctx, tx, cdbm.MachineInstanceTypeFilterInput{MachineID: &machineID}, paginator.PageInput{}, nil) + emits, _, derr := mitDAO.GetAll(ctx, tx, cdbm.MachineInstanceTypeFilterInput{MachineID: &machineID}, paginator.PageInput{Limit: cutil.GetPtr(1)}, nil) @@ - }, paginator.PageInput{}, nil) + }, paginator.PageInput{Limit: cutil.GetPtr(1)}, nil)Also applies to: 605-608
🤖 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 `@rest-api/api/pkg/api/handler/machineinstancetype.go` at line 221, The GetAll call on line 221 and similar calls around lines 605-608 are performing existence checks but use paginator.PageInput{} without a Limit, causing unnecessary database scans. Fix this by setting Limit: 1 in the PageInput struct passed to mitDAO.GetAll() to retrieve only the first matching row, and apply this same optimization to the equivalent calls around line 605-608 to avoid extra database work on these hot paths.
🤖 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/api/pkg/api/handler/machine_test.go`:
- Around line 2762-2765: The mitDAO.GetAll() method call is currently ignoring
the error return value with an underscore placeholder, which can mask database
or query failures in the test. Refactor the line to capture the error return
value instead of discarding it, then add an assertion immediately after to
verify that the error is nil before proceeding with the validation of the emits
and total results. This ensures that any query failures are properly detected
and reported during the test execution.
In `@rest-api/api/pkg/api/handler/site.go`:
- Around line 198-203: When sdDAO.Create() fails or returns nil, instead of
logging the error and continuing execution, the code must return
cutil.NewAPIError(...) to properly signal the failure and prevent persisting
incomplete transaction state. In the error handler block for sdDAO.Create when
derr is not nil, replace the logger.Error() call with a return statement that
creates and returns an appropriate API error. Similarly, when createdSSD is nil,
return cutil.NewAPIError(...) instead of only logging. Apply the same fix
pattern to the second location where sdDAO.Create() is called with the same
logging-only error handling approach.
In `@rest-api/api/pkg/api/handler/tenantaccount.go`:
- Around line 706-739: The eligibility check for "Invited" status is performed
outside the WithTx transaction, creating a TOCTOU race condition where multiple
concurrent requests can pass validation and both execute the update inside the
transaction body. To fix this, move the TenantAccount eligibility validation
(the "Invited" status check) inside the WithTx callback, so it occurs within the
same transaction snapshot as the taDAO.Update and sdDAO.Create calls. This
ensures that between validating the current state and performing the mutation,
no other request can concurrently update the same TenantAccount. You may also
consider acquiring an advisory lock on the TenantAccount at the start of the
transaction to further protect against concurrent modifications.
In `@rest-api/db/pkg/db/model/infrastructureprovider_test.go`:
- Around line 282-285: Replace the `t.Errorf` error assertion in the Create
method test case (around lines 282-285) with testify's `require.NoError` or
`require.Equal` to align with project conventions already used elsewhere in the
file. Similarly, update the Update method test case (around lines 398-401) with
testify assertions instead of `t.Errorf`. Additionally, split the inline
assign-and-condition statement at line 491 into two separate statements (one for
assignment and one for the condition check) to comply with the project's style
guidelines for splitting assign-and-condition operations. Ensure all error
handling uses the testify library consistently throughout the test file as
established in the existing patterns.
In `@rest-api/db/pkg/db/model/machineinstancetype.go`:
- Around line 210-216: Replace the nil check condition for
filter.InstanceTypeIDs in the machineinstancetype.go file from checking if it is
not nil to instead checking if its length is greater than zero. This prevents
empty-but-initialized slices from entering the filter logic and ensures the
filter is only applied when there are actual instance type IDs to filter on.
Change the condition from `filter.InstanceTypeIDs != nil` to
`len(filter.InstanceTypeIDs) > 0` in the filter block that handles the WHERE
clause for instance_type_id filtering.
In `@rest-api/db/pkg/db/model/statusdetail_test.go`:
- Around line 513-536: The Update test uses manual error handling and
dereferences the got object without nil protection, which could panic if wantErr
is true. Replace the manual `if (err != nil) != tt.wantErr` check with
`require.NoError(t, err)` to fail fast on error, add `require.NotNil(t, got)`
before accessing got fields, and replace all subsequent manual field assertions
(EntityID, Status, Message, Count, Updated comparisons) with appropriate testify
assert or require functions to comply with the coding guideline that mandates
testify usage in all rest-api/**/*_test.go files.
---
Outside diff comments:
In `@rest-api/api/pkg/api/handler/dpuextensionservice.go`:
- Around line 310-314: The code dereferences the statusDetail pointer returned
from sdDAO.Create without verifying it is not nil, which will cause a panic if
the pointer is nil even when serr is nil. In the else block where statusDetails
is appended, add a nil check to verify statusDetail is not nil before
dereferencing it with the asterisk operator on the line that appends to the
statusDetails slice.
In `@rest-api/db/pkg/db/model/statusdetail.go`:
- Around line 219-274: The Update method in StatusDetailSQLDAO loads a specific
StatusDetail row using id via input.StatusDetailID in the NewSelect call, but
then incorrectly updates using entity_id in the final NewUpdate call. This
causes all StatusDetail rows sharing the same EntityID to be updated instead of
just the one that was loaded. Change the Where clause in the db.GetIDB(tx,
sdd.dbSession).NewUpdate().Model(upsd).Column(updatedFields...).Where(...).Exec(ctx)
statement to use Where("id = ?", sd.ID) instead of Where("entity_id = ?",
sd.EntityID) to ensure only the specific row is updated.
---
Nitpick comments:
In `@rest-api/api/pkg/api/handler/machineinstancetype.go`:
- Line 221: The GetAll call on line 221 and similar calls around lines 605-608
are performing existence checks but use paginator.PageInput{} without a Limit,
causing unnecessary database scans. Fix this by setting Limit: 1 in the
PageInput struct passed to mitDAO.GetAll() to retrieve only the first matching
row, and apply this same optimization to the equivalent calls around line
605-608 to avoid extra database work on these hot paths.
In `@rest-api/api/pkg/api/handler/util/common/common.go`:
- Line 360: The mitDAO.GetAll call in the common.go helper function only uses
the total count (tot) but passes an empty cdbp.PageInput{} which applies a
default page limit causing unnecessary row scanning. Modify the PageInput
argument in the mitDAO.GetAll call to explicitly set Limit to 0, changing
cdbp.PageInput{} to cdbp.PageInput{Limit: 0}, so that only the count is fetched
without scanning unnecessary rows.
In `@rest-api/api/pkg/api/handler/vpcprefix.go`:
- Line 857: The call to sdDAO.GetAllByEntityID that retrieves fetchedSSDs on
line 857 is currently executing inside the cdb.WithTx transaction block, but
this data is only used for response construction and does not require
transaction consistency. Move this sdDAO.GetAllByEntityID call outside the
cdb.WithTx block to execute after the transaction completes and before
constructing the response. This reduces unnecessary transaction and connection
hold time in alignment with the coding guideline that reads should occur outside
transactions unless they require transactional guarantees.
🪄 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: 05dfb4bb-c07f-476a-836a-6e1b1bceca5c
📒 Files selected for processing (72)
rest-api/api/pkg/api/handler/allocation.gorest-api/api/pkg/api/handler/allocation_test.gorest-api/api/pkg/api/handler/dpuextensionservice.gorest-api/api/pkg/api/handler/fabric_test.gorest-api/api/pkg/api/handler/infinibandpartition.gorest-api/api/pkg/api/handler/infrastructureprovider.gorest-api/api/pkg/api/handler/instance.gorest-api/api/pkg/api/handler/instance_test.gorest-api/api/pkg/api/handler/instancetype.gorest-api/api/pkg/api/handler/ipblock.gorest-api/api/pkg/api/handler/ipblock_test.gorest-api/api/pkg/api/handler/machine.gorest-api/api/pkg/api/handler/machine_test.gorest-api/api/pkg/api/handler/machineinstancetype.gorest-api/api/pkg/api/handler/machineinstancetype_test.gorest-api/api/pkg/api/handler/networksecuritygroup.gorest-api/api/pkg/api/handler/nvlinklogicalpartition.gorest-api/api/pkg/api/handler/operatingsystem.gorest-api/api/pkg/api/handler/serviceaccount.gorest-api/api/pkg/api/handler/site.gorest-api/api/pkg/api/handler/site_test.gorest-api/api/pkg/api/handler/sshkey.gorest-api/api/pkg/api/handler/sshkeygroup.gorest-api/api/pkg/api/handler/statusdetail.gorest-api/api/pkg/api/handler/subnet.gorest-api/api/pkg/api/handler/tenantaccount.gorest-api/api/pkg/api/handler/tenantaccount_test.gorest-api/api/pkg/api/handler/util/common/common.gorest-api/api/pkg/api/handler/util/common/common_test.gorest-api/api/pkg/api/handler/util/common/testing.gorest-api/api/pkg/api/handler/vpc.gorest-api/api/pkg/api/handler/vpc_test.gorest-api/api/pkg/api/handler/vpcpeering.gorest-api/api/pkg/api/handler/vpcprefix.gorest-api/db/pkg/db/model/allocation.gorest-api/db/pkg/db/model/infrastructureprovider.gorest-api/db/pkg/db/model/infrastructureprovider_test.gorest-api/db/pkg/db/model/instancetype.gorest-api/db/pkg/db/model/interface.gorest-api/db/pkg/db/model/machinecapability.gorest-api/db/pkg/db/model/machineinstancetype.gorest-api/db/pkg/db/model/machineinstancetype_test.gorest-api/db/pkg/db/model/machineinterface.gorest-api/db/pkg/db/model/site.gorest-api/db/pkg/db/model/statusdetail.gorest-api/db/pkg/db/model/statusdetail_test.gorest-api/db/pkg/db/model/testing.gorest-api/db/pkg/db/model/vpc.gorest-api/db/pkg/db/model/vpc_test.gorest-api/workflow/pkg/activity/dpuextensionservice/dpuextensionservice.gorest-api/workflow/pkg/activity/infinibandpartition/infinibandpartition.gorest-api/workflow/pkg/activity/instance/instance.gorest-api/workflow/pkg/activity/instance/instance_test.gorest-api/workflow/pkg/activity/instancetype/instancetype_test.gorest-api/workflow/pkg/activity/machine/machine.gorest-api/workflow/pkg/activity/machine/machine_test.gorest-api/workflow/pkg/activity/networksecuritygroup/networksecuritygroup_test.gorest-api/workflow/pkg/activity/nvlinklogicalpartition/nvlinklogicalpartition.gorest-api/workflow/pkg/activity/operatingsystem/operatingsystem.gorest-api/workflow/pkg/activity/site/site.gorest-api/workflow/pkg/activity/sshkeygroup/sshkeygroup.gorest-api/workflow/pkg/activity/sshkeygroup/sshkeygroup_test.gorest-api/workflow/pkg/activity/subnet/subnet.gorest-api/workflow/pkg/activity/subnet/subnet_test.gorest-api/workflow/pkg/activity/vpc/vpc.gorest-api/workflow/pkg/activity/vpc/vpc_test.gorest-api/workflow/pkg/activity/vpcpeering/vpcpeering.gorest-api/workflow/pkg/activity/vpcpeering/vpcpeering_test.gorest-api/workflow/pkg/activity/vpcprefix/vpcprefix.gorest-api/workflow/pkg/activity/vpcprefix/vpcprefix_test.gorest-api/workflow/pkg/util/common.gorest-api/workflow/pkg/util/testing.go
nvlitagaki
force-pushed
the
chore/finish-dao-migration
branch
from
c4f616f to
71c330b
Compare
|
🌿 Preview your docs: https://nvidia-preview-pull-request-2759.docs.buildwithfern.com/infra-controller |
Contributor
|
@nvlitagaki I'm looking through this today. |
Contributor
|
@thossain-nv pinging you on this review, as we try to keep our PR pipeline within SLA. |
thossain-nv
approved these changes
Jun 25, 2026
thossain-nv
left a comment
thossain-nv
left a comment
Contributor
There was a problem hiding this comment.
Looks good @nvlitagaki, thanks for the changes. cc @mxh-0xbb
| type StatusDetailDAO interface { | ||
| // | ||
| GetAllByEntityID(ctx context.Context, tx *db.Tx, entityID string, offset *int, limit *int, orderBy *paginator.OrderBy) ([]StatusDetail, int, error) | ||
| GetAllByEntityID(ctx context.Context, tx *db.Tx, entityID string, page paginator.PageInput) ([]StatusDetail, int, error) |
Contributor
There was a problem hiding this comment.
Should we converge these 2 into a GetAll?
thossain-nv
changed the title
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
3 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.
Migrate StatusDetail, InfrastructureProvider, MachineInstanceType, and remaining DAOs to param-struct signatures and update all callers.
Also updated tenantaccount.go update handler to use a database transaction based on a coderabbit suggestion
Related issues
n/a
Type of Change
Breaking Changes
Testing
Additional Notes
This is the last of these DAO refactoring changes.