Coverage for server / utilities / assignment_dedupe.py: 97%
35 statements
« prev ^ index » next coverage.py v7.13.4, created at 2026-10-04 09:33 +0000
1"""One guard that stops a retried or double-clicked create from duplicating an assignment.
3THE PROBLEM
4-----------
5`POST /v1/assignments/create` (and the teacher-scoped `POST /v1/teacher/assignment/create`)
6insert unconditionally. Two identical requests produce two assignments:
8* **Sequential retry** (EI-3450 / Zephyr EI-T568) — a network retry, or a double-click
9 before the button disables. The first write already succeeded; the client, having seen
10 no response or an error, sends it again.
11* **Concurrent race** (EI-3451 / Zephyr EI-T574) — two identical requests in flight at
12 once, so a read-then-write check would pass in both before either insert lands.
14This has bitten before for a reason worth recording: `routes/common/assignments.py`
15carried `response_model=Assignment` over a route that returns an *envelope*, so FastAPI
16raised `ResponseValidationError` **after** the insert had committed. Every call created an
17assignment and reported failure, and a retrying client duplicated it (EI-T113, fixed).
18The response bug is gone; the duplicate-on-retry gap it exposed is what this module closes.
20WHY A UNIQUE INDEX AND NOT A PRE-INSERT LOOKUP
21----------------------------------------------
22A read-then-write check closes the sequential case and not the concurrent one — both
23readers see nothing, both insert. Only the database can arbitrate a race. EI-3451 asks for
24"the same guard, not two separate mechanisms", and a unique index is literally that: one
25constraint underneath all three create paths, so it cannot be bypassed by whichever
26service happens to do the insert.
28WHY NOT AN `Idempotency-Key` HEADER
29-----------------------------------
30The IETF draft (`draft-ietf-httpapi-idempotency-key-header`, Standards Track) expects a
31*client* that generates a key and resends it on retry. The clients that cause this bug are
32a double-clicked button and an automatic network retry — neither sends such a header, so a
33header-based guard would sit unreachable from the very traffic it is meant to catch. The
34header earns its keep when the payload is genuinely arbitrary (payments). Here the request
35carries a natural identity, so we use it.
37WHAT COUNTS AS "THE SAME ASSIGNMENT"
38------------------------------------
39`(created_by, assigned_class, title, date_open, date_close)`, hashed. Deliberately narrow:
40two assignments differing in any of those are different assignments. `assigned_class` is
41sorted before hashing so class order in the payload cannot produce two different keys for
42the same request.
44THE TIME WINDOW, AND WHY IT IS NOT OPTIONAL
45-------------------------------------------
46A *permanent* uniqueness constraint on that key would over-deliver. It would forbid a
47teacher from ever re-creating the same-titled assignment for the same class on the same
48dates — a business rule nobody asked for, and not what EI-3450 describes. Worse, replaying
49the original assignment for a genuine months-later duplicate would hand the caller a
50*different* assignment than the one they asked to create: a silent wrong answer, which is
51worse than an error.
53So the key is permanent in the index but the *interpretation* is time-boxed, which mirrors
54how Stripe scopes idempotency keys with an expiry:
56* collision within :data:`DEDUPE_WINDOW_SECONDS` — a retry or a race. Return the assignment
57 that already exists, 200, same envelope. Idempotent, and EI-3450's "returns the original".
58* collision after the window — not a retry. **409** with a message that says what happened,
59 so the caller can rename or change the dates. EI-3450 allows this outcome explicitly.
61Developer: Allan Ninal
62"""
64from __future__ import annotations
66import hashlib
67from datetime import datetime, timedelta, timezone
68from typing import Any, Iterable, Optional
70# A retry or a double-click lands within seconds. 60s is wide enough to cover a slow
71# connection's automatic retry and narrow enough that a deliberate re-creation minutes
72# later is treated as what it is: a new assignment, not a replay.
73DEDUPE_WINDOW_SECONDS = 60
75# Name kept in one place so the index and any future migration cannot drift apart.
76DEDUPE_INDEX_NAME = "assignment_dedupe_key_unique_sparse"
79def _normalise_date(value: Optional[datetime]) -> str:
80 """Render a date as a stable string.
82 Naive datetimes are treated as UTC. Without this, the same instant submitted once with
83 a timezone and once without would hash differently and the retry would slip through.
84 """
85 if value is None:
86 return ""
87 if value.tzinfo is None:
88 value = value.replace(tzinfo=timezone.utc)
89 return value.astimezone(timezone.utc).isoformat()
92def compute_dedupe_key(
93 created_by: Any,
94 assigned_class: Optional[Iterable[Any]],
95 title: Optional[str],
96 date_open: Optional[datetime],
97 date_close: Optional[datetime],
98) -> str:
99 """Hash the fields that make two create requests the same request.
101 Returns a hex digest. `assigned_class` is sorted by string form so the payload's class
102 ordering cannot change the key, and `title` is stripped so a stray trailing space does
103 not read as a different assignment.
104 """
105 classes = sorted(str(item) for item in (assigned_class or []))
106 parts = [
107 str(created_by or ""),
108 ",".join(classes),
109 (title or "").strip(),
110 _normalise_date(date_open),
111 _normalise_date(date_close),
112 ]
113 # Each field is LENGTH-PREFIXED rather than separator-joined, so no field's content can
114 # impersonate a field boundary.
115 #
116 # A plain join is ambiguous even with an exotic separator, which mutation testing on
117 # this function is what surfaced: with any separator S and a fixed field count,
118 # ("a"+S+"b", ["c"]) and ("a", ["b"+S+"c"]) render the SAME string, because moving S
119 # across the boundary leaves the total unchanged. Neither input is reachable today —
120 # `created_by` is a server-set auth id and class ids are ObjectIds — but the encoding
121 # costs nothing to get right, and "the inputs happen to be safe" is a property of
122 # today's callers, not of this function.
123 canonical = "".join(f"{len(part)}:{part}" for part in parts)
124 return hashlib.sha256(canonical.encode("utf-8")).hexdigest()
127def is_within_dedupe_window(
128 created_at: Optional[datetime], now: Optional[datetime] = None
129) -> bool:
130 """True when `created_at` is recent enough for a collision to be a retry.
132 A missing `created_at` returns False: with nothing to date the existing assignment by,
133 the safe reading is "not a retry", which surfaces a 409 rather than silently handing
134 back some older assignment.
135 """
136 if created_at is None:
137 return False
138 if created_at.tzinfo is None:
139 created_at = created_at.replace(tzinfo=timezone.utc)
140 now = now or datetime.now(timezone.utc)
141 if now.tzinfo is None:
142 now = now.replace(tzinfo=timezone.utc)
143 # A clock skew that puts created_at slightly in the future is still a retry.
144 return abs(now - created_at) <= timedelta(seconds=DEDUPE_WINDOW_SECONDS)
147async def resolve_duplicate_create(dedupe_key: str):
148 """Decide what a rejected duplicate insert should return.
150 Called only after the unique index has refused an insert, so a matching assignment
151 exists by construction. Returns the existing :class:`Assignment` when the collision
152 looks like a retry or a race; raises 409 when it does not.
154 Every create path funnels through here, so the retry-vs-conflict decision is made in
155 exactly one place (EI-3451).
156 """
157 # Imported here rather than at module scope: this module is imported by
158 # server/connection/database.py, which also imports the models, and a top-level import
159 # would tie the two together for no benefit.
160 from fastapi import HTTPException, status
162 from server.models.assignment import Assignment
164 existing = await Assignment.find_one({"dedupe_key": dedupe_key})
166 if existing is None:
167 # The index said this key is taken but the row is not readable — a hard delete
168 # landing between the failed insert and this read is the realistic cause. Nothing
169 # to replay, and claiming success would be a lie, so say what happened.
170 raise HTTPException(
171 status_code=status.HTTP_409_CONFLICT,
172 detail="An identical assignment was created concurrently. Please try again.",
173 )
175 if is_within_dedupe_window(existing.created_at):
176 return existing
178 raise HTTPException(
179 status_code=status.HTTP_409_CONFLICT,
180 detail=(
181 "An assignment with the same title, class and dates already exists. "
182 "Change the title or the dates to create another one."
183 ),
184 )