Opened 7 years ago

Closed 7 years ago

Last modified 5 years ago

#2836 enhancement closed fixed (fixed)

limits of web2.resource.PostableResource can't be customized

Reported by: therve Owned by:
Priority: highest Milestone:
Component: web2 Keywords:
Cc: Branch:
Author: Launchpad Bug:

Description

The maximum memory, number of fields and size aren't passed to fileupload.parseMultipartFormData. This is related to #1873.

Change History (20)

comment:1 Changed 7 years ago by therve

(In [21310]) Merge forward work from large-fileupload-1873

Refs #2836
Refs #1873

comment:2 Changed 7 years ago by therve

  • Keywords review added
  • Owner changed from therve to jknight
  • Priority changed from normal to highest

I get back the work from #1873 in web2-custom-limits-2836, because I didn't manage to kill the bug, but solve a part of the problem anyway.

Ready to review.

comment:3 Changed 7 years ago by dreid

  • Owner changed from jknight to dreid
  • Status changed from new to assigned

comment:4 Changed 7 years ago by dreid

  • Keywords review removed
  • Owner changed from dreid to therve
  • Status changed from assigned to new

Once again thanks for working on this therve

It's taken me a while to look through these diffs.

  • There don't appear to be any substantive changes to fileupload.py just stylistic ones.
  • I don't see a test to verify the behaviour of maxMem.
  • The changes to static.py don't appear to have anything to do with this bug (other than the fact that the FileUpload resource already had an attribute called maxBytes and coincidentally subclasses PostableResource). There are a lot of things wrong with the FileUpload resource in static.py and I'd rather see them addressed in a seperate branch instead of just poking at it here.
  • The same goes for the test added to test_static.py It's testing one particular part of the PostableResource API which actually has nothing to do with the FileUpload resource. I'd rather see PostableResource get it's own set of unittests.

Other than that, keep up the good work I look forward to seeing this branch merged.

comment:5 Changed 7 years ago by therve

(In [21349]) Revert unrelated changes

Refs #2836

comment:6 Changed 7 years ago by therve

  • Keywords review added
  • Owner changed from therve to dreid

Thanks for your review. I reverted unrelated changes, and added a test for maxMem. Back to review.

comment:7 Changed 7 years ago by dreid

  • Keywords review removed
  • Owner changed from dreid to therve

Thanks therve,

About the only thing left for now is that server.parsePOSTData has absolutely no unittests. It of course never had unittests but it's a pretty import piece of the puzzle tying resource.PostableResource to fileupload.parseMultipartFormData. It's error conditions are pretty well defined so it should be easy.

Thanks again for the wonderful work.

comment:8 Changed 7 years ago by therve

(In [21367]) Add some tests, fix some corner cases.

Refs #2836

comment:9 Changed 7 years ago by therve

  • Keywords review added
  • Owner changed from therve to dreid

Thanks for the motivation, I've added a good bunch of tests, so the situation is much better. I've change a few errors, please tell me if it's OK. Also, if you could check XXX in test_otherErrors it would be cool.

comment:10 Changed 7 years ago by dreid

  • Keywords review removed
  • Owner changed from dreid to therve

I like that you changed the HTTPErrors to include StatusResponse objects with actual information about the error.

However you don't use this information in the tests to determine that you're failure occurred for the correct reason, you do this instead.

        d1 = self.assertFailure(server.parsePOSTData(request, maxSize=10),
            http.HTTPError)
        # Do the standard request to be sure the failure comes from maxSize
        d2 = server.parsePOSTData(request)
        return defer.gatherResults([d1, d2])

Which feels a little wonky to me, I'd rather see it asserting things about the returned HTTPError than just trying again.

comment:11 Changed 7 years ago by therve

(In [21371]) Fix the tests

Refs #2836

comment:12 Changed 7 years ago by therve

  • Keywords review added
  • Owner changed from therve to dreid

You're right, I fixed that, and even fix one test in the process (don't understand how it happened though, oh well).

comment:13 Changed 7 years ago by exarkun

  • Owner changed from dreid to exarkun
  • Status changed from new to assigned

comment:14 Changed 7 years ago by exarkun

  • Keywords review removed
  • Owner changed from exarkun to therve
  • Status changed from assigned to new
  • twisted.web2.test.test_fileupload should have a module docstring
  • several new tests, test_tooBigUpload, test_tooManyFields, and test_maxMem, don't return the Deferreds they pass to assertFailure
  • maybe test_mimeParsingError's docstring should explain the specific example of malformedness it is testing
  • parsePOSTData isn't factored very well for testing. I guess you shouldn't do anything about this for this ticket. It really seems like a lot of the tests in ParsePostDataTests are trying to test functionality which is in functions parsePOSTData calls, not in parsePOSTData itself.
  • indent the 2nd line of parsePOSTData's parameter list to be justified with the ( on the previous line
  • parsePOSTData's docstring doesn't cover its return value
  • The API for limiting work that this change adds isn't great, but I guess since it's just exposing an existing lower-level API it's the best that can be done without much bigger changes.

comment:15 Changed 7 years ago by therve

(In [21534]) Process review comments

Refs #2836

comment:16 Changed 7 years ago by therve

  • Keywords review added
  • Owner therve deleted

That sould be a bit better. Last round?

comment:17 Changed 7 years ago by exarkun

  • Keywords review removed
  • Owner set to therve

Great, thanks! merge :)

comment:18 Changed 7 years ago by therve

  • Resolution set to fixed
  • Status changed from new to closed

(In [21556]) Merge web2-custom-limits-2836

Author: therve
Reviewers: exarkun, dreid
Fixes #2836

Make the configuration limits of postable data for memory, file size and number
of fields available in web2 PostableResource, and add a bunch of tests in the
process.

comment:19 Changed 5 years ago by exarkun

It looks like there is a bug very near to this one which still exists. See #3732.

comment:20 Changed 3 years ago by <automation>

  • Owner therve deleted
Note: See TracTickets for help on using tickets.