diff --git a/changelog.d/3349-postgres-system-schema-columns.fixed.md b/changelog.d/3349-postgres-system-schema-columns.fixed.md new file mode 100644 index 0000000000..5e54c94f01 --- /dev/null +++ b/changelog.d/3349-postgres-system-schema-columns.fixed.md @@ -0,0 +1,3 @@ +- The PostgreSQL adapter no longer picks up columns from other schemas when introspecting a table. `cfdbinfo(type="columns")` applies no schema restriction, so JDBC matched the table name across every schema on the connection — and PostgreSQL, YugabyteDB and CockroachDB all ship catalog views named `sequences`, `tables`, `columns`, `views`, `triggers` and more. An application table sharing one of those names silently collected a second batch of phantom columns typed `"information_schema"."sql_identifier"`, which no `$getType()` case matched, and the model failed to initialise with the opaque `key [RV] doesn't exist`. Rows from `information_schema`, `pg_catalog`, `crdb_internal` and `pg_extension` are now dropped — inside the `cacheDatabaseSchema` memo, so the filtering costs one pass per datasource+table per application lifetime. Reported against YugabyteDB (PostgreSQL 15 wire protocol) and reproducible on stock PostgreSQL (#3349) +- The migrator's column lookup got the same guard. `vendor/wheels/migrator/Base.cfc` calls `$dbinfo(type="columns")` directly rather than through the model adapter, so `changeTable(name="sequences")` adding a column named `data_type` or `start_value` could see the catalog view's column and treat it as already present (#3349) +- An unmapped PostgreSQL column type now throws `Wheels.UnknownColumnType` naming the type, instead of `key [RV] doesn't exist` from an unassigned return variable — the failure names the column type and points at catalog bleed as the likely cause rather than reading like a framework bug (#3349) diff --git a/vendor/wheels/Global.cfc b/vendor/wheels/Global.cfc index a855c844da..118a096c90 100644 --- a/vendor/wheels/Global.cfc +++ b/vendor/wheels/Global.cfc @@ -574,6 +574,43 @@ return local.$wheels; return local.rv; } + /** + * Drops rows belonging to a database's system schemas from a `$dbinfo(type="columns")` + * result. + * + * `cfdbinfo(type="columns")` passes no schema restriction to JDBC's `getColumns()`, so the + * table name is matched across EVERY schema on the connection. PostgreSQL and YugabyteDB + * ship real ANSI `information_schema` views named `sequences`, `tables`, `columns`, + * `views`, `triggers` and more, so an application table sharing one of those names silently + * collects a second batch of phantom columns from the catalog (issue #3349). No application + * table lives in a system schema, so filtering them out is always safe. + * + * A result set that carries no `table_schem` column — several engines omit it — is returned + * untouched. Yes, JDBC really does spell it `table_schem`, not `table_schema`. + */ + public query function $excludeSystemSchemaRows( + required query columns, + string schemas = "information_schema,pg_catalog,crdb_internal,pg_extension" + ) { + if (!ListFindNoCase(arguments.columns.columnList, "table_schem")) { + return arguments.columns; + } + local.rv = QueryNew(arguments.columns.columnList); + local.columnNames = ListToArray(arguments.columns.columnList); + local.iEnd = arguments.columns.recordCount; + for (local.i = 1; local.i <= local.iEnd; local.i++) { + if (!ListFindNoCase(arguments.schemas, arguments.columns["table_schem"][local.i])) { + QueryAddRow(local.rv); + local.jEnd = ArrayLen(local.columnNames); + for (local.j = 1; local.j <= local.jEnd; local.j++) { + local.item = local.columnNames[local.j]; + QuerySetCell(local.rv, local.item, arguments.columns[local.item][local.i]); + } + } + } + return local.rv; + } + public any function $wddx(required any input, string action = "cfml2wddx", boolean useTimeZoneInfo = true) { arguments.output = "local.output"; local.args = {}; diff --git a/vendor/wheels/databaseAdapters/PostgreSQL/PostgreSQLModel.cfc b/vendor/wheels/databaseAdapters/PostgreSQL/PostgreSQLModel.cfc index 4666a5466e..adaa892d6d 100755 --- a/vendor/wheels/databaseAdapters/PostgreSQL/PostgreSQLModel.cfc +++ b/vendor/wheels/databaseAdapters/PostgreSQL/PostgreSQLModel.cfc @@ -90,10 +90,47 @@ component extends="wheels.databaseAdapters.Base" output=false { case "geography": local.rv = "cf_sql_other"; break; + default: + // Without this branch `local.rv` is never assigned and the return throws + // `key [RV] doesn't exist` — an error that names nothing useful and reads + // like a framework bug. The classic source was catalog bleed: a table whose + // name collides with an `information_schema` view picked up phantom columns + // typed `"information_schema"."sql_identifier"` (issue #3349, fixed in + // `$getColumnInfo()` below). Anything reaching here now is a genuinely + // unmapped PostgreSQL type, so say so. + Throw( + type = "Wheels.UnknownColumnType", + message = "The PostgreSQL column type `#arguments.type#` is not mapped to a CFML SQL type.", + extendedInfo = "Add a case for `#arguments.type#` to `$getType()` in `vendor/wheels/databaseAdapters/PostgreSQL/PostgreSQLModel.cfc`. If the type name looks schema-qualified (e.g. `""information_schema"".""sql_identifier""`), the column is not yours — it came from a catalog view sharing your table's name." + ); } return local.rv; } + /** + * Override Base adapter's function. + * + * `cfdbinfo(type="columns")` applies no schema restriction, so JDBC matches the table name + * across every schema on the connection. PostgreSQL and YugabyteDB both ship ANSI + * `information_schema` views named `sequences`, `tables`, `columns`, `views`, `triggers` + * and more, so an application table named `sequences` collected a second batch of columns + * from `information_schema.sequences` — typed `"information_schema"."sql_identifier"`, + * which nothing in `$getType()` matched (issue #3349). Via `CockroachDBModel`, the + * `crdb_internal` and `pg_extension` view names collide the same way. + * + * Filtering here rather than in `$getColumns()` keeps the work behind the + * `cacheDatabaseSchema` memo that `$getColumns()` wraps around this call — once per + * datasource+table per application lifetime instead of on every read. + */ + public query function $getColumnInfo( + required string table, + required string datasource, + required string username, + required string password + ) { + return $excludeSystemSchemaRows(columns = super.$getColumnInfo(argumentCollection = arguments)); + } + /** * Call functions to make adapter specific changes to arguments before executing query. */ diff --git a/vendor/wheels/migrator/Base.cfc b/vendor/wheels/migrator/Base.cfc index 128b2611cf..408f385aa7 100644 --- a/vendor/wheels/migrator/Base.cfc +++ b/vendor/wheels/migrator/Base.cfc @@ -232,6 +232,14 @@ component extends="wheels.Global"{ type = "columns", table = arguments.tableName ); + // This path calls $dbinfo directly rather than going through the model adapter, so it + // needs its own catalog-bleed guard: an unrestricted cfdbinfo matches the table name in + // every schema on the connection, and a table named after an `information_schema` view + // (`sequences`, `tables`, `columns`, …) picks up that view's columns too. A + // changeTable() adding a column whose name collides with one of them — `data_type`, + // `start_value` — would then see it as already present (issue #3349). No application + // table lives in a system schema, so this cannot drop a real column. + local.columns = $excludeSystemSchemaRows(columns = local.columns); local.columnList = ValueList(local.columns.COLUMN_NAME); if (!StructKeyExists(request, "$wheelsMigratorColumns")) { request.$wheelsMigratorColumns = {}; diff --git a/vendor/wheels/tests/specs/model/SystemSchemaColumnFilterSpec.cfc b/vendor/wheels/tests/specs/model/SystemSchemaColumnFilterSpec.cfc new file mode 100644 index 0000000000..b5b696d276 --- /dev/null +++ b/vendor/wheels/tests/specs/model/SystemSchemaColumnFilterSpec.cfc @@ -0,0 +1,115 @@ +component extends="wheels.WheelsTest" { + + function run() { + g = application.wo + + // Regression for issue #3349. + // + // `cfdbinfo(type="columns")` passes no schema restriction to JDBC's getColumns(), so the + // table name is matched across every schema on the connection. PostgreSQL and YugabyteDB + // ship real ANSI `information_schema` views named `sequences`, `tables`, `columns`, + // `views`, `triggers` and more — so an application table named `sequences` silently + // collected a second batch of columns from `information_schema.sequences`. + // + // These specs drive the filter with a hand-built catalog result rather than a live + // PostgreSQL connection, so the logic is covered on every engine × database leg + // including the SQLite-only ones. The end-to-end behaviour is verified separately + // against a real PostgreSQL container. + describe("Tests that $excludeSystemSchemaRows", () => { + + $catalogResult = function() { + var q = QueryNew("table_schem,table_name,column_name,type_name") + var rows = [ + ["public", "sequences", "id", "uuid"], + ["public", "sequences", "name", "varchar"], + ["public", "sequences", "value", "int8"], + ["information_schema", "sequences", "sequence_catalog", '"information_schema"."sql_identifier"'], + ["information_schema", "sequences", "start_value", '"information_schema"."character_data"'], + ["pg_catalog", "sequences", "oid", "oid"], + ["crdb_internal", "sequences", "descriptor_id", "int8"], + ["pg_extension", "sequences", "ext_col", "int8"] + ] + for (var row in rows) { + QueryAddRow(q) + QuerySetCell(q, "table_schem", row[1]) + QuerySetCell(q, "table_name", row[2]) + QuerySetCell(q, "column_name", row[3]) + QuerySetCell(q, "type_name", row[4]) + } + return q + } + + it("keeps only the application schema's columns", () => { + result = g.$excludeSystemSchemaRows(columns = $catalogResult()) + + expect(result.recordCount).toBe(3) + expect(ValueList(result.column_name)).toBe("id,name,value") + }) + + it("drops every system schema, not just information_schema", () => { + result = g.$excludeSystemSchemaRows(columns = $catalogResult()) + + // `crdb_internal` and `pg_extension` collide too, reached via + // CockroachDBModel extends PostgreSQLModel + expect(ValueList(result.table_schem)).toBe("public,public,public") + }) + + it("preserves every column of the source result", () => { + source = $catalogResult() + result = g.$excludeSystemSchemaRows(columns = source) + + expect(ListSort(result.columnList, "textnocase")).toBe(ListSort(source.columnList, "textnocase")) + }) + + it("returns a result with no table_schem column untouched", () => { + // several engines' cfdbinfo omits it; filtering must not blank the result + noSchema = QueryNew("column_name") + QueryAddRow(noSchema) + QuerySetCell(noSchema, "column_name", "id") + + result = g.$excludeSystemSchemaRows(columns = noSchema) + + expect(result.recordCount).toBe(1) + }) + + it("is case-insensitive about the schema name", () => { + upper = QueryNew("table_schem,column_name") + QueryAddRow(upper) + QuerySetCell(upper, "table_schem", "INFORMATION_SCHEMA") + QuerySetCell(upper, "column_name", "sequence_catalog") + + expect(g.$excludeSystemSchemaRows(columns = upper).recordCount).toBe(0) + }) + }) + + describe("Tests that the PostgreSQL adapter", () => { + + // Before this, an unmapped type left `local.rv` unassigned and the return threw + // `key [RV] doesn't exist` — which names neither the column, the type, nor the + // table, and reads like a framework bug rather than a schema problem. + it("names the offending type instead of throwing key [RV] doesn't exist", () => { + adapter = CreateObject("component", "wheels.databaseAdapters.PostgreSQL.PostgreSQLModel") + thrown = {type: "", message: ""} + + try { + adapter.$getType(type = '"information_schema"."sql_identifier"') + } catch (any e) { + thrown.type = e.type + thrown.message = e.message + } + + expect(thrown.type).toBe("Wheels.UnknownColumnType") + expect(thrown.message).toInclude("sql_identifier") + }) + + it("still maps the types it knows", () => { + adapter = CreateObject("component", "wheels.databaseAdapters.PostgreSQL.PostgreSQLModel") + + expect(adapter.$getType(type = "int8")).toBe("cf_sql_bigint") + expect(adapter.$getType(type = "uuid")).toBe("cf_sql_varchar") + expect(adapter.$getType(type = "jsonb")).toBe("cf_sql_longvarchar") + }) + }) + } + +}