Skip to content

Adding Unit Test Cases For Optimizer REST APIs#8

Open
shekhar316 wants to merge 8 commits intokruize:mvp_demofrom
shekhar316:utest
Open

Adding Unit Test Cases For Optimizer REST APIs#8
shekhar316 wants to merge 8 commits intokruize:mvp_demofrom
shekhar316:utest

Conversation

@shekhar316
Copy link
Copy Markdown
Contributor

@shekhar316 shekhar316 commented Mar 17, 2026

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 endpoints
    • LayerResourceTest - Tests for layer configuration endpoints
    • MetadataProfileResourceTest - Tests for metadata profile endpoints
    • MetricProfileResourceTest - Tests for metric profile endpoints
    • WebhookResourceTest - Tests for webhook endpoints
  • Created MockResponseLoader utility class to load mock JSON responses from test resources

  • Added mock JSON response files for various API scenarios:

    • Datasource lists (populated and empty)
    • Layer lists (populated and empty)
    • Metadata profile lists (populated and empty)
    • Metric profile lists (populated and empty)
    • Bulk API responses

Scope & Limitations

⚠️ Note: This is a basic test implementation with limited scope. The tests currently focus on:

  • Mock API response validation
  • Basic endpoint availability checks
  • Response structure verification

Future Work

More detailed test cases will be added in subsequent PRs, including:

  • Edge case scenarios
  • Error handling tests
  • Integration tests
  • Service layer unit tests
  • Validation logic tests
  • Database interaction tests

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:

  • Expose REST endpoints for listing and installing metadata and metric profiles and layers, and for retrieving datasource lists, system status, and bulk jobs overview.
  • Introduce scheduled bulk experiment creation with webhook handling and centralized Kruize state caching and profile installation.
  • Add a REST client interface for the Kruize service plus models for datasources, profiles, jobs, status, and webhook payloads.

Enhancements:

  • Refine Optimizer constants into dedicated final nested classes and add API/path, message, and profile path constants.
  • Improve startup behavior by initializing the bulk scheduler and state/profile installation on application start.
  • Introduce a generic API response wrapper and global exception mapper for consistent error handling across resources.

Build:

  • Add Mockito and Quarkus JUnit5 Mockito dependencies for mocking in tests.

Documentation:

  • Document how to run and structure tests in a new test README.

Tests:

  • Add REST-assured based integration tests for datasource, layer, metadata profile, metric profile, and webhook resources using JSON-backed mock responses.
  • Add a MockResponseLoader test utility and JSON fixtures for datasources, profiles, layers, and bulk API responses.

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>
@sourcery-ai
Copy link
Copy Markdown

sourcery-ai bot commented Mar 17, 2026

Reviewer's Guide

Adds 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 refresh

sequenceDiagram
    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()
Loading

Sequence diagram for webhook handling and jobs statistics update

sequenceDiagram
    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
Loading

Class diagram for new REST resources, services, and models

classDiagram
    %% 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~
Loading

File-Level Changes

Change Details Files
Introduce REST client and service layer for interacting with the Kruize backend, including profiles, datasources, bulk operations, and state caching.
  • Add MicroProfile RestClient interface for Kruize APIs, including list/create profile, layer and bulk operations
  • Implement DatasourceService, ProfileService, KruizeStateService, BulkSchedulerService, StatusService, and JobsService for higher-level business logic
  • Introduce KruizeServiceException and a GlobalExceptionMapper to standardize error handling and HTTP responses
src/main/java/com/kruize/optimizer/client/KruizeClient.java
src/main/java/com/kruize/optimizer/service/DatasourceService.java
src/main/java/com/kruize/optimizer/service/ProfileService.java
src/main/java/com/kruize/optimizer/service/KruizeStateService.java
src/main/java/com/kruize/optimizer/service/BulkSchedulerService.java
src/main/java/com/kruize/optimizer/service/StatusService.java
src/main/java/com/kruize/optimizer/service/JobsService.java
src/main/java/com/kruize/optimizer/exception/KruizeServiceException.java
src/main/java/com/kruize/optimizer/exception/GlobalExceptionMapper.java
Expose new REST resources for datasources, profiles, status, jobs, and webhooks, using a shared ApiResponse wrapper and centralized constants.
  • Add DatasourceResource, MetadataProfileResource, MetricProfileResource, LayerResource, StatusResource, JobsResource, and WebhookResource endpoints
  • Use ApiResponse as a uniform response envelope with status/message/data/errors
  • Centralize API paths, messages, and profile/path constants under OptimizerConstants and new nested static classes
src/main/java/com/kruize/optimizer/resource/DatasourceResource.java
src/main/java/com/kruize/optimizer/resource/MetadataProfileResource.java
src/main/java/com/kruize/optimizer/resource/MetricProfileResource.java
src/main/java/com/kruize/optimizer/resource/LayerResource.java
src/main/java/com/kruize/optimizer/resource/StatusResource.java
src/main/java/com/kruize/optimizer/resource/JobsResource.java
src/main/java/com/kruize/optimizer/resource/WebhookResource.java
src/main/java/com/kruize/optimizer/model/api/ApiResponse.java
src/main/java/com/kruize/optimizer/utils/OptimizerConstants.java
Add models to represent Kruize domain objects and system status used by the new services and resources.
  • Introduce Datasource, KruizeProfile (with custom deserializers), JobsOverview, KruizeStatus, WebhookPayload, and DatasourceListResponse models
  • Align JSON mapping with Kruize API payloads (aliases, nested metadata/name, numeric/string profile_version, and status fields)
