[OB4] Adding internal Consent Manage PUT endpoint#916
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds transactional detailed-consent update support: new DAO batch-delete APIs and SQL, service-level updateDetailedConsent that deletes-and-replaces related collections within a transaction, new handler/validator/utils for internal PUT updates, JOIN change in a query, and accompanying unit tests across DAO, service, and handler layers. Changes
Sequence Diagram(s)sequenceDiagram
rect rgba(63,81,181,0.5)
participant Client as External Client
end
rect rgba(76,175,80,0.5)
participant Handler as DefaultConsentManageHandler
participant Utils as ConsentManageUtils
end
rect rgba(255,152,0,0.5)
participant Service as ConsentCoreService
participant DAO as ConsentCoreDAO
end
participant DB as Database
Client->>Handler: PUT /consent/{id} (internal)
Handler->>Utils: isInternalConsentRequest(consentManageData)
Utils-->>Handler: true
Handler->>Service: getDetailedConsent(consentId)
Service->>DAO: retrieveDetailedConsent(consentId)
DAO->>DB: SELECT consent + attrs + auth + mappings
DB-->>DAO: consent details
DAO-->>Service: DetailedConsentResource
Service-->>Handler: existing DetailedConsentResource
Handler->>Utils: constructDetailedConsentResourceFromUpdatePayload(payload, existing)
Utils-->>Handler: updated DetailedConsentResource
Handler->>Service: updateDetailedConsent(updatedResource)
Service->>DAO: updateConsentResource(base)
Service->>DAO: deleteAuthorizationResources(ids)
DAO->>DB: DELETE authorizations
Service->>DAO: storeAuthorizationResources(new)
DAO->>DB: INSERT authorizations
Service->>DAO: deleteConsentMappingResources(ids)
DAO->>DB: DELETE mappings
Service->>DAO: storeConsentMappingResources(new)
DAO->>DB: INSERT mappings
Service->>DAO: storeConsentAuditRecord(audit)
DAO->>DB: INSERT audit
Service-->>Handler: updated DetailedConsentResource
Handler->>Utils: constructConsentUpdateResponse(updated)
Utils-->>Handler: JSONObject
Handler-->>Client: 200 OK (updated consent)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
![wso2-engineering[bot]](https://avatars.githubusercontent.com/in/1856828?s=60&v=4){kind=small}
...in/java/org/wso2/financial/services/accelerator/consent/mgt/dao/impl/ConsentCoreDAOImpl.java
Show resolved
Hide resolved
...in/java/org/wso2/financial/services/accelerator/consent/mgt/dao/impl/ConsentCoreDAOImpl.java
Show resolved
Hide resolved
...ial/services/accelerator/consent/mgt/extensions/manage/impl/DefaultConsentManageHandler.java
Show resolved
Hide resolved
...ial/services/accelerator/consent/mgt/extensions/manage/impl/DefaultConsentManageHandler.java
Show resolved
Hide resolved
...l/services/accelerator/consent/mgt/extensions/manage/impl/DefaultConsentManageValidator.java
Show resolved
Hide resolved
...l/services/accelerator/consent/mgt/extensions/manage/impl/DefaultConsentManageValidator.java
Show resolved
Hide resolved
...2/financial/services/accelerator/consent/mgt/extensions/manage/utils/ConsentManageUtils.java
Show resolved
Hide resolved
...2/financial/services/accelerator/consent/mgt/extensions/manage/utils/ConsentManageUtils.java
Show resolved
Hide resolved
...cial/services/accelerator/consent/mgt/extensions/manage/DefaultConsentManageHandlerTest.java
Show resolved
Hide resolved
...cial/services/accelerator/consent/mgt/extensions/manage/DefaultConsentManageHandlerTest.java
Show resolved
Hide resolved
...ain/java/org/wso2/financial/services/accelerator/consent/mgt/service/ConsentCoreService.java
Show resolved
Hide resolved
...org/wso2/financial/services/accelerator/consent/mgt/service/impl/ConsentCoreServiceImpl.java
Show resolved
Hide resolved
...org/wso2/financial/services/accelerator/consent/mgt/service/impl/ConsentCoreServiceImpl.java
Show resolved
Hide resolved
...org/wso2/financial/services/accelerator/consent/mgt/service/util/ConsentCoreServiceUtil.java
Show resolved
Hide resolved
...org/wso2/financial/services/accelerator/consent/mgt/service/util/ConsentCoreServiceUtil.java
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
AI Agent Log Improvement Checklist
- The log-related comments and suggestions in this review were generated by an AI tool to assist with identifying potential improvements. Purpose of reviewing the code for log improvements is to improve the troubleshooting capabilities of our products.
- Please make sure to manually review and validate all suggestions before applying any changes. Not every code suggestion would make sense or add value to our purpose. Therefore, you have the freedom to decide which of the suggestions are helpful.
✅ Before merging this pull request:
- Review all AI-generated comments for accuracy and relevance.
- Complete and verify the table below. We need your feedback to measure the accuracy of these suggestions and the value they add. If you are rejecting a certain code suggestion, please mention the reason briefly in the suggestion for us to capture it.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (4)
financial-services-accelerator/components/org.wso2.financial.services.accelerator.consent.mgt.extensions/src/main/java/org/wso2/financial/services/accelerator/consent/mgt/extensions/manage/impl/DefaultConsentManageValidator.java (1)
490-501: Consider reusing ObjectMapper instance for better performance.Creating a new
ObjectMapperinstance on every validation call is inefficient.ObjectMapperis thread-safe and expensive to construct. Consider making it a static field.♻️ Proposed refactor
public class DefaultConsentManageValidator implements ConsentManageValidator { private static final Log log = LogFactory.getLog(DefaultConsentManageValidator.class); + private static final ObjectMapper objectMapper = new ObjectMapper(); private static final List<String> validPermissions = Arrays.asList(Then update the method:
public static ConsentPayloadValidationResult validateConsentUpdatePayload(JSONObject request) { try { - ObjectMapper objectMapper = new ObjectMapper(); objectMapper.readValue(request.toString(), InternalConsentUpdateRequestDTO.class); return new ConsentPayloadValidationResult(true); } catch (JsonProcessingException e) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@financial-services-accelerator/components/org.wso2.financial.services.accelerator.consent.mgt.extensions/src/main/java/org/wso2/financial/services/accelerator/consent/mgt/extensions/manage/impl/DefaultConsentManageValidator.java` around lines 490 - 501, The validateConsentUpdatePayload method currently instantiates a new ObjectMapper on every call; make ObjectMapper a single shared instance (e.g., a private static final ObjectMapper) in DefaultConsentManageValidator and reuse it inside validateConsentUpdatePayload (replace new ObjectMapper() with the static field). Ensure the static ObjectMapper is configured as needed and remains thread-safe, and keep the existing exception handling that constructs ConsentPayloadValidationResult.financial-services-accelerator/components/org.wso2.financial.services.accelerator.consent.mgt.service/src/test/java/org/wso2/financial/services/accelerator/consent/mgt/service/impl/ConsentMgtCoreServiceTests.java (1)
539-575: Strengthen these tests with observable side-effect assertions.
updateDetailedConsentis mainly a replace workflow, but the happy-path test only proves that some object is returned. A no-op or partial implementation that skips child-resource replacement or writes the wrong audit payload would still pass here. Please verify at least one replaced child collection or the audit interaction, and add a regression case for clearing existing children.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@financial-services-accelerator/components/org.wso2.financial.services.accelerator.consent.mgt.service/src/test/java/org/wso2/financial/services/accelerator/consent/mgt/service/impl/ConsentMgtCoreServiceTests.java` around lines 539 - 575, The happy-path test for updateDetailedConsent currently only asserts a non-null return; extend it to assert observable side-effects by verifying that child collections on the returned DetailedConsentResource were replaced (e.g., assert sizes/contents of any child lists that come from getSampleDetailedStoredTestConsentResource vs. the input), and verify the audit interaction by asserting mockedConsentCoreDAO.storeConsentStatusAuditRecord(...) was invoked with the expected audit payload (use Mockito verify with argument captor to inspect the stored record). Add a regression test where the existing stored consent has child elements and the incoming consent has an empty/null child collection, then call updateDetailedConsent and assert the stored/returned consent children are cleared and that storeConsentStatusAuditRecord was still called with the expected audit entry.financial-services-accelerator/components/org.wso2.financial.services.accelerator.consent.mgt.service/src/main/java/org/wso2/financial/services/accelerator/consent/mgt/service/impl/ConsentCoreServiceImpl.java (1)
335-343: Use the detailed-consent audit key consistently here.Other detailed-consent flows in this class put the resource under
ConsentCoreServiceConstants.DETAILED_CONSENT_RESOURCE, but this method usesCONSENT_RESOURCE.postStateChange()ignores the map today, yet the listener hook inConsentCoreServiceUtilis clearly intended to consume it later, so this inconsistency will become a subtle break when that path is enabled.💡 Small fix
- consentDataMap.put(ConsentCoreServiceConstants.CONSENT_RESOURCE, detailedConsentResource); + consentDataMap.put(ConsentCoreServiceConstants.DETAILED_CONSENT_RESOURCE, detailedConsentResource);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@financial-services-accelerator/components/org.wso2.financial.services.accelerator.consent.mgt.service/src/main/java/org/wso2/financial/services/accelerator/consent/mgt/service/impl/ConsentCoreServiceImpl.java` around lines 335 - 343, The audit map is using the wrong key: change the entry in consentDataMap from ConsentCoreServiceConstants.CONSENT_RESOURCE to ConsentCoreServiceConstants.DETAILED_CONSENT_RESOURCE so the detailedConsentResource is stored under the same key used elsewhere; update the line that does consentDataMap.put(...) to use DETAILED_CONSENT_RESOURCE so postStateChange(...) and ConsentCoreServiceUtil listener code will receive the expected map key for detailedConsentResource.financial-services-accelerator/components/org.wso2.financial.services.accelerator.consent.mgt.dao/src/test/java/org/wso2/financial/services/accelerator/consent/mgt/dao/impl/ConsentCoreDAOTests.java (1)
703-705: Assert the delete side effect, not just the return flag.
deleteAuthorizationResources()anddeleteConsentMappingResources()returntrueeven whenexecuteBatch()deletes zero rows inConsentCoreDAOImpl.java, so these assertions can still pass on a broken delete. Re-querying the resource and asserting not-found would make these tests meaningful.Also applies to: 929-931
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@financial-services-accelerator/components/org.wso2.financial.services.accelerator.consent.mgt.dao/src/test/java/org/wso2/financial/services/accelerator/consent/mgt/dao/impl/ConsentCoreDAOTests.java` around lines 703 - 705, The test currently only asserts the boolean return from consentCoreDAO.deleteAuthorizationResources(), which can be true even when no rows were deleted; update ConsentCoreDAOTests to assert the side-effect by re-querying the deleted resource after the delete call (use the DAO method that fetches authorization resources by ID, e.g., getAuthorizationResourceById or the equivalent finder used elsewhere) and assert it is not found (null/empty). Apply the same pattern for deleteConsentMappingResources() tests (re-query the consent mapping via its lookup method and assert absence) so the test fails if executeBatch() deletes zero rows.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@financial-services-accelerator/components/org.wso2.financial.services.accelerator.consent.mgt.dao/src/main/java/org/wso2/financial/services/accelerator/consent/mgt/dao/impl/ConsentCoreDAOImpl.java`:
- Around line 566-578: The method currently logs when a batch delete has partial
failures (using results and allUpdated) but always returns true; change the
control flow so that if not allUpdated the method returns false (or throws a
ConsentDataDeletionException) instead of falling through to return true;
specifically update the block that processes int[] results from
deleteAuthorizationResourcePreparedStmt to return false when
Arrays.stream(results).allMatch(result -> result > 0) is false, and apply the
identical fix to the deleteConsentMappingResources(...) implementation (the
block that processes its executeBatch results) so partial batch deletes are
reported as failures rather than success.
In
`@financial-services-accelerator/components/org.wso2.financial.services.accelerator.consent.mgt.extensions/src/main/java/org/wso2/financial/services/accelerator/consent/mgt/extensions/manage/impl/DefaultConsentManageHandler.java`:
- Around line 152-160: The internal-request branch in
ConsentManageUtils.isInternalConsentRequest(...) currently returns early after
calling consentCoreService.getDetailedConsent(consentId) and setting
consentManageData, which bypasses ownership/client-id checks and can NPE on a
null detailedConsentResource; instead remove the early return and change the
flow so you call consentCoreService.getDetailedConsent(consentId), check for
null (set consentManageData.responseStatus to NOT_FOUND and return), then
perform the existing client-id/ownership validation (the same checks at the
later block), and only after those validations set
consentManageData.setResponsePayload(new JSONObject(detailedConsentResource))
and consentManageData.setResponseStatus(ResponseStatus.OK); keep references to
ConsentManageUtils.isInternalConsentRequest,
consentCoreService.getDetailedConsent, consentManageData.setResponsePayload,
consentManageData.setResponseStatus and consentId to locate and update the code.
In
`@financial-services-accelerator/components/org.wso2.financial.services.accelerator.consent.mgt.extensions/src/main/java/org/wso2/financial/services/accelerator/consent/mgt/extensions/manage/utils/ConsentManageUtils.java`:
- Around line 322-362: constructDetailedConsentResourceFromUpdatePayload
currently overwrites stored values when the DTO omits fields; change it so
absent fields fall back to storedConsent values. Concretely: if
updateRequestDTO.getAuthorizationResources() is null, set authorizationResources
= storedConsent.getAuthorizationResources() (and similarly for
consentMappingResources = storedConsent.getConsentMappingResources()); when
iterating DTO authorizations, only replace nested resources for an authorization
if authorization.getResources() is non-null, otherwise preserve the existing
mappings for that authorization from storedConsent; likewise, for simple
optional fields (receipt, status, consentFrequency, validityPeriod,
recurringIndicator, consentAttributes) use updateRequestDTO values when non-null
and fall back to
storedConsent.getReceipt()/getStatus()/getConsentFrequency()/getValidityPeriod()/isRecurringIndicator()/getConsentAttributes()
when DTO values are null. Ensure this prevents
deleteExistingAuthorizationResources()/deleteExistingConsentMappings() from
wiping data when DTO omits those fields.
In
`@financial-services-accelerator/components/org.wso2.financial.services.accelerator.consent.mgt.extensions/src/test/java/org/wso2/financial/services/accelerator/consent/mgt/extensions/manage/DefaultConsentManageHandlerTest.java`:
- Around line 153-257: The tests create fresh Mockito ConsentManageData mocks
but never stub getHeaders(), causing a NullPointerException when
DefaultConsentManageHandler.handlePost()/handleGet()/handleDelete() call
consentManageData.getHeaders().containsKey(...); fix each affected test in
DefaultConsentManageHandlerTest by stubbing getHeaders() on the mock to return
the shared headers map (e.g., add a
doReturn(headers).when(consentManageDataMock).getHeaders() before invoking
defaultConsentManageHandler.handlePost()/handleGet()/handleDelete()); apply the
same change to all listed test blocks (including the blocks around lines
283-309, 353-380, 403-415, 438-458, 464-587, 592-665, 670-794) so getHeaders()
is never null at runtime.
- Around line 153-257: Several negative-path tests (e.g.,
testHandlePostWithoutClientId, testHandlePostWithInvalidHeaders,
testHandlePostWithoutRequestPath, testHandlePostWithInvalidRequestPath,
testHandlePostWithInvalidPayload, testHandlePostWithInvalidInitiation) call
defaultConsentManageHandler.handlePost(...) inside try blocks but only assert
inside the catch, so they silently pass if no ConsentException is thrown; after
each call to defaultConsentManageHandler.handlePost(...) add
Assert.fail("Expected ConsentException") immediately so the test fails when no
ConsentException is raised (apply the same fix to other similar tests in the
mentioned ranges), keeping the catch to assert on e.getPayload() as-is.
- Around line 843-885: In testHandlePutWithInvalidRequestPath the second stub of
consentManageDataMock.getHeaders() overrides the earlier internal-request header
and causes handlePut() to take the wrong branch; update the test so the mock
keeps the internal header (either remove the
doReturn(headers).when(consentManageDataMock).getHeaders() line or ensure the
headers variable includes ConsentManageConstants.INTERNAL_API_REQUEST_HEADER ->
"true") so that defaultConsentManageHandler.handlePut(consentManageDataMock)
exercises the invalid-path branch and the subsequent assertion on the error
description executes.
In
`@financial-services-accelerator/components/org.wso2.financial.services.accelerator.consent.mgt.extensions/src/test/java/org/wso2/financial/services/accelerator/consent/mgt/extensions/util/TestConstants.java`:
- Line 42: The test constant TestConstants.CONSENT_UPDATE_PATH is using the
plural "consents/" which doesn't match the regex in
ConsentExtensionConstants.CONSENT_UPDATE_PATH ("^consent/([^/?]+)$"); update
TestConstants.CONSENT_UPDATE_PATH to use the singular "consent/" concatenated
with SAMPLE_CONSENT_ID so the path matches the handler's validation in
DefaultConsentManageHandler (where the request path is matched against
ConsentExtensionConstants.CONSENT_UPDATE_PATH).
In
`@financial-services-accelerator/components/org.wso2.financial.services.accelerator.consent.mgt.service/src/main/java/org/wso2/financial/services/accelerator/consent/mgt/service/impl/ConsentCoreServiceImpl.java`:
- Around line 299-300: The code currently calls
getDetailedConsent(consentIdToUpdate) outside the write transaction; replace
that call so both reads occur on the current transaction connection by using
consentCoreDAO.getDetailedConsentResource(connection, consentIdToUpdate) to
fetch the existing (previous) consent and then fetch the updated consent from
the same DAO/connection before performing the update; also explicitly null-check
the previous consent result and handle the missing-consent case (return/fail)
before proceeding with the update/audit to avoid out-of-transaction reads and
partial commits.
- Around line 309-333: The update logic currently skips deletion when child
collections are empty (uses CollectionUtils.isNotEmpty/MapUtils.isNotEmpty), so
you must treat null as "no change" but empty collection as "clear children": for
each block (consentAttributes, authorizationResources, consentMappingResources)
check for != null first, call
ConsentCoreServiceUtil.deleteExistingConsentAttributes/deleteExistingAuthorizationResources/deleteExistingConsentMappings
with (consentCoreDAO, connection, consentIdToUpdate, previousConsent) to remove
existing rows, and then only call
ConsentCoreServiceUtil.addConsentAttributes/addAuthorizationResources/addConsentMappingResources
when the incoming collection is non-empty
(CollectionUtils.isNotEmpty/MapUtils.isNotEmpty) so empty collections clear
children while null leaves them unchanged.
---
Nitpick comments:
In
`@financial-services-accelerator/components/org.wso2.financial.services.accelerator.consent.mgt.dao/src/test/java/org/wso2/financial/services/accelerator/consent/mgt/dao/impl/ConsentCoreDAOTests.java`:
- Around line 703-705: The test currently only asserts the boolean return from
consentCoreDAO.deleteAuthorizationResources(), which can be true even when no
rows were deleted; update ConsentCoreDAOTests to assert the side-effect by
re-querying the deleted resource after the delete call (use the DAO method that
fetches authorization resources by ID, e.g., getAuthorizationResourceById or the
equivalent finder used elsewhere) and assert it is not found (null/empty). Apply
the same pattern for deleteConsentMappingResources() tests (re-query the consent
mapping via its lookup method and assert absence) so the test fails if
executeBatch() deletes zero rows.
In
`@financial-services-accelerator/components/org.wso2.financial.services.accelerator.consent.mgt.extensions/src/main/java/org/wso2/financial/services/accelerator/consent/mgt/extensions/manage/impl/DefaultConsentManageValidator.java`:
- Around line 490-501: The validateConsentUpdatePayload method currently
instantiates a new ObjectMapper on every call; make ObjectMapper a single shared
instance (e.g., a private static final ObjectMapper) in
DefaultConsentManageValidator and reuse it inside validateConsentUpdatePayload
(replace new ObjectMapper() with the static field). Ensure the static
ObjectMapper is configured as needed and remains thread-safe, and keep the
existing exception handling that constructs ConsentPayloadValidationResult.
In
`@financial-services-accelerator/components/org.wso2.financial.services.accelerator.consent.mgt.service/src/main/java/org/wso2/financial/services/accelerator/consent/mgt/service/impl/ConsentCoreServiceImpl.java`:
- Around line 335-343: The audit map is using the wrong key: change the entry in
consentDataMap from ConsentCoreServiceConstants.CONSENT_RESOURCE to
ConsentCoreServiceConstants.DETAILED_CONSENT_RESOURCE so the
detailedConsentResource is stored under the same key used elsewhere; update the
line that does consentDataMap.put(...) to use DETAILED_CONSENT_RESOURCE so
postStateChange(...) and ConsentCoreServiceUtil listener code will receive the
expected map key for detailedConsentResource.
In
`@financial-services-accelerator/components/org.wso2.financial.services.accelerator.consent.mgt.service/src/test/java/org/wso2/financial/services/accelerator/consent/mgt/service/impl/ConsentMgtCoreServiceTests.java`:
- Around line 539-575: The happy-path test for updateDetailedConsent currently
only asserts a non-null return; extend it to assert observable side-effects by
verifying that child collections on the returned DetailedConsentResource were
replaced (e.g., assert sizes/contents of any child lists that come from
getSampleDetailedStoredTestConsentResource vs. the input), and verify the audit
interaction by asserting mockedConsentCoreDAO.storeConsentStatusAuditRecord(...)
was invoked with the expected audit payload (use Mockito verify with argument
captor to inspect the stored record). Add a regression test where the existing
stored consent has child elements and the incoming consent has an empty/null
child collection, then call updateDetailedConsent and assert the stored/returned
consent children are cleared and that storeConsentStatusAuditRecord was still
called with the expected audit entry.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 882144ba-710b-40f6-95bd-605d55d47f34
📒 Files selected for processing (18)
financial-services-accelerator/components/org.wso2.financial.services.accelerator.consent.mgt.dao/src/main/java/org/wso2/financial/services/accelerator/consent/mgt/dao/ConsentCoreDAO.javafinancial-services-accelerator/components/org.wso2.financial.services.accelerator.consent.mgt.dao/src/main/java/org/wso2/financial/services/accelerator/consent/mgt/dao/constants/ConsentMgtDAOConstants.javafinancial-services-accelerator/components/org.wso2.financial.services.accelerator.consent.mgt.dao/src/main/java/org/wso2/financial/services/accelerator/consent/mgt/dao/impl/ConsentCoreDAOImpl.javafinancial-services-accelerator/components/org.wso2.financial.services.accelerator.consent.mgt.dao/src/main/java/org/wso2/financial/services/accelerator/consent/mgt/dao/queries/ConsentMgtCommonDBQueries.javafinancial-services-accelerator/components/org.wso2.financial.services.accelerator.consent.mgt.dao/src/test/java/org/wso2/financial/services/accelerator/consent/mgt/dao/impl/ConsentCoreDAOTests.javafinancial-services-accelerator/components/org.wso2.financial.services.accelerator.consent.mgt.extensions/src/main/java/org/wso2/financial/services/accelerator/consent/mgt/extensions/common/ConsentExtensionConstants.javafinancial-services-accelerator/components/org.wso2.financial.services.accelerator.consent.mgt.extensions/src/main/java/org/wso2/financial/services/accelerator/consent/mgt/extensions/manage/impl/DefaultConsentManageHandler.javafinancial-services-accelerator/components/org.wso2.financial.services.accelerator.consent.mgt.extensions/src/main/java/org/wso2/financial/services/accelerator/consent/mgt/extensions/manage/impl/DefaultConsentManageValidator.javafinancial-services-accelerator/components/org.wso2.financial.services.accelerator.consent.mgt.extensions/src/main/java/org/wso2/financial/services/accelerator/consent/mgt/extensions/manage/utils/ConsentManageConstants.javafinancial-services-accelerator/components/org.wso2.financial.services.accelerator.consent.mgt.extensions/src/main/java/org/wso2/financial/services/accelerator/consent/mgt/extensions/manage/utils/ConsentManageUtils.javafinancial-services-accelerator/components/org.wso2.financial.services.accelerator.consent.mgt.extensions/src/test/java/org/wso2/financial/services/accelerator/consent/mgt/extensions/manage/DefaultConsentManageHandlerTest.javafinancial-services-accelerator/components/org.wso2.financial.services.accelerator.consent.mgt.extensions/src/test/java/org/wso2/financial/services/accelerator/consent/mgt/extensions/util/DataProviders.javafinancial-services-accelerator/components/org.wso2.financial.services.accelerator.consent.mgt.extensions/src/test/java/org/wso2/financial/services/accelerator/consent/mgt/extensions/util/TestConstants.javafinancial-services-accelerator/components/org.wso2.financial.services.accelerator.consent.mgt.service/src/main/java/org/wso2/financial/services/accelerator/consent/mgt/service/ConsentCoreService.javafinancial-services-accelerator/components/org.wso2.financial.services.accelerator.consent.mgt.service/src/main/java/org/wso2/financial/services/accelerator/consent/mgt/service/constants/ConsentCoreServiceConstants.javafinancial-services-accelerator/components/org.wso2.financial.services.accelerator.consent.mgt.service/src/main/java/org/wso2/financial/services/accelerator/consent/mgt/service/impl/ConsentCoreServiceImpl.javafinancial-services-accelerator/components/org.wso2.financial.services.accelerator.consent.mgt.service/src/main/java/org/wso2/financial/services/accelerator/consent/mgt/service/util/ConsentCoreServiceUtil.javafinancial-services-accelerator/components/org.wso2.financial.services.accelerator.consent.mgt.service/src/test/java/org/wso2/financial/services/accelerator/consent/mgt/service/impl/ConsentMgtCoreServiceTests.java
...in/java/org/wso2/financial/services/accelerator/consent/mgt/dao/impl/ConsentCoreDAOImpl.java
Outdated
Show resolved
Hide resolved
...ial/services/accelerator/consent/mgt/extensions/manage/impl/DefaultConsentManageHandler.java
Show resolved
Hide resolved
...2/financial/services/accelerator/consent/mgt/extensions/manage/utils/ConsentManageUtils.java
Show resolved
Hide resolved
...cial/services/accelerator/consent/mgt/extensions/manage/DefaultConsentManageHandlerTest.java
Show resolved
Hide resolved
...cial/services/accelerator/consent/mgt/extensions/manage/DefaultConsentManageHandlerTest.java
Show resolved
Hide resolved
.../java/org/wso2/financial/services/accelerator/consent/mgt/extensions/util/TestConstants.java
Outdated
Show resolved
Hide resolved
...org/wso2/financial/services/accelerator/consent/mgt/service/impl/ConsentCoreServiceImpl.java
Outdated
Show resolved
Hide resolved
...org/wso2/financial/services/accelerator/consent/mgt/service/impl/ConsentCoreServiceImpl.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (4)
financial-services-accelerator/components/org.wso2.financial.services.accelerator.consent.mgt.extensions/src/test/java/org/wso2/financial/services/accelerator/consent/mgt/extensions/manage/DefaultConsentManageHandlerTest.java (2)
918-925:⚠️ Potential issue | 🟠 MajorThe internal-header stub is being overwritten here.
Line 924 replaces the internal-request header with
headers, which does not containINTERNAL_API_REQUEST_HEADER. This test can no longer reliably reach the invalid-path branch of the new internal PUT flow.💚 Minimal fix
- doReturn(Map.of(ConsentManageConstants.INTERNAL_API_REQUEST_HEADER, "true")) - .when(consentManageDataMock).getHeaders(); + Map<String, String> internalHeaders = new HashMap<>(headers); + internalHeaders.put(ConsentManageConstants.INTERNAL_API_REQUEST_HEADER, "true"); + doReturn(internalHeaders).when(consentManageDataMock).getHeaders(); doReturn("consent/1213").when(consentManageDataMock).getRequestPath(); doReturn(TestConstants.SAMPLE_CLIENT_ID).when(consentManageDataMock).getClientId(); - doReturn(headers).when(consentManageDataMock).getHeaders(); defaultConsentManageHandler.handlePut(consentManageDataMock);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@financial-services-accelerator/components/org.wso2.financial.services.accelerator.consent.mgt.extensions/src/test/java/org/wso2/financial/services/accelerator/consent/mgt/extensions/manage/DefaultConsentManageHandlerTest.java` around lines 918 - 925, The test stubs the internal API header on the mock ConsentManageData but then overwrites it with a second doReturn(headers).when(consentManageDataMock).getHeaders(), removing ConsentManageConstants.INTERNAL_API_REQUEST_HEADER; update the stub for consentManageDataMock.getHeaders() so it preserves the internal header (either remove the second overwrite or merge the two maps so the returned headers include ConsentManageConstants.INTERNAL_API_REQUEST_HEADER) so defaultConsentManageHandler.handlePut(consentManageDataMock) will exercise the internal PUT invalid-path branch.
153-257:⚠️ Potential issue | 🟡 MinorFail these negative-path tests on unexpected success.
These blocks only assert inside
catch, so a normal return still passes the test suite.💚 Minimal fix
defaultConsentManageHandler.handlePost(consentManageDataMock); + Assert.fail("Expected ConsentException"); } catch (ConsentException e) { Assert.assertEquals(e.getPayload().getJSONObject("error").getString("description"), "Client ID missing in the request.");Also applies to: 350-415, 461-562, 629-657, 707-789, 836-949, 968-977
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@financial-services-accelerator/components/org.wso2.financial.services.accelerator.consent.mgt.extensions/src/test/java/org/wso2/financial/services/accelerator/consent/mgt/extensions/manage/DefaultConsentManageHandlerTest.java` around lines 153 - 257, The tests (e.g., testHandlePostWithoutClientId, testHandlePostWithInvalidHeaders, testHandlePostWithoutRequestPath, testHandlePostWithInvalidRequestPath, testHandlePostWithInvalidPayload, testHandlePostWithInvalidInitiation) rely only on assertions inside the catch block so a successful return from defaultConsentManageHandler.handlePost(...) will incorrectly pass; update each test to explicitly fail when no ConsentException is thrown by adding an immediate failure after the handlePost call (e.g., call Assert.fail("Expected ConsentException") or TestNG's fail) so the test fails on unexpected success, leaving the existing catch-based assertions intact; apply the same change to the other listed test blocks as well.financial-services-accelerator/components/org.wso2.financial.services.accelerator.consent.mgt.extensions/src/main/java/org/wso2/financial/services/accelerator/consent/mgt/extensions/manage/utils/ConsentManageUtils.java (1)
333-369:⚠️ Potential issue | 🔴 CriticalOmitted fields are still being cleared instead of merged.
This still contradicts the Javadoc on Lines 313-320. Lines 346-355 only preserve mappings when the whole
authorizationResourcesarray is absent, and Lines 364-369 still write DTO values directly for scalar fields. If the payload omitsresourceson one authorization or omits fields likereceipt/status/consentAttributes, the constructed object carries empty/null/default values downstream and the update flow will wipe stored data instead of preserving it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@financial-services-accelerator/components/org.wso2.financial.services.accelerator.consent.mgt.extensions/src/main/java/org/wso2/financial/services/accelerator/consent/mgt/extensions/manage/utils/ConsentManageUtils.java` around lines 333 - 369, The code currently overwrites omitted fields instead of merging: update the construction logic for authorizationResources/consentMappingResources and scalar fields so missing values are preserved from storedConsent. Specifically, when building authorizationResources in ConsentManageUtils (variables authorizationResources, consentMappingResources, storedConsent, updateRequestDTO), if updateRequestDTO.getAuthorizationResources() is present iterate each authorization and when an authorization.getResources() is null or empty, look up the matching stored AuthorizationResource by authorizationID (or fallback to userID) from storedConsent.getAuthorizationResources() and copy its ConsentMappingResource entries into consentMappingResources; when resources are present, merge per-resource by mappingID matching storedConsent.getConsentMappingResources() to preserve mappingIDs/status if omitted. For scalar fields used in the DetailedConsentResource constructor (receipt, status, consentFrequency, validityPeriod, recurringIndicator, consentAttributes), use updateRequestDTO's value only when non-null/defined otherwise fall back to storedConsent's corresponding value so omitted scalars are not cleared.financial-services-accelerator/components/org.wso2.financial.services.accelerator.consent.mgt.service/src/main/java/org/wso2/financial/services/accelerator/consent/mgt/service/impl/ConsentCoreServiceImpl.java (1)
299-351:⚠️ Potential issue | 🟠 MajorReturn the updated consent from this transaction and null-check the original lookup.
getDetailedConsentResource()can still returnnull, and Line 351 performs a new read after Line 347 has already committed. If that second read fails, callers see an overall failure even though the update and audit were already persisted. Fetch the updated row onconnection, fail fast when the initial lookup is missing, then commit and return that in-transaction copy.Suggested fix
DetailedConsentResource previousConsent = consentCoreDAO.getDetailedConsentResource(connection, consentIdToUpdate); + if (previousConsent == null) { + throw new ConsentManagementException("Consent not found for update"); + } /* Update the base consent using updated values from ConsentResource. Immutable parameters are ignored in the update (i.e. clientId, createdTime) at DAO level.*/ ConsentResource consentResourceToUpdate = new ConsentResource(detailedConsentResource); @@ - DatabaseUtils.commitTransaction(connection); - log.debug("Updated the basic consent details, consent attributes, authorization resources and " + - "mapping resource successfully."); - - return getDetailedConsent(consentIdToUpdate); + DetailedConsentResource updatedConsent = consentCoreDAO.getDetailedConsentResource(connection, + consentIdToUpdate); + DatabaseUtils.commitTransaction(connection); + log.debug("Updated the basic consent details, consent attributes, authorization resources and " + + "mapping resource successfully."); + return updatedConsent;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@financial-services-accelerator/components/org.wso2.financial.services.accelerator.consent.mgt.service/src/main/java/org/wso2/financial/services/accelerator/consent/mgt/service/impl/ConsentCoreServiceImpl.java` around lines 299 - 351, The initial lookup previousConsent = consentCoreDAO.getDetailedConsentResource(connection, consentIdToUpdate) must be null-checked and you should return the in-transaction updated row rather than re-reading after commit; fail fast if previousConsent is null, perform consentCoreDAO.updateConsentResource(...), then fetch the updatedDetailed = consentCoreDAO.getDetailedConsentResource(connection, consentIdToUpdate) on the same connection (null-check it), call ConsentCoreServiceUtil.postStateChange(...), commit the transaction, and finally return updatedDetailed instead of calling getDetailedConsent(consentIdToUpdate).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@financial-services-accelerator/components/org.wso2.financial.services.accelerator.consent.mgt.dao/src/main/java/org/wso2/financial/services/accelerator/consent/mgt/dao/impl/ConsentCoreDAOImpl.java`:
- Around line 567-575: The batch-delete result check in ConsentCoreDAOImpl
incorrectly treats Statement.SUCCESS_NO_INFO (-2) as failure; update the
all-match predicate that inspects the int[] from
deleteAuthorizationResourcePreparedStmt.executeBatch() to accept result > 0 OR
result == java.sql.Statement.SUCCESS_NO_INFO, and make the identical change in
the other batch delete in deleteConsentMappingResources so both batch-result
checks treat -2 as success.
In
`@financial-services-accelerator/components/org.wso2.financial.services.accelerator.consent.mgt.extensions/src/main/java/org/wso2/financial/services/accelerator/consent/mgt/extensions/manage/impl/DefaultConsentManageHandler.java`:
- Around line 164-170: In DefaultConsentManageHandler's handleGet flow, the
thrown ConsentException currently uses ConsentOperationEnum.CONSENT_UPDATE;
change this to ConsentOperationEnum.CONSENT_RETRIEVE so the operation enum
matches the GET handler context—update the throw site that checks
detailedConsentResource.getClientID().equals(consentManageData.getClientId()) to
instantiate the ConsentException with ConsentOperationEnum.CONSENT_RETRIEVE
instead of CONSENT_UPDATE.
- Around line 471-501: In DefaultConsentManageHandler.handlePut add the same
interaction-id flow and header validation as the other handlers: retrieve
INTERACTION_ID_HEADER from consentManageData.getRequestHeaders(), if absent
generate a new UUID and put it into consentManageData.getResponseHeaders(); then
call validateRequestHeaders(consentManageData) before processing further. Ensure
you use the same header constant
(ConsentExtensionConstants.INTERACTION_ID_HEADER) and the same
validateRequestHeaders(ConsentManageData) method so behavior matches
handleGet/handlePost/handleDelete.
- Around line 505-512: The log message in DefaultConsentManageHandler currently
logs "Retrieved consent for ID: ..." before checking if storedConsentResource is
null, which can mislead; update the code in the method where
consentCoreService.getDetailedConsent(consentId) is called to either move the
log.info call to after the null-check (so it only logs when
storedConsentResource != null) or change the pre-check message to "Attempting to
retrieve consent for ID:" to reflect intent; ensure you reference the consentId
variable and the storedConsentResource null-check/ConsentException throw so the
log placement and message accurately reflect success vs attempt.
In
`@financial-services-accelerator/components/org.wso2.financial.services.accelerator.consent.mgt.service/src/test/java/org/wso2/financial/services/accelerator/consent/mgt/service/impl/ConsentMgtCoreServiceTests.java`:
- Around line 560-576: The test never exercises the empty-children branch
because it constructs a local detailedConsentResource with empty child
collections but then calls consentCoreServiceImpl.updateDetailedConsent(...)
with ConsentMgtServiceTestData.getSampleDetailedStoredTestConsentResource()
instead; change that call to pass the local detailedConsentResource variable so
updateDetailedConsent(...) (tested method) receives the empty collections and
the mockedConsentCoreDAO interactions remain valid (mocks for
getDetailedConsentResource, updateConsentResource, storeConsentStatusAuditRecord
still apply), then adjust or add assertions on the returned
DetailedConsentResource to verify the clear-children behavior.
---
Duplicate comments:
In
`@financial-services-accelerator/components/org.wso2.financial.services.accelerator.consent.mgt.extensions/src/main/java/org/wso2/financial/services/accelerator/consent/mgt/extensions/manage/utils/ConsentManageUtils.java`:
- Around line 333-369: The code currently overwrites omitted fields instead of
merging: update the construction logic for
authorizationResources/consentMappingResources and scalar fields so missing
values are preserved from storedConsent. Specifically, when building
authorizationResources in ConsentManageUtils (variables authorizationResources,
consentMappingResources, storedConsent, updateRequestDTO), if
updateRequestDTO.getAuthorizationResources() is present iterate each
authorization and when an authorization.getResources() is null or empty, look up
the matching stored AuthorizationResource by authorizationID (or fallback to
userID) from storedConsent.getAuthorizationResources() and copy its
ConsentMappingResource entries into consentMappingResources; when resources are
present, merge per-resource by mappingID matching
storedConsent.getConsentMappingResources() to preserve mappingIDs/status if
omitted. For scalar fields used in the DetailedConsentResource constructor
(receipt, status, consentFrequency, validityPeriod, recurringIndicator,
consentAttributes), use updateRequestDTO's value only when non-null/defined
otherwise fall back to storedConsent's corresponding value so omitted scalars
are not cleared.
In
`@financial-services-accelerator/components/org.wso2.financial.services.accelerator.consent.mgt.extensions/src/test/java/org/wso2/financial/services/accelerator/consent/mgt/extensions/manage/DefaultConsentManageHandlerTest.java`:
- Around line 918-925: The test stubs the internal API header on the mock
ConsentManageData but then overwrites it with a second
doReturn(headers).when(consentManageDataMock).getHeaders(), removing
ConsentManageConstants.INTERNAL_API_REQUEST_HEADER; update the stub for
consentManageDataMock.getHeaders() so it preserves the internal header (either
remove the second overwrite or merge the two maps so the returned headers
include ConsentManageConstants.INTERNAL_API_REQUEST_HEADER) so
defaultConsentManageHandler.handlePut(consentManageDataMock) will exercise the
internal PUT invalid-path branch.
- Around line 153-257: The tests (e.g., testHandlePostWithoutClientId,
testHandlePostWithInvalidHeaders, testHandlePostWithoutRequestPath,
testHandlePostWithInvalidRequestPath, testHandlePostWithInvalidPayload,
testHandlePostWithInvalidInitiation) rely only on assertions inside the catch
block so a successful return from defaultConsentManageHandler.handlePost(...)
will incorrectly pass; update each test to explicitly fail when no
ConsentException is thrown by adding an immediate failure after the handlePost
call (e.g., call Assert.fail("Expected ConsentException") or TestNG's fail) so
the test fails on unexpected success, leaving the existing catch-based
assertions intact; apply the same change to the other listed test blocks as
well.
In
`@financial-services-accelerator/components/org.wso2.financial.services.accelerator.consent.mgt.service/src/main/java/org/wso2/financial/services/accelerator/consent/mgt/service/impl/ConsentCoreServiceImpl.java`:
- Around line 299-351: The initial lookup previousConsent =
consentCoreDAO.getDetailedConsentResource(connection, consentIdToUpdate) must be
null-checked and you should return the in-transaction updated row rather than
re-reading after commit; fail fast if previousConsent is null, perform
consentCoreDAO.updateConsentResource(...), then fetch the updatedDetailed =
consentCoreDAO.getDetailedConsentResource(connection, consentIdToUpdate) on the
same connection (null-check it), call
ConsentCoreServiceUtil.postStateChange(...), commit the transaction, and finally
return updatedDetailed instead of calling getDetailedConsent(consentIdToUpdate).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cc380fa2-73ac-4f72-9bbd-049b3901fba1
📒 Files selected for processing (8)
financial-services-accelerator/components/org.wso2.financial.services.accelerator.consent.mgt.dao/src/main/java/org/wso2/financial/services/accelerator/consent/mgt/dao/impl/ConsentCoreDAOImpl.javafinancial-services-accelerator/components/org.wso2.financial.services.accelerator.consent.mgt.extensions/src/main/java/org/wso2/financial/services/accelerator/consent/mgt/extensions/manage/impl/DefaultConsentManageHandler.javafinancial-services-accelerator/components/org.wso2.financial.services.accelerator.consent.mgt.extensions/src/main/java/org/wso2/financial/services/accelerator/consent/mgt/extensions/manage/impl/DefaultConsentManageValidator.javafinancial-services-accelerator/components/org.wso2.financial.services.accelerator.consent.mgt.extensions/src/main/java/org/wso2/financial/services/accelerator/consent/mgt/extensions/manage/utils/ConsentManageUtils.javafinancial-services-accelerator/components/org.wso2.financial.services.accelerator.consent.mgt.extensions/src/test/java/org/wso2/financial/services/accelerator/consent/mgt/extensions/manage/DefaultConsentManageHandlerTest.javafinancial-services-accelerator/components/org.wso2.financial.services.accelerator.consent.mgt.extensions/src/test/java/org/wso2/financial/services/accelerator/consent/mgt/extensions/util/TestConstants.javafinancial-services-accelerator/components/org.wso2.financial.services.accelerator.consent.mgt.service/src/main/java/org/wso2/financial/services/accelerator/consent/mgt/service/impl/ConsentCoreServiceImpl.javafinancial-services-accelerator/components/org.wso2.financial.services.accelerator.consent.mgt.service/src/test/java/org/wso2/financial/services/accelerator/consent/mgt/service/impl/ConsentMgtCoreServiceTests.java
🚧 Files skipped from review as they are similar to previous changes (1)
- financial-services-accelerator/components/org.wso2.financial.services.accelerator.consent.mgt.extensions/src/test/java/org/wso2/financial/services/accelerator/consent/mgt/extensions/util/TestConstants.java
...in/java/org/wso2/financial/services/accelerator/consent/mgt/dao/impl/ConsentCoreDAOImpl.java
Show resolved
Hide resolved
...ial/services/accelerator/consent/mgt/extensions/manage/impl/DefaultConsentManageHandler.java
Show resolved
Hide resolved
...ial/services/accelerator/consent/mgt/extensions/manage/impl/DefaultConsentManageHandler.java
Show resolved
Hide resolved
...ial/services/accelerator/consent/mgt/extensions/manage/impl/DefaultConsentManageHandler.java
Show resolved
Hide resolved
...wso2/financial/services/accelerator/consent/mgt/service/impl/ConsentMgtCoreServiceTests.java
Outdated
Show resolved
Hide resolved
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
`@financial-services-accelerator/components/org.wso2.financial.services.accelerator.consent.mgt.extensions/src/test/java/org/wso2/financial/services/accelerator/consent/mgt/extensions/manage/DefaultConsentManageHandlerTest.java`:
- Around line 947-962: The testHandlePut is missing stubbing of the internal
request header on the ConsentManageData mock; in
DefaultConsentManageHandlerTest, update the testHandlePut setup to stub
consentManageDataMock.getHeaders() to return a Map containing
ConsentManageConstants.INTERNAL_API_REQUEST_HEADER -> "true" (so the handler's
isInternalConsentRequest() path is exercised), using
doReturn(...).when(consentManageDataMock).getHeaders(); keep the existing
payload and verifications unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 644ac469-cb8e-4a07-bb6d-79f2b9cafd4b
📒 Files selected for processing (1)
financial-services-accelerator/components/org.wso2.financial.services.accelerator.consent.mgt.extensions/src/test/java/org/wso2/financial/services/accelerator/consent/mgt/extensions/manage/DefaultConsentManageHandlerTest.java
...cial/services/accelerator/consent/mgt/extensions/manage/DefaultConsentManageHandlerTest.java
Show resolved
Hide resolved
There was a problem hiding this comment.
🧹 Nitpick comments (1)
financial-services-accelerator/components/org.wso2.financial.services.accelerator.consent.mgt.dao/src/main/java/org/wso2/financial/services/accelerator/consent/mgt/dao/impl/ConsentCoreDAOImpl.java (1)
549-582: LGTM - Batch delete implementation correctly handles JDBC semantics.The implementation properly handles
Statement.SUCCESS_NO_INFO(-2) as a successful outcome and throwsConsentDataDeletionExceptionon partial failures, which is appropriate for transactional delete-and-replace operations.One optional consideration: if an empty list is passed, the method executes an empty batch and returns
true(sinceallMatchon an empty stream returnstrue). If this edge case should be explicitly handled, consider adding an early return:💡 Optional: Early return for empty list
public boolean deleteAuthorizationResources(Connection connection, List<String> authorizationResourceIds) throws ConsentDataDeletionException { + if (authorizationResourceIds == null || authorizationResourceIds.isEmpty()) { + log.debug("No authorization resources to delete."); + return true; + } + log.info(String.format("Deleting authorization resources. Count: %s", authorizationResourceIds.size()));,
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@financial-services-accelerator/components/org.wso2.financial.services.accelerator.consent.mgt.dao/src/main/java/org/wso2/financial/services/accelerator/consent/mgt/dao/impl/ConsentCoreDAOImpl.java` around lines 549 - 582, Add an explicit early-return for empty input in the deleteAuthorizationResources method: check the authorizationResourceIds list (and null) at the start of deleteAuthorizationResources, log an informative message (e.g., "No authorization resources to delete") and return true (preserving current behavior) without preparing or executing a batch; this check should be placed before using sqlStatements.getDeleteAuthorizationResourcePreparedStatement() and before creating the PreparedStatement.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@financial-services-accelerator/components/org.wso2.financial.services.accelerator.consent.mgt.dao/src/main/java/org/wso2/financial/services/accelerator/consent/mgt/dao/impl/ConsentCoreDAOImpl.java`:
- Around line 549-582: Add an explicit early-return for empty input in the
deleteAuthorizationResources method: check the authorizationResourceIds list
(and null) at the start of deleteAuthorizationResources, log an informative
message (e.g., "No authorization resources to delete") and return true
(preserving current behavior) without preparing or executing a batch; this check
should be placed before using
sqlStatements.getDeleteAuthorizationResourcePreparedStatement() and before
creating the PreparedStatement.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 09e4cbef-0490-4e11-9301-e791c78ec912
📒 Files selected for processing (2)
financial-services-accelerator/components/org.wso2.financial.services.accelerator.consent.mgt.dao/src/main/java/org/wso2/financial/services/accelerator/consent/mgt/dao/impl/ConsentCoreDAOImpl.javafinancial-services-accelerator/components/org.wso2.financial.services.accelerator.consent.mgt.extensions/src/main/java/org/wso2/financial/services/accelerator/consent/mgt/extensions/manage/impl/DefaultConsentManageHandler.java
VimukthiRajapaksha
approved these changes
Mar 13, 2026
aka4rKO
approved these changes
Mar 14, 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.
Adding internal Consent Manage PUT endpoint
Issue link: #909
Doc Issue: Optional, link issue from documentation repository
Applicable Labels: Spec, product, version, type (specify requested labels)
Development Checklist
Testing Checklist
Summary by CodeRabbit
New Features
Tests
Refactor