Allow custom fn validation

[gfredericks]

this is a proof-of-concept for being able to customize the fn-validation logic (by setting use-fn-validation to a function), which lets you implement things like:

1. soft runtime checks that only log warnings (or something) on schema failures
2. soft runtime checks that only happen a small percentage of the time to minimize the performance cost

I'd be happy to tweak, add tests, etc. if this is a desired feature.

The main use case for this for me is when retrofitting schemas on a legacy project, it's nice to be able to deploy the schemas to production to get feedback about their correctness without causing exceptions to be thrown.

[w01fe]

Thanks for the PR! It looks like this shouldn't affect performance in the 'off' case, which would have been my main concern.

I'm down with the basic idea and the fn interface, but not really stoked about the 'boolean or fn' thing. What do you think about leaving the boolean as-is, and just pulling the current validation logic (with the signature you suggest) out of the macro as a defn fn-validator. Then you can just rebind that function as needed (with alter-var-root) -- or we could also potentially make it dynamic.

[gfredericks]

the fn-validator idea sounds great, I'll make that change and add some tests. Thanks!

[w01fe]

Great, thanks!

[gfredericks]

Where should the fn-validator function live? If it's in schema.core then we'd have a circular dependency between schema.core↔schema.macros; and otherwise users would have to reference something outside of schema.core, which I imagine is unusual but not terrible.

[gfredericks]

I was going to try putting it in schema.utils but that's hard because it will presumably call schema.macros/error!, thus creating another circular dependency.

I'll move forward with it in schema.macros until you suggest otherwise.

[gfredericks]

Where should this be documented? Just on the fn-validator docstring itself? also mentioned in set-fn-validation!? The README?

[gfredericks]

Also this change causes a couple tests to fail because they expect an inlined exception thrown, and we now have a new function at the top of the stack trace. Switching fn-validator to a macro would be problematic for redeffing purposes.

I can change the test to specifically look at the second stack trace element instead of the first...

$ lein test
Rewriting src/cljx to target/generated/src/clj (clj) with features #{clj} and 0 transformations.
Rewriting src/cljx to target/generated/src/cljs (cljs) with features #{cljs} and 1 transformations.
Rewriting test/cljx to target/generated/test/clj (clj) with features #{clj} and 0 transformations.
Rewriting test/cljx to target/generated/test/cljs (cljs) with features #{cljs} and 1 transformations.

lein test schema.coerce-test

lein test schema.core-test

lein test :only schema.core-test/simple-validated-defn-test

FAIL in (simple-validated-defn-test) (core_test.clj:1122)
expected: (.contains (.getClassName (first (.getStackTrace e))) "simple_validated_defn")
  actual: false

lein test :only schema.core-test/simple-validated-defn-test

FAIL in (simple-validated-defn-test) (core_test.clj:1123)
expected: (.startsWith (.getFileName (first (.getStackTrace e))) "core_test.clj")
  actual: false

lein test schema.experimental.abstract-map-test

lein test schema.experimental.complete-test

lein test schema.experimental.generators-test
{:result true, :num-tests 100, :seed 1469033949013, :test-var "spec-test"}

lein test schema.macros-test

lein test schema.test-macros

lein test schema.test-test

Ran 122 tests containing 701 assertions.
2 failures, 0 errors.
Tests failed.

[gfredericks]

Pushed a new commit with the changes described

[w01fe]

Aww crap, now it all comes back to me.

I think the 'top of stack trace' is actually really important, that's what the user will see as the problem and it's really unhelpful if it doesn't actually reference the function in question. (That's why I hadn't already pulled out something like fn-validator).

My apologies for potentially leading you astray. Can you see a way to patch this up? E.g., manually filtering the stack before throwing. Or I guess the alternative is to go back to what you had before?

As for placement, I don't think it can go into schema.macros since that's Clojure only. The best place is probably schema.core, and the circular-looking dependency isn't truly circular -- more of a spiral :) -- I think you'll find a number of other places where schema.macros already emits schema.core symbols in macroexpansions.

As for documentation, I think just on the var seems reasonable. I haven't heard this request before, and I think it would probably be more of a distraction than a help to mention it in e.g., the readme.

Thanks and sorry again for the brain fart.

[gfredericks]

Is there a context where the user only sees the topmost stack frame? Otherwise I don't think this would be any worse than what users currently put up with when throwers use ex-info.

I don't think my original implementation would be any better, since the user still has to provide a function that would end up at the top of the stack trace.

I'm not familiar with the implications of trying to edit jvm stack traces, but I can look into it.

[w01fe]

Yes, for example in my cider REPL only the top frame is printed (and the trace pops up in another tab). Here's a transcript showing what I see and a potential workaround (in the JVM at least).

user> (defn foo [] (throw (RuntimeException. "hi")))
#'user/foo
user> (defn bar [] (foo))
#'user/bar
user> (bar)
RuntimeException hi  user/foo (form-init7240245945236560886.clj:1)
user> (defn bar [] (try (foo) (catch Exception e (.fillInStackTrace e) (throw e))))
#'user/bar
user> (bar)
RuntimeException hi  user/bar (form-init7240245945236560886.clj:1)

(FWIW, I think your initial implementation has the property that the stack trace remains the same if you don't provide a custom handler, which will be true a vast majority of the time I imagine. )

[gfredericks]

Yes it's true that the old impl doesn't change the stack by default. If that's good enough from your perspective I can just go back to that.

Perhaps to avoid the on?-or-fn problem we can keep the fn-validator var but set it to nil. So from the user's perspective everything would be the same as the version we've been discussing, but the normal behavior would be inlined. I'll work on that.

[w01fe]

Sounds good to me, thanks!

On Fri, Jul 22, 2016 at 10:28 PM, Gary Fredericks notifications@github.com
wrote:

Yes it's true that the old impl doesn't change the stack by default. If
that's good enough from your perspective I can just go back to that.

Perhaps to avoid the on?-or-fn problem we can keep the fn-validator var
but set it to nil. So from the user's perspective everything would be the
same as the version we've been discussing, but the normal behavior would be
inlined. I'll work on that.

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#365 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAIPpvxxcnUF9DBANnNzi9Qnh6SXKZ8pks5qYMVqgaJpZM4JQBLP
.

[gfredericks]

Okay, I've pushed those changes (with fn-validator moved to schema.core as well).

Note that I haven't run the clojurescript tests, as I didn't have a js runtime handy.

[w01fe]

Looks good, thanks! Apologies again for the back and forth.

[w01fe]

Just released 1.1.3 with this change.

[gfredericks]

Looks great, thanks!