refactor: Extract helpers to eliminate duplicated HTTP handler boilerplate in TaskResource

[amitkdutta]

Summary:
TaskResource.cpp had significant code duplication across its 9 HTTP handler
methods. This extracts three helpers to reduce boilerplate:

1. executeAndRespond() - Template that creates a CallbackRequestHandler
   wrapping the full folly::via -> thenValue -> thenError pipeline for simple
   fire-and-forget handlers. Used by abortResults, acknowledgeResults, and
   removeRemoteSource, reducing each from ~30 lines to ~5 lines.

2. sendPrestoResponse<T, ThriftT>() - Consolidates the Thrift/JSON content
   negotiation dispatch pattern that was copy-pasted in 4 handlers
   (createOrUpdateTaskImpl, deleteTask, getTaskStatus, getTaskInfo).

3. shouldUseThrift() - Extracts the repeated Accept header check for Thrift
   content type, used in those same 4 handlers.

Net reduction of 60 lines (76 insertions, 136 deletions) with no behavioral
changes.

== NO RELEASE NOTE ==

Summary by Sourcery

Refactor TaskResource HTTP handlers to remove duplicated response and handler boilerplate while preserving existing behavior.

Enhancements:

• Introduce a shared helper to detect Thrift vs JSON responses from request headers and reuse it across task endpoints.
• Add a generic response helper to send either Thrift or JSON-encoded Presto responses for task-related handlers.
• Add a common executor-based helper for simple fire-and-forget task operations that returns a standardized HTTP OK or error response.

[sourcery-ai]

Reviewer's Guide

Refactors TaskResource HTTP handlers by extracting shared helper utilities for Thrift/JSON response handling and asynchronous "fire-and-forget" operations, thereby reducing duplicated boilerplate while preserving existing behavior.

Sequence diagram for executeAndRespond-based HTTP handlers

sequenceDiagram
    actor Client
    participant HttpServer
    participant TaskResource
    participant CallbackRequestHandler
    participant Executor
    participant EventBase
    participant TaskManager
    participant ResponseHandler

    Client->>HttpServer: HTTP request to abortResults / acknowledgeResults / removeRemoteSource
    HttpServer->>TaskResource: route to handler method
    TaskResource->>TaskResource: executeAndRespond(httpSrvCpuExecutor_, workFn)
    TaskResource-->>HttpServer: CallbackRequestHandler

    Client->>CallbackRequestHandler: HTTP request handling callback
    CallbackRequestHandler->>Executor: folly::via(executor, workFn)
    Executor->>TaskManager: workFn (e.g. abortResults / acknowledgeResults / removeRemoteSource)
    TaskManager-->>Executor: workFn completed
    Executor->>EventBase: via(EventBase keepAlive)

    EventBase->>CallbackRequestHandler: thenValue
    CallbackRequestHandler->>CallbackRequestHandler: check handlerState.requestExpired()
    alt not expired
        CallbackRequestHandler->>ResponseHandler: http::sendOkResponse
    else expired
        CallbackRequestHandler-->>ResponseHandler: no response
    end

    Executor->>CallbackRequestHandler: thenError(std::exception)
    CallbackRequestHandler->>CallbackRequestHandler: check handlerState.requestExpired()
    alt not expired
        CallbackRequestHandler->>ResponseHandler: http::sendErrorResponse(e.what())
    else expired
        CallbackRequestHandler-->>ResponseHandler: no response
    end

Loading

Class diagram for TaskResource and extracted HTTP helper utilities

