Merge pull request #8981 from metabase/fix-nested-queries-with-joins

Fix nested queries with JOINs 

src/metabase/driver/generic_sql/query_processor.clj

@@ -91,7 +91,7 @@
   ;; other `->honeysql` impls (e.g. the `(class Field` one) will do the correct thing automatically without having to
   ;; worry about the context in which they are being called
   (qp.store/with-pushed-store
-    (when-let [{:keys [join-alias table-id]} (mbql.u/fk-clause->join-info *query* fk-clause)]
+    (when-let [{:keys [join-alias table-id]} (mbql.u/fk-clause->join-info *query* *nested-query-level* fk-clause)]
       (when table-id
         (qp.store/store-table! (assoc (qp.store/table table-id)
                                  :schema nil

src/metabase/driver/sqlserver.clj

@@ -7,6 +7,7 @@
              [util :as u]]
             [metabase.driver.generic-sql :as sql]
             [metabase.driver.generic-sql.query-processor :as sqlqp]
+            [metabase.query-processor.interface :as qp.i]
             [metabase.util
              [honeysql-extensions :as hx]
              [i18n :refer [tru]]
@@ -155,6 +156,18 @@
                                                  (* items (dec page))
                                                  items))))
 
+;; From the dox:
+;;
+;; The ORDER BY clause is invalid in views, inline functions, derived tables, subqueries, and common table
+;; expressions, unless TOP, OFFSET or FOR XML is also specified.
+;;
+;; To fix this we'll add a max-results LIMIT to the query when we add the order-by if there's no `limit` specified,
+;; but not for `top-level` queries (since it's not needed there)
+(defn- apply-order-by [default-apply-order-by driver honeysql-form {:keys [limit], :as query}]
+  (let [add-limit? (and (not limit) (pos? sqlqp/*nested-query-level*))]
+    (cond-> (default-apply-order-by driver honeysql-form query)
+      add-limit? (apply-limit (assoc query :limit qp.i/absolute-max-results)))))
+
 ;; SQLServer doesn't support `TRUE`/`FALSE`; it uses `1`/`0`, respectively; convert these booleans to numbers.
 (defmethod sqlqp/->honeysql [SQLServerDriver Boolean]
   [_ bool]
@@ -205,17 +218,19 @@
                        (conj (sql/features this) :no-case-sensitivity-string-filter-options))})
 
   sql/ISQLDriver
-  (merge
-   (sql/ISQLDriverDefaultsMixin)
-   {:apply-limit               (u/drop-first-arg apply-limit)
-    :apply-page                (u/drop-first-arg apply-page)
-    :column->base-type         (u/drop-first-arg column->base-type)
-    :connection-details->spec  (u/drop-first-arg connection-details->spec)
-    :current-datetime-fn       (constantly :%getutcdate)
-    :date                      (u/drop-first-arg date)
-    :excluded-schemas          (constantly #{"sys" "INFORMATION_SCHEMA"})
-    :string-length-fn          (u/drop-first-arg string-length-fn)
-    :unix-timestamp->timestamp (u/drop-first-arg unix-timestamp->timestamp)}))
+  (let [{default-apply-order-by :apply-order-by, :as mixin} (sql/ISQLDriverDefaultsMixin)]
+    (merge
+     mixin
+     {:apply-limit               (u/drop-first-arg apply-limit)
+      :apply-page                (u/drop-first-arg apply-page)
+      :apply-order-by            (partial apply-order-by default-apply-order-by)
+      :column->base-type         (u/drop-first-arg column->base-type)
+      :connection-details->spec  (u/drop-first-arg connection-details->spec)
+      :current-datetime-fn       (constantly :%getutcdate)
+      :date                      (u/drop-first-arg date)
+      :excluded-schemas          (constantly #{"sys" "INFORMATION_SCHEMA"})
+      :string-length-fn          (u/drop-first-arg string-length-fn)
+      :unix-timestamp->timestamp (u/drop-first-arg unix-timestamp->timestamp)})))
 
 (defn -init-driver
   "Register the SQLServer driver"

src/metabase/mbql/util.clj

@@ -341,16 +341,26 @@
 
 
 (s/defn fk-clause->join-info :- (s/maybe mbql.s/JoinInfo)
-  "Return the matching info about the JOINed for the 'destination' Field in an `fk->` clause.
+  "Return the matching info about the JOINed for the 'destination' Field in an `fk->` clause, for the current level of
+  nesting (`0` meaning this `fk->` clause was found in the top-level query; `1` meaning it was found in the first
+  `source-query`, and so forth.)
 
-     (fk-clause->join-alias [:fk-> [:field-id 1] [:field-id 2]])
+     (fk-clause->join-info query [:fk-> [:field-id 1] [:field-id 2]] 0)
      ;; -> \"orders__via__order_id\""
-  [query :- mbql.s/Query, [_ source-field-clause] :- mbql.s/fk->]
-  (let [source-field-id (field-clause->id-or-literal source-field-clause)]
-    (some (fn [{:keys [fk-field-id], :as info}]
-            (when (= fk-field-id source-field-id)
-              info))
-          (-> query :query :join-tables))))
+  [query :- mbql.s/Query, nested-query-level :- su/NonNegativeInt, [_ source-field-clause :as fk-clause] :- mbql.s/fk->]
+  ;; if we're dealing with something that's not at the top-level go ahead and recurse a level until we get to the
+  ;; query we want to work with
+  (if (pos? nested-query-level)
+    (recur {:query (or (get-in query [:query :source-query])
+                       (throw (Exception. (str (tru "Bad nested-query-level: query does not have a source query")))))}
+           (dec nested-query-level)
+           fk-clause)
+    ;; ok, when we've reached the right level of nesting, look in `:join-tables` to find the appropriate info
+    (let [source-field-id (field-clause->id-or-literal source-field-clause)]
+      (some (fn [{:keys [fk-field-id], :as info}]
+              (when (= fk-field-id source-field-id)
+                info))
+            (-> query :query :join-tables)))))
 
 
 (s/defn expression-with-name :- mbql.s/FieldOrExpressionDef

src/metabase/query_processor/middleware/catch_exceptions.clj

@@ -8,13 +8,13 @@
 (def ^:dynamic ^:private *add-preprocessed-queries?* true)
 
 (defn- fail [{query-type :type, :as query}, ^Throwable e, & [additional-info]]
-  (merge {:status       :failed
-          :class        (class e)
-          :error        (or (.getMessage e) (str e))
-          :stacktrace   (u/filtered-stacktrace e)
+  (merge {:status     :failed
+          :class      (class e)
+          :error      (or (.getMessage e) (str e))
+          :stacktrace (u/filtered-stacktrace e)
           ;; TODO - removing this stuff is not really needed anymore since `:database` is just the ID and not the
           ;; entire map including `:details`
-          :query        (dissoc query :database :driver)}
+          :query      (dissoc query :database :driver)}
          ;; add the fully-preprocessed and native forms to the error message for MBQL queries, since they're extremely
          ;; useful for debugging purposes. Since generating them requires us to recursively run the query processor,
          ;; make sure we can skip adding them if we end up back here so we don't recurse forever

src/metabase/query_processor/middleware/resolve_joined_tables.clj

 (ns metabase.query-processor.middleware.resolve-joined-tables
   "Middleware that fetches tables that will need to be joined, referred to by `fk->` clauses, and adds information to
   the query about what joins should be done and how they should be performed."
-  (:require [metabase.db :as mdb]
+  (:require [metabase
+             [db :as mdb]
+             [driver :as driver]]
             [metabase.mbql
              [schema :as mbql.s]
              [util :as mbql.u]]
             [metabase.models
              [field :refer [Field]]
              [table :refer [Table]]]
-            [metabase.query-processor.store :as qp.store]
+            [metabase.query-processor
+             [interface :as qp.i]
+             [store :as qp.store]]
             [metabase.util.schema :as su]
             [schema.core :as s]
-            [toucan.db :as db]
-            [metabase.driver :as driver]
-            [metabase.query-processor.interface :as qp.i]))
+            [toucan.db :as db]))
 
 (defn- both-args-are-field-id-clauses? [[_ x y]]
   (and
@@ -106,17 +108,41 @@
 
 ;;; -------------------------------------------- PUTTING it all together ---------------------------------------------
 
-(defn- resolve-joined-tables* [query]
-  (if (and qp.i/*driver* (not (driver/driver-supports? qp.i/*driver* :foreign-keys)))
+(defn- resolve-joined-tables-in-top-level-query
+  "Resolve JOINs at the top-level of the query."
+  [{mbql-query :query, :as query}]
+  ;; find fk-> clauses in the query AT THE TOP LEVEL
+  (let [fk-clauses      (mbql.u/match (dissoc mbql-query :source-query) [:fk-> [:field-id _] [:field-id _]])
+        source-table-id (mbql.u/query->source-table-id query)]
+    ;; if there are none, or `source-table` isn't resolved for some reason or another (which we need in order to fetch
+    ;; FK info), return query as-is
+    (if-not (and (seq fk-clauses) source-table-id)
+      query
+      ;; otherwise fetch PK info, add relevant Tables & Fields to QP store, and add the `:join-tables` key to the query
+      (let [pk-info (fk-clauses->pk-info source-table-id fk-clauses)]
+        (store-join-tables! fk-clauses)
+        (store-join-table-pk-fields! pk-info)
+        (add-join-info-to-query query fk-clauses pk-info)))))
+
+(defn- resolve-joined-tables-in-query-all-levels
+  "Resolve JOINs at all levels of the query, including the top level and nested queries at any level of nesting."
+  [{{source-query :source-query} :query, :as query}]
+  ;; first, resolve JOINs for the top-level
+  (let [query                          (resolve-joined-tables-in-top-level-query query)
+        ;; then recursively resolve JOINs for any nested queries by pulling the query up a level and then getting the
+        ;; result
+        {resolved-source-query :query} (when source-query
+                                         (resolve-joined-tables-in-query-all-levels (assoc query :query source-query)))]
+    ;; finally, merge the resolved source-query into the top-level query as appropriate
+    (cond-> query
+      resolved-source-query (assoc-in [:query :source-query] resolved-source-query))))
+
+(defn- resolve-joined-tables* [{query-type :type, :as query}]
+  (if (or (= query-type :native)
+          (and qp.i/*driver*
+               (not (driver/driver-supports? qp.i/*driver* :foreign-keys))))
     query
-    (let [source-table-id (mbql.u/query->source-table-id query)
-          fk-clauses      (mbql.u/match (:query query) [:fk-> [:field-id _] [:field-id _]])]
-      (if-not (and (seq fk-clauses) source-table-id)
-        query
-        (let [pk-info (fk-clauses->pk-info source-table-id fk-clauses)]
-          (store-join-tables! fk-clauses)
-          (store-join-table-pk-fields! pk-info)
-          (add-join-info-to-query query fk-clauses pk-info))))))
+    (resolve-joined-tables-in-query-all-levels query)))
 
 (defn resolve-joined-tables
   "Fetch and store any Tables other than the source Table referred to by `fk->` clauses in an MBQL query, and add a

test/metabase/db/migrations_test.clj

@@ -14,6 +14,7 @@
              [permissions-group :as perm-group :refer [PermissionsGroup]]
              [pulse :refer [Pulse]]
              [user :refer [User]]]
+            [metabase.test.util.log :as tu.log]
             [metabase.util :as u]
             [metabase.util.password :as upass]
             [toucan.db :as db]
@@ -56,7 +57,8 @@
                                       :dataset_query {:database (u/get-id database)
                                                       :type     :native
                                                       :native   {:query 1000}}}]]
-    (#'migrations/add-legacy-sql-directive-to-bigquery-sql-cards)
+    (tu.log/suppress-output
+      (#'migrations/add-legacy-sql-directive-to-bigquery-sql-cards))
     (-> (db/select-one-field :dataset_query Card :id (u/get-id card))
         (update :database integer?))))
 

test/metabase/driver/sqlserver_test.clj

@@ -4,11 +4,16 @@
             [metabase.driver
              [generic-sql :as sql]
              [sqlserver :as sqlserver]]
+            [metabase.query-processor :as qp]
             [metabase.test
              [data :as data]
              [util :as tu :refer [obj->json->obj]]]
+            [medley.core :as m]
+            [metabase.driver :as driver]
+            [metabase.query-processor-test :as qp.test]
+            [metabase.query-processor.test-util :as qp.test-util]
             [metabase.test.data
-             [datasets :refer [expect-with-engine]]
+             [datasets :as datasets :refer [expect-with-engine]]
              [interface :refer [def-database-definition]]]))
 
 ;;; -------------------------------------------------- VARCHAR(MAX) --------------------------------------------------
@@ -56,3 +61,86 @@
 (expect-with-engine :sqlserver
   "UTC"
   (tu/db-timezone-id))
+
+;; SQL Server doesn't let you use ORDER BY in nested SELECTs unless you also specify a TOP (their equivalent of
+;; LIMIT). Make sure we add a max-results LIMIT to the nested query
+(datasets/expect-with-engine :sqlserver
+  {:query  (str
+            "SELECT TOP 1048576 * "
+            "FROM ("
+            "SELECT TOP 1048576 "
+            "\"dbo\".\"venues\".\"name\" AS \"name\" "
+            "FROM \"dbo\".\"venues\" "
+            "ORDER BY \"dbo\".\"venues\".\"id\" ASC"
+            " ) \"source\" ") ; not sure why this generates an extra space before the closing paren, but it does
+   :params nil}
+  (qp/query->native
+    (data/$ids [venues {:wrap-field-ids? true}]
+      {:type     :query
+       :database (data/id)
+       :query    {:source-query {:source-table $$table
+                                 :fields       [$name]
+                                 :order-by     [[:asc $id]]}}})))
+
+;; make sure when adding TOP clauses to make ORDER BY work we don't stomp over any explicit TOP clauses that may have
+;; been set in the query
+(datasets/expect-with-engine :sqlserver
+  {:query (str "SELECT TOP 10 * "
+               "FROM ("
+               "SELECT TOP 20 "
+               "\"dbo\".\"venues\".\"name\" AS \"name\" "
+               "FROM \"dbo\".\"venues\" "
+               "ORDER BY \"dbo\".\"venues\".\"id\" ASC"
+               " ) \"source\" ")
+   :params nil}
+  (qp/query->native
+    (data/$ids [venues {:wrap-field-ids? true}]
+      {:type     :query
+       :database (data/id)
+       :query    {:source-query {:source-table $$table
+                                 :fields       [$name]
+                                 :order-by     [[:asc $id]]
+                                 :limit        20}
+                  :limit        10}})))
+
+;; We don't need to add TOP clauses for top-level order by. Normally we always add one anyway because of the
+;; max-results stuff, but make sure our impl doesn't add one when it's not in the source MBQL
+(datasets/expect-with-engine :sqlserver
+  {:query (str "SELECT * "
+               "FROM ("
+               "SELECT TOP 1048576 "
+               "\"dbo\".\"venues\".\"name\" AS \"name\" "
+               "FROM \"dbo\".\"venues\" "
+               "ORDER BY \"dbo\".\"venues\".\"id\" ASC"
+               " ) \"source\" "
+               "ORDER BY \"source\".\"id\" ASC")
+   :params nil}
+  ;; in order to actually see how things would work without the implicit max-results limit added we'll preprocess
+  ;; the query, strip off the `:limit` that got added, and then feed it back to the QP where we left off
+  (let [preprocessed (-> (qp/query->preprocessed
+                           (data/$ids [venues {:wrap-field-ids? true}]
+                             {:type     :query
+                              :database (data/id)
+                              :query    {:source-query {:source-table $$table
+                                                        :fields       [$name]
+                                                        :order-by     [[:asc $id]]}
+                                         :order-by     [[:asc $id]]}}))
+                         (m/dissoc-in [:query :limit]))]
+    (qp.test-util/with-everything-store
+      (driver/mbql->native (driver/engine->driver :sqlserver) preprocessed))))
+
+;; ok, generating all that SQL above is nice, but let's make sure our queries actually work!
+(datasets/expect-with-engine :sqlserver
+  [["Red Medicine"]
+   ["Stout Burgers & Beers"]
+   ["The Apple Pan"]]
+  (qp.test/rows
+    (qp/process-query
+      (data/$ids [venues {:wrap-field-ids? true}]
+        {:type     :query
+         :database (data/id)
+         :query    {:source-query {:source-table $$table
+                                   :fields       [$name]
+                                   :order-by     [[:asc $id]]
+                                   :limit        5}
+                    :limit        3}}))))

test/metabase/query_processor/middleware/binning_test.clj

@@ -2,7 +2,7 @@
   (:require [expectations :refer [expect]]
             [metabase.models.field :as field :refer [Field]]
             [metabase.query-processor.middleware.binning :as binning]
-            [metabase.query-processor.store :as qp.store]
+            [metabase.query-processor.test-util :as qp.test-util]
             [metabase.test.data :as data]
             [metabase.util :as u]
             [toucan.util.test :as tt]))
@@ -122,8 +122,7 @@
                               {:min-value 0.0, :max-value 240.0, :num-bins 8, :bin-width 30}]]}
    :type     :query
    :database (data/id)}
-  (qp.store/with-store
-    (qp.store/store-field! field)
+  (qp.test-util/with-everything-store
     ((binning/update-binning-strategy identity)
      {:query    {:source-table (data/id :checkins)
                  :breakout     [[:binning-strategy [:field-id (u/get-id field)] :default]]}

test/metabase/query_processor/middleware/resolve_joined_tables_test.clj

 (ns metabase.query-processor.middleware.resolve-joined-tables-test
   (:require [expectations :refer :all]
-            [metabase.models
-             [field :refer [Field]]
-             [table :refer [Table]]]
             [metabase.query-processor.middleware.resolve-joined-tables :as resolve-joined-tables]
-            [metabase.query-processor.store :as qp.store]
-            [metabase.test.data :as data]))
+            [metabase.query-processor.test-util :as qp.test-util]
+            [metabase.test.data :as data]
+            [metabase.query-processor.store :as qp.store]))
 
+(defn- resolve-joined-tables [query]
+  (qp.test-util/with-everything-store
+    ((resolve-joined-tables/resolve-joined-tables identity) {:database (data/id)
+                                                             :type     :query
+                                                             :query    query})))
+
+;; make sure `:join-tables` get added automatically for `:fk->` clauses
+(expect
+  {:database (data/id)
+   :type     :query
+   :query    (data/$ids venues
+               {:source-table $$table
+                :fields       [[:field-id $name]
+                               [:fk-> [:field-id $category_id] [:field-id $categories.name]]]
+                :join-tables  [{:join-alias  "CATEGORIES__via__CATEGORY_ID"
+                                :table-id    (data/id :categories)
+                                :fk-field-id $category_id
+                                :pk-field-id $categories.id}]})}
+  (resolve-joined-tables
+   (data/$ids venues
+     {:source-table $$table
+      :fields       [[:field-id $name]
+                     [:fk-> [:field-id $category_id] [:field-id $categories.name]]]})))
+
+;; For FK clauses inside nested source queries, we should add the `:join-tables` info to the nested query instead of
+;; at the top level (#8972)
+(expect
+  {:database (data/id)
+   :type     :query
+   :query    {:source-query
+              (data/$ids venues
+                {:source-table $$table
+                 :fields       [[:field-id $name]
+                                [:fk-> [:field-id $category_id] [:field-id $categories.name]]]
+                 :join-tables  [{:join-alias  "CATEGORIES__via__CATEGORY_ID"
+                                 :table-id    (data/id :categories)
+                                 :fk-field-id $category_id
+                                 :pk-field-id $categories.id}]})}}
+  (resolve-joined-tables
+   {:source-query
+    (data/$ids venues
+      {:source-table $$table
+       :fields       [[:field-id $name]
+                      [:fk-> [:field-id $category_id] [:field-id $categories.name]]]})}))
+
+;; we should handle nested-nested queries correctly as well
+(expect
+  {:database (data/id)
+   :type     :query
+   :query    {:source-query
+              {:source-query
+               (data/$ids venues
+                 {:source-table $$table
+                  :fields       [[:field-id $name]
+                                 [:fk-> [:field-id $category_id] [:field-id $categories.name]]]
+                  :join-tables  [{:join-alias  "CATEGORIES__via__CATEGORY_ID"
+                                  :table-id    (data/id :categories)
+                                  :fk-field-id $category_id
+                                  :pk-field-id $categories.id}]})}}}
+  (resolve-joined-tables
+   {:source-query
+    {:source-query
+     (data/$ids venues
+       {:source-table $$table
+        :fields       [[:field-id $name]
+                       [:fk-> [:field-id $category_id] [:field-id $categories.name]]]})}}))
+
+;; ok, so apparently if you specify a source table at a deeper level of nesting we should still add JOINs as
+;; appropriate for that Table if you specify an `fk->` clause in an a higher level. Does this make any sense at all?
+;;
+;; TODO - I'm not sure I understand why we add the JOIN to the outer level in this case. Does it make sense?
 (expect
   {:database (data/id)
    :type     :query
-   :query    {:source-table (data/id :venues)
-              :fields       [[:field-id (data/id :venues :name)]
-                             [:fk->
-                              [:field-id (data/id :venues :category_id)]
-                              [:field-id (data/id :categories :name)]]]
-              :join-tables  [{:join-alias  "CATEGORIES__via__CATEGORY_ID"
-                              :table-id    (data/id :categories)
-                              :fk-field-id (data/id :venues :category_id)
-                              :pk-field-id (data/id :categories :id)}]}}
-  (qp.store/with-store
-    (qp.store/store-table! (Table (data/id :venues)))
-    (doseq [field-id [(data/id :venues :name)
-                      (data/id :categories :name)
-                      (data/id :venues :category_id)]]
-      (qp.store/store-field! (Field field-id)))
-    ((resolve-joined-tables/resolve-joined-tables identity)
-     {:database (data/id)
-      :type     :query
-      :query    {:source-table (data/id :venues)
-                 :fields       [[:field-id (data/id :venues :name)]
-                                [:fk->
-                                 [:field-id (data/id :venues :category_id)]
-                                 [:field-id (data/id :categories :name)]]]}})))
+   :query    (data/$ids checkins
+               {:source-query {:source-table $$table
+                               :filter       [:> [:field-id $date] "2014-01-01"]}
+                :aggregation  [[:count]]
+                :breakout     [[:fk-> [:field-id $venue_id] [:field-id $venues.price]]]
+                :order-by     [[:asc [:fk-> [:field-id $venue_id] [:field-id $venues.price]]]]
+                :join-tables  [{:join-alias  "VENUES__via__VENUE_ID"
+                                :table-id    (data/id :venues)
+                                :fk-field-id $venue_id
+                                :pk-field-id $venues.id}]})}
+  (resolve-joined-tables
+   (data/$ids [checkins {:wrap-field-ids? true}]
+     {:source-query {:source-table $$table
+                     :filter       [:> $date "2014-01-01"]}
+      :aggregation  [[:count]]
+      :breakout     [$venue_id->venues.price]
+      :order-by     [[:asc $venue_id->venues.price]]})))

test/metabase/query_processor/middleware/wrap_value_literals_test.clj

 (ns metabase.query-processor.middleware.wrap-value-literals-test
   (:require [expectations :refer :all]
-            [metabase.models.field :refer [Field]]
             [metabase.query-processor.middleware.wrap-value-literals :as wrap-value-literals]
-            [metabase.query-processor.store :as qp.store]
+            [metabase.query-processor.test-util :as qp.test-util]
             [metabase.test.data :as data]
             [metabase.util.date :as du]))
 
-(defn- wrap-value-literals {:style/indent 1} [field-ids-to-put-in-store inner-query]
-  (qp.store/with-store
-    (doseq [field-id field-ids-to-put-in-store]
-      (qp.store/store-field! (Field field-id)))
+(defn- wrap-value-literals {:style/indent 0} [inner-query]
+  (qp.test-util/with-everything-store
     (binding [du/*report-timezone* (java.util.TimeZone/getTimeZone "UTC")]
       (-> ((wrap-value-literals/wrap-value-literals identity)
            {:database (data/id)
@@ -26,7 +23,7 @@
                                 :special_type  :type/PK
                                 :database_type "BIGINT"}]]})
   (data/$ids venues
-    (wrap-value-literals [$id]
+    (wrap-value-literals
       {:source-table (data/id :venues)
        :filter       [:> [:field-id $id] 50]})))
 
@@ -41,7 +38,7 @@
                                                       :special_type  :type/Category
                                                       :database_type "INTEGER"}]]]})
   (data/$ids venues
-    (wrap-value-literals [$id $price]
+    (wrap-value-literals
       {:source-table (data/id :venues)
        :filter       [:and
                       [:> [:field-id $id] 50]
@@ -55,7 +52,7 @@
                     [:datetime-field [:field-id $date] :month]
                     [:absolute-datetime (du/->Timestamp "2018-10-01" "UTC") :month]]})
   (data/$ids checkins
-    (wrap-value-literals [$date]
+    (wrap-value-literals
       {:source-table (data/id :checkins)
        :filter       [:= [:datetime-field [:field-id $date] :month] "2018-10-01"]})))
 
@@ -68,7 +65,7 @@
                     [:field-id $date]
                     [:absolute-datetime (du/->Timestamp "2018-10-01" "UTC") :default]]})
   (data/$ids checkins
-    (wrap-value-literals [$date]
+    (wrap-value-literals
       {:source-table (data/id :checkins)
        :filter       [:= [:field-id $date] "2018-10-01"]})))
 
@@ -87,7 +84,7 @@
 
   (data/dataset sad-toucan-incidents
     (data/$ids incidents
-      (wrap-value-literals [$timestamp]
+      (wrap-value-literals
         {:source-table (data/id :incidents)
          :filter       [:and
                         [:> [:datetime-field [:field-id $timestamp] :day] "2015-06-01"]
@@ -104,7 +101,7 @@
                                            :database_type "DATE"
                                            :unit          :month}]]})
   (data/$ids checkins
-    (wrap-value-literals [$date]
+    (wrap-value-literals
       {:source-table (data/id :checkins)
        :filter       [:starts-with [:datetime-field [:field-id $date] :month] "2018-10-01"]})))
 
@@ -116,6 +113,6 @@
                                    [:field-id $date]
                                    [:absolute-datetime #inst "2014-01-01T00:00:00.000000000-00:00" :default]]}})
   (data/$ids checkins
-    (wrap-value-literals [$date]
+    (wrap-value-literals
       {:source-query {:source-table (data/id :checkins)
                       :filter       [:> [:field-id $date] "2014-01-01"]}})))

test/metabase/query_processor/test_util.clj

+(ns metabase.query-processor.test-util
+  "Utilities for writing Query Processor tests that test internal workings of the QP rather than end-to-end results,
+  e.g. middleware tests."
+  (:require [metabase.models
+             [field :refer [Field]]
+             [table :refer [Table]]]
+            [metabase.query-processor.store :as qp.store]
+            [toucan.db :as db]))
+
+(defn do-with-everything-store
+  "Impl for `with-everything-store`."
+  [f]
+  (with-redefs [qp.store/table (fn [table-id]
+                                 (or (get-in @@#'qp.store/*store* [:tables table-id])
+                                     (db/select-one (vec (cons Table qp.store/table-columns-to-fetch)), :id table-id)))
+                qp.store/field (fn [field-id]
+                                 (or (get-in @@#'qp.store/*store* [:fields field-id])
+                                     (db/select-one (vec (cons Field qp.store/field-columns-to-fetch)), :id field-id)))]
+    (qp.store/with-store
+      (f))))
+
+(defmacro with-everything-store
+  "When testing a specific piece of middleware, you often need to load things into the QP store, but doing so can be
+  tedious. This macro swaps out the normal QP store backend with one that fetches Tables and Fields from the DB
+  on-demand, making tests a lot nicer to write."
+  [& body]
+  `(do-with-everything-store (fn [] ~@body)))

test/metabase/query_processor_test/nested_queries_test.clj

@@ -131,6 +131,7 @@
                     :order-by     [[:asc [:fk-> (data/id :checkins :venue_id) (data/id :venues :price)]]]
                     :breakout     [[:fk-> (data/id :checkins :venue_id) (data/id :venues :price)]]}}))))
 
+
 ;; Test two breakout columns from the nested query, both following an FK
 (datasets/expect-with-engines (non-timeseries-engines-with-feature :nested-queries :foreign-keys)
   {:rows [[2 33.7701 7]
@@ -665,3 +666,22 @@
                                                   (data/id :venues :price)]}
                     :aggregation  [[:count]]
                     :filter       [:= [:field-id (data/id :venues :category_id)] 50]}}))))
+
+;; make sure that if a nested query includes joins queries based on it still work correctly (#8972)
+(datasets/expect-with-engines (non-timeseries-engines-with-feature :nested-queries :foreign-keys)
+  [[31 "Bludso's BBQ"         5 33.8894 -118.207 2]
+   [32 "Boneyard Bistro"      5 34.1477 -118.428 3]
+   [33 "My Brother's Bar-B-Q" 5 34.167  -118.595 2]
+   [35 "Smoke City Market"    5 34.1661 -118.448 1]
+   [37 "bigmista's barbecue"  5 34.118  -118.26  2]
+   [38 "Zeke's Smokehouse"    5 34.2053 -118.226 2]
+   [39 "Baby Blues BBQ"       5 34.0003 -118.465 2]]
+  (format-rows-by [int str int (partial u/round-to-decimals 4) (partial u/round-to-decimals 4) int]
+    (rows
+      (qp/process-query
+        (data/$ids [venues {:wrap-field-ids? true}]
+          {:type     :query
+           :database (data/id)
+           :query    {:source-query {:source-table $$table
+                                     :filter       [:= $venues.category_id->categories.name "BBQ"]
+                                     :order-by     [[:asc $id]]}}})))))

test/metabase/test/data.clj

@@ -5,6 +5,7 @@
              [string :as str]
              [walk :as walk]]
             [clojure.tools.logging :as log]
+            [medley.core :as m]
             [metabase
              [driver :as driver]
              [query-processor :as qp]
@@ -20,7 +21,6 @@
              [dataset-definitions :as defs]
              [datasets :refer [*driver*]]
              [interface :as i]]
-            [metabase.test.util.log :as tu.log]
             [toucan.db :as db])
   (:import [metabase.test.data.interface DatabaseDefinition TableDefinition]))
 
@@ -118,10 +118,20 @@
 
   Use `$$table` to refer to the table itself.
 
-    $$table -> (id :venues)"
-  {:style/indent 1}
-  [table-name & body]
-  ($->id (keyword table-name) `(do ~@body) :wrap-field-ids? false))
+    $$table -> (id :venues)
+
+  You can pass options by wrapping `table-name` in a vector:
+
+    ($ids [venues {:wrap-field-ids? true}]
+      $category_id->categories.name)
+    ;; -> [:fk-> [:field-id (id :venues :category_id(] [:field-id (id :categories :name)]]"
+  {:arglists '([table & body] [[table {:keys [wrap-field-ids?]}] & body]), :style/indent 1}
+  [table-and-options & body]
+  (let [[table-name options] (if (sequential? table-and-options)
+                               table-and-options
+                               [table-and-options])]
+    (m/mapply $->id (keyword table-name) `(do ~@body) (merge {:wrap-field-ids? false}
+                                                             options))))
 
 
 (defn wrap-inner-mbql-query