Payload refactoring #1664

fafhrd91 · 2017-02-20T21:15:33Z

Payload refactoring

fafhrd91 · 2017-02-20T21:18:46Z

some goals

unify how we handle various types of payloads
unify client/server implementation
it needs to be backward compatible with existing clients

note: this is only about how we send payload

fafhrd91 · 2017-02-20T22:17:11Z

added streamer decorator for coroutine support as data provider

for usage check test_client_functional.py::test_POST_STREAM_DATA

fafhrd91 · 2017-02-20T23:25:33Z

@kxepal is this content-disposition right? inline; filename="test.txt"; filename*=utf-8''test.txt

codecov-io · 2017-02-20T23:27:04Z

Codecov Report

❗ No coverage uploaded for pull request base (master@57cb76c). Click here to learn what that means.
The diff coverage is 100%.

@@            Coverage Diff            @@
##             master    #1664   +/-   ##
=========================================
  Coverage          ?   95.19%           
=========================================
  Files             ?       34           
  Lines             ?     7237           
  Branches          ?     1262           
=========================================
  Hits              ?     6889           
  Misses            ?      227           
  Partials          ?      121

Impacted Files	Coverage Δ
aiohttp/client.py	`90.81% <100%> (ø)`

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 57cb76c...0e765a1. Read the comment docs.

fafhrd91 · 2017-02-21T05:53:18Z

@kxepal I need you to review MultipartWriter related changes specifically

kxepal

@fafhrd91 Wonderful work!

kxepal · 2017-02-21T06:57:13Z

aiohttp/abc.py

+        """Write last chunk"""
+
+    @asyncio.coroutine  # pragma: no branch
+    @asyncio.coroutine


Twice coroutite?

kxepal · 2017-02-21T06:58:37Z

aiohttp/abc.py

+        """Write chunk into stream"""
+
+    @asyncio.coroutine  # pragma: no branch
+    @abstractmethod


You can exclude abstract methods from coverage:

[report] exclude_lines = @abc.abstractmethod @abstractmethod

kxepal · 2017-02-21T06:59:30Z

aiohttp/client_reqrep.py

-            self.chunked = True
+        # FormData
+        if isinstance(body, FormData):
+            body = body(self.encoding)


Wouldn't be better to handle there any callable?

? I dont understand

I mean that what if body could be an arbitrary callable object? FormData uses that interface to be processed. We can reuse that way for everything else. Though not sure if this would be helpful.

We can add this later, right now I do not see value in this. Developer should be able to prepare spendable data before calling client. Case with custom data generation from coroutine already covered with streamer decorator

Fair enough. Thanks!

kxepal · 2017-02-21T07:00:30Z

aiohttp/client_reqrep.py

+        try:
+            body = payload.PAYLOAD_REGISTRY.get(body)
+        except payload.LookupError:
+            body = FormData(body)(self.encoding)


Why not to send data as is?

body could be dict or list or whatever. I am fine with changing FormData api

Yes, I was though to just throw an "encode me" error in this case.

first i try to convert body into Payload, then if it fails I try to use FormData
when you do FormData(body)(self.encoding) it returns bytes for url encoded or
MultipartWriter (which is Payload) or fails

Sorry, I should be more clear. I mean that just try to send data as is without implicit transformations and fail if that's not possible (dict, list, custom object that didn't implement required interface). Implicit transformation may play a bad joke.

I want to be explicit, it may be not clear if it fails on actual write

kxepal · 2017-02-21T07:08:42Z

aiohttp/multipart.py

-        return self.append(obj, headers)
+
+        *_, params = parse_mimetype(headers.get(CONTENT_TYPE))
+        charset = params.get('charset', 'utf-8')


I think this is a bit redundant and may confuse. Because you can throw here any content-type, but in the end it will be application/json without any charset and else params.

Better have explicit charset function argument or drop this idea. We're in 2017 anyway and Python`s json escapes all non ascii chars, so it doesn't really matter which encoding will be applied.

I don't care much about this, we can remove it

kxepal · 2017-02-21T07:11:42Z

aiohttp/payload.py

+        self._encoding = encoding
+        self._filename = filename
+        if headers is not None:
+            self._headers = CIMultiDict(headers)


