improvements to event notifications & docs

docs/event-notification.md

@@ -9,8 +9,8 @@ with exponential backoff for up to 12 hours.
 
 Due to retries, the event notification endpoint _should be_ idempotent.
 
-Event notifications are _not guaranteed to be ordered_ by event
-creation order, especially when retries occur.
+Initial event notifications for each event are _guaranteed to be in
+order_ of event creation, but retries might be out-of-order.
 
 Event notification failures are logged. You can also inspect the
 `outbox` db table for the retry state of notifications.
@@ -25,9 +25,9 @@ The body of the HTTP PUT request will be a JSON object that contains:
 
 - `"event/type"`: the type of the event, a string
 - `"event/actor"`: who caused this event
+- `"event/id"`: unique event id
 - `"event/time"`: when the event occured
 - `"application/id"`: the id of the application
 - `"event/application"`: the state of the application, in the same format as the `/api/applications/:id/raw` endpoint returns (see Swagger docs)
 
 Other keys may also be present depending on the event type.
-

resources/sql/queries.sql

@@ -485,9 +485,10 @@ FROM application_event
 ORDER BY id DESC
 LIMIT 1;
 
--- :name add-application-event! :insert
+-- :name add-application-event! :? :1
 INSERT INTO application_event (appId, eventData)
-VALUES (:application, :eventdata::jsonb);
+VALUES (:application, :eventdata::jsonb)
+RETURNING id, eventData::TEXT;
 
 -- :name upsert-api-key! :insert
 INSERT INTO api_key (apiKey, comment, permittedRoles)

src/clj/rems/api/services/command.clj

@@ -80,11 +80,10 @@
               (applications/get-application-internal app-id))
         result (commands/handle-command cmd app command-injections)]
     (when-not (:errors result)
-      (doseq [event (:events result)]
-        (events/add-event! event))
-      (doseq [cmd2 (run-process-managers (:events result))]
-        (let [result (command! cmd2)]
-          (when (:errors result)
-            (log/error "process manager command failed"
-                       (pr-str {:cmd cmd2 :result result :parent-cmd cmd}))))))
+      (let [events-from-db (mapv events/add-event! (:events result))]
+        (doseq [cmd2 (run-process-managers events-from-db)]
+          (let [result (command! cmd2)]
+            (when (:errors result)
+              (log/error "process manager command failed"
+                         (pr-str {:cmd cmd2 :result result :parent-cmd cmd})))))))
     result))

src/clj/rems/db/events.clj

@@ -41,7 +41,8 @@
 (defn get-latest-event []
   (fix-event-from-db (db/get-latest-application-event {})))
 
-(defn add-event! [event]
-  (db/add-application-event! {:application (:application/id event)
-                              :eventdata (event->json event)})
-  nil)
+(defn add-event!
+  "Add event to database. Returns the event as it went into the db."
+  [event]
+  (fix-event-from-db (db/add-application-event! {:application (:application/id event)
+                                                       :eventdata (event->json event)})))

src/clj/rems/event_notification.clj

@@ -15,7 +15,7 @@
 (def ^:private default-timeout 60)
 
 (defn- notify! [target body]
-  (log/info "Sending event notification for event" (select-keys body [:application/id :event/type :event/time])
+  (log/info "Sending event notification for event" (select-keys body [:event/id :application/id :event/type :event/time])
             "to" (:url target))
   (try
     (let [timeout-ms (* 1000 (get target :timeout default-timeout))
@@ -35,6 +35,11 @@
       "failed: exception")))
 
 (defn process-outbox! []
+  ;; TODO if we want to guarantee event ordering, we need fetch all
+  ;; outbox entries here, and pick the one with the lowest outbox id
+  ;; or event id, and do nothing if it isn't due yet.
+  ;;
+  ;; This can be done per target url or globally.
   (doseq [entry (outbox/get-due-entries :event-notification)]
     (if-let [error (notify! (get-in entry [:outbox/event-notification :target])
                             (get-in entry [:outbox/event-notification :body]))]

test/clj/rems/test_event_notification.clj

@@ -5,6 +5,7 @@
             [rems.config]
             [rems.api.services.command :as command]
             [rems.api.testing :refer [api-fixture api-call]]
+            [rems.db.events]
             [rems.db.test-data :as test-data]
             [rems.event-notification :as event-notification]
             [rems.json :as json]
@@ -75,7 +76,8 @@
             app-id (:application-id (command/command! {:type :application.command/create
                                                        :actor applicant
                                                        :time (time/date-time 2001)
-                                                       :catalogue-item-ids [cat-id]}))]
+                                                       :catalogue-item-ids [cat-id]}))
+            event-id (:event/id (first (rems.db.events/get-application-events app-id)))]
         (testing "no notifications before outbox is processed"
           (is (empty? (stub/recorded-requests server))))
         (event-notification/process-outbox!)
@@ -88,6 +90,7 @@
                    (set (map :path notifications))))
             (is (= {:application/external-id "2001/1"
                     :application/id app-id
+                    :event/id event-id
                     :event/time "2001-01-01T00:00:00.000Z"
                     :workflow/type "workflow/default"
                     :application/resources [{:resource/ext-id ext-id
@@ -114,3 +117,36 @@
             (is (= "/all" (:path req)))
             (is (= "application.event/draft-saved"
                    (:event/type (:data req))))))))))
+
+(deftest test-event-notification-ordering
+  (with-open [server (stub/start! {"/" {:status 200}})]
+    (with-redefs [rems.config/env (assoc rems.config/env
+                                         :event-notification-targets [{:url (str (:uri server) "/")}])]
+      (let [get-notifications #(doall
+                                (for [r (stub/recorded-requests server)]
+                                  (-> r
+                                      :body
+                                      (get "content")
+                                      json/parse-string
+                                      (select-keys [:event/id :event/time]))))
+            form-id (test-data/create-form! {:form/title "notifications"
+                                             :form/fields [{:field/type :text
+                                                            :field/id "field-1"
+                                                            :field/title {:en "text field"}
+                                                            :field/optional false}]})
+            cat-id (test-data/create-catalogue-item! {:form-id form-id})
+            applicant "alice"
+            t (time/date-time 2010)
+            app-id (:application-id (command/command! {:type :application.command/create
+                                                       :actor applicant
+                                                       :time t
+                                                       :catalogue-item-ids [cat-id]}))]
+        (dotimes [i 100]
+          (command/command! {:type :application.command/save-draft
+                             :actor applicant
+                             :application-id app-id
+                             :time (time/plus t (time/seconds i))
+                             :field-values [{:form form-id :field "field-1" :value (str i)}]}))
+        (event-notification/process-outbox!)
+        (let [notifications (get-notifications)]
+          (is (apply < (mapv :event/id notifications))))))))