MemberJunction / MemberJunction/MJ
Remove the PostgreSQL identifier auto-quoting shim; make runtime-authored SQL dialect-aware
- Dominant language
- TSQL
- Stars
- 29
- Forks
- 6
- Avg merge
- 2d 1h
- Merged PRs (30d)
- 323
Description
## Summary
`AutoQuotePostgreSQLIdentifiers` (`packages/SQLDialect/src/postgresqlAutoQuote.ts`) is a compatibility shim that rewrites SQL at execution time on PostgreSQL, wrapping any bare word it does not recognise as a keyword in double quotes. It exists so SQL written T-SQL-first (`FROM __mj.vwAIAgentRuns WHERE TestRun IS NULL`) keeps working on PostgreSQL, which folds unquoted identifiers to lowercase.
**Long-term direction: remove the shim, and make every place that depends on it produce correctly quoted SQL for the environment's dialect at the point where the SQL is written** — rather than guessing at execution time.
An investigation into removing it found that the dependency is wider than code call sites. Besides code-written SQL (a mechanical fix), the shim is currently the **only** thing quoting several kinds of SQL that are written at runtime or stored as data: row-level security filters, saved view WHERE clauses (including AI-generated Smart Filters), and cross-entity subqueries inside `ExtraFilter`. Removing the shim before those are handled would break reads for every non-admin user on PostgreSQL. This issue records that analysis and the plan to work through the breaking points.
Not urgent — this is the tracking issue for the eventual removal.
---
## Why remove it
- **It is a recurring defect source.** Fix rounds so far: `d77cef2b05` (origin, 2026-04-29), `9bb9b0b3be`, `e5a531ec49`, `1fdd5d0632` (#3697), `706842286f`. Related: #3604, #3590, #3691, #3775, #3792.
- **Its failures are silent wrong answers, not errors.** Documented in `packages/SQLDialect/src/__tests__/postgresqlAutoQuote.shippedQueries.test.ts`:
- An apostrophe inside a SQL comment desynced the string-literal scanner, rewriting `WHERE ars."StepType" = 'Prompt'` to `= '"Prompt"'` — zero rows, no error.
- `jsonb_build_object` keys became `'"ID"'`, so consumers reading `.ID` got `undefined`.
- `{{ ConversationID | sqlString }}` became `{{ "ConversationID" | sqlString }}`, so the Nunjucks parameter never substituted and the query lost its filter.
- **It removed the pressure to fix the SQL it compensates for.** The example in the original commit message is still unquoted today at `packages/AI/Agents/src/memory-manager-agent.ts:788` (`FROM __mj.vwAIAgentRuns ar INNER JOIN __mj.vwConversations c ON ar.ConversationID = c.ID`).
---
## Where the shim runs
More entry points than it first appears:
| Entry point | Location | What goes through it |
|---|---|---|
| Runtime | `PostgreSQLDataProvider.ExecuteSQL` (`packages/PostgreSQLDataProvider/src/PostgreSQLDataProvider.ts:365`) | **Every** raw statement, including every assembled RunView query |
| CodeGen | `PostgreSQLCodeGenProvider.quoteSQLForExecution` (`.../postgresql/PostgreSQLCodeGenProvider.ts:2095`) via `ManageMetadataBase.qsql()` | All 53 `runQuery()` calls and all 78 `LogSQLAndExecute()` calls in `manage-metadata.ts` (its private `LogSQLAndExecute` wraps every statement in `qsql()`) |
| CodeGen config | `packages/CodeGenLib/src/Database/sql_codegen.ts:122` | The customer-authored `forceRegeneration.entityWhereClause` value from `mj.config.cjs` |
Note for the CodeGen side: the SQL that CodeGen logs into the `CodeGen_Run_*.sql` migration file is the **post-shim** SQL. That is useful: once code-written SQL is quoted explicitly, the logged output should be byte-identical to today's, which makes a before/after diff of CodeGen output the strongest regression check available.
---
## What depends on the shim
### A. SQL written at runtime or stored as data (the part that makes removal non-trivial)
These cannot be fixed by swapping in `QuoteIdentifier()` at a call site, because the SQL text comes from people, AI, or the database.
**1. Row-level security `FilterText` — shipped core data.**
`GenericDatabaseProvider.ts:1947-1951` appends `entityInfo.GetEffectiveRowFilterWhereClause(...)` to the WHERE clause verbatim — it never passes through `TransformExternalSQLClause` or any other quoting step. MJ itself ships at least 12 of these rows, all with bare PascalCase identifiers:
| Filter | FilterText shape |
|---|---|
| UI: Own AI Agent Runs | `UserID = '{{UserID}}'` |
| UI: Own AI Agent Run Steps | `AgentRunID IN (SELECT ID FROM __mj."vwAIAgentRuns" WHERE UserID = '{{UserID}}')` |
| UI: Own AI Prompt Runs | same shape as above |
| UI: Own Identity Claims (v6) | `[ClaimedByUserID] = '{{UserID}}' OR [NormalizedEmail] = LOWER(LTRIM(RTRIM('{{UserEmail}}')))` |
| Magic Link: Own Rows by UserID | `UserID = '{{UserID}}'` |
| Magic Link: Own Application Roles | `RoleID IN (SELECT RoleID FROM __mj.vwUserRoles WHERE UserID = '{{UserID}}')` |
| Widget Guest: Own Conversations | `ExternalID = '{{ScopeResourceID}}'` |
| Widget Guest: Own Conversation Details / Agent Sessions / Agent Runs | `ConversationID IN (SELECT ID FROM __mj.vwConversations WHERE ExternalID = ...)` |
| Widget Guest: Own Agent Session Channels | nested `vwAIAgentSessions` / `vwConversations` subqueries |
| Widget Guest: Widget-Pinned Agents | `ID IN (SELECT PinnedAgentID FROM __mj.vwConversationWidgetInstances WHERE Status = 'Active' ...)` |
| Widget Guest run entities (v6, `V202608300200`) | nested `vwAIAgentRunSteps` / `vwAIAgentRuns` / `vwConversations` subqueries |
Without the shim, every UI-role read of agent runs / run steps / prompt runs, every magic-link-scoped read, and every widget-guest read fails with e.g. `column "userid" does not exist`. It fails closed (an error, not a data leak), but it is an outage for every non-admin user on PostgreSQL. These rows already exist in every PostgreSQL database, so they need a data migration, not just a code release.
Observations:
- The first three rows in the PG baseline quote the view (`__mj."vwAIAgentRuns"`) but leave `UserID` bare — the conversion was already relying on the shim for columns.
- The Identity Claims filter uses `[bracket]` identifiers. RLS text never goes through the bracket-to-double-quote conversion, and the shim passes brackets through verbatim, so **this filter is likely broken on PostgreSQL today regardless of this issue.** (Not verified live.)
- A per-platform mechanism already exists and is unused: `RowLevelSecurityFilterInfo.GetPlatformFilterText(platform)` (`packages/MJCore/src/generic/securityInfo.ts:539`) resolves `PlatformVariants` for `FilterText`, but has **zero callers** — `MarkupFilterText` always uses the base `FilterText`.
**2. Saved view `WhereClause` — customer data.**
`GenericDatabaseProvider.ts:1862-1877` inserts the rendered view WHERE clause as `(${renderedWhere})` with no transformation. This covers:
- **Smart Filters** — `SmartFilterWhereClause` is written by an AI prompt with no knowledge of the active dialect.
- **Custom WHERE clauses** (`CustomWhereClause = 1`) — hand-written by admins.
- **View-designer filters** generated from `FilterState` by `MJUserViewEntityExtended.convertFilterToSQL` (`packages/MJCoreEntities/src/custom/MJUserViewEntityExtended.ts:727`), which emits `[Field] = 'x'` and booleans as `= 1` / `= 0`. Since brackets pass through the shim verbatim and boolean coercion only happens in `TransformExternalSQLClause`, **these views are likely broken on PostgreSQL today with or without the shim** — a pre-existing bug independent of this issue.
**3. Cross-entity subqueries inside `ExtraFilter`.**
`ExtraFilter` and `OrderBy` do go through `TransformExternalSQLClause` → `quoteIdentifiersInSQL` (`PostgreSQLDataProvider.ts:1016`), but that quoter only knows **the main entity's own field list**. Any other view name or column in a subquery is left for the shim. Example: `TypeID IN (SELECT ID FROM __mj.vwAIAgentTypes WHERE Name='Flow')` — `vwAIAgentTypes` is not a field of the main entity, so only the shim quotes it. So the earlier assumption that browser-authored filters don't depend on the shim holds only for single-entity filters.
Confirmed sites from a single-line scan (multi-line filters not yet counted, so this is a floor):
- `packages/Angular/Explorer/dashboards/src/Workflows/workflows-dashboard.component.ts:84`
- `packages/Angular/Explorer/dashboards/src/Workflows/components/workflow-runs-resource.component.ts:783`
- `packages/Angular/Explorer/dashboards/src/AI/components/autotagging/dialogs/source-type-form.dialog.component.ts:714`
- `packages/Angular/Explorer/dashboards/src/AI/components/tags/tags-resource.component.ts:1034`
- `packages/Angular/Generic/task-graph-editor/src/lib/task-graph-run-view.component.ts:562`
- `packages/Angular/Generic/flow-editor/src/lib/agent-editor/flow-agent-editor.component.ts:187`
- `packages/AI/Vectors/Dupe/src/duplicateRecordDetector.ts:772`
- `packages/AI/MCPClient/src/MCPClientManager.ts:2119`
- `packages/Actions/CoreActions/src/custom/user-management/check-user-permission.action.ts:67`
- `packages/ContentAutotagging/src/Entity/generic/AutotagEntity.ts:354`
**4. Smaller items.**
- `OverrideExcludeFilter` (RunView param) is never transformed.
- The `ExcludeUserViewRunID` subquery at `GenericDatabaseProvider.ts:1922` uses bare `RecordID` / `EntityID` / `UserViewID` (code — easy fix).
- `forceRegeneration.entityWhereClause` in customer `mj.config.cjs` files (CodeGen).
- Stored `MJ: Queries` bodies in customer databases.
### B. SQL written in code (mechanical)
From an earlier measurement pass (not re-verified in this investigation — treat as a floor, since dynamically concatenated SQL was undercounted):
| Surface | Count |
|---|---|
| Shipped `metadata/queries/SQL/**/*.pg.sql` bodies that change under the tokenizer | 3 of 10, all `external-change-detection/` |
| Server-side TS SQL template literals that change under it | 34 across 16 files |
| — of which in `packages/CodeGenLib/src/Database/manage-metadata.ts` | 16 |
Known concentrations: `manage-metadata.ts` (mixes `qi()`/`qs()` and raw identifiers on adjacent lines — a half-finished migration), the three `external-change-detection/*.pg.sql` bodies, `memory-manager-agent.ts`, `packages/ExternalChangeDetection/src/ChangeDetector.ts`. The migration target already exists: `dialect.QuoteIdentifier()` (~736 call sites) and `qi()` / `qs()` in CodeGenLib (~279).
---
## Direction for each breaking point
The principle: whoever writes the SQL knows (or can be told) the dialect, so quoting belongs at authoring time.
| Breaking point | Fix |
|---|---|
| **Customer-authored RLS filters, custom WHERE clauses, stored queries** | Authors write for their environment's dialect — a developer configuring RLS on a PostgreSQL instance writes PostgreSQL. Needs a one-time data fix for existing rows in PostgreSQL databases before they upgrade. |
| **MJ-shipped RLS filters** (must work on both platforms) | Either write them with ANSI double-quoted identifiers, which both dialects accept (SQL Server accepts `"UserID"` when `QUOTED_IDENTIFIER` is ON; tedious defaults `enableQuotedIdentifier: true` and MJ does not override it — verified from the driver source, not yet exercised live), or use `PlatformVariants` and wire `GetPlatformFilterText()` into `MarkupFilterText`. |
| **Smart Filters** | Pass the environment's SQL dialect into the Smart Filter prompt so the AI emits correctly quoted SQL. |
| **View designer (`FilterState` → `WhereClause`)** | Generate through the dialect (`QuoteIdentifier`, dialect boolean literals) instead of hard-coded `[brackets]` and `= 1`. Also fixes the pre-existing PostgreSQL bug above. |
| **Cross-entity `ExtraFilter` subqueries in code** | Quote explicitly via the provider/dialect helpers (`QuoteSchemaAndView`, `QuoteIdentifier`). |
| **Code-written SQL (section B)** | Migrate to `dialect.QuoteIdentifier()` / `qi()` / `qs()`, preferring each file's existing helper. |
| **`forceRegeneration.entityWhereClause`** | Document that it is written in the target database's dialect. |
---
## Proposed sequence
- [ ] **Instrument first.** At both `ExecuteSQL` and `quoteSQLForExecution`, log only when the tokenizer actually changes a statement: original statement, rewritten tokens, caller. Keep it in place for at least a release. This turns "what might break" into a list — including the stored rows in each PostgreSQL database (RLS filters, saved views, queries) that need fixing before they can take the removal.
- [ ] Widen the static inventory beyond template literals: string concatenation, `+=`, array `.join()`, helpers that return SQL fragments, multi-line `ExtraFilter`s.
- [ ] Fix code-written SQL (section B) and the cross-entity `ExtraFilter` sites.
- [ ] Make runtime authoring dialect-aware: view designer, Smart Filter prompt, shipped RLS filters (ANSI quoting or `PlatformVariants` + `GetPlatformFilterText`).
- [ ] Data fix for existing PostgreSQL databases: RLS filters, saved views, stored queries — driven by the instrumentation output.
- [ ] Remove the shim from `ExecuteSQL` and from CodeGen's `qsql()`. Keep the module and tests briefly for bisection, then delete them.
- [ ] Close #3775, #3604 and #3792 as obsolete.
## Verification when removing
The existing unit tests were green through both silent-corruption incidents, so they are not sufficient on their own. At minimum:
- Full monorepo build and unit suite
- `pnpm run test:integration` against PostgreSQL
- **CodeGen end-to-end against a real PostgreSQL database, diffing generated artifacts (including the `CodeGen_Run_*.sql` log) before and after — they should be byte-identical**
- Schema round-trip: migrations → CodeGen → exercise generated SPs/views → verify data
- Exercise the paths the shim was added for: Memory Manager scheduled job, external change detection, `TestRun` column lookups
- Read paths as a **UI-role user** and a **widget guest** (RLS), and saved views including Smart Filter views
- Re-run every shipped `.pg.sql` query body and compare results before/after
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Contributor guide
Research direction
Start with PostgreSQLDataProvider.ExecuteSQL, PostgreSQLCodeGenProvider.quoteSQLForExecution, and the shipped-queries regression test to understand where rewriting occurs. Then trace GenericDatabaseProvider row filters and view WhereClause handling, plus the listed dialect helpers and call sites. Done means the runtime shim's dependencies are addressed and its removal preserves the documented PostgreSQL behavior.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- postgresql, sql, typescript
- Domain
- backend, databases, devtools
- Issue type
- Refactor
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Active
- Clarity
- Mostly clear
- Newbie friendliness
- 35/100