-
-
Notifications
You must be signed in to change notification settings - Fork 88
Add list() method to KvStore interface
#500
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Add an optional list() method to the KvStore interface for enumerating entries by key prefix. This enables efficient prefix scanning which is useful for implementing features like distributed trace storage, cache invalidation by prefix, and listing related entries. - Add KvStoreListOptions interface - Add KvStoreListEntry interface - Add list() method to KvStore interface - Implement list() in MemoryKvStore Resolves fedify-dev#498 Co-Authored-By: Claude Opus 4.5 <[email protected]>
Implement the list() method for prefix scanning in SqliteKvStore. Uses SQL LIKE pattern matching on JSON-encoded keys. Co-Authored-By: Claude Opus 4.5 <[email protected]>
Co-Authored-By: Claude Opus 4.5 <[email protected]>
Co-Authored-By: Claude Opus 4.5 <[email protected]>
Co-Authored-By: Claude Opus 4.5 <[email protected]>
Co-Authored-By: Claude Opus 4.5 <[email protected]>
Co-Authored-By: Claude Opus 4.5 <[email protected]>
Co-Authored-By: Claude Opus 4.5 <[email protected]>
Change the list() method signature from list(options: KvStoreListOptions) to list(prefix?: KvKey) for a cleaner API. The KvStoreListOptions interface is removed as it only contained a single prefix field. When prefix is omitted or undefined, the method returns all entries in the store. Co-Authored-By: Claude Opus 4.5 <[email protected]>
This replaces the previous test file that was always skipped with actual working tests that run in a simulated Cloudflare Workers runtime environment using @cloudflare/vitest-pool-workers. Changes: - Add wrangler.jsonc with KV and Queue bindings for testing - Add vitest.config.ts with Workers pool configuration - Create test/env.d.ts for TypeScript type declarations - Create test/kv.test.ts with WorkersKvStore tests - Create test/mq.test.ts with WorkersMessageQueue tests - Update package.json with Vitest and Cloudflare testing dependencies - Update deno.json to remove old test configuration - Update @cloudflare/workers-types to ^4.20250906.0 in catalog Note: Tests use mock Temporal.Duration objects since Cloudflare Workers doesn't support the native Temporal API. Co-Authored-By: Claude Opus 4.5 <[email protected]>
Summary of ChangesHello @dahlia, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a powerful new Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
Co-Authored-By: Claude Opus 4.5 <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces a valuable list() method to the KvStore interface, enabling prefix-based enumeration of entries. The feature is consistently implemented across all supported backends, including Memory, SQLite, PostgreSQL, Redis, Deno KV, and Cloudflare Workers KV. A significant and commendable part of this work is the establishment of a robust testing infrastructure for Cloudflare Workers using Vitest, which replaces previously skipped tests. The documentation has also been updated thoroughly. My review focuses on improving code maintainability by reducing duplication in the new list() implementations for the Cloudflare Workers and Redis stores. Overall, this is a high-quality contribution that enhances the library's capabilities and testability.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR adds an optional list() method to the KvStore interface for enumerating entries by key prefix, addressing issue #498. The implementation includes support across all official KvStore backends and sets up proper testing infrastructure for Cloudflare Workers.
Key changes:
- Added
KvStoreListEntryinterface and optionallist()method toKvStore - Implemented
list()in all backends: MemoryKvStore, SqliteKvStore, PostgresKvStore, RedisKvStore, DenoKvStore, and WorkersKvStore - Set up Cloudflare Workers testing with Vitest and
@cloudflare/vitest-pool-workers
Reviewed changes
Copilot reviewed 22 out of 25 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
packages/fedify/src/federation/kv.ts |
Added KvStoreListEntry interface and list() method to KvStore interface; implemented in MemoryKvStore |
packages/fedify/src/federation/kv.test.ts |
Added comprehensive tests for MemoryKvStore.list() including prefix filtering, expired entries, and empty prefix |
packages/sqlite/src/kv.ts |
Implemented list() using SQL LIKE pattern matching with JSON key encoding |
packages/sqlite/src/kv.test.ts |
Added tests for SqliteKvStore.list() with various scenarios |
packages/postgres/src/kv.ts |
Implemented list() using PostgreSQL array slicing for prefix matching |
packages/postgres/src/kv.test.ts |
Added tests for PostgresKvStore.list() |
packages/redis/src/kv.ts |
Implemented list() using Redis SCAN with pattern matching and key deserialization |
packages/redis/src/kv.test.ts |
Added tests for RedisKvStore.list() |
packages/denokv/src/mod.ts |
Implemented list() delegating to Deno KV's built-in list method |
packages/denokv/src/mod.test.ts |
Added tests for DenoKvStore.list() |
packages/cfworkers/src/mod.ts |
Implemented list() using Cloudflare Workers KV list API with JSON pattern matching |
packages/cfworkers/test/kv.test.ts |
New test file for WorkersKvStore using Vitest |
packages/cfworkers/test/mq.test.ts |
New test file for WorkersMessageQueue using Vitest |
packages/cfworkers/vitest.config.ts |
Vitest configuration for Cloudflare Workers pool |
packages/cfworkers/wrangler.jsonc |
Wrangler configuration for test bindings |
packages/cfworkers/package.json |
Updated test script to use Vitest, added testing dependencies |
docs/manual/kv.md |
Updated documentation with list() method usage and implementation guide |
CHANGES.md |
Documented new list() method across all packages |
pnpm-lock.yaml, deno.lock |
Updated dependencies including @cloudflare/workers-types and Vitest packages |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
Codecov Report❌ Patch coverage is
... and 2 files with indirect coverage changes 🚀 New features to boost your workflow:
|
This test verifies that when an exact prefix key exists alongside child keys, it is yielded exactly once (not duplicated or missing). Note: Deno KV's native list() does NOT include exact prefix matches, only strictly longer keys. The explicit check in DenoKvStore.list() is necessary to include the exact prefix key in results. Co-Authored-By: Claude Opus 4.5 <[email protected]>
list() method to KvStore interface
2chanhaeng
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some of KV store integration test code looks similar. So, how about make it into a function and export it from @fedify/fedify, then import and use it for testing in each package's test code?
fedify-dev#500 (comment) Co-Authored-By: Claude <[email protected]>
fedify-dev#500 (comment) Co-Authored-By: Claude <[email protected]>
fedify-dev#500 (comment) Co-Authored-By: Claude <[email protected]>
Extract pattern and exactKey before the conditional logic, then use a unified scanning loop. fedify-dev#500 (comment) Co-Authored-By: Claude <[email protected]>
Extract pattern and exactKey before the conditional logic, then use a unified listing loop. fedify-dev#500 (comment) Co-Authored-By: Claude <[email protected]>
|
@2chanhaeng @sij411 All of your review comments are now addressed. Could you take a look again? |
|
LGTM! |
sij411
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks good to me :)
| * @since 0.5.0 | ||
| */ | ||
| export class MemoryKvStore implements KvStore { | ||
| #values: Record<string, [unknown, null | Temporal.Instant]> = {}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just curiosity) may I ask why did you use Record<...> instead of Map<K, V> (or some containers that supports hash comparison)? that would prevent you to use type conversion when loop through #values.
It would be like this:
#values = new KeyValueContainer<KvKey, [...]>(/* comparator if needed... */)and then:
async *list(prefix?: KvKey): AsyncIterable<KvStoreListEntry> {
const now = Temporal.Now.instant();
for (const [key, entry] of Object.entries(this.#values)) {
// ^^^ Now type conversion is not needed \o/There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think Map approach just hides type conversions with extra classes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean type conversions not appearing inside of that for loop, but gonna happen in another file anyway. It's just my intuitions tho.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question! The reason I used Record<string, ...> with JSON-encoded keys instead of Map<KvKey, ...> is that JavaScript's Map uses SameValueZero algorithm for key comparison, which is essentially === (strict equality) for objects. Since KvKey is an array (KvKeyComponent[]), two arrays with identical contents are still considered different objects:
const key1 = ["user", "123"];
const key2 = ["user", "123"];
console.log(key1 === key2); // false - different references
const map = new Map();
map.set(key1, "value");
map.get(key2); // undefined - key2 is treated as a different keyThis means if we used Map<KvKey, ...>, we wouldn't be able to retrieve or delete values using a different array instance with the same contents. By serializing keys to JSON strings, we get value-based equality comparison that works correctly for our use case.
Summary
This PR adds an optional
list()method to theKvStoreinterface for enumerating entries by key prefix, as proposed in #498.Closes #498.
Changes from the original proposal
The original proposal suggested:
However, during implementation, I simplified the API to:
Key differences:
Removed
KvStoreListOptionsinterface: Since the options object only contained a singleprefixproperty, passing the prefix directly as an argument is simpler and more ergonomic.Made prefix optional: When
prefixis omitted or an empty array,list()returns all entries in the store. This is useful for debugging and administrative purposes.Implementations
Implemented
list()in all officialKvStorebackends:MemoryKvStoreSqliteKvStoreLIKEquery with JSON key patternPostgresKvStorekey[1:n] = prefix)RedisKvStoreSCANwith pattern matching + key deserializationDenoKvStorelist()WorkersKvStorelist()with JSON key prefix patternBonus: Cloudflare Workers testing infrastructure
While implementing
WorkersKvStore.list(), I noticed that the existing tests in@fedify/cfworkerswere completely skipped (they checked fornavigator.userAgent === "Cloudflare-Workers").I set up proper testing using Vitest with
@cloudflare/vitest-pool-workers, which runs tests in a simulated Cloudflare Workers runtime environment. This includes:WorkersKvStoreandWorkersMessageQueueTemporal.Durationobjects (Workers runtime doesn't support Temporal)Test plan
MemoryKvStore.list()testsSqliteKvStore.list()testsPostgresKvStore.list()tests (requires PostgreSQL)RedisKvStore.list()tests (requires Redis)DenoKvStore.list()testsWorkersKvStore.list()tests