Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
Corrects misleading error message when deploying #6348
Conversation
| // Make sure the content type is zip. | ||
| contentType := r.Header.Get("Content-Type") | ||
| if contentType != "application/zip" { | ||
| - return nil, fmt.Errorf("expected Content-Type: application/zip, got: %v", contentType) | ||
| + return nil, errors.BadRequestf("expected Content-Type: application/zip, got: %v", contentType) |
| // TODO frankban: cache parsed template. | ||
| t, err := template.ParseFiles(tmpl) | ||
| if err != nil { | ||
| - sendError(w, errors.Annotate(err, "cannot parse template")) | ||
| - return | ||
| + return errors.Trace(sendError(w, errors.Annotate(err, "cannot parse template"))) |
howbazaar
Sep 28, 2016
Owner
Should we be sending the error and returning an error?
Wouldn't it be nicer to have this method not take a response writer at all, and just return an error, and have the caller send it?
| @@ -49,12 +51,18 @@ func (h *toolsDownloadHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) | ||
| tarball, err := h.processGet(r, st) | ||
| if err != nil { | ||
| logger.Errorf("GET(%s) failed: %v", r.URL, err) | ||
| - sendError(w, errors.NewBadRequest(err, "")) | ||
| + if err := sendError(w, errors.NewBadRequest(err, "")); err != nil { |
howbazaar
Sep 28, 2016
Owner
We seem to have much copying of sendError returning an error that we just log. Wouldn't it make more sense for sendError to just log the error itself and not return an error?
kat-co
Sep 29, 2016
Contributor
For the same reason I thought your other suggestion was good, no, I don't think this is a good idea. I made this change because I couldn't readily tell whether or not we were dealing with failures to send errors correctly.
Errors handling should be lazy; i.e., it should be handled at the last possible moment. This has a few benefits:
- The errors stack gets built up as it gets traced back to where it was called from.
- This gives the caller flexibility in how it's handled and centralizes that handling.
- Making this change percolated throughout the code revealing new error paths; I think it's much clearer now what might fail.
natefinch
Sep 29, 2016
Contributor
Personally, I agree with Tim here. If this is always called from an http handler that itself can't return errors, and we always just log the error, then forcing the repeat of the error handling everywhere is not very useful, and makes the code harder to follow, especially since this is already in an error handling path.
We don't ever branch based on the error, other than to log. Returning it is just not useful. We're in an error path, it's understood that if there are failures in the error path then you're kind of screwed. as long as sendError has an appropriate doc comment that explains that it may fail, but then there's nothign we can do about it, so we just log it... then I think it's fine.
Sure, it's usually nice to know when a function can fail, but if you know you'll never actually be able to do anything about it, and it's just an internal function, then I don't think it's worth the extra code to return that function one level and always handle it the same way.
Plus, if the error handling is inside sendError, then you know the error handling is always the same, without having to copy and paste the code.
Finally, it should be really rare that there's a bad request and the connection dies at just the wrong nanosecond... so we're wasting visual complexity on something that in practice will almost never happen.
| @@ -49,7 +49,6 @@ func (s *registrationSuite) TearDownTest(c *gc.C) { | ||
| s.server.Close() | ||
| } | ||
| -// FAILING |
kat-co
Sep 29, 2016
Contributor
These are just cleanups from a prior patch I landed while I was on PTO. They're just errant comments.
| @@ -465,7 +465,7 @@ func (s *serviceSuite) TestAddCharm(c *gc.C) { | ||
| func (s *serviceSuite) TestAddCharmWithAuthorization(c *gc.C) { | ||
| // Upload a new charm to the charm store. | ||
| - curl, _ := s.UploadCharm(c, "cs:~restricted/precise/wordpress-3", "wordpress") | ||
| + curl, _ := s.UploadCharm(c, "cs:precise/wordpress/~restricted/3", "wordpress") |
| @@ -45,14 +44,16 @@ func (h *charmsHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) { | ||
| var err error | ||
| switch r.Method { | ||
| case "POST": | ||
| - err = h.servePost(w, r) | ||
| + err = errors.Annotate(h.servePost(w, r), "cannot upload charm") |
natefinch
Sep 29, 2016
Contributor
if it were me, I'd split this into two lines... the error handling here obscures what's actually being done, IMO... but that's mostly trivial.
| @@ -141,10 +138,9 @@ func (h *charmsHandler) manifestSender(w http.ResponseWriter, r *http.Request, b | ||
| if err != nil { | ||
| return errors.Annotatef(err, "unable to read manifest in %q", bundle.Path) | ||
| } | ||
| - sendStatusAndJSON(w, http.StatusOK, ¶ms.CharmsResponse{ | ||
| + return errors.Trace(sendStatusAndJSON(w, http.StatusOK, ¶ms.CharmsResponse{ |
| @@ -49,12 +51,18 @@ func (h *toolsDownloadHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) | ||
| tarball, err := h.processGet(r, st) | ||
| if err != nil { | ||
| logger.Errorf("GET(%s) failed: %v", r.URL, err) | ||
| - sendError(w, errors.NewBadRequest(err, "")) | ||
| + if err := sendError(w, errors.NewBadRequest(err, "")); err != nil { |
howbazaar
Sep 28, 2016
Owner
We seem to have much copying of sendError returning an error that we just log. Wouldn't it make more sense for sendError to just log the error itself and not return an error?
kat-co
Sep 29, 2016
Contributor
For the same reason I thought your other suggestion was good, no, I don't think this is a good idea. I made this change because I couldn't readily tell whether or not we were dealing with failures to send errors correctly.
Errors handling should be lazy; i.e., it should be handled at the last possible moment. This has a few benefits:
- The errors stack gets built up as it gets traced back to where it was called from.
- This gives the caller flexibility in how it's handled and centralizes that handling.
- Making this change percolated throughout the code revealing new error paths; I think it's much clearer now what might fail.
natefinch
Sep 29, 2016
Contributor
Personally, I agree with Tim here. If this is always called from an http handler that itself can't return errors, and we always just log the error, then forcing the repeat of the error handling everywhere is not very useful, and makes the code harder to follow, especially since this is already in an error handling path.
We don't ever branch based on the error, other than to log. Returning it is just not useful. We're in an error path, it's understood that if there are failures in the error path then you're kind of screwed. as long as sendError has an appropriate doc comment that explains that it may fail, but then there's nothign we can do about it, so we just log it... then I think it's fine.
Sure, it's usually nice to know when a function can fail, but if you know you'll never actually be able to do anything about it, and it's just an internal function, then I don't think it's worth the extra code to return that function one level and always handle it the same way.
Plus, if the error handling is inside sendError, then you know the error handling is always the same, without having to copy and paste the code.
Finally, it should be really rare that there's a bad request and the connection dies at just the wrong nanosecond... so we're wasting visual complexity on something that in practice will almost never happen.
|
$$merge$$ |
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju |
|
Build failed: Tests failed |
|
$$merge$$ |
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju |
|
Build failed: Tests failed |
|
$$merge$$ |
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju |
|
Build failed: Tests failed |
|
$$merge$$ |
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju |
kat-co commentedSep 28, 2016
This addresses lp:1616200, with caveats.
Previously deploying a charm with an incorrect name would:
Subsequent deploys of the charm with an incorrect name would display:
This patch corrects the chain of errors to have proper causes which implicitly corrects the error message to:
or series, or whatever the case may be. However, because it fails faster, it takes a different path through the code. The bad charm won't be added to the
charmsmongo collection (good), but we now always see the verbose POST prepended in front of the error (bad).To correct this properly, I think we should modify the juju/httprequest
Domethod. I plan on doing this in a subsequent patch.