LNK-4621: data acquisition query plan rest api not properly validating values - #1383
Conversation
…-not-properly-validating-values
|
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 PR introduces centralized validation for Query Plans by implementing a new Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant QueryPlanManager
participant IQueryPlanValidator
participant IDatabase
participant Logger
Client->>QueryPlanManager: AddAsync(model)
QueryPlanManager->>QueryPlanManager: Extract dictionaries
QueryPlanManager->>IQueryPlanValidator: ValidateQueryPlan(initial, supplemental)
IQueryPlanValidator->>IQueryPlanValidator: Validate keys, parameters, types, ordering, references
IQueryPlanValidator-->>QueryPlanManager: ValidationResult{IsValid, Errors, Warnings}
alt Validation Failed
QueryPlanManager->>Logger: Log errors
QueryPlanManager-->>Client: BadRequestException
else Validation Succeeded
QueryPlanManager->>IDatabase: CreateAsync(plan)
IDatabase-->>QueryPlanManager: Created plan
QueryPlanManager->>Logger: Log success with facility/type
QueryPlanManager-->>Client: QueryPlan
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 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. ✨ Finishing touches🧪 Generate unit tests (beta)
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 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@DotNet/DataAcquisition.Domain/Application/Validators/QueryPlanValidator.cs`:
- Around line 324-362: The ValidateResourceIdsParameter method never increments
the ref pagedCount, so the "only one paged parameter" rule never triggers;
update ValidateResourceIdsParameter to increment pagedCount when the parameter
has a non-empty Paged value that successfully parses to a non-negative int
(i.e., inside the existing int.TryParse branch where pagedVal >= 0) so only
valid paged parameters count toward the global limit; keep not incrementing on
parse/validation failures and reference the pagedCount ref parameter and the
ResourceIdsParameter.Paged check in your change.
In
`@DotNet/ServiceTests/IntegrationTests/DataAcquisition/Managers/QueryPlanManagerTests.cs`:
- Around line 289-293: The test UpdateAsync_NotFound is asserting
BadRequestException because the validator mock currently returns an invalid
result by default; change the test to expect NotFoundException and ensure the
validator mock returns a successful validation result for
CreateValidUpdateQueryPlanModel() before calling manager.UpdateAsync(model).
Specifically, update the test setup to configure the validation mock (the
validator used by the manager) to return a valid ValidationResult/validation
success for the model and replace Assert.ThrowsAsync<BadRequestException> with
Assert.ThrowsAsync<NotFoundException> around manager.UpdateAsync(model).
- Around line 37-38: The test is creating a mock of the concrete
QueryPlanValidator instead of the IQueryPlanValidator, causing ValidateQueryPlan
to return default/invalid results; replace new Mock<QueryPlanValidator>().Object
with a mock of the interface (new Mock<IQueryPlanValidator>().Object) or
instantiate the real QueryPlanValidator for integration tests, and if mocking
configure the mock to return a successful ValidationResult for ValidateQueryPlan
before passing it into QueryPlanManager(description: QueryPlanManager,
validator: IQueryPlanValidator).
In `@DotNet/ServiceTests/UnitTests/DataAcquisition/QueryPlanValidatorTests.cs`:
- Around line 304-340: The test
ValidateQueryPlan_MultipleResourceIdsParametersInSameQuery_ReturnsError is
misnamed/constructed: it currently triggers cross-reference validation (unknown
Resource 'Encounter') and uses an empty supplementalQueries which can cause a
"SupplementalQueries cannot be null or empty" failure. Either rename the test to
reflect it verifies cross-reference errors (update method name and assert
message) or modify the Arrange section to actually exercise the "only one paged
parameter" rule by making both ResourceIdsParameter.Resource values valid (e.g.,
both "Patient"), and populate supplementalQueries with a valid IQueryConfig (so
cross-reference validation does not short-circuit); keep the Assert checking
result.IsValid == false and that result.Errors contains "Only one paged
parameter is allowed". Ensure you update the test method name or assertions
accordingly and reference ValidateQueryPlan, initialQueries,
supplementalQueries, and the test method name when making changes.
♻️ Duplicate comments (1)
DotNet/DataAcquisition.Domain/Application/Managers/QueryPlanManager.cs (1)
43-61: Validation flow looks good; consider sanitizingFacilityIdfor log safety.The validation logic correctly fails fast and provides clear error messages. Static analysis flagged potential log injection at lines 49-50 and 59-60 where
model.FacilityId(user input) is logged.While structured logging with template parameters provides some protection against injection, per coding guidelines, consider sanitizing
FacilityIdusingHtmlInputSanitizerbefore logging if it's not already sanitized upstream.
🧹 Nitpick comments (8)
DotNet/DataAcquisition.Domain/Infrastructure/Attributes/ValidateQueryPlanConfigDictionaryAttribute.cs (1)
80-91: Consider validating ResourceType against the FHIR resource enum.The
ValidateParameterQueryConfigmethod checks thatResourceTypeis not null/whitespace, but unlike the centralQueryPlanValidator, it doesn't validate that the ResourceType value is a valid FHIR resource type. This could allow invalid resource names to pass attribute validation.Since the PR objective is to "prevent creation of Query Plan records containing incorrectly named FHIR resources," consider adding enum validation here for consistency, or document that this attribute performs structural validation only while semantic validation occurs in
QueryPlanValidator.DotNet/DataAcquisition.Domain/Application/Managers/QueryPlanManager.cs (1)
159-165: Consider using a filtered query instead of fetching all records.
GetAllAsync()followed byWhere()fetches all query plans from the database before filtering in memory. For better performance, consider using a filtered query:♻️ Suggested optimization
- var allPlans = await _database.QueryPlanRepository.GetAllAsync(cancellationToken); - var facilityPlans = allPlans.Where(q => q.FacilityId == facilityId).ToList(); + var facilityPlans = await _database.QueryPlanRepository + .FindAsync(q => q.FacilityId == facilityId, cancellationToken);This assumes a
FindAsyncor similar filtered query method exists on the repository. If not, this could be deferred to a future optimization.DotNet/DataAcquisition.Domain/Application/Validators/QueryPlanValidator.cs (3)
414-419: Consider short-circuiting FHIR validation when ResourceType is empty.The FHIR
ResourceTypeenum validation at line 415 will execute even whenconfig.ResourceTypeis null or whitespace, which was already flagged as an error above. This could result in redundant error messages (one for "required" and another for "not a valid FHIR ResourceType").♻️ Suggested improvement
- //validate resource string by using the Firely Resource enum - if (!Enum.TryParse(typeof(ResourceType), config.ResourceType, out _)) + //validate resource string by using the Firely Resource enum + else if (!Enum.TryParse(typeof(ResourceType), config.ResourceType, out _)) { result.IsValid = false; result.Errors.Add($"{prefix}: ResourceType value '{config.ResourceType}' is not a valid FHIR ResourceType."); }
204-209: Same short-circuit opportunity for FHIR validation.Similar to
ValidateReferenceQueryConfig, the FHIR validation will run even whenResourceTypeis empty, potentially producing redundant errors.♻️ Suggested improvement
if (config.ResourceType.Length > MaxResourceTypeLength) { result.IsValid = false; result.Errors.Add($"{prefix}: ResourceType length exceeds maximum of {MaxResourceTypeLength} characters."); } - //validate resource string by using the Firely Resource enum - if (!Enum.TryParse(typeof(ResourceType), config.ResourceType, out _)) - { - result.IsValid = false; - result.Errors.Add($"{prefix}: Resource value '{config.ResourceType}' is not a valid FHIR ResourceType."); - } + //validate resource string by using the Firely Resource enum + else if (!Enum.TryParse(typeof(ResourceType), config.ResourceType, out _)) + { + result.IsValid = false; + result.Errors.Add($"{prefix}: Resource value '{config.ResourceType}' is not a valid FHIR ResourceType."); + }
341-346: FHIR validation runs even when Resource is empty, causing redundant errors.When
parameter.Resourceis empty, the check at line 335 will add an error, but then the FHIR enum validation at line 342 will also fail and add another error for the same root cause.♻️ Suggested improvement
// Validate Resource if (string.IsNullOrWhiteSpace(parameter.Resource)) { result.IsValid = false; result.Errors.Add($"{prefix}: Resource is required for ResourceIdsParameter."); } - - //validate resource string by using the Firely Resource enum - if (!Enum.TryParse(typeof(ResourceType), parameter.Resource, out _)) + else if (!Enum.TryParse(typeof(ResourceType), parameter.Resource, out _)) { result.IsValid = false; result.Errors.Add($"{prefix}: Resource value '{parameter.Resource}' is not a valid FHIR ResourceType."); }DotNet/DataAcquisition.Domain/Application/Models/Http/QueryPlanApiModel.cs (3)
64-66: Consider validating unique resource types for SupplementalQueries as well.
ValidateUniqueResourceTypesis only called forInitialQueriesbut not forSupplementalQueries. If duplicate resource types are a concern, this validation should be consistent across both dictionaries.♻️ Suggested change
// Validate that there are no duplicate ResourceTypes within InitialQueries ValidateUniqueResourceTypes(InitialQueries, nameof(InitialQueries), results); + ValidateUniqueResourceTypes(SupplementalQueries, nameof(SupplementalQueries), results);
176-189: Consider catching specific exception type forXmlConvert.ToTimeSpan.The empty
catchblock swallows all exceptions. While this works for validation purposes, catching the specificFormatExceptionwould be more precise and avoid masking unexpected errors.♻️ Suggested improvement
try { // Try to parse as TimeSpan using XmlConvert (built-in ISO 8601 parser) System.Xml.XmlConvert.ToTimeSpan(lookBack); return true; } - catch + catch (FormatException) { // If XmlConvert fails, do a more lenient regex check // This allows for valid ISO 8601 durations that might not parse to TimeSpan var iso8601Pattern = @"^P(?:\d+Y)?(?:\d+M)?(?:\d+W)?(?:\d+D)?(?:T(?:\d+H)?(?:\d+M)?(?:\d+(?:\.\d+)?S)?)?$"; return System.Text.RegularExpressions.Regex.IsMatch(lookBack, iso8601Pattern, System.Text.RegularExpressions.RegexOptions.IgnoreCase); }
186-188: Regex pattern allows invalid duration strings like "PT" (no time components).The regex pattern makes all components optional, which means strings like "PT" (time designator with no actual time values) would pass validation. Consider adding a check to ensure at least one component is present after P or PT.
♻️ Suggested improvement
// If XmlConvert fails, do a more lenient regex check // This allows for valid ISO 8601 durations that might not parse to TimeSpan - var iso8601Pattern = @"^P(?:\d+Y)?(?:\d+M)?(?:\d+W)?(?:\d+D)?(?:T(?:\d+H)?(?:\d+M)?(?:\d+(?:\.\d+)?S)?)?$"; - return System.Text.RegularExpressions.Regex.IsMatch(lookBack, iso8601Pattern, + // Ensure at least one date component, or T followed by at least one time component + var iso8601Pattern = @"^P(?:(?:\d+Y)|(?:\d+M)|(?:\d+W)|(?:\d+D)|(?:T(?:(?:\d+H)|(?:\d+M)|(?:\d+(?:\.\d+)?S))+))+$"; + return System.Text.RegularExpressions.Regex.IsMatch(lookBack, iso8601Pattern, System.Text.RegularExpressions.RegexOptions.IgnoreCase);
…-not-properly-validating-values
3 participants
🛠️ Description of Changes
Add enhanced validation to Query Plan when add/updating it.
🧪 Testing Performed
Local: 1. e2e, 2. validated use cases defined in ticket.
🧑🔬 Unit Testing
📓 Documentation Updated
n.a
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.