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 of lists inside params dict #4714

Closed
Lonami opened this issue Apr 26, 2020 · 15 comments · Fixed by aio-libs/yarl#443
Closed

Use of lists inside params dict #4714

Lonami opened this issue Apr 26, 2020 · 15 comments · Fixed by aio-libs/yarl#443

Comments

@Lonami
Copy link

Lonami commented Apr 26, 2020

Is there any real reason not to support the following? In my opinion, these two should behave the same:

params = {'key': ['v1', 'v2']}
params = [('key', 'v1'), ('key', 'v2')]

However, they don't (as documented in Passing Parameters In URLs).

I've also found #864, #2504, #3716, #4134, but I'm not satisfied with the answers ("it's by design", "there is no such standard"). There is a clear interest in supporting this. requests itself being such a popular library interprets the use of a list inside a dict values as repeating the key multiple times, and it's a very sensible thing to do.

So what's the real reason not to support this in aiohttp? It makes it slightly more annoying to use.


As a motivational example, compare:

params = {
    'client': 'webapp',
    'hl': 'en',
    'dt': ['at', 'bd', 'ex', 'ld', 'md', 'qca', 'rw', 'rm', 'sos', 'ss', 't'],
    'ie': 'UTF-8',
    'oe': 'UTF-8',
    'otf': 1,
    'ssel': 0,
    'tsel': 0,
}

With the workaround:

params = [
    ('client', 'webapp'),
    ('hl', 'en'),
    *[('dt', x) for x in ['at', 'bd', 'ex', 'ld', 'md', 'qca', 'rw', 'rm', 'sos', 'ss', 't']],
    ('ie', 'UTF-8'),
    ('oe', 'UTF-8'),
    ('otf', 1),
    ('ssel', 0),
    ('tsel', 0),
]

The first one is a lot easier to write, and can come up straight from, say, Firefox network tab when you copy the query parameters as JSON with no changes required.

@webknjaz
Copy link
Member

Because it's semantically ambiguous and {'key': ['val1', 'val'2]} could be read as key=[val1,val2], for example.

@webknjaz
Copy link
Member

Also, in the doc you referenced you're given an option to use multidict explicitly:

For sending data with multiple values for the same key MultiDict may be used as well.

Multidict is designed in such a way that one key can have multiple values. And it's directly converted into query args:

In [1]: import multidict                                                                                            

In [2]: multidict.MultiDict                                                                                         
Out[2]: multidict._multidict.MultiDict

In [3]: md = multidict.MultiDict()                                                                                  

In [4]: md                                                                                                          
Out[4]: <MultiDict()>

In [5]: md['key'] = 'val1'                                                                                          

In [6]: md['key'] = 'val2'                                                                                          

In [7]: md['key']                                                                                                   
Out[7]: 'val2'

In [8]: md                                                                                                          
Out[8]: <MultiDict('key': 'val2')>

In [9]: md.getall('key')                                                                                            
Out[9]: ['val2']

In [10]: md.add('key', 'val1')                                                                                      

In [11]: md                                                                                                         
Out[11]: <MultiDict('key': 'val2', 'key': 'val1')>

@Lonami
Copy link
Author

Lonami commented Apr 26, 2020

Because it's semantically ambiguous

How? If a key has multiple values and is URL-encoded, you would expect it to become key=val1&keyval=2. If I wanted key=[val1,val2] then I would do just that:

{
    'key': '[val1,val2]'
}

It's also the way Firefox represents such a thing. For example, if you open https://example.com/?a=1&b=2&b=3&c=4 with the network tab open, and click Copy All in the Query String under the Params tab, you will get the following JSON (which follows my suggestion):

{"a":"1","b":["2","3"],"c":"4"}

This happens to be valid Python, and it would be extremely convenient to be able to just copy-paste it into the code.

In case it's not clear what I'm talking about:
image

@Lonami
Copy link
Author

Lonami commented Apr 26, 2020

See also Python's built-in urllib.parse.urlencode:

import urllib.parse
params = {
    'key': ['val1', 'val2']
}
print(urllib.parse.urlencode(params, doseq=True))

Now I admit I had to use doseq=True to get it to work, but this is because urlencode turns into str everything, which aiohttp doesn't do (it errors on invalid types as opposed to silently casting to str).

Because in aiohttp it's a hard-error to use sequences, I think it would make sense to interpret them correctly as sequences. With this example, I'm just further showing it makes sense: it's even possible in the standard library.

@Lonami
Copy link
Author

Lonami commented Apr 26, 2020

With regards to your multidict approach, it would still be far less ergonomic to use with my original example:

import multidict
params = multidict.MultiDict({
    'client': 'webapp',
    'hl': 'en',
    'dt': 'at',
    'ie': 'UTF-8',
    'oe': 'UTF-8',
    'otf': 1,
    'ssel': 0,
    'tsel': 0,
})
for val in ['bd', 'ex', 'ld', 'md', 'qca', 'rw', 'rm', 'sos', 'ss', 't']:
    params.add('dt', val)

(There may be a better way using MultiDict, but I'm basing myself on your response only).

@webknjaz
Copy link
Member

I hear you. You may try sending a PR and if it doesn't introduce the performance penalty it may be accepted. It looks like params are processed here: https://github.com/aio-libs/yarl/blob/6791d42/yarl/__init__.py#L867-L905.

@Lonami
Copy link
Author

Lonami commented Apr 27, 2020

Will work on it now that I have green light, thanks. Shouldn't hurt performance if it's the last case in the elif chain (since after it'd error, uncommon case).

@webknjaz
Copy link
Member

Okay. But FTR it's a green light from me and other maintainers may still need convincing. It'll also require a docs update in this repo and good test coverage.

@Lonami
Copy link
Author

Lonami commented Apr 27, 2020

The test coverage is done in the yarl library, I don't think it would be a good idea to duplicate code here when it's already tested there anyway since aiohttp delegates to that library to do the quoting. Similarly, the documentation has been updated there, but I am willing to make the changes here if required.

@webknjaz
Copy link
Member

I think mentioning that this only works since a certain yarl version would be useful.

@Lonami
Copy link
Author

Lonami commented Apr 27, 2020

Installing aiohttp should pick up the most up-to-date version of yarl:

'yarl>=1.0,<2.0',

But surely, it can be worth mentioning it for people that already had aiohttp installed, once/if it gets merged there.

@webknjaz
Copy link
Member

Installing aiohttp should pick up the most up-to-date version of yarl:

Yes, but it is the best practice that apps have version pins in their env constraints to avoid surprises. So they'll be stuck with an old version unless they update pins. And the doc doesn't even mention YARL so most folks have no idea that this is a feature that depends on something of aiohttp itself.

@webknjaz
Copy link
Member

@Lonami mind updating the docs here?

@webknjaz webknjaz reopened this May 11, 2020
@Lonami
Copy link
Author

Lonami commented May 11, 2020

Yeah, will do later today.

@Lonami
Copy link
Author

Lonami commented Oct 15, 2020

Closed by 766897c.

@Lonami Lonami closed this as completed Oct 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants