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

Add simple support for redirects #77

Merged
merged 1 commit into from
Jul 20, 2021
Merged

Conversation

mogenson
Copy link
Contributor

@mogenson mogenson commented Jul 8, 2021

Follow 3XX status redirects with a new request. Parse the location
header for an absolute url, absolute path, or relative path. Replace or
amend the url from the original request and place a new request.

Expand the requests_simpletest_cpython.py example with new requests
utilizing the three supported types of redirects.

Note: We're using the httpbingo.org domain for redirects until
the following issue on httpbin.org is resolved:
postmanlabs/httpbin#617

This has been tested on CPython 3.8 and an ESP32-S2 based MagTag board
with CircuitPython 6.0.

@mogenson
Copy link
Contributor Author

mogenson commented Jul 8, 2021

By the way, a fun request that requires redirect support is using the Wikipedia API to fetch a random article:

import socket
import ssl
import adafruit_requests

URL = "https://en.wikipedia.org/api/rest_v1/page/random/summary"
http = adafruit_requests.Session(socket, ssl.create_default_context())

response = http.get(URL)
article = response.json()

print(article["title"])
print(article["extract"])

@anecdata
Copy link
Member

anecdata commented Jul 8, 2021

looks like it's failing on black, check out: https://learn.adafruit.com/creating-and-sharing-a-circuitpython-library/check-your-code

@mogenson
Copy link
Contributor Author

mogenson commented Jul 8, 2021

Ah, black doesn't like that line 610 is folded but my editor doesn't like lines over 80 characters.

Good point about case sensitive headers.

Multiple redirects work, however since request() has been converted into a recursive method a very large number of consecutive redirects may crash the stack, especially on a microcontroller. We could set a flag to indicate we're handling a redirect, increment a counter, and compare to a static or user-defined limit. Or, assume most CircuitPython products are fetching from a pre-defined URL that will not redirect more than a few times.

I'll fixup this PR tonight.

@anecdata
Copy link
Member

anecdata commented Jul 8, 2021

Tested with modified simpletest from the library examples using::
Adafruit CircuitPython 7.0.0-alpha.3 on 2021-06-03; Adafruit PyPortal with samd51j20

Got RuntimeError: pystack exhausted with redirects > 3 using URLs of this form:
https://httpbingo.org/absolute-redirect/3

PYSTACK is the same on virtually every CP device, but handling even 1 or 2 redirects is a huge improvement, and may help solve some previously-reported user issues.

@mogenson
Copy link
Contributor Author

mogenson commented Jul 8, 2021

Ok so maybe allow 3 redirects, then throw an exception?

@anecdata
Copy link
Member

anecdata commented Jul 8, 2021

I'm not sure what the Pythonic thing to do is. pystack may get further limited by other aspects of user code, so there's probably nothing foolproof. Users should try to minimize redirects by supplying a canonical URL. Worst case, users catch the RuntimeError, but it won't necessarily be clear what's causing it. Maybe @brentru or @tannewt would like to weigh in.

@tannewt
Copy link
Member

tannewt commented Jul 8, 2021

I think for now we can just let it run out of stack. If folks have this issue a lot we can switch this code to using a loop instead of recursion.

Follow 3XX status redirects with a new request. Parse the `location`
header for an absolute url, absolute path, or relative path. Replace or
amend the url from the original request and place a new request.

Convert all keys in the headers dictionary to lowercase to perform a
case-insensitive search.

There's no limit on the number of consecutive redirects. Since the
`requets()` method is now recursive, multiple redirects may crash the
stack. Especially on a CircuitPython microcontroller.

Expand the requests_simpletest_cpython.py example with new requests
utilizing the three supported types of redirects.

Note: We're using the httpbingo.org domain for redirects until
the following issue on httpbin.org is resolved:
postmanlabs/httpbin#617

This has been tested on CPython 3.8 and an ESP32-S2 based MagTag board
running CircuitPython 6.0.
@mogenson mogenson force-pushed the redirect-support branch from 49f58f4 to 522a61a Compare July 8, 2021 23:44
@mogenson
Copy link
Contributor Author

mogenson commented Jul 8, 2021

Left out a redirect/recursion limit check.

In order to accomplish a case-insensitive header search I'm converting all headers to lowercase. I'm unaware of other CircuitPython libraries that rely on mixed-case HTTP headers, but let me know! Also, this removes a previous optimization of doing an initial string length match for the "content-length" and "transfer-encoding" labels, since lower() will be used on all headers.

@mogenson
Copy link
Contributor Author

@anecdata When you get a chance, can you let me know if this PR looks good now?

@anecdata
Copy link
Member

anecdata commented Jul 17, 2021