Shouldn't here be a multidict proxy? Because, technically, I can update Payload.headers content type and it will cause a conflict with the content type property.

yes, maybe. this api is not settled yet.

kxepal · 2017-02-21T07:18:37Z

I'll take a second look at the evening if it wouldn't be too late (:
Massive changes, but I like the way it goes.

fafhrd91 · 2017-02-21T08:20:18Z

accidentally removed branch, now GitHub does not allow reopen PR.

@kxepal let's continue here, in any case I am mostly done with refactoring.

fafhrd91 · 2017-02-21T16:02:30Z

@asvetlov comments?

lock · 2019-10-29T04:03:57Z

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.

fafhrd91 mentioned this pull request Feb 20, 2017

Refactor data stream uploading for client api #1172

Closed

fafhrd91 force-pushed the payload-refactoring branch from 94a233d to c2df3cc Compare February 20, 2017 23:35

fafhrd91 force-pushed the payload-refactoring branch from 812ec16 to 2c01b0e Compare February 21, 2017 06:38

fafhrd91 force-pushed the master branch from cd81a5b to 57cb76c Compare February 21, 2017 06:41

fafhrd91 added 5 commits February 20, 2017 22:43

introduce Payload class

d69838a

support coroutine as data provider

22462a4

enable StreamReader support

ca1e650

allow to use payload objects on server side

7e9a381

refactor MultipartWriter to use Payload

0e765a1

fafhrd91 force-pushed the payload-refactoring branch from 2c01b0e to 0e765a1 Compare February 21, 2017 06:46

kxepal reviewed Feb 21, 2017

View reviewed changes

fafhrd91 mentioned this pull request Feb 21, 2017

Request.post() stores data in temp files #1469

Closed

fafhrd91 closed this Feb 21, 2017

fafhrd91 deleted the payload-refactoring branch February 21, 2017 08:13

lock bot added the outdated label Oct 29, 2019

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Payload refactoring #1664

Payload refactoring #1664

fafhrd91 commented Feb 20, 2017

fafhrd91 commented Feb 20, 2017 •

edited

Loading

fafhrd91 commented Feb 20, 2017

fafhrd91 commented Feb 20, 2017

codecov-io commented Feb 20, 2017 •

edited

Loading

fafhrd91 commented Feb 21, 2017

kxepal left a comment

kxepal Feb 21, 2017

fafhrd91 Feb 21, 2017

kxepal Feb 21, 2017

fafhrd91 Feb 21, 2017

kxepal Feb 21, 2017

fafhrd91 Feb 21, 2017

kxepal Feb 21, 2017

fafhrd91 Feb 21, 2017

kxepal Feb 21, 2017

kxepal Feb 21, 2017

fafhrd91 Feb 21, 2017

kxepal Feb 21, 2017

fafhrd91 Feb 21, 2017

kxepal Feb 21, 2017

fafhrd91 Feb 21, 2017

kxepal Feb 21, 2017

kxepal Feb 21, 2017

fafhrd91 Feb 21, 2017

kxepal Feb 21, 2017

kxepal Feb 21, 2017

fafhrd91 Feb 21, 2017

kxepal commented Feb 21, 2017

fafhrd91 commented Feb 21, 2017

fafhrd91 commented Feb 21, 2017 •

edited

Loading

lock bot commented Oct 29, 2019

Payload refactoring #1664

Payload refactoring #1664

Conversation

fafhrd91 commented Feb 20, 2017

fafhrd91 commented Feb 20, 2017 • edited Loading

fafhrd91 commented Feb 20, 2017

fafhrd91 commented Feb 20, 2017

codecov-io commented Feb 20, 2017 • edited Loading

Codecov Report

fafhrd91 commented Feb 21, 2017

kxepal left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kxepal commented Feb 21, 2017

fafhrd91 commented Feb 21, 2017

fafhrd91 commented Feb 21, 2017 • edited Loading

lock bot commented Oct 29, 2019

fafhrd91 commented Feb 20, 2017 •

edited

Loading

codecov-io commented Feb 20, 2017 •

edited

Loading

fafhrd91 commented Feb 21, 2017 •

edited

Loading