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

Fixing incorrect versioning on close records #4786

Merged
merged 4 commits into from
Jan 23, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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 @@ -42,6 +42,10 @@ public ImportResource Parse(long index, long offset, int length, string rawResou
var lastUpdatedIsNull = importMode == ImportMode.InitialLoad || resource.Meta.LastUpdated == null;
var lastUpdated = lastUpdatedIsNull ? Clock.UtcNow : resource.Meta.LastUpdated.Value;
resource.Meta.LastUpdated = new DateTimeOffset(lastUpdated.DateTime.TruncateToMillisecond(), lastUpdated.Offset);
if (!lastUpdatedIsNull && resource.Meta.LastUpdated.Value > Clock.UtcNow.AddSeconds(10)) // 5 sec is the max for the computers in the domain
{
throw new NotSupportedException("LastUpdated in the resource cannot be in the future.");
}

var keepVersion = true;
if (lastUpdatedIsNull || string.IsNullOrEmpty(resource.Meta.VersionId) || !int.TryParse(resource.Meta.VersionId, out var _))
Expand Down

Large diffs are not rendered by default.

5,178 changes: 5,178 additions & 0 deletions src/Microsoft.Health.Fhir.SqlServer/Features/Schema/Migrations/85.sql

Large diffs are not rendered by default.

Original file line number Diff line number Diff line change
Expand Up @@ -94,5 +94,6 @@ public enum SchemaVersion
V82 = 82,
V83 = 83,
V84 = 84,
V85 = 85,
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ namespace Microsoft.Health.Fhir.SqlServer.Features.Schema
public static class SchemaVersionConstants
{
public const int Min = (int)SchemaVersion.V83;
public const int Max = (int)SchemaVersion.V84;
public const int Max = (int)SchemaVersion.V85;
public const int MinForUpgrade = (int)SchemaVersion.V83; // this is used for upgrade tests only
public const int SearchParameterStatusSchemaVersion = (int)SchemaVersion.V6;
public const int SupportForReferencesWithMissingTypeVersion = (int)SchemaVersion.V7;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,6 @@ Go

INSERT INTO dbo.SchemaVersion
VALUES
(84, 'started')
(85, 'started')

Go
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,8 @@ BEGIN TRY
ON B.ResourceTypeId = A.ResourceTypeId AND B.ResourceId = A.ResourceId AND B.IsHistory = 0
OPTION (MAXDOP 1, OPTIMIZE FOR (@DummyTop = 1))

IF @RaiseExceptionOnConflict = 1 AND EXISTS (SELECT * FROM @ResourceInfos WHERE PreviousVersion IS NOT NULL AND Version <= PreviousVersion)
-- Consider surrogate id out of allignment as a conflict
IF @RaiseExceptionOnConflict = 1 AND EXISTS (SELECT * FROM @ResourceInfos WHERE (PreviousVersion IS NOT NULL AND Version <= PreviousVersion) OR (PreviousSurrogateId IS NOT NULL AND SurrogateId <= PreviousSurrogateId))
THROW 50409, 'Resource has been recently updated or added, please compare the resource content in code for any duplicate updates', 1

INSERT INTO @PreviousSurrogateIds
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -625,13 +625,14 @@

// If last updated on input resource is below current, then need to check the "fit".
var inputsNoVersionForCheck = new List<ImportResource>();
foreach (var input in inputs)
{
if (currentInDb.TryGetValue(input.ResourceWrapper.ToResourceKey(true), out var current) && input.ResourceWrapper.LastModified < current.LastModified)
// Include inputs only with explicit lastUpdated (input.KeepLastUpdated = true)
if (currentInDb.TryGetValue(input.ResourceWrapper.ToResourceKey(true), out var current) && input.KeepLastUpdated && input.ResourceWrapper.LastModified < current.LastModified)
{
inputsNoVersionForCheck.Add(input);
}
}

Check notice

Code scanning / CodeQL

Missed opportunity to use Where Note

This foreach loop
implicitly filters its target sequence
- consider filtering the sequence explicitly using '.Where(...)'.

// Ensure that the imported resources can "fit" in the db. We want to keep versionId alinged to lastUpdated and sequential if possible.
// Note: surrogate id is populated from last updated by ToResourceDateKey(), therefore we can trust this value as part of dictionary key.
Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
<Project Sdk="Microsoft.NET.Sdk">
<!-- Properties used by sql task to generate full script -->
<PropertyGroup>
<LatestSchemaVersion>84</LatestSchemaVersion>
<LatestSchemaVersion>85</LatestSchemaVersion>
<GeneratedFullScriptPath>Features\Schema\Migrations\$(LatestSchemaVersion).sql</GeneratedFullScriptPath>
</PropertyGroup>

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ private async Task<List<Resource>> SavePrerequisiteResourcesViaHttp(List<Resourc
},
new()
{
CreateTestPatient(id: sharedId, lastUpdated: DateTimeOffset.UtcNow.AddMinutes(1)),
CreateTestPatient(id: sharedId, lastUpdated: DateTimeOffset.UtcNow.AddSeconds(8)),
});
}

