Show error messages in create/edit GTAP modal (#14673)

* Rework tests in api.gtap-test

* Return actual error message when trying to save an invalid sandbox query

* Minor refactor

* GTAP modal should display errors

* Fix flow failure

* Test fix

* Another flow fix 



* Longer timeout for the test that keeps failing

* Return error message as :message

* New cache key for drivers/uberjar so Oracle/Vertica errors stop popping up

* Revert cache key rotation

* #14612 Repro: Sandboxing limitations should have meaningful UI error message/feedback (#14690)

* Fix random Cypress test failures when fingerprinting fails?

* Bump no-output-timeouts to 15m because CircleCI is extra slow lately

Co-authored-by: Nemanja Glumac <31325167+nemanjaglumac@users.noreply.github.com>

.circleci/config.yml

@@ -405,7 +405,7 @@ commands:
           name: lein << parameters.lein-command >>
           command: |
             lein with-profile +ci,+<< parameters.edition >> << parameters.lein-command >>
-          no_output_timeout: 10m
+          no_output_timeout: 15m
       - steps: << parameters.after-steps >>
       - store_test_results:
           path: /home/circleci/metabase/metabase/target/junit
@@ -438,7 +438,7 @@ commands:
                   - run:
                       name: << parameters.command-name >>
                       command: yarn << parameters.command >>
-                      no_output_timeout: 10m
+                      no_output_timeout: 15m
                   - steps: << parameters.after-steps >>
       - unless:
           condition: << parameters.skip-when-no-change >>
@@ -448,7 +448,7 @@ commands:
             - run:
                 name: << parameters.command-name >>
                 command: yarn << parameters.command >>
-                no_output_timeout: 10m
+                no_output_timeout: 15m
             - steps: << parameters.after-steps >>
 
   wait-for-port:
@@ -460,7 +460,7 @@ commands:
           name: Wait for port << parameters.port >> to be ready
           command: |
             while ! nc -z localhost << parameters.port >>; do sleep 0.1; done
-          no_output_timeout: 10m
+          no_output_timeout: 15m
 
   fetch-jdbc-driver:
     parameters:
@@ -476,7 +476,7 @@ commands:
           name: Download JDBC driver JAR << parameters.dest >>
           command: |
             wget --output-document=plugins/<< parameters.dest >> ${<< parameters.source >>}
-          no_output_timeout: 10m
+          no_output_timeout: 15m
 
 jobs:
 
@@ -584,7 +584,7 @@ jobs:
             - run:
                 name: Verify Liquibase Migrations
                 command: ./bin/lint-migrations-file.sh
-                no_output_timeout: 10m
+                no_output_timeout: 15m
 
 
 ########################################################################################################################
@@ -661,7 +661,7 @@ jobs:
             - run:
                 name: Run reflection warnings checker
                 command: ./bin/reflection-linter
-                no_output_timeout: 10m
+                no_output_timeout: 15m
 
   test-driver:
     parameters:
@@ -672,7 +672,7 @@ jobs:
         type: string
       timeout:
         type: string
-        default: 10m
+        default: 15m
       before-steps:
         type: steps
         default: []
@@ -709,27 +709,27 @@ jobs:
                 name: Run metabuild-common build script tests
                 command: |
                   cd /home/circleci/metabase/metabase/bin/common && clojure -M:test
-                no_output_timeout: 10m
+                no_output_timeout: 15m
             - run:
                 name: Run build-drivers build script tests
                 command: |
                   cd /home/circleci/metabase/metabase/bin/build-drivers && clojure -M:test
-                no_output_timeout: 10m
+                no_output_timeout: 15m
             - run:
                 name: Run build-mb build script tests
                 command: |
                   cd /home/circleci/metabase/metabase/bin/build-mb && clojure -M:test
-                no_output_timeout: 10m
+                no_output_timeout: 15m
             - run:
                 name: Run release script tests
                 command: |
                   cd /home/circleci/metabase/metabase/bin/release && clojure -M:test
-                no_output_timeout: 10m
+                no_output_timeout: 15m
             - run:
                 name: Run Liquibase migrations linter tests
                 command: |
                   cd /home/circleci/metabase/metabase/bin/lint-migrations-file && clojure -M:test
-                no_output_timeout: 10m
+                no_output_timeout: 15m
 
 
 ########################################################################################################################
@@ -748,7 +748,7 @@ jobs:
             - run:
                 name: Run yarn to install deps
                 command: yarn;
-                no_output_timeout: 10m
+                no_output_timeout: 15m
             - save_cache:
                 name: Cache frontend dependencies
                 <<: *CacheKeyFrontendDeps
@@ -839,7 +839,7 @@ jobs:
             - run:
                 name: Build << parameters.edition >> drivers if needed
                 command: ./bin/build-drivers.sh << parameters.edition >>
-                no_output_timeout: 10m
+                no_output_timeout: 15m
             - save_cache:
                 name: Cache local Maven installation of metabase-core
                 <<: *CacheKeyMetabaseCore
@@ -879,7 +879,7 @@ jobs:
                 environment:
                   MB_EDITION: << parameters.edition >>
                 command: ./bin/build version frontend
-                no_output_timeout: 10m
+                no_output_timeout: 15m
             - save_cache:
                 name: Cache the built frontend
                 <<: *CacheKeyFrontend
@@ -919,7 +919,7 @@ jobs:
                   INTERACTIVE: "false"
                   MB_EDITION: << parameters.edition >>
                 command: ./bin/build version drivers uberjar
-                no_output_timeout: 10m
+                no_output_timeout: 15m
             - store_artifacts:
                 path: /home/circleci/metabase/metabase/target/uberjar/metabase.jar
             - store_artifacts:
@@ -1207,14 +1207,14 @@ workflows:
           requires:
             - be-tests-ee
           driver: redshift
-          timeout: 10m
+          timeout: 15m
 
       - test-driver:
           name: be-tests-snowflake-ee
           requires:
             - be-tests-ee
           driver: snowflake
-          timeout: 110m
+          timeout: 115m
 
       - test-driver:
           name: be-tests-sparksql-ee

enterprise/backend/src/metabase_enterprise/sandbox/models/group_table_access_policy.clj

@@ -67,19 +67,23 @@
        ;; `row-level-restrictions` middleware). So include `:type` and `:status-code` information in the ExceptionInfo
        ;; data so it can be passed along if applicable.
        (when-not table-col-base-type
-         (throw (ex-info (tru "Sandbox Cards can''t return columns that aren't present in the Table they are sandboxing.")
-                         {:type        qp.error-type/bad-configuration
-                          :status-code 400
-                          :new-column  col
-                          :expected    (mapv :name table-cols)
-                          :actual      (mapv :name result-metadata-columns)})))
+         (let [msg (tru "Sandbox Cards can''t return columns that aren''t present in the Table they are sandboxing.")]
+           (throw (ex-info msg
+                           {:type        qp.error-type/bad-configuration
+                            :status-code 400
+                            :message     msg
+                            :new-column  col
+                            :expected    (mapv :name table-cols)
+                            :actual      (mapv :name result-metadata-columns)}))))
        (when-not (isa? (keyword (:base_type col)) table-col-base-type)
-         (throw (ex-info (tru "Sandbox Cards can''t return columns that have different types than the Table they are sandboxing.")
-                         {:type        qp.error-type/bad-configuration
-                          :status-code 400
-                          :new-col     col
-                          :expected    table-col-base-type
-                          :actual      (:base_type col)})))))))
+         (let [msg (tru "Sandbox Cards can''t return columns that have different types than the Table they are sandboxing.")]
+           (throw (ex-info msg
+                           {:type        qp.error-type/bad-configuration
+                            :status-code 400
+                            :message     msg
+                            :new-col     col
+                            :expected    table-col-base-type
+                            :actual      (:base_type col)}))))))))
 
 ;; TODO -- should we only check these constraints if EE features are enabled??
 (defn update-card-check-gtaps

enterprise/backend/test/metabase_enterprise/sandbox/api/gtap_test.clj

 (ns metabase-enterprise.sandbox.api.gtap-test
-  (:require [expectations :refer :all]
+  (:require [clojure.test :refer :all]
             [metabase-enterprise.sandbox.models.group-table-access-policy :refer [GroupTableAccessPolicy]]
             [metabase.http-client :as http]
-            [metabase.models.card :refer [Card]]
-            [metabase.models.permissions-group :refer [PermissionsGroup]]
-            [metabase.models.table :refer [Table]]
+            [metabase.models :refer [Card Field PermissionsGroup Table]]
             [metabase.public-settings.metastore :as metastore]
             [metabase.server.middleware.util :as middleware.u]
-            [metabase.test.data.users :refer :all]
-            [metabase.test.util :as tu]
-            [toucan.util.test :as tt]))
+            [metabase.test :as mt]
+            [schema.core :as s]))
 
 (defmacro ^:private with-sandboxes-enabled [& body]
   `(with-redefs [metastore/enable-sandboxes? (constantly true)]
      ~@body))
 
-;; Must be authenticated to query for gtaps
-(expect (get middleware.u/response-unauthentic :body)
-        (with-sandboxes-enabled
-          (http/client :get 401 "mt/gtap")))
+(deftest require-auth-test
+  (testing "Must be authenticated to query for GTAPs"
+    (with-sandboxes-enabled
+      (is (= (get middleware.u/response-unauthentic :body)
+             (http/client :get 401 "mt/gtap")))
 
-(expect
-  "You don't have permissions to do that."
-  (with-sandboxes-enabled
-    ((user->client :rasta) :get 403 (str "mt/gtap"))))
+      (is (= "You don't have permissions to do that."
+             (mt/user-http-request :rasta :get 403 (str "mt/gtap")))))))
 
 (def ^:private default-gtap-results
   {:id                   true
@@ -37,130 +33,131 @@
   case referential integrity failures for any related `Card` that would be cleaned up as part of a `with-temp*` call"
   [& body]
   `(with-sandboxes-enabled
-     (tu/with-model-cleanup [GroupTableAccessPolicy]
+     (mt/with-model-cleanup [GroupTableAccessPolicy]
        ~@body)))
 
 (defn- gtap-post
   "`gtap-data` is a map to be POSTed to the GTAP endpoint"
   [gtap-data]
-  ((user->client :crowberto) :post 200 "mt/gtap" gtap-data))
+  (mt/user-http-request :crowberto :post 200 "mt/gtap" gtap-data))
 
