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

Update main with bugfixes from develop #857

Merged
merged 19 commits into from
May 28, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
8c91d9f
Update DeliveryChannelPolicyDataValidator rules to disallow '!w,' and…
griffri May 9, 2024
57148b8
Refactor IsValidThumbnailParameter pattern matching
griffri May 9, 2024
dfcc225
Return 404 on retrieving a single asset manifest if image delivery ch…
griffri May 10, 2024
136cff0
Exclude painting annotation body if image service unavailable
griffri May 10, 2024
1611cf6
Merge pull request #844 from dlcs/fix/disallow_policy_thumbparameters
griffri May 10, 2024
bbcacf9
Add .WithTestDeliveryChannel() helper
griffri May 10, 2024
a602a45
Add HasAnyDeliveryChannel helper to AssetDeliveryChannels
griffri May 10, 2024
7aab5bf
Update GetManifestForAsset to return 404 if asset is null, use HasAny…
griffri May 10, 2024
937e97f
Add HasAnyDeliveryChannel tests
griffri May 10, 2024
a915005
Add image delivery channel to asset in Get_V3ManifestForImage_Returns…
griffri May 10, 2024
8a2be53
Update manifest handling tests
griffri May 10, 2024
6f60a04
Refactor HasAnyDeliveryChannel()
griffri May 20, 2024
07101bd
Use HasAnyDeliveryChannel() to determine if an asset has a specific d…
griffri May 20, 2024
8eb4fc1
Merge pull request #837 from dlcs/fix/manifest_shows_correct_services
griffri May 21, 2024
89c2046
Use a unique directory for working files when ingesting an image
griffri May 22, 2024
1bde5b4
Set IngestionContext IngestId on creation
griffri May 23, 2024
2857974
Separate customerId from ingestId in source folder path
griffri May 23, 2024
ba6ec61
Handle backslash directory separators in image ingestion tests
griffri May 24, 2024
2e52754
Merge pull request #851 from dlcs/fix/concurrent_ingest
griffri May 28, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -24,12 +24,14 @@ public DeliveryChannelPolicyDataValidatorTests()
.Returns(fakedAvPolicies);
sut = new DeliveryChannelPolicyDataValidator(avChannelPolicyOptionsRepository);
}

[Theory]
[InlineData("[\"400,\",\"200,\",\"100,\"]")]
[InlineData("[\"!400,\",\"!200,\",\"!100,\"]")]
[InlineData("[\",400\",\",200\",\",100\"]")]
[InlineData("[\"!,400\",\"!,200\",\"!,100\"]")]
[InlineData("[\"^400,\",\"^200,\",\"^100,\"]")]
[InlineData("[\"^,400\",\"^,200\",\"^,100\"]")]
[InlineData("[\"!400,400\",\"!200,200\",\"!100,100\"]")]
[InlineData("[\"^!400,400\",\"^!200,200\",\"^!100,100\"]")]
public async Task PolicyDataValidator_ReturnsTrue_ForValidThumbParameters(string policyData)
{
// Arrange and Act
Expand Down Expand Up @@ -88,6 +90,8 @@ public async Task PolicyDataValidator_ReturnsFalse_ForEmptyThumbSizes(string pol
[InlineData("[\"^pct:41.6,7.5\"]")]
[InlineData("[\"10,50\"]")]
[InlineData("[\",\"]")]
[InlineData("[\"!10,\"]")]
[InlineData("[\"!,10\"]")]
public async Task PolicyDataValidator_ReturnsFalse_ForInvalidThumbParameters(string policyData)
{
// Arrange and Act
Expand Down
21 changes: 0 additions & 21 deletions src/protagonist/API.Tests/Integration/DeliveryChannelTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -785,42 +785,21 @@ public async Task Get_DeliveryChannelPolicyCollection_400_IfChannelInvalid()
@"[\""400,\"",\""200,\"",\""100,\""]"
},
new object[]
{
"my-thumbs-policy-1-b",
@"[\""!400,\"",\""!200,\"",\""!100,\""]"
},
new object[]
{
"my-thumbs-policy-1-c",
@"[\"",400\"",\"",200\"",\"",100\""]"
},
new object[]
{
"my-thumbs-policy-1-d",
@"[\""!,400\"",\""!,200\"",\""!,100\""]"

},
new object[]
{
"my-thumbs-policy-1-e",
@"[\""^400,\"",\""^200,\"",\""^100,\""]"
},
new object[]
{
"my-thumbs-policy-1-f",
@"[\""^!400,\"",\""^!200,\"",\""^!100,\""]"
},
new object[]
{
"my-thumbs-policy-1-g",
@"[\""^,400\"",\""^,200\"",\""^,100\""]"
},
new object[]
{
"my-thumbs-policy-1-h",
@"[\""^!,400\"",\""^!,200\"",\""^!,100\""]"
},
new object[]
{
"my-thumbs-policy-1-i",
@"[\""!400,400\"",\""!200,200\"",\""!100,100\""]"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -68,10 +68,15 @@ private bool ValidateThumbnailPolicyData(string policyDataJson)
return true;
}

