Skip to content

[BULK-PROFILE]-11: Add validation in bulk proflie service - #1997

Open
bharathappali wants to merge 29 commits into
kruize:runtimes-iirjfrom
bharathappali:feat-bulk-feat-11
Open

[BULK-PROFILE]-11: Add validation in bulk proflie service#1997
bharathappali wants to merge 29 commits into
kruize:runtimes-iirjfrom
bharathappali:feat-bulk-feat-11

Conversation

@bharathappali

@bharathappali bharathappali commented Jun 24, 2026

Copy link
Copy Markdown
Member

Description

This PR is built on top of #1996

#1996 Needs to be merged before merging this PR

This PR adds the validation steps in bulk profile service
Fixes #1977

Type of change

  • Bug fix
  • New feature
  • Docs update
  • Breaking change (What changes might users need to make in their application due to this PR?)
  • Requires DB changes

How has this been tested?

Please describe the tests that were run to verify your changes and steps to reproduce. Please specify any test configuration required.

  • New Test X
  • Functional testsuite

Test Configuration

  • Kubernetes clusters tested on:

Checklist 🎯

  • Followed coding guidelines
  • Comments added
  • Dependent changes merged
  • Documentation updated
  • Tests added or updated

Additional information

Include any additional information such as links, test results, screenshots here

Summary by Sourcery

Introduce CRUD APIs, validation, and persistence for bulk profiles in the analyzer.

New Features:

  • Add BulkProfile REST service with create, read, update, delete, and listing operations, including optional webhook callbacks.
  • Define BulkProfile and BulkProfileUpdateRequest service objects to model bulk profile configuration and partial updates.
  • Persist bulk profiles via new KruizeBulkProfileEntry entity and associated database migration, including JSONB fields for clusters and recommendation settings.
  • Expose bulk profile operations through ExperimentDAO/ExperimentDAOImpl and register the new bulk profile servlet route in the analyzer server context.
  • Add BulkProfileValidation to enforce structural, datasource, scheduling, and URL validation rules for bulk profile definitions and updates.

Build:

  • Create database schema migration to add kruize_bulk_profile table backing bulk profile storage.

@sourcery-ai

sourcery-ai Bot commented Jun 24, 2026

Copy link
Copy Markdown
Contributor

Reviewer's Guide

Adds a full CRUD and validation stack for Bulk Profiles, including REST endpoints, validation logic, DB schema/entity, DAO methods, and servlet wiring, enabling validated creation, listing, update (with webhook trigger), deletion, and querying of bulk profile configurations.

Sequence diagram for BulkProfileService create flow

sequenceDiagram
    actor User
    participant BulkProfileService
    participant BulkProfileValidation
    participant ExperimentDAO
    participant KruizeBulkProfileEntry

    User->>BulkProfileService: POST /bulkProfiles
    BulkProfileService->>BulkProfileService: objectMapper.readValue
    BulkProfileService->>BulkProfileValidation: validateCreate
    BulkProfileValidation-->>BulkProfileService: ValidationOutputData
    alt [validation failed]
        BulkProfileService-->>User: HTTP 400/4xx error
    else [validation succeeded]
        BulkProfileService->>ExperimentDAO: loadBulkProfileByName
        alt [profile exists]
            BulkProfileService-->>User: HTTP 409 conflict
        else [profile does not exist]
            BulkProfileService->>KruizeBulkProfileEntry: fromBulkProfile
            BulkProfileService->>ExperimentDAO: addBulkProfileToDB
            ExperimentDAO-->>BulkProfileService: ValidationOutputData
            alt [DB insert ok]
                BulkProfileService-->>User: HTTP 201 BulkProfile
            else [DB insert failed]
                BulkProfileService-->>User: HTTP 500 error
            end
        end
    end
Loading

File-Level Changes

Change Details Files
Introduce Bulk Profile CRUD API and wire it into the analyzer servlet context.
  • Add BulkProfileService servlet that exposes GET/POST/PUT/DELETE handlers for bulk profiles, including error handling and JSON serialization.
  • Register BulkProfileService under a new BULK_PROFILE_SERVICE path in Analyzer and ServerContext.
  • Implement webhook triggering on profile update via GenericRestApiClient, without failing the main operation on webhook errors.
src/main/java/com/autotune/analyzer/services/BulkProfileService.java
src/main/java/com/autotune/analyzer/Analyzer.java
src/main/java/com/autotune/utils/ServerContext.java
Add Bulk Profile validation layer to enforce request correctness and datasource reachability.
  • Implement BulkProfileValidation with create and update validators that check required fields, naming rules, clusters, experiment types, recommendation settings, scheduling format, and webhook URL.
  • Reuse datasource reachability checks via ExperimentDBService and DataSourceOperatorImpl within validation.
  • Introduce constrained sets for valid terms, models, and experiment types plus regex-based scheduling validation.
