Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Issue a PUT request after a GET request responding with 202 and a Location header #2099

Merged
merged 7 commits into from
Aug 25, 2020

Conversation

aslakhellesoy
Copy link
Contributor

The https://messages.cucumber.io service responds with 413 Payload Too Large for message files larger than 6 Mb. This was discovered during a chat with one of our users.

We've adopted the solution outlined in Option 2 in this blog post. A GET https://messages.cucumber.io/api/reports request will now respond with 202 Accepted and a Location header pointing to S3.

This is not a proper redirect, so we're handling this in the code by issuing a new PUT request if the status is 202 and the Location header is present.

This change preserves backwards compatibility for all HTTP verbs except GET. However, this change in behaviour will only manifest itself for users issuing GET request to servers expecting a body in the request, which is legal, but not useful. We don't expect anyone to be affected.

For this reason we consider this change to be an addition, which should trigger a minor release.

The same change has been applied to Ruby.

@coveralls
Copy link

coveralls commented Aug 21, 2020

Coverage Status

Coverage decreased (-0.0004%) to 86.181% when pulling 7bfddc0 on get-put-redirect into 402ce89 on main.

@cbliard cbliard merged commit 8477e65 into main Aug 25, 2020
@cbliard cbliard deleted the get-put-redirect branch August 25, 2020 08:43
@mpkorstanje
Copy link
Contributor

For future consideration, it might be worth removing the UrlOutputstream from all formatters except the publish formatter. It's becoming quite protocol specific.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants