Implement SQLiteKvStore #318
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @nyeong, 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 new SQLite-based key-value store implementation, SqliteKvStore, designed to integrate with the existing KvStore interface. This initial version focuses on Node.js compatibility, laying the groundwork for a robust and familiar storage option that can be used across various runtimes, aiming to modernize tutorials and enhance production readiness.
Highlights
- New
SqliteKvStoreImplementation: A newSqliteKvStoreclass has been added insqlite/kv.ts, fully implementing theKvStoreinterface forget,set,delete, andcasoperations. This provides a SQLite-backed key-value store solution. - Node.js Compatibility and Design Choices: The initial implementation is designed for Node.js v22+ utilizing the
node:sqlitemodule. Key design decisions include atomic CAS operations via transactions, TTL management usingTemporal.epochMilliseconds, and JSON encoding for keys and values to handle various data types. - New
sqlitePackage Structure: A dedicated@fedify/sqlitepackage has been introduced, complete with its ownREADME.md,deno.json,package.json, andtsdown.config.tsfor build configuration. This package is integrated into the main project'sdeno.json,fedify/deno.json,pnpm-lock.yaml, andpnpm-workspace.yaml. - Comprehensive Test Suite: A new test file,
sqlite/kv.test.ts, has been added to thoroughly test theSqliteKvStore's functionalities, covering initialization, basic CRUD operations, compare-and-swap, and handling of complex data types like objects, arrays, and falsy values.
Using Gemini Code Assist
The 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 in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and 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 to provide feedback.
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
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
github-actions
bot
added
component/kv
There was a problem hiding this comment.
Code Review
This pull request introduces a new SqliteKvStore implementation, providing a key-value store backed by SQLite for Node.js. The implementation is comprehensive and includes a good set of tests. My review focuses on improving correctness and consistency. Key suggestions include aligning the database schema with the data types being used, ensuring consistent handling of timestamps across different operations, improving the cas method to handle undefined values gracefully, and removing leftover debugging code from tests. Overall, this is a great start for the SQLite driver.
dahlia
left a comment
dahlia
left a comment
There was a problem hiding this comment.
How do you plan to resolve the API differences between node:sqlite and bun:sqlite? Will SQL queries be shared?
Primary plan is to export each implementation for each environment, if possible. Plan B is to check runtime environment to import the appropriate dependencies for each runtime dynamically. In either case, I'd like to share the queries. |
nyeong
force-pushed
the
feature/sqlite-kv
branch
8 times, most recently
from
523d385 to
11ea60e
Compare
|
The test has been failed on mq test of postgres package, with duplicated type error on Postgres 👀 |
= Design Decisions - Atomic implementation for CAS operations with transaction - Calculate TTL with Temporal.epochMilliseconds since SQLite lacks interval type - Encode values with JSON.stringify since SQLite lacks native JSON type - Restrict table names to prevent SQL injection = Further improvements - Implement versions for Bun and Deno runtimes - Refactor to separate Node.js dependencies and create SQLite as a port & adapter pattern
This test case shuld be removed, since `JSON.stringify(Infinity)` is null, so the test case never be passed.
Use INSERT ... ON CONFLICT ... DO UPDATE instead of INSERT OR REPLACE to ensure that only the value and expires_at fields are updated when modifying an existing key. This maintains the original created timestamp while still allowing TTL updates. Added test case to verify that created timestamp remains unchanged during updates while expires_at is properly modified.
cas() has an error when set a key with undefined newValue. Added controll of edge case and a test case.
`expires` field create from the runtime as an integer. For consistency, `created` field is also set as an integer from the application rather than relying on SQLite's TIMESTAMP.
- Create adapter.ts for common SQLite interface - Update kv.ts to use the adapter interface instead of `node:sqlite` - Add Node.js implementation in node.ts
Changes: - Use virtual imports to resolve runtime dependencies. - Integrate divided tests into single test, `kv.test.ts`, using `node:test`.
provide synchronous API. - Add deno-lint-ignore to ignore `require-await` error - Add type annotations for `sqlite.node.ts` - Add type constructor for Number for SQLStatment of node:sqlite, since the type of `changes` is `number | bigint`
... and match versions of deno.json and package.json
nyeong
force-pushed
the
feature/sqlite-kv
branch
from
11ea60e to
3f8dff7
Compare
|
The docs for this pull request have been published: |
|
The docs has been published :) |
dahlia
left a comment
dahlia
left a comment
There was a problem hiding this comment.
Looks good in general, except for few docs!
- Use code-group for platform-dependent code example - Remove outdated contents of the README - Add docs for the class constructor
|
The test failed due to errors in postgreSQL's tests. Since the failure has been repeated, I've made it #346. |
|
The latest push to this pull request has been published to JSR and npm as a pre-release:
|
4 participants
Summary
Implement SQLiteKvStore for Node.js
Related Issue
Related with #274
Changes and Goals
Benefits
Checklist
deno task test-allon your machine?Additional Notes
Currently, basic implementation for Node.js has been done. I will work the Deno and Bun implementations after receiving feedbacks. :)
Design Decisions
Temporal.epochMillisecondssince SQLite lacks interval typeJSON.stringifysince SQLite lacks native JSON type