Convert ID Integer columns to strings (#13107)

* Convert ID Integer columns to strings

Converts any ID (PK or FK) column that's derived from a :type/Integer to
a string in query middleware (at the end of query processing.)

JavaScript treats all numbers as floats, with a 52 bit mantissa. Any
integer larger than 52 bits ends up looking like a strange floating
point number.

This will allow the UI to properly display a number n where n > 2^52 or
n < -2^52

Resolves #5816

[ci all]

* code review feedback

* skip the reduce phase if there are no columns to update

* handle joins properly

joins have two :field-id values in them. handle them appropriately

* update cloverage build

* update tools.namespace for newer cloverage

* handle all query types more gracefully

Only in cases where we have an explicit :field-id do we try to do the
coercions to strings. It's important to not filter the :fields clause in
such a way that anything is left out - if they were, the keep-indexed
call would modify the wrong values in the result.

project.clj

@@ -48,7 +48,7 @@
    [org.clojure/math.combinatorics "0.1.4"]                           ; combinatorics functions
    [org.clojure/math.numeric-tower "0.0.4"]                           ; math functions like `ceil`
    [org.clojure/tools.logging "1.1.0"]                                ; logging framework
-   [org.clojure/tools.namespace "0.2.11"]
+   [org.clojure/tools.namespace "1.0.0"]
    [org.clojure/tools.trace "0.7.10"]                                 ; function tracing
    [amalloy/ring-buffer "1.2.2"
     :exclusions [org.clojure/clojure
@@ -369,8 +369,8 @@
 
    :cloverage
    [:test-common
-    {:dependencies [[cloverage "1.2.0" :exclusions [riddley]]]
-     :plugins      [[lein-cloverage  "1.2.0"]]
+    {:dependencies [[camsaul/cloverage "1.2.1.1" :exclusions [riddley]]]
+     :plugins      [[camsaul/lein-cloverage  "1.2.1.1"]]
      :source-paths ^:replace ["src" "backend/mbql/src"]
      :test-paths   ^:replace ["test" "backend/mbql/test"]
      :cloverage    {:fail-threshold 69

src/metabase/api/card.clj

@@ -611,8 +611,9 @@
                             (qp/process-query-and-save-execution! query info context)))}}]
   {:pre [(u/maybe? sequential? parameters)]}
   (let [card  (api/read-check (Card card-id))
-        query (assoc (query-for-card card parameters constraints middleware)
-                     :async? true)
+        query (-> (assoc (query-for-card card parameters constraints middleware)
+                         :async? true)
+                  (assoc-in [:middleware :js-int-to-string?] true))
         info  {:executed-by  api/*current-user-id*
                :context      context
                :card-id      card-id

src/metabase/api/dataset.clj

@@ -49,7 +49,8 @@
         info           {:executed-by api/*current-user-id*
                         :context     :ad-hoc
                         :card-id     source-card-id
-                        :nested?     (boolean source-card-id)}]
+                        :nested?     (boolean source-card-id)}
+        query          (update query :middleware assoc :js-int-to-string? true)]
     (qp.streaming/streaming-response [context :api]
       (qp/process-query-and-save-with-max-results-constraints! query info context))))
 

src/metabase/query_processor.clj

@@ -29,6 +29,7 @@
              [expand-macros :as expand-macros]
              [fetch-source-query :as fetch-source-query]
              [format-rows :as format-rows]
+             [large-int-id :as large-int-id]
              [limit :as limit]
              [mbql-to-native :as mbql-to-native]
              [normalize-query :as normalize]
@@ -68,6 +69,7 @@
    #'cumulative-ags/handle-cumulative-aggregations
    #'resolve-joins/resolve-joins
    #'add-implicit-joins/add-implicit-joins
+   #'large-int-id/convert-id-to-string
    #'limit/limit
    #'format-rows/format-rows
    #'desugar/desugar

src/metabase/query_processor/middleware/large_int_id.clj

+(ns metabase.query-processor.middleware.large-int-id
+  "Middleware for handling conversion of IDs to strings for proper display of large numbers"
+  (:require [clojure.tools.logging :as log]
+            [metabase.mbql.util :as mbql.u]
+            [metabase.models.field :refer [Field]]))
+
+(defn- result-int->string
+  [field-indexes rf]
+  (fn
+    ([] (rf))
+    ([result] (rf result))
+    ([result row]
+     (rf result (reduce #(update-in %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
+  or < -2^51, the JavaScript float mantissa. This will allow proper display of large numbers,
+  like IDs from services like social media. All ID numbers are converted to avoid the performance
+  penalty of a comparison based on size."
+  [qp]
+  (fn [{{:keys [js-int-to-string?] :or {js-int-to-string? false}} :middleware, :as query} rff context]
+    ;; currently, this excludes `:field-literal` values like aggregations.
+    ;;
+    ;; for a query like below, *no* conversion will occur
+    ;; (mt/mbql-query venues
+    ;;              {:source-query {:source-table $$venues
+    ;;                              :aggregation  [[:aggregation-options
+    ;;                                              [:avg $id]
+    ;;                                              {:name "some_generated_name", :display-name "My Cool Ag"}]]
+    ;;                              :breakout     [$price]}})
+    ;; when you run in this fashion, you lose the ability to determine if it's an ID - you get a `:fields` value
+    ;; like: `:fields [[:field-literal "PRICE" :type/Integer] [:field-literal "some_generated_name" :type/BigInteger]]`
+    ;; so, short of turning all `:type/Integer` derived values into strings, this is the best approximation
+    ;; of a fix that can be accomplished.
+    (let [field-indexes (keep-indexed
+                          (fn [idx val]
+                            (let [field-id (mbql.u/field-clause->id-or-literal val)]
+                              (when-let [field (when (number? field-id)
+                                                 (Field field-id))]
+                                (when (and (or (isa? (:special_type field) :type/PK)
+                                               (isa? (:special_type field) :type/FK))
+                                           (isa? (:base_type field) :type/Integer))
+                                  idx))))
+                          (log/spy :error (:fields (:query query))))]
+      (qp query (if (and js-int-to-string? (seq field-indexes))
+                  #(result-int->string field-indexes (rff %))
+                  rff)
+          context))))

test/metabase/api/dataset_test.clj

@@ -51,7 +51,8 @@
 (defn- most-recent-query-execution [] (db/select-one QueryExecution {:order-by [[:id :desc]]}))
 
 (def ^:private query-defaults
-  {:middleware {:add-default-userland-constraints? true}})
+  {:middleware {:add-default-userland-constraints? true
+                :js-int-to-string? true}})
 
 (deftest basic-query-test
   (testing "POST /api/dataset"

test/metabase/query_processor/middleware/large_int_id_test.clj

+(ns metabase.query-processor.middleware.large-int-id-test
+  (:require [clojure.test :refer :all]
+            [metabase
+             [query-processor :as qp]
+             [test :as mt]]))
+
+(deftest convert-ids
+  (let [query (mt/mbql-query users
+                {:order-by [[:asc $id]]
+                 :limit    5})]
+    (testing "PKs become strings when middleware enabled"
+      (is (= [["1" "Plato Yeshua" "2014-04-01T08:30:00Z"]
+              ["2" "Felipinho Asklepios" "2014-12-05T15:15:00Z"]
+              ["3" "Kaneonuskatew Eiran" "2014-11-06T16:15:00Z"]
+              ["4" "Simcha Yan" "2014-01-01T08:30:00Z"]
+              ["5" "Quentin Sören" "2014-10-03T17:30:00Z"]]
+             (mt/rows
+               (qp/process-query (assoc query :middleware {:js-int-to-string? true}))))))
+
+    (testing "PKs are left alone when middleware disabled (default)"
+      (is (= [[1 "Plato Yeshua" "2014-04-01T08:30:00Z"]
+              [2 "Felipinho Asklepios" "2014-12-05T15:15:00Z"]
+              [3 "Kaneonuskatew Eiran" "2014-11-06T16:15:00Z"]
+              [4 "Simcha Yan" "2014-01-01T08:30:00Z"]
+              [5 "Quentin Sören" "2014-10-03T17:30:00Z"]]
+             (mt/rows
+               (qp/process-query (assoc query :middleware {})))))))
+
+  (let [query (mt/mbql-query users
+                {:fields   [$name]
+                 :order-by [[:asc $name]]
+                 :limit    5})]
+    (testing "handle when there are no ID columns in the query but the middleware is enabled"
+      (is (= [["Broen Olujimi"]
+              ["Conchúr Tihomir"]
+              ["Dwight Gresham"]
+              ["Felipinho Asklepios"]
+              ["Frans Hevel"]]
+             (mt/rows
+               (qp/process-query (assoc query :middleware {:js-int-to-string? true})))))))
+
+  (let [query (mt/mbql-query venues
+                {:order-by [[:asc $id]]
+                 :limit    5})]
+    (testing "FKs become strings when middleware enabled"
+      (is (= [["1" "Red Medicine" "4" 10.0646 -165.374 3]
+              ["2" "Stout Burgers & Beers" "11" 34.0996 -118.329 2]
+              ["3" "The Apple Pan" "11" 34.0406 -118.428 2]
+              ["4" "Wurstküche" "29" 33.9997 -118.465 2]
+              ["5" "Brite Spot Family Restaurant" "20" 34.0778 -118.261 2]]
+             (mt/rows
+               (qp/process-query (assoc query :middleware {:js-int-to-string? true}))))))
+
+    (testing "FKs are left alone when middleware disabled (default)"
+      (is (= [[1 "Red Medicine" 4 10.0646 -165.374 3]
+              [2 "Stout Burgers & Beers" 11 34.0996 -118.329 2]
+              [3 "The Apple Pan" 11 34.0406 -118.428 2]
+              [4 "Wurstküche" 29 33.9997 -118.465 2]
+              [5 "Brite Spot Family Restaurant" 20 34.0778 -118.261 2]]
+             (mt/rows
+               (qp/process-query (assoc query :middleware {})))))))
+
+  (let [query (mt/mbql-query checkins
+                {:fields   [$id $user_id->users.id $user_id->users.name $venue_id->venues.id $venue_id->venues.name]
+                 :order-by [[:asc $id]]
+                 :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"]]
+             (mt/rows
+               (qp/process-query (assoc query :middleware {:js-int-to-string? true})))))))
+
+  (let [query (mt/mbql-query venues
+                {:source-query {:source-table $$venues
+                                :aggregation  [[:aggregation-options
+                                                [:avg $id]
+                                                {:name "some_generated_name", :display-name "My Cool Ag"}]]
+                                :breakout     [$price]}})]
+    ;; see comment in `metabase.query-processor.middleware.large-int-id/convert-id-to-string`
+    ;; for why this value does not change
+    (testing "aggregations are not converted to strings with middleware enabled"
+      (is (= [[1 55]
+              [2 48]
+              [3 47]
+              [4 62]]
+             (mt/rows
+               (qp/process-query (assoc query :middleware {:js-int-to-string? true}))))) )
+    (testing "aggregation does not convert to strings with middleware disabled (default)"
+      (is (= [[1 55]
+              [2 48]
+              [3 47]
+              [4 62]]
+             (mt/rows
+               (qp/process-query (assoc query :middleware {}))))))))