Opened 2 years ago

Closed 22 months ago

Last modified 22 months 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, 2 years ago

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 by Carlton Gibson, 2 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, 2 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, 2 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, 2 years ago

Owner: changed from nobody to Mehrdad Moradizadeh
Status: newassigned

comment:6 by Carlton Gibson, 2 years ago

Has patch: set

comment:7 by Mehrdad Moradizadeh, 2 years ago

I have made a PR this issue.

Version 0, edited 2 years ago by Mehrdad Moradizadeh (next)

comment:8 by Mariusz Felisiak, 2 years ago

Patch needs improvement: set

comment:9 by Mariusz Felisiak <felisiak.mariusz@…>, 23 months 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@…>, 23 months 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@…>, 23 months ago

In 49b470b9:

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

comment:12 by Mariusz Felisiak, 22 months ago

comment:13 by Mariusz Felisiak <felisiak.mariusz@…>, 22 months ago

In d6e0c7c3:

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

comment:14 by Mariusz Felisiak, 22 months ago

Patch needs improvement: unset
Triage Stage: AcceptedReady for checkin

comment:15 by Mariusz Felisiak <felisiak.mariusz@…>, 22 months 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, 22 months ago

Resolution: fixed
Status: assignedclosed

comment:17 by GitHub <noreply@…>, 22 months 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