Skip to content

Replace printStackTrace with log.error for exceptions#27674

Open
Branavan2004 wants to merge 1 commit into
wso2:archive_IS-7.3from
Branavan2004:patch-1
Open

Replace printStackTrace with log.error for exceptions#27674
Branavan2004 wants to merge 1 commit into
wso2:archive_IS-7.3from
Branavan2004:patch-1

Conversation

@Branavan2004

Copy link
Copy Markdown

Description

This PR addresses a code smell in UserInformationRecoveryServiceClient.java. The integration client previously handled exceptions by calling e.printStackTrace() 14 separate times. This is an anti-pattern that causes unstructured stack traces to leak directly into the console, bypassing the centralized logging framework and making it harder to filter logs.

Changes Made

  • Injected org.apache.commons.logging.Log.
  • Instantiated standard logger: private static final Log log = LogFactory.getLog(UserInformationRecoveryServiceClient.class);
  • Replaced all raw e.printStackTrace() calls with proper error logging: log.error("An error occurred during user information recovery service invocation", e);.

This minor refactoring ensures that all identity recovery errors safely flow through WSO2's standard logging pipelines.

@coderabbitai

coderabbitai Bot commented Apr 24, 2026

Copy link
Copy Markdown
Contributor
📝 Walkthrough

Changes Summary

This PR refactors exception handling in the UserInformationRecoveryServiceClient class to use a centralized logging framework instead of printing stack traces to console output.

Key Changes

  • Added Apache Commons Logging: Imported and initialized a static logger instance (Log and LogFactory) from the Apache Commons Logging library.

  • Replaced Exception Handling: Updated 14 exception handling blocks across multiple service methods to route exceptions through the logger using log.error() instead of e.printStackTrace(). Each logged error includes a consistent, descriptive message along with the exception object for full context.

  • Methods Updated: The changes apply consistently to methods including getCaptcha(), verifyUser(), sendRecoveryNotification(), verifyConfirmationCode(), updatePassword(), getUserChallengeQuestionIds(), getUserChallengeQuestion(), verifyUserChallengeAnswer(), getAllChallengeQuestions(), getUserIdentitySupportedClaims(), verifyAccount(), registerUser(), and confirmUserSelfRegistration().

Impact

This change integrates identity recovery errors with WSO2's standard logging framework, enabling better monitoring, filtering, and management of error logs alongside other application logs rather than having them scattered across console output.

Walkthrough

The UserInformationRecoveryServiceClient.java class is modified to improve logging practices. The change replaces direct exception stack trace printing (e.printStackTrace()) with Apache Commons Logging through a class-level Log instance. Multiple service-call methods that catch stub exceptions are updated to use log.error() for exception reporting. The file also gains a proper newline character at its end, removing the prior "no newline at end of file" condition. The net change is 18 lines added and 14 lines removed.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately summarizes the main change: replacing printStackTrace() calls with proper log.error() statements for exception handling.
Description check ✅ Passed The description clearly explains the refactoring purpose, specific changes made, and the rationale for integrating with the centralized logging framework.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
modules/integration/tests-common/admin-clients/src/main/java/org/wso2/identity/integration/common/clients/mgt/UserInformationRecoveryServiceClient.java (1)

65-65: Consider adding operation context to error logs

All catches currently emit the same message. Including the operation name (for example, verifyUser, updatePassword) would make troubleshooting faster without changing behavior.

Proposed minimal refactor pattern
- log.error("An error occurred during user information recovery service invocation", e);
+ log.error("Failed to invoke UserInformationRecoveryService#verifyUser", e);

Also applies to: 75-75, 85-85, 96-96, 107-107, 117-117, 128-128, 139-139, 149-149, 159-159, 170-170, 181-181, 192-192

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@modules/integration/tests-common/admin-clients/src/main/java/org/wso2/identity/integration/common/clients/mgt/UserInformationRecoveryServiceClient.java`
at line 65, The catch blocks in UserInformationRecoveryServiceClient currently
log a generic message; update each catch to include the specific operation
context (e.g., "verifyUser", "updatePassword", and other method names in this
class) in the log message so errors read like "Error during <operationName>
invocation" and still pass the exception object (e) to log.error; locate the
log.error calls in the catch blocks of methods such as verifyUser and
updatePassword and replace the static message with one that embeds the
method/operation name for easier troubleshooting.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In
`@modules/integration/tests-common/admin-clients/src/main/java/org/wso2/identity/integration/common/clients/mgt/UserInformationRecoveryServiceClient.java`:
- Line 65: The catch blocks in UserInformationRecoveryServiceClient currently
log a generic message; update each catch to include the specific operation
context (e.g., "verifyUser", "updatePassword", and other method names in this
class) in the log message so errors read like "Error during <operationName>
invocation" and still pass the exception object (e) to log.error; locate the
log.error calls in the catch blocks of methods such as verifyUser and
updatePassword and replace the static message with one that embeds the
method/operation name for easier troubleshooting.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 42b3a395-2cb2-4f83-b119-94e25687edc3

📥 Commits

Reviewing files that changed from the base of the PR and between 1b77ede and ede6e87.

📒 Files selected for processing (1)
  • modules/integration/tests-common/admin-clients/src/main/java/org/wso2/identity/integration/common/clients/mgt/UserInformationRecoveryServiceClient.java

@sonarqubecloud

Copy link
Copy Markdown

Quality Gate Failed Quality Gate failed

Failed conditions
B Maintainability Rating on New Code (required ≥ A)

See analysis details on SonarQube Cloud

Catch issues before they fail your Quality Gate with our IDE extension SonarQube for IDE

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant