Skip to content

Conversation

@malshaniS
Copy link
Contributor

@malshaniS malshaniS commented Sep 24, 2025

Fixes for Accelerator detected from CDS Toolkit Implementation

Fix for issue: #828

Issue link: required

Doc Issue: Optional, link issue from documentation repository

Applicable Labels: Spec, product, version, type (specify requested labels)


Development Checklist

  1. Build complete solution with pull request in place.
  2. Ran checkstyle plugin with pull request in place.
  3. Ran Findbugs plugin with pull request in place.
  4. Ran FindSecurityBugs plugin and verified report.
  5. Formatted code according to WSO2 code style.
  6. Have you verified the PR doesn't commit any keys, passwords, tokens, usernames, or other secrets?
  7. Migration scripts written (if applicable).
  8. Have you followed secure coding standards in WSO2 Secure Engineering Guidelines?

Testing Checklist

  1. Written unit tests.
  2. Verified tests in multiple database environments (if applicable).
  3. Tested with BI enabled (if applicable).

Summary by CodeRabbit

  • Bug Fixes
    • Improved validation of consent identifiers to prevent processing of empty or invalid values, ensuring more reliable consent retrieval operations.

✏️ Tip: You can customize this high-level summary in your review settings.

@malshaniS malshaniS marked this pull request as ready for review September 25, 2025 05:34
@coderabbitai
Copy link

coderabbitai bot commented Jan 12, 2026

Walkthrough

Modified the conditional check in ExternalAPIConsentRetrievalStep to use StringUtils.isNotBlank(consentId) instead of null-checking, ensuring empty strings are filtered before consent retrieval logic executes.

Changes

Cohort / File(s) Summary
Consent Retrieval Validation
financial-services-accelerator/components/org.wso2.financial.services.accelerator.consent.mgt.extensions/src/main/java/org/wso2/financial/services/accelerator/consent/mgt/extensions/authorize/impl/ExternalAPIConsentRetrievalStep.java
Changed conditional gate from null-check to non-blank check for consentId. Now skips detailed consent retrieval and file loading when consentId is null or blank, preventing empty string values from passing through to consentCoreService.getDetailedConsent().

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

Poem

🐰 A single line hops with care,
From null-checks light to blanks aware,
Empty strings now find no gate,
Consent flows true, precise, and great! ✨

🚥 Pre-merge checks | ✅ 1 | ❌ 2
❌ Failed checks (2 warnings)
Check name Status Explanation Resolution
Description check ⚠️ Warning The PR description is incomplete and missing critical required sections compared to the template. Add the actual issue link for #828, specify applicable labels, complete the Development and Testing checklists with checkmarks or status indicators, and include Automation Test Details and Conformance Tests Details tables.
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 (1 passed)
Check name Status Explanation
Title check ✅ Passed The title references fixes for the Accelerator detected from CDS Toolkit, which aligns with the actual code change (fixing blank string handling in consent retrieval), though it is somewhat broad and generic.

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

✨ Finishing touches
  • 📝 Generate docstrings

📜 Recent review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between a3874cd and 14bd88c.

📒 Files selected for processing (1)
  • financial-services-accelerator/components/org.wso2.financial.services.accelerator.consent.mgt.extensions/src/main/java/org/wso2/financial/services/accelerator/consent/mgt/extensions/authorize/impl/ExternalAPIConsentRetrievalStep.java
🔇 Additional comments (1)
financial-services-accelerator/components/org.wso2.financial.services.accelerator.consent.mgt.extensions/src/main/java/org/wso2/financial/services/accelerator/consent/mgt/extensions/authorize/impl/ExternalAPIConsentRetrievalStep.java (1)

118-129: LGTM! Robust validation for consentId.

Using StringUtils.isNotBlank(consentId) instead of a null-check is the correct fix—it properly handles empty strings and whitespace-only values before attempting consent retrieval.

One consideration: Line 113 calls setMandatoryConsentData(consentId, consentData) in the pre-initiated consent flow without a similar blank check. If consentId could ever be blank in that flow, a similar guard may be warranted there. However, since pre-initiated consent flows should always have a valid consent ID, the current approach is reasonable—letting the downstream getConsent call fail explicitly on invalid input may be intentional.


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.

@Ashi1993 Ashi1993 merged commit 20e3456 into wso2:main Jan 21, 2026
3 checks passed
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.

3 participants