kwrooijen / gungnir

A fully featured, data-driven database library for Clojure.
https://kwrooijen.github.io/gungnir/
MIT License
164 stars 9 forks source link

Spec violation when using local malli registries #16

Closed Ramblurr closed 4 years ago

Ramblurr commented 4 years ago

The goal is to separate out the actual data specs, so I can re-use them in places where gungnir isn't involved. The attached example works, until you enable instrumentation.. then there is a spec violation. But it does seem to work as intended with spec disabled. You can also trigger a spec error from gungnir earlier by replacing the (m/spec User) with User on line 13. gungnir doesn't seem to like the referenced schema.

(def UserRegistry {:users/id uuid?
                   :users/email [:re {:error/message "Invalid email"} #".+@.+\..+"]
                   :users/created-at  inst?
                   :users/updated-at  inst?})

(def User [:map {:registry UserRegistry}
           [:users/id {:auto true :primary-key true}]
           :users/email
           [:users/created-at {:auto true}]
           [:users/updated-at {:auto true}]])

(model/register!
  {:users (m/schema User)}) ;; <-- remove m/schema wrap to see spec error earlier

(def data {:users/name       "Tester"
           :users/email      "test@example.com"
           :users/id         (java.util.UUID/randomUUID)
           :users/created-at (j/instant)
           :users/updated-at (j/instant)})
(m/validate User data)                  ; => true

(q/save! (changeset data)) ;; this blows up ONLY when instrumentation is enabled, otherwise it works as intended

;; the spec error
Execution error - invalid arguments to gungnir.util.malli/child-properties at (field.cljc:33).
field.cljc:33

-- Spec failed --------------------

Function arguments

  (nil)
   ^^^

should satisfy

  schema?

or

should have additional elements. The next element ":key" should satisfy

  qualified-keyword?

-- Relevant specs -------

:gungnir.model/field:
  (clojure.spec.alpha/or
   :schema
   malli.core/schema?
   :gungnir.model.field/without-props
   (clojure.spec.alpha/cat
    :key
    clojure.core/qualified-keyword?
    :spec
    clojure.core/any?)
   :gungnir.model.field/with-props
   (clojure.spec.alpha/cat
    :key
    clojure.core/qualified-keyword?
    :props
    (clojure.spec.alpha/nilable clojure.core/map?)
    :spec
    clojure.core/any?))

-------------------------
Detected 1 error
Ramblurr commented 4 years ago

From slack

@kwrooijen 16:41 That's a bug it seems What's interesting though is that child-properties is getting a nil as argument. That shouldn't happen The problem is most likely here:

(defn child
  "Get the child `k` from `?model`. Returns `nil` if not found."
  [?model k]
  (->> (?model->model ?model)
       (m/children)
       (reduce (fn [_ child] (when (#{k} (first child))
                               (reduced child)))
               nil)))

It expects fields to be of the form [key ?props spec] But since you're giving a keyword, it doesn't know how to handle it Maybe Malli has a function to actually do this for me, instead of using reduce by hand Also I think Gungnir needs to handle Model registries

kwrooijen commented 4 years ago

Note that this is an issue with the latest version of Malli (Since SNAPSHOT doesn't have the qualified keyword child feature yet https://github.com/metosin/malli/pull/239)

If this is to be fixed Gungnir will have to lock Malli to a specific commit instead of SNAPSHOT (Which isn't a bad idea at all).

Ramblurr commented 4 years ago

I think that's a good idea. I am indeed using malli master branch in my gungir project. I propose we lock to the latest malli commit, document that, and then periodically update it until a stable malli release happens.

I use the https://github.com/reifyhealth/lein-git-down plugin to achieve this. I suppose it will work with libraries like gungnir too.

kwrooijen commented 4 years ago

I'll take a look at the options for git dependencies in libraries. In the past I used lein-git-down for (private) git dependencies and I had to manually add them to the top level project. That isn't really desired behavior but probably that's fixable.