Content-Disposition fast access in ClientResponse #2455

redixin · 2017-10-31T17:07:08Z

Add content_disposition and content_disposition_dict properties

Partially implements #1670

What do these changes do?

Improves ClientResponse ergonomics

Are there changes in behavior for the user?

All changes are documented.

Related issue number

#1670

Checklist

I think the code is well written
Unit tests for the changes exist
Documentation reflects the changes
If you provide code modification, please add yourself to CONTRIBUTORS.txt
- The format is <Name> <Surname>.
- Please keep alphabetical order, the file is sorted by names.
Add a new news fragment into the CHANGES folder
- name it <issue_id>.<type> for example (588.bug)
- if you don't have an issue_id change it to the pr id after creating the pr
- ensure type is one of the following:
  - .feature: Signifying a new feature.
  - .bugfix: Signifying a bug fix.
  - .doc: Signifying a documentation improvement.
  - .removal: Signifying a deprecation or removal of public API.
  - .misc: A ticket has been closed, but it is not of interest to users.
- Make sure to use full sentences with correct case and punctuation, for example: "Fix issue with non-ascii contents in doctest text files."

redixin · 2017-10-31T17:08:19Z

@fafhrd91 should I use issue number or pull request number for CHANGES? I'm asking because this PR implements only first item in issue #1670

asvetlov · 2017-10-31T17:12:22Z

I dislike two properties.
From architectural perspective better to create a ContentDisposition class (derived from named tuple maybe) and return this instance.

fafhrd91 · 2017-10-31T17:13:06Z

Aiohttp has content disposition parsing function. I think cgi module doesn’t handle all cases.
Regarding changes, it is question for asvetlov

fafhrd91 · 2017-10-31T17:13:53Z

I like idea of ContentDisposition class

asvetlov · 2017-10-31T17:13:56Z

PR number is ok for the case -- it doesn't cover the whole issue.

fafhrd91 · 2017-10-31T17:15:29Z

https://github.com/aio-libs/aiohttp/blob/master/aiohttp/multipart.py#L35

codecov-io · 2017-10-31T17:27:23Z

Codecov Report

Merging #2455 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #2455      +/-   ##
==========================================
+ Coverage   97.14%   97.14%   +<.01%     
==========================================
  Files          39       39              
  Lines        8125     8136      +11     
  Branches     1419     1420       +1     
==========================================
+ Hits         7893     7904      +11     
  Misses        101      101              
  Partials      131      131

Impacted Files	Coverage Δ
aiohttp/client_reqrep.py	`97.2% <100%> (+0.06%)`	⬆️

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 c29b630...7c8f5f3. Read the comment docs.

asvetlov · 2017-11-01T14:39:32Z

aiohttp/helpers.py

@@ -737,3 +755,10 @@ def content_length(self, *, _CONTENT_LENGTH=hdrs.CONTENT_LENGTH):

        if content_length:
            return int(content_length)
+
+    @property
+    def content_disposition(self, *, header=hdrs.CONTENT_DISPOSITION):


Why the property is in HeadersMixin?
Should web.Request has the property?
I thought it is only for ClientResponse

Indeed it's response only header.

asvetlov · 2017-11-01T14:41:22Z

aiohttp/helpers.py

