-
-
Notifications
You must be signed in to change notification settings - Fork 14
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
Report errors from backend when a request fails #139
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@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. For example in Then we have other places like 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 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 |
Yeah, let's keep it in mind for the upcoming refactor. |
For I am on record as being very impressed by So yeah, offering my two cents, they really get around those two pennies. If there is a super amazing perfect elegant tech solution available here, then nice, otherwise I don't mind the next best thing. We can always improve it again later if the will and the ability and free time coincide to do that. |
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 not200, 201, 204, 404
, which we expect, but oftentimes when the request fails, the rest of PPM ends that interaction likereturn void reject(error);
which askssuperagent
what the error was, which is only ever generated based off the HTTP status code returned, such as500 = 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, butsuperagent
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 fromsuperagent
if needed. This way we get the best of both worlds.With this changes made, I've turned this test command output:
Into the new output (using the error returned directly by the backend:
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 requestFrom 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 thepackage.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 getServer 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.