BE: add `based_on_upload` to `GET /api/collections/:collection/items` response (#38322)

enterprise/backend/test/metabase_enterprise/upload_test.clj

@@ -32,15 +32,29 @@
                    ["venues"
                     [{:field-name "name" :base-type :type/Text}]
                     [["something"]]])
-        (mt/with-temp [:model/Database {db-id :id}    {:engine "h2"}
-                       :model/Table    {table-id :id} {:db_id     db-id
-                                                       :is_upload true}
-                       :model/Card     {card-id :id}  {:dataset       true
-                                                       :dataset_query {:type     :query
-                                                                       :database db-id
-                                                                       :query    {:source-table table-id}}}]
-          (testing "Sanity check: if the user is not sandboxed, based_on_upload is non-nil"
-            (is (= table-id (:based_on_upload (mt/user-http-request :crowberto :get 200 (str "card/" card-id))))))
-          (testing "If the user is sandboxed, based_on_upload is nil"
-           (met/with-gtaps-for-user :rasta {:gtaps {:venues {}}}
-             (is (=? {:based_on_upload nil} (mt/user-http-request :rasta :get 200 (str "card/" card-id)))))))))))
+        (mt/with-temp [:model/Collection collection     {}
+                       :model/Database   {db-id :id}    {:engine "h2"}
+                       :model/Table      {table-id :id} {:db_id     db-id
+                                                         :is_upload true}
+                       :model/Card       {card-id :id
+                                          :as card}     {:collection_id (:id collection)
+                                                         :dataset       true
+                                                         :dataset_query {:type     :query
+                                                                         :database db-id
+                                                                         :query    {:source-table table-id}}}]
+          (let [get-card (fn [] (mt/user-http-request :rasta :get 200 (str "card/" card-id)))
+                get-collection-item (fn []
+                                      (->> (mt/user-http-request :rasta :get 200 (str "collection/" (:collection_id card) "/items?models=dataset"))
+                                           :data
+                                           (filter (fn [item]
+                                                     (= (:id item) (:id card))))
+                                           first))]
+            (testing "Sanity check: if the user is not sandboxed, based_on_upload is non-nil"
+              (is (= table-id
+                     (:based_on_upload (get-card))
+                     (:based_on_upload (get-collection-item)))))
+            (testing "If the user is sandboxed, based_on_upload is nil"
+              (met/with-gtaps-for-user :rasta {:gtaps {:venues {}}}
+                (is (= nil
+                       (:based_on_upload (get-card))
+                       (:based_on_upload (get-collection-item))))))))))))

src/metabase/api/collection.clj

@@ -32,6 +32,7 @@
     :as premium-features
     :refer [defenterprise]]
    [metabase.server.middleware.offset-paging :as mw.offset-paging]
+   [metabase.upload :as upload]
    [metabase.util :as u]
    [metabase.util.honey-sql-2 :as h2x]
    [metabase.util.i18n :refer [tru]]
