MemberJunction / MemberJunction/MJ

Hand-written resolvers bypass provider.ExecuteSQL, breaking dialect portability under PostgreSQL

Open
#3,427 0 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

Several hand-written (non-CodeGen-generated) resolvers in `packages/MJServer/src/resolvers` execute raw SQL by constructing `new mssql.Request(dataSource)` / `new sql.Request(dataSource)` directly against `AppContext.dataSource`, instead of going through the dialect-portable `provider.ExecuteSQL(...)` abstraction. This works against SQL Server but is expected to fail (or silently misbehave) against a PostgreSQL-backed MJ instance.

## Why this breaks under Postgres

`AppContext.dataSource` is typed as `sql.ConnectionPool` (`packages/MJServer/src/types.ts:37`), but that type doesn't hold for the Postgres path. When `dbType === 'postgresql'`, `dataSource` is actually `createMSSQLCompatPool(pgPool)` (`packages/MJServer/src/index.ts:1712-1729`) — a plain duck-typed object (`{connected, query, request, _pgPool}`) cast `as unknown as sql.ConnectionPool`. It is **not** a real instance of the `mssql` npm package's `ConnectionPool` class.

Code that does `new mssql.Request(dataSource)` (or `new sql.Request(dataSource)`) then runs through the real `mssql` package's internals, which expect an actual driver-backed pool — not this compatibility shim. Under Postgres this construction is expected to fail. Separately, `mssql`-specific type tokens (e.g. `mssql.UniqueIdentifier`) have no Postgres equivalent.

## Known instances

- `packages/MJServer/src/resolvers/ReportResolver.ts` (`CreateReportFromConversationDetailID`) — `new mssql.Request(dataSource)`
- `packages/MJServer/src/resolvers/AdhocQueryResolver.ts` (`runSqlWithDeadline`) — `new sql.Request(ds).query(sqlText)`
- `packages/MJServer/src/resolvers/GetDataResolver.ts` — `new sql.Request(readOnlyDataSource)`, notably even after already computing a dialect (`platform = getDbType()`) and rendering dialect-aware SQL text via `compositionEngine.ResolveComposition(query, platform, ...)` — the dialect-awareness is applied to SQL *generation* but not to *execution*
- `packages/MJServer/src/util.ts` (`extendConnectionPoolWithQuery`, ~line 229) — the framework's own "backwards-compatibility" shim for `AppContext.dataSource` internally does `new sql.Request(pool)` too, so it reintroduces the same assumption for any caller that relies on it

## The portable alternative that already exists

`provider.ExecuteSQL(sql, params, ...)` — the abstract method on `DatabaseProviderBase` (`packages/MJCore/src/generic/databaseProviderBase.ts:129`), implemented separately by `SQLServerDataProvider` and `PostgreSQLDataProvider` (shared logic in `packages/GenericDatabaseProvider/src/GenericDatabaseProvider.ts`). This is what CodeGen-generated resolver code already uses everywhere (e.g. `packages/MJServer/src/generated/generated.ts:233, 401, 588`).

Each of the resolvers above already has (or could easily obtain) a `DatabaseProviderBase` via `GetReadOnlyProvider`/`GetReadWriteProvider(providers)` from `AppContext`, and could call `provider.ExecuteSQL(...)` instead of constructing a raw `mssql`/`sql` `Request`.

## Suggested fix

Audit `packages/MJServer/src/resolvers/**` and `packages/MJServer/src/util.ts` for `new mssql.Request(` / `new sql.Request(` call sites and migrate each to `provider.ExecuteSQL(...)`, parameterizing values the same way CodeGen-generated code does. Add a regression test (or lint rule) that fails the build if a hand-written resolver imports `mssql`/`sql` directly and constructs a `Request` outside of the shared provider layer.

## Scope note

This is a portability/compatibility gap, not a vulnerability — surfaced incidentally while reviewing an unrelated SQL-parameterization fix. Filing separately since the fix touches execution plumbing across several files rather than a single targeted change.

Contributor guide

Open the contributing guide

Research direction

Audit the Request call sites named in packages/MJServer/src/resolvers and packages/MJServer/src/util.ts, then compare them with provider.ExecuteSQL in packages/MJCore/src/generic/databaseProviderBase.ts and generated resolver usage. Replace the direct constructions across the listed paths, preserving parameterization, and add the proposed regression test or lint rule so hand-written resolvers cannot reintroduce them.

Written by the indexing model from the issue text.

Assessment

Tech stack
postgresql, sql, typescript
Domain
api, backend, databases
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.