( )⚙ D2776 hgweb: use a multidict for holding query string parameters

This is an archive of the discontinued Mercurial Phabricator instance.

Differential D2776

hgweb: use a multidict for holding query string parameters
ClosedPublic

Authored by indygreg on Mar 9 2018, 11:53 PM.
Tags
None
Subscribers
yuja, av6, martinvonz and 2 others

Details

Reviewers
durin42
Group Reviewers
hg-reviewers
Commits
rHGec0af9c59270: hgweb: use a multidict for holding query string parameters
Summary

My intention with refactoring the WSGI code was to make it easier
to read. I initially wanted to vendor and use WebOb, because it seems
to be a pretty reasonable abstraction layer for WSGI. However, it isn't
using relative imports and I didn't want to deal with the hassle of
patching it. But that doesn't mean we can't use good ideas from WebOb.

WebOb has a "multidict" data structure for holding parsed query string
and POST form data. It quacks like a dict but allows you to store
multiple values for each key. It offers mechanisms to return just one
value, all values, or return 1 value asserting that only 1 value is
set. I quite like its API.

This commit implements a read-only "multidict" in the spirit of
WebOb's multidict.

We replace the query string attributes of our parsed request with
an instance of it.

Diff Detail

Repository
rHG Mercurial
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

indygreg created this revision.Mar 9 2018, 11:53 PM
indygreg edited the summary of this revision. (Show Details)Mar 10 2018, 3:03 PM
indygreg added a child revision: D2779: hgweb: use our new request object for "style" parameter.
indygreg updated this revision to Diff 6838.
indygreg edited the summary of this revision. (Show Details)Mar 10 2018, 3:57 PM
indygreg updated this revision to Diff 6840.
durin42 added a subscriber: durin42.Mar 12 2018, 5:04 PM
durin42 added inline comments.
mercurial/hgweb/request.py
31

I'm a little uncomfortable with this not advertising that it's not a hash table, and htat lookups are O(N). Maybe send a docstring followup?

durin42 accepted this revision.Mar 12 2018, 5:04 PM
This revision is now accepted and ready to land.Mar 12 2018, 5:04 PM
Closed by commit rHGec0af9c59270: hgweb: use a multidict for holding query string parameters (authored by indygreg). · Explain WhyMar 12 2018, 5:32 PM
This revision was automatically updated to reflect the committed changes.
martinvonz added a subscriber: martinvonz.Mar 14 2018, 12:33 AM
martinvonz added inline comments.
mercurial/hgweb/request.py
39–41

Sure, that's probably fine, but why? Wouldn't it be easier to write it as dict of lists anyway?

45

Would it make sense to make this an error if there isn't exactly one value?

av6 added a subscriber: av6.Mar 14 2018, 5:39 AM
av6 added inline comments.
mercurial/hgweb/request.py
45

I don't think it would. AIUI, that would make handling query strings like ?style=foo&style=bar just more difficult. I'm not aware of any web tools where default access method wouldn't simply return that last value.

indygreg added inline comments.Mar 14 2018, 10:51 AM
mercurial/hgweb/request.py
31

I agree with the sentiment about this being a list in disguise. One reason I didn't bother to optimize it is because I don't think we do any qsparams lookups in loops and I don't believe we have any more than ~10 arguments to any single request. So even if we have an O(n^2) situation, n is so small that it doesn't matter.

I can add a follow-up comment easily enough. Or we could just index the fields by key. That doesn't seem too difficult.

yuja added a subscriber: yuja.Mar 15 2018, 11:38 AM
yuja added inline comments.
mercurial/hgweb/request.py
31

Isn't the n controllable by a malicious user?

I agree with @martinvonz in that it would be probably easier to
write as a dict of lists (or dict + lists).

indygreg added a comment.Mar 15 2018, 2:27 PM

I may have a shot at rewriting this class today. I do want to prioritize on adding missing components to the new wire protocol so reviewers have better context, however. So if someone else wants to do the work, I'd happily review it. Otherwise, I should get around to it sometime in the next few days.

Revision Contents
Changeset List

PathPackages
Mmercurial/hgweb/request.py (106 lines)
Mmercurial/wireprotoserver.py (4 lines)

Diff 6838

mercurial/hgweb/request.py

attr, attr,
) )
from .. import ( from .. import (
error, error,
pycompat, pycompat,
util, util,
) )
class multidict(object):
durin42Unsubmitted
Not Done

I'm a little uncomfortable with this not advertising that it's not a hash table, and htat lookups are O(N). Maybe send a docstring followup?

durin42: I'm a little uncomfortable with this not advertising that it's not a hash table, and htat…
indygregAuthorUnsubmitted
Not Done

I agree with the sentiment about this being a list in disguise. One reason I didn't bother to optimize it is because I don't think we do any qsparams lookups in loops and I don't believe we have any more than ~10 arguments to any single request. So even if we have an O(n^2) situation, n is so small that it doesn't matter.

