Wildcard keys can override specific keys

[gfredericks]

Looks like {:a s/Int, s/Keyword s/Bool} can fail when the key :a gets coincidentally generated by the s/Keyword key schema.

This causes a spurious test failure for me every few weeks.

I see there's currently a comment to this effect:

  ;; TODO: this does not currently capture proper semantics of maps with
  ;; both specific keys and key schemas that can override them.

Reproduction:

#!/usr/bin/env bash

clojure \
    -Srepro \
    -Sdeps '{:deps {prismatic/schema-generators {:mvn/version "0.1.2"}}}' \
    /dev/stdin <<EOF
(ns user
  (:require
   [clojure.test.check.generators :as gen]
   [schema.core                   :as s]
   [schema-generators.generators  :as schema-generators]))

(def schema {:a s/Int, s/Keyword s/Bool})

(def g (schema-generators/generator schema))

;; throws after a couple minutes
(count (schema-generators/sample 10000 schema))

;; throws immediately, but uses test.check impl details
#_
(prn (#'gen/call-gen g (clojure.test.check.random/make-random 2784) 30))

EOF

[gfredericks]

I have a hacky minimal fix on this branch.

There's a test failing, and I'm curious if you (@w01fe) have any ideas for a less-hacky way to do this.

I also need to add a failing test.

[gfredericks]

I think the failing test is because it's also applying the logic to sequential schemas; I'm not sure how to distinguish them.

[w01fe]

Oh man, I haven't thought about this in so long.

I just went back and looked a bit. I think the thing you have probably isn't right when there are optional keys as you notice. And it probably also does the wrong thing for sequences.

I suspect I left the TODO because it seemed hard to do this right, and such schemas where this would do the wrong thing mostly seemed dubious (although the {:foo s/Int s/Keyword s/Any}` that this fixes is definitely a reasonable thing that should work).

I suspect doing this properly would require beefing up the schema spec representation to provide a set of the "exceptions" for the remaining schema (if any). Perhaps that could just go directly into the existing :elements field, but I really don't remember how any of this works.

Alternatively, there might be a way to work around this special case for maps inside the generators library.

I have a 3-week old daughter so have my hands full at the moment but will do my best to answer questions or take another look sometime soon if it would be helpful.

Thanks, Jason

[gfredericks]

@w01fe looks like I can fix this in schema, by tweaking the wildcard key schema with a constraint that it not equal any of the other keys:

diff --git a/src/cljx/schema/core.cljx b/src/cljx/schema/core.cljx
index 056404e..13baba3 100644
--- a/src/cljx/schema/core.cljx
+++ b/src/cljx/schema/core.cljx
@@ -804,22 +804,26 @@
                               (mapv explain-kspec))]
       (macros/assert! (empty? duplicate-keys)
                       "Schema has multiple variants of the same explicit key: %s" duplicate-keys))
-    (concat
-     (for [[k v] (dissoc this extra-keys-schema)]
-       (let [rk (explicit-schema-key k)
-             required? (required-key? k)]
-         (collection/one-element
-          required? (map-entry (eq rk) v)
-          (clojure.core/fn [item-fn m]
-            (let [e (find m rk)]
-              (cond e (item-fn e)
-                    required? (item-fn (utils/error [rk 'missing-required-key])))
-              (if e
-                (dissoc #+clj (if (instance? clojure.lang.PersistentStructMap m) (into {} m) m) #+cljs m
-                        rk)
-                m))))))
-     (when extra-keys-schema
-       [(collection/all-elements (apply map-entry (find this extra-keys-schema)))]))))
+    (let [without-extra-keys-schema (dissoc this extra-keys-schema)]
+      (concat
+       (for [[k v] without-extra-keys-schema]
+         (let [rk (explicit-schema-key k)
+               required? (required-key? k)]
+           (collection/one-element
+            required? (map-entry (eq rk) v)
+            (clojure.core/fn [item-fn m]
+              (let [e (find m rk)]
+                (cond e (item-fn e)
+                      required? (item-fn (utils/error [rk 'missing-required-key])))
+                (if e
+                  (dissoc #+clj (if (instance? clojure.lang.PersistentStructMap m) (into {} m) m) #+cljs m
+                          rk)
+                  m))))))
+       (when extra-keys-schema
+         (let [specific-keys (set (map explicit-schema-key (keys without-extra-keys-schema)))
+               [ks vs] (find this extra-keys-schema)
+               restricted-ks (constrained ks #(not (contains? specific-keys %)))]
+           [(collection/all-elements (map-entry restricted-ks vs))]))))))
 
 (defn- map-error []
   (clojure.core/fn [_ elts extra]

would that be a bad way to do it?

[gfredericks]

(I don't mind also adding a predicate to the collection spec that applies to any wildcard element, and just only using it from the map spec, but thought this was worth asking about since it's relatively simple)

[w01fe]

Your change seems fine to me as long as the tests all pass. I think this was always a bit of a gray area so hopefully nobody is relying on the old behavior, and this definitely seems more right. Thanks!