feat(core): add DataTypes.VARBINARY for variable-length binary columns#18215
feat(core): add DataTypes.VARBINARY for variable-length binary columns#18215Aviel212 wants to merge 3 commits intosequelize:mainfrom
Conversation
Adds a new `VARBINARY(n)` data type for storing variable-length binary
data with a fixed maximum byte length. Unlike BLOB, VARBINARY can be
efficiently indexed in MySQL/MariaDB, making it suitable for binary
identifiers (hashes, UUIDs as bytes, foreign account IDs, etc.).
- `DataTypes.VARBINARY(n)` / `DataTypes.VARBINARY({ length: n })`
- Validates length is a positive integer between 1 and 65535
- Accepts Buffer, string, Uint8Array, and ArrayBuffer values
- Supported in: MySQL, MariaDB, MSSQL
- Unsupported dialects throw a clear error at model definition time
- Resolves the long-standing TODO comment above the BLOB class
Closes sequelize#5981
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds a VARBINARY data type to core, including options/validation, SQL generation, dialect support flag, exports, tests, and enables VARBINARY support in MySQL, MariaDB, and MSSQL dialects. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 OpenGrep (1.17.0)test/esm-named-exports.test.js┌──────────────┐ �[32m✔�[39m �[1mOpengrep OSS�[0m �[1m Loading rules from local config...�[0m Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/core/test/unit/data-types/binary-types.test.ts (1)
121-127: Add an explicitArrayBufferhappy-path assertion.
validate()acceptsArrayBuffer, but this success case is not directly asserted in the new test block.Suggested test addition
it('should not throw if `value` is a valid binary value', () => { const type = DataTypes.VARBINARY(32); expect(() => type.validate('foobar')).not.to.throw(); expect(() => type.validate(Buffer.from('foobar'))).not.to.throw(); expect(() => type.validate(new Uint8Array(4))).not.to.throw(); + expect(() => type.validate(new ArrayBuffer(4))).not.to.throw(); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/test/unit/data-types/binary-types.test.ts` around lines 121 - 127, Add an explicit happy-path assertion that DataTypes.VARBINARY(32).validate accepts an ArrayBuffer: update the test case inside the it('should not throw if `value` is a valid binary value') block to include expect(() => type.validate(new ArrayBuffer(<appropriate length>))).not.to.throw(); — use DataTypes.VARBINARY and the validate method so the ArrayBuffer case is directly asserted alongside the existing Buffer and Uint8Array assertions.packages/core/src/abstract-dialect/data-types.ts (1)
1837-1874:VARBINARYduplicatesBLOBbinary value handling logic.
validate,sanitize,escape, andgetBindParamSqlare duplicated almost verbatim fromBLOB, which increases divergence risk.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/abstract-dialect/data-types.ts` around lines 1837 - 1874, The VARBINARY methods validate, sanitize, escape, and getBindParamSql duplicate BLOB logic; refactor by extracting shared helpers (e.g., validateBinaryValue, sanitizeBinaryValue, escapeBinaryValue, sharedGetBindParamSql) or by having VARBINARY delegate to the BLOB implementation so both types call the same functions; update VARBINARY.validate, VARBINARY.sanitize, VARBINARY.escape, and VARBINARY.getBindParamSql to invoke those shared helpers (or call BLOB's methods) and remove the duplicate code.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/core/src/abstract-dialect/data-types.ts`:
- Around line 1818-1831: The VARBINARY class currently validates length up to
65535 but doesn't enforce MSSQL's 8000-byte limit; update the protected
_checkOptionSupport(dialect: AbstractDialect) method to, after verifying
support, detect MSSQL (e.g., via dialect.name or dialect.dialectName on the
provided AbstractDialect) and throw a TypeError if this.options.length > 8000
with a clear message that MSSQL VARBINARY(n) max is 8000, so SQL generation for
VARBINARY uses a valid length for that dialect.
---
Nitpick comments:
In `@packages/core/src/abstract-dialect/data-types.ts`:
- Around line 1837-1874: The VARBINARY methods validate, sanitize, escape, and
getBindParamSql duplicate BLOB logic; refactor by extracting shared helpers
(e.g., validateBinaryValue, sanitizeBinaryValue, escapeBinaryValue,
sharedGetBindParamSql) or by having VARBINARY delegate to the BLOB
implementation so both types call the same functions; update VARBINARY.validate,
VARBINARY.sanitize, VARBINARY.escape, and VARBINARY.getBindParamSql to invoke
those shared helpers (or call BLOB's methods) and remove the duplicate code.
In `@packages/core/test/unit/data-types/binary-types.test.ts`:
- Around line 121-127: Add an explicit happy-path assertion that
DataTypes.VARBINARY(32).validate accepts an ArrayBuffer: update the test case
inside the it('should not throw if `value` is a valid binary value') block to
include expect(() => type.validate(new ArrayBuffer(<appropriate
length>))).not.to.throw(); — use DataTypes.VARBINARY and the validate method so
the ArrayBuffer case is directly asserted alongside the existing Buffer and
Uint8Array assertions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d93b1cb2-dd1b-4953-9a58-491edb428b71
📒 Files selected for processing (8)
packages/core/src/abstract-dialect/data-types.tspackages/core/src/abstract-dialect/dialect.tspackages/core/src/data-types.tspackages/core/src/index.d.tspackages/core/test/unit/data-types/binary-types.test.tspackages/mariadb/src/dialect.tspackages/mssql/src/dialect.tspackages/mysql/src/dialect.ts
| if (!Number.isInteger(length) || length < 1 || length > 65_535) { | ||
| throw new TypeError( | ||
| 'The "length" option of VARBINARY must be a positive integer between 1 and 65535.', | ||
| ); | ||
| } | ||
|
|
||
| this.options = { length }; | ||
| } | ||
|
|
||
| protected _checkOptionSupport(dialect: AbstractDialect) { | ||
| if (!dialect.supports.dataTypes.VARBINARY) { | ||
| throwUnsupportedDataType(dialect, 'VARBINARY'); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
What is the maximum allowed n for SQL Server VARBINARY(n), and how is larger binary data represented (e.g., VARBINARY(MAX))?
💡 Result:
The maximum allowed n for SQL Server VARBINARY(n) is 8000 bytes. Larger binary data is represented using VARBINARY(MAX), which supports up to 2^31-1 bytes (approximately 2 GB).
Citations:
- 1: https://learn.microsoft.com/en-us/sql/t-sql/data-types/binary-and-varbinary-transact-sql?view=sql-server-ver17
- 2: https://learn.microsoft.com/en-us/sql/t-sql/data-types/binary-and-varbinary-transact-sql?view=sql-server-ver15
🏁 Script executed:
# First, let's check the file size and read the relevant lines
wc -l packages/core/src/abstract-dialect/data-types.tsRepository: sequelize/sequelize
Length of output: 116
🏁 Script executed:
# Read the snippet around lines 1818-1831
sed -n '1810,1840p' packages/core/src/abstract-dialect/data-types.tsRepository: sequelize/sequelize
Length of output: 925
🏁 Script executed:
# Let's also look at the VARBINARY class definition and constructor
sed -n '1770,1835p' packages/core/src/abstract-dialect/data-types.tsRepository: sequelize/sequelize
Length of output: 1964
🏁 Script executed:
# Check MSSQL dialect configuration for VARBINARY support
find packages -name "*.ts" -path "*mssql*dialect*" | head -5Repository: sequelize/sequelize
Length of output: 127
🏁 Script executed:
# Search for BLOB implementation to understand duplication claim
rg -A 20 "class BLOB" packages/core/src/abstract-dialect/data-types.tsRepository: sequelize/sequelize
Length of output: 643
🏁 Script executed:
# Check MSSQL dialect configuration for VARBINARY
cat packages/mssql/src/dialect.ts | grep -A 30 -B 5 "VARBINARY" | head -50Repository: sequelize/sequelize
Length of output: 999
🏁 Script executed:
# Search for how VARBINARY is used in tests or implementation
rg "VARBINARY" packages/mssql/ -A 2 -B 2Repository: sequelize/sequelize
Length of output: 1008
🏁 Script executed:
# Check if there's any existing dialect-specific length validation for other data types
rg "_checkOptionSupport" packages/core/src/abstract-dialect/data-types.ts -A 5 | head -60Repository: sequelize/sequelize
Length of output: 2029
🏁 Script executed:
# Check if VARBINARY has MSSQL-specific overrides
cat packages/mssql/src/_internal/data-types-overrides.ts | grep -B 5 -A 20 "class.*VARBINARY\|VARBINARY"Repository: sequelize/sequelize
Length of output: 725
🏁 Script executed:
# Get the full content of data-types-overrides to see what classes are overridden
head -100 packages/mssql/src/_internal/data-types-overrides.tsRepository: sequelize/sequelize
Length of output: 2728
🏁 Script executed:
# Check how data-types-overrides are used in the MSSQL dialect
rg "data-types-overrides" packages/mssql/src/dialect.ts -B 2 -A 10Repository: sequelize/sequelize
Length of output: 694
MSSQL length constraints are not validated for VARBINARY(n).
The constructor allows lengths up to 65535 globally, but MSSQL VARBINARY(n) has a maximum of 8000 bytes. This will generate invalid SQL for MSSQL when n exceeds 8000 (e.g., VARBINARY(65535)), as there is no MSSQL-specific override for the VARBINARY class.
The validation should be added to _checkOptionSupport:
protected _checkOptionSupport(dialect: AbstractDialect) {
if (!dialect.supports.dataTypes.VARBINARY) {
throwUnsupportedDataType(dialect, 'VARBINARY');
}
+
+ if (dialect.name === 'mssql' && this.options.length > 8_000) {
+ throw new TypeError(
+ 'MSSQL supports VARBINARY(n) up to 8000 bytes. Use BLOB for larger payloads (mapped to VARBINARY(MAX) in MSSQL).',
+ );
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (!Number.isInteger(length) || length < 1 || length > 65_535) { | |
| throw new TypeError( | |
| 'The "length" option of VARBINARY must be a positive integer between 1 and 65535.', | |
| ); | |
| } | |
| this.options = { length }; | |
| } | |
| protected _checkOptionSupport(dialect: AbstractDialect) { | |
| if (!dialect.supports.dataTypes.VARBINARY) { | |
| throwUnsupportedDataType(dialect, 'VARBINARY'); | |
| } | |
| } | |
| if (!Number.isInteger(length) || length < 1 || length > 65_535) { | |
| throw new TypeError( | |
| 'The "length" option of VARBINARY must be a positive integer between 1 and 65535.', | |
| ); | |
| } | |
| this.options = { length }; | |
| } | |
| protected _checkOptionSupport(dialect: AbstractDialect) { | |
| if (!dialect.supports.dataTypes.VARBINARY) { | |
| throwUnsupportedDataType(dialect, 'VARBINARY'); | |
| } | |
| if (dialect.name === 'mssql' && this.options.length > 8_000) { | |
| throw new TypeError( | |
| 'MSSQL supports VARBINARY(n) up to 8000 bytes. Use BLOB for larger payloads (mapped to VARBINARY(MAX) in MSSQL).', | |
| ); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/core/src/abstract-dialect/data-types.ts` around lines 1818 - 1831,
The VARBINARY class currently validates length up to 65535 but doesn't enforce
MSSQL's 8000-byte limit; update the protected _checkOptionSupport(dialect:
AbstractDialect) method to, after verifying support, detect MSSQL (e.g., via
dialect.name or dialect.dialectName on the provided AbstractDialect) and throw a
TypeError if this.options.length > 8000 with a clear message that MSSQL
VARBINARY(n) max is 8000, so SQL generation for VARBINARY uses a valid length
for that dialect.
2 participants
What
Adds
DataTypes.VARBINARY(n)— a variable-length binary data type with a fixed maximum byte length.Closes #5981
Why
Sequelize currently lacks a native
VARBINARYtype. Users who need efficient binary indexes (e.g. SHA-256 hashes, UUIDs stored as bytes, binary foreign account IDs) are forced to useBLOBor raw SQL types.BLOBcannot be directly indexed in MySQL, makingVARBINARYthe correct choice for indexable binary columns.There is also a
// TODO: add FIXED_BINARY & VAR_BINARY data typescomment in the codebase right above theBLOBclass — this PR addresses that TODO for the variable-length case.How
VARBINARYclass inpackages/core/src/abstract-dialect/data-types.tstoSql()returnsVARBINARY(n)Buffer,string,Uint8Array, andArrayBuffervalues (same as BLOB)lengthis a positive integer between 1 and 65535supports.dataTypes.VARBINARYsupport flag added to the dialect interfaceUnsupportedDataTypeErrorat model definition timeDataTypes.VARBINARYandVarBinaryOptionstypeTests
Added to
test/unit/data-types/binary-types.test.ts:VARBINARY(n))validate()method (accepts Buffer, string, Uint8Array; rejects numbers)All 1741 unit tests pass.
Summary by CodeRabbit
New Features
Tests
Chores