Opened 3 years ago

Closed 3 years ago

#5525 defect closed fixed (fixed)

twisted.web.server.Request.view_setResponseCode doesn't accept message parameter

Reported by: tom.prince Owned by: exarkun
Priority: normal Milestone:
Component: web Keywords:
Cc: jknight, jesstess Branch: branches/setresponsecode-distrib-5525
(diff, github, buildbot, log)
Author: exarkun Launchpad Bug:

Description


Attachments (3)

distrib_server_response_code_message.patch (2.1 KB) - added by tom.prince 3 years ago.
Add missing message parameter
distrib_server_response_code_message_2.patch (5.3 KB) - added by tom.prince 3 years ago.
patch v2
distrib_server_response_code_message_3.patch (5.9 KB) - added by tom.prince 3 years ago.
Patch v3.

Download all attachments as: .zip

Change History (19)

comment:1 Changed 3 years ago by DefaultCC Plugin

  • Cc jknight added

Changed 3 years ago by tom.prince

Add missing message parameter

comment:2 Changed 3 years ago by jesstess

  • Owner set to jesstess

comment:3 Changed 3 years ago by Fahrenheit

  • Owner changed from jesstess to Fahrenheit
  • Status changed from new to assigned

comment:4 Changed 3 years ago by Fahrenheit

  • Keywords review removed
  • Owner changed from Fahrenheit to tom.prince
  • Status changed from assigned to new

Thanks for the patch! Comments below:

  1. twisted/web/server.py
    1. Not a huge deal, but the function signature for view_setResponseCode should match that of setResponseCode, so maybe remove the whitespace around the '='.
  2. twisted/web/test/test_distrib.py
    1. Again, not a huge deal, but the second underscore in test_setResponseCode_message doesn't match the twisted guidelines; everything after "test_" should be camelcase, rather that separated by any underscores.
    2. Last issue, and this one matters, is that your test doesn't actually make sure that view_setResponseCode does the correct thing with the message argument; I was able to remove it from your implementation and the test still passed, so make sure that your test checks that the message gets used properly.

You should be good once you fix up the test and the style stuff would be nice too, while you're at it. Thanks again!

Changed 3 years ago by tom.prince

patch v2

comment:5 Changed 3 years ago by tom.prince

  • Keywords review added

I think this fixes all issues. The old _requestTest used getPage, which didn't have a way of looking
at the response. This changes the tests to use Agent.

comment:6 Changed 3 years ago by tom.prince

  • Owner tom.prince deleted

comment:7 Changed 3 years ago by jesstess

  • Owner set to jesstess

comment:8 Changed 3 years ago by jesstess

  • Cc jesstess added
  • Keywords review removed
  • Owner changed from jesstess to tom.prince

Thanks for working on this, tom.prince. A few comments:

  • Since view_setResponseCode is in twisted.web.server, it seems like tests for this change would go in twisted.web.test.test_web with the other server tests. Can you explain why they should be in test_distrib, or move them to test_web? There's a test_setResponseCode in test_http you could crib from. It would also allow you to construct a more succinct unit test than one that is forcing you to make changes in test_largeWrite and test_largeReturn.
  • If your changes stay in test_distrib, there are a couple of typos:
    • The test no longer uses getPage:
              @param **kwargs: Extra keyword arguments to pass to L{getPage} when
      
    • consiting ==> consisting in:
              @return: A L{Deferred} which fires with a tuple consiting of a                                                                                                                                           
      
    • IResposne ==> IResponse in:
                  body of the response and an L{IResposne} with the response itself.
      

Sorry for the back and forth, and thanks again for working on this!

comment:9 Changed 3 years ago by exarkun

Since view_setResponseCode is in twisted.web.server, it seems like tests for this change would go in twisted.web.test.test_web

The strange factoring of the implementation of distrib means that the test organization differs a bit from the usual approach. I think test_distrib is the right place for these tests, because that's where there's testing infrastructure for actually exercising the distrib system. That involves setting up a server and connecting a distrib client to it (which would be nice to avoid, but that's how all the existing distrib tests are written...).

Changed 3 years ago by tom.prince

Patch v3.

comment:10 Changed 3 years ago by tom.prince

  • Keywords review added
  • Owner tom.prince deleted

The typos have been fixed. I also split _requestTest, so that creating the servers is separate from making the request, allowing the existing tests to remain unchanged.

comment:11 Changed 3 years ago by exarkun

  • Author set to exarkun
  • Branch set to branches/setresponsecode-distrib-5525

(In [33876]) Branching to 'setresponsecode-distrib-5525'

comment:12 Changed 3 years ago by exarkun

(In [33877]) Apply distrib_server_response_code_message_3.patch

refs #5525

comment:13 Changed 3 years ago by exarkun

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

comment:14 Changed 3 years ago by exarkun

  • Keywords review removed

Thanks. This is looking pretty great now. Just a few minor points:

  1. Methods in a class should be separated from each other by two blank lines
  2. There are some tabs in the patch; there should never be tabs in a .py file.
  3. The change needs a news fragment. This is a nice bug fix for distrib servers, so it's worth mentioning in a .bugfix fragment.

These are pretty trivial points. If I get a clean build on buildbot, I'll deal with these changes and merge. Thanks again!

comment:16 Changed 3 years ago by exarkun

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

(In [33885]) Mege setresponsecode-distrib-5525

Author: tom.prince
Reviewer: Fahrenheit, jesstess, exarkun
Fixes: #5525

Add the message parameter to the PB view of twisted.web.server.Request.
This allows distributed servers (twisted.web.distrib) to set the response
message, just as any normal HTTP code in a Twisted Web server may do.

Note: See TracTickets for help on using tickets.