[Fix_1578] malformed JSON from HTTP 200 is not normalized into WorkflowError error with Quarkus Rest client

[gmunozfe]

Fixes #1578

[Copilot]

🟡 Not ready to approve

The new WebApplicationException handling can misclassify malformed-JSON parsing failures as COMMUNICATION (potentially even with status 200) instead of the existing DATA(422) normalization, and it risks leaking the exception response if not closed.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Pull request overview

This PR aims to normalize client-side JSON parsing failures (notably with Quarkus REST client) into consistent WorkflowError instances, matching the behavior seen with the Jersey provider for malformed JSON in HTTP 200 responses.

Changes:

• Adds handling for WebApplicationException during HTTP task execution to wrap it into a WorkflowException.
• Uses the response status from WebApplicationException to build a communication-oriented WorkflowError.

File summaries

File	Description
impl/http/src/main/java/io/serverlessworkflow/impl/executors/http/AbstractRequestExecutor.java	Adds WebApplicationException handling when invoking requests / reading entities to improve error normalization.

Review details

• Files reviewed: 1/1 changed files
• Comments generated: 1
• Review effort level: Low

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

[Copilot]

🟡 Not ready to approve

The new WebApplicationException handling can misclassify malformed JSON from successful (2xx) responses as a COMMUNICATION error with status 200 (and may leak the exception Response), which conflicts with the intended DATA/422 normalization.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Review details

Comments suppressed due to low confidence (1)

impl/http/src/main/java/io/serverlessworkflow/impl/executors/http/AbstractRequestExecutor.java:71

• The new WebApplicationException handler maps successful (e.g., HTTP 200) responses that fail during entity parsing into a COMMUNICATION error with status 200. For malformed JSON from a 2xx response, this diverges from the existing ProcessingException path (Errors.DATA / 422) and from the issue’s expected normalization into a DATA workflow error. It also leaves the exception Response unclosed when invokeRequest/readEntity throws before the try-with-resources acquires a Response.

    } catch (WebApplicationException ex) {
      throw new WorkflowException(
          WorkflowError.communication(ex.getResponse().getStatus(), task, ex).build(), ex);
    }

• Files reviewed: 1/1 changed files
• Comments generated: 0 new
• Review effort level: Low

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

[Copilot]

🟡 Not ready to approve

The updated exception mapping currently contains a compile-breaking WorkflowError.communication(Errors.DATA.toString(), ...) call and the WebApplicationException handling can produce a WorkflowError with HTTP 200 (and doesn’t close the embedded response), which conflicts with the #1578 normalization goal.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Review details

Comments suppressed due to low confidence (1)

impl/http/src/main/java/io/serverlessworkflow/impl/executors/http/AbstractRequestExecutor.java:66

