Adding consent manage PUT to update basic consent and authorisation resource with ID#911
Closed
Adding consent manage PUT to update basic consent and authorisation resource with ID#911
Conversation
WalkthroughAdds consent update flows with external pre-/post-processing: new service extension types, DTOs, utilities, core update API/implementation, handler wiring, SQL join change, tests and a large OpenAPI spec file. Changes
Sequence DiagramsequenceDiagram
participant Client
participant Handler as ConsentHandler
participant ExtSvc as ExternalService
participant Core as ConsentCoreService
participant DB as Database
Client->>Handler: PUT /consent/{id} (payload + headers)
Handler->>Handler: Validate clientId, path, internal-flag
Handler->>Core: getConsent(consentId)
Core->>DB: SELECT consent (+ attributes)
DB-->>Core: consent resource
Core-->>Handler: existing consent
alt external pre-update enabled
Handler->>ExtSvc: PRE_PROCESS_CONSENT_UPDATE (pre-update DTO)
ExtSvc-->>Handler: pre-update response (possibly modified resource)
end
Handler->>Core: updateConsent(modified resource)
Core->>DB: UPDATE consent, INSERT audit
DB-->>Core: confirmation
Core-->>Handler: updated consent
alt external post-update enabled
Handler->>ExtSvc: ENRICH_CONSENT_UPDATE_RESPONSE (post-update DTO)
ExtSvc-->>Handler: enriched response
end
Handler-->>Client: 200 OK (updated/enriched response)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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}
...wso2/financial/services/accelerator/consent/mgt/extensions/common/ConsentExtensionUtils.java
Show resolved
Hide resolved
...wso2/financial/services/accelerator/consent/mgt/extensions/common/ConsentExtensionUtils.java
Show resolved
Hide resolved
...a/org/wso2/financial/services/accelerator/consent/mgt/extensions/common/ExternalAPIUtil.java
Outdated
Show resolved
Hide resolved
...a/org/wso2/financial/services/accelerator/consent/mgt/extensions/common/ExternalAPIUtil.java
Outdated
Show resolved
Hide resolved
...celerator/consent/mgt/extensions/common/model/ExternalAPIBasicConsentResourceRequestDTO.java
Show resolved
Hide resolved
...celerator/consent/mgt/extensions/common/model/ExternalAPIBasicConsentResourceRequestDTO.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
Outdated
Show resolved
Hide resolved
...s/accelerator/consent/mgt/extensions/manage/model/ExternalAPIPreConsentUpdateRequestDTO.java
Show resolved
Hide resolved
...2/financial/services/accelerator/consent/mgt/extensions/manage/utils/ConsentManageUtils.java
Outdated
Show resolved
Hide resolved
...2/financial/services/accelerator/consent/mgt/extensions/manage/utils/ConsentManageUtils.java
Show resolved
Hide resolved
.../services/accelerator/consent/mgt/extensions/manage/utils/ExternalAPIConsentManageUtils.java
Show resolved
Hide resolved
.../services/accelerator/consent/mgt/extensions/manage/utils/ExternalAPIConsentManageUtils.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
.../test/java/org/wso2/financial/services/accelerator/consent/mgt/extensions/util/TestUtil.java
Show resolved
Hide resolved
.../test/java/org/wso2/financial/services/accelerator/consent/mgt/extensions/util/TestUtil.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
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.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR implements the consent management PUT endpoint in the WSO2 Financial Services Accelerator (issue #909), enabling toolkits to update existing basic consents and authorization resources through the accelerator.
Changes:
- Added
updateConsentmethod toConsentCoreService/ConsentCoreServiceImplto update basic consent data with audit tracking - Implemented
handlePutinDefaultConsentManageHandlersupporting both basic consent updates and internal authorization resource updates, with optional external service extension hooks - Added new DTOs, constants, and utility methods for pre/post consent update extension calls
Reviewed changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
ConsentCoreService.java |
New updateConsent interface method |
ConsentCoreServiceImpl.java |
Implementation of updateConsent with DB transaction and audit record |
ConsentCoreServiceConstants.java |
New BASIC_CONSENT_UPDATE_REASON constant |
DefaultConsentManageHandler.java |
Full handlePut implementation; also bug fixes for CONSENT_RETRIEVE→CONSENT_CREATE/CONSENT_DELETE operation enums in handlePost/handleDelete; internal GET support |
ConsentManageUtils.java |
New isInternalConsentRequest, updateConsentAuthResource, and related helper methods |
ConsentManageConstants.java |
New INTERNAL_API_REQUEST_HEADER and CLIENT_ID_MISMATCH_ERROR constants |
ExternalAPIConsentManageUtils.java |
New overloaded callExternalService for pre/post consent update |
ExternalAPIUtil.java |
New buildConsentResource utility method |
ConsentExtensionUtils.java |
New getInitiationResponse overload for ConsentResource |
ExternalAPIBasicConsentResourceRequestDTO.java |
New DTO for basic consent request |
ExternalAPIBasicConsentResourceResponseDTO.java |
New DTO for basic consent response |
ExternalAPIPreConsentUpdateRequestDTO.java |
New pre-update external service request DTO |
ExternalAPIPreConsentUpdateResponseDTO.java |
New pre-update external service response DTO |
ExternalAPIPostConsentUpdateRequestDTO.java |
New post-update external service request DTO |
ServiceExtensionTypeEnum.java |
Two new enum values for consent update extension types |
DefaultConsentManageHandlerTest.java |
Tests for new handlePut functionality; test refactor from expectedExceptions to try-catch |
ConsentMgtCoreServiceTests.java |
Tests for new updateConsent method |
TestUtil.java / TestConstants.java / DataProviders.java |
Test utilities and constants for new and refactored tests |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
...ial/services/accelerator/consent/mgt/extensions/manage/impl/DefaultConsentManageHandler.java
Show resolved
Hide resolved
...ial/services/accelerator/consent/mgt/extensions/manage/impl/DefaultConsentManageHandler.java
Outdated
Show resolved
Hide resolved
...a/org/wso2/financial/services/accelerator/consent/mgt/extensions/common/ExternalAPIUtil.java
Outdated
Show resolved
Hide resolved
...elerator/consent/mgt/extensions/common/model/ExternalAPIBasicConsentResourceResponseDTO.java
Outdated
Show resolved
Hide resolved
.../services/accelerator/consent/mgt/extensions/manage/utils/ExternalAPIConsentManageUtils.java
Outdated
Show resolved
Hide resolved
...ial/services/accelerator/consent/mgt/extensions/manage/impl/DefaultConsentManageHandler.java
Outdated
Show resolved
Hide resolved
...2/financial/services/accelerator/consent/mgt/extensions/manage/utils/ConsentManageUtils.java
Outdated
Show resolved
Hide resolved
...ial/services/accelerator/consent/mgt/extensions/manage/impl/DefaultConsentManageHandler.java
Outdated
Show resolved
Hide resolved
...2/financial/services/accelerator/consent/mgt/extensions/manage/utils/ConsentManageUtils.java
Outdated
Show resolved
Hide resolved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Nitpick comments (2)
financial-services-accelerator/components/org.wso2.financial.services.accelerator.consent.mgt.extensions/src/test/java/org/wso2/financial/services/accelerator/consent/mgt/extensions/util/DataProviders.java (1)
48-106: Remove or add tests for unused payload constants.These constants are defined but never used elsewhere in the codebase:
INITIATION_WITHOUT_PERMISSIONS(line 48)INITIATION_WITH_NON_STRING_PERMISSION(line 52)INITIATION_WITHOUT_TRANS_FROM_DATES(line 89)INITIATION_WITHOUT_TRANS_TO_DATES(line 103)Either remove them if they're not needed, or add test cases if they represent important validation scenarios for permissions and date handling.
🤖 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/util/DataProviders.java` around lines 48 - 106, The four unused payload constants in DataProviders (INITIATION_WITHOUT_PERMISSIONS, INITIATION_WITH_NON_STRING_PERMISSION, INITIATION_WITHOUT_TRANS_FROM_DATES, INITIATION_WITHOUT_TRANS_TO_DATES) should be either removed or exercised by tests; update the DataProviders class accordingly by deleting the unused constants if they’re obsolete, or add unit tests that reference these constants (e.g., in the consent validation test suite) to cover missing-permissions, non-string-permission, missing TransactionFromDateTime, and missing TransactionToDateTime scenarios so the constants are actually used.financial-services-accelerator/components/org.wso2.financial.services.accelerator.consent.mgt.extensions/src/main/java/org/wso2/financial/services/accelerator/consent/mgt/extensions/common/ConsentExtensionUtils.java (1)
123-143: Extract the common initiation-response mutation into one helper.This overload is byte-for-byte identical to the
DetailedConsentResourcevariant, so future response changes can easily diverge between the two paths.🤖 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/common/ConsentExtensionUtils.java` around lines 123 - 143, The getInitiationResponse(Object responseObj, ConsentResource createdConsent) implementation duplicates the same JSON mutation performed by the DetailedConsentResource overload; extract the shared logic into a private helper (e.g., populateInitiationData or buildInitiationData) that accepts the JSONObject response (or its dataObject) and the ConsentResource, moves the put(...) calls for CONSENT_ID, CREATION_DATE_TIME, STATUS_UPDATE_DATE_TIME and STATUS, and the remove/put of DATA into that helper, then replace the body of getInitiationResponse and the DetailedConsentResource variant to call the helper so both reuse the single implementation.
🤖 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/main/java/org/wso2/financial/services/accelerator/consent/mgt/extensions/common/ExternalAPIUtil.java`:
- Around line 452-459: The buildConsentResource method currently autounboxes
nullable fields from ExternalAPIBasicConsentResourceResponseDTO (e.g.,
getFrequency(), getValidityTime(), getRecurringIndicator(), getStatus(), and
potentially getReceipt()), which can cause NPEs for partially populated external
responses; before constructing ConsentResource validate that these boxed values
are non-null (or apply safe defaults) and throw a clear IllegalArgumentException
or return a controlled validation error if any required fields are missing, and
also handle a null receipt from getReceipt() (e.g., treat as empty JSON or
validated error) so ConsentResource(...) is only called with fully validated,
non-null primitive arguments.
In
`@financial-services-accelerator/components/org.wso2.financial.services.accelerator.consent.mgt.extensions/src/main/java/org/wso2/financial/services/accelerator/consent/mgt/extensions/common/model/ExternalAPIBasicConsentResourceRequestDTO.java`:
- Around line 55-57: The JSON parsing of consentResource.getReceipt() in
ExternalAPIBasicConsentResourceRequestDTO is unguarded and can throw
JSONException, aborting updates; wrap the JSONObject construction and toMap()
call in a try-catch that catches org.json.JSONException (or Exception), log or
warn about the malformed receipt (including consent id/context) and gracefully
handle it by leaving this.receipt null or an empty map so the flow continues;
apply the same defensive try-catch and handling to the analogous parsing in
ExternalAPIConsentResourceRequestDTO (the block around lines 63–68).
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 523-529: The branch that checks
storedConsentResource.getClientID() vs consentManageData.getClientId() leaks
existence by logging NO_CONSENT_FOR_CLIENT_ERROR but throwing
CLIENT_ID_MISMATCH_ERROR; change the thrown ConsentException to use the generic
NO_CONSENT_FOR_CLIENT_ERROR (use
ConsentManageConstants.NO_CONSENT_FOR_CLIENT_ERROR) so both the log and the
thrown error are identical and non-revealing (adjust the ConsentException
constructor call that currently uses
ConsentManageConstants.CLIENT_ID_MISMATCH_ERROR, keeping
ResponseStatus.BAD_REQUEST and ConsentOperationEnum.CONSENT_UPDATE).
- Around line 517-520: The PUT/update error paths in DefaultConsentManageHandler
are mislabeling update failures as delete/create operations; locate where
storedConsentResource is checked and ConsentException is thrown (uses
ConsentException(ResponseStatus.BAD_REQUEST, "Consent not found",
ConsentOperationEnum.CONSENT_DELETE)) and change the operation enum to
ConsentOperationEnum.CONSENT_UPDATE, and similarly update other
throw/catch/helper usages in the same class that currently use CONSENT_CREATE or
CONSENT_DELETE for PUT handling (including the blocks around the
storedConsentResource checks and the exception wrapping logic) so they
consistently use CONSENT_UPDATE and update the error messages from “creating the
consent” to “updating the consent.”
- Around line 552-559: The new ConsentResource instance "updatingConsent" is
missing the stored consent ID so ConsentCoreServiceImpl.updateConsent rejects
it; before calling consentCoreService.updateConsent(updatingConsent) set the
consent ID from storedConsentResource (e.g., call
updatingConsent.setConsentId(storedConsentResource.getConsentId()) or
equivalent) so the update includes the original consent identifier; ensure this
is done in the non-pre-extension branch in DefaultConsentManageHandler where
updatingConsent is constructed from consentManageData and storedConsentResource.
- Around line 470-499: The PUT handling path in DefaultConsentManageHandler is
missing the header validation step (validateRequestHeaders), allowing invalid
requests to proceed; add a call to the same validateRequestHeaders(...) used by
POST/DELETE before any processing (i.e., before checking clientId and
requestPath and before extracting consentId), and ensure it uses the same
operation context (ConsentOperationEnum.CONSENT_UPDATE) and consentManageData
object so invalid headers are rejected early.
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 308-313: The isInternalConsentRequest method currently checks
headers.containsKey with ConsentManageConstants.INTERNAL_API_REQUEST_HEADER
which is case-sensitive; change it to perform a case-insensitive lookup by
iterating the headers map (from consentManageData.getHeaders()) and matching
keys with ConsentManageConstants.INTERNAL_API_REQUEST_HEADER using
equalsIgnoreCase, then parse and return Boolean.parseBoolean of the matched
value (handle nulls by returning false); update only isInternalConsentRequest to
use this key comparison so any header case (e.g., "X-WSO2-Internal-Request") is
recognized.
- Around line 476-480: In ConsentManageUtils update the assignment for the
authorization type so it uses the authorization type field instead of the
status: replace the call to updatedAuthResource.getAuthorizationStatus() passed
to authorization.setType(...) with the appropriate getter for the authorisation
type (e.g., updatedAuthResource.getAuthorizationType()), ensuring the variable
names authorization and updatedAuthResource remain unchanged.
- Around line 427-430: The code currently collects all mapping IDs from
storedResource.getConsentMappingResources() and deactivates them, which disables
mappings for sibling authorisations; update the logic to only collect mapping
IDs whose mapping resource is tied to the current authorization (compare the
mapping resource's authorization id to the authId in scope) before calling
consentCoreService.deactivateAccountMappings; specifically, filter
storedResource.getConsentMappingResources() by mappingResource.getAuthId() (or
equivalent getter) == authId and only add mappingResource.getMappingID() for
those matches.
- Around line 390-391: The catch block in ConsentManageUtils currently catches
ConsentManagementException and always rethrows a ConsentException with
ResponseStatus.BAD_REQUEST (see the catch handling that throws new
ConsentException(ResponseStatus.BAD_REQUEST, e.getMessage(),
ConsentOperationEnum.CONSENT_UPDATE)); change this so only request-validation
errors are mapped to BAD_REQUEST and service/DAO/backend failures are preserved
or translated to server errors: detect whether the caught
ConsentManagementException represents a client validation failure (by checking
its error code/message or a specific type/flag) and if so throw ConsentException
with ResponseStatus.BAD_REQUEST, otherwise either rethrow the original
ConsentManagementException or throw a ConsentException with
ResponseStatus.INTERNAL_SERVER_ERROR (or appropriate server status) so backend
failures are not collapsed into 400s.
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/ExternalAPIConsentManageUtils.java`:
- Around line 197-204: The
callExternalService(ExternalAPIPostConsentUpdateRequestDTO) method incorrectly
constructs the request payload via new JSONObject(requestDTO), bypassing
ExternalAPIPostConsentUpdateRequestDTO's custom serialization; update the method
to use requestDTO.toJson() when building requestJson so the nested
consentResource is constructed as intended (keep using
ServiceExtensionTypeEnum.ENRICH_CONSENT_UPDATE_RESPONSE and gson.fromJson on the
response as before).
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 166-270: These negative tests currently only assert inside the
catch for ConsentException, so if defaultConsentManageHandler.handlePost(...)
stops throwing the test will pass silently; update each test (e.g.,
testHandlePostWithoutClientId, testHandlePostWithInvalidHeaders,
testHandlePostWithoutRequestPath, testHandlePostWithInvalidRequestPath,
testHandlePostWithInvalidPayload, testHandlePostWithInvalidInitiation) to
explicitly fail when no ConsentException is thrown by calling Assert.fail (or
equivalent) immediately after invoking
defaultConsentManageHandler.handlePost(consentManageDataMock), keeping the
existing catch block assertions for e.getPayload() intact so the test fails when
no exception occurs and still validates the error when one is thrown.
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 296-313: Replace the prior use of getConsent() (which opens a
separate transaction) with loading the existing row via the DAO on the current
connection before the update (e.g., call
consentCoreDAO.getConsentResourceById(connection,
consentResource.getConsentID()) or the DAO method that returns the persisted
ConsentResource on that connection), fail cleanly if it returns null, then call
consentCoreDAO.updateConsentResource(connection, consentResource), and for the
audit use the persisted snapshot values (the persisted clientId and previous
status) in the consentDataMap and postStateChange call instead of
consentResource.getClientID() and previousConsent.getCurrentStatus(); finally
reload the updated consent from the same DAO/connection after commit (or before
returning) and return that reloaded updated consent.
---
Nitpick 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/common/ConsentExtensionUtils.java`:
- Around line 123-143: The getInitiationResponse(Object responseObj,
ConsentResource createdConsent) implementation duplicates the same JSON mutation
performed by the DetailedConsentResource overload; extract the shared logic into
a private helper (e.g., populateInitiationData or buildInitiationData) that
accepts the JSONObject response (or its dataObject) and the ConsentResource,
moves the put(...) calls for CONSENT_ID, CREATION_DATE_TIME,
STATUS_UPDATE_DATE_TIME and STATUS, and the remove/put of DATA into that helper,
then replace the body of getInitiationResponse and the DetailedConsentResource
variant to call the helper so both reuse the single implementation.
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/DataProviders.java`:
- Around line 48-106: The four unused payload constants in DataProviders
(INITIATION_WITHOUT_PERMISSIONS, INITIATION_WITH_NON_STRING_PERMISSION,
INITIATION_WITHOUT_TRANS_FROM_DATES, INITIATION_WITHOUT_TRANS_TO_DATES) should
be either removed or exercised by tests; update the DataProviders class
accordingly by deleting the unused constants if they’re obsolete, or add unit
tests that reference these constants (e.g., in the consent validation test
suite) to cover missing-permissions, non-string-permission, missing
TransactionFromDateTime, and missing TransactionToDateTime scenarios so the
constants are actually used.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 33dc55f0-6be9-404b-98d2-5f114ee2cc50
📒 Files selected for processing (20)
financial-services-accelerator/components/org.wso2.financial.services.accelerator.common/src/main/java/org/wso2/financial/services/accelerator/common/extension/model/ServiceExtensionTypeEnum.javafinancial-services-accelerator/components/org.wso2.financial.services.accelerator.consent.mgt.extensions/src/main/java/org/wso2/financial/services/accelerator/consent/mgt/extensions/common/ConsentExtensionUtils.javafinancial-services-accelerator/components/org.wso2.financial.services.accelerator.consent.mgt.extensions/src/main/java/org/wso2/financial/services/accelerator/consent/mgt/extensions/common/ExternalAPIUtil.javafinancial-services-accelerator/components/org.wso2.financial.services.accelerator.consent.mgt.extensions/src/main/java/org/wso2/financial/services/accelerator/consent/mgt/extensions/common/model/ExternalAPIBasicConsentResourceRequestDTO.javafinancial-services-accelerator/components/org.wso2.financial.services.accelerator.consent.mgt.extensions/src/main/java/org/wso2/financial/services/accelerator/consent/mgt/extensions/common/model/ExternalAPIBasicConsentResourceResponseDTO.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/model/ExternalAPIPostConsentUpdateRequestDTO.javafinancial-services-accelerator/components/org.wso2.financial.services.accelerator.consent.mgt.extensions/src/main/java/org/wso2/financial/services/accelerator/consent/mgt/extensions/manage/model/ExternalAPIPreConsentUpdateRequestDTO.javafinancial-services-accelerator/components/org.wso2.financial.services.accelerator.consent.mgt.extensions/src/main/java/org/wso2/financial/services/accelerator/consent/mgt/extensions/manage/model/ExternalAPIPreConsentUpdateResponseDTO.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/main/java/org/wso2/financial/services/accelerator/consent/mgt/extensions/manage/utils/ExternalAPIConsentManageUtils.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.extensions/src/test/java/org/wso2/financial/services/accelerator/consent/mgt/extensions/util/TestUtil.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/test/java/org/wso2/financial/services/accelerator/consent/mgt/service/impl/ConsentMgtCoreServiceTests.java
...a/org/wso2/financial/services/accelerator/consent/mgt/extensions/common/ExternalAPIUtil.java
Show resolved
Hide resolved
...celerator/consent/mgt/extensions/common/model/ExternalAPIBasicConsentResourceRequestDTO.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
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
Outdated
Show resolved
Hide resolved
...2/financial/services/accelerator/consent/mgt/extensions/manage/utils/ConsentManageUtils.java
Outdated
Show resolved
Hide resolved
.../services/accelerator/consent/mgt/extensions/manage/utils/ExternalAPIConsentManageUtils.java
Show resolved
Hide resolved
...cial/services/accelerator/consent/mgt/extensions/manage/DefaultConsentManageHandlerTest.java
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: 9
♻️ Duplicate 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/manage/impl/DefaultConsentManageHandler.java (1)
592-595:⚠️ Potential issue | 🟡 MinorInconsistent error message in log statement.
The log says "creating the consent" but this is the PUT handler for updating. The exception message correctly says "updating".
Suggested fix
} catch (ConsentManagementException e) { - log.error("Error Occurred while creating the consent", e); + log.error("Error Occurred while updating the consent", e); throw new ConsentException(ResponseStatus.INTERNAL_SERVER_ERROR, "Error Occurred while updating the consent", ConsentOperationEnum.CONSENT_UPDATE);🤖 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/DefaultConsentManageHandler.java` around lines 592 - 595, The catch block for ConsentManagementException in DefaultConsentManageHandler currently logs "Error Occurred while creating the consent" which is inconsistent with the update flow; change the log.error message in that catch (ConsentManagementException e) block to "Error Occurred while updating the consent" (matching the ConsentException message) so the log and thrown ConsentException both refer to the update operation.
🧹 Nitpick comments (3)
financial-services-accelerator/components/org.wso2.financial.services.accelerator.consent.mgt.extensions/src/main/java/org/wso2/financial/services/accelerator/consent/mgt/extensions/manage/model/ConsentManageAuthorizationUpdateDTO.java (1)
28-34: Inconsistent access modifier onmappingResourcesfield.All other fields use
privateaccess, butmappingResourceshas package-private (default) access. This should beprivatefor consistency and proper encapsulation.Suggested fix
private String authorizationType; private long updatedTime; - List<MappingResource> mappingResources; + private List<MappingResource> mappingResources;🤖 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/model/ConsentManageAuthorizationUpdateDTO.java` around lines 28 - 34, The field mappingResources in ConsentManageAuthorizationUpdateDTO currently has package-private access while all other fields are private; change its declaration to private List<MappingResource> mappingResources to match encapsulation, and ensure any existing getter/setter methods (e.g., getMappingResources/setMappingResources) reference the renamed private field if needed.financial-services-accelerator/components/org.wso2.financial.services.accelerator.consent.mgt.extensions/src/main/java/org/wso2/financial/services/accelerator/consent/mgt/extensions/common/ConsentExtensionUtils.java (1)
123-149: Code duplication with existinggetInitiationResponseoverload.This new method duplicates lines 108-121 except for the parameter type. Consider extracting the common logic into a private helper that accepts the individual fields (consentID, createdTime, updatedTime, currentStatus) to reduce duplication.
Example refactor
private static JSONObject buildInitiationResponse(Object responseObj, String consentId, long createdTime, long updatedTime, String status) { JSONObject response = (JSONObject) responseObj; JSONObject dataObject = response.getJSONObject(ConsentExtensionConstants.DATA); dataObject.put(ConsentExtensionConstants.CONSENT_ID, consentId); dataObject.put(ConsentExtensionConstants.CREATION_DATE_TIME, convertToISO8601(createdTime)); dataObject.put(ConsentExtensionConstants.STATUS_UPDATE_DATE_TIME, convertToISO8601(updatedTime)); dataObject.put(ConsentExtensionConstants.STATUS, status); response.remove(ConsentExtensionConstants.DATA); response.put(ConsentExtensionConstants.DATA, dataObject); return response; } public static JSONObject getInitiationResponse(Object responseObj, DetailedConsentResource createdConsent) { return buildInitiationResponse(responseObj, createdConsent.getConsentID(), createdConsent.getCreatedTime(), createdConsent.getUpdatedTime(), createdConsent.getCurrentStatus()); } public static JSONObject getInitiationResponse(Object responseObj, ConsentResource createdConsent) { JSONObject result = buildInitiationResponse(responseObj, createdConsent.getConsentID(), createdConsent.getCreatedTime(), createdConsent.getUpdatedTime(), createdConsent.getCurrentStatus()); if (log.isDebugEnabled()) { log.debug(String.format("Initiation response constructed for consent ID: %s with status: %s", createdConsent.getConsentID().replaceAll("[\r\n]", ""), createdConsent.getCurrentStatus().replaceAll("[\r\n]", ""))); } return result; }🤖 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/common/ConsentExtensionUtils.java` around lines 123 - 149, The two getInitiationResponse methods duplicate logic; extract the common JSON construction into a private helper (e.g., buildInitiationResponse) that takes (Object responseObj, String consentId, long createdTime, long updatedTime, String status) and performs the JSONObject dataObject population and replacement, then have both public methods call this helper (getInitiationResponse(Object, DetailedConsentResource) and getInitiationResponse(Object, ConsentResource) pass their respective createdConsent fields into buildInitiationResponse); keep the existing debug log only in the ConsentResource overload after calling the helper.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 (1)
508-516: Path check usingcontains()is fragile.The check
consentManageData.getRequestPath().contains("authorisations")matches any path containing that substring. Consider using a regex or structured path parsing for more precise routing (e.g., matching the pattern*/authorisations/*).🤖 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/DefaultConsentManageHandler.java` around lines 508 - 516, The request path check using consentManageData.getRequestPath().contains("authorisations") is too permissive; update the conditional in DefaultConsentManageHandler so it matches the intended route pattern (e.g., segment-based or regex like .*/authorisations/.*) instead of a substring match; locate the block that calls ConsentManageUtils.isInternalConsentRequest(...) and replace the contains(...) check with a precise match (regex or parsed path segment comparison) so only requests matching the pattern /authorisations/ (as a path segment) trigger ConsentManageUtils.updateConsentAuthResource and the subsequent response handling.
🤖 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/accelerators/fs-is/repository/resources/apis/accelerator-extensions-v1.0.4.yaml`:
- Line 11: The embedded OpenAPI document still lists version: v1.0.3 while the
file name indicates v1.0.4—update the OpenAPI info.version field by changing the
version key (the top-level "version:"/info.version entry in
accelerator-extensions-v1.0.4.yaml) to v1.0.4 so docs and generated clients
reflect the correct contract version.
- Around line 2673-2692: The schema FailedResponseApplicationProcess currently
lists a required "data" property that is not defined and defines errorMessage
via a separate object schema (FailedResponseApplicationProcessData) while
examples show a top-level string; remove the nonexistent "data" requirement from
FailedResponseApplicationProcess, change its errorMessage property to type:
string (or inline a string description) instead of $ref to
FailedResponseApplicationProcessData, and delete or simplify the
FailedResponseApplicationProcessData schema (or update references) so the
contract validates failure payloads where "status" is "ERROR" and "errorMessage"
is a plain string.
- Around line 1417-1429: The schema RequestForPreProcessConsentUpdate currently
defines the property storedConsentResource but the DTO
ExternalAPIPreConsentUpdateRequestDTO uses consentResource; rename the YAML
property storedConsentResource to consentResource (and update any $ref usages if
present) so the OpenAPI schema field matches the DTO name and enables correct
deserialization into ExternalAPIPreConsentUpdateRequestDTO.consentResource.
- Around line 2109-2111: Stored consent schemas StoredBasicConsentResourceData
and StoredDetailedConsentResourceData currently define the validityTime property
as type: integer with format: int32; update both definitions so validityTime
uses format: int64 to match ExternalAPIBasicConsentResourceResponseDTO (Long)
and the non-stored consent schemas, preventing downcasting in generated
clients—locate the validityTime field in StoredBasicConsentResourceData and
StoredDetailedConsentResourceData and change format from int32 to int64.
- Around line 2464-2469: The OpenAPI schema for basicConsentData is missing the
array type for additionalProperties: update the basicConsentData object so its
additionalProperties explicitly declares type: array and retains the existing
items: { type: string } so each map value is defined as an array of strings;
locate the basicConsentData definition and add type: array under
additionalProperties (keeping items: type: string) to ensure tools generate
Map<String, List<String>> correctly.
- Around line 836-840: The request body for the operation referencing
PopulateConsentAuthorizeScreenRequestBody is currently treated as optional; set
requestBody.required: true for that operation so generated clients must supply
the payload. Locate the operation that uses the schema reference
'#/components/schemas/PopulateConsentAuthorizeScreenRequestBody' and add a
top-level requestBody.required: true alongside the existing content block
(ensuring the schema name PopulateConsentAuthorizeScreenRequestBody remains
unchanged).
In
`@financial-services-accelerator/components/org.wso2.financial.services.accelerator.consent.mgt.dao/src/main/java/org/wso2/financial/services/accelerator/consent/mgt/dao/queries/ConsentMgtCommonDBQueries.java`:
- Around line 44-45: The LEFT JOIN change can produce rows with
ATT_KEY/ATT_VALUE = NULL and the mapper
ConsentManagementDAOUtil.setDataToConsentResourceWithAttributes currently
unconditionally does put(getString(ATT_KEY), getString(ATT_VALUE)), producing
{null=null}; update the mapper to guard against null/blank keys (and optionally
null values) by skipping any ResultSet row where ATT_KEY is null/blank before
calling put, so ConsentCoreDAOImpl.getConsentResourceWithAttributes can safely
use the LEFT JOIN SQL in ConsentMgtCommonDBQueries without creating malformed
attribute entries.
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 361-378: The sequence of non-atomic calls
(consentCoreService.updateAuthorizationResources,
consentCoreService.updateAuthorizationUser, deactivateAccountMappings,
createConsentMappings, then constructUpdateResponse) can leave data inconsistent
if an intermediate call fails; refactor by moving this whole sequence into a
single transactional service method on ConsentCoreService (e.g., add
updateAuthorizationWithMappings(authId, updatingAuthResource, mappingResources)
or similar) that performs updateAuthorizationResources and
updateAuthorizationUser, deactivates and creates mappings inside one DB
transaction and either commits or rolls back, or implement compensating rollback
logic inside that service method to undo prior changes on failure; update the
call site to invoke the new transactional method and return its result (use
AuthorizationResource and List<ConsentMappingResource> handling inside the new
service method).
- Around line 407-416: The extractAuthIdFromPath method can return extra path
segments (e.g., "{authId}/something"); update extractAuthIdFromPath to, after
splitting requestPath by "/authorisations/" and assigning pathParts[1], further
split that segment by "/" and take the first element as the authId (trim it),
then validate it as before; ensure you still log the same error message via
log.error and throw ConsentManagementException when authId is blank or missing.
Use the existing method name extractAuthIdFromPath and the local variable authId
to locate and apply this change.
---
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/impl/DefaultConsentManageHandler.java`:
- Around line 592-595: The catch block for ConsentManagementException in
DefaultConsentManageHandler currently logs "Error Occurred while creating the
consent" which is inconsistent with the update flow; change the log.error
message in that catch (ConsentManagementException e) block to "Error Occurred
while updating the consent" (matching the ConsentException message) so the log
and thrown ConsentException both refer to the update operation.
---
Nitpick 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/common/ConsentExtensionUtils.java`:
- Around line 123-149: The two getInitiationResponse methods duplicate logic;
extract the common JSON construction into a private helper (e.g.,
buildInitiationResponse) that takes (Object responseObj, String consentId, long
createdTime, long updatedTime, String status) and performs the JSONObject
dataObject population and replacement, then have both public methods call this
helper (getInitiationResponse(Object, DetailedConsentResource) and
getInitiationResponse(Object, ConsentResource) pass their respective
createdConsent fields into buildInitiationResponse); keep the existing debug log
only in the ConsentResource overload after calling the helper.
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 508-516: The request path check using
consentManageData.getRequestPath().contains("authorisations") is too permissive;
update the conditional in DefaultConsentManageHandler so it matches the intended
route pattern (e.g., segment-based or regex like .*/authorisations/.*) instead
of a substring match; locate the block that calls
ConsentManageUtils.isInternalConsentRequest(...) and replace the contains(...)
check with a precise match (regex or parsed path segment comparison) so only
requests matching the pattern /authorisations/ (as a path segment) trigger
ConsentManageUtils.updateConsentAuthResource and the subsequent response
handling.
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/model/ConsentManageAuthorizationUpdateDTO.java`:
- Around line 28-34: The field mappingResources in
ConsentManageAuthorizationUpdateDTO currently has package-private access while
all other fields are private; change its declaration to private
List<MappingResource> mappingResources to match encapsulation, and ensure any
existing getter/setter methods (e.g., getMappingResources/setMappingResources)
reference the renamed private field if needed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9a8a52dd-a29d-45df-8aff-06425356e8b6
📒 Files selected for processing (10)
financial-services-accelerator/accelerators/fs-is/repository/resources/apis/accelerator-extensions-v1.0.4.yamlfinancial-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.extensions/src/main/java/org/wso2/financial/services/accelerator/consent/mgt/extensions/common/ConsentExtensionUtils.javafinancial-services-accelerator/components/org.wso2.financial.services.accelerator.consent.mgt.extensions/src/main/java/org/wso2/financial/services/accelerator/consent/mgt/extensions/common/ExternalAPIUtil.javafinancial-services-accelerator/components/org.wso2.financial.services.accelerator.consent.mgt.extensions/src/main/java/org/wso2/financial/services/accelerator/consent/mgt/extensions/common/model/ExternalAPIBasicConsentResourceResponseDTO.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/model/ConsentManageAuthorizationUpdateDTO.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/main/java/org/wso2/financial/services/accelerator/consent/mgt/extensions/manage/utils/ExternalAPIConsentManageUtils.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.java
🚧 Files skipped from review as they are similar to previous changes (3)
- 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
- financial-services-accelerator/components/org.wso2.financial.services.accelerator.consent.mgt.extensions/src/main/java/org/wso2/financial/services/accelerator/consent/mgt/extensions/common/ExternalAPIUtil.java
- 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/ExternalAPIConsentManageUtils.java
| license: | ||
| name: Apache 2.0 | ||
| url: https://www.apache.org/licenses/LICENSE-2.0.html | ||
| version: v1.0.3 |
There was a problem hiding this comment.
Bump info.version to v1.0.4.
The file is published as accelerator-extensions-v1.0.4.yaml, but the embedded OpenAPI version still says v1.0.3. That will surface the wrong contract version in docs and generated clients.
Suggested fix
- version: v1.0.3
+ version: v1.0.4📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| version: v1.0.3 | |
| version: v1.0.4 |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@financial-services-accelerator/accelerators/fs-is/repository/resources/apis/accelerator-extensions-v1.0.4.yaml`
at line 11, The embedded OpenAPI document still lists version: v1.0.3 while the
file name indicates v1.0.4—update the OpenAPI info.version field by changing the
version key (the top-level "version:"/info.version entry in
accelerator-extensions-v1.0.4.yaml) to v1.0.4 so docs and generated clients
reflect the correct contract version.
Comment on lines
+836
to
+840
| requestBody: | ||
| content: | ||
| application/json: | ||
| schema: | ||
| $ref: '#/components/schemas/PopulateConsentAuthorizeScreenRequestBody' |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In OpenAPI 3.0.x, if an operation omits requestBody.required: true, is the request body treated as optional by default for validators and generated clients?
💡 Result:
Yes. In OpenAPI 3.0.x, requestBody.required defaults to false, so if you omit requestBody.required: true, the request body is optional by default and spec-compliant validators and generated clients should treat it as optional. [1][2]
Sources: OpenAPI 3.0.1 spec (Request Body Object: “Defaults to false”) [1]; Swagger/OpenAPI 3.0 requestBody docs (“optional … false by default”) [2].
Make this request body mandatory.
In OpenAPI 3.0.x, requestBody.required defaults to false when omitted. The PopulateConsentAuthorizeScreenRequestBody schema requires requestId and data, but this operation publishes the request body as optional, allowing generated clients to call it without a payload.
Suggested fix
/populate-consent-authorize-screen:
post:
tags:
- Consent
summary: handle validations before consent authorization and consent data to load in consent authorization UI
requestBody:
content:
application/json:
schema:
$ref: '#/components/schemas/PopulateConsentAuthorizeScreenRequestBody'
+ required: true
responses:📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| requestBody: | |
| content: | |
| application/json: | |
| schema: | |
| $ref: '#/components/schemas/PopulateConsentAuthorizeScreenRequestBody' | |
| requestBody: | |
| content: | |
| application/json: | |
| schema: | |
| $ref: '#/components/schemas/PopulateConsentAuthorizeScreenRequestBody' | |
| required: true |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@financial-services-accelerator/accelerators/fs-is/repository/resources/apis/accelerator-extensions-v1.0.4.yaml`
around lines 836 - 840, The request body for the operation referencing
PopulateConsentAuthorizeScreenRequestBody is currently treated as optional; set
requestBody.required: true for that operation so generated clients must supply
the payload. Locate the operation that uses the schema reference
'#/components/schemas/PopulateConsentAuthorizeScreenRequestBody' and add a
top-level requestBody.required: true alongside the existing content block
(ensuring the schema name PopulateConsentAuthorizeScreenRequestBody remains
unchanged).
Comment on lines
+1417
to
+1429
| RequestForPreProcessConsentUpdate: | ||
| type: object | ||
| properties: | ||
| consentUpdateData: | ||
| type: object | ||
| description: The upload payload used by third parties which includes detailed information on data access request. | ||
| storedConsentResource: | ||
| $ref: '#/components/schemas/StoredBasicConsentResourceData' | ||
| requestHeaders: | ||
| $ref: '#/components/schemas/RequestHeaders' | ||
| consentResourcePath: | ||
| type: string | ||
| description: To identify requested consent type |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
spec=$(fd 'accelerator-extensions-v1\.0\.4\.yaml$')
dto=$(fd 'ExternalAPIPreConsentUpdateRequestDTO\.java$')
sed -n '1417,1430p' "$spec"
echo '---'
sed -n '28,76p' "$dto"Repository: wso2/financial-services-accelerator
Length of output: 2349
Rename storedConsentResource to consentResource to match the DTO field name.
The schema defines storedConsentResource, but ExternalAPIPreConsentUpdateRequestDTO.java exposes this field as consentResource. When the framework deserializes incoming JSON to the DTO, the mismatch will cause the payload to be dropped or left null, breaking the consent update flow.
Suggested fix
RequestForPreProcessConsentUpdate:
type: object
properties:
consentUpdateData:
type: object
description: The upload payload used by third parties which includes detailed information on data access request.
- storedConsentResource:
+ consentResource:
$ref: '#/components/schemas/StoredBasicConsentResourceData'
requestHeaders:
$ref: '#/components/schemas/RequestHeaders'
consentResourcePath:
type: string📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| RequestForPreProcessConsentUpdate: | |
| type: object | |
| properties: | |
| consentUpdateData: | |
| type: object | |
| description: The upload payload used by third parties which includes detailed information on data access request. | |
| storedConsentResource: | |
| $ref: '#/components/schemas/StoredBasicConsentResourceData' | |
| requestHeaders: | |
| $ref: '#/components/schemas/RequestHeaders' | |
| consentResourcePath: | |
| type: string | |
| description: To identify requested consent type | |
| RequestForPreProcessConsentUpdate: | |
| type: object | |
| properties: | |
| consentUpdateData: | |
| type: object | |
| description: The upload payload used by third parties which includes detailed information on data access request. | |
| consentResource: | |
| $ref: '#/components/schemas/StoredBasicConsentResourceData' | |
| requestHeaders: | |
| $ref: '#/components/schemas/RequestHeaders' | |
| consentResourcePath: | |
| type: string | |
| description: To identify requested consent type |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@financial-services-accelerator/accelerators/fs-is/repository/resources/apis/accelerator-extensions-v1.0.4.yaml`
around lines 1417 - 1429, The schema RequestForPreProcessConsentUpdate currently
defines the property storedConsentResource but the DTO
ExternalAPIPreConsentUpdateRequestDTO uses consentResource; rename the YAML
property storedConsentResource to consentResource (and update any $ref usages if
present) so the OpenAPI schema field matches the DTO name and enables correct
deserialization into ExternalAPIPreConsentUpdateRequestDTO.consentResource.
Comment on lines
+2109
to
+2111
| validityTime: | ||
| type: integer | ||
| format: int32 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
spec=$(fd 'accelerator-extensions-v1\.0\.4\.yaml$')
dto=$(fd 'ExternalAPIBasicConsentResourceResponseDTO\.java$')
sed -n '2088,2140p' "$spec"
echo '---'
sed -n '26,90p' "$dto"Repository: wso2/financial-services-accelerator
Length of output: 2728
🏁 Script executed:
#!/bin/bash
set -euo pipefail
spec=$(fd 'accelerator-extensions-v1\.0\.4\.yaml$')
# Show lines 2109-2111 to verify the first location
echo "=== Lines 2109-2111 (first location) ==="
sed -n '2109,2111p' "$spec"
# Show lines 2137-2139 to verify the second location
echo "=== Lines 2137-2139 (second location) ==="
sed -n '2137,2139p' "$spec"
# Search for non-stored consent schemas (BasicConsentResourceData and DetailedConsentResourceData)
echo "=== Searching for BasicConsentResourceData ==="
grep -n "BasicConsentResourceData:" "$spec" | head -5
echo "=== Searching for DetailedConsentResourceData ==="
grep -n "DetailedConsentResourceData:" "$spec" | head -5Repository: wso2/financial-services-accelerator
Length of output: 556
🏁 Script executed:
#!/bin/bash
set -euo pipefail
spec=$(fd 'accelerator-extensions-v1\.0\.4\.yaml$')
# Show BasicConsentResourceData including validityTime field
echo "=== BasicConsentResourceData (lines 2022-2087) ==="
sed -n '2022,2087p' "$spec" | grep -A 2 'validityTime'
# Show DetailedConsentResourceData including validityTime field
echo "=== DetailedConsentResourceData (lines 2040-2115) ==="
sed -n '2040,2115p' "$spec" | grep -A 2 'validityTime'Repository: wso2/financial-services-accelerator
Length of output: 615
Use int64 for stored consent validityTime.
ExternalAPIBasicConsentResourceResponseDTO.validityTime is Long, and the non-stored consent schemas here already use format: int64. Keeping the stored variants at int32 will downcast the field in generated clients for the update flow.
Suggested fix
validityTime:
type: integer
- format: int32
+ format: int64Apply that change to both StoredBasicConsentResourceData (lines 2109-2111) and StoredDetailedConsentResourceData (lines 2137-2139).
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| validityTime: | |
| type: integer | |
| format: int32 | |
| validityTime: | |
| type: integer | |
| format: int64 |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@financial-services-accelerator/accelerators/fs-is/repository/resources/apis/accelerator-extensions-v1.0.4.yaml`
around lines 2109 - 2111, Stored consent schemas StoredBasicConsentResourceData
and StoredDetailedConsentResourceData currently define the validityTime property
as type: integer with format: int32; update both definitions so validityTime
uses format: int64 to match ExternalAPIBasicConsentResourceResponseDTO (Long)
and the non-stored consent schemas, preventing downcasting in generated
clients—locate the validityTime field in StoredBasicConsentResourceData and
StoredDetailedConsentResourceData and change format from int32 to int64.
Comment on lines
+2464
to
+2469
| basicConsentData: | ||
| type: object | ||
| description: Structured descriptive text shown on the consent page, split into sections. Each key is a section title, and its value is a list of bullet points displayed under that section. | ||
| additionalProperties: | ||
| items: | ||
| type: string |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In OpenAPI 3.0.x, how do you define an object whose arbitrary property values are arrays of strings? Is additionalPropertieswith onlyitemsvalid withouttype: array?
💡 Result:
To model “an object with arbitrary keys whose values are arrays of strings” in OpenAPI 3.0.x, define the object as type: object and put an array schema under additionalProperties:
schema:
type: object
additionalProperties:
type: array
items:
type: stringadditionalProperties can be a Schema Object, so using an array schema there is the intended approach. [1]
If you write only:
additionalProperties:
items:
type: string…it’s not the right way to say “the values are arrays”. In OAS 3.0.x, items is the array-item schema and is tied to type: array (the spec calls out that items MUST be present if type is array, and tooling commonly flags items without type: array as a type mismatch). [1][2][3]
Add type: array to additionalProperties in basicConsentData.
The schema describes each section key mapping to a list of bullet points, but the OpenAPI definition is incomplete. In OpenAPI 3.0.x, additionalProperties must include type: array to declare that property values are arrays; items alone without type: array is not a valid way to specify this. Code generators will not correctly model this as Map<String, List<String>> in its current form.
Suggested fix
basicConsentData:
type: object
description: Structured descriptive text shown on the consent page, split into sections. Each key is a section title, and its value is a list of bullet points displayed under that section.
additionalProperties:
+ type: array
items:
type: string📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| basicConsentData: | |
| type: object | |
| description: Structured descriptive text shown on the consent page, split into sections. Each key is a section title, and its value is a list of bullet points displayed under that section. | |
| additionalProperties: | |
| items: | |
| type: string | |
| basicConsentData: | |
| type: object | |
| description: Structured descriptive text shown on the consent page, split into sections. Each key is a section title, and its value is a list of bullet points displayed under that section. | |
| additionalProperties: | |
| type: array | |
| items: | |
| type: string |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@financial-services-accelerator/accelerators/fs-is/repository/resources/apis/accelerator-extensions-v1.0.4.yaml`
around lines 2464 - 2469, The OpenAPI schema for basicConsentData is missing the
array type for additionalProperties: update the basicConsentData object so its
additionalProperties explicitly declares type: array and retains the existing
items: { type: string } so each map value is defined as an array of strings;
locate the basicConsentData definition and add type: array under
additionalProperties (keeping items: type: string) to ensure tools generate
Map<String, List<String>> correctly.
Comment on lines
+2673
to
+2692
| required: | ||
| - data | ||
| - status | ||
| type: object | ||
| properties: | ||
| status: | ||
| type: string | ||
| description: "Indicates the outcome of the request. For a failed operation, this should be set to ERROR." | ||
| enum: | ||
| - ERROR | ||
| errorMessage: | ||
| $ref: '#/components/schemas/FailedResponseApplicationProcessData' | ||
| FailedResponseApplicationProcessData: | ||
| required: | ||
| - errorMessage | ||
| type: object | ||
| properties: | ||
| errorMessage: | ||
| type: string | ||
| description: Error message to be returned |
There was a problem hiding this comment.
Align FailedResponseApplicationProcess with its own examples.
This schema requires a data property that it never defines, and it types errorMessage as an object even though the application examples return a top-level string. As written, the contract cannot validate the documented failure payloads.
Suggested fix
FailedResponseApplicationProcess:
required:
- - data
+ - errorMessage
- status
type: object
properties:
status:
type: string
description: "Indicates the outcome of the request. For a failed operation, this should be set to ERROR."
enum:
- ERROR
errorMessage:
- $ref: '#/components/schemas/FailedResponseApplicationProcessData'
+ type: string
+ description: Error message to be returned📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| required: | |
| - data | |
| - status | |
| type: object | |
| properties: | |
| status: | |
| type: string | |
| description: "Indicates the outcome of the request. For a failed operation, this should be set to ERROR." | |
| enum: | |
| - ERROR | |
| errorMessage: | |
| $ref: '#/components/schemas/FailedResponseApplicationProcessData' | |
| FailedResponseApplicationProcessData: | |
| required: | |
| - errorMessage | |
| type: object | |
| properties: | |
| errorMessage: | |
| type: string | |
| description: Error message to be returned | |
| FailedResponseApplicationProcess: | |
| required: | |
| - errorMessage | |
| - status | |
| type: object | |
| properties: | |
| status: | |
| type: string | |
| description: "Indicates the outcome of the request. For a failed operation, this should be set to ERROR." | |
| enum: | |
| - ERROR | |
| errorMessage: | |
| type: string | |
| description: Error message to be returned | |
| FailedResponseApplicationProcessData: | |
| required: | |
| - errorMessage | |
| type: object | |
| properties: | |
| errorMessage: | |
| type: string | |
| description: Error message to be returned |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@financial-services-accelerator/accelerators/fs-is/repository/resources/apis/accelerator-extensions-v1.0.4.yaml`
around lines 2673 - 2692, The schema FailedResponseApplicationProcess currently
lists a required "data" property that is not defined and defines errorMessage
via a separate object schema (FailedResponseApplicationProcessData) while
examples show a top-level string; remove the nonexistent "data" requirement from
FailedResponseApplicationProcess, change its errorMessage property to type:
string (or inline a string description) instead of $ref to
FailedResponseApplicationProcessData, and delete or simplify the
FailedResponseApplicationProcessData schema (or update references) so the
contract validates failure payloads where "status" is "ERROR" and "errorMessage"
is a plain string.
Comment on lines
+44
to
45
| "FS_CONSENT_ATTRIBUTE.ATT_KEY, FS_CONSENT_ATTRIBUTE.ATT_VALUE FROM FS_CONSENT LEFT JOIN " + | ||
| "FS_CONSENT_ATTRIBUTE ON FS_CONSENT.CONSENT_ID = FS_CONSENT_ATTRIBUTE.CONSENT_ID WHERE FS_CONSENT" + |
There was a problem hiding this comment.
Handle null attribute rows before switching this query to LEFT JOIN.
With this join, a consent that has no rows in FS_CONSENT_ATTRIBUTE now comes back as a single row with ATT_KEY/ATT_VALUE = NULL. ConsentCoreDAOImpl.getConsentResourceWithAttributes(...) passes that ResultSet straight into ConsentManagementDAOUtil.setDataToConsentResourceWithAttributes(...), and that mapper currently does an unconditional put(getString(ATT_KEY), getString(ATT_VALUE)). The result is a malformed attributes map ({null=null}) instead of an empty one for attribute-less consents. Please either guard null/blank keys in the mapper or keep the old join semantics until that mapper is fixed.
🤖 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/queries/ConsentMgtCommonDBQueries.java`
around lines 44 - 45, The LEFT JOIN change can produce rows with
ATT_KEY/ATT_VALUE = NULL and the mapper
ConsentManagementDAOUtil.setDataToConsentResourceWithAttributes currently
unconditionally does put(getString(ATT_KEY), getString(ATT_VALUE)), producing
{null=null}; update the mapper to guard against null/blank keys (and optionally
null values) by skipping any ResultSet row where ATT_KEY is null/blank before
calling put, so ConsentCoreDAOImpl.getConsentResourceWithAttributes can safely
use the LEFT JOIN SQL in ConsentMgtCommonDBQueries without creating malformed
attribute entries.
Comment on lines
+361
to
+378
| if (matchingAuthResource.isPresent()) { | ||
| // Update the authorization resource if authId is found | ||
| consentCoreService.updateAuthorizationResources(Collections.singletonList(updatingAuthResource)); | ||
| consentCoreService.updateAuthorizationUser(authId, updatingAuthResource.getUserID()); | ||
| if (log.isDebugEnabled()) { | ||
| log.debug(String.format("Authorization resource updated successfully for auth id: %s", | ||
| authId.replaceAll("[\r\n]", ""))); | ||
| } | ||
| AuthorizationResource updatedAuthResource = consentCoreService.getAuthorizationResource(authId); | ||
| List<ConsentMappingResource> createdMappingResources = null; | ||
| if (CollectionUtils.isNotEmpty(authDetails.getMappingResources())) { | ||
| // Deactivate the current account mappings of the authorization resource if available | ||
| deactivateAccountMappings(detailedConsentResource, consentCoreService, authId); | ||
| // Create new account mappings based on the request payload | ||
| createdMappingResources = createConsentMappings(authDetails.getMappingResources(), | ||
| consentCoreService, authId); | ||
| } | ||
| return constructUpdateResponse(updatedAuthResource, createdMappingResources); |
There was a problem hiding this comment.
Non-atomic update sequence could leave data in an inconsistent state.
The authorization update involves multiple sequential service calls:
updateAuthorizationResources(line 363)updateAuthorizationUser(line 364)deactivateAccountMappings(line 373)createConsentMappings(lines 375-376)
If any intermediate call fails (e.g., deactivateAccountMappings fails after auth resource is updated), the consent data could be left partially updated with no rollback.
Consider wrapping these operations in a single transactional method in the service layer, or implementing compensating actions on failure.
🤖 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 361 - 378, The sequence of non-atomic calls
(consentCoreService.updateAuthorizationResources,
consentCoreService.updateAuthorizationUser, deactivateAccountMappings,
createConsentMappings, then constructUpdateResponse) can leave data inconsistent
if an intermediate call fails; refactor by moving this whole sequence into a
single transactional service method on ConsentCoreService (e.g., add
updateAuthorizationWithMappings(authId, updatingAuthResource, mappingResources)
or similar) that performs updateAuthorizationResources and
updateAuthorizationUser, deactivates and creates mappings inside one DB
transaction and either commits or rolls back, or implement compensating rollback
logic inside that service method to undo prior changes on failure; update the
call site to invoke the new transactional method and return its result (use
AuthorizationResource and List<ConsentMappingResource> handling inside the new
service method).
Comment on lines
+407
to
+416
| private static String extractAuthIdFromPath(String requestPath) throws ConsentManagementException { | ||
|
|
||
| String[] pathParts = requestPath.split("/authorisations/"); | ||
| String authId = pathParts.length > 1 ? pathParts[1] : null; | ||
| if (StringUtils.isBlank(authId)) { | ||
| log.error("Auth id is missing in the request path."); | ||
| throw new ConsentManagementException("Invalid request path: Auth id is missing."); | ||
| } | ||
| return authId; | ||
| } |
There was a problem hiding this comment.
extractAuthIdFromPath may include trailing path segments.
If the request path has segments after the auth ID (e.g., /consent/{id}/authorisations/{authId}/something), splitting by /authorisations/ and taking pathParts[1] will return {authId}/something instead of just {authId}.
Suggested fix to extract only the auth ID
private static String extractAuthIdFromPath(String requestPath) throws ConsentManagementException {
String[] pathParts = requestPath.split("/authorisations/");
- String authId = pathParts.length > 1 ? pathParts[1] : null;
+ String authId = pathParts.length > 1 ? pathParts[1].split("/")[0] : null;
if (StringUtils.isBlank(authId)) {
log.error("Auth id is missing in the request path.");
throw new ConsentManagementException("Invalid request path: Auth id is missing.");
}
return authId;
}🤖 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 407 - 416, The extractAuthIdFromPath method can return extra path
segments (e.g., "{authId}/something"); update extractAuthIdFromPath to, after
splitting requestPath by "/authorisations/" and assigning pathParts[1], further
split that segment by "/" and take the first element as the authId (trim it),
then validate it as before; ensure you still log the same error message via
log.error and throw ConsentManagementException when authId is blank or missing.
Use the existing method name extractAuthIdFromPath and the local variable authId
to locate and apply this change.
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
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 consent manage PUT to update basic consent and authorisation resource with ID
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
Bug Fixes / Improvements
Tests