From 6aff4e917e5f14ab5e465251de369bc0b50467c4 Mon Sep 17 00:00:00 2001
From: Martin Mares <mj@ucw.cz>
Date: Tue, 10 Jan 2023 17:57:25 +0100
Subject: [PATCH] =?UTF-8?q?Bezpe=C4=8Dn=C3=A9=20transakce=20p=C5=99i=20zak?=
=?UTF-8?q?l=C3=A1d=C3=A1n=C3=AD=20u=C5=BEivatel=C5=AF/=C3=BA=C4=8Dastn?=
=?UTF-8?q?=C3=ADk=C5=AF/=C3=BA=C4=8Dast=C3=AD?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Closes #314:
Pokud organizátor odešle POST na přidání nového účastníka soutěže
dvakrát rychle po sobě (třeba kvůli nějakému automatickému retry
po rozpadu spojení), dvě různé DB transakce se snaží založit uživatele
se stejným loginem. Jedna z nich selže na unikátnosti sloupce email.
V defaultní úrovni izolace transakcí (READ COMMITTED) to nemá žádné
hezké řešení. Nepomůže SELECT ... FOR UPDATE, jelikož ten zamyká pouze
nalezené řádky, nikoliv neexistenci dalších řádků vyhovujících podmínce.
Co by se dalo dělat:
(1) Zvýšit úroveň izolace aspoň na READ REPEATABLE. To vyřeší problém,
ale současně může začít víceméně jakákoliv zapisující transakce
failovat. Vyžadovalo by dopsat retry do prakticky všech míst v OSMO,
kde je nějaký commit.
(2) Retryovat specificky transakce na zakládání užívatelů (a účastí apod.).
Tohle nejde snadno, jelikoz jsou i součástí dlouho běžících transakcí
v importech (zatím jsme se snažíli, aby byl celý import atomický
a v případě selhání se celý rollbackoval). To by možná mohly vyřešit
subtransakce.
(3) Zamykat celou tabulku s uživateli, než na ní provedeme první SELECT.
To by asi vyřešilo problém, ale byl by potřeba zápisový zámek, takže
by paralelně nemohla běžet žádná čtení. A také by se to potenciálně
mohlo deadlockovat (potřebujeme v jedné transakci postupně lock na
uživatele, účastníky a účasti a locky platí až do konce transakce).
(4) Používat INSERT ... ON CONFLICT <něco>. To vypadá bezpečně, jen to není
moc pohodlné, zejména proto, že s tím nepočítá ORM, takže je potřeba
dělat všechno ručně.
Zatím jsem zvolil (4), protože mi přijde, že to změny udržuje lokální
a funguje i s dlouhými transakcemi při importu. Výhledově bych se ale
chtěl zamyslet nad tím, jak takové věci řešit co nejuniverzálněji
a nejpohodlněji.
---
mo/users.py | 150 ++++++++++++++++++++++++++++++++++++----------------
1 file changed, 103 insertions(+), 47 deletions(-)
diff --git a/mo/users.py b/mo/users.py
index 96bc328b..b57c6e05 100644
--- a/mo/users.py
+++ b/mo/users.py
@@ -7,6 +7,7 @@ import email.errors
import email.headerregistry
import re
import secrets
+from sqlalchemy.dialects.postgresql import insert as pgsql_insert
from typing import Optional, Tuple
import mo
@@ -96,40 +97,57 @@ def change_user_to_org(user, reason: str):
def find_or_create_user(email: str, krestni: Optional[str], prijmeni: Optional[str], is_org: bool, reason: str, allow_change_user_to_org=False) -> Tuple[db.User, bool, bool]:
sess = db.get_session()
- user = sess.query(db.User).with_for_update().filter_by(email=email).one_or_none()
- is_new = user is None
+ user = sess.query(db.User).filter_by(email=email).one_or_none()
+ is_new = False
is_change_user_to_org = False
+
if user is None: # HACK: Podmínku je nutné zapsat znovu místo užití is_new, jinak si s tím mypy neporadí
if not krestni or not prijmeni:
raise mo.CheckError('Osoba s daným e-mailem zatím neexistuje, je nutné uvést její jméno.')
- user = db.User(email=email, first_name=krestni, last_name=prijmeni, is_org=is_org)
- sess.add(user)
- sess.flush() # Aby uživatel dostal user_id
- logger.info(f'{reason.title()}: Založen uživatel user=#{user.user_id} email=<{user.email}>')
- mo.util.log(
- type=db.LogType.user,
- what=user.user_id,
- details={'action': 'create-user', 'reason': reason, 'new': db.row2dict(user)},
+
+ res = sess.connection().execute(
+ pgsql_insert(db.User.__table__)
+ .values(
+ email=email,
+ first_name=krestni,
+ last_name=prijmeni,
+ is_org=is_org,
+ )
+ .on_conflict_do_nothing()
+ .returning(db.User.user_id)
)
- else:
- if (krestni and user.first_name != krestni) or (prijmeni and user.last_name != prijmeni):
- raise mo.CheckError(f'Osoba již registrována s odlišným jménem {user.full_name()}')
- if (user.is_admin or user.is_org) != is_org:
- if is_org:
- if allow_change_user_to_org:
- change_user_to_org(user, reason)
- is_change_user_to_org = True
- else:
- raise CheckErrorOrgIsUser('Nelze předefinovat účastníka na organizátora.')
+
+ user = sess.query(db.User).filter_by(email=email).one()
+
+ if res.fetchall():
+ is_new = True
+ logger.info(f'{reason.title()}: Založen uživatel user=#{user.user_id} email=<{user.email}>')
+ mo.util.log(
+ type=db.LogType.user,
+ what=user.user_id,
+ details={'action': 'create-user', 'reason': reason, 'new': db.row2dict(user)},
+ )
+
+ if (krestni and user.first_name != krestni) or (prijmeni and user.last_name != prijmeni):
+ raise mo.CheckError(f'Osoba již registrována s odlišným jménem {user.full_name()}')
+ if (user.is_admin or user.is_org) != is_org:
+ if is_org:
+ if allow_change_user_to_org:
+ change_user_to_org(user, reason)
+ is_change_user_to_org = True
else:
- raise mo.CheckError('Nelze předefinovat organizátora na účastníka.')
+ raise CheckErrorOrgIsUser('Nelze předefinovat účastníka na organizátora.')
+ else:
+ raise mo.CheckError('Nelze předefinovat organizátora na účastníka.')
+
return user, is_new, is_change_user_to_org
def find_or_create_participant(user: db.User, year: int, school_id: Optional[int], birth_year: Optional[int], grade: Optional[str], reason: str) -> Tuple[db.Participant, bool]:
sess = db.get_session()
part = sess.query(db.Participant).get((user.user_id, year))
- is_new = part is None
+ is_new = False
+
if part is None:
prev_part = sess.query(db.Participant).filter_by(user_id=user.user_id).order_by(db.Participant.year.desc()).limit(1).one_or_none()
if not school_id:
@@ -144,19 +162,37 @@ def find_or_create_participant(user: db.User, year: int, school_id: Optional[int
raise mo.CheckError('Osoba s daným e-mailem zatím není zaregistrovaná do ročníku, je nutné uvést rok narození.')
if not grade:
raise mo.CheckError('Osoba s daným e-mailem zatím není zaregistrovaná do ročníku, je nutné uvést ročník.')
- part = db.Participant(user=user, year=year, school=school_id, birth_year=birth_year, grade=grade)
- sess.add(part)
- logger.info(f'{reason.title()}: Založen účastník #{user.user_id}')
- mo.util.log(
- type=db.LogType.participant,
- what=user.user_id,
- details={'action': 'create-participant', 'reason': reason, 'new': db.row2dict(part)},
+
+ res = sess.connection().execute(
+ pgsql_insert(db.Participant.__table__)
+ .values(
+ user_id=user.user_id,
+ year=year,
+ school=school_id,
+ birth_year=birth_year,
+ grade=grade,
+ )
+ .on_conflict_do_nothing()
+ .returning(db.Participant.user_id)
)
- else:
- if ((school_id and part.school != school_id)
- or (grade and part.grade != grade)
- or (birth_year and part.birth_year != birth_year)):
- raise mo.CheckError('Účastník již zaregistrován s odlišnou školou/ročníkem/rokem narození')
+
+ part = sess.query(db.Participant).get((user.user_id, year))
+ assert part is not None
+
+ if res.fetchall():
+ is_new = True
+ logger.info(f'{reason.title()}: Založen účastník #{user.user_id}')
+ mo.util.log(
+ type=db.LogType.participant,
+ what=user.user_id,
+ details={'action': 'create-participant', 'reason': reason, 'new': db.row2dict(part)},
+ )
+
+ if ((school_id and part.school != school_id)
+ or (grade and part.grade != grade)
+ or (birth_year and part.birth_year != birth_year)):
+ raise mo.CheckError('Účastník již zaregistrován s odlišnou školou/ročníkem/rokem narození')
+
return part, is_new
@@ -165,27 +201,47 @@ def find_or_create_participation(user: db.User, contest: db.Contest, place: Opti
place = contest.place
sess = db.get_session()
- pions = (sess.query(db.Participation)
- .filter_by(user=user)
- .filter(db.Participation.contest.has(db.Contest.round == contest.round))
- .all())
+ pion = None
+ is_new = False
+ retry = False
+
+ while pion is None:
+ pions = (sess.query(db.Participation)
+ .filter_by(user=user)
+ .filter(db.Participation.contest.has(db.Contest.round == contest.round))
+ .all())
+
+ if len(pions) == 0:
+ assert not retry
+ retry = True
+ res = sess.connection().execute(
+ pgsql_insert(db.Participation.__table__)
+ .values(
+ user_id=user.user_id,
+ contest_id=contest.contest_id,
+ place_id=place.place_id,
+ state=db.PartState.active,
+ )
+ .on_conflict_do_nothing()
+ .returning(db.Participation.user_id)
+ )
+ if res.fetchall():
+ is_new = True
+ elif len(pions) == 1:
+ pion = pions[0]
+ else:
+ raise mo.CheckError('Již se tohoto kola účastní ve více oblastech, což by nemělo být možné')
+
+ if pion.place != place:
+ raise mo.CheckError(f'Již se tohoto kola účastní v {contest.round.get_level().name_locative("jiném", "jiné", "jiném")} ({pion.place.get_code()})')
- is_new = pions == []
if is_new:
- pion = db.Participation(user=user, contest=contest, place_id=place.place_id, state=db.PartState.active)
- sess.add(pion)
logger.info(f'{reason.title()}: Založena účast user=#{user.user_id} contest=#{contest.contest_id} place=#{place.place_id}')
mo.util.log(
type=db.LogType.participant,
what=user.user_id,
details={'action': 'add-to-contest', 'reason': reason, 'new': db.row2dict(pion)},
)
- elif len(pions) == 1:
- pion = pions[0]
- if pion.place != place:
- raise mo.CheckError(f'Již se tohoto kola účastní v {contest.round.get_level().name_locative("jiném", "jiné", "jiném")} ({pion.place.get_code()})')
- else:
- raise mo.CheckError('Již se tohoto kola účastní ve více oblastech, což by nemělo být možné')
return pion, is_new
--
GitLab