Skip to content

Make sure MutableHeaders._list is actually a list#1917

Merged
adriangb merged 6 commits intomasterfrom
adriangb-patch-1
Oct 21, 2022
Merged

Make sure MutableHeaders._list is actually a list#1917
adriangb merged 6 commits intomasterfrom
adriangb-patch-1

Conversation

@adriangb
Copy link
Copy Markdown
Contributor

@adriangb adriangb commented Oct 19, 2022

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
Copy Markdown
Contributor

@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
Copy Markdown
Contributor

@alex-oleshkevich alex-oleshkevich Oct 19, 2022

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
Copy Markdown
Contributor Author

@adriangb adriangb Oct 19, 2022

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
Copy Markdown
Contributor

@alex-oleshkevich alex-oleshkevich Oct 19, 2022

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
Copy Markdown
Contributor Author

adriangb commented Oct 21, 2022

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
@jose-rojas0
Copy link
Copy Markdown

jose-rojas0 commented Oct 22, 2022

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

1 more reviewer

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

Reviewers whose approvals may not affect merge requirements

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 @jose-rojas0 @alex-oleshkevich