Merge pull request #3346 from metabase/support-mongo-bson-ids

Support Mongo BSON IDs 

src/metabase/driver.clj

@@ -308,6 +308,7 @@
              [java.util.UUID                 :type/Text]       ; shouldn't this be :type/UUID ?
              [clojure.lang.IPersistentMap    :type/Dictionary]
              [clojure.lang.IPersistentVector :type/Array]
+             [org.bson.types.ObjectId        :type/MongoBSONID]
              [org.postgresql.util.PGobject   :type/*]])
       (log/warn (format "Don't know how to map class '%s' to a Field base_type, falling back to :type/*." klass))
       :type/*))

src/metabase/driver/mongo/query_processor.clj

@@ -148,6 +148,7 @@
                                        3]})
           :year            {$year field})))))
 
+
 (extend-protocol IRValue
   nil (->rvalue [_] nil)
 
@@ -160,10 +161,9 @@
     (str \$ (->lvalue this)))
 
   Value
-  (->rvalue [{value :value, {:keys [field-name base-type]} :field}]
-    (if (and (= field-name "_id")
-             (= base-type  :type/*)) ; partial workaround for BSON ID Fields -- TODO fix this propertly (#1367)
-      `(ObjectId. ~value)
+  (->rvalue [{value :value, {:keys [base-type]} :field}]
+    (if (isa? base-type :type/MongoBSONID)
+      (ObjectId. (str value))
       value))
 
   DateTimeValue

src/metabase/types.clj

@@ -59,10 +59,11 @@
 
 (derive :type/Boolean :type/*)
 
-;;; Text-Like Types: Things that should be displayed as text for most purposes but that shouldn't support advanced filter options like starts with / contains
+;;; Text-Like Types: Things that should be displayed as text for most purposes but that *shouldn't* support advanced filter options like starts with / contains
 
 (derive :type/TextLike :type/*)
 (derive :type/IPAddress :type/TextLike)
+(derive :type/MongoBSONID :type/TextLike)
 
 ;;; "Virtual" Types
 

test/metabase/driver/mongo_test.clj

@@ -7,9 +7,13 @@
                              [field :refer [Field]]
                              [table :refer [Table] :as table])
             [metabase.query-processor :as qp]
+            [metabase.query-processor.expand :as ql]
+            [metabase.query-processor-test :refer [rows]]
             [metabase.test.data :as data]
-            [metabase.test.data.datasets :as datasets])
-  (:import metabase.driver.mongo.MongoDriver))
+            [metabase.test.data.datasets :as datasets]
+            [metabase.test.data.interface :as i])
+  (:import org.bson.types.ObjectId
+           metabase.driver.mongo.MongoDriver))
 
 ;; ## Logic for selectively running mongo
 
@@ -166,3 +170,19 @@
                     :table_id (:id (table-name->table nm))
                     {:order-by [:name]})]
         (into {} field))))
+
+
+;;; Check that we support Mongo BSON ID and can filter by it (#1367)
+
+(i/def-database-definition ^:private with-bson-ids
+  ["birds"
+   [{:field-name "name", :base-type :type/Text}
+    {:field-name "bird_id", :base-type :type/MongoBSONID}]
+   [["Rasta Toucan" (ObjectId. "012345678901234567890123")]
+    ["Lucky Pigeon" (ObjectId. "abcdefabcdefabcdefabcdef")]]])
+
+(datasets/expect-with-engine :mongo
+  [[2 "Lucky Pigeon" (ObjectId. "abcdefabcdefabcdefabcdef")]]
+  (rows (data/dataset metabase.driver.mongo-test/with-bson-ids
+          (data/run-query birds
+            (ql/filter (ql/= $bird_id "abcdefabcdefabcdefabcdef"))))))

test/metabase/test/data/mongo.clj

@@ -22,15 +22,14 @@
   (destroy-db! dbdef)
   (with-mongo-connection [mongo-db (database->connection-details dbdef)]
     (doseq [{:keys [field-definitions table-name rows]} table-definitions]
-      (let [field-names (->> field-definitions
-                             (map :field-name)
-                             (map keyword))]
+      (let [field-names (for [field-definition field-definitions]
+                          (keyword (:field-name field-definition)))]
         ;; Use map-indexed so we can get an ID for each row (index + 1)
         (doseq [[i row] (map-indexed (partial vector) rows)]
           (let [row (for [v row]
                       ;; Conver all the java.sql.Timestamps to java.util.Date, because the Mongo driver insists on being obnoxious and going from
                       ;; using Timestamps in 2.x to Dates in 3.x
-                      (if (isa? (type v) java.sql.Timestamp)
+                      (if (instance? java.sql.Timestamp v)
                         (java.util.Date. (.getTime ^java.sql.Timestamp v))
                         v))]
             (try