+        else:
+            value, params = multipart.parse_content_disposition(raw)
+            filename = multipart.content_disposition_filename(params)
+            self._content_disposition = ContentDisposition(value, params,


Convert params into types.MappingProxyType. The value should be immutable

Will do in next patchset.

kxepal · 2017-11-01T14:43:35Z

aiohttp/helpers.py

+    @property
+    def content_disposition(self, *, header=hdrs.CONTENT_DISPOSITION):
+        """The value of Content-Disposition HTTP header."""
+        if self._content_disposition == False:


kxepal · 2017-11-01T14:43:55Z

aiohttp/helpers.py

@@ -703,6 +708,7 @@ class HeadersMixin:

    _content_type = None
    _content_dict = None
+    _content_disposition = False


Quite strange default. Why False?

False if header are not parsed/cached yet
None if there was an attempt to parse, but no header found
ContentDisposition if header was found and parsed

kxepal · 2017-11-01T14:44:06Z

aiohttp/helpers.py

@@ -714,6 +720,18 @@ def _parse_content_type(self, raw):
        else:
            self._content_type, self._content_dict = cgi.parse_header(raw)

+    def _parse_content_disposition(self, _CONTENT_DISPOSITION):
+        from . import multipart


Why hidden import?

It was here due to circular import

kxepal · 2017-11-01T18:17:44Z

aiohttp/client_reqrep.py

@@ -500,6 +505,7 @@ class ClientResponse(HeadersMixin):
    raw_headers = None  # Response raw headers, a sequence of pairs

    _connection = None  # current connection
+    _content_disposition = False  # content disposition cache


I still think that's not good choice. None means no value. We tried to parse it, nothing happened. Yes, this will trigger headers lookup for each property access - but that's not a problem.

helpers.sentinel is an option

Good alternative, but None is fine as well. However, even better would be to use reify here and avoid internal attributes. Headers are immutable in anyway.

+1 for reify

asvetlov · 2017-11-03T06:27:57Z

aiohttp/client_reqrep.py

+        value, params = multipart.parse_content_disposition(raw)
+        params = MappingProxyType(params)
+        filename = multipart.content_disposition_filename(params)
+        return ContentDisposition(value, params, filename)


Convert params into immutable structure

What do you mean? params is already converted to MappingProxyType line 565

Sorry.
Nevermind

asvetlov · 2017-11-03T06:28:52Z

flake8 tool still disagrees with your code style :)

kxepal · 2017-11-06T14:51:05Z

aiohttp/client_reqrep.py

@@ -527,6 +532,7 @@ def __init__(self, method, url, *,
        self._request_info = request_info
        self._timer = timer if timer is not None else TimerNoop()
        self._auto_decompress = auto_decompress
+        self._cache = {}


This looks like redundant.

Looks like no. All classes with @reify methods have this and AttributeError is raising without this line.

Implicit magic. Worth to add notice about.

Yes, it's required.

I mean the attr is required, not a notice :)
But the notice makes no harm

Oh, ok. Last time I saw reify in acttion, no special attrs were required.

kxepal · 2017-11-06T14:54:17Z

tests/test_client_response.py

+
+    assert 'attachment' == response.content_disposition.value
+    assert 'bar' == response.content_disposition.parameters["foo"]
+    assert 'archive.tar.gz' == response.content_disposition.filename


Worth to add two more cases:

content_disposition.parameters is immutable

second access to content_disposition returns the same result if ClientResponse.headers were modified in-between (I'm surprised to see it's not read-only property).

kxepal · 2017-11-06T14:55:44Z

docs/client_reference.rst

+
+    .. attribute:: value
+
+    Value of Content-Disposition header itself, e.g. ``attachment``.


"attachment" - you want to notice that the value is a string typed.

kxepal · 2017-11-06T14:55:46Z

docs/client_reference.rst

+
+    .. attribute:: parameters
+
+    A :class:`dict` instance contains all parameters.


Actually, it's not a dict, but MapptingProxy. Difference in mutability.

We could just replace it with *mapping* for sake of simplicity.
I pretty sure not many python developers know what MappingProxy is :)

kxepal · 2017-11-06T15:00:32Z

aiohttp/client_reqrep.py

@@ -33,6 +34,10 @@
 __all__ = ('ClientRequest', 'ClientResponse', 'RequestInfo')


+ContentDisposition = collections.namedtuple(
+    'ContentDisposition', ('value', 'parameters', 'filename'))


I think value is not the best choice. The RFC defines it as a type which suites more to what the value it holds.

