Fix #19300: Avoid unnecessary error logging for existing email templates#1113
Fix #19300: Avoid unnecessary error logging for existing email templates#1113SSSD-2001 wants to merge 1 commit into
Conversation
|
|
📝 WalkthroughChanges MadeModified exception handling in the email template service to differentiate between conflict-style failures and other failures. When adding an email template fails due to the template name already existing, the system now logs this condition at DEBUG level instead of ERROR level. Other types of email template addition failures continue to be logged at ERROR level. This change prevents expected client-side conflicts (which correctly receive a 409 response) from generating unnecessary backend error logs. ImpactReduces noise in error logs by distinguishing between actual backend errors and expected client-side conflicts when duplicate email template names are provided. WalkthroughThe change modifies exception handling in the 🚥 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. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@components/org.wso2.carbon.identity.api.server.email.template/org.wso2.carbon.identity.rest.api.server.email.template.v1/src/main/java/org/wso2/carbon/identity/rest/api/server/email/template/v1/core/ServerEmailTemplatesService.java`:
- Around line 267-275: The code in ServerEmailTemplatesService is comparing
e.getErrorCode() to the REST-layer constant
Constants.ErrorMessage.ERROR_EMAIL_TEMPLATE_ALREADY_EXISTS.getCode(), which can
miss manager-thrown "already exists" errors; change the comparison to use the
manager-layer error-code constant used elsewhere (the manager's mapped error
code for template-conflict) — e.g., replace
Constants.ErrorMessage.ERROR_EMAIL_TEMPLATE_ALREADY_EXISTS.getCode() with the
manager constant (the ErrorMessages/ERROR_CODE_EMAIL_TEMPLATE_ALREADY_EXISTS
from the email template manager/constants class referenced in this module) so
the conflict branch triggers correctly and avoids logging a spurious ERROR.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1c2f2479-c5ac-47f7-bbcb-8b83d0a79e53
📒 Files selected for processing (1)
components/org.wso2.carbon.identity.api.server.email.template/org.wso2.carbon.identity.rest.api.server.email.template.v1/src/main/java/org/wso2/carbon/identity/rest/api/server/email/template/v1/core/ServerEmailTemplatesService.java
| if (Constants.ErrorMessage.ERROR_EMAIL_TEMPLATE_ALREADY_EXISTS.getCode().equals(e.getErrorCode())) { | ||
| // We only log this as a DEBUG message to keep Carbon logs clean | ||
| if (log.isDebugEnabled()) { | ||
| log.debug("Email template already exists: " + emailTemplateWithID.getId(), e); | ||
| } | ||
| } else { | ||
| // Log as an actual error only if it's a real server-side failure | ||
| log.error("Error occurred while adding email template", e); | ||
| } |
There was a problem hiding this comment.
Use mapped/manager error code for conflict detection
Line 267 compares e.getErrorCode() to a REST-layer code (Constants.ErrorMessage...getCode()), while manager exceptions in this class are matched using manager error-code constants. This can miss the “already exists” case and still hit Line 274 ERROR logging.
Suggested fix
- if (Constants.ErrorMessage.ERROR_EMAIL_TEMPLATE_ALREADY_EXISTS.getCode().equals(e.getErrorCode())) {
+ Constants.ErrorMessage mappedError = Constants.getMappedErrorMessage(e.getErrorCode());
+ if (mappedError == Constants.ErrorMessage.ERROR_EMAIL_TEMPLATE_ALREADY_EXISTS) {
if (log.isDebugEnabled()) {
log.debug("Email template already exists: " + emailTemplateWithID.getId(), e);
}
} else {
log.error("Error occurred while adding email template", e);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@components/org.wso2.carbon.identity.api.server.email.template/org.wso2.carbon.identity.rest.api.server.email.template.v1/src/main/java/org/wso2/carbon/identity/rest/api/server/email/template/v1/core/ServerEmailTemplatesService.java`
around lines 267 - 275, The code in ServerEmailTemplatesService is comparing
e.getErrorCode() to the REST-layer constant
Constants.ErrorMessage.ERROR_EMAIL_TEMPLATE_ALREADY_EXISTS.getCode(), which can
miss manager-thrown "already exists" errors; change the comparison to use the
manager-layer error-code constant used elsewhere (the manager's mapped error
code for template-conflict) — e.g., replace
Constants.ErrorMessage.ERROR_EMAIL_TEMPLATE_ALREADY_EXISTS.getCode() with the
manager constant (the ErrorMessages/ERROR_CODE_EMAIL_TEMPLATE_ALREADY_EXISTS
from the email template manager/constants class referenced in this module) so
the conflict branch triggers correctly and avoids logging a spurious ERROR.
2 participants
This PR addresses a logging issue where duplicate email template names were triggering unnecessary backend error logs.
Fixes wso2/product-is#19300