classDiagram
    class TaskResource {
        - taskManager_ : TaskManager
        - httpSrvCpuExecutor_ : folly_Executor
        + registerUris(server : HttpServer) : void
        + abortResults(message : HTTPMessage, pathMatch : vector_string) : RequestHandler*
        + acknowledgeResults(message : HTTPMessage, pathMatch : vector_string) : RequestHandler*
        + removeRemoteSource(message : HTTPMessage, pathMatch : vector_string) : RequestHandler*
        + createOrUpdateTaskImpl(message : HTTPMessage, pathMatch : vector_string) : RequestHandler*
        + deleteTask(message : HTTPMessage, pathMatch : vector_string) : RequestHandler*
        + getTaskStatus(message : HTTPMessage, pathMatch : vector_string) : RequestHandler*
        + getTaskInfo(message : HTTPMessage, pathMatch : vector_string) : RequestHandler*
    }

    class TaskManager {
        + abortResults(taskId : TaskId, destination : long) : void
        + acknowledgeResults(taskId : TaskId, bufferId : long, token : long) : void
        + removeRemoteSource(taskId : TaskId, remoteId : string) : void
        + deleteTask(taskId : TaskId, abort : bool, summarize : bool) : unique_ptr_TaskInfo
        + updateOrCreateTask(...) : unique_ptr_TaskInfo
        + getTaskStatus(...) : unique_ptr_TaskStatus
        + getTaskInfo(...) : unique_ptr_TaskInfo
    }

    class TaskResourceHelpers {
        + shouldUseThrift(message : HTTPMessage) : bool
        + sendPrestoResponse_T_ThriftT(downstream : ResponseHandler*, data : T, sendThrift : bool) : void
        + executeAndRespond_WorkFn(executor : folly_Executor*, workFn : WorkFn) : RequestHandler*
    }

    class http_CallbackRequestHandler {
        + http_CallbackRequestHandler(callback : function) 
        + onRequest(message : HTTPMessage*) : void
        + onBody(body : IOBuf) : void
        + onEOM() : void
    }

    class ResponseHandler {
        + sendHeaders(...) : void
        + sendBody(...) : void
        + sendEOM() : void
    }

    class folly_Executor {
        + add(func : function) : void
    }

    TaskResource --> TaskManager : uses
    TaskResource --> TaskResourceHelpers : uses
    TaskResource --> http_CallbackRequestHandler : creates
    TaskResourceHelpers --> http_CallbackRequestHandler : returns
    http_CallbackRequestHandler --> ResponseHandler : writes responses
    TaskResourceHelpers --> folly_Executor : schedules work
    TaskResourceHelpers --> ResponseHandler : sendPrestoResponse
    TaskManager <.. TaskResourceHelpers : workFn calls methods on

Loading

File-Level Changes

Change	Details	Files
Introduce reusable helpers to centralize Thrift vs JSON response handling and Accept-header negotiation.	Add shouldUseThrift() helper that inspects the HTTP Accept header for the Thrift MIME type. Add templated sendPrestoResponse<T, ThriftT>() helper to perform toThrift conversion and send either Thrift or JSON OK responses. Update createOrUpdateTaskImpl, deleteTask, getTaskStatus, and getTaskInfo handlers to use shouldUseThrift() and sendPrestoResponse instead of inlined content negotiation logic.	presto-native-execution/presto_cpp/main/TaskResource.cpp
Abstract the common async execution-and-empty-OK-response pattern into a single reusable helper for simple handlers.	Add executeAndRespond() templated helper that wraps a work lambda in a CallbackRequestHandler, runs it on the provided executor with folly::via, and sends an empty OK response or an error response based on the result. Refactor abortResults, acknowledgeResults, and removeRemoteSource handlers to use executeAndRespond() instead of duplicating the folly::via/thenValue/thenError pipeline.	presto-native-execution/presto_cpp/main/TaskResource.cpp

---

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

• Contact our support team for questions or feedback.
• Visit our documentation for detailed guides and information.
• Keep in touch with the Sourcery team by following us on X/Twitter, LinkedIn or GitHub.

[sourcery-ai]

Hey - I've left some high level feedback:

• The executeAndRespond helper assumes a void WorkFn (per comment) but doesn’t enforce it; consider constraining the template (e.g., via static_assert(std::is_void_v<std::invoke_result_t<WorkFn>>) or a concept) to avoid accidentally ignoring non-void return values in future uses.
• In executeAndRespond, you move work into folly::via from a lambda capture; if this handler is ever reused or copied in unexpected ways, this double-move pattern could be fragile—consider wrapping the work in a small std::function<void()> or folly::Func to make the ownership semantics clearer.
• The new sendPrestoResponse helper duplicates the sendThrift boolean decision at each call site; you might consider an overload that takes the HTTPMessage and internally calls shouldUseThrift to further reduce boilerplate and keep the negotiation logic centralized.

Prompt for AI Agents

Please address the comments from this code review:

## Overall Comments
- The `executeAndRespond` helper assumes a void `WorkFn` (per comment) but doesn’t enforce it; consider constraining the template (e.g., via `static_assert(std::is_void_v<std::invoke_result_t<WorkFn>>)` or a concept) to avoid accidentally ignoring non-void return values in future uses.
- In `executeAndRespond`, you move `work` into `folly::via` from a lambda capture; if this handler is ever reused or copied in unexpected ways, this double-move pattern could be fragile—consider wrapping the work in a small `std::function<void()>` or `folly::Func` to make the ownership semantics clearer.
- The new `sendPrestoResponse` helper duplicates the `sendThrift` boolean decision at each call site; you might consider an overload that takes the `HTTPMessage` and internally calls `shouldUseThrift` to further reduce boilerplate and keep the negotiation logic centralized.

---

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

• X
• Mastodon
• LinkedIn
• Facebook

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

[aditi-pandit]

Thanks @amitkdutta