Raise BadRequestKeyError in MultiDict.__getitem__ when internal list is empty

[alexpantyukhin]

I think that it would be logically to raise BadRequestKeyError there https://github.com/pallets/werkzeug/blob/master/werkzeug/datastructures.py#L402 when the list of values is empty.

[untitaker]

Why do you think this is better behavior?

[alexpantyukhin]

It's possible to set empty values list for specified key. And then if the consumer of the MultiDict tries to get item by key he gets IndexError: list index out of range. I believe this is not good because the consumer see exception from the internal
implementation which uses lists and doesn't show exactly consumer did wrong. Also I think that actually MultiDict should not allow to set empty lists or remove specified keys from the instance when empty lists are passed.

[untitaker]

That makes sense. I'm leaning a bit more towards skipping empty lists in init

[davidism]

I like that idea more. In the case of a multi dict, setting an empty list for the key seems equivalent to not setting the key at all.

[alexpantyukhin]

@davidism Do you mean remove key (if it already exists) if empty list was passed, right?

If yes then the suggestion:
- skip keys with empty lists in the __init__ method.
- remove key if empty list was passed in the __setitem__ method
- the same for the setlist method

The question is only about setlistdefault. it could be returned and extended later, and there is no way to make user populate the list before getitem.

[untitaker]

skip keys with empty lists in the __init__ method.

👍

remove key if empty list was passed in the __setitem__ method

Why? __setitem__ always sets a single-value list with the given object.

the same for the setlist method

That makes sense.

I don't understand why setlistdefault needs any change either.

[untitaker]

To be clear, to me this is more about not letting MultiDict come in an invalid state where __getitem__ would crash like you described, not a general API revamp.

[alexpantyukhin]

@untitaker Sorry about the second point I was inattentive. Actually yes. In this case it will have at least one value anyway

[alexpantyukhin]

If pass empty list to default_list into the setlistdefault method it stores it. When you try to __getitem__ for this key later you will get the IndexError exception.

d = MultiDict([])
d.setlistdefault('a', [])
print d['a']

But it could be already used like that:

d = MultiDict([])
lst = d.setlistdefault('a', [])
lst.extend([2, 3])
print d['a']

In this case I just want show that setlistdefault could be extended later and I can't see now how to not allow to get invalid state for the Multidict in this case.

[untitaker]

Ah, I understand. I agree... in that case I guess the modification should be done in __getitem__ rather than __init__