From cab7a0a1e193609efc312340a9b11a8e4fa0bdfb Mon Sep 17 00:00:00 2001 From: "Charles Graham, SWT" Date: Sat, 11 Jul 2026 16:14:08 -0500 Subject: [PATCH] fix: harden clob download error handling --- cwmscli/commands/blob.py | 5 +++-- cwmscli/commands/clob.py | 36 +++++++++++++++++++++++++-------- cwmscli/utils/__init__.py | 1 + tests/commands/test_clob.py | 40 +++++++++++++++++++++++++++++++++++++ 4 files changed, 72 insertions(+), 10 deletions(-) diff --git a/cwmscli/commands/blob.py b/cwmscli/commands/blob.py index b058266..a1df095 100644 --- a/cwmscli/commands/blob.py +++ b/cwmscli/commands/blob.py @@ -613,12 +613,12 @@ def download_cmd( try: blob_content = cwms.get_blob(office_id=office, blob_id=bid) target = dest or _default_download_dest(bid) - _save_blob_content( + saved_target = _save_blob_content( blob_content, dest=target, media_type_hint=_blob_media_type(cwms, office, bid), ) - logging.info(f"Downloaded blob to: {target}") + logging.info(f"Downloaded blob to: {saved_target}") except requests.HTTPError as e: detail = getattr(e.response, "text", "") or str(e) logging.error(f"Failed to download (HTTP): {detail}") @@ -632,6 +632,7 @@ def download_cmd( sys.exit(1) except Exception as e: logging.error(format_local_download_error(e, BLOB_DOCS_URL)) + # Local write/path failures are not CDA credential scope problems. if not isinstance(e, (OSError, ValueError)): log_scoped_read_hint( credential_kind=credential_kind, diff --git a/cwmscli/commands/clob.py b/cwmscli/commands/clob.py index 613467f..9409d2c 100644 --- a/cwmscli/commands/clob.py +++ b/cwmscli/commands/clob.py @@ -12,10 +12,15 @@ from cwmscli.utils import ( format_local_download_error, get_api_key, + get_saved_login_token, has_invalid_chars, + init_cwms_session, log_scoped_read_hint, validate_default_download_dest, ) +from cwmscli.utils.click_help import DOCS_BASE_URL + +CLOB_DOCS_URL = f"{DOCS_BASE_URL}/cli/clob.html" def _join_api_url(api_root: str, path: str) -> str: @@ -28,6 +33,16 @@ def _resolve_optional_api_key(api_key: Optional[str], anonymous: bool) -> Option return get_api_key(api_key, None) +def _resolve_credential_kind(api_key: Optional[str], anonymous: bool) -> Optional[str]: + if anonymous: + return None + if get_saved_login_token(): + return "token" + if _resolve_optional_api_key(api_key, anonymous): + return "api_key" + return None + + def _write_clob_content(content: str, dest: str) -> str: os.makedirs(os.path.dirname(dest) or ".", exist_ok=True) with open(dest, "w", encoding="utf-8", newline="") as f: @@ -36,12 +51,17 @@ def _write_clob_content(content: str, dest: str) -> str: def _default_download_dest(clob_id: str) -> str: - return validate_default_download_dest(clob_id, resource_name="Clob") + return validate_default_download_dest( + clob_id, + resource_name="Clob", + docs_url=CLOB_DOCS_URL, + ) def _clob_endpoint_id(clob_id: str) -> tuple[str, Optional[str]]: normalized = clob_id.upper() if has_invalid_chars(normalized): + # Path-like IDs use the placeholder route and keep the real ID in query params. return "ignored", normalized return normalized, None @@ -190,8 +210,8 @@ def download_cmd( f"DRY RUN: would GET {api_root} clob with clob-id={clob_id} office={office}." ) return - resolved_api_key = _resolve_optional_api_key(api_key, anonymous) - cwms.init_session(api_root=api_root, api_key=resolved_api_key) + credential_kind = _resolve_credential_kind(api_key, anonymous) + init_cwms_session(cwms, api_root=api_root, api_key=api_key, anonymous=anonymous) bid = clob_id.upper() logging.debug(f"Office={office} clobID={bid}") @@ -215,7 +235,7 @@ def download_cmd( detail = getattr(e.response, "text", "") or str(e) logging.error(f"Failed to download (HTTP): {detail}") log_scoped_read_hint( - api_key=resolved_api_key, + credential_kind=credential_kind, anonymous=anonymous, office=office, action="download", @@ -223,7 +243,7 @@ def download_cmd( ) sys.exit(1) except Exception as e: - logging.error(format_local_download_error(e, "")) + logging.error(format_local_download_error(e, CLOB_DOCS_URL)) sys.exit(1) @@ -305,8 +325,8 @@ def list_cmd( api_key: str, anonymous: bool = False, ): - resolved_api_key = _resolve_optional_api_key(api_key, anonymous) - cwms.init_session(api_root=api_root, api_key=resolved_api_key) + credential_kind = _resolve_credential_kind(api_key, anonymous) + init_cwms_session(cwms, api_root=api_root, api_key=api_key, anonymous=anonymous) try: df = list_clobs( office=office, @@ -319,7 +339,7 @@ def list_cmd( ) except Exception: log_scoped_read_hint( - api_key=resolved_api_key, + credential_kind=credential_kind, anonymous=anonymous, office=office, action="list", diff --git a/cwmscli/utils/__init__.py b/cwmscli/utils/__init__.py index 462b8ab..76260f1 100644 --- a/cwmscli/utils/__init__.py +++ b/cwmscli/utils/__init__.py @@ -270,6 +270,7 @@ def validate_default_download_dest( f"Pass --dest explicitly if needed." ) + # Leading separators can be part of a CDA ID; default downloads stay relative. target = raw_id.lstrip("/\\") if not target: message = ( diff --git a/tests/commands/test_clob.py b/tests/commands/test_clob.py index 1f36d82..02f73fc 100644 --- a/tests/commands/test_clob.py +++ b/tests/commands/test_clob.py @@ -307,6 +307,46 @@ class FakeHTTPError(Exception): assert "/cli/blob.html" not in caplog.text +def test_download_cmd_http_error_logs_scope_hint( + tmp_path, monkeypatch: pytest.MonkeyPatch, caplog +): + class FakeResponse: + text = "Forbidden" + + class FakeHTTPError(Exception): + response = FakeResponse() + + class FakeCwms: + @staticmethod + def init_session(api_root, api_key): + return None + + @staticmethod + def get_clob(office_id, clob_id): + raise FakeHTTPError() + + monkeypatch.setitem(sys.modules, "cwms", FakeCwms) + monkeypatch.setattr("cwmscli.commands.clob.cwms", FakeCwms) + monkeypatch.setattr( + "cwmscli.commands.clob.requests", + types.SimpleNamespace(HTTPError=FakeHTTPError), + ) + + with caplog.at_level(logging.WARNING), pytest.raises(SystemExit) as exc: + download_cmd( + clob_id="test_clob", + dest=str(tmp_path / "downloaded.txt"), + office="SWT", + api_root="https://example.test/", + api_key="apikey 123", + dry_run=False, + ) + + assert exc.value.code == 1 + assert "Access scope hint: an API key was sent" in caplog.text + assert "clob content" in caplog.text + + def test_list_cmd_initializes_session_with_api_key(monkeypatch: pytest.MonkeyPatch): calls = []