Layers -15 Refactors validation logic to improve code quality and provide better error message#1847
Conversation
Reviewer's GuideAdds full CRUD support and significantly enhances validation and error handling for the Layer API, updates DB/metrics plumbing, and refactors tests to exercise new behaviors and messages. Sequence diagram for updated createLayer API flowsequenceDiagram
actor Client
participant LayerService
participant Converters
participant LayerValidation
participant ExperimentDAOImpl
participant MetricsConfig
Client->>LayerService: HTTP POST /createLayer (JSON body)
LayerService->>LayerService: read request body
alt Empty body
LayerService->>LayerService: sendErrorResponse(..., INVALID_LAYER_JSON)
LayerService->>Client: JSON error response 400
else Non empty body
LayerService->>Converters: convertInputJSONToCreateLayer(inputData)
alt Invalid JSON or missing fields
Converters-->>LayerService: throws IllegalArgumentException or JSONException
LayerService->>LayerService: catch IllegalArgumentException
LayerService->>LayerService: sendErrorResponse(..., 400, validation message)
LayerService->>Client: JSON error response 400
else Valid KruizeLayer
Converters-->>LayerService: KruizeLayer
LayerService->>LayerValidation: validate(kruizeLayer)
LayerValidation-->>LayerService: ValidationOutputData
alt Validation fails
LayerService->>LayerService: sendErrorResponse(..., 400, VALIDATION_FAILED)
LayerService->>Client: JSON error response 400
else Validation succeeds
LayerService->>ExperimentDAOImpl: addLayerToDB(layerEntry)
ExperimentDAOImpl->>MetricsConfig: start timerAddLayerDB
ExperimentDAOImpl-->>LayerService: ValidationOutputData
alt DB add succeeds
LayerService->>LayerService: sendSuccessResponse(message, 201)
LayerService->>Client: JSON success response 201
else DB add fails
LayerService->>LayerService: sendErrorResponse(..., 500, ADD_LAYER_TO_DB_FAILURE)
LayerService->>Client: JSON error response 500
end
end
end
end
note over LayerService: sendSuccessResponse/sendErrorResponse now delegate to sendJsonResponse for uniform JSON structure
Sequence diagram for new updateLayer API flowsequenceDiagram
actor Client
participant LayerService
participant Converters
participant LayerValidation
participant ExperimentDAOImpl
participant MetricsConfig
Client->>LayerService: HTTP PUT /updateLayer?name={layerName}
LayerService->>LayerService: getParameter(LAYER_NAME)
alt Missing or empty name param
LayerService->>LayerService: sendErrorResponse(..., INVALID_LAYER_NAME)
LayerService->>Client: JSON error response 400
else Has name param
LayerService->>LayerService: validate query params against UPDATE_LAYERS_QUERY_PARAMS_SUPPORTED
alt Unsupported query params
LayerService->>LayerService: sendErrorResponse(..., INVALID_QUERY_PARAMS)
LayerService->>Client: JSON error response 400
else Query params valid
LayerService->>LayerService: read request body
alt Empty body
LayerService->>LayerService: sendErrorResponse(..., INVALID_LAYER_JSON)
LayerService->>Client: JSON error response 400
else Non empty body
LayerService->>Converters: convertInputJSONToCreateLayer(inputData)
alt Converter throws IllegalArgumentException
Converters-->>LayerService: exception
LayerService->>LayerService: sendErrorResponse(..., 400, VALIDATION_FAILED)
LayerService->>Client: JSON error response 400
else Valid KruizeLayer
Converters-->>LayerService: KruizeLayer
LayerService->>LayerService: compare URL name vs kruizeLayer.getLayerName()
alt Names mismatch
LayerService->>LayerService: sendErrorResponse(..., LAYER_NAME_MISMATCH)
LayerService->>Client: JSON error response 400
else Names match
LayerService->>LayerValidation: validate(kruizeLayer)
LayerValidation-->>LayerService: ValidationOutputData
alt Validation fails
LayerService->>LayerService: sendErrorResponse(..., VALIDATION_FAILED)
LayerService->>Client: JSON error response 400
else Validation succeeds
LayerService->>ExperimentDAOImpl: loadLayerByName(layerName)
ExperimentDAOImpl-->>LayerService: List<KruizeLMLayerEntry>
alt Layer not found
LayerService->>LayerService: sendErrorResponse(..., LAYER_NOT_FOUND)
LayerService->>Client: JSON error response 404
else Layer exists
LayerService->>LayerService: convertToLayerEntry(kruizeLayer)
LayerService->>ExperimentDAOImpl: updateLayerToDB(layerEntry)
ExperimentDAOImpl->>MetricsConfig: start timerUpdateLayerDB
ExperimentDAOImpl-->>LayerService: ValidationOutputData
alt Update succeeds
LayerService->>LayerService: sendSuccessResponse(message, 200)
LayerService->>Client: JSON success response 200
else Update fails
LayerService->>LayerService: sendErrorResponse(..., 500, UPDATE_LAYER_TO_DB_FAILURE)
LayerService->>Client: JSON error response 500
end
end
end
end
end
end
end
end
Sequence diagram for new deleteLayer API flowsequenceDiagram
actor Client
participant LayerService
participant ExperimentDAOImpl
participant MetricsConfig
Client->>LayerService: HTTP DELETE /deleteLayer?name={layerName}
LayerService->>LayerService: getParameter(LAYER_NAME)
alt Missing or empty name param
LayerService->>LayerService: sendErrorResponse(..., INVALID_LAYER_NAME)
LayerService->>Client: JSON error response 400
else Has name param
LayerService->>LayerService: validate query params against DELETE_LAYERS_QUERY_PARAMS_SUPPORTED
alt Unsupported query params
LayerService->>LayerService: sendErrorResponse(..., INVALID_QUERY_PARAMS)
LayerService->>Client: JSON error response 400
else Query params valid
LayerService->>ExperimentDAOImpl: loadLayerByName(layerName)
ExperimentDAOImpl-->>LayerService: List<KruizeLMLayerEntry>
alt Layer not found
LayerService->>LayerService: sendErrorResponse(..., DELETE_LAYER_ENTRY_NOT_FOUND_WITH_NAME)
LayerService->>Client: JSON error response 404
else Layer exists
LayerService->>ExperimentDAOImpl: deleteLayerByName(layerName)
ExperimentDAOImpl->>MetricsConfig: start timerDeleteLayerDB
ExperimentDAOImpl-->>LayerService: ValidationOutputData
alt Delete succeeds
LayerService->>LayerService: sendSuccessResponse(message, 200)
LayerService->>Client: JSON success response 200
else Delete fails
LayerService->>LayerService: sendErrorResponse(..., 500, message)
LayerService->>Client: JSON error response 500
end
end
end
end
Class diagram for updated Layer API, converters, DAO, and metricsclassDiagram
class LayerService {
+doPost(HttpServletRequest request, HttpServletResponse response) void
+doGet(HttpServletRequest request, HttpServletResponse response) void
+doPut(HttpServletRequest request, HttpServletResponse response) void
+doDelete(HttpServletRequest request, HttpServletResponse response) void
-convertToLayerEntry(KruizeLayer kruizeLayer) KruizeLMLayerEntry
-sendSuccessResponse(HttpServletResponse response, String message, int httpStatusCode) void
-sendErrorResponse(HttpServletResponse response, Exception e, int httpStatusCode, String errorMsg) void
-sendJsonResponse(HttpServletResponse response, String message, int statusCode, String status) void
}
class Converters {
<<utility>>
-getRequiredString(JSONObject json, String fieldName, String objectType, String fieldDescription) String
+convertInputJSONToCreateLayer(String inputData) KruizeLayer
}
class ExperimentDAO {
<<interface>>
+addLayerToDB(KruizeLMLayerEntry kruizeLayerEntry) ValidationOutputData
+updateLayerToDB(KruizeLMLayerEntry kruizeLayerEntry) ValidationOutputData
+deleteLayerByName(String layerName) ValidationOutputData
+loadAllLayers() List~KruizeLMLayerEntry~
+loadLayerByName(String layerName) List~KruizeLMLayerEntry~
}
class ExperimentDAOImpl {
+addLayerToDB(KruizeLMLayerEntry kruizeLayerEntry) ValidationOutputData
+updateLayerToDB(KruizeLMLayerEntry kruizeLayerEntry) ValidationOutputData
+deleteLayerByName(String layerName) ValidationOutputData
+loadAllLayers() List~KruizeLMLayerEntry~
+loadLayerByName(String layerName) List~KruizeLMLayerEntry~
}
class MetricsConfig {
+meterRegistry() MeterRegistry
+timerAddLayerDB Timer
+timerLoadAllLayers Timer
+timerLoadLayerByName Timer
+timerUpdateLayerDB Timer
+timerDeleteLayerDB Timer
+timerBAddLayerDB Timer.Builder
+timerBLoadAllLayers Timer.Builder
+timerBLoadLayerByName Timer.Builder
+timerBUpdateLayerDB Timer.Builder
+timerBDeleteLayerDB Timer.Builder
}
class KruizeLayer {
+getLayerName() String
+getLayerPresence() Object
+getTunables() List~Tunable~
}
class KruizeLMLayerEntry {
+layer_name String
+presence String
+details String
+tunables String
}
class Tunable {
+name String
+value_type String
+validate() void
-validateBounds() void
-validateCategorical() void
}
class ValidationOutputData {
+isSuccess() boolean
+getMessage() String
+setSuccess(boolean success) void
+setMessage(String message) void
}
LayerService --> Converters : uses
LayerService --> ExperimentDAO : uses
LayerService --> KruizeLayer : uses
LayerService --> KruizeLMLayerEntry : converts to
LayerService --> ValidationOutputData : uses
ExperimentDAOImpl ..|> ExperimentDAO
ExperimentDAOImpl --> KruizeLMLayerEntry : persists
ExperimentDAOImpl --> ValidationOutputData : returns
ExperimentDAOImpl --> MetricsConfig : uses timers
Converters --> KruizeLayer : creates
Converters --> Tunable : validates and constructs
Tunable --> ValidationOutputData : throws validation exceptions referenced by errors
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 2 issues, and left some high level feedback:
- The new helper
getRequiredString()and the various validation strings hardcode user-facing messages; consider wiring these throughAnalyzerErrorConstants(similar to other APIs) so that error wording remains centralized and consistent across the service. - In
MetricsConfigyou definetimerBUpdateLayerDBandtimerBDeleteLayerDB, but onlyupdateLayerToDBuses its timer; consider instrumentingdeleteLayerByNamewith the corresponding timer to keep DB metrics coverage consistent. - The
sendJsonResponsemethod recreates aGsoninstance on every call viacreateGsonObject(); you could cache a singleGsoninstance inLayerServiceto avoid repeated object creation on hot paths.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new helper `getRequiredString()` and the various validation strings hardcode user-facing messages; consider wiring these through `AnalyzerErrorConstants` (similar to other APIs) so that error wording remains centralized and consistent across the service.
- In `MetricsConfig` you define `timerBUpdateLayerDB` and `timerBDeleteLayerDB`, but only `updateLayerToDB` uses its timer; consider instrumenting `deleteLayerByName` with the corresponding timer to keep DB metrics coverage consistent.
- The `sendJsonResponse` method recreates a `Gson` instance on every call via `createGsonObject()`; you could cache a single `Gson` instance in `LayerService` to avoid repeated object creation on hot paths.
## Individual Comments
### Comment 1
<location path="src/main/java/com/autotune/analyzer/services/LayerService.java" line_range="391-401" />
<code_context>
+ }
+
+ // Delete from database
+ ValidationOutputData deletedFromDB = experimentDAO.deleteLayerByName(layerName);
+
+ if (deletedFromDB.isSuccess()) {
+ LOGGER.debug(KruizeConstants.LayerAPIMessages.DELETE_LAYER_FROM_DB, layerName);
+ sendSuccessResponse(response, String.format(KruizeConstants.LayerAPIMessages.DELETE_LAYER_SUCCESS_MSG, layerName),
+ HttpServletResponse.SC_OK);
+ } else {
+ sendErrorResponse(response, null, HttpServletResponse.SC_INTERNAL_SERVER_ERROR,
+ deletedFromDB.getMessage());
+ }
</code_context>
<issue_to_address>
**suggestion (bug_risk):** A race on delete can incorrectly surface as a 500 instead of a 404 when the layer disappears between the existence check and deletion.
You pre-check existence via `loadLayerByName`, then call `deleteLayerByName`. If the row is deleted between those calls, `deleteLayerByName` returns `success=false` with a "not found" message, which you currently map to a 500. For a REST delete, that concurrent "already deleted" case should be treated as 404 (or possibly 204), not an internal error. Consider detecting the `DELETE_LAYER_ENTRY_NOT_FOUND_WITH_NAME` case (or introducing a dedicated flag) and returning 404 instead of 500.
```suggestion
// Delete from database
ValidationOutputData deletedFromDB = experimentDAO.deleteLayerByName(layerName);
if (deletedFromDB.isSuccess()) {
LOGGER.debug(KruizeConstants.LayerAPIMessages.DELETE_LAYER_FROM_DB, layerName);
sendSuccessResponse(
response,
String.format(KruizeConstants.LayerAPIMessages.DELETE_LAYER_SUCCESS_MSG, layerName),
HttpServletResponse.SC_OK
);
} else {
String notFoundMessage = String.format(
AnalyzerErrorConstants.APIErrors.DeleteLayerAPI.DELETE_LAYER_ENTRY_NOT_FOUND_WITH_NAME,
layerName
);
if (notFoundMessage.equals(deletedFromDB.getMessage())) {
// Layer was concurrently deleted between existence check and delete call; treat as 404
sendErrorResponse(response, null, HttpServletResponse.SC_NOT_FOUND, notFoundMessage);
} else {
// Unexpected failure while deleting the layer; surface as 500
sendErrorResponse(
response,
null,
HttpServletResponse.SC_INTERNAL_SERVER_ERROR,
deletedFromDB.getMessage()
);
}
}
```
</issue_to_address>
### Comment 2
<location path="src/main/java/com/autotune/database/dao/ExperimentDAOImpl.java" line_range="738-689" />
<code_context>
+ public ValidationOutputData deleteLayerByName(String layerName) {
</code_context>
<issue_to_address>
**suggestion:** Delete-layer DB method does not use the timer/metrics hooks that were added for other layer DB operations.
Since `updateLayerToDB` is already instrumented with `Timer.Sample` using `timerDeleteLayerDB` and `timerBDeleteLayerDB`, it would be helpful to apply the same pattern here: wrap `deleteLayerByName` in a `Timer.Sample`, tag by status, and stop the timer in a `finally` block to keep metrics consistent and reliable.
Suggested implementation:
```java
@Override
public ValidationOutputData deleteLayerByName(String layerName) {
// Track DB delete-layer timing metrics (same pattern as updateLayerToDB)
Timer.Sample deleteLayerDBSample = Timer.start(meterRegistry);
String deleteLayerDBStatus = KruizeConstants.MetricsDB.DB_OPERATION_STATUS_FAILURE;
ValidationOutputData validationOutputData = new ValidationOutputData(false, null, null);
Transaction tx = null;
try (Session session = KruizeHibernateUtil.getSessionFactory().openSession()) {
try {
tx = session.beginTransaction();
Query query = session.createQuery(DBConstants.SQLQUERY.DELETE_FROM_LAYER_BY_NAME, null);
query.setParameter("layerName", layerName);
int deletedCount = query.executeUpdate();
if (deletedCount == 0) {
```
1. At the point in `deleteLayerByName` where the delete is considered successful (e.g. when `deletedCount > 0` and the transaction is committed, and `validationOutputData` is set to success), set `deleteLayerDBStatus` to the appropriate success constant, mirroring what `updateLayerToDB` uses (for example `KruizeConstants.MetricsDB.DB_OPERATION_STATUS_SUCCESS`).
2. Wrap the entire existing body of `deleteLayerByName` (everything after the opening brace and before the `return validationOutputData;`) in an outer `try`/`finally` block:
- In the `try` block, keep the existing logic unchanged.
- In the `finally` block, stop and record the timer sample in both timers, using the same tag set as `updateLayerToDB`. For example (adapt names/registry to exactly match how `updateLayerToDB` is implemented):
```java
finally {
try {
Timer timer = timerDeleteLayerDB
.tag("status", deleteLayerDBStatus);
deleteLayerDBSample.stop(timer);
} catch (Exception e) {
// optionally log but do not rethrow, to avoid impacting main flow
}
try {
Timer timerB = timerBDeleteLayerDB
.tag("status", deleteLayerDBStatus);
deleteLayerDBSample.stop(timerB);
} catch (Exception e) {
// optionally log
}
}
```
3. Ensure `meterRegistry`, `timerDeleteLayerDB`, `timerBDeleteLayerDB`, and `KruizeConstants.MetricsDB.DB_OPERATION_STATUS_*` are already defined/imported as in `updateLayerToDB`. If any of these are not yet available in `ExperimentDAOImpl`, import and initialize them exactly the same way they are for the update-layer DB metrics.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| // Delete from database | ||
| ValidationOutputData deletedFromDB = experimentDAO.deleteLayerByName(layerName); | ||
|
|
||
| if (deletedFromDB.isSuccess()) { | ||
| LOGGER.debug(KruizeConstants.LayerAPIMessages.DELETE_LAYER_FROM_DB, layerName); | ||
| sendSuccessResponse(response, String.format(KruizeConstants.LayerAPIMessages.DELETE_LAYER_SUCCESS_MSG, layerName), | ||
| HttpServletResponse.SC_OK); | ||
| } else { | ||
| sendErrorResponse(response, null, HttpServletResponse.SC_INTERNAL_SERVER_ERROR, | ||
| deletedFromDB.getMessage()); | ||
| } |
There was a problem hiding this comment.
suggestion (bug_risk): A race on delete can incorrectly surface as a 500 instead of a 404 when the layer disappears between the existence check and deletion.
You pre-check existence via loadLayerByName, then call deleteLayerByName. If the row is deleted between those calls, deleteLayerByName returns success=false with a "not found" message, which you currently map to a 500. For a REST delete, that concurrent "already deleted" case should be treated as 404 (or possibly 204), not an internal error. Consider detecting the DELETE_LAYER_ENTRY_NOT_FOUND_WITH_NAME case (or introducing a dedicated flag) and returning 404 instead of 500.
| // Delete from database | |
| ValidationOutputData deletedFromDB = experimentDAO.deleteLayerByName(layerName); | |
| if (deletedFromDB.isSuccess()) { | |
| LOGGER.debug(KruizeConstants.LayerAPIMessages.DELETE_LAYER_FROM_DB, layerName); | |
| sendSuccessResponse(response, String.format(KruizeConstants.LayerAPIMessages.DELETE_LAYER_SUCCESS_MSG, layerName), | |
| HttpServletResponse.SC_OK); | |
| } else { | |
| sendErrorResponse(response, null, HttpServletResponse.SC_INTERNAL_SERVER_ERROR, | |
| deletedFromDB.getMessage()); | |
| } | |
| // Delete from database | |
| ValidationOutputData deletedFromDB = experimentDAO.deleteLayerByName(layerName); | |
| if (deletedFromDB.isSuccess()) { | |
| LOGGER.debug(KruizeConstants.LayerAPIMessages.DELETE_LAYER_FROM_DB, layerName); | |
| sendSuccessResponse( | |
| response, | |
| String.format(KruizeConstants.LayerAPIMessages.DELETE_LAYER_SUCCESS_MSG, layerName), | |
| HttpServletResponse.SC_OK | |
| ); | |
| } else { | |
| String notFoundMessage = String.format( | |
| AnalyzerErrorConstants.APIErrors.DeleteLayerAPI.DELETE_LAYER_ENTRY_NOT_FOUND_WITH_NAME, | |
| layerName | |
| ); | |
| if (notFoundMessage.equals(deletedFromDB.getMessage())) { | |
| // Layer was concurrently deleted between existence check and delete call; treat as 404 | |
| sendErrorResponse(response, null, HttpServletResponse.SC_NOT_FOUND, notFoundMessage); | |
| } else { | |
| // Unexpected failure while deleting the layer; surface as 500 | |
| sendErrorResponse( | |
| response, | |
| null, | |
| HttpServletResponse.SC_INTERNAL_SERVER_ERROR, | |
| deletedFromDB.getMessage() | |
| ); | |
| } | |
| } |
| @@ -689,6 +689,83 @@ public ValidationOutputData addLayerToDB(KruizeLMLayerEntry kruizeLayerEntry) { | |||
| return validationOutputData; | |||
There was a problem hiding this comment.
suggestion: Delete-layer DB method does not use the timer/metrics hooks that were added for other layer DB operations.
Since updateLayerToDB is already instrumented with Timer.Sample using timerDeleteLayerDB and timerBDeleteLayerDB, it would be helpful to apply the same pattern here: wrap deleteLayerByName in a Timer.Sample, tag by status, and stop the timer in a finally block to keep metrics consistent and reliable.
Suggested implementation:
@Override
public ValidationOutputData deleteLayerByName(String layerName) {
// Track DB delete-layer timing metrics (same pattern as updateLayerToDB)
Timer.Sample deleteLayerDBSample = Timer.start(meterRegistry);
String deleteLayerDBStatus = KruizeConstants.MetricsDB.DB_OPERATION_STATUS_FAILURE;
ValidationOutputData validationOutputData = new ValidationOutputData(false, null, null);
Transaction tx = null;
try (Session session = KruizeHibernateUtil.getSessionFactory().openSession()) {
try {
tx = session.beginTransaction();
Query query = session.createQuery(DBConstants.SQLQUERY.DELETE_FROM_LAYER_BY_NAME, null);
query.setParameter("layerName", layerName);
int deletedCount = query.executeUpdate();
if (deletedCount == 0) {- At the point in
deleteLayerByNamewhere the delete is considered successful (e.g. whendeletedCount > 0and the transaction is committed, andvalidationOutputDatais set to success), setdeleteLayerDBStatusto the appropriate success constant, mirroring whatupdateLayerToDBuses (for exampleKruizeConstants.MetricsDB.DB_OPERATION_STATUS_SUCCESS). - Wrap the entire existing body of
deleteLayerByName(everything after the opening brace and before thereturn validationOutputData;) in an outertry/finallyblock:- In the
tryblock, keep the existing logic unchanged. - In the
finallyblock, stop and record the timer sample in both timers, using the same tag set asupdateLayerToDB. For example (adapt names/registry to exactly match howupdateLayerToDBis implemented):
finally { try { Timer timer = timerDeleteLayerDB .tag("status", deleteLayerDBStatus); deleteLayerDBSample.stop(timer); } catch (Exception e) { // optionally log but do not rethrow, to avoid impacting main flow } try { Timer timerB = timerBDeleteLayerDB .tag("status", deleteLayerDBStatus); deleteLayerDBSample.stop(timerB); } catch (Exception e) { // optionally log } }
- In the
- Ensure
meterRegistry,timerDeleteLayerDB,timerBDeleteLayerDB, andKruizeConstants.MetricsDB.DB_OPERATION_STATUS_*are already defined/imported as inupdateLayerToDB. If any of these are not yet available inExperimentDAOImpl, import and initialize them exactly the same way they are for the update-layer DB metrics.
Description
detailsfield now correctly handles null and missing values usingoptString()getRequiredString()helper method: DRY validation for required string fields with consistent error messagesFixes # (issue)
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
Add full CRUD support and stricter validation for layer management APIs while expanding test coverage and simplifying test cleanup.
New Features:
Bug Fixes:
Enhancements:
Tests: