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

Add a setlist method to the Headers datastructure #1697

Merged
merged 2 commits into from Jan 12, 2020

Conversation

pgjones
Copy link
Member

@pgjones pgjones commented Jan 8, 2020

The update method added in baa7bdc is
meant to replace headers with the mapping passed in. If a MultDict or
Headers object is passed in it would replace headers with the final
iterated value, rather than all the values iterated over. This could
lead to unexpected results, therefore this corrects the functionality
to what I think is expected.

Consider,

h1 = Headers()
h1.add("X-Multi", "value")
h2 = Headers()
h2.add("X-Multi", "newValue")
h2.add("X-Multi", "alternativeValue")
h1.update(h2)

previously h1.getlist("X-Multi") would likely equal
["alternativeValue"] whereas now it equals ["newValue", "alternativeValue"] which is as you'd expect.

src/werkzeug/datastructures.py Outdated Show resolved Hide resolved
The update method added in baa7bdc is
meant to replace headers with the mapping passed in. If a MultDict or
Headers object is passed in it would replace headers with the final
iterated value, rather than all the values iterated over. This could
lead to unexpected results, therefore this corrects the functionality
to what I think is expected.

Consider,

    h1 = Headers()
    h1.add("X-Multi", "value")
    h2 = Headers()
    h2.add("X-Multi", "newValue")
    h2.add("X-Multi", "alternativeValue")
    h1.update(h2)

previously `h1.getlist("X-Multi")` would likely equal
`["alternativeValue"]` whereas now it equals `["newValue",
"alternativeValue"]` which is as you'd expect.
@davidism davidism added this to the 1.0.0 milestone Jan 12, 2020
@davidism
Copy link
Member

@davidism davidism commented Jan 12, 2020

This doesn't match the behavior of MultiDict.update, which extends rather than replaces existing keys. I guess you mentioned this in #1687 but I missed it.

@davidism
Copy link
Member

@davidism davidism commented Jan 12, 2020

Although in this case it probably makes more sense to be replacing rather than extending, it seems like the more likely use case for a collection of headers. If users want the other behavior we can implement update_extend or something later.

@pgjones
Copy link
Member Author

@pgjones pgjones commented Jan 12, 2020

I agree. (Also I think the test issues are unrelated?)

@davidism
Copy link
Member

@davidism davidism commented Jan 12, 2020

They are, that test uses time.sleep and is flaky on Mac for some reason.

@davidism
Copy link
Member

@davidism davidism commented Jan 12, 2020

Does it make sense for keyword arguments to update to be passed to setlist if possible? Unlike MultiDict, a header can't have a list for a value, so there wouldn't be a conflict.

I'm going to say no for now, since h[key] = [a, b, c] just converts the value to a string. We might want to address that later though.

I'm going to say yes since passing a dict does that and kwargs is a dict.

@davidism
Copy link
Member

@davidism davidism commented Jan 12, 2020

Headers.extend already exists and looks fairly similar to this.

I can update it to do for item in iter_multi_items(other): self.add(*item) and that should be the add equivalent to update.

setlist removes key if values is empty
setlist doesn't copy the values for iteration
update uses set instead of `__setitem__`
update passes list values in kwargs to setlist
add setlistdefault method
extend takes kwargs, supports MultiDict
add methods to ImmutableHeadersMixin
add tests
@davidism
Copy link
Member

@davidism davidism commented Jan 12, 2020

Pushed a bunch of things I caught while reviewing the related methods and MultiDict.

  • setlist removes the key if values is empty.
  • setlist doesn't copy the values (values[1:]) for iteration.
  • update uses set instead of __setitem__. I don't think index/slice behavior is expected there.
  • If a value in kwargs is a list, update passes it to setlist, extend adds each value.
  • extend supports MultiDict and kwargs, it's the add equivalent to update.
  • Add a setlistdefault method since MultiDict has it.
  • Add the new methods to ImmutableHeadersMixin so they can't be used on EnvironHeaders.
  • Add tests and changelog for all this.

@davidism davidism merged commit 1c4837a into pallets:master Jan 12, 2020
12 checks passed
@pgjones
Copy link
Member Author

@pgjones pgjones commented Jan 14, 2020

Cool, thanks.

@pgjones pgjones deleted the headers_update branch Jan 14, 2020
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 13, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants