[kuldeepsinh19]

Summary

Fixes #2355.

A client derived with $setOptions (like every derived client) reuses the base client's ZenStackDriver. That driver adds logging by patching each pooled connection the first time it's acquired, only if the base client's log enables it, and always logs with the base client's Log. So $setOptions({ ...db.$options, log }) could not turn logging on, switch to another logger, or turn it off.

const db = new ZenStackClient(schema, { dialect });
const logged = db.$setOptions({ ...db.$options, log: ['query'] });
await logged.user.findMany(); // before: nothing logged

Changes

• zenstack-driver.ts
  • The pool-level state (underlying driver, init/destroy lifecycle, sqlite connection mutex, transaction bookkeeping) is kept in one shared state object. withLog(log) returns a driver over the same state with its own Log, so derived clients keep sharing one connection pool.
  • When a driver acquires a connection, it records its log for that connection. Kysely drivers hand out a connection to one holder at a time until it's released, so queries on it are logged with the log of the client that acquired it.
  • A connection is patched once, the first time a driver with logging enabled acquires it (previously a connection first acquired without logging was never patched). The patched methods read the connection's current log on each query and skip timing and logging when nothing is enabled.
• client-impl.ts: a derived client whose log option differs from its base (identity check) uses withLog(new Log(options.log ?? [])) for its executor and its $qbRaw Kysely instance. $use, $setAuth, $transaction and similar keep the same log reference, so they keep the base driver as before.

Known limitation: calling $setOptions with a different log on a client inside an interactive transaction still logs that transaction's queries with the logger that started it, because those queries run on the connection the transaction already holds.

Overlap with #2899: both PRs change the connection provider argument in the derived-client branch of client-impl.ts. Whichever lands second can combine them as baseClient.isTransaction ? (baseClient.transactionConnection ?? baseClient.$qb.getExecutor()) : new DefaultConnectionProvider(driver), with the executor taking driver.

Validation

• New tests/regression/test/issue-2355.test.ts covers:

  • enabling, switching and disabling logging with $setOptions
  • logging on transactions started from the derived client and on $qbRaw
  • failed queries logged with level error
  • concurrent queries from a base and a derived client going to their own loggers
  • clients derived without changing log keeping the base logger

  Without the change, 7 of the 8 tests fail in 3 of 3 runs; with it, all 8 pass in 3 of 3 runs.

