-
Notifications
You must be signed in to change notification settings - Fork 566
Update raw resource feature branch #4897
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 raw resource feature branch #4897
Conversation
+semver: skip
* Added additional error handling to CosmosHealthchecks * Added tests * fix code scanning error in tests * make code easier to review * add test descriptions back to code * removed unnecessary exception logging * Moved logging of CosmosDiagnostic to the property bag * fix code scanning error
* Fix to allow date time to be searched against a resource with a span and if included in the span that resource will be returned. * Updated tests based on recent changes * Updated test fixtures. Removed temporary comments. * Updated test results to match new expectations. Simplified logic for this particular case and addressed pr comments. * Fixed unit test after recent changes. * Updated more tetst based on recent changes.
Update FhirService to use Firely 5.11.3 SDK Refs AB#135366
* add cosmos direct code configuration options * keep existing behavior for Cosmos Direct rediscovery * Delete docs/rest/pers-env.http * Delete docs/rest/export-test.http
* Catch shutdown cancellation exceptions for conformance provider * reenable disposing semaphores * Simplify disposal code * undo semaphore slim disposal changes
Remove resource id from hash Refs AB#135263
* Remove LegacyExportJobWorker #4750 * Remove UseQueueClientJobs configuration since it is the default now, no other possibility. Remove unused variables from CosmosFhirOperationDataStore * Remove classes and stored procedures from Cosmos DB related to LegacyExportJob. Modified unit test in CosmosDBInitializationTests to appropriately check for all incoming list of sp's * Readded necessary legacy code to be able to query a legacy export job. * Remove commented code from test * Add schema version 86, removing unused table ExportJob
Co-authored-by: Ajaj Vanu <[email protected]>
Bumps [AngleSharp](https://github.com/AngleSharp/AngleSharp) from 1.1.2 to 1.2.0. - [Release notes](https://github.com/AngleSharp/AngleSharp/releases) - [Changelog](https://github.com/AngleSharp/AngleSharp/blob/devel/CHANGELOG.md) - [Commits](AngleSharp/AngleSharp@1.1.2...1.2.0) --- updated-dependencies: - dependency-name: AngleSharp dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
--- updated-dependencies: - dependency-name: xunit.runner.visualstudio dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps [Hl7.Fhir.STU3](https://github.com/FirelyTeam/firely-net-sdk) from 5.11.3 to 5.11.4. - [Release notes](https://github.com/FirelyTeam/firely-net-sdk/releases) - [Changelog](https://github.com/FirelyTeam/firely-net-sdk/blob/develop/release-notes.md) - [Commits](FirelyTeam/firely-net-sdk@v5.11.3...v5.11.4) --- updated-dependencies: - dependency-name: Hl7.Fhir.STU3 dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps [Microsoft.Azure.Cosmos](https://github.com/Azure/azure-cosmos-dotnet-v3) and [System.Text.Encodings.Web](https://github.com/dotnet/runtime). These dependencies needed to be updated together. Updates `Microsoft.Azure.Cosmos` from 3.45.2 to 3.47.2 - [Release notes](https://github.com/Azure/azure-cosmos-dotnet-v3/releases) - [Changelog](https://github.com/Azure/azure-cosmos-dotnet-v3/blob/master/changelog.md) - [Commits](Azure/azure-cosmos-dotnet-v3@3.45.2...3.47.2) Updates `System.Text.Encodings.Web` from 9.0.2 to 6.0.0 - [Release notes](https://github.com/dotnet/runtime/releases) - [Commits](dotnet/runtime@v9.0.2...v6.0.0) --- updated-dependencies: - dependency-name: Microsoft.Azure.Cosmos dependency-type: direct:production update-type: version-update:semver-minor - dependency-name: System.Text.Encodings.Web dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
| if (inputResource is not Parameters) | ||
| { | ||
| throw new RequestNotValidException(string.Format(Resources.UnsupportedResourceType, inputResource.GetType().ToString())); | ||
| throw new RequestNotValidException(string.Format(Api.Resources.UnsupportedResourceType, inputResource.GetType().ToString())); |
Check notice
Code scanning / CodeQL
Redundant ToString() call Note
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 8 months ago
To fix the problem, we need to remove the redundant ToString call on the Type object. Specifically, we should modify the line where inputResource.GetType().ToString() is used within the string.Format method. Instead of calling ToString explicitly, we can pass the Type object directly to string.Format, which will implicitly call ToString.
-
Copy modified line R33
| @@ -32,3 +32,3 @@ | ||
| { | ||
| throw new RequestNotValidException(string.Format(Api.Resources.UnsupportedResourceType, inputResource.GetType().ToString())); | ||
| throw new RequestNotValidException(string.Format(Api.Resources.UnsupportedResourceType, inputResource.GetType())); | ||
| } |
| _contextAccessor.RequestContext = fhirRequestContext; | ||
| var result = new BulkDeleteResult(); | ||
| long numDeleted; | ||
| IDictionary<string, long> resourcesDeleted = new Dictionary<string, long>(); |
Check warning
Code scanning / CodeQL
Useless assignment to local variable Warning
resourcesDeleted
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 8 months ago
To fix the problem, we should remove the redundant assignment to resourcesDeleted on line 76. This will clean up the code and remove the unnecessary assignment without changing the existing functionality.
-
Copy modified line R76
| @@ -75,3 +75,3 @@ | ||
| var result = new BulkDeleteResult(); | ||
| IDictionary<string, long> resourcesDeleted = new Dictionary<string, long>(); | ||
| IDictionary<string, long> resourcesDeleted; | ||
| using IScoped<IDeletionService> deleter = _deleterFactory.Invoke(); |
| var resourceKey = new ResourceKey(request.Resource.InstanceType, request.Resource.Id, request.Resource.VersionId); | ||
| ResourceWrapper prevSearchParamResource = await _fhirDataStore.GetAsync(resourceKey, cancellationToken); | ||
| if (prevSearchParamResource != null) | ||
| if (prevSearchParamResource != null && prevSearchParamResource.IsDeleted == false) |
Check notice
Code scanning / CodeQL
Unnecessarily complex Boolean expression Note
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 8 months ago
To fix the problem, we need to simplify the Boolean expression prevSearchParamResource.IsDeleted == false to !prevSearchParamResource.IsDeleted. This change will make the code more readable and maintainable without altering its functionality.
- Locate the file
src/Microsoft.Health.Fhir.Core/Features/Search/Parameters/CreateOrUpdateSearchParameterBehavior.cs. - Find the line containing
prevSearchParamResource.IsDeleted == false. - Replace it with
!prevSearchParamResource.IsDeleted.
-
Copy modified line R55
| @@ -54,3 +54,3 @@ | ||
| ResourceWrapper prevSearchParamResource = await _fhirDataStore.GetAsync(resourceKey, cancellationToken); | ||
| if (prevSearchParamResource != null && prevSearchParamResource.IsDeleted == false) | ||
| if (prevSearchParamResource != null && !prevSearchParamResource.IsDeleted) | ||
| { |
| EnsureArg.IsNotNull(coreFeaturesConfiguration?.Value, nameof(coreFeaturesConfiguration)); | ||
|
|
||
| _mediator = mediator; | ||
| _coreFeaturesConfiguration = coreFeaturesConfiguration.Value; |
Check warning
Code scanning / CodeQL
Dereferenced variable may be null Warning
coreFeaturesConfiguration
this
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 8 months ago
To fix the problem, we need to ensure that coreFeaturesConfiguration is not null before it is dereferenced. This can be done by adding an explicit null check for coreFeaturesConfiguration in the constructor. This way, we can be certain that coreFeaturesConfiguration is not null when it is assigned to _coreFeaturesConfiguration.
-
Copy modified line R38
| @@ -37,2 +37,3 @@ | ||
| EnsureArg.IsNotNull(mediator, nameof(mediator)); | ||
| EnsureArg.IsNotNull(coreFeaturesConfiguration, nameof(coreFeaturesConfiguration)); | ||
| EnsureArg.IsNotNull(coreFeaturesConfiguration?.Value, nameof(coreFeaturesConfiguration)); |
Check warning
Code scanning / CodeQL
Useless assignment to local variable Warning
reindexJobId
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 8 months ago
To fix the problem, we need to remove the assignment to the reindexJobId variable since it is not used in the method. This can be done by modifying the tuple deconstruction to only include the activeReindexJobs variable, which is the only variable needed for the method's logic.
-
Copy modified line R240
| @@ -239,3 +239,3 @@ | ||
| { | ||
| (var activeReindexJobs, var reindexJobId) = await fhirOperationDataStore.Value.CheckActiveReindexJobsAsync(cancellationToken); | ||
| var (activeReindexJobs, _) = await fhirOperationDataStore.Value.CheckActiveReindexJobsAsync(cancellationToken); | ||
| if (activeReindexJobs) |
| .AcceptVisitor(DateTimeEqualityRewriter.Instance) | ||
| .AcceptVisitor(FlatteningRewriter.Instance) | ||
| .AcceptVisitor(UntypedReferenceRewriter.Instance) |
Check warning
Code scanning / CodeQL
Useless upcast Warning
null
SqlExpressionRewriterWithInitialContext - the conversion can be done implicitly.
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 8 months ago
To fix the problem, we need to remove the redundant cast from StringOverflowRewriter.Instance or LegacyStringOverflowRewriter.Instance to SqlExpressionRewriterWithInitialContext<object>. This can be done by simply removing the cast and keeping the rest of the code unchanged.
- Remove the explicit cast on line 1512.
- Ensure that the functionality remains the same by keeping the rest of the code intact.
-
Copy modified line R1512 -
Copy modified line R1514
| @@ -1511,5 +1511,5 @@ | ||
| .AcceptVisitor( | ||
| (SqlExpressionRewriterWithInitialContext<object>)(_schemaInformation.Current >= SchemaVersionConstants.PartitionedTables | ||
| _schemaInformation.Current >= SchemaVersionConstants.PartitionedTables | ||
| ? StringOverflowRewriter.Instance | ||
| : LegacyStringOverflowRewriter.Instance)) | ||
| : LegacyStringOverflowRewriter.Instance) | ||
| .AcceptVisitor(NumericRangeRewriter.Instance) |
| foreach (var resourceType in relatedResourceTypes) | ||
| { | ||
| if (!relatedResources.TryGetValue(resourceType, out _) && _relatedResourcesByResourceType.TryGetValue(resourceType, out var relatedResourcesByResourceType)) | ||
| { | ||
| var resources = relatedResourcesByResourceType.Where( | ||
| x => | ||
| { | ||
| switch (x.TypeName) | ||
| { | ||
| case KnownResourceTypes.DiagnosticReport: | ||
| return patientReferences.Contains(((DiagnosticReport)x).Subject?.Reference); | ||
|
|
||
| case KnownResourceTypes.Group: | ||
| return ((Group)x).Member.Any(y => patientReferences.Contains(y.Entity?.Reference)); | ||
|
|
||
| case KnownResourceTypes.MedicationDispense: | ||
| return patientReferences.Contains(((MedicationDispense)x).Subject?.Reference); | ||
|
|
||
| case KnownResourceTypes.MedicationRequest: | ||
| return patientReferences.Contains(((MedicationRequest)x).Subject?.Reference); | ||
|
|
||
| case KnownResourceTypes.Observation: | ||
| return patientReferences.Contains(((Observation)x).Subject?.Reference); | ||
|
|
||
| case KnownResourceTypes.Organization: | ||
| return patientResources.Any(y => ((Patient)y).ManagingOrganization?.Reference?.Equals($"{x.TypeName}/{x.Id}") ?? false); | ||
|
|
||
| case KnownResourceTypes.Practitioner: | ||
| return patientResources.Any(y => ((Patient)y).GeneralPractitioner.Any(z => z.Reference?.Equals($"{x.TypeName}/{x.Id}") ?? false)); | ||
|
|
||
| default: | ||
| throw new InvalidOperationException($"Unsupported resource type: {resourceType}"); | ||
| } | ||
| }); | ||
|
|
||
| if (resources.Any()) | ||
| { | ||
| relatedResources.Add(resourceType, new List<Resource>(resources)); | ||
| } | ||
| } | ||
| } |
Check notice
Code scanning / CodeQL
Missed opportunity to use Where Note test
implicitly filters its target sequence
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 8 months ago
To fix the problem, we will refactor the foreach loop on line 154 to use the Where method from LINQ. This will make the filtering condition explicit and improve the readability of the code. Specifically, we will apply the Where method to relatedResourceTypes to filter out the elements that do not meet the condition before iterating over them.
-
Copy modified line R154 -
Copy modified lines R156-R160 -
Copy modified lines R162-R163 -
Copy modified lines R165-R166 -
Copy modified lines R168-R169 -
Copy modified lines R171-R172 -
Copy modified lines R174-R175 -
Copy modified lines R177-R178 -
Copy modified lines R180-R181 -
Copy modified lines R183-R186 -
Copy modified lines R188-R190
| @@ -153,41 +153,39 @@ | ||
| var relatedResources = new Dictionary<string, IList<Resource>>(StringComparer.OrdinalIgnoreCase); | ||
| foreach (var resourceType in relatedResourceTypes) | ||
| foreach (var resourceType in relatedResourceTypes.Where(resourceType => !relatedResources.TryGetValue(resourceType, out _) && _relatedResourcesByResourceType.TryGetValue(resourceType, out var relatedResourcesByResourceType))) | ||
| { | ||
| if (!relatedResources.TryGetValue(resourceType, out _) && _relatedResourcesByResourceType.TryGetValue(resourceType, out var relatedResourcesByResourceType)) | ||
| { | ||
| var resources = relatedResourcesByResourceType.Where( | ||
| x => | ||
| var relatedResourcesByResourceType = _relatedResourcesByResourceType[resourceType]; | ||
| var resources = relatedResourcesByResourceType.Where( | ||
| x => | ||
| { | ||
| switch (x.TypeName) | ||
| { | ||
| switch (x.TypeName) | ||
| { | ||
| case KnownResourceTypes.DiagnosticReport: | ||
| return patientReferences.Contains(((DiagnosticReport)x).Subject?.Reference); | ||
| case KnownResourceTypes.DiagnosticReport: | ||
| return patientReferences.Contains(((DiagnosticReport)x).Subject?.Reference); | ||
|
|
||
| case KnownResourceTypes.Group: | ||
| return ((Group)x).Member.Any(y => patientReferences.Contains(y.Entity?.Reference)); | ||
| case KnownResourceTypes.Group: | ||
| return ((Group)x).Member.Any(y => patientReferences.Contains(y.Entity?.Reference)); | ||
|
|
||
| case KnownResourceTypes.MedicationDispense: | ||
| return patientReferences.Contains(((MedicationDispense)x).Subject?.Reference); | ||
| case KnownResourceTypes.MedicationDispense: | ||
| return patientReferences.Contains(((MedicationDispense)x).Subject?.Reference); | ||
|
|
||
| case KnownResourceTypes.MedicationRequest: | ||
| return patientReferences.Contains(((MedicationRequest)x).Subject?.Reference); | ||
| case KnownResourceTypes.MedicationRequest: | ||
| return patientReferences.Contains(((MedicationRequest)x).Subject?.Reference); | ||
|
|
||
| case KnownResourceTypes.Observation: | ||
| return patientReferences.Contains(((Observation)x).Subject?.Reference); | ||
| case KnownResourceTypes.Observation: | ||
| return patientReferences.Contains(((Observation)x).Subject?.Reference); | ||
|
|
||
| case KnownResourceTypes.Organization: | ||
| return patientResources.Any(y => ((Patient)y).ManagingOrganization?.Reference?.Equals($"{x.TypeName}/{x.Id}") ?? false); | ||
| case KnownResourceTypes.Organization: | ||
| return patientResources.Any(y => ((Patient)y).ManagingOrganization?.Reference?.Equals($"{x.TypeName}/{x.Id}") ?? false); | ||
|
|
||
| case KnownResourceTypes.Practitioner: | ||
| return patientResources.Any(y => ((Patient)y).GeneralPractitioner.Any(z => z.Reference?.Equals($"{x.TypeName}/{x.Id}") ?? false)); | ||
| case KnownResourceTypes.Practitioner: | ||
| return patientResources.Any(y => ((Patient)y).GeneralPractitioner.Any(z => z.Reference?.Equals($"{x.TypeName}/{x.Id}") ?? false)); | ||
|
|
||
| default: | ||
| throw new InvalidOperationException($"Unsupported resource type: {resourceType}"); | ||
| } | ||
| }); | ||
| default: | ||
| throw new InvalidOperationException($"Unsupported resource type: {resourceType}"); | ||
| } | ||
| }); | ||
|
|
||
| if (resources.Any()) | ||
| { | ||
| relatedResources.Add(resourceType, new List<Resource>(resources)); | ||
| } | ||
| if (resources.Any()) | ||
| { | ||
| relatedResources.Add(resourceType, new List<Resource>(resources)); | ||
| } |
| foreach (var jsonInt in lineItem.Item2) | ||
| { | ||
| var json = jsonInt; | ||
| var (resourceType, resourceId) = ParseJson(ref json, Guid.NewGuid().ToString()); | ||
| var entry = GetEntry(json, resourceType, resourceId); | ||
| entries.Add(entry); | ||
| } |
Check notice
Code scanning / CodeQL
Missed opportunity to use Select Note
maps its iteration variable to another variable
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 8 months ago
To fix the problem, we should replace the foreach loop with a LINQ Select method to transform the elements of lineItem.Item2 before iterating over them. This will make the code more readable by explicitly showing the transformation applied to each element.
- Replace the
foreachloop that iterates overlineItem.Item2with aSelectmethod that transforms each element. - Update the loop to iterate over the transformed sequence returned by
Select.
-
Copy modified line R297 -
Copy modified lines R301-R302
| @@ -296,4 +296,3 @@ | ||
|
|
||
| var entries = new List<string>(); | ||
| foreach (var jsonInt in lineItem.Item2) | ||
| var entries = lineItem.Item2.Select(jsonInt => | ||
| { | ||
| @@ -301,5 +300,4 @@ | ||
| var (resourceType, resourceId) = ParseJson(ref json, Guid.NewGuid().ToString()); | ||
| var entry = GetEntry(json, resourceType, resourceId); | ||
| entries.Add(entry); | ||
| } | ||
| return GetEntry(json, resourceType, resourceId); | ||
| }).ToList(); | ||
|
|
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR merges the latest upstream changes into the raw resource feature branch, updating several Architecture Decision Records (ADRs), documentation, and build pipeline scripts. Key changes include:
- Adding new ADR documents that describe the $includes operation, _include support in delete requests, resource ID integer mapping, and resource table refactoring.
- Updating build and CI/CD pipelines to include Linux-based jobs and streamline Docker build parameters.
- Enhancing documentation by adding instructions for connecting to a SQL database and updating the project README and PR template.
Reviewed Changes
Copilot reviewed 236 out of 241 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| docs/arch/adr-2503-Bundle-include-operation.md | New ADR for implementing the $includes operation. |
| docs/arch/Readme.md | Added ADR documentation guidelines. |
| docs/arch/Proposals/adr-2502-_include-on-delete.md | New ADR for supporting the _include parameter in delete requests. |
| docs/arch/Proposals/adr-2502-ResourceIdIntMap.md | New ADR for introducing integer-based resource mapping. |
| docs/arch/Proposals/adr-2502-Resource-table-refactor.md | New ADR detailing resource table refactoring and index optimization. |
| docs/HowToConnectSQLDatabase.md | New documentation on connecting to a SQL database. |
| build/pr-pipeline.yml | Updated to include a Linux_dotnet8 job and corresponding pool configuration. |
| build/jobs/provision-deploy.yml | Added registryName variable in deployment job. |
| build/jobs/docker-build-push.yml | Updated Docker build script with enhanced multi-platform support and removed the multiplePlatforms parameter. |
| build/jobs/docker-build-all.yml | Removed the multiplePlatforms parameter from composite builds. |
| build/ci-pipeline.yml | Adjusted CI pipeline job to use a Linux agent. |
| build/ci-deploy.yml | Removed the multiplePlatforms parameter from the deploy pipeline. |
| build/build-image.yml | Removed the multiplePlatforms parameter. |
| README.md | Updated to include a link for connecting to a SQL Database. |
| .github/pull_request_template.md | Minor template updates regarding ADR and squash-merge requirements. |
Files not reviewed (5)
- Directory.Packages.props: Language not supported
- build/docker/Dockerfile: Language not supported
- build/dotnet8-compat/global.json: Language not supported
- global.json: Language not supported
- samples/templates/default-azuredeploy-docker.json: Language not supported
| In order to support splitting the table, we move the raw binary of the resource to it's own table, so that the binary will not have be to be moved when resource meta data moves from the current to the history table. | ||
|
|
Copilot
AI
Apr 1, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a grammatical error on this line: consider replacing "it's" with "its" and removing the extraneous "be" (i.e., '...will not have to be moved...').
| In order to support splitting the table, we move the raw binary of the resource to it's own table, so that the binary will not have be to be moved when resource meta data moves from the current to the history table. | |
| In order to support splitting the table, we move the raw binary of the resource to its own table, so that the binary will not have to be moved when resource meta data moves from the current to the history table. |
| The above schema diagrams shows to overall reduction of one index in the new tables. | ||
|
|
Copilot
AI
Apr 1, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] The sentence is awkwardly worded; consider rephrasing it to 'The above schema diagrams show the overall reduction of one index in the new tables.'
| The above schema diagrams shows to overall reduction of one index in the new tables. | |
| The above schema diagrams show the overall reduction of one index in the new tables. |
a9ab0ca
into
feature-branch/raw-resource-split
Description
Merging latest from #4731 to this feature branch.
Related issues
Addresses [issue #].
https://microsofthealth.visualstudio.com/Health/_workitems/edit/149048
Testing
Describe how this change was tested.
FHIR Team Checklist
Semver Change (docs)
Patch|Skip|Feature|Breaking (reason)