a few more tweaks

src/metabase/driver/mongo/query_processor.clj

@@ -17,12 +17,16 @@
                              [query-processor :as qp])
             [metabase.driver.query-processor.interface :refer [qualified-name-components]]
             [metabase.driver.mongo.util :refer [with-mongo-connection *mongo-connection* values->base-type]]
-            [metabase.models.field :refer [Field]]
+            [metabase.models.field :as field]
             [metabase.util :as u])
   (:import (com.mongodb CommandResult
                         DB)
            (clojure.lang PersistentArrayMap)
-           (org.bson.types ObjectId)))
+           (org.bson.types ObjectId)
+           (metabase.driver.query_processor.interface DateTimeField
+                                                      DateTimeValue
+                                                      Field
+                                                      Value)))
 
 (declare apply-clause
          eval-raw-command
@@ -86,12 +90,22 @@
                                                                         [{$match *constraints*}])
                                                                     ~@(filter identity forms)]))
 
-(defn- field->name
-  "Return qualified string name of FIELD, e.g. `venue` or `venue.address`."
-  (^String [field separator]
-           (apply str (interpose separator (rest (qualified-name-components field))))) ; drop the first part, :table-name
-  (^String [field]
-           (field->name field ".")))
+;; Return qualified string name of FIELD, e.g. `venue` or `venue.address`.
+(defmulti field->name (fn
+                         (^String [this]           (class this))
+                         (^String [this separator] (class this))))
+
+(defmethod field->name Field
+  ([this]
+   (field->name this "."))
+  ([this separator]
+   (apply str (interpose separator (rest (qualified-name-components this))))))
+
+(defmethod field->name DateTimeField
+  ([this]
+   (field->name (:field this)))
+  ([this separator]
+   (field->name (:field this) separator)))
 
 (defn- field->$str
   "Given a FIELD, return a `$`-qualified field name for use in a Mongo aggregate query, e.g. `\"$user_id\"`."
@@ -133,7 +147,7 @@
                  keep-taking? (if limit (fn [_]
                                           (< (count values) limit))
                                   (constantly true))]
-             (->> (i/field-values-lazy-seq @(ns-resolve 'metabase.driver.mongo 'driver) (sel :one Field :id (:field-id field))) ; resolve driver at runtime to avoid circular deps
+             (->> (i/field-values-lazy-seq @(ns-resolve 'metabase.driver.mongo 'driver) (sel :one field/Field :id (:field-id field))) ; resolve driver at runtime to avoid circular deps
                   (filter identity)
                   (map hash)
                   (map #(conj! values %))
@@ -284,14 +298,16 @@
 
 ;; ### filter
 
-(defn- format-value
-  "Convert ID strings to `ObjectId`."
-  [{:keys [field-name base-type value]}]
-  (cond
-    (and (= field-name "_id")
-         (= base-type  :UnknownField))  `(ObjectId. ~value)
-    (= (type value) java.sql.Timestamp) (java.util.Date. (.getTime ^java.sql.Timestamp value)) ; ugg
-    :else                               value))
+(defmulti format-value class)
+
+(defmethod format-value Value [{value :value, {:keys [field-name base-type]} :field}]
+  (if (and (= field-name "_id")
+           (= base-type  :UnknownField))
+    `(ObjectId. ~value)
+    value))
+
+(defmethod format-value DateTimeValue [{^java.sql.Timestamp value :value}]
+  (java.util.Date. (.getTime value)))
 
 (defn- parse-filter-subclause [{:keys [filter-type field value] :as filter}]
   (let [field (when field (field->name field))

src/metabase/driver/query_processor/resolve.clj

@@ -65,13 +65,15 @@
 ;; ## Field
 (defmethod unresolved-field-id Field [{:keys [parent parent-id]}]
   (or (unresolved-field-id parent)
-      parent-id))
+      (when (instance? FieldPlaceholder parent)
+        parent-id)))
 
 (defmethod resolve-field Field [{:keys [parent parent-id], :as this} field-id->fields]
   (cond
-    parent    (if (= (type parent) Field)
-                this
-                (resolve-field parent field-id->fields))
+    parent    (or (when (instance? FieldPlaceholder parent)
+                    (when-let [resolved (resolve-field parent field-id->fields)]
+                      (assoc this :parent resolved)))
+                  this)
     parent-id (assoc this :parent (or (field-id->fields parent-id)
                                       (map->FieldPlaceholder {:field-id parent-id})))
     :else     this))
@@ -119,8 +121,7 @@
          (walk/postwalk (fn [form]
                           (when-let [id (f form)]
                             (conj! ids id)))))
-    (let [ids (persistent! ids)]
-      (when (seq ids) ids))))
+    (persistent! ids)))
 (def ^:private collect-unresolved-field-ids (partial collect-ids-with unresolved-field-id))
 (def ^:private collect-fk-field-ids         (partial collect-ids-with fk-field-id))
 
@@ -133,24 +134,27 @@
   "Resolve the `Fields` in an EXPANDED-QUERY-DICT.
    Record `:table-ids` referenced in the Query."
   [expanded-query-dict]
