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

S3 Client send Throws/Rejects on 304 #2635

Closed
ambroselittle opened this issue Aug 2, 2021 · 3 comments
Closed

S3 Client send Throws/Rejects on 304 #2635

ambroselittle opened this issue Aug 2, 2021 · 3 comments
Labels
closed-for-staleness feature-request New feature or enhancement. May require GitHub community feedback. p3 This is a minor priority issue

Comments

@ambroselittle
Copy link

Describe the bug

S3 JS client throws when IfNoneMatch causes a 304 response.

Your environment

SDK version number

@aws-sdk/[email protected]

Is the issue in the browser/Node.js/ReactNative?

Node.js

Details of the browser/Node.js/ReactNative version

Paste output of npx envinfo --browsers or node -v or react-native -v
v14.17.3

Steps to reproduce

Pared down to the essentials. (Will not run as-is, but you can supply the vars, of course.)

import { S3, GetObjectCommand, GetObjectCommandInput } from '@aws-sdk/client-s3'

const s3 = new S3({
    region: region,
});


const commandOptions = { Bucket: bucket, Key: s3FilePath, } as GetObjectCommandInput;
if (etag) {
    commandOptions.IfNoneMatch = etag;
}

const obj = await s3.send(new GetObjectCommand(commandOptions));

Observed behavior

That last line throws:

Error: 
    at deserializeAws_restXmlGetObjectCommandError (/Volumes/Dev/Repos/some-app/node_modules/@aws-sdk/client-s3/protocols/Aws_restXml.ts:7251:39)
    at processTicksAndRejections (internal/process/task_queues.js:95:5)
    at /Volumes/Dev/Repos/some-app/node_modules/@aws-sdk/middleware-serde/src/deserializerMiddleware.ts:20:15
    at /Volumes/Dev/Repos/some-app/node_modules/@aws-sdk/middleware-signing/src/middleware.ts:26:22
    at StandardRetryStrategy.retry (/Volumes/Dev/Repos/some-app/node_modules/@aws-sdk/middleware-retry/src/StandardRetryStrategy.ts:83:38)
    at /Volumes/Dev/Repos/some-app/node_modules/@aws-sdk/middleware-logger/src/loggerMiddleware.ts:22:22
    at getObject (/Volumes/Dev/Repos/some-app/src/stuff/helpers/s3/s3CodeClient.ts:128:19)
    at loadFile (/Volumes/Dev/Repos/some-app/src/stuff/helpers/s3/s3CodeClient.ts:156:107)
    at realDownload (/Volumes/Dev/Repos/some-app/src/stuff/helpers/s3/s3CodeClient.spec.ts:193:5)
    at Object.<anonymous> (/Volumes/Dev/Repos/some-app/src/stuff/helpers/s3/s3CodeClient.spec.ts:185:5) {
  '$fault': 'client',
  '$metadata': {
    httpStatusCode: 304,
    requestId: undefined,
    extendedRequestId: '[the-id-was-here]',
    cfId: undefined,
    attempts: 1,
    totalRetryDelay: 0
  }
}

Expected behavior

I do not consider 304 to be an exception. When I supply IfNoneMatch, I am explicitly anticipating that I may receive a 304 and have code to handle it as normal, non-exceptional flow.

I am (currently) catching and checking the metadata on the thrown Error, but I expect to check the status code in the GetObjectCommandOutput metadata (obj.$metadata.httpStatusCode) without an exception in this case.

Additional context

I am partly filing this because it is not at all clear that this is the intended behavior. It is certainly not what I expeected, and despite searching, I cannot find anything about if it is expected on your end. It was not clear what the deserializer in your deserializerMiddleware is expecting after a quick look.

I would suggest documenting here the cases where it will throw. For example, if any non-200 is considered to warrant throwing/rejecting, then mention that. However, as I said, I don't see handling this case to be exceptional. I'd prefer to avoid the overhead of error handling for it.

@ambroselittle ambroselittle added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Aug 2, 2021
@thebengeu
Copy link

@AllanZhengYP made #1591 throw 3xx response as exceptions here. I understand why other 3xx redirections are thrown as exceptions, but should 304 be thrown as an exception?

@vudh1 vudh1 self-assigned this Nov 8, 2021
@vudh1 vudh1 added feature-request New feature or enhancement. May require GitHub community feedback. and removed bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Dec 6, 2021
@vudh1 vudh1 removed their assignment Feb 22, 2022
@RanVaknin RanVaknin added the p3 This is a minor priority issue label Feb 22, 2023
Copy link

Greetings! We’re closing this issue because it has been open a long time and hasn’t been updated in a while and may not be getting the attention it deserves. We encourage you to check if this is still an issue in the latest release and if you find that this is still a problem, please feel free to comment or open a new issue.

@github-actions github-actions bot added closing-soon This issue will automatically close in 4 days unless further comments are made. closed-for-staleness and removed closing-soon This issue will automatically close in 4 days unless further comments are made. labels Feb 23, 2024
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 Mar 12, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
closed-for-staleness feature-request New feature or enhancement. May require GitHub community feedback. p3 This is a minor priority issue
Projects
None yet
Development

No branches or pull requests

4 participants