@@ -322,7 +323,7 @@
   (for [row rows]
     (dissoc row
             :description :display :authority_level :moderated_status :icon :personal_owner_id
-            :collection_preview :dataset_query)))
+            :collection_preview :dataset_query :table_id :query_type :is_upload)))
 
 (defenterprise snippets-collection-children-query
   "Collection children query for snippets on OSS. Returns all snippets regardless of collection, because snippet
@@ -351,7 +352,7 @@
   (for [row rows]
     (dissoc row
             :description :display :collection_position :authority_level :moderated_status
-            :collection_preview :dataset_query)))
+            :collection_preview :dataset_query :table_id :query_type :is_upload)))
 
 (defmethod post-process-collection-children :snippet
   [_ rows]
@@ -359,7 +360,7 @@
     (dissoc row
             :description :collection_position :display :authority_level
             :moderated_status :icon :personal_owner_id :collection_preview
-            :dataset_query)))
+            :dataset_query :table_id :query_type :is_upload)))
 
 (defn- card-query [dataset? collection {:keys [archived? pinned-state]}]
   (-> {:select    (cond->
@@ -394,6 +395,9 @@
                    [:= :collection_id (:id collection)]
                    [:= :archived (boolean archived?)]
                    [:= :dataset dataset?]]}
+      (cond-> dataset?
+        (-> (sql.helpers/select :c.table_id :t.is_upload :c.query_type)
+            (sql.helpers/left-join [:metabase_table :t] [:= :t.id :c.table_id])))
       (sql.helpers/where (pinned-state->clause pinned-state))))
 
 (defmethod collection-children-query :dataset
@@ -402,7 +406,15 @@
 
 (defmethod post-process-collection-children :dataset
   [_ rows]
-  (post-process-collection-children :card rows))
+  (let [dataset_queries (map :dataset_query rows)]
+    (->> rows
+         ;; normalize dataset queries just for based_on_upload hydration
+         (map #(update % :dataset_query (comp mbql.normalize/normalize json/parse-string)))
+         upload/model-hydrate-based-on-upload
+         (map (fn [dataset_query row]
+                (assoc row :dataset_query dataset_query))
+              dataset_queries)
+         (post-process-collection-children :card))))
 
 (defmethod collection-children-query :card
   [_ collection options]
@@ -450,7 +462,7 @@
 
 (defn- post-process-card-row [row]
   (-> row
-      (dissoc :authority_level :icon :personal_owner_id :dataset_query)
+      (dissoc :authority_level :icon :personal_owner_id :dataset_query :table_id :query_type :is_upload)
       (update :collection_preview api/bit->boolean)
       (assoc :fully_parameterized (fully-parameterized-query? row))))
 
@@ -485,7 +497,7 @@
   [_ rows]
   (map #(dissoc %
                 :display :authority_level :moderated_status :icon :personal_owner_id :collection_preview
-                :dataset_query)
+                :dataset_query :table_id :query_type :is_upload)
        rows))
 
 (defenterprise snippets-collection-filter-clause
@@ -535,7 +547,7 @@
       (-> row
           (assoc :can_write (mi/can-write? Collection (:id row)))
           (dissoc :collection_position :display :moderated_status :icon
-                  :collection_preview :dataset_query)
+                  :collection_preview :dataset_query :table_id :query_type :is_upload)
           update-personal-collection))))
 
 (mu/defn ^:private coalesce-edit-info :- last-edit/MaybeAnnotated
@@ -597,7 +609,9 @@
   [:id :name :description :entity_id :display [:collection_preview :boolean] :dataset_query
    :model :collection_position :authority_level [:personal_owner_id :integer]
    :last_edit_email :last_edit_first_name :last_edit_last_name :moderated_status :icon
-   [:last_edit_user :integer] [:last_edit_timestamp :timestamp] [:database_id :integer]])
+   [:last_edit_user :integer] [:last_edit_timestamp :timestamp] [:database_id :integer]
+   ;; for determining whether a model is based on a csv-uploaded table
+   [:table_id :integer] [:is_upload :boolean] :query_type])
 
 (defn- add-missing-columns
   "Ensures that all necessary columns are in the select-columns collection, adding `[nil :column]` as necessary."

src/metabase/lib/core.cljc

@@ -10,6 +10,7 @@
    [metabase.lib.card :as lib.card]
    [metabase.lib.column-group :as lib.column-group]
    [metabase.lib.common :as lib.common]
+   [metabase.lib.convert :as lib.convert]
    [metabase.lib.database :as lib.database]
    [metabase.lib.drill-thru :as lib.drill-thru]
    [metabase.lib.drill-thru.pivot :as lib.drill-thru.pivot]
@@ -34,6 +35,7 @@
    [metabase.lib.stage :as lib.stage]
    [metabase.lib.table :as lib.table]
    [metabase.lib.temporal-bucket :as lib.temporal-bucket]
+   [metabase.lib.util :as lib.util]
    [metabase.shared.util.namespaces :as shared.ns]))
 
 (comment lib.aggregation/keep-me
@@ -42,6 +44,7 @@
          lib.card/keep-me
          lib.column-group/keep-me
          lib.common/keep-me
+         lib.convert/keep-me
          lib.database/keep-me
          lib.drill-thru/keep-me
          lib.drill-thru.pivot/keep-me
@@ -63,7 +66,8 @@
          lib.segment/keep-me
          lib.stage/keep-me
          lib.table/keep-me
-         lib.temporal-bucket/keep-me)
+         lib.temporal-bucket/keep-me
+         lib.util/keep-me)
 
 (shared.ns/import-fns
  [lib.aggregation
@@ -106,6 +110,8 @@
   group-columns]
  [lib.common
   external-op]
+ [lib.convert
+  ->pMBQL]
  [lib.database
   database-id]
  [lib.drill-thru
@@ -288,4 +294,6 @@
   describe-relative-datetime
   available-temporal-buckets
   temporal-bucket
-  with-temporal-bucket])
+  with-temporal-bucket]
+ [lib.util
+  source-table-id])

