HYPERFLEET-705 - feat: Add slice field validation in SliceFilter#78
Conversation
WalkthroughThe PR refactors slice filtering in the presenters package to propagate explicit string prefixes instead of a regex-based approach, adding prefix-aware validation and mapping for nested structs, slices, pointers, and time.Time fields. It implements star-selector behavior for slice requests, preserves empty-slice responses when requested, and adjusts removal/cleanup logic for prefixed keys. Tests were expanded: new helper for an empty-conditions cluster list, clusters now include Spec.region and Status.Conditions (with messages and timestamps), and many new TestSliceFilter cases covering nested sub-fields, time sub-fields, invalid sub-fields, star selectors, whole-slice requests, and empty-slice handling. Sequence Diagram(s)(Skipped — changes are internal library logic without multi-component sequential interactions that meet generation criteria.) Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/api/presenters/slice_filter.go`:
- Around line 236-263: The slice branch currently only sets res[name] when
s.Len() > 0; change it to detect whether the slice was explicitly requested
before the length check (by checking in[nextPrefix] or starSelectorKey presence)
and, if requested but s.Len() == 0, emit an empty slice (e.g., res[name] =
[]interface{}{}) so “requested but empty” is preserved; keep the existing
starSelectorKey handling (addedStarSelector and delete) and the use of
structToMap when len>0 unchanged, but move the request-detection earlier (using
nextPrefix and starSelectorKey) so you can conditionally assign an empty result
when the slice is requested yet has zero length.
- Around line 133-145: The time-field handling inside the slice element branch
needs to honor the current prefix the same way the default branch does: when
looking up or deleting timestamp keys (e.g., created_time, last_transition_time)
use both the bare key and the prefixed variant (prefixedName + "." + key) so
prefixed selectors like "status.conditions.last_transition_time" are recognized
and removed from the input map; update the logic in the validate recursion block
(variables: subIn, subKeys, elemType, elemValue, prefixedName) to
check/lookup/delete using both forms, and make the same change in the other
identical block around the later range (lines referenced in comment) so
structToMap and validate see and consume prefixed timestamp fields consistently.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 37ae2cef-e208-4142-879b-b848bac94eb2
📒 Files selected for processing (2)
pkg/api/presenters/presenter_test.gopkg/api/presenters/slice_filter.go
Mischulee
force-pushed
the
HYPERFLEET-705
branch
from
08a113d to
47914ff
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/api/presenters/slice_filter.go`:
- Around line 251-255: The current logic builds requested using in[nextPrefix]
or in[starSelectorKey], which misses nested selectors like
"status.conditions.type" and causes empty slices to be dropped; update the
requested determination in the block around starSelectorKey/nextPrefix so it
also treats any selector that starts with nextPrefix + "." as a request (i.e.,
set requested = true if any key in the selector map has strings.HasPrefix(key,
nextPrefix + ".") ), then keep the existing s.Len() == 0 branch that sets
res[name] = []interface{}{} when requested; this change references
starSelectorKey, nextPrefix, requested, s.Len(), and res[name].
- Around line 258-266: The loop in slice handling calls structToMap for every
element which panics for non-struct element kinds; modify the loop in the
function using variables s, nextPrefix, in, starSelectorKey and structToMap so
that before calling structToMap you check the element's Kind (e.g., using
s.Index(i).Kind() or Type) and if it's a struct or pointer-to-struct call
structToMap as before, otherwise append the raw element value (or its
zero-value-safe representation) to result without recursing; keep the
addedStarSelector logic unchanged and ensure pointer elements are
dereferenced/handled safely when deciding whether to call structToMap.
- Around line 102-104: The code constructs prefixedName (e.g., prefixedName :=
name + ".") and then calls removeStar(in, prefixedName) but does not escape dots
in the prefix, so dots are treated as regex wildcards; update the three call
sites (where prefixedName is built and passed to removeStar) to escape regex
metacharacters in the prefix (specifically replace/escape '.' and other regex
metacharacters) before calling removeStar (or add an escaping step inside
removeStar) so the pattern uses literal dots when building the regex; reference
symbols: prefixedName, removeStar, and the three call sites around lines that
build prefixedName and call removeStar.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3e8dfab1-d15b-40e1-a458-1a2c18fc7eb0
📒 Files selected for processing (2)
pkg/api/presenters/presenter_test.gopkg/api/presenters/slice_filter.go
Mischulee
force-pushed
the
HYPERFLEET-705
branch
from
47914ff to
2800cc6
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
pkg/api/presenters/slice_filter.go (2)
282-284: Potential inconsistency: breaking on first empty element may silently drop requested slices.When
structToMapreturns an empty map for the first element, the loop breaks and the entire slice may be omitted from the result (sincelen(result) == 0at line 290). This could happen if:
- The slice element struct has no exported json-tagged fields
- All requested sub-fields are nil/zero for the first element
While this is likely a performance optimization assuming uniform element structure, it could cause confusion if a requested slice silently disappears. Consider whether this behavior is intentional or if you'd prefer to continue iterating to collect non-empty elements.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/api/presenters/slice_filter.go` around lines 282 - 284, The loop currently breaks when structToMap returns an empty map for the first slice element (the `if len(slice) == 0 { break }` branch), which can cause the entire slice to be omitted; change this logic in the function that builds slices (the loop using structToMap and `slice`) to skip empty elements instead of breaking (use continue-like behavior), so subsequent elements are still processed and non-empty maps are retained in the `result` collection; ensure you preserve any early-exit conditions only when appropriate and update any comment/docstring near the loop to reflect that empty elements are skipped rather than terminating the entire slice assembly.
128-148: Slice element validation handles struct types well, but may panic on[]time.Timeslices.When
elemType.Kind() == reflect.Structis true, the code creates a zero-value instance and callsvalidaterecursively. This works for most structs, buttime.Timeis a struct that receives special treatment elsewhere. If a field is[]time.Time, this would validate it as a regular struct (which has no exported json-tagged fields), leading to potentially confusing behavior.Consider adding an explicit check to exclude
time.Time:- if elemType.Kind() == reflect.Struct { + if elemType.Kind() == reflect.Struct && elemType != reflect.TypeOf(time.Time{}) {However, if
[]time.Timefields don't exist in your API models, this is a minor edge case.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/api/presenters/slice_filter.go` around lines 128 - 148, The struct-handling branch in slice element validation (where elemType.Kind() == reflect.Struct) should explicitly exclude time.Time to avoid recursing into time values (e.g., []time.Time); add a check comparing elemType to reflect.TypeOf(time.Time{}) (or otherwise detect time.Time) before creating elemValue and calling validate, and skip the recursive validate/delete logic for that type so time.Time slices are treated as primitives rather than regular structs; adjust the logic around prefixedName, sliceFieldPrefix, elemValue and validate accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/api/presenters/slice_filter.go`:
- Around line 282-284: The loop currently breaks when structToMap returns an
empty map for the first slice element (the `if len(slice) == 0 { break }`
branch), which can cause the entire slice to be omitted; change this logic in
the function that builds slices (the loop using structToMap and `slice`) to skip
empty elements instead of breaking (use continue-like behavior), so subsequent
elements are still processed and non-empty maps are retained in the `result`
collection; ensure you preserve any early-exit conditions only when appropriate
and update any comment/docstring near the loop to reflect that empty elements
are skipped rather than terminating the entire slice assembly.
- Around line 128-148: The struct-handling branch in slice element validation
(where elemType.Kind() == reflect.Struct) should explicitly exclude time.Time to
avoid recursing into time values (e.g., []time.Time); add a check comparing
elemType to reflect.TypeOf(time.Time{}) (or otherwise detect time.Time) before
creating elemValue and calling validate, and skip the recursive validate/delete
logic for that type so time.Time slices are treated as primitives rather than
regular structs; adjust the logic around prefixedName, sliceFieldPrefix,
elemValue and validate accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 19985c88-1fda-4dd8-9066-7079788b0519
📒 Files selected for processing (2)
pkg/api/presenters/presenter_test.gopkg/api/presenters/slice_filter.go
Mischulee
force-pushed
the
HYPERFLEET-705
branch
from
2800cc6 to
4ca2f89
Compare
Mischulee
force-pushed
the
HYPERFLEET-705
branch
from
4ca2f89 to
6a38db6
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
pkg/api/presenters/slice_filter.go (2)
285-292: Inconsistent behavior between empty slice and all-filtered-out slice.When the source slice is empty (s.Len() == 0) and requested, an empty array is returned (lines 254-257). However, when all elements produce empty maps after filtering (len(slice) == 0 at line 285), those elements are skipped, and if
len(result) == 0at line 290, the field is omitted entirely.This means:
- Empty source slice →
"conditions": []- All elements filtered to empty → field omitted
Is this intentional? If consistency is desired, consider emitting an empty array when
requested && len(result) == 0:♻️ Suggested fix for consistency
if len(result) > 0 { res[name] = result + } else if requested { + res[name] = []interface{}{} }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/api/presenters/slice_filter.go` around lines 285 - 292, Currently the code skips appending elements whose filtered map is empty (len(slice) == 0) causing a field to be omitted when all elements are filtered out, whereas an originally-empty source slice (s.Len() == 0) returns an empty array; to make behavior consistent, after building result check if requested is true and len(result) == 0 and then set res[name] to an empty slice (e.g. empty []interface{} or the same element type used elsewhere) instead of omitting the key; update the post-processing block where result is assigned to res[name] (the lines handling result and res[name]) to assign an explicit empty array when requested && len(result) == 0.
214-226: Minor edge case whenprefixis empty.When
prefixis empty (root level),prefixedStarbecomes".*", which is semantically incorrect for root-level time fields. In practice, no valid selector would be".*"at the root, so this won't cause issues, but consider guarding this check:🧹 Optional refinement
if t.Type == reflect.TypeOf(&time.Time{}) || t.Type == reflect.TypeOf(time.Time{}) { prefixedName := name if prefix != "" { prefixedName = fmt.Sprintf("%s.%s", prefix, name) } - prefixedStar := fmt.Sprintf("%s.*", prefix) - if _, ok := in[prefixedName]; ok || in[prefixedStar] { + matchesStar := prefix != "" && in[prefix+".*"] + if _, ok := in[prefixedName]; ok || matchesStar {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/api/presenters/slice_filter.go` around lines 214 - 226, The check that builds prefixedStar uses fmt.Sprintf("%s.*", prefix) so when prefix is empty it produces ".*" which is incorrect for root-level selectors; update the logic in the Time handling block (variables: prefix, prefixedName, prefixedStar, in, name) to set prefixedStar to "*" when prefix == "" (or otherwise only check prefixedStar when prefix != ""), then keep the existing membership check (if _, ok := in[prefixedName]; ok || in[prefixedStar]) so root-level wildcard selectors use "*" instead of ".*".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/api/presenters/slice_filter.go`:
- Around line 285-292: Currently the code skips appending elements whose
filtered map is empty (len(slice) == 0) causing a field to be omitted when all
elements are filtered out, whereas an originally-empty source slice (s.Len() ==
0) returns an empty array; to make behavior consistent, after building result
check if requested is true and len(result) == 0 and then set res[name] to an
empty slice (e.g. empty []interface{} or the same element type used elsewhere)
instead of omitting the key; update the post-processing block where result is
assigned to res[name] (the lines handling result and res[name]) to assign an
explicit empty array when requested && len(result) == 0.
- Around line 214-226: The check that builds prefixedStar uses
fmt.Sprintf("%s.*", prefix) so when prefix is empty it produces ".*" which is
incorrect for root-level selectors; update the logic in the Time handling block
(variables: prefix, prefixedName, prefixedStar, in, name) to set prefixedStar to
"*" when prefix == "" (or otherwise only check prefixedStar when prefix != ""),
then keep the existing membership check (if _, ok := in[prefixedName]; ok ||
in[prefixedStar]) so root-level wildcard selectors use "*" instead of ".*".
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 50ffa875-2f0b-4076-a16e-58fde72bd9bc
📒 Files selected for processing (2)
pkg/api/presenters/presenter_test.gopkg/api/presenters/slice_filter.go
| elem0 := conditions[0].(map[string]interface{}) | ||
| Expect(elem0["type"]).To(Equal("Ready")) | ||
| Expect(elem0).ToNot(HaveKey("message")) | ||
|
|
There was a problem hiding this comment.
Really nice work on this whole PR by the way — replacing that TODO stub with real slice validation is a big improvement, and the break → continue fix is a genuine bug catch. Well done!
One thing that would strengthen the test suite: try adding a test case that selects multiple sub-fields from the same slice element, something like fields: ["id", "status.conditions.type", "status.conditions.status"]. Right now the "valid sub-field" test only picks one sub-field, but in practice users will often want several. That would exercise the code path where subIn has multiple entries (around line 130-134 of slice_filter.go) and make sure they all come through correctly in the output.
It's a common testing pattern to think about: "what if someone does this with two of something instead of one?" — it catches a surprising number of bugs around map merging and iteration.
| conditions := status["conditions"].([]interface{}) | ||
| elem := conditions[0].(map[string]interface{}) | ||
| Expect(elem).To(HaveKey("last_transition_time")) | ||
| Expect(elem).ToNot(HaveKey("type")) |
There was a problem hiding this comment.
Good test! One thing to consider — the existing time field test (the "time field handling" case higher up) goes further and actually parses the value with time.Parse(time.RFC3339, ...) to verify it's a valid timestamp string, not just that the key exists. It'd be great to do the same here, something like:
ltt, ok := elem["last_transition_time"].(string)
Expect(ok).To(BeTrue())
_, parseErr := time.Parse(time.RFC3339, ltt)
Expect(parseErr).To(BeNil())The reason this matters: HaveKey tells you the field is present, but it doesn't tell you the value was serialized correctly. Since the time formatting happens in structToMap and this is a new code path (time fields inside slice elements), it's worth verifying the value too. A good general rule of thumb — if you're testing a transformation, check at least one value, not just the keys.
| nextPrefix = prefix + "." + name | ||
| } | ||
| starSelectorKey := nextPrefix + ".*" | ||
| requested := in[nextPrefix] || in[starSelectorKey] |
There was a problem hiding this comment.
Nice catch handling this — something to be aware of for the future though: the requested check here looks at in[nextPrefix], in[starSelectorKey], and sub-prefix matches, but it doesn't check for a parent star selector. So if a user requests status.*, scalar and time fields under status will be included (the default and time cases both check prefix + ".*"), but this slice case won't pick it up, so conditions would be missing from the output.
This isn't something you introduced — it was already the behavior before your PR — so totally fine to leave for now. But if you wanted to fix it in a follow-up, you'd just add:
parentStar := prefix + ".*"
requested := in[nextPrefix] || in[starSelectorKey] || in[parentStar]Just flagging so you're aware of the edge case!
| for k := range subIn { | ||
| subKeys = append(subKeys, k) | ||
| } | ||
| elemValue := reflect.New(elemType).Elem().Interface() |
There was a problem hiding this comment.
Something to be aware of with this approach — reflect.New(elemType).Elem().Interface() creates a zero-value struct, and when that struct has nil pointer fields (like Message *string in ResourceCondition), calling ttype.Elem().Kind() on the nil pointer gives reflect.Invalid rather than panicking. It works correctly because the default case in the switch handles it with delete(in, prefixedName).
A small comment here like // Note: nil pointer fields in zero-value structs produce reflect.Invalid Kind, handled by default case would help the next person reading this understand why it doesn't panic. Go's reflect package has a lot of these subtle behaviors that are easy to trip over.
| @@ -146,19 +173,12 @@ func validate(model interface{}, in map[string]bool, prefix string) *errors.Serv | |||
| } | |||
|
|
|||
| func removeStar(in map[string]bool, name string) map[string]bool { | |||
There was a problem hiding this comment.
Now that this uses strings.HasPrefix instead of regex, the name removeStar is slightly misleading — it's doing prefix-based key deletion, not star-pattern removal. Something like removeByPrefix or removeSubKeys would be clearer about what it actually does. A good habit: function names should tell the reader what it does without needing to read the body.
| Expect(elem0).To(HaveKey("message")) | ||
|
|
||
| elem1 := conditions[1].(map[string]interface{}) | ||
| Expect(elem1["type"]).To(Equal("Progressing")) |
There was a problem hiding this comment.
For the star selector test, it's worth checking that all fields come back, not just a few. ResourceCondition has 8 fields (type, status, message, reason, created_time, last_transition_time, last_updated_time, observed_generation), so an Expect(elem0).To(HaveLen(8)) assertion would catch cases where the star selector accidentally drops fields. The "requesting whole slice" test just below this one has near-identical assertions — differentiating them (this one checks completeness, that one checks the bare-to-star promotion) would make both tests more useful.
| prefixedName = fmt.Sprintf("%s.%s", prefix, name) | ||
| } | ||
| prefixedStar := fmt.Sprintf("%s.*", prefix) | ||
| if _, ok := in[prefixedName]; ok || in[prefixedStar] { |
There was a problem hiding this comment.
Tiny consistency thing — this line mixes two map lookup styles:
if _, ok := in[prefixedName]; ok || in[prefixedStar] {The first uses the comma-ok pattern, the second reads the bool value directly. Both work fine for map[string]bool, but mixing them in the same expression is a bit jarring. Using the same style for both is easier to scan:
if in[prefixedName] || in[prefixedStar] {Since the map values are always true, the direct access form is safe and more readable here.
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: rafabene The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-merge-bot
bot
merged commit 5a5b492
into
openshift-hyperfleet:main
3 participants
https://issues.redhat.com/browse/HYPERFLEET-705
Summary by CodeRabbit
Bug Fixes
Tests