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

@@ -65,8 +65,6 @@ public:
 
     bool SetData(wxScopedPtr<wxInputStream>& 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;
 

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;

interface/wx/webrequest.h

@@ -129,18 +129,33 @@ public:
         State_Idle,
 
         /**
-            The request is currently unauthorized. Calling GetAuthChallenge()
+            The request is currently unauthorized.
-            returns a challenge object with further details.
+
+            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.
 

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)"),
+        wxString err;
-            resp->GetStatusText(), resp->GetStatus()));
+        if ( resp )
-        return false;
+        {
+            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();

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() )
+    else
-        SetState(wxWebRequest::State_Completed);
+    {
+        SetFinalStateFromStatus();
+    }
 }
 
 wxString wxWebRequestCURL::GetError() const

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() )

src/osx/webrequest_urlsession.mm

@@ -235,8 +235,7 @@ void wxWebRequestURLSession::HandleCompletion()
             break;
 
         default:
-            if ( CheckServerStatus() )
+            SetFinalStateFromStatus();
-                SetState(wxWebRequest::State_Completed);
     }
 }
 

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]")
 {