src/metabase/upload.clj

@@ -13,7 +13,6 @@
    [metabase.driver :as driver]
    [metabase.driver.sync :as driver.s]
    [metabase.driver.util :as driver.u]
-   [metabase.lib.convert :as lib.convert]
    [metabase.lib.core :as lib]
    [metabase.mbql.util :as mbql.u]
    [metabase.models :refer [Database]]
@@ -669,7 +668,6 @@
   (when-let [error (can-append-error db table)]
     (throw error)))
 
-;; This will be used in merge 2 of milestone 1 to populate a property on the table for the FE.
 (defn can-upload-to-table?
   "Returns true if the user can upload to the given database and table, and false otherwise."
   [db table]
@@ -694,7 +692,54 @@
 ;;; |  hydrate based_on_upload for FE
 ;;; +--------------------------------
 
-(mi/define-simple-hydration-method based-on-upload
+(defn uploadable-table-ids
+  "Returns the subset of table ids where the user can upload to the table."
+  [table-ids]
+  (if (empty? table-ids)
+    #{}
+    (let [tables (t2/hydrate (t2/select :model/Table :id [:in table-ids]) :db)]
+      (set (keep (fn [t]
+                   (when (can-upload-to-table? (:db t) t)
+                     (:id t)))
+                 tables)))))
+
+(defn- no-joins?
+  "Returns true if `query` has no joins in it, otherwise false."
+  [query]
+  (let [all-joins (mapcat (fn [stage]
+                            (lib/joins query stage))
+                          (range (lib/stage-count query)))]
+    (empty? all-joins)))
+
+(mu/defn model-hydrate-based-on-upload
+  "Batch hydrates `:based_on_upload` for each item of `models`. Assumes each item of `model` represents a model."
+  [models :- [:sequential [:map
+                           ;; query_type and dataset_query can be null in tests, so we make them nullable here.
+                           ;; they should never be null in production
+                           [:dataset_query [:maybe ms/Map]]
+                           [:query_type    [:maybe [:or :string :keyword]]]
+                           [:table_id      [:maybe ms/PositiveInt]]
+                           ;; is_upload can be provided for an optional optimization
+                           [:is_upload {:optional true} [:maybe :boolean]]]]]
+  (let [table-ids            (->> models
+                                  ;; as an optimization when listing collection items (GET /api/collection/items),
+                                  ;; we might already know that the table is not an upload if is_upload=false. We
+                                  ;; can skip making more queries if so
+                                  (remove #(false? (:is_upload %)))
+                                  (keep :table_id)
+                                  set)
+        uploadable-table-ids (set (uploadable-table-ids table-ids))
+        based-on-upload      (fn [model]
+                               (when-let [dataset_query (:dataset_query model)] ; dataset_query is sometimes null in tests
+                                 (let [query (lib/->pMBQL dataset_query)]
+                                   (when (and (= (name (:query_type model)) "query")
+                                              (contains? uploadable-table-ids (:table_id model))
+                                              (no-joins? query))
+                                     (lib/source-table-id query)))))]
+    (map #(m/assoc-some % :based_on_upload (based-on-upload %))
+         models)))
+
+(mi/define-batched-hydration-method based-on-upload
   :based_on_upload
   "Add based_on_upload=<table-id> to a card if:
     - the card is a model
@@ -703,14 +748,6 @@
     - the user has permissions to upload to the table
     - uploads are enabled
   Otherwise based_on_upload is nil."
-  [card]
-  (when (and (:dataset card)
-             (= (:query_type card) :query)
-             (let [query (lib.convert/->pMBQL (:dataset_query card))
-                   all-joins (mapcat (fn [stage]
-                                       (lib/joins query stage))
-                                     (range (lib/stage-count query)))]
-               (empty? all-joins))
-             (let [table (t2/select-one :model/Table :id (:table_id card))]
-               (can-upload-to-table? (table/database table) table)))
-    (:table_id card)))
+  [cards]
+  (let [models-by-id (m/index-by :id (model-hydrate-based-on-upload (filter :dataset cards)))]
+    (map #(or (models-by-id (:id %)) %) cards)))

test/metabase/api/card_test.clj

@@ -79,7 +79,6 @@
    :entity_id           nil
    :embedding_params    nil
    :made_public_by_id   nil
-   :based_on_upload     nil
    :parameters          []
    :parameter_mappings  []
    :moderation_reviews  ()
@@ -92,7 +91,7 @@
 
 ;; Used in dashboard tests
 (def card-defaults-no-hydrate
-  (dissoc card-defaults :average_query_time :last_query_start :based_on_upload))
+  (dissoc card-defaults :average_query_time :last_query_start))
 
 (defn mbql-count-query
   ([]
@@ -3316,55 +3315,63 @@
                   {:data {:cols [{:name "USER_ID"} {:name "pivot-grouping"} {:name "sum"}]}}
                   (mt/user-http-request :rasta :post 202 (format "card/pivot/%d/query" (u/the-id card)))))))))))
 
