search

yetanalytics / lrs

Protocols, specifications, and logic for building an xAPI Learning Record Store (LRS) in Clojure(Script).
https://www.yetanalytics.com/lrs
Apache License 2.0
4 stars 1 forks source link

Incorrect parameters spec for `-get-person` #23

Open kelvinqian00 opened 3 years ago

kelvinqian00 commented 3 years ago

The following is the current params spec for get-person:

(s/def ::get-person-params 
  (sc/with-conform-gen :xapi.agents.GET.request/params))

~However, this is incorrect, since this is simply the spec for statement resource queries, not agent resource queries. In the former, agent is an optional parameter, but in the latter, agent is required. Therefore, during generative testing, -get-person would receive missing agent values, causing unnecessary errors.~

EDIT: I likely used these specs wrong in lrsql and incorrectly determined the source of the issue. The followup comment still stands, however.

kelvinqian00 commented 3 years ago

Note: there are the following specs in xapi-schema:

(s/def :xapi.common.param/agent
  (json (s/nonconforming ::xs/actor)))

(s/def :xapi.statements.GET.request.params/agent 
  :xapi.common.param/agent)

(s/def :xapi.agents.GET.request.params/agent 
  :xapi.document.param/agent)

however these do not work as ::xs/actor covers Anonymous Groups, which do not have IFIs.

milt commented 3 years ago

this is simply the spec for statement resource queries

I don't get it, it says :xapi.agents.GET.request/params. did you mean state?

milt commented 3 years ago

https://github.com/yetanalytics/xapi-schema/blob/master/src/xapi_schema/spec/resources.cljc#L254-L255

kelvinqian00 commented 3 years ago

Oh I see what you mean. My mistake I thought ::get-person-params used :xapi.statements.GET.request/params. Still, the point in my followup comment stands.

milt commented 3 years ago

Yep, that last bit seems correct, it should be changed to agent specifically. Make a PR on xapi-schema for that