Merge pull request #268 from SixLabors/js/no-double-enumerate-

Optimize Invalid command removal.
SixLabors · Jun 1, 2022 · 52a9fa0 · 52a9fa0
2 parents b86ae56 + f8fdde6
commit 52a9fa0
Show file tree

Hide file tree

Showing 6 changed files with 42 additions and 26 deletions.
diff --git a/src/ImageSharp.Web/Commands/PresetOnlyQueryCollectionRequestParser.cs b/src/ImageSharp.Web/Commands/PresetOnlyQueryCollectionRequestParser.cs
@@ -39,7 +39,8 @@ public CommandCollection ParseRequestCommands(HttpContext context)
                 return new();
             }
 
-            string requestedPreset = context.Request.Query[QueryKey][0];
+            StringValues query = context.Request.Query[QueryKey];
+            string requestedPreset = query[query.Count - 1];
             if (this.presets.TryGetValue(requestedPreset, out CommandCollection collection))
             {
                 return collection;
@@ -63,7 +64,8 @@ private static CommandCollection ParsePreset(string unparsedPresetValue)
             CommandCollection transformed = new();
             foreach (KeyValuePair<string, StringValues> pair in parsed)
             {
-                transformed.Add(new(pair.Key, pair.Value.ToString()));
+                // Use the indexer for both set and query. This replaces any previously parsed values.
+                transformed[pair.Key] = pair.Value[pair.Value.Count - 1];
             }
 
             return transformed;

diff --git a/src/ImageSharp.Web/Commands/QueryCollectionRequestParser.cs b/src/ImageSharp.Web/Commands/QueryCollectionRequestParser.cs
@@ -28,7 +28,8 @@ public CommandCollection ParseRequestCommands(HttpContext context)
             CommandCollection transformed = new();
             foreach (KeyValuePair<string, StringValues> pair in parsed)
             {
-                transformed.Add(new(pair.Key, pair.Value.ToString()));
+                // Use the indexer for both set and query. This replaces any previously parsed values.
+                transformed[pair.Key] = pair.Value[pair.Value.Count - 1];
             }
 
             return transformed;

diff --git a/src/ImageSharp.Web/ImageSharp.Web.csproj b/src/ImageSharp.Web/ImageSharp.Web.csproj
@@ -46,7 +46,7 @@
 
   <ItemGroup>
     <PackageReference Include="Microsoft.IO.RecyclableMemoryStream" Version="2.2.0" />
-    <PackageReference Include="SixLabors.ImageSharp" Version="2.1.1" />
+    <PackageReference Include="SixLabors.ImageSharp" Version="2.1.2" />
   </ItemGroup>
 
   <Import Project="..\..\shared-infrastructure\src\SharedInfrastructure\SharedInfrastructure.projitems" Label="Shared" />

diff --git a/src/ImageSharp.Web/Middleware/ImageSharpMiddleware.cs b/src/ImageSharp.Web/Middleware/ImageSharpMiddleware.cs
@@ -212,17 +212,13 @@ private async Task Invoke(HttpContext httpContext, bool retry)
             if (commands.Count > 0)
             {
                 // Strip out any unknown commands, if needed.
-                int index = 0;
-                foreach (string command in commands.Keys)
+                var keys = new List<string>(commands.Keys);
+                for (int i = keys.Count - 1; i >= 0; i--)
                 {
-                    if (!this.knownCommands.Contains(command))
+                    if (!this.knownCommands.Contains(keys[i]))
                     {
-                        // Need to actually remove, allocates new list to allow modifications
-                        this.StripUnknownCommands(commands, index);
-                        break;
+                        commands.RemoveAt(i);
                     }
-
-                    index++;
                 }
             }
 
@@ -307,19 +303,6 @@ private static void SetBadRequest(HttpContext httpContext)
             httpContext.Response.StatusCode = StatusCodes.Status400BadRequest;
         }
 
-        private void StripUnknownCommands(CommandCollection commands, int startAtIndex)
-        {
-            var keys = new List<string>(commands.Keys);
-            for (int index = startAtIndex; index < keys.Count; index++)
-            {
-                string command = keys[index];
-                if (!this.knownCommands.Contains(command))
-                {
-                    commands.Remove(command);
-                }
-            }
-        }
-
         private async Task ProcessRequestAsync(
             HttpContext httpContext,
             IImageResolver sourceImageResolver,

diff --git a/tests/ImageSharp.Web.Tests/Commands/CommandCollectionTests.cs b/tests/ImageSharp.Web.Tests/Commands/CommandCollectionTests.cs
@@ -1,6 +1,7 @@
 // Copyright (c) Six Labors.
 // Licensed under the Apache License, Version 2.0.
 
+using System;
 using System.Collections.Generic;
 using SixLabors.ImageSharp.Web.Commands;
 using Xunit;
@@ -17,6 +18,15 @@ public void CanAddCommands()
             Assert.Single(collection);
         }
 
+        [Fact]
+        public void CannotAddDuplicateCommands()
+        {
+            CommandCollection collection = new();
+            collection.Add(new("a", "b"));
+            Assert.Throws<ArgumentException>(() => collection.Add(new("a", "b")));
+            Assert.Single(collection);
+        }
+
         [Fact]
         public void CanReadCommands()
         {
@@ -48,6 +58,15 @@ public void CanInsertCommands()
             Assert.Equal(0, collection.IndexOf(kv2));
         }
 
+        [Fact]
+        public void CannotInsertDuplicateCommands()
+        {
+            CommandCollection collection = new();
+            collection.Add(new("a", "b"));
+            Assert.Throws<ArgumentException>(() => collection.Insert(0, new("a", "b")));
+            Assert.Single(collection);
+        }
+
         [Fact]
         public void CanInsertCommandsViaKey()
         {
@@ -68,6 +87,15 @@ public void CanInsertCommandsViaKey()
             Assert.Equal(0, collection.IndexOf(kv2));
         }
 
+        [Fact]
+        public void CannotInsertDuplicateCommandsViaKey()
+        {
+            CommandCollection collection = new();
+            collection.Add(new("a", "b"));
+            Assert.Throws<ArgumentException>(() => collection.Insert(0, "a", "b"));
+            Assert.Single(collection);
+        }
+
         [Fact]
         public void CanSetCommandsViaIndex()
         {

diff --git a/tests/ImageSharp.Web.Tests/Commands/QueryCollectionUriParserTests.cs b/tests/ImageSharp.Web.Tests/Commands/QueryCollectionUriParserTests.cs
@@ -28,7 +28,9 @@ private static HttpContext CreateHttpContext()
         {
             var httpContext = new DefaultHttpContext();
             httpContext.Request.Path = "/testwebsite.com/image-12345.jpeg";
-            httpContext.Request.QueryString = new QueryString("?width=400&height=200");
+
+            // Duplicate height param to test replacements
+            httpContext.Request.QueryString = new QueryString("?width=400&height=100&height=200");
             return httpContext;
         }
     }