`select-schema` silently returns the given value if the return value would not conform to the given schema

[cichli]

Hi,

Thanks for the useful library! I've encountered this behaviour with select-schema which seems odd:

user> (require '[schema.core :as sc]
               '[schema-tools.core :as stc])
nil

user> (sc/defschema MyCoolSchema {:a sc/Int
                                  :b sc/Int
                                  :c sc/Int})
...elided for brevity...

user> (stc/select-schema MyCoolSchema {:a 1 :b 2 :d 4})
{:a 1, :b 2, :d 4}

user> (stc/select-schema MyCoolSchema {:a 1 :b 2 :c "hello" :d 4})
{:a 1, :b 2, :c "hello", :d 4}

If the resulting value would not conform to the given schema, then the original value is returned as-is.

From the docstring - Removes all keys that are disallowed in the Schema. - I'd expect the above to return {:a 1, :b 2} and {:a 1, :b 2, :c "hello"} respectively.

I guess the other option would be to throw an exception, but (IMO) it makes sense to keep schema validation separate from selecting schema keys. Thoughts?

[ikitommi]

Good point, I would also expect the keys to be stripped without exception. Problem is that internally it uses (safe-)coercion, which does validation, but ignores the full coerced result if it resulted in error. In your example the inputs are Longs, not Integers and thus cause error.

You could use select-schema with a extra Long->Integer coercion-matcher to get the expected result.

Root cause is here: https://github.com/metosin/schema-tools/blob/master/src/schema_tools/coerce.cljx#L9

If you have great idea how to fix this elegantly, feel free to write a PR.

[ikitommi]

poked with this - couldn't find a robust way just to strip the extra keys without doing schema coercion (which is recursive and checks all the constraints).

So, the current idea is to change the behaviour so that it either succeeds in selecting the schema (and coercing it to match the schema) or it fails with a schema.core validation exception.

This would be a breaking change and the fn could be renamed from select-schema to select-schema!. Great thing with coercion approach is that everything is done in a single sweep - much faster than traversing the schemas twice (first just stripping the keys, then doing the actual coercion).

Json-coercion with filtering out illegal keys in a single sweep:

(st/select-schema! {:beer (s/enum :ipa :apa)} {:beer "ipa" :taste "good"})
; clojure.lang.ExceptionInfo: Value does not match schema: {:beer (not (#{:ipa :apa} "ipa"))}
;     data: {:type :schema.core/error,
;            :schema {:beer {:vs #{:ipa :apa}}},
;            :value {:beer "ipa", :taste "good"},
;            :error {:beer (not (#{:ipa :apa} "ipa"))}}

(require '[schema.coerce :as sc])

(st/select-schema! sc/json-coercion-matcher {:beer (s/enum :ipa :apa)} {:beer "ipa" :taste "good"})
; {:beer :ipa}

Linked the PR to this, would love to get your feedback to it.

[cichli]

Hi Tommi, thanks for taking the time to look into this!

Yeah, having the coercion throw an exception is very useful behaviour - we have something like this in our codebase currently (which looks very similar to the new select-schema!):

(ns social.common.schema
  (:require [schema.core :as sc]
            [schema.coerce :as sco]
            [schema.utils :as scu]))

(defn coerce
  [value schema coercion-matcher]
  (let [coerced ((sco/coercer schema coercion-matcher) value)]
    (if-let [error (and (scu/error? coerced) (scu/error-val coerced))]
      (throw (ex-info (format "Could not coerce value to schema: %s" error)
                      {:type ::error
                       :schema schema
                       :value value
                       :error error}))
      coerced)))

This means we get a useful error at the point of coercion, rather than later in the call stack when validation blows up on the resulting ErrorContainer.

The only two (minor) nitpicks I have are:

• I'm not sure the ! is needed at the end of the function name - IMO this should be reserved for side-effecting functions (i.e. unsafe to use in a transaction, or some other context where it may be retried) - if the doc string states that it throws, that should suffice?
• select-schema is analogous to clojure.core/select-keys, but the argument order is slightly different - select-keys (and friends i.e. get-in, update-in, etc.) take the value first, and then the key seq, whereas select-schema currently takes the schema and matcher before the value. What do you think?

Cheers from another IPA lover :beers:!

[ikitommi]

Thanks for the feedback!

I think the ! can be removed, originally thought it would be a nasty in-place breaking change to change the contract and because of that a new name - but I guess it's just a fix actually.

Also good point about the argument order, will do that and make a migration check for those who use it the other way around.

[ikitommi]

[metosin/schema-tools 0.5.0-SNAPSHOT]