Expand All @@ -94,7 +94,7 @@ private async Task<List<Resource>> SavePrerequisiteResourcesViaHttp(List<Resourc
},
new()
{
CreateTestPatient(id: sharedId, lastUpdated: DateTimeOffset.UtcNow.AddMinutes(1), versionId: "1"),
CreateTestPatient(id: sharedId, lastUpdated: DateTimeOffset.UtcNow.AddSeconds(8), versionId: "1"),
});
}

Expand All @@ -106,7 +106,7 @@ private async Task<List<Resource>> SavePrerequisiteResourcesViaHttp(List<Resourc
new()
{
CreateTestPatient(id: sharedId, lastUpdated: DateTimeOffset.UtcNow, versionId: "1"),
CreateTestPatient(id: sharedId, lastUpdated: DateTimeOffset.UtcNow.AddMinutes(1), versionId: "1"),
CreateTestPatient(id: sharedId, lastUpdated: DateTimeOffset.UtcNow.AddSeconds(8), versionId: "1"),
});
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -419,20 +419,33 @@ public async Task GivenIncrementalLoad_MultipleInputsWithImplicitVersionsExplici
Assert.Equal("1", history.Resource.Entry[0].Resource.VersionId);
}

[Fact]
public async Task GivenIncrementalLoad_LastUpdatedOnResourceCannotBeInTheFuture()
{
var id = Guid.NewGuid().ToString("N");
var ndJson = CreateTestPatient(id, DateTimeOffset.UtcNow.AddSeconds(20)); // set value higher than 10 seconds tolerance
var location = (await ImportTestHelper.UploadFileAsync(ndJson, _fixture.StorageAccount)).location;
var request = CreateImportRequest(location, ImportMode.IncrementalLoad, false, true);
var result = await ImportCheckAsync(request, null, 1);
var errorLocation = result.Error.ToArray()[0].Url;
var errorContent = await ImportTestHelper.DownloadFileAsync(errorLocation, _fixture.StorageAccount);
Assert.Contains("LastUpdated in the resource cannot be in the future.", errorContent);
}

[Fact]
public async Task GivenIncrementalLoad_MultipleInputsWithImplicitVersionsExplicitLastUpdatedAfterImplicit()
{
var id = Guid.NewGuid().ToString("N");
var ndJson1 = CreateTestPatient(id);
var ndJson2 = CreateTestPatient(id, DateTimeOffset.UtcNow.AddDays(1));
var ndJson2 = CreateTestPatient(id, DateTimeOffset.UtcNow.AddSeconds(8), birhDate: "1990"); // last updated within 10 seconds tolerance, different content
var location = (await ImportTestHelper.UploadFileAsync(ndJson1 + ndJson2, _fixture.StorageAccount)).location;
var request = CreateImportRequest(location, ImportMode.IncrementalLoad, false, true);
await ImportCheckAsync(request, null, 0);

var history = await _client.SearchAsync($"Patient/{id}/_history");
Assert.Equal(2, history.Resource.Entry.Count);
Assert.Equal("1", history.Resource.Entry[0].Resource.VersionId);
Assert.Equal("-1", history.Resource.Entry[1].Resource.VersionId);
//// same order
Assert.True(int.Parse(history.Resource.Entry[0].Resource.VersionId) > int.Parse(history.Resource.Entry[1].Resource.VersionId));
Assert.True(history.Resource.Entry[0].Resource.Meta.LastUpdated > history.Resource.Entry[1].Resource.Meta.LastUpdated);
}

[Fact]
Expand Down Expand Up @@ -1725,7 +1738,7 @@ public async Task GivenImportRequestWithMultipleSameFile_ThenBadRequestShouldBeR
Assert.Equal(HttpStatusCode.BadRequest, fhirException.StatusCode);
}

private async Task<Uri> ImportCheckAsync(ImportRequest request, TestFhirClient client = null, int? errorCount = null)
private async Task<ImportJobResult> ImportCheckAsync(ImportRequest request, TestFhirClient client = null, int? errorCount = null)
{
client = client ?? _client;
Uri checkLocation = await ImportTestHelper.CreateImportTaskAsync(client, request);
Expand All @@ -1746,7 +1759,7 @@ private async Task<Uri> ImportCheckAsync(ImportRequest request, TestFhirClient c

Assert.NotEmpty(result.Request);

return checkLocation;
return result;
}

private async Task<HttpResponseMessage> ImportWaitAsync(Uri checkLocation, bool checkSuccessStatus = true)
Expand Down
Loading