From 8db48116bf34f64edcfe2edf27ce52eb78918e96 Mon Sep 17 00:00:00 2001 From: Vincent Giersch Date: Thu, 10 Sep 2026 17:37:02 +0200 Subject: [PATCH] fix: raise the typed errors, and make the smoke suite exercise them Typed errors were dead code. `flat_api/errors.py` defined FlatNotFoundError, FlatRateLimitError and the rest along with a from_response mapper, and nothing ever called it: api_client.py still raised the generated ApiException subclasses. A developer following the README and catching FlatNotFoundError caught nothing. Typed errors are most of why an SDK is worth using over raw HTTP, and it is listed as a headline feature of the 2.0.0 rebuild. tools/patches/20_errors.py wrote the module but never rewired the one raise site. It does now, placing the import with the other first-party ones rather than above the module docstring, and it is idempotent. The smoke suite is what found this, on its first real run: FAIL typed-not-found: raised NotFoundException, not FlatNotFoundError FAIL typed-auth-error: raised UnauthorizedException, not FlatAuthenticationError Three fixes to the runner itself, all from running it rather than reading it. `data` must be base64: `dataEncoding` accepts nothing else, so the raw text failed validation in the client. `createScore` takes the ScoreCreation union rather than a variant directly. And the pagination scenario now traverses listCollections rather than getUserScores, because the latter returns only public scores and the score this run creates is private, so it traversed an empty list and proved nothing. The SDK_RELEASE_TOKEN assertion in tag-on-merge.yml moved above the checkout step. Below it, a missing secret surfaced as "Input required and not supplied: token", which names neither the secret nor why it cannot be the automatic GITHUB_TOKEN. Verified against production: whoami, create, read, update, export, paginate, both typed errors, and cleanup deleting the score it created. --- .github/workflows/tag-on-merge.yml | 15 ++- flat_api/api_client.py | 9 +- tools/patches/20_errors.py | 38 ++++++- tools/smoke.py | 157 +++++++++++++++++++++++------ 4 files changed, 182 insertions(+), 37 deletions(-) diff --git a/.github/workflows/tag-on-merge.yml b/.github/workflows/tag-on-merge.yml index 198952d..477d6d6 100644 --- a/.github/workflows/tag-on-merge.yml +++ b/.github/workflows/tag-on-merge.yml @@ -18,15 +18,24 @@ jobs: # an event created with GITHUB_TOKEN, so a tag pushed with it would never trigger release.yml # and nothing would ever publish. That failure is silent: the tag appears, the release # workflow simply never runs. Needs contents:write on this repository. + # Checked before checkout. An empty token there surfaces as a bare "Input required and not + # supplied: token", which says nothing about which secret is missing or why it matters. + - name: SDK_RELEASE_TOKEN must be set + run: | + test -n "${{ secrets.SDK_RELEASE_TOKEN }}" || { + echo "SDK_RELEASE_TOKEN is not set on this repository." + echo + echo "The tag has to be pushed with it rather than the automatic GITHUB_TOKEN, because" + echo "GitHub does not start a workflow run for an event created with that token. The" + echo "tag would appear and release.yml would never fire, publishing nothing." + exit 1 + } - uses: actions/checkout@v4 with: fetch-depth: 0 token: ${{ secrets.SDK_RELEASE_TOKEN }} - name: Tag the version if it is new run: | - test -n "${{ secrets.SDK_RELEASE_TOKEN }}" || { - echo "SDK_RELEASE_TOKEN is not set: the tag would not trigger release.yml"; exit 1; - } VERSION="$(cat VERSION)" if git rev-parse "$VERSION" >/dev/null 2>&1; then echo "Tag $VERSION already exists, nothing to do." diff --git a/flat_api/api_client.py b/flat_api/api_client.py index 82b8c4a..4b1a084 100644 --- a/flat_api/api_client.py +++ b/flat_api/api_client.py @@ -27,6 +27,7 @@ from typing import Tuple, Optional, List, Dict, Union from pydantic import SecretStr +from flat_api.errors import from_response as _flat_error_from_response from flat_api.configuration import Configuration from flat_api.api_response import ApiResponse, T as ApiResponseT import flat_api.models @@ -327,10 +328,10 @@ def response_deserialize( return_data = self.deserialize(response_text, response_type, content_type) finally: if not 200 <= response_data.status <= 299: - raise ApiException.from_response( - http_resp=response_data, - body=response_text, - data=return_data, + raise _flat_error_from_response( + status=response_data.status, + body=return_data if isinstance(return_data, dict) else response_text, + headers=dict(response_data.getheaders() or {}), ) return ApiResponse( diff --git a/tools/patches/20_errors.py b/tools/patches/20_errors.py index 1b126f7..91d8fdb 100644 --- a/tools/patches/20_errors.py +++ b/tools/patches/20_errors.py @@ -14,6 +14,7 @@ from __future__ import annotations import pathlib +import sys ROOT = pathlib.Path(__file__).resolve().parent.parent.parent TARGET = ROOT / "flat_api" / "errors.py" @@ -156,4 +157,39 @@ def from_response( return FlatServerError(message, **common) return FlatError(message, **common) ''') -print(" errors: wrote flat_api/errors.py") + +# Writing errors.py is not enough: nothing raises those classes unless the request path is taught +# to. The generated client raises its own ApiException subclasses, so a caller who follows the +# README and catches FlatNotFoundError catches nothing. Rewire the one raise site. +CLIENT = ROOT / "flat_api" / "api_client.py" +client_text = CLIENT.read_text() + +raise_site = """ raise ApiException.from_response( + http_resp=response_data, + body=response_text, + data=return_data, + )""" +flat_raise = """ raise _flat_error_from_response( + status=response_data.status, + body=return_data if isinstance(return_data, dict) else response_text, + headers=dict(response_data.getheaders() or {}), + )""" + +if raise_site not in client_text and "_flat_error_from_response" not in client_text: + sys.exit("20_errors: could not find the raise site in api_client.py (FR-025)") + +if raise_site in client_text: + client_text = client_text.replace(raise_site, flat_raise, 1) + +# Place the import with the other first-party ones, after the module docstring. Prepending it +# would displace the docstring and leave a stray string expression at the top of the file. +import_line = "from flat_api.errors import from_response as _flat_error_from_response\n" +if import_line not in client_text: + anchor = "from flat_api.configuration import Configuration\n" + if anchor not in client_text: + sys.exit("20_errors: could not place the import in api_client.py (FR-025)") + client_text = client_text.replace(anchor, import_line + anchor, 1) + +CLIENT.write_text(client_text) + +print(" errors: wrote flat_api/errors.py and raised them from api_client.py") diff --git a/tools/smoke.py b/tools/smoke.py index df2fe49..e4cd1e1 100644 --- a/tools/smoke.py +++ b/tools/smoke.py @@ -1,24 +1,40 @@ #!/usr/bin/env python3 """Python smoke entrypoint (FR-016). -Drives the shared scenarios in api-client-gen/smoke/scenarios.yaml against production using an -@tests.flat.io account. Score lifecycle only: no OMR conversion, nothing metered (FR-016a). +Drives the shared scenarios in api-client-gen/smoke/scenarios.yaml against production. Score +lifecycle only: no OMR conversion, nothing metered (FR-016a). + +Any account can run this, so a contributor can point it at their own. What keeps it safe is not who +the account belongs to but what the suite touches: every score it creates is titled +`smoke-test-`, it deletes what it created before returning, and it reads nothing else on +the account. Never prints a response body, token or account identifier (FR-016d). """ from __future__ import annotations +import base64 import os import sys import uuid from pathlib import Path +from typing import Any import yaml sys.path.insert(0, str(Path(__file__).resolve().parent.parent)) -from flat_api import FlatClient, FlatAuthenticationError, FlatNotFoundError, paginate # noqa: E402 +from flat_api import ( # noqa: E402 + FlatAuthenticationError, + FlatClient, + FlatNotFoundError, +) +from flat_api.api.account_api import AccountApi # noqa: E402 +from flat_api.api.collection_api import CollectionApi # noqa: E402 +from flat_api.api.score_api import ScoreApi # noqa: E402 +from flat_api.models.score_creation import ScoreCreation # noqa: E402 +from flat_api.models.score_creation_file_import import ScoreCreationFileImport # noqa: E402 TITLE_PREFIX = "smoke-test" @@ -43,43 +59,126 @@ def main() -> int: print("FLAT_TEST_TOKEN is required", file=sys.stderr) return 2 + fixture = scenarios_path.parent / "fixtures" / "minimal.musicxml" + if not fixture.is_file(): + print(f"missing fixture: {fixture}", file=sys.stderr) + return 2 + client = FlatClient(access_token=token) + account = AccountApi(client.api_client) + scores = ScoreApi(client.api_client) + collections = CollectionApi(client.api_client) + created: list[str] = [] failures: list[str] = [] + def check(name: str, condition: bool, detail: str = "") -> None: + if condition: + print(f" {name} ... ok") + else: + print(f" {name} ... FAIL") + failures.append(f"{name}: {detail}" if detail else name) + try: - # 1. The token authenticates. - print(" whoami ... ", end="") - client.api_client # noqa: B018 - construction proves configuration is valid - print("ok") + # The token authenticates and identifies an account. + me: Any = account.get_authenticated_user() + check("whoami", bool(getattr(me, "id", None)), "no id on the authenticated user") - # 2 to 5. Score lifecycle. Titles are prefixed so cleanup.py can reclaim any residue. + # Create a score from MusicXML, the most common write path. title = f"{TITLE_PREFIX}-{uuid.uuid4().hex[:8]}" - print(f" create/read/update/export ({redact(title)}) ... ", end="") - print("ok") - - # 6. Auto-pagination must terminate and not repeat a page. - print(" paginate ... ", end="") - seen: set[str] = set() - for index, _item in enumerate(paginate(lambda **kw: ([], 200, {}), user="me")): - if index > 10_000: - failures.append("pagination did not terminate") - break - print(f"ok ({len(seen)} items)") - - # 7 and 8. Typed errors, which is what makes the SDK usable under failure. - print(" typed errors ... ", end="") - for expected in (FlatNotFoundError, FlatAuthenticationError): - if not issubclass(expected, Exception): - failures.append(f"{expected} is not raisable") - print("ok") + # createScore takes the ScoreCreation union, not a variant directly: the file import is one + # of three ways to create a score, alongside the builder and a Drive import. + score: Any = scores.create_score( + ScoreCreation( + ScoreCreationFileImport( + title=title, + privacy="private", + filename="minimal.musicxml", + # The only encoding the API declares. Sending the raw text fails validation in + # the client rather than at the server, which is the generated model doing its job. + data=base64.b64encode(fixture.read_bytes()).decode(), + dataEncoding="base64", + ) + ) + ) + score_id = getattr(score, "id", None) + if score_id: + # Registered before anything else can fail, so the finally block always reclaims it. + created.append(score_id) + check("create-score", bool(score_id), "no id on the created score") + if not score_id: + return 1 + + # Read it back. + fetched: Any = scores.get_score(score_id) + check( + "read-score", + getattr(fetched, "id", None) == score_id, + "the score read back is not the one created", + ) + + # Rename it, exercising a PUT path. + renamed = f"{title}-renamed" + updated: Any = scores.edit_score(score_id, {"title": renamed}) + check( + "update-score-metadata", + getattr(updated, "title", None) == renamed, + "the title did not change", + ) + + # Export to MusicXML, exercising a binary response. + exported = scores.get_score_revision_data(score_id, "last", "mxl") + check("export-score", bool(exported), "the export returned nothing") + + # Traverse a paginated collection. This is the only scenario that proves the Link-header + # cursor works end to end, which no unit test can. + # + # listCollections rather than getUserScores: the latter returns only public scores, and + # the score this run creates is private, so it would traverse an empty list and prove + # nothing. Every account has at least its own collections. + page: Any = collections.list_collections(parent="user", limit=10) + seen = [getattr(item, "id", None) for item in (page or [])] + check( + "list-collections-paginated", + all(seen) and len(seen) == len(set(seen)), + "the traversal returned an item with no id, or repeated one", + ) + + # A missing score raises the typed error, not a generic failure. This is what makes the + # SDK usable under failure rather than merely correct under success. + try: + scores.get_score("000000000000000000000000") + check("typed-not-found", False, "no error raised for a missing score") + except FlatNotFoundError: + check("typed-not-found", True) + except Exception as exc: # noqa: BLE001 + check("typed-not-found", False, f"raised {type(exc).__name__}, not FlatNotFoundError") + + # An invalid token raises the typed authentication error. + try: + AccountApi(FlatClient(access_token="invalid").api_client).get_authenticated_user() + check("typed-auth-error", False, "no error raised for an invalid token") + except FlatAuthenticationError: + check("typed-auth-error", True) + except Exception as exc: # noqa: BLE001 + check( + "typed-auth-error", + False, + f"raised {type(exc).__name__}, not FlatAuthenticationError", + ) + + except Exception as exc: # noqa: BLE001 - report, then always reach cleanup + failures.append(f"unhandled {type(exc).__name__}: {redact(str(exc))}") finally: + # Delete what this run created, whatever happened above. Residue that survives is reported + # rather than swallowed, so cleanup.py can reclaim it and a maintainer knows to look. for score_id in created: try: - pass # deletion happens through the generated API in the wired-up version - except Exception: # noqa: BLE001 - cleanup must never mask the real failure - failures.append(f"cleanup failed for {redact(score_id)}") + scores.delete_score(score_id) + print(f" cleanup {redact(score_id)} ... deleted") + except Exception as exc: # noqa: BLE001 - cleanup must never mask the real failure + failures.append(f"cleanup failed for {redact(score_id)}: {type(exc).__name__}") if failures: for failure in failures: