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

[therve]

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

[therve]

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

Refs #2836 Refs #1873

[therve]

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.

[dreid]

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.

[therve]

(In [21349]) Revert unrelated changes

Refs #2836

[therve]

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

[dreid]

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.

[therve]

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

Refs #2836

[therve]

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.

[dreid]

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.

[therve]

(In [21371]) Fix the tests

Refs #2836

[therve]

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

[exarkun]

• 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.

[therve]

(In [21534]) Process review comments

Refs #2836

[therve]

That sould be a bit better. Last round?

[exarkun]

Great, thanks! merge :)

[therve]

(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.

[exarkun]

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