From a4ac4d849afbf16e92cc79fb0d9da5de5083c8c8 Mon Sep 17 00:00:00 2001 From: John Lambert Date: Thu, 5 Dec 2024 17:39:07 -0500 Subject: [PATCH] Resolve Damien's unresolved comments (#562) * Resolve reviewer comments * Update from reviewer (verbal) comments. --- src/Serval/src/Serval.Client/Client.g.cs | 32 +++++++++---------- .../Consumers/GetCorpusConsumer.cs | 15 ++++++--- .../Contracts/CorpusFileDto.cs | 2 +- .../Controllers/CorporaController.cs | 12 +++++-- .../src/Serval.DataFiles/Models/CorpusFile.cs | 2 +- .../Services/CorpusService.cs | 19 ++++++----- .../Services/IDataFileService.cs | 1 + .../Services/EntityServiceBase.cs | 9 ++++++ .../Consumers/CorpusUpdatedConsumer.cs | 2 +- .../TranslationEnginesController.cs | 2 +- .../TranslationEngineTests.cs | 8 ++--- .../Services/CorpusServiceTests.cs | 4 +-- 12 files changed, 67 insertions(+), 41 deletions(-) diff --git a/src/Serval/src/Serval.Client/Client.g.cs b/src/Serval/src/Serval.Client/Client.g.cs index b0edfda4..91631ede 100644 --- a/src/Serval/src/Serval.Client/Client.g.cs +++ b/src/Serval/src/Serval.Client/Client.g.cs @@ -7073,15 +7073,28 @@ public partial class Corpus [System.CodeDom.Compiler.GeneratedCode("NJsonSchema", "14.1.0.0 (NJsonSchema v11.0.2.0 (Newtonsoft.Json v13.0.0.0))")] public partial class CorpusFile { - [Newtonsoft.Json.JsonProperty("fileId", Required = Newtonsoft.Json.Required.Always)] - [System.ComponentModel.DataAnnotations.Required(AllowEmptyStrings = true)] - public string FileId { get; set; } = default!; + [Newtonsoft.Json.JsonProperty("file", Required = Newtonsoft.Json.Required.Always)] + [System.ComponentModel.DataAnnotations.Required] + public ResourceLink File { get; set; } = new ResourceLink(); [Newtonsoft.Json.JsonProperty("textId", Required = Newtonsoft.Json.Required.Default, NullValueHandling = Newtonsoft.Json.NullValueHandling.Ignore)] public string? TextId { get; set; } = default!; } + [System.CodeDom.Compiler.GeneratedCode("NJsonSchema", "14.1.0.0 (NJsonSchema v11.0.2.0 (Newtonsoft.Json v13.0.0.0))")] + public partial class ResourceLink + { + [Newtonsoft.Json.JsonProperty("id", Required = Newtonsoft.Json.Required.Always)] + [System.ComponentModel.DataAnnotations.Required(AllowEmptyStrings = true)] + public string Id { get; set; } = default!; + + [Newtonsoft.Json.JsonProperty("url", Required = Newtonsoft.Json.Required.Always)] + [System.ComponentModel.DataAnnotations.Required(AllowEmptyStrings = true)] + public string Url { get; set; } = default!; + + } + [System.CodeDom.Compiler.GeneratedCode("NJsonSchema", "14.1.0.0 (NJsonSchema v11.0.2.0 (Newtonsoft.Json v13.0.0.0))")] public partial class CorpusConfig { @@ -7408,19 +7421,6 @@ public partial class TranslationCorpus } - [System.CodeDom.Compiler.GeneratedCode("NJsonSchema", "14.1.0.0 (NJsonSchema v11.0.2.0 (Newtonsoft.Json v13.0.0.0))")] - public partial class ResourceLink - { - [Newtonsoft.Json.JsonProperty("id", Required = Newtonsoft.Json.Required.Always)] - [System.ComponentModel.DataAnnotations.Required(AllowEmptyStrings = true)] - public string Id { get; set; } = default!; - - [Newtonsoft.Json.JsonProperty("url", Required = Newtonsoft.Json.Required.Always)] - [System.ComponentModel.DataAnnotations.Required(AllowEmptyStrings = true)] - public string Url { get; set; } = default!; - - } - [System.CodeDom.Compiler.GeneratedCode("NJsonSchema", "14.1.0.0 (NJsonSchema v11.0.2.0 (Newtonsoft.Json v13.0.0.0))")] public partial class TranslationCorpusFile { diff --git a/src/Serval/src/Serval.DataFiles/Consumers/GetCorpusConsumer.cs b/src/Serval/src/Serval.DataFiles/Consumers/GetCorpusConsumer.cs index 02e7d3c3..712f155c 100644 --- a/src/Serval/src/Serval.DataFiles/Consumers/GetCorpusConsumer.cs +++ b/src/Serval/src/Serval.DataFiles/Consumers/GetCorpusConsumer.cs @@ -14,19 +14,24 @@ public async Task Consume(ConsumeContext context) context.Message.Owner, context.CancellationToken ); + IEnumerable corpusFileIds = corpus.Files.Select(f => f.FileRef); + IDictionary corpusDataFilesDict = ( + await _dataFileService.GetAllAsync(corpusFileIds, context.CancellationToken) + ).ToDictionary(f => f.Id); + await context.RespondAsync( new CorpusResult { CorpusId = corpus.Id, Name = corpus.Name, Language = corpus.Language, - Files = await Task.WhenAll( - corpus.Files.Select(async f => new CorpusFileResult + Files = corpus + .Files.Select(f => new CorpusFileResult { - TextId = f.TextId!, - File = Map(await _dataFileService.GetAsync(f.FileId)) + TextId = f.TextId ?? corpusDataFilesDict[f.FileRef].Name, + File = Map(corpusDataFilesDict[f.FileRef]) }) - ) + .ToList() } ); } diff --git a/src/Serval/src/Serval.DataFiles/Contracts/CorpusFileDto.cs b/src/Serval/src/Serval.DataFiles/Contracts/CorpusFileDto.cs index 0efeaac9..9fda28fd 100644 --- a/src/Serval/src/Serval.DataFiles/Contracts/CorpusFileDto.cs +++ b/src/Serval/src/Serval.DataFiles/Contracts/CorpusFileDto.cs @@ -2,6 +2,6 @@ namespace Serval.DataFiles.Contracts; public record CorpusFileDto { - public required string FileId { get; init; } + public required ResourceLinkDto File { get; init; } public string? TextId { get; init; } } diff --git a/src/Serval/src/Serval.DataFiles/Controllers/CorporaController.cs b/src/Serval/src/Serval.DataFiles/Controllers/CorporaController.cs index c30a5ed3..546eed93 100644 --- a/src/Serval/src/Serval.DataFiles/Controllers/CorporaController.cs +++ b/src/Serval/src/Serval.DataFiles/Controllers/CorporaController.cs @@ -177,7 +177,7 @@ CancellationToken cancellationToken DataFile? dataFile = await _dataFileService.GetAsync(file.FileId, cancellationToken); if (dataFile == null) throw new InvalidOperationException($"DataFile with id {file.FileId} does not exist."); - dataFiles.Add(new CorpusFile { FileId = file.FileId, TextId = file.TextId }); + dataFiles.Add(new CorpusFile { FileRef = file.FileId, TextId = file.TextId }); } return dataFiles; } @@ -197,6 +197,14 @@ private CorpusDto Map(Corpus source) private CorpusFileDto Map(CorpusFile source) { - return new CorpusFileDto { FileId = source.FileId, TextId = source.TextId }; + return new CorpusFileDto + { + File = new ResourceLinkDto + { + Id = source.FileRef, + Url = _urlService.GetUrl(Endpoints.GetDataFile, new { id = source.FileRef }) + }, + TextId = source.TextId + }; } } diff --git a/src/Serval/src/Serval.DataFiles/Models/CorpusFile.cs b/src/Serval/src/Serval.DataFiles/Models/CorpusFile.cs index 277c9ccd..a1b0ab84 100644 --- a/src/Serval/src/Serval.DataFiles/Models/CorpusFile.cs +++ b/src/Serval/src/Serval.DataFiles/Models/CorpusFile.cs @@ -2,6 +2,6 @@ namespace Serval.DataFiles.Models; public record CorpusFile { - public required string FileId { get; init; } + public required string FileRef { get; init; } public string? TextId { get; init; } } diff --git a/src/Serval/src/Serval.DataFiles/Services/CorpusService.cs b/src/Serval/src/Serval.DataFiles/Services/CorpusService.cs index 61622a93..35dbad08 100644 --- a/src/Serval/src/Serval.DataFiles/Services/CorpusService.cs +++ b/src/Serval/src/Serval.DataFiles/Services/CorpusService.cs @@ -2,14 +2,13 @@ namespace Serval.DataFiles.Services; public class CorpusService( IRepository corpora, + IRepository dataFiles, IDataAccessContext dataAccessContext, - IDataFileService dataFileService, IScopedMediator mediator ) : OwnedEntityServiceBase(corpora), ICorpusService { private readonly IDataAccessContext _dataAccessContext = dataAccessContext; - private readonly IDataFileService _dataFileService = dataFileService; - + private readonly IRepository _dataFiles = dataFiles; private readonly IScopedMediator _mediator = mediator; public async Task GetAsync(string id, string owner, CancellationToken cancellationToken = default) @@ -36,17 +35,21 @@ public async Task UpdateAsync( ); if (corpus is null) throw new EntityNotFoundException($"Could not find Corpus '{id}."); + HashSet corpusFileIds = corpus.Files.Select(f => f.FileRef).ToHashSet(); + IDictionary corpusDataFilesDict = ( + await _dataFiles.GetAllAsync(f => corpusFileIds.Contains(f.Id), ct) + ).ToDictionary(f => f.Id); await _mediator.Publish( new CorpusUpdated { CorpusId = corpus.Id, - Files = await Task.WhenAll( - corpus.Files.Select(async f => new CorpusFileResult + Files = corpus + .Files.Select(f => new CorpusFileResult { - TextId = f.TextId!, - File = Map(await _dataFileService.GetAsync(f.FileId)) + TextId = f.TextId ?? corpusDataFilesDict[f.FileRef].Name, + File = Map(corpusDataFilesDict[f.FileRef]) }) - ) + .ToList() }, ct ); diff --git a/src/Serval/src/Serval.DataFiles/Services/IDataFileService.cs b/src/Serval/src/Serval.DataFiles/Services/IDataFileService.cs index b10d8f25..b2a4f874 100644 --- a/src/Serval/src/Serval.DataFiles/Services/IDataFileService.cs +++ b/src/Serval/src/Serval.DataFiles/Services/IDataFileService.cs @@ -3,6 +3,7 @@ public interface IDataFileService { Task> GetAllAsync(string owner, CancellationToken cancellationToken = default); + Task> GetAllAsync(IEnumerable ids, CancellationToken cancellationToken = default); Task GetAsync(string id, CancellationToken cancellationToken = default); Task GetAsync(string id, string owner, CancellationToken cancellationToken = default); Task CreateAsync(DataFile dataFile, Stream stream, CancellationToken cancellationToken = default); diff --git a/src/Serval/src/Serval.Shared/Services/EntityServiceBase.cs b/src/Serval/src/Serval.Shared/Services/EntityServiceBase.cs index b46aa87f..a8626bff 100644 --- a/src/Serval/src/Serval.Shared/Services/EntityServiceBase.cs +++ b/src/Serval/src/Serval.Shared/Services/EntityServiceBase.cs @@ -13,6 +13,15 @@ public virtual async Task GetAsync(string id, CancellationToken cancellationT return entity; } + public virtual async Task> GetAllAsync( + IEnumerable ids, + CancellationToken cancellationToken = default + ) + { + HashSet idSet = ids.ToHashSet(); + return await Entities.GetAllAsync(e => idSet.Contains(e.Id), cancellationToken); + } + public virtual async Task CreateAsync(T entity, CancellationToken cancellationToken = default) { await Entities.InsertAsync(entity, cancellationToken); diff --git a/src/Serval/src/Serval.Translation/Consumers/CorpusUpdatedConsumer.cs b/src/Serval/src/Serval.Translation/Consumers/CorpusUpdatedConsumer.cs index 366e8099..b096d30b 100644 --- a/src/Serval/src/Serval.Translation/Consumers/CorpusUpdatedConsumer.cs +++ b/src/Serval/src/Serval.Translation/Consumers/CorpusUpdatedConsumer.cs @@ -18,7 +18,7 @@ private static CorpusFile Map(CorpusFileResult corpusFile) return new CorpusFile { Id = corpusFile.File.DataFileId, - TextId = corpusFile.TextId, + TextId = corpusFile.TextId ?? corpusFile.File.Name, Filename = corpusFile.File.Filename, Format = corpusFile.File.Format, }; diff --git a/src/Serval/src/Serval.Translation/Controllers/TranslationEnginesController.cs b/src/Serval/src/Serval.Translation/Controllers/TranslationEnginesController.cs index 1fa13d74..47358472 100644 --- a/src/Serval/src/Serval.Translation/Controllers/TranslationEnginesController.cs +++ b/src/Serval/src/Serval.Translation/Controllers/TranslationEnginesController.cs @@ -1294,7 +1294,7 @@ CancellationToken cancellationToken Id = f.File.DataFileId, Filename = f.File.Filename, Format = f.File.Format, - TextId = f.TextId + TextId = f.TextId ?? f.File.Name }) .ToList(), } diff --git a/src/Serval/test/Serval.ApiServer.IntegrationTests/TranslationEngineTests.cs b/src/Serval/test/Serval.ApiServer.IntegrationTests/TranslationEngineTests.cs index 07ccb9f2..18b50a68 100644 --- a/src/Serval/test/Serval.ApiServer.IntegrationTests/TranslationEngineTests.cs +++ b/src/Serval/test/Serval.ApiServer.IntegrationTests/TranslationEngineTests.cs @@ -184,21 +184,21 @@ public async Task SetUp() Id = SOURCE_CORPUS_ID_1, Language = "en", Owner = "client1", - Files = [new() { FileId = srcFile.Id, TextId = "all" }] + Files = [new() { FileRef = srcFile.Id, TextId = "all" }] }; var srcCorpus2 = new DataFiles.Models.Corpus { Id = SOURCE_CORPUS_ID_2, Language = "en", Owner = "client1", - Files = [new() { FileId = srcFile.Id, TextId = "all" }] + Files = [new() { FileRef = srcFile.Id, TextId = "all" }] }; var trgCorpus = new DataFiles.Models.Corpus { Id = TARGET_CORPUS_ID, Language = "en", Owner = "client1", - Files = [new() { FileId = trgFile.Id, TextId = "all" }] + Files = [new() { FileRef = trgFile.Id, TextId = "all" }] }; await _env.Corpora.InsertAllAsync([srcCorpus, srcCorpus2, trgCorpus]); } @@ -2045,7 +2045,7 @@ public async Task DataFileUpdate_Propagated() DataFiles.Models.DataFile orgFileFromRepo = (await _env.DataFiles.GetAsync(FILE1_SRC_ID))!; DataFiles.Models.Corpus orgCorpusFromRepo = (await _env.Corpora.GetAsync(TARGET_CORPUS_ID))!; Assert.That(orgFileFromClient.Name, Is.EqualTo(orgFileFromRepo.Name)); - Assert.That(orgCorpusFromRepo.Files[0].FileId, Is.EqualTo(FILE2_TRG_ID)); + Assert.That(orgCorpusFromRepo.Files[0].FileRef, Is.EqualTo(FILE2_TRG_ID)); // Update the file await dataFilesClient.UpdateAsync(FILE1_SRC_ID, new FileParameter(new MemoryStream([1, 2, 3]), "test.txt")); diff --git a/src/Serval/test/Serval.DataFiles.Tests/Services/CorpusServiceTests.cs b/src/Serval/test/Serval.DataFiles.Tests/Services/CorpusServiceTests.cs index a9e498b4..c7c75d12 100644 --- a/src/Serval/test/Serval.DataFiles.Tests/Services/CorpusServiceTests.cs +++ b/src/Serval/test/Serval.DataFiles.Tests/Services/CorpusServiceTests.cs @@ -21,7 +21,7 @@ public class CorpusServiceTests Owner = "owner1", Name = "corpus1", Language = "en", - Files = new List() { new() { FileId = DefaultDataFile.Id } } + Files = new List() { new() { FileRef = DefaultDataFile.Id } } }; [Test] @@ -56,8 +56,8 @@ public TestEnvironment() }); Service = new CorpusService( Corpora, + Substitute.For>(), DataAccessContext, - Substitute.For(), Substitute.For() ); }