adafruit / Adafruit_CircuitPython_Requests

Requests-like interface for web interfacing
MIT License
51 stars 37 forks source link

Add simple support for redirects #77

Closed mogenson closed 3 years ago

mogenson commented 3 years ago

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: https://github.com/postmanlabs/httpbin/issues/617

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

mogenson commented 3 years ago

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 commented 3 years ago

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

mogenson commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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

anecdata commented 3 years ago

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 commented 3 years ago

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.

mogenson commented 3 years ago

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 commented 3 years ago

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

anecdata commented 3 years ago

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 commented 3 years ago

Great! Whose approval is required to merge this PR?

anecdata commented 3 years ago

@mogenson Thanks for implementing this long-desired feature!

ysiegel29 commented 3 years ago

Many thanks for implementing this!