MemberJunction / MemberJunction/MJ

CodeGen PostgreSQL identifier auto-quoting is a keyword denylist — audit for other silent keyword-collision failures

Open
#3,604 2 comments 0 reactions 0 assignees View on GitHub
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

Open the contributing 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.