MemberJunction / MemberJunction/MJ

QueryPagingEngine.outerWrap hoists an alias-qualified ORDER BY out of scope, producing invalid SQL

Open Beginner friendly
#3,778 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

`QueryPagingEngine.outerWrap` enforces `MaxRows` by wrapping a query in a derived table. Because `ORDER BY` is illegal inside a derived table on SQL Server, it correctly strips the top-level `ORDER BY` from the inner query and re-attaches it to the outer `SELECT` — but it re-attaches it **verbatim, keeping the table-alias qualifier**. The alias only exists inside the derived table, so the resulting SQL cannot bind.

## Reproduction

Input query (valid, returns 21 rows on its own):

```sql
SELECT nl.NeoGovTrainingRecordID, nl.Completion_Date, ...
FROM [dbo].[vwEmployees] e
JOIN [dbo].[vwNeoGovLearns] nl ON LOWER(LTRIM(RTRIM(CASE WHEN TRY_CAST(...) ...)))
WHERE e.EmployeeID = 16615
ORDER BY nl.Completion_Date DESC
```

What `outerWrap` produces:

```sql
SELECT TOP 10 * FROM (
SELECT nl.NeoGovTrainingRecordID, nl.Completion_Date, ...
FROM [dbo].[vwEmployees] e JOIN [dbo].[vwNeoGovLearns] nl ON ...
) AS _mj_capped
ORDER BY nl.Completion_Date DESC -- `nl` does not exist at this level
```

Result:

```
The multi-part identifier "nl.Completion_Date" could not be bound.
```

Verified against a live database:

| Form | Result |
|---|---|
| as `outerWrap` emits it | **FAIL** — `"nl.Completion_Date" could not be bound` |
| identical, qualifier stripped (`ORDER BY Completion_Date`) | **OK — 10 rows** |

## When it fires

Only on the `outerWrap` fallback path. Most queries take the AST path, which injects `TOP`/`LIMIT` inline and never wraps. `outerWrap` is reached when the AST cannot parse the SQL — and the code comment names the culprits directly (`TRY_CAST`, `IIF`, `STRING_AGG`). So the trigger is:

**alias-qualified top-level `ORDER BY` + SQL the AST cannot parse.**

A query without `ORDER BY` passes; the same query with one fails.

## Impact

The failure surfaces to callers as a SQL error on a query that is actually valid. In our case the caller is an LLM-driven query-writing loop: it sees `"nl.Completion_Date" could not be bound` next to its own SQL, where `nl` is plainly in scope, so it has no way to diagnose the problem and burns its entire retry budget making cosmetic edits. The wrapper is never visible to it.

Note that `TestQuerySQLResolver` returns `RenderedSQL`, which would make this diagnosable — callers that don't surface it are flying blind.

## Suggested fix

When hoisting the `ORDER BY` to the outer `SELECT`, strip the table-alias qualifiers. The derived table exposes columns by their output name, so `ORDER BY Completion_Date` binds correctly.

One caveat worth handling: if an `ORDER BY` column is not in the `SELECT` list it won't exist in the derived table, and stripping the qualifier still won't bind. That case needs either skipping the wrap or dropping the `ORDER BY`. It fails 100% today either way, so qualifier-stripping is strictly an improvement.

## Location

`packages/GenericDatabaseProvider/src/queryPagingEngine.ts` — `QueryPagingEngine.outerWrap`

Contributor guide

Open the contributing guide

Research direction

Start in packages/GenericDatabaseProvider/src/queryPagingEngine.ts at QueryPagingEngine.outerWrap and reproduce the supplied SQL Server case with an alias-qualified ORDER BY on the fallback path. Check how the top-level ORDER BY is extracted and reattached, then verify that the wrapped query binds the ordering column and returns the capped rows without the multi-part identifier error.

Written by the indexing model from the issue text.

Assessment

Tech stack
sql, typescript
Domain
databases
Issue type
Bug
Difficulty
2/5
Estimated time
1-3 hours
Activity status
Quiet
Clarity
Clearly specified
Newbie friendliness
75/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.