-
Notifications
You must be signed in to change notification settings - Fork 1.1k
[explorer] implement local kv api #12075
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
base: main
Are you sure you want to change the base?
Conversation
Pass a JSON binding map to the Local Explorer worker that maps binding names to actual namespace IDs, allowing correct ID resolution when binding name differs from namespace ID.
🦋 Changeset detectedLatest commit: 1dc92f9 The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
create-cloudflare
@cloudflare/kv-asset-handler
miniflare
@cloudflare/pages-shared
@cloudflare/unenv-preset
@cloudflare/vite-plugin
@cloudflare/vitest-pool-workers
@cloudflare/workers-editor-shared
@cloudflare/workers-utils
wrangler
commit: |
39c3572 to
ad0bcea
Compare
Add REST API endpoints for KV storage operations: - GET /storage/kv/namespaces - List namespaces - GET /storage/kv/namespaces/:id/keys - List keys - GET /storage/kv/namespaces/:id/values/:key - Get value - PUT /storage/kv/namespaces/:id/values/:key - Write value - DELETE /storage/kv/namespaces/:id/values/:key - Delete key - POST /storage/kv/namespaces/:id/bulk/get - Bulk get values
| const response = await fetch( | ||
| `http://${ip}:${port}/cdn-cgi/explorer/api/storage/kv/namespaces` | ||
| ); | ||
| const text = await response.text(); |
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.
Should it be JSON (and should we check the content type ?)
| const KVNamespaceSchema = z.object({ | ||
| id: z.string(), | ||
| title: z.string(), | ||
| }); | ||
|
|
||
| const KVKeySchema = z.object({ | ||
| name: z.string(), | ||
| expiration: z.number().optional(), | ||
| metadata: z.unknown().optional(), | ||
| }); |
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.
Do ns id, title, key name have max length?
| const bindingName = bindingMap[namespaceId]; | ||
| if (!bindingName) return null; | ||
|
|
||
| return env[bindingName] as KVNamespace; |
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.
Validate against the schema before casting?
| direction: z | ||
| .enum(["asc", "desc"]) | ||
| .default("asc") | ||
| .describe("Direction to order namespaces"), |
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.
| .describe("Direction to order namespaces"), | |
| .describe("Namespaces sort order"), |
| // KV Helpers | ||
| // ============================================================================ | ||
|
|
||
| // Get a KV binding by namespace ID |
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.
Proper JSDoc would not harm
| }, | ||
| }; | ||
|
|
||
| async handle(c: AppContext) { |
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.
JSDoc?
| const aVal = a[order]; | ||
| const bVal = b[order]; | ||
| const cmp = aVal.localeCompare(bVal); | ||
| return direction === "asc" ? cmp : -cmp; |
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.
| const aVal = a[order]; | |
| const bVal = b[order]; | |
| const cmp = aVal.localeCompare(bVal); | |
| return direction === "asc" ? cmp : -cmp; | |
| return (direction === "asc" ? 1 : -1) * a[order].localeCompare(b[order]) |
| })); | ||
|
|
||
| namespaces.sort( | ||
| (a: { id: string; title: string }, b: { id: string; title: string }) => { |
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.
Could the type be retrieved from the schema instead of duplicated here?
| const endIndex = startIndex + per_page; | ||
| namespaces = namespaces.slice(startIndex, endIndex); |
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.
nit: inline if not used elsewhere?
| const endIndex = startIndex + per_page; | |
| namespaces = namespaces.slice(startIndex, endIndex); | |
| namespaces = namespaces.slice(startIndex, startIndex + per_page); |
| } | ||
|
|
||
| const listResult = await kv.list({ cursor, limit }); | ||
| const resultCursor = "cursor" in listResult ? listResult.cursor ?? "" : ""; |
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.
nit: inline in the obj if onyl used once?
| // Response Helpers | ||
| // ============================================================================ | ||
|
|
||
| export function wrapResponse<T>(result: T) { |
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 would love to see more JSDoc across this PR.
We don't know what response this takes nor what it returns
| title: "Local Explorer API", | ||
| version: "1.0.0", | ||
| description: | ||
| "A local subset of Cloudflare's REST API for exploring resources during local development.", |
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.
A local subset of Cloudflare's REST API
Have we changed our mind on this ?
| "get-port": "^7.1.0", | ||
| "glob-to-regexp": "0.4.1", | ||
| "heap-js": "^2.5.0", | ||
| "hono": "^4.11.5", |
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.
Curious about why we use hono for vs its size?
Looks like the route matching we do is pretty basic and would not really require an external lib or maybe a tiny one?
By commit:
Also notes on what i have implemented in the API:
bulk is not actually used in the dash, the dash seems to individually get each value, but that seems silly to do locally so i added bulk get as well.
I have not implemented key metadata (expiration_ttl etc), supports_url_encoding (we assume it is true, might be a legacy thing?), and prefix filtering when listing keys. I also haven't checked if the error codes are correct, DEVX-2426 will followup on errors.
A picture of a cute animal (not mandatory, but encouraged)