Return a readily processed `FormData` instead of raising an exception

[paulefoe]

What do these changes do?

This changes the behavior of already processed data. Instead of raising an exception when trying to access already processed data -- return the data.

Are there changes in behavior for the user?

The POST redirects with FormData no longer raise exception.

Related issue number

This commit fixes #5577 issue

Checklist

• [x] I think the code is well written
• [x] Unit tests for the changes exist
• [ ] Documentation reflects the changes
• [x] 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.
• [x] Add a new news fragment into the CHANGES folder
  • name it <issue_id>.<type> for example (588.bugfix)
  • 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."

[codecov[bot]]

Codecov Report

Merging #5583 (798d90c) into master (47b8d63) will not change coverage. The diff coverage is 100.00%.

:exclamation: Current head 798d90c differs from pull request most recent head cffb54b. Consider uploading reports for the commit cffb54b to get more accurate results

@@           Coverage Diff           @@
##           master    #5583   +/-   ##
=======================================
  Coverage   97.21%   97.21%           
=======================================
  Files          41       41           
  Lines        8863     8863           
  Branches     1425     1425           
=======================================
  Hits         8616     8616           
  Misses        130      130           
  Partials      117      117           

Flag	Coverage Δ
unit	97.10% <100.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
aiohttp/formdata.py	98.92% <100.00%> (ø)

---

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 47b8d63...cffb54b. Read the comment docs.

[paulefoe]

Good points! Thank you for your review. I'll address your comment in a period over a few days if you don't mind.

I think the original issue by @ulrikjohansson asked exactly for this, at least this is the expected behavior from the issue:

When I repeat a call to ClientSession.request with the same arguments I expect to send a copy of the first request

this is going to be a breaking change for non-redirect scenarios. Is it worth it? What cases will this affect?

This a nice guard mechanism, so I think the cases which are going to be affected are where the developer unintentionally made a mistake and sent the same request twice.

the issue is complaining about HTTP 307/308 specifically and the bugfix should fix those without breaking other flows

In general, I can see how this fix could be useful not only for redirects but for any requests. Suppose you want to retry the POST request. Right now you will need to create a new FormData. Handling specifically 307/308 wouldn't probably be that hard, we can add the property _is_redirected to FormData and either raise an exception or return processed data.

On the other hand, retrying POST requests might not seem like a good idea, so maybe handling just redirects here is better.

I'll address the third point separately when I will dig a little bit through the links you've shared, implementation in other libraries and standards. This might take some time, but I think I will come back in 2-3 days at most

[paulefoe]

Okay, so about 307

If the 307 status code is received in response to a request other than GET or HEAD, the user agent MUST NOT automatically redirect the request unless it can be confirmed by the user, since this might change the conditions under which the request was issued. https://www.w3.org/Protocols/rfc2616/rfc2616-sec10.html#sec10.3.8

However, on MDN:

The method and the body of the original request are reused to perform the redirected request. The only difference between 307 and 302 is that 307 guarantees that the method and the body will not be changed when the redirected request is made https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/307 same in spec https://tools.ietf.org/html/rfc7231#section-6.4.7

Although this is not really contradicting w3, w3 just says that they shouldn't be performed automatically, meaning that the user should confirm resending POST request.

For 308 everyone seems to agree.

The request method and the body will not be altered https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/308

The user agent MAY use the Location field value for automatic redirection https://tools.ietf.org/html/rfc7538#section-3

So my guess is that we should implement correct form data redirects definitely for 308 and maybe for 307 as well.

I'll do library comparison tomorrow

[webknjaz]

Although this is not really contradicting w3, w3 just says that they shouldn't be performed automatically, meaning that the user should confirm resending POST request.

There is no conflict here: RFC 7231 obsoletes RFC 2616, I only mentioned the link to the latter for the historic context mostly (just look at the header of https://tools.ietf.org/html/rfc2616 and you'll see). MDN references the newer standard and, as such, it does not contradict what the relevant RFC states. Also, often newer RFCs refer to things defined in earlier ones so it's useful to have both open, just in case.

[paulefoe]

There is no conflict here: RFC 7231 obsoletes RFC 2616, I only mentioned the link to the latter for the historic context mostly (just look at the header of https://tools.ietf.org/html/rfc2616 and you'll see). MDN references the newer standard and, as such, it does not contradict what the relevant RFC states. Also, often newer RFCs refer to things defined in earlier ones so it's useful to have both open, just in case.

Ah, I see. I'm kinda new to reading these rfc docs, but that seems really interesting, good to know. Thanks.

With regards to the libraries: Requests: on 308 would post the same data to the redirected URL on 307 would post the same data to the redirected URL on 301 would issue a get request to the redirected URL

http.client quite expectedly doesn't do any redirects.

httpx: on 308 would post the same data to the redirected URL on 307 would post the same data to the redirected URL on 301 would issue a get request to the redirected URL

axios quite unexpectedly doesn't do any redirects on 301, 307, 308.

[ulrikjohansson]

I'll chime in here as I was explicitly mentioned earlier in the thread. For our use case (automatically retrying requests if they fail) this change would not have any adverse effects. Like @paulefoe noted, if this change makes it possible to re-send a POST request without having to recreate the FormData (with a deep copy for example), we could just remove that workaround code.

[webknjaz]

@paulefoe all of those are libraries. What do web browsers do? FWIW we should probably ensure to align with httpx and requests. For this, could you analyze what tests are present and try to cover all the cases so that it's explicit what we support and what we don't?

@ulrikjohansson do you know how other async libs handle resending the data? @paulefoe it'd be interesting to have this in the comparison too.

[paulefoe]

Yeah, sure, here's what browsers do. Firefox: on 301 would issue a get request to the redirected URL on 307 would post the same data to the redirected URL on 308 would post the same data to the redirected URL

Chromium: on 301 would issue a get request to the redirected URL on 307 would post the same data to the redirected URL on 308 would post the same data to the redirected URL

I've added 2 test cases that fail on master here

aiohttp actually already has test cases for 308 and 307 here, but they don't take into account the form data.

I could probably write a few more test cases tomorrow, that could be missing.

[paulefoe]

One thing also to consider, is that issue that @ulrikjohansson had in my understanding is not really ok. The other libraries don't have such issues, for example, neither httpx, not requests, doesn't have a concept as a FormData exposed to the end-user, they just accept dictionaries. This might be less efficient, but you can send them as many times as you want.

That is just my 5 cents, maybe it doesn't have that much application in the real world.

[webknjaz]

Based on our conversation, we need to have a (negative?) test where one would create a FormData instance, send it once, add a field or two to it, send it again.

[inyutin]

Is this PR still in development stage strictly because of lack of tests? @paulefoe are you planning to finish this PR ? Can I help somehow ?

[brewbytes-dev]

Is this work on hold and any chance to proceed?

[Dreamsorcerer]

Given the lack of response, I'd suggest just forking from that branch and submitting another PR. We'll then close this one.