Skip to content

Keep MSGraph request configuration across paginated pages - #71649

Open
SEPURI-SAI-KRISHNA wants to merge 2 commits into
apache:mainfrom
SEPURI-SAI-KRISHNA:fix-msgraph-pagination-request-config
Open

Keep MSGraph request configuration across paginated pages#71649
SEPURI-SAI-KRISHNA wants to merge 2 commits into
apache:mainfrom
SEPURI-SAI-KRISHNA:fix-msgraph-pagination-request-config

Conversation

@SEPURI-SAI-KRISHNA

Copy link
Copy Markdown
Contributor

MSGraphAsyncOperator defers with MSGraphTrigger from two places. The initial defer in
execute passes the full request configuration. The pagination defer in
trigger_next_link, used for every @odata.nextLink page after the first, leaves out
headers, data and scopes:

self.defer(
    trigger=MSGraphTrigger(
        url=url,
        method=self.method,
        query_parameters=query_parameters,
        response_type=self.response_type,
        conn_id=self.conn_id,
        ...
    ),
)

All three default to None on the trigger, and MSGraphTrigger.run() uses them directly
when issuing the request, so nothing restores them. Page 1 is sent as configured and every
subsequent page is not.

The clearest symptom of the inconsistency is that method is forwarded. An operator
configured with a POST and a body therefore re-issues that POST on page 2 with
data=None — the verb is preserved while its body is dropped.

headers matters for the same reason in ordinary use: Microsoft Graph requires
ConsistencyLevel: eventual for $count and $search queries, so a query that works on
the first page can start failing or returning inconsistent results partway through
pagination. scopes feeds token acquisition, so follow-up pages could authenticate with
different permissions than the first.

path_parameters and url_template are deliberately left out of this change. They exist
to build a URL, and pagination already has an absolute nextLink, so not forwarding them
is correct rather than an oversight.

No newsfragment: this is a provider change, and provider changelogs are regenerated from
git log by the release manager.


Was generative AI tooling used to co-author this PR?
  • Yes — Claude Code (Opus 5)

Generated-by: Claude Code (Opus 5) following the guidelines

@fat-catTW

Copy link
Copy Markdown
Contributor

LGTM, thanks for the contribution.
It may be worth adding a slightly more end-to-end regression test for the paginated path. The current test covers the immediate bug by verifying that trigger_next_link() forwards headers, data, and scopes onto the deferred trigger. A fuller regression test could also run the created MSGraphTrigger and assert that the original request configuration is used when issuing the next-page request.

@SameerMesiah97 SameerMesiah97 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the test is fine as is. I am ambivalent regarding whether or not all parameters need to be covered as the only 3 parameters you have covered are actually relevant to the bug.

The pagination defer rebuilt the trigger without the headers, body and scopes
the operator was configured with, so only the first page of a paginated Graph
query was issued as intended. The HTTP method was still forwarded, which meant
a POST could be repeated on later pages with its body dropped.
The existing test asserts the deferred trigger is built with the operator's
  headers, body and scopes. This adds the complementary check that those survive
  all the way to the request issued for the second page, so a future regression
  in the pagination path is caught at the layer users actually observe.
@SEPURI-SAI-KRISHNA
SEPURI-SAI-KRISHNA force-pushed the fix-msgraph-pagination-request-config branch from f90f2cd to c03bc05 Compare August 15, 2026 16:10
@SEPURI-SAI-KRISHNA

Copy link
Copy Markdown
Contributor Author

Thanks both for the reviews.

Good suggestion, I've added that test. It drives the operator through the full deferrable loop across both pages and asserts on the RequestInformation that actually reaches the request adapter, rather than on the trigger that was constructed:

def test_pagination_issues_every_page_with_the_configured_request(self):
    ...
    requests = [call.args[0] for call in mock_get_http_response.call_args_list]

    assert len(requests) == 2
    for request in requests:
        assert request.headers.try_get("ConsistencyLevel") == {"eventual"}
        assert request.content == json.dumps(data).encode("utf-8")

I configured the operator with method="POST" so the failure is unambiguous. Before the fix, the two requests come out like this:

page 1 page 2
method POST POST
ConsistencyLevel eventual absent
body {"requestBody": "value"} None

which is the shape of the bug: the verb is carried over to the next page while the body and headers that gave it meaning are not.

I kept the existing test_trigger_next_link_forwards_the_request_configuration alongside it rather than replacing it. scopes feeds token acquisition and never appears on the outgoing RequestInformation, so it isn't observable at
the request layer, the trigger-level assertion is the only place it can be covered. The two tests together cover all three forwarded arguments.

Both fail with the source change reverted and pass with it applied (13 pre-existing tests in the file are unaffected either way).

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants