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

fix(middleware-sdk-s3): improve error message on stream upload #3571

Merged
merged 4 commits into from
May 6, 2022

Conversation

kuhe
Copy link
Contributor

@kuhe kuhe commented Apr 27, 2022

Issue

#1920
#2348

Description

  • add a warning if the Body of a PutObject request might be a Stream of unknown length.

Testing

Unit tests added. Manual client testing in a linked workspace.

@kuhe kuhe added the pr/needs-review This PR needs a review from a Member. label Apr 27, 2022
@kuhe kuhe self-assigned this Apr 27, 2022
@kuhe kuhe requested a review from a team as a code owner April 27, 2022 18:28
Copy link
Contributor

@AllanZhengYP AllanZhengYP left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you provide the rationale why we don't fail the request but log a warning message for future reference?

@kuhe
Copy link
Contributor Author

kuhe commented Apr 29, 2022

Do you think the conditions are concrete enough to throw an Error? I wasn't sure if there is a possibility of something matching the conditions but still being acceptable in a PutObject request.

@kuhe
Copy link
Contributor Author

kuhe commented Apr 29, 2022

from discussion: check for content length only at this middleware stage. Throw error instead of only warning. Should be safe to assume the request will fail if it reaches this step without having determined the content length.

Copy link
Contributor

@AllanZhengYP AllanZhengYP left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The original server-side error is actually complaining of missing Content-Length header. So instead of validating the input type, we should validate the header.

SDK will calculate the content length injected the Content-Length header in content length middleware. In this case, the SDK can still populate the content length if the input is stream and from the file system, hense the API request can succeed.

Regarding whether we should log warning or fail-fast, I think we can keep warning instead of throw. Because in browser, browser may populate the content-length header on behalf of users in some cases: Content-Length is a forbidden header by browser. So fail-fast for customer may block some cases where SDK cannot calculate the content length but brower can.

Copy link
Contributor

@AllanZhengYP AllanZhengYP left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approve again!

@kuhe kuhe merged commit c0ed833 into aws:main May 6, 2022
@kuhe kuhe deleted the 1920-putObject-stream branch May 6, 2022 14:25
@github-actions
Copy link

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 and link to relevant comments in this thread.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 21, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
pr/needs-review This PR needs a review from a Member.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants