From 69c7c405107d19ac8c7b7430fcada1a7801e299c Mon Sep 17 00:00:00 2001 From: Jean-Paul Calderone Date: Thu, 15 Oct 2020 11:43:42 -0400 Subject: [PATCH 01/31] handle Deferred from render --- src/allmydata/test/web/test_grid.py | 5 + src/allmydata/web/common.py | 198 +++++++++++++++++++++------- src/allmydata/web/directory.py | 10 +- src/allmydata/web/filenode.py | 43 ++---- src/allmydata/web/operations.py | 20 +-- src/allmydata/web/unlinked.py | 5 +- 6 files changed, 188 insertions(+), 93 deletions(-) diff --git a/src/allmydata/test/web/test_grid.py b/src/allmydata/test/web/test_grid.py index 01eb93fa7..8f61781d4 100644 --- a/src/allmydata/test/web/test_grid.py +++ b/src/allmydata/test/web/test_grid.py @@ -18,6 +18,10 @@ from allmydata.storage.shares import get_share_file from allmydata.scripts.debug import CorruptShareOptions, corrupt_share from allmydata.immutable import upload from allmydata.mutable import publish + +from ...web.common import ( + render_exception, +) from .. import common_util as testutil from ..common import WebErrorMixin, ShouldFailMixin from ..no_network import GridTestMixin @@ -34,6 +38,7 @@ class CompletelyUnhandledError(Exception): pass class ErrorBoom(object, resource.Resource): + @render_exception def render(self, req): raise CompletelyUnhandledError("whoops") diff --git a/src/allmydata/web/common.py b/src/allmydata/web/common.py index 102e67adc..9c88d832a 100644 --- a/src/allmydata/web/common.py +++ b/src/allmydata/web/common.py @@ -3,15 +3,43 @@ import time import json from functools import wraps +from hyperlink import ( + DecodedURL, +) + from twisted.web import ( http, resource, server, template, ) +from twisted.web.template import ( + tags, +) +from twisted.web.util import ( + FailureElement, + redirectTo, +) +from twisted.python.reflect import ( + fullyQualifiedName, +) +from twisted.python.urlpath import ( + URLPath, +) from twisted.python import log +from twisted.python.failure import ( + Failure, +) +from twisted.internet.defer import ( + Deferred, + maybeDeferred, +) +from twisted.web.resource import ( + IResource, +) from nevow import appserver from nevow.inevow import IRequest + from allmydata import blacklist from allmydata.interfaces import ( EmptyPathnameComponentError, @@ -320,48 +348,10 @@ def humanize_failure(f): class MyExceptionHandler(appserver.DefaultExceptionHandler, object): - def simple(self, ctx, text, code=http.BAD_REQUEST): - req = IRequest(ctx) - req.setResponseCode(code) - #req.responseHeaders.setRawHeaders("content-encoding", []) - #req.responseHeaders.setRawHeaders("content-disposition", []) - req.setHeader("content-type", "text/plain;charset=utf-8") - if isinstance(text, unicode): - text = text.encode("utf-8") - req.setHeader("content-length", b"%d" % len(text)) - req.write(text) - # TODO: consider putting the requested URL here - req.finishRequest(False) - def renderHTTP_exception(self, ctx, f): - try: - text, code = humanize_failure(f) - except: - log.msg("exception in humanize_failure") - log.msg("argument was %s" % (f,)) - log.err() - text, code = str(f), None - if code is not None: - return self.simple(ctx, text, code) - if f.check(server.UnsupportedMethod): - # twisted.web.server.Request.render() has support for transforming - # this into an appropriate 501 NOT_IMPLEMENTED or 405 NOT_ALLOWED - # return code, but nevow does not. - req = IRequest(ctx) - method = req.method - return self.simple(ctx, - "I don't know how to treat a %s request." % method, - http.NOT_IMPLEMENTED) req = IRequest(ctx) - accept = req.getHeader("accept") - if not accept: - accept = "*/*" - if "*/*" in accept or "text/*" in accept or "text/html" in accept: - super = appserver.DefaultExceptionHandler - return super.renderHTTP_exception(self, ctx, f) - # use plain text - traceback = f.getTraceback() - return self.simple(ctx, traceback, http.INTERNAL_SERVER_ERROR) + req.write(_renderHTTP_exception(req, f)) + req.finishRequest(False) class NeedOperationHandleError(WebError): @@ -481,16 +471,130 @@ def exception_to_child(f): return g -def render_exception(f): +def render_exception(render): """ Decorate a ``render_*`` method with exception handling behavior to render an error page reflecting the exception. """ - @wraps(f) + @wraps(render) def g(self, request): - try: - return f(self, request) - except Exception as e: - description, status = humanize_exception(e) - return resource.ErrorPage(status, "Error", description).render(request) + bound_render = render.__get__(self, type(self)) + result = maybeDeferred(bound_render, request) + if isinstance(result, Deferred): + result.addBoth(_finish, bound_render, request) + return server.NOT_DONE_YET + elif isinstance(result, bytes): + return result + elif result == server.NOT_DONE_YET: + return server.NOT_DONE_YET + else: + raise Exception("{!r} returned unusable {!r}".format( + fullyQualifiedName(bound_render), + result, + )) + return g + + +def _finish(result, render, request): + if isinstance(result, Failure): + _finish( + _renderHTTP_exception(request, result), + render, + request, + ) + elif IResource.providedBy(result): + # If result is also using @render_exception then we don't want to + # double-apply the logic. This leads to an attempt to double-finish + # the request. If it isn't using @render_exception then you should + # fix it so it is. + result.render(request) + elif isinstance(result, bytes): + request.write(result) + request.finish() + elif isinstance(result, URLPath): + if result.netloc == b"": + root = URLPath.fromRequest(request) + result.scheme = root.scheme + result.netloc = root.netloc + _finish(redirectTo(str(result), request), render, request) + elif isinstance(result, DecodedURL): + _finish(redirectTo(str(result), request), render, request) + elif result is None: + request.finish() + elif result == server.NOT_DONE_YET: + pass + else: + log.err("Request for {!r} handled by {!r} returned unusable {!r}".format( + request.uri, + fullyQualifiedName(render), + result, + )) + request.setResponseCode(http.INTERNAL_SERVER_ERROR) + _finish(b"", render, request) + + +def _renderHTTP_exception(request, failure): + try: + text, code = humanize_failure(failure) + except: + log.msg("exception in humanize_failure") + log.msg("argument was %s" % (failure,)) + log.err() + text = str(failure) + code = None + + if code is not None: + return _renderHTTP_exception_simple(request, text, code) + + if failure.check(server.UnsupportedMethod): + # twisted.web.server.Request.render() has support for transforming + # this into an appropriate 501 NOT_IMPLEMENTED or 405 NOT_ALLOWED + # return code, but nevow does not. + method = request.method + return _renderHTTP_exception_simple( + request, + "I don't know how to treat a %s request." % (method,), + http.NOT_IMPLEMENTED, + ) + + accept = request.getHeader("accept") + if not accept: + accept = "*/*" + if "*/*" in accept or "text/*" in accept or "text/html" in accept: + request.setResponseCode(http.INTERNAL_SERVER_ERROR) + return template.renderElement( + request, + tags.html( + tags.head( + tags.title(u"Exception"), + ), + tags.body( + FailureElement(failure), + ), + ), + ) + + # use plain text + traceback = failure.getTraceback() + return _renderHTTP_exception_simple( + request, + traceback, + http.INTERNAL_SERVER_ERROR, + ) + + +def _renderHTTP_exception_simple(request, text, code): + request.setResponseCode(code) + request.setHeader("content-type", "text/plain;charset=utf-8") + if isinstance(text, unicode): + text = text.encode("utf-8") + request.setHeader("content-length", b"%d" % len(text)) + return text + + +def handle_when_done(req, d): + when_done = get_arg(req, "when_done", None) + if when_done: + d.addCallback(lambda res: DecodedURL.from_text(when_done.decode("utf-8"))) + return d diff --git a/src/allmydata/web/directory.py b/src/allmydata/web/directory.py index 4d7aa1bd1..39bc297f5 100644 --- a/src/allmydata/web/directory.py +++ b/src/allmydata/web/directory.py @@ -58,6 +58,7 @@ from allmydata.web.common import ( SlotsSequenceElement, exception_to_child, render_exception, + handle_when_done, ) from allmydata.web.filenode import ReplaceMeMixin, \ FileNodeHandler, PlaceHolderNodeHandler @@ -206,6 +207,7 @@ class DirectoryNodeHandler(ReplaceMeMixin, Resource, object): ) return make_handler_for(node, self.client, self.node, name) + @render_exception def render_DELETE(self, req): assert self.parentnode and self.name d = self.parentnode.delete(self.name) @@ -310,13 +312,7 @@ class DirectoryNodeHandler(ReplaceMeMixin, Resource, object): else: raise WebError("POST to a directory with bad t=%s" % t) - when_done = get_arg(req, "when_done", None) - if when_done: - def done(res): - req.redirect(when_done) - return res - d.addCallback(done) - return d + return handle_when_done(req, d) def _POST_mkdir(self, req): name = get_arg(req, "name", "") diff --git a/src/allmydata/web/filenode.py b/src/allmydata/web/filenode.py index 0ecc8cc52..b36ce0af8 100644 --- a/src/allmydata/web/filenode.py +++ b/src/allmydata/web/filenode.py @@ -1,6 +1,10 @@ import json +from hyperlink import ( + DecodedURL, +) + from twisted.web import http, static from twisted.internet import defer from twisted.web.resource import ( @@ -8,8 +12,6 @@ from twisted.web.resource import ( ErrorPage, ) -from nevow import url - from allmydata.interfaces import ExistingChildError from allmydata.monitor import Monitor from allmydata.immutable.upload import FileHandle @@ -34,8 +36,8 @@ from allmydata.web.common import ( render_exception, should_create_intermediate_directories, text_plain, - MyExceptionHandler, WebError, + handle_when_done, ) from allmydata.web.check_results import ( CheckResultsRenderer, @@ -150,10 +152,7 @@ class PlaceHolderNodeHandler(Resource, ReplaceMeMixin): # placeholder. raise WebError("POST to a file: bad t=%s" % t) - when_done = get_arg(req, "when_done", None) - if when_done: - d.addCallback(lambda res: when_done) - return d + return handle_when_done(req, d) class FileNodeHandler(Resource, ReplaceMeMixin, object): @@ -315,10 +314,7 @@ class FileNodeHandler(Resource, ReplaceMeMixin, object): else: raise WebError("POST to file: bad t=%s" % t) - when_done = get_arg(req, "when_done", None) - if when_done: - d.addCallback(lambda res: url.URL.fromString(when_done)) - return d + return handle_when_done(req, d) def _maybe_literal(self, res, Results_Class): if res: @@ -485,23 +481,11 @@ class FileDownloader(Resource, object): if req.method == "HEAD": return "" - finished = [] - def _request_finished(ign): - finished.append(True) - req.notifyFinish().addBoth(_request_finished) - d = self.filenode.read(req, first, size) - def _finished(ign): - if not finished: - req.finish() def _error(f): lp = log.msg("error during GET", facility="tahoe.webish", failure=f, level=log.UNUSUAL, umid="xSiF3w") - if finished: - log.msg("but it's too late to tell them", parent=lp, - level=log.UNUSUAL, umid="j1xIbw") - return req._tahoe_request_had_error = f # for HTTP-style logging if req.startedWriting: # The content-type is already set, and the response code has @@ -513,15 +497,16 @@ class FileDownloader(Resource, object): # error response be shorter than the intended results. # # We don't have a lot of options, unfortunately. - req.write("problem during download\n") - req.finish() + return b"problem during download\n" else: # We haven't written anything yet, so we can provide a # sensible error message. - eh = MyExceptionHandler() - eh.renderHTTP_exception(req, f) - d.addCallbacks(_finished, _error) - return req.deferred + return f + d.addCallbacks( + lambda ignored: None, + _error, + ) + return d def _file_json_metadata(req, filenode, edge_metadata): diff --git a/src/allmydata/web/operations.py b/src/allmydata/web/operations.py index 21c2ec7ef..25e267038 100644 --- a/src/allmydata/web/operations.py +++ b/src/allmydata/web/operations.py @@ -1,10 +1,15 @@ import time -from nevow import url +from hyperlink import ( + DecodedURL, +) from twisted.web.template import ( renderer, tags as T, ) +from twisted.python.urlpath import ( + URLPath, +) from twisted.python.failure import Failure from twisted.internet import reactor, defer from twisted.web import resource @@ -84,17 +89,14 @@ class OphandleTable(resource.Resource, service.Service): """ :param allmydata.webish.MyRequest req: """ - ophandle = get_arg(req, "ophandle") + ophandle = get_arg(req, "ophandle").decode("utf-8") assert ophandle - target = get_root(req) + "/operations/" + ophandle + root = DecodedURL.from_text(unicode(URLPath.fromRequest(req))) + target = root.click(u"/").child(u"operations", ophandle) output = get_arg(req, "output") if output: - target = target + "?output=%s" % output - - # XXX: We have to use nevow.url here because nevow.appserver - # is unhappy with anything else; so this gets its own ticket. - # https://tahoe-lafs.org/trac/tahoe-lafs/ticket/3314 - return url.URL.fromString(target) + target = target.add(u"output", output.decode("utf-8")) + return target @exception_to_child def getChild(self, name, req): diff --git a/src/allmydata/web/unlinked.py b/src/allmydata/web/unlinked.py index f94e32240..f63006e7e 100644 --- a/src/allmydata/web/unlinked.py +++ b/src/allmydata/web/unlinked.py @@ -2,6 +2,9 @@ import urllib from twisted.web import http from twisted.internet import defer +from twisted.python.urlpath import ( + URLPath, +) from twisted.python.filepath import FilePath from twisted.web.resource import Resource from twisted.web.template import ( @@ -66,7 +69,7 @@ def POSTUnlinkedCHK(req, client): def _done(upload_results, redir_to): if "%(uri)s" in redir_to: redir_to = redir_to.replace("%(uri)s", urllib.quote(upload_results.get_uri())) - return url.URL.fromString(redir_to) + return URLPath.fromString(redir_to) d.addCallback(_done, when_done) else: # return the Upload Results page, which includes the URI From df949868b6fe1b955246bf0d5ddbd57be0a459e0 Mon Sep 17 00:00:00 2001 From: Jean-Paul Calderone Date: Thu, 15 Oct 2020 14:47:30 -0400 Subject: [PATCH 02/31] Stop explicitly finishing and then returning a string --- src/allmydata/web/unlinked.py | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/allmydata/web/unlinked.py b/src/allmydata/web/unlinked.py index f63006e7e..9cf315235 100644 --- a/src/allmydata/web/unlinked.py +++ b/src/allmydata/web/unlinked.py @@ -163,7 +163,6 @@ def POSTUnlinkedCreateDirectory(req, client): new_url = "uri/" + urllib.quote(res.get_uri()) req.setResponseCode(http.SEE_OTHER) # 303 req.setHeader('location', new_url) - req.finish() return '' d.addCallback(_then_redir) else: @@ -182,7 +181,6 @@ def POSTUnlinkedCreateDirectoryWithChildren(req, client): new_url = "uri/" + urllib.quote(res.get_uri()) req.setResponseCode(http.SEE_OTHER) # 303 req.setHeader('location', new_url) - req.finish() return '' d.addCallback(_then_redir) else: @@ -201,7 +199,6 @@ def POSTUnlinkedCreateImmutableDirectory(req, client): new_url = "uri/" + urllib.quote(res.get_uri()) req.setResponseCode(http.SEE_OTHER) # 303 req.setHeader('location', new_url) - req.finish() return '' d.addCallback(_then_redir) else: From 07246b35092a7434ca264c0339ddb0ea66d0a199 Mon Sep 17 00:00:00 2001 From: Jean-Paul Calderone Date: Thu, 15 Oct 2020 15:15:32 -0400 Subject: [PATCH 03/31] Render requests more thoroughly --- src/allmydata/test/web/test_root.py | 29 +++++++++++++++-------------- 1 file changed, 15 insertions(+), 14 deletions(-) diff --git a/src/allmydata/test/web/test_root.py b/src/allmydata/test/web/test_root.py index 1a29b7a15..abc0444fb 100644 --- a/src/allmydata/test/web/test_root.py +++ b/src/allmydata/test/web/test_root.py @@ -25,6 +25,9 @@ from .common import ( assert_soup_has_tag_with_content, ) +from ..common_web import ( + render, +) from ..common import ( EMPTY_CLIENT_CONFIG, ) @@ -36,13 +39,6 @@ class RenderSlashUri(unittest.TestCase): """ def setUp(self): - self.request = DummyRequest(b"/uri") - self.request.fields = {} - - def prepathURL(): - return b"http://127.0.0.1.99999/" + b"/".join(self.request.prepath) - - self.request.prePathURL = prepathURL self.client = Mock() self.res = URIHandler(self.client) @@ -50,18 +46,20 @@ class RenderSlashUri(unittest.TestCase): """ A valid capbility does not result in error """ - self.request.args[b"uri"] = [( + uri = ( b"URI:CHK:nt2xxmrccp7sursd6yh2thhcky:" b"mukesarwdjxiyqsjinbfiiro6q7kgmmekocxfjcngh23oxwyxtzq:2:5:5874882" - )] - self.res.render_GET(self.request) + ) + return render(self.res, {b"uri": [uri]}) def test_invalid(self): """ A (trivially) invalid capbility is an error """ - self.request.args[b"uri"] = [b"not a capability"] - response_body = self.res.render_GET(self.request) + query_args = {b"uri": [b"not a capability"]} + response_body = self.successResultOf( + render(self.res, query_args), + ) soup = BeautifulSoup(response_body, 'html5lib') @@ -82,8 +80,11 @@ class RenderSlashUri(unittest.TestCase): """ Let hypothesis try a bunch of invalid capabilities """ - self.request.args[b"uri"] = [cap.encode('utf8')] - response_body = self.res.render_GET(self.request) + query_args = {b"uri": [cap.encode('utf8')]} + response_body = self.successResultOf( + render(self.res, query_args, + ), + ) soup = BeautifulSoup(response_body, 'html5lib') From d38ae4d6dd0b35b850c937b0fda45cba7035bf73 Mon Sep 17 00:00:00 2001 From: Jean-Paul Calderone Date: Thu, 15 Oct 2020 15:15:54 -0400 Subject: [PATCH 04/31] Stop reading server module attributes all the time It jumps through a ton of deprecation machinery that is at least tedious in the debugger, if not wasteful at runtime. --- src/allmydata/web/common.py | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/src/allmydata/web/common.py b/src/allmydata/web/common.py index 9c88d832a..441269d6e 100644 --- a/src/allmydata/web/common.py +++ b/src/allmydata/web/common.py @@ -10,12 +10,15 @@ from hyperlink import ( from twisted.web import ( http, resource, - server, template, ) from twisted.web.template import ( tags, ) +from twisted.web.server import ( + NOT_DONE_YET, + UnsupportedMethod, +) from twisted.web.util import ( FailureElement, redirectTo, @@ -426,7 +429,7 @@ class TokenOnlyWebApi(resource.Resource, object): def render(self, req): if req.method != 'POST': - raise server.UnsupportedMethod(('POST',)) + raise UnsupportedMethod(('POST',)) if req.args.get('token', False): raise WebError("Do not pass 'token' as URL argument", http.BAD_REQUEST) # not using get_arg() here because we *don't* want the token @@ -482,11 +485,11 @@ def render_exception(render): result = maybeDeferred(bound_render, request) if isinstance(result, Deferred): result.addBoth(_finish, bound_render, request) - return server.NOT_DONE_YET + return NOT_DONE_YET elif isinstance(result, bytes): return result - elif result == server.NOT_DONE_YET: - return server.NOT_DONE_YET + elif result == NOT_DONE_YET: + return NOT_DONE_YET else: raise Exception("{!r} returned unusable {!r}".format( fullyQualifiedName(bound_render), @@ -522,7 +525,7 @@ def _finish(result, render, request): _finish(redirectTo(str(result), request), render, request) elif result is None: request.finish() - elif result == server.NOT_DONE_YET: + elif result == NOT_DONE_YET: pass else: log.err("Request for {!r} handled by {!r} returned unusable {!r}".format( @@ -547,7 +550,7 @@ def _renderHTTP_exception(request, failure): if code is not None: return _renderHTTP_exception_simple(request, text, code) - if failure.check(server.UnsupportedMethod): + if failure.check(UnsupportedMethod): # twisted.web.server.Request.render() has support for transforming # this into an appropriate 501 NOT_IMPLEMENTED or 405 NOT_ALLOWED # return code, but nevow does not. From f733a244aa2c9a33605fc6205a79afb988ccc887 Mon Sep 17 00:00:00 2001 From: Jean-Paul Calderone Date: Thu, 15 Oct 2020 15:59:05 -0400 Subject: [PATCH 05/31] Just gonna produce text instead --- src/allmydata/test/web/test_root.py | 52 ++++++----------------------- 1 file changed, 11 insertions(+), 41 deletions(-) diff --git a/src/allmydata/test/web/test_root.py b/src/allmydata/test/web/test_root.py index abc0444fb..6b0885eb2 100644 --- a/src/allmydata/test/web/test_root.py +++ b/src/allmydata/test/web/test_root.py @@ -18,9 +18,6 @@ from ...util.connection_status import ConnectionStatus from allmydata.web.root import URIHandler from allmydata.client import _Client -from hypothesis import given -from hypothesis.strategies import text - from .common import ( assert_soup_has_tag_with_content, ) @@ -46,11 +43,17 @@ class RenderSlashUri(unittest.TestCase): """ A valid capbility does not result in error """ - uri = ( + query_args = {b"uri": [ b"URI:CHK:nt2xxmrccp7sursd6yh2thhcky:" b"mukesarwdjxiyqsjinbfiiro6q7kgmmekocxfjcngh23oxwyxtzq:2:5:5874882" + ]} + response_body = self.successResultOf( + render(self.res, query_args), + ) + self.assertNotEqual( + response_body, + "Invalid capability", ) - return render(self.res, {b"uri": [uri]}) def test_invalid(self): """ @@ -60,42 +63,9 @@ class RenderSlashUri(unittest.TestCase): response_body = self.successResultOf( render(self.res, query_args), ) - - soup = BeautifulSoup(response_body, 'html5lib') - - assert_soup_has_tag_with_content( - self, soup, "title", "400 - Error", - ) - assert_soup_has_tag_with_content( - self, soup, "h1", "Error", - ) - assert_soup_has_tag_with_content( - self, soup, "p", "Invalid capability", - ) - - @given( - text() - ) - def test_hypothesis_error_caps(self, cap): - """ - Let hypothesis try a bunch of invalid capabilities - """ - query_args = {b"uri": [cap.encode('utf8')]} - response_body = self.successResultOf( - render(self.res, query_args, - ), - ) - - soup = BeautifulSoup(response_body, 'html5lib') - - assert_soup_has_tag_with_content( - self, soup, "title", "400 - Error", - ) - assert_soup_has_tag_with_content( - self, soup, "h1", "Error", - ) - assert_soup_has_tag_with_content( - self, soup, "p", "Invalid capability", + self.assertEqual( + response_body, + "Invalid capability", ) From fa02e46033d38a5350eb9b92fdf8da8d5761cd06 Mon Sep 17 00:00:00 2001 From: Jean-Paul Calderone Date: Thu, 15 Oct 2020 16:02:32 -0400 Subject: [PATCH 06/31] maybeDeferred always returns a Deferred --- src/allmydata/web/common.py | 14 ++------------ 1 file changed, 2 insertions(+), 12 deletions(-) diff --git a/src/allmydata/web/common.py b/src/allmydata/web/common.py index 441269d6e..1de1fc073 100644 --- a/src/allmydata/web/common.py +++ b/src/allmydata/web/common.py @@ -483,18 +483,8 @@ def render_exception(render): def g(self, request): bound_render = render.__get__(self, type(self)) result = maybeDeferred(bound_render, request) - if isinstance(result, Deferred): - result.addBoth(_finish, bound_render, request) - return NOT_DONE_YET - elif isinstance(result, bytes): - return result - elif result == NOT_DONE_YET: - return NOT_DONE_YET - else: - raise Exception("{!r} returned unusable {!r}".format( - fullyQualifiedName(bound_render), - result, - )) + result.addBoth(_finish, bound_render, request) + return NOT_DONE_YET return g From 0339ba97b9c2e14bff59c7d243957e865b0363c5 Mon Sep 17 00:00:00 2001 From: Jean-Paul Calderone Date: Thu, 15 Oct 2020 16:14:06 -0400 Subject: [PATCH 07/31] Turn getChild None and Deferred results into something Twisted Web can manage --- src/allmydata/web/common.py | 29 ++++++++++++++++++++++------- 1 file changed, 22 insertions(+), 7 deletions(-) diff --git a/src/allmydata/web/common.py b/src/allmydata/web/common.py index 1de1fc073..25bb5fbba 100644 --- a/src/allmydata/web/common.py +++ b/src/allmydata/web/common.py @@ -20,6 +20,7 @@ from twisted.web.server import ( UnsupportedMethod, ) from twisted.web.util import ( + DeferredResource, FailureElement, redirectTo, ) @@ -459,21 +460,35 @@ class TokenOnlyWebApi(resource.Resource, object): raise WebError("'%s' invalid type for 't' arg" % (t,), http.BAD_REQUEST) -def exception_to_child(f): +def exception_to_child(getChild): """ Decorate ``getChild`` method with exception handling behavior to render an error page reflecting the exception. """ - @wraps(f) + @wraps(getChild) def g(self, name, req): - try: - return f(self, name, req) - except Exception as e: - description, status = humanize_exception(e) - return resource.ErrorPage(status, "Error", description) + bound_getChild = getChild.__get__(self, type(self)) + result = maybeDeferred(bound_getChild, name, req) + result.addCallbacks( + _getChild_done, + _getChild_failed, + callbackArgs=(self,), + ) + return DeferredResource(result) return g +def _getChild_done(child, parent): + if child is None: + return resource.NoResource() + return child + + +def _getChild_failed(reason): + text, code = humanize_failure(reason) + return resource.ErrorPage(code, "Error", text) + + def render_exception(render): """ Decorate a ``render_*`` method with exception handling behavior to render From 0faa24d34462bfc835dcd0ae6ba9e3386eb13874 Mon Sep 17 00:00:00 2001 From: Jean-Paul Calderone Date: Fri, 16 Oct 2020 09:30:09 -0400 Subject: [PATCH 08/31] Add a mess of eliot logging to request handling --- src/allmydata/web/common.py | 71 +++++++++++++++++++++++++++++++++---- 1 file changed, 64 insertions(+), 7 deletions(-) diff --git a/src/allmydata/web/common.py b/src/allmydata/web/common.py index 25bb5fbba..35228374c 100644 --- a/src/allmydata/web/common.py +++ b/src/allmydata/web/common.py @@ -7,6 +7,14 @@ from hyperlink import ( DecodedURL, ) +from eliot import ( + Message, + start_action, +) +from eliot.twisted import ( + DeferredContext, +) + from twisted.web import ( http, resource, @@ -468,17 +476,31 @@ def exception_to_child(getChild): @wraps(getChild) def g(self, name, req): bound_getChild = getChild.__get__(self, type(self)) - result = maybeDeferred(bound_getChild, name, req) - result.addCallbacks( - _getChild_done, - _getChild_failed, - callbackArgs=(self,), + + action = start_action( + action_type=u"allmydata:web:common-getChild", + uri=req.uri, + method=req.method, + name=name, + handler=fullyQualifiedName(bound_getChild), ) + with action.context(): + result = DeferredContext(maybeDeferred(bound_getChild, name, req)) + result.addCallbacks( + _getChild_done, + _getChild_failed, + callbackArgs=(self,), + ) + result = result.addActionFinish() return DeferredResource(result) return g def _getChild_done(child, parent): + Message.log( + message_type=u"allmydata:web:common-getChild:result", + result=fullyQualifiedName(type(child)), + ) if child is None: return resource.NoResource() return child @@ -497,8 +519,17 @@ def render_exception(render): @wraps(render) def g(self, request): bound_render = render.__get__(self, type(self)) - result = maybeDeferred(bound_render, request) - result.addBoth(_finish, bound_render, request) + + action = start_action( + action_type=u"allmydata:web:common-render", + uri=request.uri, + method=request.method, + handler=fullyQualifiedName(bound_render), + ) + with action.context(): + result = DeferredContext(maybeDeferred(bound_render, request)) + result.addBoth(_finish, bound_render, request) + result.addActionFinish() return NOT_DONE_YET return g @@ -506,6 +537,10 @@ def render_exception(render): def _finish(result, render, request): if isinstance(result, Failure): + Message.log( + message_type=u"allmydata:web:common-render:failure", + message=result.getErrorMessage(), + ) _finish( _renderHTTP_exception(request, result), render, @@ -516,23 +551,45 @@ def _finish(result, render, request): # double-apply the logic. This leads to an attempt to double-finish # the request. If it isn't using @render_exception then you should # fix it so it is. + Message.log( + message_type=u"allmydata:web:common-render:resource", + resource=fullyQualifiedName(type(result)), + ) result.render(request) elif isinstance(result, bytes): + Message.log( + message_type=u"allmydata:web:common-render:bytes", + ) request.write(result) request.finish() elif isinstance(result, URLPath): + Message.log( + message_type=u"allmydata:web:common-render:URLPath", + ) if result.netloc == b"": root = URLPath.fromRequest(request) result.scheme = root.scheme result.netloc = root.netloc _finish(redirectTo(str(result), request), render, request) elif isinstance(result, DecodedURL): + Message.log( + message_type=u"allmydata:web:common-render:DecodedURL", + ) _finish(redirectTo(str(result), request), render, request) elif result is None: + Message.log( + message_type=u"allmydata:web:common-render:None", + ) request.finish() elif result == NOT_DONE_YET: + Message.log( + message_type=u"allmydata:web:common-render:NOT_DONE_YET", + ) pass else: + Message.log( + message_type=u"allmydata:web:common-render:unknown", + ) log.err("Request for {!r} handled by {!r} returned unusable {!r}".format( request.uri, fullyQualifiedName(render), From 31207e4b6b482171a95070d5226177f021c4cf5b Mon Sep 17 00:00:00 2001 From: Jean-Paul Calderone Date: Fri, 16 Oct 2020 09:30:22 -0400 Subject: [PATCH 09/31] don't double-apply the renderer logic since that leads to double-finishing requests too --- src/allmydata/web/common.py | 4 ++++ src/allmydata/web/directory.py | 5 ++++- 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/src/allmydata/web/common.py b/src/allmydata/web/common.py index 35228374c..5a196678e 100644 --- a/src/allmydata/web/common.py +++ b/src/allmydata/web/common.py @@ -526,6 +526,10 @@ def render_exception(render): method=request.method, handler=fullyQualifiedName(bound_render), ) + if getattr(request, "dont_apply_extra_processing", False): + with action: + return bound_render(request) + with action.context(): result = DeferredContext(maybeDeferred(bound_render, request)) result.addBoth(_finish, bound_render, request) diff --git a/src/allmydata/web/directory.py b/src/allmydata/web/directory.py index 39bc297f5..0d521951f 100644 --- a/src/allmydata/web/directory.py +++ b/src/allmydata/web/directory.py @@ -400,7 +400,10 @@ class DirectoryNodeHandler(ReplaceMeMixin, Resource, object): # delegate to it. We could return the resource back out of # DirectoryNodeHandler.renderHTTP, and nevow would recurse into it, # but the addCallback() that handles when_done= would break. - d.addCallback(lambda child: child.render(req)) + def render_child(child): + req.dont_apply_extra_processing = True + return child.render(req) + d.addCallback(render_child) return d def _POST_uri(self, req): From a1f1f00be7cc7d70c530622c781f8d2f49e8b2c2 Mon Sep 17 00:00:00 2001 From: Jean-Paul Calderone Date: Fri, 16 Oct 2020 09:43:24 -0400 Subject: [PATCH 10/31] Use the more feaetureful rendering helper --- src/allmydata/test/test_checker.py | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/allmydata/test/test_checker.py b/src/allmydata/test/test_checker.py index 882356aeb..0c8f05ad4 100644 --- a/src/allmydata/test/test_checker.py +++ b/src/allmydata/test/test_checker.py @@ -32,6 +32,9 @@ from allmydata.mutable.publish import MutableData from .common import ( EMPTY_CLIENT_CONFIG, ) +from .common_web import ( + render, +) from .web.common import ( assert_soup_has_favicon, @@ -164,11 +167,11 @@ class WebResultsRendering(unittest.TestCase): return c def render_json(self, resource): - return resource.render(TestRequest(args={"output": ["json"]})) + return self.successResultOf(render(resource, {"output": ["json"]})) def render_element(self, element, args=None): d = flattenString(TestRequest(args), element) - return unittest.TestCase().successResultOf(d) + return self.successResultOf(d) def test_literal(self): lcr = web_check_results.LiteralCheckResultsRendererElement() From a73a919a20dc1802751937e2096f8e3c74868cd6 Mon Sep 17 00:00:00 2001 From: Jean-Paul Calderone Date: Fri, 16 Oct 2020 10:22:42 -0400 Subject: [PATCH 11/31] flakes --- src/allmydata/test/web/test_root.py | 6 ------ src/allmydata/web/common.py | 1 - src/allmydata/web/filenode.py | 6 ------ src/allmydata/web/operations.py | 1 - src/allmydata/web/unlinked.py | 1 - 5 files changed, 15 deletions(-) diff --git a/src/allmydata/test/web/test_root.py b/src/allmydata/test/web/test_root.py index 6b0885eb2..139441a6c 100644 --- a/src/allmydata/test/web/test_root.py +++ b/src/allmydata/test/web/test_root.py @@ -2,8 +2,6 @@ from mock import Mock import time -from bs4 import BeautifulSoup - from twisted.trial import unittest from twisted.web.template import Tag from twisted.web.test.requesthelper import DummyRequest @@ -18,10 +16,6 @@ from ...util.connection_status import ConnectionStatus from allmydata.web.root import URIHandler from allmydata.client import _Client -from .common import ( - assert_soup_has_tag_with_content, -) - from ..common_web import ( render, ) diff --git a/src/allmydata/web/common.py b/src/allmydata/web/common.py index 5a196678e..d9300f818 100644 --- a/src/allmydata/web/common.py +++ b/src/allmydata/web/common.py @@ -43,7 +43,6 @@ from twisted.python.failure import ( Failure, ) from twisted.internet.defer import ( - Deferred, maybeDeferred, ) from twisted.web.resource import ( diff --git a/src/allmydata/web/filenode.py b/src/allmydata/web/filenode.py index b36ce0af8..7686b35b5 100644 --- a/src/allmydata/web/filenode.py +++ b/src/allmydata/web/filenode.py @@ -1,10 +1,6 @@ import json -from hyperlink import ( - DecodedURL, -) - from twisted.web import http, static from twisted.internet import defer from twisted.web.resource import ( @@ -484,8 +480,6 @@ class FileDownloader(Resource, object): d = self.filenode.read(req, first, size) def _error(f): - lp = log.msg("error during GET", facility="tahoe.webish", failure=f, - level=log.UNUSUAL, umid="xSiF3w") req._tahoe_request_had_error = f # for HTTP-style logging if req.startedWriting: # The content-type is already set, and the response code has diff --git a/src/allmydata/web/operations.py b/src/allmydata/web/operations.py index 25e267038..d4f407b31 100644 --- a/src/allmydata/web/operations.py +++ b/src/allmydata/web/operations.py @@ -19,7 +19,6 @@ from twisted.application import service from allmydata.web.common import ( WebError, - get_root, get_arg, boolean_of_arg, exception_to_child, diff --git a/src/allmydata/web/unlinked.py b/src/allmydata/web/unlinked.py index 9cf315235..03f582c15 100644 --- a/src/allmydata/web/unlinked.py +++ b/src/allmydata/web/unlinked.py @@ -13,7 +13,6 @@ from twisted.web.template import ( renderElement, tags, ) -from nevow import url from allmydata.immutable.upload import FileHandle from allmydata.mutable.publish import MutableFileHandle from allmydata.web.common import ( From 28505ab57d0bc05b4d007955fae3dedc227bb8a9 Mon Sep 17 00:00:00 2001 From: Jean-Paul Calderone Date: Fri, 16 Oct 2020 10:38:58 -0400 Subject: [PATCH 12/31] news fragment --- newsfragments/3428.minor | 0 1 file changed, 0 insertions(+), 0 deletions(-) create mode 100644 newsfragments/3428.minor diff --git a/newsfragments/3428.minor b/newsfragments/3428.minor new file mode 100644 index 000000000..e69de29bb From a22426011be6d0ed39a329bc9cb8794226f41af4 Mon Sep 17 00:00:00 2001 From: Jean-Paul Calderone Date: Fri, 16 Oct 2020 12:49:36 -0400 Subject: [PATCH 13/31] import and naming cleanups post-merge --- src/allmydata/web/common.py | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/src/allmydata/web/common.py b/src/allmydata/web/common.py index 39073fe84..b5afe3fa2 100644 --- a/src/allmydata/web/common.py +++ b/src/allmydata/web/common.py @@ -50,9 +50,6 @@ from twisted.internet.defer import ( from twisted.web.resource import ( IResource, ) -from nevow import appserver -from nevow.inevow import IRequest - from twisted.web.iweb import IRequest as ITwistedRequest from twisted.python import log if PY2: @@ -376,7 +373,7 @@ def humanize_failure(f): class MyExceptionHandler(DefaultExceptionHandler, object): def renderHTTP_exception(self, ctx, f): - req = IRequest(ctx) + req = INevowRequest(ctx) req.write(_renderHTTP_exception(req, f)) req.finishRequest(False) From 7b02f58da0cbe00cb6e281622505ca2c194fc852 Mon Sep 17 00:00:00 2001 From: Jean-Paul Calderone Date: Fri, 16 Oct 2020 13:15:45 -0400 Subject: [PATCH 14/31] Make this test tolerant of more than one message It is observing the log system. It is reasonable to believe more than one log event might come through over the course of the test. We only need one, though. --- integration/test_streaming_logs.py | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/integration/test_streaming_logs.py b/integration/test_streaming_logs.py index 32b97644d..b921937f1 100644 --- a/integration/test_streaming_logs.py +++ b/integration/test_streaming_logs.py @@ -53,7 +53,12 @@ class _StreamingLogClientProtocol(WebSocketClientProtocol): self.factory.on_open.callback(self) def onMessage(self, payload, isBinary): - self.on_message.callback(payload) + if self.on_message is None: + # Already did our job, ignore it + return + on_message = self.on_message + self.on_message = None + on_message.callback(payload) def onClose(self, wasClean, code, reason): self.on_close.callback(reason) From 292f136547d209b87fb28b7f43f2fa02a1a72150 Mon Sep 17 00:00:00 2001 From: Jean-Paul Calderone Date: Fri, 16 Oct 2020 13:21:07 -0400 Subject: [PATCH 15/31] pyflakes --- src/allmydata/web/common.py | 1 - 1 file changed, 1 deletion(-) diff --git a/src/allmydata/web/common.py b/src/allmydata/web/common.py index b5afe3fa2..4fb2372a5 100644 --- a/src/allmydata/web/common.py +++ b/src/allmydata/web/common.py @@ -51,7 +51,6 @@ from twisted.web.resource import ( IResource, ) from twisted.web.iweb import IRequest as ITwistedRequest -from twisted.python import log if PY2: from nevow.appserver import DefaultExceptionHandler from nevow.inevow import IRequest as INevowRequest From 9e26599a7677453722b1de7b1756cd1675f81603 Mon Sep 17 00:00:00 2001 From: Jean-Paul Calderone Date: Fri, 16 Oct 2020 13:44:37 -0400 Subject: [PATCH 16/31] Fix the race condition --- integration/test_streaming_logs.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/integration/test_streaming_logs.py b/integration/test_streaming_logs.py index b921937f1..52c813f9b 100644 --- a/integration/test_streaming_logs.py +++ b/integration/test_streaming_logs.py @@ -136,10 +136,13 @@ def _test_streaming_logs(reactor, temp_dir, alice): client.on_close = Deferred() client.on_message = Deferred() + # Capture this now before on_message perhaps goes away. + racing = _race(client.on_close, client.on_message) + # Provoke _some_ log event. yield treq.get(node_url) - result = yield _race(client.on_close, client.on_message) + result = yield racing assert isinstance(result, Right) json.loads(result.value) From 84acf4e50fc0233ac070e07dbcfde9fb3c98e7f8 Mon Sep 17 00:00:00 2001 From: Jean-Paul Calderone Date: Sun, 18 Oct 2020 10:58:09 -0400 Subject: [PATCH 17/31] Accept unicode return values and encode them to UTF-8 Nevow accepts unicode in most places it accepts bytes and does the usual sloppy Python 2 thing, lets one or the other get implicitly re-coded, typically using the ascii codec. We'll go with UTF-8 because that fails less often than ASCII. We may want to clean up the code at some point so we're not accidentally slinging both bytes and text around as if they were the same thing. --- src/allmydata/web/common.py | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/allmydata/web/common.py b/src/allmydata/web/common.py index 4fb2372a5..a3f1688d1 100644 --- a/src/allmydata/web/common.py +++ b/src/allmydata/web/common.py @@ -571,6 +571,12 @@ def _finish(result, render, request): resource=fullyQualifiedName(type(result)), ) result.render(request) + elif isinstance(result, unicode): + Message.log( + message_type=u"allmydata:web:common-render:unicode", + ) + request.write(result.encode("utf-8")) + request.finish() elif isinstance(result, bytes): Message.log( message_type=u"allmydata:web:common-render:bytes", From e710fd883a6ae9443dca7f515767161707e8caf1 Mon Sep 17 00:00:00 2001 From: Jean-Paul Calderone Date: Sun, 18 Oct 2020 11:00:57 -0400 Subject: [PATCH 18/31] Add a Twisted Web-based renderer and start using it on Python 3 This could be the thing that eventually replaces the Nevow-based testing renderer on Python 2 as well. --- src/allmydata/test/common_nevow.py | 46 ++++++++++++++++++++++++ src/allmydata/test/common_tweb.py | 54 ++++++++++++++++++++++++++++ src/allmydata/test/common_web.py | 56 ++++++++---------------------- 3 files changed, 114 insertions(+), 42 deletions(-) create mode 100644 src/allmydata/test/common_nevow.py create mode 100644 src/allmydata/test/common_tweb.py diff --git a/src/allmydata/test/common_nevow.py b/src/allmydata/test/common_nevow.py new file mode 100644 index 000000000..dcf4dc095 --- /dev/null +++ b/src/allmydata/test/common_nevow.py @@ -0,0 +1,46 @@ +""" +General helpers related to Nevow. +""" + +from nevow.context import WebContext +from nevow.testutil import FakeRequest +from nevow.appserver import ( + processingFailed, + DefaultExceptionHandler, +) +from nevow.inevow import ( + ICanHandleException, + IRequest, + IResource as INevowResource, + IData, +) + +def render(resource, query_args): + """ + Render (in the manner of the Nevow appserver) a Nevow ``Page`` or a + Twisted ``Resource`` against a request with the given query arguments . + + :param resource: The page or resource to render. + + :param query_args: The query arguments to put into the request being + rendered. A mapping from ``bytes`` to ``list`` of ``bytes``. + + :return Deferred: A Deferred that fires with the rendered response body as + ``bytes``. + """ + ctx = WebContext(tag=resource) + req = FakeRequest(args=query_args) + ctx.remember(DefaultExceptionHandler(), ICanHandleException) + ctx.remember(req, IRequest) + ctx.remember(None, IData) + + def maybe_concat(res): + if isinstance(res, bytes): + return req.v + res + return req.v + + resource = INevowResource(resource) + d = maybeDeferred(resource.renderHTTP, ctx) + d.addErrback(processingFailed, req, ctx) + d.addCallback(maybe_concat) + return d diff --git a/src/allmydata/test/common_tweb.py b/src/allmydata/test/common_tweb.py new file mode 100644 index 000000000..2af9d4f53 --- /dev/null +++ b/src/allmydata/test/common_tweb.py @@ -0,0 +1,54 @@ +from zope.interface import ( + classImplements, +) +from twisted.python.reflect import ( + fullyQualifiedName, +) +from twisted.internet.defer import ( + succeed, +) +from twisted.web.test.requesthelper import ( + DummyRequest, +) +from twisted.web.iweb import ( + IRequest, +) +from twisted.web.server import ( + NOT_DONE_YET, +) + +classImplements(DummyRequest, IRequest) + +def render(resource, query_args): + """ + Render (in the manner of the Twisted Web Site) a Twisted ``Resource`` + against a request with the given query arguments . + + :param resource: The page or resource to render. + + :param query_args: The query arguments to put into the request being + rendered. A mapping from ``bytes`` to ``list`` of ``bytes``. + + :return Deferred: A Deferred that fires with the rendered response body as + ``bytes``. + """ + request = DummyRequest([]) + request.args = query_args + result = resource.render(request) + if isinstance(result, bytes): + request.write(result) + done = succeed(None) + elif result == NOT_DONE_YET: + if request.finished: + done = succeed(None) + else: + done = request.notifyFinish() + else: + raise ValueError( + "{!r} returned {!r}, required bytes or NOT_DONE_YET.".format( + fullyQualifiedName(resource.render), + result, + ), + ) + done.addCallback(lambda ignored: b"".join(request.written)) + return done diff --git a/src/allmydata/test/common_web.py b/src/allmydata/test/common_web.py index e2ea57539..829eef153 100644 --- a/src/allmydata/test/common_web.py +++ b/src/allmydata/test/common_web.py @@ -1,3 +1,4 @@ +from future.utils import PY2 import treq from twisted.internet.defer import ( @@ -7,19 +8,6 @@ from twisted.internet.defer import ( ) from twisted.web.error import Error -from nevow.context import WebContext -from nevow.testutil import FakeRequest -from nevow.appserver import ( - processingFailed, - DefaultExceptionHandler, -) -from nevow.inevow import ( - ICanHandleException, - IRequest, - IResource as INevowResource, - IData, -) - @inlineCallbacks def do_http(method, url, **kwargs): response = yield treq.request(method, url, persistent=False, **kwargs) @@ -31,32 +19,16 @@ def do_http(method, url, **kwargs): returnValue(body) -def render(resource, query_args): - """ - Render (in the manner of the Nevow appserver) a Nevow ``Page`` or a - Twisted ``Resource`` against a request with the given query arguments . - - :param resource: The page or resource to render. - - :param query_args: The query arguments to put into the request being - rendered. A mapping from ``bytes`` to ``list`` of ``bytes``. - - :return Deferred: A Deferred that fires with the rendered response body as - ``bytes``. - """ - ctx = WebContext(tag=resource) - req = FakeRequest(args=query_args) - ctx.remember(DefaultExceptionHandler(), ICanHandleException) - ctx.remember(req, IRequest) - ctx.remember(None, IData) - - def maybe_concat(res): - if isinstance(res, bytes): - return req.v + res - return req.v - - resource = INevowResource(resource) - d = maybeDeferred(resource.renderHTTP, ctx) - d.addErrback(processingFailed, req, ctx) - d.addCallback(maybe_concat) - return d +if PY2: + # We can only use Nevow on Python 2 and Tahoe-LAFS still *does* use Nevow + # so prefer the Nevow-based renderer if we can get it. + from .common_nevow import ( + render, + ) +else: + # However, Tahoe-LAFS *will* use Twisted Web before too much longer so go + # ahead and let some tests run against the Twisted Web-based renderer on + # Python 3. Later this will become the only codepath. + from .common_tweb import ( + render, + ) From 72e60f83011e0e52f3f8d7bd7855ec8986f671e5 Mon Sep 17 00:00:00 2001 From: Jean-Paul Calderone Date: Sun, 18 Oct 2020 11:12:44 -0400 Subject: [PATCH 19/31] Fix imports --- src/allmydata/test/common_nevow.py | 4 ++++ src/allmydata/test/common_web.py | 7 ++++++- 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/src/allmydata/test/common_nevow.py b/src/allmydata/test/common_nevow.py index dcf4dc095..d6327f9c6 100644 --- a/src/allmydata/test/common_nevow.py +++ b/src/allmydata/test/common_nevow.py @@ -2,6 +2,10 @@ General helpers related to Nevow. """ +from twisted.internet.defer import ( + maybeDeferred, +) + from nevow.context import WebContext from nevow.testutil import FakeRequest from nevow.appserver import ( diff --git a/src/allmydata/test/common_web.py b/src/allmydata/test/common_web.py index 829eef153..08b356cd0 100644 --- a/src/allmydata/test/common_web.py +++ b/src/allmydata/test/common_web.py @@ -1,8 +1,13 @@ + +__all__ = [ + "do_http", + "render", +] + from future.utils import PY2 import treq from twisted.internet.defer import ( - maybeDeferred, inlineCallbacks, returnValue, ) From b68c08cff96b7593f47eab56fd455e9fee4bd731 Mon Sep 17 00:00:00 2001 From: Jean-Paul Calderone Date: Mon, 19 Oct 2020 12:33:15 -0400 Subject: [PATCH 20/31] Yank direct support for URLPath from common.py --- src/allmydata/web/common.py | 43 +++++++++++++++++++++++++---------- src/allmydata/web/unlinked.py | 7 +++--- 2 files changed, 34 insertions(+), 16 deletions(-) diff --git a/src/allmydata/web/common.py b/src/allmydata/web/common.py index a3f1688d1..56bef9dfc 100644 --- a/src/allmydata/web/common.py +++ b/src/allmydata/web/common.py @@ -37,9 +37,6 @@ from twisted.web.util import ( from twisted.python.reflect import ( fullyQualifiedName, ) -from twisted.python.urlpath import ( - URLPath, -) from twisted.python import log from twisted.python.failure import ( Failure, @@ -583,15 +580,6 @@ def _finish(result, render, request): ) request.write(result) request.finish() - elif isinstance(result, URLPath): - Message.log( - message_type=u"allmydata:web:common-render:URLPath", - ) - if result.netloc == b"": - root = URLPath.fromRequest(request) - result.scheme = root.scheme - result.netloc = root.netloc - _finish(redirectTo(str(result), request), render, request) elif isinstance(result, DecodedURL): Message.log( message_type=u"allmydata:web:common-render:DecodedURL", @@ -684,3 +672,34 @@ def handle_when_done(req, d): if when_done: d.addCallback(lambda res: DecodedURL.from_text(when_done.decode("utf-8"))) return d + + +def url_for_string(req, url_string): + """ + Construct a universal URL using the given URL string. + + :param IRequest req: The request being served. If ``redir_to`` is not + absolute then this is used to determine the net location of this + server and the resulting URL is made to point at it. + + :param bytes url_string: A byte string giving a universal or absolute URL. + + :return DecodedURL: An absolute URL based on this server's net location + and the given URL string. + """ + url = DecodedURL.from_text(url_string.decode("utf-8")) + if url.host == b"": + root = req.URLPath() + netloc = root.netloc.split(b":", 1) + if len(netloc) == 1: + host = netloc + port = None + else: + host = netloc[0] + port = int(netloc[1]) + url = url.replace( + scheme=root.scheme.decode("ascii"), + host=host.decode("ascii"), + port=port, + ) + return url diff --git a/src/allmydata/web/unlinked.py b/src/allmydata/web/unlinked.py index 03f582c15..e420a0371 100644 --- a/src/allmydata/web/unlinked.py +++ b/src/allmydata/web/unlinked.py @@ -1,10 +1,8 @@ import urllib + from twisted.web import http from twisted.internet import defer -from twisted.python.urlpath import ( - URLPath, -) from twisted.python.filepath import FilePath from twisted.web.resource import Resource from twisted.web.template import ( @@ -23,6 +21,7 @@ from allmydata.web.common import ( get_format, get_mutable_type, render_exception, + url_for_string, ) from allmydata.web import status @@ -68,7 +67,7 @@ def POSTUnlinkedCHK(req, client): def _done(upload_results, redir_to): if "%(uri)s" in redir_to: redir_to = redir_to.replace("%(uri)s", urllib.quote(upload_results.get_uri())) - return URLPath.fromString(redir_to) + return url_for_string(req, redir_to) d.addCallback(_done, when_done) else: # return the Upload Results page, which includes the URI From a9e9efb3361df9fca0621cfceb9338ca4274205c Mon Sep 17 00:00:00 2001 From: Jean-Paul Calderone Date: Mon, 19 Oct 2020 14:26:12 -0400 Subject: [PATCH 21/31] direct tests for `@render_exception` --- src/allmydata/test/web/test_common.py | 211 ++++++++++++++++++++++++++ src/allmydata/web/common.py | 2 +- 2 files changed, 212 insertions(+), 1 deletion(-) create mode 100644 src/allmydata/test/web/test_common.py diff --git a/src/allmydata/test/web/test_common.py b/src/allmydata/test/web/test_common.py new file mode 100644 index 000000000..f9152e126 --- /dev/null +++ b/src/allmydata/test/web/test_common.py @@ -0,0 +1,211 @@ +""" +Tests for ``allmydata.web.common``. +""" + +from bs4 import ( + BeautifulSoup, +) +from hyperlink import ( + DecodedURL, +) + +from testtools.matchers import ( + Equals, + Contains, + MatchesPredicate, +) +from testtools.twistedsupport import ( + succeeded, + has_no_result, +) + +from twisted.internet.defer import ( + fail, +) +from twisted.web.server import ( + NOT_DONE_YET, +) +from twisted.web.resource import ( + Resource, +) + +from ...web.common import ( + render_exception, +) + +from ..common import ( + SyncTestCase, +) +from ..common_web import ( + render, +) +from .common import ( + assert_soup_has_tag_with_attributes, +) + +class StaticResource(Resource): + def __init__(self, response): + Resource.__init__(self) + self._response = response + + @render_exception + def render(self, request): + return self._response + + +class RenderExceptionTests(SyncTestCase): + """ + Tests for ``render_exception``. + """ + def test_exception(self): + """ + If the decorated method raises an exception then the exception is rendered + into the response. + """ + class R(Resource): + @render_exception + def render(self, request): + raise Exception("synthetic exception") + + self.assertThat( + render(R(), {}), + succeeded( + Contains(b"synthetic exception"), + ), + ) + + def test_failure(self): + """ + If the decorated method returns a ``Deferred`` that fires with a + ``Failure`` then the exception the ``Failure`` wraps is rendered into + the response. + """ + resource = StaticResource(fail(Exception("synthetic exception"))) + self.assertThat( + render(resource, {}), + succeeded( + Contains(b"synthetic exception"), + ), + ) + + def test_resource(self): + """ + If the decorated method returns an ``IResource`` provider then that + resource is used to render the response. + """ + resource = StaticResource(StaticResource(b"static result")) + self.assertThat( + render(resource, {}), + succeeded( + Equals(b"static result"), + ), + ) + + def test_unicode(self): + """ + If the decorated method returns a ``unicode`` string then that string is + UTF-8 encoded and rendered into the response. + """ + text = u"\N{SNOWMAN}" + resource = StaticResource(text) + self.assertThat( + render(resource, {}), + succeeded( + Equals(text.encode("utf-8")), + ), + ) + + def test_bytes(self): + """ + If the decorated method returns a ``bytes`` string then that string is + rendered into the response. + """ + data = b"hello world" + resource = StaticResource(data) + self.assertThat( + render(resource, {}), + succeeded( + Equals(data), + ), + ) + + def test_decodedurl(self): + """ + If the decorated method returns a ``DecodedURL`` then a redirect to that + location is rendered into the response. + """ + loc = u"http://example.invalid/foo?bar=baz" + resource = StaticResource(DecodedURL.from_text(loc)) + self.assertThat( + render(resource, {}), + succeeded( + MatchesPredicate( + lambda value: assert_soup_has_tag_with_attributes( + self, + BeautifulSoup(value), + "meta", + {"http-equiv": "refresh", + "content": "0;URL={}".format(loc.encode("ascii")), + }, + ) + # The assertion will raise if it has a problem, otherwise + # return None. Turn the None into something + # MatchesPredicate recognizes as success. + or True, + "did not find meta refresh tag in %r", + ), + ), + ) + + def test_none(self): + """ + If the decorated method returns ``None`` then the response is finished + with no additional content. + """ + self.assertThat( + render(StaticResource(None), {}), + succeeded( + Equals(b""), + ), + ) + + def test_not_done_yet(self): + """ + If the decorated method returns ``NOT_DONE_YET`` then the resource is + responsible for finishing the request itself. + """ + the_request = [] + class R(Resource): + @render_exception + def render(self, request): + the_request.append(request) + return NOT_DONE_YET + + d = render(R(), {}) + + self.assertThat( + d, + has_no_result(), + ) + + the_request[0].write(b"some content") + the_request[0].finish() + + self.assertThat( + d, + succeeded( + Equals(b"some content"), + ), + ) + + def test_unknown(self): + """ + If the decorated method returns something which is not explicitly + supported, an internal server error is rendered into the response. + """ + self.assertThat( + render(StaticResource(object()), {}), + succeeded( + Equals(b"Internal Server Error"), + ), + ) diff --git a/src/allmydata/web/common.py b/src/allmydata/web/common.py index 56bef9dfc..215abb46e 100644 --- a/src/allmydata/web/common.py +++ b/src/allmydata/web/common.py @@ -605,7 +605,7 @@ def _finish(result, render, request): result, )) request.setResponseCode(http.INTERNAL_SERVER_ERROR) - _finish(b"", render, request) + _finish(b"Internal Server Error", render, request) def _renderHTTP_exception(request, failure): From 3192715e37c0bc3fd4bb217efabc86c493723392 Mon Sep 17 00:00:00 2001 From: Jean-Paul Calderone Date: Mon, 19 Oct 2020 14:37:03 -0400 Subject: [PATCH 22/31] everything newstyle --- src/allmydata/test/web/test_common.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/allmydata/test/web/test_common.py b/src/allmydata/test/web/test_common.py index f9152e126..b399b77c8 100644 --- a/src/allmydata/test/web/test_common.py +++ b/src/allmydata/test/web/test_common.py @@ -43,7 +43,7 @@ from .common import ( assert_soup_has_tag_with_attributes, ) -class StaticResource(Resource): +class StaticResource(Resource, object): def __init__(self, response): Resource.__init__(self) self._response = response From e7c04f8810639b71222da3fec30f6710d289c51c Mon Sep 17 00:00:00 2001 From: Jean-Paul Calderone Date: Mon, 19 Oct 2020 15:41:38 -0400 Subject: [PATCH 23/31] another news fragment! --- newsfragments/3314.minor | 0 1 file changed, 0 insertions(+), 0 deletions(-) create mode 100644 newsfragments/3314.minor diff --git a/newsfragments/3314.minor b/newsfragments/3314.minor new file mode 100644 index 000000000..e69de29bb From 1ed74604c7e3fd4fbd02549d24a52707b88ddced Mon Sep 17 00:00:00 2001 From: Jean-Paul Calderone Date: Wed, 21 Oct 2020 07:15:36 -0400 Subject: [PATCH 24/31] Use twisted.web.server.Request instead of DummyRequest in the tests Always prefer the real thing if possible --- src/allmydata/test/common_tweb.py | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/src/allmydata/test/common_tweb.py b/src/allmydata/test/common_tweb.py index 2af9d4f53..cfaf85c6c 100644 --- a/src/allmydata/test/common_tweb.py +++ b/src/allmydata/test/common_tweb.py @@ -8,7 +8,10 @@ from twisted.internet.defer import ( succeed, ) from twisted.web.test.requesthelper import ( - DummyRequest, + DummyChannel, +) +from twisted.web.server import ( + Request, ) from twisted.web.iweb import ( IRequest, @@ -17,8 +20,6 @@ from twisted.web.server import ( NOT_DONE_YET, ) -classImplements(DummyRequest, IRequest) - def render(resource, query_args): """ Render (in the manner of the Twisted Web Site) a Twisted ``Resource`` @@ -32,7 +33,8 @@ def render(resource, query_args): :return Deferred: A Deferred that fires with the rendered response body as ``bytes``. """ - request = DummyRequest([]) + channel = DummyChannel() + request = Request(channel) request.args = query_args result = resource.render(request) if isinstance(result, bytes): @@ -50,5 +52,9 @@ def render(resource, query_args): result, ), ) - done.addCallback(lambda ignored: b"".join(request.written)) + def get_body(ignored): + complete_response = channel.transport.written.getvalue() + header, body = complete_response.split(b"\r\n\r\n", 1) + return body + done.addCallback(get_body) return done From a91dba5f5bb07a68ea561880a8215f1516803e13 Mon Sep 17 00:00:00 2001 From: Jean-Paul Calderone Date: Wed, 21 Oct 2020 07:18:41 -0400 Subject: [PATCH 25/31] _finish docstring --- src/allmydata/web/common.py | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/src/allmydata/web/common.py b/src/allmydata/web/common.py index 215abb46e..3338b4a1f 100644 --- a/src/allmydata/web/common.py +++ b/src/allmydata/web/common.py @@ -548,6 +548,23 @@ def render_exception(render): def _finish(result, render, request): + """ + Try to finish rendering the response to a request. + + This implements extra convenience functionality not provided by Twisted + Web. Various resources in Tahoe-LAFS made use of this functionality when + it was provided by Nevow. Rather than making that application code do the + more tedious thing itself, we duplicate the functionality here. + + :param result: Something returned by a render method which we can turn + into a response. + + :param render: The original render method which produced the result. + + :param request: The request being responded to. + + :return: ``None`` + """ if isinstance(result, Failure): Message.log( message_type=u"allmydata:web:common-render:failure", From dbe2d4efd724afdb1e3cc39aad1b479bde09f429 Mon Sep 17 00:00:00 2001 From: Jean-Paul Calderone Date: Wed, 21 Oct 2020 07:22:00 -0400 Subject: [PATCH 26/31] It isn't the root, it's wherever we actually are --- src/allmydata/web/operations.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/allmydata/web/operations.py b/src/allmydata/web/operations.py index b19366ff6..0e53d075d 100644 --- a/src/allmydata/web/operations.py +++ b/src/allmydata/web/operations.py @@ -90,8 +90,8 @@ class OphandleTable(resource.Resource, service.Service): """ ophandle = get_arg(req, "ophandle").decode("utf-8") assert ophandle - root = DecodedURL.from_text(unicode(URLPath.fromRequest(req))) - target = root.click(u"/").child(u"operations", ophandle) + here = DecodedURL.from_text(unicode(URLPath.fromRequest(req))) + target = here.click(u"/").child(u"operations", ophandle) output = get_arg(req, "output") if output: target = target.add(u"output", output.decode("utf-8")) From d8b6e36c6f6c024d8227893ea27b19fd377247c1 Mon Sep 17 00:00:00 2001 From: Jean-Paul Calderone Date: Wed, 21 Oct 2020 07:23:16 -0400 Subject: [PATCH 27/31] docstring for StaticResource --- src/allmydata/test/web/test_common.py | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/allmydata/test/web/test_common.py b/src/allmydata/test/web/test_common.py index b399b77c8..5bc8ce4f0 100644 --- a/src/allmydata/test/web/test_common.py +++ b/src/allmydata/test/web/test_common.py @@ -44,6 +44,11 @@ from .common import ( ) class StaticResource(Resource, object): + """ + ``StaticResource`` is a resource that returns whatever Python object it is + given from its render method. This is useful for testing + ``render_exception``\\ 's handling of different render results. + """ def __init__(self, response): Resource.__init__(self) self._response = response From e8761c98ad0ed989a2990514fd419c55c26942d4 Mon Sep 17 00:00:00 2001 From: Jean-Paul Calderone Date: Wed, 21 Oct 2020 07:25:48 -0400 Subject: [PATCH 28/31] A comment about what `_finish` is here for --- src/allmydata/web/common.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/allmydata/web/common.py b/src/allmydata/web/common.py index 3338b4a1f..038340fe8 100644 --- a/src/allmydata/web/common.py +++ b/src/allmydata/web/common.py @@ -540,6 +540,8 @@ def render_exception(render): with action.context(): result = DeferredContext(maybeDeferred(bound_render, request)) + # Apply `_finish` all of our result handling logic to whatever it + # returned. result.addBoth(_finish, bound_render, request) result.addActionFinish() return NOT_DONE_YET From 7ce2122e71bf28949ceb703726d4f05638be8515 Mon Sep 17 00:00:00 2001 From: Jean-Paul Calderone Date: Wed, 21 Oct 2020 07:26:12 -0400 Subject: [PATCH 29/31] one more reference --- src/allmydata/test/web/test_common.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/allmydata/test/web/test_common.py b/src/allmydata/test/web/test_common.py index 5bc8ce4f0..6431ba610 100644 --- a/src/allmydata/test/web/test_common.py +++ b/src/allmydata/test/web/test_common.py @@ -60,7 +60,7 @@ class StaticResource(Resource, object): class RenderExceptionTests(SyncTestCase): """ - Tests for ``render_exception``. + Tests for ``render_exception`` (including the private helper ``_finish``). """ def test_exception(self): """ From f602382244bff91e92d17e67f603a031f5279575 Mon Sep 17 00:00:00 2001 From: Jean-Paul Calderone Date: Wed, 21 Oct 2020 07:34:27 -0400 Subject: [PATCH 30/31] Comments about __get__ calls --- src/allmydata/web/common.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/allmydata/web/common.py b/src/allmydata/web/common.py index 038340fe8..6c8b557fa 100644 --- a/src/allmydata/web/common.py +++ b/src/allmydata/web/common.py @@ -483,6 +483,8 @@ def exception_to_child(getChild): """ @wraps(getChild) def g(self, name, req): + # Bind the method to the instance so it has a better + # fullyQualifiedName later on. This is not necessary on Python 3. bound_getChild = getChild.__get__(self, type(self)) action = start_action( @@ -526,6 +528,8 @@ def render_exception(render): """ @wraps(render) def g(self, request): + # Bind the method to the instance so it has a better + # fullyQualifiedName later on. This is not necessary on Python 3. bound_render = render.__get__(self, type(self)) action = start_action( From 0dcc3e13c037ef0860636ad290a1eaeff8710213 Mon Sep 17 00:00:00 2001 From: Jean-Paul Calderone Date: Wed, 21 Oct 2020 08:21:29 -0400 Subject: [PATCH 31/31] Remove unused imports --- src/allmydata/test/common_tweb.py | 6 ------ 1 file changed, 6 deletions(-) diff --git a/src/allmydata/test/common_tweb.py b/src/allmydata/test/common_tweb.py index cfaf85c6c..37e24b5b8 100644 --- a/src/allmydata/test/common_tweb.py +++ b/src/allmydata/test/common_tweb.py @@ -1,6 +1,3 @@ -from zope.interface import ( - classImplements, -) from twisted.python.reflect import ( fullyQualifiedName, ) @@ -13,9 +10,6 @@ from twisted.web.test.requesthelper import ( from twisted.web.server import ( Request, ) -from twisted.web.iweb import ( - IRequest, -) from twisted.web.server import ( NOT_DONE_YET, )