LNK-4316: Handle Legacy QueryPlans in Converter - #1244
Conversation
|
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 WalkthroughThe pull request introduces an async existence check method to the query plan repository, replaces the JSON converter for IQueryConfig with an enhanced version supporting multiple discriminator strategies, updates service configuration to use the new converter, and refactors the controller to use existence checks instead of retrieval operations. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 1
🧹 Nitpick comments (3)
DotNet/DataAcquisition/Controllers/QueryPlanConfigController.cs (2)
3-3: Remove unused import.The
using Hl7.Fhir.Model;directive appears to be unused in this file.Apply this diff:
-using Hl7.Fhir.Model; using LantanaGroup.Link.DataAcquisition.Domain.Application.Managers;
143-148: Apply consistent facilityId sanitization across all methods.
DeleteQueryPlansanitizesfacilityIdon Line 344, butCreateQueryPlanandUpdateQueryPlando not. For consistency and defense-in-depth, all three methods should apply the same sanitization since they accept the same route parameter.In
CreateQueryPlan, add sanitization after Line 134:if (string.IsNullOrWhiteSpace(facilityId)) { throw new BadRequestException("facilityId is required."); } + +facilityId = facilityId.SanitizeAndRemove();In
UpdateQueryPlan, add sanitization after Line 249:if (string.IsNullOrWhiteSpace(facilityId)) { throw new BadRequestException("parameter facilityId is required."); } + +facilityId = facilityId.SanitizeAndRemove();Also applies to: 258-263, 344-362
DotNet/DataAcquisition.Domain/Application/Serializers/QueryPlanConverter.cs (1)
22-33: Strengthen type name matching for $type discriminator.Lines 25 and 29 use
Containsfor type matching, which could inadvertently match unintended types (e.g., "MyCustomParameterQueryConfigWrapper" would match). Consider usingEndsWithor exact match with the full type name for more robust discrimination.Apply this diff:
else if (doc.RootElement.TryGetProperty("$type", out typeElement)) { var typeName = typeElement.GetString(); - if (typeName?.Contains("ParameterQueryConfig") == true) + if (typeName?.EndsWith("ParameterQueryConfig") == true) { configType = "Parameter"; } - else if (typeName?.Contains("ReferenceQueryConfig") == true) + else if (typeName?.EndsWith("ReferenceQueryConfig") == true) { configType = "Reference"; } }
📜 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 (6)
DotNet/DataAcquisition.Domain/Application/Queries/QueryPlanQueries.cs(2 hunks)DotNet/DataAcquisition.Domain/Application/Serializers/QueryConfigConverter.cs(0 hunks)DotNet/DataAcquisition.Domain/Application/Serializers/QueryPlanConverter.cs(1 hunks)DotNet/DataAcquisition.Domain/Infrastructure/Context/DataAcquisitionDbContext.cs(1 hunks)DotNet/DataAcquisition/Controllers/QueryPlanConfigController.cs(5 hunks)DotNet/DataAcquisition/Program.cs(1 hunks)
💤 Files with no reviewable changes (1)
- DotNet/DataAcquisition.Domain/Application/Serializers/QueryConfigConverter.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/Infrastructure/Context/DataAcquisitionDbContext.csDotNet/DataAcquisition.Domain/Application/Serializers/QueryPlanConverter.csDotNet/DataAcquisition.Domain/Application/Queries/QueryPlanQueries.csDotNet/DataAcquisition/Program.csDotNet/DataAcquisition/Controllers/QueryPlanConfigController.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/Infrastructure/Context/DataAcquisitionDbContext.csDotNet/DataAcquisition.Domain/Application/Serializers/QueryPlanConverter.csDotNet/DataAcquisition.Domain/Application/Queries/QueryPlanQueries.csDotNet/DataAcquisition/Program.csDotNet/DataAcquisition/Controllers/QueryPlanConfigController.cs
🧠 Learnings (8)
📓 Common learnings
Learnt from: CR
Repo: lantanagroup/link-cloud PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-09-02T16:50:12.997Z
Learning: The PR description must follow the provided template with sections: Description of Changes, Testing Performed, Unit Testing checkbox, and Documentation Updated
Learnt from: sdmcgeown
Repo: lantanagroup/link-cloud PR: 0
File: :0-0
Timestamp: 2025-07-24T17:49:23.117Z
Learning: PR #1004 in the Link Cloud project introduced comprehensive normalization operation testing functionality including a new test operation dialog, JSON validation, resource type validation, and real-time testing capabilities that require documentation updates across multiple files in the docs folder.
📚 Learning: 2025-09-24T21:08:44.732Z
Learnt from: seanmcilvenna
Repo: lantanagroup/link-cloud PR: 669
File: DotNet/Terminology/Application/Formatters/FhirModelBinder.cs:1-8
Timestamp: 2025-09-24T21:08:44.732Z
Learning: In the FhirModelBinder.cs file (DotNet/Terminology/Application/Formatters/FhirModelBinder.cs), the implementation uses simple string comparison for content-type checking and direct stream reading without requiring additional usings like Microsoft.Net.Http.Headers or Microsoft.AspNetCore.Http beyond what's already imported. The code relies on implicit usings available in .NET 8 for basic types like StreamReader and ArgumentNullException.
Applied to files:
DotNet/DataAcquisition/Controllers/QueryPlanConfigController.cs
📚 Learning: 2025-09-24T21:08:44.732Z
Learnt from: seanmcilvenna
Repo: lantanagroup/link-cloud PR: 669
File: DotNet/Terminology/Application/Formatters/FhirModelBinder.cs:1-8
Timestamp: 2025-09-24T21:08:44.732Z
Learning: In the FhirModelBinder.cs file (DotNet/Terminology/Application/Formatters/FhirModelBinder.cs), the implementation uses simple string comparison for content-type checking and direct stream reading without requiring additional usings like Microsoft.Net.Http.Headers or Microsoft.AspNetCore.Http beyond what's already imported.
Applied to files:
DotNet/DataAcquisition/Controllers/QueryPlanConfigController.cs
📚 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/Controllers/QueryPlanConfigController.cs
📚 Learning: 2025-04-23T15:52:54.846Z
Learnt from: seanmcilvenna
Repo: lantanagroup/link-cloud PR: 792
File: DotNet/Submission/Application/Services/PathNamingService.cs:52-57
Timestamp: 2025-04-23T15:52:54.846Z
Learning: In the Link Cloud application, facilityId is sanitized before being passed to PathNamingService.GetSubmissionDirectoryName(), making additional sanitization in that method unnecessary. Measure names are based on predefined service configuration and are considered safe.
Applied to files:
DotNet/DataAcquisition/Controllers/QueryPlanConfigController.cs
📚 Learning: 2025-04-23T17:52:29.663Z
Learnt from: seanmcilvenna
Repo: lantanagroup/link-cloud PR: 792
File: DotNet/Submission/Listeners/SubmitReportListener.cs:165-174
Timestamp: 2025-04-23T17:52:29.663Z
Learning: In the Link Cloud Submission service, facilityId should be sanitized before being used in file path construction to prevent directory traversal vulnerabilities.
Applied to files:
DotNet/DataAcquisition/Controllers/QueryPlanConfigController.cs
📚 Learning: 2025-08-05T18:00:19.977Z
Learnt from: smailliwcs
Repo: lantanagroup/link-cloud PR: 1042
File: DotNet/Shared/Application/Error/Handlers/DeadLetterExceptionHandler.cs:13-13
Timestamp: 2025-08-05T18:00:19.977Z
Learning: In the Link Cloud codebase, the facilityId parameters in DeadLetterExceptionHandler methods are not actually used within the implementation, despite being part of the interface. This creates awkward situations in retry/dead-letter workflows when facility ID is unavailable but still must be passed. These unused parameters should be considered for removal in future interface refactoring.
Applied to files:
DotNet/DataAcquisition/Controllers/QueryPlanConfigController.cs
📚 Learning: 2025-08-05T18:02:54.305Z
Learnt from: smailliwcs
Repo: lantanagroup/link-cloud PR: 1042
File: DotNet/Shared/Application/Error/Handlers/TransientExceptionHandler.cs:16-16
Timestamp: 2025-08-05T18:02:54.305Z
Learning: In the Link Cloud codebase, the facilityId parameters in TransientExceptionHandler methods are not functionally necessary for the core retry operations, despite being part of the interface. They are only used for logging and Kafka header propagation. This creates awkward API design where callers must provide facility IDs even when unavailable, making retry/dead-letter workflows cumbersome when facility ID is not accessible. These unused parameters should be considered for removal or made optional in future interface refactoring.
Applied to files:
DotNet/DataAcquisition/Controllers/QueryPlanConfigController.cs
🧬 Code graph analysis (5)
DotNet/DataAcquisition.Domain/Infrastructure/Context/DataAcquisitionDbContext.cs (1)
DotNet/DataAcquisition.Domain/Application/Serializers/QueryPlanConverter.cs (1)
QueryPlanConverter(8-65)
DotNet/DataAcquisition.Domain/Application/Serializers/QueryPlanConverter.cs (3)
DotNet/DataAcquisition.Domain/Infrastructure/Models/QueryConfig/ParameterQueryConfig.cs (2)
ParameterQueryConfig(4-14)ParameterQueryConfig(10-13)DotNet/DataAcquisition.Domain/Infrastructure/Models/QueryConfig/ReferenceQueryConfig.cs (2)
ReferenceQueryConfig(6-17)ReferenceQueryConfig(13-16)DotNet/DataAcquisition.Domain/Application/Serializers/ParameterConverter.cs (1)
Write(31-34)
DotNet/DataAcquisition.Domain/Application/Queries/QueryPlanQueries.cs (2)
DotNet/DataAcquisition.Domain/Application/Factories/ReportableEventToQueryPlanTypeFactory.cs (1)
Frequency(8-20)DotNet/DataAcquisition.Domain/Application/Managers/QueryPlanManager.cs (6)
Task(16-16)Task(17-17)Task(18-18)Task(32-62)Task(64-92)Task(94-107)
DotNet/DataAcquisition/Program.cs (1)
DotNet/DataAcquisition.Domain/Application/Serializers/QueryPlanConverter.cs (1)
QueryPlanConverter(8-65)
DotNet/DataAcquisition/Controllers/QueryPlanConfigController.cs (1)
DotNet/Shared/Application/Services/Security/HtmlInputSanitizer.cs (1)
SanitizeAndRemove(18-23)
⏰ 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). (5)
- GitHub Check: Integration Tests
- GitHub Check: Unit Tests for DotNet
- GitHub Check: Smoke Test with Docker Compose
- GitHub Check: Analyze (java-kotlin)
- GitHub Check: Analyze (csharp)
🔇 Additional comments (5)
DotNet/DataAcquisition.Domain/Infrastructure/Context/DataAcquisitionDbContext.cs (1)
43-43: LGTM: Converter update aligns with legacy QueryPlan handling.The replacement of
QueryConfigConverterwithQueryPlanConverterproperly supports the multi-discriminator deserialization strategy for handling legacy QueryPlan formats.DotNet/DataAcquisition/Program.cs (1)
58-58: LGTM: Consistent converter registration.The switch to
QueryPlanConverterproperly integrates the new multi-discriminator deserialization approach across the application's JSON serialization configuration.DotNet/DataAcquisition/Controllers/QueryPlanConfigController.cs (1)
143-148: LGTM: ExistsAsync refactoring improves efficiency.The switch from
GetAsynctoExistsAsyncfor existence checks is more efficient, as it avoids retrieving and materializing the entire entity when only existence verification is needed.Also applies to: 258-263, 355-362
DotNet/DataAcquisition.Domain/Application/Serializers/QueryPlanConverter.cs (2)
35-50: Property inspection fallback provides good legacy support.The multi-level discriminator strategy (explicit properties → $type → property inspection) properly handles various legacy QueryPlan formats. The precedence of checking
ParametersbeforePagedis reasonable given the expected legacy data structure.
61-64: Write implementation correctly delegates to polymorphic serialization.Using
value.GetType()ensures the concrete type's properties are serialized, maintaining type information for future deserialization.
nvmLantana
changed the title
2 participants
🛠️ Description of Changes
Update the QueryConfigConverter and ParamaterConverter to handle legacy data that was serialized using $type namespace/project mappings and is missing the new QueryConfigType or ParamterType properties that have been added as a replacement to $type data being including in the serialized data.
🧪 Testing Performed
Created a suite of unit tests that leverage old data to test convertability.
🧑🔬 Unit Testing
📓 Documentation Updated
Please update any relevant sections in the project documentation that were impacted by the changes in the PR.
Summary by CodeRabbit
New Features
Refactor