Move request sanitization and upload disallowed out of file_upload #16032
Conversation
dstufft
force-pushed
the
refactor-upload-redux
branch
from
71088d1
to
746ad57
Compare
dstufft
force-pushed
the
refactor-upload-redux
branch
from
746ad57
to
a19ac06
Compare
There was a problem hiding this comment.
Nice extraction, it indeed does make the forklift a little easier to read.
I didn't think we used the decorator kwarg for view_config for anything other than HTTP header stuff (cache control, vary, etc) - but I don't see why it can't be as flexible for this kind of purpose either - it was a little surprising since it's the first time I'm really thinking about it.
Is there an advantage to having the implementation be a decorator pattern vs an inline function call? Does Pyramid handle this differently?
In any case, nothing blocking this from going forward, just my own curiosities.
| # ref: https://github.com/pypi/warehouse/issues/2491 | ||
| for field in set(request.POST) - {"content", "gpg_signature"}: | ||
| values = request.POST.getall(field) | ||
| if any(isinstance(value, cgi.FieldStorage) for value in values): |
There was a problem hiding this comment.
Thinking out loud - cgi is deprecated and going to be removed in 3.13 - are we still getting things coming in as FieldStorage instances? Might be good to insert a sentry message to try and track down if this is still happening. Not a merge blocker, but would be happy to re-review something like that.
This is extracting a (small) piece of #14716 out into it's own PR, with the overarching goal of making the
file_uploadendpoint easier to read and reason about what's happening.There's no real functionality change here (except we move request sanitizing to happen prior to the read only checks, which was done primarily because while the current sanitizers and the current checks are wholly independent of each other, that might not always be the case, so we sanitize first.
Beyond that, all this means is that when reading the
file_uploadmethod, it's easier to gloss over these checks, as they're just decorators instead of being part of the method body.This also means that the tests that test this functionality no longer need to do heavy weight things like setup databases and make DB queries, making our tests just a little bit faster.