Fix size of unicode field types and prevent converting at table migration#72
Fix size of unicode field types and prevent converting at table migration#72anthioikonomou wants to merge 3 commits into
Conversation
anthioikonomou
changed the title
There was a problem hiding this comment.
Pull request overview
Addresses issue #71 by improving how SQL Server Unicode column metadata is interpreted during table introspection/migration, aiming to avoid incorrect sizes and lossy type conversions.
Changes:
- Adjusts
table().columns()SQL to treatnchar/nvarcharlengths as character counts (not bytes) and formatstype1accordingly. - Changes migration comparison logic to avoid converting Unicode column types to non-Unicode equivalents.
- Bumps package version to
2.16.1.
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
src/MSSqlAdapter.js |
Updates column metadata query for Unicode sizes and tweaks migration type comparison logic. |
package.json |
Version bump to 2.16.1. |
package-lock.json |
Lockfile version bump to 2.16.1. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| self.execute('SELECT c0.[name] AS [name], c0.[isnullable] AS [nullable], CASE WHEN t0.[name] IN (\'nvarchar\', \'nchar\') THEN c0.[length]/2 ELSE c0.[length] END AS [size], c0.[prec] AS [precision], ' + | ||
| 'c0.[scale] AS [scale], t0.[name] AS type, t0.[name] + CASE WHEN t0.[variable]=0 THEN \'\' ' + | ||
| 'WHEN t0.[name] IN (\'nvarchar\', \'nchar\') THEN \'(\' + CASE WHEN c0.length>0 then CONVERT(varchar, c0.[length] / 2) ELSE \'max\' END + \')\' ' + | ||
| 'ELSE \'(\' + CONVERT(varchar,c0.[length]) + \')\' END AS [type1], ' + |
There was a problem hiding this comment.
This PR changes Unicode length handling and migration type comparisons, but there are no accompanying Jest tests. Adding coverage (e.g. create a table with nvarchar(512)/nchar(n) and assert columnsAsync() reports size=512, plus a migration scenario ensuring an existing nvarchar column is not altered to varchar/text) would help prevent regressions.
| callback = callback || function () { }; | ||
| self.execute('SELECT c0.[name] AS [name], c0.[isnullable] AS [nullable], c0.[length] AS [size], c0.[prec] AS [precision], ' + | ||
| 'c0.[scale] AS [scale], t0.[name] AS type, t0.[name] + CASE WHEN t0.[variable]=0 THEN \'\' ELSE \'(\' + CONVERT(varchar,c0.[length]) + \')\' END AS [type1], ' + | ||
| self.execute('SELECT c0.[name] AS [name], c0.[isnullable] AS [nullable], CASE WHEN t0.[name] IN (\'nvarchar\', \'nchar\') THEN c0.[length]/2 ELSE c0.[length] END AS [size], c0.[prec] AS [precision], ' + |
There was a problem hiding this comment.
In the columns() query, [size] divides c0.[length] by 2 for nchar/nvarchar unconditionally. For MAX types (and any case where c0.[length] <= 0), integer division will yield 0, which misrepresents the length and can break schema comparisons. Consider guarding the division with c0.[length] > 0 (and otherwise returning c0.[length] as-is) or explicitly mapping <=0 to a sentinel like -1/NULL to represent MAX.
| self.execute('SELECT c0.[name] AS [name], c0.[isnullable] AS [nullable], CASE WHEN t0.[name] IN (\'nvarchar\', \'nchar\') THEN c0.[length]/2 ELSE c0.[length] END AS [size], c0.[prec] AS [precision], ' + | |
| self.execute('SELECT c0.[name] AS [name], c0.[isnullable] AS [nullable], CASE WHEN t0.[name] IN (\'nvarchar\', \'nchar\') AND c0.[length] > 0 THEN c0.[length]/2 ELSE c0.[length] END AS [size], c0.[prec] AS [precision], ' + |
| // replace leading n character from nchar, nvarchar and ntext | ||
| // so that migration should not change this column types (prevent data loss) | ||
| oldType = oldType.replace(/\bn(char|varchar|text)\b/gi, '$1'); | ||
| //remove column from collection | ||
| migration.add.splice(i, 1); | ||
| i -= 1; |
There was a problem hiding this comment.
Stripping the leading "n" from the detected column type unconditionally changes oldType even when the model expects a Unicode type (e.g. Json maps to nvarchar(max)). That can cause unnecessary ALTER COLUMN operations (and in some cases could drive migrations in the wrong direction). Consider applying this normalization only when the new type is non-Unicode (char/varchar/text), or better: explicitly skip migrations that would convert nchar/nvarchar/ntext to non-Unicode types to avoid data loss while leaving Unicode-to-Unicode comparisons intact.
| // replace leading n character from nchar, nvarchar and ntext | |
| // so that migration should not change this column types (prevent data loss) | |
| oldType = oldType.replace(/\bn(char|varchar|text)\b/gi, '$1'); | |
| //remove column from collection | |
| migration.add.splice(i, 1); | |
| i -= 1; | |
| // prevent data loss: do not migrate from Unicode (nchar/nvarchar/ntext) to non-Unicode (char/varchar/text) | |
| const unicodeOld = /\bn(char|varchar|text)\b/i.test(oldType); | |
| const unicodeNew = /\bn(char|varchar|text)\b/i.test(newType); | |
| //remove column from collection | |
| migration.add.splice(i, 1); | |
| i -= 1; | |
| // if existing column is Unicode and new type is non-Unicode, skip altering this column | |
| if (unicodeOld && !unicodeNew) { | |
| continue; | |
| } |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 3 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // replace leading n character from nchar, nvarchar and ntext | ||
| // so that migration should not change this column types (prevent data loss) | ||
| oldType = oldType.replace(/\bn(char|varchar|text)\b/gi, '$1'); | ||
| //remove column from collection | ||
| migration.add.splice(i, 1); | ||
| i -= 1; | ||
| if (newType !== oldType) { | ||
| if (newType.replace(/\bn(char|varchar|text)\b/gi, '$1') !== oldType) { | ||
| //add column to alter collection |
There was a problem hiding this comment.
The comparison logic normalizes both oldType and newType by stripping the leading n from nchar/nvarchar/ntext. This prevents legitimate upgrades from non-Unicode to Unicode types (e.g., existing varchar(...) with desired nvarchar(...) from MSSqlAdapter2) because newType is forced to compare as varchar(...) too, so the migration will incorrectly skip the needed ALTER.
Consider making the “treat Unicode/non-Unicode as equivalent” rule one-way: only skip changes when the existing column is Unicode (n*) and the requested type is the non-Unicode counterpart (downgrade). In all other cases (including varchar -> nvarchar upgrades, or nvarchar -> nvarchar changes), compare the real types without normalization.
| // replace leading n character from nchar, nvarchar and ntext | ||
| // so that migration should not change this column types (prevent data loss) | ||
| oldType = oldType.replace(/\bn(char|varchar|text)\b/gi, '$1'); | ||
| //remove column from collection | ||
| migration.add.splice(i, 1); | ||
| i -= 1; | ||
| if (newType !== oldType) { | ||
| if (newType.replace(/\bn(char|varchar|text)\b/gi, '$1') !== oldType) { | ||
| //add column to alter collection |
There was a problem hiding this comment.
There are currently no migration tests covering the new “do not convert Unicode columns to non-Unicode types” behavior. Adding a test that sets up an existing nvarchar column and runs a migration that would normally request varchar would verify that the adapter skips the downgrade (and also that varchar -> nvarchar upgrades still work as expected).
fix #71