Support reading multipart data with `\n` (`LF`) lines #3492

kxepal · 2019-01-06T22:59:46Z

What do these changes do?

While RFC clearly says about CRLF newlines, there quite a lot of
implementations which uses just LF. Even Python's stdlib produces
multiparts with \n newlines by default for compatibility reasons.

We wouldn't change how we produce multipart content - here we follow
RFC. However, we can detect \n lines quite easily which makes their
support quite cheap.

Are there changes in behavior for the user?

No notable, except now they can read multipart data with "popular" newlines.

Related issue number

#2302

codecov-io · 2019-01-06T23:52:30Z

Codecov Report

Merging #3492 into master will decrease coverage by 0.02%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #3492      +/-   ##
==========================================
- Coverage   97.95%   97.93%   -0.03%     
==========================================
  Files          44       44              
  Lines        8554     8561       +7     
  Branches     1377     1378       +1     
==========================================
+ Hits         8379     8384       +5     
- Misses         71       72       +1     
- Partials      104      105       +1

Impacted Files	Coverage Δ
aiohttp/multipart.py	`96.14% <100%> (+0.04%)`	⬆️
aiohttp/web_fileresponse.py	`96.55% <0%> (-1.15%)`	⬇️

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 3a0c7bf...e9c8c33. Read the comment docs.

asvetlov · 2019-01-08T00:07:43Z

Thanks for the PR!
Does newline style autodetection make sense?

kxepal · 2019-01-08T00:24:10Z

