Opened 3 months ago

Closed 7 weeks ago

Last modified 7 weeks 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 Changed 3 months ago by Mariusz Felisiak

Triage Stage: UnreviewedAccepted

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 Changed 3 months ago by Carlton Gibson

+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 Changed 3 months ago by Kapil Bansal

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 Changed 3 months ago by Carlton Gibson

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 Changed 3 months ago by Mehrdad Moradizadeh

Owner: changed from nobody to Mehrdad Moradizadeh
Status: newassigned

comment:6 Changed 3 months ago by Carlton Gibson

Has patch: set

comment:7 Changed 3 months ago by Mehrdad Moradizadeh

I have made a PR for this issue.

Last edited 3 months ago by Mehrdad Moradizadeh (previous) (diff)

comment:8 Changed 3 months ago by Mariusz Felisiak

Patch needs improvement: set

comment:9 Changed 2 months ago by Mariusz Felisiak <felisiak.mariusz@…>

In 93cedc8:

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

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

comment:10 Changed 2 months ago by Mariusz Felisiak <felisiak.mariusz@…>

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 Changed 2 months ago by Mariusz Felisiak <felisiak.mariusz@…>

In 49b470b9:

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

comment:12 Changed 7 weeks ago by Mariusz Felisiak

comment:13 Changed 7 weeks ago by Mariusz Felisiak <felisiak.mariusz@…>

In d6e0c7c3:

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

comment:14 Changed 7 weeks ago by Mariusz Felisiak

Patch needs improvement: unset
Triage Stage: AcceptedReady for checkin

comment:15 Changed 7 weeks ago by Mariusz Felisiak <felisiak.mariusz@…>

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 Changed 7 weeks ago by Mariusz Felisiak

Resolution: fixed
Status: assignedclosed

comment:17 Changed 7 weeks ago by GitHub <noreply@…>

In 154dd1c:

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

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