diff --git a/owilix/core/exceptions.py b/owilix/core/exceptions.py new file mode 100644 index 0000000..09bb457 --- /dev/null +++ b/owilix/core/exceptions.py @@ -0,0 +1,107 @@ +"""Typed errors for conditions a caller is expected to act on. + +Most failures inside owilix are reported as a ``CommandResult`` carrying an +``ErrorType``. The exceptions here exist for conditions that must survive the +journey *up* from a repository backend to the command layer without being +flattened into "no results" along the way -- see ``AuthenticationError``. +""" +from __future__ import annotations + +__all__ = [ + "OwilixError", + "AuthenticationError", + "looks_like_authentication_failure", +] + + +class OwilixError(Exception): + """Base class for owilix's own typed errors.""" + + +class AuthenticationError(OwilixError): + """No usable credential for a remote, or the remote rejected the one given. + + "Not logged in" is the single most likely operational state for a tool whose + credential is a device-login refresh token, so it deserves a first-class + error rather than an empty result set. + + Before this existed, ``LexisRepository.list`` caught every exception, logged + it, and returned ``[]``; ``AggregatedRepository.list`` did the same per + repository. An expired credential therefore surfaced as "0 datasets found" + -- and, once matching nothing became a failure (F2), as the actively + misleading "No datasets matched specifier". An operator reading the summary + concluded the scope was empty and went looking in the wrong place. + """ + + def __init__(self, message: str | None = None, *, repository: str | None = None, + cause: BaseException | None = None) -> None: + self.repository = repository + self.cause = cause + if message is None: + where = f" for repository '{repository}'" if repository else "" + # No 'login' subcommand exists to point at: interactively, any remote + # command falls back to a browser login on its own, and a server has + # to be given a credential instead. Say what actually works in each + # case rather than naming a command that does not exist. + message = ( + f"Not authenticated{where}. The credential is missing, expired or was " + "rejected. Interactively, rerun the command and complete the browser " + "login when prompted. Non-interactively, set PY4LEXIS_TOKEN or " + "complete a device login via POST /auth/device/start." + ) + if cause is not None: + message = f"{message} Underlying error: {cause}" + super().__init__(message) + + +# Phrases that mean "the credential is the problem", matched case-insensitively +# against the *rendered* exception message. +# +# Matching text is unpleasant but necessary here: py4lexis has a typed +# Py4LexisAuthException, yet the auth failure observed in production arrives as +# a Py4LexisAPIException, because the DDI API answers a missing credential with +# an errorString py4lexis does not recognise: +# +# API -- POST -- https://api.lexis.tech/api/ddiapi/v2/meta/search +# -- Unrecognized 'errorString' obtained in the content: +# Authorization header empty. -- FAILED +# +# So type alone under-detects. Keep these phrases specific: a bare "token" would +# match unrelated failures and misreport a network fault as a login problem. +_AUTH_PHRASES = ( + "authorization header empty", + "authorization header missing", + "unauthorized", + "invalid_grant", + "invalid token", + "token expired", + "token has expired", + "expired token", + "authentication failed", + "not authenticated", + "not logged in", + "permission denied", + "forbidden", + "http 401", + "status 401", + "401 client error", + "http 403", + "status 403", + "403 client error", +) + + +def looks_like_authentication_failure(exc: BaseException) -> bool: + """Whether ``exc`` reports a credential problem rather than a data problem. + + Checked by type first -- py4lexis' own auth and session exceptions -- and + then by message, for the API-shaped failures that carry the real reason in + their text. + """ + for cls_name in type(exc).__mro__: + name = getattr(cls_name, "__name__", "") + if name in ("Py4LexisAuthException", "AuthenticationError"): + return True + + text = str(exc).lower() + return any(phrase in text for phrase in _AUTH_PHRASES) diff --git a/owilix/core/repository/aggregate.py b/owilix/core/repository/aggregate.py index 851c5c2..53deaba 100644 --- a/owilix/core/repository/aggregate.py +++ b/owilix/core/repository/aggregate.py @@ -16,6 +16,7 @@ from typing import Dict, List, Union, Sequence import fsspec +from owilix.core.exceptions import AuthenticationError from owilix.core.repository.base import ( AbstractRepository, RepoPerformanceStats, @@ -121,6 +122,8 @@ class AggregatedRepository: if ignore_data_centers is not None: repos = [(k, v) for k, v in repos if k not in ignore_data_centers] + auth_failures: List[AuthenticationError] = [] + with ThreadPoolExecutor() as executor: future_to_dc = { executor.submit(self._list_repo_wrapper, r, access, day, duration, query, cb_progress): dc @@ -133,9 +136,27 @@ class AggregatedRepository: repo = self.repositories[dc] repo.update_performance_stats("list", elapsed_time, len(repo_result)) results.extend(repo_result) + except AuthenticationError as e: + logger.error(f"Could not list datasets in datacenter {dc}: {e}") + auth_failures.append(e) except Exception as e: logger.exception(f"Could not list datasets in datacenter {dc}: {e}") + # Being unable to log in is not the same as finding nothing. + # + # Per-repository resilience is right -- one unreachable backend should + # not sink a multi-repository listing -- but when it leaves us with + # *nothing* to return, the caller cannot tell an empty scope from a + # rejected credential, and reports the former. Re-raise so the command + # layer can say "not authenticated" instead of "no datasets matched". + # + # Narrow on purpose: if some repository did answer, the partial result + # is still returned, as before. A caller cannot currently tell that a + # listing was partial -- that is a wider gap in the list() contract, + # recorded under F3 in the mirror-operability review, not fixed here. + if auth_failures and not results: + raise auth_failures[0] + # Deduplicate by dataset ID, keeping best performing repo grouped: Dict[str, List[Dataset]] = {} for ds in results: diff --git a/owilix/core/repository/lexis.py b/owilix/core/repository/lexis.py index b065564..68179ef 100644 --- a/owilix/core/repository/lexis.py +++ b/owilix/core/repository/lexis.py @@ -19,6 +19,7 @@ from typing import List, Union, Sequence import fsspec +from owilix.core.exceptions import AuthenticationError, looks_like_authentication_failure from owilix.core.repository.base import AbstractRepository from owilix.core.models.dataset import Dataset from owilix.core.sync import normalize_inventory_entry, to_epoch_seconds @@ -585,8 +586,22 @@ class LexisRepository(AbstractRepository): except Exception as e: logger.error(f"Failed to list datasets: {e}") + # An unusable credential is not an empty result set. + # + # Swallowing it here turned "not logged in" into "0 datasets found", + # and once matching nothing became a failure it became the actively + # misleading "No datasets matched specifier" -- pointing the reader + # at the scope when the problem was the token. Raising keeps the one + # operational state most likely to be true of a device-login tool + # distinguishable from a scope that genuinely holds nothing. + # + # Everything else still degrades to an empty list, as before. + if looks_like_authentication_failure(e): + raise AuthenticationError( + repository=getattr(self, "repo_name", None), cause=e + ) from e return [] - + def files(self, dataset: Dataset, files_glob: str | Sequence[str] = None) -> list: """List files in a dataset. diff --git a/owilix/core/tasks/remote.py b/owilix/core/tasks/remote.py index 882f9f1..e0abe44 100644 --- a/owilix/core/tasks/remote.py +++ b/owilix/core/tasks/remote.py @@ -32,7 +32,8 @@ from owilix.core.sync import ( write_inventory_gz, write_sync_sidecar, ) -from owilix.core.types import CommandResult +from owilix.core.exceptions import AuthenticationError +from owilix.core.types import CommandResult, ErrorType, ExitCode from owilix.core.db.models import OWIlixSQLQuery from owilix.core.db.duckdb_executor import OWIDuckDBSelectExecutor from owilix.core.tasks.query_utils import extract_domain_components, parse_files_pattern @@ -244,14 +245,29 @@ def remote_pull( # # Keeping `ignore_data_centers` is right: when pushing to a remote, do not # also treat that remote as a source to pull from. - datasets = manager.remote_data.list( - spec.get("data_center"), - access, - day=spec.get("day"), - duration=spec.get("duration") or 0, - query=query, - ignore_data_centers=[push_to_remote] if push_to_remote else None - ) + try: + datasets = manager.remote_data.list( + spec.get("data_center"), + access, + day=spec.get("day"), + duration=spec.get("duration") or 0, + query=query, + ignore_data_centers=[push_to_remote] if push_to_remote else None + ) + except AuthenticationError as e: + # Reported as auth, not as data. Without this the credential failure + # arrives at the "no datasets matched" branch below and is described as + # an empty scope -- which sends the operator to check the specifier + # while the actual fix is to log in. + console.print(f"[red]{e}[/red]") + return CommandResult( + success=False, + object={"datasets": [], "specifier": specifier}, + msg=str(e), + error_type=ErrorType.AUTH, + exit_code=ExitCode.AUTH_ERROR, + command="remote pull", + ) # Print datasets (short format like ls) for d in datasets: @@ -297,7 +313,6 @@ def remote_pull( # datasets and simply finds their files present. What is reported here is # narrower: discovery matched no dataset, so nothing was even considered. if not datasets: - from owilix.core.types import ErrorType, ExitCode return CommandResult( success=False, object={"datasets": [], "specifier": specifier}, @@ -418,7 +433,6 @@ def remote_pull( console.print(f"\n[red bold]{len(failed_files)} file(s) failed to download:[/red bold]") for path, err in failed_files: console.print(f" [red]✗[/red] {path}: {err}") - from owilix.core.types import ErrorType, ExitCode return CommandResult( success=False, object={"datasets": datasets, "failed_files": failed_files}, diff --git a/tests/owilix/core/test_exceptions.py b/tests/owilix/core/test_exceptions.py new file mode 100644 index 0000000..875373c --- /dev/null +++ b/tests/owilix/core/test_exceptions.py @@ -0,0 +1,235 @@ +"""F3 -- an unusable credential must not arrive as an empty result set. + +Observed on the OpenWebSearch.eu clusters: with a valid specifier and no usable +LEXIS credential, ``LexisRepository.list`` caught the failure, logged it, and +returned ``[]``. ``AggregatedRepository.list`` swallowed it a second time. The +command layer then reported "0 datasets found" and -- once F2 made matching +nothing a failure -- the actively misleading "No datasets matched specifier", +which sends an operator to check the scope when the fix is to log in. +""" +from unittest.mock import MagicMock + +import pytest + +from owilix.core.exceptions import ( + AuthenticationError, + OwilixError, + looks_like_authentication_failure, +) + + +class TestAuthenticationFailureDetection: + def test_detects_the_production_lexis_auth_error(self): + """The exact exception the clusters produced. + + py4lexis has a typed Py4LexisAuthException, but a missing credential + comes back as a Py4LexisAPIException, because the DDI API answers with + an errorString py4lexis does not recognise. Detection by type alone + therefore misses the case that actually happens. + """ + from py4lexis.core.exceptions import Py4LexisAPIException + + exc = Py4LexisAPIException( + "Unrecognized 'errorString' obtained in the content: Authorization header empty.", + "POST", + "https://api.lexis.tech/api/ddiapi/v2/meta/search", + ) + + assert looks_like_authentication_failure(exc) + + def test_detects_py4lexis_auth_exception_by_type(self): + from py4lexis.core.exceptions import Py4LexisAuthException + + assert looks_like_authentication_failure(Py4LexisAuthException("Token expired")) + + @pytest.mark.parametrize( + "message", + [ + "401 Client Error: Unauthorized for url: https://example.test", + "invalid_grant: refresh token is expired", + "Permission denied", + ], + ) + def test_detects_common_auth_shapes(self, message): + assert looks_like_authentication_failure(RuntimeError(message)) + + @pytest.mark.parametrize( + "message", + [ + "Cannot get all datasets' status records!!! Returned records are 'None'!!!", + "Connection reset by peer", + "No such file or directory", + # Must not fire on the word 'token' alone -- a network fault + # misreported as a login problem is its own wrong turn. + "Failed to tokenize response body", + ], + ) + def test_does_not_fire_on_non_auth_failures(self, message): + assert not looks_like_authentication_failure(RuntimeError(message)) + + def test_authentication_error_is_owilix_error(self): + assert issubclass(AuthenticationError, OwilixError) + + def test_message_names_the_repository_and_the_way_out(self): + error = AuthenticationError(repository="lexis", cause=RuntimeError("boom")) + text = str(error) + + assert "lexis" in text + assert "PY4LEXIS_TOKEN" in text + assert "boom" in text, "the underlying cause must survive, not be replaced" + + +class TestAggregateDoesNotHideAuthFailures: + """The second layer of swallowing: per-repository resilience in the aggregate.""" + + def _aggregate(self, repositories): + from owilix.core.repository.aggregate import AggregatedRepository + + return AggregatedRepository(repositories=repositories) + + def test_auth_failure_with_no_results_is_raised(self): + failing = MagicMock() + failing.list.side_effect = AuthenticationError(repository="lexis") + aggregate = self._aggregate({"lexis": failing}) + + with pytest.raises(AuthenticationError): + aggregate.list("lexis", "public") + + def test_a_repository_that_answered_still_returns_its_datasets(self): + """Resilience is kept: one broken backend must not sink the listing.""" + dataset = MagicMock() + dataset.metadata.get.return_value = "ds-1" + # Deduplication scores the owning repository, so its performance stats + # have to be numbers rather than mocks. + stats = dataset.repository.performance_stats + stats.list_response_time.get_overall_average.return_value = 1.0 + stats.list_bandwidth.get_overall_average.return_value = 1.0 + + failing = MagicMock() + failing.list.side_effect = AuthenticationError(repository="lexis") + working = MagicMock() + working.list.return_value = [dataset] + + aggregate = self._aggregate({"lexis": failing, "owi-up": working}) + + result = aggregate.list(None, "public") + + assert result == [dataset] + + def test_non_auth_failures_still_degrade_to_empty(self): + """Unchanged behaviour, asserted so the narrowing is deliberate.""" + failing = MagicMock() + failing.list.side_effect = RuntimeError("backend exploded") + aggregate = self._aggregate({"lexis": failing}) + + assert aggregate.list("lexis", "public") == [] + + +class TestRemotePullReportsAuthNotData: + """The whole point of F3: the command layer must say 'auth', not 'no data'. + + With F2 in place, a swallowed credential failure does not merely produce a + quiet success any more -- it produces `No datasets matched specifier '...'` + with ErrorType.DATA, which reads as "your scope is empty". That is worse + than the original silence for anyone debugging, because it is confidently + wrong about where the problem is. + """ + + def test_auth_failure_is_reported_as_auth(self): + from owilix.core.tasks.remote import remote_pull + from owilix.core.types import ErrorType, ExitCode + + manager = MagicMock() + manager.parse_specifier.return_value = { + "data_center": None, + "query": {"access": "public"}, + "day": None, + "duration": 0, + } + manager.remote_data.list.side_effect = AuthenticationError(repository="lexis") + + result = remote_pull( + manager, specifier="all", auto_yes=True, console=MagicMock() + ) + + assert result.success is False + assert result.error_type == ErrorType.AUTH + assert result.exit_code == ExitCode.AUTH_ERROR + # Not the F2 message: the scope is not the problem. + assert "No datasets matched" not in result.msg + assert "PY4LEXIS_TOKEN" in result.msg + + def test_a_genuinely_empty_scope_is_still_a_data_error(self): + """F2's behaviour must survive F3 -- these are different faults.""" + from owilix.core.tasks.remote import remote_pull + from owilix.core.types import ErrorType, ExitCode + + manager = MagicMock() + manager.parse_specifier.return_value = { + "data_center": "dc1", + "query": {"access": "public"}, + "day": None, + "duration": 0, + } + manager.remote_data.list.return_value = [] + + result = remote_pull( + manager, specifier="dc1/public", auto_yes=True, console=MagicMock() + ) + + assert result.success is False + assert result.error_type == ErrorType.DATA + assert result.exit_code == ExitCode.DATA_ERROR + + +class TestLexisRepositoryRaisesRatherThanReturningEmpty: + """The entry point F3 is really about. + + `LexisRepository.list` caught every exception and returned `[]`. That is + where "not logged in" first became "no datasets", before the aggregate and + the command layer each had a chance to lose it again. + """ + + class DummySession: + def __init__(self): + self._zone = "IT4ILexisV2" + self.irods_http_api_url = "https://example.com/irods" + + class DummyManager: + def __init__(self): + outer = TestLexisRepositoryRaisesRatherThanReturningEmpty + + class SessionWrapper: + lexis = outer.DummySession() + + self.session = SessionWrapper() + + def _repo(self): + from owilix.core.repository import LexisRepository + + return LexisRepository(self.DummyManager()) + + def test_auth_failure_raises(self): + from py4lexis.core.exceptions import Py4LexisAPIException + + repo = self._repo() + repo._ddi_api = MagicMock() + repo._ddi_api.get_all_datasets.side_effect = Py4LexisAPIException( + "Unrecognized 'errorString' obtained in the content: Authorization header empty.", + "POST", + "https://api.lexis.tech/api/ddiapi/v2/meta/search", + ) + + with pytest.raises(AuthenticationError) as excinfo: + repo.list(access="public") + + # The operator-actionable original must not be thrown away. + assert "Authorization header empty" in str(excinfo.value) + + def test_other_failures_still_return_empty(self): + """Deliberately unchanged, so the new behaviour stays narrow.""" + repo = self._repo() + repo._ddi_api = MagicMock() + repo._ddi_api.get_all_datasets.side_effect = RuntimeError("backend exploded") + + assert repo.list(access="public") == []