-(deftest based-on-upload-test
-  (mt/with-driver :h2 ;; just test on H2 because failure should be independent of drivers
+(defn run-based-on-upload-test
+  "Runs tests for based-on-upload `request` is a function that takes a card and returns a map which may have {:based_on_upload <table-id>}]
+  This function exists to deduplicate test logic for all API endpoints that must return `based_on_upload`,
+  including GET /api/collection/:id/items and GET /api/card/:id"
+  [request]
+  (mt/with-driver :h2 ; just test on H2 because failure should be independent of drivers
     (mt/with-temporary-setting-values [uploads-enabled true]
-      (mt/with-temp [:model/Database {db-id :id
-                                      :as   db}     {:engine "h2"}
-                     :model/Table    {table-id :id} {:db_id     db-id
-                                                     :is_upload true}
-                     :model/Card     {card-id :id}  {:dataset       true
-                                                     :dataset_query {:type     :query
-                                                                     :database db-id
-                                                                     :query    {:source-table table-id}}}]
-        (mt/with-test-user :crowberto
-          (testing "Cards based on uploads have based_on_upload=<table-id> if they meet all the criteria"
-            (is (= table-id (:based_on_upload (mt/user-http-request :crowberto :get 200 (str "card/" card-id))))))
-          (testing "If one of the criteria for appends is not met, based_on_upload should be nil."
-            (testing "\nIf the card is based on another card, which is based on the table, based_on_upload should be nil"
-              (mt/with-temp [:model/Card {card-id' :id} {:dataset       false
-                                                         :dataset_query {:type     :query
-                                                                         :database db-id
-                                                                         :query    {:source-table (str "card__" card-id)}}}]
-                (is (=? {:based_on_upload nil} (mt/user-http-request :crowberto :get 200 (str "card/" card-id'))))))
-            (testing "\nIf the card has a join in the query (even to itself), based_on_upload should be nil"
-              (mt/with-temp [:model/Card {card-id' :id} {:dataset       false
-                                                         :dataset_query {:type     :query
-                                                                         :database db-id
-                                                                         :query    {:source-table table-id
-                                                                                    :joins [{:fields       :all
-                                                                                             :source-table table-id
-                                                                                             :condition    [:= 1 2] ;; field-ids don't matter
-                                                                                             :alias        "SomeAlias"}]}}}]
-                (is (=? {:based_on_upload nil} (mt/user-http-request :crowberto :get 200 (str "card/" card-id'))))))
-            (testing "\nIf the table is not based on uploads, based_on_upload should be nil"
-              (t2/update! :model/Table table-id {:is_upload false})
-              (is (=? {:based_on_upload nil} (mt/user-http-request :crowberto :get 200 (str "card/" card-id))))
-              (t2/update! :model/Table table-id {:is_upload true}))
-            (testing "\nIf uploads are disabled, based_on_upload should be nil"
-              (mt/with-temp-copy-of-db
-                (perms/revoke-data-perms! (perms-group/all-users) db)
-                (is (=? {:based_on_upload nil} (mt/user-http-request :rasta :get 200 (str "card/" card-id))))))
-            (testing "\nIf uploads are disabled, based_on_upload should be nil"
-              (mt/with-temporary-setting-values [uploads-enabled false]
-                (is (=? {:based_on_upload nil} (mt/user-http-request :crowberto :get 200 (str "card/" card-id))))))
-            (testing "\nIf the card is not a model, based_on_upload should be nil"
-              (mt/with-temp [:model/Card {card-id' :id} {:dataset       false
-                                                         :dataset_query {:type     :query
-                                                                         :database db-id
-                                                                         :query    {:source-table table-id}}}]
-                (is (=? {:based_on_upload nil} (mt/user-http-request :crowberto :get 200 (str "card/" card-id'))))))
-            (testing "\nIf the card is a native query, based_on_upload should be nil"
-              (mt/with-temp [:model/Card {card-id' :id} {:dataset       true
-                                                         :dataset_query (mt/native-query {:native "select 1"})}]
-                (is (=? {:based_on_upload nil} (mt/user-http-request :crowberto :get 200 (str "card/" card-id'))))))))))))
+      (mt/with-temp [:model/Database   {db-id :id :as db}     {:engine "h2"}
+                     :model/Table      {table-id :id}         {:db_id db-id, :is_upload true}
+                     :model/Collection {collection-id :id}    {}]
+        (let [card-defaults {:collection_id collection-id
+                             :dataset       true
+                             :dataset_query {:type     :query
+                                             :database db-id
+                                             :query    {:source-table table-id}}}]
+          (mt/with-temp [:model/Card {card-id :id :as card} card-defaults]
+            (testing "\nCards based on uploads have based_on_upload=<table-id> if they meet all the criteria"
+              (is (= table-id (:based_on_upload (request card)))))
+            (testing "If one of the criteria for appends is not met, based_on_upload should be nil."
+              (testing "\nIf the card is based on another card, which is based on the table, based_on_upload should be nil"
+                (mt/with-temp [:model/Card card' (assoc card-defaults
+                                                        :dataset_query
+                                                        {:type     :query
+                                                         :database db-id
+                                                         :query    {:source-table (str "card__" card-id)}})]
+                  (is (nil? (:based_on_upload (request card'))))))
+              (testing "\nIf the card has a join in the query (even to itself), based_on_upload should be nil"
+                (mt/with-temp [:model/Card card' (assoc card-defaults
+                                                        :dataset_query
+                                                        {:type     :query
+                                                         :database db-id
+                                                         :query    {:source-table table-id
+                                                                    :joins [{:fields       :all
+                                                                             :source-table table-id
+                                                                             :condition    [:= 1 2] ; field-ids don't matter
+                                                                             :alias        "SomeAlias"}]}})]
+                  (is (nil? (:based_on_upload (request card'))))))
+              (testing "\nIf the table is not based on uploads, based_on_upload should be nil"
+                (t2/update! :model/Table table-id {:is_upload false})
+                (is (nil? (:based_on_upload (request card))))
+                (t2/update! :model/Table table-id {:is_upload true}))
+              (testing "\nIf uploads are disabled, based_on_upload should be nil"
+                (mt/with-temp-copy-of-db
+                  (perms/revoke-data-perms! (perms-group/all-users) db)
+                  (is (nil? (:based_on_upload (mt/user-http-request :rasta :get 200 (str "card/" card-id)))))))
+              (testing "\nIf uploads are disabled, based_on_upload should be nil"
+                (mt/with-temporary-setting-values [uploads-enabled false]
+                  (is (nil? (:based_on_upload (request card))))))
+              (testing "\nIf the card is not a model, based_on_upload should be nil"
+                (mt/with-temp [:model/Card card' (assoc card-defaults :dataset false)]
+                  (is (nil? (:based_on_upload (request card'))))))
+              (testing "\nIf the card is a native query, based_on_upload should be nil"
+                (mt/with-temp [:model/Card card' (assoc card-defaults
+                                                        :dataset_query (mt/native-query {:native "select 1"}))]
+                  (is (nil? (:based_on_upload (request card')))))))))))))
+
+(deftest based-on-upload-test
+  (run-based-on-upload-test
+   (fn [card]
+     (mt/user-http-request :crowberto :get 200 (str "card/" (:id card))))))

test/metabase/api/collection_test.clj

@@ -4,6 +4,7 @@
    [clojure.set :as set]
    [clojure.string :as str]
    [clojure.test :refer :all]
+   [metabase.api.card-test :as api.card-test]
    [metabase.api.collection :as api.collection]
    [metabase.models
     :refer [Card Collection Dashboard DashboardCard ModerationReview
@@ -639,6 +640,17 @@
                 (:data (mt/user-http-request :crowberto :get 200
                                              (str "collection/" (u/the-id collection) "/items"))))))))))
 
+(deftest collection-items-based-on-upload-test
+  (testing "GET /api/collection/:id/items"
+    (testing "check that based_on_upload is returned for cards correctly"
+      (api.card-test/run-based-on-upload-test
+       (fn [card]
+         (->> (mt/user-http-request :crowberto :get 200 (str "collection/" (:collection_id card) "/items?models=card&models=dataset"))
+              :data
+              (filter (fn [item]
+                        (= (:id item) (:id card))))
+              first))))))
+
 (deftest collection-items-return-database-id-for-datasets-test
   (testing "GET /api/collection/:id/items"
     (testing "Database id is returned for items in which dataset is true"