-;; ## POST /api/mt/gtap
-;; Must have a valid token to use GTAPs
-(expect
-  #"sandboxing is not enabled"
-  (tt/with-temp* [Table            [{table-id :id}]
-                  PermissionsGroup [{group-id :id}]
-                  Card             [{card-id :id}]]
-    ((user->client :crowberto) :post 403 "mt/gtap"
-     {:table_id             table-id
-      :group_id             group-id
-      :card_id              card-id
-      :attribute_remappings {"foo" 1}})))
+(deftest validate-token-test
+  (testing "POST /api/mt/gtap"
+    (testing "Must have a valid token to use GTAPs"
+      (with-redefs [metastore/enable-sandboxes? (constantly false)]
+        (mt/with-temp* [Table            [{table-id :id}]
+                        PermissionsGroup [{group-id :id}]
+                        Card             [{card-id :id}]]
+          (is (re= #".*sandboxing is not enabled.*"
+                   (mt/user-http-request :crowberto :post 403 "mt/gtap"
+                                         {:table_id             table-id
+                                          :group_id             group-id
+                                          :card_id              card-id
+                                          :attribute_remappings {"foo" 1}}))))))))
+(deftest create-gtap-test
+  (testing "POST /api/mt/gtap"
+    (mt/with-temp* [Table            [{table-id :id}]
+                    PermissionsGroup [{group-id :id}]]
+      (testing "Test that we can create a new GTAP"
+        (mt/with-temp Card [{card-id :id}]
+          (with-gtap-cleanup
+            (let [post-results (gtap-post {:table_id             table-id
+                                           :group_id             group-id
+                                           :card_id              card-id
+                                           :attribute_remappings {"foo" 1}})]
+              (is (= default-gtap-results
+                     (mt/boolean-ids-and-timestamps post-results)))
+              (is (= post-results
+                     (mt/user-http-request :crowberto :get 200 (format "mt/gtap/%s" (:id post-results)))))))))
 
-;; Test that we can create a new GTAP
-(expect
-  [default-gtap-results true]
-  (tt/with-temp* [Table            [{table-id :id}]
-                  PermissionsGroup [{group-id :id}]
-                  Card             [{card-id :id}]]
-    (with-gtap-cleanup
-      (let [post-results (gtap-post {:table_id             table-id
-                                     :group_id             group-id
-                                     :card_id              card-id
-                                     :attribute_remappings {"foo" 1}})]
-        [(tu/boolean-ids-and-timestamps post-results)
-         (= post-results ((user->client :crowberto) :get 200 (format "mt/gtap/%s" (:id post-results))))]))))
+      (testing "Test that we can create a new GTAP without a card"
+        (with-gtap-cleanup
+          (let [post-results (gtap-post {:table_id             table-id
+                                         :group_id             group-id
+                                         :card_id              nil
+                                         :attribute_remappings {"foo" 1}})]
+            (is (= (assoc default-gtap-results :card_id false)
+                   (mt/boolean-ids-and-timestamps post-results)))
+            (is (= post-results
+                   (mt/user-http-request :crowberto :get 200 (format "mt/gtap/%s" (:id post-results))))))))
 
-;; Test that we can create a new GTAP without a card
-(expect
-  [(assoc default-gtap-results :card_id false)
-   true]
-  (tt/with-temp* [Table            [{table-id :id}]
-                  PermissionsGroup [{group-id :id}]]
-    (with-gtap-cleanup
-      (let [post-results (gtap-post {:table_id             table-id
-                                     :group_id             group-id
-                                     :card_id              nil
-                                     :attribute_remappings {"foo" 1}})]
-        [(tu/boolean-ids-and-timestamps post-results)
-         (= post-results ((user->client :crowberto) :get 200 (format "mt/gtap/%s" (:id post-results))))]))))
+      (testing "Meaningful errors should be returned if you create an invalid GTAP"
+        (mt/with-temp* [Field [_ {:name "My field", :table_id table-id, :base_type :type/Integer}]
+                        Card  [{card-id :id} {:dataset_query {:database (mt/id)
+                                                              :type     :query
+                                                              :query    {:source-table (mt/id :venues)}}}]]
+          (with-gtap-cleanup
+            (is (schema= {:message  (s/eq "Sandbox Cards can't return columns that aren't present in the Table they are sandboxing.")
+                          :expected (s/eq [nil])
+                          :actual   (s/eq ["ID" "NAME" "CATEGORY_ID" "LATITUDE" "LONGITUDE" "PRICE"])
+                          s/Keyword s/Any}
+                         (mt/user-http-request :crowberto :post 400 "mt/gtap"
+                                               {:table_id             table-id
+                                                :group_id             group-id
+                                                :card_id              card-id
+                                                :attribute_remappings {"foo" 1}})))))))))
 
