Resolve 145: add option to ignore URL host in cache key #147
Conversation
|
|
|
I wasn't able to find tests for the ImageSharpMiddleware, so I didn't add a test.
|
Codecov Report
@@ Coverage Diff @@
## master #147 +/- ##
==========================================
+ Coverage 84.50% 84.80% +0.30%
==========================================
Files 50 46 -4
Lines 1426 1369 -57
Branches 190 178 -12
==========================================
- Hits 1205 1161 -44
+ Misses 166 160 -6
+ Partials 55 48 -7
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
|
I see that code coverage is down, but it's tricky for me to find the tests responsible because I can't run all the tests. |
|
Thanks @yringler !! For the tests you need to be running the Azure Storage Emulator If you are working on a Windows machine with Visual Studio with the Azure SDK you've likely got an emulator installed which you can find via Start + typing Azure... The current cache tests are here. |
| @@ -492,9 +492,9 @@ private ValueTask StreamDisposeAsync(Stream stream) | |||
| } | |||
| } | |||
|
|
|||
| private static string GetUri(HttpContext context, IDictionary<string, string> commands) | |||
| private static string GetUri(HttpContext context, IDictionary<string, string> commands, bool ignoreHost) | |||
There was a problem hiding this comment.
Quick note that this code can be much improved. We know the size of all strings, so we can allocate the right buffer size instead of growing it in the SB.
Also using the Format("{0}/") is too much for a simple concatenation.
Only QueryString.Create(commands) needs to be done before to know what will be the size.
Should I file an issue or not worth it?
There was a problem hiding this comment.
Yep, that's a valid point. Please file an issue. I'm up for any optimizations we can through at the library.
|
I'll be out for the next week and a bit (Passover), so I won't get to the unit tests before that. |
|
Can I just offer a thought as well. Not sure if this would be best suited via a setting, or by putting this method into an interface, and making it a replaceable component to meet any variety of use cases. In any kind of multi tenant scenario, where the host name actually means a different tenant / image source it's a little dangerous, easy to set a setting and have two tenants serving the same cached image, when the image source is actually different. I'm 50/50 on the fence about it though, so will leave design decisions up to @JimBobSquarePants |
I get your point here @deanmarcussen a setting is a little too easy to abuse. I'll have a look at rewriting the PR to introduce two implementations of a as yet unnamed interface that can be injected to allow this. It's a fair amount of work but worth the effort. |
|
I added tests for the new option... Perhaps it shouldn't be necessary to set up a whole test server to unit test a config?
In the meantime, this is what I'm able to put together, in lieu of / until the major refactor that you're suggesting |
|
It’ll be a few days til I can look at this. Sick at the moment with oral shingles. |
|
@yringler I haven't forgotten about this. Had a bout of ill health over the last 6 weeks so having to carefully choose my activities in a limited time frame. |
|
Oy. Feel better soon! |
|
This issue kind of surprised me, as it's very common to have your site served from both the Wouldn't it make a lot more sense to only use the path, as the default setup/middleware configuration doesn't support a multi-tenant setup anyway? If you want to add that support, you already need to change the default configuration, right? Also note that the The easy/quickest fix would be to change So I would suggest adding a new interface that generates the cache key, so this can be separated from the existing using Microsoft.AspNetCore.Http;
using SixLabors.ImageSharp.Web.Commands;
namespace SixLabors.ImageSharp.Web.Caching
{
public interface ICacheKey
{
string Create(HttpContext context, CommandCollection commands);
}
}And a default implementation that uses the relative path: using Microsoft.AspNetCore.Http;
using Microsoft.AspNetCore.Http.Extensions;
using SixLabors.ImageSharp.Web.Commands;
namespace SixLabors.ImageSharp.Web.Caching
{
public class RelativePathLowercaseCacheKey : ICacheKey
{
public string Create(HttpContext context, CommandCollection commands)
=> UriHelper.BuildRelative(context.Request.PathBase, context.Request.Path, QueryString.Create(commands)).ToLowerInvariant();
}
}Note: I'm already using the new |
|
If you just use the path that can (will absolutely because people are bad at naming things) increase the possibility of collision. I’m curious as to why you say the default setup/middleware doesn’t support multi tenancy because I’m using it out of the box with a multi tenant website and a custom provider. I do like your idea though just wouldn't ever use the relative path as the default implementation since that would invalidate the cache for any current installs. |
Yes, so you had to use a custom provider to support multi tenancy (the default setup, using just In that sense, maybe the responsibility of returning the correct cache key should be added to |
|
You're more on the right track with your new interface idea, I really like it. Although I think you're misunderstanding what I mean by collisions.
This will generate the same hash since the relative paths are not unique. There are implementations like this, I've seen them, they are shockingly numerous, and they are hideous.
In my working implementation media item request use guids not slugified filenames to prevent collision and queries are prefiltered using EF Core query filters so I return true/false based ion the result of a query. It's a quick and easy way to handle multitenancy done right. So back to the interface. I wouldn't change I would then create two implementations of
If I could turn back time I'd definitely use a relative path as the default (naïve implementations be damned!) but unfortunately I think we're stuck. There's breaking changes and then there's breaking changes. (However, I could maybe be convinced as I don't believe anyone is paying for me to maintain this library) |
|
Creating different cache keys and thereby invalidating all existing cached files would indeed be a functional change, especially because the cached files aren't automatically cleaned up! It wouldn't really break sites that upgrade though... I've abstracted/implemented the existing cache key generation in PR #206 and also added some example/reference implementations. If this gets released as part of v2, changing the default cache key implementation to use a lowercased relative URI wouldn't be a big issue, right? Upgrading will probably already require some code changes and adding a release note to advise users to manually clean up their image cache (or switch back to the v1 implementation) should be enough. Otherwise it will just re-generate the images and use up a bit more disk space (and potentially result in colliding with previously generated cache hashes for different images). Regarding that last line: users should already always manually clean up their image cache after changing either the cache hash implementation anyway! So the same applies for changing the cache key implementation.
Yeah, would be nice if you could go back and fix things before they bit you in the *ss 😆 Umbraco is sponsoring SixLabors on a monthly basis, although I guess that only pays for the coffee for the whole team 😅 |
|
Newly merged #206 replaces this. Thanks everyone! |
Prerequisites
Description