Skip to content

Throw S3FileStorageException when something goes wrong and response is not as expected. #305

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

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open
5 changes: 4 additions & 1 deletion src/Foundatio.AWS/Storage/S3FileStorage.cs
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
using Amazon.S3.Model;
using Amazon.S3.Util;
using Foundatio.AWS.Extensions;
using Foundatio.AWS.Storage;
using Foundatio.Extensions;
using Foundatio.Serializer;
using Microsoft.Extensions.Logging;
Expand Down Expand Up @@ -137,10 +138,12 @@ public async Task<FileSpec> GetFileInfoAsync(string path)
try
{
var response = await _client.GetObjectMetadataAsync(req).AnyContext();
if (response.HttpStatusCode == System.Net.HttpStatusCode.NotFound)
return null;
if (!response.HttpStatusCode.IsSuccessful())
{
_logger.LogDebug("[{HttpStatusCode}] Unable to get file info for {Path}", response.HttpStatusCode, req.Key);
return null;
throw new Exception($"Invalid status code {res.HttpStatusCode} ({(int)res.HttpStatusCode}): Expected 200 OK or 404 NotFound");
}

return new FileSpec
Expand Down
16 changes: 16 additions & 0 deletions src/Foundatio.AWS/Storage/S3FileStorageException.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
using System;
using System.Collections.Generic;
using System.Text;

namespace Foundatio.AWS.Storage {
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

@frabe1579 I think this was the main concern with merging this PR. We don't want to have different exceptions being thrown per implementation.

Copy link
Author

Choose a reason for hiding this comment

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

Sorry I'm not comfortable with branches and pull-requests. I've removed S3FileStorageException and corrected code. Can you see it?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, thank you, we can delete this file.

Copy link
Member

@niemyjski niemyjski May 15, 2025

Choose a reason for hiding this comment

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

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!

public class S3FileStorageException : Exception {
public S3FileStorageException() : base() {
}

public S3FileStorageException(string message) : base(message) {
}

public S3FileStorageException(string message, Exception innerException) : base(message, innerException) {
}
}
}