src/main/java/com/kruize/optimizer/model/kruize/Datasource.java
src/main/java/com/kruize/optimizer/model/kruize/KruizeProfile.java
src/main/java/com/kruize/optimizer/model/kruize/JobsOverview.java
src/main/java/com/kruize/optimizer/model/kruize/KruizeStatus.java
src/main/java/com/kruize/optimizer/model/WebhookPayload.java
src/main/java/com/kruize/optimizer/model/api/DatasourceListResponse.java
Configure bulk scheduling, webhook and default profiles/datasource, and initialize scheduling/state at startup.
  • Extend application.yml to configure Kruize URL, bulk scheduler interval/startup-delay/measurement-duration, webhook URL, default datasource/metadata/metric profiles, and target labels JSON
  • Inject BulkSchedulerService into Startup and invoke initialize() after startup to refresh state and install profiles before scheduled bulk calls run
src/main/resources/application.yml
src/main/java/com/kruize/optimizer/Startup.java
Restructure constants into static utility-style classes and add configuration/path/message constants for the new features.
  • Make OptimizerConstants and its nested classes final/static with private constructors to enforce utility semantics
  • Add OptimizerApiConstants for base paths and resource suffixes, MessageConstants for user-facing messages, ProfileType and ProfilePathConstants for profile handling
src/main/java/com/kruize/optimizer/utils/OptimizerConstants.java
Seed local profile and layer definitions used by ProfileService for installing missing profiles at runtime.
  • Add configsReferenceIndex.json describing available metadata profiles, metric profiles, and layers
  • Add JSON definitions for v1.0 metadata and metric profiles and for container/semeru/hotspot/quarkus layer configs
src/main/resources/configs/configsReferenceIndex.json
src/main/resources/configs/v1.0/metadata-profiles/cluster-metadata-local-monitoring.json
src/main/resources/configs/v1.0/metric-profiles/resource-optimization-local-monitoring.json
src/main/resources/configs/layers/container.json
src/main/resources/configs/layers/semeru.json
src/main/resources/configs/layers/hotspot.json
src/main/resources/configs/layers/quarkus.json
Introduce a test infrastructure with Mockito-based Quarkus tests and JSON-backed mock responses for REST resources.
  • Add quarkus-junit5-mockito and mockito-core test dependencies and a test-specific application.yml
  • Add MockResponseLoader utility and JSON fixtures for datasources, profiles, layers, and bulk API responses
  • Create REST-assured tests for DatasourceResource, LayerResource, MetadataProfileResource, MetricProfileResource, and WebhookResource that mock KruizeClient and scheduler/state services
pom.xml
src/test/resources/application.yml
src/test/java/com/kruize/optimizer/util/MockResponseLoader.java
src/test/java/com/kruize/optimizer/resource/DatasourceResourceTest.java
src/test/java/com/kruize/optimizer/resource/LayerResourceTest.java
src/test/java/com/kruize/optimizer/resource/MetadataProfileResourceTest.java
src/test/java/com/kruize/optimizer/resource/MetricProfileResourceTest.java
src/test/java/com/kruize/optimizer/resource/WebhookResourceTest.java
src/test/resources/mock-responses/datasource_list.json
src/test/resources/mock-responses/empty_datasource_list.json
src/test/resources/mock-responses/layer_list.json
src/test/resources/mock-responses/empty_layer_list.json
src/test/resources/mock-responses/metadata_profile_list.json
src/test/resources/mock-responses/empty_metadata_profile_list.json
src/test/resources/mock-responses/metric_profile_list.json
src/test/resources/mock-responses/empty_metric_profile_list.json
src/test/resources/mock-responses/bulk_api_response.json
src/test/README.md

Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it. You can also reply to a
    review comment with @sourcery-ai issue to create an issue from it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time. You can also comment
    @sourcery-ai title on the pull request to (re-)generate the title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time exactly where you
    want it. You can also comment @sourcery-ai summary on the pull request to
    (re-)generate the summary at any time.
  • Generate reviewer's guide: Comment @sourcery-ai guide on the pull
    request to (re-)generate the reviewer's guide at any time.
  • Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
    pull request to resolve all Sourcery comments. Useful if you've already
    addressed all the comments and don't want to see them anymore.
  • Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
    request to dismiss all existing Sourcery reviews. Especially useful if you
    want to start fresh with a new review - don't forget to comment
    @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

Copy link
Copy Markdown

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey - I've found 3 issues, and left some high level feedback:

  • 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 ApiResponses, 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.
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>

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

Comment on lines +28 to +36
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) {
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Comment on lines +166 to +175
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);
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  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:
        } catch (Exception e) {
            LOG.errorf(e, "Failed to parse targetLabelsJson, returning empty labels. Raw value: %s", targetLabelsJson);
            return Collections.emptyMap();
        }
  1. 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.

Comment on lines +55 to +64
// Refresh the global state cache
kruizeStateService.refreshState();

// Get datasources from cache
List<Datasource> datasources = kruizeStateService.getCachedDatasources();
status.setDatasources(new KruizeStatus.DatasourceStatus(
datasources.size(),
datasources
));

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  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.

@rbadagandi1
Copy link
Copy Markdown

@shekhar316 Can you update the description of the PR to provide more insights on what this PR is for?

@shekhar316
Copy link
Copy Markdown
Contributor Author

@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.

@shekhar316 shekhar316 changed the title Utest Adding Unit Test Cases For Optimizer REST APIs Mar 17, 2026
@kusumachalasani kusumachalasani moved this from In Progress to Under Review in Monitoring Mar 17, 2026
@chandrams chandrams requested a review from khansaad March 17, 2026 08:56
@shekhar316
Copy link
Copy Markdown
Contributor Author

NOTE: THIS PR IS BASED ON PR#5 and PR#6.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: Under Review

Development

Successfully merging this pull request may close these issues.

3 participants