src/main/java/com/autotune/common/bulk/BulkProfileValidation.java
Define Bulk Profile service-layer DTOs used by the REST API.
  • Add BulkProfile service object with nested Cluster and RecommendationSettings types, including Jackson annotations for JSON field names and timestamps.
  • Add BulkProfileUpdateRequest DTO to support partial updates with a hasUpdates guard for PUT requests.
src/main/java/com/autotune/analyzer/serviceObjects/BulkProfile.java
src/main/java/com/autotune/analyzer/serviceObjects/BulkProfileUpdateRequest.java
Persist Bulk Profiles in the database via a new entity, schema migration, and DAO methods.
  • Create KruizeBulkProfileEntry JPA entity mapped to a new kruize_bulk_profile table, storing clusters and recommendation_settings as JSONB and providing conversion to/from BulkProfile.
  • Add a Flyway migration to create the kruize_bulk_profile table with appropriate columns and defaults.
  • Extend ExperimentDAO/ExperimentDAOImpl with CRUD operations for bulk profiles (add, load by name, load all, update, delete, load enabled), including transaction handling and HTTP-aware ValidationOutputData responses, plus new HQL constants for bulk profiles.
  • Register KruizeBulkProfileEntry with KruizeHibernateUtil so it participates in Hibernate sessionFactory configuration.
src/main/java/com/autotune/database/table/lm/KruizeBulkProfileEntry.java
migrations/lm/v111__create_kruize_bulk_profile_table.sql
src/main/java/com/autotune/database/dao/ExperimentDAO.java
src/main/java/com/autotune/database/dao/ExperimentDAOImpl.java
src/main/java/com/autotune/database/helper/DBConstants.java
src/main/java/com/autotune/database/init/KruizeHibernateUtil.java

Possibly linked issues

  • #0: PR delivers BulkProfile table, entity, DAO, servlet, and validation matching the Bulk Profile API feature request.

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

@sourcery-ai sourcery-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

  • The HQL constants for bulk profiles (e.g., LOAD_ALL_ENABLED_BULK_PROFILES and DELETE_BULK_PROFILE_BY_NAME) use the column name profile_name instead of the entity field profileName, which will cause HQL errors; update these to reference the Java property names rather than the DB column names.
  • BulkProfileService calls BulkProfileValidation but does not import it, so the class will not compile as-is; add the missing import for com.autotune.common.bulk.BulkProfileValidation.
  • The BulkProfile API model exposes fields like description and measurement_duration but KruizeBulkProfileEntry and the migration do not persist them, so if these fields are meant to be durable you should add corresponding columns and map them in toBulkProfile/fromBulkProfile.
Prompt for AI Agents
Please address the comments from this code review:

## Overall Comments
- The HQL constants for bulk profiles (e.g., `LOAD_ALL_ENABLED_BULK_PROFILES` and `DELETE_BULK_PROFILE_BY_NAME`) use the column name `profile_name` instead of the entity field `profileName`, which will cause HQL errors; update these to reference the Java property names rather than the DB column names.
- `BulkProfileService` calls `BulkProfileValidation` but does not import it, so the class will not compile as-is; add the missing import for `com.autotune.common.bulk.BulkProfileValidation`.
- The `BulkProfile` API model exposes fields like `description` and `measurement_duration` but `KruizeBulkProfileEntry` and the migration do not persist them, so if these fields are meant to be durable you should add corresponding columns and map them in `toBulkProfile`/`fromBulkProfile`.

## Individual Comments

