Remove TransactionLocal and SessionSearchPath modes; use QualifiedNames only - #88
Merged
Merged
Conversation
…; reduce SchemaSwitchingMode to QualifiedNames Non-transactional CompositeUnitOfWork roots no longer create/open a shared DbConnection. Contexts are built via the new BuildOwnedOptions/ApplyOwned path bound to the connection string, so EF Core rents a pooled connection per operation and returns it immediately. This removes hold-for-request connection pinning that inflated pool demand (Npgsql connect timeouts under long-polling load). Transactional roots keep the shared connection + transaction semantics unchanged. SchemaSwitchingMode now has a single member, QualifiedNames; the TransactionLocal and SessionSearchPath modes and all search_path manipulation are removed. SearchPathCommandInterceptor is replaced by QualifiedNamesCommandInterceptor(schema, currentSchema). NpgsqlAetherProvider is parameterless; AddAetherNpgsql keeps the optional mode parameter for signature compatibility and defaults to QualifiedNames. Tests updated for the new topology (owned connections, qualified-names DDL placeholder rewriting) and multi-schema/unit-of-work docs rewritten accordingly. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BKsqFuUQTnEJaJKygkEh2c
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
Reviewer's GuideConsolidates multi-schema support to a single QualifiedNames strategy, removing all search_path-based modes and interceptors, changing the Npgsql provider and UoW to support either a shared transactional connection or EF Core-owned pooled connections, and updating docs/tests to reflect the new behavior and non-transactional connection management. Sequence diagram for command execution with QualifiedNamesCommandInterceptorsequenceDiagram
participant CS as ICurrentSchema
participant Ctx as DbContext
participant INT as QualifiedNamesCommandInterceptor
participant PG as PostgreSQL
Ctx->>INT: ReaderExecuting(command,eventData,result)
INT->>CS: read Name
INT->>INT: ApplyQualifiedNames(command)
INT->>INT: PostgreSqlRawSchemaTokenRewriter.RewriteModelPlaceholder(commandText,quotedSchema)
INT->>INT: PostgreSqlRawSchemaTokenRewriter.Rewrite(commandText,quotedSchema)
INT-->>Ctx: InterceptionResult
Ctx->>PG: Execute command.CommandText
Ctx->>INT: NonQueryExecuting/ScalarExecuting(...)
INT->>CS: read Name
INT->>INT: ApplyQualifiedNames(command)
INT-->>Ctx: InterceptionResult
Ctx->>PG: Execute command.CommandText
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
📝 WalkthroughWalkthroughPostgreSQL multi-schema support now uses qualified relation names exclusively. The provider rewrites schema placeholders and runtime tokens, transactional and non-transactional Unit of Work connection handling is separated, and tests and documentation are updated accordingly. ChangesQualified-names multi-schema migration
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant CompositeUnitOfWork
participant AetherDbContextConfigurator
participant NpgsqlAetherProvider
participant QualifiedNamesCommandInterceptor
participant PostgreSQL
Caller->>CompositeUnitOfWork: resolve schema-bound DbContext
CompositeUnitOfWork->>AetherDbContextConfigurator: build transactional or owned options
AetherDbContextConfigurator->>NpgsqlAetherProvider: apply schema binding
NpgsqlAetherProvider->>QualifiedNamesCommandInterceptor: register bound schema and current-schema guard
Caller->>PostgreSQL: execute EF Core command
QualifiedNamesCommandInterceptor->>PostgreSQL: rewrite placeholders and execute qualified SQL
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
Not up to standards ⛔🔴 Issues
|
| Category | Results |
|---|---|
| Security | 1 critical |
🟢 Metrics 8 complexity · 4 duplication
Metric Results Complexity 8 Duplication 4
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 security issue, 5 other issues, and left some high level feedback:
Security issues:
- Detected a formatted string in a SQL statement. This could lead to SQL injection if variables in the SQL statement are not properly sanitized. Use a prepared statements instead. You can obtain a PreparedStatement using 'SqlCommand' and 'SqlParameter'. (link)
Fixed security issues:
- Detected a formatted string in a SQL statement. This could lead to SQL injection if variables in the SQL statement are not properly sanitized. Use a prepared statements instead. You can obtain a PreparedStatement using 'SqlCommand' and 'SqlParameter'. (link)
General comments:
- In
CompositeUnitOfWork.GetDbContextAsynctheMaxDbContextCountguardrail is now commented out; if this change is intentional it would be good either to remove it entirely or re-enable it, otherwise long-lived UoWs can silently grow unbounded numbers of contexts. - The
SchemaSwitchingMode modeparameter onAddAetherNpgsqlis kept for signature compatibility but any non-QualifiedNamesvalue is silently ignored; consider either validating and throwing for unsupported values or marking the parameter obsolete to avoid confusing callers who still try to select a mode. - The various
ArrangeSchemaAsynchelpers now duplicate the same string replacement logic onGenerateCreateScriptto inject the schema; consider extracting this into a shared utility to reduce repetition and keep the placeholder-rewrite behavior consistent across tests.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `CompositeUnitOfWork.GetDbContextAsync` the `MaxDbContextCount` guardrail is now commented out; if this change is intentional it would be good either to remove it entirely or re-enable it, otherwise long-lived UoWs can silently grow unbounded numbers of contexts.
- The `SchemaSwitchingMode mode` parameter on `AddAetherNpgsql` is kept for signature compatibility but any non-`QualifiedNames` value is silently ignored; consider either validating and throwing for unsupported values or marking the parameter obsolete to avoid confusing callers who still try to select a mode.
- The various `ArrangeSchemaAsync` helpers now duplicate the same string replacement logic on `GenerateCreateScript` to inject the schema; consider extracting this into a shared utility to reduce repetition and keep the placeholder-rewrite behavior consistent across tests.
## Individual Comments
### Comment 1
<location path="framework/src/BBT.Aether.Infrastructure/BBT/Aether/Uow/CompositeUnitOfWork.cs" line_range="166-168" />
<code_context>
- throw new InvalidOperationException(
- $"UnitOfWork DbContext limit exceeded. Limit: {_options.MaxDbContextCount}");
- }
+ // if (_contexts.Count >= _options.MaxDbContextCount)
+ // {
+ // throw new InvalidOperationException(
+ // $"UnitOfWork DbContext limit exceeded. Limit: {_options.MaxDbContextCount}");
+ // }
</code_context>
<issue_to_address>
**issue (performance):** Removing the DbContext count guard may allow unbounded context creation and increased resource usage.
With this guard removed, a single unit of work can now create an arbitrary number of DbContext instances. While transactional roots still share one connection/transaction, long‑lived scopes or unusual call patterns can significantly increase memory use and EF Core tracking overhead. If the previous limit was intended to prevent this, consider reintroducing it (possibly with a higher threshold) or adding another safeguard/monitoring mechanism so misuse is caught before it causes performance degradation.
</issue_to_address>
### Comment 2
<location path="framework/src/BBT.Aether.Npgsql/Microsoft/Extensions/DependencyInjection/AetherNpgsqlServiceCollectionExtensions.cs" line_range="36" />
<code_context>
this IServiceCollection services,
string connectionString,
- SchemaSwitchingMode mode = SchemaSwitchingMode.TransactionLocal,
+ SchemaSwitchingMode mode = SchemaSwitchingMode.QualifiedNames,
Action<IServiceProvider, DbContextOptionsBuilder>? configure = null)
where TDbContext : AetherDbContext<TDbContext>
</code_context>
<issue_to_address>
**issue (bug_risk):** The `mode` parameter is effectively ignored, which can hide misconfiguration.
Since `new NpgsqlAetherProvider()` ignores `mode` and `SchemaSwitchingMode` effectively only supports `QualifiedNames`, callers can still pass other values that are silently ignored. To prevent this, validate that `mode == SchemaSwitchingMode.QualifiedNames` and throw `ArgumentOutOfRangeException` (or log a warning) for other values, or mark the parameter as obsolete in a follow-up change.
</issue_to_address>
### Comment 3
<location path="framework/src/BBT.Aether.Infrastructure/BBT/Aether/Uow/EntityFrameworkCore/AetherDbContextConfigurator.cs" line_range="32-29" />
<code_context>
return builder.Options;
}
+
+ public DbContextOptions<TDbContext> BuildOwnedOptions(string schema)
+ {
+ var builder = new DbContextOptionsBuilder<TDbContext>();
+ configure(serviceProvider, builder);
+ provider.ApplyOwned(
+ builder,
+ connectionString,
+ schema,
+ serviceProvider.GetRequiredService<ICurrentSchema>());
+ return builder.Options;
+ }
}
</code_context>
<issue_to_address>
**suggestion (bug_risk):** The configuration order in `BuildOwnedOptions` differs from `BuildOptions`, which may lead to inconsistent behavior.
Here `configure` runs before `provider.ApplyOwned(...)`, while in `BuildOptions` it runs after `provider.ApplyShared(...)`. This difference can change how `configure` behaves (e.g., if it expects provider-specific settings to be applied first). Please align the call order between the two methods, or, if the difference is intentional, document it clearly in the configurator’s contract.
</issue_to_address>
### Comment 4
<location path="framework/docs/multi-schema/IMPLEMENTATION_NOTES.md" line_range="7" />
<code_context>
+> Earlier revisions described the `TransactionLocal` and `SessionSearchPath` switching modes
+> (`SET LOCAL search_path` / session `SET search_path` + `RESET search_path`), a session-level
+> interceptor (`NpgsqlSchemaConnectionInterceptor`), plus an `ICurrentSchema.Set()` /
+> `IsResolved` accessor. Those are gone: qualified names is the only strategy, and no
+> `search_path` manipulation happens anywhere. See the corrected design below.
</code_context>
<issue_to_address>
**nitpick (typo):** Adjust subject–verb agreement for “qualified names” or treat it as the proper mode name.
Consider either keeping it as a proper mode name (`QualifiedNames is the only strategy`) or using plural agreement (`qualified names are the only strategy`) to fix the grammar.
```suggestion
> `IsResolved` accessor. Those are gone: QualifiedNames is the only strategy, and no
```
</issue_to_address>
### Comment 5
<location path="framework/docs/multi-schema/IMPLEMENTATION_NOTES.md" line_range="86" />
<code_context>
-6. **PgBouncer-safe choices.** `TransactionLocal` never leaks because PostgreSQL reverts
- `SET LOCAL` with the transaction. `QualifiedNames` has no connection schema state at all.
- `SessionSearchPath` remains limited to a session-pinned/native connection.
+6. **Safe under any pooling.** Qualified names has no connection schema state at all — nothing
+ is ever written to session or transaction state — so it is safe under PgBouncer transaction
+ or session pooling as well as the native Npgsql pool. Non-transactional UoWs additionally
</code_context>
<issue_to_address>
**nitpick (typo):** Fix subject–verb agreement in “Qualified names has no connection schema state at all”.
I’d suggest either using the mode name (`QualifiedNames has no connection schema state at all`) or keeping the phrase but fixing agreement (`Qualified names have no connection schema state at all`).
```suggestion
6. **Safe under any pooling.** `QualifiedNames` has no connection schema state at all — nothing
```
</issue_to_address>
### Comment 6
<location path="framework/src/BBT.Aether.Npgsql/QualifiedNamesCommandInterceptor.cs" line_range="86-88" />
<code_context>
command.CommandText = PostgreSqlRawSchemaTokenRewriter
.Rewrite(modelRewritten, _quotedSchema)
.CommandText;
</code_context>
<issue_to_address>
**security (csharp.lang.security.sqli.csharp-sqli):** Detected a formatted string in a SQL statement. This could lead to SQL injection if variables in the SQL statement are not properly sanitized. Use a prepared statements instead. You can obtain a PreparedStatement using 'SqlCommand' and 'SqlParameter'.
*Source: opengrep*
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+166
to
+168
| // if (_contexts.Count >= _options.MaxDbContextCount) | ||
| // { | ||
| // throw new InvalidOperationException( |
There was a problem hiding this comment.
issue (performance): Removing the DbContext count guard may allow unbounded context creation and increased resource usage.
With this guard removed, a single unit of work can now create an arbitrary number of DbContext instances. While transactional roots still share one connection/transaction, long‑lived scopes or unusual call patterns can significantly increase memory use and EF Core tracking overhead. If the previous limit was intended to prevent this, consider reintroducing it (possibly with a higher threshold) or adding another safeguard/monitoring mechanism so misuse is caught before it causes performance degradation.
| this IServiceCollection services, | ||
| string connectionString, | ||
| SchemaSwitchingMode mode = SchemaSwitchingMode.TransactionLocal, | ||
| SchemaSwitchingMode mode = SchemaSwitchingMode.QualifiedNames, |
There was a problem hiding this comment.
issue (bug_risk): The mode parameter is effectively ignored, which can hide misconfiguration.
Since new NpgsqlAetherProvider() ignores mode and SchemaSwitchingMode effectively only supports QualifiedNames, callers can still pass other values that are silently ignored. To prevent this, validate that mode == SchemaSwitchingMode.QualifiedNames and throw ArgumentOutOfRangeException (or log a warning) for other values, or mark the parameter as obsolete in a follow-up change.
| @@ -28,4 +28,16 @@ public DbContextOptions<TDbContext> BuildOptions(DbConnection sharedConnection, | |||
| serviceProvider.GetRequiredService<ICurrentSchema>()); | |||
| return builder.Options; | |||
There was a problem hiding this comment.
suggestion (bug_risk): The configuration order in BuildOwnedOptions differs from BuildOptions, which may lead to inconsistent behavior.
Here configure runs before provider.ApplyOwned(...), while in BuildOptions it runs after provider.ApplyShared(...). This difference can change how configure behaves (e.g., if it expects provider-specific settings to be applied first). Please align the call order between the two methods, or, if the difference is intentional, document it clearly in the configurator’s contract.
| > Earlier revisions described the `TransactionLocal` and `SessionSearchPath` switching modes | ||
| > (`SET LOCAL search_path` / session `SET search_path` + `RESET search_path`), a session-level | ||
| > interceptor (`NpgsqlSchemaConnectionInterceptor`), plus an `ICurrentSchema.Set()` / | ||
| > `IsResolved` accessor. Those are gone: qualified names is the only strategy, and no |
There was a problem hiding this comment.
nitpick (typo): Adjust subject–verb agreement for “qualified names” or treat it as the proper mode name.
Consider either keeping it as a proper mode name (QualifiedNames is the only strategy) or using plural agreement (qualified names are the only strategy) to fix the grammar.
Suggested change
| > `IsResolved` accessor. Those are gone: qualified names is the only strategy, and no | |
| > `IsResolved` accessor. Those are gone: QualifiedNames is the only strategy, and no |
| 6. **PgBouncer-safe choices.** `TransactionLocal` never leaks because PostgreSQL reverts | ||
| `SET LOCAL` with the transaction. `QualifiedNames` has no connection schema state at all. | ||
| `SessionSearchPath` remains limited to a session-pinned/native connection. | ||
| 6. **Safe under any pooling.** Qualified names has no connection schema state at all — nothing |
There was a problem hiding this comment.
nitpick (typo): Fix subject–verb agreement in “Qualified names has no connection schema state at all”.
I’d suggest either using the mode name (QualifiedNames has no connection schema state at all) or keeping the phrase but fixing agreement (Qualified names have no connection schema state at all).
Suggested change
| 6. **Safe under any pooling.** Qualified names has no connection schema state at all — nothing | |
| 6. **Safe under any pooling.** `QualifiedNames` has no connection schema state at all — nothing |
Comment on lines
+86
to
+88
| command.CommandText = PostgreSqlRawSchemaTokenRewriter | ||
| .Rewrite(modelRewritten, _quotedSchema) | ||
| .CommandText; |
There was a problem hiding this comment.
security (csharp.lang.security.sqli.csharp-sqli): Detected a formatted string in a SQL statement. This could lead to SQL injection if variables in the SQL statement are not properly sanitized. Use a prepared statements instead. You can obtain a PreparedStatement using 'SqlCommand' and 'SqlParameter'.
Source: opengrep
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
framework/src/BBT.Aether.Npgsql/NpgsqlAetherProvider.cs (1)
19-21: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winUse the ambient
ICurrentSchemafor the 3-arg overload.Only tests pass the 5th argument directly; production config routes through the 3-arg overload, which is implemented as
ApplyShared(..., new StaticCurrentSchema(schema)). That makesQualifiedNamesCommandInterceptorcompareschematocurrentSchema.Name, both of which are fixed to the same value, so any request-mutated DIICurrentSchemacannot change or fail the check. Resolve/consumeICurrentSchemahere or delegate to a 5-arg caller path that preserves the runtime schema value.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@framework/src/BBT.Aether.Npgsql/NpgsqlAetherProvider.cs` around lines 19 - 21, The 3-argument ApplyShared overload currently hardcodes a StaticCurrentSchema, bypassing the runtime DI schema. Update NpgsqlAetherProvider.ApplyShared to resolve and use the ambient ICurrentSchema, or delegate through a caller path that preserves it, so QualifiedNamesCommandInterceptor validates against the request-mutated schema.
🧹 Nitpick comments (2)
framework/test/BBT.Aether.Postgres.Tests/BackgroundJob/ArmingProcessorTests.cs (1)
289-292: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftCentralize and validate the PostgreSQL schema-script binding. These eight tests duplicate exact string replacements against Npgsql-generated SQL, so provider formatting changes or partial updates can leave placeholder handling inconsistent.
framework/test/BBT.Aether.Postgres.Tests/BackgroundJob/ArmingProcessorTests.cs#L289-L292: make this the shared helper implementation and validate that schema placeholders are fully removed.framework/test/BBT.Aether.Postgres.Tests/BackgroundJob/DaprBridgeTests.cs#L113-L116: replace the local transformation with the shared helper.framework/test/BBT.Aether.Postgres.Tests/BackgroundJob/EndToEndJobLifecycleTests.cs#L206-L209: replace the local transformation with the shared helper.framework/test/BBT.Aether.Postgres.Tests/BackgroundJob/EnqueueAtomicityTests.cs#L114-L117: replace the local transformation with the shared helper.framework/test/BBT.Aether.Postgres.Tests/BackgroundJob/JobDispatcherTests.cs#L257-L260: replace the local transformation with the shared helper.framework/test/BBT.Aether.Postgres.Tests/BackgroundJob/JobStoreCasTests.cs#L84-L87: replace the local transformation with the shared helper.framework/test/BBT.Aether.Postgres.Tests/BackgroundJob/JobStoreClaimReaperTests.cs#L70-L73: replace the local transformation with the shared helper.framework/test/BBT.Aether.Postgres.Tests/BackgroundJob/ReaperTests.cs#L125-L128: replace the local transformation with the shared helper.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@framework/test/BBT.Aether.Postgres.Tests/BackgroundJob/ArmingProcessorTests.cs` around lines 289 - 292, Centralize PostgreSQL schema-script transformation in the shared helper at ArmingProcessorTests.cs lines 289-292, preserving all required replacements and validating that no schema placeholders remain after binding. Replace the duplicated local transformations with this helper in DaprBridgeTests.cs lines 113-116, EndToEndJobLifecycleTests.cs lines 206-209, EnqueueAtomicityTests.cs lines 114-117, JobDispatcherTests.cs lines 257-260, JobStoreCasTests.cs lines 84-87, JobStoreClaimReaperTests.cs lines 70-73, and ReaperTests.cs lines 125-128.framework/src/BBT.Aether.Npgsql/NpgsqlAetherProvider.cs (1)
19-28: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the unused
stateparameter from the provider contract.
NpgsqlAetherProviderqualified-names binding usesICurrentSchema, andSqlServerAetherProviderno longer honors state either, so keep removingstatefromIAetherDatabaseProvider.ApplyShared(...)and both provider overload signatures now that this API exists for QualifiedNames design.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@framework/src/BBT.Aether.Npgsql/NpgsqlAetherProvider.cs` around lines 19 - 28, Remove the unused state parameter from IAetherDatabaseProvider.ApplyShared and both ApplyShared overloads in NpgsqlAetherProvider, updating all affected call sites and the corresponding SqlServerAetherProvider signatures while preserving the ICurrentSchema-based qualified-names binding.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@framework/docs/multi-schema/IMPLEMENTATION_NOTES.md`:
- Around line 58-70: Update the documentation sentences beginning with
“Qualified names is” to use correct grammatical agreement, such as “Qualified
names are” or “The qualified-names strategy is,” including the corresponding
sentences in the additionally referenced section. Preserve the existing
technical content.
In `@framework/docs/multi-schema/README.md`:
- Around line 93-96: Update the preceding paragraph’s connection-state wording
to remove the claim that the formatted schema ends up on the connection. State
instead that the formatted schema name is used in the bound SQL or
qualified-name rewriter, while preserving the surrounding explanation.
In
`@framework/src/BBT.Aether.Infrastructure/BBT/Aether/Uow/CompositeUnitOfWork.cs`:
- Around line 166-170: Restore the MaxDbContextCount guard in the
context-creation flow of CompositeUnitOfWork, using _contexts.Count and
_options.MaxDbContextCount to throw the existing InvalidOperationException when
the configured limit is reached or exceeded. Keep the current context creation
behavior unchanged below the guard.
---
Outside diff comments:
In `@framework/src/BBT.Aether.Npgsql/NpgsqlAetherProvider.cs`:
- Around line 19-21: The 3-argument ApplyShared overload currently hardcodes a
StaticCurrentSchema, bypassing the runtime DI schema. Update
NpgsqlAetherProvider.ApplyShared to resolve and use the ambient ICurrentSchema,
or delegate through a caller path that preserves it, so
QualifiedNamesCommandInterceptor validates against the request-mutated schema.
---
Nitpick comments:
In `@framework/src/BBT.Aether.Npgsql/NpgsqlAetherProvider.cs`:
- Around line 19-28: Remove the unused state parameter from
IAetherDatabaseProvider.ApplyShared and both ApplyShared overloads in
NpgsqlAetherProvider, updating all affected call sites and the corresponding
SqlServerAetherProvider signatures while preserving the ICurrentSchema-based
qualified-names binding.
In
`@framework/test/BBT.Aether.Postgres.Tests/BackgroundJob/ArmingProcessorTests.cs`:
- Around line 289-292: Centralize PostgreSQL schema-script transformation in the
shared helper at ArmingProcessorTests.cs lines 289-292, preserving all required
replacements and validating that no schema placeholders remain after binding.
Replace the duplicated local transformations with this helper in
DaprBridgeTests.cs lines 113-116, EndToEndJobLifecycleTests.cs lines 206-209,
EnqueueAtomicityTests.cs lines 114-117, JobDispatcherTests.cs lines 257-260,
JobStoreCasTests.cs lines 84-87, JobStoreClaimReaperTests.cs lines 70-73, and
ReaperTests.cs lines 125-128.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4e0cc9c6-c072-4fb5-b4f9-3e1c2a495932
📒 Files selected for processing (31)
framework/docs/multi-schema/ADOPTION-GUIDE.mdframework/docs/multi-schema/IMPLEMENTATION_NOTES.mdframework/docs/multi-schema/README.mdframework/docs/unit-of-work/README.mdframework/src/BBT.Aether.Infrastructure/BBT/Aether/Uow/CompositeUnitOfWork.csframework/src/BBT.Aether.Infrastructure/BBT/Aether/Uow/EntityFrameworkCore/AetherDbContextConfigurator.csframework/src/BBT.Aether.Infrastructure/BBT/Aether/Uow/EntityFrameworkCore/IAetherDatabaseProvider.csframework/src/BBT.Aether.Infrastructure/BBT/Aether/Uow/EntityFrameworkCore/IAetherDbContextConfigurator.csframework/src/BBT.Aether.Npgsql/BBT/Aether/Uow/EntityFrameworkCore/SchemaSwitchingMode.csframework/src/BBT.Aether.Npgsql/Microsoft/Extensions/DependencyInjection/AetherNpgsqlServiceCollectionExtensions.csframework/src/BBT.Aether.Npgsql/NpgsqlAetherProvider.csframework/src/BBT.Aether.Npgsql/QualifiedNamesCommandInterceptor.csframework/src/BBT.Aether.Npgsql/SearchPathCommandInterceptor.csframework/test/BBT.Aether.Infrastructure.Tests/BBT/Aether/Uow/DomainEventFailureTests.csframework/test/BBT.Aether.Postgres.Tests/BackgroundJob/ArmingProcessorTests.csframework/test/BBT.Aether.Postgres.Tests/BackgroundJob/DaprBridgeTests.csframework/test/BBT.Aether.Postgres.Tests/BackgroundJob/EndToEndJobLifecycleTests.csframework/test/BBT.Aether.Postgres.Tests/BackgroundJob/EnqueueAtomicityTests.csframework/test/BBT.Aether.Postgres.Tests/BackgroundJob/JobDispatcherTests.csframework/test/BBT.Aether.Postgres.Tests/BackgroundJob/JobStoreCasTests.csframework/test/BBT.Aether.Postgres.Tests/BackgroundJob/JobStoreClaimReaperTests.csframework/test/BBT.Aether.Postgres.Tests/BackgroundJob/ReaperTests.csframework/test/BBT.Aether.Postgres.Tests/DbContextConfiguratorTests.csframework/test/BBT.Aether.Postgres.Tests/MultiSchemaUnitOfWorkTests.csframework/test/BBT.Aether.Postgres.Tests/NonTransactionalOutboxDispatchTests.csframework/test/BBT.Aether.Postgres.Tests/NpgsqlLeaseStoreTests.csframework/test/BBT.Aether.Postgres.Tests/OutboxWithinSharedTransactionTests.csframework/test/BBT.Aether.Postgres.Tests/PgBouncerSearchPathTests.csframework/test/BBT.Aether.Postgres.Tests/QualifiedNamesTests.csframework/test/BBT.Aether.Postgres.Tests/UnitOfWorkDisposalTests.csframework/test/BBT.Aether.Postgres.Tests/UnitOfWorkMiddlewareTests.cs
💤 Files with no reviewable changes (1)
- framework/src/BBT.Aether.Npgsql/SearchPathCommandInterceptor.cs
Comment on lines
+58
to
+70
| 3. **Qualified names is the only isolation strategy.** The former `TransactionLocal` and | ||
| `SessionSearchPath` switching modes were removed, along with all `search_path` manipulation | ||
| (`SET LOCAL search_path`, session `SET search_path`, `RESET search_path` cleanup). | ||
| `SchemaSwitchingMode` now has the single member `QualifiedNames`, and isolation is enforced | ||
| by `QualifiedNamesCommandInterceptor(schema, currentSchema)`: | ||
|
|
||
| - Uses one tenant-independent model placeholder, then rewrites it to the validated schema | ||
| bound to the context immediately before execution. Schema-dependent | ||
| `FromSqlRaw`/`ExecuteSqlRaw` relations use the exact `{{schema}}` token, rewritten to the | ||
| quoted bound schema. | ||
| - Throws if `ICurrentSchema.Name` does not match the context's bound schema (guard against | ||
| a context leaking across schema scopes). | ||
| - Emits no `SET`, `SET LOCAL`, or `RESET search_path` and requires no transaction. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix the grammatical agreement for “qualified names”.
Use “Qualified names are/have…” or “The qualified-names strategy is/has…” in these sentences.
Also applies to: 86-89
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@framework/docs/multi-schema/IMPLEMENTATION_NOTES.md` around lines 58 - 70,
Update the documentation sentences beginning with “Qualified names is” to use
correct grammatical agreement, such as “Qualified names are” or “The
qualified-names strategy is,” including the corresponding sentences in the
additionally referenced section. Preserve the existing technical content.
Comment on lines
+93
to
96
| Before a name is interpolated into SQL, it is validated and quoted by | ||
| `PostgreSqlIdentifier.QuoteSchema(...)` (regex `^[a-zA-Z_][a-zA-Z0-9_]*$`). An invalid name | ||
| throws `InvalidOperationException: Invalid PostgreSQL identifier: <name>`. Schema names | ||
| cannot be passed as SQL parameters, so this validate-then-quote step is the injection guard. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Remove the stale connection-state wording.
The preceding paragraph says the formatted schema “ends up on the connection,” but qualified-name rewriting does not store schema state on connections. Reword that sentence to say the formatted name is used in the bound SQL/rewriter instead.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@framework/docs/multi-schema/README.md` around lines 93 - 96, Update the
preceding paragraph’s connection-state wording to remove the claim that the
formatted schema ends up on the connection. State instead that the formatted
schema name is used in the bound SQL or qualified-name rewriter, while
preserving the surrounding explanation.
Comment on lines
+166
to
+170
| // if (_contexts.Count >= _options.MaxDbContextCount) | ||
| // { | ||
| // throw new InvalidOperationException( | ||
| // $"UnitOfWork DbContext limit exceeded. Limit: {_options.MaxDbContextCount}"); | ||
| // } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Restore MaxDbContextCount enforcement.
Commenting out this guard breaks the existing option contract: UnitOfWorkOptions still exposes it, the UoW docs list the error, and Exceeds_max_context_limit_throws expects it. A UoW can now retain unbounded contexts for distinct schemas/types.
Proposed fix
- // if (_contexts.Count >= _options.MaxDbContextCount)
- // {
- // throw new InvalidOperationException(
- // $"UnitOfWork DbContext limit exceeded. Limit: {_options.MaxDbContextCount}");
- // }
+ if (_contexts.Count >= _options.MaxDbContextCount)
+ {
+ throw new InvalidOperationException(
+ $"UnitOfWork DbContext limit exceeded. Limit: {_options.MaxDbContextCount}");
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // if (_contexts.Count >= _options.MaxDbContextCount) | |
| // { | |
| // throw new InvalidOperationException( | |
| // $"UnitOfWork DbContext limit exceeded. Limit: {_options.MaxDbContextCount}"); | |
| // } | |
| if (_contexts.Count >= _options.MaxDbContextCount) | |
| { | |
| throw new InvalidOperationException( | |
| $"UnitOfWork DbContext limit exceeded. Limit: {_options.MaxDbContextCount}"); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@framework/src/BBT.Aether.Infrastructure/BBT/Aether/Uow/CompositeUnitOfWork.cs`
around lines 166 - 170, Restore the MaxDbContextCount guard in the
context-creation flow of CompositeUnitOfWork, using _contexts.Count and
_options.MaxDbContextCount to throw the existing InvalidOperationException when
the configured limit is reached or exceeded. Keep the current context creation
behavior unchanged below the guard.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Simplifies the multi-schema implementation by removing the
TransactionLocalandSessionSearchPathswitching modes and their associatedsearch_pathmanipulation logic. Schema targeting now exclusively usesSchemaSwitchingMode.QualifiedNames, where SQL is rewritten to fully-qualified"schema"."table"names at command execution time. This eliminates connection-level state dependencies, making the system compatible with any connection pool topology (including PgBouncer transaction pooling) and allowing non-transactional units of work to leave connection management entirely to EF Core.Key Changes
SearchPathCommandInterceptor: The 217-line interceptor that issuedSET LOCAL search_path(TransactionLocal) or session-levelSET search_path(SessionSearchPath) is gone.QualifiedNamesCommandInterceptor: A simpler 90-line interceptor that only rewrites model placeholders and raw SQL{{schema}}tokens to the quoted bound schema; no async overhead since rewriting is synchronous.NpgsqlAetherProvider: Removed theSchemaSwitchingModeconstructor parameter (now parameterless); always applies qualified-names binding viaApplySchemaBinding().SchemaSwitchingModeenum: Now contains onlyQualifiedNamesas the sole member; the parameter onAddAetherNpgsqlis optional and kept for signature compatibility.CompositeUnitOfWorkdocumentation: Clarified that transactional roots open a shared connection/transaction lazily, while non-transactional roots never open a connection (EF Core rents pooled connections per operation).BuildOwnedOptions()toIAetherDatabaseProvider: Enables non-transactional contexts to bindUseNpgsql(connectionString)directly, letting EF Core manage the connection lifecycle.SessionSearchPathtest provider; simplified interceptor instantiation in unit tests; added assertions that non-transactional contexts leave connection state to EF Core.Implementation Details
search_pathstate exists on the connection. This is safe on any pooled connection and with PgBouncer transaction pooling.SchemaSwitchingModeparameter onAddAetherNpgsqlremains optional for signature compatibility, but onlyQualifiedNamesis supported.https://claude.ai/code/session_01BKsqFuUQTnEJaJKygkEh2c
Summary by Sourcery
Simplify PostgreSQL multi-schema support to rely solely on qualified-name SQL rewriting and decouple non-transactional units of work from shared connections.
Enhancements:
Documentation:
Tests:
Summary by CodeRabbit
New Features
Documentation
Bug Fixes