From 3bcf4762022abf701997b1bfb41a6850c7aba8e0 Mon Sep 17 00:00:00 2001 From: Magnus Hedemark Date: Wed, 26 Aug 2026 19:35:15 -0400 Subject: [PATCH] fix(openlibrary): warn when merge-stub redirect walk exhausts its budget Addresses the tracked follow-up debt from thicken-openlibrary: the client already bounds /type/redirect stub chasing at MAX_REDIRECT_HOPS, but a chain that outlives the budget silently handed back an opaque stub, which downstream commands rendered as an empty-shaped record with no hint why. The walk now emits a stderr warning naming the unresolved location before returning; mocked test drives HOPS+1 chained stubs end to end. Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com> --- openlibrary/scripts/openlibrary | 5 +++++ openlibrary/scripts/test_openlibrary.py | 19 +++++++++++++++++++ 2 files changed, 24 insertions(+) diff --git a/openlibrary/scripts/openlibrary b/openlibrary/scripts/openlibrary index af1a08a..abb82ae 100755 --- a/openlibrary/scripts/openlibrary +++ b/openlibrary/scripts/openlibrary @@ -148,6 +148,11 @@ class OpenLibraryClient: params = None hops += 1 continue + if is_redirect_stub(data): + # Bounded stub walk exhausted (>N chained merges); make that + # visible instead of handing back an opaque stub silently. + warn(f"Redirect chain did not resolve within {MAX_REDIRECT_HOPS}" + f" hops (still stuck at {data.get('location', '?')})") self.last_url = resp.url return data diff --git a/openlibrary/scripts/test_openlibrary.py b/openlibrary/scripts/test_openlibrary.py index 4d4b2d9..f892d0e 100644 --- a/openlibrary/scripts/test_openlibrary.py +++ b/openlibrary/scripts/test_openlibrary.py @@ -379,6 +379,25 @@ class MockedClientTests(unittest.TestCase): "https://openlibrary.org" + WORK_KEY + ".json") self.assertEqual(json.loads(out)["title"], "Nineteen Eighty-Four") + def test_redirect_stub_chain_beyond_hop_budget_warns_instead_of_silence(self): + # A work that keeps resolving into further merge stubs exhausts the + # bounded walk; the CLI must say so (stderr warning) rather than emit + # an unexplained /type/redirect payload. + stub = FakeResponse(200, {"type": {"key": "/type/redirect"}, + "location": WORK_KEY}) + responses = [stub] * (ol_cli.MAX_REDIRECT_HOPS + 1) + with mock.patch.object(requests, "get", + side_effect=responses) as req: + code, out, err = run_cli("--json", "work", "OL24776360W") + self.assertEqual(code, 0) + self.assertEqual(req.call_count, ol_cli.MAX_REDIRECT_HOPS + 1) + self.assertIn("did not resolve", err) + # Without the resolution the command degrades to an empty-shaped + # record; the stderr warning is what keeps that from being silent. + self.assertEqual(json.loads(out), { + "key": "OL24776360W", "title": "?", "authors": [], + "description": "", "subjects": [], "cover_url": None}) + def test_text_wrapper_dict_is_unwrapped(self): self.assertEqual(ol_cli.unwrap_text({"type": "/type/text", "value": "hi"}), "hi") self.assertEqual(ol_cli.unwrap_text("plain"), "plain")