• tests/e2e ORM suites that create derived clients and use the driver (client-api/transaction, raw-query, slicing, omit, error-handling, plugin-infra/*): 183 passed before and after. packages/orm unit tests: 7 passed before and after.

• @zenstackhq/orm lint and build (tsc + tsdown), tsc for tests/regression and prettier on the changed files pass.

• Ran on SQLite only (Node 24, pnpm 10.33.0, in a container). The PostgreSQL and MySQL legs of the CI matrix were not run locally.

Review notes

• Before opening this PR, the diff, the issue and the test results went through an independent review. It found no blockers. Changes made from it:
  • The error test uses a unique-constraint violation instead of a quoted raw query, so it is portable to MySQL and PostgreSQL.
  • Setup queries from createTestClient are cleared before assertions.
  • Tests were added for $qbRaw and for concurrent base and derived queries.
  • withLog reuses the shared state instead of building a throwaway one.
  • The transaction-client limitation and the fix(orm): clients derived from a transaction client use their own plugins and auth #2899 overlap are documented above.
• CodeRabbit pointed out that pg-pool can hand a released connection to a queued acquisition before releaseConnection returns. That acquisition can run queries on it (for example a dialect's onReserveConnection hook) while the previous holder's log is still recorded for the connection. This is fixed in 07264a9: the recorded log is now cleared before the connection is released. After that commit:
  • the regression tests pass (8 of 8, 3 of 3 runs);
  • the e2e ORM suites listed above pass (183 of 183) and the packages/orm unit tests pass (7 of 7);
  • lint and prettier pass.
• The workflow runs are waiting for maintainer approval (first-time contributor).

This change was written with help from an AI coding assistant (Claude Code).

Summary by CodeRabbit

• Bug Fixes
  • Query logging now respects logging options configured on derived clients, including when logging is enabled or disabled. Queries, transactions, raw queries, and failed operations are logged by the client that runs them, with concurrent queries kept separate.
  • Derived clients that retain the base client’s logging options continue to share its logging behavior.
  • Logged queries and streams include their execution duration.

[coderabbitai]

No actionable comments were generated in the recent review.

Walkthrough

Derived clients now use their own logger when their log option differs from the base client’s. They share driver state and connections. Regression tests cover logging across queries, transactions, errors, and concurrent base and derived client operations.

Changes: Derived-client logging

Layer / File(s)	Summary
Shared driver state packages/orm/src/client/executor/zenstack-driver.ts	ZenStackDriver stores driver, lifecycle, connection, and transaction state in shared state that logger-specific driver instances can reuse.
Connection-level logging packages/orm/src/client/executor/zenstack-driver.ts	Connection acquisition associates the current driver’s logger with the connection. Query and stream wrappers use that logger to report errors and successful operations with duration data.
Derived-client driver selection packages/orm/src/client/client-impl.ts	When the log option differs from the base client’s, the derived client uses a logger-specific driver and raw Kysely instance. Otherwise, it reuses the base instances.
Logging regression coverage tests/regression/test/issue-2355.test.ts	Tests cover log configuration, base and derived client event routing, transactions, errors, raw query builders, and concurrent queries.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix · Severity of issue fixed: Low

Possibly related PRs

• zenstackhq/zenstack#2899: Both changes affect derived-client executors and connection routing, including transaction clients.

Suggested reviewers: ymc9

Merge Risk: Minimal · up to 07264

Derived clients select their own query logger while retaining shared connections. The previous logger-handoff issue is corrected; no merge-blocking issue remains identified.

Pre-merge checks: 4 · 1

Failed checks: 1 warning

Check name	Status	Explanation	Resolution
Docstring Coverage		Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentat…	Write docstrings for the functions missing them to satisfy the coverage threshold.

Passed checks: 4 passed

Check name	Status	Explanation
Description Check		Check skipped - CodeRabbit’s high-level summary is enabled.
Title check		The title clearly and concisely identifies the main change: making the log option effective for clients derived with $setOptions.
Linked Issues check		Issue #2355 requires $setOptions to make logging effective when it enables, switches, or disables logging. At the reviewed head, packages/orm/src/client/client-impl.ts compares options.log with …
Out of Scope Changes check		All changed production code serves the #2355 requirement. zenstack-driver.ts changes are: the shared ZenStackDriverState (needed so a derived driver can use a different Log without a second pool…

Full details: Linked Issues check

Explanation

Issue #2355 requires $setOptions to make logging effective when it enables, switches, or disables logging. At the reviewed head, packages/orm/src/client/client-impl.ts compares options.log with baseClient.$options.log by identity. When the values differ, the derived client calls ZenStackDriver.withLog(new Log(options.log ?? [])). This driver is used for the derived ZenStackQueryExecutor and for the $qbRaw Kysely instance. In zenstack-driver.ts, withLog shares the pool-level state (ZenStackDriverState), so derived clients keep one connection pool. acquireConnection records the acquiring driver's log per connection (connectionLogs). It patches a connection once, when a driver with logging enabled first acquires it (loggedConnections). The patched methods read the connection's current log on each query and skip logging when no level is enabled. This covers enabling, switching, and disabling logging. Transaction and $qbRaw paths use the same executor or driver, so they are covered by the same mechanism. releaseConnection clears the recorded log before release, which covers the pg-pool handoff case. The PR reports 8 passing regression tests in tests/regression/test/issue-2355.test.ts, covering these paths. I did not read the test file directly. The reported $setOptions call path was not read in this review. The known limitation (an interactive transaction keeps the logger that started it) is outside the stated requirement and does not change this result. No non-coding requirement exists in the linked issue.

Full details: Out of Scope Changes check

Explanation

All changed production code serves the #2355 requirement. zenstack-driver.ts changes are: the shared ZenStackDriverState (needed so a derived driver can use a different Log without a second pool), per-connection log tracking, one-time patching on first logging-enabled acquisition, per-query log lookup, and clearing the recorded log on release. The moved query-logging helpers (addLogging, logQuery, logError, needsLogging) implement the same logging behavior with the connection's current log. client-impl.ts changes apply the new driver only when the log identity changes. Other derived clients ($use, $setAuth, $transaction) keep the base driver. The new ZenStackDriverState type and the optional sharedState constructor parameter are internal support for this change. The added regression test file covers only #2355 behavior. The connectionLogs.delete in releaseConnection and the stream-path early yield* are direct parts of the logging change. No unrelated feature or unrelated test change is shown in the reviewed diff.

Full details: Docstring Coverage

Explanation

Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 2 functions across 3 files.

Resolution

Write docstrings for the functions missing them to satisfy the coverage threshold.

Finishing Touches

Generate unit tests (beta)

• Create a new PR

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Warning

Some tools did not complete. Review the errors below.

ESLint

If the error stems from missing dependencies, add them to the package.json file. For unrecoverable errors (e.g., due to private dependencies), disable the tool in the CodeRabbit configuration.

packages/orm/src/client/executor/zenstack-driver.ts

ESLint skipped: missing config or dependency (missing-dependency). The ESLint configuration references a package that is not available in the sandbox.

Tip

🎯 Customize your risk assessments

Guide how CodeRabbit assesses merge risk and security architecture risk. Highlight sensitive changes and explain your security boundaries using Merge Risk Instructions and Security Risk Instructions in your review settings. Security architecture risk assessments require the Advanced plan or above.

✅ Automatic approvals, with your rules

Let CodeRabbit approve pull requests once required threads are resolved, pre-merge checks pass, and your risk limits are met. Enable Automatic approvals in review settings, with overrides for teams or file paths.

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.

Share

• X
• Mastodon
• Reddit
• LinkedIn

CodeRabbit Verify

• Run CodeRabbit Verify to test the changes in this pull request.

Recent review info

Run configuration

• Source mode: Auto
• Repository: zenstackhq/zenstack
• Branch: dev
• File: .coderabbit.yaml
• Commit: d44e489
• Layers: Repository YAML
• Review profile: CHILL
• Plan: Advanced
• Run ID: 993e0202-faa1-4e2d-8e11-dd07baedcf59

Commits

Reviewing files that changed from the base of the PR and between 5901ff8 and 07264a9.

Files selected for processing: 1

• packages/orm/src/client/executor/zenstack-driver.ts

Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 7 remain after this review.

[coderabbitai]

• Fix comments: Fix CodeRabbit comments on this PR