From 30a10bbc036942c944101d88606914d13bbf0504 Mon Sep 17 00:00:00 2001 From: bennr01 Date: Tue, 13 Apr 2021 14:34:11 +0200 Subject: [PATCH 1/4] treq.testing.StubTreq: fix session persistence between requests Before this commit, twisted.web.server.Session objects were not persisted properly between requests. Thus, two requests with the same cookies would result in two different sessions. See #327 for details. --- changelog.d/327.bugfix.rst | 1 + docs/api.rst | 8 +++++++ docs/testing.rst | 3 +++ src/treq/test/test_testing.py | 45 +++++++++++++++++++++++++++++++++++ src/treq/testing.py | 20 ++++++++++++---- 5 files changed, 72 insertions(+), 5 deletions(-) create mode 100644 changelog.d/327.bugfix.rst diff --git a/changelog.d/327.bugfix.rst b/changelog.d/327.bugfix.rst new file mode 100644 index 00000000..e6391df7 --- /dev/null +++ b/changelog.d/327.bugfix.rst @@ -0,0 +1 @@ +``treq.testing.StubTreq`` now persists ``twisted.web.server.Session`` instances between requests. diff --git a/docs/api.rst b/docs/api.rst index b01ee010..30e76f77 100644 --- a/docs/api.rst +++ b/docs/api.rst @@ -114,6 +114,14 @@ StubTreq Objects asynchronous. In that case, after each write from the server, :meth:`flush()` must be called so the client can see it. + .. method:: cleanSessions() + + Clean up sessions to prevent leaving behind a dirty reactor. + + If you are using :obj:`StubTreq` with :obj:`twisted.web.server.Session` + objects, you most likely have to call this method once you are done, + for example during the tearDown of a unittest TestCase. + As the methods on :class:`treq.client.HTTPClient`: .. method:: request diff --git a/docs/testing.rst b/docs/testing.rst index 2f292b51..1cf504b9 100644 --- a/docs/testing.rst +++ b/docs/testing.rst @@ -57,3 +57,6 @@ This is superior to calling your resource's methods directly or passing mock obj Thus, the ``request`` object your code interacts with is a *real* :class:`twisted.web.server.Request` and behaves the same as it would in production. Note that if your resource returns :data:`~twisted.web.server.NOT_DONE_YET` you must keep a reference to the :class:`~treq.testing.RequestTraversalAgent` and call its :meth:`~treq.testing.RequestTraversalAgent.flush()` method to spin the memory reactor once the server writes additional data before the client will receive it. + +If you are using :class:`~twisted.web.server.Session` instances in the wrapped resource, you may need to call :meth:`~treq.testing.StubTreq.cleanSessions()` to expire all sessions and prevent an unclean/dirty reactor. + diff --git a/src/treq/test/test_testing.py b/src/treq/test/test_testing.py index eb804158..61308163 100644 --- a/src/treq/test/test_testing.py +++ b/src/treq/test/test_testing.py @@ -52,6 +52,18 @@ def render(self, request): return NOT_DONE_YET +class _SessionIdTestResource(Resource): + """ + Resource that returns the current session ID. + """ + isLeaf = True + + def render(self, request): + session = request.getSession() + uid = session.uid + return uid + + class StubbingTests(TestCase): """ Tests for :class:`StubTreq`. @@ -242,6 +254,39 @@ def test_handles_successful_asynchronous_requests_with_streaming(self): stub.flush() self.successResultOf(d) + def test_session_persistence_between_requests(self): + """ + Calling request.getSession() in the wrapped resource will return + a session with the same ID, until the sessions are cleaned. + """ + rsrc = _SessionIdTestResource() + stub = StubTreq(rsrc) + # request 1, getting original session ID + d = stub.request("method", "http://example.com/") + resp = self.successResultOf(d) + cookies = resp.cookies() + sid_1 = self.successResultOf(resp.content()) + # request 2, ensuring session ID stays the same + d = stub.request("method", "http://example.com/", cookies=cookies) + resp = self.successResultOf(d) + sid_2 = self.successResultOf(resp.content()) + self.assertEqual(sid_1, sid_2) + # request 3, ensuring the session IDs are different after cleaning + # or expiring the sessions + stub.cleanSessions() + d = stub.request("method", "http://example.com/") + resp = self.successResultOf(d) + cookies = resp.cookies() + sid_3 = self.successResultOf(resp.content()) + self.assertNotEqual(sid_1, sid_3) + # request 4, ensuring that once again the session IDs are the same + d = stub.request("method", "http://example.com/", cookies=cookies) + resp = self.successResultOf(d) + sid_4 = self.successResultOf(resp.content()) + self.assertEqual(sid_3, sid_4) + # when done, clean sessions to avoid leaving a dirty reactor behind + stub.cleanSessions() + class HasHeadersTests(TestCase): """ diff --git a/src/treq/testing.py b/src/treq/testing.py index df633beb..88a9b9ee 100644 --- a/src/treq/testing.py +++ b/src/treq/testing.py @@ -88,6 +88,7 @@ def __init__(self, rootResource): reactor=self._memoryReactor, endpointFactory=_EndpointFactory(self._memoryReactor)) self._rootResource = rootResource + self._serverFactory = Site(self._rootResource, reactor=self._memoryReactor) self._pumps = set() def request(self, method, uri, headers=None, bodyProducer=None): @@ -126,8 +127,7 @@ def check_already_called(r): # Create the protocol and fake transport for the client and server, # using the factory that was passed to the MemoryReactor for the # client, and a Site around our rootResource for the server. - serverFactory = Site(self._rootResource, reactor=self._memoryReactor) - serverProtocol = serverFactory.buildProtocol(clientAddress) + serverProtocol = self._serverFactory.buildProtocol(clientAddress) serverTransport = iosim.FakeTransport( serverProtocol, isServer=True, hostAddress=serverAddress, peerAddress=clientAddress) @@ -228,8 +228,8 @@ def __init__(self, resource): :param resource: A :obj:`Resource` object that provides the fake responses """ - _agent = RequestTraversalAgent(resource) - _client = HTTPClient(agent=_agent, + self._agent = RequestTraversalAgent(resource) + _client = HTTPClient(agent=self._agent, data_to_body_producer=_SynchronousProducer) for function_name in treq.__all__: function = getattr(_client, function_name, None) @@ -239,7 +239,17 @@ def __init__(self, resource): function = _reject_files(function) setattr(self, function_name, function) - self.flush = _agent.flush + self.flush = self._agent.flush + + def cleanSessions(self): + """ + Clean up sessions to prevent leaving behind a dirty reactor. + If you are using :obj:`StubTreq` with :obj:`twisted.web.server.Session` + objects, you most likely have to call this method once you are done, + for example during the tearDown of a unittest TestCase. + """ + for sid in list(self._agent._serverFactory.sessions.keys()): + self._agent._serverFactory.sessions[sid].expire() class StringStubbingResource(Resource): From f8b6e253658f9d87bd19f135365c17795fafdf17 Mon Sep 17 00:00:00 2001 From: bennr01 Date: Wed, 21 Apr 2021 12:56:08 +0200 Subject: [PATCH 2/4] remove .cleanSessions() again --- docs/api.rst | 8 -------- docs/testing.rst | 2 -- src/treq/test/test_testing.py | 12 ++++++++++-- src/treq/testing.py | 10 ---------- 4 files changed, 10 insertions(+), 22 deletions(-) diff --git a/docs/api.rst b/docs/api.rst index 30e76f77..b01ee010 100644 --- a/docs/api.rst +++ b/docs/api.rst @@ -114,14 +114,6 @@ StubTreq Objects asynchronous. In that case, after each write from the server, :meth:`flush()` must be called so the client can see it. - .. method:: cleanSessions() - - Clean up sessions to prevent leaving behind a dirty reactor. - - If you are using :obj:`StubTreq` with :obj:`twisted.web.server.Session` - objects, you most likely have to call this method once you are done, - for example during the tearDown of a unittest TestCase. - As the methods on :class:`treq.client.HTTPClient`: .. method:: request diff --git a/docs/testing.rst b/docs/testing.rst index 1cf504b9..855bc69c 100644 --- a/docs/testing.rst +++ b/docs/testing.rst @@ -58,5 +58,3 @@ Thus, the ``request`` object your code interacts with is a *real* :class:`twiste Note that if your resource returns :data:`~twisted.web.server.NOT_DONE_YET` you must keep a reference to the :class:`~treq.testing.RequestTraversalAgent` and call its :meth:`~treq.testing.RequestTraversalAgent.flush()` method to spin the memory reactor once the server writes additional data before the client will receive it. -If you are using :class:`~twisted.web.server.Session` instances in the wrapped resource, you may need to call :meth:`~treq.testing.StubTreq.cleanSessions()` to expire all sessions and prevent an unclean/dirty reactor. - diff --git a/src/treq/test/test_testing.py b/src/treq/test/test_testing.py index 61308163..6a80a2ef 100644 --- a/src/treq/test/test_testing.py +++ b/src/treq/test/test_testing.py @@ -273,7 +273,11 @@ def test_session_persistence_between_requests(self): self.assertEqual(sid_1, sid_2) # request 3, ensuring the session IDs are different after cleaning # or expiring the sessions - stub.cleanSessions() + + # manually expire the sessions. + for sid in list(stub._agent._serverFactory.sessions.keys()): + stub._agent._serverFactory.sessions[sid].expire() + d = stub.request("method", "http://example.com/") resp = self.successResultOf(d) cookies = resp.cookies() @@ -284,8 +288,12 @@ def test_session_persistence_between_requests(self): resp = self.successResultOf(d) sid_4 = self.successResultOf(resp.content()) self.assertEqual(sid_3, sid_4) + # when done, clean sessions to avoid leaving a dirty reactor behind - stub.cleanSessions() + # NOTE: this is a temporary fix discussed in #328. + # This should be removed once twisted ticked #10177 is fixed + for sid in list(stub._agent._serverFactory.sessions.keys()): + stub._agent._serverFactory.sessions[sid].expire() class HasHeadersTests(TestCase): diff --git a/src/treq/testing.py b/src/treq/testing.py index 88a9b9ee..ec85b9fd 100644 --- a/src/treq/testing.py +++ b/src/treq/testing.py @@ -241,16 +241,6 @@ def __init__(self, resource): setattr(self, function_name, function) self.flush = self._agent.flush - def cleanSessions(self): - """ - Clean up sessions to prevent leaving behind a dirty reactor. - If you are using :obj:`StubTreq` with :obj:`twisted.web.server.Session` - objects, you most likely have to call this method once you are done, - for example during the tearDown of a unittest TestCase. - """ - for sid in list(self._agent._serverFactory.sessions.keys()): - self._agent._serverFactory.sessions[sid].expire() - class StringStubbingResource(Resource): """ From c31725638291f29df2b97de0b3af995566e6b6aa Mon Sep 17 00:00:00 2001 From: bennr01 Date: Mon, 26 Apr 2021 11:57:16 +0200 Subject: [PATCH 3/4] treq.testing.RequestTraversalAgent: explicitly pass reactor to Site --- src/treq/test/test_testing.py | 25 +++++++++++++++++-------- src/treq/testing.py | 3 ++- 2 files changed, 19 insertions(+), 9 deletions(-) diff --git a/src/treq/test/test_testing.py b/src/treq/test/test_testing.py index 6a80a2ef..1615472a 100644 --- a/src/treq/test/test_testing.py +++ b/src/treq/test/test_testing.py @@ -58,11 +58,27 @@ class _SessionIdTestResource(Resource): """ isLeaf = True + def __init__(self): + super().__init__() + # keep track of all sessions created, so we can manually expire them later + self.sessions = [] + def render(self, request): session = request.getSession() + if session not in self.sessions: + # new session, add to internal list + self.sessions.append(session) uid = session.uid return uid + def expire_sessions(self): + """ + Manually expire all sessions created by this resource. + """ + for session in self.sessions: + session.expire() + self.sessions = [] + class StubbingTests(TestCase): """ @@ -275,8 +291,7 @@ def test_session_persistence_between_requests(self): # or expiring the sessions # manually expire the sessions. - for sid in list(stub._agent._serverFactory.sessions.keys()): - stub._agent._serverFactory.sessions[sid].expire() + rsrc.expire_sessions() d = stub.request("method", "http://example.com/") resp = self.successResultOf(d) @@ -289,12 +304,6 @@ def test_session_persistence_between_requests(self): sid_4 = self.successResultOf(resp.content()) self.assertEqual(sid_3, sid_4) - # when done, clean sessions to avoid leaving a dirty reactor behind - # NOTE: this is a temporary fix discussed in #328. - # This should be removed once twisted ticked #10177 is fixed - for sid in list(stub._agent._serverFactory.sessions.keys()): - stub._agent._serverFactory.sessions[sid].expire() - class HasHeadersTests(TestCase): """ diff --git a/src/treq/testing.py b/src/treq/testing.py index ec85b9fd..cc051ccf 100644 --- a/src/treq/testing.py +++ b/src/treq/testing.py @@ -26,7 +26,7 @@ from twisted.web.error import SchemeNotSupported from twisted.web.iweb import IAgent, IAgentEndpointFactory, IBodyProducer from twisted.web.resource import Resource -from twisted.web.server import Site +from twisted.web.server import Session, Site from zope.interface import directlyProvides, implementer @@ -89,6 +89,7 @@ def __init__(self, rootResource): endpointFactory=_EndpointFactory(self._memoryReactor)) self._rootResource = rootResource self._serverFactory = Site(self._rootResource, reactor=self._memoryReactor) + self._serverFactory.sessionFactory = lambda site, uid: Session(site, uid, reactor=self._memoryReactor) self._pumps = set() def request(self, method, uri, headers=None, bodyProducer=None): From bbf412cada94d0d311ce6331611cd6ba9380d930 Mon Sep 17 00:00:00 2001 From: bennr01 Date: Wed, 12 May 2021 13:02:53 +0200 Subject: [PATCH 4/4] treq.testing: fix lint error (E501) --- src/treq/testing.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/treq/testing.py b/src/treq/testing.py index cc051ccf..33d8c94d 100644 --- a/src/treq/testing.py +++ b/src/treq/testing.py @@ -89,7 +89,11 @@ def __init__(self, rootResource): endpointFactory=_EndpointFactory(self._memoryReactor)) self._rootResource = rootResource self._serverFactory = Site(self._rootResource, reactor=self._memoryReactor) - self._serverFactory.sessionFactory = lambda site, uid: Session(site, uid, reactor=self._memoryReactor) + self._serverFactory.sessionFactory = lambda site, uid: Session( + site, + uid, + reactor=self._memoryReactor, + ) self._pumps = set() def request(self, method, uri, headers=None, bodyProducer=None):