From 1787aeb13e4404c3b1a7443259f7696ea9a65916 Mon Sep 17 00:00:00 2001 From: Pablo Takara Date: Mon, 6 Jul 2026 12:10:00 +0200 Subject: [PATCH 1/8] Fixes #29764: refactor(ingestion): migrate powerbi test-connection to 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. --- .../test_connection/checks/dashboard.py | 72 ++++++++++ .../source/dashboard/powerbi/connection.py | 127 +++++++++++++----- .../dashboard/powerbi/test_connection.py | 119 ++++++++++++++-- .../testConnections/dashboard/powerbi.json | 10 +- 4 files changed, 288 insertions(+), 40 deletions(-) create mode 100644 ingestion/src/metadata/core/connections/test_connection/checks/dashboard.py diff --git a/ingestion/src/metadata/core/connections/test_connection/checks/dashboard.py b/ingestion/src/metadata/core/connections/test_connection/checks/dashboard.py new file mode 100644 index 000000000000..0e198db9cb73 --- /dev/null +++ b/ingestion/src/metadata/core/connections/test_connection/checks/dashboard.py @@ -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) diff --git a/ingestion/src/metadata/ingestion/source/dashboard/powerbi/connection.py b/ingestion/src/metadata/ingestion/source/dashboard/powerbi/connection.py index c4ba5105f406..924771dad7ea 100644 --- a/ingestion/src/metadata/ingestion/source/dashboard/powerbi/connection.py +++ b/ingestion/src/metadata/ingestion/source/dashboard/powerbi/connection.py @@ -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 + + 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.", + ), + when(_http_status(403)).diagnose( + "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", + ) + + 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) diff --git a/ingestion/tests/unit/source/dashboard/powerbi/test_connection.py b/ingestion/tests/unit/source/dashboard/powerbi/test_connection.py index ba434a6359ed..c76d636dda66 100644 --- a/ingestion/tests/unit/source/dashboard/powerbi/test_connection.py +++ b/ingestion/tests/unit/source/dashboard/powerbi/test_connection.py @@ -8,16 +8,38 @@ # 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. -"""Unit tests for Power BI connection handling.""" +"""Unit tests for Power BI test-connection checks.""" +import socket from unittest.mock import MagicMock, patch +import pytest + +from metadata.core.connections.test_connection.check import CheckError, collect_checks +from metadata.core.connections.test_connection.checks.dashboard import DashboardStep +from metadata.ingestion.api.steps import InvalidSourceException from metadata.ingestion.connections.connection import BaseConnection -from metadata.ingestion.source.dashboard.powerbi.connection import PowerBIConnection +from metadata.ingestion.ometa.client import APIError +from metadata.ingestion.source.dashboard.powerbi.connection import ( + POWERBI_ERRORS, + PowerBIChecks, + PowerBIConnection, +) CONNECTION_MODULE = "metadata.ingestion.source.dashboard.powerbi.connection" +def _api_error(status_code: int) -> APIError: + http_error = MagicMock() + http_error.response.status_code = status_code + return APIError({"message": "boom", "code": "x"}, http_error) + + +def _checks() -> tuple[PowerBIChecks, MagicMock]: + client = MagicMock() + return PowerBIChecks(client=client), client + + def test_powerbi_connection_is_base_connection(): assert issubclass(PowerBIConnection, BaseConnection) @@ -31,10 +53,91 @@ def test_get_client_delegates_to_get_connection(): mock_get.assert_called_once_with(conn.service_connection) -def test_test_connection_runs_steps(): - conn = PowerBIConnection(MagicMock()) - conn._client = MagicMock() - with patch(f"{CONNECTION_MODULE}.test_connection_steps") as mock_step: - result = conn.test_connection(metadata=MagicMock()) +def test_checks_does_not_touch_the_network(): + with patch(f"{CONNECTION_MODULE}.get_connection") as mock_get: + conn = PowerBIConnection(MagicMock()) + provider = conn.checks() + + assert isinstance(provider, PowerBIChecks) + api_client = mock_get.return_value.api_client + api_client.get_auth_token.assert_not_called() + api_client.fetch_dashboards.assert_not_called() + + +def test_collect_checks_maps_every_step(): + provider, _ = _checks() + collected = collect_checks(provider) + + assert set(collected) == {DashboardStep.CheckAccess, DashboardStep.GetDashboards} + + +def test_check_access_authenticates(): + provider, client = _checks() + client.api_client.get_auth_token.return_value = ("token", "3600") + + evidence = provider.check_access() + + client.api_client.get_auth_token.assert_called_once_with() + assert evidence.summary == "authenticated" + assert evidence.command == "acquire OAuth token" + + +def test_check_access_wraps_failure_as_check_error(): + provider, client = _checks() + client.api_client.get_auth_token.side_effect = InvalidSourceException("no token") + + with pytest.raises(CheckError) as exc_info: + provider.check_access() + + assert isinstance(exc_info.value.cause, InvalidSourceException) + assert exc_info.value.evidence.command == "acquire OAuth token" + + +def test_get_dashboards_counts_results(): + provider, client = _checks() + client.api_client.fetch_dashboards.return_value = [object(), object(), object()] + + evidence = provider.get_dashboards() + + assert evidence.summary == "3 dashboards enumerated" + assert evidence.command == "fetch dashboards" + + +def test_get_dashboards_empty_is_singular_aware(): + provider, client = _checks() + client.api_client.fetch_dashboards.return_value = None + + evidence = provider.get_dashboards() + + assert evidence.summary == "0 dashboards enumerated" + + +def test_get_dashboards_wraps_failure_as_check_error(): + provider, client = _checks() + client.api_client.fetch_dashboards.side_effect = _api_error(403) + + with pytest.raises(CheckError) as exc_info: + provider.get_dashboards() + + assert exc_info.value.evidence.command == "fetch dashboards" + + +@pytest.mark.parametrize( + ("error", "title"), + [ + (_api_error(401), "Authentication failed"), + (InvalidSourceException("bad creds"), "Authentication failed"), + (_api_error(403), "Insufficient permissions"), + (_api_error(404), "Resource not found"), + (socket.gaierror("name resolution failed"), "Host could not be resolved"), + ], +) +def test_error_pack_classifies(error, title): + diagnosis = POWERBI_ERRORS.classify(error) + + assert diagnosis is not None + assert diagnosis.title == title + - assert result is mock_step.return_value +def test_error_pack_ignores_unknown_error(): + assert POWERBI_ERRORS.classify(ValueError("something else")) is None diff --git a/openmetadata-service/src/main/resources/json/data/testConnections/dashboard/powerbi.json b/openmetadata-service/src/main/resources/json/data/testConnections/dashboard/powerbi.json index 9a995212e779..c6d1f72e6567 100644 --- a/openmetadata-service/src/main/resources/json/data/testConnections/dashboard/powerbi.json +++ b/openmetadata-service/src/main/resources/json/data/testConnections/dashboard/powerbi.json @@ -3,6 +3,14 @@ "displayName": "PowerBI Test Connection", "description": "This Test Connection validates the access against the server and basic metadata extraction of dashboards and charts.", "steps": [ + { + "name": "CheckAccess", + "description": "Validate that we can acquire an OAuth token and authenticate against the PowerBI REST API with the given service principal credentials.", + "errorMessage": "Failed to authenticate against PowerBI, please validate the Client ID, Client Secret and Tenant ID.", + "mandatory": true, + "shortCircuit": true, + "category": "ConnectionGate" + }, { "name": "GetDashboards", "description": "List all the dashboards available to the user", @@ -12,5 +20,3 @@ } ] } - - \ No newline at end of file From adf89f02f8a650343aa2c5a6f5b16d8518392151 Mon Sep 17 00:00:00 2001 From: Pablo Takara Date: Mon, 6 Jul 2026 16:23:57 +0200 Subject: [PATCH 2/8] fix(powerbi): defer REST client build into CheckAccess; cap dashboard 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. --- .../test_connection/checks/dashboard.py | 34 +++++++--- .../source/dashboard/powerbi/connection.py | 33 +++++++-- .../dashboard/powerbi/test_connection.py | 68 ++++++++++++------- 3 files changed, 95 insertions(+), 40 deletions(-) diff --git a/ingestion/src/metadata/core/connections/test_connection/checks/dashboard.py b/ingestion/src/metadata/core/connections/test_connection/checks/dashboard.py index 0e198db9cb73..b8509022b999 100644 --- a/ingestion/src/metadata/core/connections/test_connection/checks/dashboard.py +++ b/ingestion/src/metadata/core/connections/test_connection/checks/dashboard.py @@ -31,6 +31,12 @@ from collections.abc import Callable, Sized +# 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. +DEFAULT_LIST_CAP = 100 + + class DashboardStep(StepName): """The steps a dashboard connector can be asked to verify.""" @@ -38,9 +44,15 @@ class DashboardStep(StepName): 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 _count(number: int, noun: str, cap: int | None = None) -> str: + """``3 dashboards`` / ``1 dashboard`` - pluralize the noun to match the count. + + When ``cap`` is given and the count reaches it, the figure is rendered + ``+`` so a capped sample is not read as an exact total. + """ + plural = noun if number == 1 else noun + "s" + shown = f"{cap}+" if cap is not None and number >= cap else str(number) + return f"{shown} {plural}" def verify_access(authenticate: Callable[[], object], command: str) -> Evidence: @@ -57,16 +69,22 @@ def verify_access(authenticate: Callable[[], object], command: str) -> Evidence: return Evidence(summary="authenticated", command=command) -def fetch_list(fetch: Callable[[], Sized | None], noun: str, command: str) -> Evidence: +def fetch_list( + fetch: Callable[[], Sized | None], + noun: str, + command: str, + cap: int = DEFAULT_LIST_CAP, +) -> 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. + Reports the command it attempted and a count summary, capped at ``cap`` (shown + as ``+`` 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. """ 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) + return Evidence(summary=f"{_count(count, noun, cap)} enumerated", command=command) diff --git a/ingestion/src/metadata/ingestion/source/dashboard/powerbi/connection.py b/ingestion/src/metadata/ingestion/source/dashboard/powerbi/connection.py index 924771dad7ea..8ec5e03fd085 100644 --- a/ingestion/src/metadata/ingestion/source/dashboard/powerbi/connection.py +++ b/ingestion/src/metadata/ingestion/source/dashboard/powerbi/connection.py @@ -67,6 +67,11 @@ def match(error: BaseException) -> bool: POWERBI_ERRORS = ErrorPack( + when(Matchers.contains("authority")).diagnose( + "Invalid tenant or authority", + fix="The authority could not be resolved. Check the Tenant ID (a valid GUID or " + "tenant name) and the Authority URI, e.g. https://login.microsoftonline.com/.", + ), when(_http_status(401)).diagnose( "Authentication failed", fix="The service principal token was rejected (401). Check the Client ID, Client Secret, " @@ -106,26 +111,40 @@ class PowerBIChecks: ``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. + then exercises list access. + + The REST client is built lazily inside the first check, never at + construction: the underlying MSAL client performs authority/instance + discovery over the network in its constructor, so building it eagerly would + run before the runner's gate and surface as a raw workflow error instead of a + classified ``CheckAccess`` failure. """ errors = POWERBI_ERRORS - def __init__(self, client: PowerBiClient) -> None: - self.client = client + def __init__(self, connection: PowerBIConnectionConfig) -> None: + self._connection = connection + self._api_client: PowerBiApiClient | None = None + + def _client(self) -> PowerBiApiClient: + """Build (once) and return the REST client. The MSAL client it wraps does + authority/instance discovery over the network in its constructor, so this + is only ever called from inside a check - never at construction.""" + if self._api_client is None: + self._api_client = get_connection(self._connection).api_client + return self._api_client @check(DashboardStep.CheckAccess) def check_access(self) -> Evidence: return verify_access( - self.client.api_client.get_auth_token, + lambda: self._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, + self._client().fetch_dashboards, noun="dashboard", command="fetch dashboards", ) @@ -136,4 +155,4 @@ def _get_client(self) -> PowerBiClient: return get_connection(self.service_connection) def checks(self) -> ChecksProvider: - return PowerBIChecks(client=self.client) + return PowerBIChecks(connection=self.service_connection) diff --git a/ingestion/tests/unit/source/dashboard/powerbi/test_connection.py b/ingestion/tests/unit/source/dashboard/powerbi/test_connection.py index c76d636dda66..a699e041ffb5 100644 --- a/ingestion/tests/unit/source/dashboard/powerbi/test_connection.py +++ b/ingestion/tests/unit/source/dashboard/powerbi/test_connection.py @@ -35,9 +35,11 @@ def _api_error(status_code: int) -> APIError: return APIError({"message": "boom", "code": "x"}, http_error) -def _checks() -> tuple[PowerBIChecks, MagicMock]: - client = MagicMock() - return PowerBIChecks(client=client), client +def _checks(get_connection) -> tuple[PowerBIChecks, MagicMock]: + """A provider whose lazily-built REST client is the returned mock.""" + api_client = MagicMock() + get_connection.return_value.api_client = api_client + return PowerBIChecks(connection=MagicMock()), api_client def test_powerbi_connection_is_base_connection(): @@ -59,65 +61,79 @@ def test_checks_does_not_touch_the_network(): provider = conn.checks() assert isinstance(provider, PowerBIChecks) - api_client = mock_get.return_value.api_client - api_client.get_auth_token.assert_not_called() - api_client.fetch_dashboards.assert_not_called() + mock_get.assert_not_called() def test_collect_checks_maps_every_step(): - provider, _ = _checks() + with patch(f"{CONNECTION_MODULE}.get_connection") as mock_get: + provider, _ = _checks(mock_get) collected = collect_checks(provider) assert set(collected) == {DashboardStep.CheckAccess, DashboardStep.GetDashboards} def test_check_access_authenticates(): - provider, client = _checks() - client.api_client.get_auth_token.return_value = ("token", "3600") + with patch(f"{CONNECTION_MODULE}.get_connection") as mock_get: + provider, client = _checks(mock_get) + client.get_auth_token.return_value = ("token", "3600") - evidence = provider.check_access() + evidence = provider.check_access() - client.api_client.get_auth_token.assert_called_once_with() + client.get_auth_token.assert_called_once_with() assert evidence.summary == "authenticated" assert evidence.command == "acquire OAuth token" def test_check_access_wraps_failure_as_check_error(): - provider, client = _checks() - client.api_client.get_auth_token.side_effect = InvalidSourceException("no token") + with patch(f"{CONNECTION_MODULE}.get_connection") as mock_get: + provider, client = _checks(mock_get) + client.get_auth_token.side_effect = InvalidSourceException("no token") - with pytest.raises(CheckError) as exc_info: - provider.check_access() + with pytest.raises(CheckError) as exc_info: + provider.check_access() assert isinstance(exc_info.value.cause, InvalidSourceException) assert exc_info.value.evidence.command == "acquire OAuth token" def test_get_dashboards_counts_results(): - provider, client = _checks() - client.api_client.fetch_dashboards.return_value = [object(), object(), object()] + with patch(f"{CONNECTION_MODULE}.get_connection") as mock_get: + provider, client = _checks(mock_get) + client.fetch_dashboards.return_value = [object(), object(), object()] - evidence = provider.get_dashboards() + evidence = provider.get_dashboards() assert evidence.summary == "3 dashboards enumerated" assert evidence.command == "fetch dashboards" +def test_get_dashboards_caps_the_count(): + with patch(f"{CONNECTION_MODULE}.get_connection") as mock_get: + provider, client = _checks(mock_get) + client.fetch_dashboards.return_value = [object()] * 250 + + evidence = provider.get_dashboards() + + assert evidence.summary == "100+ dashboards enumerated" + + def test_get_dashboards_empty_is_singular_aware(): - provider, client = _checks() - client.api_client.fetch_dashboards.return_value = None + with patch(f"{CONNECTION_MODULE}.get_connection") as mock_get: + provider, client = _checks(mock_get) + client.fetch_dashboards.return_value = None - evidence = provider.get_dashboards() + evidence = provider.get_dashboards() assert evidence.summary == "0 dashboards enumerated" def test_get_dashboards_wraps_failure_as_check_error(): - provider, client = _checks() - client.api_client.fetch_dashboards.side_effect = _api_error(403) + with patch(f"{CONNECTION_MODULE}.get_connection") as mock_get: + provider, client = _checks(mock_get) + client.fetch_dashboards.side_effect = _api_error(403) - with pytest.raises(CheckError) as exc_info: - provider.get_dashboards() + with pytest.raises(CheckError) as exc_info: + provider.get_dashboards() assert exc_info.value.evidence.command == "fetch dashboards" @@ -129,6 +145,8 @@ def test_get_dashboards_wraps_failure_as_check_error(): (InvalidSourceException("bad creds"), "Authentication failed"), (_api_error(403), "Insufficient permissions"), (_api_error(404), "Resource not found"), + (ValueError("Unable to get authority configuration for https://login..."), "Invalid tenant or authority"), + (ValueError("invalid_instance: The authority you provided is not known"), "Invalid tenant or authority"), (socket.gaierror("name resolution failed"), "Host could not be resolved"), ], ) From b230165fbe35a9f16a554044814554f48886fac3 Mon Sep 17 00:00:00 2001 From: Pablo Takara Date: Mon, 6 Jul 2026 16:44:10 +0200 Subject: [PATCH 3/8] fix(powerbi): classify blank-tenant MSAL error as invalid authority 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. --- .../source/dashboard/powerbi/connection.py | 17 ++++++++++++++++- .../source/dashboard/powerbi/test_connection.py | 4 ++++ 2 files changed, 20 insertions(+), 1 deletion(-) diff --git a/ingestion/src/metadata/ingestion/source/dashboard/powerbi/connection.py b/ingestion/src/metadata/ingestion/source/dashboard/powerbi/connection.py index 8ec5e03fd085..444e720a399d 100644 --- a/ingestion/src/metadata/ingestion/source/dashboard/powerbi/connection.py +++ b/ingestion/src/metadata/ingestion/source/dashboard/powerbi/connection.py @@ -66,8 +66,23 @@ def match(error: BaseException) -> bool: return match +def _contains_any(*tokens: str) -> Matcher: + """Match when any of ``tokens`` appears in the error (or its cause chain). + + MSAL reports a bad authority through more than one message shape - a malformed + tenant, a blank tenant - so a single ``Matchers.contains`` token would miss + some; this ORs the stable tokens across those shapes.""" + lowered = tuple(token.lower() for token in tokens) + + def match(error: BaseException) -> bool: + chain = " ".join(str(current) for current in exception_chain(error)).lower() + return any(token in chain for token in lowered) + + return match + + POWERBI_ERRORS = ErrorPack( - when(Matchers.contains("authority")).diagnose( + when(_contains_any("authority", "should consist of an https url")).diagnose( "Invalid tenant or authority", fix="The authority could not be resolved. Check the Tenant ID (a valid GUID or " "tenant name) and the Authority URI, e.g. https://login.microsoftonline.com/.", diff --git a/ingestion/tests/unit/source/dashboard/powerbi/test_connection.py b/ingestion/tests/unit/source/dashboard/powerbi/test_connection.py index a699e041ffb5..c1a70738c9cf 100644 --- a/ingestion/tests/unit/source/dashboard/powerbi/test_connection.py +++ b/ingestion/tests/unit/source/dashboard/powerbi/test_connection.py @@ -147,6 +147,10 @@ def test_get_dashboards_wraps_failure_as_check_error(): (_api_error(404), "Resource not found"), (ValueError("Unable to get authority configuration for https://login..."), "Invalid tenant or authority"), (ValueError("invalid_instance: The authority you provided is not known"), "Invalid tenant or authority"), + ( + ValueError("Your given address (https://login...) should consist of an https url"), + "Invalid tenant or authority", + ), (socket.gaierror("name resolution failed"), "Host could not be resolved"), ], ) From b798dca2e83a5e7eb0d5e83da8935238ca341feb Mon Sep 17 00:00:00 2001 From: Pablo Takara Date: Mon, 6 Jul 2026 17:08:04 +0200 Subject: [PATCH 4/8] fix(powerbi): classify raw HTTPError status; order authority matcher 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. --- .../source/dashboard/powerbi/connection.py | 25 +++++++++++++------ .../dashboard/powerbi/test_connection.py | 19 ++++++++++++-- 2 files changed, 34 insertions(+), 10 deletions(-) diff --git a/ingestion/src/metadata/ingestion/source/dashboard/powerbi/connection.py b/ingestion/src/metadata/ingestion/source/dashboard/powerbi/connection.py index 444e720a399d..aa86d5ec9dbc 100644 --- a/ingestion/src/metadata/ingestion/source/dashboard/powerbi/connection.py +++ b/ingestion/src/metadata/ingestion/source/dashboard/powerbi/connection.py @@ -50,15 +50,21 @@ 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.""" + The status is the stable signal - the message body varies by tenant and + endpoint. Two shapes carry it: the client's ``APIError`` exposes it on a + top-level ``.status_code`` property, but when the error body has no ``code`` + field the client re-raises the raw ``requests.HTTPError`` unchanged, which + carries the status at ``.response.status_code``. We consult both, across the + cause chain.""" wanted = frozenset(codes) def match(error: BaseException) -> bool: result = False for current in exception_chain(error): code = getattr(current, "status_code", None) + if code is None: + response = getattr(current, "response", None) + code = getattr(response, "status_code", None) if isinstance(code, int) and code in wanted: result = True return result @@ -82,11 +88,6 @@ def match(error: BaseException) -> bool: POWERBI_ERRORS = ErrorPack( - when(_contains_any("authority", "should consist of an https url")).diagnose( - "Invalid tenant or authority", - fix="The authority could not be resolved. Check the Tenant ID (a valid GUID or " - "tenant name) and the Authority URI, e.g. https://login.microsoftonline.com/.", - ), when(_http_status(401)).diagnose( "Authentication failed", fix="The service principal token was rejected (401). Check the Client ID, Client Secret, " @@ -108,6 +109,14 @@ def match(error: BaseException) -> bool: 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.", ), + # Kept last: authority/instance-discovery failures are MSAL ValueErrors that + # carry no HTTP status, so a broad substring match here must not shadow a + # status-coded 401/403/404 whose message happens to echo the authority URL. + when(_contains_any("authority", "should consist of an https url")).diagnose( + "Invalid tenant or authority", + fix="The authority could not be resolved. Check the Tenant ID (a valid GUID or " + "tenant name) and the Authority URI, e.g. https://login.microsoftonline.com/.", + ), ).including(NETWORK_ERRORS) diff --git a/ingestion/tests/unit/source/dashboard/powerbi/test_connection.py b/ingestion/tests/unit/source/dashboard/powerbi/test_connection.py index c1a70738c9cf..fad9e6b57299 100644 --- a/ingestion/tests/unit/source/dashboard/powerbi/test_connection.py +++ b/ingestion/tests/unit/source/dashboard/powerbi/test_connection.py @@ -14,6 +14,7 @@ from unittest.mock import MagicMock, patch import pytest +from requests.exceptions import HTTPError from metadata.core.connections.test_connection.check import CheckError, collect_checks from metadata.core.connections.test_connection.checks.dashboard import DashboardStep @@ -29,10 +30,18 @@ CONNECTION_MODULE = "metadata.ingestion.source.dashboard.powerbi.connection" -def _api_error(status_code: int) -> APIError: +def _api_error(status_code: int, message: str = "boom") -> APIError: http_error = MagicMock() http_error.response.status_code = status_code - return APIError({"message": "boom", "code": "x"}, http_error) + return APIError({"message": message, "code": "x"}, http_error) + + +def _raw_http_error(status_code: int) -> HTTPError: + """A bare requests.HTTPError as the client re-raises when the body has no + 'code' field - status lives on .response.status_code, not .status_code.""" + response = MagicMock() + response.status_code = status_code + return HTTPError("server said no", response=response) def _checks(get_connection) -> tuple[PowerBIChecks, MagicMock]: @@ -145,6 +154,12 @@ def test_get_dashboards_wraps_failure_as_check_error(): (InvalidSourceException("bad creds"), "Authentication failed"), (_api_error(403), "Insufficient permissions"), (_api_error(404), "Resource not found"), + (_raw_http_error(401), "Authentication failed"), + (_raw_http_error(403), "Insufficient permissions"), + (_raw_http_error(404), "Resource not found"), + # A 401 whose message echoes the authority URL must still classify as an + # auth failure, not be shadowed by the broad 'authority' matcher. + (_api_error(401, "AADSTS700016 https://login.microsoftonline.com/ authority"), "Authentication failed"), (ValueError("Unable to get authority configuration for https://login..."), "Invalid tenant or authority"), (ValueError("invalid_instance: The authority you provided is not known"), "Invalid tenant or authority"), ( From e08a800cca8a659d2692c6b67fb7f168f146976b Mon Sep 17 00:00:00 2001 From: Pablo Takara Date: Mon, 6 Jul 2026 17:53:19 +0200 Subject: [PATCH 5/8] refactor(powerbi): build api client directly in checks; early-return 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. --- .../source/dashboard/powerbi/connection.py | 15 ++++---- .../dashboard/powerbi/test_connection.py | 36 +++++++++---------- 2 files changed, 26 insertions(+), 25 deletions(-) diff --git a/ingestion/src/metadata/ingestion/source/dashboard/powerbi/connection.py b/ingestion/src/metadata/ingestion/source/dashboard/powerbi/connection.py index aa86d5ec9dbc..cc7dd653efdd 100644 --- a/ingestion/src/metadata/ingestion/source/dashboard/powerbi/connection.py +++ b/ingestion/src/metadata/ingestion/source/dashboard/powerbi/connection.py @@ -59,15 +59,14 @@ def _http_status(*codes: int) -> Matcher: wanted = frozenset(codes) def match(error: BaseException) -> bool: - result = False for current in exception_chain(error): code = getattr(current, "status_code", None) if code is None: response = getattr(current, "response", None) code = getattr(response, "status_code", None) if isinstance(code, int) and code in wanted: - result = True - return result + return True + return False return match @@ -151,11 +150,13 @@ def __init__(self, connection: PowerBIConnectionConfig) -> None: self._api_client: PowerBiApiClient | None = None def _client(self) -> PowerBiApiClient: - """Build (once) and return the REST client. The MSAL client it wraps does - authority/instance discovery over the network in its constructor, so this - is only ever called from inside a check - never at construction.""" + """Build (once) and return the REST client. Built directly rather than via + ``get_connection`` so the file client (unused by the checks) is never + constructed. The MSAL client it wraps does authority/instance discovery + over the network in its constructor, so this is only ever called from + inside a check - never at construction.""" if self._api_client is None: - self._api_client = get_connection(self._connection).api_client + self._api_client = PowerBiApiClient(self._connection) return self._api_client @check(DashboardStep.CheckAccess) diff --git a/ingestion/tests/unit/source/dashboard/powerbi/test_connection.py b/ingestion/tests/unit/source/dashboard/powerbi/test_connection.py index fad9e6b57299..e1f71c91b5c9 100644 --- a/ingestion/tests/unit/source/dashboard/powerbi/test_connection.py +++ b/ingestion/tests/unit/source/dashboard/powerbi/test_connection.py @@ -44,10 +44,10 @@ def _raw_http_error(status_code: int) -> HTTPError: return HTTPError("server said no", response=response) -def _checks(get_connection) -> tuple[PowerBIChecks, MagicMock]: +def _checks(api_client_cls) -> tuple[PowerBIChecks, MagicMock]: """A provider whose lazily-built REST client is the returned mock.""" api_client = MagicMock() - get_connection.return_value.api_client = api_client + api_client_cls.return_value = api_client return PowerBIChecks(connection=MagicMock()), api_client @@ -65,25 +65,25 @@ def test_get_client_delegates_to_get_connection(): def test_checks_does_not_touch_the_network(): - with patch(f"{CONNECTION_MODULE}.get_connection") as mock_get: + with patch(f"{CONNECTION_MODULE}.PowerBiApiClient") as mock_client: conn = PowerBIConnection(MagicMock()) provider = conn.checks() assert isinstance(provider, PowerBIChecks) - mock_get.assert_not_called() + mock_client.assert_not_called() def test_collect_checks_maps_every_step(): - with patch(f"{CONNECTION_MODULE}.get_connection") as mock_get: - provider, _ = _checks(mock_get) + with patch(f"{CONNECTION_MODULE}.PowerBiApiClient") as mock_client: + provider, _ = _checks(mock_client) collected = collect_checks(provider) assert set(collected) == {DashboardStep.CheckAccess, DashboardStep.GetDashboards} def test_check_access_authenticates(): - with patch(f"{CONNECTION_MODULE}.get_connection") as mock_get: - provider, client = _checks(mock_get) + with patch(f"{CONNECTION_MODULE}.PowerBiApiClient") as mock_client: + provider, client = _checks(mock_client) client.get_auth_token.return_value = ("token", "3600") evidence = provider.check_access() @@ -94,8 +94,8 @@ def test_check_access_authenticates(): def test_check_access_wraps_failure_as_check_error(): - with patch(f"{CONNECTION_MODULE}.get_connection") as mock_get: - provider, client = _checks(mock_get) + with patch(f"{CONNECTION_MODULE}.PowerBiApiClient") as mock_client: + provider, client = _checks(mock_client) client.get_auth_token.side_effect = InvalidSourceException("no token") with pytest.raises(CheckError) as exc_info: @@ -106,8 +106,8 @@ def test_check_access_wraps_failure_as_check_error(): def test_get_dashboards_counts_results(): - with patch(f"{CONNECTION_MODULE}.get_connection") as mock_get: - provider, client = _checks(mock_get) + with patch(f"{CONNECTION_MODULE}.PowerBiApiClient") as mock_client: + provider, client = _checks(mock_client) client.fetch_dashboards.return_value = [object(), object(), object()] evidence = provider.get_dashboards() @@ -117,8 +117,8 @@ def test_get_dashboards_counts_results(): def test_get_dashboards_caps_the_count(): - with patch(f"{CONNECTION_MODULE}.get_connection") as mock_get: - provider, client = _checks(mock_get) + with patch(f"{CONNECTION_MODULE}.PowerBiApiClient") as mock_client: + provider, client = _checks(mock_client) client.fetch_dashboards.return_value = [object()] * 250 evidence = provider.get_dashboards() @@ -127,8 +127,8 @@ def test_get_dashboards_caps_the_count(): def test_get_dashboards_empty_is_singular_aware(): - with patch(f"{CONNECTION_MODULE}.get_connection") as mock_get: - provider, client = _checks(mock_get) + with patch(f"{CONNECTION_MODULE}.PowerBiApiClient") as mock_client: + provider, client = _checks(mock_client) client.fetch_dashboards.return_value = None evidence = provider.get_dashboards() @@ -137,8 +137,8 @@ def test_get_dashboards_empty_is_singular_aware(): def test_get_dashboards_wraps_failure_as_check_error(): - with patch(f"{CONNECTION_MODULE}.get_connection") as mock_get: - provider, client = _checks(mock_get) + with patch(f"{CONNECTION_MODULE}.PowerBiApiClient") as mock_client: + provider, client = _checks(mock_client) client.fetch_dashboards.side_effect = _api_error(403) with pytest.raises(CheckError) as exc_info: From 54569185c1575bfc1329ee22bac4f32407c0caeb Mon Sep 17 00:00:00 2001 From: Pablo Takara Date: Tue, 7 Jul 2026 12:08:28 +0200 Subject: [PATCH 6/8] fix(powerbi): correct 401/403 diagnoses to authorization, not credentials Per Microsoft's Power BI REST API troubleshooting guide, a service principal that is not enabled for the (admin) APIs returns 401 (a tenant/app-registration gap, not an auth failure), while a missing workspace/resource grant returns 403. Because CheckAccess acquires the token first, any 401/403 from a REST call means the token was accepted but the call was not authorized - so neither diagnosis should point at the client secret. Reframe 401 as a Power BI enablement problem and keep 403 as a workspace-permission problem; credential failures stay on the CheckAccess InvalidSourceException path. --- .../source/dashboard/powerbi/connection.py | 25 +++++++++++++------ .../dashboard/powerbi/test_connection.py | 13 ++++++---- 2 files changed, 25 insertions(+), 13 deletions(-) diff --git a/ingestion/src/metadata/ingestion/source/dashboard/powerbi/connection.py b/ingestion/src/metadata/ingestion/source/dashboard/powerbi/connection.py index cc7dd653efdd..06f097099a85 100644 --- a/ingestion/src/metadata/ingestion/source/dashboard/powerbi/connection.py +++ b/ingestion/src/metadata/ingestion/source/dashboard/powerbi/connection.py @@ -87,21 +87,30 @@ def match(error: BaseException) -> bool: 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.", - ), + # CheckAccess acquires the OAuth token, so a bad secret fails there as an + # InvalidSourceException - not here. Any 401/403 from a REST call therefore + # means the token was accepted but Power BI would not authorize the call, so + # neither diagnosis points at the credentials. Per Microsoft's REST API + # troubleshooting guide, a service principal that is not enabled for the + # (admin) APIs returns 401, while a missing workspace/resource grant returns + # 403 - so the two split on tenant-enablement vs. workspace access. 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.", ), + when(_http_status(401)).diagnose( + "Power BI did not authorize the service principal", + fix="The token was accepted but Power BI rejected the call (401). In the Fabric admin " + "portal enable 'Allow service principals to use Power BI APIs' (and the read-only admin " + "APIs setting when Use Admin APIs is on), add the service principal to the allowed " + "security group, and grant the app registration the required API permission.", + ), when(_http_status(403)).diagnose( "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.", + fix="The service principal is authenticated but not authorized for this resource (403). " + "Add it as a member (or admin) of the target workspace and grant the permissions the " + "call needs.", ), when(_http_status(404)).diagnose( "Resource not found", diff --git a/ingestion/tests/unit/source/dashboard/powerbi/test_connection.py b/ingestion/tests/unit/source/dashboard/powerbi/test_connection.py index e1f71c91b5c9..4cc2dafcbfc4 100644 --- a/ingestion/tests/unit/source/dashboard/powerbi/test_connection.py +++ b/ingestion/tests/unit/source/dashboard/powerbi/test_connection.py @@ -150,16 +150,19 @@ def test_get_dashboards_wraps_failure_as_check_error(): @pytest.mark.parametrize( ("error", "title"), [ - (_api_error(401), "Authentication failed"), + (_api_error(401), "Power BI did not authorize the service principal"), (InvalidSourceException("bad creds"), "Authentication failed"), (_api_error(403), "Insufficient permissions"), (_api_error(404), "Resource not found"), - (_raw_http_error(401), "Authentication failed"), + (_raw_http_error(401), "Power BI did not authorize the service principal"), (_raw_http_error(403), "Insufficient permissions"), (_raw_http_error(404), "Resource not found"), - # A 401 whose message echoes the authority URL must still classify as an - # auth failure, not be shadowed by the broad 'authority' matcher. - (_api_error(401, "AADSTS700016 https://login.microsoftonline.com/ authority"), "Authentication failed"), + # A status-coded 401 whose message echoes the authority URL must classify + # by its status, not be shadowed by the broad 'authority' matcher. + ( + _api_error(401, "AADSTS700016 https://login.microsoftonline.com/ authority"), + "Power BI did not authorize the service principal", + ), (ValueError("Unable to get authority configuration for https://login..."), "Invalid tenant or authority"), (ValueError("invalid_instance: The authority you provided is not known"), "Invalid tenant or authority"), ( From 777f9784c2e14b00a08d57e05db3d12a2b796a31 Mon Sep 17 00:00:00 2001 From: Pablo Takara Date: Tue, 7 Jul 2026 12:33:52 +0200 Subject: [PATCH 7/8] refactor(dashboard-checks): cap the counted list; surface empty as a caveat Review follow-ups on the shared dashboard helpers: - fetch_list caps the counted number at cap (min), so the "+" 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. --- .../test_connection/checks/dashboard.py | 31 ++++++++++++------- .../source/dashboard/powerbi/connection.py | 7 +++++ .../dashboard/powerbi/test_connection.py | 15 ++++++++- 3 files changed, 41 insertions(+), 12 deletions(-) diff --git a/ingestion/src/metadata/core/connections/test_connection/checks/dashboard.py b/ingestion/src/metadata/core/connections/test_connection/checks/dashboard.py index b8509022b999..fac8e5494e0c 100644 --- a/ingestion/src/metadata/core/connections/test_connection/checks/dashboard.py +++ b/ingestion/src/metadata/core/connections/test_connection/checks/dashboard.py @@ -25,15 +25,16 @@ from typing import TYPE_CHECKING from metadata.core.connections.test_connection.check import CheckError, StepName -from metadata.core.connections.test_connection.records import Evidence +from metadata.core.connections.test_connection.records import Diagnosis, Evidence if TYPE_CHECKING: from collections.abc import Callable, Sized # 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. +# not enumerate every one, so ``fetch_list`` counts at most this many and renders +# ``+`` when the count meets or exceeds it, keeping the summary bounded on +# huge tenants. DEFAULT_LIST_CAP = 100 @@ -47,8 +48,10 @@ class DashboardStep(StepName): def _count(number: int, noun: str, cap: int | None = None) -> str: """``3 dashboards`` / ``1 dashboard`` - pluralize the noun to match the count. - When ``cap`` is given and the count reaches it, the figure is rendered - ``+`` so a capped sample is not read as an exact total. + When ``cap`` is given and the count meets or exceeds it, the figure is + rendered ``+`` so a capped sample is not read as an exact total. The + caller caps ``number`` at ``cap`` first, so this only ever renders ``+`` + at the boundary, never a larger bare number. """ plural = noun if number == 1 else noun + "s" shown = f"{cap}+" if cap is not None and number >= cap else str(number) @@ -74,17 +77,23 @@ def fetch_list( noun: str, command: str, cap: int = DEFAULT_LIST_CAP, + empty_caveat: Diagnosis | None = None, ) -> Evidence: """Call a REST list endpoint and report how many items it returned. - Reports the command it attempted and a count summary, capped at ``cap`` (shown - as ``+`` 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. + Reports the command it attempted and a count summary; the count is capped at + ``cap`` and rendered ``+`` when it meets or exceeds it, so a huge tenant + does not produce an unbounded figure. When the endpoint returns nothing and + ``empty_caveat`` is given, the step still passes but carries the caveat as a + non-blocking Warning - a connector opts in when "nothing visible" is worth + surfacing (e.g. it may mean the fetch was filtered or silently failed rather + than the source being genuinely empty). 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, cap)} enumerated", command=command) + count = min(len(items) if items else 0, cap) + caveat = empty_caveat if count == 0 else None + return Evidence(summary=f"{_count(count, noun, cap)} enumerated", command=command, caveat=caveat) diff --git a/ingestion/src/metadata/ingestion/source/dashboard/powerbi/connection.py b/ingestion/src/metadata/ingestion/source/dashboard/powerbi/connection.py index 06f097099a85..8c28fe6ee073 100644 --- a/ingestion/src/metadata/ingestion/source/dashboard/powerbi/connection.py +++ b/ingestion/src/metadata/ingestion/source/dashboard/powerbi/connection.py @@ -18,6 +18,7 @@ from typing import TYPE_CHECKING from metadata.core.connections.test_connection import ( + Diagnosis, ErrorPack, Evidence, Matchers, @@ -181,6 +182,12 @@ def get_dashboards(self) -> Evidence: self._client().fetch_dashboards, noun="dashboard", command="fetch dashboards", + empty_caveat=Diagnosis( + title="No dashboards visible", + remediation="The connection works but returned no dashboards. Confirm the service " + "principal has access to a workspace that contains dashboards - an empty result can " + "also mean the listing was filtered or could not be read.", + ), ) diff --git a/ingestion/tests/unit/source/dashboard/powerbi/test_connection.py b/ingestion/tests/unit/source/dashboard/powerbi/test_connection.py index 4cc2dafcbfc4..1b0def049609 100644 --- a/ingestion/tests/unit/source/dashboard/powerbi/test_connection.py +++ b/ingestion/tests/unit/source/dashboard/powerbi/test_connection.py @@ -124,9 +124,20 @@ def test_get_dashboards_caps_the_count(): evidence = provider.get_dashboards() assert evidence.summary == "100+ dashboards enumerated" + assert evidence.caveat is None -def test_get_dashboards_empty_is_singular_aware(): +def test_get_dashboards_at_cap_shows_plus(): + with patch(f"{CONNECTION_MODULE}.PowerBiApiClient") as mock_client: + provider, client = _checks(mock_client) + client.fetch_dashboards.return_value = [object()] * 100 + + evidence = provider.get_dashboards() + + assert evidence.summary == "100+ dashboards enumerated" + + +def test_get_dashboards_empty_passes_with_caveat(): with patch(f"{CONNECTION_MODULE}.PowerBiApiClient") as mock_client: provider, client = _checks(mock_client) client.fetch_dashboards.return_value = None @@ -134,6 +145,8 @@ def test_get_dashboards_empty_is_singular_aware(): evidence = provider.get_dashboards() assert evidence.summary == "0 dashboards enumerated" + assert evidence.caveat is not None + assert evidence.caveat.title == "No dashboards visible" def test_get_dashboards_wraps_failure_as_check_error(): From 3555f6b84088ce50b324e8a534cf83416f55289b Mon Sep 17 00:00:00 2001 From: Pablo Takara Date: Tue, 7 Jul 2026 15:11:34 +0200 Subject: [PATCH 8/8] fix(powerbi): build client inside fetch_list error handling for GetDashboards 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. --- .../source/dashboard/powerbi/connection.py | 2 +- .../source/dashboard/powerbi/test_connection.py | 14 ++++++++++++++ 2 files changed, 15 insertions(+), 1 deletion(-) diff --git a/ingestion/src/metadata/ingestion/source/dashboard/powerbi/connection.py b/ingestion/src/metadata/ingestion/source/dashboard/powerbi/connection.py index 8c28fe6ee073..61baea96d824 100644 --- a/ingestion/src/metadata/ingestion/source/dashboard/powerbi/connection.py +++ b/ingestion/src/metadata/ingestion/source/dashboard/powerbi/connection.py @@ -179,7 +179,7 @@ def check_access(self) -> Evidence: @check(DashboardStep.GetDashboards) def get_dashboards(self) -> Evidence: return fetch_list( - self._client().fetch_dashboards, + lambda: self._client().fetch_dashboards(), noun="dashboard", command="fetch dashboards", empty_caveat=Diagnosis( diff --git a/ingestion/tests/unit/source/dashboard/powerbi/test_connection.py b/ingestion/tests/unit/source/dashboard/powerbi/test_connection.py index 1b0def049609..5192fa14631d 100644 --- a/ingestion/tests/unit/source/dashboard/powerbi/test_connection.py +++ b/ingestion/tests/unit/source/dashboard/powerbi/test_connection.py @@ -160,6 +160,20 @@ def test_get_dashboards_wraps_failure_as_check_error(): assert exc_info.value.evidence.command == "fetch dashboards" +def test_get_dashboards_wraps_client_build_failure(): + # Client construction (MSAL discovery) happens inside fetch_list's try, so a + # build failure still reports the step's command rather than escaping raw. + with patch(f"{CONNECTION_MODULE}.PowerBiApiClient") as mock_client: + provider = PowerBIChecks(connection=MagicMock()) + mock_client.side_effect = ValueError("authority discovery failed") + + with pytest.raises(CheckError) as exc_info: + provider.get_dashboards() + + assert exc_info.value.evidence.command == "fetch dashboards" + assert isinstance(exc_info.value.cause, ValueError) + + @pytest.mark.parametrize( ("error", "title"), [