private bool IsValidThumbnailParameter(SizeParameter param)
=> !(param.Max || param.PercentScale.HasValue ||
(param.Width.HasValue && param.Height.HasValue && !param.Confined) ||
(!param.Width.HasValue && !param.Height.HasValue));
private bool IsValidThumbnailParameter(SizeParameter param) => param switch
{
{ Max: true } => false,
{ PercentScale: not null } => false,
{ Confined: false, Width: not null, Height: not null } => false,
{ Confined: true } and ({ Width: null } or { Height : null }) => false,
{ Width: null, Height: null } => false,
_ => true,
};

private async Task<bool> ValidateTimeBasedPolicyData(string policyDataJson)
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -108,4 +108,35 @@ public void HasSingleDeliveryChannel_True(string channel)

asset.HasSingleDeliveryChannel(channel).Should().BeTrue();
}

[Theory]
[InlineData(AssetDeliveryChannels.File)]
[InlineData(AssetDeliveryChannels.Image)]
[InlineData(AssetDeliveryChannels.Timebased)]
public void HasAnyDeliveryChannel_True(string channel)
{
var asset = new Asset { ImageDeliveryChannels = new List<ImageDeliveryChannel>()
{
new()
{
Channel = channel,
}
} };

asset.HasAnyDeliveryChannel(AssetDeliveryChannels.All).Should().BeTrue();
}

[Fact]
public void HasAnyDeliveryChannel_False()
{
var asset = new Asset { ImageDeliveryChannels = new List<ImageDeliveryChannel>()
{
new()
{
Channel = "not-a-channel",
}
} };

asset.HasAnyDeliveryChannel(AssetDeliveryChannels.All).Should().BeFalse();
}
}
24 changes: 15 additions & 9 deletions src/protagonist/DLCS.Model/Assets/AssetDeliveryChannels.cs
Original file line number Diff line number Diff line change
Expand Up @@ -21,22 +21,28 @@ public static class AssetDeliveryChannels
/// All possible delivery channels as a comma-delimited string
/// </summary>
public static readonly string AllString = string.Join(',', All);

