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

Refs #33697 -- Used django.utils.http.parse_header_parameters() for parsing boundary streams. #15797

Conversation

mehrdadmoradii
Copy link
Contributor

@felixxm Made parse_boundary_stream in django.http.multipartparser use django.utils.http.parse_header_parameters(). Also, changing django.utils.http.parse_header_parameters in order to return the key in lowercase; also, dealing with language Lang/encoding embedded in the value.

Copy link
Member

@felixxm felixxm left a comment

Choose a reason for hiding this comment

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

@mehrdadmoradii Thanks 👍

django/http/multipartparser.py Outdated Show resolved Hide resolved
django/http/multipartparser.py Outdated Show resolved Hide resolved
@mehrdadmoradii
Copy link
Contributor Author

@felixxm Currently, there is no more usage for multipartparser.parse_header. I believe it is safe to get rid of that function. Shall I try to do so?

@felixxm
Copy link
Member

felixxm commented Jun 27, 2022

@felixxm Currently, there is no more usage for multipartparser.parse_header. I believe it is safe to get rid of that function. Shall I try to do so?

Please do 👍

@mehrdadmoradii
Copy link
Contributor Author

@felixxm Currently, there is no more usage for multipartparser.parse_header. I believe it is safe to get rid of that function. Shall I try to do so?

Please do 👍

All done!!

Thanks a lot for your supervision.

…arsing boundary streams.

This also removes unused parse_header() and _parse_header_params()
helpers in django.http.multipartparser.
@felixxm felixxm changed the title Refs #33697 -- Made parse_boundary_stream use django.utils.http.parse_header_parameters() for parsing Content-Type header. Refs #33697 -- Used django.utils.http.parse_header_parameters() for parsing boundary streams. Jun 28, 2022
Copy link
Member

@felixxm felixxm left a comment

Choose a reason for hiding this comment

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

@mehrdadmoradii Thanks 👍

Comment on lines 514 to 518
def test_parse_header_with_double_quotes_and_semicolon(self):
self.assertEqual(
parse_header_parameters('form-data; name="files"; filename="fo\\"o;bar"'),
("form-data", {"name": "files", "filename": 'fo"o;bar'}),
)
Copy link
Member

Choose a reason for hiding this comment

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

This is already covered by ParseHeaderParameterTests.test_basic().

@felixxm felixxm force-pushed the switch-to-parse_header_parameters-in-parse_boundary_stream branch from 0df41d7 to d4d5427 Compare June 28, 2022 07:44
@felixxm felixxm merged commit d4d5427 into django:main Jun 28, 2022
@Scotchester
Copy link

I'm not sure if this is Django's problem, per se, but I wanted to note that django-rest-framework relies on the removed parse_header function in four of its files, so this is causing some CI breakage for Wagtail's testing against django:main. May at least warrant a release note?

@felixxm
Copy link
Member

felixxm commented Jun 28, 2022

I'm not sure if this is Django's problem, per se, but I wanted to note that django-rest-framework relies on the removed parse_header function in four of its files, so this is causing some CI breakage for Wagtail's testing against django:main. May at least warrant a release note?

@Scotchester Thanks for letting us know 👍 This is a private API, but we can add a release note, see #15804.

abhiabhi94 added a commit to abhiabhi94/Comment that referenced this pull request Jul 24, 2022
- there are sometimes breaking changes introducted in django's
  main branch, that other dependencies take time to handle.
  For example: our CI has been broken for almost a month now,
  due to the breaking change in django/django#15797.

  Hence, ignoring the results of the django main branch for now,
  to allow all other patches to be merged.
abhiabhi94 added a commit to abhiabhi94/Comment that referenced this pull request Jul 24, 2022
- there are sometimes breaking changes introducted in django's
  main branch, that other dependencies take time to handle.
  For example: our CI has been broken for almost a month now,
  due to the breaking change in django/django#15797.

  Hence, ignoring the results of the django main branch for now,
  to allow all other patches to be merged.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants