You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by GitBox <gi...@apache.org> on 2021/12/06 05:35:52 UTC
[GitHub] [druid] kfaraz commented on a change in pull request #12026: Fix vulnerabilities in some HTTP endpoints
kfaraz commented on a change in pull request #12026:
URL: https://github.com/apache/druid/pull/12026#discussion_r762712632
##########
File path: indexing-service/src/main/java/org/apache/druid/indexing/seekablestream/SeekableStreamIndexTaskRunner.java
##########
@@ -1637,30 +1631,23 @@ public Response setEndOffsets(
resume();
return Response.ok(sequenceNumbers).build();
} else if (latestSequence.isCheckpointed()) {
- return Response.status(Response.Status.BAD_REQUEST)
- .entity(StringUtils.format(
+ return ResponseStatusExceptionMapper.toResponse(Response.Status.BAD_REQUEST,
Review comment:
_Nit: style choice_
Maybe break the line after `.toResponse(`, would look more readable if the arguments start from a new line.
##########
File path: indexing-service/src/main/java/org/apache/druid/indexing/overlord/http/OverlordResource.java
##########
@@ -569,9 +557,7 @@ public Response getTasks(
//check for valid state
if (state != null) {
if (!API_TASK_STATES.contains(StringUtils.toLowerCase(state))) {
- return Response.status(Status.BAD_REQUEST)
- .entity(StringUtils.format("Invalid state : %s, valid values are: %s", state, API_TASK_STATES))
- .build();
+ throw new BadRequestException(StringUtils.format("Invalid state : %s, valid values are: %s", state, API_TASK_STATES));
Review comment:
Same comment of change from returning a `Response` to throwing an exception.
##########
File path: indexing-service/src/main/java/org/apache/druid/indexing/seekablestream/SeekableStreamIndexTaskRunner.java
##########
@@ -1637,30 +1631,23 @@ public Response setEndOffsets(
resume();
return Response.ok(sequenceNumbers).build();
} else if (latestSequence.isCheckpointed()) {
- return Response.status(Response.Status.BAD_REQUEST)
- .entity(StringUtils.format(
+ return ResponseStatusExceptionMapper.toResponse(Response.Status.BAD_REQUEST,
+ StringUtils.format(
"Sequence [%s] has already endOffsets set, cannot set to [%s]",
latestSequence,
- sequenceNumbers
- )).build();
+ sequenceNumbers));
} else if (!isPaused()) {
- return Response.status(Response.Status.BAD_REQUEST)
- .entity("Task must be paused before changing the end offsets")
- .build();
+ return ResponseStatusExceptionMapper.toResponse(Response.Status.BAD_REQUEST,
+ "Task must be paused before changing the end offsets");
}
for (Map.Entry<PartitionIdType, SequenceOffsetType> entry : sequenceNumbers.entrySet()) {
if (createSequenceNumber(entry.getValue()).compareTo(createSequenceNumber(currOffsets.get(entry.getKey())))
< 0) {
- return Response.status(Response.Status.BAD_REQUEST)
- .entity(
- StringUtils.format(
- "End sequence must be >= current sequence for partition [%s] (current: %s)",
- entry.getKey(),
- currOffsets.get(entry.getKey())
- )
- )
- .build();
+ return ResponseStatusExceptionMapper.toResponse(Response.Status.BAD_REQUEST,
Review comment:
_Nit: same format suggestion as above_
##########
File path: indexing-service/src/main/java/org/apache/druid/indexing/overlord/http/OverlordResource.java
##########
@@ -177,25 +179,13 @@ public Response taskPost(final Task task, @Context final HttpServletRequest req)
return asLeaderWith(
taskMaster.getTaskQueue(),
- new Function<TaskQueue, Response>()
- {
- @Override
- public Response apply(TaskQueue taskQueue)
- {
- try {
- taskQueue.add(task);
- return Response.ok(ImmutableMap.of("task", task.getId())).build();
- }
- catch (EntryExistsException e) {
- return Response.status(Response.Status.BAD_REQUEST)
- .entity(
- ImmutableMap.of(
- "error",
- StringUtils.format("Task[%s] already exists!", task.getId())
- )
- )
- .build();
- }
+ taskQueue -> {
+ try {
+ taskQueue.add(task);
+ return Response.ok(ImmutableMap.of("task", task.getId())).build();
+ }
+ catch (EntryExistsException e) {
+ throw new BadRequestException(StringUtils.format("Task[%s] already exists!", task.getId()));
Review comment:
Earlier, this was returning a response like `return Response.status(BAD_REQUEST).entity().build()`. Shouldn't we continue to return a `Response` here instead of throwing an exception?
##########
File path: indexing-service/src/main/java/org/apache/druid/indexing/overlord/http/OverlordResource.java
##########
@@ -468,9 +458,7 @@ public Response getWorkerConfigHistory(
return Response.ok(workerEntryList).build();
}
catch (IllegalArgumentException e) {
- return Response.status(Response.Status.BAD_REQUEST)
- .entity(ImmutableMap.<String, Object>of("error", e.getMessage()))
- .build();
+ throw new BadRequestException(e.getMessage());
Review comment:
Same comment of change from returning a `Response` to throwing an exception.
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org