-;; Test that we can delete a GTAP
-(expect
-  [default-gtap-results
-   "Not found."]
-  (tt/with-temp* [Table            [{table-id :id}]
-                  PermissionsGroup [{group-id :id}]
-                  Card             [{card-id :id}]]
-    (with-gtap-cleanup
-      (let [{:keys [id]} (gtap-post {:table_id             table-id
-                                     :group_id             group-id
-                                     :card_id              card-id
-                                     :attribute_remappings {"foo" 1}})]
-        [(tu/boolean-ids-and-timestamps ((user->client :crowberto) :get 200 (format "mt/gtap/%s" id)))
-         (do
-           ((user->client :crowberto) :delete 204 (format "mt/gtap/%s" id))
-           ((user->client :crowberto) :get 404 (format "mt/gtap/%s" id)))]))))
+(deftest delete-gtap-test
+  (testing "DELETE /api/mt/gtap/:id"
+    (testing "Test that we can delete a GTAP"
+      (mt/with-temp* [Table            [{table-id :id}]
+                      PermissionsGroup [{group-id :id}]
+                      Card             [{card-id :id}]]
+        (with-gtap-cleanup
+          (let [{:keys [id]} (gtap-post {:table_id             table-id
+                                         :group_id             group-id
+                                         :card_id              card-id
+                                         :attribute_remappings {"foo" 1}})]
+            (is (= default-gtap-results
+                   (mt/boolean-ids-and-timestamps (mt/user-http-request :crowberto :get 200 (format "mt/gtap/%s" id)))))
+            (is (= nil
+                   (mt/user-http-request :crowberto :delete 204 (format "mt/gtap/%s" id))))
+            (is (= "Not found."
+                   (mt/user-http-request :crowberto :get 404 (format "mt/gtap/%s" id))))))))))
 
