Introduce fraud detection configuration support with ConfigsAPI #1045
Conversation
WalkthroughAdds tenant-level fraud detection configuration: new provided dependencies, OSGi wiring for FraudDetectionConfigsService, service constructor injection and API methods, factory wiring, REST endpoints and OpenAPI schemas, plus two new error codes. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant API as ConfigsApiServiceImpl
participant Service as ServerConfigManagementService
participant Core as FraudDetectionConfigsService
Client->>API: GET /configs/fraud-detection
API->>Service: getFraudDetectionConfigs()
Service->>Core: fetch tenant fraud detection config
Core-->>Service: FraudDetectionConfigDTO
Service-->>API: FraudDetectionConfig (mapped)
API-->>Client: 200 OK
Client->>API: PUT /configs/fraud-detection {payload}
API->>Service: updateFraudDetectionConfigs(payload)
Service->>Core: update tenant fraud detection config (DTO)
Core-->>Service: updated FraudDetectionConfigDTO
Service-->>API: FraudDetectionConfig (mapped)
API-->>Client: 202/200 Accepted
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (5)
📒 Files selected for processing (9)
🚧 Files skipped from review as they are similar to previous changes (3)
🧰 Additional context used🧬 Code graph analysis (3)components/org.wso2.carbon.identity.api.server.configs/org.wso2.carbon.identity.api.server.configs.v1/src/main/java/org/wso2/carbon/identity/api/server/configs/v1/impl/ConfigsApiServiceImpl.java (1)
components/org.wso2.carbon.identity.api.server.configs/org.wso2.carbon.identity.api.server.configs.v1/src/main/java/org/wso2/carbon/identity/api/server/configs/v1/core/ServerConfigManagementService.java (4)
components/org.wso2.carbon.identity.api.server.configs/org.wso2.carbon.identity.api.server.configs.v1/src/main/java/org/wso2/carbon/identity/api/server/configs/v1/factories/ServerConfigManagementServiceFactory.java (1)
🔇 Additional comments (6)
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}
| public FraudDetectionConfig getFraudDetectionConfigs() { | ||
|
|
||
| String tenantDomain = ContextLoader.getTenantDomainFromContext(); |
There was a problem hiding this comment.
Log Improvement Suggestion No: 1
| public FraudDetectionConfig getFraudDetectionConfigs() { | |
| String tenantDomain = ContextLoader.getTenantDomainFromContext(); | |
| public FraudDetectionConfig getFraudDetectionConfigs() { | |
| String tenantDomain = ContextLoader.getTenantDomainFromContext(); | |
| log.info("Retrieving fraud detection configurations for tenant: " + tenantDomain); |
| public FraudDetectionConfig updateFraudDetectionConfigs(FraudDetectionConfig fraudDetectionConfig) { | ||
|
|
||
| String tenantDomain = ContextLoader.getTenantDomainFromContext(); |
There was a problem hiding this comment.
Log Improvement Suggestion No: 2
| public FraudDetectionConfig updateFraudDetectionConfigs(FraudDetectionConfig fraudDetectionConfig) { | |
| String tenantDomain = ContextLoader.getTenantDomainFromContext(); | |
| public FraudDetectionConfig updateFraudDetectionConfigs(FraudDetectionConfig fraudDetectionConfig) { | |
| String tenantDomain = ContextLoader.getTenantDomainFromContext(); | |
| log.info("Updating fraud detection configurations for tenant: " + tenantDomain); |
| if (fraudDetectionConfigsService == null) { | ||
| throw new IllegalStateException("FraudDetectionConfigsService is not available from OSGi context."); | ||
| } |
There was a problem hiding this comment.
Log Improvement Suggestion No: 3
| if (fraudDetectionConfigsService == null) { | |
| throw new IllegalStateException("FraudDetectionConfigsService is not available from OSGi context."); | |
| } | |
| if (fraudDetectionConfigsService == null) { | |
| log.error("FraudDetectionConfigsService is not available from OSGi context."); | |
| throw new IllegalStateException("FraudDetectionConfigsService is not available from OSGi context."); | |
| } |
| dcrConfigurationMgtService, | ||
| jwtClientAuthenticatorMgtService); | ||
| jwtClientAuthenticatorMgtService, | ||
| fraudDetectionConfigsService); |
There was a problem hiding this comment.
Log Improvement Suggestion No: 4
| fraudDetectionConfigsService); | |
| fraudDetectionConfigsService); | |
| log.info("ServerConfigManagementService initialized successfully with all required services."); |
| @Override | ||
| public Response getFraudDetectionConfigs() { | ||
|
|
||
| return Response.ok().entity(configManagementService.getFraudDetectionConfigs()).build(); | ||
| } |
There was a problem hiding this comment.
Log Improvement Suggestion No: 5
| @Override | |
| public Response getFraudDetectionConfigs() { | |
| return Response.ok().entity(configManagementService.getFraudDetectionConfigs()).build(); | |
| } | |
| @Override | |
| public Response getFraudDetectionConfigs() { | |
| log.debug("Fetching fraud detection configurations."); | |
| return Response.ok().entity(configManagementService.getFraudDetectionConfigs()).build(); | |
| } |
| @Override | ||
| public Response updateFraudDetectionConfigs(FraudDetectionConfig fraudDetectionConfig) { | ||
|
|
||
| return Response.ok().entity(configManagementService.updateFraudDetectionConfigs(fraudDetectionConfig)).build(); | ||
| } |
There was a problem hiding this comment.
Log Improvement Suggestion No: 6
| @Override | |
| public Response updateFraudDetectionConfigs(FraudDetectionConfig fraudDetectionConfig) { | |
| return Response.ok().entity(configManagementService.updateFraudDetectionConfigs(fraudDetectionConfig)).build(); | |
| } | |
| @Override | |
| public Response updateFraudDetectionConfigs(FraudDetectionConfig fraudDetectionConfig) { | |
| log.info("Updating fraud detection configurations."); | |
| return Response.ok().entity(configManagementService.updateFraudDetectionConfigs(fraudDetectionConfig)).build(); | |
| } |
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: 7
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (5)
components/org.wso2.carbon.identity.api.server.configs/org.wso2.carbon.identity.api.server.configs.v1/src/gen/java/org/wso2/carbon/identity/api/server/configs/v1/ConfigsApi.javais excluded by!**/gen/**components/org.wso2.carbon.identity.api.server.configs/org.wso2.carbon.identity.api.server.configs.v1/src/gen/java/org/wso2/carbon/identity/api/server/configs/v1/ConfigsApiService.javais excluded by!**/gen/**components/org.wso2.carbon.identity.api.server.configs/org.wso2.carbon.identity.api.server.configs.v1/src/gen/java/org/wso2/carbon/identity/api/server/configs/v1/model/EventConfig.javais excluded by!**/gen/**components/org.wso2.carbon.identity.api.server.configs/org.wso2.carbon.identity.api.server.configs.v1/src/gen/java/org/wso2/carbon/identity/api/server/configs/v1/model/EventProperty.javais excluded by!**/gen/**components/org.wso2.carbon.identity.api.server.configs/org.wso2.carbon.identity.api.server.configs.v1/src/gen/java/org/wso2/carbon/identity/api/server/configs/v1/model/FraudDetectionConfig.javais excluded by!**/gen/**
📒 Files selected for processing (9)
components/org.wso2.carbon.identity.api.server.configs/org.wso2.carbon.identity.api.server.configs.common/pom.xml(1 hunks)components/org.wso2.carbon.identity.api.server.configs/org.wso2.carbon.identity.api.server.configs.common/src/main/java/org/wso2/carbon/identity/api/server/configs/common/ConfigsServiceHolder.java(3 hunks)components/org.wso2.carbon.identity.api.server.configs/org.wso2.carbon.identity.api.server.configs.common/src/main/java/org/wso2/carbon/identity/api/server/configs/common/Constants.java(1 hunks)components/org.wso2.carbon.identity.api.server.configs/org.wso2.carbon.identity.api.server.configs.v1/pom.xml(1 hunks)components/org.wso2.carbon.identity.api.server.configs/org.wso2.carbon.identity.api.server.configs.v1/src/main/java/org/wso2/carbon/identity/api/server/configs/v1/core/ServerConfigManagementService.java(7 hunks)components/org.wso2.carbon.identity.api.server.configs/org.wso2.carbon.identity.api.server.configs.v1/src/main/java/org/wso2/carbon/identity/api/server/configs/v1/factories/ServerConfigManagementServiceFactory.java(3 hunks)components/org.wso2.carbon.identity.api.server.configs/org.wso2.carbon.identity.api.server.configs.v1/src/main/java/org/wso2/carbon/identity/api/server/configs/v1/impl/ConfigsApiServiceImpl.java(3 hunks)components/org.wso2.carbon.identity.api.server.configs/org.wso2.carbon.identity.api.server.configs.v1/src/main/resources/configs.yaml(2 hunks)pom.xml(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
components/org.wso2.carbon.identity.api.server.configs/org.wso2.carbon.identity.api.server.configs.v1/src/main/java/org/wso2/carbon/identity/api/server/configs/v1/core/ServerConfigManagementService.java (5)
components/org.wso2.carbon.identity.api.server.configs/org.wso2.carbon.identity.api.server.configs.v1/src/gen/java/org/wso2/carbon/identity/api/server/configs/v1/model/EventConfig.java (1)
EventConfig(36-153)components/org.wso2.carbon.identity.api.server.configs/org.wso2.carbon.identity.api.server.configs.v1/src/gen/java/org/wso2/carbon/identity/api/server/configs/v1/model/EventProperty.java (1)
EventProperty(33-120)components/org.wso2.carbon.identity.api.server.configs/org.wso2.carbon.identity.api.server.configs.v1/src/gen/java/org/wso2/carbon/identity/api/server/configs/v1/model/FraudDetectionConfig.java (1)
FraudDetectionConfig(36-175)components/org.wso2.carbon.identity.api.server.common/src/main/java/org/wso2/carbon/identity/api/server/common/ContextLoader.java (1)
ContextLoader(42-146)components/org.wso2.carbon.identity.api.server.configs/org.wso2.carbon.identity.api.server.configs.common/src/main/java/org/wso2/carbon/identity/api/server/configs/common/Constants.java (1)
Constants(22-252)
🔇 Additional comments (9)
components/org.wso2.carbon.identity.api.server.configs/org.wso2.carbon.identity.api.server.configs.v1/pom.xml (1)
188-192: LGTM - Dependency addition follows conventions.The fraud detection core dependency is correctly added with
providedscope, which is appropriate for OSGi runtime dependencies.pom.xml (1)
548-553: LGTM - Dependency management follows standard patterns.The fraud detection core dependency is correctly declared in dependency management with appropriate version property reference and scope.
components/org.wso2.carbon.identity.api.server.configs/org.wso2.carbon.identity.api.server.configs.v1/src/main/resources/configs.yaml (1)
1586-1630: LGTM - Schema definitions are well-structured.The
FraudDetectionConfig,EventConfig, andEventPropertyschemas are clearly defined with appropriate descriptions and examples. The nested structure logically represents the fraud detection configuration hierarchy.components/org.wso2.carbon.identity.api.server.configs/org.wso2.carbon.identity.api.server.configs.common/pom.xml (1)
85-89: LGTM - Dependency addition is consistent.The fraud detection core dependency is appropriately added with
providedscope, following the same pattern as other framework dependencies in this module.components/org.wso2.carbon.identity.api.server.configs/org.wso2.carbon.identity.api.server.configs.v1/src/main/java/org/wso2/carbon/identity/api/server/configs/v1/factories/ServerConfigManagementServiceFactory.java (1)
7-7: LGTM - Service wiring follows established patterns.The
FraudDetectionConfigsServiceis wired consistently with the existing services:
- Retrieved from
ConfigsServiceHolder- Validated with a null check
- Passed to the
ServerConfigManagementServiceconstructorThe implementation follows the same pattern as the other seven services in this factory.
Also applies to: 34-35, 65-67, 74-75
components/org.wso2.carbon.identity.api.server.configs/org.wso2.carbon.identity.api.server.configs.v1/src/main/java/org/wso2/carbon/identity/api/server/configs/v1/impl/ConfigsApiServiceImpl.java (1)
135-139: LGTM - Implementation follows existing endpoint patterns.Both endpoints correctly delegate to
configManagementServicefor business logic, which is consistent with other endpoints in this class.Note: There's a response status code mismatch between the OpenAPI spec (202) and this implementation (200), which has been flagged separately in the
configs.yamlreview.Also applies to: 230-234
components/org.wso2.carbon.identity.api.server.configs/org.wso2.carbon.identity.api.server.configs.common/src/main/java/org/wso2/carbon/identity/api/server/configs/common/ConfigsServiceHolder.java (1)
26-26: LGTM - Service holder follows OSGi service access patterns.The
FraudDetectionConfigsServiceholder implementation is consistent with the other nine service holders in this class:
- Uses the standard static inner class pattern for lazy initialization
- Retrieves the service via
PrivilegedCarbonContextOSGi lookup- Provides a public getter with appropriate JavaDoc
Also applies to: 98-103, 195-203
components/org.wso2.carbon.identity.api.server.configs/org.wso2.carbon.identity.api.server.configs.v1/src/main/java/org/wso2/carbon/identity/api/server/configs/v1/core/ServerConfigManagementService.java (2)
153-153: LGTM: Service wiring follows established patterns.The fraud detection service is properly injected via constructor and stored as a final field, consistent with other service dependencies in this class.
Also applies to: 169-170, 179-179
2115-2152: LGTM: Exception handler follows established patterns.The
handleFraudDetectionConfigException()method correctly handles both client and server exceptions, maps them to appropriate HTTP status codes, and follows the same pattern as other exception handlers in this class.
...nfigs.common/src/main/java/org/wso2/carbon/identity/api/server/configs/common/Constants.java
Outdated
Show resolved
Hide resolved
| public FraudDetectionConfig getFraudDetectionConfigs() { | ||
|
|
||
| String tenantDomain = ContextLoader.getTenantDomainFromContext(); | ||
| try { | ||
| FraudDetectionConfigDTO fraudDetectionConfigDTO | ||
| = fraudDetectionConfigsService.getFraudDetectionConfigs(tenantDomain); | ||
| return buildFraudDetectionConfig(fraudDetectionConfigDTO); | ||
| } catch (FraudDetectionConfigServerException e) { | ||
| throw handleFraudDetectionConfigException(e, | ||
| Constants.ErrorMessage.ERROR_CODE_FRAUD_DETECTION_CONFIG_RETRIEVE, null); | ||
| } | ||
| } |
There was a problem hiding this comment.
Catch the base exception type, not just server exceptions.
The catch block only handles FraudDetectionConfigServerException. If FraudDetectionConfigClientException is thrown (e.g., validation errors), it won't be caught, resulting in unhandled exceptions.
🔎 Apply this diff to catch all fraud detection exceptions:
public FraudDetectionConfig getFraudDetectionConfigs() {
String tenantDomain = ContextLoader.getTenantDomainFromContext();
try {
FraudDetectionConfigDTO fraudDetectionConfigDTO
= fraudDetectionConfigsService.getFraudDetectionConfigs(tenantDomain);
return buildFraudDetectionConfig(fraudDetectionConfigDTO);
- } catch (FraudDetectionConfigServerException e) {
+ } catch (IdentityFraudDetectionException e) {
throw handleFraudDetectionConfigException(e,
Constants.ErrorMessage.ERROR_CODE_FRAUD_DETECTION_CONFIG_RETRIEVE, 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.
| public FraudDetectionConfig getFraudDetectionConfigs() { | |
| String tenantDomain = ContextLoader.getTenantDomainFromContext(); | |
| try { | |
| FraudDetectionConfigDTO fraudDetectionConfigDTO | |
| = fraudDetectionConfigsService.getFraudDetectionConfigs(tenantDomain); | |
| return buildFraudDetectionConfig(fraudDetectionConfigDTO); | |
| } catch (FraudDetectionConfigServerException e) { | |
| throw handleFraudDetectionConfigException(e, | |
| Constants.ErrorMessage.ERROR_CODE_FRAUD_DETECTION_CONFIG_RETRIEVE, null); | |
| } | |
| } | |
| public FraudDetectionConfig getFraudDetectionConfigs() { | |
| String tenantDomain = ContextLoader.getTenantDomainFromContext(); | |
| try { | |
| FraudDetectionConfigDTO fraudDetectionConfigDTO | |
| = fraudDetectionConfigsService.getFraudDetectionConfigs(tenantDomain); | |
| return buildFraudDetectionConfig(fraudDetectionConfigDTO); | |
| } catch (IdentityFraudDetectionException e) { | |
| throw handleFraudDetectionConfigException(e, | |
| Constants.ErrorMessage.ERROR_CODE_FRAUD_DETECTION_CONFIG_RETRIEVE, null); | |
| } | |
| } |
🤖 Prompt for AI Agents
In
components/org.wso2.carbon.identity.api.server.configs/org.wso2.carbon.identity.api.server.configs.v1/src/main/java/org/wso2/carbon/identity/api/server/configs/v1/core/ServerConfigManagementService.java
around lines 2034-2045, the current catch only handles
FraudDetectionConfigServerException so FraudDetectionConfigClientException
(validation/client errors) can escape; change the catch to the common base
exception (e.g., FraudDetectionConfigException) so all fraud-detection-related
exceptions are caught and then call the existing
handleFraudDetectionConfigException(...) with the caught exception and the same
error code/context to rethrow a properly handled API error.
| public FraudDetectionConfig updateFraudDetectionConfigs(FraudDetectionConfig fraudDetectionConfig) { | ||
|
|
||
| String tenantDomain = ContextLoader.getTenantDomainFromContext(); | ||
| try { | ||
| FraudDetectionConfigDTO fraudDetectionConfigDTO = | ||
| fraudDetectionConfigsService.updateFraudDetectionConfigs( | ||
| buildFraudDetectionConfigDTO(fraudDetectionConfig), tenantDomain); | ||
| return buildFraudDetectionConfig(fraudDetectionConfigDTO); | ||
| } catch (FraudDetectionConfigServerException e) { | ||
| throw handleFraudDetectionConfigException(e, | ||
| Constants.ErrorMessage.ERROR_CODE_FRAUD_DETECTION_CONFIG_UPDATE, null); | ||
| } | ||
| } |
There was a problem hiding this comment.
Catch the base exception type, not just server exceptions.
Same issue as getFraudDetectionConfigs() - only FraudDetectionConfigServerException is caught, leaving client exceptions unhandled.
🔎 Apply this diff to catch all fraud detection exceptions:
public FraudDetectionConfig updateFraudDetectionConfigs(FraudDetectionConfig fraudDetectionConfig) {
String tenantDomain = ContextLoader.getTenantDomainFromContext();
try {
FraudDetectionConfigDTO fraudDetectionConfigDTO =
fraudDetectionConfigsService.updateFraudDetectionConfigs(
buildFraudDetectionConfigDTO(fraudDetectionConfig), tenantDomain);
return buildFraudDetectionConfig(fraudDetectionConfigDTO);
- } catch (FraudDetectionConfigServerException e) {
+ } catch (IdentityFraudDetectionException e) {
throw handleFraudDetectionConfigException(e,
Constants.ErrorMessage.ERROR_CODE_FRAUD_DETECTION_CONFIG_UPDATE, null);
}
}🤖 Prompt for AI Agents
In
components/org.wso2.carbon.identity.api.server.configs/org.wso2.carbon.identity.api.server.configs.v1/src/main/java/org/wso2/carbon/identity/api/server/configs/v1/core/ServerConfigManagementService.java
around lines 2047 to 2059, the updateFraudDetectionConfigs method currently only
catches FraudDetectionConfigServerException leaving client-side exceptions
unhandled; change the catch clause to catch the base
FraudDetectionConfigException (the common superclass for server and client fraud
detection exceptions) and pass that exception into
handleFraudDetectionConfigException exactly as before so both client and server
errors are handled uniformly.
| private FraudDetectionConfig buildFraudDetectionConfig(FraudDetectionConfigDTO dto) { | ||
|
|
||
| FraudDetectionConfig fraudDetectionConfig = new FraudDetectionConfig(); | ||
| fraudDetectionConfig.setPublishUserInfo(dto.isPublishUserInfo()); | ||
| fraudDetectionConfig.setPublishDeviceMetadata(dto.isPublishDeviceMetadata()); | ||
| fraudDetectionConfig.setLogRequestPayload(dto.isLogRequestPayload()); | ||
|
|
||
| List<EventConfig> eventConfigs = new ArrayList<>(); | ||
| dto.getEvents().forEach((eventName, eventConfigDTO) -> { | ||
|
|
||
| List<EventProperty> eventProperties = new ArrayList<>(); | ||
| eventConfigDTO.getProperties().forEach((key, value) -> { | ||
|
|
||
| EventProperty eventProperty = new EventProperty(); | ||
| eventProperty.setPropertyKey(key); | ||
| eventProperty.setPropertyValue(value); | ||
| eventProperties.add(eventProperty); | ||
| }); | ||
|
|
||
| EventConfig eventConfig = new EventConfig(); | ||
| eventConfig.setEventName(eventName); | ||
| eventConfig.setEnabled(eventConfigDTO.isEnabled()); | ||
| eventConfig.setProperties(eventProperties); | ||
| eventConfigs.add(eventConfig); | ||
| }); | ||
|
|
||
| fraudDetectionConfig.setEvents(eventConfigs); | ||
| return fraudDetectionConfig; | ||
| } |
There was a problem hiding this comment.
Add null safety checks for collections.
The method calls forEach() on dto.getEvents() (line 2069) and eventConfigDTO.getProperties() (line 2072) without null checks, which will throw NPE if these collections are null.
🔎 Apply this diff to add defensive null checks:
private FraudDetectionConfig buildFraudDetectionConfig(FraudDetectionConfigDTO dto) {
FraudDetectionConfig fraudDetectionConfig = new FraudDetectionConfig();
fraudDetectionConfig.setPublishUserInfo(dto.isPublishUserInfo());
fraudDetectionConfig.setPublishDeviceMetadata(dto.isPublishDeviceMetadata());
fraudDetectionConfig.setLogRequestPayload(dto.isLogRequestPayload());
List<EventConfig> eventConfigs = new ArrayList<>();
- dto.getEvents().forEach((eventName, eventConfigDTO) -> {
+ Map<String, EventConfigDTO> events = dto.getEvents();
+ if (events != null) {
+ events.forEach((eventName, eventConfigDTO) -> {
- List<EventProperty> eventProperties = new ArrayList<>();
- eventConfigDTO.getProperties().forEach((key, value) -> {
+ List<EventProperty> eventProperties = new ArrayList<>();
+ Map<String, String> properties = eventConfigDTO.getProperties();
+ if (properties != null) {
+ properties.forEach((key, value) -> {
- EventProperty eventProperty = new EventProperty();
- eventProperty.setPropertyKey(key);
- eventProperty.setPropertyValue(value);
- eventProperties.add(eventProperty);
- });
+ EventProperty eventProperty = new EventProperty();
+ eventProperty.setPropertyKey(key);
+ eventProperty.setPropertyValue(value);
+ eventProperties.add(eventProperty);
+ });
+ }
- EventConfig eventConfig = new EventConfig();
- eventConfig.setEventName(eventName);
- eventConfig.setEnabled(eventConfigDTO.isEnabled());
- eventConfig.setProperties(eventProperties);
- eventConfigs.add(eventConfig);
- });
+ EventConfig eventConfig = new EventConfig();
+ eventConfig.setEventName(eventName);
+ eventConfig.setEnabled(eventConfigDTO.isEnabled());
+ eventConfig.setProperties(eventProperties);
+ eventConfigs.add(eventConfig);
+ });
+ }
fraudDetectionConfig.setEvents(eventConfigs);
return fraudDetectionConfig;
}📝 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.
| private FraudDetectionConfig buildFraudDetectionConfig(FraudDetectionConfigDTO dto) { | |
| FraudDetectionConfig fraudDetectionConfig = new FraudDetectionConfig(); | |
| fraudDetectionConfig.setPublishUserInfo(dto.isPublishUserInfo()); | |
| fraudDetectionConfig.setPublishDeviceMetadata(dto.isPublishDeviceMetadata()); | |
| fraudDetectionConfig.setLogRequestPayload(dto.isLogRequestPayload()); | |
| List<EventConfig> eventConfigs = new ArrayList<>(); | |
| dto.getEvents().forEach((eventName, eventConfigDTO) -> { | |
| List<EventProperty> eventProperties = new ArrayList<>(); | |
| eventConfigDTO.getProperties().forEach((key, value) -> { | |
| EventProperty eventProperty = new EventProperty(); | |
| eventProperty.setPropertyKey(key); | |
| eventProperty.setPropertyValue(value); | |
| eventProperties.add(eventProperty); | |
| }); | |
| EventConfig eventConfig = new EventConfig(); | |
| eventConfig.setEventName(eventName); | |
| eventConfig.setEnabled(eventConfigDTO.isEnabled()); | |
| eventConfig.setProperties(eventProperties); | |
| eventConfigs.add(eventConfig); | |
| }); | |
| fraudDetectionConfig.setEvents(eventConfigs); | |
| return fraudDetectionConfig; | |
| } | |
| private FraudDetectionConfig buildFraudDetectionConfig(FraudDetectionConfigDTO dto) { | |
| FraudDetectionConfig fraudDetectionConfig = new FraudDetectionConfig(); | |
| fraudDetectionConfig.setPublishUserInfo(dto.isPublishUserInfo()); | |
| fraudDetectionConfig.setPublishDeviceMetadata(dto.isPublishDeviceMetadata()); | |
| fraudDetectionConfig.setLogRequestPayload(dto.isLogRequestPayload()); | |
| List<EventConfig> eventConfigs = new ArrayList<>(); | |
| Map<String, EventConfigDTO> events = dto.getEvents(); | |
| if (events != null) { | |
| events.forEach((eventName, eventConfigDTO) -> { | |
| List<EventProperty> eventProperties = new ArrayList<>(); | |
| Map<String, String> properties = eventConfigDTO.getProperties(); | |
| if (properties != null) { | |
| properties.forEach((key, value) -> { | |
| EventProperty eventProperty = new EventProperty(); | |
| eventProperty.setPropertyKey(key); | |
| eventProperty.setPropertyValue(value); | |
| eventProperties.add(eventProperty); | |
| }); | |
| } | |
| EventConfig eventConfig = new EventConfig(); | |
| eventConfig.setEventName(eventName); | |
| eventConfig.setEnabled(eventConfigDTO.isEnabled()); | |
| eventConfig.setProperties(eventProperties); | |
| eventConfigs.add(eventConfig); | |
| }); | |
| } | |
| fraudDetectionConfig.setEvents(eventConfigs); | |
| return fraudDetectionConfig; | |
| } |
🤖 Prompt for AI Agents
In
components/org.wso2.carbon.identity.api.server.configs/org.wso2.carbon.identity.api.server.configs.v1/src/main/java/org/wso2/carbon/identity/api/server/configs/v1/core/ServerConfigManagementService.java
around lines 2061 to 2089, the code calls forEach() on dto.getEvents() and
eventConfigDTO.getProperties() without null checks which can cause NPEs; before
iterating, defensively check if dto.getEvents() is null and treat it as an empty
collection (skip iteration if null), and inside the events loop check if
eventConfigDTO.getProperties() is null and treat it as empty (skip or use empty
map/list) so you only iterate when non-null, ensuring
fraudDetectionConfig.setEvents(...) still receives an empty list when there are
no events.
| private FraudDetectionConfigDTO buildFraudDetectionConfigDTO(FraudDetectionConfig config) { | ||
|
|
||
| FraudDetectionConfigDTO fraudDetectionConfigDTO = new FraudDetectionConfigDTO(); | ||
| fraudDetectionConfigDTO.setPublishUserInfo(config.getPublishUserInfo()); | ||
| fraudDetectionConfigDTO.setPublishDeviceMetadata(config.getPublishDeviceMetadata()); | ||
| fraudDetectionConfigDTO.setLogRequestPayload(config.getLogRequestPayload()); | ||
|
|
||
| Map<String, EventConfigDTO> eventConfigDTOMap = new HashMap<>(); | ||
| config.getEvents().forEach(eventConfig -> { | ||
|
|
||
| Map<String, String> propertiesMap = new HashMap<>(); | ||
| eventConfig.getProperties().forEach(eventProperty -> | ||
| propertiesMap.put(eventProperty.getPropertyKey(), eventProperty.getPropertyValue())); | ||
|
|
||
| EventConfigDTO eventConfigDTO = new EventConfigDTO(eventConfig.getEnabled()); | ||
| eventConfigDTO.setProperties(propertiesMap); | ||
| eventConfigDTOMap.put(eventConfig.getEventName(), eventConfigDTO); | ||
| }); | ||
|
|
||
| fraudDetectionConfigDTO.setEvents(eventConfigDTOMap); | ||
|
|
||
| return fraudDetectionConfigDTO; | ||
| } |
There was a problem hiding this comment.
Add null safety checks for collections.
Similar to buildFraudDetectionConfig(), this method calls forEach() on config.getEvents() (line 2099) and eventConfig.getProperties() (line 2102) without null checks.
🔎 Apply this diff to add defensive null checks:
private FraudDetectionConfigDTO buildFraudDetectionConfigDTO(FraudDetectionConfig config) {
FraudDetectionConfigDTO fraudDetectionConfigDTO = new FraudDetectionConfigDTO();
fraudDetectionConfigDTO.setPublishUserInfo(config.getPublishUserInfo());
fraudDetectionConfigDTO.setPublishDeviceMetadata(config.getPublishDeviceMetadata());
fraudDetectionConfigDTO.setLogRequestPayload(config.getLogRequestPayload());
Map<String, EventConfigDTO> eventConfigDTOMap = new HashMap<>();
- config.getEvents().forEach(eventConfig -> {
+ List<EventConfig> events = config.getEvents();
+ if (events != null) {
+ events.forEach(eventConfig -> {
- Map<String, String> propertiesMap = new HashMap<>();
- eventConfig.getProperties().forEach(eventProperty ->
- propertiesMap.put(eventProperty.getPropertyKey(), eventProperty.getPropertyValue()));
+ Map<String, String> propertiesMap = new HashMap<>();
+ List<EventProperty> properties = eventConfig.getProperties();
+ if (properties != null) {
+ properties.forEach(eventProperty ->
+ propertiesMap.put(eventProperty.getPropertyKey(), eventProperty.getPropertyValue()));
+ }
- EventConfigDTO eventConfigDTO = new EventConfigDTO(eventConfig.getEnabled());
- eventConfigDTO.setProperties(propertiesMap);
- eventConfigDTOMap.put(eventConfig.getEventName(), eventConfigDTO);
- });
+ EventConfigDTO eventConfigDTO = new EventConfigDTO(eventConfig.getEnabled());
+ eventConfigDTO.setProperties(propertiesMap);
+ eventConfigDTOMap.put(eventConfig.getEventName(), eventConfigDTO);
+ });
+ }
fraudDetectionConfigDTO.setEvents(eventConfigDTOMap);
return fraudDetectionConfigDTO;
}📝 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.
| private FraudDetectionConfigDTO buildFraudDetectionConfigDTO(FraudDetectionConfig config) { | |
| FraudDetectionConfigDTO fraudDetectionConfigDTO = new FraudDetectionConfigDTO(); | |
| fraudDetectionConfigDTO.setPublishUserInfo(config.getPublishUserInfo()); | |
| fraudDetectionConfigDTO.setPublishDeviceMetadata(config.getPublishDeviceMetadata()); | |
| fraudDetectionConfigDTO.setLogRequestPayload(config.getLogRequestPayload()); | |
| Map<String, EventConfigDTO> eventConfigDTOMap = new HashMap<>(); | |
| config.getEvents().forEach(eventConfig -> { | |
| Map<String, String> propertiesMap = new HashMap<>(); | |
| eventConfig.getProperties().forEach(eventProperty -> | |
| propertiesMap.put(eventProperty.getPropertyKey(), eventProperty.getPropertyValue())); | |
| EventConfigDTO eventConfigDTO = new EventConfigDTO(eventConfig.getEnabled()); | |
| eventConfigDTO.setProperties(propertiesMap); | |
| eventConfigDTOMap.put(eventConfig.getEventName(), eventConfigDTO); | |
| }); | |
| fraudDetectionConfigDTO.setEvents(eventConfigDTOMap); | |
| return fraudDetectionConfigDTO; | |
| } | |
| private FraudDetectionConfigDTO buildFraudDetectionConfigDTO(FraudDetectionConfig config) { | |
| FraudDetectionConfigDTO fraudDetectionConfigDTO = new FraudDetectionConfigDTO(); | |
| fraudDetectionConfigDTO.setPublishUserInfo(config.getPublishUserInfo()); | |
| fraudDetectionConfigDTO.setPublishDeviceMetadata(config.getPublishDeviceMetadata()); | |
| fraudDetectionConfigDTO.setLogRequestPayload(config.getLogRequestPayload()); | |
| Map<String, EventConfigDTO> eventConfigDTOMap = new HashMap<>(); | |
| List<EventConfig> events = config.getEvents(); | |
| if (events != null) { | |
| events.forEach(eventConfig -> { | |
| Map<String, String> propertiesMap = new HashMap<>(); | |
| List<EventProperty> properties = eventConfig.getProperties(); | |
| if (properties != null) { | |
| properties.forEach(eventProperty -> | |
| propertiesMap.put(eventProperty.getPropertyKey(), eventProperty.getPropertyValue())); | |
| } | |
| EventConfigDTO eventConfigDTO = new EventConfigDTO(eventConfig.getEnabled()); | |
| eventConfigDTO.setProperties(propertiesMap); | |
| eventConfigDTOMap.put(eventConfig.getEventName(), eventConfigDTO); | |
| }); | |
| } | |
| fraudDetectionConfigDTO.setEvents(eventConfigDTOMap); | |
| return fraudDetectionConfigDTO; | |
| } |
🤖 Prompt for AI Agents
In
components/org.wso2.carbon.identity.api.server.configs/org.wso2.carbon.identity.api.server.configs.v1/src/main/java/org/wso2/carbon/identity/api/server/configs/v1/core/ServerConfigManagementService.java
around lines 2091 to 2113, add defensive null checks so we never call forEach on
a null collection: check config.getEvents() for null before iterating (or
iterate over Collections.emptyList() if null) and check
eventConfig.getProperties() for null before iterating (or treat as empty) so
propertiesMap and eventConfigDTOMap are only populated when collections exist;
ensure fraudDetectionConfigDTO.setEvents() still receives an empty map when
there are no events.
...erver.configs/org.wso2.carbon.identity.api.server.configs.v1/src/main/resources/configs.yaml
Show resolved
Hide resolved
| description: Update fraud detection related configuration of a tenant. | ||
| responses: | ||
| '202': | ||
| description: Successully updated. |
There was a problem hiding this comment.
Fix typo in response description.
"Successully" should be "Successfully".
🔎 Apply this diff:
- description: Successully updated.
+ description: Successfully updated.📝 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.
| description: Successully updated. | |
| description: Successfully updated. |
🤖 Prompt for AI Agents
components/org.wso2.carbon.identity.api.server.configs/org.wso2.carbon.identity.api.server.configs.v1/src/main/resources/configs.yaml
around line 1168: the response description contains a typo "Successully
updated."; update the string to "Successfully updated." (fix the spelling only,
preserving punctuation and surrounding formatting).
ZiyamSanthosh
force-pushed
the
master-fraud-detection-configs-2
branch
from
480be28 to
c94cbae
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (5)
components/org.wso2.carbon.identity.api.server.configs/org.wso2.carbon.identity.api.server.configs.v1/src/main/resources/configs.yaml (1)
1160-1173: Unresolved: Response status code mismatch between spec and implementation.The OpenAPI spec still defines the PUT operation to return
202 Accepted(line 1167), but according to previous review comments, the implementation returns200 OK. This inconsistency remains unaddressed.Please ensure both the specification and implementation use the same status code. If the operation is synchronous, both should use
200. If asynchronous or queued, both should use202.components/org.wso2.carbon.identity.api.server.configs/org.wso2.carbon.identity.api.server.configs.v1/src/main/java/org/wso2/carbon/identity/api/server/configs/v1/core/ServerConfigManagementService.java (4)
2034-2045: Catch the base exception type, not just server exceptions.The catch block only handles
FraudDetectionConfigServerException. IfFraudDetectionConfigClientExceptionis thrown (e.g., validation errors), it won't be caught, resulting in unhandled exceptions. Change the catch clause to catchIdentityFraudDetectionException(the base exception type) to handle both client and server exceptions uniformly.🔎 Apply this diff to catch all fraud detection exceptions:
- } catch (FraudDetectionConfigServerException e) { + } catch (IdentityFraudDetectionException e) { throw handleFraudDetectionConfigException(e, Constants.ErrorMessage.ERROR_CODE_FRAUD_DETECTION_CONFIG_RETRIEVE, null); }
2047-2059: Catch the base exception type, not just server exceptions.Same issue as
getFraudDetectionConfigs()- onlyFraudDetectionConfigServerExceptionis caught, leaving client exceptions unhandled. Change the catch clause to catchIdentityFraudDetectionExceptionto handle both client and server exceptions.🔎 Apply this diff to catch all fraud detection exceptions:
- } catch (FraudDetectionConfigServerException e) { + } catch (IdentityFraudDetectionException e) { throw handleFraudDetectionConfigException(e, Constants.ErrorMessage.ERROR_CODE_FRAUD_DETECTION_CONFIG_UPDATE, null); }
2061-2089: Add null safety checks for collections.The method calls
forEach()ondto.getEvents()(line 2069) andeventConfigDTO.getProperties()(line 2072) without null checks, which will throw NPE if these collections are null. Add defensive null checks before iterating over these collections.🔎 Apply this diff to add defensive null checks:
private FraudDetectionConfig buildFraudDetectionConfig(FraudDetectionConfigDTO dto) { FraudDetectionConfig fraudDetectionConfig = new FraudDetectionConfig(); fraudDetectionConfig.setPublishUserInfo(dto.isPublishUserInfo()); fraudDetectionConfig.setPublishDeviceMetadata(dto.isPublishDeviceMetadata()); fraudDetectionConfig.setLogRequestPayload(dto.isLogRequestPayload()); List<EventConfig> eventConfigs = new ArrayList<>(); - dto.getEvents().forEach((eventName, eventConfigDTO) -> { + Map<String, EventConfigDTO> events = dto.getEvents(); + if (events != null) { + events.forEach((eventName, eventConfigDTO) -> { - List<EventProperty> eventProperties = new ArrayList<>(); - eventConfigDTO.getProperties().forEach((key, value) -> { + List<EventProperty> eventProperties = new ArrayList<>(); + Map<String, String> properties = eventConfigDTO.getProperties(); + if (properties != null) { + properties.forEach((key, value) -> { - EventProperty eventProperty = new EventProperty(); - eventProperty.setPropertyKey(key); - eventProperty.setPropertyValue(value); - eventProperties.add(eventProperty); - }); + EventProperty eventProperty = new EventProperty(); + eventProperty.setPropertyKey(key); + eventProperty.setPropertyValue(value); + eventProperties.add(eventProperty); + }); + } - EventConfig eventConfig = new EventConfig(); - eventConfig.setEventName(eventName); - eventConfig.setEnabled(eventConfigDTO.isEnabled()); - eventConfig.setProperties(eventProperties); - eventConfigs.add(eventConfig); - }); + EventConfig eventConfig = new EventConfig(); + eventConfig.setEventName(eventName); + eventConfig.setEnabled(eventConfigDTO.isEnabled()); + eventConfig.setProperties(eventProperties); + eventConfigs.add(eventConfig); + }); + } fraudDetectionConfig.setEvents(eventConfigs); return fraudDetectionConfig; }
2091-2113: Add null safety checks for collections.Similar to
buildFraudDetectionConfig(), this method callsforEach()onconfig.getEvents()(line 2099) andeventConfig.getProperties()(line 2102) without null checks. Add defensive null checks to prevent NPEs when collections are null.🔎 Apply this diff to add defensive null checks:
private FraudDetectionConfigDTO buildFraudDetectionConfigDTO(FraudDetectionConfig config) { FraudDetectionConfigDTO fraudDetectionConfigDTO = new FraudDetectionConfigDTO(); fraudDetectionConfigDTO.setPublishUserInfo(config.getPublishUserInfo()); fraudDetectionConfigDTO.setPublishDeviceMetadata(config.getPublishDeviceMetadata()); fraudDetectionConfigDTO.setLogRequestPayload(config.getLogRequestPayload()); Map<String, EventConfigDTO> eventConfigDTOMap = new HashMap<>(); - config.getEvents().forEach(eventConfig -> { + List<EventConfig> events = config.getEvents(); + if (events != null) { + events.forEach(eventConfig -> { - Map<String, String> propertiesMap = new HashMap<>(); - eventConfig.getProperties().forEach(eventProperty -> - propertiesMap.put(eventProperty.getPropertyKey(), eventProperty.getPropertyValue())); + Map<String, String> propertiesMap = new HashMap<>(); + List<EventProperty> properties = eventConfig.getProperties(); + if (properties != null) { + properties.forEach(eventProperty -> + propertiesMap.put(eventProperty.getPropertyKey(), eventProperty.getPropertyValue())); + } - EventConfigDTO eventConfigDTO = new EventConfigDTO(eventConfig.getEnabled()); - eventConfigDTO.setProperties(propertiesMap); - eventConfigDTOMap.put(eventConfig.getEventName(), eventConfigDTO); - }); + EventConfigDTO eventConfigDTO = new EventConfigDTO(eventConfig.getEnabled()); + eventConfigDTO.setProperties(propertiesMap); + eventConfigDTOMap.put(eventConfig.getEventName(), eventConfigDTO); + }); + } fraudDetectionConfigDTO.setEvents(eventConfigDTOMap); return fraudDetectionConfigDTO; }
🧹 Nitpick comments (2)
components/org.wso2.carbon.identity.api.server.configs/org.wso2.carbon.identity.api.server.configs.common/src/main/java/org/wso2/carbon/identity/api/server/configs/common/Constants.java (1)
213-218: Consider whether tenant context should be included in error descriptions.The fraud detection error descriptions don't include a placeholder for tenant domain or organization context, unlike similar configuration errors (e.g., impersonation errors at lines 197-202, 209-211). If fraud detection configurations are tenant-specific, consider adding contextual information to the error descriptions for consistency and improved debugging.
Example with tenant context:
- ERROR_CODE_FRAUD_DETECTION_CONFIG_RETRIEVE("65023", - "Unable to retrieve Fraud Detection configuration.", - "Server encountered an error while retrieving the Fraud Detection configuration."), + ERROR_CODE_FRAUD_DETECTION_CONFIG_RETRIEVE("65023", + "Unable to retrieve Fraud Detection configuration.", + "Server encountered an error while retrieving the Fraud Detection configuration of %s."), - ERROR_CODE_FRAUD_DETECTION_CONFIG_UPDATE("65024", - "Unable to update Fraud Detection configuration.", - "Server encountered an error while updating the Fraud Detection configuration."); + ERROR_CODE_FRAUD_DETECTION_CONFIG_UPDATE("65024", + "Unable to update Fraud Detection configuration.", + "Server encountered an error while updating the Fraud Detection configuration of %s.");components/org.wso2.carbon.identity.api.server.configs/org.wso2.carbon.identity.api.server.configs.v1/src/main/resources/configs.yaml (1)
1586-1630: Consider specifying required fields for schema validation.None of the fraud detection schemas define
requiredfields. This means all properties are optional by default, which may lead to incomplete or ambiguous data being accepted by the API.Consider marking essential fields as required:
- In
EventConfig,eventNameshould likely be required to identify which event is being configured.- In
EventProperty, bothpropertyKeyandpropertyValueshould likely be required when a property object is provided.🔎 Example of adding required constraints:
EventConfig: type: object + required: + - eventName properties: eventName:EventProperty: type: object + required: + - propertyKey + - propertyValue properties: propertyKey:
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (5)
components/org.wso2.carbon.identity.api.server.configs/org.wso2.carbon.identity.api.server.configs.v1/src/gen/java/org/wso2/carbon/identity/api/server/configs/v1/ConfigsApi.javais excluded by!**/gen/**components/org.wso2.carbon.identity.api.server.configs/org.wso2.carbon.identity.api.server.configs.v1/src/gen/java/org/wso2/carbon/identity/api/server/configs/v1/ConfigsApiService.javais excluded by!**/gen/**components/org.wso2.carbon.identity.api.server.configs/org.wso2.carbon.identity.api.server.configs.v1/src/gen/java/org/wso2/carbon/identity/api/server/configs/v1/model/EventConfig.javais excluded by!**/gen/**components/org.wso2.carbon.identity.api.server.configs/org.wso2.carbon.identity.api.server.configs.v1/src/gen/java/org/wso2/carbon/identity/api/server/configs/v1/model/EventProperty.javais excluded by!**/gen/**components/org.wso2.carbon.identity.api.server.configs/org.wso2.carbon.identity.api.server.configs.v1/src/gen/java/org/wso2/carbon/identity/api/server/configs/v1/model/FraudDetectionConfig.javais excluded by!**/gen/**
📒 Files selected for processing (9)
components/org.wso2.carbon.identity.api.server.configs/org.wso2.carbon.identity.api.server.configs.common/pom.xml(1 hunks)components/org.wso2.carbon.identity.api.server.configs/org.wso2.carbon.identity.api.server.configs.common/src/main/java/org/wso2/carbon/identity/api/server/configs/common/ConfigsServiceHolder.java(3 hunks)components/org.wso2.carbon.identity.api.server.configs/org.wso2.carbon.identity.api.server.configs.common/src/main/java/org/wso2/carbon/identity/api/server/configs/common/Constants.java(1 hunks)components/org.wso2.carbon.identity.api.server.configs/org.wso2.carbon.identity.api.server.configs.v1/pom.xml(1 hunks)components/org.wso2.carbon.identity.api.server.configs/org.wso2.carbon.identity.api.server.configs.v1/src/main/java/org/wso2/carbon/identity/api/server/configs/v1/core/ServerConfigManagementService.java(7 hunks)components/org.wso2.carbon.identity.api.server.configs/org.wso2.carbon.identity.api.server.configs.v1/src/main/java/org/wso2/carbon/identity/api/server/configs/v1/factories/ServerConfigManagementServiceFactory.java(3 hunks)components/org.wso2.carbon.identity.api.server.configs/org.wso2.carbon.identity.api.server.configs.v1/src/main/java/org/wso2/carbon/identity/api/server/configs/v1/impl/ConfigsApiServiceImpl.java(3 hunks)components/org.wso2.carbon.identity.api.server.configs/org.wso2.carbon.identity.api.server.configs.v1/src/main/resources/configs.yaml(2 hunks)pom.xml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- components/org.wso2.carbon.identity.api.server.configs/org.wso2.carbon.identity.api.server.configs.common/pom.xml
- pom.xml
- components/org.wso2.carbon.identity.api.server.configs/org.wso2.carbon.identity.api.server.configs.v1/src/main/java/org/wso2/carbon/identity/api/server/configs/v1/factories/ServerConfigManagementServiceFactory.java
🧰 Additional context used
🧬 Code graph analysis (2)
components/org.wso2.carbon.identity.api.server.configs/org.wso2.carbon.identity.api.server.configs.v1/src/main/java/org/wso2/carbon/identity/api/server/configs/v1/impl/ConfigsApiServiceImpl.java (1)
components/org.wso2.carbon.identity.api.server.configs/org.wso2.carbon.identity.api.server.configs.v1/src/gen/java/org/wso2/carbon/identity/api/server/configs/v1/model/FraudDetectionConfig.java (1)
FraudDetectionConfig(36-175)
components/org.wso2.carbon.identity.api.server.configs/org.wso2.carbon.identity.api.server.configs.v1/src/main/java/org/wso2/carbon/identity/api/server/configs/v1/core/ServerConfigManagementService.java (3)
components/org.wso2.carbon.identity.api.server.configs/org.wso2.carbon.identity.api.server.configs.v1/src/gen/java/org/wso2/carbon/identity/api/server/configs/v1/model/EventConfig.java (1)
EventConfig(36-153)components/org.wso2.carbon.identity.api.server.configs/org.wso2.carbon.identity.api.server.configs.v1/src/gen/java/org/wso2/carbon/identity/api/server/configs/v1/model/EventProperty.java (1)
EventProperty(33-120)components/org.wso2.carbon.identity.api.server.configs/org.wso2.carbon.identity.api.server.configs.v1/src/gen/java/org/wso2/carbon/identity/api/server/configs/v1/model/FraudDetectionConfig.java (1)
FraudDetectionConfig(36-175)
🔇 Additional comments (10)
components/org.wso2.carbon.identity.api.server.configs/org.wso2.carbon.identity.api.server.configs.common/src/main/java/org/wso2/carbon/identity/api/server/configs/common/Constants.java (1)
211-218: LGTM! Duplicate error code issue resolved.The error codes have been updated to "65023" and "65024", which are unique and maintain the sequential numbering pattern. This resolves the critical duplicate error code issue flagged in the previous review.
components/org.wso2.carbon.identity.api.server.configs/org.wso2.carbon.identity.api.server.configs.v1/pom.xml (1)
188-192: Verify version management and dependency availability.The new fraud detection dependency is correctly scoped as
provided, but ensure the following:
- The version for
org.wso2.carbon.identity.fraud.detection.coreis properly defined in the parent POM's<dependencyManagement>section.- The artifact exists in the configured Maven repositories.
- The version is compatible with other
org.wso2.carbon.identity.frameworkdependencies in use.Run the following script to verify version management in the parent POM:
#!/bin/bash # Description: Verify that the fraud detection dependency version is managed in parent POMs # Check root pom.xml for version management echo "=== Checking root pom.xml for fraud detection dependency version ===" rg -A 3 "org.wso2.carbon.identity.fraud.detection.core" pom.xml # Check parent module pom.xml echo -e "\n=== Checking parent configs module pom.xml ===" rg -A 3 "org.wso2.carbon.identity.fraud.detection.core" components/org.wso2.carbon.identity.api.server.configs/pom.xml # Verify other identity.framework dependency versions for compatibility echo -e "\n=== Checking other identity.framework dependency versions ===" rg -n "org.wso2.carbon.identity.framework" pom.xml | head -20components/org.wso2.carbon.identity.api.server.configs/org.wso2.carbon.identity.api.server.configs.v1/src/main/java/org/wso2/carbon/identity/api/server/configs/v1/core/ServerConfigManagementService.java (3)
51-53: LGTM!The new imports for fraud detection models, exceptions, DTOs, and the HashMap utility are correctly added to support the fraud detection configuration feature.
Also applies to: 95-100, 127-127
153-153: LGTM!The
FraudDetectionConfigsServiceis properly wired through constructor dependency injection, following the same pattern as other services in this class.Also applies to: 169-170, 179-179
2115-2152: LGTM!The exception handler correctly differentiates between client and server exceptions, maps them to appropriate HTTP status codes, and follows the same pattern as other exception handlers in this class.
components/org.wso2.carbon.identity.api.server.configs/org.wso2.carbon.identity.api.server.configs.common/src/main/java/org/wso2/carbon/identity/api/server/configs/common/ConfigsServiceHolder.java (2)
26-26: LGTM! Import follows WSO2 package conventions.The import statement is correct and consistent with other service imports in the file.
98-103: Implementation follows the established service holder pattern correctly.The FraudDetectionConfigsServiceHolder and its getter method match the pattern used by all other services in this file. Null handling is properly managed at the consumer level—the ServerConfigManagementServiceFactory validates FraudDetectionConfigsService availability and throws IllegalStateException if unavailable, consistent with all other required services.
components/org.wso2.carbon.identity.api.server.configs/org.wso2.carbon.identity.api.server.configs.v1/src/main/java/org/wso2/carbon/identity/api/server/configs/v1/impl/ConfigsApiServiceImpl.java (3)
26-26: LGTM: Import statement is appropriate.The import is necessary for the new fraud detection configuration endpoints.
135-139: Add null checking for fraud detection configs retrieval.The
getFraudDetectionConfigs()method directly passes the service result tobuildFraudDetectionConfig()without null checking. IffraudDetectionConfigsService.getFraudDetectionConfigs()returns null, the code will throw aNullPointerExceptionwhenbuildFraudDetectionConfig()attempts to call methods on the null DTO (e.g.,dto.isPublishUserInfo()).Follow the pattern used in
getRemoteLoggingConfig()(lines 114-123): check if the result is null and return a 404 response, or ensure the service layer always returns a valid object with defaults.
230-234: Return pattern forupdateFraudDetectionConfigsis justified—the service enriches the configuration during update.The method returns the updated entity, which differs from other update methods (
updateInboundScimConfigs,updateRemoteLoggingConfig,updateSAMLInboundAuthConfig,updatePassiveSTSInboundAuthConfig). However, this is intentional: the service layer transforms the input through DTO conversion (buildFraudDetectionConfigDTO→ service call →buildFraudDetectionConfig), enriching the configuration before returning it. Returning the entity allows clients to see the final state with any server-applied transformations or defaults. This pattern is consistent with RESTful practices for resources that undergo server-side enrichment.
ZiyamSanthosh
force-pushed
the
master-fraud-detection-configs-2
branch
from
c94cbae to
df0b0bb
Compare
|
PR builder started |
|
PR builder completed |
jenkins-is-staging
left a comment
jenkins-is-staging
left a comment
There was a problem hiding this comment.
Approving the pull request based on the successful pr build https://github.com/wso2/product-is/actions/runs/20367260184
3 participants
This introduces the Fraud detection configuration management APIs through the existing Configs Mgt endpoint
Related issue:
Summary by CodeRabbit
New Features
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.