Preserve MultipartWriter parts headers on write

[kxepal]

This fixes #3035

[asvetlov]

@kxepal please fix make mypy run

[kxepal]

Should be fixed now. Waiting for CI resolution.

[codecov-io]

Codecov Report

Merging #3475 into master will decrease coverage by 0.02%.
The diff coverage is 100%.

@@            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

Impacted Files	Coverage Δ
aiohttp/multipart.py	95.47% <100%> (-0.36%)	⬇️
aiohttp/payload.py	98.61% <100%> (+0.02%)	⬆️
aiohttp/web_fileresponse.py	96.55% <0%> (+0.04%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6205cce...7c3623c. Read the comment docs.

[asvetlov]

Good!
Sorry, type annotations for multipart are quite not strict.
I had problems with understanding actual types and used Any/type: ignore somewhere.

[asvetlov]

LGTM, please merge when green

[kxepal]

No worries. Actually, that failure was quite right one: I didn't to fix the _Part annonation which continued thinks that detached headers are passed around.

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...

[kxepal]

Ah, too outdated local master branch probably. Rebased now.

[asvetlov]

Coverage report shows a couple missed lines: https://codecov.io/gh/aio-libs/aiohttp/pull/3475/diff
If you'll fix it -- would be nice!

[kxepal]

@asvetlov
Well, it's possible to fix coverage here with some, but that fix will have no links with the real life. Proper one will require quite of refactoring which will touch too many bits and unrelated with bugfix itself. For now I added this commit: 7c3623c to make CI happy and will handle this issue in another PR. Are you ok with it?

[asvetlov]

@kxepal sounds perfect!

[elsampsa]

Just tried the latest main branch. Unfortunately, the bug still persists.

Expected behaviour

I want to modify the Content-Disposition field in the headers to this:

Content-Disposition: attachments; filename="5c2ca3d448cf7d16922acfdc.jpg"

Actual behaviour

Whatever I try, it always reads:

Content-Disposition: attachment; filename="test.jpg"; filename*=utf-8''test.jpg

Example code

Please try this simple code snippet:

# https://docs.aiohttp.org/en/stable/multipart.html

# nix
import sys
import io

# async
import asyncio
import aiohttp

class debugWriter:
    
    def __init__(self):
        self.buffer=""
        
    async def write(self, stuff):
        self.buffer += stuff.decode("utf-8","ignore")
        
    def done(self):
        print(self.buffer[:200])
 

async def bug1():
    """This doesn't work because:
    
    - payload.py : Payload.set_content_disposition : gets called and it overwrites header with some default values
    
    """
    
    mpwriter = aiohttp.MultipartWriter('form-data')
    f=open("test.jpg","wb"); f.write("123".encode("utf-8")); f.close() # create a dummy file
    
    f=open("test.jpg","rb")
    subwriter = mpwriter.append(f, {
        aiohttp.hdrs.CONTENT_DISPOSITION : 'attachments; filename="5c2ca3d448cf7d16922acfdc.jpg"', # has no effect
        aiohttp.hdrs.CONTENT_TYPE : "image/whatever"
            })

    w = debugWriter()
    await mpwriter.write(w)
    w.done()
    f.close()
    

async def bug2():
    """This doesn't work because:
    
    - multipart.py : MultiparWriter.append : headers get constructed once and only when append is called.
    - .. calling set_content_disposition after that has no effect
    
    """
    mpwriter = aiohttp.MultipartWriter('form-data')
    f=open("test.jpg","wb"); f.write("123".encode("utf-8")); f.close() # create a dummy file
    
    f=open("test.jpg","rb")
    subwriter = mpwriter.append(f, {
        aiohttp.hdrs.CONTENT_TYPE : "image/whatever"
            })
    
    mpwriter.set_content_disposition('attachments', filename='5c2ca3d448cf7d16922acfdc.jpg') # has no effect
    
    w = debugWriter()
    await mpwriter.write(w)
    w.done()
    f.close()
    
    
if __name__ == '__main__':
    asyncio.get_event_loop().run_until_complete(bug1())
    asyncio.get_event_loop().run_until_complete(bug2())

Quickfix

Modify Payload.set_content_disposition

def set_content_disposition(self,
                                disptype: str,
                                quote_fields: bool=True,
                                **params: Any) -> None:
        """Sets ``Content-Disposition`` header."""
        if self._headers is None:
            self._headers = CIMultiDict()

        if (hdrs.CONTENT_DISPOSITION in self._headers):
            print("set_content_disposition : CONTENT_DISPOSITION found : ", self._headers[hdrs.CONTENT_DISPOSITION])

        else:
            print("set_content_disposition : overwriting ")
            self._headers[hdrs.CONTENT_DISPOSITION] = content_disposition_header(
                disptype, quote_fields=quote_fields, **params)
        print("set_content_disposition : now ", self._headers[hdrs.CONTENT_DISPOSITION])

.. this way the case "bug1" in the code snippet above starts working

[asvetlov]

@kxepal ?

[kxepal]

@elsampsa
Thanks! That's actually another story, but with the similar consequences. Let me see how to fix this once and for all time.

[kxepal]

@elsampsa
Addressed this in #3479 - that patch should fix the issue now, but I'll add more tests later this evening to make sure we'll not repeat the same mistakes in future.

[elsampsa]

Thanks! I'll try it tomorrow then ..

[elsampsa]

Tested this (version 3.5.4 from pypi). Works as expected.

Thanks for the great library and keep up the good work!

[asvetlov]

Welcome!