From ef08d499cec8c33fc4308cb951aa9dcb15d5db49 Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Fri, 15 Jan 2021 02:59:54 +0100 Subject: [PATCH] Remove unnecessary SetIgnoreServerErrorStatus() from the API It's up to the application code to decide how it handles the HTTP status codes it gets back from server, there is no need to have a special method for handling them in wxWebRequest itself. We also shouldn't skip downloading the response body just because it was unsuccessful, we may still need it (e.g. it's very common to return the detailed error description in a JSON object in the message body when returning some 4xx error), so don't do it in wxMSW implementation and add a test verifying that we still get the expected body even for an error status. Also improve wxWebRequest::State values documentation. --- include/wx/private/webrequest.h | 7 +++---- include/wx/webrequest.h | 2 -- interface/wx/webrequest.h | 35 +++++++++++++++++--------------- src/common/webrequest.cpp | 27 ++++++++++++------------ src/common/webrequest_curl.cpp | 10 ++++++--- src/msw/webrequest_winhttp.cpp | 4 ++-- src/osx/webrequest_urlsession.mm | 3 +-- tests/net/webrequest.cpp | 15 ++++++++++++++ 8 files changed, 60 insertions(+), 43 deletions(-) diff --git a/include/wx/private/webrequest.h b/include/wx/private/webrequest.h index a2f9299ca2..9a54668ae3 100644 --- a/include/wx/private/webrequest.h +++ b/include/wx/private/webrequest.h @@ -65,8 +65,6 @@ public: bool SetData(wxScopedPtr& dataStream, const wxString& contentType, wxFileOffset dataSize = wxInvalidOffset); - void SetIgnoreServerErrorStatus(bool ignore) { m_ignoreServerErrorStatus = ignore; } - void SetStorage(wxWebRequest::Storage storage) { m_storage = storage; } wxWebRequest::Storage GetStorage() const { return m_storage; } @@ -111,7 +109,9 @@ protected: wxWebRequestImpl(wxWebSession& session, wxEvtHandler* handler, int id); - bool CheckServerStatus(); + // Call SetState() with either State_Failed or State_Completed appropriate + // for the response status. + void SetFinalStateFromStatus(); static bool IsActiveState(wxWebRequest::State state); @@ -120,7 +120,6 @@ private: wxEvtHandler* const m_handler; const int m_id; wxWebRequest::State m_state; - bool m_ignoreServerErrorStatus; wxFileOffset m_bytesReceived; wxCharBuffer m_dataText; diff --git a/include/wx/webrequest.h b/include/wx/webrequest.h index 42e6312ad5..52499915c8 100644 --- a/include/wx/webrequest.h +++ b/include/wx/webrequest.h @@ -157,8 +157,6 @@ public: bool SetData(wxInputStream* dataStream, const wxString& contentType, wxFileOffset dataSize = wxInvalidOffset); - void SetIgnoreServerErrorStatus(bool ignore); - void SetStorage(Storage storage); Storage GetStorage() const; diff --git a/interface/wx/webrequest.h b/interface/wx/webrequest.h index 1d8cf06a36..11e592eebc 100644 --- a/interface/wx/webrequest.h +++ b/interface/wx/webrequest.h @@ -129,18 +129,33 @@ public: State_Idle, /** - The request is currently unauthorized. Calling GetAuthChallenge() - returns a challenge object with further details. + The request is currently unauthorized. + + Calling GetAuthChallenge() returns a challenge object with further + details and calling SetCredentials() on this object will retry the + request using these credentials. */ State_Unauthorized, /// The request has been started and is transferring data State_Active, - /// The request completed successfully and all data has been received. + /** + The request completed successfully and all data has been received. + + The HTTP status code returned by wxWebResponse::GetStatus() will be + in 100-399 range, and typically 200. + */ State_Completed, - /// The request failed + /** + The request failed. + + This can happen either because the request couldn't be performed at + all (e.g. a connection error) or if the server returned an HTTP + error. In the former case wxWebResponse::GetStatus() returns 0, + while in the latter it returns a value in 400-599 range. + */ State_Failed, /// The request has been cancelled before completion by calling Cancel() @@ -321,18 +336,6 @@ public: bool SetData(wxInputStream* dataStream, const wxString& contentType, wxFileOffset dataSize = wxInvalidOffset); - /** - Instructs the request to ignore server error status codes. - - Per default, server side errors (status code 400-599) will enter - the State_Failed state just like network errors, but - if the response is still required in such cases (e.g. to get more - details from the response body), set this option to ignore all errors. - If ignored, wxWebResponse::GetStatus() has to be checked - from the State_Completed event handler. - */ - void SetIgnoreServerErrorStatus(bool ignore); - /** Sets how response data will be stored. diff --git a/src/common/webrequest.cpp b/src/common/webrequest.cpp index 93e967a896..a405b3fcef 100644 --- a/src/common/webrequest.cpp +++ b/src/common/webrequest.cpp @@ -63,22 +63,28 @@ wxWebRequestImpl::wxWebRequestImpl(wxWebSession& session, wxEvtHandler* handler, m_handler(handler), m_id(id), m_state(wxWebRequest::State_Idle), - m_ignoreServerErrorStatus(false), m_bytesReceived(0) { } -bool wxWebRequestImpl::CheckServerStatus() +void wxWebRequestImpl::SetFinalStateFromStatus() { const wxWebResponseImplPtr& resp = GetResponse(); - if ( resp && resp->GetStatus() >= 400 && !m_ignoreServerErrorStatus ) + if ( !resp || resp->GetStatus() >= 400 ) { - SetState(wxWebRequest::State_Failed, wxString::Format(_("Error: %s (%d)"), - resp->GetStatusText(), resp->GetStatus())); - return false; + wxString err; + if ( resp ) + { + err.Printf(_("Error: %s (%d)"), + resp->GetStatusText(), resp->GetStatus()); + } + + SetState(wxWebRequest::State_Failed, err); } else - return true; + { + SetState(wxWebRequest::State_Completed); + } } bool wxWebRequestImpl::IsActiveState(wxWebRequest::State state) @@ -360,13 +366,6 @@ wxWebRequest::SetData(wxInputStream* dataStream, return m_impl->SetData(streamPtr, contentType, dataSize); } -void wxWebRequest::SetIgnoreServerErrorStatus(bool ignore) -{ - wxCHECK_IMPL_VOID(); - - m_impl->SetIgnoreServerErrorStatus(ignore); -} - void wxWebRequest::SetStorage(Storage storage) { wxCHECK_IMPL_VOID(); diff --git a/src/common/webrequest_curl.cpp b/src/common/webrequest_curl.cpp index 3e9c7053cd..d2927039b9 100644 --- a/src/common/webrequest_curl.cpp +++ b/src/common/webrequest_curl.cpp @@ -272,16 +272,20 @@ void wxWebRequestCURL::HandleCompletion() { int status = m_response ? m_response->GetStatus() : 0; - if ( status == 0) + if ( status == 0 ) + { SetState(wxWebRequest::State_Failed, GetError()); + } else if ( status == 401 || status == 407 ) { m_authChallenge.reset(new wxWebAuthChallengeCURL( (status == 407) ? wxWebAuthChallenge::Source_Proxy : wxWebAuthChallenge::Source_Server, *this)); SetState(wxWebRequest::State_Unauthorized, m_response->GetStatusText()); } - else if ( CheckServerStatus() ) - SetState(wxWebRequest::State_Completed); + else + { + SetFinalStateFromStatus(); + } } wxString wxWebRequestCURL::GetError() const diff --git a/src/msw/webrequest_winhttp.cpp b/src/msw/webrequest_winhttp.cpp index 9aa07a0f3d..99ba8380d0 100644 --- a/src/msw/webrequest_winhttp.cpp +++ b/src/msw/webrequest_winhttp.cpp @@ -170,7 +170,7 @@ wxWebRequestWinHTTP::HandleCallback(DWORD dwInternetStatus, } else { - SetState(wxWebRequest::State_Completed); + SetFinalStateFromStatus(); } break; @@ -259,7 +259,7 @@ void wxWebRequestWinHTTP::CreateResponse() else SetFailedWithLastError(); } - else if ( CheckServerStatus() ) + else { // Start reading the response if ( !m_response->ReadData() ) diff --git a/src/osx/webrequest_urlsession.mm b/src/osx/webrequest_urlsession.mm index fb8a126458..822c65dda9 100644 --- a/src/osx/webrequest_urlsession.mm +++ b/src/osx/webrequest_urlsession.mm @@ -235,8 +235,7 @@ void wxWebRequestURLSession::HandleCompletion() break; default: - if ( CheckServerStatus() ) - SetState(wxWebRequest::State_Completed); + SetFinalStateFromStatus(); } } diff --git a/tests/net/webrequest.cpp b/tests/net/webrequest.cpp index 115ae97b14..2b63851e12 100644 --- a/tests/net/webrequest.cpp +++ b/tests/net/webrequest.cpp @@ -192,6 +192,21 @@ TEST_CASE_METHOD(RequestFixture, Run(wxWebRequest::State_Failed, 404); } +TEST_CASE_METHOD(RequestFixture, + "WebRequest::Error::Body", "[net][webrequest][error]") +{ + if ( !InitBaseURL() ) + return; + + // We can't use the same httpbin.org server that we use for the other tests + // for this one because it doesn't return anything in the body when + // returning an error status code, so use another one. + CreateAbs("https://httpstat.us/418"); + Run(wxWebRequest::State_Failed, 418); + + CHECK( request.GetResponse().AsString() == "418 I'm a teapot" ); +} + TEST_CASE_METHOD(RequestFixture, "WebRequest::Error::Connect", "[net][webrequest][error]") {