Report errors from backend when a request fails

[confused-Techie]

As many of you already know, often times a user reports an issue with the backend, stating they have no helpful error message to go off of. Just receiving Internal Server Error.

We had always assumed that this was a failing of the backend, that errors weren't being created there that can help the user. While in some cases this is true, over time I have continued to add more and more intelligent error messages, and written tests to confirm this gets back to the user.

That is until I finally put this to the test and realized that PPM itself is at fault for eating our helpful error messages.

Basically when a request is made via request.js it can error when the returned status code is not 200, 201, 204, 404, which we expect, but oftentimes when the request fails, the rest of PPM ends that interaction like return void reject(error); which asks superagent what the error was, which is only ever generated based off the HTTP status code returned, such as 500 = Internal Server Error.

That's why we see it so often, when a request on the backend errors out it returns the 500 HTTP Status Code, along with helpful information in the returned JSON, but superagent doesn't look there when asked to return an error to the rest of PPM.

So what I've done is in this PR, is that whenever a request using our backend fails, we always call request.getErrorMessage() which will either find an error from the backend if it exists, or will get an error message from superagent if needed. This way we get the best of both worlds.

With this changes made, I've turned this test command output:

Preparing and tagging a new version done
Pushing v1.6.0 tag done
Publishing autocomplete-powershell-winserver2022@v1.6.0 failed
Internal Server Error

Into the new output (using the error returned directly by the backend:

Preparing and tagging a new version done
Pushing v1.16.0 tag done
Publishing autocomplete-powershell-winserver2022@v1.16.0 failed
Creating new version failed: 'Server Error: From Bad Repo: Failed to get repo: onfused-Techie/autocomplete-powershell-winserver2022 - Server Error'

In this new output, oftentimes the user would now be able to see where they messed up and resolve the issue on their own, or if they cannot once provided to us, each section of the error message has meaning to where we may be able to tell the user exactly what went wrong.

Such as in this example case:

• Server Error: The status the backend is returning for the request
• From Bad Repo: The sub-error that caused the status to be returned, bad-repo meaning something about the package's data being incorrect or malformed. Either in the backend itself, or in the package.json on GitHub.
• Failed to get repo: The backend failed to get the repo's details from GitHub.
• onfused-Techie/autocomplete-powershell-winserver2022: The repo the backend attempted to get
• Server Error: The status the backend got when attempting to get the repo.

With just this error message we know that the backend attempted to use package data, to get details of that package from GitHub. But the package string here resulting in GitHub returning a server-error status code. And since this was a version publish we know that the backend already has the package name, meaning the data is malformed on the backend and must be repaired by a database admin. (Keep in mind I purposefully mangled the data of my package for testing purposes)

But hopefully with this example above, it's obvious how much more helpful getting proper error messages into PPM will be, for users and for us.

[savetheclocktower]

I haven't checked out this PR; this is just a reaction to reading the code.

I know src/request.js is on the chopping block, but I wonder if we can't solve this problem within that file instead of having to touch a bunch of other files.

The problem seems to be that the error variable that the callbacks receive has a message property that matches its status code. If we can adjust the message before it makes it to the callback, then life gets easier. Imagine a function like this:

module.exports = {
  // ...
  wrapError (err, body) {
    let message = this.getErrorMessage(body, err);
    err.message = message;
    return err;
  }
  // ...
}

Then each catch clause can look like this…

  return void callback(this.wrapError(error), null, null);

…and suddenly request.(get|post|del) all report errors with useful message properties.

I'm pretty sure the message property can simply be reassigned; if not, we could try to subclass an error, but that's a bit of a headache.

I figure this approach would still be useful in a future where we convert request.js to promises, so I don't think this would be wasted work.

To be clear, though: this isn't blocking feedback. Some of the other changes (restructuring our error/success checks) seem worthwhile, too, and I don't think this feedback is more important than getting this fix into the next release.

[confused-Techie]

@savetheclocktower You make a great point, and that absolutely would work in some cases, but tbh I still think this is our best method forward.

Essentially our current error checking in PPM is all over the place. request.js already had the function getErrorMessage which is behaving much like you mentioned with a wrapError function. It's just that it's usage wasn't great.

For example in featured.js prior to my PR we already used request.getErrorMessage in the exact way we need to, in order to get good error messages. And the same can be said with stars.js. These files and others already are doing exactly what we are doing in this PR. Just in more places.

Then we have other places like upgrade.js who implemented their own nearly identical logic that getErrorMessage provides, but it just wasn't updated when I rewrote request.js a while back.

---

But your idea is sound, just modifying the error object we return, which would fix this in the vast majority of cases, without touching all these files, since often times the error object from a request is returned as is, in most files. So you are totally correct there, but then we still wouldn't get the enhanced error messages when there wasn't technically an error. For example if the statusCode was non-erroring but still isn't correct for that case. So we would still lose out in some instances.

But with that said I'd still support going this route to get enhanced error messages everywhere, and to bring the way we check for errors uniform, since some locations are already doing this logic, but I do very much appreciate the feedback, and if we think this is too much churn for the prospect, your method would probably fix this in 90% of cases, so I would be in no way against that method compared to the current status quo

[savetheclocktower]

Yeah, let's keep it in mind for the upcoming refactor.