-  (let [field-ids (collect-unresolved-field-ids expanded-query-dict)]
-    (if-not field-ids
-      ;; If there are no more Field IDs to resolve we're done.
-      expanded-query-dict
-      ;; Otherwise fetch + resolve the Fields in question
-      (let [fields (->> (sel :many :id->fields [field/Field :name :display_name :base_type :special_type :preview_display :table_id :parent_id :description]
-                             :id [in field-ids])
-                        (m/map-vals rename-mb-field-keys)
-                        (m/map-vals #(assoc % :parent (when-let [parent-id (:parent-id %)]
-                                                        (map->FieldPlaceholder {:field-id parent-id})))))]
-        (->>
-         ;; Now record the IDs of Tables these fields references in the :table-ids property of the expanded query dict.
-         ;; Those will be used for Table resolution in the next step.
-         (update expanded-query-dict :table-ids set/union (set (map :table-id (vals fields))))
-         ;; Walk the query and resolve all fields
-         (walk/postwalk #(resolve-field % fields))
-         ;; Recurse in case any new (nested) unresolved fields were found.
-         recur)))))
+  (loop [max-iterations 5, expanded-query-dict expanded-query-dict]
+    (when (< max-iterations 0)
+      (throw (Exception. "Failed to resolve fields: too many iterations.")))
+    (let [field-ids (collect-unresolved-field-ids expanded-query-dict)]
+      (if-not (seq field-ids)
+        ;; If there are no more Field IDs to resolve we're done.
+        expanded-query-dict
+        ;; Otherwise fetch + resolve the Fields in question
+        (let [fields (->> (sel :many :id->fields [field/Field :name :display_name :base_type :special_type :preview_display :table_id :parent_id :description]
+                               :id [in field-ids])
+                          (m/map-vals rename-mb-field-keys)
+                          (m/map-vals #(assoc % :parent (when-let [parent-id (:parent-id %)]
+                                                          (map->FieldPlaceholder {:field-id parent-id})))))]
+          (->>
+           ;; Now record the IDs of Tables these fields references in the :table-ids property of the expanded query dict.
+           ;; Those will be used for Table resolution in the next step.
+           (update expanded-query-dict :table-ids set/union (set (map :table-id (vals fields))))
+           ;; Walk the query and resolve all fields
+           (walk/postwalk #(resolve-field % fields))
+           ;; Recurse in case any new (nested) unresolved fields were found.
+           (recur (dec max-iterations))))))))
 
 (defn- resolve-database
   "Resolve the `Database` in question for an EXPANDED-QUERY-DICT."

test/metabase/driver/postgres_test.clj

@@ -2,7 +2,8 @@
   (:require [expectations :refer :all]
             [metabase.driver.generic-sql.interface :as i]
             [metabase.driver.postgres :refer :all]
-            [metabase.test.data.interface :refer [def-database-definition]]
+            (metabase.test.data [datasets :refer [expect-with-dataset]]
+                                [interface :refer [def-database-definition]])
             [metabase.test.util.q :refer [Q]]))
 
 ;; # Check that database->connection details still works whether we're dealing with new-style or legacy details
@@ -59,21 +60,23 @@
     [#uuid "84ed434e-80b4-41cf-9c88-e334427104ae"]]])
 
 ;; Check that we can load a Postgres Database with a :UUIDField
-(expect {:cols    [{:description nil, :base_type :IntegerField, :name "id", :display_name "Id", :preview_display true, :special_type :id, :target nil, :extra_info {}}
-                   {:description nil, :base_type :UUIDField, :name "user_id", :display_name "User Id", :preview_display true, :special_type :category, :target nil, :extra_info {}}],
-         :columns ["id" "user_id"],
-         :rows    [[1 #uuid "4f01dcfd-13f7-430c-8e6f-e505c0851027"]
-                   [2 #uuid "4652b2e7-d940-4d55-a971-7e484566663e"]
-                   [3 #uuid "da1d6ecc-e775-4008-b366-c38e7a2e8433"]
-                   [4 #uuid "7a5ce4a2-0958-46e7-9685-1a4eaa3bd08a"]
-                   [5 #uuid "84ed434e-80b4-41cf-9c88-e334427104ae"]]}
+(expect-with-dataset :postgres
+  {:cols    [{:description nil, :base_type :IntegerField, :name "id", :display_name "Id", :preview_display true, :special_type :id, :target nil, :extra_info {}}
+             {:description nil, :base_type :UUIDField, :name "user_id", :display_name "User Id", :preview_display true, :special_type :category, :target nil, :extra_info {}}],
+   :columns ["id" "user_id"],
+   :rows    [[1 #uuid "4f01dcfd-13f7-430c-8e6f-e505c0851027"]
+             [2 #uuid "4652b2e7-d940-4d55-a971-7e484566663e"]
+             [3 #uuid "da1d6ecc-e775-4008-b366-c38e7a2e8433"]
+             [4 #uuid "7a5ce4a2-0958-46e7-9685-1a4eaa3bd08a"]
+             [5 #uuid "84ed434e-80b4-41cf-9c88-e334427104ae"]]}
   (-> (Q dataset metabase.driver.postgres-test/with-uuid use postgres
          return :data
          aggregate rows of users)
       (update :cols (partial mapv #(dissoc % :id :table_id)))))
 
 ;; Check that we can filter by a UUID Field
-(expect [[2 #uuid "4652b2e7-d940-4d55-a971-7e484566663e"]]
+(expect-with-dataset :postgres
+  [[2 #uuid "4652b2e7-d940-4d55-a971-7e484566663e"]]
   (Q dataset metabase.driver.postgres-test/with-uuid use postgres
      return rows
      aggregate rows of users