[LFXV2-1141] Fix count endpoint ignoring parent filter#34
Conversation
The count converters used criteria.ParentRef instead of criteria.Parent, causing the OpenSearch template to skip the parent_refs term query. Also fixes pre-existing lint errors (SA9003, SA1029) and bumps chart version to 0.4.12. Signed-off-by: Asitha de Silva <asithade@gmail.com>
WalkthroughThis pull request updates the chart version to 0.4.12, corrects field usage from ParentRef to Parent in converter functions, and introduces typed context keys in test files for improved type safety. An unnecessary else branch is also removed from a test. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 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 |
There was a problem hiding this comment.
Pull request overview
Fixes /query/resources/count incorrectly ignoring the parent query parameter by mapping it to the SearchCriteria.Parent field that the OpenSearch template actually consumes, plus a small chart bump and lint-driven test cleanups.
Changes:
- Fix parent filtering for count requests by setting
criteria.Parent(instead of the unusedcriteria.ParentRef) in both count criteria builders. - Bump Helm chart version
0.4.11→0.4.12. - Address lint issues in tests (typed context keys, remove empty
elsebranch).
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
cmd/service/converters.go |
Correctly maps payload.Parent to SearchCriteria.Parent for count queries so OpenSearch applies the parent filter. |
charts/lfx-v2-query-service/Chart.yaml |
Chart version bump to reflect the fix release. |
pkg/paging/codec_test.go |
Replaces string context keys with a typed key to satisfy staticcheck. |
cmd/service/error_test.go |
Same typed context key adjustment for staticcheck. |
internal/service/resource_search_test.go |
Removes an empty else branch to satisfy staticcheck without changing behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| criteria.ResourceType = payload.Type | ||
| } | ||
| if payload.Parent != nil { | ||
| criteria.ParentRef = payload.Parent | ||
| criteria.Parent = payload.Parent | ||
| } |
There was a problem hiding this comment.
The bug fix here (mapping payload.Parent to criteria.Parent) isn’t currently guarded by a test that would have failed before this change. The existing QueryResourcesCount tests cover a parent payload but the mocks return a fixed response and don’t assert on the criteria/query, so a regression back to ParentRef would still pass. Consider adding an assertion (e.g., by having the mock searcher record the received criteria or by rendering the OpenSearch query and checking it includes the parent_refs term) so the parent filter behavior is verified end-to-end for /query/resources/count.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cmd/service/converters.go (1)
190-251: Consider extracting common filter/date parsing logic.Both
payloadToCountPublicCriteriaandpayloadToCountAggregationCriteriashare significant duplicate code for parsing filters, setting common criteria fields (Tags, TagsAll, Name, Type, Parent), and handling date filtering. Consider extracting this into a shared helper to reduce duplication.♻️ Example helper extraction
// applyCountPayloadToCriteria sets common fields from QueryResourcesCountPayload to criteria func applyCountPayloadToCriteria(criteria *model.SearchCriteria, payload *querysvc.QueryResourcesCountPayload) error { filters, err := parseFilters(payload.Filters) if err != nil { return fmt.Errorf("invalid filters: %w", err) } criteria.Tags = payload.Tags criteria.TagsAll = payload.TagsAll criteria.Filters = filters criteria.Name = payload.Name criteria.ResourceType = payload.Type criteria.Parent = payload.Parent // Date filtering validation and parsing... // (extract the date handling logic here) return nil }Then both functions would call this helper after setting their specific fields (
PublicOnly/PrivateOnly,PageSize,GroupBy).Also applies to: 253-317
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/service/converters.go` around lines 190 - 251, payloadToCountPublicCriteria and payloadToCountAggregationCriteria duplicate filter/date parsing and common field assignments; extract that shared logic into a helper (e.g., applyCountPayloadToCriteria(criteria *model.SearchCriteria, payload *querysvc.QueryResourcesCountPayload) error) that runs parseFilters, assigns Tags/TagsAll/Name/ResourceType/Parent, validates date_field presence when DateFrom/DateTo are used, prefixes DateField with "data.", and calls parseDateFilter for DateFrom/DateTo, returning any formatted errors; then simplify both payloadToCountPublicCriteria and payloadToCountAggregationCriteria to set their specific fields (PublicOnly/PrivateOnly, PageSize, GroupBy/GroupBySize) and call the new helper to populate the rest.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@cmd/service/converters.go`:
- Around line 190-251: payloadToCountPublicCriteria and
payloadToCountAggregationCriteria duplicate filter/date parsing and common field
assignments; extract that shared logic into a helper (e.g.,
applyCountPayloadToCriteria(criteria *model.SearchCriteria, payload
*querysvc.QueryResourcesCountPayload) error) that runs parseFilters, assigns
Tags/TagsAll/Name/ResourceType/Parent, validates date_field presence when
DateFrom/DateTo are used, prefixes DateField with "data.", and calls
parseDateFilter for DateFrom/DateTo, returning any formatted errors; then
simplify both payloadToCountPublicCriteria and payloadToCountAggregationCriteria
to set their specific fields (PublicOnly/PrivateOnly, PageSize,
GroupBy/GroupBySize) and call the new helper to populate the rest.
Summary
/query/resources/count) ignoring theparentquery parameter — bothpayloadToCountPublicCriteriaandpayloadToCountAggregationCriteriawere settingcriteria.ParentRefinstead ofcriteria.Parent, which the OpenSearch template never reads0.4.11→0.4.12JIRA
LFXV2-1141