MemberJunction / MemberJunction/MJ
CodeGen PostgreSQL identifier auto-quoting is a keyword denylist — audit for other silent keyword-collision failures
- Dominant language
- TSQL
- Stars
- 29
- Forks
- 6
- Avg merge
- 2d 1h
- Merged PRs (30d)
- 323
Description
## Summary
CodeGen's PostgreSQL identifier auto-quoting (`PostgreSQLCodeGenProvider.quoteSQLForExecution` → `processWord`) is a **denylist heuristic**: it auto-quotes PascalCase identifiers *unless* the word (upper-cased) is in the static `_SQL_KEYWORDS` set. Any SQL that references a column whose name collides with that keyword set is passed through **unquoted**, folds to lowercase on PostgreSQL, and either throws `column "..." does not exist` or silently binds the wrong thing. SQL Server is unaffected (case-insensitive resolution), so these defects are invisible on the dialect most authoring happens against.
This was the root cause of one of the two defects fixed in #3590 (`Length` collided with `LENGTH` in `_SQL_KEYWORDS`, so `manageSingleEntityParentFields` threw before mirroring any IS-A field). That PR patched the two IS-A instances. This issue tracks the **systemic** problem, of which those were just two instances.
## Why this matters
- **Silent on the dialect people develop against.** T-SQL-first authoring means these ship undetected and only surface on a live PostgreSQL deployment.
- **Latent across the whole codegen SQL surface.** Any current or future query referencing a keyword-colliding column is exposed. Candidate collision names present in `_SQL_KEYWORDS` that are plausible MJ column names include: `Length`, `Position`, `Comment`, `Level`(via none? verify), `Language`, `Action`, `Order`, `Key`, `Default`, `Check`, `Column`, `Table`, `View`, `Index`, `Source`/`Target` (verify), etc. The exact exposed set = (MJ column names) ∩ `_SQL_KEYWORDS`.
## Proposed work
1. **Audit** `packages/CodeGenLib/src/Database/manage-metadata.ts` and the codegen SQL-generation paths for raw SQL strings that reference entity/metadata columns **unquoted** (i.e. relying on the tokenizer rather than `qi()`/`qs()`). Cross-reference column names against `_SQL_KEYWORDS`.
2. **Fix** by wrapping identifiers in `qi()` explicitly — the robust pattern established by #3590 — rather than trusting the denylist.
3. **Guard going forward.** Options to consider:
- A lint/test that scans codegen SQL string literals for unquoted mixed-case identifiers colliding with `_SQL_KEYWORDS`.
- An integration-tier test that exercises the affected paths against a real PostgreSQL instance.
4. **Consider inverting the heuristic long-term.** A denylist of keywords is inherently incomplete; explicit quoting at the call site (or an allowlist/structured builder) removes the class of bug entirely.
## References
- PR #3590 — `fix(codegen): IS-A inheritance silently does nothing on PostgreSQL`
- `PostgreSQLCodeGenProvider._SQL_KEYWORDS` and `processWord` in `packages/CodeGenLib/src/Database/providers/postgresql/PostgreSQLCodeGenProvider.ts`
## Related follow-up
Also from #3590 review: the identifier-quoting regression test in `isa-postgres-portability.test.ts` currently asserts the `QuoteIdentifier` primitive (which was never broken) rather than that the fixed method actually emits quoted identifiers through `quoteSQLForExecution`. A stronger test composing the real SELECT/UPDATE belongs with this work.
Contributor guide
Research direction
Start with packages/CodeGenLib/src/Database/manage-metadata.ts and the codegen SQL-generation paths, then inspect PostgreSQLCodeGenProvider.ts, especially _SQL_KEYWORDS and processWord. Cross-reference unquoted entity or metadata identifiers with the keyword set and review isa-postgres-portability.test.ts. Done means affected identifiers use qi() or equivalent coverage demonstrates quoted SQL through quoteSQLForExecution.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- postgresql, sql, typescript
- Domain
- backend, database
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Quiet
- Clarity
- Mostly clear
- Newbie friendliness
- 48/100