v1.0.33 - #89
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
…out-mgzobn Remove TransactionLocal and SessionSearchPath modes; use QualifiedNames only
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 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 |
Reviewer's GuideConsolidates PostgreSQL multi-schema support onto a single QualifiedNames strategy, updates the Unit of Work to distinguish transactional (shared connection) vs non-transactional (EF-owned) connection lifecycles, removes all search_path-based modes and interceptors, and refreshes docs and tests to match the new behavior and APIs. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Not up to standards ⛔🔴 Issues
|
| Category | Results |
|---|---|
| Security | 1 critical |
🟢 Metrics 8 complexity · 1 duplication
Metric Results Complexity 8 Duplication 1
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, 1 other issue, 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.GetDbContextAsync the MaxDbContextCount guard is commented out rather than removed or replaced; if the limit is intentionally being dropped, consider deleting the dead code and updating any related configuration/docs instead of leaving it commented, or re-enable it with a clear rationale.
- QualifiedNamesCommandInterceptor runs both RewriteModelPlaceholder and Rewrite on every command; if command frequency is high, it may be worth considering a small optimization (e.g., early-exit when the placeholder/token is absent, or combining the passes) to reduce repeated string processing on hot paths.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In CompositeUnitOfWork.GetDbContextAsync the MaxDbContextCount guard is commented out rather than removed or replaced; if the limit is intentionally being dropped, consider deleting the dead code and updating any related configuration/docs instead of leaving it commented, or re-enable it with a clear rationale.
- QualifiedNamesCommandInterceptor runs both RewriteModelPlaceholder and Rewrite on every command; if command frequency is high, it may be worth considering a small optimization (e.g., early-exit when the placeholder/token is absent, or combining the passes) to reduce repeated string processing on hot paths.
## Individual Comments
### Comment 1
<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>
**issue (typo):** Fix subject–verb agreement in the phrase "Qualified names has".
Change the sentence to: "Qualified names have no connection schema state at all" to ensure correct subject–verb agreement and consistency with other references to qualified names.
```suggestion
6. **Safe under any pooling.** Qualified names have no connection schema state at all — nothing
```
</issue_to_address>
### Comment 2
<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.
| 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.
issue (typo): Fix subject–verb agreement in the phrase "Qualified names has".
Change the sentence to: "Qualified names have no connection schema state at all" to ensure correct subject–verb agreement and consistency with other references to qualified names.
Suggested change
| 6. **Safe under any pooling.** Qualified names has no connection schema state at all — nothing | |
| 6. **Safe under any pooling.** Qualified names have 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
|
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 by Sourcery
Consolidate PostgreSQL multi-schema support around qualified-name schema targeting, remove search_path-based switching modes and interceptor, and adjust the Unit of Work to allow non-transactional flows to use EF Core-owned pooled connections instead of a shared connection.
Enhancements:
Documentation:
Tests: