Skip to content
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

Replace generic dictionary for commands with specialized collection type. #203

Merged
merged 9 commits into from
Jan 27, 2022
181 changes: 181 additions & 0 deletions src/ImageSharp.Web/Commands/CommandCollection.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,181 @@
// Copyright (c) Six Labors.
// Licensed under the Apache License, Version 2.0.

using System;
using System.Collections.Generic;
using System.Collections.ObjectModel;
using System.Runtime.CompilerServices;

namespace SixLabors.ImageSharp.Web.Commands
{
/// <summary>
/// Represents an ordered collection of processing commands.
/// </summary>
public sealed class CommandCollection : KeyedCollection<string, KeyValuePair<string, string>>
{
/// <summary>
/// Initializes a new instance of the <see cref="CommandCollection"/> class.
/// </summary>
public CommandCollection()
: this(StringComparer.OrdinalIgnoreCase)
{
}

private CommandCollection(IEqualityComparer<string> comparer)
: base(comparer)
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Member Author

@JimBobSquarePants JimBobSquarePants Jan 25, 2022

Choose a reason for hiding this comment

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

I considered implementing IDictionary<string, string> but it leads to a confusing API since the type inherits Collection<T>. Contains vs ContainsKey for example, I also didn't want to expose some unordered properties like Values. There's also virtualization issues passing around IDictionary<T> all the time. I also experimented with using a wrapper type with a private implementation of KeyedCollection<T,TItem> but this started to get messy quickly so I took the easy route.

I'm not quite sure what you're pointing out re the constructor. Are you suggesting using the internal dictionary to implement the interface or for something else?

Copy link
Contributor

Choose a reason for hiding this comment

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

Confusion can be avoided by using an explicit interface implementation (which might already be required for some conflicting members)...

The point I wanted to make, is that this implementation stores the items both in the collection and internal lookup dictionary, in addition to keeping a sorted list of keys.

Too bad OrderedDictionary isn't using generics, although I see there is an internal MS implementation.

{
}

/// <summary>
/// Gets an <see cref="IEnumerable{String}"/> representing the keys of the collection.
/// </summary>
public IEnumerable<string> Keys
{
get
{
foreach (KeyValuePair<string, string> item in this)
{
yield return this.GetKeyForItem(item);
}
}
}

/// <summary>
/// Gets or sets the value associated with the specified key.
/// </summary>
/// <param name="key">The key of the value to get or set.</param>
/// <returns>
/// The value associated with the specified key. If the specified key is not found,
/// a get operation throws a <see cref="KeyNotFoundException"/>, and
/// a set operation creates a new element with the specified key.
/// </returns>
/// <exception cref="ArgumentNullException"><paramref name="key"/> is null.</exception>
/// <exception cref="KeyNotFoundException">An element with the specified key does not exist in the collection.</exception>
public new string this[string key]
{
get
{
if (!this.TryGetValue(key, out string value))
{
ThrowKeyNotFound();
}

return value;
}

set
{
if (this.TryGetValue(key, out KeyValuePair<string, string> item))
{
this.SetItem(this.IndexOf(item), new(key, value));
}
else
{
this.Add(key, value);
}
}
}

/// <summary>
/// Adds an element with the provided key and value to the <see cref="CommandCollection"/>.
/// </summary>
/// <param name="key">The <see cref="string"/> to use as the key of the element to add.</param>
/// <param name="value">The <see cref="string"/> to use as the value of the element to add.</param>
/// <exception cref="ArgumentNullException"><paramref name="key"/> is null.</exception>
public void Add(string key, string value) => this.Add(new(key, value));

/// <summary>
/// Inserts an element into the <see cref="CommandCollection"/> at the
/// specified index.
/// </summary>
/// <param name="index">The zero-based index at which item should be inserted.</param>
/// <param name="key">The <see cref="string"/> to use as the key of the element to insert.</param>
/// <param name="value">The <see cref="string"/> to use as the value of the element to insert.</param>
/// <exception cref="ArgumentOutOfRangeException">index is less than zero. -or- index is greater than <see cref="P:CommandCollection.Count"/>.</exception>
public void Insert(int index, string key, string value) => this.Insert(index, new(key, value));

/// <summary>
/// Gets the value associated with the specified key.
/// </summary>
/// <param name="key">The key whose value to get.</param>
/// <param name="value">
/// When this method returns, the value associated with the specified key, if the
/// key is found; otherwise, the default value for the type of the value parameter.
/// This parameter is passed uninitialized.
/// </param>
/// <returns>
/// <see langword="true"/> if the object that implements <see cref="CommandCollection"/> contains
/// an element with the specified key; otherwise, <see langword="false"/>.
/// </returns>
/// <exception cref="ArgumentNullException"><paramref name="key"/> is null.</exception>
public bool TryGetValue(string key, out string value)
{
if (this.TryGetValue(key, out KeyValuePair<string, string> keyValue))
{
value = keyValue.Value;
return true;
}

value = default;
return false;
}

/// <summary>
/// Searches for an element that matches the conditions defined by the specified
/// predicate, and returns the zero-based index of the first occurrence within the
/// entire <see cref="CommandCollection"/>.
/// </summary>
/// <param name="match">
/// The <see cref="Predicate{T}"/> delegate that defines the conditions of the element to
/// search for.
/// </param>
/// <returns>
/// The zero-based index of the first occurrence of an element that matches the conditions
/// defined by match, if found; otherwise, -1.
/// </returns>
/// <exception cref="ArgumentNullException"><paramref name="match"/> is null.</exception>
public int FindIndex(Predicate<string> match)
{
Guard.NotNull(match, nameof(match));

int index = 0;
foreach (KeyValuePair<string, string> item in this)
{
if (match(item.Key))
{
return index;
}

index++;
}

return -1;
}

/// <summary>
/// Searches for the specified key and returns the zero-based index of the first
/// occurrence within the entire <see cref="CommandCollection"/>.
/// </summary>
/// <param name="key">The key to locate in the <see cref="CommandCollection"/>.</param>
/// <returns>
/// The zero-based index of the first occurrence of key within the entire <see cref="CommandCollection"/>,
/// if found; otherwise, -1.
/// </returns>
public int IndexOf(string key)
{
if (this.TryGetValue(key, out KeyValuePair<string, string> item))
{
return this.IndexOf(item);
}

return -1;
}

/// <inheritdoc/>
[MethodImpl(MethodImplOptions.AggressiveInlining)]
protected override string GetKeyForItem(KeyValuePair<string, string> item) => item.Key;

[MethodImpl(MethodImplOptions.NoInlining)]
private static void ThrowKeyNotFound() => throw new KeyNotFoundException();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -6,22 +6,20 @@
namespace SixLabors.ImageSharp.Web.Commands
{
/// <summary>
/// Extension methods for <see cref="IDictionary{TKey, TValue}"/>.
/// Extension methods for <see cref="CommandCollectionExtensions"/>.
/// </summary>
public static class DictionaryExtensions
public static class CommandCollectionExtensions
{
/// <summary>
/// Gets the value associated with the specified key or the default value.
/// </summary>
/// <param name="dictionary">The dictionary instance.</param>
/// <param name="collection">The collection instance.</param>
/// <param name="key">The key of the value to get.</param>
/// <typeparam name="TValue">The value type.</typeparam>
/// <typeparam name="TKey">The key type.</typeparam>
/// <returns>The value associated with the specified key or the default value.</returns>
public static TValue GetValueOrDefault<TValue, TKey>(this IDictionary<TKey, TValue> dictionary, TKey key)
public static string GetValueOrDefault(this CommandCollection collection, string key)
deanmarcussen marked this conversation as resolved.
Show resolved Hide resolved
{
dictionary.TryGetValue(key, out TValue result);
return result;
collection.TryGetValue(key, out KeyValuePair<string, string> result);
return result.Value;
}
}
}
2 changes: 1 addition & 1 deletion src/ImageSharp.Web/Commands/IRequestParser.cs
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,6 @@ public interface IRequestParser
/// </summary>
/// <param name="context">Encapsulates all HTTP-specific information about an individual HTTP request.</param>
/// <returns>The <see cref="IDictionary{TKey,TValue}"/>.</returns>
IDictionary<string, string> ParseRequestCommands(HttpContext context);
CommandCollection ParseRequestCommands(HttpContext context);
}
}
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// Copyright (c) Six Labors.
// Copyright (c) Six Labors.
// Licensed under the Apache License, Version 2.0.

