[BULK-PROFILE]-11: Add validation in bulk proflie service - #1997
[BULK-PROFILE]-11: Add validation in bulk proflie service#1997bharathappali wants to merge 29 commits into
Conversation
Reviewer's GuideAdds 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 flowsequenceDiagram
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
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- The HQL constants for bulk profiles (e.g.,
LOAD_ALL_ENABLED_BULK_PROFILESandDELETE_BULK_PROFILE_BY_NAME) use the column nameprofile_nameinstead of the entity fieldprofileName, which will cause HQL errors; update these to reference the Java property names rather than the DB column names. BulkProfileServicecallsBulkProfileValidationbut does not import it, so the class will not compile as-is; add the missing import forcom.autotune.common.bulk.BulkProfileValidation.- The
BulkProfileAPI model exposes fields likedescriptionandmeasurement_durationbutKruizeBulkProfileEntryand the migration do not persist them, so if these fields are meant to be durable you should add corresponding columns and map them intoBulkProfile/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>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| Query query = session.createQuery( | ||
| DELETE_BULK_PROFILE_BY_NAME, null); |
There was a problem hiding this comment.
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.
| LOGGER.error("Not able to save bulk profile due to {}", e.getMessage()); | ||
| if (tx != null) tx.rollback(); | ||
| e.printStackTrace(); |
There was a problem hiding this comment.
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:
- Search for
printStackTraceinExperimentDAOImpl.javaand related DAO classes and remove those calls. - Ensure corresponding
LOGGER.error(orLOGGER.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>
bf37223 to
d031bed
Compare
Signed-off-by: bharathappali <abharath@redhat.com>
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
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.
Test Configuration
Checklist 🎯
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:
Build: