Skip to content

[Fix-18687][HTTP Task] Preserve response bodies for non-200 status codes - #18688

Open
csurong wants to merge 3 commits into
apache:devfrom
csurong:Fix-18687
Open

csurong wants to merge 3 commits into
apache:devfrom
csurong:Fix-18687

Conversation

@csurong

@csurong csurong commented Oct 9, 2026 •

Copy link
Copy Markdown
Contributor

Was this PR generated or assisted by AI?

YES. AI assisted with investigation, the code change, regression tests, documentation, and this description. The contributor reviewed the scope and approved submission.

Purpose of the pull request

HTTP tasks currently replace response bodies for non-200 statuses with an error string containing a Java object reference. A POST returning 201 can pass custom status validation while losing its JSON output, and a 202 response can fail a matching body-content check.

Fixes #18687

Brief change log

  • Read the actual response body whenever it exists, regardless of status code. Return an empty string for a missing body. Keep status validation in the callers.
  • Add HTTP task regression tests for 201 output preservation, 202 body validation, default status rejection for 201/400/500, and an empty 204 response.
  • Cover BODY_CONTAINS and BODY_NOT_CONTAINS on 400/500 responses with matching, non-matching, and empty bodies; directly test absent ResponseBody handling.
  • Document validation semantics in the English/Chinese HTTP task guides and the upgrade compatibility notes.

Behavior and compatibility

BODY_CONTAINS and BODY_NOT_CONTAINS inspect the body only; neither enforces an HTTP status code. With the actual body restored, a 4xx/5xx response can pass a body check. For example, HTTP 500 with body {"status":"success"} passes BODY_CONTAINS success, and a nonempty error body without that keyword passes BODY_NOT_CONTAINS success. Both checks continue to reject empty bodies.

Existing workflows that matched the synthetic error text, or relied on it not containing a keyword, may change results. Choose status-code validation when an error status must fail the task. Default validation still requires 200, and custom validation still requires the configured code; this PR does not combine status and body validation.

The shared utility also affects HTTP alerts and OAuth login. HTTP alerts retain their explicit 200-only status check, but error diagnostics now include the actual body. OAuth receives the actual body for its existing JSON parsing; no additional OAuth status policy is introduced here.

Verify this pull request

The two original reproductions failed before the fix. The new absent-body test also failed before changing the null fallback. After the fix, all 16 HttpTaskTest tests and 4 OkHttpUtilsTest tests passed using Java 11:

mvn -B -ntp -o -pl dolphinscheduler-task-plugin/dolphinscheduler-task-http -am -Dtest=HttpTaskTest,OkHttpUtilsTest -Dsurefire.failIfNoSpecifiedTests=false clean test

Spotless apply/check passed for dolphinscheduler-common and dolphinscheduler-task-http; git diff --check passed.

Tests use a local MockWebServer HTTP connection and a mocked task execution context. The absent-body case uses a constructed OkHttp Response because a normal network response has a non-null ResponseBody. A full deployed workflow and OAuth/alert integration were not tested.

@det101

det101 commented Oct 10, 2026

Copy link
Copy Markdown
Contributor

OkHttpUtils.getResponseBody() is shared by the HTTP task, the HTTP alert sender, and OAuth login. After this change, a non-200 response returns the real body instead of the synthetic Request execute failed... string.

HTTP task BODY_CONTAINS / BODY_NOT_CONTAINS do not look at the status code. Previously a 4xx/5xx body was that synthetic string, so a contains-check almost never matched. Now a 500 whose body contains the configured keyword is treated as success. The 202 test shows this is intended for accepted responses; please mention the 4xx/5xx behavior in the PR description so users are not surprised.

The body == null branch still returns Request execute failed, httpBody: null. Call.execute() usually has a non-null body, but if it is null, returning an empty string would avoid labeling a successful empty response as a failed request.

@csurong

csurong commented Oct 10, 2026

Copy link
Copy Markdown
Contributor Author

@det101 Thanks for the feedback. Updated the PR description and docs to clarify the 4xx/5xx body-check behavior, and changed the null-body fallback to an empty string. Added regression tests for both cases; all 20 related tests pass.

@SbloodyS SbloodyS added this to the 3.5.0 milestone Oct 10, 2026
@SbloodyS SbloodyS added the bug Something isn't working label Oct 10, 2026

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backend bug Something isn't working document test

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] [HTTP Task] Non-200 responses lose their actual response body

3 participants