From 125b74a478b2cce31ff88e59f76ea9e6e04f09bc Mon Sep 17 00:00:00 2001 From: Donald Gray Date: Wed, 31 May 2023 11:32:09 +0100 Subject: [PATCH 1/5] Update path parser to remove query params --- .../AssetDeliveryPathParserTests.cs | 28 +++++++++++++++++++ .../AssetDelivery/AssetDeliveryPathParser.cs | 12 ++++++-- 2 files changed, 38 insertions(+), 2 deletions(-) diff --git a/src/protagonist/DLCS.Web.Tests/Requests/AssetDelivery/AssetDeliveryPathParserTests.cs b/src/protagonist/DLCS.Web.Tests/Requests/AssetDelivery/AssetDeliveryPathParserTests.cs index 82fcb92d1..5d92b0e0e 100644 --- a/src/protagonist/DLCS.Web.Tests/Requests/AssetDelivery/AssetDeliveryPathParserTests.cs +++ b/src/protagonist/DLCS.Web.Tests/Requests/AssetDelivery/AssetDeliveryPathParserTests.cs @@ -42,6 +42,34 @@ public async Task Parse_ImageRequest_WithCustomerId_ReturnsCorrectRequest() thumbnailRequest.AssetPath.Should().Be("the-astronaut"); thumbnailRequest.AssetId.Should().Be("the-astronaut"); } + + [Theory] + [InlineData("/thumbs/99/1/the-astronaut?foo=bar")] + [InlineData("/thumbs/99/1/the-astronaut?foo=bar&baz=qux")] + [InlineData("/thumbs/99/1/the-astronaut#test")] + [InlineData("/thumbs/99/1/the-astronaut#test?foo=bar")] + [InlineData("/thumbs/99/1/the-astronaut?foo=bar#test")] + public async Task Parse_ImageRequest_WithAdditionalParams_ReturnsCorrectRequest(string path) + { + // Arrange + var customer = new CustomerPathElement(99, "Test-Customer"); + A.CallTo(() => pathCustomerRepository.GetCustomerPathElement("99")) + .Returns(customer); + + // Act + var thumbnailRequest = await sut.Parse(path); + + // Assert + thumbnailRequest.RoutePrefix.Should().Be("thumbs"); + thumbnailRequest.VersionedRoutePrefix.Should().Be("thumbs"); + thumbnailRequest.VersionPathValue.Should().BeNull(); + thumbnailRequest.CustomerPathValue.Should().Be("99"); + thumbnailRequest.Customer.Should().Be(customer); + thumbnailRequest.BasePath.Should().Be("/thumbs/99/1/"); + thumbnailRequest.Space.Should().Be(1); + thumbnailRequest.AssetPath.Should().Be("the-astronaut"); + thumbnailRequest.AssetId.Should().Be("the-astronaut"); + } [Fact] public async Task Parse_ImageRequest_WithCustomerName_ReturnsCorrectRequest() diff --git a/src/protagonist/DLCS.Web/Requests/AssetDelivery/AssetDeliveryPathParser.cs b/src/protagonist/DLCS.Web/Requests/AssetDelivery/AssetDeliveryPathParser.cs index fb72baa81..047193df7 100644 --- a/src/protagonist/DLCS.Web/Requests/AssetDelivery/AssetDeliveryPathParser.cs +++ b/src/protagonist/DLCS.Web/Requests/AssetDelivery/AssetDeliveryPathParser.cs @@ -27,6 +27,9 @@ public class AssetDeliveryPathParser : IAssetDeliveryPathParser { private readonly IPathCustomerRepository pathCustomerRepository; + // regex to clean query params and hashmark from asset id + private static readonly Regex AssetIdClean = new(@"[\?\#].*$", RegexOptions.Compiled); + // regex to match v1, v2 etc but not a v23 private static readonly Regex VersionRegex = new("^(v\\d)$", RegexOptions.Compiled); @@ -83,12 +86,11 @@ private async Task ParseBaseAssetRequest(string path, BaseAssetRequest request) request.CustomerPathValue = parts[defaultCustomerIndex + versionOffset]; var space = parts[defaultSpacesIndex + versionOffset]; request.Space = int.Parse(space); - request.AssetPath = string.Join("/", parts.Skip(3 + versionOffset)); + request.AssetPath = GetAssetPath(parts, versionOffset); request.AssetId = request.AssetPath.Split("/")[0]; request.BasePath = GenerateBasePath(request.RoutePrefix, request.CustomerPathValue, space, isVersioned, versionCandidate); - // TODO - should we verify Space exists here? request.Customer = await pathCustomerRepository.GetCustomerPathElement(request.CustomerPathValue); request.NormalisedBasePath = @@ -97,6 +99,12 @@ private async Task ParseBaseAssetRequest(string path, BaseAssetRequest request) request.NormalisedFullPath = string.Concat(request.NormalisedBasePath, request.AssetPath); } + private static string GetAssetPath(string[] parts, int versionOffset) + { + var assetPath = string.Join("/", parts.Skip(3 + versionOffset)); + return AssetIdClean.Replace(assetPath, string.Empty); + } + private static string GenerateBasePath(string route, string customer, string space, bool isVersioned, string? version) { From df74035965e3f75e4c7e4073a1ce1e8d60d9852d Mon Sep 17 00:00:00 2001 From: Donald Gray Date: Wed, 31 May 2023 11:34:25 +0100 Subject: [PATCH 2/5] Update GetDisplayUrl to take optional param to remove query string GetBaseUrl can now call GetDisplayUrl with new param. Added verifying tests --- .../Requests/HttpRequestXTests.cs | 197 +++++++++++++++++- .../DLCS.Web/Requests/HttpRequestX.cs | 36 +--- 2 files changed, 204 insertions(+), 29 deletions(-) diff --git a/src/protagonist/DLCS.Web.Tests/Requests/HttpRequestXTests.cs b/src/protagonist/DLCS.Web.Tests/Requests/HttpRequestXTests.cs index 11ce06557..e227e869f 100644 --- a/src/protagonist/DLCS.Web.Tests/Requests/HttpRequestXTests.cs +++ b/src/protagonist/DLCS.Web.Tests/Requests/HttpRequestXTests.cs @@ -21,7 +21,6 @@ public void GetFirstQueryParamValue_Finds_Single_Value() test.Should().Be("aaa"); } - [Fact] public void GetFirstQueryParamValue_Ignores_Further_Values() { @@ -36,7 +35,6 @@ public void GetFirstQueryParamValue_Ignores_Further_Values() test.Should().Be("aaa"); } - [Fact] public void GetFirstQueryParamValueAsInt_Finds_Int() { @@ -50,4 +48,199 @@ public void GetFirstQueryParamValueAsInt_Finds_Int() // assert test.Should().Be(12); } + + [Fact] + public void GetDisplayUrl_ReturnsFullUrl_WhenCalledWithDefaultParams() + { + // Arrange + var httpRequest = new DefaultHttpContext().Request; + httpRequest.Path = new PathString("/anything"); + httpRequest.QueryString = new QueryString("?foo=bar"); + httpRequest.Host = new HostString("test.example"); + httpRequest.Scheme = "https"; + + const string expected = "https://test.example?foo=bar"; + + // Act + var result = httpRequest.GetDisplayUrl(); + + // Assert + result.Should().Be(expected); + } + + [Fact] + public void GetDisplayUrl_WithPathBase_ReturnsFullUrl_WhenCalledWithDefaultParams() + { + // Arrange + var httpRequest = new DefaultHttpContext().Request; + httpRequest.Path = new PathString("/anything"); + httpRequest.QueryString = new QueryString("?foo=bar"); + httpRequest.Host = new HostString("test.example"); + httpRequest.PathBase = new PathString("/v2"); + httpRequest.Scheme = "https"; + + const string expected = "https://test.example/v2?foo=bar"; + + // Act + var result = httpRequest.GetDisplayUrl(); + + // Assert + result.Should().Be(expected); + } + + [Fact] + public void GetDisplayUrl_ReturnsFullUrl_WhenCalledWithPath() + { + // Arrange + var httpRequest = new DefaultHttpContext().Request; + httpRequest.Path = new PathString("/anything"); + httpRequest.QueryString = new QueryString("?foo=bar"); + httpRequest.Host = new HostString("test.example"); + httpRequest.Scheme = "https"; + + const string expected = "https://test.example/my-path/one/two?foo=bar"; + + // Act + var result = httpRequest.GetDisplayUrl("/my-path/one/two"); + + // Assert + result.Should().Be(expected); + } + + [Fact] + public void GetDisplayUrl_WithPathBase_ReturnsFullUrl_WhenCalledWithWithPath() + { + // Arrange + var httpRequest = new DefaultHttpContext().Request; + httpRequest.Path = new PathString("/anything"); + httpRequest.QueryString = new QueryString("?foo=bar"); + httpRequest.Host = new HostString("test.example"); + httpRequest.PathBase = new PathString("/v2"); + httpRequest.Scheme = "https"; + + const string expected = "https://test.example/v2/my-path/one/two?foo=bar"; + + // Act + var result = httpRequest.GetDisplayUrl("/my-path/one/two"); + + // Assert + result.Should().Be(expected); + } + + [Fact] + public void GetDisplayUrl_ReturnsFullUrl_WithoutQueryParam_WhenCalledWithDoNotIncludeQueryParams() + { + // Arrange + var httpRequest = new DefaultHttpContext().Request; + httpRequest.Path = new PathString("/anything"); + httpRequest.QueryString = new QueryString("?foo=bar"); + httpRequest.Host = new HostString("test.example"); + httpRequest.Scheme = "https"; + + const string expected = "https://test.example"; + + // Act + var result = httpRequest.GetDisplayUrl(includeQueryParams: false); + + // Assert + result.Should().Be(expected); + } + + [Fact] + public void GetDisplayUrl_WithPathBase_ReturnsFullUrl_WithoutQueryParam_WhenCalledWithDoNotIncludeQueryParams() + { + // Arrange + var httpRequest = new DefaultHttpContext().Request; + httpRequest.Path = new PathString("/anything"); + httpRequest.QueryString = new QueryString("?foo=bar"); + httpRequest.Host = new HostString("test.example"); + httpRequest.PathBase = new PathString("/v2"); + httpRequest.Scheme = "https"; + + const string expected = "https://test.example/v2"; + + // Act + var result = httpRequest.GetDisplayUrl(includeQueryParams: false); + + // Assert + result.Should().Be(expected); + } + + [Fact] + public void GetBaseUrl_ReturnsFullUrl_WithoutQueryParam() + { + // Arrange + var httpRequest = new DefaultHttpContext().Request; + httpRequest.Path = new PathString("/anything"); + httpRequest.QueryString = new QueryString("?foo=bar"); + httpRequest.Host = new HostString("test.example"); + httpRequest.Scheme = "https"; + + const string expected = "https://test.example"; + + // Act + var result = httpRequest.GetBaseUrl(); + + // Assert + result.Should().Be(expected); + } + + [Fact] + public void GetBaseUrl_WithPathBase_ReturnsFullUrl_WithoutQueryParam_WhenCalledWithDoNotIncludeQueryParams() + { + // Arrange + var httpRequest = new DefaultHttpContext().Request; + httpRequest.Path = new PathString("/anything"); + httpRequest.QueryString = new QueryString("?foo=bar"); + httpRequest.Host = new HostString("test.example"); + httpRequest.PathBase = new PathString("/v2"); + httpRequest.Scheme = "https"; + + const string expected = "https://test.example/v2"; + + // Act + var result = httpRequest.GetBaseUrl(); + + // Assert + result.Should().Be(expected); + } + + [Fact] + public void GetJsonLdId_Correct() + { + // Arrange + var httpRequest = new DefaultHttpContext().Request; + httpRequest.Path = new PathString("/anything"); + httpRequest.QueryString = new QueryString("?foo=bar"); + httpRequest.Host = new HostString("test.example"); + httpRequest.Scheme = "https"; + + const string expected = "https://test.example/anything"; + + // Act + var result = httpRequest.GetJsonLdId(); + + // Assert + result.Should().Be(expected); + } + + [Fact] + public void GetJsonLdId_WithPathBase_Correct() + { + // Arrange + var httpRequest = new DefaultHttpContext().Request; + httpRequest.Path = new PathString("/anything"); + httpRequest.QueryString = new QueryString("?foo=bar"); + httpRequest.Host = new HostString("test.example"); + httpRequest.PathBase = new PathString("/v2"); + httpRequest.Scheme = "https"; + + const string expected = "https://test.example/v2/anything"; + + // Act + var result = httpRequest.GetJsonLdId(); + + // Assert + result.Should().Be(expected); + } } \ No newline at end of file diff --git a/src/protagonist/DLCS.Web/Requests/HttpRequestX.cs b/src/protagonist/DLCS.Web/Requests/HttpRequestX.cs index 217f936d1..fff696443 100644 --- a/src/protagonist/DLCS.Web/Requests/HttpRequestX.cs +++ b/src/protagonist/DLCS.Web/Requests/HttpRequestX.cs @@ -13,16 +13,19 @@ public static class HttpRequestX /// /// HttpRequest to generate display URL for /// Path to append to URL + /// If true, query params are included in path. Else they are omitted /// Full URL, including scheme, host, pathBase, path and queryString /// /// based on Microsoft.AspNetCore.Http.Extensions.UriHelper.GetDisplayUrl(this HttpRequest request) /// - public static string GetDisplayUrl(this HttpRequest request, string? path = null) + public static string GetDisplayUrl(this HttpRequest request, string? path = null, bool includeQueryParams = true) { var host = request.Host.Value ?? string.Empty; var scheme = request.Scheme ?? string.Empty; var pathBase = request.PathBase.Value ?? string.Empty; - var queryString = request.QueryString.Value ?? string.Empty; + var queryString = includeQueryParams + ? request.QueryString.Value ?? string.Empty + : string.Empty; var pathElement = path ?? string.Empty; // PERF: Calculate string length to allocate correct buffer size for StringBuilder. @@ -38,39 +41,18 @@ public static string GetDisplayUrl(this HttpRequest request, string? path = null .Append(queryString) .ToString(); } - + /// /// Generate a display URL, deriving values from specified HttpRequest. Omitting path and query string /// - /// Similar to GetDisplayUrl but omits path and query string public static string GetBaseUrl(this HttpRequest request) - { - var host = request.Host.Value ?? string.Empty; - var scheme = request.Scheme ?? string.Empty; - var pathBase = request.PathBase.Value ?? string.Empty; - - // PERF: Calculate string length to allocate correct buffer size for StringBuilder. - var length = scheme.Length + SchemeDelimiter.Length + host.Length - + pathBase.Length; - - return new StringBuilder(length) - .Append(scheme) - .Append(SchemeDelimiter) - .Append(host) - .Append(pathBase) - .ToString(); - } + => request.GetDisplayUrl(path: null, includeQueryParams: false); /// /// Generate the "@id" property for a JSON-LD API response. /// - /// - /// - public static string GetJsonLdId(this HttpRequest request) - { - return GetDisplayUrl(request, request.Path); - } - + public static string GetJsonLdId(this HttpRequest request) => GetDisplayUrl(request, request.Path, false); + public static string? GetFirstQueryParamValue(this HttpRequest request, string paramName) { if (request.Query.ContainsKey(paramName)) From f38379e0ef4269b861c0f1605a5b84862c8a2865 Mon Sep 17 00:00:00 2001 From: Donald Gray Date: Wed, 31 May 2023 11:35:40 +0100 Subject: [PATCH 3/5] Update AssetPathGenerater to take optional param to include querystring --- .../ConfigDrivenAssetPathGeneratorTests.cs | 70 ++++--------------- .../ConfigDrivenAssetPathGenerator.cs | 20 +++--- .../DLCS.Web/Response/IAssetPathGenerator.cs | 44 ++++-------- 3 files changed, 34 insertions(+), 100 deletions(-) diff --git a/src/protagonist/DLCS.Web.Tests/Response/ConfigDrivenAssetPathGeneratorTests.cs b/src/protagonist/DLCS.Web.Tests/Response/ConfigDrivenAssetPathGeneratorTests.cs index 407b6b247..d29c4617c 100644 --- a/src/protagonist/DLCS.Web.Tests/Response/ConfigDrivenAssetPathGeneratorTests.cs +++ b/src/protagonist/DLCS.Web.Tests/Response/ConfigDrivenAssetPathGeneratorTests.cs @@ -1,4 +1,5 @@ -using System.Collections.Generic; +using System; +using System.Collections.Generic; using DLCS.Core; using DLCS.Model.PathElements; using DLCS.Web.Requests.AssetDelivery; @@ -94,10 +95,10 @@ public void GetFullPathForRequest_OverrideAvailable_ButIgnoredIfNativeFormatRequ [Theory] [InlineData("123")] [InlineData("test-customer")] - public void GetFullPathForRequest_PathGenerator_Default(string customerPathValue) + public void GetFullPathForRequest_Default_QueryParam(string customerPathValue) { // Arrange - var sut = GetSut("default.com"); + var sut = GetSut("default.com", req => req.QueryString = new QueryString("?foo=bar")); var request = new BaseAssetRequest { Customer = new CustomerPathElement(123, "test-customer"), @@ -108,17 +109,10 @@ public void GetFullPathForRequest_PathGenerator_Default(string customerPathValue RoutePrefix = "thumbs" }; - var expected = $"https://default.com/thumbs/{customerPathValue}/2000/not-asset"; + var expected = $"https://default.com/thumbs/{customerPathValue}/10/path/to/asset?foo=bar"; // Act - var actual = sut.GetFullPathForRequest(request, - (assetRequest, template) => - DlcsPathHelpers.GeneratePathFromTemplate( - template, - assetRequest.RoutePrefix, - assetRequest.CustomerPathValue, - space: "2000", - assetPath: "not-asset")); + var actual = sut.GetFullPathForRequest(request); // Assert actual.Should().Be(expected); @@ -127,10 +121,10 @@ public void GetFullPathForRequest_PathGenerator_Default(string customerPathValue [Theory] [InlineData("123")] [InlineData("test-customer")] - public void GetFullPathForRequest_PathGenerator_Override(string customerPathValue) + public void GetFullPathForRequest_Default_QueryParam_OmittedIfIncludeFalse(string customerPathValue) { // Arrange - var sut = GetSut("test.example.com"); + var sut = GetSut("default.com", req => req.QueryString = new QueryString("?foo=bar")); var request = new BaseAssetRequest { Customer = new CustomerPathElement(123, "test-customer"), @@ -141,57 +135,16 @@ public void GetFullPathForRequest_PathGenerator_Override(string customerPathValu RoutePrefix = "thumbs" }; - var expected = "https://test.example.com/thumbs/not-asset"; - - // Act - var actual = sut.GetFullPathForRequest(request, - (assetRequest, template) => - DlcsPathHelpers.GeneratePathFromTemplate( - template, - assetRequest.RoutePrefix, - assetRequest.CustomerPathValue, - space: "2000", - assetPath: "not-asset")); + var expected = $"https://default.com/thumbs/{customerPathValue}/10/path/to/asset"; - // Assert - actual.Should().Be(expected); - } - - [Theory] - [InlineData("123")] - [InlineData("test-customer")] - public void GetFullPathForRequest_PathGenerator_OverrideAvailable_ButIgnoredIfNativeFormatRequested(string customerPathValue) - { - // Arrange - var sut = GetSut("test.example.com"); - var request = new BaseAssetRequest - { - Customer = new CustomerPathElement(123, "test-customer"), - CustomerPathValue = customerPathValue, - Space = 10, - AssetPath = "path/to/asset", - BasePath = "thumbs/123/10", - RoutePrefix = "thumbs" - }; - - var expected = $"https://test.example.com/thumbs/{customerPathValue}/2000/not-asset"; - // Act - var actual = sut.GetFullPathForRequest(request, - (assetRequest, template) => - DlcsPathHelpers.GeneratePathFromTemplate( - template, - assetRequest.RoutePrefix, - assetRequest.CustomerPathValue, - space: "2000", - assetPath: "not-asset"), - true); + var actual = sut.GetFullPathForRequest(request, includeQueryParams: false); // Assert actual.Should().Be(expected); } - private ConfigDrivenAssetPathGenerator GetSut(string host) + private ConfigDrivenAssetPathGenerator GetSut(string host, Action requestModifier = null) { var context = new DefaultHttpContext(); var request = context.Request; @@ -199,6 +152,7 @@ private ConfigDrivenAssetPathGenerator GetSut(string host) A.CallTo(() => contextAccessor.HttpContext).Returns(context); request.Host = new HostString(host); request.Scheme = "https"; + requestModifier?.Invoke(request); var options = Options.Create(new PathTemplateOptions { diff --git a/src/protagonist/DLCS.Web/Response/ConfigDrivenAssetPathGenerator.cs b/src/protagonist/DLCS.Web/Response/ConfigDrivenAssetPathGenerator.cs index 1ebb6600c..3ccc49f30 100644 --- a/src/protagonist/DLCS.Web/Response/ConfigDrivenAssetPathGenerator.cs +++ b/src/protagonist/DLCS.Web/Response/ConfigDrivenAssetPathGenerator.cs @@ -28,24 +28,22 @@ public ConfigDrivenAssetPathGenerator( } public string GetRelativePathForRequest(IBasicPathElements assetRequest, bool useNativeFormat = false) - => GetForPath(assetRequest, false, useNativeFormat); + => GetForPath(assetRequest, false, useNativeFormat, false); - public string GetFullPathForRequest(IBasicPathElements assetRequest, bool useNativeFormat = false) - => GetForPath(assetRequest, true, useNativeFormat); + public string GetFullPathForRequest(IBasicPathElements assetRequest, bool useNativeFormat = false, + bool includeQueryParams = true) + => GetForPath(assetRequest, true, useNativeFormat, includeQueryParams); - public string GetFullPathForRequest(IBasicPathElements assetRequest, PathGenerator pathGenerator, - bool useNativeFormat = false) - => GetPathForRequestInternal(assetRequest, pathGenerator, true, useNativeFormat); - - private string GetForPath(IBasicPathElements assetRequest, bool fullRequest, bool useNativeFormat) + private string GetForPath(IBasicPathElements assetRequest, bool fullRequest, bool useNativeFormat, bool includeQueryParams) => GetPathForRequestInternal( assetRequest, (request, template) => GeneratePathFromTemplate(request, template), fullRequest, - useNativeFormat); + useNativeFormat, + includeQueryParams); private string GetPathForRequestInternal(IBasicPathElements assetRequest, PathGenerator pathGenerator, - bool fullRequest, bool useNativeFormat) + bool fullRequest, bool useNativeFormat, bool includeQueryParams) { var request = httpContextAccessor.HttpContext.Request; var host = request.Host.Value ?? string.Empty; @@ -55,7 +53,7 @@ private string GetPathForRequestInternal(IBasicPathElements assetRequest, PathGe var path = pathGenerator(assetRequest, template); - return fullRequest ? request.GetDisplayUrl(path) : path; + return fullRequest ? request.GetDisplayUrl(path, includeQueryParams) : path; } // Default path replacements diff --git a/src/protagonist/DLCS.Web/Response/IAssetPathGenerator.cs b/src/protagonist/DLCS.Web/Response/IAssetPathGenerator.cs index 1fe3529f4..d0d3cfee4 100644 --- a/src/protagonist/DLCS.Web/Response/IAssetPathGenerator.cs +++ b/src/protagonist/DLCS.Web/Response/IAssetPathGenerator.cs @@ -14,40 +14,22 @@ public interface IAssetPathGenerator { /// /// Generate path for specified excluding host. - /// Uses default template replacements. /// - /// - /// - /// + /// for current request + /// + /// If true, native DLCS path /{prefix}/{version}/{customer}/{space}/{assetPath} used. Else path can differ by path. + /// string GetRelativePathForRequest(IBasicPathElements assetRequest, bool useNativeFormat = false); - + /// /// Generate full path for specified , including host. /// Uses default template replacements. /// - /// - /// - /// - string GetFullPathForRequest(IBasicPathElements assetRequest, bool useNativeFormat = false); - - /// - /// Generate full path for specified , using provided delegate to generate - /// path element. - /// This can be useful for constructing paths that do not use the default path elements. - /// - string GetFullPathForRequest(IBasicPathElements assetRequest, PathGenerator pathGenerator, - bool useNativeFormat = false); -} - -// DONE -// GetFullyQualifiedId - versioned, always standard path, IIIFCanvasFactory.GetFullyQualifiedId ln 246 -// GetFullQualifiedImagePath - not versioned, always standard path, IIIFCanvasFactory.GetFullQualifiedImagePath ln 233 -// GetFullyQualifiedId - versioned, always standard path, GetManifestForAsset.GetFullyQualifiedId ln 124 - -// NOT DONE -// GetImageId - versioned, use replacements, GetImageInfoJson.GetImageId ln 161 -// GetFullImagePath - versioned, use replacements. ThumbsMiddleware.GetFullImagePath ln 183 - -/* - * Have I broken Thumbs handling by having 1 generic setting in parameterStore? - */ \ No newline at end of file + /// for current request + /// + /// If true, native DLCS path /{prefix}/{version}/{customer}/{space}/{assetPath} used. Else path can differ by path. + /// + /// If true, query params are included in path. Else they are omitted + string GetFullPathForRequest(IBasicPathElements assetRequest, bool useNativeFormat = false, + bool includeQueryParams = true); +} \ No newline at end of file From d4120e0e2876d1657e91757ee58dac2b517bf22b Mon Sep 17 00:00:00 2001 From: Donald Gray Date: Wed, 31 May 2023 11:36:30 +0100 Subject: [PATCH 4/5] Update thumbs redirect to exclude queryparam --- src/protagonist/Thumbs.Tests/Integration/InfoJsonTests.cs | 6 ++++++ src/protagonist/Thumbs/ThumbsMiddleware.cs | 2 +- 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/src/protagonist/Thumbs.Tests/Integration/InfoJsonTests.cs b/src/protagonist/Thumbs.Tests/Integration/InfoJsonTests.cs index 96370bd5f..169791904 100644 --- a/src/protagonist/Thumbs.Tests/Integration/InfoJsonTests.cs +++ b/src/protagonist/Thumbs.Tests/Integration/InfoJsonTests.cs @@ -32,16 +32,22 @@ public InfoJsonTests(ProtagonistAppFactory factory, StorageFixture stor [Theory] [InlineData("/thumbs/99/1/image", "http://localhost/thumbs/99/1/image/info.json")] + [InlineData("/thumbs/99/1/image?x=y", "http://localhost/thumbs/99/1/image/info.json")] [InlineData("/thumbs/99/1/image/", "http://localhost/thumbs/99/1/image/info.json")] [InlineData("/thumbs/test/1/image", "http://localhost/thumbs/test/1/image/info.json")] + [InlineData("/thumbs/test/1/image?x=y", "http://localhost/thumbs/test/1/image/info.json")] [InlineData("/thumbs/test/1/image/", "http://localhost/thumbs/test/1/image/info.json")] [InlineData("/thumbs/v2/99/1/image", "http://localhost/thumbs/v2/99/1/image/info.json")] + [InlineData("/thumbs/v2/99/1/image?x=y", "http://localhost/thumbs/v2/99/1/image/info.json")] [InlineData("/thumbs/v2/99/1/image/", "http://localhost/thumbs/v2/99/1/image/info.json")] [InlineData("/thumbs/v2/test/1/image", "http://localhost/thumbs/v2/test/1/image/info.json")] + [InlineData("/thumbs/v2/test/1/image?x=y", "http://localhost/thumbs/v2/test/1/image/info.json")] [InlineData("/thumbs/v2/test/1/image/", "http://localhost/thumbs/v2/test/1/image/info.json")] [InlineData("/thumbs/v3/99/1/image", "http://localhost/thumbs/99/1/image/info.json")] // Canonical version goes to canonical url + [InlineData("/thumbs/v3/99/1/image?x=y", "http://localhost/thumbs/99/1/image/info.json")] [InlineData("/thumbs/v3/99/1/image/", "http://localhost/thumbs/99/1/image/info.json")] [InlineData("/thumbs/v3/test/1/image", "http://localhost/thumbs/test/1/image/info.json")] + [InlineData("/thumbs/v3/test/1/image?x=y", "http://localhost/thumbs/test/1/image/info.json")] [InlineData("/thumbs/v3/test/1/image/", "http://localhost/thumbs/test/1/image/info.json")] public async Task Get_ImageRoot_RedirectsToInfoJson(string path, string expected) { diff --git a/src/protagonist/Thumbs/ThumbsMiddleware.cs b/src/protagonist/Thumbs/ThumbsMiddleware.cs index a9a458e6f..d5949d2db 100644 --- a/src/protagonist/Thumbs/ThumbsMiddleware.cs +++ b/src/protagonist/Thumbs/ThumbsMiddleware.cs @@ -191,7 +191,7 @@ private string GetFullImagePath(ImageAssetDeliveryRequest imageAssetDeliveryRequ baseRequest.VersionPathValue = null; } - return pathGenerator.GetFullPathForRequest(baseRequest); + return pathGenerator.GetFullPathForRequest(baseRequest, includeQueryParams: false); } private bool IsCanonical(Version requestedVersion) From 2b43383d24019c778a0149f8ab84e8fd1e5b27f0 Mon Sep 17 00:00:00 2001 From: Donald Gray Date: Wed, 31 May 2023 11:45:52 +0100 Subject: [PATCH 5/5] Add tests to confirm info.json image redirect --- .../Integration/ImageHandlingTests.cs | 45 ++++++++++++++----- 1 file changed, 33 insertions(+), 12 deletions(-) diff --git a/src/protagonist/Orchestrator.Tests/Integration/ImageHandlingTests.cs b/src/protagonist/Orchestrator.Tests/Integration/ImageHandlingTests.cs index 4201984e6..6dead582a 100644 --- a/src/protagonist/Orchestrator.Tests/Integration/ImageHandlingTests.cs +++ b/src/protagonist/Orchestrator.Tests/Integration/ImageHandlingTests.cs @@ -85,19 +85,24 @@ public async Task Options_Returns200_WithCorsHeaders(string path) } [Theory] - [InlineData("/iiif-img/2/1/image")] - [InlineData("/iiif-img/2/1/image/")] - [InlineData("/iiif-img/display-name/1/image")] - [InlineData("/iiif-img/display-name/1/image/")] - [InlineData("/iiif-img/v2/2/1/image")] - [InlineData("/iiif-img/v2/2/1/image/")] - [InlineData("/iiif-img/v2/display-name/1/image")] - [InlineData("/iiif-img/v2/display-name/1/image/")] - public async Task Get_ImageRoot_RedirectsToInfoJson(string path) + [InlineData("/iiif-img/2/1/image", "/iiif-img/2/1/image/info.json")] + [InlineData("/iiif-img/2/1/image/", "/iiif-img/2/1/image/info.json")] + [InlineData("/iiif-img/2/1/image?x=y", "/iiif-img/2/1/image/info.json")] + [InlineData("/iiif-img/2/1/image/?x=y", "/iiif-img/2/1/image/info.json")] + [InlineData("/iiif-img/display-name/1/image", "/iiif-img/display-name/1/image/info.json")] + [InlineData("/iiif-img/display-name/1/image/", "/iiif-img/display-name/1/image/info.json")] + [InlineData("/iiif-img/display-name/1/image?x=y", "/iiif-img/display-name/1/image/info.json")] + [InlineData("/iiif-img/display-name/1/image/?x=y", "/iiif-img/display-name/1/image/info.json")] + [InlineData("/iiif-img/v2/2/1/image", "/iiif-img/v2/2/1/image/info.json")] + [InlineData("/iiif-img/v2/2/1/image/", "/iiif-img/v2/2/1/image/info.json")] + [InlineData("/iiif-img/v2/2/1/image?x=y", "/iiif-img/v2/2/1/image/info.json")] + [InlineData("/iiif-img/v2/2/1/image/?x=y", "/iiif-img/v2/2/1/image/info.json")] + [InlineData("/iiif-img/v2/display-name/1/image", "/iiif-img/v2/display-name/1/image/info.json")] + [InlineData("/iiif-img/v2/display-name/1/image/", "/iiif-img/v2/display-name/1/image/info.json")] + [InlineData("/iiif-img/v2/display-name/1/image?x=y", "/iiif-img/v2/display-name/1/image/info.json")] + [InlineData("/iiif-img/v2/display-name/1/image/?x=y", "/iiif-img/v2/display-name/1/image/info.json")] + public async Task Get_ImageRoot_RedirectsToInfoJson(string path, string expected) { - // Arrange - var expected = path[^1] == '/' ? $"{path}info.json" : $"{path}/info.json"; - // Act var response = await httpClient.GetAsync(path); @@ -109,16 +114,28 @@ public async Task Get_ImageRoot_RedirectsToInfoJson(string path) [Theory] [InlineData("/iiif-img/2/1/image", "/const_value/2/image/info.json")] [InlineData("/iiif-img/2/1/image/", "/const_value/2/image/info.json")] + [InlineData("/iiif-img/2/1/image?x=y", "/const_value/2/image/info.json")] + [InlineData("/iiif-img/2/1/image/?x=y", "/const_value/2/image/info.json")] [InlineData("/iiif-img/display-name/1/image", "/const_value/display-name/image/info.json")] [InlineData("/iiif-img/display-name/1/image/", "/const_value/display-name/image/info.json")] + [InlineData("/iiif-img/display-name/1/image?x=y", "/const_value/display-name/image/info.json")] + [InlineData("/iiif-img/display-name/1/image/?x=y", "/const_value/display-name/image/info.json")] [InlineData("/iiif-img/v2/2/1/image", "/const_value/v2/2/image/info.json")] [InlineData("/iiif-img/v2/2/1/image/", "/const_value/v2/2/image/info.json")] + [InlineData("/iiif-img/v2/2/1/image?x=y", "/const_value/v2/2/image/info.json")] + [InlineData("/iiif-img/v2/2/1/image/?x=y", "/const_value/v2/2/image/info.json")] [InlineData("/iiif-img/v3/2/1/image", "/const_value/2/image/info.json")] [InlineData("/iiif-img/v3/2/1/image/", "/const_value/2/image/info.json")] + [InlineData("/iiif-img/v3/2/1/image?x=y", "/const_value/2/image/info.json")] + [InlineData("/iiif-img/v3/2/1/image/?x=y", "/const_value/2/image/info.json")] [InlineData("/iiif-img/v2/display-name/1/image", "/const_value/v2/display-name/image/info.json")] [InlineData("/iiif-img/v2/display-name/1/image/", "/const_value/v2/display-name/image/info.json")] + [InlineData("/iiif-img/v2/display-name/1/image?x=y", "/const_value/v2/display-name/image/info.json")] + [InlineData("/iiif-img/v2/display-name/1/image/?x=y", "/const_value/v2/display-name/image/info.json")] [InlineData("/iiif-img/v3/display-name/1/image", "/const_value/display-name/image/info.json")] [InlineData("/iiif-img/v3/display-name/1/image/", "/const_value/display-name/image/info.json")] + [InlineData("/iiif-img/v3/display-name/1/image?x=y", "/const_value/display-name/image/info.json")] + [InlineData("/iiif-img/v3/display-name/1/image/?x=y", "/const_value/display-name/image/info.json")] public async Task Get_ImageRoot_RedirectsToInfoJson_CustomPathValues(string path, string expected) { // Act @@ -133,9 +150,13 @@ public async Task Get_ImageRoot_RedirectsToInfoJson_CustomPathValues(string path [Theory] [InlineData("/iiif-img/v3/2/1/image", "/iiif-img/2/1/image/info.json")] + [InlineData("/iiif-img/v3/2/1/image?x=y", "/iiif-img/2/1/image/info.json")] [InlineData("/iiif-img/v3/2/1/image/", "/iiif-img/2/1/image/info.json")] + [InlineData("/iiif-img/v3/2/1/image/?x=y", "/iiif-img/2/1/image/info.json")] [InlineData("/iiif-img/v3/display-name/1/image", "/iiif-img/display-name/1/image/info.json")] + [InlineData("/iiif-img/v3/display-name/1/image?x=y", "/iiif-img/display-name/1/image/info.json")] [InlineData("/iiif-img/v3/display-name/1/image/", "/iiif-img/display-name/1/image/info.json")] + [InlineData("/iiif-img/v3/display-name/1/image/?x=y", "/iiif-img/display-name/1/image/info.json")] public async Task Get_ImageRoot_RedirectsToCanonicalInfoJson_IfRequestingCanonicalVersion(string path, string expected) { // Act