Functionality to fail gracefully in ImageResolver

[marklagendijk]

The interface of IImageResolver is:

public interface IImageResolver
    {
        Task<ImageMetadata> GetMetaDataAsync();
        Task<Stream> OpenReadAsync();
    }

The problem is that there can be several issues with an image that can only be determined during the resolve. Currently an ImageResolver can only throw an exception in such cases. This will result in a 500 status code.

The main problem with this is that it pollutes the error logs of the application.

Here are a few issues that may occur, with the proper status code:

• Image does not exist => 404 (NotFound)
• There is something wrong about the request for the image => 400 (BadRequest)
• There is something wrong about the image response => 502 (Bad Gateway)
• The request for a remote image timed out => 504 (Gateway Timeout)

In .NET Framework on could throw an System.Web.HttpException, but that no longer exists in .NET Core (for good reasons).
A possible solution would be to allow GetMetaDataAsync to return either an ImageMetadata or an typed error.

[JimBobSquarePants]

A possible solution would be to allow GetMetaDataAsync to return either an ImageMetadata or an typed error.

We cannot change the interface. That's a breaking change requiring a major version, there's also no correlation between the types.

We already handle 404s.

ImageSharp.Web/src/ImageSharp.Web/Middleware/ImageSharpMiddleware.cs

Lines 223 to 233 in c625825

	IImageResolver sourceImageResolver = await provider.GetAsync(context);
	if (sourceImageResolver is null)
	{
	// Log the error but let the pipeline handle the 404
	// by calling the next delegate/middleware in the pipeline.
	var imageContext = new ImageContext(context, this.options);
	this.logger.LogImageResolveFailed(imageContext.GetDisplayUrl());
	await this.next(context);
	return;
	}

We don't handle remote images other than from Azure (But you know this since you've developed an independent provider).

ImageSharp.Web/src/ImageSharp.Web.Providers.Azure/Providers/AzureBlobStorageImageProvider.cs

Lines 104 to 125 in c625825

	// Something has gone horribly wrong for this to happen but check anyway.
	if (container is null)
	{
	return null;
	}
	// Blob name should be the remaining path string.
	string blobName = path.Substring(containerName.Length).TrimStart(SlashChars);
	if (string.IsNullOrWhiteSpace(blobName))
	{
	return null;
	}
	BlobClient blob = container.GetBlobClient(blobName);
	if (!await blob.ExistsAsync())
	{
	return null;
	}
	return new AzureBlobStorageImageResolver(blob);

.If you're getting 500s in your logs, you should be dealing with those errors. I'm very curious to see what they are.

[marklagendijk]

Ahh, you're doing the check before creating the resolver...
I think this may be wrong or else just counter intuitive:

• I would expect a resolver that returns metadata of something also to be able to deal with things going wrong. Now the only way of dealing with this is by returning null instead of a resolver.
• By having to return null instead of a resolver, you have to do similar things as the resolver is doing (check something (existence, correctness) about the target that needs to be resolved).
• If the resolver is used more than once (this part of the flow is not yet clear to me), it can happen that the target becomes invalid after the resolver was created, for example it may be removed.

Based on all of this it seemed to me that the best way to tackle these issues is by changing the GetMetaDataAsync method of IImageResolver.
There may be ways to do this without it being a breaking change (I don't know enough about C# yet to know these things exactly). Things that would come to mind are:

• Create an interface empty MetadataResult that ImageMetadata implements, and use that for GetMetaDataAsync.
• Change Task<ImageMetadata> to Task<ImageMetadata?>

If that doesn't work the only way to fix it would indeed be a breaking change (if you're following semver). On the other hand, with semver the idea is also to not be too afraid of releasing major versions. Many projects just release often, and whenever something happens to be a breaking change the release happens to be a major version.

[JimBobSquarePants]

I'm sorry but that's simply not going to happen.

I have no basis for making these changes. The system is designed to work as efficiently as possible. If you are having issues you should isolate and fix the source of the issues.

[marklagendijk]

No problem.

I was just pointing out some points that in my opinion are architectural improvements.
Do you disagree with my points, or don't you think that they're worth the effort?

[JimBobSquarePants]

I'm afraid I disagree with the points.

We already handle errors and throw appropriately when something catastrophic happens.

If an IImageProvider cannot for some reason create an IImageResolver we log and return a 404. IImageProvider implementations are simply providers for resolvers. They match URLs and do the absolute minimum to ensure the image exists as the source.

Barring catastrophic error an IImageResolver will always return a result. We've already ensured that the source image exists.

During resolution and any other following process, If something goes wrong we log the issue then rethrow.

ImageSharp.Web/src/ImageSharp.Web/Middleware/ImageSharpMiddleware.cs

Line 373 in c625825

this.logger.LogImageProcessingFailed(imageContext.GetDisplayUrl(), ex);

It's important to rethrow so that unhandled errors don't simply disappear into logs. The whole process is asynchronous so doesn't bring down an application.

If you're getting 500's then something catastrophic is happening and you need to fix those issues.

Based upon the points you've brought up it seems to me that the desire is driven by the need to ensure remote image are resolvable. This is another reason why I did not want a remote provider. It's a leaky abstraction with too many potential points of error.