### Comment 1
<location path="src/main/java/com/autotune/database/dao/ExperimentDAOImpl.java" line_range="2198-2199" />
<code_context>
+        try (Session session = KruizeHibernateUtil.getSessionFactory().openSession()) {
+            try {
+                tx = session.beginTransaction();
+                Query query = session.createQuery(
+                        DELETE_BULK_PROFILE_BY_NAME, null);
+                query.setParameter("profileName", profileName);
+                int result = query.executeUpdate();
</code_context>
<issue_to_address>
**issue (bug_risk):** Avoid passing null as the result type to createQuery; use the typed or untyped overload explicitly.

Using `null` as the second argument is version-dependent and may cause runtime failures. It also obscures whether this query is intended to be typed or untyped, so it’s safer and clearer to call the appropriate overload directly.
</issue_to_address>

### Comment 2
<location path="src/main/java/com/autotune/database/dao/ExperimentDAOImpl.java" line_range="2079-2081" />
<code_context>
+                validationOutputData.setMessage("Bulk profile with name " + kruizeBulkProfileEntry.getProfileName() + " already exists");
+                validationOutputData.setErrorCode(HttpServletResponse.SC_CONFLICT);
+            } catch (HibernateException e) {
+                LOGGER.error("Not able to save bulk profile due to {}", e.getMessage());
+                if (tx != null) tx.rollback();
+                e.printStackTrace();
+                validationOutputData.setSuccess(false);
+                validationOutputData.setMessage(e.getMessage());
</code_context>
<issue_to_address>
**suggestion:** Avoid using e.printStackTrace() and rely on structured logging instead.

In this and other bulk-profile DAO methods, the exception is already logged, so calling `e.printStackTrace()` is redundant and can clutter logs or bypass logging config. Rely on `LOGGER.error("...", e)` for the stack trace and remove direct `printStackTrace()` calls.

Suggested implementation:

```java
            } catch (HibernateException e) {
                LOGGER.error("Not able to save bulk profile due to {}", e.getMessage(), e);
                if (tx != null) tx.rollback();
                validationOutputData.setSuccess(false);
                validationOutputData.setMessage(e.getMessage());
            }
        } catch (Exception e) {
            LOGGER.error("Not able to save bulk profile due to {}", e.getMessage(), e);
            validationOutputData.setMessage(e.getMessage());
        }

```

There may be other bulk-profile-related DAO methods in this class (or other DAO classes) that still use `e.printStackTrace()` or log without passing the exception object. For consistency with this change:
1. Search for `printStackTrace` in `ExperimentDAOImpl.java` and related DAO classes and remove those calls.
2. Ensure corresponding `LOGGER.error` (or `LOGGER.warn`) calls include the exception as the last argument, e.g., `LOGGER.error("Some message", e);`.
</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 +2198 to +2199
Query query = session.createQuery(
DELETE_BULK_PROFILE_BY_NAME, null);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

issue (bug_risk): Avoid passing null as the result type to createQuery; use the typed or untyped overload explicitly.

Using null as the second argument is version-dependent and may cause runtime failures. It also obscures whether this query is intended to be typed or untyped, so it’s safer and clearer to call the appropriate overload directly.

Comment on lines +2079 to +2081
LOGGER.error("Not able to save bulk profile due to {}", e.getMessage());
if (tx != null) tx.rollback();
e.printStackTrace();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

suggestion: Avoid using e.printStackTrace() and rely on structured logging instead.

In this and other bulk-profile DAO methods, the exception is already logged, so calling e.printStackTrace() is redundant and can clutter logs or bypass logging config. Rely on LOGGER.error("...", e) for the stack trace and remove direct printStackTrace() calls.

Suggested implementation:

            } catch (HibernateException e) {
                LOGGER.error("Not able to save bulk profile due to {}", e.getMessage(), e);
                if (tx != null) tx.rollback();
                validationOutputData.setSuccess(false);
                validationOutputData.setMessage(e.getMessage());
            }
        } catch (Exception e) {
            LOGGER.error("Not able to save bulk profile due to {}", e.getMessage(), e);
            validationOutputData.setMessage(e.getMessage());
        }

There may be other bulk-profile-related DAO methods in this class (or other DAO classes) that still use e.printStackTrace() or log without passing the exception object. For consistency with this change:

  1. Search for printStackTrace in ExperimentDAOImpl.java and related DAO classes and remove those calls.
  2. Ensure corresponding LOGGER.error (or LOGGER.warn) calls include the exception as the last argument, e.g., LOGGER.error("Some message", e);.

Signed-off-by: bharathappali <abharath@redhat.com>
Signed-off-by: bharathappali <abharath@redhat.com>
Signed-off-by: bharathappali <abharath@redhat.com>
Signed-off-by: bharathappali <abharath@redhat.com>
Signed-off-by: bharathappali <abharath@redhat.com>
Signed-off-by: bharathappali <abharath@redhat.com>
Signed-off-by: bharathappali <abharath@redhat.com>
Signed-off-by: bharathappali <abharath@redhat.com>
Signed-off-by: bharathappali <abharath@redhat.com>
Signed-off-by: bharathappali <abharath@redhat.com>
Signed-off-by: bharathappali <abharath@redhat.com>
Signed-off-by: bharathappali <abharath@redhat.com>
Signed-off-by: bharathappali <abharath@redhat.com>
Signed-off-by: bharathappali <abharath@redhat.com>
Signed-off-by: bharathappali <abharath@redhat.com>
Signed-off-by: bharathappali <abharath@redhat.com>
Signed-off-by: bharathappali <abharath@redhat.com>
Signed-off-by: bharathappali <abharath@redhat.com>
…ulk profile

Signed-off-by: bharathappali <abharath@redhat.com>
Signed-off-by: bharathappali <abharath@redhat.com>
Signed-off-by: bharathappali <abharath@redhat.com>
Signed-off-by: bharathappali <abharath@redhat.com>
Signed-off-by: bharathappali <abharath@redhat.com>
Signed-off-by: bharathappali <abharath@redhat.com>
Signed-off-by: bharathappali <abharath@redhat.com>
Signed-off-by: bharathappali <abharath@redhat.com>
Signed-off-by: bharathappali <abharath@redhat.com>
Signed-off-by: bharathappali <abharath@redhat.com>
Signed-off-by: bharathappali <abharath@redhat.com>
@bharathappali bharathappali moved this to Under Review in Monitoring Jul 24, 2026
@rbadagandi1 rbadagandi1 added this to the Release 0.0.1 milestone Jul 24, 2026
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.

2 participants