diff --git a/backend/app/config.py b/backend/app/config.py index 7bbdec43f..3e4bdd985 100644 --- a/backend/app/config.py +++ b/backend/app/config.py @@ -46,7 +46,7 @@ def get_secret(env_var: str, default: str = None) -> str | None: MIN_FRONTEND_VERSION = 71 -BACKEND_VERSION = 120 +BACKEND_VERSION = 121 APP_DIR = os.path.dirname(os.path.abspath(__file__)) PROJECT_DIR = os.path.dirname(APP_DIR) diff --git a/backend/app/controller/shoppinglist/schemas.py b/backend/app/controller/shoppinglist/schemas.py index 5b7c3bf4d..fabe2a02f 100644 --- a/backend/app/controller/shoppinglist/schemas.py +++ b/backend/app/controller/shoppinglist/schemas.py @@ -1,4 +1,15 @@ -from marshmallow import fields, Schema, EXCLUDE +from datetime import datetime, timezone + +from marshmallow import fields, Schema, EXCLUDE, ValidationError + + +def _validate_epoch_ms(value: int) -> None: + """Reject timestamps that are non-positive or more than 5 minutes in the future.""" + if value <= 0: + raise ValidationError("Timestamp must be a positive integer.") + max_ms = int(datetime.now(timezone.utc).timestamp() * 1000) + 300_000 # +5 min + if value > max_ms: + raise ValidationError("Timestamp is too far in the future.") class GetShoppingLists(Schema): @@ -42,17 +53,27 @@ class GetRecentItems(Schema): class UpdateDescription(Schema): + class Meta: + unknown = EXCLUDE + description = fields.String(required=True) + client_timestamp = fields.Integer(validate=_validate_epoch_ms) class RemoveItem(Schema): + class Meta: + unknown = EXCLUDE + item_id = fields.Integer( required=True, ) - removed_at = fields.Integer() + removed_at = fields.Integer(validate=_validate_epoch_ms) class RemoveItems(Schema): + class Meta: + unknown = EXCLUDE + class RecipeItem(Schema): class Meta: unknown = EXCLUDE @@ -60,6 +81,6 @@ class Meta: item_id = fields.Integer( required=True, ) - removed_at = fields.Integer() + removed_at = fields.Integer(validate=_validate_epoch_ms) items = fields.List(fields.Nested(RecipeItem)) diff --git a/backend/app/controller/shoppinglist/shoppinglist_controller.py b/backend/app/controller/shoppinglist/shoppinglist_controller.py index 64cdf47aa..5050bdd89 100644 --- a/backend/app/controller/shoppinglist/shoppinglist_controller.py +++ b/backend/app/controller/shoppinglist/shoppinglist_controller.py @@ -25,6 +25,7 @@ from app.errors import NotFoundRequest, InvalidUsage from datetime import datetime, timedelta, timezone import app.util.description_merger as description_merger +import app.util.transaction_resolver as resolver from app import socketio @@ -128,21 +129,54 @@ def deleteShoppinglist(id): @validate_args(UpdateDescription) def updateItemDescription(args, id: int, item_id: int): con = ShoppinglistItems.find_by_ids(id, item_id) - if not con: - shoppinglist = Shoppinglist.find_by_id(id) - item = Item.find_by_id(item_id) - if not item or not shoppinglist: - raise NotFoundRequest() - if shoppinglist.household_id != item.household_id: - raise InvalidUsage() + client_timestamp = args.get("client_timestamp") + + shoppinglist_obj = Shoppinglist.find_by_id(id) + item = Item.find_by_id(item_id) + if not item or not shoppinglist_obj: + raise NotFoundRequest() + if shoppinglist_obj.household_id != item.household_id: + raise InvalidUsage() + shoppinglist_obj.checkAuthorized() + + # Look up History DROPPED record for the resolver (needed when item is not on list) + most_recent_dropped = None + if con is None: + most_recent_dropped = History.find_most_recent_dropped(id, item_id) + + result = resolver.resolve_update_description( + con, client_timestamp, most_recent_dropped + ) + + if result.resolution == resolver.Resolution.REJECT: + # Operation is stale — return success to the client (don't retry) but do nothing + return jsonify({"msg": "CONFLICT_REJECTED", "reason": result.reason}) + + if result.resolution == resolver.Resolution.NOOP: + # Item not on list and no History to update + return jsonify({"msg": "NOOP", "reason": result.reason}) + + if result.resolution == resolver.Resolution.UPDATE_HISTORY: + # Item was removed — update the History DROPPED record's description + most_recent_dropped.description = args["description"] or "" + most_recent_dropped.save() + return jsonify({"msg": "HISTORY_UPDATED", "reason": result.reason}) + + # Resolution.ACCEPT — proceed with the update (or create if not on list) + created = con is None + if con is None: con = ShoppinglistItems() - con.shoppinglist = shoppinglist + con.shoppinglist = shoppinglist_obj con.item = item con.created_by = current_user.id - con.shoppinglist.checkAuthorized() con.description = args["description"] or "" + resolver.set_updated_at(con, client_timestamp) con.save() + + if created: + History.create_added(shoppinglist_obj, item, con.description) + socketio.emit( "shoppinglist_item:add", { @@ -378,14 +412,18 @@ def removeShoppinglistItemFunc( if not item: return None con = ShoppinglistItems.find_by_ids(shoppinglist.id, item.id) - if not con: + + # Use the transaction resolver to check if this remove should proceed + result = resolver.resolve_remove(con, removed_at) + if result.resolution != resolver.Resolution.ACCEPT: return None + description = con.description con.delete() removed_at_datetime = None if removed_at: - removed_at_datetime = datetime.fromtimestamp(removed_at / 1000, timezone.utc) + removed_at_datetime = resolver.epoch_ms_to_datetime(removed_at) History.create_dropped(shoppinglist, item, description, removed_at_datetime) return con diff --git a/backend/app/models/history.py b/backend/app/models/history.py index 0425b2082..b81c0076f 100644 --- a/backend/app/models/history.py +++ b/backend/app/models/history.py @@ -97,6 +97,25 @@ def find_dropped_by_shoppinglist_id(cls, shoppinglist_id: int) -> list[Self]: def find_by_shoppinglist_id(cls, shoppinglist_id: int) -> list[Self]: return cls.query.filter(cls.shoppinglist_id == shoppinglist_id).all() + @classmethod + def find_most_recent_dropped( + cls, shoppinglist_id: int, item_id: int + ) -> Self | None: + """ + Find the most recent History record with status=DROPPED for a given + shoppinglist + item combination. Used by the transaction resolver to + update the description on a removed item's History record. + """ + return ( + cls.query.filter( + cls.shoppinglist_id == shoppinglist_id, + cls.item_id == item_id, + cls.status == Status.DROPPED, + ) + .order_by(cls.created_at.desc()) + .first() + ) + @classmethod def find_all(cls) -> list[Self]: return cls.query.all() diff --git a/backend/app/util/transaction_resolver.py b/backend/app/util/transaction_resolver.py new file mode 100644 index 000000000..77ace882d --- /dev/null +++ b/backend/app/util/transaction_resolver.py @@ -0,0 +1,208 @@ +""" +Transaction Resolver for Shopping List Conflict Resolution. + +When clients go offline and queue operations, those operations may conflict +with changes made by other clients while they were disconnected. This module +provides guards for each mutation endpoint that evaluate whether a stale +client operation is still relevant against the current server state. + +Core principle: each operation is evaluated individually against the current +server state + timestamps. The backend answers "is this operation still +relevant right now?" — not global event ordering. + +When an operation wins (client_timestamp > updated_at), the server sets +updated_at = client_timestamp (not server receive time) so that subsequent +stale operations from other clients are ordered correctly against it. +""" + +from __future__ import annotations + +from datetime import datetime, timezone +from enum import Enum +from typing import NamedTuple, TYPE_CHECKING + +if TYPE_CHECKING: + from app.models import ShoppinglistItems, History + + +class Resolution(Enum): + """Outcome of a conflict resolution check.""" + + ACCEPT = "accept" # Operation should proceed normally + REJECT = "reject" # Operation is stale, skip it + NOOP = "noop" # Operation has no effect (item already gone, etc.) + UPDATE_HISTORY = "update_history" # Update a History record's description + + +class ResolveResult(NamedTuple): + """Result of a conflict resolution check.""" + + resolution: Resolution + reason: str + + +def epoch_ms_to_datetime(epoch_ms: int) -> datetime: + """Convert epoch milliseconds (as sent by Flutter clients) to UTC datetime.""" + return datetime.fromtimestamp(epoch_ms / 1000, timezone.utc) + + +def _ensure_aware(dt: datetime) -> datetime: + """ + Ensure a datetime is timezone-aware (UTC). + + SQLite stores datetimes as naive strings and returns them without tzinfo, + even when the original value was timezone-aware. PostgreSQL preserves + timezone info. This helper normalizes both cases to aware-UTC so + comparisons don't raise TypeError. + """ + if dt.tzinfo is None: + return dt.replace(tzinfo=timezone.utc) + return dt + + +def resolve_remove( + con: "ShoppinglistItems | None", + client_timestamp: int | None, +) -> ResolveResult: + """ + Evaluate whether a remove operation should proceed. + + Resolution table: + - Item not on list → NOOP + - Item on list, no ts → ACCEPT (backwards compat: no timestamp = always allow) + - Item on list, ts > updated_at → ACCEPT + - Item on list, ts <= updated_at → REJECT (item was modified after this remove was queued) + + Args: + con: The ShoppinglistItems row, or None if item is not on the list. + client_timestamp: Epoch milliseconds from the client, or None for + legacy clients that don't send timestamps. + + Returns: + ResolveResult with the resolution and a human-readable reason. + """ + if con is None: + return ResolveResult(Resolution.NOOP, "Item is not on the shopping list") + + # No timestamp provided — backwards compatible, always allow + if client_timestamp is None: + return ResolveResult(Resolution.ACCEPT, "No client timestamp (legacy)") + + client_dt = epoch_ms_to_datetime(client_timestamp) + + updated_at = _ensure_aware(con.updated_at) + + if client_dt > updated_at: + return ResolveResult( + Resolution.ACCEPT, + "Client timestamp is newer than last update", + ) + else: + return ResolveResult( + Resolution.REJECT, + "Item was modified after this remove was queued " + f"(client_ts={client_dt.isoformat()}, updated_at={updated_at.isoformat()})", + ) + + +def resolve_update_description( + con: "ShoppinglistItems | None", + client_timestamp: int | None, + most_recent_dropped: "History | None" = None, +) -> ResolveResult: + """ + Evaluate whether an update-description operation should proceed. + + Resolution table: + - Item on list, no ts → ACCEPT (backwards compat) + - Item on list, ts > updated_at → ACCEPT + - Item on list, ts <= updated_at → REJECT + - Item NOT on list, History DROPPED exists with created_at < client_ts + → UPDATE_HISTORY + - Item NOT on list, no matching History or History created_at >= client_ts + → NOOP + + Args: + con: The ShoppinglistItems row, or None if item is not on the list. + client_timestamp: Epoch milliseconds from the client, or None. + most_recent_dropped: The most recent History record with status=DROPPED + for this item on this shoppinglist (only needed when con is None). + + Returns: + ResolveResult with the resolution and a human-readable reason. + """ + if con is not None: + # Item is currently on the shopping list + if client_timestamp is None: + return ResolveResult(Resolution.ACCEPT, "No client timestamp (legacy)") + + client_dt = epoch_ms_to_datetime(client_timestamp) + updated_at = _ensure_aware(con.updated_at) + + if client_dt > updated_at: + return ResolveResult( + Resolution.ACCEPT, + "Client timestamp is newer than last update", + ) + else: + return ResolveResult( + Resolution.REJECT, + "Item was modified after this update was queued " + f"(client_ts={client_dt.isoformat()}, " + f"updated_at={updated_at.isoformat()})", + ) + else: + # Item is NOT on the shopping list — check if we should update History + if client_timestamp is None: + # Backwards compat: no timestamp means this is a legacy client. + # The PUT endpoint historically acts as an upsert (add item to list + # if not present), so we ACCEPT to preserve that behavior. + return ResolveResult( + Resolution.ACCEPT, + "No client timestamp (legacy), upsert behavior", + ) + + if most_recent_dropped is None: + return ResolveResult( + Resolution.NOOP, + "Item not on list, no History DROPPED record found", + ) + + client_dt = epoch_ms_to_datetime(client_timestamp) + history_created_at = _ensure_aware(most_recent_dropped.created_at) + + if history_created_at < client_dt: + return ResolveResult( + Resolution.UPDATE_HISTORY, + "Updating History DROPPED record description " + f"(history_created_at={history_created_at.isoformat()}, " + f"client_ts={client_dt.isoformat()})", + ) + else: + return ResolveResult( + Resolution.NOOP, + "Edit predates the removal " + f"(history_created_at={history_created_at.isoformat()}, " + f"client_ts={client_dt.isoformat()})", + ) + + +def set_updated_at(obj, client_timestamp: int | None) -> None: + """ + Set updated_at to the client timestamp if provided, bypassing SQLAlchemy's + onupdate auto-setter. + + This is important for conflict resolution ordering: when an operation wins, + we record the client's timestamp (not the server receive time) so that + subsequent stale operations from other clients are correctly compared. + + We use SQLAlchemy's attribute system to set the value, which will be + persisted on the next flush/commit. The onupdate lambda will be overridden + because we're explicitly setting the column value. + + Args: + obj: Any model instance with an updated_at column. + client_timestamp: Epoch milliseconds, or None to use default behavior. + """ + if client_timestamp is not None: + obj.updated_at = epoch_ms_to_datetime(client_timestamp) diff --git a/backend/tests/api/test_api_shoppinglist_conflict.py b/backend/tests/api/test_api_shoppinglist_conflict.py new file mode 100644 index 000000000..5528a22b0 --- /dev/null +++ b/backend/tests/api/test_api_shoppinglist_conflict.py @@ -0,0 +1,588 @@ +""" +Tests for shopping list conflict resolution. + +Covers all cases from the resolution table: + Remove: NOOP (item gone), ACCEPT (legacy), ACCEPT (ts > updated_at), REJECT (ts <= updated_at) + Update: ACCEPT (legacy), ACCEPT (ts > updated_at), REJECT (ts <= updated_at), + NOOP (item gone, no history), UPDATE_HISTORY (item gone, history exists) + Add: Idempotent (already on list), always succeeds (not on list) + Validation: zero, negative, and far-future timestamps are rejected by schema +""" + +from datetime import datetime, timedelta, timezone + +import pytest + + +# --------------------------------------------------------------------------- +# Helpers +# --------------------------------------------------------------------------- + + +def _epoch_ms(dt: datetime) -> int: + """Convert a datetime to epoch milliseconds (what the Flutter client sends).""" + return int(dt.timestamp() * 1000) + + +def _now_ms() -> int: + return _epoch_ms(datetime.now(timezone.utc)) + + +def _future_ms(seconds: int = 60) -> int: + """Return an epoch-ms timestamp ``seconds`` in the future (within validation window).""" + return _epoch_ms(datetime.now(timezone.utc) + timedelta(seconds=seconds)) + + +def _past_ms(seconds: int = 3600) -> int: + """Return an epoch-ms timestamp ``seconds`` in the past.""" + return _epoch_ms(datetime.now(timezone.utc) - timedelta(seconds=seconds)) + + +def _add_item_to_list(client, shoppinglist_id, item_name="conflict_item"): + """Add an item by name and return (item_id, shoppinglist_id).""" + resp = client.post( + f"/api/shoppinglist/{shoppinglist_id}/add-item-by-name", + json={"name": item_name}, + ) + assert resp.status_code == 200 + item_data = resp.get_json() + return item_data["id"] + + +def _get_items(client, shoppinglist_id): + """Get all items on the shopping list.""" + resp = client.get(f"/api/shoppinglist/{shoppinglist_id}/items") + assert resp.status_code == 200 + return resp.get_json() + + +# =========================================================================== +# REMOVE conflict resolution +# =========================================================================== + + +class TestRemoveConflictResolution: + """Tests for the remove operation conflict resolver.""" + + def test_remove_item_not_on_list_is_noop( + self, user_client_with_household, shoppinglist_id + ): + """Removing an item that isn't on the list should succeed (NOOP, no error).""" + # Add and then remove the item first so we have a valid item_id + item_id = _add_item_to_list( + user_client_with_household, shoppinglist_id, "gone_item" + ) + resp = user_client_with_household.delete( + f"/api/shoppinglist/{shoppinglist_id}/item", + json={"item_id": item_id}, + ) + assert resp.status_code == 200 + + # Now try to remove it again — should be NOOP, not error + resp = user_client_with_household.delete( + f"/api/shoppinglist/{shoppinglist_id}/item", + json={"item_id": item_id}, + ) + assert resp.status_code == 200 + + def test_remove_legacy_no_timestamp_always_accepts( + self, user_client_with_household, shoppinglist_id + ): + """Legacy clients (no removed_at) should always be able to remove items.""" + item_id = _add_item_to_list( + user_client_with_household, shoppinglist_id, "legacy_remove" + ) + resp = user_client_with_household.delete( + f"/api/shoppinglist/{shoppinglist_id}/item", + json={"item_id": item_id}, + ) + assert resp.status_code == 200 + + items = _get_items(user_client_with_household, shoppinglist_id) + assert not any(i["id"] == item_id for i in items) + + def test_remove_fresh_timestamp_accepts( + self, user_client_with_household, shoppinglist_id + ): + """Remove with a timestamp newer than updated_at should succeed.""" + item_id = _add_item_to_list( + user_client_with_household, shoppinglist_id, "fresh_remove" + ) + + # Use a timestamp slightly in the future (within the 5-min validation window) + future_ts = _future_ms(60) + resp = user_client_with_household.delete( + f"/api/shoppinglist/{shoppinglist_id}/item", + json={"item_id": item_id, "removed_at": future_ts}, + ) + assert resp.status_code == 200 + + items = _get_items(user_client_with_household, shoppinglist_id) + assert not any(i["id"] == item_id for i in items) + + def test_remove_stale_timestamp_rejects( + self, user_client_with_household, shoppinglist_id + ): + """Remove with a timestamp older than updated_at should be silently rejected.""" + item_id = _add_item_to_list( + user_client_with_household, shoppinglist_id, "stale_remove" + ) + + # First, update the item's description with a fresh timestamp to bump updated_at + fresh_ts = _future_ms(120) + resp = user_client_with_household.put( + f"/api/shoppinglist/{shoppinglist_id}/item/{item_id}", + json={ + "description": "updated after remove queued", + "client_timestamp": fresh_ts, + }, + ) + assert resp.status_code == 200 + + # Now try to remove with a stale timestamp (before the update) + stale_ts = _past_ms(3600) + resp = user_client_with_household.delete( + f"/api/shoppinglist/{shoppinglist_id}/item", + json={"item_id": item_id, "removed_at": stale_ts}, + ) + assert resp.status_code == 200 # Still returns 200 (no retry) + + # Item should still be on the list + items = _get_items(user_client_with_household, shoppinglist_id) + assert any(i["id"] == item_id for i in items) + + +# =========================================================================== +# UPDATE DESCRIPTION conflict resolution +# =========================================================================== + + +class TestUpdateDescriptionConflictResolution: + """Tests for the update-description operation conflict resolver.""" + + def test_update_legacy_no_timestamp_accepts( + self, user_client_with_household, shoppinglist_id + ): + """Legacy clients (no client_timestamp) should always be able to update.""" + item_id = _add_item_to_list( + user_client_with_household, shoppinglist_id, "legacy_update" + ) + resp = user_client_with_household.put( + f"/api/shoppinglist/{shoppinglist_id}/item/{item_id}", + json={"description": "legacy update"}, + ) + assert resp.status_code == 200 + data = resp.get_json() + assert data.get("description") == "legacy update" + + def test_update_fresh_timestamp_accepts( + self, user_client_with_household, shoppinglist_id + ): + """Update with timestamp newer than updated_at should succeed.""" + item_id = _add_item_to_list( + user_client_with_household, shoppinglist_id, "fresh_update" + ) + future_ts = _future_ms(60) + resp = user_client_with_household.put( + f"/api/shoppinglist/{shoppinglist_id}/item/{item_id}", + json={"description": "fresh update", "client_timestamp": future_ts}, + ) + assert resp.status_code == 200 + data = resp.get_json() + assert data.get("description") == "fresh update" + + def test_update_stale_timestamp_rejects( + self, user_client_with_household, shoppinglist_id + ): + """Update with timestamp older than updated_at should be rejected.""" + item_id = _add_item_to_list( + user_client_with_household, shoppinglist_id, "stale_update" + ) + + # First update with a fresh timestamp to bump updated_at + fresh_ts = _future_ms(120) + resp = user_client_with_household.put( + f"/api/shoppinglist/{shoppinglist_id}/item/{item_id}", + json={"description": "first update", "client_timestamp": fresh_ts}, + ) + assert resp.status_code == 200 + + # Now try to update with a stale timestamp + stale_ts = _past_ms(3600) + resp = user_client_with_household.put( + f"/api/shoppinglist/{shoppinglist_id}/item/{item_id}", + json={"description": "stale update", "client_timestamp": stale_ts}, + ) + assert resp.status_code == 200 + data = resp.get_json() + assert data.get("msg") == "CONFLICT_REJECTED" + + # Description should remain the first update + items = _get_items(user_client_with_household, shoppinglist_id) + item = next(i for i in items if i["id"] == item_id) + assert item["description"] == "first update" + + def test_update_item_not_on_list_legacy_upserts( + self, user_client_with_household, shoppinglist_id + ): + """Legacy client updating a non-existent item should upsert (add it).""" + item_id = _add_item_to_list( + user_client_with_household, shoppinglist_id, "upsert_item" + ) + # Remove it + resp = user_client_with_household.delete( + f"/api/shoppinglist/{shoppinglist_id}/item", + json={"item_id": item_id}, + ) + assert resp.status_code == 200 + + # Legacy update (no client_timestamp) should upsert — add item back + resp = user_client_with_household.put( + f"/api/shoppinglist/{shoppinglist_id}/item/{item_id}", + json={"description": "upserted"}, + ) + assert resp.status_code == 200 + + items = _get_items(user_client_with_household, shoppinglist_id) + assert any(i["id"] == item_id for i in items) + + def test_update_item_not_on_list_with_timestamp_updates_history( + self, user_client_with_household, shoppinglist_id + ): + """ + When item is not on list and a History DROPPED record exists with + created_at < client_timestamp, should update the History description. + """ + item_id = _add_item_to_list( + user_client_with_household, shoppinglist_id, "history_update_item" + ) + + # Remove the item (creates a History DROPPED record) + resp = user_client_with_household.delete( + f"/api/shoppinglist/{shoppinglist_id}/item", + json={"item_id": item_id}, + ) + assert resp.status_code == 200 + + # Update with a timestamp in the future (> History DROPPED created_at) + future_ts = _future_ms(60) + resp = user_client_with_household.put( + f"/api/shoppinglist/{shoppinglist_id}/item/{item_id}", + json={"description": "updated in history", "client_timestamp": future_ts}, + ) + assert resp.status_code == 200 + data = resp.get_json() + assert data.get("msg") == "HISTORY_UPDATED" + + # Item should NOT be back on the list + items = _get_items(user_client_with_household, shoppinglist_id) + assert not any(i["id"] == item_id for i in items) + + def test_update_item_not_on_list_stale_timestamp_is_noop( + self, user_client_with_household, shoppinglist_id + ): + """ + When item is not on list, but the edit predates the removal + (client_timestamp <= History DROPPED created_at), should be NOOP. + """ + item_id = _add_item_to_list( + user_client_with_household, shoppinglist_id, "noop_history_item" + ) + + # Remove the item (legacy — no removed_at). History created_at ~ server now. + resp = user_client_with_household.delete( + f"/api/shoppinglist/{shoppinglist_id}/item", + json={"item_id": item_id}, + ) + assert resp.status_code == 200 + + # Try to update with a timestamp in the past (before the removal) + past_ts = _past_ms(3600) + resp = user_client_with_household.put( + f"/api/shoppinglist/{shoppinglist_id}/item/{item_id}", + json={"description": "should be ignored", "client_timestamp": past_ts}, + ) + assert resp.status_code == 200 + data = resp.get_json() + assert data.get("msg") == "NOOP" + + +# =========================================================================== +# ADD idempotency +# =========================================================================== + + +class TestAddIdempotency: + """Tests that add operations are idempotent.""" + + def test_add_item_already_on_list_is_noop( + self, user_client_with_household, shoppinglist_id + ): + """Adding an item that's already on the list should not duplicate it.""" + item_id = _add_item_to_list( + user_client_with_household, shoppinglist_id, "idempotent_add" + ) + + # Add the same item again via PUT (the putItem path) + resp = user_client_with_household.put( + f"/api/shoppinglist/{shoppinglist_id}/item/{item_id}", + json={"description": ""}, + ) + assert resp.status_code == 200 + + items = _get_items(user_client_with_household, shoppinglist_id) + matching = [i for i in items if i["id"] == item_id] + assert len(matching) == 1 + + def test_add_item_not_on_list_always_succeeds( + self, user_client_with_household, shoppinglist_id + ): + """Adding an item that's not on the list should always work.""" + item_id = _add_item_to_list( + user_client_with_household, shoppinglist_id, "new_add_item" + ) + items = _get_items(user_client_with_household, shoppinglist_id) + assert any(i["id"] == item_id for i in items) + + +# =========================================================================== +# Backwards compatibility +# =========================================================================== + + +class TestBackwardsCompatibility: + """Tests that unknown fields are silently dropped (EXCLUDE behavior).""" + + def test_remove_with_unknown_fields_excluded( + self, user_client_with_household, shoppinglist_id + ): + """RemoveItem schema should silently drop unknown fields.""" + item_id = _add_item_to_list( + user_client_with_household, shoppinglist_id, "exclude_test" + ) + resp = user_client_with_household.delete( + f"/api/shoppinglist/{shoppinglist_id}/item", + json={ + "item_id": item_id, + "some_future_field": "should be ignored", + }, + ) + assert resp.status_code == 200 + + def test_update_with_unknown_fields_excluded( + self, user_client_with_household, shoppinglist_id + ): + """UpdateDescription schema should silently drop unknown fields.""" + item_id = _add_item_to_list( + user_client_with_household, shoppinglist_id, "exclude_update" + ) + resp = user_client_with_household.put( + f"/api/shoppinglist/{shoppinglist_id}/item/{item_id}", + json={ + "description": "test", + "some_future_field": "should be ignored", + }, + ) + assert resp.status_code == 200 + + def test_bulk_remove_with_unknown_fields_excluded( + self, user_client_with_household, shoppinglist_id + ): + """RemoveItems (bulk) schema should silently drop unknown fields.""" + item_id = _add_item_to_list( + user_client_with_household, shoppinglist_id, "bulk_exclude" + ) + resp = user_client_with_household.delete( + f"/api/shoppinglist/{shoppinglist_id}/items", + json={ + "items": [{"item_id": item_id, "unknown_field": 42}], + "top_level_unknown": True, + }, + ) + assert resp.status_code == 200 + + +# =========================================================================== +# Ordering correctness +# =========================================================================== + + +class TestTimestampOrdering: + """ + Tests that updated_at is set to client_timestamp (not server receive time) + so subsequent stale operations are ordered correctly. + """ + + def test_updated_at_set_to_client_timestamp_not_server_time( + self, user_client_with_household, shoppinglist_id + ): + """ + After a successful update with client_timestamp, a slightly-older + timestamp should be rejected — proving updated_at was set to the + client's timestamp, not the server's wall clock. + """ + item_id = _add_item_to_list( + user_client_with_household, shoppinglist_id, "ordering_item" + ) + + # Update with a timestamp at the edge of the validation window + far_future = _future_ms(240) + resp = user_client_with_household.put( + f"/api/shoppinglist/{shoppinglist_id}/item/{item_id}", + json={"description": "future update", "client_timestamp": far_future}, + ) + assert resp.status_code == 200 + + # Try to update with a timestamp that's AFTER now but BEFORE far_future + slightly_less = _future_ms(60) + resp = user_client_with_household.put( + f"/api/shoppinglist/{shoppinglist_id}/item/{item_id}", + json={ + "description": "should be rejected", + "client_timestamp": slightly_less, + }, + ) + assert resp.status_code == 200 + data = resp.get_json() + assert data.get("msg") == "CONFLICT_REJECTED" + + # Confirm original description is preserved + items = _get_items(user_client_with_household, shoppinglist_id) + item = next(i for i in items if i["id"] == item_id) + assert item["description"] == "future update" + + def test_remove_respects_client_timestamp_ordering( + self, user_client_with_household, shoppinglist_id + ): + """ + After an update with a far-future client_timestamp, a remove with + a timestamp between now and that future should be rejected. + """ + item_id = _add_item_to_list( + user_client_with_household, shoppinglist_id, "remove_order_item" + ) + + # Update with future timestamp + far_future = _future_ms(240) + resp = user_client_with_household.put( + f"/api/shoppinglist/{shoppinglist_id}/item/{item_id}", + json={"description": "pinned", "client_timestamp": far_future}, + ) + assert resp.status_code == 200 + + # Remove with a timestamp between now and far_future + mid_ts = _future_ms(60) + resp = user_client_with_household.delete( + f"/api/shoppinglist/{shoppinglist_id}/item", + json={"item_id": item_id, "removed_at": mid_ts}, + ) + assert resp.status_code == 200 + + # Item should still be on the list + items = _get_items(user_client_with_household, shoppinglist_id) + assert any(i["id"] == item_id for i in items) + + +# =========================================================================== +# Timestamp validation (schema-level) +# =========================================================================== + + +class TestTimestampValidation: + """Tests that the schema rejects invalid timestamp values.""" + + def test_update_rejects_zero_timestamp( + self, user_client_with_household, shoppinglist_id + ): + """client_timestamp=0 should be rejected by schema validation.""" + item_id = _add_item_to_list( + user_client_with_household, shoppinglist_id, "val_zero" + ) + resp = user_client_with_household.put( + f"/api/shoppinglist/{shoppinglist_id}/item/{item_id}", + json={"description": "test", "client_timestamp": 0}, + ) + assert resp.status_code == 400 + + def test_update_rejects_negative_timestamp( + self, user_client_with_household, shoppinglist_id + ): + """Negative client_timestamp should be rejected by schema validation.""" + item_id = _add_item_to_list( + user_client_with_household, shoppinglist_id, "val_negative" + ) + resp = user_client_with_household.put( + f"/api/shoppinglist/{shoppinglist_id}/item/{item_id}", + json={"description": "test", "client_timestamp": -1000}, + ) + assert resp.status_code == 400 + + def test_update_rejects_far_future_timestamp( + self, user_client_with_household, shoppinglist_id + ): + """client_timestamp far in the future (>5 min) should be rejected.""" + item_id = _add_item_to_list( + user_client_with_household, shoppinglist_id, "val_future" + ) + far_future = _epoch_ms(datetime.now(timezone.utc) + timedelta(hours=1)) + resp = user_client_with_household.put( + f"/api/shoppinglist/{shoppinglist_id}/item/{item_id}", + json={"description": "test", "client_timestamp": far_future}, + ) + assert resp.status_code == 400 + + def test_remove_rejects_zero_timestamp( + self, user_client_with_household, shoppinglist_id + ): + """removed_at=0 should be rejected by schema validation.""" + item_id = _add_item_to_list( + user_client_with_household, shoppinglist_id, "rm_val_zero" + ) + resp = user_client_with_household.delete( + f"/api/shoppinglist/{shoppinglist_id}/item", + json={"item_id": item_id, "removed_at": 0}, + ) + assert resp.status_code == 400 + + def test_remove_rejects_negative_timestamp( + self, user_client_with_household, shoppinglist_id + ): + """Negative removed_at should be rejected by schema validation.""" + item_id = _add_item_to_list( + user_client_with_household, shoppinglist_id, "rm_val_neg" + ) + resp = user_client_with_household.delete( + f"/api/shoppinglist/{shoppinglist_id}/item", + json={"item_id": item_id, "removed_at": -999}, + ) + assert resp.status_code == 400 + + def test_remove_rejects_far_future_timestamp( + self, user_client_with_household, shoppinglist_id + ): + """removed_at far in the future (>5 min) should be rejected.""" + item_id = _add_item_to_list( + user_client_with_household, shoppinglist_id, "rm_val_future" + ) + far_future = _epoch_ms(datetime.now(timezone.utc) + timedelta(hours=1)) + resp = user_client_with_household.delete( + f"/api/shoppinglist/{shoppinglist_id}/item", + json={"item_id": item_id, "removed_at": far_future}, + ) + assert resp.status_code == 400 + + def test_valid_timestamp_within_window_accepted( + self, user_client_with_household, shoppinglist_id + ): + """A timestamp within the 5-minute window should be accepted.""" + item_id = _add_item_to_list( + user_client_with_household, shoppinglist_id, "val_ok" + ) + valid_ts = _future_ms(60) + resp = user_client_with_household.put( + f"/api/shoppinglist/{shoppinglist_id}/item/{item_id}", + json={"description": "valid", "client_timestamp": valid_ts}, + ) + assert resp.status_code == 200 + data = resp.get_json() + assert data.get("description") == "valid" diff --git a/kitchenowl/lib/services/api/shoppinglist.dart b/kitchenowl/lib/services/api/shoppinglist.dart index cda557012..7a61c184f 100644 --- a/kitchenowl/lib/services/api/shoppinglist.dart +++ b/kitchenowl/lib/services/api/shoppinglist.dart @@ -59,12 +59,16 @@ extension ShoppinglistApi on ApiService { Future putItem( ShoppingList shoppinglist, - Item item, - ) async { + Item item, [ + DateTime? timestamp, + ]) async { final Map data = {}; if (item is ItemWithDescription) { data['description'] = item.description; } + if (timestamp != null) { + data['client_timestamp'] = timestamp.toUtc().millisecondsSinceEpoch; + } final res = await put( '${route(shoppinglist: shoppinglist)}/item/${item.id}', jsonEncode(data), diff --git a/kitchenowl/lib/services/transactions/shoppinglist.dart b/kitchenowl/lib/services/transactions/shoppinglist.dart index 460038b3d..4d538be6c 100644 --- a/kitchenowl/lib/services/transactions/shoppinglist.dart +++ b/kitchenowl/lib/services/transactions/shoppinglist.dart @@ -344,6 +344,7 @@ class TransactionShoppingListUpdateItem extends Transaction { return ApiService.getInstance().putItem( shoppinglist, ItemWithDescription.fromItem(item: item, description: description), + timestamp, ); } }