Merge pull request #8842 from metabase/better-geo-json-error-messages

Add better error messages to loading GeoJSON files

src/metabase/api/geojson.clj

@@ -9,9 +9,12 @@
              [i18n :as ui18n :refer [tru]]
              [schema :as su]]
             [ring.util.response :as rr]
-            [schema.core :as s]
-            [metabase.util.i18n :as ui18n])
-  (:import org.apache.commons.io.input.ReaderInputStream))
+            [schema.core :as s])
+  (:import com.fasterxml.jackson.core.JsonParseException
+           java.io.FileNotFoundException
+           [java.net ConnectException NoRouteToHostException UnknownHostException]
+           java.util.concurrent.TimeoutException
+           org.apache.commons.io.input.ReaderInputStream))
 
 (def ^:private ^:const ^Integer geojson-fetch-timeout-ms
   "Number of milliseconds we have to fetch (and parse, if applicable) a GeoJSON file before we consider the request to
@@ -23,23 +26,57 @@
   URL-OR-RESOURCE should be something that can be passed to `slurp`, like an HTTP URL or a `java.net.URL` (which is
   what `io/resource` returns below)."
   [url-or-resource]
-  (u/with-timeout geojson-fetch-timeout-ms
-    (dorun (json/parse-stream (io/reader url-or-resource))))
+  (try
+    (u/with-timeout geojson-fetch-timeout-ms
+      (dorun (json/parse-stream (io/reader url-or-resource))))
+    ;; `with-timeout` executes the body in a future. If an exception occurs, it will throw an ExcecutionException
+    ;; which isn't very useful. The cause of the ExcutionException is what we want to propagate as that is something
+    ;; that will be actionable by the user to fix
+    (catch java.util.concurrent.ExecutionException e
+      (throw (.getCause e))))
   true)
 
+(defn- rethrow-with-message
+  "Throws an exception using `exception` as the cause (so we don't lose the related stacktrace) but includes a new,
+  hopefully more user friendly error message `msg`."
+  [msg exception]
+  (throw (Exception. (str msg) exception)))
+
 (defn- valid-json-resource?
   "Does this RELATIVE-PATH point to a valid local JSON resource? (RELATIVE-PATH is something like
   \"app/assets/geojson/us-states.json\".)"
   [relative-path]
   (when-let [^java.net.URI uri (u/ignore-exceptions (java.net.URI. relative-path))]
     (when-not (.isAbsolute uri)
-      (valid-json? (io/resource (str "frontend_client/" uri))))))
+      (let [relative-path-with-prefix (str "frontend_client/" uri)]
+        (if-let [resource (io/resource relative-path-with-prefix)]
+          (try
+            (valid-json? resource)
+            (catch JsonParseException e
+              (rethrow-with-message (tru "Unable to parse resource `{0}` as JSON" relative-path-with-prefix) e)))
+          (throw (FileNotFoundException. (str (tru "Unable to find JSON via relative path `{0}`" relative-path-with-prefix)))))))))
 
 (defn- valid-json-url?
   "Is URL a valid HTTP URL and does it point to valid JSON?"
   [url]
   (when (u/url? url)
-    (valid-json? url)))
+    (try
+      (valid-json? url)
+      ;; There could be many reasons why we are not able to use a URL as a GeoJSON source. The various catch clauses
+      ;; below attempt to provide the user with more info around the failure so they can correct the issue and try
+      ;; again.
+      (catch TimeoutException e
+        (rethrow-with-message (tru "Connection to host timed out for URL `{0}`" url) e))
+      (catch UnknownHostException e
+        (rethrow-with-message (tru "Unable to connect to unknown host at URL `{0}`" url) e))
+      (catch NoRouteToHostException e
+        (rethrow-with-message (tru "Unable to connect to host at URL `{0}`" url) e))
+      (catch ConnectException e
+        (rethrow-with-message (tru "Connection refused by host at for URL `{0}`" url) e))
+      (catch FileNotFoundException e
+        (rethrow-with-message (tru "Unable to retrieve resource at URL `{0}`" url) e))
+      (catch JsonParseException e
+        (rethrow-with-message (tru "Unable to parse resource at URL `{0}` as JSON" url) e)))))
 
 (def ^:private valid-json-url-or-resource?
   "Check that remote URL points to a valid JSON file, or throw an exception.
@@ -52,10 +89,7 @@
 
 (def ^:private CustomGeoJSON
   {s/Keyword {:name                     s/Str
-              :url                      (s/constrained
-                                         s/Str
-                                         valid-json-url-or-resource?
-                                         "URL must point to a valid JSON file.")
+              :url                      s/Str
               :region_key               (s/maybe s/Str)
               :region_name              (s/maybe s/Str)
               (s/optional-key :builtin) s/Bool}})
@@ -72,6 +106,11 @@
                      :region_name "NAME"
                      :builtin     true}})
 
