Ees 4944 read next data set version metadata #4952

duncan-at-hiveit · 2024-06-12T15:50:01Z

Update 19/06/2024

Based on feedback from Nick, we've decided to go with an in-memory-only extraction of metadata from the next DataSetVersion rather than storing them in the database. As a result of this, the following changes have changed since originally raising this PR:

We have a new function called "CreateMappings" - this in turn reads all the appropriate meta for generating the diffs and the meta summary.
In order to gather the required metadata, each BlahMetaRepository in use now has a Read* and a Create* method which under the hood use a bunch of shared code for extracting the metadata.
CompleteProcessingFunction is no longer broken out as a separate shared function - instead, it's moved back into ProcessInitialDataSetVersionFunction, and a new CompleteNextDataSetVersionMappingsProcessing function is broken out.

Overview

This PR covers EES-4944 (the importing of the next data set version's metadata) and EES-4953 (the Admin API endpoint from which to initiate this action).

Behaviour

Importing

The import process begins in the same way as the initial DataSet workflow:

The Subject CSVs are copied to the file share.
The Metadata is imported.

Then it skips the import of the data itself and jumps straight to completion whereupon the DataSetVersionImport is marked as completed and the DataSetVersion set to its next state, which in this case is "Mapping".

The import process is initiated from the Admin endpoint POST /api/public-data/data-set-versions which takes a DataSetId and the ReleaseFileId for the next candidate version.

Validation

This PR adds validation to the choice of ReleaseFile when it receives the import request from Admin. We'd expect most / all of this to be handled by the dropdown that shows the available Subjects that can be chosen for the next DataSetVersion, but this extra validation is done down in the importer just in case:

There is some shared validation from the initial version import:

The chosen ReleaseFile must not already be in use by a DataSet.
The chosen ReleaseFile must come from a ReleaseVersion that is currently in Draft.
The chosen ReleaseFile must be of type "Data".

And there is some new validation specific to the "next" version import:

The chosen ReleaseFile must belong to a Release of the same Publication as the DataSet.
The chosen ReleaseFile must not belong to the same Release as any of the DataSet's currrent Versions (i.e. you can't add a version "1.1" from a file on the 2021 Release if version "1.0" also lives on the 2021 Release - they must be from another Release entirely and never from any of its amendments).
The chosen DataSet must already have a current live Version.

Refactoring

Breaking "CompleteProcessing" into its own file

As I've found that the CompleteProcessing stage seemed to be relevant in its current form for both the initial DataSetVersion and "next" Versions for now, I have separated it into its own class for reuse between the "initial" and "next" workflows. The only change in behaviour for now is the fact that initial versions end up in a "Draft" state whereas next versions end up in
a "Mapping" state. It may prove down the line that it's best separated out should the 2 workflows differ significantly for any reason but this is pretty trivial to do.

Ben and I talked about the potential for the Import Metadata Function to break into 2 parts which may also affect the CompleteProcessing Function, but we agreed not to try to tackle that now as it would cause issues on his testing branch.

Miscellaneous changes

Adding DbContext.RequireTransaction extension method

Given that more occurrences of the following code block were starting to appear around the codebase (including in this PR), I broke it out into a DbContext extension method for ease of maintenance and readability of the code.

        var strategy = context.Database.CreateExecutionStrategy();

        return await strategy.ExecuteAsync(
            async () =>
            {
                using var transactionScope = new TransactionScope(
                    TransactionScopeOption.Required,
                    new TransactionOptions {IsolationLevel = IsolationLevel.ReadCommitted},
                    TransactionScopeAsyncFlowOption.Enabled);
                
                // Do something with dbs here

                transactionScope.Complete();
            });

now becomes:

await context.RequireTransaction(async () => {
  // Do something with dbs here
});

I added a whole suite of tests around this that share a transaction amongst different DbContexts and tests the behaviour if shared transactions and rollback, the support for DbContexts that use EnableRetryOnFailure(), and nested transactions.

Adding EnableRetryOnFailure to all PostgreSQL DbContexts

As this option was available for setting up as we register DbContexts, I have added it to all of our PostgreSQL DbContexts.

...xploreEducationStatistics.Admin.Tests/Controllers/Api/Public.Data/DataSetsControllerTests.cs

...ducationStatistics.Admin.Tests/Controllers/Api/Public.Data/DataSetVersionsControllerTests.cs

...Education.ExploreEducationStatistics.Public.Data.Processor/Services/DataSetVersionService.cs

ntsim · 2024-06-20T00:57:40Z

This branch really needs to be rebased onto dev as it's missing some significant new changes. Think there's probably a fair bit of merge conflict pain heading your way!

