diff --git a/zeeguu/api/endpoints/article_upload.py b/zeeguu/api/endpoints/article_upload.py index df40eaaf1..fbf5470e0 100644 --- a/zeeguu/api/endpoints/article_upload.py +++ b/zeeguu/api/endpoints/article_upload.py @@ -198,6 +198,7 @@ def article_upload_translate_and_adapt(upload_id): from zeeguu.core.model.url import Url from zeeguu.core.model.source import Source from zeeguu.core.model.source_type import SourceType + from zeeguu.core.model.race_safe import find_or_create_and_report from datetime import datetime user = User.find_by_id(flask.g.user_id) @@ -260,28 +261,47 @@ def article_upload_translate_and_adapt(upload_id): ) clean_summary = result.get("summary") or (result["content"][:200] + "...") - - translated_article = Article( - translated_url, - result["title"], - None, - source_obj, - clean_summary, - datetime.now(), - None, - target_lang_obj, - result["content"], - None, - ) - translated_article.cefr_level = user_level - translated_article.source_upload_id = upload.id - if upload.image_url: - translated_article.img_url = Url.find_or_create(db_session, upload.image_url) - - db_session.add(translated_article) - db_session.commit() - - translated_article.create_article_fragments(db_session) + img_url = Url.find_or_create(db_session, upload.image_url) if upload.image_url else None + + def make_translated_article(): + article = Article( + translated_url, + result["title"], + None, + source_obj, + clean_summary, + datetime.now(), + None, + target_lang_obj, + result["content"], + None, + ) + article.cefr_level = user_level + article.source_upload_id = upload.id + article.img_url = img_url + return article + + if translated_url is None: + # Url.find_or_create gives None when the path is over 255 chars, and + # the "#translated-from-..." suffix can push an upload's url over. + # With no url to key on, the helper would look up `url_id IS NULL` + # and find unrelated articles; insert directly, as before. + translated_article, created = make_translated_article(), True + db_session.add(translated_article) + else: + # A second request for the same upload (double tap, retry) can miss + # the Article.find above while the first one waits on the LLM. Both + # then insert at this url; the loser gets the winner's article back. + translated_article, created = find_or_create_and_report( + db_session, Article, dict(url=translated_url), make_translated_article + ) + # Fragments only for an article this request created: the loser must not + # add a second set to the winner's. One commit for article and fragments, + # so whoever sees the article sees its fragments too. For the loser the + # commit also ends its REPEATABLE READ snapshot, which predates the + # winner's commit and would show the article without fragments. + if created: + translated_article.create_article_fragments(db_session) db_session.commit() _ensure_personal_copy(user, translated_article) diff --git a/zeeguu/api/test/test_article_upload_translate_race.py b/zeeguu/api/test/test_article_upload_translate_race.py new file mode 100644 index 000000000..6704d47a0 --- /dev/null +++ b/zeeguu/api/test/test_article_upload_translate_race.py @@ -0,0 +1,149 @@ +""" +Two requests to /article_upload//translate_and_adapt for the same upload +(a double tap, or a retry while the first waits on the LLM) both miss the +existing-article lookup and both insert an article at the same url. The +loser used to hit the unique key on article.url_id and return a 500. +""" + +from unittest import mock + +from sqlalchemy.orm import Query + +from zeeguu.api.test.fixtures import logged_in_client +from zeeguu.core.model import Article +from zeeguu.core.model.article_fragment import ArticleFragment +from zeeguu.core.model.db import db + +UPLOAD_URL = "https://example.com/race/bau-von-trumps-triumphbogen" +TEXT = ( + "Der Bau des Triumphbogens hat begonnen. Er wird sechsundsiebzig Meter " + "hoch sein und steht in der Hauptstadt. Viele Leute sprechen darĂ¼ber." +) + + +def _llm_output(): + # Each call returns a different text, as the real LLM does: the loser's + # fragments would differ from the winner's and add a second set. + calls = [] + + def translate_and_adapt(self, **kwargs): + calls.append(kwargs) + n = len(calls) + return { + "title": f"Triumphbogen {n}", + "content": f"Erster Absatz, Version {n}.\n\nZweiter Absatz, Version {n}.", + "summary": f"Zusammenfassung {n}", + } + + return translate_and_adapt, calls + + +def _create_upload(client, url=UPLOAD_URL): + upload = client.post( + "/article_upload/create", + data=dict(url=url, text_content=TEXT, title="Triumphbogen"), + ) + return upload["id"] + + +def _translate(client, upload_id): + return client.response_from_post( + f"/article_upload/{upload_id}/translate_and_adapt" + ) + + +def _fragment_count(article_id): + return ArticleFragment.query.filter_by(article_id=article_id).count() + + +def _articles_from(upload_id): + return Article.query.filter_by(source_upload_id=upload_id).all() + + +def _cleanup(upload_id): + # article and article_fragment survive between API tests (conftest's + # _KEEP_TABLES), so remove what this test made. + for article in _articles_from(upload_id): + ArticleFragment.query.filter_by(article_id=article.id).delete() + db.session.delete(article) + db.session.commit() + + +def test_losing_the_insert_race_returns_the_winners_article(logged_in_client): + upload_id = _create_upload(logged_in_client) + translate_and_adapt, llm_calls = _llm_output() + + with mock.patch( + "zeeguu.core.llm_services.simplification_service." + "SimplificationService.translate_and_adapt", + translate_and_adapt, + ): + try: + # The winner: the first request creates the article. + first = _translate(logged_in_client, upload_id) + assert first.status_code == 200 + winner_id = first.get_json()["id"] + winner_fragments = _fragment_count(winner_id) + assert winner_fragments > 0 + + # The loser: its lookups miss the winner's article (on MySQL: its + # REPEATABLE READ snapshot predates the winner's commit), so it + # calls the LLM and inserts at the same url. + real_one_or_none = Query.one_or_none + article_lookups = [] + + def first_article_lookup_misses(query): + if query.column_descriptions[0]["entity"] is Article: + article_lookups.append(query) + if len(article_lookups) == 1: + return None + return real_one_or_none(query) + + with mock.patch.object(Article, "find", return_value=None), \ + mock.patch.object(Query, "one_or_none", first_article_lookup_misses): + second = _translate(logged_in_client, upload_id) + + assert len(llm_calls) == 2, "the loser did reach the insert" + assert second.status_code == 200 + assert second.get_json()["id"] == winner_id + assert second.get_json()["title"] == "Triumphbogen 1" + assert len(_articles_from(upload_id)) == 1 + assert _fragment_count(winner_id) == winner_fragments + finally: + _cleanup(upload_id) + + +def test_an_upload_whose_translated_url_is_too_long_gets_its_own_article( + logged_in_client, +): + # Url.find_or_create gives None for a path over 255 chars. These paths + # fit, but not with the "#translated-from-de-to-de-A2" suffix, so both + # translated articles get no url. Each must still get its own article, + # not the other one (an `url_id IS NULL` lookup would find it). + def long_url(n): + path = f"/race/{n}-" + "x" * 240 + assert len(path) <= 255 + return "https://example.com" + path + + upload_ids = [_create_upload(logged_in_client, long_url(n)) for n in (1, 2)] + translate_and_adapt, llm_calls = _llm_output() + + with mock.patch( + "zeeguu.core.llm_services.simplification_service." + "SimplificationService.translate_and_adapt", + translate_and_adapt, + ): + try: + responses = [_translate(logged_in_client, u) for u in upload_ids] + + assert [r.status_code for r in responses] == [200, 200] + first, second = [r.get_json() for r in responses] + assert first["id"] != second["id"] + assert second["title"] == "Triumphbogen 2" + for upload_id in upload_ids: + [article] = _articles_from(upload_id) + assert article.url is None + assert _fragment_count(article.id) > 0 + finally: + for upload_id in upload_ids: + _cleanup(upload_id) diff --git a/zeeguu/core/model/race_safe.py b/zeeguu/core/model/race_safe.py index 627d2b1b3..7159579a1 100644 --- a/zeeguu/core/model/race_safe.py +++ b/zeeguu/core/model/race_safe.py @@ -12,9 +12,20 @@ def find_or_create(session, cls, filters: dict, make, commit=False): Commits only when it created the row and `commit` is set; a found or recovered row is returned without committing, as the per-model copies did. """ + row, _ = find_or_create_and_report(session, cls, filters, make, commit) + return row + + +def find_or_create_and_report(session, cls, filters: dict, make, commit=False): + """ + As find_or_create, but returns `(row, created)`: `created` is True only + when this call inserted the row, and False when it found it or lost the + race to another request. For callers that must do follow-up work (e.g. + child rows) only for a row they created. + """ existing = session.query(cls).filter_by(**filters).one_or_none() if existing: - return existing + return existing, False # Two requests that both miss the query above both insert, and the unique # key lets exactly one of them through. @@ -33,15 +44,22 @@ def find_or_create(session, cls, filters: dict, make, commit=False): # A locking read (SELECT ... LOCK IN SHARE MODE). Under REPEATABLE # READ a plain SELECT would reuse this transaction's snapshot, taken # at its first read, before the winner committed: it would find - # nothing and .one() would raise NoResultFound. A locking read sees - # the latest committed row. (SQLite ignores it; tests don't race.) - return ( + # nothing. A locking read sees the latest committed row. (SQLite + # ignores it; tests don't race.) + winner = ( session.query(cls) .filter_by(**filters) .with_for_update(read=True) - .one() + .one_or_none() ) + if winner is None: + # No row matches `filters`, so the insert collided on some other + # unique key: there is no winner to return. Re-raise the + # IntegrityError, which names that key, rather than hide it + # behind a NoResultFound. + raise + return winner, False if commit: session.commit() - return new + return new, True diff --git a/zeeguu/core/test/test_race_safe.py b/zeeguu/core/test/test_race_safe.py index faf768c38..2dedb652f 100644 --- a/zeeguu/core/test/test_race_safe.py +++ b/zeeguu/core/test/test_race_safe.py @@ -1,10 +1,13 @@ from unittest import mock +import pytest + +from sqlalchemy.exc import IntegrityError from sqlalchemy.orm import Query from zeeguu.core.model.article_summary_context import ArticleSummaryContext from zeeguu.core.model.db import db -from zeeguu.core.model.race_safe import find_or_create +from zeeguu.core.model.race_safe import find_or_create, find_or_create_and_report from zeeguu.core.test.model_test_mixin import ModelTestMixIn from zeeguu.core.test.rules.article_rule import ArticleRule from zeeguu.core.test.rules.bookmark_rule import BookmarkRule @@ -94,3 +97,56 @@ def first_lookup_misses(query): assert self._count() == 1 session.refresh(self.article) assert self.article.title == "changed by the caller" + + def test_report_says_whether_this_call_created_the_row(self): + created_row, created = find_or_create_and_report( + session, ArticleSummaryContext, self.filters, self._make + ) + assert created + + found_row, created = find_or_create_and_report( + session, ArticleSummaryContext, self.filters, self._make + ) + assert found_row is created_row + assert not created + + def test_report_says_not_created_when_losing_the_race(self): + winner = self._make() + session.add(winner) + session.commit() + + real_one_or_none = Query.one_or_none + lookups = [] + + def first_lookup_misses(query): + lookups.append(query) + if len(lookups) == 1: + return None + return real_one_or_none(query) + + with mock.patch.object(Query, "one_or_none", first_lookup_misses): + row, created = find_or_create_and_report( + session, ArticleSummaryContext, self.filters, self._make + ) + + assert row.id == winner.id + assert not created + + def test_collision_on_another_unique_key_raises_the_integrity_error(self): + # The row make() builds collides on (bookmark, article), but the + # filters don't cover that key, so there is no winner to return. + existing = self._make() + session.add(existing) + session.commit() + + self.article.title = "changed by the caller" + + with pytest.raises(IntegrityError): + find_or_create( + session, ArticleSummaryContext, dict(id=existing.id + 1000), self._make + ) + session.commit() + + assert self._count() == 1 + session.refresh(self.article) + assert self.article.title == "changed by the caller"