HYPERFLEET-1142 - fix: enable field filtering on single-resource GET#202
HYPERFLEET-1142 - fix: enable field filtering on single-resource GET#202ldornele wants to merge 3 commits into
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Central YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (4)
💤 Files with no reviewable changes (1)
✅ Files skipped from review due to trivial changes (2)
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds conditional nested-field validation and an exported FilterSingle(fields, resource) helper that projects a single presented resource into map[string]interface{}. Integrates FilterSingle into single-item GET handlers (Cluster, NodePool, ClusterNodePools, Resource.Get/GetByOwner). Updates presenter defaults and adds comprehensive table-driven tests for Cluster and NodePool filtering plus handler tests exercising the fields query parameter and error cases. Sequence Diagram(s)sequenceDiagram
participant Client
participant Handler
participant Presenter
participant FilterSingle
Client->>Handler: GET /resource/{id}?fields=...
Handler->>Presenter: present(resource)
Presenter->>Handler: presented struct
alt fields provided
Handler->>FilterSingle: FilterSingle(fields, presented)
FilterSingle->>Handler: projected map[string]interface{}
Handler->>Client: 200 JSON(projected)
else no fields
Handler->>Client: 200 JSON(presented)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
Risk Score: 2 —
|
| Signal | Detail | Points |
|---|---|---|
| PR size | 805 lines (>500) | +2 |
| Sensitive paths | none | +0 |
| Test coverage | Tests cover changed packages | +0 |
Computed by hyperfleet-risk-scorer
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/handlers/cluster.go (1)
145-154: ⚡ Quick winSame filter block copy-pasted across 5 GET handlers — extract a helper.
This exact "parse
listArgs→ ifFields != nilfilter viaFilterSingle" snippet is repeated inClusterHandler.Get,ClusterNodePoolsHandler.Get(Lines 104-112),NodePoolHandler.Get(Lines 155-163), andResourceHandler.Get/GetByOwner(Lines 64-72, 182-190). One shared helper keeps the projection contract consistent across resource types and avoids drift.♻️ Suggested helper (e.g. in a shared handler util)
func applyFieldFilter(r *http.Request, presented interface{}) (interface{}, *errors.ServiceError) { listArgs := services.NewListArguments(r.URL.Query()) if listArgs.Fields == nil { return presented, nil } return presenters.FilterSingle(listArgs.Fields, presented) }Each call site collapses to:
- listArgs := services.NewListArguments(r.URL.Query()) - if listArgs.Fields != nil { - filtered, filterErr := presenters.FilterSingle(listArgs.Fields, presented) - if filterErr != nil { - return nil, filterErr - } - return filtered, nil - } - return presented, nil + return applyFieldFilter(r, presented)🤖 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 `@pkg/handlers/cluster.go` around lines 145 - 154, Extract the repeated "parse listArgs → apply Fields filter" logic into a shared helper (e.g. applyFieldFilter) that takes the *http.Request and the presented value and returns (interface{}, *errors.ServiceError); implement it to call services.NewListArguments(r.URL.Query()) and, if listArgs.Fields != nil, call presenters.FilterSingle(listArgs.Fields, presented) and return that result, otherwise return presented; then replace the duplicate blocks in ClusterHandler.Get, ClusterNodePoolsHandler.Get, NodePoolHandler.Get, and ResourceHandler.Get/GetByOwner with a single call to this helper to keep projection behavior consistent.
🤖 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 `@pkg/handlers/cluster.go`:
- Around line 145-154: Extract the repeated "parse listArgs → apply Fields
filter" logic into a shared helper (e.g. applyFieldFilter) that takes the
*http.Request and the presented value and returns (interface{},
*errors.ServiceError); implement it to call
services.NewListArguments(r.URL.Query()) and, if listArgs.Fields != nil, call
presenters.FilterSingle(listArgs.Fields, presented) and return that result,
otherwise return presented; then replace the duplicate blocks in
ClusterHandler.Get, ClusterNodePoolsHandler.Get, NodePoolHandler.Get, and
ResourceHandler.Get/GetByOwner with a single call to this helper to keep
projection behavior consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: c07797e3-62e7-4d91-bb93-426f80a848a9
📒 Files selected for processing (8)
pkg/api/presenters/presenter_test.gopkg/api/presenters/slice_filter.gopkg/handlers/cluster.gopkg/handlers/cluster_nodepools.gopkg/handlers/cluster_nodepools_test.gopkg/handlers/cluster_test.gopkg/handlers/node_pool.gopkg/handlers/resource_handler.go
| return nil, errors.GeneralError("Failed to present cluster: %v", presErr) | ||
| } | ||
|
|
||
| listArgs := services.NewListArguments(r.URL.Query()) |
There was a problem hiding this comment.
rabbit already mentioned that, I agree. This code appears 5 times. It's worth making a helper function. WDYT?
| // 1. No prefix and field has no dots (top-level field) | ||
| // 2. Has prefix and field starts with it (belongs to this struct) | ||
| if prefix == "" { | ||
| if !strings.Contains(k, ".") { |
There was a problem hiding this comment.
Warning
Blocking
Category: Bug
This level-based filter introduces a false negative: ?fields=nonexistent_parent.sub passes validation silently (200 with id-only response) instead of returning 400. The parent nonexistent_parent doesn't match any struct field, so no subIn is created for it. At the bottom check, prefix == "" and the field contains a dot → it's skipped.
This also regresses SliceFilter (list endpoints) since they share this validate() function.
Two changes needed together:
- Here (line 190): At the top level, report all remaining fields — valid nested fields will have been consumed by
subInrecursion (after fix Add OWNERS file and README repository access information #2):
if prefix == "" {
fields = append(fields, k)
} else {- Line ~113-120 (struct case cleanup): Save keys before recursion so consumed entries are properly removed from
in, matching the slice case pattern at line 147:
if len(subIn) > 0 {
subKeys := make([]string, 0, len(subIn))
for k := range subIn {
subKeys = append(subKeys, k)
}
if err := validate(field, subIn, prefixedName); err != nil {
return err
}
for _, k := range subKeys {
delete(in, k)
}
}Also add a test case for this scenario:
{
name: "invalid nested field with nonexistent parent",
fields: []string{"id", "nonexistent_parent.sub"},
model: createTestCluster(),
validate: func(result map[string]interface{}, err *errors.ServiceError) {
Expect(result).To(BeNil())
Expect(err).ToNot(BeNil())
Expect(err.Type).To(Equal(errors.ErrorTypeValidation))
Expect(err.Error()).To(ContainSubstring("doesn't exist"))
},
},3 participants
Summary
Implements field filtering support for single-resource GET endpoints using the
?fields=query parameter, bringing them to parity with list endpoints.Problem (Original Issue)
The
fieldsquery parameter works correctly on list endpoints but is silently ignored on individual resource GET endpoints.Reproduction
List endpoint (works correctly):
Single resource endpoint (broken - ignores filter):
Impact
Solution
Main Implementation
Added FilterSingle() function and integrated field filtering into all single-resource GET handlers:
Bug Fixed During Implementation
While implementing, discovered and fixed a validation bug in slice_filter.go:
Changes
Core Logic
Handler Integration
Test Coverage
Testing
Acceptance Criteria ✅
Basic field filtering:
Nested fields:
Wildcard selectors:
Error handling:
Automated Tests
make verify-all
Result: ✓ All tests pass
Backward Compatibility
Checklist
Fixes: HYPERFLEET-1142