Make sure QP handles sequable? (non-vector) result rows properly [ci drivers] (#13484)

modules/drivers/bigquery/test/metabase/driver/bigquery_test.clj

@@ -76,3 +76,9 @@
                    :type     :query
                    :query    {:source-table (mt/id view-name)
                               :order-by     [[:asc (mt/id view-name :id)]]}}))))))))
+
+(deftest query-integer-pk-or-fk-test
+  (mt/test-driver :bigquery
+    (testing "We should be able to query a Table that has a :type/Integer column marked as a PK or FK"
+      (is (= [["1" "Plato Yeshua" "2014-04-01T08:30:00Z"]]
+             (mt/rows (mt/user-http-request :rasta :post 202 "dataset" (mt/mbql-query users {:limit 1, :order-by [[:asc $id]]}))))))))

src/metabase/query_processor/middleware/catch_exceptions.clj

@@ -161,7 +161,7 @@
                 (swap! extra-info assoc :native query)
                 (nativef query context))
               (raisef* [e context]
-               ;; if the exception is the special quit-early exception, forward this to our parent `raisef` exception
+               ;; if the exception is the special quit-result exception, forward this to our parent `raisef` exception
                ;; handler, which has logic for handling that case
                 (if (qp.reducible/quit-result e)
                   (raisef e context)

src/metabase/query_processor/middleware/large_int_id.clj

@@ -9,7 +9,7 @@
     ([] (rf))
     ([result] (rf result))
     ([result row]
-     (rf result (reduce #(update-in %1 [%2] str) row field-indexes)))))
+     (rf result (reduce #(update (vec %1) %2 str) row field-indexes)))))
 
 (defn convert-id-to-string
   "Converts any ID (:type/PK and :type/FK) in a result to a string to handle a number > 2^51

test/metabase/http_client.clj

@@ -236,11 +236,11 @@
                              should perform the request as
    *  `method`               `:get`, `:post`, `:delete`, or `:put`
    *  `expected-status-code` When passed, throw an exception if the response has a different status code.
-   *  `url`                  Base URL of the request, which will be appended to `*url-prefix*`. e.g. `card/1/favorite`
+   *  `endpoint`             URL minus the `<host>/api/` part e.g. `card/1/favorite`. Appended to `*url-prefix*`.
    *  `request-options`      Optional map of options to pass as part of request to `clj-http.client`, e.g. `:headers`.
                              The map must be wrapped in `{:request-options}` e.g. `{:request-options {:headers ...}}`
    *  `http-body-map`        Optional map to send as the JSON-serialized HTTP body of the request
-   *  `url-kwargs`           key-value pairs that will be encoded and added to the URL as GET params"
-  {:arglists '([credentials? method expected-status-code? url request-options? http-body-map? & url-kwargs])}
+   *  `query-params`           key-value pairs that will be encoded and added to the URL as query params"
+  {:arglists '([credentials? method expected-status-code? endpoint request-options? http-body-map? & {:as query-params}])}
   [& args]
   (:body (apply client-full-response args)))

test/metabase/query_processor/middleware/large_int_id_test.clj

@@ -2,7 +2,8 @@
   (:require [clojure.test :refer :all]
             [metabase
              [query-processor :as qp]
-             [test :as mt]]))
+             [test :as mt]]
+            [metabase.query-processor.middleware.large-int-id :as large-int-id]))
 
 (deftest convert-ids
   (let [query (mt/mbql-query users
@@ -66,10 +67,10 @@
                  :limit    5})]
     (testing "joins work correctly"
       (is (= [["1" "5" "Quentin Sören" "12" "The Misfit Restaurant + Bar"]
-	      ["2" "1" "Plato Yeshua" "31" "Bludso's BBQ"]
-	      ["3" "8" "Szymon Theutrich" "56" "Philippe the Original"]
-	      ["4" "5" "Quentin Sören" "4" "Wurstküche"]
-	      ["5" "3" "Kaneonuskatew Eiran" "49" "Hotel Biron"]]
+              ["2" "1" "Plato Yeshua" "31" "Bludso's BBQ"]
+              ["3" "8" "Szymon Theutrich" "56" "Philippe the Original"]
+              ["4" "5" "Quentin Sören" "4" "Wurstküche"]
+              ["5" "3" "Kaneonuskatew Eiran" "49" "Hotel Biron"]]
              (mt/rows
                (qp/process-query (assoc query :middleware {:js-int-to-string? true})))))))
 
@@ -95,3 +96,24 @@
               [4 62]]
              (mt/rows
                (qp/process-query (assoc query :middleware {}))))))))
