Use Uri.EscapeDataString instead of HttpUtility.UrlEncode in Boilerplate (#9868) #9875
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe changes update how query strings are handled across multiple components. The legacy Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant AppControllerBase
participant AppQueryStringCollection
User->>AppControllerBase: Call AddQueryString(key, value)
AppControllerBase->>AppQueryStringCollection: Add key/value with encoding
User->>AppControllerBase: Invoke GetDynamicQueryString()
AppControllerBase->>AppQueryStringCollection: Convert collection to string & clear entries
AppControllerBase->>User: Return query string result
sequenceDiagram
participant Client
participant ODataQuery
participant AppQueryStringCollection
Client->>ODataQuery: Invoke ToString()
ODataQuery->>AppQueryStringCollection: Retrieve query parameters as string
ODataQuery->>Client: Return constructed query string
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (2)
src/SourceGenerators/Bit.SourceGenerators/HttpClientProxy/HttpClientProxySourceGenerator.cs (2)
70-70:⚠️ Potential issueEnsure proper URL encoding for dynamic query strings.
The current implementation directly concatenates the dynamic query string to the URL without encoding. This could lead to malformed URLs or potential security issues if the query string contains special characters.
Apply this diff to ensure proper URL encoding:
- __url += {(action.Url.Contains('?') ? "'&'" : "'?'")} + dynamicQS; + __url += {(action.Url.Contains('?') ? "'&'" : "'?'")} + Uri.EscapeDataString(dynamicQS);
94-94: 🛠️ Refactor suggestionRemove unnecessary using directive.
Since we're using
Uri.EscapeDataStringinstead ofHttpUtility.UrlEncodeas per PR objectives, theSystem.Webnamespace import is no longer needed.-using System.Web;
🧹 Nitpick comments (4)
src/Templates/Boilerplate/Bit.Boilerplate/src/Shared/Services/AppQueryStringCollection.cs (1)
12-12: Avoid double-decoding for partially-escaped keys and values.
Since the code applies bothUri.UnescapeDataStringand then re-escapes the result, partially-encoded segments (e.g."%2B") might be altered unexpectedly. Consider removing the unescape step or adding checks to handle partially-escaped inputs.src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Services/ODataQuery.cs (2)
31-31: Consider specifying an initial capacity.
If you expect to add multiple parameters, providing an initial capacity toAppQueryStringCollectioncan reduce resizing overhead.
68-68: Decide whether to prepend a question mark.
Returningqs.ToString()will omit the leading “?”. If you want a valid query portion for a URL, consider prefixing it with “?”.src/SourceGenerators/Bit.SourceGenerators/HttpClientProxy/HttpClientProxySourceGenerator.cs (1)
117-123: Add null check for dictionary parameter.While the implementation is good, it's missing a null check for the input parameter.
- public void AddQueryStrings(Dictionary<string, object?> queryString) + public void AddQueryStrings(Dictionary<string, object?> queryString) + { + ArgumentNullException.ThrowIfNull(queryString); + + foreach (var key in queryString.Keys) + { + AddQueryString(key, queryString[key]); + } + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/SourceGenerators/Bit.SourceGenerators/HttpClientProxy/HttpClientProxySourceGenerator.cs(2 hunks)src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Services/ODataQuery.cs(2 hunks)src/Templates/Boilerplate/Bit.Boilerplate/src/Shared/Controllers/IAppController.cs(1 hunks)src/Templates/Boilerplate/Bit.Boilerplate/src/Shared/Extensions/UriExtensions.cs(2 hunks)src/Templates/Boilerplate/Bit.Boilerplate/src/Shared/Services/AppQueryStringCollection.cs(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build and test
🔇 Additional comments (7)
src/Templates/Boilerplate/Bit.Boilerplate/src/Shared/Extensions/UriExtensions.cs (2)
27-28: LGTM! Safer culture value retrieval.The changes improve error handling by using
TryGetValueinstead of direct access, making the code more robust against potential exceptions.
1-64: Verify implementation of PR objective.While the changes in this file improve query string handling, they don't appear to address the PR's objective of replacing
HttpUtility.UrlEncodewithUri.EscapeDataString. Please verify if this change is implemented in other files.Run the following script to check for any remaining usages of
HttpUtility.UrlEncode:✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Search for any remaining usages of HttpUtility.UrlEncode # Search for HttpUtility.UrlEncode usage rg -i "HttpUtility.UrlEncode" # Search for using statements that might import HttpUtility rg -i "using.*HttpUtility"Length of output: 337
No remaining active usage of HttpUtility.UrlEncode found – PR objective verified.
The search only revealed a reference to HttpUtility.UrlEncode in an XML comment within
AppQueryStringCollection.cs, confirming that the change to useUri.EscapeDataStringinstead has been broadly implemented.src/Templates/Boilerplate/Bit.Boilerplate/src/Shared/Services/AppQueryStringCollection.cs (1)
8-8: Confirm this constructor syntax is supported in your C# version.
It appears you're using a record-like declaration (parentheses after the class name). Ensure the project targets a C# version that supports this syntax. Otherwise, consider rewriting it as a standard class constructor:-public class AppQueryStringCollection() : Dictionary<string, object?>(StringComparer.OrdinalIgnoreCase) +public class AppQueryStringCollection : Dictionary<string, object?> +{ + public AppQueryStringCollection() : base(StringComparer.OrdinalIgnoreCase) {} +}src/Templates/Boilerplate/Bit.Boilerplate/src/Shared/Controllers/IAppController.cs (1)
19-19: Handle invalid or partially-encoded query strings.
CallingAppQueryStringCollection.Parse(existingQueryString)may cause unexpected behavior if the query string contains malformed or partially-encoded segments. Consider adding error handling or validation before parsing.src/SourceGenerators/Bit.SourceGenerators/HttpClientProxy/HttpClientProxySourceGenerator.cs (3)
110-110: LGTM! Good choice using collection initializer syntax.The change to
AppQueryStringCollectionwith array collection initializer syntax is clean and efficient.
112-115: LGTM! Clear and concise implementation.The new
AddQueryStringmethod signature with key-value parameters is more intuitive and type-safe compared to accepting a raw query string.
125-133: LGTM! Clean implementation.The simplified
GetDynamicQueryStringmethod is more maintainable while preserving the clear-after-use behavior.
src/Templates/Boilerplate/Bit.Boilerplate/src/Shared/Extensions/UriExtensions.cs
Outdated
Show resolved
Hide resolved
ysmoradi
changed the title
closes #9868
Summary by CodeRabbit
Refactor
Bug Fixes