Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support Var-refs #985

Merged
merged 18 commits into from
Jan 6, 2024
Merged

Support Var-refs #985

merged 18 commits into from
Jan 6, 2024

Conversation

ikitommi
Copy link
Member

@ikitommi ikitommi commented Jan 4, 2024

Open Questions

  • should var-registry be enabled by default? ❌ let's sleep over this one
  • is it ok user to remove the var-registry? we still accept Vars as references (via m/-reference?) and document them in pretty printing ✅ it's ok to remove
  • does this work with all runtimes? bb? cherry? cljs? ✅ tests included
  • can we serialize var refs with edamame/sci? ❌ just clojure, not enabled by default

Sample

(require '[malli.core :as m])
(require '[malli.registry :as mr])
(require '[malli.generator :as mg])

;; enable var registry
(mr/set-default-registry!
 (mr/composite-registry
  (m/default-schemas)
  (mr/var-registry)))

(def UserId :string)

(def User
  [:map
   [:id #'UserId]
   [:friends {:optional true} [:set [:ref #'User]]]])

(m/form User)
;[:map 
; [:id #'user/UserId]
; [:friends {:optional true} [:set [:ref #'user/User]]]]

(m/validate User {:id "123", :friends #{{:id "234"}}})
; => true

(mg/sample User {:seed 42})
;({:id ""}
; {:id "5"}
; {:id ""}
; {:id "B62"}
; {:id "Vc16", :friends #{}}
; {:id "E",
;  :friends #{{:id ""}
;             {:id "u", :friends #{}}
;             {:id "", :friends #{}}
;             {:id "GL", :friends #{{:id "i"} {:id "o", :friends #{}}}}
;             {:id "d"}}}
; {:id "", :friends #{{:id ""} {:id "0Ql1", :friends #{{:id "3", :friends #{}}}}}}
; {:id "l"}
; {:id "",
;  :friends #{{:id "R8p", :friends #{}}
;             {:id "d8"}
;             {:id "A2u5k9G6"}
;             {:id ""}
;             {:id "C2", :friends #{{:id "33"} {:id "", :friends #{}} {:id "g4"}}}}}
; {:id "IrcCSYwSO"})

Invalid ref error

((requiring-resolve 'malli.dev/start!))

(m/schema [:ref 123])
invalid-ref

@bsless
Copy link
Contributor

bsless commented Jan 4, 2024

My 2c:

  • should var-registry be enabled by default? if it works with all runtimes, yes
  • is it ok user to remove the var-registry? we still accept Vars as references (via m/-reference?) and document them in pretty printing 🤔 - hope so. Var is a reference type, it should be a valid reference in malli imo

The part I'm most interested in wrt this MR is how var references interact with JSON schema and its derivatives:

  • Do they get lifted to definitions?
  • Do they get qualified?
  • Should they?

@ilmoraunio
Copy link

should var-registry be enabled by default?

What would this this mean in terms of code?

If it means running mr/set-default-registry! implicitly using a composite-var-registry, then there also needs to be a way to disable it easily. Also, this would likely constitute a bigger jump in terms of versioning semantics. But I'm carefully for it.

@ikitommi
Copy link
Member Author

ikitommi commented Jan 4, 2024

@bsless

The part I'm most interested in wrt this MR is how var references interact with JSON schema and its derivatives:

Do they get lifted to definitions?

yes, now with tests. moved all references to strings to ensure they get encoded correctly.

(def UserId :string)

(def User
  [:map {:registry {::location [:tuple :double :double]
                    `description :string}}
   [:id #'UserId]
   ::location
   `description
   [:friends {:optional true} [:set [:ref #'User]]]])

(json-schema/transform User)
;{:type "object",
; :properties {:id {:$ref "#/definitions/malli.json-schema-test~1UserId"},
;              :malli.json-schema-test/location {:$ref "#/definitions/malli.json-schema-test~1location"},
;              malli.json-schema-test/description {:$ref "#/definitions/malli.json-schema-test~1description"},
;              :friends {:type "array", :items {:$ref "#/definitions/malli.json-schema-test~1User"}, :uniqueItems true}},
; :required [:id :malli.json-schema-test/location malli.json-schema-test/description],
; :definitions {"malli.json-schema-test/UserId" {:type "string"},
;               "malli.json-schema-test/location" {:type "array",
;                                                  :items [{:type "number"} {:type "number"}],
;                                                  :additionalItems false},
;               "malli.json-schema-test/description" {:type "string"},
;               "malli.json-schema-test/User" {:type "object",
;                                              :properties {:id {:$ref "#/definitions/malli.json-schema-test~1UserId"},
;                                                           :malli.json-schema-test/location {:$ref "#/definitions/malli.json-schema-test~1location"},
;                                                           malli.json-schema-test/description {:$ref "#/definitions/malli.json-schema-test~1description"},
;                                                           :friends {:type "array",
;                                                                     :items {:$ref "#/definitions/malli.json-schema-test~1User"},
;                                                                     :uniqueItems true}},
;                                              :required [:id
;                                                         :malli.json-schema-test/location
;                                                         malli.json-schema-test/description]}}}

Do they get qualified?

yes.

Should they?

yes. qualified keywords, symbols and Vars are all global in Clojure, so they can and should be global in JSON Schema too.

@bsless
Copy link
Contributor

bsless commented Jan 5, 2024

In that case, I think we'll need properties support to override the namespace qualification, and some "global" property to add a mapping of qualifying namespace to other names when transforming the schema, with some means of conveying it to reitit

@ikitommi
Copy link
Member Author

ikitommi commented Jan 5, 2024

@bsless so, you would not like to expose the namespaces? just for Vars or also for qualified keywords and symbols?

@bsless
Copy link
Contributor

bsless commented Jan 5, 2024

so, you would not like to expose the namespaces?

Exactly. It's an implementation detail and can just add noise and confusion. Why would the consumer care that my schema is a com.foo.my-service.blash.schema/User? The benefit is it maps directly to the place where the schema is defined. Since it has pros and cons for each option, it makes sense to make it configurable and leave it up to users.

just for Vars or also for qualified keywords and symbols?

With qualified idents you can already give them whatever namespace prefix you like (and some argue you shouldn't use a ::foo anyway), so it's less of an issue.

@ikitommi
Copy link
Member Author

ikitommi commented Jan 5, 2024

also, added report for the invalid function reg:

register-function-schemas

@ikitommi
Copy link
Member Author

ikitommi commented Jan 6, 2024

@bsless extracted the requirement to mask Var namespaces in JSON Schema / Swagger into separate ns - #988

@ilmoraunio - didn't push the VarRegistry into defaults. Will sleep over this.

@ikitommi ikitommi merged commit 930d3e3 into master Jan 6, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants