LNK-4705: Resource queries not populated with all parameters - #1368
Conversation
- ParameterQueryFactory handles paged parameters better. - ResourceIdParameterFactory queries for resourceIds from the DB before constructing the query.
…tory to confirm correct handling of paging scenarios
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughThis pull request refactors parameter factories and query processing to introduce logging, make methods asynchronous where needed, and centralize resource ID retrieval through a new database query interface. SearchParams objects are replaced with KeyValuePair collections, and QueryListProcessor adopts dependency injection for the query interface to support dynamic resource ID fetching. Changes
Sequence Diagram(s)sequenceDiagram
participant ParameterQueryFactory
participant ResourceIdParameterFactory
participant IDataAcquisitionLogQueries
participant Database
participant QueryListProcessor
ParameterQueryFactory->>ResourceIdParameterFactory: Build(resourceIds param, dataAcquisitionLogQueries)
ResourceIdParameterFactory->>IDataAcquisitionLogQueries: GetResourceIdsForReportPatient(correlationId, facilityId, resourceType)
IDataAcquisitionLogQueries->>Database: Query FhirQueryResourceTypes, join logs
Database-->>IDataAcquisitionLogQueries: Return resource IDs
IDataAcquisitionLogQueries-->>ResourceIdParameterFactory: List<string> IDs or null
ResourceIdParameterFactory->>ResourceIdParameterFactory: Validate & chunk into pages if paged=true
ResourceIdParameterFactory-->>ParameterQueryFactory: ParameterFactoryResult (paged or joined)
ParameterQueryFactory->>ParameterQueryFactory: Build KeyValuePair parameter lists
ParameterQueryFactory-->>QueryListProcessor: ParameterQueryFactoryResult (Singular or Paged)
QueryListProcessor->>QueryListProcessor: CreateDataAcquisitionLogAsync for each query
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
DotNet/DataAcquisition.Domain/Application/Factories/ParameterFactories/VariableParameterFactory.cs (1)
47-58: Pre-existing bug:DateTime.Subtract()result is discarded.Line 52:
date.Subtract(ts)returns a newDateTimevalue but the result is not assigned.DateTimeis a value type, soSubtract()does not modifydatein place. The lookback calculation is effectively ignored.While this appears to be pre-existing code, it directly impacts the correctness of the lookback functionality this PR aims to improve.
Suggested fix
private static string CalculateLookBackStartDate(VariableParameter parameter, ScheduledReport scheduledReport, string lookback) { TimeSpan ts = XmlConvert.ToTimeSpan(lookback); var date = scheduledReport.StartDate; - date.Subtract(ts); + date = date.Subtract(ts); var dateOnly = date.Date.ToString("yyyy-MM-dd");
🤖 Fix all issues with AI agents
In
@DotNet/DataAcquisition.Domain/Application/Factories/ParameterFactories/LiteralParameterFactory.cs:
- Around line 11-15: The static constructor for LiteralParameterFactory disposes
the LoggerFactory immediately (using LoggerFactory.Create inside a using)
causing the created _logger to reference a disposed provider; replace this with
dependency-injected ILogger<LiteralParameterFactory> by adding a constructor
that accepts ILogger<LiteralParameterFactory> and assign it to _logger
(preferred), or if you must keep static logging remove the using and make the
LoggerFactory/ILogger static singletons that are not disposed here; update
references to _logger accordingly and remove the temporary LoggerFactory.Create
call in the static constructor.
In
@DotNet/DataAcquisition.Domain/Application/Factories/ParameterFactories/VariableParameterFactory.cs:
- Around line 15-19: The static constructor for VariableParameterFactory creates
a LoggerFactory inside a using block which disposes it immediately; replace this
pattern by accepting an ILogger<VariableParameterFactory> via dependency
injection: remove the static ctor and static LoggerFactory usage, change _logger
to be an instance field (ILogger<VariableParameterFactory> _logger), add a ctor
that takes ILogger<VariableParameterFactory> logger and assigns it to _logger,
and update any callers/registrations so the factory is resolved from DI with the
logger provided.
In
@DotNet/DataAcquisition.Domain/Application/Queries/DataAcquisitionLogQueries.cs:
- Around line 103-121: The try/catch around the loop is incorrect because no
JSON deserialization occurs (resourceReferences is a List<string>), so remove
the surrounding try and the JsonException catch and keep the null check and
filtering logic (symbols: logsWithResources, resourceReferences,
resourceTypePrefix, result); if there was an intended guard for unexpected
failures instead, replace the JsonException catch with a generic catch
(Exception ex) and add a clarifying comment explaining what unexpected scenario
it protects against and include contextual logging of resourceReferences and ex
in the log message.
- Around line 89-98: In GetResourceIdsForReportPatient, replace the unsafe
Enum.Parse call with Enum.TryParse to handle invalid resourceType values
gracefully: attempt to parse resourceType into a ResourceType using TryParse
(optionally ignore case), and if parsing fails either log/return an empty
List<string> or throw a clear ArgumentException indicating the invalid
resourceType; ensure the rest of the method uses the parsed enum only after
successful TryParse so you don’t risk an unhandled exception from bad external
input.
In @DotNet/DataAcquisition.Domain/Application/Services/QueryListProcessor.cs:
- Around line 65-73: The constructor for QueryListProcessor assigns
_dataAcquisitionLogQueries without null-check like the other dependencies; add
an ArgumentNullException check for the constructor parameter
dataAcquisitionLogQueries and throw new
ArgumentNullException(nameof(dataAcquisitionLogQueries)) if null, mirroring the
pattern used for _logger, _fhirRepo, _kafkaProducer, _referenceResourceService,
and _dataAcquisitionLogManager to prevent potential NullReferenceException when
using _dataAcquisitionLogQueries.
In
@DotNet/ServiceTests/IntegrationTests/DataAcquisition/Factories/ResourceIdParameterFactoryTests.cs:
- Around line 51-80: Rename the test method to correct the typo: change
Build_WhenPagedSetAndResultsLessThenPageSize_ReturnsJoinedEntries to
Build_WhenPagedSetAndResultsLessThanPageSize_ReturnsJoinedEntries; update the
test method identifier (the method name in ResourceIdParameterFactoryTests) so
the new name is used everywhere it's referenced (e.g., test runner/attributes)
to keep naming consistent.
🧹 Nitpick comments (2)
DotNet/ServiceTests/IntegrationTests/DataAcquisition/Factories/ParameterQueryFactoryTests.cs (1)
109-131: Consider adding a test for VariableParameter handling.The current tests cover
LiteralParameterandResourceIdsParameter, butVariableParameteris also supported by the factory. Consider adding a test that includes aVariableParameterto ensure complete branch coverage.DotNet/DataAcquisition.Domain/Application/Services/QueryListProcessor.cs (1)
135-135: Unused variableresourceIds.The variable
resourceIdsis declared but never used in the method.- List<string> resourceIds = null;
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (11)
DotNet/DataAcquisition.Domain/Application/Factories/ParameterFactories/LiteralParameterFactory.csDotNet/DataAcquisition.Domain/Application/Factories/ParameterFactories/ResourceIdParameterFactory.csDotNet/DataAcquisition.Domain/Application/Factories/ParameterFactories/VariableParameterFactory.csDotNet/DataAcquisition.Domain/Application/Factories/QueryFactories/ParameterQueryFactory.csDotNet/DataAcquisition.Domain/Application/Models/Factory/ParameterQuery/PagedParameterQueryFactoryResult.csDotNet/DataAcquisition.Domain/Application/Models/Factory/ParameterQuery/ParameterFactoryResult.csDotNet/DataAcquisition.Domain/Application/Models/Factory/ParameterQuery/SingularParameterQueryFactoryResult.csDotNet/DataAcquisition.Domain/Application/Queries/DataAcquisitionLogQueries.csDotNet/DataAcquisition.Domain/Application/Services/QueryListProcessor.csDotNet/ServiceTests/IntegrationTests/DataAcquisition/Factories/ParameterQueryFactoryTests.csDotNet/ServiceTests/IntegrationTests/DataAcquisition/Factories/ResourceIdParameterFactoryTests.cs
🧰 Additional context used
📓 Path-based instructions (2)
**/*.cs
⚙️ CodeRabbit configuration file
**/*.cs: TheHtmlInputSanitizerclass'sSanitize()andSanitizeAndRemove()methods should be used when dealing withstringquery parameters from REST requests.
Files:
DotNet/DataAcquisition.Domain/Application/Models/Factory/ParameterQuery/ParameterFactoryResult.csDotNet/ServiceTests/IntegrationTests/DataAcquisition/Factories/ResourceIdParameterFactoryTests.csDotNet/ServiceTests/IntegrationTests/DataAcquisition/Factories/ParameterQueryFactoryTests.csDotNet/DataAcquisition.Domain/Application/Factories/ParameterFactories/LiteralParameterFactory.csDotNet/DataAcquisition.Domain/Application/Models/Factory/ParameterQuery/SingularParameterQueryFactoryResult.csDotNet/DataAcquisition.Domain/Application/Models/Factory/ParameterQuery/PagedParameterQueryFactoryResult.csDotNet/DataAcquisition.Domain/Application/Queries/DataAcquisitionLogQueries.csDotNet/DataAcquisition.Domain/Application/Factories/ParameterFactories/ResourceIdParameterFactory.csDotNet/DataAcquisition.Domain/Application/Factories/QueryFactories/ParameterQueryFactory.csDotNet/DataAcquisition.Domain/Application/Factories/ParameterFactories/VariableParameterFactory.csDotNet/DataAcquisition.Domain/Application/Services/QueryListProcessor.cs
**
⚙️ CodeRabbit configuration file
**: Pull requests that have "TECH_DEBT" in the title should only contain changes related to typos, unused code, linter/IDE suggestions, swagger specification updates,
and logging improvements. These TECH_DEBT PRs must not affect core functionality. All PRs that are not considered technical debt must include information on what
testing was performed in the description of the PR. If it does not, ask the author to provide details on what testing was performed.
When reviewing code, suggest unit tests using XUnit in the following scenarios:
- If/Else or Switch/Case blocks are introduced or modified — ensure each branch has a corresponding unit test.
- Logic that depends on service or interface configuration — suggest tests to validate different implementations are correctly resolved.
- No network activity (HTTP calls, sockets, etc.) should appear in unit tests. Recommend using mocks (via Moq) for any external communication.
Large unit tests should be avoided; keeping unit tests small and focused on targeted business logic (i.e. string sanitization)
**: Pull requests that have DOCS in the title should only contain changes related to documentation within the /docs folder or in .md files through-out the code-base. The description
of the PR should specify what documentation was updated. Documentation updates should use EventCatalog.dev structure, where service-specific functionality should be described
in the service's index.mdx (i.e. /services/XXX/index.mdx or /domains/XXX/services/YYY/index.mdx). Configurations that are shared by multiple services should be
reflected in the /docs/docs/config files.
Files:
DotNet/DataAcquisition.Domain/Application/Models/Factory/ParameterQuery/ParameterFactoryResult.csDotNet/ServiceTests/IntegrationTests/DataAcquisition/Factories/ResourceIdParameterFactoryTests.csDotNet/ServiceTests/IntegrationTests/DataAcquisition/Factories/ParameterQueryFactoryTests.csDotNet/DataAcquisition.Domain/Application/Factories/ParameterFactories/LiteralParameterFactory.csDotNet/DataAcquisition.Domain/Application/Models/Factory/ParameterQuery/SingularParameterQueryFactoryResult.csDotNet/DataAcquisition.Domain/Application/Models/Factory/ParameterQuery/PagedParameterQueryFactoryResult.csDotNet/DataAcquisition.Domain/Application/Queries/DataAcquisitionLogQueries.csDotNet/DataAcquisition.Domain/Application/Factories/ParameterFactories/ResourceIdParameterFactory.csDotNet/DataAcquisition.Domain/Application/Factories/QueryFactories/ParameterQueryFactory.csDotNet/DataAcquisition.Domain/Application/Factories/ParameterFactories/VariableParameterFactory.csDotNet/DataAcquisition.Domain/Application/Services/QueryListProcessor.cs
🧠 Learnings (4)
📚 Learning: 2025-03-20T22:11:00.226Z
Learnt from: amphillipsLGC
Repo: lantanagroup/link-cloud PR: 737
File: DotNet/Admin.BFF/Presentation/Endpoints/Aggregation/Handlers/Report/GetReportSummaries.cs:23-23
Timestamp: 2025-03-20T22:11:00.226Z
Learning: The facilityId validation in GetReportSummaries.Handle method in DotNet/Admin.BFF/Presentation/Endpoints/Aggregation/Handlers/Report/GetReportSummaries.cs will be implemented in a future phase of work by amphillipsLGC.
Applied to files:
DotNet/DataAcquisition.Domain/Application/Queries/DataAcquisitionLogQueries.cs
📚 Learning: 2025-09-09T19:15:39.938Z
Learnt from: smailliwcs
Repo: lantanagroup/link-cloud PR: 1070
File: DotNet/Report/KafkaProducers/AuditableEventOccurredProducer.cs:18-22
Timestamp: 2025-09-09T19:15:39.938Z
Learning: "X-Correlation-Id" is defined as a constant in KafkaConstants.HeaderConstants.CorrelationId in DotNet/Shared/Settings/KafkaConstants.cs and is used consistently across all Kafka producers in the LantanaGroup.Link system.
Applied to files:
DotNet/DataAcquisition.Domain/Application/Services/QueryListProcessor.cs
📚 Learning: 2025-09-09T19:15:39.938Z
Learnt from: smailliwcs
Repo: lantanagroup/link-cloud PR: 1070
File: DotNet/Report/KafkaProducers/AuditableEventOccurredProducer.cs:18-22
Timestamp: 2025-09-09T19:15:39.938Z
Learning: In the LantanaGroup.Link codebase, Headers headers = [] syntax compiles with their version of Confluent.Kafka library, and this initialization pattern should be used for consistency across the codebase.
Applied to files:
DotNet/DataAcquisition.Domain/Application/Services/QueryListProcessor.cs
📚 Learning: 2025-09-12T14:22:06.888Z
Learnt from: smailliwcs
Repo: lantanagroup/link-cloud PR: 1070
File: DotNet/Report/Application/Extensions/KafkaProducerRegistration.cs:18-18
Timestamp: 2025-09-12T14:22:06.888Z
Learning: The team prefers to consolidate duplicate DI registrations (like IKafkaProducerFactory<string, AuditEventMessage>) into shared extension methods as part of larger DI refactoring efforts, rather than addressing them piecemeal in feature PRs.
Applied to files:
DotNet/DataAcquisition.Domain/Application/Services/QueryListProcessor.cs
🧬 Code graph analysis (8)
DotNet/DataAcquisition.Domain/Application/Models/Factory/ParameterQuery/ParameterFactoryResult.cs (2)
DotNet/DataAcquisition.Domain/Application/Factories/ParameterFactories/LiteralParameterFactory.cs (1)
ParameterFactoryResult(17-26)DotNet/DataAcquisition.Domain/Application/Factories/ParameterFactories/VariableParameterFactory.cs (1)
ParameterFactoryResult(21-45)
DotNet/ServiceTests/IntegrationTests/DataAcquisition/Factories/ResourceIdParameterFactoryTests.cs (3)
DotNet/ServiceTests/IntegrationTests/DataAcquisition/Factories/ParameterQueryFactoryTests.cs (4)
Fact(22-45)Fact(47-81)Fact(83-107)Fact(109-131)DotNet/DataAcquisition.Domain/Infrastructure/Models/QueryConfig/Parameter/ResourceIdsParameter.cs (1)
ResourceIdsParameter(5-11)DotNet/DataAcquisition.Domain/Application/Models/Api/Requests/GetPatientDataRequest.cs (1)
GetPatientDataRequest(7-13)
DotNet/DataAcquisition.Domain/Application/Factories/ParameterFactories/LiteralParameterFactory.cs (3)
DotNet/ServiceTests/IntegrationTests/Census/CensusIntegrationTestFixture.cs (1)
ILogger(143-143)DotNet/DataAcquisition.Domain/Application/Factories/ParameterFactories/VariableParameterFactory.cs (1)
ParameterFactoryResult(21-45)DotNet/DataAcquisition.Domain/Infrastructure/Models/QueryConfig/Parameter/LiteralParameter.cs (1)
LiteralParameter(5-10)
DotNet/DataAcquisition.Domain/Application/Models/Factory/ParameterQuery/SingularParameterQueryFactoryResult.cs (1)
DotNet/DataAcquisition.Domain/Application/Services/FhirApi/FhirApiService.cs (1)
SearchParams(257-270)
DotNet/DataAcquisition.Domain/Application/Factories/ParameterFactories/ResourceIdParameterFactory.cs (3)
DotNet/DataAcquisition.Domain/Application/Factories/ParameterFactories/LiteralParameterFactory.cs (1)
ParameterFactoryResult(17-26)DotNet/DataAcquisition.Domain/Infrastructure/Models/QueryConfig/Parameter/ResourceIdsParameter.cs (1)
ResourceIdsParameter(5-11)DotNet/DataAcquisition.Domain/Application/Models/Api/Requests/GetPatientDataRequest.cs (1)
GetPatientDataRequest(7-13)
DotNet/DataAcquisition.Domain/Application/Factories/QueryFactories/ParameterQueryFactory.cs (10)
DotNet/ServiceTests/IntegrationTests/Census/CensusIntegrationTestFixture.cs (1)
ILogger(143-143)DotNet/DataAcquisition.Domain/Application/Factories/ParameterFactories/ResourceIdParameterFactory.cs (3)
Task(18-42)ResourceIdParameterFactory(9-43)ResourceIdParameterFactory(13-17)DotNet/DataAcquisition.Domain/Application/Queries/DataAcquisitionLogQueries.cs (15)
Task(38-38)Task(45-45)Task(59-59)Task(61-61)Task(63-63)Task(65-65)Task(67-67)Task(69-69)Task(71-71)Task(73-73)Task(89-124)Task(126-192)Task(194-213)Task(215-271)Task(273-283)DotNet/DataAcquisition.Domain/Application/Models/Api/Requests/GetPatientDataRequest.cs (1)
GetPatientDataRequest(7-13)DotNet/DataAcquisition.Domain/Application/Factories/ParameterFactories/LiteralParameterFactory.cs (3)
ParameterFactoryResult(17-26)LiteralParameterFactory(7-27)LiteralParameterFactory(11-15)DotNet/DataAcquisition.Domain/Application/Factories/ParameterFactories/VariableParameterFactory.cs (3)
ParameterFactoryResult(21-45)VariableParameterFactory(11-86)VariableParameterFactory(15-19)DotNet/DataAcquisition.Domain/Infrastructure/Models/QueryConfig/Parameter/LiteralParameter.cs (1)
LiteralParameter(5-10)DotNet/DataAcquisition.Domain/Infrastructure/Models/QueryConfig/Parameter/VariableParameter.cs (1)
VariableParameter(5-11)DotNet/DataAcquisition.Domain/Infrastructure/Models/QueryConfig/Parameter/ResourceIdsParameter.cs (1)
ResourceIdsParameter(5-11)DotNet/Shared/Application/SerDes/ResourceTypeJsonConverter.cs (1)
ResourceType(9-18)
DotNet/DataAcquisition.Domain/Application/Factories/ParameterFactories/VariableParameterFactory.cs (4)
DotNet/ServiceTests/IntegrationTests/Census/CensusIntegrationTestFixture.cs (1)
ILogger(143-143)DotNet/DataAcquisition.Domain/Application/Factories/ParameterFactories/LiteralParameterFactory.cs (1)
ParameterFactoryResult(17-26)DotNet/DataAcquisition.Domain/Infrastructure/Models/QueryConfig/Parameter/VariableParameter.cs (1)
VariableParameter(5-11)DotNet/DataAcquisition.Domain/Application/Models/Api/Requests/GetPatientDataRequest.cs (1)
GetPatientDataRequest(7-13)
DotNet/DataAcquisition.Domain/Application/Services/QueryListProcessor.cs (4)
DotNet/DataAcquisition.Domain/Application/Factories/QueryFactories/ParameterQueryFactory.cs (2)
ParameterQueryFactory(13-85)ParameterQueryFactory(17-21)DotNet/DataAcquisition.Domain/Infrastructure/Models/Enums/FhirQueryType.cs (2)
FhirQueryTypeUtilities(18-32)FhirQueryType(20-31)DotNet/DataAcquisition.Domain/Application/Models/CreateFhirQueryModel.cs (1)
CreateFhirQueryModel(6-21)DotNet/DataAcquisition.Domain/Application/Models/CreateDataAcquisitionLogModel.cs (1)
CreateDataAcquisitionLogModel(7-25)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Integration Tests
- GitHub Check: Smoke Test with Docker Compose
- GitHub Check: Unit Tests for DotNet
- GitHub Check: Build Documentation
- GitHub Check: Analyze (csharp)
- GitHub Check: Analyze (java-kotlin)
🔇 Additional comments (18)
DotNet/DataAcquisition.Domain/Application/Models/Factory/ParameterQuery/ParameterFactoryResult.cs (1)
3-3: LGTM!Making
valuenullable aligns with the updated factory methods that can returnnullresults and supports paged scenarios wherevaluesis populated instead ofvalue.DotNet/DataAcquisition.Domain/Application/Factories/ParameterFactories/LiteralParameterFactory.cs (1)
17-25: Validation logic and nullable return look good.The validation correctly checks for null/whitespace conditions and the structured logging provides useful diagnostic information.
DotNet/DataAcquisition.Domain/Application/Models/Factory/ParameterQuery/PagedParameterQueryFactoryResult.cs (1)
5-5: LGTM!The change to
List<List<KeyValuePair<string, string>>>is consistent with theSingularParameterQueryFactoryResultrefactoring and properly represents the nested structure for paged queries (each page having its own set of parameters).DotNet/DataAcquisition.Domain/Application/Factories/ParameterFactories/VariableParameterFactory.cs (1)
21-44: Validation logic and nullable return look good.The validation correctly handles multiple failure conditions and provides comprehensive diagnostic logging.
DotNet/DataAcquisition.Domain/Application/Models/Factory/ParameterQuery/SingularParameterQueryFactoryResult.cs (1)
5-5: Type change fromSearchParamstoList<KeyValuePair<string, string>>is correctly implemented.This aligns with the PR objective to decouple from Firely's types. All usages have been properly updated:
ParameterQueryFactory.cscorrectly instantiates withList<KeyValuePair<string, string>>QueryListProcessor.csproperly handles the new type viafactoryResult.SearchParams?.Select(x => $"{x.Key}={x.Value}").ToList()- Tests validate the type change with proper assertions on
KeyValuePairpropertiesDotNet/ServiceTests/IntegrationTests/DataAcquisition/Factories/ParameterQueryFactoryTests.cs (3)
22-45: LGTM! Good coverage for the singular result path.The test validates that literal parameters produce a
SingularParameterQueryFactoryResultwith correct key-value pairs.
47-81: LGTM! Paged parameter scenario is well-tested.The test correctly validates that when a
ResourceIdsParameterwith paging produces multiple pages, the result is aPagedParameterQueryFactoryResultwith literal parameters replicated across all pages.
83-107: LGTM! Multiple paged parameters correctly returns null.This validates the error path where more than one paged parameter is configured.
DotNet/ServiceTests/IntegrationTests/DataAcquisition/Factories/ResourceIdParameterFactoryTests.cs (3)
19-49: LGTM! Non-paged scenario correctly tested.Validates that when
Pagedis null, resource IDs are joined and returned as a single value.
82-115: LGTM! Multi-page scenario is well-validated.The test correctly verifies that 5 resource IDs with page size 2 produces 3 pages with the expected distribution.
117-141: LGTM! Empty resource IDs scenario returns null as expected.DotNet/DataAcquisition.Domain/Application/Services/QueryListProcessor.cs (1)
226-250: LGTM! Good extraction of log creation logic.The
CreateDataAcquisitionLogAsynchelper method centralizes log creation and reduces code duplication across the three query result branches.DotNet/DataAcquisition.Domain/Application/Queries/DataAcquisitionLogQueries.cs (1)
72-73: LGTM! Interface method signature is well-defined.The method signature appropriately accepts correlation context and resource type for filtering, with an optional cancellation token.
DotNet/DataAcquisition.Domain/Application/Factories/QueryFactories/ParameterQueryFactory.cs (3)
15-21: Static logger initialization is consistent with other factories.This pattern matches
LiteralParameterFactoryandResourceIdParameterFactory. Note that theLoggerFactoryis disposed after logger creation, which is acceptable since the logger remains functional. However, for production code, consider injecting the logger via DI if the factory is refactored to be non-static in the future.
44-60: LGTM! Paging logic correctly handles parameter accumulation.The logic properly:
- Prevents multiple paged parameters with an error return
- Creates copies of existing non-paged params for each page
- Appends the paged key-value to each page set
64-78: LGTM! Non-paged parameters are correctly distributed to paged queries.When a non-paged parameter appears after a paged one, it's correctly added to all existing page sets via
ForEach.DotNet/DataAcquisition.Domain/Application/Factories/ParameterFactories/ResourceIdParameterFactory.cs (2)
18-27: LGTM! Async resource ID retrieval with proper validation.The method correctly fetches resource IDs from the database and validates the result before proceeding. The warning log helps with debugging when no resources are found.
29-41: LGTM! Paging logic handles edge cases correctly.The use of
TryParsesafely handles invalidPagedvalues (defaulting to 0), and the conditionresourceIds.Count > pageSizeprevents unnecessary paging when all IDs fit in one page.
edward-miller-lcg
left a comment
edward-miller-lcg
left a comment
There was a problem hiding this comment.
I think this looks good. There are a couple of coderabbit comments that I think are valid and should be addressed.
3 participants
🛠️ Description of Changes
Parameter handling enhancements
List<KeyValuePair<string, string>>instead of Firely'sQueryParamsso that _count parameter is not ignoredAdding unit tests to ParameterQueryFactory and ResourceIdParameterFactory to confirm correct handling of paging scenarios
🧪 Testing Performed
Ran simple patient report as well as a mega-patient report and confirmed that fhir queries are constructed with parameters more accurately. Mega patient gets through data acquisition much quicker locally, now.
🧑🔬 Unit Testing
📓 Documentation Updated
N/A
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.