-
Notifications
You must be signed in to change notification settings - Fork 2.2k
Fixes #29764: refactor(ingestion): migrate powerbi test-connection to declarative framework #29767
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 1 commit
1787aeb
adf89f0
b230165
b798dca
e08a800
5456918
777f978
23ee401
3555f6b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,72 @@ | ||
| # Copyright 2025 Collate | ||
| # Licensed under the Collate Community License, Version 1.0 (the "License"); | ||
| # you may not use this file except in compliance with the License. | ||
| # You may obtain a copy of the License at | ||
| # https://github.com/open-metadata/OpenMetadata/blob/main/ingestion/LICENSE | ||
| # Unless required by applicable law or agreed to in writing, software | ||
| # distributed under the License is distributed on an "AS IS" BASIS, | ||
| # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| # See the License for the specific language governing permissions and | ||
| # limitations under the License. | ||
| """ | ||
| Dashboard service step identity and shared check helpers. | ||
|
|
||
| Dashboard connectors reach their source over a REST API (with OAuth or token | ||
| auth), not a SQLAlchemy engine, so these helpers stay HTTP-generic: they call a | ||
| connector-supplied callable and summarize what it returned, and on failure raise | ||
| ``CheckError`` carrying the label of the call they attempted, so a failed step | ||
| still reports its ``Evidence`` to the backend. The runner and the rest of the | ||
| package stay engine- and protocol-agnostic; connectors reuse these helpers from | ||
| their ``@check`` methods. | ||
| """ | ||
|
|
||
| from __future__ import annotations | ||
|
|
||
| from typing import TYPE_CHECKING | ||
|
|
||
| from metadata.core.connections.test_connection.check import CheckError, StepName | ||
| from metadata.core.connections.test_connection.records import Evidence | ||
|
|
||
| if TYPE_CHECKING: | ||
| from collections.abc import Callable, Sized | ||
|
|
||
|
|
||
| class DashboardStep(StepName): | ||
| """The steps a dashboard connector can be asked to verify.""" | ||
|
|
||
| CheckAccess = "CheckAccess" | ||
| GetDashboards = "GetDashboards" | ||
|
|
||
|
|
||
| def _count(number: int, noun: str) -> str: | ||
| """``3 dashboards`` / ``1 dashboard`` - pluralize the noun to match the count.""" | ||
| return f"{number} {noun if number == 1 else noun + 's'}" | ||
|
|
||
|
|
||
| def verify_access(authenticate: Callable[[], object], command: str) -> Evidence: | ||
| """Prove the connector can authenticate against the REST API. | ||
|
|
||
| Runs the connector's auth callable (e.g. an OAuth token acquisition) and, on | ||
| failure, re-raises as ``CheckError`` carrying the attempted command so the | ||
| gate step still reports what it tried. | ||
| """ | ||
| try: | ||
| authenticate() | ||
| except Exception as cause: | ||
| raise CheckError(cause, Evidence(command=command)) from cause | ||
| return Evidence(summary="authenticated", command=command) | ||
|
|
||
|
|
||
| def fetch_list(fetch: Callable[[], Sized | None], noun: str, command: str) -> Evidence: | ||
| """Call a REST list endpoint and report how many items it returned. | ||
|
|
||
| Reports the command it attempted and a count summary. On failure, re-raise as | ||
| ``CheckError`` carrying the command so the failed step still reports what it | ||
| ran. | ||
| """ | ||
| try: | ||
| items = fetch() | ||
| except Exception as cause: | ||
| raise CheckError(cause, Evidence(command=command)) from cause | ||
| count = len(items) if items else 0 | ||
| return Evidence(summary=f"{_count(count, noun)} enumerated", command=command) | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -13,26 +13,82 @@ | |
| Source connection handler | ||
| """ | ||
|
|
||
| from typing import Optional | ||
| from __future__ import annotations | ||
|
|
||
| from metadata.generated.schema.entity.automations.workflow import ( | ||
| Workflow as AutomationWorkflow, | ||
| from typing import TYPE_CHECKING | ||
|
|
||
| from metadata.core.connections.test_connection import ( | ||
| ErrorPack, | ||
| Evidence, | ||
| Matchers, | ||
| check, | ||
| when, | ||
| ) | ||
| from metadata.core.connections.test_connection.checks.dashboard import ( | ||
| DashboardStep, | ||
| fetch_list, | ||
| verify_access, | ||
| ) | ||
| from metadata.core.connections.test_connection.classifier import exception_chain | ||
| from metadata.core.connections.test_connection.network import NETWORK_ERRORS | ||
| from metadata.generated.schema.entity.services.connections.dashboard.powerBIConnection import ( | ||
| PowerBIConnection as PowerBIConnectionConfig, | ||
| ) | ||
| from metadata.generated.schema.entity.services.connections.testConnectionResult import ( | ||
| TestConnectionResult, | ||
| ) | ||
| from metadata.ingestion.api.steps import InvalidSourceException | ||
| from metadata.ingestion.connections.connection import BaseConnection | ||
| from metadata.ingestion.connections.test_connections import test_connection_steps | ||
| from metadata.ingestion.ometa.ometa_api import OpenMetadata | ||
| from metadata.ingestion.source.dashboard.powerbi.client import ( | ||
| PowerBiApiClient, | ||
| PowerBiClient, | ||
| ) | ||
| from metadata.ingestion.source.dashboard.powerbi.file_client import PowerBiFileClient | ||
| from metadata.utils.constants import THREE_MIN | ||
|
|
||
| if TYPE_CHECKING: | ||
| from metadata.core.connections.test_connection import ChecksProvider | ||
| from metadata.core.connections.test_connection.classifier import Matcher | ||
|
|
||
|
|
||
| def _http_status(*codes: int) -> Matcher: | ||
| """Match a PowerBI REST error by HTTP status. | ||
|
|
||
| The REST client raises ``APIError`` carrying the response status on its | ||
| ``.status_code`` property; we walk the cause chain and match on it. The status | ||
| is the stable signal - the message body varies by tenant and endpoint.""" | ||
| wanted = frozenset(codes) | ||
|
|
||
| def match(error: BaseException) -> bool: | ||
| result = False | ||
| for current in exception_chain(error): | ||
| code = getattr(current, "status_code", None) | ||
| if isinstance(code, int) and code in wanted: | ||
| result = True | ||
| return result | ||
|
gitar-bot[bot] marked this conversation as resolved.
Outdated
|
||
|
|
||
| return match | ||
|
|
||
|
|
||
| POWERBI_ERRORS = ErrorPack( | ||
| when(_http_status(401)).diagnose( | ||
| "Authentication failed", | ||
| fix="The service principal token was rejected (401). Check the Client ID, Client Secret, " | ||
| "and Tenant ID, and that the secret has not expired.", | ||
| ), | ||
| when(Matchers.exception(InvalidSourceException)).diagnose( | ||
| "Authentication failed", | ||
| fix="Could not acquire an OAuth token. Check the Client ID, Client Secret, and Tenant ID, " | ||
| "and that the app registration is allowed to request the configured scope.", | ||
|
gitar-bot[bot] marked this conversation as resolved.
|
||
| ), | ||
| when(_http_status(403)).diagnose( | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does a service principal with no access actually return 403 here, or 401? MS APIs often give 401. Worth confirming against a real tenant before merge — the tests can't tell us which one PowerBI really sends.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Confirmed against the MS docs and fixed in 5456918. Microsoft's Troubleshoot Power BI REST APIs documents that a service principal not enabled for the (admin) APIs returns 401 (a tenant/app-registration gap, not an auth failure), while a missing workspace grant returns 403. Since CheckAccess acquires the token first, any 401/403 here is post-auth, so neither points at the secret: 401 → "Power BI did not authorize the service principal", 403 → "Insufficient permissions". Still worth a live confirm of the exact code per endpoint. |
||
| "Insufficient permissions", | ||
| fix="The token authenticated but is not authorized (403). Grant the service principal the " | ||
| "required Power BI tenant settings / admin API access, and enable service-principal access " | ||
| "in the Power BI admin portal.", | ||
| ), | ||
| when(_http_status(404)).diagnose( | ||
| "Resource not found", | ||
| fix="The requested resource was not found (404). Check the API URL and that the configured " | ||
| "tenant/workspace exists and is visible to the service principal.", | ||
| ), | ||
| ).including(NETWORK_ERRORS) | ||
|
|
||
|
|
||
| def get_connection(connection: PowerBIConnectionConfig) -> PowerBiClient: | ||
|
|
@@ -45,28 +101,39 @@ def get_connection(connection: PowerBIConnectionConfig) -> PowerBiClient: | |
| return PowerBiClient(api_client=PowerBiApiClient(connection), file_client=file_client) | ||
|
|
||
|
|
||
| class PowerBIChecks: | ||
| """Test-connection checks for PowerBI. | ||
|
|
||
| ``CheckAccess`` is the gate: it acquires an OAuth token, so a bad service | ||
| principal fails fast before any list endpoint is dialled. ``GetDashboards`` | ||
| then exercises list access. No network call happens at construction; each | ||
| ``@check`` builds and runs its own call. | ||
| """ | ||
|
|
||
| errors = POWERBI_ERRORS | ||
|
|
||
| def __init__(self, client: PowerBiClient) -> None: | ||
| self.client = client | ||
|
|
||
| @check(DashboardStep.CheckAccess) | ||
| def check_access(self) -> Evidence: | ||
| return verify_access( | ||
| self.client.api_client.get_auth_token, | ||
| command="acquire OAuth token", | ||
| ) | ||
|
|
||
| @check(DashboardStep.GetDashboards) | ||
| def get_dashboards(self) -> Evidence: | ||
| return fetch_list( | ||
| self.client.api_client.fetch_dashboards, | ||
| noun="dashboard", | ||
| command="fetch dashboards", | ||
| ) | ||
|
|
||
|
IceS2 marked this conversation as resolved.
|
||
|
|
||
| class PowerBIConnection(BaseConnection[PowerBIConnectionConfig, PowerBiClient]): | ||
| def _get_client(self) -> PowerBiClient: | ||
| return get_connection(self.service_connection) | ||
|
|
||
| def test_connection( | ||
| self, | ||
| metadata: OpenMetadata, | ||
| automation_workflow: Optional[AutomationWorkflow] = None, # noqa: UP045 | ||
| timeout_seconds: Optional[int] = THREE_MIN, # noqa: UP045 | ||
| ) -> TestConnectionResult: | ||
| """ | ||
| Test connection. This can be executed either as part | ||
| of a metadata workflow or during an Automation Workflow | ||
| """ | ||
| client = self.client | ||
| service_connection = self.service_connection | ||
| test_fn = {"GetDashboards": client.api_client.fetch_dashboards} | ||
|
|
||
| return test_connection_steps( | ||
| metadata=metadata, | ||
| test_fn=test_fn, | ||
| service_type=service_connection.type.value, # pyright: ignore[reportOptionalMemberAccess] | ||
| automation_workflow=automation_workflow, | ||
| timeout_seconds=timeout_seconds, | ||
| ) | ||
| def checks(self) -> ChecksProvider: | ||
| return PowerBIChecks(client=self.client) | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Heads up, since Tableau/Looker will reuse this:
Noneand[]both become 0, so if a fetch quietly fails and returnsNone, the step shows "0 dashboards" and passes instead of failing. Worth telling "empty" apart from "broke" before other connectors depend on it?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should not be an error but a warning and should be handled in the connector itself if it makes sense
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in 777f978 — per @IceS2's call this is now a non-blocking Warning, not an error.
fetch_listtakes an optionalempty_caveat; PowerBI's GetDashboards opts in, so an empty/None result still passes but surfaces "No dashboards visible" (with a note that it can also mean a filtered/failed listing). The shared helper stays connector-agnostic — the connector decides whether empty is worth flagging.