Adjusted PR example for ESP32SPI (Adafruit CircuitPython 6.3.0 on 2021-06-01; Adafruit PyPortal with samd51j20) and re-tested latest commit:
Success with 3 redirects on each URL

Also tested against a single-redirect URL with Mixed Case HTTP/1.1 header keys:
Success

I'm not sure what the memory impact of using .lower() on all of the headers is, but functionally LGTM!

@mogenson
Copy link
Contributor Author

Great! Whose approval is required to merge this PR?

Copy link
Member

@tannewt tannewt left a comment

Choose a reason for hiding this comment

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

Looks good to me. This will generate brief intermediate string objects but they should be short since they are header keys. I'd worry more if it was the header values. (It'd be nice if .lower() was in-place but I don't think it is.)

@tannewt tannewt merged commit 03f5442 into adafruit:main Jul 20, 2021
@mogenson mogenson deleted the redirect-support branch July 20, 2021 01:33
@anecdata
Copy link
Member

@mogenson Thanks for implementing this long-desired feature!

@ysiegel29
Copy link

Many thanks for implementing this!

adafruit-adabot added a commit to adafruit/Adafruit_CircuitPython_Bundle that referenced this pull request Jul 23, 2021
Updating https://github.com/adafruit/Adafruit_CircuitPython_CircuitPlayground to 4.4.1 from 4.4.0:
  > Merge pull request adafruit/Adafruit_CircuitPython_CircuitPlayground#107 from adafruit/remove-gamepad-from-mock-imports

Updating https://github.com/adafruit/Adafruit_CircuitPython_EPD to 2.9.5 from 2.9.4:
  > Merge pull request adafruit/Adafruit_CircuitPython_EPD#51 from icegoat9/master

Updating https://github.com/adafruit/Adafruit_CircuitPython_MONSTERM4SK to 0.2.0 from 0.1.4:
  > Merge pull request adafruit/Adafruit_CircuitPython_MONSTERM4SK#8 from lesamouraipourpre/max-size

Updating https://github.com/adafruit/Adafruit_CircuitPython_SSD1325 to 1.3.0 from 1.2.5:
  > Merge pull request adafruit/Adafruit_CircuitPython_SSD1325#11 from lesamouraipourpre/max-size

Updating https://github.com/adafruit/Adafruit_CircuitPython_ST7735R to 1.5.0 from 1.4.0:
  > Merge pull request adafruit/Adafruit_CircuitPython_ST7735R#25 from lesamouraipourpre/max-size

Updating https://github.com/adafruit/Adafruit_CircuitPython_AdafruitIO to 5.5.0 from 5.4.0:
  > Merge pull request adafruit/Adafruit_CircuitPython_AdafruitIO#78 from ryanplusplus/patch-1

Updating https://github.com/adafruit/Adafruit_CircuitPython_DisplayIO_Layout to 1.10.1 from 1.10.0:
  > Merge pull request adafruit/Adafruit_CircuitPython_DisplayIO_Layout#41 from FoamyGuy/fix_odb

Updating https://github.com/adafruit/Adafruit_CircuitPython_PortalBase to 1.9.1 from 1.9.0:
  > Merge pull request adafruit/Adafruit_CircuitPython_PortalBase#44 from lesamouraipourpre/ondiskbitmap-changes

Updating https://github.com/adafruit/Adafruit_CircuitPython_PYOA to 2.4.0 from 2.3.0:
  > Merge pull request adafruit/Adafruit_CircuitPython_PYOA#24 from lesamouraipourpre/ondiskbitmap-changes

Updating https://github.com/adafruit/Adafruit_CircuitPython_Requests to 1.10.0 from 1.9.9:
  > Merge pull request adafruit/Adafruit_CircuitPython_Requests#77 from mogenson/redirect-support
  > Moved default branch to main
  > Moved CI to Python 3.7
  > Added help text and problem matcher
  > Added pull request template

Updating https://github.com/adafruit/Adafruit_CircuitPython_Slideshow to 1.6.0 from 1.5.7:
  > Merge pull request adafruit/Adafruit_CircuitPython_Slideshow#36 from lesamouraipourpre/ondiskbitmap-changes
  > Moved default branch to main
  > Moved CI to Python 3.7
  > Added help text and problem matcher
  > Added pull request template
  > "Increase duplicate code check threshold "

Updating https://github.com/adafruit/Adafruit_CircuitPython_T to 0.9.2 from 0.9.0:
  > Merge pull request adafruit/Adafruit_CircuitPython_T#1 from adafruit/fix-no-monotonic-ns

Updating https://github.com/adafruit/Adafruit_CircuitPython_Bundle/circuitpython_library_list.md to NA from NA:
  > Added the following libraries: Adafruit_CircuitPython_Ticks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants