Skip to content
Snippets Groups Projects
Unverified Commit 1290ecf7 authored by dpsutton's avatar dpsutton Committed by GitHub
Browse files

Alternative fix mu defn (#35939)

* Revert "Clarify stacktraces from malli validation errors (#34712)"

This reverts commit 1a0f2d65.

* Alternative fix for mu/defn correctness issue

https://github.com/metabase/metabase/pull/34712 introduced a correctness
bug:

```clojure
(macroexpand '(metabase.util.malli/defn foo [x :- string?] :foo))

(def
 foo
 "Inputs: [x :- string?]\n  Return: :any"
 (clojure.core/fn
  ([a] (metabase.util.malli.fn/validate-input {:fn-name 'user/foo} string? a) ((clojure.core/fn [x] :foo) a))))
```

The simplest demo of the bug is

```clojure
user=> (metabase.util.malli/defn foo [x] a)
 #'user/foo
```

The macro used simple and predictable args for it's emitted
functions. This was not a problem with the original shape:

(different macroexpansions of `(mu/defn foo [x :- int?] (inc x))`):

prior to 34712:

```clojure
(def
 foo
 "Inputs: [x :- int?]\n  Return: :any"
 (clojure.core/let
  [&f (clojure.core/fn [x] (inc x))]
  (clojure.core/fn
   ([a] ;; simple argument name not a problem
    (metabase.util.malli.fn/validate-input
      {:fn-name (quote metabase.util.malli.fn-test/foo)}
      int?
      a)
    (&f a)))))
```

after 34712:

```clojure
(def
 foo
 "Inputs: [x :- int?]\n  Return: :any"
 (clojure.core/fn
  ([a] ;; simple argument name is now a problem
   (metabase.util.malli.fn/validate-input {:fn-name 'user/foo} int? a)
   ((clojure.core/fn [x] (inc x))
    a))))
```

after this PR:
```clojure
(def
 foo
 "Inputs: [x :- int?]\n  Return: :any"
 (clojure.core/let
  [&f (clojure.core/fn [x] (inc x))]
  (clojure.core/fn
   ([a] ;; again simple argument name is not a problem
    (try
     (metabase.util.malli.fn/validate-input
      {:fn-name (quote metabase.util.malli.fn-test/foo)}
      int?
      a)
     (&f a)
     (catch
      java.lang.Exception
      error
      (throw (metabase.util.malli.fn/fixup-stacktrace error))))))))
```

And the fix here is just to remove the StackTraceElements which
reference `validate`, `validate-input`, or `validate-output`.

* fix tests

tests are run in the user namespace similar to

```clojure
user=> (clojure.test/run-tests 'metabase.util.malli.fn-test)

Testing metabase.util.malli.fn-test

Ran 8 tests containing 43 assertions.
0 failures, 0 errors.
{:test 8, :pass 43, :fail 0, :error 0, :type :summary}
```

But we still want the namespace of _this_ file m.u.m.fn-test
parent a0004dea
No related branches found
No related tags found
No related merge requests found
Loading
0% Loading or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment