From 2744a5f865046e6b39d8811ac1cd1c73dc32f143 Mon Sep 17 00:00:00 2001 From: Joel Kaasinen Date: Mon, 20 Apr 2020 14:09:44 +0300 Subject: [PATCH 1/3] feat: send :event/id in event notifications by roundtripping events via the db before giving them to process managers --- docs/event-notification.md | 1 + resources/sql/queries.sql | 5 +++-- src/clj/rems/api/services/command.clj | 13 ++++++------- src/clj/rems/db/events.clj | 9 +++++---- src/clj/rems/event_notification.clj | 2 +- test/clj/rems/test_event_notification.clj | 5 ++++- 6 files changed, 20 insertions(+), 15 deletions(-) diff --git a/docs/event-notification.md b/docs/event-notification.md index 8ec97e7a25..fda2e7e4f9 100644 --- a/docs/event-notification.md +++ b/docs/event-notification.md @@ -25,6 +25,7 @@ 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) diff --git a/resources/sql/queries.sql b/resources/sql/queries.sql index ba3cc8e0c3..4c3aff418d 100644 --- a/resources/sql/queries.sql +++ b/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) diff --git a/src/clj/rems/api/services/command.clj b/src/clj/rems/api/services/command.clj index 27447aff60..675ab1124c 100644 --- a/src/clj/rems/api/services/command.clj +++ b/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)) diff --git a/src/clj/rems/db/events.clj b/src/clj/rems/db/events.clj index ff51055113..1bb4846062 100644 --- a/src/clj/rems/db/events.clj +++ b/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)}))) diff --git a/src/clj/rems/event_notification.clj b/src/clj/rems/event_notification.clj index f066c2a1f1..12845b5423 100644 --- a/src/clj/rems/event_notification.clj +++ b/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)) diff --git a/test/clj/rems/test_event_notification.clj b/test/clj/rems/test_event_notification.clj index 93a6149ef3..46a3aa2a85 100644 --- a/test/clj/rems/test_event_notification.clj +++ b/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 From e64613085064ba3390715994036f4fa7886df8e2 Mon Sep 17 00:00:00 2001 From: Joel Kaasinen Date: Mon, 20 Apr 2020 14:45:14 +0300 Subject: [PATCH 2/3] doc: clarify & test event notification ordering --- docs/event-notification.md | 5 ++-- src/clj/rems/event_notification.clj | 5 ++++ test/clj/rems/test_event_notification.clj | 33 +++++++++++++++++++++++ 3 files changed, 40 insertions(+), 3 deletions(-) diff --git a/docs/event-notification.md b/docs/event-notification.md index fda2e7e4f9..7fdcf959b8 100644 --- a/docs/event-notification.md +++ b/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. @@ -31,4 +31,3 @@ The body of the HTTP PUT request will be a JSON object that contains: - `"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. - diff --git a/src/clj/rems/event_notification.clj b/src/clj/rems/event_notification.clj index 12845b5423..52b532b8f6 100644 --- a/src/clj/rems/event_notification.clj +++ b/src/clj/rems/event_notification.clj @@ -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]))] diff --git a/test/clj/rems/test_event_notification.clj b/test/clj/rems/test_event_notification.clj index 46a3aa2a85..d98b165fe3 100644 --- a/test/clj/rems/test_event_notification.clj +++ b/test/clj/rems/test_event_notification.clj @@ -117,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)))))))) From 554cd23ef77719003de411524e9028ae236131b7 Mon Sep 17 00:00:00 2001 From: Joel Kaasinen Date: Tue, 21 Apr 2020 17:40:16 +0300 Subject: [PATCH 3/3] refactor: use :returning-execute instead of :? for INSERT RETURNING --- resources/sql/queries.sql | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/resources/sql/queries.sql b/resources/sql/queries.sql index 4c3aff418d..1ac10f7afb 100644 --- a/resources/sql/queries.sql +++ b/resources/sql/queries.sql @@ -485,7 +485,7 @@ FROM application_event ORDER BY id DESC LIMIT 1; --- :name add-application-event! :? :1 +-- :name add-application-event! :returning-execute :1 INSERT INTO application_event (appId, eventData) VALUES (:application, :eventdata::jsonb) RETURNING id, eventData::TEXT;