Add @fedify/mysql package with MysqlKvStore#597
Conversation
Add a new @fedify/mysql package that provides a MysqlKvStore class implementing the KvStore interface backed by MySQL/MariaDB. Uses the mysql2 driver and supports all KvStore operations: get, set (with TTL), delete, list (with prefix matching), and compare-and-swap (cas). The package follows the same structure as @fedify/postgres: - Table auto-creation via initialize() and cleanup via drop() - Key serialization via JSON.stringify into VARCHAR(512) - Values stored as native JSON columns - TTL via absolute DATETIME(6) expiration timestamps - CAS using SELECT ... FOR UPDATE within transactions - Prefix listing via LIKE with proper escaping Also updates: - CI workflow to run MySQL 8 service containers for all test jobs - Documentation (kv.md, federation.md, basics.md, relay.md) - fedify init to offer MySQL/MariaDB as a KvStore option - CHANGES.md, AGENTS.md, and package README files fedify-dev#585 fedify-dev#587 Co-Authored-By: Claude <noreply@anthropic.com>
Summary of ChangesHello, 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 significantly enhances Fedify's data persistence capabilities by introducing a dedicated MySQL/MariaDB key-value store. This new integration provides a robust and scalable option for managing key-value data in production environments, complete with advanced features like time-to-live and atomic compare-and-swap operations. The changes also include comprehensive updates to documentation, build configurations, and the project initialization tool to fully support this new database adapter. Highlights
Changelog
Ignored Files
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
|
There was a problem hiding this comment.
Code Review
This pull request introduces the @fedify/mysql package, providing a MysqlKvStore for using MySQL or MariaDB as a persistent key-value store with Fedify. The implementation is robust, correctly handling key serialization, TTLs with DATETIME(6), and atomic compare-and-swap operations using transactions. The changes are well-integrated into the project, with thorough tests, documentation updates across the manual and tutorials, and inclusion in the fedify init tool. My review found only minor inconsistencies in the naming within documentation files. Overall, this is an excellent and valuable addition to the Fedify ecosystem.
Note: Security Review did not run due to the size of the PR.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a new @fedify/mysql package, providing a KvStore implementation for MySQL and MariaDB. The implementation is well-structured and includes comprehensive tests and documentation updates. I've identified a few areas for improvement in the MysqlKvStore implementation concerning potential key length limitations, performance of the key expiration mechanism, and the robustness of prefix searching. Additionally, there's an opportunity to simplify the TTL duration calculation.
Note: Security Review did not run due to the size of the PR.
There was a problem hiding this comment.
Pull request overview
This PR introduces a new @fedify/mysql package that provides a MySQL/MariaDB-backed KvStore implementation (MysqlKvStore) using the mysql2 driver, and wires it into the monorepo, documentation, CI, and fedify init.
Changes:
- Added
@fedify/mysqlpackage withMysqlKvStore, build config, and integration tests. - Updated workspace/dependency configuration (pnpm + Deno) to include
mysql2and the new package. - Updated docs, changelog, CI, and
fedify inittemplates to reference/use MySQL/MariaDB KV storage.
Reviewed changes
Copilot reviewed 22 out of 24 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| pnpm-workspace.yaml | Adds packages/mysql to the workspace and introduces mysql2 to the catalog. |
| pnpm-lock.yaml | Locks new workspace package and mysql2 dependency graph. |
| packages/relay/README.md | Mentions MysqlKvStore as a production KV option for relay usage. |
| packages/mysql/tsdown.config.ts | Adds build configuration, including Temporal polyfill intro injection. |
| packages/mysql/src/mod.ts | Exports MysqlKvStore public API surface. |
| packages/mysql/src/kv.ts | Implements MysqlKvStore (get/set/delete/list/cas + initialize/drop). |
| packages/mysql/src/kv.test.ts | Adds MySQL integration tests for the KV store operations. |
| packages/mysql/package.json | Defines the new package metadata, exports, and deps/peers. |
| packages/mysql/deno.json | Adds Deno/JSR metadata and tasks for the new package. |
| packages/mysql/README.md | Adds package README with installation and usage snippet. |
| packages/init/src/json/kv.json | Adds MySQL/MariaDB as a fedify init KV backend option. |
| packages/fedify/README.md | Registers @fedify/mysql in the monorepo package list. |
| docs/tutorial/basics.md | Mentions MysqlKvStore as a production KV option in the tutorial. |
| docs/package.json | Adds @fedify/mysql to docs dependencies for examples/build. |
| docs/manual/relay.md | Updates relay manual to include MySQL/MariaDB KV store option. |
| docs/manual/kv.md | Adds a new MysqlKvStore section with install + example code. |
| docs/manual/federation.md | Mentions @fedify/mysql as an additional production KV backend. |
| docs/.vitepress/config.mts | Adds @fedify/mysql to references list. |
| deno.lock | Locks npm:mysql2 and transitive deps for Deno usage. |
| deno.json | Registers packages/mysql workspace path and adds mysql2 imports. |
| CHANGES.md | Adds changelog entry for the new @fedify/mysql package. |
| AGENTS.md | Updates contributor/agent instructions to include packages/mysql. |
| .github/workflows/main.yaml | Adds a MySQL 8 service and MYSQL_URL to CI test jobs. |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
Codecov Report❌ Patch coverage is
... and 1 file with indirect coverage changes 🚀 New features to boost your workflow:
|
The tableName option was interpolated directly into SQL statements without any sanitization, which could allow SQL injection if the caller passes a malicious table name. Added a regex check in the constructor that restricts table names to identifiers starting with a letter or underscore, followed by letters, digits, or underscores. A RangeError is thrown for invalid names. fedify-dev#597 (comment) Co-Authored-By: Claude <noreply@anthropic.com>
The 512-character limit was too tight for keys containing long URLs or other lengthy strings. VARCHAR(768) aligns with the InnoDB index prefix limit for utf8mb4 (768 characters × 4 bytes = 3072 bytes), making full use of the available index space without risking index prefix truncation errors. fedify-dev#597 (comment) Co-Authored-By: Claude <noreply@anthropic.com>
Without an index on the expires column, the DELETE query in #expire() had to perform a full table scan on every write, degrading performance as the table grows. initialize() now creates an index named idx_<tableName>_expires on the expires column, making the periodic cleanup query an efficient index range scan. fedify-dev#597 (comment) Co-Authored-By: Claude <noreply@anthropic.com>
When a MySQL server runs with NO_BACKSLASH_ESCAPES, the implicit backslash escape behaviour used in the LIKE pattern is disabled, causing prefix lookups for keys containing %, _, or \ to return incorrect results. Added an explicit ESCAPE '\\' clause to make the escape character unambiguous regardless of the server's SQL mode. fedify-dev#597 (comment) Co-Authored-By: Claude <noreply@anthropic.com>
When value is undefined, setting a key should be treated as a no-op rather than attempting to store JSON.stringify(undefined) (which produces undefined, not a valid JSON value), which would fail with a NOT NULL constraint violation on the value column. Addresses: fedify-dev#597 (comment) Co-Authored-By: Claude <noreply@anthropic.com>
When newValue is undefined in a compare-and-swap operation, the intended semantics are to delete the key (if the expected value matches) rather than inserting NULL into the NOT NULL value column, which would cause a constraint violation. Addresses: fedify-dev#597 (comment) Co-Authored-By: Claude <noreply@anthropic.com>
The set() and delete() methods both call #expire() to clean up stale entries after each mutation, but cas() was missing this call. Without it, expired entries accumulate until the next set() or delete(). Addresses: fedify-dev#597 (comment) Co-Authored-By: Claude <noreply@anthropic.com>
Adds an expireCleanupRate option (0–1, default 1) that controls the probability of running expiry cleanup on each mutation. Setting it to 0 disables automatic cleanup entirely (useful when a separate cleanup job handles expiry), while values between 0 and 1 perform cleanup probabilistically to reduce per-write overhead on high-traffic tables. Addresses: fedify-dev#597 (comment) Co-Authored-By: Claude <noreply@anthropic.com>
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a new package @fedify/mysql which provides a MysqlKvStore for using MySQL or MariaDB as a key-value store with Fedify. The implementation is robust, well-tested, and covers all the necessary features of the KvStore interface, including atomic operations and TTL handling. The changes also include updates to documentation, CI, and the project initializer to integrate the new package.
I've identified a small area for improvement in the durationToSeconds helper function within packages/mysql/src/kv.ts, where using the Temporal.Duration.prototype.total() method can simplify the code. This comment aligns with general best practices and does not contradict any specific rules.
Note: Security Review did not run due to the size of the PR.
Use total({ unit: "second", relativeTo: ... }) instead of round() followed
by manual arithmetic, which is simpler and less error-prone. The relativeTo
option is required when the duration contains calendar units (years, months,
weeks, or days) and ensures correct conversion regardless of DST or leap
seconds.
Reviewed-at: fedify-dev#597 (comment)
Co-Authored-By: Claude <noreply@anthropic.com>
After drop() the table no longer exists, so the next call to initialize() must recreate it. Without resetting #initialized the flag stays true and initialize() is skipped, causing all subsequent operations to fail with a "table not found" error. Reviewed-at: fedify-dev#597 (comment) Co-Authored-By: Claude <noreply@anthropic.com>
MySQL identifier names are limited to 64 characters. The derived index name is "idx_<tableName>_expires" which uses 12 additional characters, so the table name itself must be at most 50 characters long. Now a RangeError is thrown at construction time if the name exceeds this limit, preventing a cryptic ER_TOO_LONG_IDENT error from MySQL at initialization time. Reviewed-at: fedify-dev#597 (comment) Co-Authored-By: Claude <noreply@anthropic.com>
The --health-cmd previously used "mysqladmin ping -h 127.0.0.1" without credentials. While ping itself doesn't require auth, supplying -uroot -pmysql makes the intent explicit and avoids potential failures on MySQL configurations that do require auth for any connection. Reviewed-at: fedify-dev#597 (comment) fedify-dev#597 (comment) fedify-dev#597 (comment) Co-Authored-By: Claude <noreply@anthropic.com>
npm, pnpm, Yarn, and Bun users must install mysql2 alongside @fedify/mysql because mysql2 is a peer dependency not automatically pulled in by those package managers. Deno users do not need a separate step since mysql2/promise is available through the import map bundled with the package. Updated: - docs/manual/kv.md (MysqlKvStore section) - packages/mysql/README.md (Installation section) Reviewed-at: fedify-dev#597 (comment) fedify-dev#597 (comment) Co-Authored-By: Claude <noreply@anthropic.com>
The SELECT ... FOR UPDATE in cas() acquires a gap lock on missing rows under InnoDB REPEATABLE READ (the default isolation level), which prevents concurrent insertions for the same key. Under READ COMMITTED this gap lock is not applied, so concurrent CAS operations on absent keys can race. Added a comment explaining this assumption so operators who change the pool's isolation level are warned. Reviewed-at: fedify-dev#597 (comment) Co-Authored-By: Claude <noreply@anthropic.com>
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces the @fedify/mysql package, providing a MysqlKvStore implementation. The changes include the new package with its source code, tests, and documentation, as well as updates to the monorepo configuration and documentation to integrate the new package. The implementation of MysqlKvStore uses MySQL-specific features like INSERT ... ON DUPLICATE KEY UPDATE and transactional locking with SELECT ... FOR UPDATE to ensure correctness and efficiency. The test suite is comprehensive and covers various use cases and edge cases. After a thorough review, I have not identified any issues of medium or higher severity.
Note: Security Review did not run due to the size of the PR.
When SELECT ... FOR UPDATE included the expiry predicate (AND expires > NOW(6)), a physically-present but logically-expired row was not locked. Two concurrent cas(key, undefined, ...) calls could therefore both see "no locked row" and both succeed, violating the create-if-absent atomicity guarantee. Fix: remove the expiry condition from the WHERE clause of SELECT ... FOR UPDATE so the lock is always acquired on a physically present row. Introduce an is_expired computed column and evaluate expiry in application code after acquiring the lock. Regression tests added: - test 19: cas() treats physically-present but expired rows as undefined - test 20: concurrent create-if-absent on expired key is atomic Reviewed-at: fedify-dev#597 (comment) Co-Authored-By: Claude <noreply@anthropic.com>
Summary
Closes #585
This pull request adds the
@fedify/mysqlpackage, which provides aMysqlKvStoreclass implementing theKvStoreinterface backed by MySQL or MariaDB via themysql2driver.New package:
@fedify/mysqlAPI
Implementation notes
KvKey) are serialized viaJSON.stringify()into aVARCHAR(768)column withutf8mb4_bincollation for case-sensitive matching.JSONcolumn;mysql2parses them back to JS objects automatically on read.DATETIME(6)computed at write time (DATE_ADD(NOW(6), INTERVAL ? SECOND)), and expired rows are filtered out on read.list()uses aLIKEprefix scan (with proper escaping of%,_, and\characters).cas()(compare-and-swap) usesSELECT ... FOR UPDATEinside a transaction for atomicity. Relies on InnoDB REPEATABLE READ (the default) for gap locking on missing rows.initialize()and can be dropped withdrop().Other changes
test,test-node,test-bun). Integration tests run when theMYSQL_URLenvironment variable is set.MysqlKvStoresection to docs/manual/kv.md, and updated federation.md, basics.md, and relay.md to mention it alongside the other production-readyKvStoreimplementations.fedify init: AddedMySQL/MariaDBas aKvStoreoption.Testing
19 integration tests cover all operations:
initialize,get,set,delete,drop,list(including expired-row filtering, single-element keys, LIKE special characters, and empty-prefix listing),cas,caswith TTL, and various constructor validations. All pass against a local MySQL 8 instance.Note
This PR implements only the
KvStorehalf of #587.MysqlMessageQueue(#586) will follow in a separate PR.