-;; ## PUT /api/mt/gtap
-;; Test that we can update only the attribute remappings for a GTAP
-(expect
-  (assoc default-gtap-results :attribute_remappings {:bar 2})
-  (tt/with-temp* [Table                  [{table-id :id}]
-                  PermissionsGroup       [{group-id :id}]
-                  Card                   [{card-id :id}]
-                  GroupTableAccessPolicy [{gtap-id :id} {:table_id             table-id
-                                                         :group_id             group-id
-                                                         :card_id              card-id
-                                                         :attribute_remappings {"foo" 1}}]]
-    (with-sandboxes-enabled
-      (tu/boolean-ids-and-timestamps
-       ((user->client :crowberto) :put 200 (format "mt/gtap/%s" gtap-id)
-        {:attribute_remappings {:bar 2}})))))
+(deftest update-gtap-test
+  (testing "PUT /api/mt/gtap"
+    (mt/with-temp* [Table                  [{table-id :id}]
+                    PermissionsGroup       [{group-id :id}]
+                    Card                   [{card-id :id}]]
+      (with-sandboxes-enabled
+        (testing "Test that we can update only the attribute remappings for a GTAP"
+          (mt/with-temp GroupTableAccessPolicy [{gtap-id :id} {:table_id             table-id
+                                                               :group_id             group-id
+                                                               :card_id              card-id
+                                                               :attribute_remappings {"foo" 1}}]
+            (is (= (assoc default-gtap-results :attribute_remappings {:bar 2})
+                   (mt/boolean-ids-and-timestamps
+                    (mt/user-http-request :crowberto :put 200 (format "mt/gtap/%s" gtap-id)
+                                          {:attribute_remappings {:bar 2}}))))))
 
-;; Test that we can add a card_id via PUT
-(expect
-  default-gtap-results
-  (tt/with-temp* [Table                  [{table-id :id}]
-                  PermissionsGroup       [{group-id :id}]
-                  Card                   [{card-id :id}]
-                  GroupTableAccessPolicy [{gtap-id :id} {:table_id             table-id
-                                                         :group_id             group-id
-                                                         :card_id              nil
-                                                         :attribute_remappings {"foo" 1}}]]
-    (with-sandboxes-enabled
-      (tu/boolean-ids-and-timestamps
-       ((user->client :crowberto) :put 200 (format "mt/gtap/%s" gtap-id)
-        {:card_id card-id})))))
+        (testing "Test that we can add a card_id via PUT"
+          (mt/with-temp GroupTableAccessPolicy [{gtap-id :id} {:table_id             table-id
+                                                               :group_id             group-id
+                                                               :card_id              nil
+                                                               :attribute_remappings {"foo" 1}}]
+            (is (= default-gtap-results
+                   (mt/boolean-ids-and-timestamps
+                    (mt/user-http-request :crowberto :put 200 (format "mt/gtap/%s" gtap-id)
+                                          {:card_id card-id}))))))
 
-;; Test that we can remove a card_id via PUT
-(expect
-  (assoc default-gtap-results :card_id false)
-  (tt/with-temp* [Table                  [{table-id :id}]
-                  PermissionsGroup       [{group-id :id}]
-                  Card                   [{card-id :id}]
-                  GroupTableAccessPolicy [{gtap-id :id} {:table_id             table-id
-                                                         :group_id             group-id
-                                                         :card_id              card-id
-                                                         :attribute_remappings {"foo" 1}}]]
-    (with-sandboxes-enabled
-      (tu/boolean-ids-and-timestamps
-       ((user->client :crowberto) :put 200 (format "mt/gtap/%s" gtap-id)
-        {:card_id nil})))))
+        (testing "Test that we can remove a card_id via PUT"
+          (mt/with-temp GroupTableAccessPolicy [{gtap-id :id} {:table_id             table-id
+                                                               :group_id             group-id
+                                                               :card_id              card-id
+                                                               :attribute_remappings {"foo" 1}}]
+            (is (= (assoc default-gtap-results :card_id false)
+                   (mt/boolean-ids-and-timestamps
+                    (mt/user-http-request :crowberto :put 200 (format "mt/gtap/%s" gtap-id)
+                                          {:card_id nil}))))))
 
-;; Test that we can remove a card_id and change attribute remappings via PUT
-(expect
-  (assoc default-gtap-results :card_id false, :attribute_remappings {:bar 2})
-  (tt/with-temp* [Table                  [{table-id :id}]
-                  PermissionsGroup       [{group-id :id}]
-                  Card                   [{card-id :id}]
-                  GroupTableAccessPolicy [{gtap-id :id} {:table_id             table-id
-                                                         :group_id             group-id
-                                                         :card_id              card-id
-                                                         :attribute_remappings {"foo" 1}}]]
-    (with-sandboxes-enabled
-      (tu/boolean-ids-and-timestamps
-       ((user->client :crowberto) :put 200 (format "mt/gtap/%s" gtap-id)
-        {:card_id              nil
-         :attribute_remappings {:bar 2}})))))
+        (testing "Test that we can remove a card_id and change attribute remappings via PUT"
+          (mt/with-temp GroupTableAccessPolicy [{gtap-id :id} {:table_id             table-id
+                                                               :group_id             group-id
+                                                               :card_id              card-id
+                                                               :attribute_remappings {"foo" 1}}]
+            (is (= (assoc default-gtap-results :card_id false, :attribute_remappings {:bar 2})
+                   (mt/boolean-ids-and-timestamps
+                    (mt/user-http-request :crowberto :put 200 (format "mt/gtap/%s" gtap-id)
+                                          {:card_id              nil
+                                           :attribute_remappings {:bar 2}}))))))))))

