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.

Sign up for GitHub

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

Jump to bottom

Make sure MutableHeaders._list is actually a list #1917

Merged
merged 6 commits into from
Oct 21, 2022
Merged

Make sure MutableHeaders._list is actually a list #1917

merged 6 commits into from
Oct 21, 2022

Conversation

adriangb
Copy link
Member

Fixed #1909

alex-oleshkevich
alex-oleshkevich reviewed Oct 19, 2022
View reviewed changes
starlette/datastructures.py Outdated
scope: typing.Optional[typing.Mapping[str, typing.Any]] = None,
) -> None:
super().__init__(headers, raw, scope)
self._list = list(self._list)
Copy link
Member

@alex-oleshkevich alex-oleshkevich Oct 19, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think doing this (line 595) in the parent class is good enough.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also a test case covering the issue is wanted.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Feel free to update the branch, I’m on my phone and AFK the rest of the day

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same thing. Let's postpone it till morning.

@adriangb
Copy link
Member Author

Looks like the test revealed a very real issue: we assume that MutableHeaders updates the headers but if we cast it to a list were making a copy. So I had to assign this new list back to scope. Guess that makes sense. What do you think @alex-oleshkevich? Thanks for pushing for a test btw, it was the right call here.

@adriangb adriangb merged commit 9aa0db4 into master Oct 21, 2022
@adriangb adriangb deleted the adriangb-patch-1 branch October 21, 2022 12:57
@jarojasm95
Copy link

Looks like the test revealed a very real issue: we assume that MutableHeaders updates the headers but if we cast it to a list were making a copy. So I had to assign this new list back to scope. Guess that makes sense. What do you think @alex-oleshkevich? Thanks for pushing for a test btw, it was the right call here.

thank you! this was the problem I was alluding to in #1909 - this definitely fixes it. 💯

aminalaee pushed a commit that referenced this pull request Feb 13, 2023
@adriangb @aminalaee
Make sure MutableHeaders._list is actually a list (#1917)
cee4284
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alex-oleshkevich alex-oleshkevich alex-oleshkevich approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@adriangb @jarojasm95 @alex-oleshkevich