Honestly, I'm still not sure, but if it solves user problems and while it's quite cheap (let's not take in account tests...) in implementation it feels fine. So far only @thehesiod reported issue about.

We can hold this change for awhile and gather more user feedback to make sure we have strong reasons to relax RFC implemenation on reader part. But it feels for me it will be long time to wait.

thehesiod

~~This does not fix StreamReader.readline, called via MultipartReader.next~~

thehesiod · 2019-01-08T00:27:01Z

oh, I may have tested this incorrectly, checking

kxepal · 2019-01-08T00:27:40Z

@thehesiod Hey! (:
Could you provide some code as an example for test case? Because we already have tests for next and readline and both are fine with both newlines.

thehesiod · 2019-01-08T01:29:55Z

ya I was using the wrong version, sorry, and now and now getting issues starting up multiple threaded servers after conforming to the new APIs. How can I dismiss my review? I'm still trying to get my branch working with these changes, will report back when I have findings.

kxepal · 2019-01-08T01:32:57Z

IIRC, on merge block right below the comments there should be some button about review cancellation. As backup plan, I can just change the latest commit hash via rebase over master or just ask @asvetlov for sudo git merge. In anyway, no worries about (:

thehesiod · 2019-01-08T02:04:31Z

ok so, after a few tries it works, will have to look into it more later, seems like a multithreading issue with aiohttp (one loop and server per thread with new release), but for now:

I was calling web.Request.multipart(), however there's no param to set for the newline, so I now have to do: MultipartReader(req.headers, req.content, newline=b'\n'), should we add a param to this method?
for automatic detection, this would be really nice because otherwise your server can only accept one type or the other, but not both. Similar for reading multipart, somehow you'd have to detect/know what the newlines are otherwise. I'd prefer having an auto-detect mechanism.

thanks guys! sorry about that little disruption, got my virtualenvs confused :)

thehesiod · 2019-01-08T02:07:03Z

btw I don't see anything about cancelling my review there, perhaps asvetlov or you can dismiss my review

kxepal · 2019-01-08T02:09:12Z

The initial idea is about to autodetect on MultipartReader side, while BodyPartReader have to be informed about what kind of newline to expect. So it should work in seconds case. But since the former one is deep in implementation details, everything should work for you without worry about what kind of newline is there.

kxepal · 2019-01-08T02:09:17Z

Hm...I should make a test case for this. Hold on.

thehesiod · 2019-01-08T02:16:38Z

sorry not following, from what I understand of this PR, you need to state upfront to your multipart reader what the newline is....which means if I write a server, two things are true:

It needs to know upfront what the multipart newline format is
It can only accept or format or another

These are two big negatives. In my case it's ok, because I know upfront what it is, and I don't need to support mixed newline requests. However in general this is not true if you have a generic server, you can't control what format clients will send their newlines.

kxepal · 2019-01-08T03:06:57Z

No, you don't need to care about what newline is. It handles by this part of changes for you. In other words: first boundary dictates newline format fot the rest ones.

thehesiod · 2019-01-08T03:11:13Z

very cool! 🙇

asvetlov · 2019-01-08T09:02:46Z

Sorry, I don't follow the design.

Looks like newline argument is accepted here: https://github.com/aio-libs/aiohttp/pull/3492/files#diff-235eeadd1960735e1191879aa500b7b7R538
but overridden later here: https://github.com/aio-libs/aiohttp/pull/3492/files#diff-235eeadd1960735e1191879aa500b7b7R651

Did I miss something?

kxepal · 2019-01-08T10:51:56Z

@asvetlov
Multipart is recursive format. Once we detect newline at the very first boundary we have to pass it around for nested readers. So we have to have it as an argument and have to rewrite it during parsing, but only once.
I just noticed that current implementation allows to have mix of different newlines - that's need to forbid for good.

asvetlov · 2019-01-13T15:50:03Z

Is it ready to merge?

kxepal · 2019-01-13T15:55:09Z

Sorry, not yet. Will be able to finish this a bit later today.

While RFC clearly says about `CRLF` newlines, there quite a lot of implementations which uses just `LF`. Even Python's stdlib produces multiparts with `\n` newlines by default for compatibility reasons. We wouldn't change how we produce multipart content - here we follow RFC. However, we can detect `\n` lines quite easily which makes their support quite cheap.

kxepal · 2019-01-14T18:55:26Z

@asvetlov
Ok, I think it's ready. I was tries to catch the mixed newlines case when CRLF and LF may vary, but it seems to be not issue for us. PR rebased. If everythink ok for you and @thehesiod let's merge it.

Just for case. That's a strange case, but it seems we pass it.

aiohttp/multipart.py

This argument is not designed to be defined by users. Depending on parsing multipart newline format it will be chosen automatically and due to recursive native of multipart format it have to be passed around for nested readers.

asvetlov · 2019-01-15T07:01:28Z

Thanks!

thehesiod · 2019-01-15T10:08:54Z

Nice work, thank you!!

kxepal · 2019-01-15T10:34:41Z

🍻

lock · 2020-01-15T11:29:05Z

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.

webknjaz · 2022-12-19T15:12:10Z

CHANGES/2302.feature

@@ -0,0 +1 @@
+Support reading multipart data with `\n` (`LF`) lines


Hm.. we need to watch for incorrect single quotes in RST...

kxepal requested a review from asvetlov as a code owner January 6, 2019 22:59

kxepal requested a review from thehesiod January 6, 2019 23:01

kxepal force-pushed the 2302-multipart-crlf branch from e751ef7 to e9c8c33 Compare January 6, 2019 23:06

thehesiod suggested changes Jan 8, 2019

View reviewed changes

kxepal force-pushed the 2302-multipart-crlf branch from e9c8c33 to d05a640 Compare January 14, 2019 18:53

kxepal force-pushed the 2302-multipart-crlf branch from 4308069 to e66e379 Compare January 14, 2019 18:58

Add test about mixed newlines

e66e379

Just for case. That's a strange case, but it seems we pass it.

kxepal commented Jan 14, 2019

View reviewed changes

aiohttp/multipart.py Outdated Show resolved Hide resolved

asvetlov approved these changes Jan 14, 2019

View reviewed changes

asvetlov added 2 commits January 14, 2019 23:33

Update multipart.py

e2b45ba

Update multipart.py

5f52ae1

asvetlov merged commit fcedc66 into aio-libs:master Jan 15, 2019

lock bot added the outdated label Jan 15, 2020

lock bot locked as resolved and limited conversation to collaborators Jan 15, 2020

psf-chronographer bot added the bot:chronographer:provided There is a change note present in this PR label Jan 15, 2020

webknjaz reviewed Dec 19, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support reading multipart data with `\n` (`LF`) lines #3492

Support reading multipart data with `\n` (`LF`) lines #3492

kxepal commented Jan 6, 2019

codecov-io commented Jan 6, 2019 •

edited

Loading

asvetlov commented Jan 8, 2019

kxepal commented Jan 8, 2019

thehesiod left a comment •

edited

Loading

thehesiod commented Jan 8, 2019

kxepal commented Jan 8, 2019

thehesiod commented Jan 8, 2019

kxepal commented Jan 8, 2019

thehesiod commented Jan 8, 2019 •

edited

Loading

thehesiod commented Jan 8, 2019

kxepal commented Jan 8, 2019

kxepal commented Jan 8, 2019

thehesiod commented Jan 8, 2019

kxepal commented Jan 8, 2019

thehesiod commented Jan 8, 2019

asvetlov commented Jan 8, 2019

kxepal commented Jan 8, 2019

asvetlov commented Jan 13, 2019

kxepal commented Jan 13, 2019

kxepal commented Jan 14, 2019

asvetlov commented Jan 15, 2019

thehesiod commented Jan 15, 2019

kxepal commented Jan 15, 2019

lock bot commented Jan 15, 2020

webknjaz Dec 19, 2022

		@@ -0,0 +1 @@
		Support reading multipart data with `\n` (`LF`) lines

Support reading multipart data with \n (LF) lines #3492

Support reading multipart data with \n (LF) lines #3492

Conversation

kxepal commented Jan 6, 2019

What do these changes do?

Are there changes in behavior for the user?

Related issue number

codecov-io commented Jan 6, 2019 • edited Loading

Codecov Report

asvetlov commented Jan 8, 2019

kxepal commented Jan 8, 2019

thehesiod left a comment • edited Loading

Choose a reason for hiding this comment

thehesiod commented Jan 8, 2019

kxepal commented Jan 8, 2019

thehesiod commented Jan 8, 2019

kxepal commented Jan 8, 2019

thehesiod commented Jan 8, 2019 • edited Loading

thehesiod commented Jan 8, 2019

kxepal commented Jan 8, 2019

kxepal commented Jan 8, 2019

thehesiod commented Jan 8, 2019

kxepal commented Jan 8, 2019

thehesiod commented Jan 8, 2019

asvetlov commented Jan 8, 2019

kxepal commented Jan 8, 2019

asvetlov commented Jan 13, 2019

kxepal commented Jan 13, 2019

kxepal commented Jan 14, 2019

asvetlov commented Jan 15, 2019

thehesiod commented Jan 15, 2019

kxepal commented Jan 15, 2019

lock bot commented Jan 15, 2020

webknjaz Dec 19, 2022

Choose a reason for hiding this comment

Support reading multipart data with `\n` (`LF`) lines #3492

Support reading multipart data with `\n` (`LF`) lines #3492

codecov-io commented Jan 6, 2019 •

edited

Loading

thehesiod left a comment •

edited

Loading

thehesiod commented Jan 8, 2019 •

edited

Loading