…e metadata of a new DataSetVersion, and an Admin endpoint for triggering it. Added initial validation to endpoint.

… tests for processing next DataSetVersion and the new separated CompleteProcessingFunction

…ng NextDataSetVersions

…ding choices

…de to place code creating the DataSetVersions in the DataSetVersionService and leaving only the DataSet creation in the DataSetService. Creating a convenience DbContext.RequireTransaction() method to handle transaction opening, completing and rollback that supports exception throwing code and Either-returning code in order to cause a rollback. Added test suite covering the behaviour of transactions that are creaqted via this extension method, and added RetryOnFailure to all PostgreSQL DbContexts.

…the creation of mappings, instead reading metadata only

…o be in-memory only

…vertions! Better naming and messages, better return types on methods. Refactoring tests for easier reading.

...vUk.Education.ExploreEducationStatistics.Admin/Services/Public.Data/DataSetVersionService.cs

jack-hive · 2024-06-20T16:01:48Z

src/GovUk.Education.ExploreEducationStatistics.Admin/Services/Public.Data/ProcessorClient.cs

    }

-    public async Task<Either<ActionResult, Unit>> DeleteDataSetVersion(
-        Guid dataSetVersionId,
+    private async Task<Either<ActionResult, TResponse>> SendRequest<TResponse>(


I was speaking to Ben about this on one of my previous PR's. We were torn which way to go but in the end we decided not to break the handling of the HTTP response out into a common method - because it felt like each public method should be responsible for handling it's HTTP response individually, as the way it handles it may differ from endpoint to endpoint. For example, the DeleteDataSetVersion endpoint previously handled NotFound status codes; whereas now that case would fall into a 500.

I'm still torn which way I'd go on it personally. It does save a lot of code, that is mostly just reused. But at the same time it feels like each endpoint should be handled individually as there isn't technically anything to say they won't be different. They're just the same now by coincidence (aside from DeleteDataSetVersion like I mentioned)

...vUk.Education.ExploreEducationStatistics.Common/Extensions/DbContextTransactionExtensions.cs

src/GovUk.Education.ExploreEducationStatistics.Public.Data.Processor/Functions/ActivityNames.cs

benoutram · 2024-06-21T09:40:17Z

...eEducationStatistics.Public.Data.Processor/Functions/ProcessInitialDataSetVersionFunction.cs

-
-    [Function(ActivityNames.CompleteProcessing)]
-    public async Task CompleteProcessing(
+


Minor one, there's some whitespace crept in here into ProcessInitialDataSetVersionFunction.

Heh - I'm not seeing what you're seeing I'm afraid. I have a method CompleteInitialDataSetVersionProcessing which has no additional whitespace under it

It's not a big deal but it's strange that you are not seeing this. The introduction of that line containing only whitespace is shown on Github when I look through the 'Files changed' list. It's line 70 directly above the line containing the annotation [Function(ActivityNames.CompleteInitialDataSetVersionProcessing)].

If I check out this branch locally and reformat this file it shows as a change which can be seen here:

Aha ok got it ta

...ovUk.Education.ExploreEducationStatistics.Common.Tests/Extensions/DbContextTestExtensions.cs

ntsim · 2024-06-21T12:37:52Z

...cation.ExploreEducationStatistics.Public.Data.Processor/Repository/LocationMetaRepository.cs

+    private async Task<List<LocationOptionMetaRow>> GetLocationOptionMetas(
+        IDuckDbConnection duckDbConnection,
+        DataSetVersion dataSetVersion,
+        CancellationToken cancellationToken,


Lint here - the cancellation token should be the last parameter

Cool, amended thanks

jack-hive · 2024-06-20T16:46:40Z

src/GovUk.Education.ExploreEducationStatistics.Public.Data.Model/DataSetVersion.cs

@@ -11,6 +11,8 @@ namespace GovUk.Education.ExploreEducationStatistics.Public.Data.Model;

 public class DataSetVersion : ICreatedUpdatedTimestamps<DateTimeOffset, DateTimeOffset?>
 {
+    public const string FirstVersionString = "1.0";


I think I'd either make this private, or if we wanted to make it public (assuming we have a use for it then I'd instead create a public method which takes in versionMajor and versionMinor integers, and returns $"{versionMajor}.{versionMinor}". You can then use that method publicly to retrieve the string representation of whatever version you want, including "1.0", and you can also re-use it on line 72 to calculate the Version. That way we can ensure that the two are being formatted consistently

Haha. You know what, it was because I was originally using it in tests, but have since rolled that back out. I'm just going to return it to the way it was - no const.

...Education.ExploreEducationStatistics.Public.Data.Processor/Services/DataSetVersionService.cs

jack-hive · 2024-06-21T11:24:58Z

src/GovUk.Education.ExploreEducationStatistics.Public.Data.Processor/ProcessorHostBuilder.cs

+                    .AddScoped<IValidator<DataSetCreateRequest>, DataSetCreateRequest.Validator>()
+                    .AddScoped<IValidator<NextDataSetVersionCreateRequest>,
+                        NextDataSetVersionCreateRequest.Validator>()


Do we need to do this? I thought the .AddFluentValidation() line covered it?

When I remove these lines I get:

System.InvalidOperationException : Unable to resolve service for type 'FluentValidation.IValidator`1[GovUk.Education.ExploreEducationStatistics.Public.Data.Processor.Requests.DataSetCreateRequest]' while attempting to activate 'GovUk.Education.ExploreEducationStatistics.Public.Data.Processor.Functions.CreateDataSetFunction'

But then I added:

services.AddValidatorsFromAssemblyContaining<DataSetCreateRequest.Validator>();

and all was well :)

Good spot, thanks

Hmmmm... I'd expect .AddFluentValidation to do that for you. If you take a look under 'FluentValidationServiceCollectionExtensions' it seems like it should work with that 1 line alone :/

I'm not sure why it's failing

Not sure tbh. We also do this in the Content API Startup.cs:

services.AddFluentValidation(); services.AddValidatorsFromAssemblyContaining<DataSetFileListRequest.Validator>();

jack-hive · 2024-06-21T11:41:31Z

...ducation.ExploreEducationStatistics.Admin.Tests/Services/Public.Data/ProcessorClientTests.cs

-        public async Task HttpClientNotFound_ReturnsNotFound()
-        {
-            var dataSetVersionId = Guid.NewGuid();
-
-            _mockHttp.Expect(HttpMethod.Delete, $"{Uri.AbsoluteUri}/{dataSetVersionId}")
-                .Respond(HttpStatusCode.NotFound);
-
-            var response = await _processorClient.DeleteDataSetVersion(dataSetVersionId);
-
-            _mockHttp.VerifyNoOutstandingExpectation();
-
-            var left = response.AssertLeft();
-            left.AssertNotFoundResult();
-        }


Are we happy to convert NotFounds into 500s?

I've reinstated this endpoint's special-case handling of 404s by allowing each endpoint to supply an optional "additionalResponseValidator" so that each endpoint can get first stab at deciding what to do with the response. 404s are now returning NotFoundResults for this endpoint alone, although I'd personally be tempted to consider doing this with all endpoints :)

...on.ExploreEducationStatistics.Common.Tests/Extensions/DbContextTransactionExtensionsTests.cs

...ionStatistics.Public.Data.Processor.Tests/Functions/CreateNextDataSetVersionFunctionTests.cs

jack-hive · 2024-06-21T12:24:04Z

...ionStatistics.Public.Data.Processor.Tests/Functions/CreateNextDataSetVersionFunctionTests.cs

+            var subjectId = Guid.NewGuid();
+
+            var (releaseFile, releaseMetaFile) = DataFixture.DefaultReleaseFile()
+                .WithReleaseVersion(DataFixture.DefaultReleaseVersion()
+                    .WithApprovalStatus(ReleaseApprovalStatus.Draft))
+                .WithFiles([
+                    DataFixture
+                        .DefaultFile(FileType.Data)
+                        .WithSubjectId(subjectId),
+                    DataFixture
+                        .DefaultFile(FileType.Metadata)
+                        .WithSubjectId(subjectId)
+                ])
+                .GenerateList()
+                .ToTuple2();


Could maybe use your AddDataAndMetadataFiles method here to shorten?

Oh yeah! Thanks, changed that

jack-hive · 2024-06-21T12:37:51Z

...onStatistics.Public.Data.Processor.Tests/Functions/ProcessNextDataSetVersionFunctionTests.cs

+            var initialLocationMeta = DataFixture
+                .DefaultLocationMeta()
+                .WithDataSetVersionId(initialDataSetVersion.Id)
+                .ForIndex(0, s => s
+                    .SetLevel(GeographicLevel.LocalAuthority)
+                    .SetOptions(DataFixture
+                        .DefaultLocationLocalAuthorityOptionMeta()
+                        .GenerateList(2)
+                        .Select(meta => meta as LocationOptionMeta)
+                        .ToList()))
+                .ForIndex(1, s => s
+                    .SetLevel(GeographicLevel.RscRegion)
+                    .SetOptions(DataFixture
+                        .DefaultLocationRscRegionOptionMeta()
+                        .GenerateList(2)
+                        .Select(meta => meta as LocationOptionMeta)
+                        .ToList()))
+                .GenerateList();
+
+            var initialFilterMeta = DataFixture
+                .DefaultFilterMeta()
+                .WithDataSetVersionId(initialDataSetVersion.Id)
+                .WithOptions(() => DataFixture
+                    .DefaultFilterOptionMeta()
+                    .GenerateList(2))
+                .GenerateList(2);
+
+            var initialIndicatorMeta = DataFixture
+                .DefaultIndicatorMeta()
+                .WithDataSetVersionId(initialDataSetVersion.Id)
+                .GenerateList(2);
+
+            var initialTimePeriodMeta = DataFixture
+                .DefaultTimePeriodMeta()
+                .WithDataSetVersionId(initialDataSetVersion.Id)
+                .GenerateList(4);
+
+            await AddTestData<PublicDataDbContext>(context =>
+            {
+                context.LocationMetas.AddRange(initialLocationMeta);
+                context.FilterMetas.AddRange(initialFilterMeta);
+                context.IndicatorMetas.AddRange(initialIndicatorMeta);
+                context.TimePeriodMetas.AddRange(initialTimePeriodMeta);
+            });


Don't think you need any of this now since you've changed it to do the work in-memory

We're only reading the next data set version's meta in-memory, but the original data set version's meta is still read from the db as it's already in there and ready to go!

Oh sorry

Do we do anything with it though? The test seems to pass if you remove all this

Oh yeah of course, because this PR doesn't contain the "create mappings" work! Cool, removed here, but it's reinstated in the 4945 PR! Ta

jack-hive · 2024-06-21T12:40:11Z

...onStatistics.Public.Data.Processor.Tests/Functions/ProcessNextDataSetVersionFunctionTests.cs

+            var dataSetVersionPathResolver = GetRequiredService<IDataSetVersionPathResolver>();
+            Directory.CreateDirectory(dataSetVersionPathResolver.DirectoryPath(nextDataSetVersion));


don't think you need these 2 lines?

100% correct! Removed thanks

...cation.ExploreEducationStatistics.Public.Data.Processor/Repository/LocationMetaRepository.cs

…val of unnecessary code

…equences into extracted DbContextTestExtensions.ClearTestData method

…ersion in response to PR comments, by adding additional validation parameter which allows each endpoint to be handled differently if required.

jack-hive · 2024-06-25T12:24:23Z

src/GovUk.Education.ExploreEducationStatistics.Admin/Services/Public.Data/ProcessorClient.cs

            cancellationToken);
    }

    private async Task<Either<ActionResult, TResponse>> SendRequest<TResponse>(
        Func<Task<HttpResponseMessage>> requestFunction,
+        Func<HttpResponseMessage, ActionResult?>? additionalResponseValidator = null,


I'd potentially rename this to additionalFailedResponseValidator or something similar to indicate that it's only used in non-success cases. But other than that, I think this looks good :)

I've renamed to customResponseHandler to keep it open-ended and moved it so that it can potentially handle any and all responses, failures or otherwise. Doesn't have to be solely for validation anymore but could be used to do something custom with success cases as well.

duncan-at-hiveit force-pushed the EES-4944-read-next-data-set-version-metadata branch 2 times, most recently from f8fe25e to dd3784f Compare June 13, 2024 13:55

duncan-at-hiveit marked this pull request as ready for review June 13, 2024 14:05

duncan-at-hiveit force-pushed the EES-4944-read-next-data-set-version-metadata branch 11 times, most recently from 7c8aeef to aa11d3e Compare June 19, 2024 12:23

ntsim requested changes Jun 20, 2024

View reviewed changes

duncan-at-hiveit added 9 commits June 20, 2024 09:59

EES-4944 and EES-4953 - added new Function orchestration to import th…

0e4494a

…e metadata of a new DataSetVersion, and an Admin endpoint for triggering it. Added initial validation to endpoint.

EES-4944 - split CompleteProcessing into its own Function file. Added…

78f751a

… tests for processing next DataSetVersion and the new separated CompleteProcessingFunction

EES-4944 - added tests for Admin DataSetVersioncontroller when creati…

564a609

…ng NextDataSetVersions

EES-4944 - adding tests for new CreateNextDataSetVersionFunction.

793d4f6

EES-4944 - updating class and method names to fit in with updated wor…

73cf53d

…ding choices

EES-4944 - cleanup before raising PR

448c18a

EES-4944 - removing the persisting of DataSetVersion metadata during …

9639bdf

…the creation of mappings, instead reading metadata only

EES-4994 - more refactoring following rework of metadata extraction t…

906103f

…o be in-memory only

duncan-at-hiveit force-pushed the EES-4944-read-next-data-set-version-metadata branch from aa11d3e to 906103f Compare June 20, 2024 09:24

EES-4944 - responding to PR comments and lots of formatting change re…

abdc95b

…vertions! Better naming and messages, better return types on methods. Refactoring tests for easier reading.

jack-hive requested changes Jun 20, 2024

View reviewed changes

benoutram reviewed Jun 21, 2024

View reviewed changes

ntsim reviewed Jun 21, 2024

View reviewed changes

jack-hive requested changes Jun 21, 2024

View reviewed changes

ntsim reviewed Jun 21, 2024

View reviewed changes

...cation.ExploreEducationStatistics.Public.Data.Processor/Repository/LocationMetaRepository.cs Show resolved Hide resolved

duncan-at-hiveit added 3 commits June 21, 2024 18:15

EES-4944 - responding to various PR suggestions. Method renames, remo…

c72894d

…val of unnecessary code

EES-4944 - resolving merge conflicts with dev. Pulling resetting of s…

c52407b

…equences into extracted DbContextTestExtensions.ClearTestData method

EES-4944 - reinstated specia-case handling of 404s for DeleteDataSetV…

7929a22

…ersion in response to PR comments, by adding additional validation parameter which allows each endpoint to be handled differently if required.

jack-hive approved these changes Jun 25, 2024

View reviewed changes

ntsim approved these changes Jun 25, 2024

View reviewed changes

duncan-at-hiveit added 2 commits June 25, 2024 14:14

EES-4944 - final PR changes upon request

8354ed0

EES-4944 - final PR changes upon request

4b5b02a

duncan-at-hiveit merged commit 42991af into dev Jun 25, 2024
3 of 7 checks passed

duncan-at-hiveit deleted the EES-4944-read-next-data-set-version-metadata branch June 25, 2024 13:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ees 4944 read next data set version metadata #4952

Ees 4944 read next data set version metadata #4952

duncan-at-hiveit commented Jun 12, 2024 •

edited

Loading

ntsim commented Jun 20, 2024

jack-hive Jun 20, 2024

benoutram Jun 21, 2024

duncan-at-hiveit Jun 21, 2024

benoutram Jun 24, 2024

duncan-at-hiveit Jun 25, 2024

duncan-at-hiveit Jun 25, 2024

ntsim Jun 21, 2024

duncan-at-hiveit Jun 21, 2024

jack-hive Jun 20, 2024

duncan-at-hiveit Jun 21, 2024

jack-hive Jun 21, 2024

duncan-at-hiveit Jun 21, 2024 •

edited

Loading

jack-hive Jun 21, 2024

duncan-at-hiveit Jun 24, 2024 •

edited

Loading

jack-hive Jun 21, 2024

duncan-at-hiveit Jun 24, 2024

jack-hive Jun 21, 2024

duncan-at-hiveit Jun 21, 2024

jack-hive Jun 21, 2024

duncan-at-hiveit Jun 21, 2024

jack-hive Jun 21, 2024

duncan-at-hiveit Jun 24, 2024

jack-hive Jun 21, 2024

duncan-at-hiveit Jun 21, 2024

jack-hive Jun 25, 2024 •

edited

Loading

duncan-at-hiveit Jun 25, 2024


		[Function(ActivityNames.CompleteProcessing)]
		public async Task CompleteProcessing(

		var dataSetVersionPathResolver = GetRequiredService<IDataSetVersionPathResolver>();
		Directory.CreateDirectory(dataSetVersionPathResolver.DirectoryPath(nextDataSetVersion));

Ees 4944 read next data set version metadata #4952

Ees 4944 read next data set version metadata #4952

Conversation

duncan-at-hiveit commented Jun 12, 2024 • edited Loading

Update 19/06/2024

Overview

Behaviour

Importing

Validation

Refactoring

Breaking "CompleteProcessing" into its own file

Miscellaneous changes

Adding DbContext.RequireTransaction extension method

Adding EnableRetryOnFailure to all PostgreSQL DbContexts

ntsim commented Jun 20, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

duncan-at-hiveit Jun 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

duncan-at-hiveit Jun 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jack-hive Jun 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

duncan-at-hiveit commented Jun 12, 2024 •

edited

Loading

duncan-at-hiveit Jun 21, 2024 •

edited

Loading

duncan-at-hiveit Jun 24, 2024 •

edited

Loading

jack-hive Jun 25, 2024 •

edited

Loading