mysql - fix: moving to varbinary for key (fixes) - #2016
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request migrates the MySQL adapter to use byte-exact VARBINARY columns for keys and namespaces, validating character limits in Unicode code points before encoding. It also reorders the composite index to (namespace, id) for improved query performance. The review feedback highlights a critical issue in both the adapter initialization and the migration script: using some() to check if columns need migration triggers a redundant and potentially unsafe conversion of both columns even if only one needs it. It is recommended to check and migrate each column individually.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8ec42e7adc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const keyByteLength = this._keyLength * 4; | ||
| const namespaceByteLength = this._namespaceLength * 4; | ||
| const createTable = `CREATE TABLE IF NOT EXISTS ${tableEsc}(id VARBINARY(${keyByteLength}) NOT NULL, value TEXT, namespace VARBINARY(${namespaceByteLength}) NOT NULL DEFAULT '', expires BIGINT UNSIGNED DEFAULT NULL, UNIQUE INDEX ${indexName} (namespace, id), INDEX ${expiresIndexName} (expires))`; |
There was a problem hiding this comment.
When keyLength + namespaceLength exceeds 768, multiplying both limits by four makes the composite UNIQUE INDEX (namespace, id) larger than InnoDB's 3072-byte maximum, so adapter initialization fails. The documented keyLength: 512, namespaceLength: 512 combination produces a 4096-byte key and cannot create the table or complete migration; validate the combined limits or use an index representation that remains within MySQL's limit.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 40455eaa. The adapter constructor, keyLength and namespaceLength setters, and migration script now reject configurations where (keyLength + namespaceLength) * 4 exceeds the 3072-byte composite-index limit, before schema changes begin. The invalid 512 + 512 example was corrected in the README and regression tests cover both code paths.
…-VARBINARY-for-key # Conflicts: # storage/mysql/README.md # storage/mysql/scripts/migrate-v6.ts # storage/mysql/src/index.ts # storage/mysql/test/test.ts
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2016 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 55 55
Lines 4885 4905 +20
Branches 784 786 +2
=========================================
+ Hits 4885 4905 +20 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
1 participant
Please check if the PR fulfills these requirements
What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
mysql - fix: moving to varbinary for key (fixes)