Touch up Content-Disposition header #3439
Conversation
dbutenhof
added
Server
API
| def mock_send_file(path_or_file, *args, **kwargs): | ||
| nonlocal file_sent | ||
| file_sent = path_or_file | ||
| assert str(path_or_file) == "/dataset/1-default/default.csv" | ||
| assert kwargs["as_attachment"] is False | ||
| assert kwargs["download_name"] == "default.csv" | ||
| return {"status": "OK"} |
There was a problem hiding this comment.
Be aware that, if any of these assert's fails, it will raise an exception which will be caught by Werkzeug, not PyTest. I expect that that behavior will be noted by the assertion at line 117, but you might want to keep the file_sent mechanism (or replace it with something more comprehensive) as a belt-and-suspenders approach (i.e., the assertions are great, but they should probably be performed in the test function itself rather than the mock).
Ditto in test_get_result_tarball().
There was a problem hiding this comment.
That raises several interesting points.
First off, we are directly calling Flask's send_file, not Werkzeug, whereas the test is mocking werkzeug.utils ... we're depending on an implicit assumed behavior of Flask's send_file that I'm not sure is formally guaranteed anywhere.
Be that as it may, currently Flask's send_file does nothing but directly call Werkzeug's, and there is no exception handler involved other than our own in ApiBase.__dispatch.
On the other hand, although the assert message will show in the traceback the primary visible symptom of a failure will still be a server internal error, which isn't ideal, and keeping the assert outside of the mock would result in a much better diagnostic in case of failure. (Sigh.)
PBENCH-1169 Help clients with the output of `GET /datasets/<id>/inventory/` by "tuning" the `Content-Disposition` header automatically generated by Werkzeug's `send_file` API. I started thinking I would construct the header and looking into how to add headers to the `Response` object constructed by `send_file`. It actually does a good job of guessing the filename, but I decided to help it by providing the name we want, and I'm designating the raw tarfile as an `attachment` that we can't reasonably expect a client (especially the UI) to do anything with "inline" (the default).
PBENCH-1169
Help clients with the output of
GET /datasets/<id>/inventory/by "tuning" theContent-Dispositionheader automatically generated by Werkzeug'ssend_fileAPI.I started thinking I would construct the header and looking into how to add headers to the
Responseobject constructed bysend_file. It actually does a good job of guessing the filename, but I decided to help it by providing the name we want, and I'm designating the raw tarfile as anattachmentthat we can't reasonably expect a client (especially the UI) to do anything with "inline" (the default).