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

Missing error class for S3 Client #5630

Closed
2 tasks
garysassano opened this issue Dec 30, 2023 · 4 comments
Closed
2 tasks

Missing error class for S3 Client #5630

garysassano opened this issue Dec 30, 2023 · 4 comments
Assignees
Labels
feature-request New feature or enhancement. May require GitHub community feedback.

Comments

@garysassano
Copy link

Describe the feature

Right now I have to write some code that looks like this:

if (error instanceof NotFound || (error as Error).name === "403") {
  throw new Error(`Object does not exist (key=${key}).`);
}

Whereas I'd rather write:

if (error instanceof NotFound || error instanceof Forbidden) {
  throw new Error(`Object does not exist (key=${key}).`);
}

This is especially useful when using the HeadObjectCommand which can return 403 error if s3:ListBucket permission wasn't granted.

Use Case

Uniform error handling across the SDK by expanding the pool of error classes.

Proposed Solution

No response

Other Information

No response

Acknowledgements

  • I may be able to implement this feature request
  • This feature might incur a breaking change

SDK version used

3.449.0

Environment details (OS name and version, etc.)

Ubuntu 22.04 LTS on WSL2

@garysassano garysassano added feature-request New feature or enhancement. May require GitHub community feedback. needs-triage This issue or PR still needs to be triaged. labels Dec 30, 2023
@RanVaknin RanVaknin self-assigned this Jan 2, 2024
@RanVaknin RanVaknin removed the needs-triage This issue or PR still needs to be triaged. label Jan 2, 2024
@RanVaknin
Copy link
Contributor

Hi @garysassano ,

Thanks for reaching out. The problem you are seeing is a modeling issue with the S3 service itself, and not the SDK.

If you didn't know, all the AWS SDKs are code generated directly from the API model of each AWS service. This means, that the SDK will generate those concrete errors based on the service definitions. In this specific case, S3 only defines NotFound as a possible error thrown, as shown in the model file:

"com.amazonaws.s3#HeadBucket": {
            "type": "operation",
            "input": {
                "target": "com.amazonaws.s3#HeadBucketRequest"
            },
            "output": {
                "target": "com.amazonaws.s3#HeadBucketOutput"
            },
            "errors": [
                {
                    "target": "com.amazonaws.s3#NotFound"
                }
            ],

They have supplemented this lack of modeling with the correct documentation:

If you don’t have the s3:ListBucket permission, Amazon S3 returns an HTTP status code 403 Forbidden error.

We have multiple internal tickets open to s3 to update their modeling, but this is not on their roadmap.

Additionally, we will update our docs soon to advise customers about potential issues with using instanceof contrary to what is suggested in our docs right now. Because this is a common pattern of errors not being modeled correctly, customers are not able to reliably use instanceof in which case using Error.name should be find and safe to use.

If you feel inclined you can create a support ticket through the AWS developer console and ask to be rerouted to the S3 team. As it stands, the SDK team cannot take action on this because of the reason I highlighted above.

Thanks again for taking the time and reaching out.

All the best,
Ran~

@RanVaknin RanVaknin closed this as not planned Won't fix, can't repro, duplicate, stale Jan 2, 2024
@garysassano
Copy link
Author

Additionally, we will update our docs soon to advise customers about potential issues with using instanceof contrary to what is suggested in our docs right now.

Are you suggesting that you will advise against using the concrete error classes that were introduced in SDK for JavaScript v3? I was under the impression that this was the recommended approach for error handling.

@RanVaknin
Copy link
Contributor

Hi @garysassano,

No, let me clarify. I advise against solely relying on service defined errors as often times the services will return errors that they do not model. instanceof can and should be used if the service is returning the same exception as it defines, but as you are seeing first hand that is not always the case. This could be safely supplemented with the use of Error.name.

We have an item in our backlog to update the same blogpost you have shared because we see this problem coming up all the time. The reality is that AWS is so big, and there are multiple release systems involved. Sometimes service roll out changes service side that are not reflected in their model, causing problems like this.

Our aim on the SDK team is to equip our customers with the knowledge needed to overcome those hiccups.

Thanks again.
Ran~

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 Jan 18, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
feature-request New feature or enhancement. May require GitHub community feedback.
Projects
None yet
Development

No branches or pull requests

2 participants