kxepal · 2017-11-06T15:01:18Z

aiohttp/client_reqrep.py

+        raw = self._headers.get(hdrs.CONTENT_DISPOSITION)
+        if raw is None:
+            return None
+        value, params = multipart.parse_content_disposition(raw)


It seems only two values here are valid for HTTP headers case: inline and attachment. May be worth to raise a warning about invalid type or turn the value in Enum?

According to https://tools.ietf.org/html/rfc6266#section-4.1 any token is valid

I like Enum but how the change affects multidict and existing users.
I suspect parse_content_disposition is internal API but not 100% sure.

@redixin Yes, but RFC defines more general Content-Disposition definition. Mozilla explicitly splits it into HTTP headers and Multpart headers. The second one utilize token rule completely, but here we are in HTTP response land.

Add ContentDisposition class and content_disposition property Partially implements aio-libs#1670

asvetlov · 2017-11-07T18:18:47Z

Great work!
Thank you!

lock · 2019-10-28T14:03:31Z

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].
[new issue]: https://github.com/aio-libs/aiohttp/issues/new

redixin force-pushed the content-disposition branch from be43246 to 6afa66c Compare October 31, 2017 18:36

asvetlov requested changes Nov 1, 2017

View reviewed changes

kxepal reviewed Nov 1, 2017

View reviewed changes

redixin force-pushed the content-disposition branch from 6afa66c to 41fd2c3 Compare November 1, 2017 18:03

kxepal reviewed Nov 1, 2017

View reviewed changes

redixin force-pushed the content-disposition branch from 41fd2c3 to 2ede86e Compare November 1, 2017 20:53

asvetlov requested changes Nov 3, 2017

View reviewed changes

kxepal reviewed Nov 6, 2017

View reviewed changes

redixin force-pushed the content-disposition branch 3 times, most recently from ff12184 to dec87c0 Compare November 6, 2017 15:51

Content-Disposition fast access in ClientResponse

9b6a528

Add ContentDisposition class and content_disposition property Partially implements aio-libs#1670

redixin force-pushed the content-disposition branch from dec87c0 to 9b6a528 Compare November 6, 2017 16:53

Merge branch 'master' into content-disposition

7c8f5f3

asvetlov approved these changes Nov 7, 2017

View reviewed changes

asvetlov merged commit 78d08ce into aio-libs:master Nov 7, 2017

redixin deleted the content-disposition branch November 7, 2017 23:47

lock bot added the outdated label Oct 28, 2019

lock bot locked as resolved and limited conversation to collaborators Oct 28, 2019

psf-chronographer bot added the bot:chronographer:provided There is a change note present in this PR label Oct 28, 2019


		.. attribute:: value

		Value of Content-Disposition header itself, e.g. ``attachment``.


		.. attribute:: parameters

		A :class:`dict` instance contains all parameters.

Content-Disposition fast access in ClientResponse #2455

Content-Disposition fast access in ClientResponse #2455

Conversation

redixin commented Oct 31, 2017 • edited Loading

What do these changes do?

Are there changes in behavior for the user?

Related issue number

Checklist

redixin commented Oct 31, 2017

asvetlov commented Oct 31, 2017 • edited Loading

fafhrd91 commented Oct 31, 2017 • edited Loading

fafhrd91 commented Oct 31, 2017

asvetlov commented Oct 31, 2017

fafhrd91 commented Oct 31, 2017

codecov-io commented Oct 31, 2017 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kxepal Nov 1, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asvetlov commented Nov 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asvetlov commented Nov 7, 2017

lock bot commented Oct 28, 2019

redixin commented Oct 31, 2017 •

edited

Loading

asvetlov commented Oct 31, 2017 •

edited

Loading

fafhrd91 commented Oct 31, 2017 •

edited

Loading

codecov-io commented Oct 31, 2017 •

edited

Loading

kxepal Nov 1, 2017 •

edited

Loading