/// <summary>
/// Check if asset has specified deliveryChannel
/// Checks if an asset has any delivery channel specified in a list
/// </summary>
public static bool HasDeliveryChannel(this Asset asset, string deliveryChannel)
public static bool HasAnyDeliveryChannel(this Asset asset, params string[] deliveryChannels)
{
if (asset.ImageDeliveryChannels.IsNullOrEmpty()) return false;
if (!All.Contains(deliveryChannel))
if (asset.ImageDeliveryChannels.IsNullOrEmpty() || deliveryChannels.IsNullOrEmpty()) return false;
if (deliveryChannels.Any(dc => !All.Contains(dc)))
{
throw new ArgumentOutOfRangeException(nameof(deliveryChannel), deliveryChannel,
throw new ArgumentOutOfRangeException(nameof(deliveryChannels), deliveryChannels,
$"Acceptable delivery-channels are: {AllString}");
}

return asset.ImageDeliveryChannels.Any(i => i.Channel == deliveryChannel);
return asset.ImageDeliveryChannels.Any(dc => deliveryChannels.Contains(dc.Channel));
}


/// <summary>
/// Check if asset has specified deliveryChannel
/// </summary>
public static bool HasDeliveryChannel(this Asset asset, string deliveryChannel)
=> HasAnyDeliveryChannel(asset, deliveryChannel);

/// <summary>
/// Checks if asset has specified deliveryChannel only (e.g. 1 channel and it matches specified value
/// </summary>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,8 @@ private ImageIngestPostProcessing GetSut(bool orchestrateAfterIngest)
ImageIngest = new ImageIngestSettings
{
SourceTemplate = "{customer}_{space}_{image}",
OrchestrateImageAfterIngest = orchestrateAfterIngest
OrchestrateImageAfterIngest = orchestrateAfterIngest,
ScratchRoot = "scratch/"
},
};
var optionsMonitor = OptionsHelpers.GetOptionsMonitor(engineSettings);
Expand Down Expand Up @@ -132,11 +133,15 @@ public async Task PostIngest_DeletesWorkingFolder_IfSuccessOrFailure(bool succes
var assetId = AssetId.FromString("2/1/avalon");
var asset = new Asset(assetId);
var sut = GetSut(true);

var context = new IngestionContext(asset);

// Act
await sut.CompleteIngestion(new IngestionContext(asset), success);
await sut.CompleteIngestion(context, success);

// Assert
fileSystem.DeletedDirectories.Should().Contain("2_1_avalon");
var expectedWorkingFolder = $"scratch/{context.IngestId}/";
fileSystem.DeletedDirectories.Should().Contain(d =>
// Account for backslashes being used to separate directories when running on Windows
d.Replace("\\", "/") == expectedWorkingFolder);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -91,10 +91,13 @@ public async Task ProcessImage_ChangesFileSavedLocationBasedOnImageIdWithBracket
await sut.ProcessImage(context);

// Assert
A.CallTo(() => fileSystem.CreateDirectory( "scratch/1/2/some_id_/output")).MustHaveHappenedOnceExactly();
var expectedDirectory = $"scratch/{context.IngestId}/1/2/some_id_/output";
A.CallTo(() => fileSystem.CreateDirectory(
// Account for backslashes being used to separate directories when running on Windows
A<string>.That.Matches(x => x.Replace("\\", "/") == expectedDirectory ))).MustHaveHappenedOnceExactly();
A.CallTo(() => fileSystem.DeleteDirectory(A<string>._, true, true)).MustHaveHappenedTwiceExactly();
}

