fix(model): exclude system-schema columns from PostgreSQL table introspection - #3356
Conversation
There was a problem hiding this comment.
Wheels Bot — Reviewer
TL;DR — This PR fixes #3349: a PostgreSQL table whose name collides with an ANSI information_schema view (sequences, tables, columns, …) was picking up phantom catalog columns via an unrestricted cfdbinfo(type="columns"), then dying with key [RV] doesn't exist. The fix — a shared $excludeSystemSchemaRows() filter on wheels.Global, wired into both the model adapter and the migrator, plus a default: branch in $getType() — is well-targeted, correctly placed behind the cacheDatabaseSchema memo, and covered by engine-agnostic specs. Verdict: comment. The only blocker-adjacent item is a batch of accidentally-committed test artifacts; the framework change itself is clean.
Conventions
7 unrelated MockBox stub artifacts committed — public/testbox/system/stubs/{139184C0…,16CA3942…,30942F4D…,754BEF48…,B7681C49…,C383511C…,CDCB73D6…}.cfm. These are runtime-generated TestBox mock stubs for SSE / channel helpers (closeSSEStream, sendSSEComment, checkError, sendSSEEvent, initSSEStream, poll, $getChannelEngine) — e.g.:
variables[ "closeSSEStream" ] = variables[ "tmp_closeSSEStream_139184C0543CCDB338AEFB643110CB89" ];git show cba6a1d21 --stat confirms all 7 land in the same commit as the PostgreSQL fix (12 files changed), and each is a new file. They have nothing to do with #3349 — MockBox regenerates these under content-hashed names on every run, so committing stale copies just pollutes the tree. Please git rm the seven public/testbox/system/stubs/*.cfm additions (and consider a .gitignore entry for public/testbox/system/stubs/ so they don't recur). This is a hygiene nit, not a functional regression — hence the non-blocking verdict.
Tests
Coverage is solid and, deliberately, engine-agnostic — the filter is driven with a hand-built catalog result rather than a live PostgreSQL connection (SystemSchemaColumnFilterSpec.cfc:11), so it runs on every engine × DB leg. It exercises the multi-schema drop, crdb_internal/pg_extension, column-list preservation, the missing-table_schem passthrough, case-insensitivity, and both $getType() branches. The catch block correctly mutates a struct field rather than a local (SystemSchemaColumnFilterSpec.cfc:713-717), which is the BoxLang-safe pattern (Cross-Engine Invariant 11).
One gap worth noting (not blocking): the two integration seams — PostgreSQLModel.$getColumnInfo() and the migrator/Base.cfc call site — are not exercised by an automated spec; the PR body states they're verified against a real PostgreSQL container out-of-band. Given the filter logic itself is unit-covered and the seams are one-line delegations, that's an acceptable split.
Cross-engine
No issues. $excludeSystemSchemaRows() is public with a $ prefix on wheels.Global (Invariant 7), its for (local.i = …) loops are not inside a finally (Invariant 12), and it uses only portable QueryNew/QueryAddRow/QuerySetCell/arguments.columns[col][row] idioms. The override signature matches Base.cfc:559 and databaseAdapters/Base.cfc extends wheels.Global, so the shared helper resolves on both hierarchies.
Docs
Changelog fragment present and correctly named: changelog.d/3349-postgres-system-schema-columns.fixed.md (.fixed.md type suffix, no direct CHANGELOG.md edit). Good.
Commits
fix(model): exclude system-schema columns from PostgreSQL table introspection — valid type/scope, subject well under 100 chars, describes the why. Conforms to commitlint.config.js.
Wheels Test Results 31 files 9 800 suites 20m 50s ⏱️ For more details on these failures and errors, see this check. Results for commit cba6a1d. |
a0272e3 to
58cbba8
Compare
…spection A model whose table is named `sequences` failed to initialise with `key [RV] doesn't exist`, thrown from `$initModelClass` while building the property list. The message names nothing useful and reads like a framework bug. `cfdbinfo(type="columns")` passes no schema restriction, so JDBC's `getColumns()` matches the table name across every schema on the connection. Verified directly against the driver — `getColumns(null, null, "sequences", null)` on stock PostgreSQL 16 with one three-column application table returns FIFTEEN rows: information_schema | sequence_catalog | "information_schema"."sql_identifier" information_schema | data_type | "information_schema"."character_data" information_schema | start_value | "information_schema"."character_data" ... 9 more ... public | id | uuid public | name | varchar public | value | int8 PostgreSQL, YugabyteDB and CockroachDB all ship ANSI `information_schema` views named `sequences`, `tables`, `columns`, `views`, `triggers` and more, so any table sharing one of those names collects the view's columns too. Nothing in `$getType()` matches a type named `"information_schema"."sql_identifier"`, and that function has no `default:` branch, so `local.rv` was never assigned and the return threw. Three changes: 1. `PostgreSQLModel.$getColumnInfo()` drops rows from `information_schema`, `pg_catalog`, `crdb_internal` and `pg_extension`. `$getColumnInfo` is the right seam rather than `$getColumns`: the latter wraps this call in the `cacheDatabaseSchema` memo, so filtering underneath costs one pass per datasource+table per application lifetime instead of one per read. (H2 filters the same class of collision one level up, so it re-filters on every call.) 2. The migrator got the same guard. `vendor/wheels/migrator/Base.cfc` calls `$dbinfo` directly rather than through the model adapter, so it is a second, independent path to the same bleed — a `changeTable(name="sequences")` adding `data_type` or `start_value` would see the catalog view's column and treat it as already present. Both of those really are columns of `information_schema.sequences`; see the probe output above. 3. `$getType()` gained a `default:` that throws `Wheels.UnknownColumnType` naming the type, and pointing at catalog bleed when the name looks schema-qualified. This does not widen what the adapter accepts — an unmapped type already failed, just unreadably. Deliberately NOT a fallback to `cf_sql_varchar`: only SQLite does that, justified there by its dynamic typing. The shared filter lives on `wheels.Global` because the two call sites are in different class hierarchies (`databaseAdapters.Base` and `migrator.Base`) that both extend it. A result set with no `table_schem` column is returned untouched, since several engines omit it. 7 specs. The filter is driven with a hand-built catalog result rather than a live PostgreSQL connection, so the logic is covered on every engine × database leg rather than only the PostgreSQL ones — including the case-insensitivity of the schema match and the missing-`table_schem` passthrough. Verification, lucee7 + sqlite, full core suite: develop ab901cf 4732 pass / 0 fail / 0 error this branch 4739 pass / 0 fail / 0 error Exactly +7, the new specs. Compat matrix dispatched for the real PostgreSQL and CockroachDB legs across all five engines. Closes #3349 Signed-off-by: Peter Amiri <peter@alurium.com>
58cbba8 to
42f6e46
Compare
There was a problem hiding this comment.
Wheels Bot — Reviewer
TL;DR — This PR fixes #3349: on PostgreSQL/YugabyteDB/CockroachDB an unrestricted cfdbinfo(type="columns") matches the table name across every schema on the connection, so an application table whose name collides with an ANSI information_schema view (sequences, tables, columns, …) silently picked up phantom catalog columns typed "information_schema"."sql_identifier" — which no $getType() case matched, so the model died with the opaque key [RV] doesn't exist. The fix is a shared $excludeSystemSchemaRows() filter on wheels.Global, wired into both the model adapter ($getColumnInfo override, behind the cacheDatabaseSchema memo) and the migrator's direct $dbinfo path, plus a default: branch in $getType() that names the offending type. It is well-targeted, correctly placed, cross-engine clean, and unit-covered on every engine × DB leg. The prior review's only finding — seven stray MockBox stub artifacts — has been dropped from the tree. Verdict: approve.
Prior finding resolved
The earlier COMMENTED review flagged seven accidentally-committed public/testbox/system/stubs/*.cfm MockBox artifacts. The current head (42f6e460) no longer contains them — gh pr view --json files lists exactly the five files that belong to the fix (changelog.d/3349-…, Global.cfc, PostgreSQLModel.cfc, migrator/Base.cfc, SystemSchemaColumnFilterSpec.cfc). Resolved.
Correctness
- The override signature matches the base seam exactly (
databaseAdapters/Base.cfc:559-564), and$getColumns()wraps$getColumnInfo()in thecacheDatabaseSchemamemo (Base.cfc:434-467), so the filter genuinely runs once per datasource+table per application lifetime as the doc block claims — verified, not inferred. - The filter is safe for the common (non-colliding) table: JDBC returns only
public-schema rows, all of which pass the!ListFindNoCase(schemas, table_schem)test, so nothing is dropped. Thepreserves every column of the source resultspec confirms theQueryNew(columnList)rebuild keeps all metadata columns. - The
$getType()default:branch is strictly better than the prior behavior, not a regression: an unmapped type previously leftlocal.rvunassigned and threwkey [RV] doesn't exist; it now throwsWheels.UnknownColumnTypenaming the type. No previously-mapped type is affected (the switch cases are unchanged).
Cross-engine
No issues. $excludeSystemSchemaRows() is public with a $ prefix on wheels.Global (Invariant 7); both databaseAdapters/Base.cfc:1 and migrator/Base.cfc:1 extends="wheels.Global", so the shared helper resolves on both hierarchies. Its for (local.i = …) loops are not inside a finally (Invariant 12), and arguments.columns["table_schem"][local.i] is a bracket read, not a bracket-notation method call, so it dodges the Adobe parser crash (Invariant 4). Idioms are portable QueryNew/QueryAddRow/QuerySetCell. CockroachDBModel does not override $getColumnInfo, so it inherits the filter — matching the PR body's claim about crdb_internal/pg_extension.
Tests
Solid and deliberately engine-agnostic: the filter is driven with a hand-built catalog result (SystemSchemaColumnFilterSpec.cfc) rather than a live PostgreSQL connection, so it runs on every engine × DB leg. It covers the multi-schema drop, crdb_internal/pg_extension, column-list preservation, the missing-table_schem passthrough, case-insensitivity, and both $getType() branches. The catch block mutates a struct field (thrown.type/thrown.message) rather than a local var, which is the BoxLang-safe pattern (Invariant 11).
One acknowledged, acceptable gap (non-blocking): the two integration seams — PostgreSQLModel.$getColumnInfo() and the migrator/Base.cfc call site — are one-line delegations not exercised by an automated spec; the PR body states they were verified against a real PostgreSQL container out-of-band.
Docs
Changelog fragment present and correctly named — changelog.d/3349-postgres-system-schema-columns.fixed.md (.fixed.md type suffix, no direct CHANGELOG.md [Unreleased] edit). Good.
Commits
fix(model): exclude system-schema columns from PostgreSQL table introspection — valid type/scope, subject well under 100 chars, describes the why. Conforms to commitlint.config.js.
Minor observation (non-blocking)
CockroachDBModel.$getType() is its own override and does not carry the new default: branch, so a genuinely-unmapped CockroachDB type would still surface the opaque key [RV] doesn't exist. This is pre-existing and outside #3349's scope — and the $excludeSystemSchemaRows() filter (inherited via $getColumnInfo) already prevents the actual catalog bleed on CockroachDB — so it does not block. Worth a follow-up if you want the friendlier error there too.
Closes #3349.
The bug
A model whose table is named
sequencesfailed to initialise withkey [RV] doesn't exist, thrown from$initModelClasswhile building the property list.cfdbinfo(type="columns")passes no schema restriction, so JDBC'sgetColumns()matches the table name across every schema on the connection.Verified against the driver, not inferred
getColumns(null, null, "sequences", null)— exactly the callcfdbinfoissues — against stock PostgreSQL 16 with one three-column application table returns fifteen rows:PostgreSQL, YugabyteDB and CockroachDB all ship ANSI
information_schemaviews namedsequences,tables,columns,views,triggersand more. Nothing in$getType()matches"information_schema"."sql_identifier", and that function has nodefault:branch — solocal.rvwas never assigned and the return threw.Three changes
1.
PostgreSQLModel.$getColumnInfo()filters system schemas —information_schema,pg_catalog,crdb_internal,pg_extension.$getColumnInfois the right seam rather than$getColumns: the latter wraps this call in thecacheDatabaseSchemamemo, so filtering underneath costs one pass per datasource+table per application lifetime instead of one per read. (H2 filters the same class of collision one level up, so it re-filters on every call — not changed here, but worth knowing.)2. The migrator got the same guard.
vendor/wheels/migrator/Base.cfccalls$dbinfodirectly rather than through the model adapter, so it is a second, independent path to the same bleed. AchangeTable(name="sequences")adding a column nameddata_typeorstart_valuewould see the catalog view's column and treat it as already present — and both of those really are columns ofinformation_schema.sequences, per the probe above.3.
$getType()gained adefault:that throwsWheels.UnknownColumnTypenaming the type, and points at catalog bleed when the name looks schema-qualified.This does not widen what the adapter accepts — an unmapped type already failed, just unreadably. Deliberately not a fallback to
cf_sql_varchar: only SQLite does that, and it justifies it by its dynamic typing. Happy to change it to a fallback if you would rather exotic PostgreSQL types (tsvector,hstore,int4range) start working — that is a separate, larger decision.The shared filter lives on
wheels.Globalbecause the two call sites are in different class hierarchies (databaseAdapters.Baseandmigrator.Base) that both extend it. A result set with notable_schemcolumn is returned untouched, since several engines omit it.Specs
7 specs, driving the filter with a hand-built catalog result rather than a live PostgreSQL connection — so the logic is covered on every engine × database leg rather than only the PostgreSQL ones. Covers the multi-schema drop, that
crdb_internal/pg_extensionare dropped too, column-list preservation, the missing-table_schempassthrough, case-insensitive schema matching, and both$getType()branches.Verification
lucee7 + sqlite, full core suite:
ab901cff7Exactly +7 — the new specs, nothing else moved.
Compat matrix dispatched for the real PostgreSQL and CockroachDB legs across all five engines. Expect the usual red
Wheels Test Resultscheck on the head — the #3302 misattribution artifact from my own dispatch, not a regression.🤖 Generated with Claude Code