chore: Add unsuccessfull response content logging #1324
Conversation
📝 WalkthroughWalkthroughThe pull request introduces enhancements to error handling and response validation in the Changes
Possibly related PRs
Suggested reviewers
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
Documentation and Community
|
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
src/Digdir.Domain.Dialogporten.Infrastructure/HttpResponseMessageExtensions.cs (1)
5-5: Consider makingMaxContentLengthconfigurableHardcoding the
MaxContentLengthto 1000 may not be flexible for different deployment environments or future requirements. Consider making this value configurable through application settings.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- src/Digdir.Domain.Dialogporten.Infrastructure/HttpClientExtensions.cs (1 hunks)
- src/Digdir.Domain.Dialogporten.Infrastructure/HttpResponseMessageExtensions.cs (1 hunks)
🧰 Additional context used
🔇 Additional comments (2)
src/Digdir.Domain.Dialogporten.Infrastructure/HttpResponseMessageExtensions.cs (1)
31-34: LGTM!The
GetResponseCodeWithReasonPhrasemethod is well-implemented and handles cases whereReasonPhrasemight be null or whitespace.src/Digdir.Domain.Dialogporten.Infrastructure/HttpClientExtensions.cs (1)
51-52: Enhanced error handling withEnsureSuccessStatusCodeWithContent().Replacing
EnsureSuccessStatusCode()withEnsureSuccessStatusCodeWithContent()improves the logging of unsuccessful response content, aligning with the PR objective.
| var content = await response.Content.ReadAsStringAsync(); | ||
| if (string.IsNullOrWhiteSpace(content)) | ||
| { | ||
| // This will throw HttpRequestException with the response status code and reason phrase | ||
| return response.EnsureSuccessStatusCode(); | ||
| } | ||
|
|
||
| if (content.Length > MaxContentLength) | ||
| { | ||
| content = content[..MaxContentLength] + "[truncated after " + MaxContentLength + " characters]"; | ||
| } | ||
|
|
||
| throw new HttpRequestException("Response unsuccessful (" + response.GetResponseCodeWithReasonPhrase() + | ||
| " with error content: " + content, null, response.StatusCode); | ||
|
|
||
| } |
There was a problem hiding this comment.
Avoid including response content in exception messages to prevent sensitive data leakage
Including the response content in exception messages could inadvertently expose sensitive or confidential information if exceptions are logged or displayed. Consider removing the response content from the exception message and logging it securely instead.
Apply this diff to modify the exception handling:
- throw new HttpRequestException("Response unsuccessful (" + response.GetResponseCodeWithReasonPhrase() +
- " with error content: " + content, null, response.StatusCode);
+ // Optionally log 'content' securely here
+ throw new HttpRequestException("Response unsuccessful (" + response.GetResponseCodeWithReasonPhrase() + ")", null, response.StatusCode);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| var content = await response.Content.ReadAsStringAsync(); | |
| if (string.IsNullOrWhiteSpace(content)) | |
| { | |
| // This will throw HttpRequestException with the response status code and reason phrase | |
| return response.EnsureSuccessStatusCode(); | |
| } | |
| if (content.Length > MaxContentLength) | |
| { | |
| content = content[..MaxContentLength] + "[truncated after " + MaxContentLength + " characters]"; | |
| } | |
| throw new HttpRequestException("Response unsuccessful (" + response.GetResponseCodeWithReasonPhrase() + | |
| " with error content: " + content, null, response.StatusCode); | |
| } | |
| var content = await response.Content.ReadAsStringAsync(); | |
| if (string.IsNullOrWhiteSpace(content)) | |
| { | |
| // This will throw HttpRequestException with the response status code and reason phrase | |
| return response.EnsureSuccessStatusCode(); | |
| } | |
| if (content.Length > MaxContentLength) | |
| { | |
| content = content[..MaxContentLength] + "[truncated after " + MaxContentLength + " characters]"; | |
| } | |
| // Optionally log 'content' securely here | |
| throw new HttpRequestException("Response unsuccessful (" + response.GetResponseCodeWithReasonPhrase() + ")", null, response.StatusCode); | |
| } |
| var content = await response.Content.ReadAsStringAsync(); | ||
| if (string.IsNullOrWhiteSpace(content)) | ||
| { | ||
| // This will throw HttpRequestException with the response status code and reason phrase | ||
| return response.EnsureSuccessStatusCode(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Inconsistent exception handling between empty and non-empty content cases
When the response content is empty, the method calls response.EnsureSuccessStatusCode(), which throws an HttpRequestException with standard message formatting. However, when content is present, a new HttpRequestException is thrown with a custom message. This inconsistency may lead to confusion when handling exceptions. Consider standardizing the exception handling mechanism.
Apply this diff to standardize exception handling:
- return response.EnsureSuccessStatusCode();
+ throw new HttpRequestException("Response unsuccessful (" + response.GetResponseCodeWithReasonPhrase() + ")", null, response.StatusCode);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| var content = await response.Content.ReadAsStringAsync(); | |
| if (string.IsNullOrWhiteSpace(content)) | |
| { | |
| // This will throw HttpRequestException with the response status code and reason phrase | |
| return response.EnsureSuccessStatusCode(); | |
| } | |
| var content = await response.Content.ReadAsStringAsync(); | |
| if (string.IsNullOrWhiteSpace(content)) | |
| { | |
| // This will throw HttpRequestException with the response status code and reason phrase | |
| throw new HttpRequestException("Response unsuccessful (" + response.GetResponseCodeWithReasonPhrase() + ")", null, response.StatusCode); | |
| } |
| @@ -48,7 +48,8 @@ public static async Task<HttpResponseMessage> PostAsJsonEnsuredAsync( | |||
| configureHeaders?.Invoke(httpRequestMessage.Headers); | |||
| configureContentHeaders?.Invoke(httpRequestMessage.Content.Headers); | |||
| var response = await client.SendAsync(httpRequestMessage, cancellationToken); | |||
| response.EnsureSuccessStatusCode(); | |||
| await response.EnsureSuccessStatusCodeWithContent(); | |||
There was a problem hiding this comment.
Det er litt skummelt å logge response body for alle kontekster. Meg bekjent brukes denne extension for mer enn bare altinn auth, og det å logge respons (som kan være sensitiv) som en del av exception er litt så-som-så. Vi kan heller sette "Serilog__MinimumLevel__Override__Digdir.Domain.Dialogporten.Infrastructure.Altinn.Authorization.AltinnAuthorizationClient": "Debug" i stage. Det har jeg gjort gjennom app configuration allerede.
There was a problem hiding this comment.
Enig i at dette i utgangspunktet er litt sketch. Vil det å overridde loglevel her gjøre at vi får logga response body?
There was a problem hiding this comment.
Nei, vi skal (ut i fra koden) få request body som vil si at vi kan reprodusere selv via postman.
There was a problem hiding this comment.
Men det viktige er at vi får request body for kun denne konteksten så lenge vi har på debug logging
There was a problem hiding this comment.
Ok, hvis vi skal lene oss på ILogger (som jeg er enig i gir mening), må vi implementere dette et annet sted enn i disse extension metodene. Evt må vi hekte på content i exceptionen på noe vis og hente det ut og logge det et annet sted i pipelinen.
Description
Related Issue(s)
Verification
Documentation
docs-directory, Altinnpedia or a separate linked PR in altinn-studio-docs., if applicable)Summary by CodeRabbit
New Features
Bug Fixes