LNK-3317: Add Tenant API endpoints to generate ad hoc reports #611
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis pull request introduces functionality for generating ad-hoc reports in a Kafka-based system. A new Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
🔭 Outside diff range comments (1)
DotNet/Tenant/Services/FacilityConfigurationService.cs (1)
Line range hint
338-365: Security and reliability improvements needed inMeasureDefinitionExists.Several concerns need to be addressed:
- The hardcoded token expiration of 2 minutes
- Using
CancellationToken.Noneinstead of propagating the cancellation token- Missing timeout on the HTTP request
Apply these improvements:
public async Task MeasureDefinitionExists(String reportType) { if (_measureConfig.Value.CheckIfMeasureExists) { if (String.IsNullOrEmpty(_serviceRegistry.Value.MeasureServiceUrl)) throw new ApplicationException($"MeasureEval service configuration from \"ServiceRegistry.MeasureServiceUrl\" is missing"); string requestUrl = _serviceRegistry.Value.MeasureServiceUrl + $"/api/measure-definition/{reportType}"; //get link token if (!_linkBearerServiceOptions.Value.AllowAnonymous) { //TODO: add method to get key that includes looking at redis for future use case if (_linkTokenServiceConfig.Value.SigningKey is null) throw new Exception("Link Token Service Signing Key is missing."); - var token = await _createSystemToken.ExecuteAsync(_linkTokenServiceConfig.Value.SigningKey, 2); + // Use configuration for token expiration + var tokenExpirationMinutes = _linkTokenServiceConfig.Value.TokenExpirationMinutes ?? 5; + var token = await _createSystemToken.ExecuteAsync(_linkTokenServiceConfig.Value.SigningKey, tokenExpirationMinutes); _httpClient.DefaultRequestHeaders.Authorization = new AuthenticationHeaderValue("Bearer", token); } - var response = await _httpClient.GetAsync(requestUrl, CancellationToken.None); + using var cts = new CancellationTokenSource(TimeSpan.FromSeconds(30)); + using var linkedCts = CancellationTokenSource.CreateLinkedTokenSource(cts.Token, cancellationToken); + var response = await _httpClient.GetAsync(requestUrl, linkedCts.Token); // check respone status code if (!response.IsSuccessStatusCode) { throw new ApplicationException($"Report Type {reportType} is not setup in MeasureEval service."); } } }
🧹 Nitpick comments (5)
DotNet/Tenant/Controllers/FacilityController.cs (3)
320-320: Use UTF8 encoding for header valuesIt's recommended to use UTF8 encoding when converting header values to bytes to properly support all characters.
Apply this diff to update the encoding:
- headers.Add("X-Correlation-Id", System.Text.Encoding.ASCII.GetBytes(correlationId)); + headers.Add("X-Correlation-Id", System.Text.Encoding.UTF8.GetBytes(correlationId)); - { "X-Report-Tracking-Id", System.Text.Encoding.ASCII.GetBytes(Guid.NewGuid().ToString()) } + { "X-Report-Tracking-Id", System.Text.Encoding.UTF8.GetBytes(Guid.NewGuid().ToString()) }Also applies to: 371-371
316-316: Dispose of the Kafka producer properly to prevent resource leaksThe Kafka producer should be disposed of correctly to release resources. Consider using a
usingstatement to ensure proper disposal.Apply this diff to use a
usingstatement:- var producer = _adHocKafkaProducerFactory.CreateProducer(producerConfig); + using var producer = _adHocKafkaProducerFactory.CreateProducer(producerConfig);Also applies to: 368-368
307-311: Evaluate exception handling inMeasureDefinitionExistscallsThe
MeasureDefinitionExistsmethod is called within a loop and may throw exceptions. Relying on exceptions for control flow can impact performance. Consider implementing a validation method that returns a boolean or handles validation without exceptions.For example, modify
MeasureDefinitionExiststo return a boolean:bool exists = await _facilityConfigurationService.MeasureDefinitionExists(rt); if (!exists) { return BadRequest($"Measure definition '{rt}' does not exist."); }DotNet/Tenant/Interfaces/IFacilityConfigurationService.cs (1)
16-16: Clarify method nameMeasureDefinitionExistsThe method
MeasureDefinitionExiststhrows an exception if the measure definition does not exist. Consider renaming it to reflect its behavior, such asValidateMeasureDefinitionExists, to indicate that it performs validation and may throw exceptions.DotNet/Tenant/Services/FacilityConfigurationService.cs (1)
338-338: Consider adding XML documentation for the public method.Since
MeasureDefinitionExistsis now public, it should have XML documentation describing its purpose, parameters, and exceptions.Add documentation:
+/// <summary> +/// Validates if a measure definition exists in the MeasureEval service. +/// </summary> +/// <param name="reportType">The type of report to validate.</param> +/// <returns>A task that represents the asynchronous operation.</returns> +/// <exception cref="ApplicationException">Thrown when the measure definition doesn't exist or when service configuration is missing.</exception> +/// <exception cref="Exception">Thrown when the signing key is missing.</exception> public async Task MeasureDefinitionExists(String reportType)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
DotNet/Shared/Application/Models/Kafka/GenerateReportValue.cs(1 hunks)DotNet/Shared/Application/Models/KafkaTopic.cs(1 hunks)DotNet/Tenant/Controllers/FacilityController.cs(4 hunks)DotNet/Tenant/Interfaces/IFacilityConfigurationService.cs(1 hunks)DotNet/Tenant/Program.cs(2 hunks)DotNet/Tenant/Services/FacilityConfigurationService.cs(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Analyze (java-kotlin)
- GitHub Check: Analyze (csharp)
🔇 Additional comments (4)
DotNet/Shared/Application/Models/Kafka/GenerateReportValue.cs (1)
5-13: ClassGenerateReportValueis well-definedThe
GenerateReportValueclass includes all necessary properties for generating reports and is structured appropriately.DotNet/Shared/Application/Models/KafkaTopic.cs (1)
60-61: LGTM! The new Kafka topic follows naming conventions.The addition of
GenerateReportRequestedtopic aligns with the existing event-based naming pattern and is appropriately placed in the enum.DotNet/Tenant/Program.cs (1)
151-152: LGTM! Kafka producer factories are properly registered.The registration of both specific (
GenerateReportValue) and generic (object) producer factories as transient services follows dependency injection best practices.DotNet/Tenant/Services/FacilityConfigurationService.cs (1)
Line range hint
101-106: LGTM! Nullable return type improves type safety.The change to return
FacilityConfigModel?makes the possibility of no facility being found explicit in the method signature.
|
One thing we should add to this PR is an API GET endpoint in the Report service that accepts a ReportId and returns the following:
In Tenant, RegenerateReport() validates the incoming report id against this new endpoint. If a value is not returned from Report, Tenant returns a Bad Request that states 'Report Id not found'. This will help ensure that any regenerated report requests are for actual existing reports. |
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (1)
DotNet/Tenant/Controllers/FacilityController.cs (1)
Line range hint
38-44: Remove duplicate scheduler factory assignmentThe
_schedulerFactoryis assigned twice in the constructor.public FacilityController(ILogger<FacilityController> logger, IFacilityConfigurationService facilityConfigurationService, ISchedulerFactory schedulerFactory, IKafkaProducerFactory<string, GenerateReportValue> adHocKafkaProducerFactory) { _facilityConfigurationService = facilityConfigurationService; - _schedulerFactory = schedulerFactory; _logger = logger; _schedulerFactory = schedulerFactory;
🧹 Nitpick comments (2)
DotNet/Tenant/Controllers/FacilityController.cs (2)
337-337: Propagate cancellation tokenUsing
CancellationToken.Noneignores the cancellation context. Consider propagating the cancellation token from the controller method.- await producer.ProduceAsync(KafkaTopic.GenerateReportRequested.ToString(), message, CancellationToken.None); + using var cts = new CancellationTokenSource(TimeSpan.FromSeconds(30)); + using var linkedCts = CancellationTokenSource.CreateLinkedTokenSource(cts.Token, HttpContext.RequestAborted); + await producer.ProduceAsync(KafkaTopic.GenerateReportRequested.ToString(), message, linkedCts.Token);
350-351: Add route template and XML documentationThe endpoint needs a route template and XML documentation for API documentation generation.
[ProducesResponseType(StatusCodes.Status500InternalServerError)] +/// <summary> +/// Regenerates a report for a specific facility. +/// </summary> +/// <param name="facilityId">The ID of the facility.</param> +/// <param name="reportId">The ID of the report to regenerate.</param> +/// <param name="bypassSubmission">Optional flag to bypass submission.</param> +/// <returns>OK if the regeneration request was successful.</returns> - [HttpPost] + [HttpPost("regenerate")] public async Task<IActionResult> RegenerateReport(string facilityId, string reportId, bool? bypassSubmission)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
DotNet/Tenant/Controllers/FacilityController.cs(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Analyze (java-kotlin)
- GitHub Check: Analyze (csharp)
🔇 Additional comments (2)
DotNet/Tenant/Controllers/FacilityController.cs (2)
339-343: Catch more specific exceptions instead of genericExceptionCatching the general
Exceptionclass can mask specific issues and make debugging harder. Consider catching specific exceptions that you expect might occur.Also applies to: 387-391
313-315: Configure Kafka producer settingsEmpty
ProducerConfigmight need broker settings for proper connection.Consider injecting Kafka configuration through dependency injection or configuration files instead of creating an empty config.
There was a problem hiding this comment.
Actionable comments posted: 4
🔭 Outside diff range comments (1)
DotNet/Tenant/Controllers/FacilityController.cs (1)
Line range hint
45-71: Fix duplicate scheduler factory assignment.The
_schedulerFactoryis assigned twice in the constructor.public FacilityController(ILogger<FacilityController> logger, IFacilityConfigurationService facilityConfigurationService, ISchedulerFactory schedulerFactory, IKafkaProducerFactory<string, GenerateReportValue> adHocKafkaProducerFactory, IOptions<ServiceRegistry> serviceRegistry, IHttpClientFactory httpClient) { _facilityConfigurationService = facilityConfigurationService; - _schedulerFactory = schedulerFactory; _logger = logger; _schedulerFactory = schedulerFactory;
♻️ Duplicate comments (2)
DotNet/Tenant/Controllers/FacilityController.cs (2)
291-292:⚠️ Potential issueUse distinct routes for different actions.
Both
GenerateAdHocReportandRegenerateReportuse the same base route withHttpPost. This can lead to routing conflicts.- [HttpPost] + [HttpPost("generate")] public async Task<IActionResult> GenerateAdHocReport(string facilityId, bool? bypassSubmission, DateTime? startDate, DateTime? endDate, List<string>? reportTypes, List<string>? patientIds)
353-357: 🛠️ Refactor suggestionCatch specific exceptions.
Catching a generic
Exceptioncan mask specific issues. Consider catching and handling specific exceptions separately.- catch (Exception ex) + catch (KafkaException ex) { - _logger.LogError(ex, "Exception encountered in FacilityController.GenerateAdHocReport"); + _logger.LogError(ex, "Kafka error encountered in FacilityController.GenerateAdHocReport"); + return Problem("Failed to generate report.", statusCode: 500); + } + catch (Exception ex) + { + _logger.LogError(ex, "Unexpected error in FacilityController.GenerateAdHocReport"); return Problem("An internal server error occurred.", statusCode: 500); }
🧹 Nitpick comments (6)
DotNet/Tenant/Program.cs (2)
151-152: Add Kafka connection validation and clarify producer registrations.While the new producer factory registrations align with the ad hoc report generation requirements, consider these improvements:
- Add validation for the Kafka connection configuration before registering the producers.
- Consider using the typed
GenerateReportValueproducer consistently instead of mixing with generic object producer.Here's how to add the validation:
+ if (kafkaConnection?.BootstrapServers == null) + { + throw new InvalidOperationException("Kafka bootstrap servers configuration is missing"); + } builder.Services.AddTransient<IKafkaProducerFactory<string, GenerateReportValue>, KafkaProducerFactory<string, GenerateReportValue>>(); builder.Services.AddTransient<IKafkaProducerFactory<string, object>, KafkaProducerFactory<string, object>>();
156-156: Consider adding a strongly-typed consumer for report validation.Based on the PR requirements for report validation, consider adding a strongly-typed consumer factory for handling report validation responses. This would provide type safety and better align with the service's responsibilities.
Add the following registration:
builder.Services.AddTransient<IKafkaConsumerFactory<string, object>, KafkaConsumerFactory<string, object>>(); + // Add strongly-typed consumer for report validation responses + builder.Services.AddTransient<IKafkaConsumerFactory<string, ReportValidationResponse>, KafkaConsumerFactory<string, ReportValidationResponse>>();DotNet/Shared/Application/Models/Responses/ReportScheduleSummaryModel.cs (2)
5-6: Add null checks for string properties.Consider adding null checks or making the properties required to prevent potential null reference issues:
- public string ReportId { get; set; } - public string FacilityId { get; set; } + public string ReportId { get; set; } = null!; + public string FacilityId { get; set; } = null!;
3-10: Consider making the model immutable.For better encapsulation and thread safety, consider making the properties init-only:
public class ReportScheduleSummaryModel { - public string ReportId { get; set; } - public string FacilityId { get; set; } - public DateTime StartDate { get; set; } - public DateTime EndDate { get; set; } - public DateTime? SubmitReportDateTime { get; set; } + public string ReportId { get; init; } = null!; + public string FacilityId { get; init; } = null!; + public DateTime StartDate { get; init; } + public DateTime EndDate { get; init; } + public DateTime? SubmitReportDateTime { get; init; }DotNet/Report/Controllers/ReportController.cs (2)
94-94: Optimize database query performance.The current implementation using
SingleOrDefault()afterFindAsync()may retrieve more records than necessary. Consider using a more efficient query:- var model = (await _database.ReportScheduledRepository.FindAsync(r => r.FacilityId == facilityId && r.Id == reportScheduleId)).SingleOrDefault(); + var model = await _database.ReportScheduledRepository.FirstOrDefaultAsync(r => r.FacilityId == facilityId && r.Id == reportScheduleId);
71-81: Enhance API documentation with response examples.The XML documentation is good but could be improved by adding:
- Response examples for each status code
- Description of error scenarios
- Documentation for the return type
/// <summary> /// Returns a summary of a ReportSchedule based on the provided facilityId and reportScheduleId /// </summary> /// <param name="facilityId"></param> /// <param name="reportScheduleId"></param> - /// <returns></returns> + /// <returns>A ReportScheduleSummaryModel containing the schedule details if found</returns> + /// <response code="200">Returns the report schedule summary</response> + /// <response code="400">If facilityId or reportScheduleId is null or empty</response> + /// <response code="404">If no report schedule is found</response> + /// <response code="500">If there was an internal server error</response>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
DotNet/Report/Controllers/ReportController.cs(5 hunks)DotNet/Shared/Application/Models/Responses/ReportScheduleSummaryModel.cs(1 hunks)DotNet/Tenant/Controllers/FacilityController.cs(4 hunks)DotNet/Tenant/Program.cs(2 hunks)
🔇 Additional comments (6)
DotNet/Tenant/Controllers/FacilityController.cs (3)
292-292: Encapsulate request parameters in a model.The method has too many parameters. Consider creating a request model to encapsulate these parameters.
Example model:
public class GenerateAdHocReportRequest { public string FacilityId { get; set; } public bool? BypassSubmission { get; set; } public DateTime StartDate { get; set; } public DateTime EndDate { get; set; } public List<string> ReportTypes { get; set; } public List<string> PatientIds { get; set; } }
364-365:⚠️ Potential issueUse distinct routes for different actions.
Both
RegenerateReportandGenerateAdHocReportuse the same base route withHttpPost. This can lead to routing conflicts.- [HttpPost] + [HttpPost("regenerate")] public async Task<IActionResult> RegenerateReport(string facilityId, string reportId, bool? bypassSubmission)Likely invalid or redundant comment.
420-424: 🛠️ Refactor suggestionCatch specific exceptions.
Catching a generic
Exceptioncan mask specific issues. Consider catching and handling specific exceptions separately.- catch (Exception ex) + catch (HttpRequestException ex) { - _logger.LogError(ex, "Exception encountered in FacilityController.RegenerateReport"); + _logger.LogError(ex, "HTTP error encountered in FacilityController.RegenerateReport"); + return Problem("Failed to fetch report schedule.", statusCode: 500); + } + catch (KafkaException ex) + { + _logger.LogError(ex, "Kafka error encountered in FacilityController.RegenerateReport"); + return Problem("Failed to regenerate report.", statusCode: 500); + } + catch (Exception ex) + { + _logger.LogError(ex, "Unexpected error in FacilityController.RegenerateReport"); return Problem("An internal server error occurred.", statusCode: 500); }Likely invalid or redundant comment.
DotNet/Shared/Application/Models/Responses/ReportScheduleSummaryModel.cs (1)
7-9: Validate date ranges and consider removing default values.The default DateTime values might not be appropriate for these fields. Consider:
- Making the dates required (non-nullable)
- Adding validation to ensure EndDate is not before StartDate
- public DateTime StartDate { get; set; } = default; - public DateTime EndDate { get; set; } = default; + public DateTime StartDate { get; set; } + public DateTime EndDate { get; set; }DotNet/Report/Controllers/ReportController.cs (2)
22-26: LGTM! Constructor properly injects the new dependency.The constructor correctly initializes the new
IDatabasedependency while maintaining existing dependencies.
94-108: 🛠️ Refactor suggestionSanitize input parameters for security.
Consider sanitizing the input parameters before using them in the database query:
+ var sanitizedFacilityId = HtmlInputSanitizer.SanitizeAndRemove(facilityId); + var sanitizedReportId = HtmlInputSanitizer.SanitizeAndRemove(reportScheduleId); - var model = await _database.ReportScheduledRepository.FirstOrDefaultAsync(r => r.FacilityId == facilityId && r.Id == reportScheduleId); + var model = await _database.ReportScheduledRepository.FirstOrDefaultAsync(r => + r.FacilityId == sanitizedFacilityId && r.Id == sanitizedReportId); if (model == null) { return Problem(detail: "No Report Schedule found for the provided FacilityId and ReportId", statusCode: (int)HttpStatusCode.NotFound); } return Ok(new ReportScheduleSummaryModel { - FacilityId = facilityId, - ReportId = reportScheduleId, + FacilityId = sanitizedFacilityId, + ReportId = sanitizedReportId, StartDate = model.ReportStartDate, EndDate = model.ReportEndDate, SubmitReportDateTime = model.SubmitReportDateTime });Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
DotNet/Tenant/Controllers/FacilityController.cs (1)
Line range hint
47-73: Fix duplicate scheduler factory assignment.The
_schedulerFactoryis assigned twice in the constructor.public FacilityController(ILogger<FacilityController> logger, IFacilityConfigurationService facilityConfigurationService, ISchedulerFactory schedulerFactory, IKafkaProducerFactory<string, GenerateReportValue> adHocKafkaProducerFactory, IOptions<ServiceRegistry> serviceRegistry, IHttpClientFactory httpClient) { _facilityConfigurationService = facilityConfigurationService; - _schedulerFactory = schedulerFactory; _logger = logger; _schedulerFactory = schedulerFactory;
🧹 Nitpick comments (2)
DotNet/Tenant/Controllers/FacilityController.cs (2)
291-293: Complete the XML documentation.The XML documentation for the GenerateAdHocReport method is incomplete.
/// <summary> - /// Generat + /// Generates an ad-hoc report for the specified facility. /// </summary> /// <param name="facilityId">The ID of the facility to generate the report for.</param> + /// <param name="request">The ad-hoc report generation parameters.</param> + /// <returns>200 OK if the report generation was successfully queued, or an error if validation fails.</returns>
389-389: Use string interpolation for better readability.The URL construction can be simplified using string interpolation.
- string requestUrl = $"{_serviceRegistry.ReportServiceApiUrl.Trim('/')}/Report/Schedule?FacilityId={facilityId}&reportScheduleId={request.ReportId}"; + string requestUrl = $"{_serviceRegistry.ReportServiceApiUrl.Trim('/')}/Report/Schedule?facilityId={facilityId}&reportScheduleId={request.ReportId}";
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
DotNet/Tenant/Controllers/FacilityController.cs(4 hunks)DotNet/Tenant/Models/AdHocReportRequest.cs(1 hunks)DotNet/Tenant/Models/RegenerateReportRequest.cs(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Analyze (java-kotlin)
- GitHub Check: Analyze (csharp)
🔇 Additional comments (5)
DotNet/Tenant/Controllers/FacilityController.cs (5)
299-300: Add route template to distinguish between endpoints.
337-359: Ensure proper disposal of Kafka producer.
372-373: Add route template to distinguish between endpoints.
387-391: Add timeout to HTTP client call.
428-432: Catch more specific exceptions instead of genericException.
MontaltoNick
changed the base branch from
dev
to
feature/LNK-3316-On-demand-reporting
…lantanagroup/link-cloud into nmontalto/LMK-3317_AdHoc_Report
MontaltoNick
merged commit 61d2560
into
feature/LNK-3316-On-demand-reporting
🛠️ Description of Changes
Please provide a high-level overview of the changes included in this PR.
🧪 Testing Performed
Please describe the testing that was performed on the changes included in this PR.
📓 Documentation Updated
Please update any relevant sections in the project documentation that were impacted by the changes in the PR.
Summary by CodeRabbit
Release Notes
New Features
Improvements
Technical Updates