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

ds/spec namespaced keys not prefixed with name #252

Open
dehli opened this issue Jan 28, 2021 · 4 comments
Open

ds/spec namespaced keys not prefixed with name #252

dehli opened this issue Jan 28, 2021 · 4 comments

Comments

@dehli
Copy link
Contributor

dehli commented Jan 28, 2021

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!

@dehli dehli changed the title Data Specs Overriding Global Registry Data Spec Namespaced keys not prefixed with name Jan 28, 2021
@dehli dehli changed the title Data Spec Namespaced keys not prefixed with name ds/spec namespaced keys not prefixed with name Jan 28, 2021
@miikka
Copy link
Contributor

miikka commented Feb 5, 2021

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).

@miikka miikka added this to Candidates in Metosin Maintenance Mob Feb 5, 2021
@arichiardi
Copy link
Contributor

arichiardi commented Aug 20, 2021

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 😄) ?

@arichiardi
Copy link
Contributor

arichiardi commented Aug 20, 2021

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
Copy link
Contributor Author

dehli commented Oct 27, 2021

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

No branches or pull requests

3 participants