metosin / spec-tools

Clojure(Script) tools for clojure.spec
Eclipse Public License 2.0
592 stars 94 forks source link

ds/spec namespaced keys not prefixed with name #252

Open dehli opened 3 years ago

dehli commented 3 years ago

Hello, I think we are experiencing a bug and I wanted to confirm that this behavior isn't intentional.

We have two specs that we've created using data-spec. When we reference the first, it seems to be using the second spec. In the example below, my assumption was that the :a/x spec will be qualified using either ::spec-a or ::spec-b so in the registry it would show up as ::spec-a$a/x (or something to that effect) however it seems to be registering as :a/x which means that the second ds/spec overrides the same key in the registry. This results in unexpected behavior like shown below.

(def spec-a (ds/spec ::spec-a {:a/x int?}))
(def spec-b (ds/spec ::spec-b {:a/x nil?}))

(s/valid? spec-a {:a/x nil}) ;; true

Thanks in advance for your time and for this library!

miikka commented 3 years ago

It certainly is surprising behavior. For comparison:

(def spec-a (ds/spec ::spec-a {:x int?}))
(def spec-b (ds/spec ::spec-b {:x nil?}))

(s/valid? spec-a {:x nil}) ;; false

The idea is that if you provide a qualified name for the map key, it is used as-is, and for not-qualified keys the name is generated (example: user$spec-a/x). This is in line with Rich Hickey's and clojure.spec's idea that the same (qualified) map key should always hold the same kind of data.

So it's not an outright bug, but ds/spec silently redefining existing specs seems bad. It should probably throw an exception (maybe there should be an option to allow redefinitions – I don't immediately see an use case, but it would at least allow migration in case any users rely on the existing behavior).

arichiardi commented 3 years ago

Oh I have run into this when trying to override a key for an entity before the following:

(def ^:private crux-entity-w-ranges
  (assoc crux-entity
         :reference-range-set/reference-ranges
         (s/coll-of ::mapper.reference-range/crux-entity)))
(def crux-entity-w-ranges-spec (ds/spec {:name ::crux-entity-w-ranges :spec crux-entity-w-ranges}))

I basically wanted to reuse the same structure for both, but it does not seem to be possible if I understand it right? The :reference-range-set/reference-ranges always gets overridden if qualified.

What would be a possible workaround (apart from malli :smile:) ?

arichiardi commented 3 years ago

I tried the following but for some reason it still does not work (still checking for the :reference-range-set/reference-ranges key)

(def ^:private crux-entity-w-ranges
  (dissoc crux-entity :reference-range-set/reference-ranges))
(def crux-entity-w-ranges-spec (ds/spec {:name ::crux-entity-w-ranges :spec crux-entity-w-ranges}))

It makes sense, being the spec globally defined.

I ended up patching like so:

;; Data Spec follows what spec does here and seems to always
;; override :reference-range-set/reference-ranges.
;;
;; See https://github.com/metosin/spec-tools/issues/252
;;
;; Quite horrible and we should really consider Malli as an alternative.
(s/def ::reference-ranges
  (s/or :uuids (s/coll-of ::mapper.reference-range/id)
        :entities (s/coll-of ::mapper.reference-range/crux-entity)))
dehli commented 3 years ago

Started looking into this a little and unfortunately I don't think it would be possible due to the fact that spec doesn't support having the same namespace qualified keyword spec'd in more than one way.

For example, given the following specs:

(def spec-a (ds/spec ::spec-a {:a/x int?}))
(def spec-b (ds/spec ::spec-b {:a/x nil?}))

we could auto-generate the following two specs for the above namespace qualified keys.

(s/def ::$spec-a$a/x int?)
(s/def ::$spec-b$a/x nil?)

And then in theory we could write the following specs:

(s/def ::$spec-a (s/keys :req [::$spec-a$a/x]))
(s/def ::$spec-b (s/keys :req [::$spec-b$a/x]))

however there's no way for us to write the above spec so that we're checking a map that contains the :a/x key instead of ::$spec-a$a/x.

I think throwing an error like you had initially suggested would be the best option so that users don't accidentally redefine specs.

If there are other suggestions for how this could be possible I'd be happy to explore them. Thanks!