feat(poll): make exact transitions idempotent
This commit is contained in:
@@ -169,8 +169,13 @@ metadata. Every applied transition has a Poll-owned lifecycle audit record.
|
|||||||
Re-deciding always appends a record and supersedes the current decision. An
|
Re-deciding always appends a record and supersedes the current decision. An
|
||||||
exact retry carrying the same `Idempotency-Key` is a no-op and returns the
|
exact retry carrying the same `Idempotency-Key` is a no-op and returns the
|
||||||
original transition record; reusing that key for a different action or option
|
original transition record; reusing that key for a different action or option
|
||||||
is rejected. Without an idempotency identity, a repeated transition is invalid
|
is rejected. A command whose requested lifecycle state already holds is also
|
||||||
except for the deliberately auditable `decided` → `decided` action.
|
an idempotent domain no-op, even without an idempotency identity. It returns
|
||||||
|
the current Poll with `replayed=true` and `transition=null`, does not append a
|
||||||
|
lifecycle audit record, and is emitted only to operational logs as duplicate
|
||||||
|
command telemetry. A new idempotency key supplied for such an audit-free no-op
|
||||||
|
is not consumed. Re-deciding with a different option remains a new auditable
|
||||||
|
action; repeating the current decision is a no-op.
|
||||||
|
|
||||||
Poll API representations expose every lifecycle action with its availability
|
Poll API representations expose every lifecycle action with its availability
|
||||||
and, when unavailable, the policy reason. Management clients can use the
|
and, when unavailable, the policy reason. Management clients can use the
|
||||||
|
|||||||
@@ -100,8 +100,12 @@ def _poll_response(poll) -> PollResponse:
|
|||||||
def _transition_status_response(result) -> PollStatusResponse:
|
def _transition_status_response(result) -> PollStatusResponse:
|
||||||
return PollStatusResponse(
|
return PollStatusResponse(
|
||||||
poll=_poll_response(result.poll),
|
poll=_poll_response(result.poll),
|
||||||
transition=PollLifecycleTransitionResponse.model_validate(
|
transition=(
|
||||||
|
PollLifecycleTransitionResponse.model_validate(
|
||||||
poll_lifecycle_transition_response(result.transition)
|
poll_lifecycle_transition_response(result.transition)
|
||||||
|
)
|
||||||
|
if result.transition is not None
|
||||||
|
else None
|
||||||
),
|
),
|
||||||
replayed=result.replayed,
|
replayed=result.replayed,
|
||||||
)
|
)
|
||||||
|
|||||||
@@ -1,6 +1,7 @@
|
|||||||
from __future__ import annotations
|
from __future__ import annotations
|
||||||
|
|
||||||
import hashlib
|
import hashlib
|
||||||
|
import logging
|
||||||
import re
|
import re
|
||||||
import secrets
|
import secrets
|
||||||
from dataclasses import dataclass
|
from dataclasses import dataclass
|
||||||
@@ -24,6 +25,9 @@ from govoplan_poll.backend.schemas import (
|
|||||||
from govoplan_poll.backend.transitions import DEFAULT_POLL_TRANSITION_ENGINE, PollTransitionEngine
|
from govoplan_poll.backend.transitions import DEFAULT_POLL_TRANSITION_ENGINE, PollTransitionEngine
|
||||||
|
|
||||||
|
|
||||||
|
logger = logging.getLogger("govoplan.poll.lifecycle")
|
||||||
|
|
||||||
|
|
||||||
class PollError(ValueError):
|
class PollError(ValueError):
|
||||||
pass
|
pass
|
||||||
|
|
||||||
@@ -67,7 +71,7 @@ YES_NO_MAYBE_OPTIONS = (
|
|||||||
@dataclass(frozen=True)
|
@dataclass(frozen=True)
|
||||||
class PollTransitionResult:
|
class PollTransitionResult:
|
||||||
poll: Poll
|
poll: Poll
|
||||||
transition: PollLifecycleTransition
|
transition: PollLifecycleTransition | None
|
||||||
replayed: bool = False
|
replayed: bool = False
|
||||||
|
|
||||||
|
|
||||||
@@ -721,6 +725,77 @@ def _replayed_transition(
|
|||||||
return PollTransitionResult(poll=poll, transition=existing, replayed=True)
|
return PollTransitionResult(poll=poll, transition=existing, replayed=True)
|
||||||
|
|
||||||
|
|
||||||
|
def _latest_lifecycle_transition(
|
||||||
|
session: Session,
|
||||||
|
*,
|
||||||
|
poll_id: str,
|
||||||
|
) -> PollLifecycleTransition | None:
|
||||||
|
return (
|
||||||
|
session.query(PollLifecycleTransition)
|
||||||
|
.filter(PollLifecycleTransition.poll_id == poll_id)
|
||||||
|
.order_by(
|
||||||
|
PollLifecycleTransition.created_at.desc(),
|
||||||
|
PollLifecycleTransition.id.desc(),
|
||||||
|
)
|
||||||
|
.first()
|
||||||
|
)
|
||||||
|
|
||||||
|
|
||||||
|
def _is_exact_transition_noop(
|
||||||
|
session: Session,
|
||||||
|
*,
|
||||||
|
poll: Poll,
|
||||||
|
action: str,
|
||||||
|
decision_option: PollOption | None,
|
||||||
|
transition_engine: PollTransitionEngine,
|
||||||
|
) -> bool:
|
||||||
|
"""Recognize commands whose requested lifecycle state already holds."""
|
||||||
|
|
||||||
|
rule = transition_engine.policy.rules.get(action)
|
||||||
|
if rule is None:
|
||||||
|
return False
|
||||||
|
if action == "decide":
|
||||||
|
return (
|
||||||
|
rule.target_status == poll.status
|
||||||
|
and decision_option is not None
|
||||||
|
and decision_option.id == poll.decided_option_id
|
||||||
|
)
|
||||||
|
if rule.restore_archived_status:
|
||||||
|
latest = _latest_lifecycle_transition(session, poll_id=poll.id)
|
||||||
|
return (
|
||||||
|
latest is not None
|
||||||
|
and latest.action == action
|
||||||
|
and latest.to_status == poll.status
|
||||||
|
)
|
||||||
|
return rule.target_status == poll.status
|
||||||
|
|
||||||
|
|
||||||
|
def _log_duplicate_transition(
|
||||||
|
*,
|
||||||
|
poll: Poll,
|
||||||
|
action: str,
|
||||||
|
actor_user_id: str | None,
|
||||||
|
actor_api_key_id: str | None,
|
||||||
|
reason: str,
|
||||||
|
has_idempotency_key: bool,
|
||||||
|
) -> None:
|
||||||
|
"""Record command duplication as operational telemetry, not lifecycle audit."""
|
||||||
|
|
||||||
|
logger.info(
|
||||||
|
"Ignored repeated exact Poll lifecycle transition",
|
||||||
|
extra={
|
||||||
|
"event": "poll.lifecycle_transition.duplicate",
|
||||||
|
"tenant_id": poll.tenant_id,
|
||||||
|
"poll_id": poll.id,
|
||||||
|
"transition_action": action,
|
||||||
|
"actor_user_id": actor_user_id,
|
||||||
|
"actor_api_key_id": actor_api_key_id,
|
||||||
|
"duplicate_reason": reason,
|
||||||
|
"has_idempotency_key": has_idempotency_key,
|
||||||
|
},
|
||||||
|
)
|
||||||
|
|
||||||
|
|
||||||
def _clear_expired_close_for_open(poll: Poll, *, action: str, now: datetime) -> datetime | None:
|
def _clear_expired_close_for_open(poll: Poll, *, action: str, now: datetime) -> datetime | None:
|
||||||
if action != "open" or poll.closes_at is None:
|
if action != "open" or poll.closes_at is None:
|
||||||
return None
|
return None
|
||||||
@@ -793,7 +868,7 @@ def transition_poll(
|
|||||||
mutation_owner: PollMutationOwner | None = None,
|
mutation_owner: PollMutationOwner | None = None,
|
||||||
transition_engine: PollTransitionEngine = DEFAULT_POLL_TRANSITION_ENGINE,
|
transition_engine: PollTransitionEngine = DEFAULT_POLL_TRANSITION_ENGINE,
|
||||||
) -> PollTransitionResult:
|
) -> PollTransitionResult:
|
||||||
"""Apply one policy-controlled transition and append its durable audit record."""
|
"""Apply a policy transition, or return an audit-free exact no-op."""
|
||||||
|
|
||||||
poll = _lock_poll_for_transition(session, tenant_id=tenant_id, poll_id=poll_id)
|
poll = _lock_poll_for_transition(session, tenant_id=tenant_id, poll_id=poll_id)
|
||||||
_assert_poll_mutation_owner(poll, mutation_owner=mutation_owner)
|
_assert_poll_mutation_owner(poll, mutation_owner=mutation_owner)
|
||||||
@@ -817,8 +892,33 @@ def transition_poll(
|
|||||||
decision_option=decision_option,
|
decision_option=decision_option,
|
||||||
)
|
)
|
||||||
if replayed is not None:
|
if replayed is not None:
|
||||||
|
_log_duplicate_transition(
|
||||||
|
poll=poll,
|
||||||
|
action=action,
|
||||||
|
actor_user_id=actor_user_id,
|
||||||
|
actor_api_key_id=actor_api_key_id,
|
||||||
|
reason="idempotency_key_replay",
|
||||||
|
has_idempotency_key=True,
|
||||||
|
)
|
||||||
return replayed
|
return replayed
|
||||||
|
|
||||||
|
if _is_exact_transition_noop(
|
||||||
|
session,
|
||||||
|
poll=poll,
|
||||||
|
action=action,
|
||||||
|
decision_option=decision_option,
|
||||||
|
transition_engine=transition_engine,
|
||||||
|
):
|
||||||
|
_log_duplicate_transition(
|
||||||
|
poll=poll,
|
||||||
|
action=action,
|
||||||
|
actor_user_id=actor_user_id,
|
||||||
|
actor_api_key_id=actor_api_key_id,
|
||||||
|
reason="requested_state_already_active",
|
||||||
|
has_idempotency_key=normalized_key is not None,
|
||||||
|
)
|
||||||
|
return PollTransitionResult(poll=poll, transition=None, replayed=True)
|
||||||
|
|
||||||
try:
|
try:
|
||||||
plan = transition_engine.plan(
|
plan = transition_engine.plan(
|
||||||
current_status=poll.status,
|
current_status=poll.status,
|
||||||
|
|||||||
@@ -266,12 +266,86 @@ class PollLifecycleTests(unittest.TestCase):
|
|||||||
idempotency_key="decision-2",
|
idempotency_key="decision-2",
|
||||||
)
|
)
|
||||||
|
|
||||||
def test_repeated_exact_transition_without_an_identity_is_rejected(self) -> None:
|
def test_repeated_exact_transition_without_an_identity_is_an_audit_free_noop(self) -> None:
|
||||||
|
poll = self._poll()
|
||||||
|
applied = transition_poll(self.session, tenant_id="tenant-1", poll_id=poll.id, action="open")
|
||||||
|
|
||||||
|
with self.assertLogs("govoplan.poll.lifecycle", level="INFO") as logs:
|
||||||
|
repeated = transition_poll(
|
||||||
|
self.session,
|
||||||
|
tenant_id="tenant-1",
|
||||||
|
poll_id=poll.id,
|
||||||
|
action="open",
|
||||||
|
actor_user_id="owner",
|
||||||
|
)
|
||||||
|
|
||||||
|
self.assertFalse(applied.replayed)
|
||||||
|
self.assertTrue(repeated.replayed)
|
||||||
|
self.assertIsNone(repeated.transition)
|
||||||
|
self.assertEqual(poll.status, "open")
|
||||||
|
self.assertEqual(len(poll.lifecycle_transitions), 1)
|
||||||
|
self.assertIn("Ignored repeated exact Poll lifecycle transition", logs.output[0])
|
||||||
|
|
||||||
|
def test_same_decision_is_a_noop_but_a_changed_decision_is_audited(self) -> None:
|
||||||
poll = self._poll()
|
poll = self._poll()
|
||||||
transition_poll(self.session, tenant_id="tenant-1", poll_id=poll.id, action="open")
|
transition_poll(self.session, tenant_id="tenant-1", poll_id=poll.id, action="open")
|
||||||
|
transition_poll(self.session, tenant_id="tenant-1", poll_id=poll.id, action="close")
|
||||||
|
first = transition_poll(
|
||||||
|
self.session,
|
||||||
|
tenant_id="tenant-1",
|
||||||
|
poll_id=poll.id,
|
||||||
|
action="decide",
|
||||||
|
option_key="yes",
|
||||||
|
)
|
||||||
|
repeated = transition_poll(
|
||||||
|
self.session,
|
||||||
|
tenant_id="tenant-1",
|
||||||
|
poll_id=poll.id,
|
||||||
|
action="decide",
|
||||||
|
option_key="yes",
|
||||||
|
)
|
||||||
|
changed = transition_poll(
|
||||||
|
self.session,
|
||||||
|
tenant_id="tenant-1",
|
||||||
|
poll_id=poll.id,
|
||||||
|
action="decide",
|
||||||
|
option_key="no",
|
||||||
|
)
|
||||||
|
|
||||||
|
self.assertTrue(repeated.replayed)
|
||||||
|
self.assertIsNone(repeated.transition)
|
||||||
|
self.assertIsNotNone(first.transition)
|
||||||
|
self.assertIsNotNone(changed.transition)
|
||||||
|
self.assertEqual(changed.transition.previous_decision_option_id, first.transition.decision_option_id)
|
||||||
|
self.assertEqual(
|
||||||
|
[item.action for item in poll.lifecycle_transitions],
|
||||||
|
["open", "close", "decide", "decide"],
|
||||||
|
)
|
||||||
|
|
||||||
|
def test_exact_unarchive_retry_is_a_noop_but_unarchive_without_history_is_invalid(self) -> None:
|
||||||
|
poll = self._poll()
|
||||||
|
transition_poll(self.session, tenant_id="tenant-1", poll_id=poll.id, action="archive")
|
||||||
|
transition_poll(self.session, tenant_id="tenant-1", poll_id=poll.id, action="unarchive")
|
||||||
|
|
||||||
|
repeated = transition_poll(
|
||||||
|
self.session,
|
||||||
|
tenant_id="tenant-1",
|
||||||
|
poll_id=poll.id,
|
||||||
|
action="unarchive",
|
||||||
|
)
|
||||||
|
|
||||||
|
self.assertTrue(repeated.replayed)
|
||||||
|
self.assertIsNone(repeated.transition)
|
||||||
|
self.assertEqual([item.action for item in poll.lifecycle_transitions], ["archive", "unarchive"])
|
||||||
|
|
||||||
|
never_archived = self._poll(title="Never archived")
|
||||||
with self.assertRaisesRegex(PollError, "not allowed"):
|
with self.assertRaisesRegex(PollError, "not allowed"):
|
||||||
transition_poll(self.session, tenant_id="tenant-1", poll_id=poll.id, action="open")
|
transition_poll(
|
||||||
|
self.session,
|
||||||
|
tenant_id="tenant-1",
|
||||||
|
poll_id=never_archived.id,
|
||||||
|
action="unarchive",
|
||||||
|
)
|
||||||
|
|
||||||
def test_archive_and_unarchive_restore_status_and_append_audit_history(self) -> None:
|
def test_archive_and_unarchive_restore_status_and_append_audit_history(self) -> None:
|
||||||
poll = self._poll()
|
poll = self._poll()
|
||||||
@@ -444,6 +518,7 @@ class PollLifecycleTests(unittest.TestCase):
|
|||||||
self.assertEqual(opened.poll.status, "open")
|
self.assertEqual(opened.poll.status, "open")
|
||||||
self.assertFalse(opened.replayed)
|
self.assertFalse(opened.replayed)
|
||||||
self.assertTrue(replay.replayed)
|
self.assertTrue(replay.replayed)
|
||||||
|
self.assertIsNotNone(replay.transition)
|
||||||
self.assertEqual(len(lifecycle.history), 1)
|
self.assertEqual(len(lifecycle.history), 1)
|
||||||
self.assertEqual(lifecycle.history[0].actor_user_id, "owner-membership")
|
self.assertEqual(lifecycle.history[0].actor_user_id, "owner-membership")
|
||||||
self.assertEqual(
|
self.assertEqual(
|
||||||
@@ -462,6 +537,27 @@ class PollLifecycleTests(unittest.TestCase):
|
|||||||
)
|
)
|
||||||
self.assertEqual(rejected.exception.status_code, 400)
|
self.assertEqual(rejected.exception.status_code, 400)
|
||||||
|
|
||||||
|
def test_generic_api_returns_null_transition_for_domain_noop(self) -> None:
|
||||||
|
poll = self._poll(status="open", title="Already open")
|
||||||
|
|
||||||
|
repeated = api_transition_poll(
|
||||||
|
poll.id,
|
||||||
|
PollTransitionRequest(action="open"),
|
||||||
|
idempotency_key=None,
|
||||||
|
session=self.session,
|
||||||
|
principal=self._principal(),
|
||||||
|
)
|
||||||
|
|
||||||
|
self.assertEqual(repeated.poll.status, "open")
|
||||||
|
self.assertTrue(repeated.replayed)
|
||||||
|
self.assertIsNone(repeated.transition)
|
||||||
|
self.assertEqual(
|
||||||
|
self.session.query(PollLifecycleTransition)
|
||||||
|
.filter(PollLifecycleTransition.poll_id == poll.id)
|
||||||
|
.count(),
|
||||||
|
0,
|
||||||
|
)
|
||||||
|
|
||||||
|
|
||||||
if __name__ == "__main__":
|
if __name__ == "__main__":
|
||||||
unittest.main()
|
unittest.main()
|
||||||
|
|||||||
Reference in New Issue
Block a user