MX-287: Isolate BIRT report execution from shared template state#483
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds BirtReportExecutionFactory to produce execution-local BIRT runnables by copying cached designs, integrates it into BirtReportingProcessServiceImpl with safe task closure, changes BirtDataSourceConfigurer to throw on datasource configuration failures, and adds/updates tests validating factory isolation and task closure. ChangesBIRT Report Execution Factory and Integration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/java/org/apache/fineract/infrastructure/report/service/BirtReportingProcessServiceImpl.java (1)
70-73:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve
PlatformDataIntegrityExceptioninstead of rewrapping it.This catch block currently rewrites domain-specific
PlatformDataIntegrityExceptions (from execution factory/datasource configuration) intoerror.msg.reporting.error, which drops actionable error codes and breaks error-contract fidelity.Suggested fix
- } catch (Exception e) { + } catch (PlatformDataIntegrityException e) { + throw e; + } catch (Exception e) { log.error("Failed to generate BIRT report: {}", reportName, e); throw new PlatformDataIntegrityException( "error.msg.reporting.error", "Report generation failed: " + e.getMessage(), e); } finally {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/org/apache/fineract/infrastructure/report/service/BirtReportingProcessServiceImpl.java` around lines 70 - 73, The catch in BirtReportingProcessServiceImpl currently catches Exception and always wraps it into a new PlatformDataIntegrityException, losing original PlatformDataIntegrityException details; change the handler in the method containing this catch so that if the caught exception is already a PlatformDataIntegrityException you rethrow it unchanged (throw (PlatformDataIntegrityException) e), otherwise wrap non-PlatformDataIntegrityException instances as you do now (log and throw new PlatformDataIntegrityException). Update the catch block around the report generation logic (the try/catch that logs "Failed to generate BIRT report" and throws PlatformDataIntegrityException) to perform an instanceof check and rethrow existing PlatformDataIntegrityException to preserve its error codes and cause.
🧹 Nitpick comments (2)
src/test/java/org/apache/fineract/infrastructure/report/service/BirtReportingProcessServiceImplTest.java (1)
199-214: ⚡ Quick winAdd a task-close test for renderer failure path.
Current coverage validates
task.close()only after successful rendering. Add a case whererenderer.render(...)throws and still verifytask.close()is invoked fromfinally.As per coding guidelines, "Verify both happy path and edge cases."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/test/java/org/apache/fineract/infrastructure/report/service/BirtReportingProcessServiceImplTest.java` around lines 199 - 214, Add a new test that ensures the IRunAndRenderTask is closed even when the renderer fails: in the test class BirtReportingProcessServiceImplTest create a test (e.g., shouldCloseTaskAfterRenderingWhenRendererFails) that mocks IReportRunnable (design), ReportDesignHandle, and IRunAndRenderTask, stubs reportExecutionFactory.createExecutionRunnable(...) to return design and reportEngine.createRunAndRenderTask(design) to return task, then make pdfRenderer.render(...) throw an exception (e.g., RuntimeException) when called, invoke service.processRequest("sample", queryParams("PDF")) (allowing the exception or asserting it is thrown), and finally verify(task).close() is invoked in the failure path; reuse the same mock setup as in shouldCloseTaskAfterRendering and assert close() is called in the finally path.src/test/java/org/apache/fineract/infrastructure/report/service/BirtReportExecutionFactoryTest.java (1)
38-116: ⚡ Quick winAdd explicit failure-path tests for factory exception behavior.
Please add tests for: (1)
PlatformDataIntegrityExceptionis propagated unchanged, and (2) non-platform exceptions are converted throughreportErrorHandler.reportError(...).As per coding guidelines, "Verify both happy path and edge cases."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/test/java/org/apache/fineract/infrastructure/report/service/BirtReportExecutionFactoryTest.java` around lines 38 - 116, Add two tests for executionFactory.createExecutionRunnable: (1) mock reportLoader.loadReport(...) to throw a PlatformDataIntegrityException and assert that createExecutionRunnable propagates that same PlatformDataIntegrityException (no wrapping or conversion); (2) mock reportLoader.loadReport(...) to throw a non-platform exception (e.g., RuntimeException), stub reportErrorHandler.reportError(originalException) to return a different Throwable and assert that createExecutionRunnable calls reportErrorHandler.reportError with the original exception and then throws the exception returned by reportErrorHandler.reportError; reference the existing symbols executionFactory.createExecutionRunnable, reportLoader.loadReport, reportErrorHandler.reportError, and PlatformDataIntegrityException to locate where to add the tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In
`@src/main/java/org/apache/fineract/infrastructure/report/service/BirtReportingProcessServiceImpl.java`:
- Around line 70-73: The catch in BirtReportingProcessServiceImpl currently
catches Exception and always wraps it into a new PlatformDataIntegrityException,
losing original PlatformDataIntegrityException details; change the handler in
the method containing this catch so that if the caught exception is already a
PlatformDataIntegrityException you rethrow it unchanged (throw
(PlatformDataIntegrityException) e), otherwise wrap
non-PlatformDataIntegrityException instances as you do now (log and throw new
PlatformDataIntegrityException). Update the catch block around the report
generation logic (the try/catch that logs "Failed to generate BIRT report" and
throws PlatformDataIntegrityException) to perform an instanceof check and
rethrow existing PlatformDataIntegrityException to preserve its error codes and
cause.
---
Nitpick comments:
In
`@src/test/java/org/apache/fineract/infrastructure/report/service/BirtReportExecutionFactoryTest.java`:
- Around line 38-116: Add two tests for
executionFactory.createExecutionRunnable: (1) mock reportLoader.loadReport(...)
to throw a PlatformDataIntegrityException and assert that
createExecutionRunnable propagates that same PlatformDataIntegrityException (no
wrapping or conversion); (2) mock reportLoader.loadReport(...) to throw a
non-platform exception (e.g., RuntimeException), stub
reportErrorHandler.reportError(originalException) to return a different
Throwable and assert that createExecutionRunnable calls
reportErrorHandler.reportError with the original exception and then throws the
exception returned by reportErrorHandler.reportError; reference the existing
symbols executionFactory.createExecutionRunnable, reportLoader.loadReport,
reportErrorHandler.reportError, and PlatformDataIntegrityException to locate
where to add the tests.
In
`@src/test/java/org/apache/fineract/infrastructure/report/service/BirtReportingProcessServiceImplTest.java`:
- Around line 199-214: Add a new test that ensures the IRunAndRenderTask is
closed even when the renderer fails: in the test class
BirtReportingProcessServiceImplTest create a test (e.g.,
shouldCloseTaskAfterRenderingWhenRendererFails) that mocks IReportRunnable
(design), ReportDesignHandle, and IRunAndRenderTask, stubs
reportExecutionFactory.createExecutionRunnable(...) to return design and
reportEngine.createRunAndRenderTask(design) to return task, then make
pdfRenderer.render(...) throw an exception (e.g., RuntimeException) when called,
invoke service.processRequest("sample", queryParams("PDF")) (allowing the
exception or asserting it is thrown), and finally verify(task).close() is
invoked in the failure path; reuse the same mock setup as in
shouldCloseTaskAfterRendering and assert close() is called in the finally path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 220508ad-bf3b-4b0d-949d-cc768eda3c47
📒 Files selected for processing (5)
src/main/java/org/apache/fineract/infrastructure/report/service/BirtDataSourceConfigurer.javasrc/main/java/org/apache/fineract/infrastructure/report/service/BirtReportExecutionFactory.javasrc/main/java/org/apache/fineract/infrastructure/report/service/BirtReportingProcessServiceImpl.javasrc/test/java/org/apache/fineract/infrastructure/report/service/BirtReportExecutionFactoryTest.javasrc/test/java/org/apache/fineract/infrastructure/report/service/BirtReportingProcessServiceImplTest.java
Member
Author
Contributor
|
Reviewing pr |
dr-fuch
approved these changes
Jun 8, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR addresses MX-287 by eliminating mutation of shared BIRT report design state during report execution.
Previously, the plugin cached
IReportRunnableinstances and then modified the underlyingReportDesignHandlewith tenant-specific datasource properties before rendering. Since the same cached report instance could be reused across requests, this created a risk of shared mutable state and potential datasource leakage between concurrent report executions.Problem
Current execution flow:
The cached runnable's design handle was being modified directly.
In BIRT 4.23,
createRunAndRenderTask()does not automatically clone the underlying design model. As a result, concurrent executions could potentially operate on shared mutable datasource configuration.Solution
Introduced execution-local report creation.
New execution flow:
Changes included:
BirtReportExecutionFactoryIReportRunnableinstances from copiedReportDesignHandlesBirtReportingProcessServiceImplto use execution-local runnablesIRunAndRenderTask.close()cleanupValidation
Unit Tests
Added:
BirtReportExecutionFactoryTestBirtReportingProcessServiceImplTestValidation includes:
Local Runtime Validation
Tested against a local Apache Fineract 1.12 deployment with the BIRT reporting plugin enabled.
Verified:
Caching Status
This PR does not introduce or modify cache implementation.
The goal of MX-287 is execution isolation.
The solution is intentionally designed so that if caching is enabled or improved in the future (e.g. Spring Cache, Caffeine, or other caching strategies), execution-level datasource isolation will already be guaranteed.
In other words:
This architecture remains safe regardless of future cache optimizations.
Scope
Included:
Not included:
Summary by CodeRabbit