From 79965714c478d75ffef2cc43343f523434f0534b Mon Sep 17 00:00:00 2001
From: dpsutton <dan@dpsutton.com>
Date: Wed, 15 Jun 2022 10:35:44 -0500
Subject: [PATCH] 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
---
.../middleware/catch_exceptions.clj | 10 ++-
.../middleware/catch_exceptions_test.clj | 73 +++++++++++--------
2 files changed, 51 insertions(+), 32 deletions(-)
diff --git a/src/metabase/query_processor/middleware/catch_exceptions.clj b/src/metabase/query_processor/middleware/catch_exceptions.clj
index dd28df493fb..06e67124caa 100644
--- a/src/metabase/query_processor/middleware/catch_exceptions.clj
+++ b/src/metabase/query_processor/middleware/catch_exceptions.clj
@@ -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
diff --git a/test/metabase/query_processor/middleware/catch_exceptions_test.clj b/test/metabase/query_processor/middleware/catch_exceptions_test.clj
index f1145a999ac..f4cc08fd5bb 100644
--- a/test/metabase/query_processor/middleware/catch_exceptions_test.clj
+++ b/test/metabase/query_processor/middleware/catch_exceptions_test.clj
@@ -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
--
GitLab