Replace generic dictionary for commands with specialized collection type. #203
Conversation
Codecov Report
@@ Coverage Diff @@
## master #203 +/- ##
==========================================
- Coverage 85.56% 85.55% -0.02%
==========================================
Files 54 55 +1
Lines 1552 1585 +33
Branches 231 237 +6
==========================================
+ Hits 1328 1356 +28
- Misses 167 171 +4
- Partials 57 58 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
|
It will be marginally breaking for us, only in the sense that the api has changed, so we can't easily release to our preview feed, and get some people trying the new version, as we don't like to push anything that isn't on NuGet. We do quite a lot with commands, for DDOS prevention, and such, but it's mostly about removal. Either way the api still does what it used to, so all good from us. |
|
@deanmarcussen Thanks for getting back to me. Good to hear it's not an issue. |
|
Thanks for bringing this to my attention 👍🏻 We'll only update the ImageSharp dependency to v2 in the next major Umbraco version (v10, planned Q2, 2022), so breaking this API shouldn't be a big issue for us. However, I'm not aware of any other breaking API changes in ImageSharp/ImageSharp.Web and this would prevent users from upgrading Umbraco 9 sites to use v2, as the codebase uses the affected I've quickly tested this on an Umbraco 9.2.0 project using the nightly NuGet feed (SixLabors.ImageSharp 2.0.0-alpha.0.159 and SixLabors.ImageSharp.Web 2.0.0-alpha.0.3) and both the command parsing (configurable max width/height) and the custom crops still work. This is the only concern I currently have with these changes, but I also see the value in allowing commands to be inserted (adding auto-rotation, as you already showed). |
There was a problem hiding this comment.
A lot of the breaking changes can be avoided by having CommandCollection implement IDictionary<string, string> and forward that to the KeyedCollection<>, right?
That would make accessing the context.Commands in the OnParseCommandsAsync work, as it can still use the members of the IDictionary<string, string> on the new collection. And IImageWebProcessor can keep the existing signature, as that doesn't alter the order of the commands and thus doesn't need the concrete implementation.
| } | ||
|
|
||
| private CommandCollection(IEqualityComparer<string> comparer) | ||
| : base(comparer) |
There was a problem hiding this comment.
Keep in mind that this constructor already creates an internal lookup dictionary: https://docs.microsoft.com/en-us/dotnet/api/system.collections.objectmodel.keyedcollection-2.-ctor?view=net-6.0#system-collections-objectmodel-keyedcollection-2-ctor(system-collections-generic-iequalitycomparer((-0))-system-int32).
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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="ICollection{T}"/> representing the keys of the collection. | ||
| /// </summary> | ||
| public ICollection<string> Keys => this.keys; |
There was a problem hiding this comment.
This can also use the internal lookup dictionary (which might be null, which might need a fallback to an empty list):
| public ICollection<string> Keys => this.keys; | |
| public ICollection<string> Keys => this.Dictionary?.Keys; |
There was a problem hiding this comment.
The internal lookup Dictionary.Keys property isn't actually in order. There's a private List<T> but I can't touch it which is frustrating so I've duplicated it. Memory usage should be fairly minimal but if it's a problem I'll reimplement the entire type.
There was a problem hiding this comment.
Ah, of course, I already thought I was missing something! 😖
I see this is only used in ImageSharpMiddleware.StripUnknownCommands(), so might be worthwhile to convert this into something like this:
| public ICollection<string> Keys => this.keys; | |
| public IEnumerable<string> Keys | |
| { | |
| get | |
| { | |
| foreach (var item in this) | |
| { | |
| yield return this.GetKeyForItem(item); | |
| } | |
| } | |
| } |
There was a problem hiding this comment.
That's a neat suggestion! The enumerator should still be fast here.
I'll not commit this directly as it means I can remove a bunch of overrides at the same time.
I thought it was possible to swap out the implementation in Umbraco, so potentially a package could bridge Umbraco 9 to ImageSharp 2? |
@skttl Umbraco does indeed use abstractions (for image URL generation and getting image dimensions), but @JimBobSquarePants I've done some light hacking to get this PR compatible with Umbraco 9: ronaldbarendse@66f843d. Having |
|
@ronaldbarendse I hard a look at your branch and while the refactor is impressive I still believe the dedicated type is the right way to go.
Oh and re upgrading a major... There's precedence - Maybe we can convince the powers that be to let it happen. The memory usage improvements alone are well worth it and there's not many libraries that unit test as much as we do. |
|
@JimBobSquarePants My attempt was mainly to see whether it was at least possible to minimize the breaking changes so it would still work on an existing Umbraco 9 project. You make valid points about the property/virtual dispatch performance issues and the nastiness it requires, so lets break some (more) stuff! To fix things and make it better of course! 😉 Regarding the precedence though: that major version bump in I do have some suggestions on adding a couple of convenience methods to the public void Add(string key, string value) => this.Add(new(key, value));
public void Insert(int index, string key, string value) => this.Insert(index, new(key, value));
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;
} |
|
Just added the comments for future us. Once this builds I think we're ready to merge. |
Prerequisites
Description
This breaking change is required in order to allow users to easily insert commands into the processing pipeline via the
OnParseCommandsAsync.It introduces a new type
CommandCollectionbased uponKeyedCollection<string, KeyValuePair<string, string>>to replace the genericIDictionary<string, string>previously used to house parsed commands.The requirement was brought to my attention in #191 where an issue was correctly identified when attempting to insert commands into the beginning of the command collection.
While the change is breaking and changes the signature of a fundamental component (
IImageWebProcessor) I believe that it is an important change to make and we should do so now before it is too late.Following the change, automatically inserting the command for a potential
AutoOrientWebProcessorprocessor to manage EXIF rotation would be as simple as following.CC @ronaldbarendse as I want to know what affect this would have on Umbraco.
CC @deanmarcussen as I want to know what affect this would have on Orchard.