Content-Disposition fast access in ClientResponse by redixin · Pull Request #2455 · aio-libs/aiohttp

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

            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

+        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

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


    _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

            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

    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

+        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

        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

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

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

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



+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

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



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

Uh oh!

Conversation

redixin commented Oct 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What do these changes do?

Are there changes in behavior for the user?

Related issue number

Checklist

Uh oh!

redixin commented Oct 31, 2017

Uh oh!

asvetlov commented Oct 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fafhrd91 commented Oct 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fafhrd91 commented Oct 31, 2017

Uh oh!

asvetlov commented Oct 31, 2017

Uh oh!

fafhrd91 commented Oct 31, 2017

Uh oh!

codecov-io commented Oct 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kxepal Nov 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

asvetlov commented Nov 3, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

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