+
+(deftest different-row-types-test
+  (testing "Middleware should work regardless of the type of each row (#13475)"
+    (doseq [rows [[[1]
+                   [Integer/MAX_VALUE]]
+                  [(list 1)
+                   (list Integer/MAX_VALUE)]
+                  [(cons 1 nil)
+                   (cons Integer/MAX_VALUE nil)]
+                  [(lazy-seq [1])
+                   (lazy-seq [Integer/MAX_VALUE])]]]
+      (testing (format "rows = ^%s %s" (.getCanonicalName (class rows)) (pr-str rows))
+        (is (= [["1"]
+                ["2147483647"]]
+               (:post
+                (mt/test-qp-middleware
+                 large-int-id/convert-id-to-string
+                 {:type       :query
+                  :query      {:fields [[:field-id (mt/id :venues :id)]]}
+                  :middleware {:js-int-to-string? true}}
+                 rows))))))))

test/metabase/query_processor/reducible_test.clj

@@ -163,3 +163,37 @@
                          (respond {:cols [{:name "n"}]}
                                   [[1] [2] [3] [4] [5]]))
              :rff      maps-rff})))))
+
+(deftest row-type-agnostic-test
+  (let [api-qp-middleware-options (delay (-> (mt/user-http-request :rasta :post 202 "dataset" (mt/mbql-query users {:limit 1}))
+                                             :json_query
+                                             :middleware))]
+    (mt/test-drivers (mt/normal-drivers)
+      (testing "All QP middleware should work regardless of the type of each row (#13475)"
+        (doseq [rows [[[1]
+                       [Integer/MAX_VALUE]]
+                      [(list 1)
+                       (list Integer/MAX_VALUE)]
+                      [(cons 1 nil)
+                       (cons Integer/MAX_VALUE nil)]
+                      [(lazy-seq [1])
+                       (lazy-seq [Integer/MAX_VALUE])]]]
+          (testing (format "rows = ^%s %s" (.getCanonicalName (class rows)) (pr-str rows))
+            (letfn [(process-query [& {:as additional-options}]
+                      (:post
+                       (mt/test-qp-middleware
+                        qp/default-middleware
+                        (merge
+                         {:database (mt/id)
+                          :type     :query
+                          :query    {:source-table (mt/id :venues)
+                                     :fields       [[:field-id (mt/id :venues :id)]]}}
+                         additional-options)
+                        rows)))]
+              (is (= [[1]
+                      [2147483647]]
+                     (process-query)))
+              (testing "Should work with the middleware options used by API requests as well"
+                (= [["1"]
+                    ["2147483647"]]
+                   (process-query :middleware @api-qp-middleware-options))))))))))

test/metabase/test.clj

@@ -139,9 +139,10 @@
  [test-users
   fetch-user
   test-user?
-  user->id
   user->client
   user->credentials
+  user->id
+  user-http-request
   with-test-user]
 
  [tt

test/metabase/test/data/users.clj

@@ -144,15 +144,26 @@
         (clear-cached-session-tokens!)
         (apply client-fn username args)))))
 
-(s/defn user->client :- (s/pred fn?)
+(s/defn ^:deprecated user->client :- (s/pred fn?)
   "Returns a `metabase.http-client/client` partially bound with the credentials for User with `username`.
    In addition, it forces lazy creation of the User if needed.
 
-     ((user->client) :get 200 \"meta/table\")"
+     ((user->client) :get 200 \"meta/table\")
+
+  DEPRECATED -- use `user-http-request` instead, which has proper `:arglists` metadata which makes it a bit easier to
+  use when writing code."
   [username :- TestUserName]
   (fetch-user username) ; force creation of the user if not already created
   (partial client-fn username))
 
+(defn user-http-request
+  "A version of our test HTTP client that issues the request with credentials for `username`."
+  {:arglists '([username credentials? method expected-status-code? endpoint
+                request-options? http-body-map? & {:as query-params}])}
+  [username & args]
+  (fetch-user username)
+  (apply client-fn username args))
+
 (defmacro with-test-user
   "Call `body` with various `metabase.api.common` dynamic vars like `*current-user*` bound to the test User named by
   `user-kwd`."