Merge pull request #2138 from CSCfi/event-notify-2095

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! :returning-execute :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))))))))