From 3a740014b0bcf69b70bcb5be27af1f03815b8e7a Mon Sep 17 00:00:00 2001 From: kelvinqian00 Date: Thu, 14 Mar 2024 16:36:59 -0400 Subject: [PATCH 1/3] Add statement spec redefinition to include from param validation --- src/main/lrsql/spec/statement.clj | 53 +++++++++++++++++++++++++++++-- 1 file changed, 51 insertions(+), 2 deletions(-) diff --git a/src/main/lrsql/spec/statement.clj b/src/main/lrsql/spec/statement.clj index 1ce4df53b..af2750cba 100644 --- a/src/main/lrsql/spec/statement.clj +++ b/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]))) ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;; From e7e429175474b41b7314febaf9287aa354f8c6da Mon Sep 17 00:00:00 2001 From: kelvinqian00 Date: Thu, 14 Mar 2024 16:37:04 -0400 Subject: [PATCH 2/3] Add params tests --- src/test/lrsql/params_test.clj | 65 ++++++++++++++++++++++++++++++++++ 1 file changed, 65 insertions(+) create mode 100644 src/test/lrsql/params_test.clj diff --git a/src/test/lrsql/params_test.clj b/src/test/lrsql/params_test.clj new file mode 100644 index 000000000..4d45b5ef3 --- /dev/null +++ b/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?unwrap_html=true" + get-map))))) + (component/stop sys')))) From d8336a27feab2e784f8251c67355881bdf212635 Mon Sep 17 00:00:00 2001 From: kelvinqian00 Date: Thu, 14 Mar 2024 16:49:09 -0400 Subject: [PATCH 3/3] Fix test for page param --- src/test/lrsql/params_test.clj | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/test/lrsql/params_test.clj b/src/test/lrsql/params_test.clj index 4d45b5ef3..8aac33895 100644 --- a/src/test/lrsql/params_test.clj +++ b/src/test/lrsql/params_test.clj @@ -60,6 +60,6 @@ ;; TODO: Disallow `page` param? (is (= 200 (:status - (curl/get "http://0.0.0.0:8080/xapi/statements?unwrap_html=true" + (curl/get "http://0.0.0.0:8080/xapi/statements?page=123" get-map))))) (component/stop sys'))))