-
Notifications
You must be signed in to change notification settings - Fork 574
Added multiple id read using stored proc. #5278
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
Conversation
| try | ||
| { | ||
| // Create a DataTable for the table-valued parameter | ||
| var resourceKeysTable = new DataTable(); |
Check warning
Code scanning / CodeQL
Missing Dispose call on local IDisposable Warning
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 2 days ago
In general, to fix a “missing Dispose call” for a local IDisposable, wrap the creation and use of the object in a using statement (or using declaration in newer C#), so that Dispose is called automatically when execution leaves the using scope, even if an exception is thrown.
For this specific case in SqlServerSearchService.cs, the best fix is to wrap the DataTable’s lifetime in a using block. That means replacing the standalone var resourceKeysTable = new DataTable(); and subsequent code that uses resourceKeysTable with a using (var resourceKeysTable = new DataTable()) { ... } block, moving all uses of resourceKeysTable (column definitions, row population, and parameter creation) inside that block. This keeps behavior the same: the DataTable exists for the duration of building the TVP parameter and calling AddWithValue, and then is disposed when the block exits. No additional imports or helper methods are needed, since DataTable and IDisposable are already available via existing using directives.
Concretely, within src/Microsoft.Health.Fhir.SqlServer/Features/Search/SqlServerSearchService.cs, in the shown try block around line 990, introduce a using block starting at line 993, move lines 994–1002 and 1005–1009 inside that block, and adjust indentation accordingly. No other code outside that local section needs to change.
-
Copy modified line R993 -
Copy modified lines R995-R997 -
Copy modified lines R999-R1003 -
Copy modified lines R1005-R1011
| @@ -990,23 +990,25 @@ | ||
| try | ||
| { | ||
| // Create a DataTable for the table-valued parameter | ||
| var resourceKeysTable = new DataTable(); | ||
| resourceKeysTable.Columns.Add("ResourceTypeId", typeof(short)); | ||
| resourceKeysTable.Columns.Add("ResourceId", typeof(string)); | ||
| resourceKeysTable.Columns.Add("Version", typeof(int)); | ||
|
|
||
| // Populate the table with resource keys | ||
| foreach (var resourceId in resourceIds) | ||
| using (var resourceKeysTable = new DataTable()) | ||
| { | ||
| resourceKeysTable.Rows.Add(resourceTypeId, resourceId, DBNull.Value); | ||
| } | ||
| resourceKeysTable.Columns.Add("ResourceTypeId", typeof(short)); | ||
| resourceKeysTable.Columns.Add("ResourceId", typeof(string)); | ||
| resourceKeysTable.Columns.Add("Version", typeof(int)); | ||
|
|
||
| // Populate command to use GetResources stored procedure | ||
| command.CommandType = CommandType.StoredProcedure; | ||
| command.CommandText = "dbo.GetResources"; | ||
| command.Connection = connection; | ||
| // Populate the table with resource keys | ||
| foreach (var resourceId in resourceIds) | ||
| { | ||
| resourceKeysTable.Rows.Add(resourceTypeId, resourceId, DBNull.Value); | ||
| } | ||
|
|
||
| var parameter = command.Parameters.AddWithValue("@ResourceKeys", resourceKeysTable); | ||
| // Populate command to use GetResources stored procedure | ||
| command.CommandType = CommandType.StoredProcedure; | ||
| command.CommandText = "dbo.GetResources"; | ||
| command.Connection = connection; | ||
|
|
||
| var parameter = command.Parameters.AddWithValue("@ResourceKeys", resourceKeysTable); | ||
| } | ||
| parameter.SqlDbType = SqlDbType.Structured; | ||
| parameter.TypeName = "dbo.ResourceKeyList"; | ||
|
|
|
|
||
| // Update to create version 2 | ||
| patient.Name[0].Given = new[] { "Version2" }; | ||
| var updateResult = await _fixture.Mediator.UpsertResourceAsync(patient.ToResourceElement(), Core.Features.Persistence.WeakETag.FromVersionId(saveResult.RawResourceElement.VersionId)); |
Check warning
Code scanning / CodeQL
Useless assignment to local variable Warning test
updateResult
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 2 days ago
In general, when a local variable is assigned a value that is never read, either remove the variable and, if necessary, keep only the side‑effecting expression, or start actually using the variable if the value was intended to be consumed. Here, the test only needs to perform the second upsert to create a new version of the patient resource. The return value (updateResult) is not used in any assertions or subsequent logic.
The best minimal fix that preserves existing behavior is to remove the updateResult variable entirely and just await the upsert call. Concretely, in SqlServerSearchServiceResourceReadOptimizationTests.cs, around line 165, replace:
var updateResult = await _fixture.Mediator.UpsertResourceAsync(patient.ToResourceElement(), Core.Features.Persistence.WeakETag.FromVersionId(saveResult.RawResourceElement.VersionId));with:
await _fixture.Mediator.UpsertResourceAsync(patient.ToResourceElement(), Core.Features.Persistence.WeakETag.FromVersionId(saveResult.RawResourceElement.VersionId));No additional imports, methods, or definitions are needed; we are only removing the unused local variable binding while preserving the awaited call and its side effects.
-
Copy modified line R165
| @@ -162,7 +162,7 @@ | ||
|
|
||
| // Update to create version 2 | ||
| patient.Name[0].Given = new[] { "Version2" }; | ||
| var updateResult = await _fixture.Mediator.UpsertResourceAsync(patient.ToResourceElement(), Core.Features.Persistence.WeakETag.FromVersionId(saveResult.RawResourceElement.VersionId)); | ||
| await _fixture.Mediator.UpsertResourceAsync(patient.ToResourceElement(), Core.Features.Persistence.WeakETag.FromVersionId(saveResult.RawResourceElement.VersionId)); | ||
|
|
||
| // Act - Search for specific version using history | ||
| var query = new List<Tuple<string, string>> |
Description
Previously we optimized reads to use store proc instead of sql generator for simple resource searches. This change works for multiple ids in a single call using get resources.
Related issues
Addresses [issue AB#179104].
Testing
Describe how this change was tested.
FHIR Team Checklist
Semver Change (docs)
Patch|Skip|Feature|Breaking (reason)