using System;
Expand All @@ -16,7 +16,7 @@ namespace SixLabors.ImageSharp.Web.Commands
/// </summary>
public class PresetOnlyQueryCollectionRequestParser : IRequestParser
{
private readonly IDictionary<string, IDictionary<string, string>> presets;
private readonly IDictionary<string, CommandCollection> presets;

/// <summary>
/// The command constant for the preset query parameter.
Expand All @@ -31,31 +31,39 @@ public PresetOnlyQueryCollectionRequestParser(IOptions<PresetOnlyQueryCollection
this.presets = ParsePresets(presetOptions.Value.Presets);

/// <inheritdoc/>
public IDictionary<string, string> ParseRequestCommands(HttpContext context)
public CommandCollection ParseRequestCommands(HttpContext context)
{
if (context.Request.Query.Count == 0 || !context.Request.Query.ContainsKey(QueryKey))
{
return new Dictionary<string, string>(StringComparer.OrdinalIgnoreCase);
// We return new here and below to ensure the collection is still mutable via events.
return new();
deanmarcussen marked this conversation as resolved.
Show resolved Hide resolved
}

var requestedPreset = context.Request.Query["preset"][0];
return this.presets.GetValueOrDefault(requestedPreset) ?? new Dictionary<string, string>(StringComparer.OrdinalIgnoreCase);
string requestedPreset = context.Request.Query[QueryKey][0];
if (this.presets.TryGetValue(requestedPreset, out CommandCollection collection))
{
return collection;
}

return new();
}

private static IDictionary<string, IDictionary<string, string>> ParsePresets(
private static IDictionary<string, CommandCollection> ParsePresets(
IDictionary<string, string> unparsedPresets) =>
unparsedPresets
.Select(keyValue =>
new KeyValuePair<string, IDictionary<string, string>>(keyValue.Key, ParsePreset(keyValue.Value)))
new KeyValuePair<string, CommandCollection>(keyValue.Key, ParsePreset(keyValue.Value)))
.ToDictionary(keyValue => keyValue.Key, keyValue => keyValue.Value, StringComparer.OrdinalIgnoreCase);

private static IDictionary<string, string> ParsePreset(string unparsedPresetValue)
private static CommandCollection ParsePreset(string unparsedPresetValue)
{
// TODO: Investigate skipping the double allocation here.
// In .NET 6 we can directly use the QueryStringEnumerable type and enumerate stright to our command collection
Dictionary<string, StringValues> parsed = QueryHelpers.ParseQuery(unparsedPresetValue);
var transformed = new Dictionary<string, string>(parsed.Count, StringComparer.OrdinalIgnoreCase);
foreach (KeyValuePair<string, StringValues> keyValue in parsed)
CommandCollection transformed = new();
foreach (KeyValuePair<string, StringValues> pair in parsed)
{
transformed[keyValue.Key] = keyValue.Value.ToString();
transformed.Add(new(pair.Key, pair.Value.ToString()));
}

return transformed;
Expand Down
12 changes: 7 additions & 5 deletions src/ImageSharp.Web/Commands/QueryCollectionRequestParser.cs
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
// Copyright (c) Six Labors.
// Licensed under the Apache License, Version 2.0.

using System;
using System.Collections.Generic;
using Microsoft.AspNetCore.Http;
using Microsoft.AspNetCore.WebUtilities;
Expand All @@ -15,18 +14,21 @@ namespace SixLabors.ImageSharp.Web.Commands
public sealed class QueryCollectionRequestParser : IRequestParser
{
/// <inheritdoc/>
public IDictionary<string, string> ParseRequestCommands(HttpContext context)
public CommandCollection ParseRequestCommands(HttpContext context)
{
if (context.Request.Query.Count == 0)
{
return new Dictionary<string, string>(StringComparer.OrdinalIgnoreCase);
// We return new to ensure the collection is still mutable via events.
return new();
deanmarcussen marked this conversation as resolved.
Show resolved Hide resolved
}

// TODO: Investigate skipping the double allocation here.
// In .NET 6 we can directly use the QueryStringEnumerable type and enumerate stright to our command collection
Dictionary<string, StringValues> parsed = QueryHelpers.ParseQuery(context.Request.QueryString.ToUriComponent());
var transformed = new Dictionary<string, string>(parsed.Count, StringComparer.OrdinalIgnoreCase);
CommandCollection transformed = new();
foreach (KeyValuePair<string, StringValues> pair in parsed)
{
transformed[pair.Key] = pair.Value.ToString();
transformed.Add(new(pair.Key, pair.Value.ToString()));
}

return transformed;
Expand Down
7 changes: 3 additions & 4 deletions src/ImageSharp.Web/Middleware/ImageCommandContext.cs
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
// Copyright (c) Six Labors.
// Licensed under the Apache License, Version 2.0.

using System.Collections.Generic;
using System.Globalization;
using Microsoft.AspNetCore.Http;
using SixLabors.ImageSharp.Web.Commands;
Expand All @@ -22,7 +21,7 @@ public class ImageCommandContext
/// <param name="culture">The culture used to parse commands.</param>
public ImageCommandContext(
HttpContext context,
IDictionary<string, string> commands,
CommandCollection commands,
CommandParser parser,
CultureInfo culture)
{
Expand All @@ -38,9 +37,9 @@ public ImageCommandContext(
public HttpContext Context { get; }

/// <summary>
/// Gets the dictionary containing the collection of URI derived processing commands.
/// Gets the collection of URI derived processing commands.
/// </summary>
public IDictionary<string, string> Commands { get; }
public CommandCollection Commands { get; }

/// <summary>
/// Gets the command parser for parsing URI derived processing commands.
Expand Down
7 changes: 4 additions & 3 deletions src/ImageSharp.Web/Middleware/ImageProcessingContext.cs
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
using System.Collections.Generic;
using System.IO;
using Microsoft.AspNetCore.Http;
using SixLabors.ImageSharp.Web.Commands;

namespace SixLabors.ImageSharp.Web.Middleware
{
Expand All @@ -23,7 +24,7 @@ public class ImageProcessingContext
public ImageProcessingContext(
HttpContext context,
Stream stream,
IDictionary<string, string> commands,
CommandCollection commands,
string contentType,
string extension)
{
Expand All @@ -47,10 +48,10 @@ public ImageProcessingContext(
/// <summary>
/// Gets the parsed collection of processing commands.
/// </summary>
public IDictionary<string, string> Commands { get; }
public CommandCollection Commands { get; }

/// <summary>
/// Gets the content type for for the processed image.
/// Gets the content type for the processed image.
/// </summary>
public string ContentType { get; }

Expand Down
Loading