Merge pull request #5260 from metabase/more-backports-from-nested-queries-branch

More improvements backported from nested-queries

src/metabase/models/card.clj

@@ -17,6 +17,7 @@
              [permissions :as perms]
              [revision :as revision]]
             [metabase.query-processor.middleware.permissions :as qp-perms]
+            [metabase.query-processor.util :as qputil]
             [toucan
              [db :as db]
              [models :as models]]))
@@ -43,36 +44,58 @@
 
 ;;; ------------------------------------------------------------ Permissions Checking ------------------------------------------------------------
 
-(defn- permissions-path-set:mbql [{database-id :database, :as query}]
-  {:pre [(integer? database-id) (map? (:query query))]}
-  (try (let [{{:keys [source-table join-tables]} :query} (qp/expand query)]
-         (set (for [table (cons source-table join-tables)]
-                (perms/object-path database-id
-                                   (:schema table)
-                                   (or (:id table) (:table-id table))))))
+(defn- native-permissions-path
+  "Return the `:read` (for running) or `:write` (for saving) native permissions path for DATABASE-OR-ID."
+  [read-or-write database-or-id]
+  ((case read-or-write
+     :read  perms/native-read-path
+     :write perms/native-readwrite-path) (u/get-id database-or-id)))
+
+(defn- query->source-and-join-tables
+  "Return a sequence of all Tables (as TableInstance maps) referenced by QUERY."
+  [{:keys [source-table join-tables native], :as query}]
+  (cond
+    ;; if we come across a native query just put a placeholder (`::native`) there so we know we need to add native permissions to the complete set below.
+    native       [::native]
+    ;; for root MBQL queries just return source-table + join-tables
+    :else        (cons source-table join-tables)))
+
+(defn- tables->permissions-path-set
+  "Given a sequence of TABLES referenced by a query, return a set of required permissions."
+  [read-or-write database-or-id tables]
+  (set (for [table tables]
+         (if (= ::native table)
+           ;; Any `::native` placeholders from above mean we need READ-OR-WRITE native permissions for this DATABASE
+           (native-permissions-path read-or-write database-or-id)
+           ;; anything else (i.e., a normal table) just gets normal table permissions
+           (perms/object-path (u/get-id database-or-id)
+                              (:schema table)
+                              (or (:id table) (:table-id table)))))))
+
+(defn- mbql-permissions-path-set
+  "Return the set of required permissions needed to run QUERY."
+  [read-or-write query]
+  {:pre [(map? query) (map? (:query query))]}
+  (try (let [{:keys [query database]} (qp/expand query)]
+         (tables->permissions-path-set read-or-write database (query->source-and-join-tables query)))
        ;; if for some reason we can't expand the Card (i.e. it's an invalid legacy card)
        ;; just return a set of permissions that means no one will ever get to see it
        (catch Throwable e
          (log/warn "Error getting permissions for card:" (.getMessage e) "\n" (u/pprint-to-str (u/filtered-stacktrace e)))
-         #{"/db/0/"}))) ; DB 0 will never exist
-
-(defn- permissions-path-set:native [read-or-write {database-id :database}]
-  #{((case read-or-write
-       :read  perms/native-read-path
-       :write perms/native-readwrite-path) database-id)})
+         #{"/db/0/"})))                        ; DB 0 will never exist
 
 ;; it takes a lot of DB calls and function calls to expand/resolve a query, and since they're pure functions we can save ourselves some a lot of DB calls
 ;; by caching the results. Cache the permissions reqquired to run a given query dictionary for up to 6 hours
-(defn- query-perms-set* [{query-type :type, :as query} read-or-write]
+(defn- query-perms-set* [{query-type :type, database :database, :as query} read-or-write]
   (cond
     (= query {})                     #{}
-    (= (keyword query-type) :native) (permissions-path-set:native read-or-write query)
-    (= (keyword query-type) :query)  (permissions-path-set:mbql query)
+    (= (keyword query-type) :native) #{(native-permissions-path read-or-write database)}
+    (= (keyword query-type) :query)  (mbql-permissions-path-set read-or-write query)
     :else                            (throw (Exception. (str "Invalid query type: " query-type)))))
 
 (def ^{:arglists '([query read-or-write])} query-perms-set
   "Return a set of required permissions for *running* QUERY (if READ-OR-WRITE is `:read`) or *saving* it (if READ-OR-WRITE is `:write`)."
-  (memoize/ttl query-perms-set* :ttl/threshold (* 6 60 60 1000)))
+  (memoize/ttl query-perms-set* :ttl/threshold (* 6 60 60 1000))) ; memoize for 6 hours
 
 
 (defn- perms-objects-set
@@ -111,17 +134,21 @@
 
 ;;; ------------------------------------------------------------ Lifecycle ------------------------------------------------------------
 
-
-(defn- populate-query-fields [card]
-  (let [{query :query, database-id :database, query-type :type} (:dataset_query card)
-        table-id (or (:source_table query) ; legacy (MBQL '95)
-                     (:source-table query))
-        defaults {:database_id database-id
-                  :table_id    table-id
-                  :query_type  (keyword query-type)}]
-    (if query-type
-      (merge defaults card)
-      card)))
+(defn- query->database-and-table-ids
+  "Return a map with `:database-id` and source `:table-id` that should be saved for a Card."
+  [outer-query]
+  (let [database     (qputil/get-normalized outer-query :database)
+        source-table (qputil/get-in-normalized outer-query [:query :source-table])]
+    (when source-table
+      {:database-id (u/get-id database), :table-id (u/get-id source-table)})))
+
+(defn- populate-query-fields [{{query-type :type, :as outer-query} :dataset_query, :as card}]
+  (merge (when query-type
+           (let [{:keys [database-id table-id]} (query->database-and-table-ids outer-query)]
+             {:database_id database-id
+              :table_id    table-id
+              :query_type  (keyword query-type)}))
+         card))
 
 (defn- pre-insert [{:keys [dataset_query], :as card}]
   ;; TODO - make sure if `collection_id` is specified that we have write permissions for tha tcollection

src/metabase/models/user.clj

 (ns metabase.models.user
   (:require [cemerick.friend.credentials :as creds]
             [clojure.string :as s]
+            [clojure.tools.logging :as log]
             [metabase
              [public-settings :as public-settings]
              [util :as u]]
@@ -42,12 +43,12 @@
   (u/prog1 user
     ;; add the newly created user to the magic perms groups
     (binding [perm-membership/*allow-changing-all-users-group-members* true]
-      #_(log/info (format "Adding user %d to All Users permissions group..." user-id))
+      (log/info (format "Adding user %d to All Users permissions group..." user-id))
       (db/insert! PermissionsGroupMembership
         :user_id  user-id
         :group_id (:id (group/all-users))))
     (when superuser?
-      #_(log/info (format "Adding user %d to Admin permissions group..." user-id))
+      (log/info (format "Adding user %d to Admin permissions group..." user-id))
       (db/insert! PermissionsGroupMembership
         :user_id  user-id
         :group_id (:id (group/admin))))))

src/metabase/query_processor.clj

@@ -61,34 +61,55 @@
 ;; PRE-PROCESSING fns are applied from bottom to top, and POST-PROCESSING from top to bottom;
 ;; the easiest way to wrap your head around this is picturing a the query as a ball being thrown in the air
 ;; (up through the preprocessing fns, back down through the post-processing ones)
+(defn- qp-pipeline
+  "Construct a new Query Processor pipeline with F as the final 'piviotal' function. e.g.:
+
+     All PRE-PROCESSING (query) --> F --> All POST-PROCESSING (result)
+
+   Or another way of looking at it is
+
+     (post-process (f (pre-process query)))
+
+   Normally F is something that runs the query, like the `execute-query` function above, but this can be swapped out when we want to do things like
+   process a query without actually running it."
+  [f]
+  ;; ▼▼▼ POST-PROCESSING ▼▼▼  happens from TOP-TO-BOTTOM, e.g. the results of `f` are (eventually) passed to `limit`
+  (-> f
+      dev/guard-multiple-calls
+      mbql-to-native/mbql->native                      ; ▲▲▲ NATIVE-ONLY POINT ▲▲▲ Query converted from MBQL to native here; all functions *above* will only see the native query
+      annotate-and-sort/annotate-and-sort
+      perms/check-query-permissions
+      log-query/log-expanded-query
+      dev/check-results-format
+      limit/limit
+      cumulative-ags/handle-cumulative-aggregations
+      implicit-clauses/add-implicit-clauses
+      format-rows/format-rows
+      expand-resolve/expand-resolve                    ; ▲▲▲ QUERY EXPANSION POINT  ▲▲▲ All functions *above* will see EXPANDED query during PRE-PROCESSING
+      row-count-and-status/add-row-count-and-status    ; ▼▼▼ RESULTS WRAPPING POINT ▼▼▼ All functions *below* will see results WRAPPED in `:data` during POST-PROCESSING
+      parameters/substitute-parameters
+      expand-macros/expand-macros
+      driver-specific/process-query-in-context         ; (drivers can inject custom middleware if they implement IDriver's `process-query-in-context`)
+      add-settings/add-settings
+      resolve-driver/resolve-driver                    ; ▲▲▲ DRIVER RESOLUTION POINT ▲▲▲ All functions *above* will have access to the driver during PRE- *and* POST-PROCESSING
+      log-query/log-initial-query
+      cache/maybe-return-cached-results
+      catch-exceptions/catch-exceptions))
+;; ▲▲▲ PRE-PROCESSING ▲▲▲ happens from BOTTOM-TO-TOP, e.g. the results of `expand-macros` are (eventually) passed to `expand-resolve`
+
+(defn query->native
+  "Return the native form for QUERY (e.g. for a MBQL query on Postgres this would return a map containing the compiled SQL form)."
+  {:style/indent 0}
+  [query]
+  (-> ((qp-pipeline identity) query)
+      (get-in [:data :native_form])))
+
 (defn process-query
   "A pipeline of various QP functions (including middleware) that are used to process MB queries."
   {:style/indent 0}
   [query]
-  ;; ▼▼▼ POST-PROCESSING ▼▼▼  happens from TOP-TO-BOTTOM, e.g. the results of `run-query` are (eventually) passed to `limit`
-  ((-> execute-query
-       dev/guard-multiple-calls
-       mbql-to-native/mbql->native                   ; ▲▲▲ NATIVE-ONLY POINT ▲▲▲ Query converted from MBQL to native here; all functions *above* will only see the native query
-       annotate-and-sort/annotate-and-sort
-       perms/check-query-permissions
-       log-query/log-expanded-query
-       dev/check-results-format
-       limit/limit
-       cumulative-ags/handle-cumulative-aggregations
-       implicit-clauses/add-implicit-clauses
-       format-rows/format-rows
-       expand-resolve/expand-resolve                 ; ▲▲▲ QUERY EXPANSION POINT  ▲▲▲ All functions *above* will see EXPANDED query during PRE-PROCESSING
-       row-count-and-status/add-row-count-and-status ; ▼▼▼ RESULTS WRAPPING POINT ▼▼▼ All functions *below* will see results WRAPPED in `:data` during POST-PROCESSING
-       parameters/substitute-parameters
-       expand-macros/expand-macros
-       driver-specific/process-query-in-context      ; (drivers can inject custom middleware if they implement IDriver's `process-query-in-context`)
-       add-settings/add-settings
-       resolve-driver/resolve-driver                 ; ▲▲▲ DRIVER RESOLUTION POINT ▲▲▲ All functions *above* will have access to the driver during PRE- *and* POST-PROCESSING
-       log-query/log-initial-query
-       cache/maybe-return-cached-results
-       catch-exceptions/catch-exceptions)
-   query))
-;; ▲▲▲ PRE-PROCESSING ▲▲▲ happens from BOTTOM-TO-TOP, e.g. the results of `expand-macros` are (eventually) passed to `expand-resolve`
+  ((qp-pipeline execute-query) query))
+
 
 
 (def ^{:arglists '([query])} expand

test/metabase/events/activity_feed_test.clj

@@ -26,83 +26,84 @@
 
 
 ;; `:card-create` event
-(tt/expect-with-temp [Card [card {:name "My Cool Card"}]]
+(expect
   {:topic       :card-create
    :user_id     (user->id :rasta)
    :model       "card"
-   :model_id    (:id card)
    :database_id nil
    :table_id    nil
    :details     {:name "My Cool Card", :description nil}}
-  (with-temp-activities
-    (process-activity-event! {:topic :card-create, :item card})
-    (db/select-one [Activity :topic :user_id :model :model_id :database_id :table_id :details]
-      :topic    "card-create"
-      :model_id (:id card))))
+  (tt/with-temp Card [card {:name "My Cool Card"}]
+    (with-temp-activities
+      (process-activity-event! {:topic :card-create, :item card})
+      (db/select-one [Activity :topic :user_id :model :database_id :table_id :details]
+        :topic    "card-create"
+        :model_id (:id card)))))
+
 
 
 ;; `:card-update` event
-(tt/expect-with-temp [Card [card {:name "My Cool Card"}]]
+(expect
   {:topic       :card-update
    :user_id     (user->id :rasta)
    :model       "card"
-   :model_id    (:id card)
    :database_id nil
    :table_id    nil
    :details     {:name "My Cool Card", :description nil}}
-  (with-temp-activities
-    (process-activity-event! {:topic :card-update, :item card})
-    (db/select-one [Activity :topic :user_id :model :model_id :database_id :table_id :details]
-      :topic    "card-update"
-      :model_id (:id card))))
+  (tt/with-temp Card [card {:name "My Cool Card"}]
+    (with-temp-activities
+      (process-activity-event! {:topic :card-update, :item card})
+      (db/select-one [Activity :topic :user_id :model :database_id :table_id :details]
+        :topic    "card-update"
+        :model_id (:id card)))))
 
 
 ;; `:card-delete` event
-(tt/expect-with-temp [Card [card {:name "My Cool Card"}]]
+(expect
   {:topic       :card-delete
    :user_id     (user->id :rasta)
    :model       "card"
-   :model_id    (:id card)
    :database_id nil
    :table_id    nil
    :details     {:name "My Cool Card", :description nil}}
-  (with-temp-activities
-    (process-activity-event! {:topic :card-delete, :item card})
-    (db/select-one [Activity :topic :user_id :model :model_id :database_id :table_id :details]
-      :topic    "card-delete"
-      :model_id (:id card))))
+  (tt/with-temp Card [card {:name "My Cool Card"}]
+    (with-temp-activities
+      (process-activity-event! {:topic :card-delete, :item card})
+      (db/select-one [Activity :topic :user_id :model :database_id :table_id :details]
+        :topic    "card-delete"
+        :model_id (:id card)))))
 
 
 ;; `:dashboard-create` event
-(tt/expect-with-temp [Dashboard [dashboard {:name "My Cool Dashboard"}]]
+(expect
   {:topic       :dashboard-create
    :user_id     (user->id :rasta)
    :model       "dashboard"
-   :model_id    (:id dashboard)
    :database_id nil
    :table_id    nil
    :details     {:name "My Cool Dashboard", :description nil}}
-  (with-temp-activities
-    (process-activity-event! {:topic :dashboard-create, :item dashboard})
-    (db/select-one [Activity :topic :user_id :model :model_id :database_id :table_id :details]
-      :topic    "dashboard-create"
-      :model_id (:id dashboard))))
+  (tt/with-temp Dashboard [dashboard {:name "My Cool Dashboard"}]
+    (with-temp-activities
+      (process-activity-event! {:topic :dashboard-create, :item dashboard})
+      (db/select-one [Activity :topic :user_id :model :database_id :table_id :details]
+        :topic    "dashboard-create"
+        :model_id (:id dashboard)))))
 
 
 ;; `:dashboard-delete` event
-(tt/expect-with-temp [Dashboard [dashboard {:name "My Cool Dashboard"}]]
+(expect
   {:topic       :dashboard-delete
    :user_id     (user->id :rasta)
    :model       "dashboard"
-   :model_id    (:id dashboard)
    :database_id nil
    :table_id    nil
    :details     {:name "My Cool Dashboard", :description nil}}
-  (with-temp-activities
-    (process-activity-event! {:topic :dashboard-delete, :item dashboard})
-    (db/select-one [Activity :topic :user_id :model :model_id :database_id :table_id :details]
-      :topic    "dashboard-delete"
-      :model_id (:id dashboard))))
+  (tt/with-temp Dashboard [dashboard {:name "My Cool Dashboard"}]
+    (with-temp-activities
+      (process-activity-event! {:topic :dashboard-delete, :item dashboard})
+      (db/select-one [Activity :topic :user_id :model :database_id :table_id :details]
+        :topic    "dashboard-delete"
+        :model_id (:id dashboard)))))
 
 
 ;; `:dashboard-add-cards` event

test/metabase/query_processor_test/middleware/limit_test.clj → test/metabase/query_processor/middleware/limit_test.clj

-(ns metabase.query-processor-test.middleware.limit-test
+(ns metabase.query-processor.middleware.limit-test
   "Tests for the `:limit` clause and `:max-results` constraints."
   (:require [expectations :refer :all]
             [metabase.query-processor.interface :as i]

test/metabase/test/data/bigquery.clj

@@ -147,7 +147,7 @@
     (for [[i row] (m/indexed rows)]
       (assoc (zipmap field-names (for [v row]
                                    (u/prog1 (if (instance? java.util.Date v)
-                                              (DateTime. v)             ; convert to Google version of DateTime, otherwise it doesn't work (!)
+                                              (DateTime. ^java.util.Date v) ; convert to Google version of DateTime, otherwise it doesn't work (!)
                                               v)
                                             (assert (not (nil? <>)))))) ; make sure v is non-nil
              :id (inc i)))))