From 7d9d718b4161ce48dcce0d9fb7f0c6d7251fae97 Mon Sep 17 00:00:00 2001 From: hermes Date: Mon, 9 Nov 2026 18:24:57 +1000 Subject: [PATCH] fix(advice): an empty token must not be used as a guard None of the 47 existing readings have a token - they all predate it. A reading read from the database therefore has an EMPTY token, and the guard was checking `expected_token is not None`, which is TRUE for an empty string. So the write was conditioned on a token that does not exist, matched nothing, and **the advice would never have been written for any of the existing readings**. Verified against the live database, before the fix: write with expected_token='' -> 0 (0 = the advice would NEVER be written) After: write for a pre-token live reading: 1 (must be 1) An empty token is now not a usable guard - there is no token to match - so the write falls through to the gravity check instead. A non-empty token is still the primary guard. There is a test for it, with a reading inserted the way the existing ones are - directly, with no token. 178 tests (up from 177). --- src/flask/mongo/repository.py | 7 ++++++- tests/test_brew_advice.py | 35 +++++++++++++++++++++++++++++++++++ 2 files changed, 41 insertions(+), 1 deletion(-) diff --git a/src/flask/mongo/repository.py b/src/flask/mongo/repository.py index a0f0505..9d5ddec 100644 --- a/src/flask/mongo/repository.py +++ b/src/flask/mongo/repository.py @@ -263,10 +263,15 @@ class BeerRepository: return 0 query: dict = {"_id": {"$in": doc_id}} - if expected_token is not None: + if expected_token: # The editing token is the PRIMARY guard: it changes on every # edit, so it catches a stale generation whatever else # changed - the gravity, the comment, or nothing. + # + # An EMPTY token is not a usable guard: it means the reading + # predates the token, so there is no token to match. + # Querying on it would match nothing, and the advice would + # never be written for the existing readings. query["advice_token"] = str(expected_token) elif expected_sg is not None: # No token - a direct call with a gravity to check against. diff --git a/tests/test_brew_advice.py b/tests/test_brew_advice.py index 314c9a4..e27e95c 100644 --- a/tests/test_brew_advice.py +++ b/tests/test_brew_advice.py @@ -710,6 +710,41 @@ def test_the_token_is_the_primary_guard(repo): assert after.get("advice") is None +def test_the_advice_is_written_for_a_reading_that_predates_the_token(repo): + """None of the existing readings have a token. + + They all predate it, so a reading read from the database has an EMPTY + token. An empty token is not a usable guard - there is no token to + match - so the write must not be conditioned on it, or the advice + would never be written for any of them. + """ + # Inserted directly, the way the readings already in the database + # are - no token. + repo.readings.insert_one({ + "_id": "l78ZidBJ3QaYn98tvB88x4uM", + "beer_run_id": "1", + "beer_type": "Ale", + "sg": "1042", + "date": "2024-09-01", + "final_reading": "False", + "comment": "a comment", + }) + + run = repo.run("1") + assert run.readings[0].advice_token == "", "the reading should have no token" + + # The advice flow passes the token it read, which is empty. + written = repo.set_reading_advice("l78ZidBJ3QaYn98tvB88x4uM", + "THE ADVICE", True, + expected_sg=None, + expected_token=run.readings[0].advice_token) + assert written == 1, "the advice was never written for a pre-token reading" + + after = repo.readings.find_one({"beer_run_id": "1"}) + assert after["advice"] == "THE ADVICE" + assert after["comment"] == "a comment" + + def test_a_reply_cut_off_at_the_token_ceiling_is_marked(repo): """A reply that stopped at the token limit is incomplete.