Adding Unit Test Cases For Optimizer REST APIs#8
Adding Unit Test Cases For Optimizer REST APIs#8shekhar316 wants to merge 8 commits intokruize:mvp_demofrom
Conversation
Signed-off-by: Shekhar Saxena <shekhar.saxena@ibm.com>
Signed-off-by: Shekhar Saxena <shekhar.saxena@ibm.com>
Signed-off-by: Shekhar Saxena <shekhar.saxena@ibm.com>
Signed-off-by: Shekhar Saxena <shekhar.saxena@ibm.com>
Signed-off-by: Shekhar Saxena <shekhar.saxena@ibm.com>
Signed-off-by: Shekhar Saxena <shekhar.saxena@ibm.com>
Signed-off-by: Shekhar Saxena <shekhar.saxena@ibm.com>
Signed-off-by: Shekhar Saxena <shekhar.saxena@ibm.com>
Reviewer's GuideAdds a full REST integration layer around Kruize with datasource/profile/layer/status/jobs/webhook endpoints, centralizes constants and configuration, introduces services for state/profile/datasource management and bulk scheduling, and adds mocked REST-assured tests with JSON fixtures to validate the new API surface. Sequence diagram for scheduled bulk API call and state refreshsequenceDiagram
participant Scheduler as QuarkusScheduler
participant BulkSchedulerService
participant KruizeStateService
participant DatasourceService
participant ProfileService
participant KruizeClient
participant JobsService
%% Startup initialization
Scheduler->>BulkSchedulerService: initialize()
BulkSchedulerService->>KruizeStateService: refreshStateAndInstallProfiles()
KruizeStateService->>DatasourceService: getDatasources()
DatasourceService->>KruizeClient: getDatasources()
KruizeClient-->>DatasourceService: DatasourceListResponse
DatasourceService-->>KruizeStateService: List Datasource
KruizeStateService->>ProfileService: getMetadataProfiles()
ProfileService->>KruizeClient: getMetadataProfiles(verbose=true)
KruizeClient-->>ProfileService: List KruizeProfile
ProfileService-->>KruizeStateService: List KruizeProfile
KruizeStateService->>ProfileService: getMetricProfiles()
ProfileService->>KruizeClient: getMetricProfiles(verbose=true)
KruizeClient-->>ProfileService: List KruizeProfile
ProfileService-->>KruizeStateService: List KruizeProfile
KruizeStateService->>ProfileService: getLayers()
ProfileService->>KruizeClient: getLayers()
KruizeClient-->>ProfileService: List KruizeProfile
ProfileService-->>KruizeStateService: List KruizeProfile
KruizeStateService-->>BulkSchedulerService: state cached
%% Periodic bulk scheduler run
Scheduler->>BulkSchedulerService: scheduledBulkApiCall()
BulkSchedulerService->>KruizeStateService: isCacheEmpty()
KruizeStateService-->>BulkSchedulerService: boolean
alt cache empty
BulkSchedulerService->>KruizeStateService: refreshState()
end
BulkSchedulerService->>KruizeStateService: getDefaultDatasourceName()
KruizeStateService-->>BulkSchedulerService: Optional String
BulkSchedulerService->>KruizeStateService: getDefaultMetadataProfileName()
KruizeStateService-->>BulkSchedulerService: Optional String
BulkSchedulerService->>KruizeStateService: getDefaultMetricProfileName()
KruizeStateService-->>BulkSchedulerService: Optional String
BulkSchedulerService->>BulkSchedulerService: buildBulkPayload()
BulkSchedulerService->>KruizeClient: bulkCreateExperiments(payload)
KruizeClient-->>BulkSchedulerService: String response
BulkSchedulerService->>JobsService: incrementJobsTriggered()
Sequence diagram for webhook handling and jobs statistics updatesequenceDiagram
participant Kruize as KruizeBulkAPI
participant WebhookResource
participant BulkSchedulerService
participant JobsService
Kruize->>WebhookResource: POST /webhook [List WebhookPayload]
WebhookResource->>BulkSchedulerService: handleWebhook(payloads)
loop for each payload
BulkSchedulerService->>BulkSchedulerService: read Summary
alt status COMPLETED and jobId new
BulkSchedulerService->>JobsService: updateExperimentCounters(total, processed, existing)
else status not completed or duplicate
BulkSchedulerService->>BulkSchedulerService: skip update
end
end
BulkSchedulerService-->>WebhookResource: processing done
WebhookResource-->>Kruize: 200 OK
Class diagram for new REST resources, services, and modelsclassDiagram
%% Resources
class DatasourceResource {
+listDatasources() Response
}
class MetadataProfileResource {
+listMetadataProfiles() Response
+installMetadataProfiles() Response
}
class MetricProfileResource {
+listMetricProfiles() Response
+installMetricProfiles() Response
}
class LayerResource {
+listLayers() Response
+installLayers() Response
}
class StatusResource {
+getStatus() Response
}
class JobsResource {
+getJobsOverview() Response
}
class WebhookResource {
+receiveWebhook(List WebhookPayload) Response
}
%% Services
class DatasourceService {
+getDatasources() List~Datasource~
+getDatasourceCount() int
+isKruizeAvailable() boolean
}
class ProfileService {
+getMetadataProfiles() List~KruizeProfile~
+getMetricProfiles() List~KruizeProfile~
+getLayers() List~KruizeProfile~
+installMissingProfiles(profileType String) List~String~
}
class KruizeStateService {
+refreshState() void
+installMissingProfiles() void
+refreshStateAndInstallProfiles() void
+scheduledStateRefresh() void
+getDefaultDatasourceName() Optional~String~
+getDefaultMetadataProfileName() Optional~String~
+getDefaultMetricProfileName() Optional~String~
+getCachedDatasources() List~Datasource~
+getCachedMetadataProfiles() List~KruizeProfile~
+getCachedMetricProfiles() List~KruizeProfile~
+getCachedLayers() List~KruizeProfile~
+isCacheEmpty() boolean
}
class BulkSchedulerService {
+initialize() void
+scheduledBulkApiCall() void
+handleWebhook(List WebhookPayload) void
}
class StatusService {
+getSystemStatus() KruizeStatus
+isKruizeHealthy() boolean
}
class JobsService {
+incrementJobsTriggered() void
+updateExperimentCounters(total int, processed int, existing int) void
+getJobsOverview() JobsOverview
+getTotalJobsTriggered() int
+getTotalExperimentsCreated() int
+getTotalExperimentsProcessed() int
+getTotalExperimentsUnique() int
}
%% Client
class KruizeClient {
<<interface>>
+getDatasources() DatasourceListResponse
+getMetadataProfiles(verbose boolean) List~KruizeProfile~
+getMetricProfiles(verbose boolean) List~KruizeProfile~
+getLayers() List~KruizeProfile~
+createMetadataProfile(profileDefinition Object) String
+createMetricProfile(profileDefinition Object) String
+createLayer(layerDefinition Object) String
+bulkCreateExperiments(bulkRequest Object) String
+getBulkJobStatus(jobId String) String
}
%% Models
class ApiResponse~T~ {
-status String
-message String
-data T
-errors List~String~
+getStatus() String
+getMessage() String
+getData() T
+getErrors() List~String~
+setStatus(status String) void
+setMessage(message String) void
+setData(data T) void
+setErrors(errors List~String~) void
+success(message String, data T) ApiResponse~T~
+success(message String) ApiResponse~T~
+error(message String, errors List~String~) ApiResponse~T~
+error(message String) ApiResponse~T~
}
class DatasourceListResponse {
-datasources List~Datasource~
+getDatasources() List~Datasource~
+setDatasources(datasources List~Datasource~) void
}
class Datasource {
-name String
-provider String
-url String
-serviceName String
-namespace String
+getName() String
+getProvider() String
+getUrl() String
+getServiceName() String
+getNamespace() String
}
class KruizeProfile {
-name String
-profileVersion String
-profileType String
+getName() String
+getProfileVersion() String
+getProfileType() String
+setName(name String) void
+setProfileVersion(profileVersion String) void
+setProfileType(profileType String) void
}
class KruizeStatus {
-datasources DatasourceStatus
-metadataProfiles ProfileStatus
-metricProfiles ProfileStatus
-layers ProfileStatus
-alerts List~String~
+getDatasources() DatasourceStatus
+getMetadataProfiles() ProfileStatus
+getMetricProfiles() ProfileStatus
+getLayers() ProfileStatus
+getAlerts() List~String~
+addAlert(alert String) void
}
class DatasourceStatus {
-count int
-list List~Datasource~
}
class ProfileStatus {
-installed List~ProfileInfo~
}
class ProfileInfo {
-name String
-profileVersion String
}
class JobsOverview {
-jobsTriggered int
-totalExperiments int
-processedExperiments int
-uniqueExperiments int
+getJobsTriggered() int
+getTotalExperiments() int
+getProcessedExperiments() int
+getUniqueExperiments() int
}
class WebhookPayload {
-summary Summary
-webhook WebhookStatus
+getSummary() Summary
+getWebhook() WebhookStatus
}
class Summary {
-jobId String
-status String
-totalExperiments int
-processedExperiments int
-existingExperiments int
}
class WebhookStatus {
-status String
}
%% Exceptions and mappers
class KruizeServiceException {
-statusCode int
+getStatusCode() int
}
class GlobalExceptionMapper {
+toResponse(exception Exception) Response
}
%% Constants container
class OptimizerConstants {
}
class KruizeClientConstants {
}
class OptimizerApiConstants {
}
class GenericOptimizerConstants {
}
class MessageConstants {
}
class ProfileType {
}
class ProfilePathConstants {
}
OptimizerConstants <|-- KruizeClientConstants
OptimizerConstants <|-- OptimizerApiConstants
OptimizerConstants <|-- GenericOptimizerConstants
OptimizerConstants <|-- MessageConstants
OptimizerConstants <|-- ProfileType
OptimizerConstants <|-- ProfilePathConstants
%% Resource to service dependencies
DatasourceResource ..> DatasourceService
MetadataProfileResource ..> ProfileService
MetricProfileResource ..> ProfileService
LayerResource ..> ProfileService
StatusResource ..> StatusService
JobsResource ..> JobsService
WebhookResource ..> BulkSchedulerService
%% Service dependencies
DatasourceService ..> KruizeClient
ProfileService ..> KruizeClient
KruizeStateService ..> DatasourceService
KruizeStateService ..> ProfileService
BulkSchedulerService ..> KruizeClient
BulkSchedulerService ..> KruizeStateService
BulkSchedulerService ..> JobsService
StatusService ..> DatasourceService
StatusService ..> ProfileService
StatusService ..> KruizeStateService
JobsService ..> JobsOverview
%% Model usage
DatasourceService ..> DatasourceListResponse
DatasourceListResponse o--> Datasource
KruizeStatus o--> DatasourceStatus
KruizeStatus o--> ProfileStatus
ProfileStatus o--> ProfileInfo
WebhookPayload o--> Summary
WebhookPayload o--> WebhookStatus
%% Exception mapping
GlobalExceptionMapper ..> KruizeServiceException
GlobalExceptionMapper ..> ApiResponse~Object~
%% Generic ApiResponse usage
DatasourceResource ..> ApiResponse~List~
MetadataProfileResource ..> ApiResponse~List~
MetricProfileResource ..> ApiResponse~List~
LayerResource ..> ApiResponse~List~
StatusResource ..> ApiResponse~KruizeStatus~
JobsResource ..> ApiResponse~JobsOverview~
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- The
KruizeClientis registered with@RegisterRestClient(configKey = "kruize-api")butapplication.ymlconfiguresquarkus.rest-client."com.kruize.optimizer.client.KruizeClient", which means the main app won’t pick up the intended URL; align the config key and property name so the client is actually using the configured endpoint. - In
BulkSchedulerService.parseTargetLabels, you’re manually parsing JSON using string operations even though anObjectMapperis available; consider using Jackson (readTreeor binding to a small DTO) to make this more robust to whitespace/escaping and easier to extend (e.g., support array forms). - Most resource methods wrap their bodies in broad try/catch and return error
ApiResponses, while aGlobalExceptionMapperis also introduced; to reduce duplication and keep error handling consistent, consider letting services throwKruizeServiceExceptionand relying on the mapper instead of catching genericExceptionin each resource.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `KruizeClient` is registered with `@RegisterRestClient(configKey = "kruize-api")` but `application.yml` configures `quarkus.rest-client."com.kruize.optimizer.client.KruizeClient"`, which means the main app won’t pick up the intended URL; align the config key and property name so the client is actually using the configured endpoint.
- In `BulkSchedulerService.parseTargetLabels`, you’re manually parsing JSON using string operations even though an `ObjectMapper` is available; consider using Jackson (`readTree` or binding to a small DTO) to make this more robust to whitespace/escaping and easier to extend (e.g., support array forms).
- Most resource methods wrap their bodies in broad try/catch and return error `ApiResponse`s, while a `GlobalExceptionMapper` is also introduced; to reduce duplication and keep error handling consistent, consider letting services throw `KruizeServiceException` and relying on the mapper instead of catching generic `Exception` in each resource.
## Individual Comments
### Comment 1
<location path="src/main/java/com/kruize/optimizer/exception/GlobalExceptionMapper.java" line_range="28-36" />
<code_context>
+ * Global exception mapper for handling exceptions across the application
+ */
+@Provider
+public class GlobalExceptionMapper implements ExceptionMapper<Exception> {
+
+ private static final Logger LOG = Logger.getLogger(GlobalExceptionMapper.class);
+
+ @Override
+ public Response toResponse(Exception exception) {
+ LOG.error("Exception occurred", exception);
+
+ if (exception instanceof KruizeServiceException) {
+ KruizeServiceException kse = (KruizeServiceException) exception;
+ return Response.status(kse.getStatusCode())
</code_context>
<issue_to_address>
**issue (bug_risk):** Catching all Exception types here may inadvertently override HTTP status codes from WebApplicationException and similar.
Since this mapper is registered for the base `Exception` type, it will also intercept `WebApplicationException` (and subclasses like `NotFoundException`) and map them to 500 unless they are `KruizeServiceException`. This discards explicitly set HTTP status codes. Consider either narrowing the mapper’s type (e.g., to `RuntimeException`) or adding logic that detects `WebApplicationException` and preserves its `getResponse().getStatus()` (and possibly its entity).
</issue_to_address>
### Comment 2
<location path="src/main/java/com/kruize/optimizer/service/BulkSchedulerService.java" line_range="166-175" />
<code_context>
+ private Map<String, String> parseTargetLabels() {
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Manual string parsing of JSON labels is fragile; consider leveraging ObjectMapper or a typed config.
Splitting on commas/colons and stripping quotes will fail for values that contain those characters, extra whitespace, or any nested/structured content, and can easily drift from the actual config format. Since `ObjectMapper` is already available, parse `targetLabelsJson` into a `Map<String, String>` (or a small DTO) instead to eliminate these edge cases and reduce maintenance.
Suggested implementation:
```java
import com.fasterxml.jackson.core.type.TypeReference;
import com.fasterxml.jackson.databind.ObjectMapper;
```
```java
import java.util.Collections;
import java.util.HashMap;
```
```java
/**
* Parse target labels from JSON configuration
*
* @return Map of label key-value pairs
*/
private Map<String, String> parseTargetLabels() {
if (targetLabelsJson == null || targetLabelsJson.isBlank()) {
return Collections.emptyMap();
}
try {
// Parse JSON directly into a Map<String, String> using ObjectMapper
return objectMapper.readValue(
targetLabelsJson,
new TypeReference<Map<String, String>>() {}
);
```
To fully apply the suggestion and remove the fragile manual parsing:
1. Delete the remainder of the old `parseTargetLabels` method body after the `try {` shown in the SEARCH block, including:
- The `Map<String, String> labels = new HashMap<>();` line
- All the manual string operations (`substring`, `split`, quote stripping, etc.)
- Any `catch` block that was handling parsing failures for the manual logic.
2. Replace that deleted code with a proper `catch` block matching the new implementation, for example:
```java
} catch (Exception e) {
LOG.errorf(e, "Failed to parse targetLabelsJson, returning empty labels. Raw value: %s", targetLabelsJson);
return Collections.emptyMap();
}
```
3. Ensure that:
- `objectMapper` is available as a field in this class (e.g., injected via `@Inject ObjectMapper objectMapper;` or constructor).
- There is no remaining manual string-based parsing of `targetLabelsJson` anywhere else in the method.
</issue_to_address>
### Comment 3
<location path="src/main/java/com/kruize/optimizer/service/StatusService.java" line_range="55-64" />
<code_context>
+ // Refresh the global state cache
</code_context>
<issue_to_address>
**suggestion (performance):** StatusService refreshes the cache and then bypasses it by calling ProfileService directly, causing redundant remote calls.
In `getSystemStatus()`, `kruizeStateService.refreshState()` already calls the datasource and profile services, but you then call `profileService.getMetadataProfiles()`, `getMetricProfiles()`, and `getLayers()` again. This doubles the remote calls per request. Prefer using the cached values from `KruizeStateService` (e.g., `getCachedMetadataProfiles()`, etc.) after the refresh to avoid the extra calls and keep a single source of truth.
Suggested implementation:
```java
// Get metadata profiles from cache (already refreshed above)
List<MetadataProfile> metadataProfiles = kruizeStateService.getCachedMetadataProfiles();
```
```java
// Get metric profiles from cache (already refreshed above)
List<MetricProfile> metricProfiles = kruizeStateService.getCachedMetricProfiles();
```
```java
// Get layers from cache (already refreshed above)
List<Layer> layers = kruizeStateService.getCachedLayers();
```
If `getSystemStatus()` uses `profileService` anywhere else (e.g., for other profile-related data), those calls should also be replaced with the corresponding `kruizeStateService.getCached*()` methods so that:
1) `kruizeStateService.refreshState()` is the single source of truth for refreshing remote state, and
2) all reads use the in-memory cached values instead of issuing extra remote calls.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| public class GlobalExceptionMapper implements ExceptionMapper<Exception> { | ||
|
|
||
| private static final Logger LOG = Logger.getLogger(GlobalExceptionMapper.class); | ||
|
|
||
| @Override | ||
| public Response toResponse(Exception exception) { | ||
| LOG.error("Exception occurred", exception); | ||
|
|
||
| if (exception instanceof KruizeServiceException) { |
There was a problem hiding this comment.
issue (bug_risk): Catching all Exception types here may inadvertently override HTTP status codes from WebApplicationException and similar.
Since this mapper is registered for the base Exception type, it will also intercept WebApplicationException (and subclasses like NotFoundException) and map them to 500 unless they are KruizeServiceException. This discards explicitly set HTTP status codes. Consider either narrowing the mapper’s type (e.g., to RuntimeException) or adding logic that detects WebApplicationException and preserves its getResponse().getStatus() (and possibly its entity).
| private Map<String, String> parseTargetLabels() { | ||
| Map<String, String> labels = new HashMap<>(); | ||
| try { | ||
| // Simple JSON parsing for {"key": "value"} format | ||
| String json = targetLabelsJson.trim(); | ||
| if (json.startsWith("{") && json.endsWith("}")) { | ||
| json = json.substring(1, json.length() - 1); | ||
| String[] pairs = json.split(","); | ||
| for (String pair : pairs) { | ||
| String[] keyValue = pair.split(":", 2); |
There was a problem hiding this comment.
suggestion (bug_risk): Manual string parsing of JSON labels is fragile; consider leveraging ObjectMapper or a typed config.
Splitting on commas/colons and stripping quotes will fail for values that contain those characters, extra whitespace, or any nested/structured content, and can easily drift from the actual config format. Since ObjectMapper is already available, parse targetLabelsJson into a Map<String, String> (or a small DTO) instead to eliminate these edge cases and reduce maintenance.
Suggested implementation:
import com.fasterxml.jackson.core.type.TypeReference;
import com.fasterxml.jackson.databind.ObjectMapper;import java.util.Collections;
import java.util.HashMap; /**
* Parse target labels from JSON configuration
*
* @return Map of label key-value pairs
*/
private Map<String, String> parseTargetLabels() {
if (targetLabelsJson == null || targetLabelsJson.isBlank()) {
return Collections.emptyMap();
}
try {
// Parse JSON directly into a Map<String, String> using ObjectMapper
return objectMapper.readValue(
targetLabelsJson,
new TypeReference<Map<String, String>>() {}
);To fully apply the suggestion and remove the fragile manual parsing:
- Delete the remainder of the old
parseTargetLabelsmethod body after thetry {shown in the SEARCH block, including:- The
Map<String, String> labels = new HashMap<>();line - All the manual string operations (
substring,split, quote stripping, etc.) - Any
catchblock that was handling parsing failures for the manual logic.
- The
- Replace that deleted code with a proper
catchblock matching the new implementation, for example:
} catch (Exception e) {
LOG.errorf(e, "Failed to parse targetLabelsJson, returning empty labels. Raw value: %s", targetLabelsJson);
return Collections.emptyMap();
}- Ensure that:
objectMapperis available as a field in this class (e.g., injected via@Inject ObjectMapper objectMapper;or constructor).- There is no remaining manual string-based parsing of
targetLabelsJsonanywhere else in the method.
| // Refresh the global state cache | ||
| kruizeStateService.refreshState(); | ||
|
|
||
| // Get datasources from cache | ||
| List<Datasource> datasources = kruizeStateService.getCachedDatasources(); | ||
| status.setDatasources(new KruizeStatus.DatasourceStatus( | ||
| datasources.size(), | ||
| datasources | ||
| )); | ||
|
|
There was a problem hiding this comment.
suggestion (performance): StatusService refreshes the cache and then bypasses it by calling ProfileService directly, causing redundant remote calls.
In getSystemStatus(), kruizeStateService.refreshState() already calls the datasource and profile services, but you then call profileService.getMetadataProfiles(), getMetricProfiles(), and getLayers() again. This doubles the remote calls per request. Prefer using the cached values from KruizeStateService (e.g., getCachedMetadataProfiles(), etc.) after the refresh to avoid the extra calls and keep a single source of truth.
Suggested implementation:
// Get metadata profiles from cache (already refreshed above)
List<MetadataProfile> metadataProfiles = kruizeStateService.getCachedMetadataProfiles(); // Get metric profiles from cache (already refreshed above)
List<MetricProfile> metricProfiles = kruizeStateService.getCachedMetricProfiles(); // Get layers from cache (already refreshed above)
List<Layer> layers = kruizeStateService.getCachedLayers();If getSystemStatus() uses profileService anywhere else (e.g., for other profile-related data), those calls should also be replaced with the corresponding kruizeStateService.getCached*() methods so that:
kruizeStateService.refreshState()is the single source of truth for refreshing remote state, and- all reads use the in-memory cached values instead of issuing extra remote calls.
|
@shekhar316 Can you update the description of the PR to provide more insights on what this PR is for? |
|
@rbadagandi1 sure, I have updated summary to be more descriptive. This PR aims to add unit test cases using mockito for the optimizer microservice. Detailed description of tests and methods are already added to PR description. |
|
Summary
This PR adds the unit test cases of REST API endpoints in Optimizer application. These unit test cases aim to test the basic functionality of REST API endpoints present in the applications which are related to checking the installed profiles, layers and datasources in kruize app.
Changes
Added unit tests for the following resource endpoints:
DatasourceResourceTest- Tests for datasource API endpointsLayerResourceTest- Tests for layer configuration endpointsMetadataProfileResourceTest- Tests for metadata profile endpointsMetricProfileResourceTest- Tests for metric profile endpointsWebhookResourceTest- Tests for webhook endpointsCreated
MockResponseLoaderutility class to load mock JSON responses from test resourcesAdded mock JSON response files for various API scenarios:
Scope & Limitations
Future Work
More detailed test cases will be added in subsequent PRs, including:
Test Coverage: Basic API endpoint tests with mock responses only
Summary by Sourcery
Add REST client, services, resources, and models to integrate with Kruize APIs for datasources, profiles, layers, jobs, status, and webhooks, along with configuration updates and supporting utilities.
New Features:
Enhancements:
Build:
Documentation:
Tests: