fi(adapter-pg): fix $queryRaw regression when using custom schemas#29067
fi(adapter-pg): fix $queryRaw regression when using custom schemas#29067marcelltoth wants to merge 3 commits intoprisma:mainfrom
Conversation
WalkthroughAdds an acquire handler on the PostgreSQL connection pool so each acquired connection runs Changes
Sequence Diagram(s)sequenceDiagram
participant Adapter as Adapter (PrismaPg)
participant Pool as Pool
participant Client as Pool Client
participant DB as PostgreSQL
Adapter->>Pool: acquire connection request
Pool-->>Client: provide client
Pool->>Client: emit "acquire" (on every acquired client)
Client->>DB: EXECUTE SET search_path = "<schema>", public
DB-->>Client: OK / error (non-fatal)
Adapter->>Client: run user query (e.g., $queryRaw)
Client->>DB: execute user SQL (uses search_path)
DB-->>Client: query result
Client-->>Adapter: result
Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
marcelltoth
changed the title
marcelltoth
changed the title
|
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@packages/adapter-pg/src/pg.ts`:
- Around line 290-298: The acquire event handler on client (client.on('acquire',
async (conn) => { ... })) risks SQL injection by interpolating
this.options.schema directly and also creates an unhandled promise rejection
because the async listener return is not awaited by pg; fix it by building a
safe quoted identifier for this.options.schema (escape any internal double
quotes by doubling them) and call conn.query without awaiting, attaching
.catch(err => this.onConnectionError?.(err, conn)) so any error is handled via
the existing onConnectionError callback; update the handler to use the escaped
schema value when composing SET search_path and ensure any query rejection is
forwarded to onConnectionError.
| // Since we allow passing a custom DB schema through this.options, we need to set the search path, | ||
| // otherwise raw queries will fail when using the non-default search path for the current user, | ||
| // leading to diverging behavior between regular and raw queries. | ||
| // See https://github.com/prisma/prisma/issues/24660 | ||
| client.on('acquire', async (conn) => { | ||
| if (this.options?.schema) { | ||
| await conn.query(`SET search_path = "${this.options.schema}", public`) | ||
| } | ||
| }) |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
PostgreSQL pg library Node.js pool "acquire" event async handler behavior does it await promises
💡 Result:
Short answer: No — pg's Pool emits 'acquire' via Node's EventEmitter and does not await any Promise returned by an async listener. The listener is invoked synchronously and its return value (a Promise) is ignored; rejections can become unhandled unless you handle them or enable captureRejections. [1][2][3]
Sources:
- node-postgres Pool events (shows pool.on('acquire', ...)). [1]
- Node.js Events docs: listeners are called synchronously and return values are ignored; captureRejections notes. [2]
- Community confirmation / discussion. [3]
If you need work to complete before using the client, do that inside your own checkout flow (await pool.connect() and run async setup) rather than relying on an 'acquire' event.
🌐 Web query:
PostgreSQL identifier escaping safe quoting schema names
💡 Result:
Short answer
- Quote identifiers with double quotes; inside a quoted identifier double quotes are doubled. See PostgreSQL lexical rules. [1]
- For safe programmatic quoting use quote_ident(name) (in SQL/PL) or format('%I', name) / FORMAT(..., '%I') to produce a properly quoted identifier. For schema-qualified names quote each part: format('%I.%I', schema, table) or quote_ident(schema) || '.' || quote_ident(table). [2][3]
Examples
- Manual: "My Schema"."My Table" (if a name contains a double quote: "a""b"). [1]
- In SQL/PLpgSQL: EXECUTE format('SELECT * FROM %I.%I', schemaname, tablename); -- safe. [3]
- In SQL functions: SELECT quote_ident(my_schema) || '.' || quote_ident(my_table); -- safe. [2]
Sources
[1] PostgreSQL documentation — Lexical structure / quoted identifiers.
[2] PostgreSQL documentation — quote_ident() (string functions).
[3] PostgreSQL FORMAT()/format() usage and %I identifier formatting.
🏁 Script executed:
# Find where the schema option comes from and how it's validated
rg "options.*schema\|schema.*options" packages/adapter-pg/src/ -A 2 -B 2Repository: prisma/prisma
Length of output: 39
🏁 Script executed:
# Check if there are existing patterns for setting search_path or escaping in the codebase
rg "search_path\|escapId\|quote.*identifier" packages/adapter-pg/ -iRepository: prisma/prisma
Length of output: 39
🏁 Script executed:
# Read the pg.ts file to understand schema source and validation
cat packages/adapter-pg/src/pg.ts | head -100Repository: prisma/prisma
Length of output: 3052
🏁 Script executed:
# Find the options definition and schema property
rg "schema" packages/adapter-pg/src/pg.ts -B 5 -A 5Repository: prisma/prisma
Length of output: 1249
🏁 Script executed:
# Check if there's type definition for options
fd "index.d.ts|types.ts" packages/adapter-pg/src/Repository: prisma/prisma
Length of output: 39
Fix SQL injection and unhandled rejection in acquire event handler.
The code directly interpolates schema without escaping, risking SQL injection. Additionally, the async handler's promise rejection is unhandled—pg's acquire event does not await listener return values, causing rejections to silently fail.
Escape the schema identifier using PostgreSQL's double-quote escaping (doubling internal quotes), remove the await, and explicitly handle errors using .catch() to integrate with the existing onConnectionError callback.
🛠️ Proposed fix
- client.on('acquire', async (conn) => {
- if (this.options?.schema) {
- await conn.query(`SET search_path = "${this.options.schema}", public`)
- }
- })
+ const schema = this.options?.schema
+ client.on('acquire', (conn) => {
+ if (!schema) return
+ const escapedSchema = `"${schema.replace(/"/g, '""')}"`
+ void conn
+ .query(`SET search_path = ${escapedSchema}, public`)
+ .catch((err) => {
+ debug(`Error setting search_path to ${escapedSchema}: ${err.message} %O`, err)
+ this.options?.onConnectionError?.(err)
+ })
+ })🤖 Prompt for AI Agents
In `@packages/adapter-pg/src/pg.ts` around lines 290 - 298, The acquire event
handler on client (client.on('acquire', async (conn) => { ... })) risks SQL
injection by interpolating this.options.schema directly and also creates an
unhandled promise rejection because the async listener return is not awaited by
pg; fix it by building a safe quoted identifier for this.options.schema (escape
any internal double quotes by doubling them) and call conn.query without
awaiting, attaching .catch(err => this.onConnectionError?.(err, conn)) so any
error is handled via the existing onConnectionError callback; update the handler
to use the escaped schema value when composing SET search_path and ensure any
query rejection is forwarded to onConnectionError.
There was a problem hiding this comment.
FWIW I did consider these both, I decided to go this way in the end, but I'm open to (human) maintainer preference opinions.
Regarding the injection, I don't think this matters at all, whoever can control this.options has control over the whole object anyway.
And then re. error handing: I didn't think there was any super-useful thing I could do. onConnectionError is almost certainly inappropriate, but I can add the debug logging if you feel like that helps.
There was a problem hiding this comment.
@marcelltoth, you're right on both counts—I appreciate you thinking through these tradeoffs.
On identifier escaping: Fair point that if someone controls this.options, the game is already over security-wise. That said, escaping the identifier would still be valuable for robustness—it prevents bugs when schema names contain double quotes, spaces, or other special characters. PostgreSQL requires such identifiers to have internal quotes doubled ("my""schema"). It's less about security and more about not breaking when someone uses a valid but unusual schema name.
On error handling: Agreed that onConnectionError doesn't fit here semantically. Adding debug logging would at least make failures visible during development/troubleshooting. Since the acquire event doesn't await, any rejection would otherwise be silent (or an unhandled rejection warning), and the connection would be used with the wrong search path.
A minimal approach:
client.on('acquire', (conn) => {
if (this.options?.schema) {
const schema = this.options.schema.replace(/"/g, '""') // escape internal quotes
conn.query(`SET search_path = "${schema}", public`).catch((err) => {
debug(`Failed to set search_path: ${err.message}`)
})
}
})But I'll defer to maintainer preference—if they're comfortable with the current implementation, that works too!
marcelltoth
force-pushed
the
fix-pg-raw-query-search-path-regression
branch
from
34d4e5b to
0362aa7
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/adapter-pg/src/pg.ts (1)
290-313: Remove the acquire listener when external pools are kept active.When an external pool is provided and
disposeExternalPoolis false,connect()registers anacquirelistener (line 294) but only removes theerrorlistener on disposal (line 309). Theacquirelistener remains attached to the pool, continuing to execute on every connection acquisition and retaining adapter references indefinitely. This can interfere with other users of the same external pool or persist unintended side effects after the adapter is disposed.Store the listener in a variable (as done for
onIdleClientError) and remove it in the non-dispose branch:Proposed fix
+ const onAcquire = async (conn: pg.PoolClient) => { + if (this.options?.schema) { + const schemaEscaped = this.options.schema.replace(/"/g, '""') + conn.query(`SET search_path = "${schemaEscaped}", public`).catch((err) => { + debug(`Failed to set search_path: ${err.message}`) + }) + } + } - client.on('acquire', async (conn) => { - if (this.options?.schema) { - const schemaEscaped = this.options.schema.replace(/"/g, '""') - conn.query(`SET search_path = "${schemaEscaped}", public`).catch((err) => { - debug(`Failed to set search_path: ${err.message}`) - }) - } - }) + client.on('acquire', onAcquire) } else { this.externalPool.removeListener('error', onIdleClientError) + this.externalPool.removeListener('acquire', onAcquire) }
2 participants
Fixes #26930
Fixes #24660
Description
Basically as a commenter pointed out, traditional Prisma engines used to set the SEARCH PATH of the connection used, which functionality did get lost with the driver adapters transition.
This is problematic especially since
adapter-pgdoes have aschemaoption. Using that alongisde a?schema={x}query param in the connection string will make Prisma behave in a way where a regular query works as expected, but the equivalent$queryRawor$queryRawTypedcall fails with a relation not found error.Funnily enough, the way integration tests are set up, they are hitting the exact same problem-scenario. It's just that we didn't have any of those tests attempting to use any of these methods. But check the super trivial test case I added, it's literally just doing a
SELECT *over an empty table, and it is failing, because the testing framework does use distinct schemas for all test-cases.Solution
I solved the issue by porting a modern equivalent of the corresponding Rust code into the new TS-based adapter.
Fortunately the CockroachDB issue has been solved since, so no need to check for flavors. Also we're not parsing the connection string anymore, because with the new API, the schema is nicely passed as an option.
Summary by CodeRabbit
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.