feat: Support filter by location info in Flow Tray API#545
Draft
kunzhao-nv wants to merge 1 commit into
Draft
Conversation
Contributor
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThis PR introduces position-based filtering for tray endpoints via optional ChangesTray Position Targeting
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 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
| // @Param componentId query string false "Filter by component ID (use repeated params for multiple values)" | ||
| // @Param id query string false "Filter by tray UUID (use repeated params for multiple values)" | ||
| // @Param slot query int false "Filter by rack slot ID. Requires rackId or rackName; mutually exclusive with id/componentId." | ||
| // @Param trayIdx query int false "Disambiguates trays sharing the same slot. Requires slot." |
Contributor
There was a problem hiding this comment.
Why? Slot number is per rack, tray index is per <rack, tray type>. These two things have nothing to do with each other. A tray is always staying in one slot and no trays share a same slot.
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.
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 optionalslotandtrayIdxtoTrayFilter,APITrayGetAllRequestandAPITrayValidateAllRequest.Type of Change
Services Affected
Related Issues (Optional)
Breaking Changes
Testing
Additional Notes