Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

part.set_content_disposition is ignored #3035

Closed
MattIPv4 opened this issue May 28, 2018 · 7 comments · Fixed by #3475
Closed

part.set_content_disposition is ignored #3035

MattIPv4 opened this issue May 28, 2018 · 7 comments · Fixed by #3475
Assignees
Labels

Comments

@MattIPv4
Copy link

MattIPv4 commented May 28, 2018

Long story short

Using part.set_content_disposition for some multipart file stuff, couldn't get it to work.
So I checked the part header, which is as I expected and then made a fake writer to check what the payload being sent actually was. The content disposition in the payload doesn't at all match what I have set in the part.

Expected behaviour

The content disposition in the payload is as set in the part.

Actual behaviour

It appears to generate a fresh content disposition.

Steps to reproduce

Demo

class Writer:

    def __init__(self):
        self.buffer = ""

    async def write(self, stuff):
        self.buffer += stuff.decode('utf-8','ignore')

    def done(self):
        print(self.buffer[:300])

# Generate multipart
with aiohttp.MultipartWriter('form-data') as mp:
    part = mp.append(open(path, 'rb'))
    part.set_content_disposition(
        'form-data',
        quote_fields=False,
        name='files[]',
        filename=os.path.split(path)[1]
    )
    # Fix aiohttp being retarded
    part._headers['Content-Disposition'] = part._headers['Content-Disposition'].split(" filename*=utf-8", 1)[0]
    print("\n", part._headers['Content-Disposition'], "\n")
    w = Writer()
    await mp.write(w)
    w.done()

Your environment

Aiohttp 3.2.1
Python 3.6
macOS 10.13.4

The issue (as far as I can tell)

https://github.com/aio-libs/aiohttp/blob/master/aiohttp/multipart.py#L754-L755
It appears there that the payload headers are saved separately and so no matter what you do to the returned part after it's appended, the headers will never actually be updated in the multipart.

Hotfix (until this issue is resolved)

Create your payload yourself, set headers etc. before you append to the multipart.
eg.

# Hotfix
from multidict import CIMultiDict
payload = aiohttp.payload.get_payload(open(path, 'rb'), headers=CIMultiDict())
payload.set_content_disposition(
    'form-data',
    quote_fields=False,
    name='files[]',
    filename=os.path.split(path)[1]
)
payload._headers['Content-Disposition'] = payload._headers['Content-Disposition'].split(" filename*=utf-8", 1)[0]

# Add part to multipart
part = mp.append(payload)
@asvetlov
Copy link
Member

asvetlov commented May 29, 2018

@kxepal ?
You remember this code better than me.

@asvetlov asvetlov reopened this May 29, 2018
@SphericalNumerics
Copy link

Any updates on the status of this bug? The hotfix given above doesn't seem to work for me---the content-disposition header is still blank.

@asvetlov
Copy link
Member

No progress yet.
Pull Request is welcome!

@elsampsa
Copy link

elsampsa commented Jan 2, 2019

I stumbled to this same bug today .. please fix

@kxepal kxepal self-assigned this Jan 2, 2019
@kxepal
Copy link
Member

kxepal commented Jan 2, 2019

Will do this today. I seems like sort of mistake happended after payloads introduction where payload headers in MultipartWriter.parts lives own life separated from Payload.headers.

@elsampsa
Copy link

elsampsa commented Jan 2, 2019

Thanks!

I started to look at this myself but its a bit intractable at first sight .. I was looking at when Payload.set_content_type gets called (printing logs) .. and it seems to work fine. But in the end, something else (what?) overwrites Content-Disposition

EDIT

When doing like this:

mpwriter = aiohttp.MultipartWriter('form-data')
...
subwriter = mpwriter.append(f)
...
subwriter.set_content_disposition('attachments', filename='5c2ca3d448cf7d16922acfdc.jpg')

The problem seems to be that when MultipartWriter.append is called, that's the only time when the headers are constructed (this takes place in MultipartWriter.append_payload)

Using set_content_disposition after that has no effect : the headers don't get updated

kxepal added a commit that referenced this issue Jan 3, 2019
* 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.
kxepal added a commit to kxepal/aiohttp that referenced this issue Jan 3, 2019
This change actually solves three separated, but heavy coupled
issues:

1. `Payload.content_type` may conflict with `Payload.headers[CONTENT_TYPE]`.

While in the end priority goes to the former one, it seems quite strange that
Payload object may have dual state about what content type it contains.

2.IOPayload respects Content-Disposition which comes with headers.

This is part of aio-libs#3035 issue.

3. ClientRequest.skip_autoheaders now filters Payload.headers as well.

This issue was eventually found due to refactoring: Payload object
may setup some autoheaders, but those will bypass skip logic.
asvetlov pushed a commit that referenced this issue Jan 3, 2019
* 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.
kxepal added a commit to kxepal/aiohttp that referenced this issue Jan 4, 2019
This change actually solves three separated, but heavy coupled
issues:

1. `Payload.content_type` may conflict with `Payload.headers[CONTENT_TYPE]`.

While in the end priority goes to the former one, it seems quite strange that
Payload object may have dual state about what content type it contains.

2.IOPayload respects Content-Disposition which comes with headers.

This is part of aio-libs#3035 issue.

3. ClientRequest.skip_autoheaders now filters Payload.headers as well.

This issue was eventually found due to refactoring: Payload object
may setup some autoheaders, but those will bypass skip logic.
kxepal added a commit to kxepal/aiohttp that referenced this issue Jan 4, 2019
This change actually solves three separated, but heavy coupled
issues:

1. `Payload.content_type` may conflict with `Payload.headers[CONTENT_TYPE]`.

While in the end priority goes to the former one, it seems quite strange that
Payload object may have dual state about what content type it contains.

2.IOPayload respects Content-Disposition which comes with headers.

This is part of aio-libs#3035 issue.

3. ClientRequest.skip_autoheaders now filters Payload.headers as well.

This issue was eventually found due to refactoring: Payload object
may setup some autoheaders, but those will bypass skip logic.
@lock
Copy link

lock bot commented Jan 3, 2020

This thread has been automatically locked since there has not been
any recent activity after it was closed. Please open a new issue for
related bugs.

If you feel like there's important points made in this discussion,
please include those exceprts into that new issue.

@lock lock bot added the outdated label Jan 3, 2020
@lock lock bot locked as resolved and limited conversation to collaborators Jan 3, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants