Skip to content
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

Better follow redirection for HTTPClient #7157

Merged
merged 7 commits into from
Mar 25, 2020
Merged

Better follow redirection for HTTPClient #7157

merged 7 commits into from
Mar 25, 2020

Conversation

AgainPsychoX
Copy link
Contributor

@AgainPsychoX AgainPsychoX commented Mar 17, 2020

Solves #7156

First commit adds way to force follow redirections. The RFC document does not allow to redirect request with methods other than GET/HEAD without any user confirmation. To workaround that, and there is now setFollowRedirects(followRedirects_t follow) which allow library user to choose between following redirects as RFC allow without user confirmation (default); or force redirects - as user confirmed the redirection. There is still setFollowRedirects(bool follow) left, marked as deprecated.

Second commit makes the client follow most other implementations about 302 HTTP code (Found (1.1), previously Moved temporarily (1.0)). As mentioned already in the RFC:

Note: RFC 1945 and RFC 2068 specify that the client is not allowed
      to change the method on the redirected request.  However, **most
      existing user agent implementations treat 302 as if it were a 303
      response**, performing a GET on the Location field-value regardless
      of the original request method. The status codes 303 and 307 have
      been added for servers that wish to make unambiguously clear which
      kind of reaction is expected of the client.

This behavior is required in some cases, even in modern world. My today's example: Google Scripts Apps endpoint. I have POST handler setup to run some macro. It should response with JSON containing number of affected rows, but the server handle first POST request (execute macro) and response with redirect request with 302 and other location. Then my code needs to GET from the other location. By browsers (Chrome&Firefox) it works just nicely - as they understand 302 "correctly" (well, not in terms of HTTP 1.0). This commit brings this important behavior to the library.

Also, there is a small rewrite of sendRequest code just to make it more clear, without do {} while();. Used recursion - as it was also used in other parts. There is only one int in this whole function declared, so I think its fine.

@d-a-v
Copy link
Collaborator

d-a-v commented Mar 17, 2020

@PsychoXIVI
CI requires you also updates libraries so there's no warning:

libraries/ESP8266httpUpdate/src/ESP8266httpUpdate.cpp:264:45: error: 'void HTTPClient::setFollowRedirects(bool)' is deprecated (declared at /home/travis/arduino_ide/hardware/esp8266com/esp8266/libraries/ESP8266HTTPClient/src/ESP8266HTTPClient.h:184) [-Werror=deprecated-declarations]
     http.setFollowRedirects(_followRedirects);
                                             ^
cc1plus: all warnings being treated as errors

@AgainPsychoX
Copy link
Contributor Author

AgainPsychoX commented Mar 17, 2020

About that warning, should I make compiler ignore this warning using something like:

#pragma GCC diagnostic push
#pragma GCC diagnostic ignored  "-Wdeprecated-declarations"
// Here the deprecated function declaration
#pragma GCC diagnostic pop

Is there common macro or something for deprecating functions?

Nevermind, I see now, other code uses this deprecated function, so I should also update other files.

@d-a-v
Copy link
Collaborator

d-a-v commented Mar 17, 2020

Just fix this file, commit your changes and push them to your branch.

Also changed a bit order of the enums (0 element to be DISABLED)
Also got rid of unnecessary `redirectCount` field. Now redirect counting and limiting is handled in `sendRequest` directly.
@AgainPsychoX
Copy link
Contributor Author

AgainPsychoX commented Mar 17, 2020

I will also update ESP8266httpUpdate library and tests in test_sw_http_client.ino (but I won't add more tests, because I have no proper Linux/MacOS environment and separate NodeMCU to run them...)

(Sorry for force pushes, I have no other way of testing all the library at the moment...)

@devyte devyte added this to the 2.7.0 milestone Mar 22, 2020
@d-a-v d-a-v merged commit d91f1da into esp8266:master Mar 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants