Skip to content

Enhance error logging in ApiRequestHelper to include received content on parsing failure #4895

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
May 5, 2025

Conversation

gautamdsheth
Copy link
Collaborator

Before creating a pull request, make sure that you have read the contribution file located at

https://github.com/pnp/powerShell/blob/dev/CONTRIBUTING.md

Type

  • Bug Fix
  • New Feature
  • Sample

Related Issues?

Fixes #X, partially fixes #Y, mentioned in #Z, etc.

What is in this Pull Request ?

Please describe the changes in the PR.

Guidance

  • You can delete this section when you are submitting the pull request.*
  • Please update this PR information accordingly. We use this as part of our release notes in monthly communications.
  • Please target your PR to Dev branch. If you do not target the Dev branch we will not accept this PR.

@gautamdsheth gautamdsheth requested a review from Copilot May 5, 2025 09:03
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR improves the error logging in ApiRequestHelper to include the received content when parsing fails. It enhances the logged message by escaping curly braces in the response content and removes a commented-out warning log.

@@ -263,7 +263,7 @@ public T Get<T>(string url, bool camlCasePolicy = true, bool propertyNameCaseIns
}
catch (Exception e)
{
LogError($"Failed to parse response from server. Error message: '{e.Message}'. Model type to parse it to: '{typeof(T)}'.");
LogError($"Failed to parse response from server. Error message: '{e.Message}'. Received content: '{stringContent.Replace("{", "{{").Replace("}", "}}")}'. Model type to parse it to: '{typeof(T)}'.");
Copy link
Preview

Copilot AI May 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] Consider extracting the transformation logic for 'stringContent' into a dedicated helper function to improve clarity and maintainability of the logging statement.

Suggested change
LogError($"Failed to parse response from server. Error message: '{e.Message}'. Received content: '{stringContent.Replace("{", "{{").Replace("}", "}}")}'. Model type to parse it to: '{typeof(T)}'.");
LogError($"Failed to parse response from server. Error message: '{e.Message}'. Received content: '{EscapeCurlyBraces(stringContent)}'. Model type to parse it to: '{typeof(T)}'.");

Copilot uses AI. Check for mistakes.

@gautamdsheth gautamdsheth merged commit 9ccebde into pnp:dev May 5, 2025
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant