diff --git a/src/main/com/yetanalytics/lrs/pedestal/interceptor.cljc b/src/main/com/yetanalytics/lrs/pedestal/interceptor.cljc index cefed053..f851ebed 100644 --- a/src/main/com/yetanalytics/lrs/pedestal/interceptor.cljc +++ b/src/main/com/yetanalytics/lrs/pedestal/interceptor.cljc @@ -10,6 +10,7 @@ [io.pedestal.http.body-params :as body-params] [io.pedestal.http.ring-middlewares :as middlewares] [com.yetanalytics.lrs.pedestal.interceptor.xapi :as xapi] + [com.yetanalytics.lrs.xapi.document :as doc] [com.yetanalytics.lrs.util.hash :refer [sha-1]] [com.yetanalytics.lrs.pedestal.interceptor.xapi.statements :as si] [xapi-schema.spec :as xs :include-macros true] @@ -182,13 +183,12 @@ (defn calculate-etag [x] (sha-1 x)) -;; TODO: handle weak etags (def etag-string-pattern - #"\w+") + doc/etag-string-pattern) (defn etag-header->etag-set [etag-header] - (into #{} (re-seq etag-string-pattern etag-header))) + (doc/etag-header->etag-set etag-header)) (defn- quote-etag [etag] (str "\"" etag "\"")) diff --git a/src/main/com/yetanalytics/lrs/pedestal/routes/documents.cljc b/src/main/com/yetanalytics/lrs/pedestal/routes/documents.cljc index f7074791..837adcb5 100644 --- a/src/main/com/yetanalytics/lrs/pedestal/routes/documents.cljc +++ b/src/main/com/yetanalytics/lrs/pedestal/routes/documents.cljc @@ -4,6 +4,7 @@ [com.yetanalytics.lrs.protocol :as p] [com.yetanalytics.lrs.pedestal.interceptor :as i] [com.yetanalytics.lrs.pedestal.interceptor.xapi :as xi] + [com.yetanalytics.lrs.xapi.document :as doc] [clojure.spec.alpha :as s :include-macros true] [clojure.core.async :as a :include-macros true] #?(:clj [cheshire.core :as json]))) @@ -155,39 +156,24 @@ :xapi.agents.profile.GET.request/params)))) (defn etags-preproc - "Process if-match rules and etags for the handler. Will call `handle-get` - to check doc state." + "Normalize ETag preconditions for document mutations. Implementations that + opt into atomic validation receive them directly; other implementations use + the preliminary `handle-get` check before receiving them." [enter-fn] (fn wrap-enter [{:keys [xapi request com.yetanalytics/lrs] :as ctx}] - (let [;; Destructuring - {:keys [headers]} request - ;; VSCode incorrectly marks `if-match` and `if-none-match` as - ;; if macros - {hif-match "if-match" - hif-none-match "if-none-match"} headers - ;; Helper fns - hif-match-ok? - (fn [ctx hif-match] - (case hif-match - nil true - "*" (= 200 (get-in ctx [:response :status])) - ;; else - (contains? (i/etag-header->etag-set hif-match) - (::i/etag ctx)))) - hif-none-match-ok? - (fn [ctx hif-none-match] - (case hif-none-match - nil true - "*" (= 404 (get-in ctx [:response :status])) - ;; else - (not (contains? (i/etag-header->etag-set hif-none-match) - (::i/etag ctx)))))] - (if (= nil hif-match hif-none-match) - ;; If no headers provided, go ahead - (enter-fn ctx) + (let [preconditions (doc/parse-etag-preconditions + (get request :headers)) + operation-ctx (cond-> ctx + (seq preconditions) + (assoc ::doc/preconditions preconditions))] + (if (or (empty? preconditions) + (p/atomic-document-preconditions? lrs)) + ;; No condition to validate, or the implementation validates it + ;; authoritatively while applying the mutation. + (enter-fn operation-ctx) (let [;; TODO: Params overhaul, very silly rn get-params-enter (get-params-enter-fn xapi) {get-enter :enter @@ -202,10 +188,14 @@ get-params-enter get-enter a/etag-set + "Parse an ETag header value into a set of unquoted ETags." + [etag-header] + (into #{} (re-seq etag-string-pattern etag-header))) + +(s/def ::etag-condition + (s/or :wildcard #{:*} + :etags (s/coll-of string? :kind set?))) + +(s/def ::if-match ::etag-condition) +(s/def ::if-none-match ::etag-condition) + +(s/def ::preconditions + (s/keys :opt-un [::if-match ::if-none-match])) + +(defn parse-etag-preconditions + "Parse If-Match and If-None-Match request headers into normalized + preconditions. Wildcards are represented by `:*`; other values are sets of + unquoted ETags." + [{if-match "if-match" + if-none-match "if-none-match"}] + (cond-> {} + if-match + (assoc :if-match (if (= "*" if-match) + :* + (etag-header->etag-set if-match))) + if-none-match + (assoc :if-none-match (if (= "*" if-none-match) + :* + (etag-header->etag-set if-none-match))))) + +(defn etag-preconditions-met? + "Return true when normalized ETag `preconditions` are satisfied by the + current resource state. `exists?` indicates whether the resource exists; + `etag` is its unquoted ETag when available." + [{:keys [if-match if-none-match]} + {:keys [exists? etag]}] + (and (case if-match + nil true + :* exists? + (contains? if-match etag)) + (case if-none-match + nil true + :* (not exists?) + (not (contains? if-none-match etag))))) + +(defn precondition-failed-error + "Return a document operation error indicating that an ETag precondition + failed. Optional `data` is included in the exception data." + ([] + (precondition-failed-error {})) + ([data] + {:error (ex-info "Document precondition failed" + (assoc data :type ::precondition-failed))})) + +(defn precondition-failed? + "Return true when `error` represents a document precondition failure." + [error] + (= ::precondition-failed (:type (ex-data error)))) + (defn updated-stamp [document] (or diff --git a/src/test/com/yetanalytics/lrs/impl/memory_test.cljc b/src/test/com/yetanalytics/lrs/impl/memory_test.cljc index cdce7574..9fdfa3c7 100644 --- a/src/test/com/yetanalytics/lrs/impl/memory_test.cljc +++ b/src/test/com/yetanalytics/lrs/impl/memory_test.cljc @@ -2,7 +2,8 @@ (:require [clojure.test :as test :refer [deftest is] :include-macros true] [clojure.spec.test.alpha :as stest :include-macros true] [com.yetanalytics.test-support :refer [failures stc-opts]] - [com.yetanalytics.lrs.impl.memory :as mem])) + [com.yetanalytics.lrs.impl.memory :as mem] + [com.yetanalytics.lrs.protocol :as p])) (deftest store-ref-test (is (empty? @@ -95,4 +96,8 @@ (is (empty? (failures (stest/check `mem/new-lrs - {stc-opts {:num-tests 1}}))))) + {stc-opts {:num-tests 1}})))) + (doseq [mode [:sync :async :both]] + (is (false? (p/atomic-document-preconditions? + (mem/new-lrs {:mode mode}))) + (str "memory LRS mode " (name mode) " remains unopted")))) diff --git a/src/test/com/yetanalytics/lrs/pedestal/routes/documents_test.cljc b/src/test/com/yetanalytics/lrs/pedestal/routes/documents_test.cljc new file mode 100644 index 00000000..5cecce1e --- /dev/null +++ b/src/test/com/yetanalytics/lrs/pedestal/routes/documents_test.cljc @@ -0,0 +1,137 @@ +(ns com.yetanalytics.lrs.pedestal.routes.documents-test + (:require [clojure.test :refer [deftest is testing] :include-macros true] + [clojure.core.async :as a :include-macros true] + [clojure.spec.alpha :as s :include-macros true] + [com.yetanalytics.test-support :as support] + [com.yetanalytics.lrs.pedestal.routes.documents :as routes] + [com.yetanalytics.lrs.protocol :as p] + [com.yetanalytics.lrs.xapi.document :as doc])) + +(def ctx + {:context-value ::preserved}) + +(def unexpected-error + (ex-info "Unexpected document error" {:type ::unexpected})) + +(defn- atomic-sync-lrs + [enabled?] + (reify + p/AtomicDocumentPreconditions + (-atomic-document-preconditions? [_] + enabled?) + p/DocumentResource + (-set-document [_ _ _ _ _ _] {}) + (-get-document [_ _ _ _] + (throw (ex-info "Preliminary GET must be skipped" {}))) + (-get-document-ids [_ _ _ _] + (throw (ex-info "Preliminary GET must be skipped" {}))) + (-delete-document [_ _ _ _] {}) + (-delete-documents [_ _ _ _] {}))) + +(defn- atomic-async-lrs + [enabled?] + (reify + p/AtomicDocumentPreconditions + (-atomic-document-preconditions? [_] + enabled?) + p/DocumentResourceAsync + (-set-document-async [_ _ _ _ _ _] (a/go {})) + (-get-document-async [_ _ _ _] + (throw (ex-info "Preliminary GET must be skipped" {}))) + (-get-document-ids-async [_ _ _ _] + (throw (ex-info "Preliminary GET must be skipped" {}))) + (-delete-document-async [_ _ _ _] (a/go {})) + (-delete-documents-async [_ _ _ _] (a/go {})))) + +(deftest atomic-document-preconditions-capability-test + (testing "unimplemented and disabled capabilities are false" + (is (false? (p/atomic-document-preconditions? nil))) + (is (false? (p/atomic-document-preconditions? + (atomic-sync-lrs false))))) + (testing "enabled synchronous and asynchronous capabilities are true" + (is (true? (p/atomic-document-preconditions? + (atomic-sync-lrs true)))) + (is (true? (p/atomic-document-preconditions? + (atomic-async-lrs true)))))) + +(deftest atomic-etag-precondition-handoff-test + (let [preconditions {:if-match #{"abc" "def"} + :if-none-match :*} + request {:headers {"if-match" "\"abc\", \"def\"" + "if-none-match" "*"}} + enter-fn (fn [ctx] + (assoc ctx :response + {:preconditions (::doc/preconditions ctx)}))] + (testing "synchronous implementation skips preliminary GET" + (let [result ((routes/etags-preproc enter-fn) + {:request request + :com.yetanalytics/lrs (atomic-sync-lrs true)})] + (is (= preconditions + (get-in result [:response :preconditions]))))) + (testing "asynchronous implementation skips preliminary GET" + (support/test-async + (a/go + (let [result (a/