[OB4] Adding consent manage PUT to update basic consent#914
Closed
[OB4] Adding consent manage PUT to update basic consent#914
Conversation
WalkthroughThis pull request introduces external API consent update extensibility to a financial services accelerator. It adds new OpenAPI contract definitions, deployment configurations, enum constants, data transfer objects, utility methods, core service updates, and handler logic to support pre-processing and post-processing consent updates via external services, along with comprehensive test coverage. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![wso2-engineering[bot]](https://avatars.githubusercontent.com/in/1856828?s=60&v=4){kind=small}
Comment on lines
+130
to
+132
| public static JSONObject getInitiationResponse(Object responseObj, ConsentResource createdConsent) { | ||
|
|
||
| JSONObject response = (JSONObject) responseObj; |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 1
Suggested change
| public static JSONObject getInitiationResponse(Object responseObj, ConsentResource createdConsent) { | |
| JSONObject response = (JSONObject) responseObj; | |
| public static JSONObject getInitiationResponse(Object responseObj, ConsentResource createdConsent) { | |
| log.info("Constructing initiation response for consent ID: " + createdConsent.getConsentID()); | |
| JSONObject response = (JSONObject) responseObj; |
Comment on lines
+452
to
+454
| public static ConsentResource buildConsentResource(ExternalAPIBasicConsentResourceResponseDTO basicConsentResource, | ||
| String consentID, String clientID, long createTime) { | ||
|
|
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 2
Suggested change
| public static ConsentResource buildConsentResource(ExternalAPIBasicConsentResourceResponseDTO basicConsentResource, | |
| String consentID, String clientID, long createTime) { | |
| public static ConsentResource buildConsentResource(ExternalAPIBasicConsentResourceResponseDTO basicConsentResource, | |
| String consentID, String clientID, long createTime) { | |
| log.debug("Building consent resource for consentID: {}, clientID: {}", consentID, clientID); |
Comment on lines
+459
to
+460
| basicConsentResource.getRecurringIndicator(), basicConsentResource.getStatus(), | ||
| createTime, 0); |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 3
Suggested change
| basicConsentResource.getRecurringIndicator(), basicConsentResource.getStatus(), | |
| createTime, 0); | |
| basicConsentResource.getRecurringIndicator(), basicConsentResource.getStatus(), | |
| createTime, 0); | |
| log.info("Successfully built consent resource for consentID: {}", consentID); | |
| return new ConsentResource(consentID, clientID, receipt, basicConsentResource.getType(), |
Comment on lines
+42
to
+45
|
|
||
| public ExternalAPIBasicConsentResourceRequestDTO(ConsentResource consentResource) { | ||
|
|
||
| this.id = consentResource.getConsentID(); |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 4
Suggested change
| public ExternalAPIBasicConsentResourceRequestDTO(ConsentResource consentResource) { | |
| this.id = consentResource.getConsentID(); | |
| public ExternalAPIBasicConsentResourceRequestDTO(ConsentResource consentResource) { | |
| log.debug("Creating ExternalAPIBasicConsentResourceRequestDTO for consent ID: " + consentResource.getConsentID()); | |
| this.id = consentResource.getConsentID(); |
Comment on lines
+54
to
+59
|
|
||
| if (consentResource.getReceipt() != null && !consentResource.getReceipt().isEmpty()) { | ||
| JSONObject receiptJson = new JSONObject(consentResource.getReceipt()); | ||
| this.receipt = receiptJson.toMap(); | ||
| } else { | ||
| this.receipt = Collections.emptyMap(); |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 5
Suggested change
| if (consentResource.getReceipt() != null && !consentResource.getReceipt().isEmpty()) { | |
| JSONObject receiptJson = new JSONObject(consentResource.getReceipt()); | |
| this.receipt = receiptJson.toMap(); | |
| } else { | |
| this.receipt = Collections.emptyMap(); | |
| if (consentResource.getReceipt() != null && !consentResource.getReceipt().isEmpty()) { | |
| JSONObject receiptJson = new JSONObject(consentResource.getReceipt()); | |
| this.receipt = receiptJson.toMap(); | |
| log.debug("Successfully parsed consent receipt for consent ID: " + this.id); | |
| } else { | |
| this.receipt = Collections.emptyMap(); | |
| log.debug("No receipt found for consent ID: " + this.id); | |
| } |
Comment on lines
162
to
164
| try { | ||
| ConsentResource consent = consentCoreService.getConsent(consentId, false); | ||
| if (consent == null) { |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 6
Suggested change
| try { | |
| ConsentResource consent = consentCoreService.getConsent(consentId, false); | |
| if (consent == null) { | |
| try { | |
| ConsentResource consent = consentCoreService.getConsent(consentId, false); | |
| log.info("Retrieved consent for ID: {}", consentId.replaceAll("[\r\n]+", " ")); | |
| if (consent == null) { |
Comment on lines
+529
to
+531
|
|
||
| updatedConsent = updateConsent(consentManageData, consentId, storedConsentResource, consentType); | ||
| } |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 7
Suggested change
| updatedConsent = updateConsent(consentManageData, consentId, storedConsentResource, consentType); | |
| } | |
| updatedConsent = updateConsent(consentManageData, consentId, storedConsentResource, consentType); | |
| } | |
| log.info("Successfully updated consent with ID: {}", consentId.replaceAll("[\r\n]+", " ")); | |
| if (isExtensionsEnabled && isExternalPostConsentUpdateEnabled) { |
Comment on lines
+33
to
+40
|
|
||
| public ExternalAPIPostConsentUpdateRequestDTO(ExternalAPIBasicConsentResourceRequestDTO consentResource, | ||
| String resourcePath) { | ||
|
|
||
| this.consentId = consentResource.getId(); | ||
| this.consentResource = consentResource; | ||
| this.consentResourcePath = resourcePath; | ||
|
|
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 8
Suggested change
| public ExternalAPIPostConsentUpdateRequestDTO(ExternalAPIBasicConsentResourceRequestDTO consentResource, | |
| String resourcePath) { | |
| this.consentId = consentResource.getId(); | |
| this.consentResource = consentResource; | |
| this.consentResourcePath = resourcePath; | |
| public ExternalAPIPostConsentUpdateRequestDTO(ExternalAPIBasicConsentResourceRequestDTO consentResource, | |
| String resourcePath) { | |
| log.debug("Creating ExternalAPIPostConsentUpdateRequestDTO with consentId: {} and resourcePath: {}", | |
| consentResource != null ? consentResource.getId() : null, resourcePath); | |
| this.consentId = consentResource.getId(); | |
| this.consentResource = consentResource; | |
| this.consentResourcePath = resourcePath; | |
| } |
Comment on lines
+71
to
+77
| */ | ||
| public JSONObject toJson() { | ||
|
|
||
| JSONObject dtoJson = new JSONObject(this); | ||
| JSONObject consentResourceJson = new JSONObject(consentResource); | ||
| dtoJson.put("consentResource", consentResourceJson); | ||
| return dtoJson; |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 9
Suggested change
| */ | |
| public JSONObject toJson() { | |
| JSONObject dtoJson = new JSONObject(this); | |
| JSONObject consentResourceJson = new JSONObject(consentResource); | |
| dtoJson.put("consentResource", consentResourceJson); | |
| return dtoJson; | |
| public JSONObject toJson() { | |
| if (log.isDebugEnabled()) { | |
| log.debug("Converting ExternalAPIPostConsentUpdateRequestDTO to JSON for consentId: {}", consentId); | |
| } | |
| JSONObject dtoJson = new JSONObject(this); | |
| JSONObject consentResourceJson = new JSONObject(consentResource); | |
| dtoJson.put("consentResource", consentResourceJson); | |
| return dtoJson; | |
| } |
Comment on lines
+35
to
+38
|
|
||
| public ExternalAPIPreConsentUpdateRequestDTO(ConsentManageData consentManageData, | ||
| ExternalAPIBasicConsentResourceRequestDTO storedConsentResource) { | ||
|
|
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 10
Suggested change
| public ExternalAPIPreConsentUpdateRequestDTO(ConsentManageData consentManageData, | |
| ExternalAPIBasicConsentResourceRequestDTO storedConsentResource) { | |
| public ExternalAPIPreConsentUpdateRequestDTO(ConsentManageData consentManageData, | |
| ExternalAPIBasicConsentResourceRequestDTO storedConsentResource) { | |
| if (log.isDebugEnabled()) { | |
| log.debug("Creating ExternalAPIPreConsentUpdateRequestDTO with consent resource path: " + | |
| consentManageData.getRequestPath()); | |
| } |
| * @param payload request payload | ||
| * @return status value if present in the request, null otherwise | ||
| */ | ||
| public static String getStatus(Object payload) { |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 11
Suggested change
| public static String getStatus(Object payload) { | |
| public static String getStatus(Object payload) { | |
| log.debug("Extracting status from consent request payload"); |
Comment on lines
+298
to
+300
| if (data.has(ConsentExtensionConstants.STATUS)) { | ||
| return data.getString(ConsentExtensionConstants.STATUS); | ||
| } |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 12
Suggested change
| if (data.has(ConsentExtensionConstants.STATUS)) { | |
| return data.getString(ConsentExtensionConstants.STATUS); | |
| } | |
| if (data.has(ConsentExtensionConstants.STATUS)) { | |
| String status = data.getString(ConsentExtensionConstants.STATUS); | |
| log.debug("Status value found in request payload"); | |
| return status; | |
| } | |
| log.debug("Status value not present in request payload"); |
Comment on lines
+181
to
+185
| public static ExternalAPIPreConsentUpdateResponseDTO callExternalService( | ||
| ExternalAPIPreConsentUpdateRequestDTO requestDTO) throws FinancialServicesException { | ||
|
|
||
| log.debug("Calling external service for pre consent update"); | ||
| JSONObject requestJson = new JSONObject(requestDTO); |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 13
Suggested change
| public static ExternalAPIPreConsentUpdateResponseDTO callExternalService( | |
| ExternalAPIPreConsentUpdateRequestDTO requestDTO) throws FinancialServicesException { | |
| log.debug("Calling external service for pre consent update"); | |
| JSONObject requestJson = new JSONObject(requestDTO); | |
| public static ExternalAPIPreConsentUpdateResponseDTO callExternalService( | |
| ExternalAPIPreConsentUpdateRequestDTO requestDTO) throws FinancialServicesException { | |
| log.debug("Calling external service for pre consent update"); | |
| log.info("Initiating pre consent update external service call"); |
Comment on lines
+197
to
+201
| public static ExternalAPIModifiedResponseDTO callExternalService( | ||
| ExternalAPIPostConsentUpdateRequestDTO requestDTO) throws FinancialServicesException { | ||
|
|
||
| log.debug("Calling external service for post consent update"); | ||
| JSONObject requestJson = requestDTO.toJson(); |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 14
Suggested change
| public static ExternalAPIModifiedResponseDTO callExternalService( | |
| ExternalAPIPostConsentUpdateRequestDTO requestDTO) throws FinancialServicesException { | |
| log.debug("Calling external service for post consent update"); | |
| JSONObject requestJson = requestDTO.toJson(); | |
| public static ExternalAPIModifiedResponseDTO callExternalService( | |
| ExternalAPIPostConsentUpdateRequestDTO requestDTO) throws FinancialServicesException { | |
| log.debug("Calling external service for post consent update"); | |
| log.info("Initiating post consent update external service call"); |
Comment on lines
161
to
+171
|
|
||
| setConsentManageBuilder(); | ||
| JSONObject payload = new JSONObject(TestConstants.VALID_INITIATION); | ||
| doReturn(payload).when(consentManageDataMock).getPayload(); | ||
|
|
||
| defaultConsentManageHandler.handlePost(consentManageDataMock); | ||
| try { | ||
| setConsentManageBuilder(); | ||
| ConsentManageData consentManageDataMock = mock(ConsentManageData.class); | ||
| doReturn(null).when(consentManageDataMock).getClientId(); | ||
|
|
||
| defaultConsentManageHandler.handlePost(consentManageDataMock); | ||
| } catch (ConsentException e) { | ||
| Assert.assertEquals(e.getPayload().getJSONObject("error").getString("description"), | ||
| "Client ID missing in the request."); | ||
| } |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 15
Suggested change
| setConsentManageBuilder(); | |
| JSONObject payload = new JSONObject(TestConstants.VALID_INITIATION); | |
| doReturn(payload).when(consentManageDataMock).getPayload(); | |
| defaultConsentManageHandler.handlePost(consentManageDataMock); | |
| try { | |
| setConsentManageBuilder(); | |
| ConsentManageData consentManageDataMock = mock(ConsentManageData.class); | |
| doReturn(null).when(consentManageDataMock).getClientId(); | |
| defaultConsentManageHandler.handlePost(consentManageDataMock); | |
| } catch (ConsentException e) { | |
| Assert.assertEquals(e.getPayload().getJSONObject("error").getString("description"), | |
| "Client ID missing in the request."); | |
| } | |
| try { | |
| log.info("Handling POST request for consent management. Client ID: {}", consentManageDataMock.getClientId()); | |
| setConsentManageBuilder(); | |
| ConsentManageData consentManageDataMock = mock(ConsentManageData.class); | |
| doReturn(null).when(consentManageDataMock).getClientId(); | |
| defaultConsentManageHandler.handlePost(consentManageDataMock); | |
| } catch (ConsentException e) { | |
| log.error("Failed to handle POST request due to missing client ID: {}", e.getMessage()); | |
| Assert.assertEquals(e.getPayload().getJSONObject("error").getString("description"), | |
| "Client ID missing in the request."); | |
| } |
Comment on lines
611
to
+623
| public void testHandleFileGetWithoutRequestPath() { | ||
|
|
||
| setConsentManageBuilder(); | ||
| doReturn(TestConstants.SAMPLE_CLIENT_ID).when(consentManageDataMock).getClientId(); | ||
| doReturn(null).when(consentManageDataMock).getRequestPath(); | ||
|
|
||
| defaultConsentManageHandler.handleFileGet(consentManageDataMock); | ||
| try { | ||
| setConsentManageBuilder(); | ||
| ConsentManageData consentManageDataMock = mock(ConsentManageData.class); | ||
| doReturn(TestConstants.SAMPLE_CLIENT_ID).when(consentManageDataMock).getClientId(); | ||
| doReturn(null).when(consentManageDataMock).getRequestPath(); | ||
|
|
||
| defaultConsentManageHandler.handleFileGet(consentManageDataMock); | ||
| } catch (ConsentException e) { | ||
| Assert.assertEquals(e.getPayload().getJSONObject("error").getString("description"), | ||
| "Resource path not found in the request"); | ||
| } |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 16
Suggested change
| public void testHandleFileGetWithoutRequestPath() { | |
| setConsentManageBuilder(); | |
| doReturn(TestConstants.SAMPLE_CLIENT_ID).when(consentManageDataMock).getClientId(); | |
| doReturn(null).when(consentManageDataMock).getRequestPath(); | |
| defaultConsentManageHandler.handleFileGet(consentManageDataMock); | |
| try { | |
| setConsentManageBuilder(); | |
| ConsentManageData consentManageDataMock = mock(ConsentManageData.class); | |
| doReturn(TestConstants.SAMPLE_CLIENT_ID).when(consentManageDataMock).getClientId(); | |
| doReturn(null).when(consentManageDataMock).getRequestPath(); | |
| defaultConsentManageHandler.handleFileGet(consentManageDataMock); | |
| } catch (ConsentException e) { | |
| Assert.assertEquals(e.getPayload().getJSONObject("error").getString("description"), | |
| "Resource path not found in the request"); | |
| } | |
| try { | |
| log.info("Handling GET request for file consent. Client ID: {}, Request Path: {}", | |
| consentManageDataMock.getClientId(), consentManageDataMock.getRequestPath()); | |
| setConsentManageBuilder(); | |
| ConsentManageData consentManageDataMock = mock(ConsentManageData.class); | |
| doReturn(TestConstants.SAMPLE_CLIENT_ID).when(consentManageDataMock).getClientId(); | |
| doReturn(null).when(consentManageDataMock).getRequestPath(); | |
| defaultConsentManageHandler.handleFileGet(consentManageDataMock); | |
| } catch (ConsentException e) { | |
| log.error("Failed to handle file GET request due to missing request path: {}", e.getMessage()); | |
| Assert.assertEquals(e.getPayload().getJSONObject("error").getString("description"), | |
| "Resource path not found in the request"); | |
| } |
Comment on lines
+265
to
+268
| public static ExternalAPIPreConsentUpdateResponseDTO getSampleExternalAPIPreConsentUpdateResponseDTO() { | ||
|
|
||
| ExternalAPIPreConsentUpdateResponseDTO responseDTO = new ExternalAPIPreConsentUpdateResponseDTO(); | ||
|
|
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 17
Suggested change
| public static ExternalAPIPreConsentUpdateResponseDTO getSampleExternalAPIPreConsentUpdateResponseDTO() { | |
| ExternalAPIPreConsentUpdateResponseDTO responseDTO = new ExternalAPIPreConsentUpdateResponseDTO(); | |
| public static ExternalAPIPreConsentUpdateResponseDTO getSampleExternalAPIPreConsentUpdateResponseDTO() { | |
| ExternalAPIPreConsentUpdateResponseDTO responseDTO = new ExternalAPIPreConsentUpdateResponseDTO(); | |
| log.debug("Creating sample ExternalAPIPreConsentUpdateResponseDTO for testing"); |
Comment on lines
+290
to
+292
| public static ExternalAPIModifiedResponseDTO getSampleExternalAPIModifiedResponseDTO() { | ||
|
|
||
| ExternalAPIModifiedResponseDTO responseDTO = new ExternalAPIModifiedResponseDTO(); |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 18
Suggested change
| public static ExternalAPIModifiedResponseDTO getSampleExternalAPIModifiedResponseDTO() { | |
| ExternalAPIModifiedResponseDTO responseDTO = new ExternalAPIModifiedResponseDTO(); | |
| public static ExternalAPIModifiedResponseDTO getSampleExternalAPIModifiedResponseDTO() { | |
| ExternalAPIModifiedResponseDTO responseDTO = new ExternalAPIModifiedResponseDTO(); | |
| log.debug("Creating sample ExternalAPIModifiedResponseDTO for testing"); |
Comment on lines
+283
to
+289
| @Override | ||
| public ConsentResource updateConsent(ConsentResource consentResource) throws ConsentManagementException { | ||
|
|
||
| if (consentResource == null || StringUtils.isBlank(consentResource.getConsentID())) { | ||
| log.error("Consent or consentId is missing"); | ||
| throw new ConsentManagementException("Consent or consentId is missing"); | ||
| } |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 19
Suggested change
| @Override | |
| public ConsentResource updateConsent(ConsentResource consentResource) throws ConsentManagementException { | |
| if (consentResource == null || StringUtils.isBlank(consentResource.getConsentID())) { | |
| log.error("Consent or consentId is missing"); | |
| throw new ConsentManagementException("Consent or consentId is missing"); | |
| } | |
| public ConsentResource updateConsent(ConsentResource consentResource) throws ConsentManagementException { | |
| if (consentResource == null || StringUtils.isBlank(consentResource.getConsentID())) { | |
| log.error("Consent or consentId is missing"); | |
| throw new ConsentManagementException("Consent or consentId is missing"); | |
| } | |
| log.info("Updating consent with ID: " + consentResource.getConsentID()); |
Comment on lines
+311
to
+316
| DatabaseUtils.commitTransaction(connection); | ||
| log.debug("Updated consent successfully."); | ||
| return consentResource; | ||
|
|
||
| } catch (ConsentDataInsertionException | ConsentDataUpdationException e) { | ||
| log.error("Error during updating consent, rolling back", e); |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 20
Suggested change
| DatabaseUtils.commitTransaction(connection); | |
| log.debug("Updated consent successfully."); | |
| return consentResource; | |
| } catch (ConsentDataInsertionException | ConsentDataUpdationException e) { | |
| log.error("Error during updating consent, rolling back", e); | |
| DatabaseUtils.commitTransaction(connection); | |
| log.debug("Updated consent successfully."); | |
| return consentResource; | |
| } catch (ConsentDataInsertionException | ConsentDataUpdationException e) { | |
| log.error("Error during updating consent, rolling back: " + e.getMessage()); |
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: 12
🧹 Nitpick comments (2)
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)
293-302: Javadoc promises null return, but method throws exception when "Data" key is missing.The Javadoc states "If not present, return null" but
getJSONObject(DATA)at line 296 throwsJSONExceptionif the "Data" key is absent. The null-return guarantee only applies to the nested "Status" field. This is consistent with the existinggetValidityTimemethod pattern, but the documentation could mislead callers.Consider either:
- Updating the Javadoc to clarify that the payload must contain a "Data" object, or
- Adding defensive checks for consistency with the documented contract.
Option 1: Clarify Javadoc
/** - * Get status value from the request payload. If not present, return null. + * Get status value from the request payload. If Status is not present within the Data object, return null. + * The payload must contain a "Data" JSON object; otherwise a JSONException is thrown. * * `@param` payload request payload * `@return` status value if present in the request, null otherwise + * `@throws` org.json.JSONException if payload is missing "Data" key */Option 2: Add defensive check (aligns with Javadoc promise)
public static String getStatus(Object payload) { JSONObject requestPayload = (JSONObject) payload; - JSONObject data = requestPayload.getJSONObject(ConsentExtensionConstants.DATA); + if (!requestPayload.has(ConsentExtensionConstants.DATA)) { + return null; + } + JSONObject data = requestPayload.getJSONObject(ConsentExtensionConstants.DATA); if (data.has(ConsentExtensionConstants.STATUS)) { return data.getString(ConsentExtensionConstants.STATUS); } return null; }🤖 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 293 - 302, The getStatus method currently calls requestPayload.getJSONObject(ConsentExtensionConstants.DATA) which throws if the "Data" key is missing, contradicting the Javadoc that promises a null return; update getStatus to be defensive: verify payload is a JSONObject and that requestPayload.has(ConsentExtensionConstants.DATA) (and that the DATA value is a JSONObject) before calling getJSONObject, then check for ConsentExtensionConstants.STATUS and return its string or null; mirror the defensive pattern used in getValidityTime so callers get null instead of an exception when "Data" or "Status" is absent.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)
130-148: Extract the shared initiation-response mutation into one helper.Both overloads now carry the same
DATAmutation logic, so the next field added here can easily land in only one path. Prefer delegating both overloads to a single private helper and keep the overload-specific behavior limited to logging.♻️ Suggested refactor
public static JSONObject getInitiationResponse(Object responseObj, DetailedConsentResource createdConsent) { - JSONObject response = (JSONObject) responseObj; - JSONObject dataObject = response.getJSONObject(ConsentExtensionConstants.DATA); - dataObject.put(ConsentExtensionConstants.CONSENT_ID, createdConsent.getConsentID()); - dataObject.put(ConsentExtensionConstants.CREATION_DATE_TIME, convertToISO8601(createdConsent.getCreatedTime())); - dataObject.put(ConsentExtensionConstants.STATUS_UPDATE_DATE_TIME, - convertToISO8601(createdConsent.getUpdatedTime())); - dataObject.put(ConsentExtensionConstants.STATUS, createdConsent.getCurrentStatus()); - - response.remove(ConsentExtensionConstants.DATA); - response.put(ConsentExtensionConstants.DATA, dataObject); - - return response; + return populateInitiationResponse((JSONObject) responseObj, createdConsent.getConsentID(), + createdConsent.getCreatedTime(), createdConsent.getUpdatedTime(), createdConsent.getCurrentStatus()); } public static JSONObject getInitiationResponse(Object responseObj, ConsentResource createdConsent) { - - JSONObject response = (JSONObject) responseObj; - JSONObject dataObject = response.getJSONObject(ConsentExtensionConstants.DATA); - dataObject.put(ConsentExtensionConstants.CONSENT_ID, createdConsent.getConsentID()); - dataObject.put(ConsentExtensionConstants.CREATION_DATE_TIME, convertToISO8601(createdConsent.getCreatedTime())); - dataObject.put(ConsentExtensionConstants.STATUS_UPDATE_DATE_TIME, - convertToISO8601(createdConsent.getUpdatedTime())); - dataObject.put(ConsentExtensionConstants.STATUS, createdConsent.getCurrentStatus()); - - response.remove(ConsentExtensionConstants.DATA); - response.put(ConsentExtensionConstants.DATA, dataObject); + JSONObject response = populateInitiationResponse((JSONObject) 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 response; } + +private static JSONObject populateInitiationResponse(JSONObject response, String consentId, long createdTime, + long updatedTime, String status) { + 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.put(ConsentExtensionConstants.DATA, dataObject); + return response; +}🤖 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 130 - 148, The DATA mutation logic in getInitiationResponse is duplicated across overloads; create a single private helper (e.g., populateInitiationData or buildInitiationData) that accepts the JSONObject response and ConsentResource createdConsent, performs the dataObject retrieval and updates (CONSENT_ID, CREATION_DATE_TIME via convertToISO8601, STATUS_UPDATE_DATE_TIME, STATUS) and replaces the DATA entry, then return the mutated JSONObject. Change both getInitiationResponse overloads to delegate to that helper and keep only the overload-specific logging (with the same .replaceAll sanitization) in each overload.
🤖 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`:
- Around line 1-11: Update the OpenAPI info.version field from "v1.0.3" to
"v1.0.4" so the declared contract version matches the bumped spec; locate and
edit the info.version entry in the OpenAPI YAML (the line currently set to
v1.0.3) to v1.0.4 and verify there are no other stale version strings in the
file such as in documentation metadata or examples.
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 returns rows where
FS_CONSENT_ATTRIBUTE columns can be null, so update the mapper
ConsentManagementDAOUtil.setDataToConsentResourceWithAttributes(...) to guard
against null attribute rows: when ATT_KEY and/or ATT_VALUE are null, do not
insert an entry into the attributes map and ensure the map remains empty (or
absent) for attribute-less consents; locate where ATT_KEY/ATT_VALUE are read and
add a null-check (or Optional check) before calling map.put(...) so the query
change does not produce a {null=null} entry.
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-460: The buildConsentResource method currently reconstructs a
ConsentResource solely from ExternalAPIBasicConsentResourceResponseDTO
(basicConsentResource) and defaults a missing receipt to "{}", which overwrites
existing stored consent data on partial updates; change buildConsentResource (or
add an overload) to accept the existing ConsentResource (or load it by
consentID) and merge values: for each field on basicConsentResource only
overwrite when the DTO field is non-null/explicitly provided, preserve existing
ConsentResource values otherwise, and for receipt parse and merge JSON objects
rather than replacing with "{}" when receipt is missing; ensure the returned
ConsentResource keeps unchanged fields from the stored consent and only updates
fields present in basicConsentResource.
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 468-488: The PUT/update path in DefaultConsentManageHandler is
missing the standard header validation call; ensure you invoke the existing
validateRequestHeaders(...) method (the same one used by the GET/POST/DELETE
handlers) early in the PUT flow—e.g., right after entering the handler or before
any clientId/resource-path checks using consentManageData—to run the same header
checks and abort with the appropriate error if headers are invalid.
- Around line 826-839: The updateConsent method builds a new ConsentResource but
fails to preserve existing basic-consent fields (notably consentFrequency and
recurringIndicator) from storedConsentResource, causing local PUTs to overwrite
them; modify updateConsent (the code that constructs ConsentResource before
calling consentCoreService.updateConsent) to copy consentFrequency and
recurringIndicator (and any other non-requested basic fields) from
storedConsentResource into the new ConsentResource instance so those values are
preserved when persisted; ensure the fields are set on the consentResource prior
to calling consentCoreService.updateConsent.
- Around line 493-530: In DefaultConsentManageHandler, before validating and
updating, compare the consent type derived from the request path/payload with
the stored consent's type (storedConsentResource.getConsentType()) and reject
the update if they differ; specifically, after retrieving storedConsentResource
and before calling
ConsentManageUtils.getConsentManageValidator().validateRequestPayload(...)
obtain the request consent type (e.g., via
ConsentManageUtils.getConsentManageValidator().getConsentType(consentManageData)
or equivalent) and if
(!requestConsentType.equals(storedConsentResource.getConsentType())) log an
appropriate error and throw a ConsentException (ResponseStatus.BAD_REQUEST) to
prevent persisting mismatched types (this affects the branch that calls
updateConsent(consentManageData, consentId, storedConsentResource,
consentType)).
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 159-172: The test methods (e.g., testHandlePostWithoutClientId in
DefaultConsentManageHandlerTest) currently catch ConsentException but do not
fail if no exception is thrown; update each such test (including the ~27 listed
like testHandlePostWithInvalidHeaders, testHandleGetWithoutClientId,
testHandleDeleteWithoutClientId, testHandlePutWithInvalidConsentId,
testHandlePatch, etc.) by inserting an explicit failure immediately after
invoking the handler (e.g., after defaultConsentManageHandler.handlePost(...) /
handleGet(...) / handleDelete(...) / handlePut(...) / handleFilePost(...)) using
Assert.fail("Expected ConsentException not thrown") so the test fails when the
expected ConsentException is not raised, and keep the existing catch block
assertions unchanged to validate the exception payload when thrown.
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`:
- Line 33: The test maps INITIATION_WITHOUT_ACCOUNT_PERMISSION to the wrong
expected message because the fixture contains invalid JSON (unquoted
ReadBeneficiariesDetail) and will fail during payload parsing; update the
DataProviders entry for INITIATION_WITHOUT_ACCOUNT_PERMISSION to either (a)
change the expected message from "Permissions are invalid" to the parsing error
message your parser produces (so the test asserts the payload parse failure), or
(b) if you intended to test semantic permission validation, fix the fixture JSON
so ReadBeneficiariesDetail is quoted (making it valid JSON) and keep the
"Permissions are invalid" expectation; locate the mapping in class DataProviders
where INITIATION_WITHOUT_ACCOUNT_PERMISSION is defined and apply one of these
two fixes.
In
`@financial-services-accelerator/components/org.wso2.financial.services.accelerator.consent.mgt.service/src/main/java/org/wso2/financial/services/accelerator/consent/mgt/service/ConsentCoreService.java`:
- Around line 123-132: The updateConsent(ConsentResource consentResource) API
lacks an explicit acting user parameter which prevents proper audit attribution;
modify the ConsentCoreService interface to add an actor parameter (e.g., String
actingUserId or a UserContext/Actor object) to the updateConsent method
signature, update all implementations of ConsentCoreService.updateConsent to
accept and propagate this actor, and ensure the audit record creation logic in
those implementations uses the new acting user value instead of pulling user
info from ConsentResource or context; also update all callers to pass the
current authenticated user when invoking updateConsent so the contract clearly
requires the actor for auditing.
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-307: The code calls getConsent(...) and assigns to
previousConsent then immediately dereferences
previousConsent.getCurrentStatus(), which can NPE if getConsent returns null; in
ConsentCoreServiceImpl, check previousConsent for null right after calling
getConsent (before calling consentCoreDAO.updateConsentResource and before using
previousConsent.getCurrentStatus()) and if null throw a controlled
ConsentManagementException (with an appropriate message/enum) or return an error
path; ensure the null-check happens before invoking
ConsentCoreServiceUtil.postStateChange so postStateChange always receives a
valid previous status.
- Around line 299-318: After calling
consentCoreDAO.updateConsentResource(connection, consentResource) retrieve the
persisted record (e.g., call consentCoreDAO.getConsentResourceById or
equivalent) into a new variable (persistedConsent) and use persistedConsent
instead of the inbound consentResource when building consentDataMap, passing
clientID, and for the returned value; call
ConsentCoreServiceUtil.postStateChange with persistedConsent.getConsentID() and
persistedConsent.getCurrentStatus() (and previousConsent as before), commit the
transaction and return persistedConsent to ensure audit and response reflect
stored immutable fields.
---
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 130-148: The DATA mutation logic in getInitiationResponse is
duplicated across overloads; create a single private helper (e.g.,
populateInitiationData or buildInitiationData) that accepts the JSONObject
response and ConsentResource createdConsent, performs the dataObject retrieval
and updates (CONSENT_ID, CREATION_DATE_TIME via convertToISO8601,
STATUS_UPDATE_DATE_TIME, STATUS) and replaces the DATA entry, then return the
mutated JSONObject. Change both getInitiationResponse overloads to delegate to
that helper and keep only the overload-specific logging (with the same
.replaceAll sanitization) in each overload.
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 293-302: The getStatus method currently calls
requestPayload.getJSONObject(ConsentExtensionConstants.DATA) which throws if the
"Data" key is missing, contradicting the Javadoc that promises a null return;
update getStatus to be defensive: verify payload is a JSONObject and that
requestPayload.has(ConsentExtensionConstants.DATA) (and that the DATA value is a
JSONObject) before calling getJSONObject, then check for
ConsentExtensionConstants.STATUS and return its string or null; mirror the
defensive pattern used in getValidityTime so callers get null instead of an
exception when "Data" or "Status" is absent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 62429a81-b4fc-46eb-91be-cca2ebc21153
📒 Files selected for processing (25)
financial-services-accelerator/accelerators/fs-is/repository/resources/apis/accelerator-extensions-v1.0.4.yamlfinancial-services-accelerator/accelerators/fs-is/repository/resources/wso2is-7.0.0-deployment.tomlfinancial-services-accelerator/accelerators/fs-is/repository/resources/wso2is-7.1.0-deployment.tomlfinancial-services-accelerator/accelerators/fs-is/repository/resources/wso2is-7.2.0-deployment.tomlfinancial-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.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/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
Comment on lines
+1
to
+11
| openapi: 3.0.1 | ||
| info: | ||
| title: API contract for financial accelerator extension points in WSO2 IS and APIM | ||
| description: This API defines the REST API contract for services that implements logic to extend the Open Data accelerator flow. | ||
| contact: | ||
| name: WSO2 | ||
| url: https://wso2.com/solutions/financial-services/open-banking/ | ||
| 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 the declared contract version to v1.0.4.
This new v1.0.4 contract still advertises v1.0.3 in info.version, so generated docs/SDKs will publish the new consent-update surface under the old version.
📝 Proposed 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
| openapi: 3.0.1 | |
| info: | |
| title: API contract for financial accelerator extension points in WSO2 IS and APIM | |
| description: This API defines the REST API contract for services that implements logic to extend the Open Data accelerator flow. | |
| contact: | |
| name: WSO2 | |
| url: https://wso2.com/solutions/financial-services/open-banking/ | |
| license: | |
| name: Apache 2.0 | |
| url: https://www.apache.org/licenses/LICENSE-2.0.html | |
| version: v1.0.3 | |
| openapi: 3.0.1 | |
| info: | |
| title: API contract for financial accelerator extension points in WSO2 IS and APIM | |
| description: This API defines the REST API contract for services that implements logic to extend the Open Data accelerator flow. | |
| contact: | |
| name: WSO2 | |
| url: https://wso2.com/solutions/financial-services/open-banking/ | |
| license: | |
| name: Apache 2.0 | |
| url: https://www.apache.org/licenses/LICENSE-2.0.html | |
| 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`
around lines 1 - 11, Update the OpenAPI info.version field from "v1.0.3" to
"v1.0.4" so the declared contract version matches the bumped spec; locate and
edit the info.version entry in the OpenAPI YAML (the line currently set to
v1.0.3) to v1.0.4 and verify there are no other stale version strings in the
file such as in documentation metadata or examples.
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 to LEFT JOIN.
Lines 44-45 now return a consent even when no FS_CONSENT_ATTRIBUTE row exists, but ConsentManagementDAOUtil.setDataToConsentResourceWithAttributes(...) currently inserts ATT_KEY/ATT_VALUE into the map without a null check. That means attribute-less consents come back with a {null=null} entry instead of an empty attribute map. Please update the mapper in the same change, otherwise this query change breaks the no-attributes path.
Suggested follow-up fix in the mapper
while (resultSet.next()) {
- retrievedConsentAttributeMap.put(resultSet.getString(ConsentMgtDAOConstants.ATT_KEY),
- resultSet.getString(ConsentMgtDAOConstants.ATT_VALUE));
+ String attributeKey = resultSet.getString(ConsentMgtDAOConstants.ATT_KEY);
+ if (attributeKey != null) {
+ retrievedConsentAttributeMap.put(attributeKey,
+ resultSet.getString(ConsentMgtDAOConstants.ATT_VALUE));
+ }
}🤖 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 returns rows where
FS_CONSENT_ATTRIBUTE columns can be null, so update the mapper
ConsentManagementDAOUtil.setDataToConsentResourceWithAttributes(...) to guard
against null attribute rows: when ATT_KEY and/or ATT_VALUE are null, do not
insert an entry into the attributes map and ensure the map remains empty (or
absent) for attribute-less consents; locate where ATT_KEY/ATT_VALUE are read and
add a null-check (or Optional check) before calling map.put(...) so the query
change does not produce a {null=null} entry.
Comment on lines
+452
to
+460
| public static ConsentResource buildConsentResource(ExternalAPIBasicConsentResourceResponseDTO basicConsentResource, | ||
| String consentID, String clientID, long createTime) { | ||
|
|
||
| String receipt = basicConsentResource.getReceipt() != null ? | ||
| new JSONObject(basicConsentResource.getReceipt()).toString() : "{}"; | ||
| return new ConsentResource(consentID, clientID, receipt, basicConsentResource.getType(), | ||
| basicConsentResource.getFrequency(), basicConsentResource.getValidityTime(), | ||
| basicConsentResource.getRecurringIndicator(), basicConsentResource.getStatus(), | ||
| createTime, 0); |
There was a problem hiding this comment.
Don’t treat the external pre-update payload as a full replacement.
Line 455 defaults a missing receipt to {}, and the ConsentResource returned on Line 457 is built solely from the extension response. If the extension only returns the fields it changed, the rest of the stored basic-consent state will be overwritten on update. Merge with the existing consent instead of rebuilding from scratch.
🤖 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/ExternalAPIUtil.java`
around lines 452 - 460, The buildConsentResource method currently reconstructs a
ConsentResource solely from ExternalAPIBasicConsentResourceResponseDTO
(basicConsentResource) and defaults a missing receipt to "{}", which overwrites
existing stored consent data on partial updates; change buildConsentResource (or
add an overload) to accept the existing ConsentResource (or load it by
consentID) and merge values: for each field on basicConsentResource only
overwrite when the DTO field is non-null/explicitly provided, preserve existing
ConsentResource values otherwise, and for receipt parse and merge JSON objects
rather than replacing with "{}" when receipt is missing; ensure the returned
ConsentResource keeps unchanged fields from the stored consent and only updates
fields present in basicConsentResource.
Comment on lines
+468
to
+488
| //Check whether client ID exists | ||
| if (StringUtils.isEmpty(consentManageData.getClientId())) { | ||
| log.error("Client ID missing in the request."); | ||
| throw new ConsentException(ResponseStatus.BAD_REQUEST, "Client ID missing in the request.", | ||
| ConsentOperationEnum.CONSENT_UPDATE); | ||
| } | ||
|
|
||
| //Check whether client ID is valid | ||
| if (!FinancialServicesUtils.isValidClientId(consentManageData.getClientId())) { | ||
| log.error("Client ID does not exist in the system."); | ||
| throw new ConsentException(ResponseStatus.BAD_REQUEST, "Client ID does not exist in the system.", | ||
| ConsentOperationEnum.CONSENT_UPDATE); | ||
| } | ||
|
|
||
| if (consentManageData.getRequestPath() == null) { | ||
| log.error("Resource Path Not Found"); | ||
| throw new ConsentException(ResponseStatus.BAD_REQUEST, "Resource Path Not Found", | ||
| ConsentOperationEnum.CONSENT_UPDATE); | ||
| } | ||
|
|
||
| String consentId = ConsentManageUtils.extractConsentIdFromPath(consentManageData.getRequestPath(), |
There was a problem hiding this comment.
Run the standard header validation on PUT requests too.
This new path never calls validateRequestHeaders, unlike the existing GET/POST/DELETE consent handlers. That makes update requests bypass the same header checks enforced on the rest of the base consent flow.
🛡️ Proposed fix
if (!FinancialServicesUtils.isValidClientId(consentManageData.getClientId())) {
log.error("Client ID does not exist in the system.");
throw new ConsentException(ResponseStatus.BAD_REQUEST, "Client ID does not exist in the system.",
ConsentOperationEnum.CONSENT_UPDATE);
}
+
+ ConsentPayloadValidationResult headerValidationResult = ConsentManageUtils.getConsentManageValidator()
+ .validateRequestHeaders(consentManageData);
+ if (!headerValidationResult.isValid()) {
+ log.error(headerValidationResult.getErrorMessage().replaceAll("[\r\n]+", " "));
+ throw new ConsentException(headerValidationResult.getHttpCode(),
+ headerValidationResult.getErrorMessage(), ConsentOperationEnum.CONSENT_UPDATE);
+ }
if (consentManageData.getRequestPath() == null) {📝 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
| //Check whether client ID exists | |
| if (StringUtils.isEmpty(consentManageData.getClientId())) { | |
| log.error("Client ID missing in the request."); | |
| throw new ConsentException(ResponseStatus.BAD_REQUEST, "Client ID missing in the request.", | |
| ConsentOperationEnum.CONSENT_UPDATE); | |
| } | |
| //Check whether client ID is valid | |
| if (!FinancialServicesUtils.isValidClientId(consentManageData.getClientId())) { | |
| log.error("Client ID does not exist in the system."); | |
| throw new ConsentException(ResponseStatus.BAD_REQUEST, "Client ID does not exist in the system.", | |
| ConsentOperationEnum.CONSENT_UPDATE); | |
| } | |
| if (consentManageData.getRequestPath() == null) { | |
| log.error("Resource Path Not Found"); | |
| throw new ConsentException(ResponseStatus.BAD_REQUEST, "Resource Path Not Found", | |
| ConsentOperationEnum.CONSENT_UPDATE); | |
| } | |
| String consentId = ConsentManageUtils.extractConsentIdFromPath(consentManageData.getRequestPath(), | |
| //Check whether client ID exists | |
| if (StringUtils.isEmpty(consentManageData.getClientId())) { | |
| log.error("Client ID missing in the request."); | |
| throw new ConsentException(ResponseStatus.BAD_REQUEST, "Client ID missing in the request.", | |
| ConsentOperationEnum.CONSENT_UPDATE); | |
| } | |
| //Check whether client ID is valid | |
| if (!FinancialServicesUtils.isValidClientId(consentManageData.getClientId())) { | |
| log.error("Client ID does not exist in the system."); | |
| throw new ConsentException(ResponseStatus.BAD_REQUEST, "Client ID does not exist in the system.", | |
| ConsentOperationEnum.CONSENT_UPDATE); | |
| } | |
| ConsentPayloadValidationResult headerValidationResult = ConsentManageUtils.getConsentManageValidator() | |
| .validateRequestHeaders(consentManageData); | |
| if (!headerValidationResult.isValid()) { | |
| log.error(headerValidationResult.getErrorMessage().replaceAll("[\r\n]+", " ")); | |
| throw new ConsentException(headerValidationResult.getHttpCode(), | |
| headerValidationResult.getErrorMessage(), ConsentOperationEnum.CONSENT_UPDATE); | |
| } | |
| if (consentManageData.getRequestPath() == null) { | |
| log.error("Resource Path Not Found"); | |
| throw new ConsentException(ResponseStatus.BAD_REQUEST, "Resource Path Not Found", | |
| ConsentOperationEnum.CONSENT_UPDATE); | |
| } | |
| String consentId = ConsentManageUtils.extractConsentIdFromPath(consentManageData.getRequestPath(), |
🤖 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 468 - 488, The PUT/update path in DefaultConsentManageHandler is
missing the standard header validation call; ensure you invoke the existing
validateRequestHeaders(...) method (the same one used by the GET/POST/DELETE
handlers) early in the PUT flow—e.g., right after entering the handler or before
any clientId/resource-path checks using consentManageData—to run the same header
checks and abort with the appropriate error if headers are invalid.
Comment on lines
+493
to
+530
| ConsentResource storedConsentResource = consentCoreService.getConsent(consentId, true); | ||
|
|
||
| if (storedConsentResource == null) { | ||
| log.error("Consent not found"); | ||
| throw new ConsentException(ResponseStatus.BAD_REQUEST, "Consent not found", | ||
| ConsentOperationEnum.CONSENT_UPDATE); | ||
| } | ||
|
|
||
| if (!storedConsentResource.getClientID().equals(consentManageData.getClientId())) { | ||
| //Throwing this error in a generic manner since client will not be able to identify if consent | ||
| // exists if consent does not belong to them | ||
| log.error(ConsentManageConstants.CLIENT_ID_MISMATCH_ERROR); | ||
| throw new ConsentException(ResponseStatus.BAD_REQUEST, ConsentManageConstants.CLIENT_ID_MISMATCH_ERROR, | ||
| ConsentOperationEnum.CONSENT_UPDATE); | ||
| } | ||
|
|
||
| ConsentResource updatedConsent; | ||
| if (isExtensionsEnabled && isExternalPreConsentUpdateEnabled) { | ||
| // Call external service before updating consent | ||
| ExternalAPIBasicConsentResourceRequestDTO externalAPIConsentResource = | ||
| new ExternalAPIBasicConsentResourceRequestDTO(storedConsentResource); | ||
| ExternalAPIPreConsentUpdateRequestDTO preRequestDTO = | ||
| new ExternalAPIPreConsentUpdateRequestDTO(consentManageData, externalAPIConsentResource); | ||
| ExternalAPIPreConsentUpdateResponseDTO preResponseDTO = ExternalAPIConsentManageUtils. | ||
| callExternalService(preRequestDTO); | ||
| updatedConsent = updateConsent(preResponseDTO, storedConsentResource); | ||
| } else { | ||
| String consentType = ConsentManageUtils.getConsentManageValidator().getConsentType(consentManageData); | ||
| //Validate Initiation request | ||
| ConsentPayloadValidationResult validationResponse = ConsentManageUtils.getConsentManageValidator() | ||
| .validateRequestPayload(consentManageData, consentType); | ||
| if (!validationResponse.isValid()) { | ||
| log.error(validationResponse.getErrorMessage().replaceAll("[\r\n]+", " ")); | ||
| throw new ConsentException(validationResponse.getHttpCode(), validationResponse.getErrorCode(), | ||
| validationResponse.getErrorMessage()); | ||
| } | ||
|
|
||
| updatedConsent = updateConsent(consentManageData, consentId, storedConsentResource, consentType); |
There was a problem hiding this comment.
Reject updates when the URL consent type does not match the stored consent.
Unlike GET/DELETE/file retrieval, this flow never compares the type encoded in the request path with storedConsentResource.getConsentType(). The validator therefore uses one consent type, while the update logic later persists the stored consent as another.
🧭 Proposed fix
if (!storedConsentResource.getClientID().equals(consentManageData.getClientId())) {
//Throwing this error in a generic manner since client will not be able to identify if consent
// exists if consent does not belong to them
log.error(ConsentManageConstants.CLIENT_ID_MISMATCH_ERROR);
throw new ConsentException(ResponseStatus.BAD_REQUEST, ConsentManageConstants.CLIENT_ID_MISMATCH_ERROR,
ConsentOperationEnum.CONSENT_UPDATE);
}
+
+ String requestConsentType = ConsentExtensionUtils.getConsentType(consentManageData.getRequestPath());
+ if (!requestConsentType.equals(storedConsentResource.getConsentType())) {
+ log.error(ConsentManageConstants.CONSENT_TYPE_MISMATCH_ERROR);
+ throw new ConsentException(ResponseStatus.BAD_REQUEST,
+ ConsentManageConstants.CONSENT_TYPE_MISMATCH_ERROR,
+ ConsentOperationEnum.CONSENT_UPDATE);
+ }
ConsentResource 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.extensions/src/main/java/org/wso2/financial/services/accelerator/consent/mgt/extensions/manage/impl/DefaultConsentManageHandler.java`
around lines 493 - 530, In DefaultConsentManageHandler, before validating and
updating, compare the consent type derived from the request path/payload with
the stored consent's type (storedConsentResource.getConsentType()) and reject
the update if they differ; specifically, after retrieving storedConsentResource
and before calling
ConsentManageUtils.getConsentManageValidator().validateRequestPayload(...)
obtain the request consent type (e.g., via
ConsentManageUtils.getConsentManageValidator().getConsentType(consentManageData)
or equivalent) and if
(!requestConsentType.equals(storedConsentResource.getConsentType())) log an
appropriate error and throw a ConsentException (ResponseStatus.BAD_REQUEST) to
prevent persisting mismatched types (this affects the branch that calls
updateConsent(consentManageData, consentId, storedConsentResource,
consentType)).
| {INITIATION_WITH_INVALID_TRANS_TO_DATES}, | ||
| {INITIATION_WITH_PAST_TRANS_TO_DATES} | ||
| {"{}", "Invalid request payload"}, | ||
| {INITIATION_WITHOUT_ACCOUNT_PERMISSION, "Permissions are invalid"}, |
There was a problem hiding this comment.
Fix the expected error for the malformed permission fixture.
Line 33 expects "Permissions are invalid", but INITIATION_WITHOUT_ACCOUNT_PERMISSION is not valid JSON (ReadBeneficiariesDetail is unquoted). This case will usually fail at payload parsing first, so the test is asserting the wrong error path.
Proposed fix
- {INITIATION_WITHOUT_ACCOUNT_PERMISSION, "Permissions are invalid"},
+ {INITIATION_WITHOUT_ACCOUNT_PERMISSION, "Invalid request payload"},If the intent was to test semantic permission validation instead, fix the fixture itself to be valid JSON and keep the current message.
📝 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
| {INITIATION_WITHOUT_ACCOUNT_PERMISSION, "Permissions are invalid"}, | |
| {INITIATION_WITHOUT_ACCOUNT_PERMISSION, "Invalid request payload"}, |
🤖 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`
at line 33, The test maps INITIATION_WITHOUT_ACCOUNT_PERMISSION to the wrong
expected message because the fixture contains invalid JSON (unquoted
ReadBeneficiariesDetail) and will fail during payload parsing; update the
DataProviders entry for INITIATION_WITHOUT_ACCOUNT_PERMISSION to either (a)
change the expected message from "Permissions are invalid" to the parsing error
message your parser produces (so the test asserts the payload parse failure), or
(b) if you intended to test semantic permission validation, fix the fixture JSON
so ReadBeneficiariesDetail is quoted (making it valid JSON) and keep the
"Permissions are invalid" expectation; locate the mapping in class DataProviders
where INITIATION_WITHOUT_ACCOUNT_PERMISSION is defined and apply one of these
two fixes.
Comment on lines
+123
to
+132
| /** | ||
| * This method is used to update an existing consent (excluding client ID and CreatedTime) | ||
| * based on the given consent. | ||
| * An audit record will be created. | ||
| * | ||
| * @param consentResource the consent resource with updated values and new associations | ||
| * @return the updated consent resource | ||
| * @throws ConsentManagementException thrown if an error occurs during the operation | ||
| */ | ||
| ConsentResource updateConsent(ConsentResource consentResource) throws ConsentManagementException; |
There was a problem hiding this comment.
Pass the acting user through this API.
This method says it creates an audit record, but unlike the other state-changing APIs here it has no explicit userID/actor parameter. That makes the new PUT flow hard to attribute correctly and nudges callers toward stuffing auth context into ConsentResource, which weakens the contract for a consent-management API.
Suggested API shape
- ConsentResource updateConsent(ConsentResource consentResource) throws ConsentManagementException;
+ ConsentResource updateConsent(ConsentResource consentResource, String userID)
+ throws ConsentManagementException;📝 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
| /** | |
| * This method is used to update an existing consent (excluding client ID and CreatedTime) | |
| * based on the given consent. | |
| * An audit record will be created. | |
| * | |
| * @param consentResource the consent resource with updated values and new associations | |
| * @return the updated consent resource | |
| * @throws ConsentManagementException thrown if an error occurs during the operation | |
| */ | |
| ConsentResource updateConsent(ConsentResource consentResource) throws ConsentManagementException; | |
| /** | |
| * This method is used to update an existing consent (excluding client ID and CreatedTime) | |
| * based on the given consent. | |
| * An audit record will be created. | |
| * | |
| * `@param` consentResource the consent resource with updated values and new associations | |
| * `@return` the updated consent resource | |
| * `@throws` ConsentManagementException thrown if an error occurs during the operation | |
| */ | |
| ConsentResource updateConsent(ConsentResource consentResource, String userID) | |
| throws ConsentManagementException; |
🤖 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/ConsentCoreService.java`
around lines 123 - 132, The updateConsent(ConsentResource consentResource) API
lacks an explicit acting user parameter which prevents proper audit attribution;
modify the ConsentCoreService interface to add an actor parameter (e.g., String
actingUserId or a UserContext/Actor object) to the updateConsent method
signature, update all implementations of ConsentCoreService.updateConsent to
accept and propagate this actor, and ensure the audit record creation logic in
those implementations uses the new acting user value instead of pulling user
info from ConsentResource or context; also update all callers to pass the
current authenticated user when invoking updateConsent so the contract clearly
requires the actor for auditing.
Comment on lines
+296
to
+307
| ConsentResource previousConsent = getConsent(consentResource.getConsentID(), false); | ||
| /* Update the base consent using updated values from ConsentResource. | ||
| Immutable parameters are ignored in the update (i.e. clientId, createdTime) at DAO level.*/ | ||
| consentCoreDAO.updateConsentResource(connection, consentResource); | ||
|
|
||
| // Add audit record | ||
| HashMap<String, Object> consentDataMap = new HashMap<>(); | ||
| consentDataMap.put(ConsentCoreServiceConstants.CONSENT_RESOURCE, consentResource); | ||
|
|
||
| ConsentCoreServiceUtil.postStateChange(connection, consentCoreDAO, consentResource.getConsentID(), | ||
| null, consentResource.getCurrentStatus(), previousConsent.getCurrentStatus(), | ||
| ConsentCoreServiceConstants.BASIC_CONSENT_UPDATE_REASON, consentResource.getClientID(), |
There was a problem hiding this comment.
Handle the unknown-consent path before dereferencing previousConsent.
getConsent() can yield null for a missing consent, and Line 306 dereferences it unconditionally. That turns an invalid consent ID into a 500/NPE instead of a controlled ConsentManagementException.
Suggested fix
- ConsentResource previousConsent = getConsent(consentResource.getConsentID(), false);
+ ConsentResource previousConsent = getConsent(consentResource.getConsentID(), false);
+ if (previousConsent == null) {
+ throw new ConsentManagementException("Consent not found for consentId: "
+ + consentResource.getConsentID());
+ }📝 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
| ConsentResource previousConsent = getConsent(consentResource.getConsentID(), false); | |
| /* Update the base consent using updated values from ConsentResource. | |
| Immutable parameters are ignored in the update (i.e. clientId, createdTime) at DAO level.*/ | |
| consentCoreDAO.updateConsentResource(connection, consentResource); | |
| // Add audit record | |
| HashMap<String, Object> consentDataMap = new HashMap<>(); | |
| consentDataMap.put(ConsentCoreServiceConstants.CONSENT_RESOURCE, consentResource); | |
| ConsentCoreServiceUtil.postStateChange(connection, consentCoreDAO, consentResource.getConsentID(), | |
| null, consentResource.getCurrentStatus(), previousConsent.getCurrentStatus(), | |
| ConsentCoreServiceConstants.BASIC_CONSENT_UPDATE_REASON, consentResource.getClientID(), | |
| ConsentResource previousConsent = getConsent(consentResource.getConsentID(), false); | |
| if (previousConsent == null) { | |
| throw new ConsentManagementException("Consent not found for consentId: " | |
| consentResource.getConsentID()); | |
| } | |
| /* Update the base consent using updated values from ConsentResource. | |
| Immutable parameters are ignored in the update (i.e. clientId, createdTime) at DAO level.*/ | |
| consentCoreDAO.updateConsentResource(connection, consentResource); | |
| // Add audit record | |
| HashMap<String, Object> consentDataMap = new HashMap<>(); | |
| consentDataMap.put(ConsentCoreServiceConstants.CONSENT_RESOURCE, consentResource); | |
| ConsentCoreServiceUtil.postStateChange(connection, consentCoreDAO, consentResource.getConsentID(), | |
| null, consentResource.getCurrentStatus(), previousConsent.getCurrentStatus(), | |
| ConsentCoreServiceConstants.BASIC_CONSENT_UPDATE_REASON, consentResource.getClientID(), |
🤖 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 296 - 307, The code calls getConsent(...) and assigns to
previousConsent then immediately dereferences
previousConsent.getCurrentStatus(), which can NPE if getConsent returns null; in
ConsentCoreServiceImpl, check previousConsent for null right after calling
getConsent (before calling consentCoreDAO.updateConsentResource and before using
previousConsent.getCurrentStatus()) and if null throw a controlled
ConsentManagementException (with an appropriate message/enum) or return an error
path; ensure the null-check happens before invoking
ConsentCoreServiceUtil.postStateChange so postStateChange always receives a
valid previous status.
Comment on lines
+299
to
+318
| consentCoreDAO.updateConsentResource(connection, consentResource); | ||
|
|
||
| // Add audit record | ||
| HashMap<String, Object> consentDataMap = new HashMap<>(); | ||
| consentDataMap.put(ConsentCoreServiceConstants.CONSENT_RESOURCE, consentResource); | ||
|
|
||
| ConsentCoreServiceUtil.postStateChange(connection, consentCoreDAO, consentResource.getConsentID(), | ||
| null, consentResource.getCurrentStatus(), previousConsent.getCurrentStatus(), | ||
| ConsentCoreServiceConstants.BASIC_CONSENT_UPDATE_REASON, consentResource.getClientID(), | ||
| consentDataMap | ||
| ); | ||
|
|
||
| DatabaseUtils.commitTransaction(connection); | ||
| log.debug("Updated consent successfully."); | ||
| return consentResource; | ||
|
|
||
| } catch (ConsentDataInsertionException | ConsentDataUpdationException e) { | ||
| log.error("Error during updating consent, rolling back", e); | ||
| DatabaseUtils.rollbackTransaction(connection); | ||
| throw new ConsentManagementException("Failed to update consent and create related records", e); |
There was a problem hiding this comment.
Use the persisted consent for audit and the response.
The DAO explicitly ignores immutable fields during update, but Lines 303-313 still use the inbound consentResource. If the caller sends stale/changed immutable values, the audit payload, clientID, and returned object can diverge from what was actually stored.
Suggested fix
consentCoreDAO.updateConsentResource(connection, consentResource);
+ ConsentResource updatedConsent =
+ consentCoreDAO.getConsentResource(connection, consentResource.getConsentID());
// Add audit record
HashMap<String, Object> consentDataMap = new HashMap<>();
- consentDataMap.put(ConsentCoreServiceConstants.CONSENT_RESOURCE, consentResource);
+ consentDataMap.put(ConsentCoreServiceConstants.CONSENT_RESOURCE, updatedConsent);
- ConsentCoreServiceUtil.postStateChange(connection, consentCoreDAO, consentResource.getConsentID(),
- null, consentResource.getCurrentStatus(), previousConsent.getCurrentStatus(),
- ConsentCoreServiceConstants.BASIC_CONSENT_UPDATE_REASON, consentResource.getClientID(),
+ ConsentCoreServiceUtil.postStateChange(connection, consentCoreDAO, updatedConsent.getConsentID(),
+ null, updatedConsent.getCurrentStatus(), previousConsent.getCurrentStatus(),
+ ConsentCoreServiceConstants.BASIC_CONSENT_UPDATE_REASON, updatedConsent.getClientID(),
consentDataMap
);
DatabaseUtils.commitTransaction(connection);
log.debug("Updated consent successfully.");
- return consentResource;
+ return updatedConsent;
- } catch (ConsentDataInsertionException | ConsentDataUpdationException e) {
+ } catch (ConsentDataInsertionException | ConsentDataUpdationException |
+ ConsentDataRetrievalException 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.service/src/main/java/org/wso2/financial/services/accelerator/consent/mgt/service/impl/ConsentCoreServiceImpl.java`
around lines 299 - 318, After calling
consentCoreDAO.updateConsentResource(connection, consentResource) retrieve the
persisted record (e.g., call consentCoreDAO.getConsentResourceById or
equivalent) into a new variable (persistedConsent) and use persistedConsent
instead of the inbound consentResource when building consentDataMap, passing
clientID, and for the returned value; call
ConsentCoreServiceUtil.postStateChange with persistedConsent.getConsentID() and
persistedConsent.getCurrentStatus() (and previousConsent as before), commit the
transaction and return persistedConsent to ensure audit and response reflect
stored immutable fields.
Comment on lines
+539
to
+571
| @Test | ||
| public void testUpdateConsent() throws ConsentManagementException, | ||
| ConsentDataRetrievalException, ConsentDataUpdationException { | ||
|
|
||
| doReturn(ConsentMgtServiceTestData.getSampleStoredConsentResource()).when(mockedConsentCoreDAO) | ||
| .getConsentResource(any(), anyString()); | ||
| doNothing().when(mockedConsentCoreDAO).updateConsentResource(any(), any()); | ||
|
|
||
| ConsentResource result = consentCoreServiceImpl.updateConsent( | ||
| ConsentMgtServiceTestData.getSampleStoredConsentResource()); | ||
| Assert.assertNotNull(result); | ||
| } | ||
|
|
||
| @Test(expectedExceptions = ConsentManagementException.class) | ||
| public void testUpdateConsentWithoutConsentId() throws ConsentManagementException { | ||
|
|
||
| ConsentResource consentResource = new ConsentResource(); | ||
| consentResource.setConsentID(null); | ||
|
|
||
| consentCoreServiceImpl.updateConsent(consentResource); | ||
| } | ||
|
|
||
| @Test(expectedExceptions = ConsentManagementException.class) | ||
| public void testUpdateConsentWithConsentUpdateError() throws ConsentManagementException, | ||
| ConsentDataUpdationException, ConsentDataRetrievalException { | ||
|
|
||
| doReturn(ConsentMgtServiceTestData.getSampleStoredConsentResource()).when(mockedConsentCoreDAO) | ||
| .getConsentResource(any(), anyString()); | ||
| doThrow(ConsentDataUpdationException.class).when(mockedConsentCoreDAO).updateConsentResource(any(), any()); | ||
|
|
||
| ConsentResource result = consentCoreServiceImpl.updateConsent( | ||
| ConsentMgtServiceTestData.getSampleStoredConsentResource()); | ||
| } |
There was a problem hiding this comment.
Strengthen these tests to assert the actual update contract.
Right now testUpdateConsent only proves the method returns something non-null. It would still pass if the implementation simply reloaded the stored consent, overwrote immutable fields like clientID/CreatedTime, or skipped the audit/write path entirely. Please assert at least one updated field, verify the immutable fields stay unchanged, and verify the expected DAO/audit interaction for the PUT flow.
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
1 participant
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
Issue link: required
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