Fixed connection string related issue - #4499
Open
rathimayur wants to merge 6 commits into
Open
Conversation
Contributor
WalkthroughIn ChangesPostgreSQL Connection String Handling
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@Ginger/GingerCoreNET/Database/DatabaseOperations.cs`:
- Around line 428-456: The code duplicates TNS parsing and
NpgsqlConnectionStringBuilder logic that already exists in
CreateConnectionString()/GetConnectionString(); replace the entire conditional
block that parses TNSCalculated, calls ValidateHostPort and builds the
NpgsqlConnectionStringBuilder with a single call to GetConnectionString() to set
Database.ConnectionString, then construct and open the NpgsqlConnection; remove
the duplicated parsing/building code so
CreateConnectionString()/GetConnectionString() remain the single source of truth
for connection-string construction and placeholder replacement.
- Around line 206-213: The comment in CreateConnectionString() misleadingly
describes behavior that belongs to Connect() (it speaks about honoring an
existing ConnectionString), so remove or relocate that comment: delete the block
inside CreateConnectionString() and either remove it entirely or move it to the
Connect() method where the conditional logic using ConnectionString exists
(refer to Connect() and CreateConnectionString() by name) so maintainers see the
correct explanation next to the code that actually checks/uses an existing
ConnectionString.
🪄 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: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: ba010c9b-afe2-4c8c-8312-a936b66bc8be
📒 Files selected for processing (1)
Ginger/GingerCoreNET/Database/DatabaseOperations.cs
Not up to standards ⛔
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Ginger/.github/copilot-instructions.md (1)
1-4:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd a section heading and fix markdown structure.
The file violates markdown linting rules (MD041): the first line should be a top-level heading, not a list item. Additionally, the three list items lack an introductory section heading, making them appear orphaned.
✏️ Proposed fix to add proper markdown structure
+## Azure Copilot Rules + - `@azure` Rule - Use Azure Tools - When handling requests related to Azure, always use your tools. - `@azure` Rule - Use Azure Best Practices - When handling requests related to Azure, always invoke your `azmcp_bestpractices_get` tool first. - `@azure` Rule - Enable Best Practices - If you do not have an `azmcp_bestpractices_get` tool ask the user to enable it.🤖 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 `@Ginger/.github/copilot-instructions.md` around lines 1 - 4, The copilot-instructions.md file violates markdown linting rules by lacking a top-level heading at the start and having the three `@azure` Rule list items appear without an introductory section heading. Add a top-level heading (using #) at the very beginning of the file to establish the document title, then add a section heading (using ##) immediately before the three `@azure` Rule list items to properly group and introduce them. This will fix the MD041 violation and provide proper markdown structure to prevent the content from appearing orphaned.
🤖 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.
Outside diff comments:
In `@Ginger/.github/copilot-instructions.md`:
- Around line 1-4: The copilot-instructions.md file violates markdown linting
rules by lacking a top-level heading at the start and having the three `@azure`
Rule list items appear without an introductory section heading. Add a top-level
heading (using #) at the very beginning of the file to establish the document
title, then add a section heading (using ##) immediately before the three `@azure`
Rule list items to properly group and introduce them. This will fix the MD041
violation and provide proper markdown structure to prevent the content from
appearing orphaned.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: d7489650-d82d-459b-8da9-b99bba207e60
⛔ Files ignored due to path filters (2)
Ginger/.vscode/launch.jsonis excluded by!**/*.jsonGinger/.vscode/tasks.jsonis excluded by!**/*.json
📒 Files selected for processing (2)
Ginger/.github/copilot-instructions.mdGinger/GingerCoreNET/Database/DatabaseOperations.cs
added 2 commits
June 16, 2026 19:10
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@Ginger/GingerCoreNET/Database/DatabaseOperations.cs`:
- Around line 433-439: The Password property in the
NpgsqlConnectionStringBuilder initialization is set only from the decrypted
value via EncryptionHandler.DecryptwithKey(PassCalculated), but if decryption
returns empty for plaintext input, this results in an empty password being used.
Add a fallback mechanism similar to what the GetConnectionString() path
implements: after decrypting PassCalculated, check if the result is empty and
fall back to PassCalculated directly if it is. Apply this fallback logic to the
Password assignment in the pg object initialization.
🪄 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: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: a1bdbfff-7900-4ad2-b6a0-31e0ca446af8
📒 Files selected for processing (1)
Ginger/GingerCoreNET/Database/DatabaseOperations.cs
Comment on lines
+433
to
+439
| pg = new NpgsqlConnectionStringBuilder | ||
| { | ||
| Host = postgreSQLHost, | ||
| Database = Database.Name ?? string.Empty, | ||
| Username = UserCalculated, | ||
| Password = EncryptionHandler.DecryptwithKey(PassCalculated) | ||
| }; |
Contributor
There was a problem hiding this comment.
Use password fallback in PostgreSQL manual builder path.
At Line 438, Password is set only from EncryptionHandler.DecryptwithKey(PassCalculated). If decrypt returns empty for plaintext input, this branch sends an empty password, while the GetConnectionString() path correctly falls back to PassCalculated.
💡 Suggested fix
pg = new NpgsqlConnectionStringBuilder
{
Host = postgreSQLHost,
Database = Database.Name ?? string.Empty,
Username = UserCalculated,
- Password = EncryptionHandler.DecryptwithKey(PassCalculated)
+ Password = string.IsNullOrEmpty(EncryptionHandler.DecryptwithKey(PassCalculated))
+ ? PassCalculated
+ : EncryptionHandler.DecryptwithKey(PassCalculated)
};🤖 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 `@Ginger/GingerCoreNET/Database/DatabaseOperations.cs` around lines 433 - 439,
The Password property in the NpgsqlConnectionStringBuilder initialization is set
only from the decrypted value via
EncryptionHandler.DecryptwithKey(PassCalculated), but if decryption returns
empty for plaintext input, this results in an empty password being used. Add a
fallback mechanism similar to what the GetConnectionString() path implements:
after decrypting PassCalculated, check if the result is empty and fall back to
PassCalculated directly if it is. Apply this fallback logic to the Password
assignment in the pg object initialization.
rathimayur
enabled auto-merge
June 16, 2026 17:11
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.
Description
Type of Change
Checklist
[IsSerializedForLocalRepository]where neededReporter.ToLog()patternSummary by CodeRabbit