Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#33697 closed Cleanup/optimization (fixed)

Cleanup duplication of HTTP header parsing in utils.http and multipart parser.

Reported by: Carlton Gibson Owned by: Mehrdad Moradizadeh
Component: Utilities Version: 4.0
Severity: Normal Keywords:
Cc: Florian Apolloner Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: yes UI/UX: no

Description

PY311 deprecated the cgi module.

We removed usage of that in ​PR 15679.

There's similar logic already in place in multipartparser.py, but header parameters are parsed slightly differently, and the return types of the helper functions used don't match exactly. (​Comment on the PR linking to the code locations.)

Nonetheless it looks like we should be able to combine the two functions to have a single parsing routine.

Change History (17)

comment:1 by Mariusz Felisiak, 4 years ago

Triage Stage: Unreviewed → Accepted

It looks that the only difference between _parseparam() and django.http.multipartparser._parse_header_params() is that it works on strings (instead of bytes) and that it contains ​https://github.com/python/cpython/commit/1ef0c0349e8fdb5415e21231cb42edbf232b742a (which is desired in both places).

comment:2 by Carlton Gibson, 4 years ago

+1 Yes. (And there's the list vs scalar thing... but that's just needing to sit and think it through clearly.) — Ideal would be to unify on a single version in utils and use in both places.

comment:3 by Kapil Bansal, 4 years ago

Hi,
I want to give this issue a try. Although I am not sure how to run and check what these functions are doing?
Would it be like creating a new django project and tests from where this function is called? Or something else?

comment:4 by Carlton Gibson, 4 years ago

Hi Kapil.

There's already some coverage, so making the change ensuring the existing tests pass would be a start.

On implementation, you may find you need to adjust tests, or add new ones. That's OK.

If we need extra tests after that, they can be suggested on review.

The ​Submitting patches guide should help you on your next steps.

Thanks!

comment:5 by Mehrdad Moradizadeh, 4 years ago

Owner: changed from nobody to Mehrdad Moradizadeh
Status: new → assigned

comment:6 by Carlton Gibson, 4 years ago

Has patch: set

comment:7 by Mehrdad Moradizadeh, 4 years ago

I have made a ​PR for this issue.

Last edited 4 years ago by Mehrdad Moradizadeh (previous) (diff)

comment:8 by Mariusz Felisiak, 4 years ago

Patch needs improvement: set

comment:9 by Mariusz Felisiak <felisiak.mariusz@…>, 4 years ago

In 93cedc8:

Refs #33697 -- Fixed multipart parsing of headers with double quotes and semicolons.

See ​https://github.com/python/cpython/commit/1ef0c0349e8fdb5415e21231cb42edbf232b742a

comment:10 by Mariusz Felisiak <felisiak.mariusz@…>, 4 years ago

In 2f974e3:

[4.1.x] Refs #33697 -- Fixed multipart parsing of headers with double quotes and semicolons.

See ​https://github.com/python/cpython/commit/1ef0c0349e8fdb5415e21231cb42edbf232b742a

Backport of 93cedc82f29076c824d476354527af1150888e4f from main

comment:11 by Mariusz Felisiak <felisiak.mariusz@…>, 4 years ago

In 49b470b9:

Refs #33697 -- Made MultiPartParser use django.utils.http.parse_header_parameters() for parsing Content-Type header.

comment:12 by Mariusz Felisiak, 4 years ago

comment:13 by Mariusz Felisiak <felisiak.mariusz@…>, 4 years ago

In d6e0c7c3:

Refs #33697 -- Made MediaType use django.utils.http.parse_header_parameters().

comment:14 by Mariusz Felisiak, 4 years ago

Patch needs improvement: unset
Triage Stage: Accepted → Ready for checkin

comment:15 by Mariusz Felisiak <felisiak.mariusz@…>, 4 years ago

In d4d54275:

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

This also removes unused parse_header() and _parse_header_params()
helpers in django.http.multipartparser.

comment:16 by Mariusz Felisiak, 4 years ago

Resolution: → fixed
Status: assigned → closed

comment:17 by GitHub <noreply@…>, 4 years ago

In 154dd1c:

Refs #33697 -- Added backward incompatibility note about removing multipartparser.parse_header().

Note: See TracTickets for help on using tickets.
Back to Top