Repository navigation
fix(migrator): add addForeignKeyOptions to PostgreSQL adapter so FK migrations run - #2877
Conversation
…igrations run The PostgreSQL migrator was the only adapter missing addForeignKeyOptions(sql, options), so any migration emitting an inline foreign-key constraint — e.g. wheels generate scaffold ... --belongsTo=author — crashed with "Component [wheels.databaseAdapters.PostgreSQL.PostgreSQLMigrator] has no function with name [addForeignKeyOptions]" when run against PG. The new implementation mirrors the MySQL signature; CockroachDB inherits the fix via its PostgreSQLMigrator subclass. Fixes #2876 Signed-off-by: claude[bot] <41898282+claude[bot]@users.noreply.github.com>
Wheels Bot — No doc updatesReviewed this PR's diff and found no docs that need updating (purely internal bug fix — adds the missing |
![wheels-bot[bot]](https://avatars.githubusercontent.com/in/3657588?s=60&v=4){kind=small}
There was a problem hiding this comment.
Wheels Bot — Reviewer A
TL;DR: This is a correct, narrowly-scoped bug fix that adds the missing addForeignKeyOptions() to PostgreSQLMigrator, mirrors the established MySQL/MSSQL pattern, and ships a focused regression spec. No correctness, cross-engine, or security issues — submitting as a comment with two minor observations that do not block merge.
Correctness
The fix is accurate. Abstract.createTable() calls foreignKeys[i].toForeignKeySQL() then ForeignKeyDefinition.toForeignKeySQL() then ForeignKeyDefinition.addForeignKeyOptions() then adapter.addForeignKeyOptions(sql, options). MySQL, MSSQL, SQLite, and Oracle all implement that last hop; PostgreSQL (and by inheritance CockroachDB) did not, so any migration that produced an inline FK column threw at runtime. The new implementation generates valid standard SQL that PostgreSQL accepts inside a CREATE TABLE body.
One minor guard inconsistency worth noting:
vendor/wheels/databaseAdapters/PostgreSQL/PostgreSQLMigrator.cfc, lines ~32–40:
arguments.sql = arguments.sql & " FOREIGN KEY (" & arguments.options.column & ")";
if (StructKeyExists(arguments.options, "referenceTable")) {
if (StructKeyExists(arguments.options, "referenceColumn")) {referenceTable and referenceColumn are guarded with StructKeyExists, but options.column is dereferenced unconditionally. In the production call path this is always safe (column is a required param of ForeignKeyDefinition.init() and is always populated in the options struct by ForeignKeyDefinition.addForeignKeyOptions()), but it is inconsistent with how the sibling keys are handled. The MySQL and MSSQL adapters share this gap, so it is pre-existing behavior — no action needed now, but worth making uniform in a follow-up.
Conventions
No issues. The function is public, which is the correct access for an adapter method. No mixed positional/named argument calls. No query-vs-array confusion. Commit type is fix, scope is migrator, header is 81 characters — all conformant with commitlint.config.js. DCO sign-off present and matches the commit author identity.
Cross-engine
No cross-engine concerns introduced. The implementation is pure string concatenation with no closures, no application-scope members, no bracket-notation calls, no struct-literal array aliasing, and no Left(str, 0) edge. PostgreSQLMigrator.quoteTableName() and quoteColumnName() both return objectCase(name) without added delimiters, so the lack of explicit quoting calls in the new method is functionally equivalent to calling them — consistent with the adapter's existing style.
Tests
The regression spec covers the four cases that mattered most: method existence on the PG adapter, correct SQL output for PG, CockroachDB inheritance, and the ForeignKeyDefinition.toForeignKeySQL() integration path. That is sufficient to catch the original failure and its transitive sibling.
Minor nit on the first test (vendor/wheels/tests/specs/migrator/postgreSQLForeignKeyOptionsSpec.cfc, lines ~40–51):
it("exists as a public method on the adapter", () => {
var fns = getMetaData(variables.pgAdapter).functions;
var found = false;
for (var fn in fns) {
if (fn.name == "addForeignKeyOptions") {
found = true;
break;
}
}
expect(found).toBeTrue();
});The subsequent test that actually calls addForeignKeyOptions() already proves existence — a missing method throws, it does not return falsy. The getMetaData walk adds test-suite lines without adding meaningful coverage. Consider dropping it in a follow-up — not a blocker.
Neither the partial-options edge cases (e.g. referenceTable present but referenceColumn absent) nor the empty-options case are tested, but those gaps mirror the MySQL/MSSQL specs and are pre-existing. No action required here.
Docs
CHANGELOG entry under [Unreleased] ### Fixed is present and accurate. The PR author has correctly marked the AI reference docs and CLAUDE.md checklist items as N/A for a bug fix of this scope.
Security
No concerns. options.column, options.referenceTable, and options.referenceColumn originate from developer-written migration files, not from user-supplied request input, so the unquoted string interpolation does not expose an end-user SQL injection surface.
Overall: clean fix, correct pattern, good regression coverage. The two items above (unguarded options.column and the redundant getMetaData test) are worth noting in a follow-up but do not block this merge.
|
Wheels Bot — Reviewer B (round 1) A's review is accurate and well-evidenced. The correctness chain, cross-engine analysis, and security assessment all hold up against the diff. One weak test assertion A missed is not a blocker; the getMetaData nit A raised is borderline but defensible. I agree with A's non-blocking assessment; joint recommendation is merge. Sycophancy: None detected. A submitted as COMMENTED rather than APPROVED, cited specific lines for every claim, and explicitly labeled both findings as non-blocking follow-ups rather than approving without evidence. False positives: One borderline case: A calls the getMetaData walk test (lines ~40-51 of postgreSQLForeignKeyOptionsSpec.cfc) fully redundant on the grounds that a missing method throws rather than returning falsy. That reasoning is correct at the call site, but the getMetaData walk has a slightly different meaning — it verifies the method is declared directly on PostgreSQLMigrator rather than just inherited through the chain. Since the fix is precisely adding the declaration to that CFC, the test has non-zero signal. A's nit is not wrong but slightly overstates redundancy. Not worth continuing the loop over; reads as a style preference either way. Missed issues: One weak assertion A did not flag — postgreSQLForeignKeyOptionsSpec.cfc line ~70: Verdict alignment: A's COMMENTED verdict is consistent with the findings: both items are explicitly labeled non-blocking follow-ups and the fix itself is confirmed correct. request-changes would not be warranted. Convergence: Aligned. A correctly identified the fix as accurate, cross-engine-safe, and adequately tested. My one additional observation (weak toInclude assertion) is a minor test-quality nit that does not change the recommendation — regression coverage is sufficient to catch the original failure and its CockroachDB sibling. Joint recommendation: no blocking issues; this PR is ready to merge. |
Summary
wheels migrate latestcrashed on PostgreSQL (and CockroachDB) whenever a migration tried to emit an inline foreign-key constraint — e.g. anythingwheels generate scaffold ... --belongsTo=authorproduced — with the error reported in the issue:Abstract.createTable()builds the inline FK clause viaforeignKeys[i].toForeignKeySQL()→ForeignKeyDefinition.cfc→adapter.addForeignKeyOptions(sql, options). Every other adapter (MySQLMigrator,SQLiteMigrator,MicrosoftSQLServerMigrator,OracleMigrator) implements that method; onlyPostgreSQLMigratorwas missing it, so the call failed at runtime.CockroachDBMigratorextendsPostgreSQLMigratorand inherited the same gap, fixed transitively by this change.The new implementation mirrors the MySQL signature —
FOREIGN KEY (col) REFERENCES tbl (refCol)— which PostgreSQL accepts as-is. The reporter's "works on Windows" observation lined up with thewheels newSQLite default: only PostgreSQL/CockroachDB datasources ever reached the missing method.Related Issue
Closes #2876
Type of Change
Feature Completeness Checklist
Signed-off-by:(usegit commit -s); see CONTRIBUTING.mdpostgreSQLForeignKeyOptionsSpec.cfcreproduces the missing-method error (4 cases failed before the fix; all pass after), covering the PG adapter directly, the CockroachDB inheritance path, and theForeignKeyDefinition.toForeignKeySQL()integrationbot-update-docs.ymlif needed)bot-update-docs.ymlif needed)bot-update-docs.ymlif needed)[Unreleased]### Fixedbash tools/test-local.sh migrator(Lucee 7 + SQLite) reports 256 pass / 0 fail / 0 error with the fix in place; the same suite reports 252 pass / 1 fail / 3 error without the implementation change (the 4 failing cases are the new spec)Test Plan
bash tools/test-local.sh migrator— greenwheels new test_app && cd test_app→ switch the dev datasource to PostgreSQL →wheels generate scaffold Post title body:text publishedAt:datetime --belongsTo=author→wheels migrate latestnow completes instead of throwing theaddForeignKeyOptionserrortools/test-matrix.sh lucee7 postgres(and the same withadobe2023/boxlangfor parity) — the unit spec is adapter-instantiation-based so it runs on every engine, but a real-DDL roundtrip against Postgres catches anything the unit assertions missScreenshots / Output