Add support for filtering when listing objects in store.Store#56
Open
gonzolino wants to merge 4 commits into
Open
Add support for filtering when listing objects in store.Store#56gonzolino wants to merge 4 commits into
store.Store#56gonzolino wants to merge 4 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (6)
📝 WalkthroughWalkthroughThis PR adds variadic list options, label and field selector primitives, host-store field indexing, event wiring updates, and tests covering label, field, and combined selector filtering. ChangesStore Selector Filtering
Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
storeutils/host/store_test.go (1)
72-104: ⚡ Quick winExact
HaveLenassertions rely on shared store state.
Listwithout a filter usesBeNumerically(">=", 3)to tolerate objects created by other specs, but the filtered assertions (HaveLen(2/1/3)andBeEmpty()) assume no other spec ever creates objects carryingapp/envlabels. This holds today but is fragile under spec randomization or future tests. Consider addingDeferCleanupto delete the created objects, or scope this test to a dedicated store instance to keep counts deterministic.🤖 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 `@storeutils/host/store_test.go` around lines 72 - 104, The test relies on exact counts against shared dummyStore state (calls to dummyStore.List with store.MatchingLabels and store.HasLabels), which makes HaveLen assertions fragile; modify the test to either create and use a dedicated store instance for this spec or ensure created objects are removed after the test using t.DeferCleanup (or Ginkgo’s DeferCleanup) to delete the specific objects you inserted, so that filtered assertions (Expect(filtered).To(HaveLen(...)) and Expect(filtered).To(BeEmpty())) are deterministic; update the setup/teardown around dummyStore, the object creation helpers, and the List calls accordingly to scope state cleanup.
🤖 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 `@storeutils/host/store_test.go`:
- Around line 72-104: The test relies on exact counts against shared dummyStore
state (calls to dummyStore.List with store.MatchingLabels and store.HasLabels),
which makes HaveLen assertions fragile; modify the test to either create and use
a dedicated store instance for this spec or ensure created objects are removed
after the test using t.DeferCleanup (or Ginkgo’s DeferCleanup) to delete the
specific objects you inserted, so that filtered assertions
(Expect(filtered).To(HaveLen(...)) and Expect(filtered).To(BeEmpty())) are
deterministic; update the setup/teardown around dummyStore, the object creation
helpers, and the List calls accordingly to scope state cleanup.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 296e2d3d-9e56-444b-ac8d-061bf204e07e
📒 Files selected for processing (4)
storeutils/host/store.gostoreutils/host/store_test.gostoreutils/store/listoptions.gostoreutils/store/store.go
gonzolino
force-pushed
the
enh/extend-store
branch
2 times, most recently
from
9cad0d5 to
72c44ab
Compare
adracus
previously approved these changes
Jun 3, 2026
gonzolino
force-pushed
the
enh/extend-store
branch
2 times, most recently
from
088269a to
b7e41a8
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
storeutils/store/listoptions.go (1)
30-37: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winGuard
labels.NewRequirementerrors inHasLabels.ApplyToList
labels.NewRequirement(key, selection.Exists, nil)can returnnil, errorfor malformed keys, and*reqwill panic when that happens. Handle the error instead of dereferencing unconditionally.🤖 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 `@storeutils/store/listoptions.go` around lines 30 - 37, `HasLabels.ApplyToList` currently ignores the error from `labels.NewRequirement` and blindly dereferences the returned requirement, which can panic on malformed keys. Update the loop in `HasLabels.ApplyToList` to handle the `labels.NewRequirement` error explicitly before calling `sel.Add`, and only append valid requirements to the selector; keep `andSelectors` and `ListOptions.LabelSelector` behavior unchanged for valid labels.
🤖 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 `@storeutils/store/listoptions.go`:
- Around line 47-51: The MatchingFields.ApplyToList method currently overwrites
any existing field selector on ListOptions, which discards earlier filters
applied by other options. Update ApplyToList to merge the new selector with
o.FieldSelector using fields.AndSelectors, matching the composition behavior
used by MatchingLabels and HasLabels, so existing selectors are preserved
instead of replaced.
---
Outside diff comments:
In `@storeutils/store/listoptions.go`:
- Around line 30-37: `HasLabels.ApplyToList` currently ignores the error from
`labels.NewRequirement` and blindly dereferences the returned requirement, which
can panic on malformed keys. Update the loop in `HasLabels.ApplyToList` to
handle the `labels.NewRequirement` error explicitly before calling `sel.Add`,
and only append valid requirements to the selector; keep `andSelectors` and
`ListOptions.LabelSelector` behavior unchanged for valid labels.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b23350ed-c705-47ac-97ed-5b7159b564ff
📒 Files selected for processing (4)
storeutils/host/host_suite_test.gostoreutils/host/store.gostoreutils/host/store_test.gostoreutils/store/listoptions.go
🚧 Files skipped from review as they are similar to previous changes (1)
- storeutils/host/store.go
gonzolino
force-pushed
the
enh/extend-store
branch
3 times, most recently
from
9059c5b to
d9bacb1
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
storeutils/store/store.go (1)
47-56: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider splitting
IndexFieldinto a separate interface.Bundling
IndexFieldintoStore[E]forces every implementation (including future ones like the ceph-provider store mentioned in the linked issue) to support field indexing even if they only need label-based filtering. Mirroring controller-runtime's separation ofclient.Readerandclient.FieldIndexerwould keep the coreStorecontract minimal and let implementations opt into field indexing.♻️ Possible interface split
type Store[E api.Object] interface { Create(ctx context.Context, obj E) (E, error) Get(ctx context.Context, id string) (E, error) Update(ctx context.Context, obj E) (E, error) Delete(ctx context.Context, id string) error List(ctx context.Context, opts ...ListOption) ([]E, error) Watch(ctx context.Context) (Watch[E], error) +} - IndexField(field string, fn FieldIndexerFunc[E]) +type FieldIndexer[E api.Object] interface { + IndexField(field string, fn FieldIndexerFunc[E]) }🤖 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 `@storeutils/store/store.go` around lines 47 - 56, The Store[E] contract currently includes IndexField, which forces every implementation to support field indexing even when it is unnecessary. Split this out of Store[E] in storeutils/store/store.go by moving IndexField(field string, fn FieldIndexerFunc[E]) into a separate opt-in interface, and update the affected Store-related types/usages to depend only on the minimal Store methods unless field indexing is explicitly required. Use the Store[E] interface and IndexField method as the key symbols when making the change.
🤖 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 `@storeutils/host/store.go`:
- Line 70: The Store indexers map is accessed concurrently without
synchronization, since IndexField mutates indexers while List iterates over it.
Add the same mutex protection used for watches around all indexers reads and
writes in the Store methods, and update IndexField and List to lock/unlock
consistently when touching indexers.
- Around line 205-214: The Store.List field-selector check only rejects
selectors when no indexers exist, so invalid selectors on partially indexed
stores still fall through and behave like no-op filters. Update Store.List to
validate the requested FieldSelector against the registered indexers before
calling Matches(merged), and return a clear error when the selector references
any unindexed field; use the existing List and FieldSelector handling in
Store[E] to keep the validation alongside the current list option merging.
---
Nitpick comments:
In `@storeutils/store/store.go`:
- Around line 47-56: The Store[E] contract currently includes IndexField, which
forces every implementation to support field indexing even when it is
unnecessary. Split this out of Store[E] in storeutils/store/store.go by moving
IndexField(field string, fn FieldIndexerFunc[E]) into a separate opt-in
interface, and update the affected Store-related types/usages to depend only on
the minimal Store methods unless field indexing is explicitly required. Use the
Store[E] interface and IndexField method as the key symbols when making the
change.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f95517af-8cd5-4096-b7ae-ed15a72555ec
📒 Files selected for processing (6)
eventutils/event/event.gostoreutils/host/host_suite_test.gostoreutils/host/store.gostoreutils/host/store_test.gostoreutils/store/listoptions.gostoreutils/store/store.go
🚧 Files skipped from review as they are similar to previous changes (4)
- eventutils/event/event.go
- storeutils/host/host_suite_test.go
- storeutils/host/store_test.go
- storeutils/store/listoptions.go
gonzolino
force-pushed
the
enh/extend-store
branch
2 times, most recently
from
a036686 to
3062587
Compare
Contributor
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
gonzolino
dismissed
adracus’s stale review
Included additional feature in this PR (MatchingFields filters) that invalidate the former review.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
storeutils/store/store.go (1)
45-59: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider doc comments for new exported API.
IndexerFunc, the updatedStore.Listsignature, andFieldIndexerare all new/changed public API surface in a shared library module, but none carry GoDoc comments explaining semantics (e.g., whatIndexerFuncshould return, thatListwith no options returns everything, or thatIndexFieldmust be called before referencing a field in aFieldSelector). This last point is a hard runtime contract enforced instoreutils/host/store.go(field selector references unindexed field %qerror) that isn't otherwise documented at the interface level.📝 Suggested doc comments
+// IndexerFunc extracts an indexable value from an object for a given field. type IndexerFunc[E api.Object] func(E) string type Store[E api.Object] interface { Create(ctx context.Context, obj E) (E, error) Get(ctx context.Context, id string) (E, error) Update(ctx context.Context, obj E) (E, error) Delete(ctx context.Context, id string) error + // List returns objects matching all provided ListOptions. Implementations + // may return an error if a FieldSelector references a field that has not + // been registered via FieldIndexer.IndexField. List(ctx context.Context, opts ...ListOption) ([]E, error) Watch(ctx context.Context) (Watch[E], error) } +// FieldIndexer allows registering additional indexed fields that can later +// be used in ListOptions.FieldSelector. type FieldIndexer[E api.Object] interface { IndexField(field string, fn IndexerFunc[E]) }🤖 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 `@storeutils/store/store.go` around lines 45 - 59, Add GoDoc comments for the exported API in Store and related types: document IndexerFunc, Store.List, and FieldIndexer so callers understand their behavior and contracts. In particular, explain what IndexerFunc should return for a field value, that List with no options returns all objects, and that IndexField must be registered before any FieldSelector can reference that field. Use the symbols IndexerFunc, Store.List, and FieldIndexer/IndexField to place the comments near the interface definitions.
🤖 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 `@storeutils/store/store.go`:
- Around line 45-59: Add GoDoc comments for the exported API in Store and
related types: document IndexerFunc, Store.List, and FieldIndexer so callers
understand their behavior and contracts. In particular, explain what IndexerFunc
should return for a field value, that List with no options returns all objects,
and that IndexField must be registered before any FieldSelector can reference
that field. Use the symbols IndexerFunc, Store.List, and FieldIndexer/IndexField
to place the comments near the interface definitions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 349d2c8e-c597-44c4-9106-75f2268d3128
📒 Files selected for processing (6)
eventutils/event/event.gostoreutils/host/host_suite_test.gostoreutils/host/store.gostoreutils/host/store_test.gostoreutils/store/listoptions.gostoreutils/store/store.go
🚧 Files skipped from review as they are similar to previous changes (5)
- storeutils/host/store_test.go
- eventutils/event/event.go
- storeutils/store/listoptions.go
- storeutils/host/host_suite_test.go
- storeutils/host/store.go
gonzolino
force-pushed
the
enh/extend-store
branch
from
3062587 to
0ab4eff
Compare
Contributor
Author
|
@coderabbitai resume |
✅ Action performedReviews resumed. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
storeutils/host/store.go (1)
236-249: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueMinor: recomputes all indexers per object regardless of selector fields.
mergedis built by invoking every registeredIndexerFuncper object even when theFieldSelectoronly references a subset of fields. With few indexers this is negligible, but for stores with many field indexers this adds unnecessary per-object work on everyListcall.♻️ Optional: only compute fields referenced by the selector
if listOpts.FieldSelector != nil { merged := fields.Set{} - for field, fn := range s.indexers { - merged[field] = fn(object) + for _, req := range listOpts.FieldSelector.Requirements() { + if fn, ok := s.indexers[req.Field]; ok { + merged[req.Field] = fn(object) + } } if !listOpts.FieldSelector.Matches(merged) { continue } }🤖 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 `@storeutils/host/store.go` around lines 236 - 249, In storeutils/host/store.go, the List path in the field-selector check builds merged by calling every indexer for every object even when only a few fields are actually queried. Update the List logic around the FieldSelector handling to compute only the fields referenced by listOpts.FieldSelector instead of iterating all entries in s.indexers, while preserving the existing matches behavior for object.GetLabels() and field matching.
🤖 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 `@storeutils/host/store.go`:
- Around line 236-249: In storeutils/host/store.go, the List path in the
field-selector check builds merged by calling every indexer for every object
even when only a few fields are actually queried. Update the List logic around
the FieldSelector handling to compute only the fields referenced by
listOpts.FieldSelector instead of iterating all entries in s.indexers, while
preserving the existing matches behavior for object.GetLabels() and field
matching.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 549995bc-00f2-47db-b70b-b9d0d11fd513
📒 Files selected for processing (6)
eventutils/event/event.gostoreutils/host/host_suite_test.gostoreutils/host/store.gostoreutils/host/store_test.gostoreutils/store/listoptions.gostoreutils/store/store.go
🚧 Files skipped from review as they are similar to previous changes (4)
- storeutils/host/store_test.go
- eventutils/event/event.go
- storeutils/store/listoptions.go
- storeutils/host/host_suite_test.go
Allow filtering results in the `store.Store` `List(ctx)` method by adding a new `ListOption` parameter. Signed-off-by: Daniel Gonzalez Nothnagel <daniel.gonzalez.nothnagel@sap.com>
These ListOptions allow to filter objects based on their labels. Signed-off-by: Daniel Gonzalez Nothnagel <daniel.gonzalez.nothnagel@sap.com>
This ListOptions allows to filter objects based on their fields. Signed-off-by: Daniel Gonzalez Nothnagel <daniel.gonzalez.nothnagel@sap.com>
Signed-off-by: Daniel Gonzalez Nothnagel <daniel.gonzalez.nothnagel@sap.com>
gonzolino
force-pushed
the
enh/extend-store
branch
from
0ab4eff to
1e4b523
Compare
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.
Summary
Extend the
Listmethod of thestore.Storeinterface to allow filtering ofobjects. This allows for native filtering, instead of requiring clients to list
all objects in the store and then filter on the cleint side.
Also extend
host.Storefor the new interface.Proposed Changes
store.StoreHasLabels,MatchingLabelsandMatchingFieldsListOptionshost.Storeto support ListOptionsFixes #55
Summary by CodeRabbit
Summary