+(defn- validate-custom-geo-json [geojson-value]
+  (s/validate CustomGeoJSON geojson-value)
+  (doseq [[_ {geo-url-or-uri :url}] geojson-value]
+    (valid-json-url-or-resource? geo-url-or-uri)))
+
 (defsetting custom-geojson
   (tru "JSON containing information about custom GeoJSON files for use in map visualizations instead of the default US State or World GeoJSON.")
   :type    :json
@@ -79,7 +118,7 @@
   :getter  (fn [] (merge (setting/get-json :custom-geojson) builtin-geojson))
   :setter  (fn [new-value]
              (when new-value
-               (s/validate CustomGeoJSON new-value))
+               (validate-custom-geo-json new-value))
              (setting/set-json! :custom-geojson new-value)))
 
 

src/metabase/util.clj

@@ -15,7 +15,8 @@
             [metabase.util.i18n :refer [trs]]
             [ring.util.codec :as codec])
   (:import [java.net InetAddress InetSocketAddress Socket]
-           [java.text Normalizer Normalizer$Form]))
+           [java.text Normalizer Normalizer$Form]
+           java.util.concurrent.TimeoutException))
 
 ;; This is the very first log message that will get printed.  It's here because this is one of the very first
 ;; namespaces that gets loaded, and the first that has access to the logger It shows up a solid 10-15 seconds before
@@ -307,7 +308,7 @@
   [futur timeout-ms]
   (let [result (deref futur timeout-ms ::timeout)]
     (when (= result ::timeout)
-      (throw (Exception. (format "Timed out after %d milliseconds." timeout-ms))))
+      (throw (TimeoutException. (format "Timed out after %d milliseconds." timeout-ms))))
     result))
 
 (defmacro with-timeout

test/metabase/api/geojson_test.clj

@@ -59,6 +59,39 @@
       ((user->client :crowberto) :put 200 "setting/custom-geojson" {:value test-custom-geojson})
       ((user->client :crowberto) :get 200 "setting/custom-geojson"))))
 
+;; Test that a bad url will return a descriptive error message
+(expect
+  #"Unable to retrieve resource"
+  ;; try this up to 3 times since Circle's outbound connections likes to randomly stop working
+  (u/auto-retry 3
+    ;; bind a temporary value so it will get set back to its old value here after the API calls are done
+    ;; stomping all over it
+    (tu/with-temporary-setting-values [custom-geojson nil]
+      (let [bad-url-custom-geojson (update-in test-custom-geojson [:middle-earth :url] str "something-random")]
+        (:message ((user->client :crowberto) :put 500 "setting/custom-geojson" {:value bad-url-custom-geojson}))))))
+
+;; Test that a bad host will return a connection refused error
+(expect
+  #"Unable to connect"
+  ;; try this up to 3 times since Circle's outbound connections likes to randomly stop working
+  (u/auto-retry 3
+    ;; bind a temporary value so it will get set back to its old value here after the API calls are done
+    ;; stomping all over it
+    (tu/with-temporary-setting-values [custom-geojson nil]
+      (let [bad-url-custom-geojson (assoc-in test-custom-geojson [:middle-earth :url] "https://somethingrandom.metabase.com")]
+        (:message ((user->client :crowberto) :put 500 "setting/custom-geojson" {:value bad-url-custom-geojson}))))))
+
+;; Test out the error message for a relative path file we can't find
+(expect
+  #"Unable to find JSON via relative path"
+  ;; try this up to 3 times since Circle's outbound connections likes to randomly stop working
+  (u/auto-retry 3
+    ;; bind a temporary value so it will get set back to its old value here after the API calls are done
+    ;; stomping all over it
+    (tu/with-temporary-setting-values [custom-geojson nil]
+      (let [bad-url-custom-geojson (assoc-in test-custom-geojson [:middle-earth :url] "some/relative/path")]
+        (:message ((user->client :crowberto) :put 500 "setting/custom-geojson" {:value bad-url-custom-geojson}))))))
+
 
 ;;; test the endpoint that acts as a proxy for JSON files
 (expect