Skip to content
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

Losing information in unexpected error scenarios #138

Closed
drhoroscope opened this issue Sep 5, 2018 · 4 comments

Comments

Projects
None yet
3 participants
@drhoroscope
Copy link
Contributor

commented Sep 5, 2018

In HttpHelper.cs we find this bit:

                if (response.IsSuccessStatusCode)
                {
                    // ...
                }
                else
                {
                    Debug.WriteLine(responseString);
                    var badrequest = JsonConvert.DeserializeObject<BadRequest>(responseString);
                    throw new WPException(badrequest.Message, badrequest);
                }

This works fine when you're querying a valid WordPress API well enough so you get a well formed bad request response. But in a wide range of error scenarios the call to DeserializeObject() blows up and throws an exception up the stack without logging some critical information, such as the HTTP response code and the URL being referenced. The Debug statement is arguably some help, but in most configurations that trace isn't going to hit a log file, which means if you've debugging a server issue you're staring at a now familiar error:

Newtonsoft.Json.JsonReaderException: Unexpected character encountered while parsing value: <. Path '', line 0, position 0

with no actionable information about what the underlying problem is. My suggestion is that we use an error handling delegate to catch the deserialization error and, in the event of a deserialization error, we record context information, including the HTTP status code, request URL, and possibly other details in some standard format, either using the existing BadRequest structure or possibly a new data format created for this purpose.

I'd be happy to submit a PR for this, but just wanted to get some review and guidance first.

@ThomasPe

This comment has been minimized.

Copy link
Member

commented Sep 6, 2018

I agree that error handling should be definitely improved and I'm certainly open to suggestions / PRs!
@polushinmk any thoughts on this?

@polushinmk

This comment has been minimized.

Copy link
Contributor

commented Sep 6, 2018

@ThomasPe @drhoroscope Yeah, I agree too that this is good suggestion. For now we really loosing info about non wordpress-specific bad requests. What about realisation? I see these approaches:

  • If Json deserialization to WPBadRequest fails - throw an exception with HTTP request&responce info
  • add a delegate (like HttpPrePocessing) which can be used for log all requests and responces info

and, I think, we could combine them together.

@drhoroscope

This comment has been minimized.

Copy link
Contributor Author

commented Sep 6, 2018

@polushinmk sounds good - I will work on a PR for this

drhoroscope added a commit to drhoroscope/WordPressPCL that referenced this issue Sep 8, 2018

ThomasPe added a commit that referenced this issue Sep 10, 2018

@ThomasPe

This comment has been minimized.

Copy link
Member

commented Sep 10, 2018

Package has been updated on nuget.org.

@ThomasPe ThomasPe closed this Sep 10, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.