Processing does not use query string parameter order

[ronaldbarendse]

Prerequisites

• I have written a descriptive issue title
• I have verified that I am running the latest version of ImageSharp.Web
• I have verified if the problem exist in both DEBUG and RELEASE mode
• I have searched open and closed issues to ensure it has not already been reported

Description

According to the documentation processing operations should use the order in which they are supplied in the query string, but this is currently not the case. Instead, it loops through all processors in the order they are registered and calls the Process method on each one (even when none of the commands for that processor are provided):

ImageSharp.Web/src/ImageSharp.Web/Processors/WebProcessingExtensions.cs

Lines 28 to 45 in d055952

	public static FormattedImage Process(
	this FormattedImage source,
	ILogger logger,
	IEnumerable<IImageWebProcessor> processors,
	IDictionary<string, string> commands,
	CommandParser commandParser,
	CultureInfo culture)
	{
	if (commands.Count != 0)
	{
	foreach (IImageWebProcessor processor in processors)
	{
	source = processor.Process(source, logger, commands, commandParser, culture);
	}
	}
	return source;
	}

This probably wasn't noticed before, because the order in which the built-in processors are called doesn't affect the end-result (ResizeWebProcessor, FormatWebProcessor, BackgroundColorWebProcessor and JpegQualityWebProcessor all affect different aspects of an image). All processors also include a check before mutating the image, so that ensured there were no side-effects by always invoking them.

Umbraco required a custom processor to crop images using coordinates (representing a crop box/rectangle), but adding that after the default built-in processors would result in cropping the already resized image, instead of using the original.

Steps to Reproduce

You can use the CropWebProcessor from umbraco/Umbraco-CMS#10623 to test this issue (or create a similar simple one):

• Only adding the processor (after the default ones) ends up with wrongly cropped/resized images;
• Removing the ResizeWebProcessor, adding the custom processor and then re-adding it again will correctly crop before resizing.

Suggested solution

The ImageSharpMiddleware already keeps track of all known processing commands, filters unknown ones from the current request and short-circuits when there are none. This would mean only the above Process extension method will need to be updated.

Besides invoking the processor in the order they are provided in the query string, we need to ensure they aren't invoked for every command (e.g. twice when using ?width=200&height=200). This can be done by tracking the all commands used by a processor:

var processedCommands = new List<string>();
foreach (KeyValuePair<string, string> command in commands)
{
    if (processedCommands.Contains(command.Key, StringComparer.OrdinalIgnoreCase))
    {
        // Skip commands already used by a processor
        continue;
    }

    foreach (IImageWebProcessor processor in processors.Where(p => p.Commands.Contains(command.Key, StringComparer.OrdinalIgnoreCase)))
    {
        source = processor.Process(source, logger, commands, commandParser, culture);

        processedCommands.AddRange(processor.Commands);
    }
}

Or by tracking available processors and remove them after invoking:

var availableProcessors = processors.ToList();
foreach (KeyValuePair<string, string> command in commands)
{
    foreach (IImageWebProcessor processor in availableProcessors.Where(p => p.Commands.Contains(command.Key, StringComparer.OrdinalIgnoreCase)).ToList())
    {
        source = processor.Process(source, logger, commands, commandParser, culture);

        // Prevent invoking the processor again
        availableProcessors.Remove(processor);
    }
}

I haven't tested the above code, but it should give a good start for a PR/fix and might be something to benchmark/compare against each other.

Other notes

The current API (using IDictionary<string, string> commands) combines duplicate query string keys, making it impossible to run processors more than once using different commands, e.g. resize width to 200, crop and then resize the cropped version to a height of 300 (there are definitely better examples to illustrate the use case).

This might be because QueryHelpers.ParseQuery() is used, which creates an IDictionary<string, StringValues> and thereby already loses the order of duplicate query string keys. Because the StringValues are then converted into a single string (joining multiple values with a comma), you might end up with invalid values (e.g. ?width=100&width=200 will result in 100,200, which gets interpreted as 100200 when used as resize width)!

Also something to note: the cache key only uses known processor commands, so adding cache busting parameters (e.g. rnd or v) doesn't currently work. There is a CacheBusterWebProcessor, but that's only used/added for tests. Changing the order does though, so that part should already work as expected!

System Configuration

• ImageSharp.Web version: 1.0.3
• Other Six Labors packages and versions: ImageSharp 1.0.3
• Environment (Operating system, version and so on): Windows 10 (21H1 64-bit)
• .NET Framework version: .NET 5
• Additional information: Umbraco 9.0.0-rc001

[ronaldbarendse]

A quick note on the suggested solutions: keeping track of already used commands might skip processors that use the same commands (e.g. quality, which is currently processed by JpegQualityWebProcessor, but might have more - for different formats - in the future).

The safest way would then be to keep track of processors that have already run (second suggested solution) or order them based on the index of the first supported command:

var commandKeys = commands.Keys.ToList();
foreach (IImageWebProcessor processor in processors.GroupBy(p => p.Commands?.Min(c => commandKeys.IndexOf(c)) ?? -1).Where(g => g.Key != -1).OrderBy(g => g.Key).SelectMany(g => g))
{
    source = processor.Process(source, logger, commands, commandParser, culture);
}

This should also ensure when multiple processors support the same command(s), the registration order is still used to resolve that ambiguity.

[deanmarcussen]

Would it make more sense, given that the crop processor you are using, always wants to be first (if I read the issue right), to register that in the DI container as the first processor?

Basing the processor order on the order of query string parameters feels like it could be risky for consumers, if for example, they added the cropping commands later in the query string, they would get a different behaviour, I think? Whereas it sounds like you are quite specific about what order the crop processor should run.

[ronaldbarendse]

@deanmarcussen The first paragraph of the documentation explicitly states this behavior: "The ImageSharp.Web processing API is imperative. This means that the order in which you supply the individual processing operations is the order in which they are compiled and applied. This allows the API to be very flexible, allowing you to combine processes in any order."

This makes more sense if you have processors that use transparency and/or combines images (e.g. a watermark). For example: watermarks should be added last, but what if you want to add a semi-transparent color overlay (tint the image)? Should this color overlay only affect the original image or also the watermark? Both could be valid within the same application, so using the order in which the processors are registered won't allow for this. Besides that: there's currently no way to easily re-order the processors (besides removing and re-adding).

Regarding existing URLs: the default processors (resize, format, quality, background color) all affect different parts of the image, so changing the order shouldn't cause issues. For Umbraco the crops are already added before any of the resize commands, so that should already work as expected 😄

[JimBobSquarePants]

Yeah, this one's on me. I messed up!

I haven't looked at the code yet but I think it might be impossible to fix this in a performant matter without a breaking change (I'm happy to be proven wrong). ImageSharp v2 isn't that far away so it might be best to wait until then.

[ronaldbarendse]

@JimBobSquarePants It only needs to find the index of the first supported command per processor and sort on that, so I don't think that will impact the performance that much (especially compared to invoking each processor and have it do similar checks). Once the image is processed, it won't need to do this again and can just return the cached version...

And the breaking change will only be behavioral, but that's the case for most bug fixes and PR #183 just ensures it's working as documented.

[JimBobSquarePants]

Fixed by #183