MemberJunction / MemberJunction/MJ

PostgreSQL auto-quote: guard the ALL-CAPS-column / keyword-set invariant in CI

Open Beginner friendly
#3,792 0 comments 0 reactions 0 assignees View on GitHub
chore priority: low
Dominant language
TSQL
Stars
29
Forks
6
Avg merge
1d 8h
Merged PRs (30d)
308

Description

Follow-up from the review of #3697, filed as agreed there. Not a defect today — a load-bearing invariant that is currently asserted in prose rather than in CI.

## The invariant

`AutoQuotePostgreSQLIdentifiers` evaluates its ALL-CAPS keyword tier **before** the dot rule. That ordering is deliberate and correct: several entries exist specifically for their dot-qualified form (`INFORMATION_SCHEMA.COLUMNS`, `.TABLES`, `.ROUTINES`), the catalog's real relation names are lower case, and quoting the right-hand half yields `INFORMATION_SCHEMA."COLUMNS"`, which does not resolve. CodeGen executes that exact SQL through `qsql()` on every PostgreSQL run, so an unconditional dot rule would turn a working CodeGen run into a hard failure.

The cost of that ordering is a standing invariant:

> **No ALL-CAPS column name in any shipped schema may appear in `PostgreSQLQuotingKeywords`.**

If one ever does, tier 1 swallows it *even when dot-qualified* — `t.ACTION` comes back bare, folds to `t.action`, and fails with `column "action" does not exist`. That is precisely the failure class #3697 exists to eliminate, reachable through the one door left open.

## Why it holds today, and why that is fragile

The invariant currently holds, and #3697's changeset states so. I verified it against `migrations-pg/v5/B202607091514__v5.46.x__Baseline.pg.sql` — the complete set of ALL-CAPS columns in the shipped schema is:

```
BCMID ID ISO2 ISO3 ISO3166_2 SQL URI URL
```

None is in the keyword set, and all eight quote correctly in both positions:

```
SELECT SQL FROM t => SELECT "SQL" FROM t
SELECT t.SQL FROM x t => SELECT t."SQL" FROM x t
```

The fragility is that the invariant can be broken from **either side**, by someone who has no reason to be looking at the other:

- a migration adds an ALL-CAPS column that happens to be a keyword (`ACTION`, `TEXT`, `LOG`, `RANK`, `VALUES` — all already in the set as ALL-CAPS entries), or
- someone adds a word to `PostgreSQLQuotingKeywords` to fix an unrelated keyword-quoting problem, which is a routine and otherwise-safe edit.

The second is the likely one. #3697 documents that adding a word to that set is free *because* the mixed-case column form still quotes — which is true for mixed-case columns and silently false for ALL-CAPS ones.

## Proposed guard

The same shape as the structural-tier guard `postgresqlAutoQuote.baseline.test.ts` already carries, in the same file, over the column set it already extracts:

```ts
it('no ALL-CAPS baseline column collides with the ALL-CAPS keyword tier', () => {
const colliding = distinctColumns
.filter((c) => c === c.toUpperCase())
.filter((c) => PostgreSQLQuotingKeywords.has(c));

expect(colliding, colliding.length === 0 ? '' :
`These ALL-CAPS baseline columns are in PostgreSQLQuotingKeywords: ${colliding.join(', ')}.\n\n` +
`Tier 1 runs before the dot rule, so such a column is emitted bare even as \`alias.COL\`, ` +
`folds to lowercase on PostgreSQL, and fails with 'column "..." does not exist'. Either ` +
`rename the column, or remove the word from PostgreSQLQuotingKeywords and confirm nothing ` +
`relies on its ALL-CAPS dot-qualified form (INFORMATION_SCHEMA.COLUMNS is the reason the ` +
`ordering exists).`
).toEqual([]);
});
```

Worth pairing with a positive assertion that the eight known ALL-CAPS columns still quote both bare and dot-qualified, so the guard fails on a rule regression as well as on a set/schema collision.

## Scope, stated honestly

This inherits the same limitation as the guards beside it, and the limitation should be repeated rather than assumed: it reads the **newest baseline only**. Columns added by later `migrations-pg` files and non-`__mj` schemas are not covered, so a green run means "no collision as of the last baseline", not "no collision". That is adequate here for the same reason it is adequate for the structural tier — the population is tiny and slow-moving — but it is not the same thing as proof.

Refs #3697.

Contributor guide

Open the contributing guide

Research direction

Start in postgresqlAutoQuote.baseline.test.ts and inspect how it extracts columns from migrations-pg/v5/B202607091514__v5.46.x__Baseline.pg.sql. Add the invariant guard and the proposed positive coverage for the eight known ALL-CAPS columns, then run the relevant PostgreSQL auto-quote test or CI check; done means both assertions pass while retaining the newest-baseline limitation.

Written by the indexing model from the issue text.

Assessment

Tech stack
postgresql, typescript
Domain
ci-cd, databases, testing
Issue type
Feature
Difficulty
2/5
Estimated time
1-3 hours
Activity status
Quiet
Clarity
Clearly specified
Newbie friendliness
78/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.