Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Fix Content-Disposition in media repository #4176

Merged
merged 12 commits into from
Nov 15, 2018

Conversation

hawkowl
Copy link
Contributor

@hawkowl hawkowl commented Nov 12, 2018

No description provided.

@hawkowl hawkowl changed the base branch from master to develop November 12, 2018 21:38
@hawkowl hawkowl requested a review from a team November 12, 2018 21:41
.coveragerc Outdated
@@ -0,0 +1,12 @@
[run]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry, but could you put the coverage stuff in a separate PR?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bump.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Isn't that #4180?)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, #4180 :) (If we merge that first then this squashed merge won't have it, which is why I didn't bother removing it)

synapse/rest/media/v1/_base.py Outdated Show resolved Hide resolved
synapse/rest/media/v1/_base.py Outdated Show resolved Hide resolved
synapse/rest/media/v1/_base.py Outdated Show resolved Hide resolved
synapse/rest/media/v1/_base.py Outdated Show resolved Hide resolved
tests/rest/media/v1/test_media_storage.py Outdated Show resolved Hide resolved
@richvdh
Copy link
Member

richvdh commented Nov 13, 2018

I'm assuming this is meant to fix #4160

@hawkowl hawkowl requested a review from a team November 13, 2018 20:28
@richvdh
Copy link
Member

richvdh commented Nov 15, 2018

pending merge of #4180 then

@richvdh richvdh removed the request for review from a team November 15, 2018 11:43
@hawkowl hawkowl requested a review from richvdh November 15, 2018 17:08
Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm apart from some nits about comments

if not content_disposition[0]:
return

params = {}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd find a comment which documents the type of this useful. It seems to be a map from a unicode to a bytes?

(Edit: I see that you kinda document this below, but I'd find it clearer here. Also the terms "decoded" and "unencoded" are pretty overloaded and unclear here)

upload_name_utf8 = upload_name_utf8[7:]
if PY3:
try:
# We have a filename*= section. This MUST be ASCII, and any
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Half of this comment seems to apply to PY2 as well as PY3, so could it be pulled up? Also "quoted" doesn't mean much to me, even if that's what urllib calls it. Can we call it "%-encoded" or "%-escaped" or something?

if PY3:
try:
# We have a filename*= section. This MUST be ASCII, and any
# UTF-8 bytes are quoted. Once it is decoded, we can then
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"decoded" is unclear to me. Can we say something like:

First decode the ascii bytes to a str, then we can %-decode it safely

# Incorrect UTF-8.
pass
else:
# On Python 2, we can unquote it directly, and then decode it
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# On Python 2, we can unquote it directly, and then decode it
# On Python 2, we can %-decode it directly, and then decode the utf8 bytes to a unicode

Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@hawkowl hawkowl merged commit 8b1affe into develop Nov 15, 2018
@hawkowl hawkowl deleted the hawkowl/py3-content-disposition branch November 15, 2018 21:56
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants