Add Azure Analysis Services model refresh support - #71350
Conversation
dabla
left a comment
There was a problem hiding this comment.
The PR adds a well-structured set of components (Hook, Operator, Sensor, Trigger) for Azure Analysis Services model refreshes. The architecture is sound: the trigger correctly offloads synchronous HTTP calls to asyncio.to_thread, deferrable mode is properly implemented, and the test suite is thorough.
Two items must be fixed before merge:
-
_get_base_urlallowsuserinfoand port injection (CWE-918). The host validation explicitly guards against path/query/fragment but silently acceptsuser@host(userinfo) andhost:portinputs, which can redirect the AAS bearer token to an attacker-controlled server. Addingor parsed_host.userinfo or parsed_host.portto the guard closes the gap. A test case should also be added. -
requestsmust not be added as a dependency.httpxis already provided by the provider's transitive dependency onkiota-httpand is the established HTTP client in this provider. The tworequests.get/requests.postcalls are straightforward to replace with thehttpxsync API — see the inline comment for the exact diff.
Drafted-by: Claude Sonnet 4.6 (claude-sonnet-4.6); reviewed by @dabla before posting
|
I learned a lot from this round of reviews, many thanks 😄 |
…ervices integration
…d _get_credential for service principal credential
… _assert_host method
…te tests for full lifecycle execution
3e858bd to
c52856f
Compare
dabla
left a comment
There was a problem hiding this comment.
The design is solid: a purely async, deferred-only Hook/Operator/Sensor/Trigger set for Azure Analysis Services refreshes, correctly avoiding worker-slot blocking, with httpx.AsyncClient used natively (no asyncio.to_thread workaround needed), proper aclose() resource cleanup of both the HTTP client and the credential, and thorough test coverage including full deferred-lifecycle execution via execute_operator. The two concerns raised in earlier review rounds (_get_base_url userinfo/port injection, and adding requests as a redundant dependency alongside httpx) have both been correctly addressed in later commits (_assert_host now rejects credentials/ports in the netloc, and the hook is httpx-only). _get_headers correctly builds the Authorization header as an OAuth bearer credential using the fetched token's .token attribute.
One design suggestion (see comment [1]): move the trigger's polling loop into an async hook method, mirroring the split used in #72336 for SFTPTransferTrigger/SFTPHookAsync.transfer(). This keeps run() a thin dispatcher and makes the polling logic independently testable. Not a blocker - a worthwhile follow-up polish either in this PR or a fast-follow.
* Add Azure Analysis Services model refresh support
* Add Azure Analysis Services model refresh support
closes: #51377
Summary
azure_analysis_servicesconnection type using Microsoft Entra service-principal client-secret authentication.Testing
Run the system-test Dag in Breeze against a live Azure Analysis Services instance:
calculaterefreshesThe Dag runs a fire-and-forget Operator, a deferrable-enabled Sensor, and a deferrable-enabled Operator. All three task instances complete successfully on their first try.
Verify both refresh IDs returned by Airflow through the Azure Analysis Services REST API:
437aa86c-…-8169ecalculatesucceededd9f4974e-…-9020calculatesucceededThe Azure screenshot confirms the D1 resource, West US location, and compatibility-level-1200 model.
Was generative AI tooling used to co-author this PR?
Generated-by: [GPT 5.6-sol] following the guidelines
{pr_number}.significant.rst, in airflow-core/newsfragments. You can add this file in a follow-up commit after the PR is created so you know the PR number.