feat: Support filter by location info in Flow Tray API#545
Open
kunzhao-nv wants to merge 4 commits into
Open
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (7)
✅ Files skipped from review due to trivial changes (1)
Summary by CodeRabbit
WalkthroughAdds optional ChangesTray Position Targeting
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
api/pkg/api/model/tray_test.go (1)
637-722: ⚡ Quick winAdd symmetric position-filter tests for
APITrayValidateAllRequest.The model now adds slot/tray-index validation/matching/query serialization for
APITrayValidateAllRequest, but this file only adds position tests forTrayFilterandAPITrayGetAllRequest. Add a small table test set for Validate/HasPositionFilter/MatchesPosition/QueryValues to prevent contract drift.🤖 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/model/tray_test.go` around lines 637 - 722, Add a new table-driven test in tray_test.go that mirrors the TrayFilter position tests but targets APITrayValidateAllRequest: create cases exercising Validate(), HasPositionFilter(), MatchesPosition(component *flowv1.Component) and QueryValues() for combinations (nil request, rack-only, slot-only with rack, slot+trayIdx, trayIdx without slot, slot with IDs) and assert expected boolean results and validation errors; reference the APITrayValidateAllRequest type and its methods Validate, HasPositionFilter, MatchesPosition, and QueryValues so the test verifies validation, position matching behavior, and generated query params to prevent contract drift.api/pkg/api/model/tray.go (1)
193-208: ⚡ Quick winConsolidate duplicated position-matching logic to prevent drift.
MatchesPositionis implemented three times with identical logic. Extracting a shared helper will reduce future divergence risk and keep behavior consistent across filters/requests.♻️ Proposed refactor
+func hasPositionFilter(slot *int32) bool { + return slot != nil +} + +func matchesRackPosition(comp *flowv1.Component, slot, trayIdx *int32) bool { + if slot == nil { + return true + } + pos := comp.GetPosition() + if pos == nil { + return false + } + if pos.GetSlotId() != *slot { + return false + } + if trayIdx != nil && pos.GetTrayIdx() != *trayIdx { + return false + } + return true +} + func (f *TrayFilter) HasPositionFilter() bool { - return f != nil && f.Slot != nil + return f != nil && hasPositionFilter(f.Slot) } func (f *TrayFilter) MatchesPosition(comp *flowv1.Component) bool { - if f == nil || f.Slot == nil { - return true - } - pos := comp.GetPosition() - if pos == nil { - return false - } - if pos.GetSlotId() != *f.Slot { - return false - } - if f.TrayIdx != nil && pos.GetTrayIdx() != *f.TrayIdx { - return false - } - return true + if f == nil { + return true + } + return matchesRackPosition(comp, f.Slot, f.TrayIdx) }Also applies to: 374-389, 572-587
🤖 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/model/tray.go` around lines 193 - 208, Multiple identical implementations of MatchesPosition are drifting; extract a single helper function (e.g., PositionMatches or matchesComponentPosition) in the same package and replace each MatchesPosition implementation with a call to it. The helper should accept a *flowv1.Component or its Position plus the slot and trayIdx pointers, perform the nil checks and comparisons (f == nil || slot == nil => true; pos == nil => false; slot mismatch => false; trayIdx mismatch => false), and return the boolean; update the existing methods named MatchesPosition to delegate to this helper so all three locations use the same shared logic.api/pkg/api/handler/tray.go (1)
559-559: 💤 Low valueConsider a more explicit slice initialization for clarity.
The expression
components[:0:0]is a valid Go idiom to create a new slice that will allocate a fresh backing array on append, but it may hinder readability for developers unfamiliar with three-index slice syntax. A more conventional approach improves intent signaling.♻️ Suggested refactor
- filtered := components[:0:0] + filtered := make([]*flowv1.Component, 0, len(components))🤖 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/tray.go` at line 559, Replace the terse three-index slice expression components[:0:0] (used to initialize filtered) with an explicit make call using the element type of components (e.g., filtered := make([]ElementType, 0, 0)); locate the element type from the declaration of components and use that type in make to create a clear zero-length zero-capacity slice, keeping the rest of the logic that appends into filtered 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.
Nitpick comments:
In `@api/pkg/api/handler/tray.go`:
- Line 559: Replace the terse three-index slice expression components[:0:0]
(used to initialize filtered) with an explicit make call using the element type
of components (e.g., filtered := make([]ElementType, 0, 0)); locate the element
type from the declaration of components and use that type in make to create a
clear zero-length zero-capacity slice, keeping the rest of the logic that
appends into filtered unchanged.
In `@api/pkg/api/model/tray_test.go`:
- Around line 637-722: Add a new table-driven test in tray_test.go that mirrors
the TrayFilter position tests but targets APITrayValidateAllRequest: create
cases exercising Validate(), HasPositionFilter(), MatchesPosition(component
*flowv1.Component) and QueryValues() for combinations (nil request, rack-only,
slot-only with rack, slot+trayIdx, trayIdx without slot, slot with IDs) and
assert expected boolean results and validation errors; reference the
APITrayValidateAllRequest type and its methods Validate, HasPositionFilter,
MatchesPosition, and QueryValues so the test verifies validation, position
matching behavior, and generated query params to prevent contract drift.
In `@api/pkg/api/model/tray.go`:
- Around line 193-208: Multiple identical implementations of MatchesPosition are
drifting; extract a single helper function (e.g., PositionMatches or
matchesComponentPosition) in the same package and replace each MatchesPosition
implementation with a call to it. The helper should accept a *flowv1.Component
or its Position plus the slot and trayIdx pointers, perform the nil checks and
comparisons (f == nil || slot == nil => true; pos == nil => false; slot mismatch
=> false; trayIdx mismatch => false), and return the boolean; update the
existing methods named MatchesPosition to delegate to this helper so all three
locations use the same shared logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: cd04c7de-a0a2-47d7-ba1f-a6042581bc7b
📒 Files selected for processing (4)
api/pkg/api/handler/tray.goapi/pkg/api/model/tray.goapi/pkg/api/model/tray_test.goopenapi/spec.yaml
Add slot and trayIdx as independent position filters on TrayFilter and the list/validate request types. Either, both, or neither may be set, and they compose with the rest of the filter via AND: a tray must satisfy every set field to match. An incompatible combination (e.g. ids that don't sit at the requested slot) is not a 400 — it just yields an empty result, matching the AND-of-filters semantics the rest of the filter already follows. Flow has no by-position component-target shape, so the REST layer resolves position to component UUIDs by running GetTrays against whatever scope the request would otherwise have produced (rack scope, ids, componentIds, or site-wide), post-filters the response by slot and trayIdx, and drives the downstream workflow with a ComponentTargets spec built from the resolved UUIDs. The list handler skips Flow-side pagination when a position filter is active and re-paginates after the post-filter so the total count reflects the filtered set. Signed-off-by: Kun Zhao <kunzhao@nvidia.com>
kunzhao-nv
force-pushed
the
feat/tray-location
branch
from
2ff17c9 to
d23ad8b
Compare
🔐 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-18 08:09:09 UTC | Commit: d23ad8b |
zhaozhongn
reviewed
May 19, 2026
Drop trayIdx position filtering and rename slot to slotId so query/body parameters align with TrayPosition.slotId. REST resolves slotId to component UUIDs before driving Flow workflows, as before. Signed-off-by: Kun Zhao <kunzhao@nvidia.com>
# Conflicts: # docs/index.html
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
api/pkg/api/model/tray_test.go (1)
449-473: ⚡ Quick winAdd negative
slotIdvalidation test casesNew slot-based tests cover valid compositions, but they do not assert rejection for invalid negative
slotId. Please add one negative case forAPITrayGetAllRequest.Validate()and one forTrayFilter.Validate().Also applies to: 666-701
🤖 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/model/tray_test.go` around lines 449 - 473, Add negative test cases asserting validation fails for negative slotId: in the APITrayGetAllRequest table (tests exercising APITrayGetAllRequest.Validate) add a case like name: "negative slotId invalid", req: APITrayGetAllRequest{SlotID: int32Ptr(-1)}, wantErr: true; and in the TrayFilter table (tests for TrayFilter.Validate) add a case like name: "negative slotId invalid", filter: TrayFilter{SlotID: int32Ptr(-1)}, wantErr: true. Use the same int32Ptr helper already in the file and ensure the test harness expects an error for these cases so APITrayGetAllRequest.Validate and TrayFilter.Validate are exercised for negative slot IDs.api/pkg/api/handler/tray.go (1)
113-113: ⚡ Quick winSplit assign-and-condition for error handling
Please split the inline assignment into separate statements to match repository Go conventions.
As per coding guidelines: "Split assign-and-condition into two statements; prefer separate `derr := action()` and `if derr != nil` over `if derr := action(); derr != nil`."Proposed fix
- if err := we.Get(wfCtx, &resp); err != nil { + err = we.Get(wfCtx, &resp) + if err != nil { var timeoutErr *tp.TimeoutError if errors.As(err, &timeoutErr) || errors.Is(err, context.DeadlineExceeded) || wfCtx.Err() != nil { return nil, fmt.Errorf("GetTrays workflow timed out: %w", err) } return nil, fmt.Errorf("get GetTrays result: %w", err) }🤖 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/tray.go` at line 113, The inline assignment in the if statement using we.Get(wfCtx, &resp) should be split into two statements following repo Go conventions: first call we.Get and store the error in a named variable (e.g., derr or err) and then check that variable in a separate if (e.g., derr := we.Get(wfCtx, &resp); if derr != nil -> split to derr := we.Get(wfCtx, &resp) on its own line, then if derr != nil { ... }). Update the code around we.Get, wfCtx, and resp to use the separated assignment and conditional check.
🤖 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/tray.go`:
- Around line 61-69: The matches method can panic when comp is nil; add a nil
guard at the top of positionMatcher.matches to return false if comp == nil
before calling comp.GetPosition(). Keep the existing p.active() logic (which can
stay before or after the nil check), then safely call comp.GetPosition() and
compare slot IDs as before (referencing positionMatcher.matches,
comp.GetPosition, and p.SlotID).
In `@api/pkg/api/model/tray.go`:
- Around line 131-132: The new SlotID pointer field on API tray requests can be
negative and currently lacks validation; update both
APITrayGetAllRequest.Validate() and APITrayValidateAllRequest.Validate() to
reject negative values by adding the same check used elsewhere (e.g., if
r.SlotID != nil && *r.SlotID < 0 { return validation error }). Ensure the error
message clearly references SlotID and returns a validation error consistent with
existing patterns so negative slot IDs are rejected early.
- Around line 110-118: matchesRackSlot currently dereferences comp without
checking for nil which can cause a panic; add a nil guard at the top of the
function (check comp == nil) and return false if comp is nil, then proceed to
the existing slotID and position checks (use the existing comp.GetPosition() and
pos.GetSlotId() logic unchanged). Ensure you reference the same function name
matchesRackSlot and variables comp and slotID so callers' behavior stays
consistent.
---
Nitpick comments:
In `@api/pkg/api/handler/tray.go`:
- Line 113: The inline assignment in the if statement using we.Get(wfCtx, &resp)
should be split into two statements following repo Go conventions: first call
we.Get and store the error in a named variable (e.g., derr or err) and then
check that variable in a separate if (e.g., derr := we.Get(wfCtx, &resp); if
derr != nil -> split to derr := we.Get(wfCtx, &resp) on its own line, then if
derr != nil { ... }). Update the code around we.Get, wfCtx, and resp to use the
separated assignment and conditional check.
In `@api/pkg/api/model/tray_test.go`:
- Around line 449-473: Add negative test cases asserting validation fails for
negative slotId: in the APITrayGetAllRequest table (tests exercising
APITrayGetAllRequest.Validate) add a case like name: "negative slotId invalid",
req: APITrayGetAllRequest{SlotID: int32Ptr(-1)}, wantErr: true; and in the
TrayFilter table (tests for TrayFilter.Validate) add a case like name: "negative
slotId invalid", filter: TrayFilter{SlotID: int32Ptr(-1)}, wantErr: true. Use
the same int32Ptr helper already in the file and ensure the test harness expects
an error for these cases so APITrayGetAllRequest.Validate and
TrayFilter.Validate are exercised for negative slot IDs.
🪄 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: 49b2878c-e6b3-438f-af5b-3063832d21fb
📒 Files selected for processing (7)
api/pkg/api/handler/tray.goapi/pkg/api/model/tray.goapi/pkg/api/model/tray_test.godocs/index.htmlopenapi/spec.yamlsdk/standard/api_tray.gosdk/standard/model_tray_filter.go
💤 Files with no reviewable changes (3)
- sdk/standard/api_tray.go
- openapi/spec.yaml
- sdk/standard/model_tray_filter.go
thossain-nv
reviewed
May 22, 2026
Contributor
thossain-nv
left a comment
thossain-nv
left a comment
There was a problem hiding this comment.
@kunzhao-nv Can ComponentTarget accept SlotID? Then we don't have to make an extra call to resolve TrayID.
Contributor
Author
Per earlier team discussion, we don't want to extend ComponentTarget for now. |
Require rackId or rackName when slotId is set, reject negative slot IDs, and consolidate slot matching under RackComponentSlotMatcher with clearer handler resolver naming. Regenerate OpenAPI docs, SDK, and tests. Signed-off-by: Kun Zhao <kunzhao@nvidia.com>
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.
Description
The tray REST APIs already accepted
rackId/rackNamefor selecting trays in a rack, but had no way to point at a specific position within that rack — the natural human identifier of "rack a12, slot 3". Add optionalslotIdtoTrayFilter,APITrayGetAllRequestandAPITrayValidateAllRequest.Type of Change
Services Affected
Related Issues (Optional)
Breaking Changes
Testing
Additional Notes