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

This is an archive of the discontinued Mercurial Phabricator instance.

hgweb: use a multidict for holding query string parameters
ClosedPublic

Authored by indygreg on Mar 9 2018, 11:53 PM.
Tags
None
Subscribers

Details

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 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 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
This revision was automatically updated to reflect the committed changes.
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).

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.