From 40c83532c0dad968e0550b0aa19b11bdad8c4f78 Mon Sep 17 00:00:00 2001 From: Taimoor Ahmed Date: Tue, 31 Mar 2026 11:37:54 +0500 Subject: [PATCH] docs: Add ADR for ensuring GET requests are idempotent Add edx-platform/docs/decisions/0030-ensure-get-requests-are-idempotent.rst as an accepted ADR. Define policy that GET endpoints must be strictly read-only, with side effects moved to explicit write endpoints or async event pipelines. Include edx-platform relevance, anti-pattern vs preferred code examples, and rollout guidance for testing and migration. --- ...030-ensure-get-requests-are-idempotent.rst | 122 ++++++++++++++++++ 1 file changed, 122 insertions(+) create mode 100644 docs/decisions/0030-ensure-get-requests-are-idempotent.rst diff --git a/docs/decisions/0030-ensure-get-requests-are-idempotent.rst b/docs/decisions/0030-ensure-get-requests-are-idempotent.rst new file mode 100644 index 000000000000..2f072564637c --- /dev/null +++ b/docs/decisions/0030-ensure-get-requests-are-idempotent.rst @@ -0,0 +1,122 @@ +Ensure GET is Idempotent +======================== + +:Status: Proposed +:Date: 2026-03-31 +:Deciders: API Working Group + +Context +======= + +Some Open edX endpoints use ``GET`` requests that have side-effects (e.g., firing +openedx-events, triggering Django signals, writing tracking logs, recording first access +events). This violates REST safety/idempotency expectations and can break +caching/proxy behavior and automated clients/agents. + +Decision +======== + +1. Treat ``GET`` as strictly read-only with respect to **domain state**: a ``GET`` handler + must not create, update, or delete records that are part of the transactional domain model + (e.g. enrollments, grades, user profile fields). +2. Move domain-state-mutating side-effects out of ``GET`` handlers: + + * **openedx-events and Django signals must not be fired from ``GET`` handlers.** + These are the primary concern: signal receivers may perform writes, trigger + downstream workflows, or update domain state in ways that are invisible to the + ``GET`` handler itself. + * Create explicit write endpoints (``POST``, ``PUT``, ``PATCH``) for state changes, + including any side-effects that need to emit openedx-events or Django signals. + * Simple telemetry writes to a **separate analytics store** (e.g. ``tracker.emit``, + Segment events, read-count increments) are acceptable inside a ``GET`` handler + **provided** the response content does not depend on them and no openedx-events + or Django signals are involved. These writes do not need to be moved to + async pipelines unless there is a specific performance or reliability reason to do so. + +3. Add regression tests to ensure ``GET`` handlers do not modify domain state. +4. Document exceptions (if any) and provide migration notes for clients. + +Relevance in edx-platform +========================= + +* **openedx-events and Django signals on read**: The primary concern is ``GET`` + handlers that fire openedx-events (e.g. ``COURSE_ENROLLMENT_CREATED``, + ``STUDENT_REGISTRATION_COMPLETED``) or Django signals (e.g. ``post_save``, + ``m2m_changed``) as a side-effect. Receivers of these events/signals can trigger + domain-state mutations that are invisible to the ``GET`` handler, making the + request non-idempotent in ways that are difficult to audit. These must be moved + to explicit write endpoints. +* **GET used with side-effects**: Various views use ``@require_GET`` while + triggering writes (e.g. tracking, first-access, or logging). Discussion views + (``lms/djangoapps/discussion/views.py``) use ``@require_GET`` for thread/topic + listing; any implicit domain-state mutation on read should be moved to separate + endpoints or async events. +* **Legacy analytics on read**: ``common/djangoapps/student`` and courseware code + sometimes emit pure analytics events (e.g. ``tracker.emit``, streak updates) in + code paths triggered by GET. Pure telemetry that does not affect domain state and + does not involve openedx-events or Django signals may remain, but anything that + can cause downstream domain writes must be decoupled. + +Code example +============ + +**Anti-pattern (GET that fires an openedx-event):** + +.. code-block:: python + + @require_GET + def get_enrollment(request, course_id): + # BAD: firing an openedx-event from a GET handler; receivers may + # perform domain-state writes invisible to this handler. + COURSE_ENROLLMENT_CHANGED.send_event( + enrollment=EnrollmentData(user=request.user, course_key=course_id) + ) + return JsonResponse(fetch_enrollment_data(...)) + +**Preferred: read-only GET + explicit write endpoint for state-changing events** + +.. code-block:: python + + @require_GET + def get_enrollment(request, course_id): + # GOOD: pure read, no signals or openedx-events fired + return Response(EnrollmentSerializer(fetch_enrollment_data(...)).data) + + @require_POST + def track_enrollment_event(request, course_id): + # Explicit write endpoint; openedx-event fired safely on POST + COURSE_ENROLLMENT_CHANGED.send_event( + enrollment=EnrollmentData(user=request.user, course_key=course_id) + ) + return Response(status=204) + +Consequences +============ + +* Pros + + * REST-compliant behavior; safer automated consumption (AI agents, integrations). + * Predictable caching/proxy semantics. + * Prevents unintended downstream side-effects from read operations (e.g. duplicate + event emissions when a response is served from cache without hitting the handler). + +* Cons / Costs + + * Requires refactoring legacy courseware/analytics endpoints that currently fire + openedx-events or Django signals on read. + * Potential behavior changes for internal systems that relied on implicit GET-triggered + events or signals. + +Implementation Notes +==================== + +* Inventory endpoints with GET side-effects, paying particular attention to those + that fire openedx-events or Django signals. +* For each, define a read-only GET representation and a separate write/track endpoint + (or async event emission) if needed. + +References +========== + +* "Non-Idempotent GET Requests" recommendation in the Open edX REST API standardization notes. +* `openedx-events `_ — Open edX architectural events.