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

Ensure :error is present for frontend (#23346) 

* Ensure :error is present for frontend

The frontend checks for a string at "error" to determine if there's an
error or not. Any error that lacks a message (`(.getMessage e) = nil`)
will report as a successful query with no results. This was hard to
reproduce at first because java 14+ added a nice error message to NPEs
in [JEP-358](https://openjdk.org/jeps/358).

```java
;; java 11
jshell> HashSet x = null;
x ==> null

jshell> x.contains(3);
|  Exception java.lang.NullPointerException
|        at (#2:1)

jshell>

;; java 17
jshell> HashSet x = null;
x ==> null

jshell> x.contains(3);
|  Exception java.lang.NullPointerException: Cannot invoke "java.util.HashSet.contains(Object)" because "REPL.$JShell$11.x" is null
|        at (#4:1)
```

```clojure
;; java 17
catch-exceptions=> (let [x nil] (x 3))
Execution error (NullPointerException) at metabase.query-processor.middleware.catch-exceptions/eval130385 (REPL:41).
Cannot invoke "clojure.lang.IFn.invoke(Object)" because "x" is null

;; java 11

catch-exceptions=> (let [x nil] (x 3))
Execution error (NullPointerException) at metabase.driver.sql.parameters.substitution/eval118508 (REPL:17).
null
```

Here are the responses to the FE edited to the relevant bits:

```javascript
// objects are edited for clarity/brevity:
// from java-11
{
  "error_type": "qp",
  "status": "failed",
  "class": "class java.lang.NullPointerException",
  "stacktrace": [...],
  "error": null,   // <---- the FE ignores all of the other data and only sees this
}

// vs java-17
{
  "error_type": "qp",
  "status": "failed",
  "class": "class java.lang.NullPointerException",
  "stacktrace": [...],
  "error": "Cannot invoke \"clojure.lang.IFn.invoke(Object)\" because \"to_period\" is null",,
}
```

Also, note that we were still logging

> ERROR middleware.catch-exceptions :: Error processing query: null

because we were looking for `(:error format-exception)` instead of
`(:error formatted-exception)`. `format-exception` is a multimethod and
lookup on non-associative things will happily return nil.

* Tests
parent 2de2ea57
No related branches found
No related tags found
No related merge requests found
Hide whitespace changes
Inline Side-by-side
src/metabase/query_processor/middleware/catch_exceptions.clj
+ 8
− 2
View file @ 79965714
......@@ -154,9 +154,15 @@
(letfn [(raisef* [e context]
;; format the Exception and return it
(let [formatted-exception (format-exception* query e @extra-info)]
(log/error (str (trs "Error processing query: {0}" (:error format-exception))
(log/error (str (trs "Error processing query: {0}"
(or (:error formatted-exception)
;; log in server locale, respond in user locale
(trs "Error running query")))
"\n" (u/pprint-to-str formatted-exception)))
(qp.context/resultf formatted-exception context)))]
;; ensure always a message on the error otherwise FE thinks query was successful. (#23258, #23281)
(qp.context/resultf (update formatted-exception
:error (fnil identity (trs "Error running query")))
context)))]
(try
(qp query rff (assoc context :raisef raisef*))
(catch Throwable e
......
test/metabase/query_processor/middleware/catch_exceptions_test.clj
+ 43
− 30
View file @ 79965714
......@@ -104,37 +104,50 @@
:metadata
(update :stacktrace boolean)))))))
(deftest include-query-execution-info-test
(testing "Should include info from QueryExecution if added to the thrown/raised Exception"
(deftest catch-exceptions-test
(testing "include-query-execution-info-test"
(testing "Should include info from QueryExecution if added to the thrown/raised Exception"
(tu.log/suppress-output
(is (= {:status :failed
:class java.lang.Exception
:error "Something went wrong"
:stacktrace true
:card_id 300
:json_query {}
:row_count 0
:data {:cols []}
:a 100
:b 200}
(-> (mt/test-qp-middleware catch-exceptions/catch-exceptions
{} {} []
{:runf (fn [_ _ context]
(qp.context/raisef (ex-info "Something went wrong."
{:query-execution {:a 100
:b 200
:card_id 300
;; these keys should all get removed
:result_rows 400
:hash 500
:executor_id 500
:dashboard_id 700
:pulse_id 800
:native 900}}
(Exception. "Something went wrong"))
context))})
:metadata
(update :stacktrace boolean)))))))
(testing "Should always include :error (#23258, #23281)"
(tu.log/suppress-output
(is (= {:status :failed
:class java.lang.Exception
:error "Something went wrong"
:stacktrace true
:card_id 300
:json_query {}
:row_count 0
:data {:cols []}
:a 100
:b 200}
(-> (mt/test-qp-middleware catch-exceptions/catch-exceptions
{} {} []
{:runf (fn [_ _ context]
(qp.context/raisef (ex-info "Something went wrong."
{:query-execution {:a 100
:b 200
:card_id 300
;; these keys should all get removed
:result_rows 400
:hash 500
:executor_id 500
:dashboard_id 700
:pulse_id 800
:native 900}}
(Exception. "Something went wrong"))
context))})
:metadata
(update :stacktrace boolean)))))))
(testing "Uses error message if present"
(is (= "Something went wrong"
(-> (fn [] (throw (Exception. "Something went wrong")))
(catch-exceptions)
:error))))
(testing "Has a default if no message on error"
(is (= "Error running query"
(-> (fn [] (throw (ex-info nil {})))
(catch-exceptions)
:error)))))))
(deftest permissions-test
(data/with-temp-copy-of-db
......
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment