Fixes #29764: refactor(ingestion): migrate powerbi test-connection to declarative framework#29767
Fixes #29764: refactor(ingestion): migrate powerbi test-connection to declarative framework#29767IceS2 wants to merge 9 commits into
Conversation
… declarative framework Migrate the PowerBI dashboard connector's test-connection onto the declarative @check / ChecksProvider / ErrorPack framework. PowerBI is the first Dashboard-vertical connector to migrate, so this also seeds the shared Dashboard scaffolding (checks/dashboard.py: DashboardStep enum + REST-generic verify_access/fetch_list helpers) that Tableau and Looker will reuse. - Add a CheckAccess OAuth gate step (ConnectionGate) that acquires a token so a bad service principal fails fast before any list endpoint is dialled. - Keep GetDashboards as the list-access step. - HTTP-status-aware error pack (401/403/404) folding in the shared network pack.
github-actions
Bot
added
Ingestion
safe to test
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
… count The MSAL client that PowerBiApiClient builds in its constructor performs authority/instance discovery over the network. Building it while constructing the checks provider (via self.client) ran that network call before the runner's gate, so a bad authority/tenant surfaced as a raw automation-workflow error instead of a classified CheckAccess failure. Build the client lazily inside the first check instead, and classify MSAL authority errors. Also cap the GetDashboards count summary (100+) so a huge tenant does not produce an unbounded figure.
An empty Tenant ID makes MSAL raise a differently-worded ValueError ("...should
consist of an https url") that the "authority" token missed, leaving CheckAccess
unclassified. Match both message shapes.
…last Address review on the error pack: - _http_status now falls back to .response.status_code, so a raw requests.HTTPError (which the REST client re-raises when the error body has no "code" field) is still classified as 401/403/404 instead of falling through. - Move the broad "authority" substring matcher below the HTTP-status matchers so a status-coded 401 whose message echoes the authority URL is not misclassified as an authority error. MSAL authority ValueErrors carry no status, so they still reach the rule.
|
Both review findings addressed in b798dca:
|
Code Review ✅ Approved 2 resolved / 2 findingsMigrates PowerBI test-connection to the declarative framework and adds a shared dashboard scaffolding, resolving the HTTPError classification and authority matcher shadowing issues. ✅ 2 resolved✅ Bug: HTTP-status matcher misses raw requests.HTTPError (only APIError)
✅ Edge Case: 'authority' matcher ordered first may shadow 401 auth errors
OptionsDisplay: compact → Showing less information. Comment with these commands to change the behavior for this request:
Was this helpful? React with 👍 / 👎 | Gitar |
✅ PR checks passedThe linked issue has a description and all required Shipping project fields set. Thanks! |
There was a problem hiding this comment.
Pull request overview
This PR migrates the PowerBI dashboard connector’s test-connection implementation from the legacy test_connection_steps helper to the declarative @check / ChecksProvider / ErrorPack framework, while also introducing shared “dashboard-vertical” scaffolding intended for reuse by other dashboard connectors.
Changes:
- Added a new dashboard test-connection checks module with
DashboardStepand shared REST-oriented helpers (verify_access,fetch_list). - Refactored the PowerBI connector to expose
checks()via a newPowerBIChecksprovider, including an HTTP-status-awareErrorPackand lazy REST client construction. - Updated the PowerBI test-connection definition JSON to add a
CheckAccessConnectionGate step, and added/updated unit tests for the new checks + classification behavior.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| openmetadata-service/src/main/resources/json/data/testConnections/dashboard/powerbi.json | Adds the CheckAccess gate step ahead of GetDashboards in the PowerBI test-connection definition. |
| ingestion/tests/unit/source/dashboard/powerbi/test_connection.py | Updates unit tests to validate the new checks provider behavior and error classification. |
| ingestion/src/metadata/ingestion/source/dashboard/powerbi/connection.py | Replaces the legacy test_connection override with a checks() provider, lazy client creation, and a PowerBI-specific ErrorPack. |
| ingestion/src/metadata/core/connections/test_connection/checks/dashboard.py | Introduces shared dashboard-step identity (DashboardStep) and generic REST check helpers for dashboard connectors. |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
…in matcher Address review: - The checks provider builds PowerBiApiClient directly instead of via get_connection, so the file client (which the checks never use, and which does I/O for pbit sources) is not constructed. get_connection stays the ingestion path. - _http_status returns on the first matching status instead of scanning the rest of the cause chain.
|
Thanks @greptile-apps — both applied in the latest commit:
|
|
Both fixes look correct:
The parametrized test for Tip: You can customize Greptile's behavior for this repo with |
|
Good catch on the 401-vs-403 question — I checked the Microsoft docs and the original split was wrong. Fixed in 5456918. Per Troubleshoot Power BI REST APIs (Microsoft Learn):
So the docs do not back up "no-access SP → 403"; the enablement case is 401. Also, since Reframed accordingly:
Which exact code a no-access SP returns can still vary by endpoint (admin vs group APIs), but both now map to authorization guidance, so the hint is correct either way. Will confirm the precise code live against the tenant. |
| # A check only needs to prove the list endpoint is reachable and returns items, | ||
| # not enumerate every one, so ``fetch_list`` reports the count up to this cap and | ||
| # marks it ``+`` beyond, keeping the summary bounded on huge tenants. |
There was a problem hiding this comment.
Fixed in 777f978 — wording is now "meets or exceeds", and the count is capped so <cap>+ only ever renders at the boundary.
| Reports the command it attempted and a count summary, capped at ``cap`` (shown | ||
| as ``<cap>+`` beyond) so a huge tenant does not produce an unbounded figure. On | ||
| failure, re-raise as ``CheckError`` carrying the command so the failed step | ||
| still reports what it ran. |
There was a problem hiding this comment.
Fixed in 777f978 — docstring now says "meets or exceeds" and matches the >= behavior.
…caveat Review follow-ups on the shared dashboard helpers: - fetch_list caps the counted number at cap (min), so the "<cap>+" boundary is exact and a bare number never exceeds the cap; docs say "meets or exceeds". - fetch_list takes an optional empty_caveat: when the endpoint returns nothing the step still passes but carries a non-blocking Warning, so an empty (or silently-filtered/failed) listing is visible rather than a silent green. PowerBI's GetDashboards opts in with a "No dashboards visible" caveat.
…shboards Pass a zero-arg lambda so the lazy _client() construction (MSAL authority discovery) runs inside fetch_list's try/except. A build failure is then wrapped as a CheckError carrying the step command instead of escaping raw, matching the CheckAccess path.
Code Review ✅ Approved 2 resolved / 2 findingsMigrates PowerBI test-connection to the declarative framework, establishing a reusable scaffold for dashboard connectors. This change resolves previous issues with raw HTTP error handling and matcher ordering through improved status-code classification. ✅ 2 resolved✅ Bug: HTTP-status matcher misses raw requests.HTTPError (only APIError)
✅ Edge Case: 'authority' matcher ordered first may shadow 401 auth errors
OptionsDisplay: compact → Showing less information. Comment with these commands to change the behavior for this request:
Was this helpful? React with 👍 / 👎 | Gitar |
|
Migrates the PowerBI dashboard connector's test-connection off the legacy
test_connection_stepshelper onto the declarative@check/ChecksProvider/ErrorPackframework. Closes #29764.PowerBI is the first Dashboard-vertical connector to migrate, so this also seeds the shared Dashboard scaffolding that Tableau and Looker will reuse.
What changed
core/connections/test_connection/checks/dashboard.py—DashboardStepenum (CheckAccess,GetDashboards) + REST-generic helpersverify_access(auth probe) andfetch_list(count summary), modeled onchecks/database.py.powerbi/connection.py— dropped thetest_connectionoverride; addedPowerBIChecks(checks()provider) with aCheckAccessOAuth gate that acquires a token so a bad service principal fails fast, andGetDashboardsfor list access. No network call happens while building the provider.powerbi.json— added theCheckAccessgate step (category: ConnectionGate,shortCircuit,mandatory, ordered first); keptGetDashboards.InvalidSourceException→ auth, folding in the sharedNETWORK_ERRORSfor DNS/refused/timeout. Matches on the REST client'sAPIError.status_code.Notes
APIError.status_codevs message tokens) is confirmed live against a real tenant before merge.Greptile Summary
Migrates PowerBI's test-connection from the legacy
test_connection_stepshelper to the declarative@check/ChecksProvider/ErrorPackframework, and seeds the sharedchecks/dashboard.pyscaffolding that Tableau and Looker will reuse.checks/dashboard.py— addsDashboardStepenum,verify_access(auth probe wrapping failures asCheckError), andfetch_list(count summary with a100+cap for large tenants), mirroring the existingchecks/database.pypattern.powerbi/connection.py— replaces thetest_connectionoverride withPowerBIChecks, a lazy-client provider whoseCheckAccessgate acquires an OAuth token before any REST call is made;POWERBI_ERRORScoversInvalidSourceException, HTTP 401/403/404, MSAL authority errors, and network errors in priority order.powerbi.json— addsCheckAccessas amandatory/shortCircuitConnectionGatestep ordered beforeGetDashboards; the unit tests validate step discovery, evidence content, cap behavior, client-build failures, and the full error-pack classification matrix against bothAPIErrorand rawHTTPErrorshapes.Confidence Score: 5/5
Safe to merge — the migration is a clean drop-in replacement with no logic changes to the ingestion path and comprehensive unit-test coverage of every code branch.
The change is well-scoped: the get_connection function and _get_client path are untouched, so live ingestion is unaffected. The declarative checks are thoroughly tested including APIError vs raw HTTPError shapes, cap boundary behavior, lazy client construction failures, and the complete error-pack classification matrix. The gate ordering in POWERBI_ERRORS is carefully reasoned and verified by a dedicated test case. No unguarded code paths or missing error classifications were found.
No files require special attention.
Important Files Changed
Sequence Diagram
%%{init: {'theme': 'neutral'}}%% sequenceDiagram participant Runner participant PowerBIChecks participant PowerBiApiClient participant MSAL participant PowerBIRestAPI Runner->>PowerBIChecks: checks() Note over PowerBIChecks: No network I/O at construction Runner->>PowerBIChecks: check_access() [CheckAccess gate] PowerBIChecks->>PowerBIChecks: _client() - build PowerBiApiClient lazily PowerBiApiClient->>MSAL: ConfidentialClientApplication(authority) PowerBIChecks->>PowerBiApiClient: get_auth_token() PowerBiApiClient->>MSAL: acquire_token_for_client(scope) MSAL-->>PowerBiApiClient: token or error dict alt token missing PowerBiApiClient-->>PowerBIChecks: raise InvalidSourceException PowerBIChecks-->>Runner: raise CheckError Note over Runner: shortCircuit - skip GetDashboards else token OK PowerBiApiClient-->>PowerBIChecks: access_token PowerBIChecks-->>Runner: Evidence authenticated end Runner->>PowerBIChecks: get_dashboards() [GetDashboards] PowerBIChecks->>PowerBiApiClient: fetch_dashboards() PowerBiApiClient->>PowerBIRestAPI: GET /myorg/admin/dashboards PowerBIRestAPI-->>PowerBiApiClient: dashboards list or HTTP error alt HTTP error 401/403/404 PowerBiApiClient-->>PowerBIChecks: raise APIError or HTTPError PowerBIChecks-->>Runner: raise CheckError else success PowerBiApiClient-->>PowerBIChecks: list of PowerBIDashboard PowerBIChecks-->>Runner: Evidence N dashboards enumerated end%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%% sequenceDiagram participant Runner participant PowerBIChecks participant PowerBiApiClient participant MSAL participant PowerBIRestAPI Runner->>PowerBIChecks: checks() Note over PowerBIChecks: No network I/O at construction Runner->>PowerBIChecks: check_access() [CheckAccess gate] PowerBIChecks->>PowerBIChecks: _client() - build PowerBiApiClient lazily PowerBiApiClient->>MSAL: ConfidentialClientApplication(authority) PowerBIChecks->>PowerBiApiClient: get_auth_token() PowerBiApiClient->>MSAL: acquire_token_for_client(scope) MSAL-->>PowerBiApiClient: token or error dict alt token missing PowerBiApiClient-->>PowerBIChecks: raise InvalidSourceException PowerBIChecks-->>Runner: raise CheckError Note over Runner: shortCircuit - skip GetDashboards else token OK PowerBiApiClient-->>PowerBIChecks: access_token PowerBIChecks-->>Runner: Evidence authenticated end Runner->>PowerBIChecks: get_dashboards() [GetDashboards] PowerBIChecks->>PowerBiApiClient: fetch_dashboards() PowerBiApiClient->>PowerBIRestAPI: GET /myorg/admin/dashboards PowerBIRestAPI-->>PowerBiApiClient: dashboards list or HTTP error alt HTTP error 401/403/404 PowerBiApiClient-->>PowerBIChecks: raise APIError or HTTPError PowerBIChecks-->>Runner: raise CheckError else success PowerBiApiClient-->>PowerBIChecks: list of PowerBIDashboard PowerBIChecks-->>Runner: Evidence N dashboards enumerated endReviews (6): Last reviewed commit: "fix(powerbi): build client inside fetch_..." | Re-trigger Greptile