Added data publishing capability from accelerators on APIM,IS to SI #879
Conversation
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
Data publishing changes
WalkthroughAdds a configurable data-publishing subsystem: template and runtime configuration, parser support, event executor framework, a new data.publisher module (Thrift publishers, pools, queues, clients, reporters), identity filters/handlers, consent publishing hooks, tests, and FindBugs filter updates. Changes
Sequence Diagram(s)sequenceDiagram
participant Source as Event Source (Auth/Consent/HTTP)
participant Handler as Handler / Filter
participant Util as FSDataPublisherUtil
participant Queue as EventQueue
participant Worker as QueueWorker / ParallelQueueWorker
participant Pool as DataPublisherPool
participant Pub as FinancialServicesDataPublisher
Source->>Handler: event / HTTP request arrives
Handler->>Handler: extract fields -> build analytics map
Handler->>Util: publishData(streamName, version, data)
Note over Util: Check config and ELK flag
Util->>Util: isDataPublishingEnabled?
alt Data publishing enabled
Util->>Queue: enqueue FSAnalyticsEvent
Queue->>Queue: enqueue & submit worker task
Worker->>Queue: poll event
Worker->>Pool: borrow publisher instance
Pool-->>Worker: FinancialServicesDataPublisher
Worker->>Pub: publish(stream, version, data)
Pub->>Pub: validate attributes & build payload
Pub->>Pub: tryPublish / optionally write ELK log
Worker->>Pool: return publisher
else Data publishing disabled
Util->>Util: log "data publishing is disabled"
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
Pre-merge checks❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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
+214
to
+215
| private void buildFSEventExecutors() { | ||
| log.debug("Building Financial Services Event Executors configuration"); |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 1
Suggested change
| private void buildFSEventExecutors() { | |
| log.debug("Building Financial Services Event Executors configuration"); | |
| private void buildFSEventExecutors() { | |
| log.info("Building Financial Services Event Executors configuration"); |
Comment on lines
+321
to
+322
| protected void buildDataPublishingStreams() { | ||
| log.debug("Building Data Publishing Streams configuration"); |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 2
Suggested change
| protected void buildDataPublishingStreams() { | |
| log.debug("Building Data Publishing Streams configuration"); | |
| protected void buildDataPublishingStreams() { | |
| log.info("Building Data Publishing Streams configuration"); |
Comment on lines
+64
to
+68
| @Override | ||
| public Map<String, Map<Integer, String>> getDataPublishingStreams() { | ||
|
|
||
| return configParser.getDataPublishingStreams(); | ||
| } |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 3
Suggested change
| @Override | |
| public Map<String, Map<Integer, String>> getDataPublishingStreams() { | |
| return configParser.getDataPublishingStreams(); | |
| } | |
| @Override | |
| public Map<String, Map<Integer, String>> getDataPublishingStreams() { | |
| log.debug("Retrieving data publishing streams configuration"); | |
| return configParser.getDataPublishingStreams(); | |
| } |
Comment on lines
+70
to
+74
| @Override | ||
| public Map<String, Map<String, Object>> getDataPublishingValidationMap() { | ||
|
|
||
| return configParser.getDataPublishingValidationMap(); | ||
| } |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 4
Suggested change
| @Override | |
| public Map<String, Map<String, Object>> getDataPublishingValidationMap() { | |
| return configParser.getDataPublishingValidationMap(); | |
| } | |
| @Override | |
| public Map<String, Map<String, Object>> getDataPublishingValidationMap() { | |
| log.debug("Retrieving data publishing validation configuration"); | |
| return configParser.getDataPublishingValidationMap(); | |
| } |
Comment on lines
+30
to
+34
| @Override | ||
| public void processEvent(FSEvent fsEvent) { | ||
| if (log.isDebugEnabled()) { | ||
| log.debug("Processing FSEvent: " + fsEvent.getEventType().replaceAll("[\r\n]", "")); | ||
| } |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 5
Suggested change
| @Override | |
| public void processEvent(FSEvent fsEvent) { | |
| if (log.isDebugEnabled()) { | |
| log.debug("Processing FSEvent: " + fsEvent.getEventType().replaceAll("[\r\n]", "")); | |
| } | |
| public void processEvent(FSEvent fsEvent) { | |
| log.info("Processing FSEvent of type: " + fsEvent.getEventType()); | |
| if (log.isDebugEnabled()) { | |
| log.debug("Processing FSEvent: " + fsEvent.getEventType().replaceAll("[\r\n]", "")); | |
| } |
Comment on lines
+48
to
+50
| // Note : Using a fixed worker thread pool and a bounded queue to control the load on the server | ||
| executorService = Executors.newFixedThreadPool(workerThreadCount); | ||
| eventQueue = new ArrayBlockingQueue<>(queueSize); |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 6
Suggested change
| // Note : Using a fixed worker thread pool and a bounded queue to control the load on the server | |
| executorService = Executors.newFixedThreadPool(workerThreadCount); | |
| eventQueue = new ArrayBlockingQueue<>(queueSize); | |
| executorService = Executors.newFixedThreadPool(workerThreadCount); | |
| eventQueue = new ArrayBlockingQueue<>(queueSize); | |
| log.info("FSEventQueue initialized with queue size: " + queueSize + " and worker threads: " + workerThreadCount); |
Comment on lines
+52
to
+53
|
|
||
| public void put(FSEvent obEvent) { |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 7
Suggested change
| public void put(FSEvent obEvent) { | |
| public void put(FSEvent obEvent) { | |
| if (log.isDebugEnabled()) { | |
| log.debug("Attempting to add event to queue: " + obEvent.getClass().getSimpleName()); | |
| } |
Comment on lines
+50
to
+51
| @Override | ||
| public void run() { |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 8
Suggested change
| @Override | |
| public void run() { | |
| @Override | |
| public void run() { | |
| log.info("FSQueueWorker started processing events"); |
Comment on lines
+67
to
+70
| } | ||
| obEventExecutor.processEvent(event); | ||
| } | ||
| } else { |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 9
Suggested change
| } | |
| obEventExecutor.processEvent(event); | |
| } | |
| } else { | |
| obEventExecutor.processEvent(event); | |
| } | |
| log.info("Successfully processed FSEvent"); | |
| } else { | |
| log.error("OB Event is null"); |
Comment on lines
+34
to
+37
|
|
||
| public FSEvent(String eventType, Map<String, Object> eventData) { | ||
| if (log.isDebugEnabled()) { | ||
| log.debug("Creating FSEvent with type: " + eventType.replaceAll("[\r\n]", "")); |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 10
Suggested change
| public FSEvent(String eventType, Map<String, Object> eventData) { | |
| if (log.isDebugEnabled()) { | |
| log.debug("Creating FSEvent with type: " + eventType.replaceAll("[\r\n]", "")); | |
| public FSEvent(String eventType, Map<String, Object> eventData) { | |
| log.info("Creating new FSEvent with type: " + eventType); | |
| if (log.isDebugEnabled()) { | |
| log.debug("Creating FSEvent with type: " + eventType.replaceAll("[\r\n]", "")); | |
| } |
| int workerThreadCount = | ||
| Integer.parseInt((String) FinancialServicesConfigParser.getInstance().getConfiguration() | ||
| .get(FinancialServicesConstants.EVENT_WORKER_THREAD_COUNT)); | ||
| fsEventQueue = new FSEventQueue(queueSize, workerThreadCount); |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 11
Suggested change
| fsEventQueue = new FSEventQueue(queueSize, workerThreadCount); | |
| fsEventQueue = new FSEventQueue(queueSize, workerThreadCount); | |
| log.info("Initialized FSEventQueue with queue size: " + queueSize + " and worker thread count: " + workerThreadCount); |
Comment on lines
+84
to
+86
| public void setFSEventExecutor(Map<Integer, String> fsEventExecutors) { | ||
|
|
||
| this.fsEventExecutors = fsEventExecutors; |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 12
Suggested change
| public void setFSEventExecutor(Map<Integer, String> fsEventExecutors) { | |
| this.fsEventExecutors = fsEventExecutors; | |
| public void setFSEventExecutor(Map<Integer, String> fsEventExecutors) { | |
| log.debug("Setting " + (fsEventExecutors != null ? fsEventExecutors.size() : 0) + " event executors"); | |
| this.fsEventExecutors = fsEventExecutors; |
Comment on lines
108
to
+112
| consentPersist(consentPersistData, consentResource); | ||
| if (log.isDebugEnabled()) { | ||
| log.info("Consent persisted successfully for consent ID: " + consentResource.getConsentID()); | ||
| } | ||
| ConsentAuthorizeUtil.publishConsentApprovalStatus(consentPersistData); |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 13
Suggested change
| consentPersist(consentPersistData, consentResource); | |
| if (log.isDebugEnabled()) { | |
| log.info("Consent persisted successfully for consent ID: " + consentResource.getConsentID()); | |
| } | |
| ConsentAuthorizeUtil.publishConsentApprovalStatus(consentPersistData); | |
| consentPersist(consentPersistData, consentResource); | |
| log.info("Consent persisted successfully for consent ID: " + consentResource.getConsentID()); | |
| ConsentAuthorizeUtil.publishConsentApprovalStatus(consentPersistData); |
Comment on lines
187
to
+191
| ExternalAPIConsentResourceResponseDTO responseConsentResource = responseDTO.getConsentResource(); | ||
| persistConsent(responseConsentResource, consentData); | ||
| if (log.isDebugEnabled()) { | ||
| log.info("Consent persisted successfully for consent ID: " + consentData.getConsentId()); | ||
| } |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 15
Suggested change
| ExternalAPIConsentResourceResponseDTO responseConsentResource = responseDTO.getConsentResource(); | |
| persistConsent(responseConsentResource, consentData); | |
| if (log.isDebugEnabled()) { | |
| log.info("Consent persisted successfully for consent ID: " + consentData.getConsentId()); | |
| } | |
| ExternalAPIConsentResourceResponseDTO responseConsentResource = responseDTO.getConsentResource(); | |
| persistConsent(responseConsentResource, consentData); | |
| log.info("Consent persisted successfully for consent ID: " + consentData.getConsentId()); |
Comment on lines
194
to
+195
| } catch (FinancialServicesException e) { | ||
| log.error("Failed to persist consent: " + e); |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 16
Suggested change
| } catch (FinancialServicesException e) { | |
| log.error("Failed to persist consent: " + e); | |
| } catch (FinancialServicesException e) { | |
| log.error("Failed to persist consent: " + e.getMessage()); |
Comment on lines
+672
to
+675
| if (log.isDebugEnabled()) { | ||
| log.debug("Publishing consent approval status for consent: " + consentPersistData.getConsentData(). | ||
| getConsentId()); | ||
| } |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 17
Suggested change
| if (log.isDebugEnabled()) { | |
| log.debug("Publishing consent approval status for consent: " + consentPersistData.getConsentData(). | |
| getConsentId()); | |
| } | |
| if (log.isDebugEnabled()) { | |
| log.debug("Publishing consent approval status for consent: " + consentPersistData.getConsentData(). | |
| getConsentId()); | |
| } | |
| log.info("Publishing consent approval status for consent: " + consentPersistData.getConsentData().getConsentId()); |
Comment on lines
+738
to
+743
|
|
||
| if (StringUtils.isBlank(requestUri)) { | ||
| log.error("Request URI not found."); | ||
| return null; | ||
| } | ||
|
|
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 18
Suggested change
| if (StringUtils.isBlank(requestUri)) { | |
| log.error("Request URI not found."); | |
| return null; | |
| } | |
| if (StringUtils.isBlank(requestUri)) { | |
| log.error("Request URI not found."); | |
| return null; | |
| } | |
| log.debug("Extracting request URI key from request URI"); | |
| if (log.isDebugEnabled()) { | |
| log.debug("Extracting request URI key from request URI"); | |
| } |
Comment on lines
+63
to
+66
| CacheBuilder.newBuilder().expireAfterWrite(1, TimeUnit.MINUTES).maximumSize(20).build(); | ||
|
|
||
| @Override | ||
| public void processEvent(FSEvent fsEvent) { |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 19
Suggested change
| CacheBuilder.newBuilder().expireAfterWrite(1, TimeUnit.MINUTES).maximumSize(20).build(); | |
| @Override | |
| public void processEvent(FSEvent fsEvent) { | |
| @Override | |
| public void processEvent(FSEvent fsEvent) { | |
| log.info("Processing consent lifecycle event"); | |
| Map<String, Object> eventData = fsEvent.getEventData(); |
Comment on lines
+83
to
+86
| String primaryUserId; | ||
| try { | ||
| primaryUserId = getPrimaryUserForConsent(detailedConsentResource, consentId); | ||
| } catch (ConsentManagementException e) { |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 20
Suggested change
| String primaryUserId; | |
| try { | |
| primaryUserId = getPrimaryUserForConsent(detailedConsentResource, consentId); | |
| } catch (ConsentManagementException e) { | |
| } catch (ConsentManagementException e) { | |
| log.error("Error while trying to retrieve consent data for consent ID: " + consentId, e); | |
| return; | |
| } |
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: 15
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
Fix all issues with AI Agents 🤖
In
@financial-services-accelerator/accelerators/fs-is/carbon-home/repository/conf/financial-services.xml:
- Around line 143-145: The configuration currently uses insecure defaults
(Enabled, Username, Password entries) set to admin/admin; update the <Enabled>
element to false by default or replace the <Username> and <Password> values with
non-functional placeholder tokens that force operators to set real credentials
(e.g., use EMPTY or CHANGE_ME), and add a prominent comment or documentation
near these entries reminding deployers to set strong, unique credentials before
production; ensure any automated deployment or template validation fails if
placeholders remain.
- Around line 158-172: The APIInvocationsEventStream contains duplicate
priority="1" for Attribute messageId and consentId which can cause
nondeterministic ordering; update the priority values so each Attribute
(messageId, consentId, consumerId, clientId, userAgent, statusCode,
statusMessage, httpMethod, responsePayloadSize, electedResource, apiName,
apiSpecVersion, requestedTimestamp) has a unique, sequential priority (e.g.,
messageId=1, consentId=2, then increment the rest accordingly) so ordering is
deterministic.
In
@financial-services-accelerator/accelerators/fs-is/carbon-home/repository/resources/conf/templates/repository/conf/financial-services.xml.j2:
- Around line 657-666: The {% set %} blocks are assigning the literal strings
"{{attribute.required}}" and "{{attribute.type}}"; change them to reference the
Jinja2 variables directly and without quotes/mustache syntax, e.g. use {% if
attribute.required is defined %}{% set required = attribute.required %}{% else
%}{% set required = false %}{% endif %} and similarly for attribute.type with {%
set type = attribute.type %} (or "string" as the fallback) so the variables
required and type receive the actual values.
- Around line 581-583: The executor class name in the template is incorrect:
replace the non-existent
org.wso2.financial.services.accelerator.common.event.executor.DefaultOBEventExecutor
with the actual implementation
org.wso2.financial.services.accelerator.common.event.executor.DefaultFSEventExecutor
so the configuration uses the correct executor class (update the <Executor
class="..."> entry to reference DefaultFSEventExecutor).
In
@financial-services-accelerator/components/org.wso2.financial.services.accelerator.common/src/main/java/org/wso2/financial/services/accelerator/common/event/executor/FSEventQueue.java:
- Around line 53-64: The current put(FSEvent) submits a new FSQueueWorker per
event which is inefficient; instead, pre-start a fixed number of long-running
workers in the FSEventQueue constructor that continuously poll eventQueue (using
the existing FSQueueWorker logic adapted to loop until shutdown) and submit
those workers once using executorService.submit(...) from the constructor;
update put(FSEvent) to only offer the event to eventQueue and not submit
workers; ensure FSQueueWorker exposes a shutdown/stop signal and the constructor
wires executorService's pool size (or a configured worker count) to how many
workers are pre-started so you don't create a worker per event.
In
@financial-services-accelerator/components/org.wso2.financial.services.accelerator.common/src/main/java/org/wso2/financial/services/accelerator/common/event/executor/FSQueueWorker.java:
- Around line 56-72: The code treats a normal empty-queue poll as an error and
recreates executors on every event; change the log at the null branch from error
to debug/trace (so empty poll isn’t treated as an error) and stop instantiating
executors per event by caching FSEventExecutor instances once (e.g., resolve the
FQN strings returned by
FinancialServicesCommonDataHolder.getInstance().getFSEventExecutors() into
FSEventExecutor objects using FinancialServicesUtils.getClassInstanceFromFQN and
store them in a Map<Integer,FSEventExecutor> for reuse), then iterate that
cached executor map and call obEventExecutor.processEvent(event) from
FSQueueWorker when an event is present.
- Line 53: The constructor in FSQueueWorker unsafely casts the executorService
parameter to ThreadPoolExecutor; update the FSQueueWorker constructor to either
accept ThreadPoolExecutor instead of ExecutorService or perform an instanceof
check on executorService before casting (e.g., if (executorService instanceof
ThreadPoolExecutor) assign to threadPoolExecutor, otherwise throw an
IllegalArgumentException with a clear message), and ensure the rest of the code
uses the validated threadPoolExecutor variable.
In
@financial-services-accelerator/components/org.wso2.financial.services.accelerator.common/src/main/resources/findbugs-exclude.xml:
- Around line 97-100: Add a brief explanatory comment above the EI_EXPOSE_REP
exclusion that references
org.wso2.financial.services.accelerator.common.event.executor.model.FSEvent and
its getEventData() method, stating that the method intentionally returns the
internal Map (EI_EXPOSE_REP) and why this is safe in this codebase (e.g., shared
mutable design or performance rationale, callers are trusted/contracted not to
mutate, or modifications are synchronized/validated elsewhere); mirror the style
and level of detail used in the existing comment block around lines 101-107 so
future readers understand the tradeoff.
In
@financial-services-accelerator/components/org.wso2.financial.services.accelerator.common/src/main/resources/findbugs-include.xml:
- Around line 18-22: The current findbugs-include.xml uses a FindBugsFilter that
whitelists only Bug category="SECURITY", which unintentionally hides
non-security categories; fix by removing or replacing that restrictive Match
(the FindBugsFilter / Match / Bug category="SECURITY" block) so the include file
no longer limits reporting to SECURITY-only — either delete the include filter
entirely (so all categories are reported) and rely on findbugs-exclude.xml for
suppressions, or swap the contents of findbugs-include.xml and
findbugs-exclude.xml so the exclude rules are applied and the include file does
not act as a whitelist.
In
@financial-services-accelerator/components/org.wso2.financial.services.accelerator.common/src/test/resources/repository/conf/financial-services.xml:
- Around line 309-323: The APIInvocationsEventStream defines attributes with
duplicate priority values (Attribute name="messageId" and Attribute
name="consentId" both have priority="1"); update the priority values so each
Attribute in APIInvocationsEventStream has a unique priority (e.g., increment
consentId to "2" and shift subsequent Attribute priorities accordingly) by
editing the Attribute elements (messageId, consentId, consumerId, clientId,
userAgent, statusCode, statusMessage, httpMethod, responsePayloadSize,
electedResource, apiName, apiSpecVersion, requestedTimestamp) to ensure a
deterministic, non-overlapping ordering.
In
@financial-services-accelerator/components/org.wso2.financial.services.accelerator.consent.mgt.extensions/src/main/java/org/wso2/financial/services/accelerator/consent/mgt/extensions/authorize/impl/DefaultConsentPersistStep.java:
- Around line 109-112: The code is checking log.isDebugEnabled() but calling
log.info(), which suppresses the info message except when debug is enabled;
update the logging to be consistent by replacing the log.info(...) inside the
isDebugEnabled() guard with log.debug("Consent persisted successfully for
consent ID: " + consentResource.getConsentID()); leave
ConsentAuthorizeUtil.publishConsentApprovalStatus(consentPersistData) unchanged;
alternatively, if the message should always be at INFO level, remove the
isDebugEnabled() guard and keep log.info(...) instead.
In
@financial-services-accelerator/components/org.wso2.financial.services.accelerator.consent.mgt.extensions/src/main/java/org/wso2/financial/services/accelerator/consent/mgt/extensions/authorize/impl/ExternalAPIConsentPersistStep.java:
- Around line 189-192: The logging guard in ExternalAPIConsentPersistStep checks
log.isDebugEnabled() but calls log.info(), causing a level mismatch; update
either the guard or the call so they match (prefer changing the call to
log.debug for debug-level guard) in the block where
ConsentAuthorizeUtil.publishConsentApprovalStatus(consentPersistData) follows
the log statement inside ExternalAPIConsentPersistStep so the check and logging
method (log.isDebugEnabled() vs log.debug(...) or change to log.isInfoEnabled()
vs log.info(...)) are consistent.
In
@financial-services-accelerator/components/org.wso2.financial.services.accelerator.consent.mgt.extensions/src/main/java/org/wso2/financial/services/accelerator/consent/mgt/extensions/authorize/util/ConsentAuthorizeUtil.java:
- Around line 696-701: The publish call is using empty streamName and
streamVersion which will break routing; update the
FSDataPublisherUtil.publishData invocation to pass the correct stream name and
version (use the configured constants or properties—e.g., the analytics stream
name/version used elsewhere in this module or a new constants in
ConsentAuthorizeUtil) instead of "" and ""; also populate or remove the
commented-out userInteractionTime line (e.g., add
consentAuthorizationData.put("userInteractionTime",
<appropriateTimestampOrValue>) if required) so the payload is complete before
calling FSDataPublisherUtil.publishData(consentStreamName, consentStreamVersion,
consentAuthorizationData).
- Around line 718-729: getRequestUriKeyFromQueryParams should guard against a
null or empty spQueryParams before calling URLEncodedUtils.parse; add a
null/empty check at the start of getRequestUriKeyFromQueryParams and return null
immediately if spQueryParams is null or blank so you don't build "?" +
spQueryParams or pass an unexpected value to URLEncodedUtils.parse, then proceed
to parse and call getRequestUriKey(param.getValue()) for the REQUEST_URI param
as before.
In
@financial-services-accelerator/components/org.wso2.financial.services.accelerator.consent.mgt.extensions/src/main/java/org/wso2/financial/services/accelerator/consent/mgt/extensions/event/executor/ConsentLCEventExecutor.java:
- Around line 47-49: ConsentLCEventExecutor currently instantiates
ConsentCoreServiceImpl directly via the consentCoreService field which breaks
DI; change it to accept a ConsentCoreServiceImpl (or better, an interface) via
constructor or a setter and remove the direct "new" instantiation so
callers/tests can inject a mock or alternative implementation; update the
ConsentLCEventExecutor constructor (or add a setConsentCoreService method) to
assign the injected instance to the consentCoreService field and adjust any call
sites that instantiate ConsentLCEventExecutor to pass the dependency.
In
@financial-services-accelerator/components/org.wso2.financial.services.accelerator.consent.mgt.extensions/src/test/java/org/wso2/financial/services/accelerator/consent/mgt/extensions/event/executor/ConsentLCEventExecutorTests.java:
- Around line 42-53: The tests redirect System.out in
ConsentLCEventExecutorTests by calling System.setOut(printStream) but never
restore it; add a private static PrintStream originalOut field, capture
System.out into originalOut before redirecting in beforeTests, and add an
@AfterClass method (e.g., tearDownClass) that calls System.setOut(originalOut)
and closes printStream and outContent to avoid affecting other tests.
In
@financial-services-accelerator/components/org.wso2.financial.services.accelerator.data.publisher/pom.xml:
- Around line 106-118: The Import-Package block has a missing comma after the
org.apache.commons.pool2 entry which breaks OSGi parsing; edit the
Import-Package list (the XML element containing entries like
org.apache.commons.pool2.*; version="${commons-pool2.version}" and
org.apache.synapse.*,) and add a trailing comma after the
org.apache.commons.pool2.*; version="${commons-pool2.version}" entry so each
package entry is properly comma-separated; also scan the Import-Package entries
to ensure all items are separated by commas and no other punctuation errors
remain.
- Around line 46-58: The pom declares the vulnerable dependency
org.apache.synapse:synapse-core (artifactId synapse-core) which is not patched
for CVE-2017-15708; either update that dependency to a patched release (upgrade
synapse-core to 3.0.1 or later, e.g., 3.0.2) in the pom.xml dependency block for
org.apache.synapse:synapse-core, or if you cannot upgrade, add a clear note and
ensure deployment-time mitigation by disabling or restricting Java RMI/JMX
endpoints (authenticate or bind to trusted hosts) in your Synapse/WSo2 runtime
configuration; apply one of these fixes and document it adjacent to the
synapse-core dependency so reviewers can verify the remediation.
In
@financial-services-accelerator/components/org.wso2.financial.services.accelerator.data.publisher/src/main/java/org/wso2/financial/services/accelerator/data/publisher/DataPublisherFactory.java:
- Around line 33-34: The class mistakenly declares the generic parameter as the
concrete name "<FinancialServicesDataPublisher>", causing a shadow and unchecked
cast; change the class declaration to use a type variable (e.g.,
"DataPublisherFactory<T extends FinancialServicesDataPublisher> extends
BasePooledObjectFactory<T>") and update the factory methods (create(), wrap()
and any return types or fields referencing FinancialServicesDataPublisher) to
use T so you can return the proper generic type instead of performing the
unchecked cast to (FinancialServicesDataPublisher); ensure method signatures and
the pooled-object wrapper (wrap/instantiate methods) use T consistently.
In
@financial-services-accelerator/components/org.wso2.financial.services.accelerator.data.publisher/src/main/java/org/wso2/financial/services/accelerator/data/publisher/EventQueue.java:
- Around line 63-68: The class EventQueue currently overrides the deprecated
finalize() to call publisherExecutorService.shutdown(), which is unreliable;
replace this by implementing AutoCloseable (or adding a public shutdown()
method) and move the shutdown logic into a new close() method that calls
publisherExecutorService.shutdown() (and awaitTermination or shutdownNow as
appropriate), remove or drop the finalize() override, and update callers/tests
to invoke close() or shutdown() (or use try-with-resources) so the executor is
deterministically shut down.
In
@financial-services-accelerator/components/org.wso2.financial.services.accelerator.data.publisher/src/main/java/org/wso2/financial/services/accelerator/data/publisher/FinancialServicesDataPublisher.java:
- Around line 22-24: Update the Javadoc block above the interface to correctly
describe the interface name: replace the phrase "Open Banking Data publisher"
with "Financial Services Data publisher" (or a brief description matching
FinancialServicesDataPublisher) so the documentation matches the interface
identifier FinancialServicesDataPublisher and accurately describes its purpose.
In
@financial-services-accelerator/components/org.wso2.financial.services.accelerator.data.publisher/src/main/java/org/wso2/financial/services/accelerator/data/publisher/FinancialServicesThriftDataPublisher.java:
- Around line 175-210: The validateAttributes method assumes attribute metadata
exists; call getAttributeValidationMap() result into attributeValidations
(already done) but before accessing attributeValidations.get(attributeNameKey),
check whether attributeValidations.containsKey(attributeNameKey) and that
attributeValidations.get(attributeNameKey) is non-null; if missing, log a clear
error (including streamName and attribute) and return false to avoid NPEs when
reading FinancialServicesConstants.REQUIRED or
FinancialServicesConstants.ATTRIBUTE_TYPE, then proceed with the existing
required/type checks and isValidDataType call.
- Around line 85-90: The code in FinancialServicesThriftDataPublisher calls
Long.parseLong((String)
obConfigurations.get(DataPublishingConstants.THRIFT_PUBLISHING_TIMEOUT)) without
handling NumberFormatException or null, which can break publish flow; wrap the
parse in a try/catch (or validate the value first) when preparing the timeout
for dataPublisher.tryPublish, fall back to a sensible default timeout if parsing
fails or the config is absent, and log a clear error/warn via log.error or
log.warn including the invalid value and that the default is used; ensure the
published boolean handling via tryPublish remains unchanged.
In
@financial-services-accelerator/components/org.wso2.financial.services.accelerator.data.publisher/src/main/java/org/wso2/financial/services/accelerator/data/publisher/internal/FSAnalyticsDataHolder.java:
- Around line 84-94: Wrap the Integer.parseInt calls in initializePool() with a
try/catch for NumberFormatException, validate configurationMap lookups for
DataPublishingConstants.DATA_PUBLISHING_POOL_SIZE and
DATA_PUBLISHING_POOL_WAIT_TIME, and on parse failure log the error (using the
class logger) and fall back to sensible defaults before setting poolSize and
timeout; then proceed to configure and create the pool with those safe values so
initialization cannot fail due to malformed config.
- Around line 106-112: initializeEventQueue parses QUEUE_SIZE and
WORKER_THREAD_COUNT without guarding against NumberFormatException; wrap the
parsing in a try-catch inside initializeEventQueue, catch NumberFormatException
(and optionally NullPointerException), log the parsing error via the class
logger, and fall back to safe defaults (e.g.,
DataPublishingConstants.DEFAULT_QUEUE_SIZE and
DataPublishingConstants.DEFAULT_WORKER_THREAD_COUNT) before constructing
eventQueue = new EventQueue(queueSize, workerThreadCount); ensure you reference
the same configurationMap keys (DataPublishingConstants.QUEUE_SIZE,
DataPublishingConstants.WORKER_THREAD_COUNT) and the eventQueue field.
In
@financial-services-accelerator/components/org.wso2.financial.services.accelerator.data.publisher/src/main/java/org/wso2/financial/services/accelerator/data/publisher/internal/FSAnalyticsServiceComponent.java:
- Around line 71-74: The unsetConfigService method is incorrectly calling
setFinancialServicesConfigurationService with the provided
financialServicesConfigurationService instead of clearing it; update
unsetConfigService to clear the reference by calling
FSAnalyticsDataHolder.getInstance().setFinancialServicesConfigurationService(null)
(or remove the setter call and ensure the holder's field is nulled) so the
service is properly unbound; reference: method unsetConfigService and setter
setFinancialServicesConfigurationService in FSAnalyticsDataHolder.
In
@financial-services-accelerator/components/org.wso2.financial.services.accelerator.data.publisher/src/main/java/org/wso2/financial/services/accelerator/data/publisher/QueueWorker.java:
- Around line 49-58: The current do/while in QueueWorker uses non-atomic
getActiveCount()/eventQueue.size() and poll(), causing race conditions and
premature exits; replace it with a loop that uses eventQueue.take() (blocking)
to fetch FSAnalyticsEvent, remove the getActiveCount()/size checks, add a
volatile boolean shutdown flag (e.g., volatile boolean shutdownRequested) and a
public shutdown request method that sets the flag and interrupts worker threads,
and make the worker loop exit only when shutdownRequested is true and the queue
is drained; also handle InterruptedException properly and ensure
FSDataPublisherUtil.releaseDataPublishingInstance(...) is called in a finally
block after publish to avoid resource leaks.
- Around line 52-56: The current code in QueueWorker calls
dataPublisher.publish(...) and then
FSDataPublisherUtil.releaseDataPublishingInstance(dataPublisher) only on the
happy path, so if dataPublisher.publish throws the publisher is never returned;
wrap the publish call in a try-finally (or try/catch/finally) around the block
that obtains FinancialServicesDataPublisher from
FSDataPublisherUtil.getDataPublisherInstance(), call dataPublisher.publish(...)
inside try, and ensure
FSDataPublisherUtil.releaseDataPublishingInstance(dataPublisher) is invoked in
the finally block (checking for non-null) to guarantee the publisher is always
released even on exceptions.
- Line 47: The direct cast ThreadPoolExecutor threadPoolExecutor =
((ThreadPoolExecutor) executorService) in QueueWorker is unsafe; add an
instanceof check on executorService before casting and handle the alternative
path: if executorService is a ThreadPoolExecutor, cast and proceed; otherwise
either wrap the provided ExecutorService in a new ThreadPoolExecutor (or create
a suitable fallback via Executors.newFixedThreadPool) or log/throw a clear
exception so you don’t get a ClassCastException at runtime. Ensure the fix
updates any code that assumes a ThreadPoolExecutor instance when using
threadPoolExecutor.
In
@financial-services-accelerator/components/org.wso2.financial.services.accelerator.data.publisher/src/main/java/org/wso2/financial/services/accelerator/data/publisher/thrift/publisher/EventQueue.java:
- Around line 32-36: The class Javadoc for EventQueue incorrectly references
LinkedBlockingDeque while the implementation uses LinkedBlockingQueue; update
the documentation comment on class EventQueue to mention LinkedBlockingQueue
(and adjust any @link tag to {@link java.util.concurrent.LinkedBlockingQueue})
and scan the EventQueue javadoc block for matching phrasing (e.g., "wraps
LinkedBlockingDeque") to replace so the docstring accurately describes the
implementation.
- Around line 63-65: The catch block for RejectedExecutionException is
unreachable because put() only uses eventQueue.offer() (which does not throw
that exception) and task submissions occur in the constructor; remove the catch
(RejectedExecutionException e) { log.error(...)} block from the put() method (or
the method containing eventQueue.offer()) and keep only the appropriate
exception handling (e.g., for InterruptedException or other relevant exceptions)
to eliminate dead code; verify the constructor where tasks are submitted still
handles RejectedExecutionException if needed.
In
@financial-services-accelerator/components/org.wso2.financial.services.accelerator.data.publisher/src/main/java/org/wso2/financial/services/accelerator/data/publisher/thrift/publisher/InputValidator.java:
- Around line 72-74: The class Javadoc for InputValidator contains a typo
("Inout Validator"); update the Javadoc comment above the InputValidator class
to read "Input Validator" instead, ensuring the comment accurately reflects the
class name InputValidator.
In
@financial-services-accelerator/components/org.wso2.financial.services.accelerator.data.publisher/src/main/java/org/wso2/financial/services/accelerator/data/publisher/thrift/publisher/ThriftClient.java:
- Around line 47-58: The constructor for ThriftClient can leave dataPublisher
null on failure, causing a NPE in publish(); add a defensive null-check at the
start of the publish(...) method: if dataPublisher is null, log an error
(including context such as client type/serverURL) and gracefully return or throw
a descriptive runtime exception to fail fast; ensure the check covers the call
site dataPublisher.tryPublish(event) and use the same logger (log) and gson as
present in the class so callers don’t hit a NullPointerException.
In
@financial-services-accelerator/components/org.wso2.financial.services.accelerator.data.publisher/src/main/java/org/wso2/financial/services/accelerator/data/publisher/thrift/publisher/ThriftMetricEventBuilder.java:
- Around line 166-168: In ThriftMetricEventBuilder where REQUEST_TIMESTAMP is
handled, wrap the
OffsetDateTime.parse(timestampString).toInstant().toEpochMilli() call in a
try/catch for DateTimeParseException; on parse failure log the error (including
the bad timestampString) and either skip putting REQUEST_TIMESTAMP into eventMap
or set a safe fallback (e.g., current epoch millis) to avoid propagating an
unchecked exception and breaking event publishing.
- Around line 182-191: The RESPONSE_SIZE branch in ThriftMetricEventBuilder uses
Long.parseLong(propertyValue.toString()) without guarding against
NumberFormatException; wrap the parse in a try/catch (catch
NumberFormatException) and on failure fall back to a safe numeric default (e.g.,
0L) or attempt alternate conversion (e.g., parse as Double then cast) before
putting into eventMap with getRenamedKey(propertyKey); also log or record the
parse error so malformed values don’t break event building and ensure the
control flow (break) remains correct after handling both valid and invalid
values.
- Around line 34-78: ThriftMetricEventBuilder contains many duplicate string
constants that already live in Constants.java (e.g., PROPERTIES_KEY,
META_CLIENT_TYPE, KEY_TYPE, CORRELATION_ID, REQUEST_TIMESTAMP, APPLICATION_ID,
API_CONTEXT, API_RESOURCE_PATH, RESPONSE_TIME, RESPONSE_CODE, USER_AGENT,
THROTTLED, etc.); remove these duplicate declarations from
ThriftMetricEventBuilder, add an import (or static imports) to reference the
canonical constants from Constants.java, and keep only those constants that are
genuinely unique to this class (e.g., any not present in Constants.java such as
DEFAULT_UNKNOWN_VALUE if it is unique). Update all usages in
ThriftMetricEventBuilder to refer to Constants.<CONSTANT_NAME> (or use static
imports) so there are no local duplicate definitions and compilation/imports are
adjusted accordingly.
In
@financial-services-accelerator/components/org.wso2.financial.services.accelerator.data.publisher/src/main/java/org/wso2/financial/services/accelerator/data/publisher/thrift/publisher/ThriftMetricReporter.java:
- Around line 58-72: Parsing of numeric configuration values in
ThriftMetricReporter using Integer.parseInt(...) can throw NumberFormatException
and crash initialization; wrap each parse for
Constants.THRIFT_DATA_PUBLISHER_EVENT_QUEUE_SIZE,
Constants.THRIFT_DATA_PUBLISHER_WORKER_THREAD_COUNT,
Constants.THRIFT_DATA_PUBLISHER_POOL_MAX_IDLE and
Constants.THRIFT_DATA_PUBLISHER_INIT_IDLE_CAPACITY in try/catch (or centralize
into a parseIntOrDefault helper) to catch NumberFormatException, fall back to
the existing defaults (queueSize, workerThreads, maxIdle, initIdleCapacity) and
emit a clear warning via the reporter's logger (include the offending property
name and value).
In
@financial-services-accelerator/components/org.wso2.financial.services.accelerator.data.publisher/src/main/java/org/wso2/financial/services/accelerator/data/publisher/thrift/ThriftAPIMAnalyticsDataProvider.java:
- Around line 131-155: getRequestHeaders currently mutates the Axis2 transport
headers map (headers) when removing excluded keys; instead, create a shallow
copy of the headers map immediately after obtaining it (e.g., new
HashMap<>(headers)) and perform removals on that copy (use the same
excludedHeaders and keysToRemove logic), leaving the original Axis2 message
context property untouched; ensure you handle null checks (headers may be null)
and return the copied/filtered map from getRequestHeaders.
- Around line 72-75: getResponseBodyDetails(messageContext) is being invoked
twice which rebuilds the message each time; call it once, store its returned Map
(e.g., responseDetails) and then use
responseDetails.get(PropertyKeys.RESPONSE_BODY) and
responseDetails.get(PropertyKeys.RESPONSE_BODY_SIZE) to populate
customProperties. Update the code around getResponseBodyDetails,
PropertyKeys.RESPONSE_BODY and PropertyKeys.RESPONSE_BODY_SIZE so the
RelayUtils.buildMessage is only triggered by the single getResponseBodyDetails
call and the cached result is reused.
In
@financial-services-accelerator/components/org.wso2.financial.services.accelerator.data.publisher/src/main/java/org/wso2/financial/services/accelerator/data/publisher/util/FSDataPublisherUtil.java:
- Around line 77-78: The config lookup in FSDataPublisherUtil currently casts
the value from
FSAnalyticsDataHolder.getInstance().getConfigurationMap().get(DataPublishingConstants.DATA_PUBLISHING_ENABLED)
directly to String, which can throw ClassCastException; update the check to
safely obtain a string (e.g., verify instanceof String or use
String.valueOf/Objects.toString with a default) and handle nulls before calling
Boolean.parseBoolean so the code uses a safe string representation of the config
key instead of an unchecked cast.
- Around line 42-52: The method FSDataPublisherUtil.getDataPublisherInstance
currently swallows exceptions and returns null; change it to throw a checked
exception (e.g., DataPublisherUnavailableException) instead of returning null:
catch the Exception from DataPublisherPool.borrowObject(), log with the
exception, wrap it in a new DataPublisherUnavailableException (include the cause
and a clear message) and rethrow; update the method signature to declare throws
DataPublisherUnavailableException and update callers to handle or propagate this
exception so downstream code no longer risks NPEs when using the returned
FinancialServicesDataPublisher.
In
@financial-services-accelerator/components/org.wso2.financial.services.accelerator.data.publisher/src/main/resources/findbugs-exclude.xml:
- Around line 40-47: Remove the duplicate <Match> block that excludes the
CRLF_INJECTION_LOGS bug for the class
org.wso2.financial.services.accelerator.data.publisher.thrift.publisher.EventQueue
so only a single <Match><Class
name="org.wso2.financial.services.accelerator.data.publisher.thrift.publisher.EventQueue"/><Bug
pattern="CRLF_INJECTION_LOGS"/></Match> entry remains; delete the repeated block
to avoid redundant exclusions.
In
@financial-services-accelerator/components/org.wso2.financial.services.accelerator.data.publisher/src/test/java/org/wso2/financial/services/accelerator/data/publisher/EventQueueTest.java:
- Around line 34-39: Remove the test that directly calls EventQueue.finalize()
(deprecated) and replace it with a test that exercises the explicit cleanup API
on EventQueue (e.g., EventQueue.shutdown() or close()), invoking that method
instead of finalize() and adding assertions to verify the executor was shut down
(for example by checking isShutdown/isTerminated or submitting a task and
expecting rejection) or that resources are released; update the test name from
testFinalizeShutsDownExecutor to something like testShutdownCleansUpResources
and ensure it does not call finalize().
In
@financial-services-accelerator/components/org.wso2.financial.services.accelerator.data.publisher/src/test/java/org/wso2/financial/services/accelerator/data/publisher/FSAnalyticsEventQueueTest.java:
- Line 60: Replace the deprecated MockitoAnnotations.initMocks(this) call by
calling MockitoAnnotations.openMocks(this) and store the returned AutoCloseable
in a new field (e.g., AutoCloseable closeable) on the test class; then add an
@AfterClass method (e.g., afterTests) that calls closeable.close() to release
resources. Ensure you reference the existing FSAnalyticsEventQueueTest test
class and replace initMocks usage with openMocks while adding the closeable
field and cleanup method.
In
@financial-services-accelerator/components/org.wso2.financial.services.accelerator.data.publisher/src/test/java/org/wso2/financial/services/accelerator/data/publisher/thrift/publisher/EventQueueTest.java:
- Around line 1-13: Add the missing Apache 2.0 license header at the top of
EventQueueTest.java and make the test framework consistent by removing the JUnit
Jupiter import (org.junit.jupiter.api.Assertions.assertTrue) and replacing it
with the TestNG assertion (org.testng.Assert.assertTrue) or use
Assert.assertTrue wherever assertTrue is used; ensure TestNG annotations
(@BeforeMethod, @Test) remain and imports are adjusted accordingly so all tests
use TestNG only.
In
@financial-services-accelerator/components/org.wso2.financial.services.accelerator.data.publisher/src/test/java/org/wso2/financial/services/accelerator/data/publisher/thrift/publisher/ParallelQueueQorkerTest.java:
- Line 19: Rename the class symbol ParallelQueueQorkerTest to
ParallelQueueWorkerTest to correct the typo; update the public class
declaration, the filename if it matches the class, any constructors or inner
references using ParallelQueueQorkerTest, and all references/imports/usages in
tests or suites to the new name so compilation and test discovery continue to
work.
- Line 3: The import in ParallelQueueQorkerTest pulls the deprecated
commons-pool ObjectPool; replace the import org.apache.commons.pool.ObjectPool
with org.apache.commons.pool2.ObjectPool and update any type references in that
test (e.g., fields, method parameters, mocks or generics that reference
ObjectPool) to use the commons-pool2 ObjectPool type so the test compiles
against the maintained library.
In
@financial-services-accelerator/components/org.wso2.financial.services.accelerator.data.publisher/src/test/java/org/wso2/financial/services/accelerator/data/publisher/thrift/publisher/ThriftCounterMetricTest.java:
- Around line 22-27: The test method testConstructorAndGetters in
ThriftCounterMetricTest uses TestNG Assert.assertEquals with arguments reversed;
swap the arguments in the two assertions so the actual value (metric.getName(),
metric.getSchema()) is the first parameter and the expected values ("test",
MetricSchema.RESPONSE) are the second parameter to restore the correct (actual,
expected) order.
In
@financial-services-accelerator/components/org.wso2.financial.services.accelerator.data.publisher/src/test/java/org/wso2/financial/services/accelerator/data/publisher/thrift/publisher/ThriftMetricReporterTest.java:
- Line 1: The file ThriftMetricReporterTest is missing the project's Apache 2.0
license header; add the standard repository copyright and Apache 2.0 license
header block at the very top of the file (above the existing package declaration
in org.wso2.financial.services.accelerator.data.publisher.thrift.publisher) so
the test class ThriftMetricReporterTest matches other files' headers and
complies with project licensing.
In
@financial-services-accelerator/components/org.wso2.financial.services.accelerator.data.publisher/src/test/java/org/wso2/financial/services/accelerator/data/publisher/thrift/ThriftAPIMAnalyticsDataProviderTest.java:
- Around line 22-28: In testGetApplicationConsumerKey in
ThriftAPIMAnalyticsDataProviderTest the TestNG Assert.assertEquals arguments are
reversed; change the call to pass the actual value first and the expected value
second by swapping the parameters so it calls
Assert.assertEquals(provider.getApplicationConsumerKey(authContext), "key123")
while leaving the Assert.assertNull(provider.getApplicationConsumerKey(null))
check intact.
- Around line 38-51: The two tests use the wrong assertEquals argument order and
need a formatting fix: in testGetUserId and testGetConsentId swap the
Assert.assertEquals arguments so the actual value from
provider.getUserId(authContext) and provider.getConsentId(messageContext) is the
first parameter and the expected string is the second, and add a blank line
separator before the testGetConsentId method for consistency.
In
@financial-services-accelerator/components/org.wso2.financial.services.accelerator.data.publisher/src/test/java/org/wso2/financial/services/accelerator/data/publisher/util/FSDataPublisherUtilTest.java:
- Around line 29-31: The test creates and stubs configParserMock but the static
mock for FinancialServicesConfigParser is commented out, so
FinancialServicesConfigParser.getInstance() returns the real instance and breaks
tests; restore the static mock by uncommenting the
MockedStatic<FinancialServicesConfigParser> configParserStatic declaration and
its initialization in setup (so FinancialServicesConfigParser.getInstance() is
stubbed to return configParserMock) and ensure configParserStatic.close() is
called in teardown, or alternatively remove all stubs interacting with
configParserMock if you prefer not to use the static mock.
- Around line 59-75: Replace the Java built-in assertions in the tests with
TestNG assertions: in testGetDataPublisherInstanceSuccess use
Assert.assertEquals(result, publisherMock) (or static import assertEquals)
instead of "assert result == publisherMock", and in
testGetDataPublisherInstanceException use Assert.assertNull(result) (or static
import assertNull) instead of "assert result == null"; also add the TestNG
static imports (import static org.testng.Assert.assertEquals; import static
org.testng.Assert.assertNull;) or import org.testng.Assert and call Assert.
explicitly so the tests reliably fail when expectations are not met.
In
@financial-services-accelerator/components/org.wso2.financial.services.accelerator.data.publisher/src/test/resources/testng.xml:
- Line 26: The testng.xml references the utility class InputValidator (which has
a private constructor) instead of the actual test class InputValidatorTest;
update the <class name="..."> entry in testng.xml to reference
org.wso2.financial.services.accelerator.data.publisher.thrift.publisher.InputValidatorTest
so TestNG can instantiate and run the tests, ensuring the fully-qualified class
name matches the test class in src/test/java.
- Line 29: TestNG configuration contains a duplicate <class> entry for
EventQueueTest in testng.xml; remove the redundant <class
name="org.wso2.financial.services.accelerator.data.publisher.thrift.publisher.EventQueueTest"/>
occurrence (the second listing) so only a single entry for EventQueueTest
remains to avoid running the same test twice.
In
@financial-services-accelerator/components/org.wso2.financial.services.accelerator.identity.extensions/src/main/java/org/wso2/financial/services/accelerator/identity/extensions/auth/extensions/event/handler/AuthenticationEventHandler.java:
- Around line 106-120: AuthenticationEventHandler: avoid NPE and ensure
consistent user identifier by null-checking the AuthenticatedUser returned from
context.getLastAuthenticatedUser() in the AUTHENTICATION_SUCCESSFUL branch
before calling getAuthenticatedSubjectIdentifier(); if null, fall back to the
same resolution logic used in the AUTHENTICATION_FAILED branch (check
context.getParameters().get(AUTHENTICATED_USER) and get(LAST_LOGIN_FAILED_USER)
and use getAuthenticatedSubjectIdentifier() when possible, otherwise
getUserName()), and log an error only if all attempts yield null—this keeps
identifier format consistent and prevents NullPointerException.
- Around line 106-120: AuthenticationEventHandler's user retrieval logic doesn't
handle TOKEN_EXCHANGED, leaving userName null for POST_ISSUE_ACCESS_TOKEN_V2
events; add a branch for TOKEN_EXCHANGED (check the TOKEN_EXCHANGED constant)
and populate userName the same way as AUTHENTICATION_SUCCESSFUL (use
context.getLastAuthenticatedUser().getAuthenticatedSubjectIdentifier()), and if
that is null fall back to checking context.getParameters() for
AUTHENTICATED_USER and LAST_LOGIN_FAILED_USER to extract getUserName(),
otherwise log an error as currently done.
- Around line 70-71: The code in AuthenticationEventHandler blindly casts the
value returned by event.getEventProperties().get("context") to
AuthenticationContext (contextObject → AuthenticationContext) which can throw
NPE or ClassCastException; update the handler to first check that contextObject
is not null and is an instance of AuthenticationContext before casting, and if
the check fails handle it gracefully (log a descriptive warning/error via the
class logger and return or skip processing) so only a valid
AuthenticationContext is used.
In
@financial-services-accelerator/components/org.wso2.financial.services.accelerator.identity.extensions/src/main/java/org/wso2/financial/services/accelerator/identity/extensions/filter/exception/FilterException.java:
- Around line 22-24: The JavaDoc for the FilterException class is incorrect/too
specific ("CDS filter exception"); update the class-level JavaDoc in
org.wso2.financial.services.accelerator.identity.extensions.filter.exception.FilterException
to describe it as a generic filter exception used by the identity extensions
package (e.g., "Generic filter exception for identity extension filters" and
brief description of purpose/usage). Ensure the updated comment matches the
class name and package semantics and remove any CDS-specific wording.
In
@financial-services-accelerator/components/org.wso2.financial.services.accelerator.identity.extensions/src/main/java/org/wso2/financial/services/accelerator/identity/extensions/filter/InfoSecDataPublishingFilter.java:
- Around line 160-179: In extractClientId, avoid calling replaceAll on possibly
null request parameters; first read oauthJwtAssertion and clientId using
Optional.ofNullable(request.getParameter(...)) and then map to
.replaceAll("[\r\n]","") (or use a null-safe utility) before further processing;
update the signedObject and clientIdAsReqParam initialization to use these
null-safe values, keep the SignedJWT.parse logic and fallback to clientId when
present, and ensure you treat empty strings as absent so no NullPointerException
occurs.
- Around line 56-60: The fields externalTrafficHeaderName and
expectedExternalTrafficHeaderValue in InfoSecDataPublishingFilter are assigned
from configMap without null checks, which can cause NPEs in
shouldPublishCurrentRequestData; add null-safety when reading the config (e.g.,
default to empty string or Optional) and update shouldPublishCurrentRequestData
to defensively handle null/empty header name or expected value before calling
request.getHeader or equals—use StringUtils.isNotBlank or explicit null checks
around externalTrafficHeaderName and expectedExternalTrafficHeaderValue and only
perform header comparison if both are non-null/non-empty.
- Around line 118-133: The Authorization header handling in
InfoSecDataPublishingFilter is unsafe: before doing authHeader.split(" ")[1] to
extract token, validate the header format (e.g., check authHeader != null &&
authHeader.startsWith("Bearer ") or split into parts and ensure parts.length >
1) and only then assign token; if the header is malformed, log a warning via LOG
(or handle gracefully) and leave consentId null (or set an explicit default) to
avoid ArrayIndexOutOfBoundsException; keep the existing SignedJWT parsing in the
try/catch (SignedJWT.parse(token)) unchanged but guard token extraction with the
above validation.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
financial-services-accelerator/accelerators/fs-is/carbon-home/repository/resources/conf/templates/repository/conf/financial-services.xml.j2 (1)
558-559: XML tag mismatch:<Token>closed with</UserName>.The opening tag is
<Token>but the closing tag is</UserName>. This will produce malformed XML.🔎 Proposed fix
{% if financial_services.extensions.endpoint.security.token is defined %} - <Token>{{financial_services.extensions.endpoint.security.token}}</UserName> + <Token>{{financial_services.extensions.endpoint.security.token}}</Token> {% endif %}financial-services-accelerator/components/org.wso2.financial.services.accelerator.consent.mgt.extensions/src/main/java/org/wso2/financial/services/accelerator/consent/mgt/extensions/authorize/util/ConsentAuthorizeUtil.java (1)
752-756: Malformed Javadoc comment - missing leading asterisk.The comment starts with
/*instead of/**, which means it won't be processed as Javadoc documentation.🔎 Proposed fix
- /* Builds authorized data object from account and permission parameters. + /** + * Builds authorized data object from account and permission parameters.
Comment on lines
+581
to
+583
| {% else %} | ||
| <Executor class="org.wso2.financial.services.accelerator.common.event.executor.DefaultOBEventExecutor"/> | ||
| {% endif %} |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find all references to executor class names
rg "DefaultOBEventExecutor|DefaultFSEventExecutor" -t java -t xml -t jinja --no-heading -nRepository: wso2/financial-services-accelerator
Length of output: 1453
🏁 Script executed:
#!/bin/bash
# Search for any definition or reference to DefaultOBEventExecutor
rg "class.*DefaultOBEventExecutor" --type java -nRepository: wso2/financial-services-accelerator
Length of output: 61
Update executor class name to match actual implementation.
The template references DefaultOBEventExecutor at line 582, but this class does not exist in the codebase. The actual executor implementation is DefaultFSEventExecutor, which is the only executor class defined. Update line 582 to use the correct class name:
<Executor class="org.wso2.financial.services.accelerator.common.event.executor.DefaultFSEventExecutor"/>This mismatch will cause runtime failures when the configuration is loaded.
🤖 Prompt for AI Agents
In
@financial-services-accelerator/accelerators/fs-is/carbon-home/repository/resources/conf/templates/repository/conf/financial-services.xml.j2
around lines 581-583, The executor class name in the template is incorrect:
replace the non-existent
org.wso2.financial.services.accelerator.common.event.executor.DefaultOBEventExecutor
with the actual implementation
org.wso2.financial.services.accelerator.common.event.executor.DefaultFSEventExecutor
so the configuration uses the correct executor class (update the <Executor
class="..."> entry to reference DefaultFSEventExecutor).
Comment on lines
+657
to
+666
| {% if attribute.required is defined %} | ||
| {% set required = "{{attribute.required}}" %} | ||
| {% else %} | ||
| {% set required = "false" %} | ||
| {% endif %} | ||
| {% if attribute.type is defined %} | ||
| {% set type = "{{attribute.type}}" %} | ||
| {% else %} | ||
| {% set type = "string" %} | ||
| {% endif %} |
There was a problem hiding this comment.
Incorrect Jinja2 syntax in {% set %} blocks.
Using "{{attribute.required}}" inside a {% set %} statement will produce the literal string "{{attribute.required}}" rather than the attribute's value. Jinja2 variables should be referenced directly without mustache syntax inside control statements.
🔎 Proposed fix
{% if attribute.required is defined %}
- {% set required = "{{attribute.required}}" %}
+ {% set required = attribute.required %}
{% else %}
{% set required = "false" %}
{% endif %}
{% if attribute.type is defined %}
- {% set type = "{{attribute.type}}" %}
+ {% set type = attribute.type %}
{% else %}
{% set type = "string" %}
{% endif %}🤖 Prompt for AI Agents
In
@financial-services-accelerator/accelerators/fs-is/carbon-home/repository/resources/conf/templates/repository/conf/financial-services.xml.j2
around lines 657-666, The {% set %} blocks are assigning the literal strings
"{{attribute.required}}" and "{{attribute.type}}"; change them to reference the
Jinja2 variables directly and without quotes/mustache syntax, e.g. use {% if
attribute.required is defined %}{% set required = attribute.required %}{% else
%}{% set required = false %}{% endif %} and similarly for attribute.type with {%
set type = attribute.type %} (or "string" as the fallback) so the variables
required and type receive the actual values.
Comment on lines
+46
to
+58
| <groupId>org.apache.synapse</groupId> | ||
| <artifactId>synapse-core</artifactId> | ||
| <exclusions> | ||
| <exclusion> | ||
| <groupId>org.apache.axis2</groupId> | ||
| <artifactId>axis2-codegen</artifactId> | ||
| </exclusion> | ||
| <exclusion> | ||
| <groupId>org.wso2.orbit.com.github.fge</groupId> | ||
| <artifactId>json-schema-validator-all</artifactId> | ||
| </exclusion> | ||
| </exclusions> | ||
| </dependency> |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
GHSA-p694-23q3-rvrc Apache Synapse RCE vulnerability details
💡 Result:
Short summary — GHSA-p694-23q3-rvrc
- Alias: CVE‑2017‑15708. [Published NVD: 2017‑12‑11; GitHub advisory record: 2020‑11‑04]. (osv.dev)
- Impact: Remote code execution (RCE) via unauthenticated Java RMI combined with deserialization gadget in Apache Commons Collections present in Synapse distributions. Attackers can send crafted serialized objects to the RMI endpoint to achieve code execution. (osv.dev)
- Affected versions: Apache Synapse 1.0 → 3.0.0 (listed: 1.0, 1.1, 1.1.1, 1.1.2, 1.2, 2.0.0, 2.1.0, 3.0.0). Fixed in 3.0.1. (osv.dev)
- Severity: CVSSv3 9.8 (Critical). (osv.dev)
- Root cause / exploitability: Default RMI service required no authentication; presence of commons‑collections 3.2.1 (or earlier) provided deserialization gadgets that enable RCE. (cve.circl.lu)
- Mitigation / remediation: Restrict/disable RMI access to trusted hosts, remove or upgrade vulnerable commons‑collections, or upgrade Synapse to 3.0.1 (which updates commons‑collections to 3.2.2). (osv.dev)
If you want, I can fetch the full NVD entry, Apache mailing‑list notice, or patch/commit that fixed the issue.
Citations:
- 1: https://osv.dev/vulnerability/GHSA-p694-23q3-rvrc?utm_source=openai
- 2: https://osv.dev/vulnerability/GHSA-p694-23q3-rvrc?utm_source=openai
- 3: https://osv.dev/vulnerability/GHSA-p694-23q3-rvrc?utm_source=openai
- 4: https://osv.dev/vulnerability/GHSA-p694-23q3-rvrc?utm_source=openai
- 5: https://cve.circl.lu/cve/CVE-2017-15708?utm_source=openai
- 6: https://osv.dev/vulnerability/GHSA-p694-23q3-rvrc?utm_source=openai
🌐 Web query:
Apache Synapse 2.1.7 wso2v217 GHSA-p694-23q3-rvrc patch status
💡 Result:
Short answer: GHSA-p694-23q3-rvrc is the GitHub alias for CVE-2017-15708 (RMI/Commons‑Collections RCE). WSO2’s published justification states their products ship a forked synapse-core (2.1.7.wso2v217) and they consider this CVE not exploitable in their products (RMI/JMX is authenticated or disabled), so they did not upgrade/patch synapse for that CVE. [1][2]
Sources:
- OSV / GitHub advisory (GHSA-p694-23q3-rvrc → CVE-2017-15708). [1]
- WSO2 justification / security note for CVE-2017-15708 (states forked synapse-core used and no synapse distribution; no upgrade performed). [2]
🌐 Web query:
Apache Synapse latest patched version 2.1.7 security updates
💡 Result:
Short answer: Apache Synapse’s current upstream stable release is 3.0.2 (released May 10, 2023). The 2.1.7 line is an older branch (commonly seen as synapse-core 2.1.7 in WSO2 forks) and contains known historical security issues (notably CVE-2017-15708). I recommend upgrading to the latest upstream release (3.0.2) or applying vendor-provided patched builds. [1][2][3]
Key references:
- Apache Synapse downloads / latest stable (3.0.2, 10‑May‑2023). [1]
- CVE-2017-15708 (RMI/JMX unauthenticated remote code execution affecting older Synapse releases). [2]
- WSO2 note / builds referencing synapse-core 2.1.7 (vendor forked packages / patched variants). [3]
If you want, I can:
- list all public CVEs affecting Synapse 2.x with links, or
- fetch specific changelog entries for the 2.1.7-wso2 patched package you’re using.
Upgrade synapse-core or explicitly verify RMI/JMX security configuration.
CVE-2017-15708 (GHSA-p694-23q3-rvrc) is a confirmed critical RCE vulnerability (CVSSv3 9.8) affecting Apache Synapse via unauthenticated Java RMI with commons-collections deserialization gadgets. The version synapse-core 2.1.7-wso2v217 is not patched for this CVE—WSO2 chose to mitigate it through configuration (disabling or authenticating RMI/JMX endpoints) rather than upgrading the library.
The library code itself remains vulnerable. Verify that:
- RMI/JMX endpoints are explicitly disabled or restricted to authenticated/trusted hosts in your deployment
- Or upgrade to Apache Synapse 3.0.1+ (current stable: 3.0.2, May 2023) which includes the fix
🤖 Prompt for AI Agents
In
@financial-services-accelerator/components/org.wso2.financial.services.accelerator.data.publisher/pom.xml
around lines 46-58, The pom declares the vulnerable dependency
org.apache.synapse:synapse-core (artifactId synapse-core) which is not patched
for CVE-2017-15708; either update that dependency to a patched release (upgrade
synapse-core to 3.0.1 or later, e.g., 3.0.2) in the pom.xml dependency block for
org.apache.synapse:synapse-core, or if you cannot upgrade, add a clear note and
ensure deployment-time mitigation by disabling or restricting Java RMI/JMX
endpoints (authenticate or bind to trusted hosts) in your Synapse/WSo2 runtime
configuration; apply one of these fixes and document it adjacent to the
synapse-core dependency so reviewers can verify the remediation.
Comment on lines
+106
to
+118
| <Import-Package> | ||
| org.osgi.framework; version="${osgi.framework.imp.pkg.version.range}", | ||
| org.osgi.service.component; version="${osgi.service.component.imp.pkg.version.range}", | ||
| org.wso2.carbon.databridge.agent.*; version="${carbon.analytics.common.version.range}", | ||
| org.wso2.carbon.databridge.commons.*; version="${carbon.analytics.common.version.range}", | ||
| org.wso2.financial.services.accelerator.common.*; version="${project.version}", | ||
| org.apache.commons.pool2.*; version="${commons-pool2.version}" | ||
| org.apache.synapse.*, | ||
| org.apache.commons.logging.*, | ||
| org.wso2.carbon.apimgt.common.analytics.*;version="${carbon.apimgt.import.version.range}", | ||
| org.wso2.carbon.apimgt.gateway.*;version="${carbon.apimgt.import.version.range}", | ||
| *;resolution:=optional | ||
| </Import-Package> |
There was a problem hiding this comment.
Fix OSGi Import-Package syntax error.
Line 112 is missing a comma after the commons-pool2 import, which will cause the OSGi bundle to fail:
org.apache.commons.pool2.*; version="${commons-pool2.version}"
org.apache.synapse.*,
Should be:
org.apache.commons.pool2.*; version="${commons-pool2.version}",
org.apache.synapse.*,
This is a critical issue that will prevent the bundle from being properly resolved.
🔎 Proposed fix
org.apache.commons.pool2.*; version="${commons-pool2.version}"
+ org.apache.commons.pool2.*; version="${commons-pool2.version}",
org.apache.synapse.*,📝 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
| <Import-Package> | |
| org.osgi.framework; version="${osgi.framework.imp.pkg.version.range}", | |
| org.osgi.service.component; version="${osgi.service.component.imp.pkg.version.range}", | |
| org.wso2.carbon.databridge.agent.*; version="${carbon.analytics.common.version.range}", | |
| org.wso2.carbon.databridge.commons.*; version="${carbon.analytics.common.version.range}", | |
| org.wso2.financial.services.accelerator.common.*; version="${project.version}", | |
| org.apache.commons.pool2.*; version="${commons-pool2.version}" | |
| org.apache.synapse.*, | |
| org.apache.commons.logging.*, | |
| org.wso2.carbon.apimgt.common.analytics.*;version="${carbon.apimgt.import.version.range}", | |
| org.wso2.carbon.apimgt.gateway.*;version="${carbon.apimgt.import.version.range}", | |
| *;resolution:=optional | |
| </Import-Package> | |
| <Import-Package> | |
| org.osgi.framework; version="${osgi.framework.imp.pkg.version.range}", | |
| org.osgi.service.component; version="${osgi.service.component.imp.pkg.version.range}", | |
| org.wso2.carbon.databridge.agent.*; version="${carbon.analytics.common.version.range}", | |
| org.wso2.carbon.databridge.commons.*; version="${carbon.analytics.common.version.range}", | |
| org.wso2.financial.services.accelerator.common.*; version="${project.version}", | |
| org.apache.commons.pool2.*; version="${commons-pool2.version}", | |
| org.apache.synapse.*, | |
| org.apache.commons.logging.*, | |
| org.wso2.carbon.apimgt.common.analytics.*;version="${carbon.apimgt.import.version.range}", | |
| org.wso2.carbon.apimgt.gateway.*;version="${carbon.apimgt.import.version.range}", | |
| *;resolution:=optional | |
| </Import-Package> |
🤖 Prompt for AI Agents
In
@financial-services-accelerator/components/org.wso2.financial.services.accelerator.data.publisher/pom.xml
around lines 106-118, The Import-Package block has a missing comma after the
org.apache.commons.pool2 entry which breaks OSGi parsing; edit the
Import-Package list (the XML element containing entries like
org.apache.commons.pool2.*; version="${commons-pool2.version}" and
org.apache.synapse.*,) and add a trailing comma after the
org.apache.commons.pool2.*; version="${commons-pool2.version}" entry so each
package entry is properly comma-separated; also scan the Import-Package entries
to ensure all items are separated by commas and no other punctuation errors
remain.
Comment on lines
+33
to
+34
| public class DataPublisherFactory<FinancialServicesDataPublisher> extends | ||
| BasePooledObjectFactory<FinancialServicesDataPublisher> { |
There was a problem hiding this comment.
Fix the incorrect generic type parameter declaration.
The generic type parameter is incorrectly declared as <FinancialServicesDataPublisher>, which uses the actual class name instead of a proper type variable. This shadows the intended class type and forces an unchecked cast on line 40. Generic type parameters should use single-letter convention (e.g., <T>) or descriptive names like <P>.
🔎 Proposed fix
-public class DataPublisherFactory<FinancialServicesDataPublisher> extends
- BasePooledObjectFactory<FinancialServicesDataPublisher> {
+public class DataPublisherFactory extends
+ BasePooledObjectFactory<FinancialServicesDataPublisher> {Then update the methods:
@Override
- public FinancialServicesDataPublisher create() {
+ public FinancialServicesDataPublisher create() {
log.debug("Creating new FinancialServicesDataPublisher instance");
- return (FinancialServicesDataPublisher) FSAnalyticsDataHolder.getInstance().getFinancialServicesDataPublisher();
+ return FSAnalyticsDataHolder.getInstance().getFinancialServicesDataPublisher();
}
@Override
- public PooledObject<FinancialServicesDataPublisher> wrap(FinancialServicesDataPublisher
+ public PooledObject<FinancialServicesDataPublisher> wrap(FinancialServicesDataPublisher
financialServicesDataPublisher) {🤖 Prompt for AI Agents
In
@financial-services-accelerator/components/org.wso2.financial.services.accelerator.data.publisher/src/main/java/org/wso2/financial/services/accelerator/data/publisher/DataPublisherFactory.java
around lines 33-34, The class mistakenly declares the generic parameter as the
concrete name "<FinancialServicesDataPublisher>", causing a shadow and unchecked
cast; change the class declaration to use a type variable (e.g.,
"DataPublisherFactory<T extends FinancialServicesDataPublisher> extends
BasePooledObjectFactory<T>") and update the factory methods (create(), wrap()
and any return types or fields referencing FinancialServicesDataPublisher) to
use T so you can return the proper generic type instead of performing the
unchecked cast to (FinancialServicesDataPublisher); ensure method signatures and
the pooled-object wrapper (wrap/instantiate methods) use T consistently.
| import static org.mockito.Mockito.when; | ||
|
|
||
|
|
||
| public class ParallelQueueQorkerTest { |
There was a problem hiding this comment.
Fix the typo in the class name.
The class name ParallelQueueQorkerTest contains a typo—it should be ParallelQueueWorkerTest (with "Worker" instead of "Qorker").
🔎 Proposed fix
-public class ParallelQueueQorkerTest {
+public class ParallelQueueWorkerTest {📝 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
| public class ParallelQueueQorkerTest { | |
| public class ParallelQueueWorkerTest { |
🤖 Prompt for AI Agents
In
@financial-services-accelerator/components/org.wso2.financial.services.accelerator.data.publisher/src/test/java/org/wso2/financial/services/accelerator/data/publisher/thrift/publisher/ParallelQueueQorkerTest.java
around line 19, Rename the class symbol ParallelQueueQorkerTest to
ParallelQueueWorkerTest to correct the typo; update the public class
declaration, the filename if it matches the class, any constructors or inner
references using ParallelQueueQorkerTest, and all references/imports/usages in
tests or suites to the new name so compilation and test discovery continue to
work.
| @@ -0,0 +1,29 @@ | |||
| package org.wso2.financial.services.accelerator.data.publisher.thrift.publisher; | |||
There was a problem hiding this comment.
Add the Apache 2.0 license header.
This file is missing the copyright and license header that is present in all other files in this repository. This is required for legal compliance and project consistency.
Add this header at the beginning of the file
+/*
+ * Copyright (c) 2025, WSO2 LLC. (http://www.wso2.org) All Rights Reserved.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ *
+ */
+
package org.wso2.financial.services.accelerator.data.publisher.thrift.publisher;📝 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
| package org.wso2.financial.services.accelerator.data.publisher.thrift.publisher; | |
| /* | |
| * Copyright (c) 2025, WSO2 LLC. (http://www.wso2.org) All Rights Reserved. | |
| * | |
| * Licensed under the Apache License, Version 2.0 (the "License"); | |
| * you may not use this file except in compliance with the License. | |
| * You may obtain a copy of the License at | |
| * | |
| * http://www.apache.org/licenses/LICENSE-2.0 | |
| * | |
| * Unless required by applicable law or agreed to in writing, software | |
| * distributed under the License is distributed on an "AS IS" BASIS, | |
| * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | |
| * See the License for the specific language governing permissions and | |
| * limitations under the License. | |
| * | |
| */ | |
| package org.wso2.financial.services.accelerator.data.publisher.thrift.publisher; |
🤖 Prompt for AI Agents
In
@financial-services-accelerator/components/org.wso2.financial.services.accelerator.data.publisher/src/test/java/org/wso2/financial/services/accelerator/data/publisher/thrift/publisher/ThriftMetricReporterTest.java
around line 1, The file ThriftMetricReporterTest is missing the project's Apache
2.0 license header; add the standard repository copyright and Apache 2.0 license
header block at the very top of the file (above the existing package declaration
in org.wso2.financial.services.accelerator.data.publisher.thrift.publisher) so
the test class ThriftMetricReporterTest matches other files' headers and
complies with project licensing.
| <classes> | ||
| <class name="org.wso2.financial.services.accelerator.data.publisher.FinancialServicesThriftDataPublisherTest"/> | ||
| <class name="org.wso2.financial.services.accelerator.data.publisher.DataPublisherPoolTest"/> | ||
| <class name="org.wso2.financial.services.accelerator.data.publisher.thrift.publisher.InputValidator"/> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Search for InputValidator class definition
rg -n --type java 'class\s+InputValidator' -A 15Repository: wso2/financial-services-accelerator
Length of output: 8082
🏁 Script executed:
# Also search for any InputValidator references in test directories
find . -type f -name "*InputValidator*" 2>/dev/nullRepository: wso2/financial-services-accelerator
Length of output: 492
Change to InputValidatorTest — wrong class referenced.
The testng.xml references InputValidator (a utility class with a private constructor in src/main/java), not the actual test class InputValidatorTest located in src/test/java. This will cause TestNG to fail during execution since it cannot instantiate a class with a private constructor and no test methods.
🤖 Prompt for AI Agents
In
@financial-services-accelerator/components/org.wso2.financial.services.accelerator.data.publisher/src/test/resources/testng.xml
around line 26, The testng.xml references the utility class InputValidator
(which has a private constructor) instead of the actual test class
InputValidatorTest; update the <class name="..."> entry in testng.xml to
reference
org.wso2.financial.services.accelerator.data.publisher.thrift.publisher.InputValidatorTest
so TestNG can instantiate and run the tests, ensuring the fully-qualified class
name matches the test class in src/test/java.
Comment on lines
+56
to
+60
| private final Map<String, Object> configMap = FinancialServicesConfigParser.getInstance().getConfiguration(); | ||
| private final String externalTrafficHeaderName = (String) configMap.get(FilterConstants | ||
| .EXTERNAL_TRAFFIC_HEADER_NAME); | ||
| private final String expectedExternalTrafficHeaderValue = (String) configMap.get(FilterConstants | ||
| .EXTERNAL_TRAFFIC_EXPECTED_VALUE); |
There was a problem hiding this comment.
Add null safety checks for configuration values.
The configuration values for externalTrafficHeaderName and expectedExternalTrafficHeaderValue are retrieved without null checks. If these configurations are missing, the fields will be null, causing NullPointerException at runtime (e.g., at lines 199-200 in shouldPublishCurrentRequestData).
🔎 Proposed fix
- private final Map<String, Object> configMap = FinancialServicesConfigParser.getInstance().getConfiguration();
- private final String externalTrafficHeaderName = (String) configMap.get(FilterConstants
- .EXTERNAL_TRAFFIC_HEADER_NAME);
- private final String expectedExternalTrafficHeaderValue = (String) configMap.get(FilterConstants
- .EXTERNAL_TRAFFIC_EXPECTED_VALUE);
+ private final Map<String, Object> configMap;
+ private final String externalTrafficHeaderName;
+ private final String expectedExternalTrafficHeaderValue;
+
+ public InfoSecDataPublishingFilter() {
+ this.configMap = FinancialServicesConfigParser.getInstance().getConfiguration();
+ this.externalTrafficHeaderName = (String) configMap.get(FilterConstants.EXTERNAL_TRAFFIC_HEADER_NAME);
+ this.expectedExternalTrafficHeaderValue = (String) configMap.get(FilterConstants.EXTERNAL_TRAFFIC_EXPECTED_VALUE);
+
+ if (externalTrafficHeaderName == null || expectedExternalTrafficHeaderValue == null) {
+ LOG.warn("External traffic configuration is missing. Filter will not publish data.");
+ }
+ }Then update shouldPublishCurrentRequestData to handle null:
public boolean shouldPublishCurrentRequestData(ServletRequest request) {
+ if (externalTrafficHeaderName == null || expectedExternalTrafficHeaderValue == null) {
+ return false;
+ }
+
// If the request is internal traffic, no need to publish data
if (request instanceof HttpServletRequest) {
+ String headerValue = ((HttpServletRequest) request).getHeader(externalTrafficHeaderName);
+ if (headerValue == null) {
+ return false;
+ }
return expectedExternalTrafficHeaderValue.equals(
- ((HttpServletRequest) request).getHeader(externalTrafficHeaderName).replaceAll("[\r\n]",
- ""));
+ headerValue.replaceAll("[\r\n]", ""));
}
return false;
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In
@financial-services-accelerator/components/org.wso2.financial.services.accelerator.identity.extensions/src/main/java/org/wso2/financial/services/accelerator/identity/extensions/filter/InfoSecDataPublishingFilter.java
around lines 56-60, The fields externalTrafficHeaderName and
expectedExternalTrafficHeaderValue in InfoSecDataPublishingFilter are assigned
from configMap without null checks, which can cause NPEs in
shouldPublishCurrentRequestData; add null-safety when reading the config (e.g.,
default to empty string or Optional) and update shouldPublishCurrentRequestData
to defensively handle null/empty header name or expected value before calling
request.getHeader or equals—use StringUtils.isNotBlank or explicit null checks
around externalTrafficHeaderName and expectedExternalTrafficHeaderValue and only
perform header comparison if both are non-null/non-empty.
Comment on lines
+160
to
+179
| private String extractClientId(HttpServletRequest request) { | ||
| String oauthJwtAssertion = request | ||
| .getParameter(FilterConstants.OAUTH_JWT_ASSERTION).replaceAll("[\r\n]", ""); | ||
| String clientId = request | ||
| .getParameter(FilterConstants.CLIENT_ID).replaceAll("[\r\n]", ""); | ||
| Optional<String> signedObject = Optional.ofNullable(oauthJwtAssertion); | ||
| Optional<String> clientIdAsReqParam = Optional.ofNullable(clientId); | ||
| if (signedObject.isPresent()) { | ||
| SignedJWT signedJWT = null; | ||
| try { | ||
| signedJWT = SignedJWT.parse(signedObject.get()); | ||
| return signedJWT.getJWTClaimsSet().getIssuer(); | ||
| } catch (ParseException e) { | ||
| LOG.error("Invalid assertion found in the request", e); | ||
| } | ||
| } else if (clientIdAsReqParam.isPresent()) { | ||
| return clientIdAsReqParam.get(); | ||
| } | ||
| return null; | ||
| } |
There was a problem hiding this comment.
Fix potential NullPointerException when extracting client ID.
Lines 162 and 164 call .replaceAll() on request parameters without null checks. If client_assertion or client_id parameters are missing, getParameter() returns null, causing a NullPointerException.
🔎 Proposed fix
private String extractClientId(HttpServletRequest request) {
- String oauthJwtAssertion = request
- .getParameter(FilterConstants.OAUTH_JWT_ASSERTION).replaceAll("[\r\n]", "");
- String clientId = request
- .getParameter(FilterConstants.CLIENT_ID).replaceAll("[\r\n]", "");
+ String oauthJwtAssertion = request.getParameter(FilterConstants.OAUTH_JWT_ASSERTION);
+ String clientId = request.getParameter(FilterConstants.CLIENT_ID);
+
+ if (oauthJwtAssertion != null) {
+ oauthJwtAssertion = oauthJwtAssertion.replaceAll("[\r\n]", "");
+ }
+ if (clientId != null) {
+ clientId = clientId.replaceAll("[\r\n]", "");
+ }
+
Optional<String> signedObject = Optional.ofNullable(oauthJwtAssertion);
Optional<String> clientIdAsReqParam = Optional.ofNullable(clientId);
if (signedObject.isPresent()) {
SignedJWT signedJWT = null;
try {
signedJWT = SignedJWT.parse(signedObject.get());
return signedJWT.getJWTClaimsSet().getIssuer();
} catch (ParseException e) {
LOG.error("Invalid assertion found in the request", e);
}
} else if (clientIdAsReqParam.isPresent()) {
return clientIdAsReqParam.get();
}
return 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
| private String extractClientId(HttpServletRequest request) { | |
| String oauthJwtAssertion = request | |
| .getParameter(FilterConstants.OAUTH_JWT_ASSERTION).replaceAll("[\r\n]", ""); | |
| String clientId = request | |
| .getParameter(FilterConstants.CLIENT_ID).replaceAll("[\r\n]", ""); | |
| Optional<String> signedObject = Optional.ofNullable(oauthJwtAssertion); | |
| Optional<String> clientIdAsReqParam = Optional.ofNullable(clientId); | |
| if (signedObject.isPresent()) { | |
| SignedJWT signedJWT = null; | |
| try { | |
| signedJWT = SignedJWT.parse(signedObject.get()); | |
| return signedJWT.getJWTClaimsSet().getIssuer(); | |
| } catch (ParseException e) { | |
| LOG.error("Invalid assertion found in the request", e); | |
| } | |
| } else if (clientIdAsReqParam.isPresent()) { | |
| return clientIdAsReqParam.get(); | |
| } | |
| return null; | |
| } | |
| private String extractClientId(HttpServletRequest request) { | |
| String oauthJwtAssertion = request.getParameter(FilterConstants.OAUTH_JWT_ASSERTION); | |
| String clientId = request.getParameter(FilterConstants.CLIENT_ID); | |
| if (oauthJwtAssertion != null) { | |
| oauthJwtAssertion = oauthJwtAssertion.replaceAll("[\r\n]", ""); | |
| } | |
| if (clientId != null) { | |
| clientId = clientId.replaceAll("[\r\n]", ""); | |
| } | |
| Optional<String> signedObject = Optional.ofNullable(oauthJwtAssertion); | |
| Optional<String> clientIdAsReqParam = Optional.ofNullable(clientId); | |
| if (signedObject.isPresent()) { | |
| SignedJWT signedJWT = null; | |
| try { | |
| signedJWT = SignedJWT.parse(signedObject.get()); | |
| return signedJWT.getJWTClaimsSet().getIssuer(); | |
| } catch (ParseException e) { | |
| LOG.error("Invalid assertion found in the request", e); | |
| } | |
| } else if (clientIdAsReqParam.isPresent()) { | |
| return clientIdAsReqParam.get(); | |
| } | |
| return null; | |
| } |
🤖 Prompt for AI Agents
In
@financial-services-accelerator/components/org.wso2.financial.services.accelerator.identity.extensions/src/main/java/org/wso2/financial/services/accelerator/identity/extensions/filter/InfoSecDataPublishingFilter.java
around lines 160-179, In extractClientId, avoid calling replaceAll on possibly
null request parameters; first read oauthJwtAssertion and clientId using
Optional.ofNullable(request.getParameter(...)) and then map to
.replaceAll("[\r\n]","") (or use a null-safe utility) before further processing;
update the signedObject and clientIdAsReqParam initialization to use these
null-safe values, keep the SignedJWT.parse logic and fallback to clientId when
present, and ensure you treat empty strings as absent so no NullPointerException
occurs.
There was a problem hiding this comment.
Actionable comments posted: 1
Fix all issues with AI Agents 🤖
In
@financial-services-accelerator/components/org.wso2.financial.services.accelerator.identity.extensions/src/main/resources/findbugs-exclude.xml:
- Around line 21-24: Add an XML comment above the existing <Match> for class
org.wso2.financial.services.accelerator.identity.extensions.auth.extensions.request.validator.FSRequestObjectValidationExtension
in the findbugs-exclude.xml explaining that the CRLF_INJECTION_LOGS finding is
intentionally excluded because the FSRequestObjectValidationExtension sanitizes
CRLF characters before logging (it uses .replaceAll("[\r\n]+", " ") to strip
carriage returns and newlines), so future maintainers understand the mitigation.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
financial-services-accelerator/components/org.wso2.financial.services.accelerator.identity.extensions/src/main/resources/findbugs-exclude.xml
Comment on lines
+21
to
+24
| <Match> | ||
| <Class name="org.wso2.financial.services.accelerator.identity.extensions.auth.extensions.request.validator.FSRequestObjectValidationExtension"/> | ||
| <Bug pattern="CRLF_INJECTION_LOGS"/> | ||
| </Match> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
echo "=== Searching for FSRequestObjectValidationExtension class ==="
fd -e java FSRequestObjectValidationExtension
echo -e "\n=== Checking findbugs-exclude.xml content ==="
cat -n financial-services-accelerator/components/org.wso2.financial.services.accelerator.identity.extensions/src/main/resources/findbugs-exclude.xml | head -30Repository: wso2/financial-services-accelerator
Length of output: 1823
🏁 Script executed:
#!/bin/bash
echo "=== Reading FSRequestObjectValidationExtension.java ==="
wc -l financial-services-accelerator/components/org.wso2.financial.services.accelerator.identity.extensions/src/main/java/org/wso2/financial/services/accelerator/identity/extensions/auth/extensions/request/validator/FSRequestObjectValidationExtension.java
echo -e "\n=== Examining the class ==="
cat -n financial-services-accelerator/components/org.wso2.financial.services.accelerator.identity.extensions/src/main/java/org/wso2/financial/services/accelerator/identity/extensions/auth/extensions/request/validator/FSRequestObjectValidationExtension.javaRepository: wso2/financial-services-accelerator
Length of output: 8384
Document the FindBugs exclusion with explanation of the existing CRLF sanitization.
The CRLF_INJECTION_LOGS exclusion for FSRequestObjectValidationExtension is justified—the code already sanitizes CRLF characters before logging. At lines 120–121, the violation message explicitly strips carriage returns and newlines using .replaceAll("[\r\n]+", " ") before logging. However, add an XML comment explaining this mitigation to clarify the exclusion for future maintainers:
<Match>
<!-- CRLF injection is mitigated: violation messages are sanitized at line 120 -->
<Class name="org.wso2.financial.services.accelerator.identity.extensions.auth.extensions.request.validator.FSRequestObjectValidationExtension"/>
<Bug pattern="CRLF_INJECTION_LOGS"/>
</Match>🤖 Prompt for AI Agents
In
@financial-services-accelerator/components/org.wso2.financial.services.accelerator.identity.extensions/src/main/resources/findbugs-exclude.xml
around lines 21-24, Add an XML comment above the existing <Match> for class
org.wso2.financial.services.accelerator.identity.extensions.auth.extensions.request.validator.FSRequestObjectValidationExtension
in the findbugs-exclude.xml explaining that the CRLF_INJECTION_LOGS finding is
intentionally excluded because the FSRequestObjectValidationExtension sanitizes
CRLF characters before logging (it uses .replaceAll("[\r\n]+", " ") to strip
carriage returns and newlines), so future maintainers understand the mitigation.
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.
Pull Request Title
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
Tests
✏️ Tip: You can customize this high-level summary in your review settings.