Conversation
Also improve exception handling (might break edge cases)
WalkthroughThis PR adds an internal RequestHandler implementing a request/response pipeline, makes RestClientContext.DefaultContentType non-nullable, updates RestClientBuilder to enforce/default the content type and forbid setting Content-Type via generic header methods, and updates domain-related tests to expect RestClientException and remove MyDomainErrorEnum parameters (one test renamed). Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant RequestHandler
participant Serializer
participant HttpClient
participant Response as HTTP Response
Caller->>RequestHandler: Send(methodInfo, args)
RequestHandler->>RequestHandler: Bind params (path, query, form, header, parts)
RequestHandler->>RequestHandler: Expand template & build URI
RequestHandler->>RequestHandler: Build headers & query string
alt Body required
RequestHandler->>RequestHandler: Create HttpContent (Serialized / FormUrlEncoded / Multipart)
opt Multipart
RequestHandler->>Serializer: Serialize parts as needed
RequestHandler->>RequestHandler: Compose multipart content
end
RequestHandler->>RequestHandler: Attach content
end
RequestHandler->>HttpClient: SendAsync(HttpRequestMessage)
HttpClient->>Response: HTTP response
alt 2xx Success
Response-->>RequestHandler: Response message
RequestHandler->>Serializer: Deserialize (if applicable)
RequestHandler-->>Caller: Return typed result
else 4xx/5xx Error
Response-->>RequestHandler: Error response
RequestHandler->>RequestHandler: Invoke domain-exception mapper or create RestClientException
RequestHandler-->>Caller: Throw mapped/RestClientException
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Areas to focus during review:
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ 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 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (5)
Activout.RestClient.Newtonsoft.Json.Test/DomainExceptions/DefaultDomainExceptionErrorObjectTests.cs (1)
88-90: Good shift to StatusCode; consider asserting error detail as well.Optional: also assert that ErrorResponse contains “No deserializer found for application/foobar” to tighten the expectation without coupling to specific exception types.
Example:-Assert.Equal(httpStatusCode, exception.StatusCode); +Assert.Equal(httpStatusCode, exception.StatusCode); +Assert.Contains("No deserializer found for application/foobar", exception.Message);Also applies to: 98-103
Activout.RestClient/Implementation/RequestHandler.cs (4)
242-243: Method naming guideline: avoid “Async” suffix.Rename SendAsync to a non-Async name (e.g., SendRequest) and update the call site.
Apply:- var task = SendAsync(request, cancellationToken); + var task = SendRequest(request, cancellationToken);- private async Task<object?> SendAsync(HttpRequestMessage request, CancellationToken cancellationToken) + private async Task<object?> SendRequest(HttpRequestMessage request, CancellationToken cancellationToken)Also applies to: 476-476
40-41: Adopt collection expressions for field initialization.Use [] list syntax per guidelines.
- private readonly List<KeyValuePair<string, object>> _requestHeaders = new List<KeyValuePair<string, object>>(); + private readonly List<KeyValuePair<string, object>> _requestHeaders = [];
215-218: Query assembly is naive with existing query in template.If BaseTemplate/Path already contains '?', this will produce an invalid URI. Consider a proper query builder or UriBuilder.
Example approach:
- Use UriBuilder with NameValueCollection.
- Or use WebUtilities.QueryHelpers to add parameters.
450-458: TODO: resolve part serializer upfront (perf).Cache serializer per part content type to avoid repeated lookups in hot paths.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
Activout.RestClient.Newtonsoft.Json.Test/DomainExceptions/DefaultDomainExceptionErrorObjectTests.cs(1 hunks)Activout.RestClient.Newtonsoft.Json.Test/DomainExceptions/DomainExceptionErrorObjectTests.cs(1 hunks)Activout.RestClient/Implementation/RequestHandler.cs(1 hunks)Activout.RestClient/Implementation/RestClientContext.cs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.cs
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.cs: Use [] list syntax for collections
Use file-scoped namespaces
Use primary constructors wherever possible
Use records for data transfer objects (DTOs) and immutable data structures
Use var for local variable declarations when possible
Use expression-bodied members for simple methods and properties
Use pattern matching for type checks and deconstruction
Use using statements for resource management
Use async and await for asynchronous programming
Never include "Async" in method names
Only include comments that explain why something is done, not what is done
Files:
Activout.RestClient.Newtonsoft.Json.Test/DomainExceptions/DefaultDomainExceptionErrorObjectTests.csActivout.RestClient.Newtonsoft.Json.Test/DomainExceptions/DomainExceptionErrorObjectTests.csActivout.RestClient/Implementation/RequestHandler.csActivout.RestClient/Implementation/RestClientContext.cs
🧬 Code graph analysis (3)
Activout.RestClient.Newtonsoft.Json.Test/DomainExceptions/DefaultDomainExceptionErrorObjectTests.cs (2)
Activout.RestClient/Implementation/RequestHandler.cs (3)
Task(476-530)Task(532-565)Task(567-572)Activout.RestClient.Newtonsoft.Json/NewtonsoftJsonDeserializer.cs (1)
Task(20-33)
Activout.RestClient/Implementation/RequestHandler.cs (8)
Activout.RestClient/Implementation/RestClient.cs (3)
RestClient(10-47)RestClient(18-22)IEnumerable(24-27)Activout.RestClient/Helpers/ITaskConverterFactory.cs (1)
ITaskConverter(7-7)Activout.RestClient/Helpers/Implementation/TaskConverter3Factory.cs (1)
ITaskConverter(9-14)Activout.RestClient/Serialization/Implementation/SerializationManager.cs (3)
ISerializer(43-47)SerializationManager(8-48)SerializationManager(30-35)Activout.RestClient/ParamConverter/Implementation/ParamConverterManager.cs (3)
IParamConverter(19-30)ParamConverterManager(7-31)ParamConverterManager(13-17)Activout.RestClient/RestClientException.cs (4)
ToString(34-37)RestClientException(7-38)RestClientException(9-14)RestClientException(16-22)Activout.RestClient/Part.cs (2)
Part(4-9)Part(11-18)Activout.RestClient/Serialization/ISerializer.cs (1)
HttpContent(9-9)
Activout.RestClient/Implementation/RestClientContext.cs (1)
Activout.RestClient/MediaType.cs (2)
MediaType(8-11)MediaType(15-22)
🪛 GitHub Check: devskim
Activout.RestClient/Implementation/RequestHandler.cs
[notice] 452-452: A "TODO" or similar was left in source code, possibly indicating incomplete functionality
Suspicious comment
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build (10.0.x)
- GitHub Check: Analyze (csharp)
🔇 Additional comments (2)
Activout.RestClient.Newtonsoft.Json.Test/DomainExceptions/DomainExceptionErrorObjectTests.cs (1)
113-115: Switch to RestClientException aligns with new pipeline.The expectation change is correct and matches RequestHandler’s error path.
Activout.RestClient.Newtonsoft.Json.Test/DomainExceptions/DefaultDomainExceptionErrorObjectTests.cs (1)
69-71: Incorrect review comment—StringDeserializer explicitly handles text/html.The assertion on inner exception is stable. StringDeserializer's
CanDeserializemethod returns true for anytext/*media type (viamediaType.Value.StartsWith("text/")), so text/html is matched. When deserialization fails—e.g.,Activator.CreateInstancethrowsMissingMethodExceptiondue to missing constructor—RequestHandler catches and wraps it inRestClientException, making theInnerExceptionnon-null and the assertion correct. The code is working as intended.Likely an incorrect or invalid review comment.
Activout.RestClient.Newtonsoft.Json.Test/DomainExceptions/DomainExceptionErrorObjectTests.cs
Show resolved
Hide resolved
Most notably dispose HttpResponseMessage if possible
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
Activout.RestClient/Implementation/RequestHandler.cs (4)
343-348: Path parameter null handling.While line 313-316 continues when
rawValueis null, the past review flagged line 345 for explicit null validation to provide a clearer error message when path parameters are unexpectedly null.If you'd like to maintain the current flow, consider adding a comment explaining that null values are filtered earlier. Otherwise, apply the previously suggested explicit null check for clarity.
420-424: Default path substitution null handling.Similar to the path parameter case, line 422 calls
ToString(rawValue)whererawValueshould be non-null due to the earlier continue statement. This was previously flagged for explicit null guarding.The current flow relies on the null check at line 313-316. Consider adding a clarifying comment or explicit null check here as well for defensive programming.
491-500: Debug logging may expose sensitive data.Lines 498-499 and 514-515 log request/response content at debug level. As previously noted, this can leak PII/PHI even at debug level. Consider the previously suggested opt-in configuration and sanitization approach if this is a concern for your application.
Also applies to: 509-516
327-334: Array casting fails for value-type arrays.Line 329 casts
rawValuetoobject?[]?which will throwInvalidCastExceptionfor value-type arrays (e.g.,int[],byte[]). This was previously flagged.Apply this diff to handle all array types:
if (_parameters[i].ParameterType.IsArray) { - var items = (object?[]?)rawValue; - if (items != null) + if (rawValue is IEnumerable enumerable and not string) { - parts.AddRange(items.SelectMany(item => - GetPartNameAndHttpContent(partAttribute, parameterName, item))); + foreach (var item in enumerable) + { + parts.AddRange(GetPartNameAndHttpContent(partAttribute, parameterName, item)); + } } }
🧹 Nitpick comments (5)
Activout.RestClient/Implementation/RequestHandler.cs (4)
40-40: Consider using collection expression syntax per coding guidelines.Line 40 uses
new List<KeyValuePair<string, object>>()instead of the collection expression syntax[]specified in the coding guidelines.Apply this diff to align with the coding guidelines:
- private readonly List<KeyValuePair<string, object>> _requestHeaders = new List<KeyValuePair<string, object>>(); + private readonly List<KeyValuePair<string, object>> _requestHeaders = [];As per coding guidelines.
244-249: Synchronous Result access may deadlock in some contexts.Line 248 uses
task.Resultwhich blocks the calling thread and can deadlock if called from a UI context or with certain synchronization contexts. Consider documenting this behavior or ensuring callers are aware of the synchronous blocking.
457-457: TODO comment indicates potential optimization.The TODO suggests preparing part serializers in advance rather than per-part lookup. This could improve performance for multipart requests with multiple parts.
Would you like me to suggest an implementation that caches part serializers during initialization, or would you prefer to track this separately?
205-211: Use collection expression syntax per coding guidelines.Lines 205-211 use traditional
new List<>()andnew Dictionary<>()syntax. The coding guidelines specify using[]collection expression syntax.Apply this diff:
- var headers = new List<KeyValuePair<string, object>>(); + var headers = new List<KeyValuePair<string, object>>(); headers.AddRange(_requestHeaders); - var routeParams = new Dictionary<string, object>(); - var queryParams = new List<string>(); - var formParams = new List<KeyValuePair<string, string>>(); - var partParams = new List<Part<HttpContent>>(); + var routeParams = new Dictionary<string, object>(); + var queryParams = new List<string>(); + var formParams = new List<KeyValuePair<string, string>>(); + var partParams = new List<Part<HttpContent>>();Note: For
headers, you may need to keep the constructor form ifAddRangerequires a concrete type, or use:List<KeyValuePair<string, object>> headers = [.. _requestHeaders];As per coding guidelines.
Activout.RestClient/Implementation/RestClientBuilder.cs (1)
166-166: Redundant null check - already guaranteed non-null.Line 150 sets
_defaultContentType ??= DefaultContentType, ensuring it's non-null. The null-coalescing with throw on line 166 can never execute and is redundant.Apply this diff to remove the dead code:
- DefaultContentType: _defaultContentType ?? throw new InvalidOperationException("DefaultContentType is not set."), + DefaultContentType: _defaultContentType,Alternatively, add a null-forgiving operator if you want to be explicit:
_defaultContentType!
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Activout.RestClient/Implementation/RequestHandler.cs(1 hunks)Activout.RestClient/Implementation/RestClientBuilder.cs(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.cs
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.cs: Use [] list syntax for collections
Use file-scoped namespaces
Use primary constructors wherever possible
Use records for data transfer objects (DTOs) and immutable data structures
Use var for local variable declarations when possible
Use expression-bodied members for simple methods and properties
Use pattern matching for type checks and deconstruction
Use using statements for resource management
Use async and await for asynchronous programming
Never include "Async" in method names
Only include comments that explain why something is done, not what is done
Files:
Activout.RestClient/Implementation/RequestHandler.csActivout.RestClient/Implementation/RestClientBuilder.cs
🧬 Code graph analysis (1)
Activout.RestClient/Implementation/RequestHandler.cs (11)
Activout.RestClient/Implementation/RestClient.cs (3)
RestClient(10-47)RestClient(18-22)IEnumerable(24-27)Activout.RestClient/Helpers/ITaskConverterFactory.cs (1)
ITaskConverter(7-7)Activout.RestClient/Helpers/Implementation/TaskConverter3Factory.cs (1)
ITaskConverter(9-14)Activout.RestClient/Serialization/Implementation/SerializationManager.cs (3)
ISerializer(43-47)SerializationManager(8-48)SerializationManager(30-35)Activout.RestClient/ParamConverter/Implementation/ParamConverterManager.cs (3)
IParamConverter(19-30)ParamConverterManager(7-31)ParamConverterManager(13-17)Activout.RestClient/Implementation/HeaderListExtensions.cs (1)
AddOrReplaceHeader(8-18)Activout.RestClient/RestClientException.cs (4)
ToString(34-37)RestClientException(7-38)RestClientException(9-14)RestClientException(16-22)Activout.RestClient/Serialization/IDeserializer.cs (1)
Task(10-10)Activout.RestClient/Part.cs (2)
Part(4-9)Part(11-18)Activout.RestClient/Serialization/ISerializer.cs (1)
HttpContent(9-9)Activout.RestClient/Implementation/StringExtensions.cs (1)
SafeSubstring(5-20)
🔇 Additional comments (3)
Activout.RestClient/Implementation/RequestHandler.cs (2)
521-572: Resource management properly implemented.The response disposal issue previously flagged has been well addressed. The
shouldDisposeResponseflag (line 528) combined with the finally block (lines 565-571) ensuresHttpResponseMessageis disposed except when returned to the caller or when returning rawHttpContent.
169-176: Guard against null return from ToString().Line 173 calls
entry.Value.ToString()without null handling. While rare,ToString()can return null, which would cause issues in the replacement.Apply this diff to add null-coalescing:
private string ExpandTemplate(Dictionary<string, object> routeParams) { var expanded = _template; foreach (var entry in routeParams) - expanded = expanded.Replace("{" + entry.Key + "}", entry.Value.ToString()); + expanded = expanded.Replace("{" + entry.Key + "}", entry.Value.ToString() ?? string.Empty); return expanded; }Likely an incorrect or invalid review comment.
Activout.RestClient/Implementation/RestClientBuilder.cs (1)
59-62: LGTM - Clear validation for Content-Type header.Good validation to enforce using the
ContentType()method instead of the genericHeader()method. The case-insensitive check and clear error message provide excellent developer experience.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
Activout.RestClient.Newtonsoft.Json.Test/DomainExceptions/DomainExceptionErrorObjectTests.cs (1)
118-118: Remove or correct the MissingMethodException assertion.This assertion is incorrect. When no deserializer is found for text/html,
RequestHandler.Deserialize(line 576) throwsRestClientExceptionusing the 3-parameter constructor which does NOT set anInnerException. TheInnerExceptionwill benull, notMissingMethodException.Apply this diff:
- Assert.IsType<MissingMethodException>(exception.InnerException); + Assert.Null(exception.InnerException);Alternatively, remove this assertion entirely if the inner exception is not relevant to this test case.
🧹 Nitpick comments (2)
Activout.RestClient.Newtonsoft.Json.Test/DomainExceptions/DomainExceptionErrorObjectTests.cs (2)
12-12: Convert to file-scoped namespace per coding guidelines.The coding guidelines specify using file-scoped namespaces for all C# files.
As per coding guidelines.
Apply this diff:
-namespace Activout.RestClient.Newtonsoft.Json.Test.DomainExceptions -{ +namespace Activout.RestClient.Newtonsoft.Json.Test.DomainExceptions; +Then remove the closing brace at the end of the file and reduce indentation for all members.
14-22: Consider using record type for this DTO.
MyDomainErrorObjectis an immutable data structure that fits the pattern for a record type per coding guidelines.As per coding guidelines.
Apply this diff to convert to a record with primary constructor:
- internal class MyDomainErrorObject - { - public MyDomainErrorEnum ErrorEnum { get; } - - public MyDomainErrorObject(MyDomainErrorEnum errorEnum) - { - ErrorEnum = errorEnum; - } - } + internal record MyDomainErrorObject(MyDomainErrorEnum ErrorEnum);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Activout.RestClient.Newtonsoft.Json.Test/DomainExceptions/DomainExceptionErrorObjectTests.cs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.cs
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.cs: Use [] list syntax for collections
Use file-scoped namespaces
Use primary constructors wherever possible
Use records for data transfer objects (DTOs) and immutable data structures
Use var for local variable declarations when possible
Use expression-bodied members for simple methods and properties
Use pattern matching for type checks and deconstruction
Use using statements for resource management
Use async and await for asynchronous programming
Never include "Async" in method names
Only include comments that explain why something is done, not what is done
Files:
Activout.RestClient.Newtonsoft.Json.Test/DomainExceptions/DomainExceptionErrorObjectTests.cs
🧬 Code graph analysis (1)
Activout.RestClient.Newtonsoft.Json.Test/DomainExceptions/DomainExceptionErrorObjectTests.cs (2)
Activout.RestClient/Implementation/RequestHandler.cs (4)
Task(481-485)Task(487-519)Task(521-572)Task(574-595)Activout.RestClient.Newtonsoft.Json/NewtonsoftJsonDeserializer.cs (1)
Task(20-33)
🔇 Additional comments (1)
Activout.RestClient.Newtonsoft.Json.Test/DomainExceptions/DomainExceptionErrorObjectTests.cs (1)
105-105: LGTM: Test correctly reflects the updated exception behavior.The test method has been appropriately renamed and updated to expect
RestClientExceptionwith the correctStatusCodeassertion, aligning with the behavior when no deserializer is found for the HTML content type.Also applies to: 113-113, 117-117
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
Activout.RestClient/Implementation/RequestHandler.cs (2)
115-118: Consider expression-bodied members for simple methods.Per coding guidelines, these simple methods could use expression-bodied syntax for brevity.
As per coding guidelines.
Apply this diff:
- private bool IsHttpMethodWithBody() - { - return _httpMethod == HttpMethod.Post || _httpMethod == HttpMethod.Put || _httpMethod == HttpMethod.Patch; - } + private bool IsHttpMethodWithBody() => + _httpMethod == HttpMethod.Post || _httpMethod == HttpMethod.Put || _httpMethod == HttpMethod.Patch; - private static HttpMethod GetHttpMethod(HttpMethodAttribute attribute) - { - return attribute.HttpMethod; - } + private static HttpMethod GetHttpMethod(HttpMethodAttribute attribute) => attribute.HttpMethod; - private bool IsVoidTask() - { - return _returnType == typeof(Task); - } + private bool IsVoidTask() => _returnType == typeof(Task); - private bool IsGenericTask() - { - return _returnType.BaseType == typeof(Task) && _returnType.IsGenericType; - } + private bool IsGenericTask() => _returnType.BaseType == typeof(Task) && _returnType.IsGenericType;Also applies to: 133-136, 150-153, 155-158
350-372: Remove redundant null check.Line 364 checks
rawValue != null, but line 313 already continues whenrawValue == null, making this check redundant. Similar pattern appears in FormParamAttribute (line 387) and HeaderParamAttribute (line 410) handling.Apply this diff:
else if (attribute is QueryParamAttribute queryParamAttribute) { if (rawValue is IDictionary dictionary) { foreach (DictionaryEntry entry in dictionary) { var key = entry.Key.ToString(); var value = ConvertValueToString(entry.Value, _parameters[i]); if (key != null && value != null) { queryParams.Add(Uri.EscapeDataString(key) + "=" + Uri.EscapeDataString(value)); } } } - else if (rawValue != null) + else { var stringValue = _paramConverters[i].ToString(rawValue); queryParams.Add(Uri.EscapeDataString(queryParamAttribute.Name ?? parameterName) + "=" + Uri.EscapeDataString(stringValue)); } handled = true; }Apply similar changes to FormParamAttribute (line 387) and HeaderParamAttribute (line 410) branches.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Activout.RestClient/Implementation/RequestHandler.cs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.cs
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.cs: Use [] list syntax for collections
Use file-scoped namespaces
Use primary constructors wherever possible
Use records for data transfer objects (DTOs) and immutable data structures
Use var for local variable declarations when possible
Use expression-bodied members for simple methods and properties
Use pattern matching for type checks and deconstruction
Use using statements for resource management
Use async and await for asynchronous programming
Never include "Async" in method names
Only include comments that explain why something is done, not what is done
Files:
Activout.RestClient/Implementation/RequestHandler.cs
🧬 Code graph analysis (1)
Activout.RestClient/Implementation/RequestHandler.cs (14)
Activout.RestClient/Implementation/RestClient.cs (3)
RestClient(10-47)RestClient(18-22)IEnumerable(24-27)Activout.RestClient/Helpers/ITaskConverterFactory.cs (1)
ITaskConverter(7-7)Activout.RestClient/Helpers/Implementation/TaskConverter3Factory.cs (1)
ITaskConverter(9-14)Activout.RestClient/Serialization/Implementation/SerializationManager.cs (3)
ISerializer(43-47)SerializationManager(8-48)SerializationManager(30-35)Activout.RestClient/ParamConverter/Implementation/ParamConverterManager.cs (3)
IParamConverter(19-30)ParamConverterManager(7-31)ParamConverterManager(13-17)Activout.RestClient/Implementation/HeaderListExtensions.cs (1)
AddOrReplaceHeader(8-18)Activout.RestClient/RestClientException.cs (4)
ToString(34-37)RestClientException(7-38)RestClientException(9-14)RestClientException(16-22)Activout.RestClient/Serialization/IDeserializer.cs (1)
Task(10-10)Activout.RestClient/Part.cs (2)
Part(4-9)Part(11-18)Activout.RestClient/Serialization/ISerializer.cs (1)
HttpContent(9-9)Activout.RestClient/Serialization/Implementation/StringSerializer.cs (1)
HttpContent(13-14)Activout.RestClient/Serialization/Implementation/ByteArraySerializer.cs (1)
HttpContent(11-23)Activout.RestClient/Serialization/Implementation/FormUrlEncodedSerializer.cs (1)
HttpContent(18-39)Activout.RestClient/Implementation/StringExtensions.cs (1)
SafeSubstring(5-20)
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
Activout.RestClient/Implementation/RequestHandler.cs (4)
1-16: Route content headers torequest.Content.Headersand useTryAddWithoutValidation.Adding e.g., Content-Type via
request.Headers.Add(...)throws. Handle it explicitly and avoid strict validation for custom headers.Apply:
using System.Net.Http; +using System.Net.Http.Headers; ... - private void SetHeaders(HttpRequestMessage request, List<KeyValuePair<string, object>> headers) - { - headers.ForEach(p => request.Headers.Add(p.Key, p.Value.ToString())); - } + private void SetHeaders(HttpRequestMessage request, List<KeyValuePair<string, object>> headers) + { + foreach (var header in headers) + { + var key = header.Key; + var value = header.Value?.ToString(); + if (string.IsNullOrWhiteSpace(value)) continue; + + // Content headers must go to Content.Headers + if (string.Equals(key, "Content-Type", StringComparison.OrdinalIgnoreCase)) + { + request.Content ??= new ByteArrayContent(Array.Empty<byte>()); + request.Content.Headers.ContentType = MediaTypeHeaderValue.Parse(value); + continue; + } + + // Everything else: best-effort add + request.Headers.TryAddWithoutValidation(key, value); + } + }To confirm there are no usages trying to set Content-Type via attributes:
#!/bin/bash # Find attribute usages setting "Content-Type" rg -nP --type=cs -C2 '\[(Header|HeaderParam)Attribute?\s*\(\s*"Content-Type"\s*,'Also applies to: 276-279
494-505: Gate and sanitize body logging to avoid PII/PHI leakage.Even at Debug, emitting raw bodies risks sensitive data exposure. Add an opt‑in flag in context (e.g.,
_context.LogBodies) and sanitize/truncate before logging.Also applies to: 512-519
460-465: TODO left in code — track or resolve.“prepare part serializer in advance” should be implemented or tracked in an issue; avoid TODOs in source.
315-318: Reject null path parameters explicitly; don’t silently skip.Current early
continuehides nulls and leaves{tokens}unresolved. Throw a clear error for path params (annotated and default).- if (rawValue == null) - { - continue; - } + // Defer null handling to attribute-specific branches so path params can be validated ... else if (attribute is PathParamAttribute pathParamAttribute) { - var stringValue = _paramConverters[i].ToString(rawValue); - pathParams[pathParamAttribute.Name ?? parameterName] = Uri.EscapeDataString(stringValue); + if (rawValue is null) + throw new InvalidOperationException( + $"Path parameter '{pathParamAttribute.Name ?? parameterName}' cannot be null."); + var stringValue = _paramConverters[i].ToString(rawValue)!; + pathParams[pathParamAttribute.Name ?? parameterName] = Uri.EscapeDataString(stringValue); handled = true; } ... if (!handled) { - var stringValue = _paramConverters[i].ToString(rawValue); - pathParams[parameterName] = Uri.EscapeDataString(stringValue); + if (rawValue is null) + throw new InvalidOperationException($"Path parameter '{parameterName}' cannot be null."); + var stringValue = _paramConverters[i].ToString(rawValue)!; + pathParams[parameterName] = Uri.EscapeDataString(stringValue); }Also applies to: 346-351, 423-427
🧹 Nitpick comments (6)
Activout.RestClient/Implementation/RequestHandler.cs (6)
169-178: Validate unresolved route tokens after expansion.Throw if any
{...}remain to catch missing/nullable path params early.- return expanded; + if (expanded.IndexOf('{') >= 0 || expanded.IndexOf('}') >= 0) + throw new InvalidOperationException( + $"Unresolved route parameters in template '{_template}'. Expanded: '{expanded}'"); + return expanded;
52-56: Remove redundant_serializerassignment.Assigned from context at Line 52 and overwritten at Lines 100‑102. Keep only the latter.
- _serializer = context.DefaultSerializer; ... _serializer = context.SerializationManager.GetSerializer(_contentType) ?? throw new InvalidOperationException("No serializer found for content type: " + _contentType);Also applies to: 100-102
300-304: Clarify condition precedence.Parentheses improve readability of the arg-count check.
- if (_parameters.Length > 0 && args == null || _parameters.Length != args?.Length) + if ((_parameters.Length > 0 && args == null) || (_parameters.Length != args?.Length))
133-137: Use expression‑bodied members for simple helpers.Aligns with guidelines.
[As per coding guidelines]- private static HttpMethod GetHttpMethod(HttpMethodAttribute attribute) - { - return attribute.HttpMethod; - } + private static HttpMethod GetHttpMethod(HttpMethodAttribute attribute) => attribute.HttpMethod; - private bool IsVoidTask() - { - return _returnType == typeof(Task); - } + private bool IsVoidTask() => _returnType == typeof(Task); - private bool IsGenericTask() - { - return _returnType.BaseType == typeof(Task) && _returnType.IsGenericType; - } + private bool IsGenericTask() => _returnType.BaseType == typeof(Task) && _returnType.IsGenericType;Also applies to: 150-158
40-41: Prefer C# collection expressions ([]) for lists.Simplifies initialization; keeps types explicit.
[As per coding guidelines]- private readonly List<KeyValuePair<string, object>> _requestHeaders = new List<KeyValuePair<string, object>>(); + private readonly List<KeyValuePair<string, object>> _requestHeaders = []; ... - var headers = new List<KeyValuePair<string, object>>(); - headers.AddRange(_requestHeaders); - var routeParams = new Dictionary<string, object>(); - var queryParams = new List<string>(); - var formParams = new List<KeyValuePair<string, string>>(); - var partParams = new List<Part<HttpContent>>(); + List<KeyValuePair<string, object>> headers = [.._requestHeaders]; + var routeParams = new Dictionary<string, object>(); + List<string> queryParams = []; + List<KeyValuePair<string, string>> formParams = []; + List<Part<HttpContent>> partParams = [];Also applies to: 207-214
185-199: Improve exception messages inPrepareRequestMessage.Throw actionable messages when
BaseUri/RequestUriare missing to aid diagnostics.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Activout.RestClient/Implementation/RequestHandler.cs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.cs
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.cs: Use [] list syntax for collections
Use file-scoped namespaces
Use primary constructors wherever possible
Use records for data transfer objects (DTOs) and immutable data structures
Use var for local variable declarations when possible
Use expression-bodied members for simple methods and properties
Use pattern matching for type checks and deconstruction
Use using statements for resource management
Use async and await for asynchronous programming
Never include "Async" in method names
Only include comments that explain why something is done, not what is done
Files:
Activout.RestClient/Implementation/RequestHandler.cs
🧬 Code graph analysis (1)
Activout.RestClient/Implementation/RequestHandler.cs (12)
Activout.RestClient/Implementation/RestClient.cs (3)
RestClient(10-47)RestClient(18-22)IEnumerable(24-27)Activout.RestClient/Helpers/ITaskConverterFactory.cs (1)
ITaskConverter(7-7)Activout.RestClient/Helpers/Implementation/TaskConverter3Factory.cs (1)
ITaskConverter(9-14)Activout.RestClient/Serialization/Implementation/SerializationManager.cs (3)
ISerializer(43-47)SerializationManager(8-48)SerializationManager(30-35)Activout.RestClient/ParamConverter/Implementation/ParamConverterManager.cs (3)
IParamConverter(19-30)ParamConverterManager(7-31)ParamConverterManager(13-17)Activout.RestClient/Implementation/HeaderListExtensions.cs (1)
AddOrReplaceHeader(8-18)Activout.RestClient/ParamConverter/IParamConverter.cs (1)
ToString(10-10)Activout.RestClient/RestClientException.cs (4)
ToString(34-37)RestClientException(7-38)RestClientException(9-14)RestClientException(16-22)Activout.RestClient/Serialization/IDeserializer.cs (1)
Task(10-10)Activout.RestClient/Part.cs (2)
Part(4-9)Part(11-18)Activout.RestClient/Serialization/ISerializer.cs (1)
HttpContent(9-9)Activout.RestClient/Implementation/StringExtensions.cs (1)
SafeSubstring(5-20)
🔇 Additional comments (2)
Activout.RestClient/Implementation/RequestHandler.cs (2)
329-337: Good fix: avoid iterating strings as collections.
and not stringprevents char-by-char multipart parts. LGTM.
531-575: Response disposal logic is correct.Disposes
HttpResponseMessageexcept when returning message orHttpContent. Prevents handler/socket exhaustion.
1 participant
Summary by CodeRabbit
Bug Fixes
Refactor
Tests