diff --git a/changelog.d/migration-null-alias.fixed.md b/changelog.d/migration-null-alias.fixed.md new file mode 100644 index 0000000000..46a481830d --- /dev/null +++ b/changelog.d/migration-null-alias.fixed.md @@ -0,0 +1 @@ +- Migration column functions (`t.string()` and the others, `t.column()`, `t.references()`) read `null`, the option's name before Wheels 3.0, as a deprecated alias of `allowNull` again. Since 3.0 it was ignored, so an older migration's `null=false` created a nullable column. `allowNull` wins when both are passed, and the deprecation is logged once per migration run. `wheels upgrade check` lists migrations that use `null=false` or `null=true`; check `null=false` columns for `NULL` rows before tightening them diff --git a/cli/lucli/Module.cfc b/cli/lucli/Module.cfc index dbba430f32..4473dc3e36 100644 --- a/cli/lucli/Module.cfc +++ b/cli/lucli/Module.cfc @@ -9437,6 +9437,25 @@ component extends="modules.BaseModule" { scanTargets: [{path: ".gitignore"}], fix: "Remove the vendor line and commit vendor/, so a clone, CI run or image build has the framework and your packages. Guide: ""Commit vendor/"", #guide#" }); + // `null` was the migration column option's name before 3.0 renamed it allowNull, with no + // alias until 4.2. From 3.0 through 4.1 a null=false column was created nullable; 4.2 reads + // null=false again, so a database rebuilt on 4.2 can be stricter than one built earlier. + arrayAppend(arguments.checks, { + description: "Migrations use null=false, the column option's name before Wheels 3.0", + severity: "advisory", + checkType: "grep", + pattern: "[,(]\s*null\s*=\s*[""']?(false|no|0)[""']?\s*[,)]", + scanTargets: [{path: "app/migrator/migrations", extensions: "cfc", recurse: true}], + fix: "Wheels 3.0 through 4.1 ignored null=false, so a database built or rebuilt by them from these migrations has these columns NULLABLE, and they may now hold NULL rows. 4.2 reads null=false again: a database rebuilt on 4.2 gets NOT NULL columns, stricter than the one you run. Check each column for NULL rows before you tighten it with changeColumn(allowNull=false), and rename null to allowNull in the migrations. Guide: ""Migration column option null"", #guide#" + }); + arrayAppend(arguments.checks, { + description: "Migrations use null=true, the column option's name before Wheels 3.0", + severity: "advisory", + checkType: "grep", + pattern: "[,(]\s*null\s*=\s*[""']?(true|yes|1)[""']?\s*[,)]", + scanTargets: [{path: "app/migrator/migrations", extensions: "cfc", recurse: true}], + fix: "4.2 reads null as a deprecated alias of allowNull and logs a warning. Rename it to allowNull. Guide: ""Migration column option null"", #guide#" + }); arrayAppend(arguments.checks, { description: "findAll(returnAs=""structs"") now returns an array", severity: "advisory", diff --git a/cli/lucli/tests/specs/commands/UpgradeCheck4x2Spec.cfc b/cli/lucli/tests/specs/commands/UpgradeCheck4x2Spec.cfc index 66d4a9bf82..f47fde42da 100644 --- a/cli/lucli/tests/specs/commands/UpgradeCheck4x2Spec.cfc +++ b/cli/lucli/tests/specs/commands/UpgradeCheck4x2Spec.cfc @@ -188,6 +188,30 @@ component extends="wheels.wheelstest.system.BaseSpec" { expect(found).toBe(["app/controllers/Signups.cfc:4"]); }); + it("lists migrations that use null=false and null=true separately", () => { + var nl = chr(10); + put("app/migrator/migrations/20180519_Users.cfc", "component extends=""wheels.migrator.Migration"" {" & nl + & "function up() {" & nl + & "t.string(columnNames='email', default='', null=false, limit='255');" & nl + & "t.text(columnNames='bio', null=true);" & nl + & "t.integer(columnNames='age', allowNull=false);" & nl + & "}" & nl & "}"); + var report = runCheck(); + expect(matchesFor(report.advisories, "use null=false")).toBe(["app/migrator/migrations/20180519_Users.cfc:3"]); + expect(matchesFor(report.advisories, "use null=true")).toBe(["app/migrator/migrations/20180519_Users.cfc:4"]); + }); + + it("doesn't list allowNull or a commented-out null=", () => { + var nl = chr(10); + put("app/migrator/migrations/20180519_Users.cfc", "component extends=""wheels.migrator.Migration"" {" & nl + & "// t.string(columnNames='email', null=false);" & nl + & "function up() { t.string(columnNames='email', allowNull=false); }" & nl + & "}"); + var report = runCheck(); + expect(has(report.advisories, "use null=false")).toBeFalse(); + expect(has(report.advisories, "use null=true")).toBeFalse(); + }); + it("doesn't flag persisted in a comment or inside another name", () => { put("app/controllers/Signups.cfc", "component {#chr(10)#// if (!result.persisted) {}#chr(10)#function create() { var persistedCount = 1; }#chr(10)#}"); expect(has(runCheck().advisories, "reads persisted from a job result")).toBeFalse(); diff --git a/vendor/wheels/migrator/TableDefinition.cfc b/vendor/wheels/migrator/TableDefinition.cfc index 308f450d46..3bd4c76e3d 100644 --- a/vendor/wheels/migrator/TableDefinition.cfc +++ b/vendor/wheels/migrator/TableDefinition.cfc @@ -103,6 +103,7 @@ component extends="Base" { numeric precision, numeric scale ) { + $applyNullAlias(arguments); arguments.adapter = this.adapter; arguments.name = arguments.columnName; arguments.type = arguments.columnType; @@ -124,6 +125,40 @@ component extends="Base" { return this; } + /** + * Internal function. `null` was the column option's name before 3.0, when it became `allowNull`. + * It's read again as a deprecated alias, so an older migration's `null = false` still gives a + * NOT NULL column. `allowNull` wins when both are passed. Read through arguments["null"]: `null` + * is a keyword on some engines. + */ + public void function $applyNullAlias(required struct args) { + if (!StructKeyExists(arguments.args, "null")) { + return; + } + if (!StructKeyExists(arguments.args, "allowNull")) { + arguments.args.allowNull = arguments.args["null"]; + } + StructDelete(arguments.args, "null"); + $warnNullAlias(); + } + + /** + * Internal function. Logs the `null` deprecation once per request (one migration run), not once + * per column. Returns whether it logged. + */ + public boolean function $warnNullAlias() { + if (StructKeyExists(request, "$wheelsMigratorNullAliasWarned")) { + return false; + } + request.$wheelsMigratorNullAliasWarned = true; + writeLog( + file = "wheels", + type = "warning", + text = "Wheels: a migration column uses `null`, the option's name before Wheels 3.0. It's read as `allowNull` for now; rename it to `allowNull`." + ); + return true; + } + /** * Shared implementation for the typed column helpers below: resolves the * columnNames/columnName alias, stamps the column type, and adds one column @@ -133,6 +168,7 @@ component extends="Base" { */ private any function $addTypedColumns(required string columnType, required struct args) { $combineArguments(args = arguments.args, combine = "columnNames,columnName", required = true); + $applyNullAlias(arguments.args); arguments.args.columnType = arguments.columnType; local.columnNamesArray = ListToArray(arguments.args.columnNames); local.iEnd = ArrayLen(local.columnNamesArray); @@ -340,7 +376,7 @@ component extends="Base" { string referenceNames, string columnNames, default, - boolean allowNull = "false", + boolean allowNull, boolean polymorphic = "false", boolean foreignKey = "true", string onUpdate = "", @@ -348,6 +384,10 @@ component extends="Base" { string referenceColumn = "id" ) { $combineArguments(args = arguments, combine = "referenceNames,columnNames", required = true); + $applyNullAlias(arguments); + if (!StructKeyExists(arguments, "allowNull")) { + arguments.allowNull = false; + } local.idSuffix = $get("useUnderscoreReferenceColumns") ? "_id" : "id"; local.typeSuffix = $get("useUnderscoreReferenceColumns") ? "_type" : "type"; local.referenceNamesArray = ListToArray(arguments.referenceNames); diff --git a/vendor/wheels/tests/specs/migrator/nullAliasSpec.cfc b/vendor/wheels/tests/specs/migrator/nullAliasSpec.cfc new file mode 100644 index 0000000000..1b8d49a530 --- /dev/null +++ b/vendor/wheels/tests/specs/migrator/nullAliasSpec.cfc @@ -0,0 +1,74 @@ +/** + * `null` is the column option's 2.x name, renamed `allowNull` in 3.0. It's a deprecated alias + * again: column(), the typed column functions and references() read it when `allowNull` isn't + * passed, so a 2.x migration's `null = false` gives a NOT NULL column. `null` is a keyword on some + * engines, so the specs pass it through argumentCollection. No table is created. + */ +component extends="wheels.WheelsTest" { + + function beforeAll() { + variables.migration = CreateObject("component", "wheels.migrator.Migration").init(); + } + + function newTable() { + return variables.migration.createTable(name = "c_o_r_e_nullalias", force = true, id = false); + } + + // Column options with `null` set to `value`, plus `extra`. + function options(required struct extra, required boolean value) { + var rv = Duplicate(arguments.extra); + rv["null"] = arguments.value; + return rv; + } + + function lastColumn(required any table) { + return arguments.table.columns[ArrayLen(arguments.table.columns)]; + } + + function run() { + + describe("The null column option", () => { + + it("sets allowNull on the typed column functions", () => { + var t = newTable(); + t.string(argumentCollection = options({columnNames = "title"}, false)); + expect(lastColumn(t).allowNull).toBeFalse(); + t.integer(argumentCollection = options({columnNames = "views"}, true)); + expect(lastColumn(t).allowNull).toBeTrue(); + }); + + it("sets allowNull on column() and references()", () => { + var t = newTable(); + t.column(argumentCollection = options({columnName = "code", columnType = "string"}, false)); + expect(lastColumn(t).allowNull).toBeFalse(); + t.references(argumentCollection = options({columnNames = "author"}, true)); + expect(lastColumn(t).allowNull).toBeTrue(); + }); + + it("loses to allowNull when both are passed", () => { + var t = newTable(); + t.string(argumentCollection = options({columnNames = "title", allowNull = true}, false)); + expect(lastColumn(t).allowNull).toBeTrue(); + t.references(argumentCollection = options({columnNames = "author", allowNull = false}, true)); + expect(lastColumn(t).allowNull).toBeFalse(); + }); + + it("keeps references() NOT NULL by default", () => { + var t = newTable(); + t.references(columnNames = "author"); + expect(lastColumn(t).allowNull).toBeFalse(); + }); + + it("logs the deprecation once per request", () => { + var t = newTable(); + StructDelete(request, "$wheelsMigratorNullAliasWarned"); + t.string(argumentCollection = options({columnNames = "a"}, false)); + expect(StructKeyExists(request, "$wheelsMigratorNullAliasWarned")).toBeTrue(); + expect(t.$warnNullAlias()).toBeFalse(); + }); + + }); + + } + +} diff --git a/web/sites/guides/src/content/docs/v4-2-0/upgrading/4x-1-to-4x-2.mdx b/web/sites/guides/src/content/docs/v4-2-0/upgrading/4x-1-to-4x-2.mdx index 03a8fdfd0c..4ef1387a94 100644 --- a/web/sites/guides/src/content/docs/v4-2-0/upgrading/4x-1-to-4x-2.mdx +++ b/web/sites/guides/src/content/docs/v4-2-0/upgrading/4x-1-to-4x-2.mdx @@ -352,6 +352,16 @@ Instances that share a database now take turns running migrations ([#4134](https In a migration, `default=""` on a column that isn't string-like (`bigInteger`, `binary`, `uniqueidentifier`, `integer`, `decimal`, `float`, `date` and so on) now means "no default" ([#4248](https://github.com/wheels-dev/wheels/pull/4248)). It renders as `DEFAULT NULL` when the column allows null and as no `DEFAULT` clause on a `NOT NULL` column. In 4.1, `bigInteger` and `uniqueidentifier` emitted a bare `DEFAULT`, `binary` emitted `DEFAULT ''`, and a `NOT NULL` column got `DEFAULT NULL NOT NULL`, which MySQL rejects; those migrations failed. They now run. Migrations that already worked produce the same schema. +### Migration column option `null` + +Before Wheels 3.0, a migration column's nullability option was called `null`. 3.0 renamed it `allowNull` and dropped the old name, so Wheels 3.0 through 4.1 ignored `null`. No `NULL` / `NOT NULL` clause was emitted, and the column got the database's default, nullable on most databases. 4.2 reads `null` again as a deprecated alias of `allowNull`: `allowNull` wins when both are passed, and a warning is logged once per migration run. + +This matters for `null=false`. A database built or rebuilt by 3.0 through 4.1 from such a migration has the column nullable, and it may hold `NULL` rows. A database rebuilt on 4.2 from the same migration gets a `NOT NULL` column, stricter than the one you run in production. `wheels upgrade check` lists the migrations that use `null=false` and `null=true`. For each `null=false` column: + +1. Check the running database for `NULL` rows in that column. +2. Fix or backfill them, then tighten the column with a new migration (`changeColumn(..., allowNull=false)`), or change the old migration to `allowNull=true` if the column should allow `NULL`. +3. Rename `null` to `allowNull` in the migrations. + ### Unsupported migration column types fail at migrate time A migration that uses a column type the database adapter doesn't map, such as `t.column(columnName="payload", columnType="jsonb")`, now fails with `Wheels.Migrator.UnsupportedColumnType`, naming the type and the adapter ([#4095](https://github.com/wheels-dev/wheels/issues/4095)). Earlier releases emitted the column with no type. On most databases that was already a DDL error. On SQLite it created a typeless column whose model failed to load. If an existing SQLite migration relies on that, change the column to a supported type (for example `text` for JSON) before running it on a fresh database. The error's details list the supported types. Every type the `t.*()` column helpers emit is supported on every adapter.