• This maps WebApplicationException to a workflow error using the underlying response status verbatim; if the exception wraps a successful HTTP response (e.g., 200 with malformed JSON), this can generate a WorkflowError with status 200, which doesn’t satisfy the #1578 normalization expectation (422 for unparseable JSON). Also, the Response inside WebApplicationException is not closed here, which can leak the entity stream/connection.

    } catch (WebApplicationException ex) {
      throw new WorkflowException(
          WorkflowError.communication(ex.getResponse().getStatus(), task, ex).build(), ex);

• Files reviewed: 1/1 changed files
• Comments generated: 1
• Review effort level: Low

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

[Copilot]

🟡 Not ready to approve

The new normalization drops the error detail payload compared to prior behavior and lacks a regression test for the malformed-JSON-on-200 scenario described in #1578.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Review details

Comments suppressed due to low confidence (3)

impl/http/src/main/java/io/serverlessworkflow/impl/executors/http/AbstractRequestExecutor.java:66

• Same as above: WorkflowError.communication(..., ex) sets the exception message as title but leaves detail null. Populate detail as well so the error payload consistently contains details for response-processing failures.

    } catch (WebApplicationException ex) {
      throw new WorkflowException(
          WorkflowError.communication(ex.getResponse().getStatus(), task, ex).build(), ex);

impl/http/src/main/java/io/serverlessworkflow/impl/executors/http/AbstractRequestExecutor.java:63

• This normalization no longer populates the WorkflowError detail field (previously details(ex.getMessage())); WorkflowError.communication(..., ex) only sets the message as the title, leaving detail null. If callers / retry catch filters rely on detail/details (and the issue’s expected JSON includes details), this is a behavior regression. Consider also setting .details(ex.getMessage()) here.

This issue also appears on line 64 of the same file.

    } catch (ProcessingException | IllegalStateException ex) {
      throw new WorkflowException(
          WorkflowError.communication(Errors.DATA.status(), task, ex).build(), ex);

impl/http/src/main/java/io/serverlessworkflow/impl/executors/http/AbstractRequestExecutor.java:66

• The new exception-to-WorkflowError normalization path isn’t covered by tests. Since there are already HTTP executor tests in HTTPWorkflowDefinitionTest, add a test case for 200 OK + Content-Type: application/json + malformed JSON body asserting that a WorkflowException is raised and the resulting WorkflowError is normalized as intended (type/status and details/title).

    } catch (ProcessingException | IllegalStateException ex) {
      throw new WorkflowException(
          WorkflowError.communication(Errors.DATA.status(), task, ex).build(), ex);
    } catch (WebApplicationException ex) {
      throw new WorkflowException(
          WorkflowError.communication(ex.getResponse().getStatus(), task, ex).build(), ex);

• Files reviewed: 1/1 changed files
• Comments generated: 0 new
• Review effort level: Low

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

[Copilot]

🟡 Not ready to approve

The new WebApplicationException mapping can yield a WorkflowError with status=200 on parse failures (breaking downstream error filtering), and the behavior should be covered by a regression test for malformed JSON on HTTP 200.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Review details

Comments suppressed due to low confidence (2)

impl/http/src/main/java/io/serverlessworkflow/impl/executors/http/AbstractRequestExecutor.java:66

• The WebApplicationException path propagates ex.getResponse().getStatus() directly into the WorkflowError. If the client throws WebApplicationException while processing a 2xx response (e.g., malformed JSON on HTTP 200), this will produce an error with status=200, which is misleading and can break Catch/Try filtering that matches on error.status() (see TryExecutor.java:232). Also, switching from .details(ex.getMessage()) to communication(..., ex) moves the exception message into title and leaves detail null, which changes what users can match on via error.details/detail. Consider normalizing successful-status exceptions to a non-2xx status (e.g., Errors.DATA.status()) and populating .details(...) for parity with the previous behavior.

      throw new WorkflowException(
          WorkflowError.communication(Errors.DATA.status(), task, ex).build(), ex);
    } catch (WebApplicationException ex) {
      throw new WorkflowException(
          WorkflowError.communication(ex.getResponse().getStatus(), task, ex).build(), ex);

impl/http/src/main/java/io/serverlessworkflow/impl/executors/http/AbstractRequestExecutor.java:66

• This change is addressing a client-specific failure mode (malformed JSON with Content-Type: application/json on HTTP 200) but there doesn't appear to be an existing test covering the invalid-JSON/parse-failure path in the HTTP executor. Adding an integration test (similar to the other MockWebServer cases in HTTPWorkflowDefinitionTest) that returns a 200 with malformed JSON and asserts the resulting WorkflowException has a normalized non-2xx WorkflowError.status() would help prevent regressions across REST client implementations.

      throw new WorkflowException(
          WorkflowError.communication(Errors.DATA.status(), task, ex).build(), ex);
    } catch (WebApplicationException ex) {
      throw new WorkflowException(
          WorkflowError.communication(ex.getResponse().getStatus(), task, ex).build(), ex);

• Files reviewed: 1/1 changed files
• Comments generated: 0 new
• Review effort level: Low

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.