Skip to content

throw specific HTTPStatusException on http request errors #5551

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.

Sign up for GitHub

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
dlang-bot merged 1 commit into from
Jul 8, 2017
Merged

throw specific HTTPStatusException on http request errors #5551

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 31 additions & 4 deletions std/net/curl.d
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1047,9 +1047,8 @@ private auto _basicHTTP(T)(const(char)[] url, const(void)[] sendData, HTTP clien
};
client.onReceiveStatusLine = (HTTP.StatusLine l) { statusLine = l; };
client.perform();
enforce!CurlException(statusLine.code / 100 == 2,
format("HTTP request returned status code %d (%s)",
statusLine.code, statusLine.reason));
enforce(statusLine.code / 100 == 2, new HTTPStatusException(statusLine.code,
format("HTTP request returned status code %d (%s)", statusLine.code, statusLine.reason)));
Copy link
Contributor

@wilzbach wilzbach Jul 6, 2017

Choose a reason for hiding this comment

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

This message seems to be rather generic - what was the motivation for not generating this message in HTTPStatusException (or providing a second overload)?

Copy link
Member Author

@MartinNowak MartinNowak Jul 7, 2017

Choose a reason for hiding this comment

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

It's the baseline of a useful error message but doesn't contain enough information on it's own (e.g. the url or the status reason), those are only available by the calling code.

Copy link
Contributor

@wilzbach wilzbach Jul 8, 2017

Choose a reason for hiding this comment

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

Yes that was my point - if no specific message is passed, this could be the baseline.

Copy link
Member

@andralex andralex Jul 8, 2017

Choose a reason for hiding this comment

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

So... not sure what the proposal is here.

Copy link
Contributor

@wilzbach wilzbach Jul 8, 2017

Choose a reason for hiding this comment

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

Sth. like below could be handy, but I don't feel strongly about this (I am not using std.net.curl anyways):

@safe pure nothrow
this(int status,
  string file = __FILE__,
  size_t line = __LINE__,
  Throwable next = null)
{
  const msg = format("HTTP request returned status code %d", status);
  super(msg, file, line, next);
  this.status = status;
}

@safe pure nothrow
this(int status,
  string msg,
  string file = __FILE__,
  size_t line = __LINE__,
  Throwable next = null)
{
  const _msg = format("HTTP request returned status code %d (%s)", status, msg);
  super(_msg, file, line, next);
  this.status = status;
}


return _decodeContent!T(content.data, client.p.charset);
}
Expand All @@ -1063,8 +1062,9 @@ private auto _basicHTTP(T)(const(char)[] url, const(void)[] sendData, HTTP clien
assert(req.hdrs.canFind("GET /path"));
s.send(httpNotFound());
});
auto e = collectException!CurlException(get(testServer.addr ~ "/path"));
auto e = collectException!HTTPStatusException(get(testServer.addr ~ "/path"));
assert(e.msg == "HTTP request returned status code 404 (Not Found)");
assert(e.status == 404);
}

// Bugzilla 14760 - content length must be reset after post
Expand Down Expand Up @@ -4058,6 +4058,33 @@ class CurlTimeoutException : CurlException
}
}

/++
Exception thrown on HTTP request failures, e.g. 404 Not Found.
+/
class HTTPStatusException : CurlException
{
/++
Copy link
Member

@andralex andralex Jul 8, 2017

Choose a reason for hiding this comment

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

eh, bug in -cov

Params:
status = The HTTP status code.
msg = The message for the exception.
file = The file where the exception occurred.
line = The line number where the exception occurred.
Copy link
Member

@andralex andralex Jul 8, 2017

Choose a reason for hiding this comment

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

eh, another bug in -cov

Copy link
Member Author

@MartinNowak MartinNowak Jul 8, 2017

Choose a reason for hiding this comment

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

Looks weird https://codecov.io/gh/dlang/phobos/compare/ecef6f0e3b3da14d483b3dd6e05d8780d16fd719...97e9db7f02ea072acb1a3f1bd11e3a1f3fca193a/changes, but if I run the single test locally dmd reports correct coverage.
It's also weird what we're doing the circleci script, looks like tests are supposed to run twice but fail to do so.
https://circleci.com/gh/dlang/phobos/3504 (no log line links in circleci :().

next = The previous exception in the chain of exceptions, if any.
+/
@safe pure nothrow
this(int status,
string msg,
string file = __FILE__,
size_t line = __LINE__,
Throwable next = null)
{
super(msg, file, line, next);
this.status = status;
}

immutable int status; /// The HTTP status code
}

/// Equal to $(REF CURLcode, etc,c,curl)
alias CurlCode = CURLcode;

Expand Down