Opened 4 years ago

Closed 3 years ago

Last modified 2 years ago

#7016 enhancement closed fixed (fixed)

Add requestFactory as an argument to Site.__init__

Reported by: Adi Roiban Owned by:
Priority: normal Milestone:
Component: web Keywords:
Cc: jknight Branch: branches/site-requestfactory-7016-3
branch-diff, diff-cov, branch-cov, buildbot
Author: adiroiban, rwall

Description

From #6933

  1. Customising the session name can then be achieved by assigning a curried Request constructor to server.Site.requestFactory eg mysite.requestFactory = functools.partial(Request, sessionCookieBaseName='MYSITE_SESSION')
    1. It would be even better if server.Site accepted a

"requestFactory"

argument, but that's another ticket.

Attachments (2)

7016-1.diff (3.0 KB) - added by Adi Roiban 4 years ago.
7016-2.diff (3.0 KB) - added by Adi Roiban 3 years ago.

Download all attachments as: .zip

Change History (16)

comment:1 Changed 4 years ago by DefaultCC Plugin

Cc: jknight added

Changed 4 years ago by Adi Roiban

Attachment: 7016-1.diff added

comment:2 Changed 4 years ago by Adi Roiban

Keywords: review added
Owner: Adi Roiban deleted

comment:3 Changed 4 years ago by Adi Roiban

I have attached a patch.

I have also added a test for buildProtocol since it was missing and I wanted to check how requestFactory leaves the Site instance... as its final destination is the HTTPChannel.

Thanks!

comment:4 Changed 3 years ago by Richard Wall

Owner: set to Richard Wall
Status: newassigned

comment:5 Changed 3 years ago by Richard Wall

Author: rwall
Branch: branches/site-requestfactory-7016

(In [43416]) Branching to site-requestfactory-7016.

comment:6 Changed 3 years ago by Richard Wall

Keywords: review removed
Owner: changed from Richard Wall to Adi Roiban
Status: assignednew

Thanks adiroban,

This looks useful.

Notes:

Points:

  1. branches/site-requestfactory-7016/twisted/web/server.py
    1. The standard way to handle optional initialiser arguments is to use if requestFactory is None: and set the default inside the initialiser. For example, see how the optional reactor argument of twisted.web.server.Session is handled.
  1. branches/site-requestfactory-7016/twisted/web/test/test_web.py
    1. twistedchecker warning W9016: 79,0: Too many blank lines after docstring, found 2
    2. test_constructorRequestFactory
      1. customFactory could just be a dummy. ie dummyRequestFactory = object() and server.Site(..., requestFactory=dummyRequestFactory
    3. test_buildProtocol
      1. I found the docstring difficult to understand. Perhaps it could be restated as "L{server.Site.buildProtocol} returns a C{Channel} whose C{site} and C{requestFactory} attributes are assigned from the C{site} instance.
  1. Add a test to demonstrate that the default requestFactory is Request.
  1. Custom session cookie documentation from #6933 can be updated to use this requestFactory argument instead.

Please answer or address the numbered points above and submit a new patch against source:branches/site-requestfactory-7016 .

Thanks again and look forward to being able to use this feature in my Twisted web servers.

-RichardW.

comment:7 Changed 3 years ago by Adi Roiban

Keywords: review added
Owner: Adi Roiban deleted
  • Why is this ticked blocked by #6933 ?

  1. branches/site-requestfactory-7016/twisted/web/server.py - Done. Hope the fix is good.
  1. branches/site-requestfactory-7016/twisted/web/test/test_web.py
  1. twistedchecker warning W9016: 79,0: Done
  1. test_constructorRequestFactory: True. Done!
  1. test_buildProtocol. Thanks. I left out L{server.Site.buildProtocol} as this is the target of the test and should be understood from the context.
  1. A assume that cookie documentation from #6933 should be updated after this is merged.
  1. Done. I wrote a tests for default value.

I will send the patch based on your branch. Thanks for the review!

Changed 3 years ago by Adi Roiban

Attachment: 7016-2.diff added

comment:8 Changed 3 years ago by Glyph

Author: rwallglyph, rwall
Branch: branches/site-requestfactory-7016branches/site-requestfactory-7016-2

(In [44001]) Branching to site-requestfactory-7016-2.

comment:9 Changed 3 years ago by Glyph

Author: glyph, rwallrwall
Branch: branches/site-requestfactory-7016-2branches/site-requestfactory-7016
Keywords: review removed
Owner: set to Adi Roiban

Hi adiroiban,

Sorry to say, the patch doesn't apply cleanly any more at *all*; all the hunks were rejected. So please update this for current trunk. I guess the good news is that Twisted is a fairly active project ;-).

However, for this to be a real review I wanted to look at the patch as well, so you have something to go on.

I think you were attempting to address rwall's previous comment:

The standard way to handle optional initialiser arguments is to use if requestFactory is None: and set the default inside the initialiser. For example, see how the optional reactor argument of twisted.web.server.Session is handled.

by setting Site.requestFactory to be None at the class level. However, I'm pretty sure what rwall was saying was simply to do the check with is instead of the implicit .__nonzero__() of putting the requestFactory argument as the condition of an if block. Setting the class variable to None is, among other problems, an incompatible change, because Site.requestFactory is a public name (even if it is documented and used as an @ivar, which is why we now generally prefer to avoid having the class variable at all).

comment:10 Changed 3 years ago by Adi Roiban

Author: rwalladiroiban, rwall
Branch: branches/site-requestfactory-7016branches/site-requestfactory-7016-3

(In [44098]) Branching to site-requestfactory-7016-3.

comment:11 Changed 3 years ago by Adi Roiban

Keywords: review added
Owner: Adi Roiban deleted

Thanks for the review!

I am confused about how to implement this but I have created a new branch and updated the code with what I understood from current feedback.

Sending the branch to buildbot.

Please check latest changes.

Thanks!

comment:12 Changed 3 years ago by Wilfredo Sánchez Vega

Keywords: review removed

Reviewed; go for merge. Don't forget NEWS file.

comment:13 Changed 3 years ago by Adi Roiban

Resolution: fixed
Status: newclosed

(In [44505]) Merge site-requestfactory-7016-3: Add requestFactory as an argument to Site.init.

Author: adiroiban Reviewers: rwall, glyph, wsanchez Fixes: #7016

comment:14 Changed 2 years ago by Tom Prince

Unfortunately, this introduced an incompatible change. If people were passing positional arguments, this changed the meaning of the first one, which used to be logFile.

Note: See TracTickets for help on using tickets.