enterprise/backend/test/metabase_enterprise/sandbox/models/group_table_access_policy_test.clj

@@ -11,7 +11,7 @@
 (deftest normalize-attribute-remappings-test
   (testing "make sure attribute-remappings come back from the DB normalized the way we'd expect"
     (mt/with-temp GroupTableAccessPolicy [gtap {:table_id             (mt/id :venues)
-                                                :group_id             (u/get-id (group/all-users))
+                                                :group_id             (u/the-id (group/all-users))
                                                 :attribute_remappings {"venue_id"
                                                                        {:type   "category"
                                                                         :target ["variable" ["field-id" (mt/id :venues :id)]]
@@ -19,20 +19,20 @@
       (is (= {"venue_id" {:type   :category
                           :target [:variable [:field-id (mt/id :venues :id)]]
                           :value  5}}
-             (db/select-one-field :attribute_remappings GroupTableAccessPolicy :id (u/get-id gtap)))))
+             (db/select-one-field :attribute_remappings GroupTableAccessPolicy :id (u/the-id gtap)))))
 
     (testing (str "apparently sometimes they are saved with just the target, but not type or value? Make sure these "
                   "get normalized correctly.")
       (mt/with-temp GroupTableAccessPolicy [gtap {:table_id             (mt/id :venues)
-                                                  :group_id             (u/get-id (group/all-users))
+                                                  :group_id             (u/the-id (group/all-users))
                                                   :attribute_remappings {"user" ["variable" ["field-id" (mt/id :venues :id)]]}}]
         (is (= {"user" [:variable [:field-id (mt/id :venues :id)]]}
-               (db/select-one-field :attribute_remappings GroupTableAccessPolicy :id (u/get-id gtap))))))))
+               (db/select-one-field :attribute_remappings GroupTableAccessPolicy :id (u/the-id gtap))))))))
 
 (deftest disallow-changing-table-id-test
   (testing "You can't change the table_id of a GTAP after it has been created."
     (mt/with-temp GroupTableAccessPolicy [gtap {:table_id (mt/id :venues)
-                                                :group_id (u/get-id (group/all-users))}]
+                                                :group_id (u/the-id (group/all-users))}]
       (is (thrown-with-msg?
            clojure.lang.ExceptionInfo
            #"You cannot change the Table ID of a GTAP once it has been created"
@@ -45,7 +45,7 @@
                        (mt/with-temp* [Card                   [card {:dataset_query   query
                                                                      :result_metadata (qp/query->expected-cols query)}]
                                        GroupTableAccessPolicy [gtap {:table_id (mt/id :venues)
-                                                                     :group_id (u/get-id (group/all-users))
+                                                                     :group_id (u/the-id (group/all-users))
                                                                      :card_id  (:id card)}]]
                          :ok))
 
@@ -54,7 +54,7 @@
                        (mt/with-temp* [Card                   [card {:dataset_query   query
                                                                      :result_metadata (qp/query->expected-cols query)}]
                                        GroupTableAccessPolicy [gtap {:table_id (mt/id :venues)
-                                                                     :group_id (u/get-id (group/all-users))}]]
+                                                                     :group_id (u/the-id (group/all-users))}]]
                          (db/update! GroupTableAccessPolicy (:id gtap) :card_id (:id card))
                          :ok))
 
