Skip to content

Changes to read raw resource from blob #4868

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

Merged
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 @@ -7,6 +7,8 @@
using System.Collections.Generic;
using System.Globalization;
using System.IO;
using System.Linq;
using System.Text;
using System.Threading;
using System.Threading.Tasks;
using Azure;
Expand All @@ -32,8 +34,12 @@
public class BlobStoreTests
{
private static readonly string _resourceFilePath = Path.Combine(AppContext.BaseDirectory, "Fixtures", "resources.ndjson");

private static readonly RecyclableMemoryStreamManager RecyclableMemoryStreamManagerInstance = new RecyclableMemoryStreamManager();
protected const long DefaultStorageIdentifier = 101010101010;
protected const long SecondaryStorageIdentifier = 202020202020;
internal static readonly int EndOfLine = Encoding.UTF8.GetByteCount(Environment.NewLine);
internal static readonly byte FirstOffset = 3;

internal static void InitializeBlobStore(out BlobRawResourceStore blobStore, out TestBlobClient blobClient)
{
Expand All @@ -54,7 +60,7 @@

internal static ResourceWrapper CreateTestResourceWrapper(string rawResourceContent)
{
// We are only interested in the raw resource content from this object, setting dummy values for other ctor arguements
// We are only interested in the raw resource content from this object, setting dummy values for other ctor arguments
var rawResource = Substitute.For<RawResource>(rawResourceContent, FhirResourceFormat.Json, false);
var resourceWrapper = new ResourceWrapper(
"resourceId",
Expand All @@ -71,21 +77,83 @@
return resourceWrapper;
}

internal static IReadOnlyList<ResourceWrapper> GetResourceWrappersWithData()
internal static ResourceWrapper CreateTestResourceWrapper(long storageIdentifier, int offset, string rawResourceContent)
{
var rawResource = Substitute.For<RawResource>(rawResourceContent, FhirResourceFormat.Json, false);
var resourceWrapper = new ResourceWrapper(
"resourceId",
"versionId",
"resourceTypeName",
rawResource,
null,
DateTimeOffset.UtcNow,
false,
null,
null,
null);

resourceWrapper.ResourceStorageIdentifier = storageIdentifier;
resourceWrapper.ResourceStorageOffset = offset;
return resourceWrapper;
}

internal static ResourceWrapper CreateTestResourceWrapper(long storageIdentifier, int offset)
{
var resourceWrapper = new ResourceWrapper(
"resourceId",
"versionId",
"resourceTypeName",
null,
null,
DateTimeOffset.UtcNow,
false,
null,
null,
null);

resourceWrapper.ResourceStorageIdentifier = storageIdentifier;
resourceWrapper.ResourceStorageOffset = offset;
return resourceWrapper;
}

internal IReadOnlyList<ResourceWrapper> GetResourceWrappersWithData()
{
var resourceWrappers = new List<ResourceWrapper>();
using var stream = new FileStream(_resourceFilePath, FileMode.Open, FileAccess.Read);
using var reader = new StreamReader(stream);
using var stream = GetBlobDownloadStreamingPartialResult(0);
using var reader = new StreamReader(stream, new UTF8Encoding(false));
string line;
int characterPosition = FirstOffset;

while ((line = reader.ReadLine()) != null)
{
characterPosition += Encoding.UTF8.GetByteCount(line) + EndOfLine;
var resourceWrapper = CreateTestResourceWrapper(line);
resourceWrappers.Add(resourceWrapper);
}

return resourceWrappers;
}

internal static IReadOnlyList<ResourceWrapper> GetResourceWrappersMetaData(IReadOnlyList<ResourceWrapper> resources)
{
var resourceWrappersWithMetadata = new List<ResourceWrapper>();
int lastOffset = FirstOffset;

for (int i = 0; i < resources.Count; i++)
{
var tempResourceWrapper = CreateTestResourceWrapper(DefaultStorageIdentifier, lastOffset, resources[i].RawResource.Data);
resourceWrappersWithMetadata.Add(tempResourceWrapper);

// No need to process length of last resource, since offset for next resource doesn't exist
if (i < resources.Count - 1)
{
lastOffset = lastOffset + resources[i].RawResource.Data.Length + BlobRawResourceStore.EndOfLine;
}
}

return resourceWrappersWithMetadata;
}

[Fact]
public async Task GivenResourceStore_WhenUploadFails_ThenThrowExceptionWithRightMessage()
{
Expand Down Expand Up @@ -151,4 +219,89 @@
{
Assert.Throws<ArgumentException>(() => BlobUtility.ComputeHashPrefixForBlobName(0));
}

[Fact]
public void TestGetBlobOffsetCombinations()
{
var rawResources = new List<RawResourceLocator>();

var tempRawResource = new RawResourceLocator(DefaultStorageIdentifier, 0);
rawResources.Add(tempRawResource);

tempRawResource = new RawResourceLocator(SecondaryStorageIdentifier, 9000);
rawResources.Add(tempRawResource);

tempRawResource = new RawResourceLocator(DefaultStorageIdentifier, 6000);
rawResources.Add(tempRawResource);

tempRawResource = new RawResourceLocator(SecondaryStorageIdentifier, 3700);
rawResources.Add(tempRawResource);

tempRawResource = new RawResourceLocator(DefaultStorageIdentifier, 2500);
rawResources.Add(tempRawResource);

var resultDictionary = BlobRawResourceStore.GetBlobOffsetCombinations(rawResources);
Assert.NotNull(resultDictionary);
Assert.Equal(2, resultDictionary.Count);
Assert.Equal(3, resultDictionary[DefaultStorageIdentifier].Count);
Assert.Equal(2, resultDictionary[SecondaryStorageIdentifier].Count);
}

private MemoryStream GetBlobDownloadStreamingPartialResult(int offset = 0)
{
var memoryStream = new MemoryStream();
using (var stream = new FileStream(_resourceFilePath, FileMode.Open, FileAccess.Read))
{
stream.Seek(offset, SeekOrigin.Begin);
stream.CopyTo(memoryStream);
}

memoryStream.Position = 0; // Reset the position for further use
return memoryStream; // Return the MemoryStream for manipulation by other methods
}

[Theory]
[InlineData(0)]
[InlineData(1)]
[InlineData(2)]
public async Task GivenResourceStore_WhenReadingSingleResource_ThenValidateContent(int resourceNumber)
{
InitializeBlobStore(out BlobRawResourceStore blobFileStore, out TestBlobClient client);
var resourceWrappersMetaData = GetResourceWrappersMetaData(GetResourceWrappersWithData());

var key = new RawResourceLocator(resourceWrappersMetaData[resourceNumber].ResourceStorageIdentifier, resourceWrappersMetaData[resourceNumber].ResourceStorageOffset);
var expectedContent = resourceWrappersMetaData[resourceNumber].RawResource.Data;

var memoryStream = GetBlobDownloadStreamingPartialResult(0);
client.BlockBlobClient.OpenReadAsync(Arg.Any<BlobOpenReadOptions>(), Arg.Any<CancellationToken>()).Returns(Task.FromResult((Stream)memoryStream));

var result = await blobFileStore.ReadRawResourcesAsync(new List<RawResourceLocator> { key }, CancellationToken.None);

Assert.NotNull(result);
Assert.Single(result);
Assert.Equal(expectedContent, result[key]);
}

[Fact]
public async Task GivenResourceStore_WhenReadingMultipleResources_ThenValidateContents()
{
InitializeBlobStore(out BlobRawResourceStore blobFileStore, out TestBlobClient client);
var resourceWrappersMetaData = GetResourceWrappersMetaData(GetResourceWrappersWithData());

var resourceLocators = resourceWrappersMetaData.Select(wrapper => new RawResourceLocator(wrapper.ResourceStorageIdentifier, wrapper.ResourceStorageOffset)).ToList();
var memoryStream = GetBlobDownloadStreamingPartialResult(0);
client.BlockBlobClient.OpenReadAsync(Arg.Any<BlobOpenReadOptions>(), Arg.Any<CancellationToken>()).Returns(Task.FromResult((Stream)memoryStream));

var result = await blobFileStore.ReadRawResourcesAsync(resourceLocators, CancellationToken.None);

Assert.NotNull(result);
Assert.Equal(resourceWrappersMetaData.Count, result.Count);

foreach (var wrapper in resourceWrappersMetaData)
{
var key = new RawResourceLocator(wrapper.ResourceStorageIdentifier, wrapper.ResourceStorageOffset);
Assert.True(result.ContainsKey(key));

Check notice

Code scanning / CodeQL

Inefficient use of ContainsKey Note

Inefficient use of 'ContainsKey' and
indexer
.

Copilot Autofix

AI 4 months ago

To fix the problem, we need to replace the ContainsKey check followed by the dictionary access with a single call to TryGetValue. This change will ensure that the dictionary is only accessed once, improving the efficiency of the code.

Specifically, we will modify the code in the GivenResourceStore_WhenReadingMultipleResources_ThenValidateContents method to use TryGetValue instead of ContainsKey and the indexer. We will introduce a new variable to hold the value retrieved by TryGetValue and use this variable in the subsequent assertion.

Suggested changeset 1
src/Microsoft.Health.Fhir.Blob.UnitTests/Features/Storage/BlobStoreTests.cs

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/src/Microsoft.Health.Fhir.Blob.UnitTests/Features/Storage/BlobStoreTests.cs b/src/Microsoft.Health.Fhir.Blob.UnitTests/Features/Storage/BlobStoreTests.cs
--- a/src/Microsoft.Health.Fhir.Blob.UnitTests/Features/Storage/BlobStoreTests.cs
+++ b/src/Microsoft.Health.Fhir.Blob.UnitTests/Features/Storage/BlobStoreTests.cs
@@ -302,4 +302,4 @@
             var key = new RawResourceLocator(wrapper.ResourceStorageIdentifier, wrapper.ResourceStorageOffset);
-            Assert.True(result.ContainsKey(key));
-            Assert.Equal(wrapper.RawResource.Data, result[key]);
+            Assert.True(result.TryGetValue(key, out var value));
+            Assert.Equal(wrapper.RawResource.Data, value);
         }
EOF
@@ -302,4 +302,4 @@
var key = new RawResourceLocator(wrapper.ResourceStorageIdentifier, wrapper.ResourceStorageOffset);
Assert.True(result.ContainsKey(key));
Assert.Equal(wrapper.RawResource.Data, result[key]);
Assert.True(result.TryGetValue(key, out var value));
Assert.Equal(wrapper.RawResource.Data, value);
}
Copilot is powered by AI and may make mistakes. Always verify output.
Assert.Equal(wrapper.RawResource.Data, result[key]);
}
}
}
Original file line number Diff line number Diff line change
@@ -1,2 +1,3 @@
{"resourceType":"Patient","id":"01332066-fca8-cce4-d9b7-75b7fd1e2004","meta":{"profile":["http://hl7.org/fhir/us/core/StructureDefinition/us-core-patient"]},"text":{"status":"generated","div":"<div xmlns=\"http://www.w3.org/1999/xhtml\">Generated by <a href=\"https://github.com/synthetichealth/synthea\">Synthea</a>.Version identifier: dd1e3be\n . Person seed: -7060589838743381365 Population seed: 54321</div>"},"extension":[{"url":"http://hl7.org/fhir/us/core/StructureDefinition/us-core-race","extension":[{"url":"ombCategory","valueCoding":{"system":"urn:oid:2.16.840.1.113883.6.238","code":"2106-3","display":"White"}},{"url":"text","valueString":"White"}]},{"url":"http://hl7.org/fhir/us/core/StructureDefinition/us-core-ethnicity","extension":[{"url":"ombCategory","valueCoding":{"system":"urn:oid:2.16.840.1.113883.6.238","code":"2186-5","display":"Not Hispanic or Latino"}},{"url":"text","valueString":"Not Hispanic or Latino"}]},{"url":"http://hl7.org/fhir/StructureDefinition/patient-mothersMaidenName","valueString":"Cicely661 Pacocha935"},{"url":"http://hl7.org/fhir/us/core/StructureDefinition/us-core-birthsex","valueCode":"F"},{"url":"http://hl7.org/fhir/StructureDefinition/patient-birthPlace","valueAddress":{"city":"Pratt","state":"Kansas","country":"US"}},{"url":"http://synthetichealth.github.io/synthea/disability-adjusted-life-years","valueDecimal":0.05295623081989285},{"url":"http://synthetichealth.github.io/synthea/quality-adjusted-life-years","valueDecimal":0.9470437691801071}],"identifier":[{"system":"https://github.com/synthetichealth/synthea","value":"01332066-fca8-cce4-d9b7-75b7fd1e2004"},{"type":{"coding":[{"system":"http://terminology.hl7.org/CodeSystem/v2-0203","code":"MR","display":"Medical Record Number"}],"text":"Medical Record Number"},"system":"http://hospital.smarthealthit.org","value":"01332066-fca8-cce4-d9b7-75b7fd1e2004"},{"type":{"coding":[{"system":"http://terminology.hl7.org/CodeSystem/v2-0203","code":"SS","display":"Social Security Number"}],"text":"Social Security Number"},"system":"http://hl7.org/fhir/sid/us-ssn","value":"999-81-5679"}],"name":[{"use":"official","family":"Yundt842","given":["Donya787","Mikaela760"]}],"telecom":[{"system":"phone","value":"555-907-9875","use":"home"}],"gender":"female","birthDate":"1949-11-14","deceasedDateTime":"1951-02-20T08:15:54-05:00","address":[{"extension":[{"url":"http://hl7.org/fhir/StructureDefinition/geolocation","extension":[{"url":"latitude","valueDecimal":39.155185939682845},{"url":"longitude","valueDecimal":-94.59968151629131}]}],"line":["1045 Schmitt Spur"],"city":"Kansas City","state":"KS","postalCode":"66104","country":"US"}],"maritalStatus":{"coding":[{"system":"http://terminology.hl7.org/CodeSystem/v3-MaritalStatus","code":"S","display":"Never Married"}],"text":"Never Married"},"multipleBirthBoolean":false,"communication":[{"language":{"coding":[{"system":"urn:ietf:bcp:47","code":"en-US","display":"English (United States)"}],"text":"English (United States)"}}]}
{"resourceType":"Patient","id":"01707a0c-9619-ccba-695a-b270744d76c2","meta":{"profile":["http://hl7.org/fhir/us/core/StructureDefinition/us-core-patient"]},"text":{"status":"generated","div":"<div xmlns=\"http://www.w3.org/1999/xhtml\">Generated by <a href=\"https://github.com/synthetichealth/synthea\">Synthea</a>.Version identifier: dd1e3be\n . Person seed: 4999451714027775673 Population seed: 54321</div>"},"extension":[{"url":"http://hl7.org/fhir/us/core/StructureDefinition/us-core-race","extension":[{"url":"ombCategory","valueCoding":{"system":"urn:oid:2.16.840.1.113883.6.238","code":"2106-3","display":"White"}},{"url":"text","valueString":"White"}]},{"url":"http://hl7.org/fhir/us/core/StructureDefinition/us-core-ethnicity","extension":[{"url":"ombCategory","valueCoding":{"system":"urn:oid:2.16.840.1.113883.6.238","code":"2186-5","display":"Not Hispanic or Latino"}},{"url":"text","valueString":"Not Hispanic or Latino"}]},{"url":"http://hl7.org/fhir/StructureDefinition/patient-mothersMaidenName","valueString":"Leeanne840 McLaughlin530"},{"url":"http://hl7.org/fhir/us/core/StructureDefinition/us-core-birthsex","valueCode":"F"},{"url":"http://hl7.org/fhir/StructureDefinition/patient-birthPlace","valueAddress":{"city":"Clear Creek","state":"Kansas","country":"US"}},{"url":"http://synthetichealth.github.io/synthea/disability-adjusted-life-years","valueDecimal":22.041937018127722},{"url":"http://synthetichealth.github.io/synthea/quality-adjusted-life-years","valueDecimal":52.958062981872274}],"identifier":[{"system":"https://github.com/synthetichealth/synthea","value":"01707a0c-9619-ccba-695a-b270744d76c2"},{"type":{"coding":[{"system":"http://terminology.hl7.org/CodeSystem/v2-0203","code":"MR","display":"Medical Record Number"}],"text":"Medical Record Number"},"system":"http://hospital.smarthealthit.org","value":"01707a0c-9619-ccba-695a-b270744d76c2"},{"type":{"coding":[{"system":"http://terminology.hl7.org/CodeSystem/v2-0203","code":"SS","display":"Social Security Number"}],"text":"Social Security Number"},"system":"http://hl7.org/fhir/sid/us-ssn","value":"999-46-1590"},{"type":{"coding":[{"system":"http://terminology.hl7.org/CodeSystem/v2-0203","code":"DL","display":"Driver's license number"}],"text":"Driver's license number"},"system":"urn:oid:2.16.840.1.113883.4.3.25","value":"S99963906"},{"type":{"coding":[{"system":"http://terminology.hl7.org/CodeSystem/v2-0203","code":"PPN","display":"Passport Number"}],"text":"Passport Number"},"system":"http://standardhealthrecord.org/fhir/StructureDefinition/passportNumber","value":"X14757936X"}],"name":[{"use":"official","family":"Reynolds644","given":["Silvana620","Coralee911"],"prefix":["Ms."]}],"telecom":[{"system":"phone","value":"555-907-8526","use":"home"}],"gender":"female","birthDate":"1947-01-14","address":[{"extension":[{"url":"http://hl7.org/fhir/StructureDefinition/geolocation","extension":[{"url":"latitude","valueDecimal":39.20354220841662},{"url":"longitude","valueDecimal":-95.92647404121237}]}],"line":["718 D'Amore Byway Apt 11"],"city":"Rossville","state":"KS","postalCode":"66533","country":"US"}],"maritalStatus":{"coding":[{"system":"http://terminology.hl7.org/CodeSystem/v3-MaritalStatus","code":"S","display":"Never Married"}],"text":"Never Married"},"multipleBirthBoolean":false,"communication":[{"language":{"coding":[{"system":"urn:ietf:bcp:47","code":"en-US","display":"English (United States)"}],"text":"English (United States)"}}]}
{"resourceType":"Patient","id":"01365484-dba4-bba5-e8a6-64a6cc0d2393","meta":{"profile":["http://hl7.org/fhir/us/core/StructureDefinition/us-core-patient"]},"text":{"status":"generated","div":"<div xmlns=\"http://www.w3.org/1999/xhtml\">Generated by <a href=\"https://github.com/synthetichealth/synthea\">Synthea</a>.Version identifier: dd1e3be\n . Person seed: -7156842697965412357 Population seed: 54321</div>"},"extension":[{"url":"http://hl7.org/fhir/us/core/StructureDefinition/us-core-race","extension":[{"url":"ombCategory","valueCoding":{"system":"urn:oid:2.16.840.1.113883.6.238","code":"2106-3","display":"White"}},{"url":"text","valueString":"White"}]},{"url":"http://hl7.org/fhir/us/core/StructureDefinition/us-core-ethnicity","extension":[{"url":"ombCategory","valueCoding":{"system":"urn:oid:2.16.840.1.113883.6.238","code":"2186-5","display":"Not Hispanic or Latino"}},{"url":"text","valueString":"Not Hispanic or Latino"}]},{"url":"http://hl7.org/fhir/StructureDefinition/patient-mothersMaidenName","valueString":"Doris Green"},{"url":"http://hl7.org/fhir/us/core/StructureDefinition/us-core-birthsex","valueCode":"M"},{"url":"http://hl7.org/fhir/StructureDefinition/patient-birthPlace","valueAddress":{"city":"Boston","state":"Massachusets","country":"US"}},{"url":"http://synthetichealth.github.io/synthea/disability-adjusted-life-years","valueDecimal":35.1},{"url":"http://synthetichealth.github.io/synthea/quality-adjusted-life-years","valueDecimal":46.9470437691801071}],"identifier":[{"system":"https://github.com/synthetichealth/synthea","value":"01332066-fca8-cce4-d9b7-75b7fd1e2004"},{"type":{"coding":[{"system":"http://terminology.hl7.org/CodeSystem/v2-0203","code":"MR","display":"Medical Record Number"}],"text":"Medical Record Number"},"system":"http://hospital.smarthealthit.org","value":"01332066-fca8-cce4-d9b7-75b7fd1e2004"},{"type":{"coding":[{"system":"http://terminology.hl7.org/CodeSystem/v2-0203","code":"SS","display":"Social Security Number"}],"text":"Social Security Number"},"system":"http://hl7.org/fhir/sid/us-ssn","value":"999-81-5679"}],"name":[{"use":"official","family":"Green","given":["John","Johnny"]}],"telecom":[{"system":"phone","value":"682-564-9866","use":"home"}],"gender":"male","birthDate":"1995-01-13","address":[{"extension":[{"url":"http://hl7.org/fhir/StructureDefinition/geolocation","extension":[{"url":"latitude","valueDecimal":39.155185939682845},{"url":"longitude","valueDecimal":-94.59968151629131}]}],"line":["1045 Schmitt Spur"],"city":"Boston City","state":"MA","postalCode":"66104","country":"US"}],"maritalStatus":{"coding":[{"system":"http://terminology.hl7.org/CodeSystem/v3-MaritalStatus","code":"M","display":"Married"}],"text":"Married"},"multipleBirthBoolean":false,"communication":[{"language":{"coding":[{"system":"urn:ietf:bcp:47","code":"en-US","display":"English (United States)"}],"text":"English (United States)"}}]}
Loading
Loading