[ES-2992B] added custom audit executor#203
Conversation
Signed-off-by: Harsh Kashiwal <harsh.kashiwal@infosys.com>
WalkthroughTwo Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 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. ✨ 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 |
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)
mosip-identity-plugin/src/main/java/io/mosip/esignet/plugin/mosipid/service/IdaAuditPluginImpl.java (1)
55-65:⚠️ Potential issue | 🟠 MajorEnsure consuming application configures
auditTaskExecutorbean and@EnableAsync; otherwise@Asyncwill be a no-op or fail at runtime.The
@Async("auditTaskExecutor")annotations added at lines 55 and 61 require two things in the Spring context of the consuming application:
@EnableAsyncmust be present on a configuration class in the consumer's application.- A bean named
auditTaskExecutor(TaskExecutor or Executor) must be defined in the consumer's application.Neither exists in this plugin repository. When the named executor is not found, Spring's behavior varies: older versions silently fall back to
SimpleAsyncTaskExecutor, while newer versions may throw an exception at invocation. Since the plugin is a library consumed by eSignet (or another parent app), responsibility for this configuration lies with the consumer—but the lack of enforcement or documentation here creates a significant risk of misconfiguration going unnoticed in production.Additionally:
- Existing unit tests instantiate
IdaAuditPluginImpldirectly without a Spring proxy, so the@Asyncpath is uncovered by tests. Misconfiguration in the consuming app will not be caught by the plugin's test suite.- Exceptions from async calls are silently logged (no exception propagation with
voidreturn type), which is acceptable givenaudit(...)already wraps logic in try/catch.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mosip-identity-plugin/src/main/java/io/mosip/esignet/plugin/mosipid/service/IdaAuditPluginImpl.java` around lines 55 - 65, The async annotations on IdaAuditPluginImpl.logAudit(...) require a named executor and `@EnableAsync` in the consumer; add a safe library-side fallback by creating a configuration class (e.g., AuditAsyncConfig) that is annotated with `@Configuration` and `@EnableAsync` and defines a `@Bean`(name = "auditTaskExecutor") method that is `@ConditionalOnMissingBean`(name = "auditTaskExecutor") and returns a sensible TaskExecutor (e.g., a ThreadPoolTaskExecutor with reasonable defaults) so consumers that don't provide one still get async behavior; also add JavaDoc to IdaAuditPluginImpl referencing the requirement and update unit tests to load AuditAsyncConfig (or use a Spring test context) so the `@Async` path for logAudit(...) is exercised.
🧹 Nitpick comments (1)
mosip-identity-plugin/src/main/java/io/mosip/esignet/plugin/mosipid/service/IdaAuditPluginImpl.java (1)
22-22: Minor: indentation inconsistency.Lines 22 (import), 55 and 61 (the new
@Asyncannotations) use space indentation, while the rest of the file uses tabs. Not functional, but worth aligning to keep the file consistent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mosip-identity-plugin/src/main/java/io/mosip/esignet/plugin/mosipid/service/IdaAuditPluginImpl.java` at line 22, The import and two `@Async` annotations in IdaAuditPluginImpl are indented with spaces while the file uses tabs; update the indentation to use tabs for the import statement "import org.springframework.scheduling.annotation.Async;" and for the two occurrences of the "@Async" annotation (around the methods in IdaAuditPluginImpl) so they match the rest of the file's tab-based indentation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In
`@mosip-identity-plugin/src/main/java/io/mosip/esignet/plugin/mosipid/service/IdaAuditPluginImpl.java`:
- Around line 55-65: The async annotations on IdaAuditPluginImpl.logAudit(...)
require a named executor and `@EnableAsync` in the consumer; add a safe
library-side fallback by creating a configuration class (e.g., AuditAsyncConfig)
that is annotated with `@Configuration` and `@EnableAsync` and defines a `@Bean`(name
= "auditTaskExecutor") method that is `@ConditionalOnMissingBean`(name =
"auditTaskExecutor") and returns a sensible TaskExecutor (e.g., a
ThreadPoolTaskExecutor with reasonable defaults) so consumers that don't provide
one still get async behavior; also add JavaDoc to IdaAuditPluginImpl referencing
the requirement and update unit tests to load AuditAsyncConfig (or use a Spring
test context) so the `@Async` path for logAudit(...) is exercised.
---
Nitpick comments:
In
`@mosip-identity-plugin/src/main/java/io/mosip/esignet/plugin/mosipid/service/IdaAuditPluginImpl.java`:
- Line 22: The import and two `@Async` annotations in IdaAuditPluginImpl are
indented with spaces while the file uses tabs; update the indentation to use
tabs for the import statement "import
org.springframework.scheduling.annotation.Async;" and for the two occurrences of
the "@Async" annotation (around the methods in IdaAuditPluginImpl) so they match
the rest of the file's tab-based indentation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 56ac79db-4add-49fe-a63b-933e198bf0bc
📒 Files selected for processing (1)
mosip-identity-plugin/src/main/java/io/mosip/esignet/plugin/mosipid/service/IdaAuditPluginImpl.java
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Summary by CodeRabbit