[SQL-239] from parameter validation

src/main/lrsql/spec/statement.clj

@@ -2,7 +2,8 @@
   (:require [clojure.spec.alpha :as s]
             [clojure.spec.gen.alpha :as sgen]
             [xapi-schema.spec :as xs]
-            [com.yetanalytics.lrs.protocol :as lrsp]
+            [xapi-schema.spec.resources :as xsr]
+            [com.yetanalytics.lrs.spec.common :as sc]
             [com.yetanalytics.lrs.xapi.statements :as ss]
             [lrsql.backend.protocol :as bp]
             [lrsql.spec.common     :as c]
@@ -171,8 +172,56 @@
 ;; Query params specs
 ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
 
+;; The `:from` param is a custom parameter that is used by SQL LRS. However,
+;; since it is not a param specified in the xAPI. Because of how the spec is
+;; structured, however, we basically need to redefine the entire the entire
+;; params spec just to add a new spec. Fortunately, though, it is easy to
+;; redefine specs thanks to how spec keywords work.
+
+;; The spec definitions are based on xapi-schema.resources:
+;; https://github.com/yetanalytics/xapi-schema/blob/master/src/xapi_schema/spec/resources.cljc#L65-L157
+
+(s/def :xapi.statements.GET.request.params/from
+  ::xs/uuid)
+
+(defmulti query-type
+  #(if (xsr/singular-query? %)
+     :xapi.statements.GET.request.params/singular
+     :xapi.statements.GET.request.params/multiple))
+
+(defmethod query-type :xapi.statements.GET.request.params/singular [_]
+  (s/keys :req-un [(or :xapi.statements.GET.request.params/statementId
+                       :xapi.statements.GET.request.params/voidedStatementId)]
+          :opt-un [:xapi.statements.GET.request.params/format
+                   :xapi.statements.GET.request.params/attachments]))
+
+(defmethod query-type :xapi.statements.GET.request.params/multiple [_]
+  (s/keys :opt-un [:xapi.statements.GET.request.params/agent
+                   :xapi.statements.GET.request.params/verb
+                   :xapi.statements.GET.request.params/activity
+                   :xapi.statements.GET.request.params/registration
+                   :xapi.statements.GET.request.params/related_activities
+                   :xapi.statements.GET.request.params/related_agents
+                   :xapi.statements.GET.request.params/since
+                   :xapi.statements.GET.request.params/until
+                   :xapi.statements.GET.request.params/limit
+                   :xapi.statements.GET.request.params/format
+                   :xapi.statements.GET.request.params/attachments
+                   :xapi.statements.GET.request.params/ascending
+                   ;; NEW SPEC
+                   :xapi.statements.GET.request.params/from]))
+
+(s/def :xapi.statements.GET.request/params
+  (s/multi-spec query-type (fn [gen-val _] gen-val)))
+
+;; See: :com.yetanalytics.lrs.protocol/get-statement-params
+(s/def ::get-statement-params
+  (sc/with-conform-gen :xapi.statements.GET.request/params))
+
+;; This is to instrument functions, not for production-time validation,
+;; which is handled by the `:xapi.statements.GET.request/params` spec.
 (s/def ::query-params
-  (s/merge ::lrsp/get-statements-params
+  (s/merge ::get-statement-params
            (s/keys :req-un [::limit])))
 
 ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;

src/test/lrsql/params_test.clj

@@ -0,0 +1,65 @@
+(ns lrsql.params-test
+  "REST API parameter tests"
+  (:require [clojure.test :refer [deftest testing is use-fixtures]]
+            [babashka.curl :as curl]
+            [com.stuartsierra.component :as component]
+            [lrsql.test-support :as support])
+  (:import [clojure.lang ExceptionInfo]))
+
+;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
+;; Init
+;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
+
+(def get-map
+  {:headers {"Content-Type"             "application/json"
+             "X-Experience-API-Version" "1.0.3"}
+   :basic-auth ["username" "password"]})
+
+(support/instrument-lrsql)
+
+(use-fixtures :each support/fresh-db-fixture)
+
+;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
+;; Tests
+;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
+
+;; There are three non-spec query params that are defined in the upstream lrs
+;; lib: `page`, `from`, and `unwrap_html`.
+;; `from` is actively used and needs to be validated, `unwrap_html` is used by
+;; the UI but not the backend, and `page` is completely unused.
+;; See: https://github.com/yetanalytics/lrs/blob/master/src/main/com/yetanalytics/lrs/pedestal/interceptor/xapi.cljc
+
+(deftest extra-params-test
+  (testing "Extra parameters"
+    (let [sys  (support/test-system)
+          sys' (component/start sys)]
+      (is (= 400
+             (try (curl/get "http://0.0.0.0:8080/xapi/statements?foo=bar"
+                            get-map)
+                  (catch ExceptionInfo e (-> e ex-data :status)))))
+      (testing "- `from`"
+        (is (= 200
+               (:status
+                (curl/get "http://0.0.0.0:8080/xapi/statements?from=00000000-4000-8000-0000-111122223333"
+                          get-map))))
+        (is (= 400
+               (try (curl/get "http://0.0.0.0:8080/xapi/statements?from=2024-10-10T10:10:10Z"
+                              get-map)
+                    (catch ExceptionInfo e (-> e ex-data :status))))))
+      (testing "- `unwrap_html`"
+        (is (= 200
+               (:status
+                (curl/get "http://0.0.0.0:8080/xapi/statements?unwrap_html=true"
+                          get-map))))
+        ;; TODO: Disallow non-boolean `unrwap_html` values?
+        (is (= 200
+               (:status
+                (curl/get "http://0.0.0.0:8080/xapi/statements?unwrap_html=not-a-boolean"
+                          get-map)))))
+      (testing "- `page`"
+        ;; TODO: Disallow `page` param?
+        (is (= 200
+               (:status
+                (curl/get "http://0.0.0.0:8080/xapi/statements?page=123"
+                          get-map)))))
+      (component/stop sys'))))