This repository was archived by the owner on Dec 18, 2025. It is now read-only.
Enabled ability for users to specify customer headers - for example x-forwarded-for#681
Merged
Conversation
|
Test Results3 727 tests ±0 3 727 ✅ ±0 55s ⏱️ ±0s Results for commit 9738fe4. ± Comparison against base commit 2aec8c9. This pull request removes 339 and adds 339 tests. Note that renamed tests count towards both. |
![korbit-ai[bot]](https://avatars.githubusercontent.com/in/322216?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review by Korbit AI
Korbit automatically attempts to detect when you fix issues in new commits.
| Category | Issue | Fix Detected |
|---|---|---|
 |
Missing null check in header key access ▹ view | |
 |
Unsealed Dispose Method ▹ view | |
|
Unsafe Header Value Validation ▹ view | |
 |
Unclear tuple field names ▹ view | |
|
Overly verbose variable declarations ▹ view |
Files scanned
| File Path | Reviewed |
|---|---|
| src/PinguApps.Appwrite.Shared/Constants.cs | ✅ |
| src/PinguApps.Appwrite.Client/Handlers/HeaderHandler.cs | ✅ |
| src/PinguApps.Appwrite.Server/Handlers/HeaderHandler.cs | ✅ |
| src/PinguApps.Appwrite.Shared/AppwriteRequestContext.cs | ✅ |
| src/PinguApps.Appwrite.Shared/AppwriteHeaderScope.cs | ✅ |
Explore our documentation to understand the languages and file types we support and the files we ignore.
Need a new review? Comment
/korbit-reviewon this PR and I'll review your latest changes.Korbit Guide: Usage and Customization
Interacting with Korbit
- You can manually ask Korbit to review your PR using the
/korbit-reviewcommand in a comment at the root of your PR.- You can ask Korbit to generate a new PR description using the
/korbit-generate-pr-descriptioncommand in any comment on your PR.- Too many Korbit comments? I can resolve all my comment threads if you use the
/korbit-resolvecommand in any comment on your PR.- On any given comment that Korbit raises on your pull request, you can have a discussion with Korbit by replying to the comment.
- Help train Korbit to improve your reviews by giving a 👍 or 👎 on the comments Korbit posts.
Customizing Korbit
- Check out our docs on how you can make Korbit work best for you and your team.
- Customize Korbit for your organization through the Korbit Console.
Current Korbit Configuration
General Settings
Setting Value Review Schedule Automatic excluding drafts Max Issue Count 10 Automatic PR Descriptions ✅ Issue Categories
Category Enabled Documentation ✅ Logging ✅ Error Handling ✅ Readability ✅ Design ✅ Performance ✅ Security ✅ Functionality ✅ Feedback and Support
Note
Korbit Pro is free for open source projects 🎉
Looking to add Korbit to your team? Get started with a free 2 week trial here
| /// </summary> | ||
| public static void AddHeader(string key, string value) | ||
| { | ||
| CurrentHeaders[key] = value; |
There was a problem hiding this comment.
Missing null check in header key access 
Tell me more
What is the issue?
The AddHeader method does not handle the case where key is null, which could throw a NullReferenceException when used as a dictionary key.
Why this matters
Dictionary operations with null keys will throw exceptions at runtime, potentially causing application crashes in production.
Suggested change ∙ Feature Preview
Add null check and throw ArgumentNullException for better error handling:
public static void AddHeader(string key, string value)
{
if (key == null)
{
throw new ArgumentNullException(nameof(key));
}
CurrentHeaders[key] = value;
}
💬 Looking for more details? Reply to this comment to chat with Korbit.
Comment on lines
+44
to
+56
| public void Dispose() | ||
| { | ||
| foreach (var entry in _originalState) | ||
| { | ||
| var key = entry.Key; | ||
| var (hadValue, oldValue) = entry.Value; | ||
|
|
||
| if (hadValue) | ||
| AppwriteRequestContext.AddHeader(key, oldValue); | ||
| else | ||
| AppwriteRequestContext.CurrentHeaders.Remove(key); | ||
| } | ||
| } |
There was a problem hiding this comment.
Unsealed Dispose Method 
Tell me more
What is the issue?
The Dispose method is not marked as sealed, which could lead to incorrect header restoration if a derived class overrides Dispose without calling base.Dispose().
Why this matters
If a derived class improperly overrides Dispose, the original header values won't be restored, potentially causing request header state corruption in subsequent operations.
Suggested change ∙ Feature Preview
Mark the Dispose method as sealed to prevent inheritance issues:
public sealed override void Dispose()
{
foreach (var entry in _originalState)
{
var key = entry.Key;
var (hadValue, oldValue) = entry.Value;
if (hadValue)
AppwriteRequestContext.AddHeader(key, oldValue);
else
AppwriteRequestContext.CurrentHeaders.Remove(key);
}
}💬 Looking for more details? Reply to this comment to chat with Korbit.
|
|
||
| foreach (var header in AppwriteRequestContext.CurrentHeaders) | ||
| { | ||
| request.Headers.TryAddWithoutValidation(header.Key, header.Value); |
There was a problem hiding this comment.
Unsafe Header Value Validation
Tell me more
What is the issue?
Using TryAddWithoutValidation could allow invalid or malformed header values to be added to the request, potentially causing issues with the HTTP request.
Why this matters
Invalid header values could cause the request to fail or be rejected by the server, leading to runtime errors that would be difficult to diagnose.
Suggested change ∙ Feature Preview
Use proper header validation and handle failures:
foreach (var header in AppwriteRequestContext.CurrentHeaders)
{
if (!_protectedHeaders.Contains(header.Key, StringComparer.OrdinalIgnoreCase))
{
try
{
request.Headers.Add(header.Key, header.Value);
}
catch (InvalidOperationException ex)
{
// Log the error or handle invalid headers appropriately
// Could throw a custom exception or skip the invalid header
}
}
}💬 Looking for more details? Reply to this comment to chat with Korbit.
|
|
||
| public class AppwriteHeaderScope : IDisposable | ||
| { | ||
| private readonly Dictionary<string, (bool hadValue, string oldValue)> _originalState = []; |
There was a problem hiding this comment.
Unclear tuple field names 
Tell me more
What is the issue?
The tuple field names 'hadValue' and 'oldValue' create indirect naming that makes the code harder to follow. More descriptive names would better convey their purpose.
Why this matters
Using more semantic names would make it immediately clear what these values represent in the context of header state management.
Suggested change ∙ Feature Preview
private readonly Dictionary<string, (bool headerExists, string previousValue)> _originalState = [];💬 Looking for more details? Reply to this comment to chat with Korbit.
Comment on lines
+44
to
+49
| public void Dispose() | ||
| { | ||
| foreach (var entry in _originalState) | ||
| { | ||
| var key = entry.Key; | ||
| var (hadValue, oldValue) = entry.Value; |
There was a problem hiding this comment.
Overly verbose variable declarations
Tell me more
What is the issue?
The deconstruction of the dictionary entry creates unnecessary variable assignments that add visual noise.
Why this matters
Multiple intermediate variables make the code more verbose without adding clarity. The logic can be simplified while maintaining readability.
Suggested change ∙ Feature Preview
public void Dispose()
{
foreach (var (headerName, (headerExists, previousValue)) in _originalState)
{
if (headerExists)
AppwriteRequestContext.AddHeader(headerName, previousValue);
else
AppwriteRequestContext.CurrentHeaders.Remove(headerName);
}
}💬 Looking for more details? Reply to this comment to chat with Korbit.
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
1 participant
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.
Changes
Issue
Categorise the PR
featurebugdocssecuritymetapatchminormajorDescription by Korbit AI
What change is being made?
Enable users to specify custom headers, such as
x-forwarded-for, in requests by implementingAppwriteRequestContextandAppwriteHeaderScopefor managing headers, and updating the header handlers in the client and server projects to incorporate these custom headers.Why are these changes being made?
These changes allow for greater flexibility in request handling by permitting users to add and manage custom headers dynamically, which is essential for supporting scenarios like forwarding header information in HTTP requests. This design provides an easy-to-use, encapsulated way to manage headers within a defined scope, allowing for easy integration into existing workflows.