Fix HttpOperator deferrable pagination returning only the last page instead of all page - #72388
Conversation
SameerMesiah97
left a comment
There was a problem hiding this comment.
Just one nit but lets wait for CI.
In deferred mode, execute_complete() discarded the accumulated list that paginate_async() builds on the final resume and instead called process_response() again with just the current page's response. This made deferrable pagination return only the last page while the synchronous path correctly returns every page. The existing test for this path put both execute_complete() calls inside a single contextlib.suppress(TaskDeferred) block, so the first call's TaskDeferred exited the block before the second call and its assertion ever ran, silently passing.
73a4938 to
d2f4c08
Compare
64815d0 to
d2f4c08
Compare
|
Apologies, I forgot to switch branches and accidentally pushed an unrelated commit. I have restored the branch to its original state. I'm very sorry for the noise and for mistakenly tagging the code owners. |
potiuk
left a comment
There was a problem hiding this comment.
Verified both halves of this.
The bug is worse than a discarded return value: on every page except the last, paginate_async raises TaskDeferred from self.defer(...), so the trailing return self.process_response(context, response=response) was only ever reached on the final resume — returning the last page's text as a string, where execute_sync returns a list of every page (test_pagination asserts result == [5, 10]).
The dead-test diagnosis is right too. contextlib.suppress(TaskDeferred) wrapped both calls and the first one raises, so the with block exited there and the second execute_complete plus its assertion never ran. Even had they run, the old fixture served identical content for both pages, so the assertion could not have distinguished "all pages" from "last page twice". Switching to iter([5, 10]) is what actually gives the test discriminating power.
One upside worth recording: the old code called process_response twice on the final page — once with the accumulated list (discarded) and once with the lone response — so response_check ran twice, first against the list, and could raise "Response check returned False." from the evaluation whose result was being thrown away. response_filter likewise. Now it runs once, against the list, matching sync.
I checked that paginate_async has no other callers or overrides in the repo, and that both surviving paths match execute_sync exactly.
Note for the release note: this changes the XCom shape for anyone running paginated HttpOperator with deferrable=True — string today, list after. That is the fix, but it is worth saying plainly.
Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting
Sumarry
In
execute_complete(), the return value ofpaginate_async()was discarded, and the code calledprocess_response()a second time with just the current page'sresponse.paginate_async()already callsprocess_response()itself with the full accumulatedall_responseslist once pagination ends, so its result was thrown away and immediately overwritten with a single-page result.The practical effect: in synchronous mode,
HttpOperatorwith apagination_functionreturns a list of every page's response. In deferrable mode, it silently returned only the last page. Any Dag that switched a paginatedHttpOperatortodeferrable=Truewould quietly lose all pages but the last one, with no error raised.The existing regression test,
test_async_pagination, could not catch this because bothexecute_complete()calls were wrapped in a singlecontextlib.suppress(TaskDeferred)block. The first call raisesTaskDeferred(correctly, to request the next page), which is suppressed — but that also exits thewithblock immediately, so the secondexecute_complete()call and itsassert result == [...]never executed. The test always reported as passing regardless of what the code actually returned.Changes
paginate_async()now owns the full response-building/return logic (including the non-paginated case, which previously lived inexecute_complete()), andexecute_complete()simply returns its result.test_async_paginationto usepytest.raises(TaskDeferred)around each deferring call individually, and assert on the final, non-raising resume call's result — so the test actually exercises the code path instead of skipping it.test_async_pagination_with_response_filterto cover aresponse_filterreceiving the full accumulated page list across three pages (two deferrals, one final page).test_async_execute_complete_without_pagination_returns_single_responseto lock in the unpaginated resume behavior, which is unchanged but now goes through the refactoredpaginate_async().Was generative AI tooling used to co-author this PR?