Improve handling for POST parameters

[chabala]

I saw this example code for handling POST requests in another issue:

@web_app.route("/led_on", ["POST"])
def led_on(request):
    print("led on!")
    r = request.query_params["r"]
    g = request.query_params["g"]
    b = request.query_params["b"]
    status_light.fill((int(r), int(g), int(b)))
    return ("200 OK", [], [])

It looks pretty straightforward, but when I tried it, it doesn't work. query_params only has the parameters from the query string; the POST parameters are in the request body as one might expect.

My working code looks like this:

@web_app.route("/led_on", ["POST"])
def led_on(request):
    print("led on!")
    if request.method == "POST":
        post_params = request.__parse_query_params(request.body.getvalue())
        request.body.close()
        r = post_params.get('r')
        g = post_params.get('g')
        b = post_params.get('b')
        status_light.fill((int(r), int(g), int(b)))
    return ("200 OK", [], [])

I can reuse the __parse_query_params() logic, but it's a little less intuitive than the first example would suggest. Perhaps this could be improved? Should query_params contain request body parameters for POST requests? Maybe it should be a different collection, or only parse the request body on demand.

[tekktrik]

I'm not an expert on these things, but it does seem the query_params should be separate from POST params, since they are different.

But I do like the idea of something like a helper method at the very least, so you can just something and out pops your POST params. Maybe something that will copy _body contents and then parse and return it like __parse_query_params? It could even be refactored to use the same base method!

I had to manually do this as well for my own project, I feel you. Want to submit a PR for this?

[chabala]

Want to submit a PR for this?

Probably not until there's some more buy in on how it should be implemented. query_params gets eagerly computed on every request: https://github.com/adafruit/Adafruit_CircuitPython_WSGI/blob/9db2666b756b00e2f5afccdba2dae21739338297/adafruit_wsgi/request.py#L29

Could make a _post_params attribute and guard it to only populate on POST requests, a la:

if self._method == "POST":
    self._post_params = self.__parse_query_params(self._body.getvalue())
else:
    self._post_params = {}

Not sure if _body would need closing, or if as you suggest it should be copied to another variable first and then parsed.

My preference would lean toward computing it lazily on first access of a property like:

@property
def post_params(self) -> Dict[str, str]:
    if not self._post_params:
        if self._method == "POST":
            self._post_params = self.__parse_query_params(self._body.getvalue())
        else:
            self._post_params = {}
    return self._post_params

I don't have a good example of lazy loading in python to crib from at the moment. Cribbed from here: https://stackoverflow.com/a/17486190/62462

[dhalbert]

We now recommend using https://github.com/adafruit/Adafruit_CircuitPython_HTTPServer instead, and would like to discontinue supporting this library. Would that library meet your needs? Maybe it already addresses what you would like here?

[ayourk]

We now recommend using https://github.com/adafruit/Adafruit_CircuitPython_HTTPServer instead, and would like to discontinue supporting this library. Would that library meet your needs? Maybe it already addresses what you would like here?

I will look into it.

[chabala]

Would that library meet your needs?

I will look into it.

Likewise. I know several libraries I was & am using have moved around, so I need to cut a new version that incorporates them and see what's working in the new versions.

[chabala]

@dhalbert I think this could use a migration guide. I'm not sure how https://github.com/adafruit/Adafruit_CircuitPython_HTTPServer is intended to replace WSGI.

https://github.com/adafruit/Adafruit_CircuitPython_Wiznet5k/pull/160 killed the Wiznet WSGI server and suggested https://github.com/adafruit/Adafruit_CircuitPython_WSGI was an alternative, but as far as I can tell, Adafruit_CircuitPython_WSGI is just an interface with no server implementation (though it might have an ESP32 implementation included). I think deleting the WSGI server should have been paired with updating the existing example code to work with the alternative; that would have exposed any shortcomings.

From your comment, you want to remove/deprecate Adafruit_CircuitPython_WSGI as well. But how much of my code can I reuse? If I'm not starting a WSGI server, and I'm starting Adafruit_CircuitPython_HTTPServer instead, how do I tie my routes into that?