[Fact]
public async Task ProcessImage_False_IfImageProcessorCallFails()
{
Expand Down Expand Up @@ -178,13 +181,13 @@ public async Task ProcessImage_UpdatesAssetDimensions()
{
// Arrange
var imageProcessorResponse = new AppetiserResponseModel();

var context = IngestionContextFactory.GetIngestionContext("/1/2/test");

A.CallTo(() => appetiserClient.GenerateJP2(A<IngestionContext>._, A<AssetId>._, A<CancellationToken>._))
.Returns(Task.FromResult(imageProcessorResponse as IAppetiserResponse));
A.CallTo(() => appetiserClient.GetJP2FilePath(A<AssetId>._, A<bool>._))
.Returns("scratch/1/2/test/outputtest.jp2");

var context = IngestionContextFactory.GetIngestionContext("/1/2/test");
A.CallTo(() => appetiserClient.GetJP2FilePath(A<AssetId>._, context.IngestId, A<bool>._))
.Returns($"scratch/{context.IngestId}/1/2/test/outputtest.jp2");

context.AssetFromOrigin.CustomerOriginStrategy = new CustomerOriginStrategy
{
Optimised = optimised,
Expand All @@ -201,7 +204,7 @@ public async Task ProcessImage_UpdatesAssetDimensions()
// Assert
bucketWriter
.ShouldHaveKey("1/2/test")
.WithFilePath("scratch/1/2/test/outputtest.jp2")
.WithFilePath($"scratch/{context.IngestId}/1/2/test/outputtest.jp2")
.WithContentType("image/jp2");
context.ImageLocation.S3.Should().Be(expected);
context.StoredObjects.Should().NotBeEmpty();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,8 @@ public ImageIngesterWorkerTests()
ImageIngest = new ImageIngestSettings
{
SourceTemplate = "{root}",
OrchestrateImageAfterIngest = true
OrchestrateImageAfterIngest = true,
ScratchRoot = "scratch/"
},
};
var optionsMonitor = OptionsHelpers.GetOptionsMonitor(engineSettings);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ private bool ShouldOrchestrate(int customerId)

private void DeleteWorkingFolder(IngestionContext ingestionContext)
{
var sourceTemplate = ImageIngestionHelpers.GetSourceFolder(ingestionContext.Asset, engineSettings);
var sourceTemplate = ImageIngestionHelpers.GetWorkingFolder(ingestionContext.IngestId, engineSettings.ImageIngest);
fileSystem.DeleteDirectory(sourceTemplate, true);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ public class ImageIngesterWorker : IAssetIngesterWorker, IAssetIngesterPostProce
{
bool ingestSuccess;
var asset = ingestionContext.Asset;
var sourceTemplate = ImageIngestionHelpers.GetSourceFolder(asset, engineSettings);
var sourceTemplate = ImageIngestionHelpers.GetSourceFolder(ingestionContext, engineSettings);

try
{
Expand Down
20 changes: 14 additions & 6 deletions src/protagonist/Engine/Ingest/Image/ImageIngestionHelpers.cs
Original file line number Diff line number Diff line change
Expand Up @@ -7,19 +7,27 @@ namespace Engine.Ingest.Image;

internal static class ImageIngestionHelpers
{
/// <summary>
/// Get the top level working directory for an ingest request
/// </summary>
public static string GetWorkingFolder(string ingestId, ImageIngestSettings imageIngestSettings, bool forImageProcessor = false)
{
return $"{Path.Combine(imageIngestSettings.GetRoot(forImageProcessor), ingestId)}{Path.DirectorySeparatorChar}";
}

/// <summary>
/// Get folder location where working assets are to be saved to
/// </summary>
public static string GetSourceFolder(Asset asset, EngineSettings engineSettings)
public static string GetSourceFolder(IngestionContext ingestionContext, EngineSettings engineSettings)
{
var imageIngest = engineSettings.ImageIngest;
var root = imageIngest.GetRoot();
var workingFolder = GetWorkingFolder(ingestionContext.IngestId, imageIngest);

// source is the main folder for storing downloaded image
var assetId = new AssetId(asset.Id.Customer, asset.Id.Space,
asset.Id.Asset.Replace("(", imageIngest.OpenBracketReplacement)
var assetId = new AssetId(ingestionContext.AssetId.Customer, ingestionContext.AssetId.Space,
ingestionContext.AssetId.Asset.Replace("(", imageIngest.OpenBracketReplacement)
.Replace(")", imageIngest.CloseBracketReplacement));
var source = TemplatedFolders.GenerateFolderTemplate(imageIngest.SourceTemplate, assetId, root: root);
var source = TemplatedFolders.GenerateFolderTemplate(imageIngest.SourceTemplate, assetId, root: workingFolder);
return source;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ public class AppetiserClient : IAppetiserClient

public async Task<IAppetiserResponse> GenerateJP2(
IngestionContext context,
AssetId modifiedAssetId,
AssetId modifiedAssetId,
CancellationToken cancellationToken = default)
{
var requestModel = CreateModel(context, modifiedAssetId);
Expand Down Expand Up @@ -58,7 +58,7 @@ private AppetiserRequestModel CreateModel(IngestionContext context, AssetId modi
{
var requestModel = new AppetiserRequestModel
{
Destination = GetJP2FilePath(modifiedAssetId, true),
Destination = GetJP2FilePath(modifiedAssetId, context.IngestId, true),
Operation = "image-only",
Optimisation = "kdu_max",
Origin = context.Asset.Origin,
Expand All @@ -76,23 +76,24 @@ private string GetRelativeLocationOnDisk(IngestionContext context, AssetId modif
var extension = assetOnDisk.EverythingAfterLast('.');

// this is to get it working nice locally as appetiser/tizer root needs to be unix + relative to it
var imageProcessorRoot = engineSettings.ImageIngest.GetRoot(true);

var imageProcessorRoot = ImageIngestionHelpers.GetWorkingFolder(context.IngestId, engineSettings.ImageIngest, true);
var unixPath = TemplatedFolders.GenerateTemplateForUnix(engineSettings.ImageIngest.SourceTemplate, modifiedAssetId,
root: imageProcessorRoot);

unixPath += $"/{modifiedAssetId.Asset}.{extension}";
return unixPath;
}

public string GetJP2FilePath(AssetId assetId, bool forImageProcessor)
public string GetJP2FilePath(AssetId assetId, string ingestId, bool forImageProcessor)
{
// Appetiser/Tizer want unix paths relative to mount share.
// This logic allows handling when running locally on win/unix and when deployed to unix
var destFolder = forImageProcessor
? TemplatedFolders.GenerateTemplateForUnix(engineSettings.ImageIngest.DestinationTemplate,
assetId, root: engineSettings.ImageIngest.GetRoot(true))
assetId, root: ImageIngestionHelpers.GetWorkingFolder(ingestId, engineSettings.ImageIngest, true))
: TemplatedFolders.GenerateFolderTemplate(engineSettings.ImageIngest.DestinationTemplate,
assetId, root: engineSettings.ImageIngest.GetRoot());
assetId, root: ImageIngestionHelpers.GetWorkingFolder(ingestId, engineSettings.ImageIngest));

return $"{destFolder}{assetId.Asset}.jp2";
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,8 @@ public interface IAppetiserClient
/// Retrieves a JP2 filepath for an image
/// </summary>
/// <param name="assetId">The asset id used to retrieve the JP2 filepath</param>
/// <param name="ingestId">The id for the ingest operation associated with this image</param>
/// <param name="forImageProcessor">Whether this is for the image processor or not</param>
/// <returns></returns>
public string GetJP2FilePath(AssetId assetId, bool forImageProcessor);
public string GetJP2FilePath(AssetId assetId, string ingestId, bool forImageProcessor);
}
Original file line number Diff line number Diff line change
Expand Up @@ -53,11 +53,11 @@ public async Task<bool> ProcessImage(IngestionContext context)
var modifiedAssetId = new AssetId(context.AssetId.Customer, context.AssetId.Space,
context.AssetId.Asset.Replace("(", engineSettings.ImageIngest.OpenBracketReplacement)
.Replace(")", engineSettings.ImageIngest.CloseBracketReplacement));
var (dest, thumb) = CreateRequiredFolders(modifiedAssetId);
var (dest, thumb) = CreateRequiredFolders(modifiedAssetId, context.IngestId);

try
{
var flags = new ImageProcessorFlags(context, appetiserClient.GetJP2FilePath(modifiedAssetId, false));
var flags = new ImageProcessorFlags(context, appetiserClient.GetJP2FilePath(modifiedAssetId, context.IngestId, false));
logger.LogDebug("Got flags '{@Flags}' for {AssetId}", flags, context.AssetId);
var responseModel = await appetiserClient.GenerateJP2(context, modifiedAssetId);

Expand Down Expand Up @@ -89,16 +89,16 @@ public async Task<bool> ProcessImage(IngestionContext context)
}
}

private (string dest, string thumb) CreateRequiredFolders(AssetId assetId)
private (string dest, string thumb) CreateRequiredFolders(AssetId assetId, string ingestId)
{
var imageIngest = engineSettings.ImageIngest;
var root = imageIngest.GetRoot();
var workingFolder = ImageIngestionHelpers.GetWorkingFolder(ingestId, imageIngest);

// dest is the folder where appetiser will copy output to
var dest = TemplatedFolders.GenerateFolderTemplate(imageIngest.DestinationTemplate, assetId, root: root);
var dest = TemplatedFolders.GenerateFolderTemplate(imageIngest.DestinationTemplate, assetId, root: workingFolder);

// thumb is the folder where generated thumbnails will be output to
var thumb = TemplatedFolders.GenerateFolderTemplate(imageIngest.ThumbsTemplate, assetId, root: root);
var thumb = TemplatedFolders.GenerateFolderTemplate(imageIngest.ThumbsTemplate, assetId, root: workingFolder);

fileSystem.CreateDirectory(dest);
fileSystem.CreateDirectory(thumb);
Expand Down
Loading
Loading