Preserve MultipartWriter parts headers on write#3475
Preserve MultipartWriter parts headers on write#3475kxepal merged 2 commits intoaio-libs:masterfrom kxepal:3035-respect-payload-headers
Conversation
|
@kxepal please fix |
|
Should be fixed now. Waiting for CI resolution. |
Codecov Report
@@ Coverage Diff @@
## master #3475 +/- ##
=========================================
- Coverage 97.93% 97.9% -0.03%
=========================================
Files 44 44
Lines 8559 8564 +5
Branches 1383 1383
=========================================
+ Hits 8382 8385 +3
- Misses 74 75 +1
- Partials 103 104 +1
Continue to review full report at Codecov.
|
|
Good! |
asvetlov
left a comment
There was a problem hiding this comment.
LGTM, please merge when green
|
No worries. Actually, that failure was quite right one: I didn't to fix the I'm actually wonder why coverage failed so dramatically. Should I worry about and fix that? Probably, it's because removed useless test. May be I should return it with few meaninful assertions... |
|
Ah, too outdated local master branch probably. Rebased now. |
|
Coverage report shows a couple missed lines: https://codecov.io/gh/aio-libs/aiohttp/pull/3475/diff |
This case is actually impossible because all the payload instances will have headers defined with at least `Content-Type` definition. While, it's theoretically possible to create `Payload` without headers definition and Multipart format itself allows such parts, in multipart module we already have set of assertions which wouldn't make this possible. Since `_binary_headers` is private property with unknown fate and created just to not copy-paste headers serialization logic twice and used exactly within `multipart` module, we're safe to ignore this branch. Proper fix would be refactoring the way how headers and their fragments will get handled by `Payload` instances, but this quite a work out of scope of current bugfix and will be addressed in upcoming PR.
|
@asvetlov |
|
@kxepal sounds perfect! |
|
Just tried the latest main branch. Unfortunately, the bug still persists. Expected behaviourI want to modify the Content-Disposition field in the headers to this: Actual behaviourWhatever I try, it always reads: Example codePlease try this simple code snippet: QuickfixModify Payload.set_content_disposition .. this way the case "bug1" in the code snippet above starts working |
|
@kxepal ? |
|
@elsampsa |
|
Thanks! I'll try it tomorrow then .. |
* Preserve MultipartWriter parts headers on write This fixes #3035 * Mark case when payload has no headers as unreachable with FIXME This case is actually impossible because all the payload instances will have headers defined with at least `Content-Type` definition. While, it's theoretically possible to create `Payload` without headers definition and Multipart format itself allows such parts, in multipart module we already have set of assertions which wouldn't make this possible. Since `_binary_headers` is private property with unknown fate and created just to not copy-paste headers serialization logic twice and used exactly within `multipart` module, we're safe to ignore this branch. Proper fix would be refactoring the way how headers and their fragments will get handled by `Payload` instances, but this quite a work out of scope of current bugfix and will be addressed in upcoming PR.
|
Tested this (version 3.5.4 from pypi). Works as expected. Thanks for the great library and keep up the good work! |
|
Welcome! |
This fixes #3035