translate_and_adapt: no 500 when two requests create the same article

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)

zeeguu/api/test/test_article_upload_translate_race.py

@@ -0,0 +1,149 @@
+"""
+Two requests to /article_upload/<id>/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)

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

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"