I can add a follow-up comment easily enough. Or we could just index the fields by key. That doesn't seem too difficult.

indygreg: I agree with the sentiment about this being a list in disguise. One reason I didn't bother to…
yujaUnsubmitted
Not Done

Isn't the n controllable by a malicious user?

I agree with @martinvonz in that it would be probably easier to
write as a dict of lists (or dict + lists).

yuja: Isn't the `n` controllable by a malicious user? I agree with @martinvonz in that it would be…
"""A dict like object that can store multiple values for a key.
Used to store parsed request parameters.
This is inspired by WebOb's class of the same name.
"""
def __init__(self):
# Stores (key, value) 2-tuples. This isn't the most efficient. But we
# don't rely on parameters that much, so it shouldn't be a perf issue.
# we can always add dict for fast lookups.
martinvonzUnsubmitted
Not Done

Sure, that's probably fine, but why? Wouldn't it be easier to write it as dict of lists anyway?

martinvonz: Sure, that's probably fine, but why? Wouldn't it be easier to write it as dict of lists anyway?
self._items = []
def __getitem__(self, key):
"""Returns the last set value for a key."""
martinvonzUnsubmitted
Not Done

Would it make sense to make this an error if there isn't exactly one value?

martinvonz: Would it make sense to make this an error if there isn't exactly one value?
av6Unsubmitted
Not Done

I don't think it would. AIUI, that would make handling query strings like ?style=foo&style=bar just more difficult. I'm not aware of any web tools where default access method wouldn't simply return that last value.

