Fix flaky test and test for a new case (#28757) (#28989)

* Fix flaky test and test for a new case

flaky test was annoying. It would timeout after 45 seconds. And
annoyingly, it times out on the _client_, not the server. So we don't
get the proper error message we expect.

And what's happening is that the backend just hooks up the pipe from
it's request to the response. And if no bytes ever come across, we just
twiddle our thumbs until the http request in the test gives up.

The only reliable way i could think of to fix this is to spin up a
webserver that just accepts and then ignores requests (which is what
github did to us in the flaky test).

Had to bump the ring adapter to get the newly exposed `:async-timeout
60000` parameter. By default the webserver in the test has a timeout of
30 seconds, which is shorter than the 45 second timeout for the test's
request to the geojson endpoint.

* add comment

* clean up test server logic

---------

Co-authored-by: dpsutton <dan@dpsutton.com>
Co-authored-by: Nemanja Glumac <31325167+nemanjaglumac@users.noreply.github.com>

deps.edn

@@ -63,7 +63,7 @@
   hiccup/hiccup                             {:mvn/version "1.0.5"}              ; HTML templating
   honeysql/honeysql                         {:mvn/version "1.0.461"             ; Transform Clojure data structures to SQL
                                              :exclusions  [org.clojure/clojurescript]}
-  info.sunng/ring-jetty9-adapter            {:mvn/version "0.18.3"}             ; Drop-in replacement for official Ring Jetty adapter. Supports Jetty 11 webserver.
+  info.sunng/ring-jetty9-adapter            {:mvn/version "0.18.5"}             ; Drop-in replacement for official Ring Jetty adapter. Supports Jetty 11 webserver.
   instaparse/instaparse                     {:mvn/version "1.4.12"}             ; Make your own parser
   io.forward/yaml                           {:mvn/version "1.0.11"              ; Clojure wrapper for YAML library SnakeYAML. Don't upgrade yet, new version doesn't support Java 8 (see https://github.com/owainlewis/yaml/issues/37)
                                              :exclusions  [org.clojure/clojure

src/metabase/api/geojson.clj

@@ -112,8 +112,10 @@
   [url respond]
   (with-open [^BufferedReader reader (if-let [resource (io/resource url)]
                                        (io/reader resource)
-                                       (:body (http/get url {:as                :reader
-                                                             :redirect-strategy :none})))
+                                       (:body (http/get url {:as                 :reader
+                                                             :redirect-strategy  :none
+                                                             :socket-timeout     8000
+                                                             :connection-timeout 8000})))
               is                     (ReaderInputStream. reader)]
     (respond (-> (response/response is)
                  (response/content-type "application/json")))))

test/metabase/api/geojson_test.clj

@@ -8,8 +8,11 @@
    [metabase.test :as mt]
    [metabase.util :as u]
    [metabase.util.schema :as su]
+   [ring.adapter.jetty9 :as ring-jetty]
    [schema.core :as s]))
 
+(set! *warn-on-reflection* true)
+
 (def ^String test-geojson-url
   "URL of a GeoJSON file used for test purposes."
   "https://raw.githubusercontent.com/metabase/metabase/master/test_resources/test.geojson")
@@ -132,6 +135,26 @@
                                          {:value resource-geojson})
                    (mt/user-http-request :crowberto :get 200 "setting/custom-geojson")))))))))
 
+(defprotocol GeoJsonTestServer
+  (-port [_]))
+
+(defn- non-responding-server
+  "Returns a server which accepts requests but never responds to them. Implements [[GeoJsonTestServer]] so you can
+  call [[-port]] to get the port. Implements java.io.Closeable so can be used in a `with-open`."
+  ^java.io.Closeable []
+  (let [server (ring-jetty/run-jetty (fn silent-async-handler
+                                       [_request _respond _raise])
+                                     {:join?         false
+                                      :async?        true
+                                      :port          0
+                                      :async-timeout 60000})]
+    (reify
+      java.io.Closeable
+      (close [_] (.stop server))
+
+      GeoJsonTestServer
+      (-port [_] (.. server getURI getPort)))))
+
 (deftest url-proxy-endpoint-test
   (testing "GET /api/geojson"
     (testing "test the endpoint that fetches JSON files given a URL"
@@ -140,7 +163,19 @@
              (mt/user-http-request :crowberto :get 200 "geojson" :url test-geojson-url))))
     (testing "error is returned if URL connection fails"
       (is (= "GeoJSON URL failed to load"
-             (mt/user-http-request :crowberto :get 400 "geojson" :url test-broken-geojson-url))))
+             (mt/user-http-request :crowberto :get 400 "geojson"
+                                   :url test-broken-geojson-url))))
+    (testing "error is returned if URL server never responds (#28752)"
+      ;; use a webserver which accepts a connection and never responds. The geojson endpoint opens a reader to the url
+      ;; and responds with it. And if there are never any bytes going across, the whole thing just sits there. Our
+      ;; test flakes after 45 seconds with `mt/user-http-request` times out. And presumably other clients have similar
+      ;; issues. This ensures we give a good error message in this case.
+      (with-open [server (non-responding-server)]
+        (let [never-responds-url (str "http://localhost:" (-port server))]
+          (testing "error is returned if URL connection fails"
+            (is (= "GeoJSON URL failed to load"
+                   (mt/user-http-request :crowberto :get 400 "geojson"
+                                         :url never-responds-url)))))))
     (testing "error is returned if URL is invalid"
       (is (= (str "Invalid GeoJSON file location: must either start with http:// or https:// or be a relative path to "
                   "a file on the classpath. URLs referring to hosts that supply internal hosting metadata are "