@@ -63,7 +63,7 @@
                        (mt/with-temp* [Card                   [card {:dataset_query   (mt/mbql-query :venues)
                                                                      :result_metadata (qp/query->expected-cols (mt/mbql-query :venues))}]
                                        GroupTableAccessPolicy [gtap {:table_id (mt/id :venues)
-                                                                     :group_id (u/get-id (group/all-users))
+                                                                     :group_id (u/the-id (group/all-users))
                                                                      :card_id  (:id card)}]]
                          (db/update! Card (:id card) :dataset_query query)
                          :ok))}]
@@ -74,7 +74,7 @@
         (testing "adding new columns = not ok"
           (is (thrown-with-msg?
                clojure.lang.ExceptionInfo
-               #"Sandbox Cards can't return columns that arent present in the Table they are sandboxing"
+               #"Sandbox Cards can't return columns that aren't present in the Table they are sandboxing"
                (f (mt/mbql-query checkins)))))
         (testing "removing columns = ok"
           (is (= :ok
@@ -92,7 +92,7 @@
                        (mt/with-temp* [Card                   [card {:dataset_query   (mt/mbql-query :venues)
                                                                      :result_metadata metadata}]
                                        GroupTableAccessPolicy [gtap {:table_id (mt/id :venues)
-                                                                     :group_id (u/get-id (group/all-users))
+                                                                     :group_id (u/the-id (group/all-users))
                                                                      :card_id  (:id card)}]]
                          :ok))
 
@@ -101,7 +101,7 @@
                        (mt/with-temp* [Card                   [card {:dataset_query   (mt/mbql-query :venues)
                                                                      :result_metadata metadata}]
                                        GroupTableAccessPolicy [gtap {:table_id (mt/id :venues)
-                                                                     :group_id (u/get-id (group/all-users))}]]
+                                                                     :group_id (u/the-id (group/all-users))}]]
                          (db/update! GroupTableAccessPolicy (:id gtap) :card_id (:id card))
                          :ok))
 