av6: I don't think it would. AIUI, that would make handling query strings like ?style=foo&style=bar…
for k, v in reversed(self._items):
if k == key:
return v
raise KeyError(key)
def __setitem__(self, key, value):
"""Replace a values for a key with a new value."""
try:
del self[key]
except KeyError:
pass
self._items.append((key, value))
def __delitem__(self, key):
"""Delete all values for a key."""
oldlen = len(self._items)
self._items[:] = [(k, v) for k, v in self._items if k != key]
if oldlen == len(self._items):
raise KeyError(key)
def __contains__(self, key):
return any(k == key for k, v in self._items)
def __len__(self):
return len(self._items)
def get(self, key, default=None):
try:
return self.__getitem__(key)
except KeyError:
return default
def add(self, key, value):
"""Add a new value for a key. Does not replace existing values."""
self._items.append((key, value))
def getall(self, key):
"""Obtains all values for a key."""
return [v for k, v in self._items if k == key]
def getone(self, key):
"""Obtain a single value for a key.
Raises KeyError if key not defined or it has multiple values set.
"""
vals = self.getall(key)
if not vals:
raise KeyError(key)
if len(vals) > 1:
raise KeyError('multiple values for %r' % key)
return vals[0]
def asdictoflists(self):
d = {}
for k, v in self._items:
if k in d:
d[k].append(v)
else:
d[k] = [v]
return d
@attr.s(frozen=True) @attr.s(frozen=True)
class parsedrequest(object): class parsedrequest(object):
"""Represents a parsed WSGI request. """Represents a parsed WSGI request.
Contains both parsed parameters as well as a handle on the input stream. Contains both parsed parameters as well as a handle on the input stream.
""" """
# Request method. # Request method.
dispatchparts = attr.ib() dispatchparts = attr.ib()
# URL path component (no query string) used for dispatch. # URL path component (no query string) used for dispatch.
dispatchpath = attr.ib() dispatchpath = attr.ib()
# Whether there is a path component to this request. This can be true # Whether there is a path component to this request. This can be true
# when ``dispatchpath`` is empty due to REPO_NAME muckery. # when ``dispatchpath`` is empty due to REPO_NAME muckery.
havepathinfo = attr.ib() havepathinfo = attr.ib()
# Raw query string (part after "?" in URL). # Raw query string (part after "?" in URL).
querystring = attr.ib() querystring = attr.ib()
# List of 2-tuples of query string arguments. # multidict of query string parameters.
querystringlist = attr.ib() qsparams = attr.ib()
# Dict of query string arguments. Values are lists with at least 1 item.
querystringdict = attr.ib()
# wsgiref.headers.Headers instance. Operates like a dict with case # wsgiref.headers.Headers instance. Operates like a dict with case
# insensitive keys. # insensitive keys.
headers = attr.ib() headers = attr.ib()
# Request body input stream. # Request body input stream.
bodyfh = attr.ib() bodyfh = attr.ib()
def parserequestfromenv(env, bodyfh): def parserequestfromenv(env, bodyfh):
"""Parse URL components from environment variables. """Parse URL components from environment variables.
dispatchparts = [] dispatchparts = []
dispatchpath = '/'.join(dispatchparts) dispatchpath = '/'.join(dispatchparts)
querystring = env.get('QUERY_STRING', '') querystring = env.get('QUERY_STRING', '')
# We store as a list so we have ordering information. We also store as # We store as a list so we have ordering information. We also store as
# a dict to facilitate fast lookup. # a dict to facilitate fast lookup.
querystringlist = util.urlreq.parseqsl(querystring, keep_blank_values=True) qsparams = multidict()
for k, v in util.urlreq.parseqsl(querystring, keep_blank_values=True):
querystringdict = {} qsparams.add(k, v)
for k, v in querystringlist:
if k in querystringdict:
querystringdict[k].append(v)
else:
querystringdict[k] = [v]
# HTTP_* keys contain HTTP request headers. The Headers structure should # HTTP_* keys contain HTTP request headers. The Headers structure should
# perform case normalization for us. We just rewrite underscore to dash # perform case normalization for us. We just rewrite underscore to dash
# so keys match what likely went over the wire. # so keys match what likely went over the wire.
headers = [] headers = []
for k, v in env.iteritems(): for k, v in env.iteritems():
if k.startswith('HTTP_'): if k.startswith('HTTP_'):
headers.append((k[len('HTTP_'):].replace('_', '-'), v)) headers.append((k[len('HTTP_'):].replace('_', '-'), v))
return parsedrequest(method=env['REQUEST_METHOD'], return parsedrequest(method=env['REQUEST_METHOD'],
url=fullurl, baseurl=baseurl, url=fullurl, baseurl=baseurl,
advertisedurl=advertisedfullurl, advertisedurl=advertisedfullurl,
advertisedbaseurl=advertisedbaseurl, advertisedbaseurl=advertisedbaseurl,
apppath=apppath, apppath=apppath,
dispatchparts=dispatchparts, dispatchpath=dispatchpath, dispatchparts=dispatchparts, dispatchpath=dispatchpath,
havepathinfo='PATH_INFO' in env, havepathinfo='PATH_INFO' in env,
querystring=querystring, querystring=querystring,
querystringlist=querystringlist, qsparams=qsparams,
querystringdict=querystringdict,
headers=headers, headers=headers,
bodyfh=bodyfh) bodyfh=bodyfh)
class wsgiresponse(object): class wsgiresponse(object):
"""Represents a response to a WSGI request. """Represents a response to a WSGI request.
A response consists of a status line, headers, and a body. A response consists of a status line, headers, and a body.
inp = util.cappedreader(inp, int(wsgienv[r'CONTENT_LENGTH'])) inp = util.cappedreader(inp, int(wsgienv[r'CONTENT_LENGTH']))
self.err = wsgienv[r'wsgi.errors'] self.err = wsgienv[r'wsgi.errors']
self.threaded = wsgienv[r'wsgi.multithread'] self.threaded = wsgienv[r'wsgi.multithread']
self.multiprocess = wsgienv[r'wsgi.multiprocess'] self.multiprocess = wsgienv[r'wsgi.multiprocess']
self.run_once = wsgienv[r'wsgi.run_once'] self.run_once = wsgienv[r'wsgi.run_once']
self.env = wsgienv self.env = wsgienv
self.req = parserequestfromenv(wsgienv, inp) self.req = parserequestfromenv(wsgienv, inp)
self.form = self.req.querystringdict self.form = self.req.qsparams.asdictoflists()
self.res = wsgiresponse(self.req, start_response) self.res = wsgiresponse(self.req, start_response)
self._start_response = start_response self._start_response = start_response
self.server_write = None self.server_write = None
self.headers = [] self.headers = []
def respond(self, status, type, filename=None, body=None): def respond(self, status, type, filename=None, body=None):
if not isinstance(type, str): if not isinstance(type, str):
type = pycompat.sysstr(type) type = pycompat.sysstr(type)

mercurial/wireprotoserver.py

# Avoid cycle involving hg module. # Avoid cycle involving hg module.
from .hgweb import common as hgwebcommon from .hgweb import common as hgwebcommon
repo = rctx.repo repo = rctx.repo
# HTTP version 1 wire protocol requests are denoted by a "cmd" query # HTTP version 1 wire protocol requests are denoted by a "cmd" query
# string parameter. If it isn't present, this isn't a wire protocol # string parameter. If it isn't present, this isn't a wire protocol
# request. # request.
if 'cmd' not in req.querystringdict: if 'cmd' not in req.qsparams:
return False return False
cmd = req.querystringdict['cmd'][0] cmd = req.qsparams['cmd']
# The "cmd" request parameter is used by both the wire protocol and hgweb. # The "cmd" request parameter is used by both the wire protocol and hgweb.
# While not all wire protocol commands are available for all transports, # While not all wire protocol commands are available for all transports,
# if we see a "cmd" value that resembles a known wire protocol command, we # if we see a "cmd" value that resembles a known wire protocol command, we
# route it to a protocol handler. This is better than routing possible # route it to a protocol handler. This is better than routing possible
# wire protocol requests to hgweb because it prevents hgweb from using # wire protocol requests to hgweb because it prevents hgweb from using
# known wire protocol commands and it is less confusing for machine # known wire protocol commands and it is less confusing for machine
# clients. # clients.