From 43f51a0553b61972e929d9a24a8e445c5c2eb078 Mon Sep 17 00:00:00 2001
From: Cam Saul <1455846+camsaul@users.noreply.github.com>
Date: Mon, 8 Feb 2021 10:16:18 -0800
Subject: [PATCH] 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 :wrench:
* 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 | 38 +--
.../models/group_table_access_policy.clj | 28 +-
.../sandbox/api/gtap_test.clj | 255 +++++++++---------
.../models/group_table_access_policy_test.clj | 24 +-
.../row_level_restrictions_test.clj | 2 +-
.../sandboxes/components/GTAPModal.jsx | 28 +-
.../sandboxes/sandboxes.cy.spec.js | 112 ++++++++
.../analyze/fingerprint/fingerprinters.clj | 3 +-
8 files changed, 312 insertions(+), 178 deletions(-)
diff --git a/.circleci/config.yml b/.circleci/config.yml
index 1ea2ed6e3a7..d4d1bc452eb 100644
--- a/.circleci/config.yml
+++ b/.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
diff --git a/enterprise/backend/src/metabase_enterprise/sandbox/models/group_table_access_policy.clj b/enterprise/backend/src/metabase_enterprise/sandbox/models/group_table_access_policy.clj
index 9533897ddab..b7d760653cc 100644
--- a/enterprise/backend/src/metabase_enterprise/sandbox/models/group_table_access_policy.clj
+++ b/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
diff --git a/enterprise/backend/test/metabase_enterprise/sandbox/api/gtap_test.clj b/enterprise/backend/test/metabase_enterprise/sandbox/api/gtap_test.clj
index 82cadf4a2a3..983d0096aa4 100644
--- a/enterprise/backend/test/metabase_enterprise/sandbox/api/gtap_test.clj
+++ b/enterprise/backend/test/metabase_enterprise/sandbox/api/gtap_test.clj
@@ -1,29 +1,25 @@
(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}}))))))))))
diff --git a/enterprise/backend/test/metabase_enterprise/sandbox/models/group_table_access_policy_test.clj b/enterprise/backend/test/metabase_enterprise/sandbox/models/group_table_access_policy_test.clj
index b1e84bc94d8..529b1aaf78e 100644
--- a/enterprise/backend/test/metabase_enterprise/sandbox/models/group_table_access_policy_test.clj
+++ b/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))}]
diff --git a/enterprise/backend/test/metabase_enterprise/sandbox/query_processor/middleware/row_level_restrictions_test.clj b/enterprise/backend/test/metabase_enterprise/sandbox/query_processor/middleware/row_level_restrictions_test.clj
index e2a5d2ace45..f0dcfbd213f 100644
--- a/enterprise/backend/test/metabase_enterprise/sandbox/query_processor/middleware/row_level_restrictions_test.clj
+++ b/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"
diff --git a/enterprise/frontend/src/metabase-enterprise/sandboxes/components/GTAPModal.jsx b/enterprise/frontend/src/metabase-enterprise/sandboxes/components/GTAPModal.jsx
index ff554d6ca8e..b61463fe27a 100644
--- a/enterprise/frontend/src/metabase-enterprise/sandboxes/components/GTAPModal.jsx
+++ b/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>
);
diff --git a/enterprise/frontend/test/metabase-enterprise/sandboxes/sandboxes.cy.spec.js b/enterprise/frontend/test/metabase-enterprise/sandboxes/sandboxes.cy.spec.js
index 39f3f9a28d0..af65fd0b76f 100644
--- a/enterprise/frontend/test/metabase-enterprise/sandboxes/sandboxes.cy.spec.js
+++ b/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);
+ });
});
});
diff --git a/src/metabase/sync/analyze/fingerprint/fingerprinters.clj b/src/metabase/sync/analyze/fingerprint/fingerprinters.clj
index 8cdc6119b7f..09b5c08d2ea 100644
--- a/src/metabase/sync/analyze/fingerprint/fingerprinters.clj
+++ b/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)
--
GitLab