diff --git a/zeeguu/core/model/article_cefr_assessment.py b/zeeguu/core/model/article_cefr_assessment.py index c5a15af34..8d3018d30 100644 --- a/zeeguu/core/model/article_cefr_assessment.py +++ b/zeeguu/core/model/article_cefr_assessment.py @@ -11,10 +11,10 @@ from datetime import datetime from sqlalchemy import Column, Integer, String, ForeignKey, DateTime, Enum -from sqlalchemy.exc import IntegrityError from sqlalchemy.orm import relationship from zeeguu.core.model.db import db +from zeeguu.core.model.race_safe import find_or_create class ArticleCefrAssessment(db.Model): @@ -79,32 +79,16 @@ def find_or_create(cls, session, article_id, commit=True): Returns: ArticleCefrAssessment instance """ - assessment = session.query(cls).filter_by(article_id=article_id).first() - if not assessment: - # Two requests assessing the same article (e.g. a double tap on - # promote_to_article) can both miss the query above: it reads the - # caller's REPEATABLE READ snapshot, taken before the slow LLM call, - # so it can't see the row the other request committed meanwhile. - # The article_id primary key then lets only one insert through. - # The savepoint undoes only the losing insert, not the caller's - # pending work (see UserLanguage.find_or_create). - try: - with session.begin_nested(): - assessment = cls(article_id=article_id) - session.add(assessment) - except IntegrityError: - # A locking read: a plain SELECT would reuse this - # transaction's REPEATABLE READ snapshot from the query - # above, which predates the winner's row. - assessment = ( - session.query(cls) - .filter_by(article_id=article_id) - .with_for_update(read=True) - .one() - ) - if commit: - session.commit() - return assessment + # Two requests assessing the same article (e.g. a double tap on + # promote_to_article) can race here; the article_id primary key lets + # one insert through and race_safe.find_or_create recovers the other. + return find_or_create( + session, + cls, + {"article_id": article_id}, + lambda: cls(article_id=article_id), + commit=commit, + ) def set_llm_assessment(self, level, method): """ diff --git a/zeeguu/core/model/article_fragment_context.py b/zeeguu/core/model/article_fragment_context.py index d23784ad9..3a58b5151 100644 --- a/zeeguu/core/model/article_fragment_context.py +++ b/zeeguu/core/model/article_fragment_context.py @@ -1,5 +1,6 @@ from zeeguu.core.model.db import db import sqlalchemy +from zeeguu.core.model.race_safe import find_or_create class ArticleFragmentContext(db.Model): @@ -50,34 +51,13 @@ def find_by_bookmark(cls, bookmark): @classmethod def find_or_create(cls, session, bookmark, article_fragment, commit=True): - existing = cls.query.filter( - cls.bookmark == bookmark, - cls.article_fragment == article_fragment, - ).one_or_none() - if existing: - return existing - - # Insert inside a SAVEPOINT so that if a concurrent request created the - # same (bookmark, article fragment) between our SELECT and INSERT, the - # unique constraint fires and we roll back just this insert (not the - # caller's whole transaction, which may still be uncommitted when - # commit=False) and return the row the other request created. - new = cls( - bookmark, - article_fragment, + return find_or_create( + session, + cls, + dict(bookmark=bookmark, article_fragment=article_fragment), + lambda: cls(bookmark, article_fragment), + commit=commit, ) - try: - with session.begin_nested(): - session.add(new) - except sqlalchemy.exc.IntegrityError: - return cls.query.filter( - cls.bookmark == bookmark, - cls.article_fragment == article_fragment, - ).one() - - if commit: - session.commit() - return new @classmethod def get_all_user_bookmarks_for_article_fragment( diff --git a/zeeguu/core/model/article_public_code.py b/zeeguu/core/model/article_public_code.py index 8d78e016b..7b5957bf0 100644 --- a/zeeguu/core/model/article_public_code.py +++ b/zeeguu/core/model/article_public_code.py @@ -5,6 +5,7 @@ from sqlalchemy.orm import relationship from zeeguu.core.model.db import db +from zeeguu.core.model.race_safe import find_or_create class ArticlePublicCode(db.Model): @@ -39,18 +40,17 @@ class ArticlePublicCode(db.Model): @classmethod def for_article(cls, session, article_id): - existing = cls.query.filter_by(article_id=article_id).first() - if existing: - return existing - row = cls(article_id=article_id, code="".join(secrets.choice(cls.ALPHABET) for _ in range(cls.CODE_LENGTH))) # Two readers opening a never-linked article at once race the primary - # key; the savepoint keeps the outer transaction usable, winner's code wins. - try: - with session.begin_nested(): - session.add(row) - return row - except Exception: - return cls.query.filter_by(article_id=article_id).one() + # key; the winner's code wins. + return find_or_create( + session, + cls, + dict(article_id=article_id), + lambda: cls( + article_id=article_id, + code="".join(secrets.choice(cls.ALPHABET) for _ in range(cls.CODE_LENGTH)), + ), + ) @classmethod def find_article(cls, code): diff --git a/zeeguu/core/model/article_summary_context.py b/zeeguu/core/model/article_summary_context.py index 844a02190..cd34c63b2 100644 --- a/zeeguu/core/model/article_summary_context.py +++ b/zeeguu/core/model/article_summary_context.py @@ -2,6 +2,7 @@ from zeeguu.core.model.bookmark import Bookmark from zeeguu.core.model.db import db import sqlalchemy +from zeeguu.core.model.race_safe import find_or_create class ArticleSummaryContext(db.Model): @@ -54,34 +55,13 @@ def find_or_create( article, commit=True, ): - existing = cls.query.filter( - cls.bookmark == bookmark, - cls.article == article, - ).one_or_none() - if existing: - return existing - - # Insert inside a SAVEPOINT so that if a concurrent request created the - # same (bookmark, article) between our SELECT and INSERT, the unique - # constraint fires and we roll back just this insert (not the caller's - # whole transaction, which may still be uncommitted when commit=False) and - # return the row the other request created. - new = cls( - bookmark, - article, + return find_or_create( + session, + cls, + dict(bookmark=bookmark, article=article), + lambda: cls(bookmark, article), + commit=commit, ) - try: - with session.begin_nested(): - session.add(new) - except sqlalchemy.exc.IntegrityError: - return cls.query.filter( - cls.bookmark == bookmark, - cls.article == article, - ).one() - - if commit: - session.commit() - return new @classmethod def get_all_user_bookmarks_for_article_summary( diff --git a/zeeguu/core/model/article_title_context.py b/zeeguu/core/model/article_title_context.py index 65f580627..c7ae9b220 100644 --- a/zeeguu/core/model/article_title_context.py +++ b/zeeguu/core/model/article_title_context.py @@ -2,6 +2,7 @@ from zeeguu.core.model.bookmark import Bookmark from zeeguu.core.model.db import db import sqlalchemy +from zeeguu.core.model.race_safe import find_or_create class ArticleTitleContext(db.Model): @@ -54,34 +55,13 @@ def find_or_create( article, commit=True, ): - existing = cls.query.filter( - cls.bookmark == bookmark, - cls.article == article, - ).one_or_none() - if existing: - return existing - - # Insert inside a SAVEPOINT so that if a concurrent request created the - # same (bookmark, article) between our SELECT and INSERT, the unique - # constraint fires and we roll back just this insert (not the caller's - # whole transaction, which may still be uncommitted when commit=False) and - # return the row the other request created. - new = cls( - bookmark, - article, + return find_or_create( + session, + cls, + dict(bookmark=bookmark, article=article), + lambda: cls(bookmark, article), + commit=commit, ) - try: - with session.begin_nested(): - session.add(new) - except sqlalchemy.exc.IntegrityError: - return cls.query.filter( - cls.bookmark == bookmark, - cls.article == article, - ).one() - - if commit: - session.commit() - return new @classmethod def get_all_user_bookmarks_for_article_title( diff --git a/zeeguu/core/model/example_sentence_context.py b/zeeguu/core/model/example_sentence_context.py index 2c45e18bb..3aee29185 100644 --- a/zeeguu/core/model/example_sentence_context.py +++ b/zeeguu/core/model/example_sentence_context.py @@ -2,6 +2,7 @@ from zeeguu.core.model.db import db from zeeguu.core.model.example_sentence import ExampleSentence import sqlalchemy +from zeeguu.core.model.race_safe import find_or_create class ExampleSentenceContext(db.Model): @@ -56,31 +57,13 @@ def find_or_create( example_sentence, commit=True, ): - existing = cls.query.filter( - cls.bookmark == bookmark, - cls.example_sentence == example_sentence, - ).one_or_none() - if existing: - return existing - - # Insert inside a SAVEPOINT so that if a concurrent request created the - # same (bookmark, example sentence) between our SELECT and INSERT, the - # unique constraint fires and we roll back just this insert (not the - # caller's whole transaction, which may still be uncommitted when - # commit=False) and return the row the other request created. - new = cls(bookmark, example_sentence) - try: - with session.begin_nested(): - session.add(new) - except sqlalchemy.exc.IntegrityError: - return cls.query.filter( - cls.bookmark == bookmark, - cls.example_sentence == example_sentence, - ).one() - - if commit: - session.commit() - return new + return find_or_create( + session, + cls, + dict(bookmark=bookmark, example_sentence=example_sentence), + lambda: cls(bookmark, example_sentence), + commit=commit, + ) @classmethod def get_all_user_bookmarks_for_example( diff --git a/zeeguu/core/model/level_adapted_article_summary_context.py b/zeeguu/core/model/level_adapted_article_summary_context.py index 8053d5e7a..346995d7c 100644 --- a/zeeguu/core/model/level_adapted_article_summary_context.py +++ b/zeeguu/core/model/level_adapted_article_summary_context.py @@ -1,5 +1,6 @@ from zeeguu.core.model.db import db import sqlalchemy +from zeeguu.core.model.race_safe import find_or_create class LevelAdaptedArticleSummaryContext(db.Model): @@ -56,31 +57,13 @@ def find_by_bookmark(cls, bookmark): @classmethod def find_or_create(cls, session, bookmark, level_adapted_article_text, commit=True): - existing = cls.query.filter( - cls.bookmark == bookmark, - cls.level_adapted_article_text == level_adapted_article_text, - ).one_or_none() - if existing: - return existing - - # Insert inside a SAVEPOINT so that if a concurrent request created the - # same (bookmark, level summary) between our SELECT and INSERT, the unique - # constraint fires and we roll back just this insert (not the caller's - # whole transaction, which may still be uncommitted when commit=False) and - # return the row the other request created. - new = cls(bookmark, level_adapted_article_text) - try: - with session.begin_nested(): - session.add(new) - except sqlalchemy.exc.IntegrityError: - return cls.query.filter( - cls.bookmark == bookmark, - cls.level_adapted_article_text == level_adapted_article_text, - ).one() - - if commit: - session.commit() - return new + return find_or_create( + session, + cls, + dict(bookmark=bookmark, level_adapted_article_text=level_adapted_article_text), + lambda: cls(bookmark, level_adapted_article_text), + commit=commit, + ) @classmethod def get_all_user_bookmarks_for_level_adapted_summary( diff --git a/zeeguu/core/model/level_adapted_article_title_context.py b/zeeguu/core/model/level_adapted_article_title_context.py index d0415887f..eba5768ea 100644 --- a/zeeguu/core/model/level_adapted_article_title_context.py +++ b/zeeguu/core/model/level_adapted_article_title_context.py @@ -1,5 +1,6 @@ from zeeguu.core.model.db import db import sqlalchemy +from zeeguu.core.model.race_safe import find_or_create class LevelAdaptedArticleTitleContext(db.Model): @@ -58,31 +59,13 @@ def find_by_bookmark(cls, bookmark): @classmethod def find_or_create(cls, session, bookmark, level_adapted_article_text, commit=True): - existing = cls.query.filter( - cls.bookmark == bookmark, - cls.level_adapted_article_text == level_adapted_article_text, - ).one_or_none() - if existing: - return existing - - # Insert inside a SAVEPOINT so that if a concurrent request created the - # same (bookmark, level title) between our SELECT and INSERT, the unique - # constraint fires and we roll back just this insert (not the caller's - # whole transaction, which may still be uncommitted when commit=False) and - # return the row the other request created. - new = cls(bookmark, level_adapted_article_text) - try: - with session.begin_nested(): - session.add(new) - except sqlalchemy.exc.IntegrityError: - return cls.query.filter( - cls.bookmark == bookmark, - cls.level_adapted_article_text == level_adapted_article_text, - ).one() - - if commit: - session.commit() - return new + return find_or_create( + session, + cls, + dict(bookmark=bookmark, level_adapted_article_text=level_adapted_article_text), + lambda: cls(bookmark, level_adapted_article_text), + commit=commit, + ) @classmethod def get_all_user_bookmarks_for_level_adapted_title( diff --git a/zeeguu/core/model/race_safe.py b/zeeguu/core/model/race_safe.py new file mode 100644 index 000000000..627d2b1b3 --- /dev/null +++ b/zeeguu/core/model/race_safe.py @@ -0,0 +1,47 @@ +from sqlalchemy.exc import IntegrityError + + +def find_or_create(session, cls, filters: dict, make, commit=False): + """ + The row of `cls` matching `filters` (as for filter_by; relationship + objects work too), created with `make()` if there is none. + + Safe when two requests create the same row at once, as long as a unique + key covers `filters`: the loser gets the winner's row back. + + 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. + """ + existing = session.query(cls).filter_by(**filters).one_or_none() + if existing: + return existing + + # Two requests that both miss the query above both insert, and the unique + # key lets exactly one of them through. + # + # The savepoint is what makes losing survivable. A failed flush leaves the + # whole session needing a rollback, so recovering without one raises + # PendingRollbackError, and recovering with a full rollback throws away + # whatever the caller had pending. Rolling back to a savepoint undoes this + # insert and nothing else. `make()` runs inside the savepoint, so the new + # object can't be flushed by begin_nested()'s own flush, outside it. + try: + with session.begin_nested(): + new = make() + session.add(new) + except IntegrityError: + # 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 ( + session.query(cls) + .filter_by(**filters) + .with_for_update(read=True) + .one() + ) + + if commit: + session.commit() + return new diff --git a/zeeguu/core/model/search_filter.py b/zeeguu/core/model/search_filter.py index 2826c33fc..601d099e3 100644 --- a/zeeguu/core/model/search_filter.py +++ b/zeeguu/core/model/search_filter.py @@ -2,11 +2,10 @@ from sqlalchemy.orm import relationship from zeeguu.core.model.user import User -import sqlalchemy - import zeeguu.core from zeeguu.core.model.db import db +from zeeguu.core.model.race_safe import find_or_create from zeeguu.core.model.search import Search @@ -52,28 +51,13 @@ def __str__(self): @classmethod def find_or_create(cls, session, user, search): - try: - return cls.query.filter(cls.user == user).filter(cls.search == search).one() - except sqlalchemy.orm.exc.NoResultFound: - # Two requests that both miss the query above both insert, and the - # unique key lets exactly one of them through. The savepoint is what - # makes losing survivable: a failed flush leaves the whole session - # needing a rollback, so recovering without one raises - # PendingRollbackError and recovering with a full rollback discards - # whatever the caller had pending. Rolling back to a savepoint undoes - # this insert and nothing else. - try: - with session.begin_nested(): - new = cls(user, search) - session.add(new) - except sqlalchemy.exc.IntegrityError: - return ( - cls.query.filter(cls.user == user) - .filter(cls.search == search) - .one() - ) - session.commit() - return new + return find_or_create( + session, + cls, + dict(user=user, search=search), + lambda: cls(user, search), + commit=True, + ) @classmethod def all_for_user(cls, user): diff --git a/zeeguu/core/model/search_subscription.py b/zeeguu/core/model/search_subscription.py index 90b05e1b5..6ca5af200 100644 --- a/zeeguu/core/model/search_subscription.py +++ b/zeeguu/core/model/search_subscription.py @@ -1,10 +1,10 @@ from sqlalchemy import UniqueConstraint from sqlalchemy.orm import relationship from zeeguu.core.model.user import User -import sqlalchemy import zeeguu.core from zeeguu.core.model.db import db +from zeeguu.core.model.race_safe import find_or_create from zeeguu.core.model.search import Search @@ -61,28 +61,13 @@ def as_dictionary(self): @classmethod def find_or_create(cls, session, user, search, receive_email): - try: - return cls.query.filter(cls.user == user).filter(cls.search == search).one() - except sqlalchemy.orm.exc.NoResultFound: - # Two requests that both miss the query above both insert, and the - # unique key lets exactly one of them through. The savepoint is what - # makes losing survivable: a failed flush leaves the whole session - # needing a rollback, so recovering without one raises - # PendingRollbackError and recovering with a full rollback discards - # whatever the caller had pending. Rolling back to a savepoint undoes - # this insert and nothing else. - try: - with session.begin_nested(): - new = cls(user, search, receive_email) - session.add(new) - except sqlalchemy.exc.IntegrityError: - return ( - cls.query.filter(cls.user == user) - .filter(cls.search == search) - .one() - ) - session.commit() - return new + return find_or_create( + session, + cls, + dict(user=user, search=search), + lambda: cls(user, search, receive_email), + commit=True, + ) @classmethod def all_for_user(cls, user): diff --git a/zeeguu/core/model/topic_filter.py b/zeeguu/core/model/topic_filter.py index 0593e88af..f29d5e249 100644 --- a/zeeguu/core/model/topic_filter.py +++ b/zeeguu/core/model/topic_filter.py @@ -3,11 +3,11 @@ from zeeguu.core.model.topic import Topic from zeeguu.core.model.user import User -import sqlalchemy import zeeguu.core from zeeguu.core.model.db import db +from zeeguu.core.model.race_safe import find_or_create class TopicFilter(db.Model): @@ -48,28 +48,13 @@ def __str__(self): @classmethod def find_or_create(cls, session, user, topic): - try: - return cls.query.filter(cls.user == user).filter(cls.topic == topic).one() - except sqlalchemy.orm.exc.NoResultFound: - # Two requests that both miss the query above both insert, and the - # unique key lets exactly one of them through. The savepoint is what - # makes losing survivable: a failed flush leaves the whole session - # needing a rollback, so recovering without one raises - # PendingRollbackError and recovering with a full rollback discards - # whatever the caller had pending. Rolling back to a savepoint undoes - # this insert and nothing else. - try: - with session.begin_nested(): - new = cls(user, topic) - session.add(new) - except sqlalchemy.exc.IntegrityError: - return ( - cls.query.filter(cls.user == user) - .filter(cls.topic == topic) - .one() - ) - session.commit() - return new + return find_or_create( + session, + cls, + dict(user=user, topic=topic), + lambda: cls(user, topic), + commit=True, + ) @classmethod def all_for_user(cls, user): diff --git a/zeeguu/core/model/topic_subscription.py b/zeeguu/core/model/topic_subscription.py index 6afed331e..7c8fb5964 100644 --- a/zeeguu/core/model/topic_subscription.py +++ b/zeeguu/core/model/topic_subscription.py @@ -3,11 +3,11 @@ from zeeguu.core.model.topic import Topic from zeeguu.core.model.user import User -import sqlalchemy import zeeguu.core from zeeguu.core.model.db import db +from zeeguu.core.model.race_safe import find_or_create class TopicSubscription(db.Model): @@ -50,27 +50,12 @@ def __str__(self): @classmethod def find_or_create(cls, session, user, topic): - try: - return cls.query.filter(cls.user == user).filter(cls.topic == topic).one() - except sqlalchemy.orm.exc.NoResultFound: - # Two requests that both miss the query above both insert, and the - # unique key lets exactly one of them through. The savepoint is what - # makes losing survivable: a failed flush leaves the whole session - # needing a rollback, so recovering without one raises - # PendingRollbackError and recovering with a full rollback discards - # whatever the caller had pending. Rolling back to a savepoint undoes - # this insert and nothing else. - try: - with session.begin_nested(): - new = cls(user, topic) - session.add(new) - except sqlalchemy.exc.IntegrityError: - return ( - cls.query.filter(cls.user == user) - .filter(cls.topic == topic) - .one() - ) - return new + return find_or_create( + session, + cls, + dict(user=user, topic=topic), + lambda: cls(user, topic), + ) @classmethod def all_for_user(cls, user): diff --git a/zeeguu/core/model/user_language.py b/zeeguu/core/model/user_language.py index 20adbb5c2..bf1d7005e 100644 --- a/zeeguu/core/model/user_language.py +++ b/zeeguu/core/model/user_language.py @@ -1,6 +1,5 @@ import datetime -import sqlalchemy from sqlalchemy import Column, Integer, String, ForeignKey, Boolean, DateTime, UniqueConstraint from sqlalchemy.orm import relationship @@ -10,6 +9,7 @@ import zeeguu.core from zeeguu.core.model.db import db +from zeeguu.core.model.race_safe import find_or_create class UserLanguage(db.Model): @@ -177,33 +177,12 @@ def dialect_for(cls, user, language): @classmethod def find_or_create(cls, session, user, language): - try: - return ( - cls.query.filter(cls.user == user) - .filter(cls.language == language) - .one() - ) - except sqlalchemy.orm.exc.NoResultFound: - # Two requests that both miss the query above both insert, and the - # UNIQUE (user_id, language_id) lets exactly one of them through. - # - # The savepoint is what makes losing survivable. A failed flush leaves - # the whole session needing a rollback before it can be used again -- - # so recovering without one raises PendingRollbackError, and - # recovering with one throws away whatever the caller had pending, - # answering 200 over changes that were never written. Rolling back to - # a savepoint undoes this insert and nothing else. - try: - with session.begin_nested(): - new = cls(user, language) - session.add(new) - return new - except sqlalchemy.exc.IntegrityError: - return ( - cls.query.filter(cls.user == user) - .filter(cls.language == language) - .one() - ) + return find_or_create( + session, + cls, + dict(user=user, language=language), + lambda: cls(user, language), + ) @classmethod def with_language_id(cls, i, user): diff --git a/zeeguu/core/model/user_word.py b/zeeguu/core/model/user_word.py index f2bc3a59b..afdb3aa7d 100644 --- a/zeeguu/core/model/user_word.py +++ b/zeeguu/core/model/user_word.py @@ -1,7 +1,7 @@ from datetime import datetime import sqlalchemy -from sqlalchemy.exc import IntegrityError, NoResultFound +from sqlalchemy.exc import NoResultFound from wordstats import Word @@ -11,6 +11,7 @@ from zeeguu.logging import log from zeeguu.core.model.db import db +from zeeguu.core.model.race_safe import find_or_create class UserWord(db.Model): @@ -455,28 +456,15 @@ def find_or_create(cls, session, user, meaning, is_user_added=False): Since Meaning.find_or_create() already handles semantic deduplication, we can trust that each meaning is unique and canonical. """ - try: - user_word = cls.query.filter_by(user=user, meaning=meaning).one() - except NoResultFound: - # A double tap sends two translate_word requests that both miss - # the query above, and unique_user_word lets one insert through. - # The savepoint undoes only the losing insert, not the caller's - # pending work (see UserLanguage.find_or_create). - try: - with session.begin_nested(): - user_word = cls(user, meaning, is_user_added=is_user_added) - session.add(user_word) - session.commit() - return user_word - except IntegrityError: - # A locking read: a plain SELECT would reuse this - # transaction's REPEATABLE READ snapshot from the query - # above, which predates the winner's row. - user_word = ( - cls.query.filter_by(user=user, meaning=meaning) - .with_for_update(read=True) - .one() - ) + # A double tap sends two translate_word requests for the same word; + # unique_user_word lets one insert through and the other gets its row. + user_word = find_or_create( + session, + cls, + dict(user=user, meaning=meaning), + lambda: cls(user, meaning, is_user_added=is_user_added), + commit=True, + ) # If it exists but we're marking it as user_added, update the flag if is_user_added and not user_word.is_user_added: diff --git a/zeeguu/core/model/video_caption_context.py b/zeeguu/core/model/video_caption_context.py index 04a08f0d2..26135318a 100644 --- a/zeeguu/core/model/video_caption_context.py +++ b/zeeguu/core/model/video_caption_context.py @@ -1,6 +1,7 @@ from zeeguu.core.model.db import db import sqlalchemy from zeeguu.core.model.bookmark import Bookmark +from zeeguu.core.model.race_safe import find_or_create class VideoCaptionContext(db.Model): @@ -56,34 +57,13 @@ def find_or_create( caption, commit=True, ): - existing = cls.query.filter( - cls.bookmark == bookmark, - cls.caption == caption, - ).one_or_none() - if existing: - return existing - - # Insert inside a SAVEPOINT so that if a concurrent request created the - # same (bookmark, caption) between our SELECT and INSERT, the unique - # constraint fires and we roll back just this insert (not the caller's - # whole transaction, which may still be uncommitted when commit=False) and - # return the row the other request created. - new = cls( - bookmark, - caption, + return find_or_create( + session, + cls, + dict(bookmark=bookmark, caption=caption), + lambda: cls(bookmark, caption), + commit=commit, ) - try: - with session.begin_nested(): - session.add(new) - except sqlalchemy.exc.IntegrityError: - return cls.query.filter( - cls.bookmark == bookmark, - cls.caption == caption, - ).one() - - if commit: - session.commit() - return new @classmethod def get_all_user_bookmarks_for_caption( diff --git a/zeeguu/core/model/video_title_context.py b/zeeguu/core/model/video_title_context.py index b76587651..f0cf3e24e 100644 --- a/zeeguu/core/model/video_title_context.py +++ b/zeeguu/core/model/video_title_context.py @@ -3,6 +3,7 @@ import sqlalchemy from zeeguu.core.model.bookmark import Bookmark +from zeeguu.core.model.race_safe import find_or_create class VideoTitleContext(db.Model): @@ -55,31 +56,13 @@ def find_or_create( video, commit=True, ): - existing = cls.query.filter( - cls.bookmark == bookmark, - cls.video == video, - ).one_or_none() - if existing: - return existing - - # Insert inside a SAVEPOINT so that if a concurrent request created the - # same (bookmark, video) between our SELECT and INSERT, the unique - # constraint fires and we roll back just this insert (not the caller's - # whole transaction, which may still be uncommitted when commit=False) and - # return the row the other request created. - new = cls(bookmark, video) - try: - with session.begin_nested(): - session.add(new) - except sqlalchemy.exc.IntegrityError: - return cls.query.filter( - cls.bookmark == bookmark, - cls.video == video, - ).one() - - if commit: - session.commit() - return new + return find_or_create( + session, + cls, + dict(bookmark=bookmark, video=video), + lambda: cls(bookmark, video), + commit=commit, + ) @classmethod def get_all_user_bookmarks_for_video_title( diff --git a/zeeguu/core/test/test_race_safe.py b/zeeguu/core/test/test_race_safe.py new file mode 100644 index 000000000..faf768c38 --- /dev/null +++ b/zeeguu/core/test/test_race_safe.py @@ -0,0 +1,96 @@ +from unittest import mock + +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.test.model_test_mixin import ModelTestMixIn +from zeeguu.core.test.rules.article_rule import ArticleRule +from zeeguu.core.test.rules.bookmark_rule import BookmarkRule +from zeeguu.core.test.rules.user_rule import UserRule + +session = db.session + + +class RaceSafeFindOrCreateTest(ModelTestMixIn): + def setUp(self): + super().setUp() + self.article = ArticleRule().article + self.bookmark = BookmarkRule(UserRule().user).bookmark + self.filters = dict(bookmark=self.bookmark, article=self.article) + session.commit() + + def _make(self): + return ArticleSummaryContext(self.bookmark, self.article) + + def _count(self): + return ArticleSummaryContext.query.filter_by( + bookmark_id=self.bookmark.id, article_id=self.article.id + ).count() + + def test_creates_the_row_when_missing(self): + row = find_or_create(session, ArticleSummaryContext, self.filters, self._make) + + assert row.id is not None + assert self._count() == 1 + + def test_returns_the_existing_row_without_making_one(self): + first = find_or_create(session, ArticleSummaryContext, self.filters, self._make) + make = mock.Mock() + + again = find_or_create(session, ArticleSummaryContext, self.filters, make) + + assert again is first + make.assert_not_called() + assert self._count() == 1 + + # These spy on commit rather than roll back and look: under pysqlite the + # savepoint's RELEASE can itself commit, which MySQL doesn't do. + def test_does_not_commit_by_default(self): + with mock.patch.object(session, "commit") as commit: + find_or_create(session, ArticleSummaryContext, self.filters, self._make) + commit.assert_not_called() + + def test_commit_flag_commits_only_a_created_row(self): + with mock.patch.object(session, "commit") as commit: + find_or_create( + session, ArticleSummaryContext, self.filters, self._make, commit=True + ) + commit.assert_called_once() + + find_or_create( + session, ArticleSummaryContext, self.filters, self._make, commit=True + ) + commit.assert_called_once() + + def test_losing_the_insert_race_returns_the_winners_row(self): + # The other request's row is in the table, but this request's lookup + # misses it (on MySQL: it reads an older REPEATABLE READ snapshot). + winner = self._make() + session.add(winner) + session.commit() + winner_id = winner.id + + # Pending work of the caller that losing the race must not throw away. + self.article.title = "changed by the caller" + + 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): + loser = find_or_create( + session, ArticleSummaryContext, self.filters, self._make + ) + session.commit() + + assert loser.id == winner_id + assert self._count() == 1 + session.refresh(self.article) + assert self.article.title == "changed by the caller"