@@ -110,7 +110,7 @@
                        (mt/with-temp* [Card                   [card {:dataset_query   (mt/mbql-query :venues)
                                                                      :result_metadata (qp/query->expected-cols (mt/mbql-query :venues))}]
                                        GroupTableAccessPolicy [gtap {:table_id (mt/id :venues)
-                                                                     :group_id (u/get-id (group/all-users))
+                                                                     :group_id (u/the-id (group/all-users))
                                                                      :card_id  (:id card)}]]
                          (db/update! Card (:id card) :result_metadata metadata)
                          :ok))}]

enterprise/backend/test/metabase_enterprise/sandbox/query_processor/middleware/row_level_restrictions_test.clj

@@ -637,7 +637,7 @@
            (testing "Should throw an Exception when running the query"
              (is (thrown-with-msg?
                   clojure.lang.ExceptionInfo
-                  #"Sandbox Cards can't return columns that arent present in the Table they are sandboxing"
+                  #"Sandbox Cards can't return columns that aren't present in the Table they are sandboxing"
                   (run-query)))))))
 
       (testing "Don't allow people to change the types of columns in the original Table"

enterprise/frontend/src/metabase-enterprise/sandboxes/components/GTAPModal.jsx

@@ -19,6 +19,8 @@ import Icon from "metabase/components/Icon";
 import Tooltip from "metabase/components/Tooltip";
 import { GTAPApi } from "metabase/services";
 
+import { UNKNOWN_ERROR_MESSAGE } from "metabase/components/form/FormMessage";
+
 import EntityObjectLoader from "metabase/entities/containers/EntityObjectLoader";
 import QuestionLoader from "metabase/containers/QuestionLoader";
 
@@ -47,6 +49,7 @@ type State = {
   gtap: ?GTAP,
   attributesOptions: ?(string[]),
   simple: boolean,
+  error: ?string,
 };
 
 @withRouter
@@ -60,6 +63,7 @@ export default class GTAPModal extends React.Component {
     gtap: null,
     attributesOptions: null,
     simple: true,
+    error: null,
   };
   // $FlowFixMe: componentWillMount expected to return void
   async componentWillMount() {
@@ -119,10 +123,21 @@ export default class GTAPModal extends React.Component {
     if (!gtap) {
       throw new Error("No GTAP");
     }
-    if (gtap.id != null) {
-      await GTAPApi.update(gtap);
-    } else {
-      await GTAPApi.create(gtap);
+    try {
+      if (gtap.id != null) {
+        await GTAPApi.update(gtap);
+      } else {
+        await GTAPApi.create(gtap);
+      }
+    } catch (error) {
+      console.error("Error saving GTAP", error);
+      const message = error
+        ? error.data
+          ? error.data.message || JSON.stringify(error.data)
+          : JSON.stringify(error)
+        : UNKNOWN_ERROR_MESSAGE;
+      this.setState({ error: message });
+      throw new Error(message);
     }
     this.close();
   };
@@ -253,6 +268,11 @@ export default class GTAPModal extends React.Component {
               {t`Save`}
             </ActionButton>
           </div>
+          {this.state.error && (
+            <div className="flex align-center my2 text-error">
+              {this.state.error}
+            </div>
+          )}
         </div>
       </div>
     );

enterprise/frontend/test/metabase-enterprise/sandboxes/sandboxes.cy.spec.js

@@ -18,6 +18,7 @@ const {
   ORDERS_ID,
   PRODUCTS,
   PRODUCTS_ID,
+  REVIEWS,
   REVIEWS_ID,
   PEOPLE,
   PEOPLE_ID,
@@ -878,6 +879,117 @@ describeWithToken("formatting > sandboxes", () => {
       });
       cy.contains("37.65");
     });
+
+    it("attempt to sandbox based on question with more columns than a sandboxed table should provide meaningful UI error (metabase#14612)", () => {
+      const [ORDERS_ALIAS, PRODUCTS_ALIAS, REVIEWS_ALIAS] = [
+        "Orders",
+        "Products",
+        "Reviews",
+      ];
+      const QUESTION_NAME = "Extra columns question";
+      const ERROR_MESSAGE =
+        "Sandbox Cards can't return columns that aren't present in the Table they are sandboxing.";
+
+      cy.server();
+      cy.route("POST", "/api/mt/gtap").as("sandboxTable");
+
+      cy.log(
+        "**-- 1. Create question that will have more columns (different columns) than the sandboxed table --**",
+      );
+
+      cy.request("POST", "/api/card", {
+        name: QUESTION_NAME,
+        dataset_query: {
+          database: 1,
+          query: {
+            joins: [
+              // a. People join Orders
+              {
+                alias: ORDERS_ALIAS,
+                condition: [
+                  "=",
+                  ["field-id", PEOPLE.ID],
+                  ["joined-field", ORDERS_ALIAS, ["field-id", ORDERS.USER_ID]],
+                ],
+                fields: "all",
+                "source-table": ORDERS_ID,
+                strategy: "inner-join",
+              },
+              // b. Previous results join Products
+              {
+                alias: PRODUCTS_ALIAS,
+                condition: [
+                  "=",
+                  [
+                    "joined-field",
+                    ORDERS_ALIAS,
+                    ["field-id", ORDERS.PRODUCT_ID],
+                  ],
+                  ["joined-field", PRODUCTS_ALIAS, ["field-id", PRODUCTS.ID]],
+                ],
+                fields: "all",
+                "source-table": PRODUCTS_ID,
+                strategy: "inner-join",
+              },
+              // c. Previous results join Reviews
+              {
+                alias: REVIEWS_ALIAS,
+                condition: [
+                  "=",
+                  ["joined-field", PRODUCTS_ALIAS, ["field-id", PRODUCTS.ID]],
+                  [
+                    "joined-field",
+                    REVIEWS_ALIAS,
+                    ["field-id", REVIEWS.PRODUCT_ID],
+                  ],
+                ],
+                fields: "all",
+                "source-table": REVIEWS_ID,
+                strategy: "inner-join",
+              },
+            ],
+            "source-table": PEOPLE_ID,
+          },
+          type: "query",
+        },
+        display: "table",
+        visualization_settings: {},
+      });
+
+      cy.visit("/admin/permissions/databases/1/schemas/PUBLIC/tables");
+      // |                | All users | collection |
+      // |--------------- |:---------:|:----------:|
+      // | Orders         |   X (0)   |    X (1)   |
+      cy.get(".Icon-close")
+        .eq(1) // No better way of doing this, undfortunately (see table above)
+        .click();
+      cy.findByText("Grant sandboxed access").click();
+      cy.findAllByRole("button", { name: "Change" }).click();
+      cy.findByText(
+        "Use a saved question to create a custom view for this table",
+      ).click();
+      cy.findByText(QUESTION_NAME).click();
+
+      cy.findByText("Pick a parameter").click();
+      popover().within(() => {
+        // PERSON.ID
+        cy.findAllByText("ID")
+          .first()
+          .click();
+      });
+
+      cy.findByText("Pick a user attribute").click();
+      cy.findByText(ATTR_UID).click();
+      cy.findAllByRole("button", { name: "Save" }).click();
+
+      cy.wait("@sandboxTable").then(xhr => {
+        cy.log(xhr);
+        expect(xhr.status).to.eq(400);
+        expect(xhr.response.body.message).to.eq(ERROR_MESSAGE);
+      });
+      cy.get(".Modal").scrollTo("bottom");
+      cy.findByText(ERROR_MESSAGE);
+    });
   });
 });
 

src/metabase/sync/analyze/fingerprint/fingerprinters.clj

@@ -188,7 +188,8 @@
   Integer  (->temporal [this] (->temporal (t/instant this)))
   ChronoLocalDateTime (->temporal [this] (.toInstant this (ZoneOffset/UTC)))
   ChronoZonedDateTime (->temporal [this] (.toInstant this))
-  Temporal (->temporal [this] this))
+  Temporal (->temporal [this] this)
+  java.util.Date (->temporal [this] (t/instant this)))
 
 (deffingerprinter :type/DateTime
   ((map ->temporal)