Throw S3FileStorageException when something goes wrong and response is not as expected.#305
Conversation
…s not as expected for GetFileStreamAsync, GetFileInfoAsync, and ExistsAsync.
|
|
|
There is a failure on Queue tests, but this PR touches only FileStorage, maybe future merges can solve this. |
| } catch (AmazonS3Exception ex) { | ||
| if (ex.StatusCode == System.Net.HttpStatusCode.NotFound) | ||
| return null; | ||
| throw new S3FileStorageException("Error accessing S3 storage: " + ex.Message, ex); | ||
| } |
There was a problem hiding this comment.
Would be nice to use exception filters here
|
In this specific case, trace/debug logging I think isn't needed, because in case of not-managed error, an exception is thrown to the caller. |
ejsmith
left a comment
ejsmith
left a comment
There was a problem hiding this comment.
Wow. Just realized that I reviewed this a long while back and didn't submit the review. Sorry.
| using System.Collections.Generic; | ||
| using System.Text; | ||
|
|
||
| namespace Foundatio.AWS.Storage { |
There was a problem hiding this comment.
We definitely don't want to have exceptions that are unique to each implementation where a user would need to check for different kinds of exceptions depending on which implementation they are using.
There was a problem hiding this comment.
@frabe1579 I think this was the main concern with merging this PR. We don't want to have different exceptions being thrown per implementation.
There was a problem hiding this comment.
Sorry I'm not comfortable with branches and pull-requests. I've removed S3FileStorageException and corrected code. Can you see it?
There was a problem hiding this comment.
Yes, thank you, we can delete this file.
There was a problem hiding this comment.
I'm going to work towards merging this pr then create a StorageException in the core library and update the core implementations and then update this line (post merge) to use this new exception type.
Thanks again for the pr!
|
@frabe1579 could you please address Erics comments, would love to get this merged. |
Use more generic exception for error in GetFileInfo
`res` variable is now named `resource`.
niemyjski
left a comment
niemyjski
left a comment
There was a problem hiding this comment.
I'm going to merge this and then update it shortly to use the new StorageException we created in Foundatio.
|
Thank you so much for your PR! |
4 participants
This PR changes GetFileStreamAsync, GetFileInfoAsync, and ExistsAsync.
The changes test for precise response status code, in order to avoid silent catching of particular S3 errors.
All tests passes, but not new tests were added because of the difficult to simulate errors. A classic error simulation is network error, but this already throws other exceptions.
Related to FoundatioFx/Foundatio#281