Copilot commented on code in PR #845:
URL:
https://github.com/apache/rocketmq-dashboard/pull/845#discussion_r3702961799
##########
server/src/main/java/org/apache/rocketmq/studio/settings/SettingsController.java:
##########
@@ -52,13 +53,13 @@ public Result<List<DataSourceVO>> listDataSources() {
}
@PostMapping("/datasources/create")
- public Result<DataSourceVO> createDataSource(@Valid @RequestBody
DataSourceVO dataSource) {
- return Result.ok(settingsService.createDataSource(dataSource));
+ public Result<DataSourceVO> createDataSource(@Valid @RequestBody(required
= false) DataSourceVO dataSource) {
+ return
Result.ok(settingsService.createDataSource(requireDataSource(dataSource)));
}
@PostMapping("/datasources/update")
- public Result<DataSourceVO> updateDataSource(@Valid @RequestBody
DataSourceVO dataSource) {
- return Result.ok(settingsService.updateDataSource(dataSource));
+ public Result<DataSourceVO> updateDataSource(@Valid @RequestBody(required
= false) DataSourceVO dataSource) {
+ return
Result.ok(settingsService.updateDataSource(requireDataSource(dataSource)));
}
Review Comment:
The status code `400` and message string are hard-coded here (and duplicated
in the service). To reduce drift and keep errors consistent, extract these into
a shared constant (e.g., an error-code enum/constant holder) or use
`HttpStatus.BAD_REQUEST.value()` plus a single shared message constant.
##########
server/src/main/java/org/apache/rocketmq/studio/settings/SettingsService.java:
##########
@@ -99,13 +99,19 @@ public List<DataSourceVO> listDataSources() {
public DataSourceVO createDataSource(DataSourceVO dataSource) {
+ if (dataSource == null) {
+ throw new BusinessException(400, "Data source request is
required");
+ }
log.info("Creating data source: {}", dataSource.getName());
dataSource.setKey(UUID.randomUUID().toString());
return settingsRepository.saveDataSource(dataSource);
}
public DataSourceVO updateDataSource(DataSourceVO dataSource) {
+ if (dataSource == null) {
+ throw new BusinessException(400, "Data source request is
required");
+ }
String key = normalizeDataSourceKey(dataSource.getKey());
Review Comment:
Service-layer code throwing an exception that embeds an HTTP status code
(`400`) couples the service API to HTTP semantics. If this service is intended
to be called outside the web layer, consider throwing a domain/argument
exception (or a BusinessException with a domain error code) and mapping it to
HTTP 400 at the controller/advice layer; this keeps transport concerns at the
boundary while preserving the null guard.
##########
server/src/main/java/org/apache/rocketmq/studio/settings/SettingsController.java:
##########
@@ -71,4 +72,11 @@ public Result<Void> deleteDataSource(@RequestParam(required
= false) String key)
public Result<DataSourceTestResultVO> testDataSource(@Valid @RequestBody
DataSourceTestDTO request) {
return Result.ok(settingsService.testDataSource(request));
}
+
+ private DataSourceVO requireDataSource(DataSourceVO dataSource) {
+ if (dataSource == null) {
+ throw new BusinessException(400, "Data source request is
required");
+ }
+ return dataSource;
+ }
Review Comment:
The status code `400` and message string are hard-coded here (and duplicated
in the service). To reduce drift and keep errors consistent, extract these into
a shared constant (e.g., an error-code enum/constant holder) or use
`HttpStatus.BAD_REQUEST.value()` plus a single shared message constant.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
