Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use bytes or unicode to get args #164

Closed
wants to merge 6 commits into from
Closed
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 3 additions & 3 deletions src/klein/app.py
Expand Up @@ -24,7 +24,7 @@ def iscoroutine(*args, **kwargs):

from twisted.web.iweb import IRenderable
from twisted.web.template import renderElement
from twisted.web.server import Site, Request
from twisted.web.server import Request
from twisted.internet import reactor, endpoints

try:
Expand All @@ -35,7 +35,7 @@ def ensureDeferred(*args, **kwagrs):

from zope.interface import implementer

from klein.resource import KleinResource
from klein.resource import KleinResource, KleinSite
from klein.interfaces import IKleinRequest

__all__ = ['Klein', 'run', 'route', 'resource']
Expand Down Expand Up @@ -367,7 +367,7 @@ def run(self, host=None, port=None, logFile=None,
host)

endpoint = endpoints.serverFromString(reactor, endpoint_description)
endpoint.listen(Site(self.resource()))
endpoint.listen(KleinSite(self.resource()))
reactor.run()


Expand Down
38 changes: 37 additions & 1 deletion src/klein/resource.py
Expand Up @@ -5,7 +5,7 @@
from twisted.internet import defer
from twisted.python import log, failure
from twisted.python.compat import unicode, intToBytes
from twisted.web import server
from twisted.web import http, server
from twisted.web.iweb import IRenderable
from twisted.web.resource import Resource, IResource, getChildForRequest
from twisted.web.server import NOT_DONE_YET
Expand Down Expand Up @@ -280,3 +280,39 @@ def write_response(r):
d.addCallback(write_response).addErrback(log.err, _why="Unhandled Error writing response")

return server.NOT_DONE_YET



class KleinHTTPRequest(server.Request):

def getArg(self, key):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really don't like the name getArg. "args" are an incredibly ill-defined mess in Twisted, mixing together query parameters ("GET args") and URL-encoded or multipart form-values in the body ("POST args" or "form args"). If we're going to try to do something better, it would be nice to clean this up a bit. Even if it is accessing the underlying .args, I feel like we should be disambiguating these namespaces.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not a big fan of getters and setters either but it was the simple and easy to understand. I had a more elaborate solution using a dict-like object for the args variable but other devs weren't convinced with it and ultimatly it was scraped in favor of getArgs. As far as the Twisted mess, you provided a ticket #228 but work on this might have stalled. Any hope of reviving that?

"""
Get a single arg value.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You forgot to document @param key, in particular @type key, which seems like rather the whole point of this change :).


@raises KeyError: If key doesn't exist
@raises ValueError: If there is more than 1 value
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You forgot about UnicodeDecodeError, a natural consequence of ensure_utf8_bytes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch

@return: L{list} of L{bytes}
"""
key = ensure_utf8_bytes(key)
value = self.args[key]
if len(value) != 1:
raise ValueError('Too many values for: {0}'.format(key))
return value[0]

def getArgs(self, key):
"""
Get the list of values for a key.

@return: L{list} of L{bytes}
"""
key = ensure_utf8_bytes(key)
return self.args.get(key, [])



class KleinSite(server.Site):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this intended to be public? Do we need to use it for all Klein applications now? That seems like a pretty big change to quietly be foisting on all Klein applications. (That implies .app.resource() won't work in a normal server any more, or will randomly be missing some methods.)

If it is intended to be public, some docstrings seems like a bare minimum. Also, Site is an old-style class, so this should definitely have an , object at the end of its inheritance hierarchy.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a rather ugly hack on my part and I'd like to move away from it. At the time I did not know of alternative ways to overload Request. In retrospect, I should've created an Request adapter or simply append the function to the request object in app.route(). Any thoughts on how to go about this?

def buildProtocol(self, addr):
channel = http.HTTPFactory.buildProtocol(self, addr)
channel.requestFactory = KleinHTTPRequest
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you considered using the requestFactory argument to Site.__init__ rather than having a custom subclass?

channel.site = self
return channel
6 changes: 3 additions & 3 deletions src/klein/test/test_app.py
Expand Up @@ -246,12 +246,12 @@ def foo(request):


@patch('klein.app.KleinResource')
@patch('klein.app.Site')
@patch('klein.app.KleinSite')
@patch('klein.app.log')
@patch('klein.app.reactor')
def test_run(self, reactor, mock_log, mock_site, mock_kr):
"""
L{Klein.run} configures a L{KleinResource} and a L{Site}
L{Klein.run} configures a L{KleinResource} and a L{KleinSite}
listening on the specified interface and port, and logs
to stdout.
"""
Expand All @@ -270,7 +270,7 @@ def test_run(self, reactor, mock_log, mock_site, mock_kr):


@patch('klein.app.KleinResource')
@patch('klein.app.Site')
@patch('klein.app.KleinSite')
@patch('klein.app.log')
@patch('klein.app.reactor')
def test_runWithLogFile(self, reactor, mock_log, mock_site, mock_kr):
Expand Down
61 changes: 61 additions & 0 deletions src/klein/test/test_request.py
@@ -0,0 +1,61 @@
from __future__ import absolute_import

from klein import Klein
from klein.resource import KleinHTTPRequest, KleinSite
from klein.test.util import TestCase
from twisted.web.test.requesthelper import DummyChannel

class BytesUnicodeTest(TestCase):

def setUp(self):
self.encoding = 'utf-8'
app = Klein()
site = KleinSite(app.resource())
channel = site.buildProtocol('127.0.0.1')
self.request = channel.requestFactory(DummyChannel(), None)
self.request.args = {}

def test_getArg(self):
str_key = 'test'
bytes_key = str_key.encode(self.encoding)
value = b'hello world'

self.request.args[bytes_key] = [value]
self.assertEquals(self.request.getArg(str_key), value)
self.assertEquals(self.request.getArg(bytes_key), value)

def test_getArg_not_1(self):
"""
Raise exception if there are more or less values than 1
"""
str_key = 'test'
bytes_key = str_key.encode(self.encoding)
values = []

self.request.args[bytes_key] = values
self.assertRaises(ValueError, self.request.getArg, str_key)
self.assertRaises(ValueError, self.request.getArg, bytes_key)

values.extend([b'hello', b'world'])
self.assertRaises(ValueError, self.request.getArg, str_key)
self.assertRaises(ValueError, self.request.getArg, bytes_key)

def test_getArgs(self):
str_key = 'test'
bytes_key = str_key.encode(self.encoding)
values = [b'hello world', b'hey earth']
self.request.args[bytes_key] = values

self.assertEquals(len(self.request.getArgs(str_key)), len(values))
self.assertEquals(self.request.getArgs(str_key), values)
self.assertEquals(self.request.getArgs(bytes_key), values)

def test_getArgs_no_key(self):
"""
By default, an empty list is returned if a key doesn't exist
"""
str_key = 'test'
bytes_key = str_key.encode(self.encoding)

self.assertEquals(self.request.getArgs(str_key), [])
self.assertEquals(self.request.getArgs(bytes_key), [])