From 16e31051b0987f3e54ea92b2ae76146802265b54 Mon Sep 17 00:00:00 2001
From: appleby <86076+appleby@users.noreply.github.com>
Date: Fri, 13 Sep 2024 12:58:04 -0600
Subject: [PATCH] Revert to non-truncating mbql.u/unique-name-generator in
annotate-native-cols (#47866)
* Rollback to legacy unique-name-generator in annotate-native-cols
* Add native query long column alias test to nested queries tests
* Add native query long column alias test to dataset api tests
* Add native query long column alias test to models e2e tests
* Move limit into inner native query
* PR suggestion: simplify lambda argslist
Co-authored-by: metamben <103100869+metamben@users.noreply.github.com>
* PR suggestion: remove unnecessary mt/with-temp-copy-of-db
* Tweak test name
---------
Co-authored-by: metamben <103100869+metamben@users.noreply.github.com>
---
e2e/test/scenarios/models/models.cy.spec.js | 89 ++++++++++++++++++-
.../query_processor/middleware/annotate.clj | 3 +-
test/metabase/api/dataset_test.clj | 70 +++++++++++++++
.../nested_queries_test.clj | 55 ++++++++++++
4 files changed, 211 insertions(+), 6 deletions(-)
diff --git a/e2e/test/scenarios/models/models.cy.spec.js b/e2e/test/scenarios/models/models.cy.spec.js
index 8d88f8dec6c..48ee9eb6019 100644
--- a/e2e/test/scenarios/models/models.cy.spec.js
+++ b/e2e/test/scenarios/models/models.cy.spec.js
@@ -162,6 +162,91 @@ describe("scenarios > models", () => {
cy.location("pathname").should("eq", "/collection/root");
});
+ it("allows to turn a native question with a long alias into a model (metabase#47584)", () => {
+ const nativeQuery = `
+ SELECT
+ count(*) AS coun,
+ state AS Total_number_of_people_from_each_state_separated_by_state_and_then_we_do_a_count
+ FROM people
+ GROUP BY
+ Total_number_of_people_from_each_state_separated_by_state_and_then_we_do_a_count`;
+ cy.createNativeQuestion(
+ {
+ name: "People Model with long alias",
+ native: {
+ query: nativeQuery,
+ },
+ },
+ { visitQuestion: true, wrapId: true },
+ );
+
+ turnIntoModel();
+ openQuestionActions();
+ assertIsModel();
+
+ cy.get("@questionId").then(questionId => {
+ cy.wait("@dataset").then(({ response }) => {
+ expect(response.body.json_query.query["source-table"]).to.equal(
+ `card__${questionId}`,
+ );
+ expect(response.body.error).to.not.exist;
+ });
+ });
+
+ // Filtering on the long column is currently broken in master (metabase#47863),
+ // but this works in the release-x.50.x branch.
+ //
+ // filter();
+ // filterField(
+ // "TOTAL_NUMBER_OF_PEOPLE_FROM_EACH_STATE_SEPARATED_BY_STATE_AND_THEN_WE_DO_A_COUNT",
+ // {
+ // operator: "Contains",
+ // value: "A",
+ // },
+ // );
+
+ // cy.findByTestId("apply-filters").click();
+ // cy.wait("@dataset").then(({ response }) => {
+ // expect(response.body.error).to.not.exist;
+ // });
+
+ filter();
+ filterField("COUN", {
+ operator: "Greater than",
+ value: 30,
+ });
+
+ cy.findByTestId("apply-filters").click();
+ cy.wait("@dataset").then(({ response }) => {
+ expect(response.body.error).to.not.exist;
+ });
+
+ assertQuestionIsBasedOnModel({
+ model: "People Model with long alias",
+ collection: "Our analytics",
+ table: "People",
+ });
+
+ cy.get("@questionId").then(questionId => {
+ saveQuestionBasedOnModel({ modelId: questionId, name: "Q1" });
+ });
+
+ assertQuestionIsBasedOnModel({
+ questionName: "Q1",
+ model: "People Model with long alias",
+ collection: "Our analytics",
+ table: "People",
+ });
+
+ cy.findByTestId("qb-header").findAllByText("Our analytics").first().click();
+ getCollectionItemCard("People Model with long alias").within(() => {
+ cy.icon("model");
+ });
+ getCollectionItemRow("Q1").icon("table2");
+
+ cy.location("pathname").should("eq", "/collection/root");
+ });
+
it("changes model's display to table", () => {
visitQuestion(ORDERS_BY_YEAR_QUESTION_ID);
@@ -520,11 +605,7 @@ describe("scenarios > models", () => {
it("should automatically pin newly created models", () => {
visitQuestion(ORDERS_QUESTION_ID);
-
- cy.intercept("PUT", "/api/card/*").as("cardUpdate");
turnIntoModel();
- cy.wait("@cardUpdate");
-
visitCollection("root");
cy.findByTestId("pinned-items").within(() => {
cy.findByText("Models");
diff --git a/src/metabase/query_processor/middleware/annotate.clj b/src/metabase/query_processor/middleware/annotate.clj
index 347c610ceb2..787971ae1ac 100644
--- a/src/metabase/query_processor/middleware/annotate.clj
+++ b/src/metabase/query_processor/middleware/annotate.clj
@@ -14,7 +14,6 @@
[metabase.lib.metadata :as lib.metadata]
[metabase.lib.metadata.calculation :as lib.metadata.calculation]
[metabase.lib.schema.common :as lib.schema.common]
- [metabase.lib.util :as lib.util]
[metabase.lib.util.match :as lib.util.match]
[metabase.models.humanization :as humanization]
[metabase.query-processor.debug :as qp.debug]
@@ -80,7 +79,7 @@
:type qp.error-type/qp}))))))
(defn- annotate-native-cols [cols]
- (let [unique-name-fn (lib.util/unique-name-generator (qp.store/metadata-provider))]
+ (let [unique-name-fn (mbql.u/unique-name-generator)]
(mapv (fn [{col-name :name, base-type :base_type, :as driver-col-metadata}]
(let [col-name (name col-name)]
(merge
diff --git a/test/metabase/api/dataset_test.clj b/test/metabase/api/dataset_test.clj
index 2ca996148a5..78ef32bbc6b 100644
--- a/test/metabase/api/dataset_test.clj
+++ b/test/metabase/api/dataset_test.clj
@@ -16,11 +16,14 @@
[metabase.lib.core :as lib]
[metabase.lib.metadata :as lib.metadata]
[metabase.lib.metadata.jvm :as lib.metadata.jvm]
+ [metabase.lib.metadata.protocols :as lib.metadata.protocols]
[metabase.lib.schema.id :as lib.schema.id]
+ [metabase.lib.util :as lib.util]
[metabase.models.card :refer [Card]]
[metabase.models.data-permissions :as data-perms]
[metabase.models.permissions-group :as perms-group]
[metabase.models.query-execution :refer [QueryExecution]]
+ [metabase.query-processor.compile :as qp.compile]
[metabase.query-processor.middleware.constraints :as qp.constraints]
[metabase.query-processor.test-util :as qp.test-util]
[metabase.query-processor.util :as qp.util]
@@ -227,6 +230,73 @@
(data-perms/set-database-permission! (perms-group/all-users) (mt/id) :perms/create-queries :no)
(do-test)))))))
+(deftest native-query-with-long-column-alias
+ (testing "nested native query with long column alias (metabase#47584)"
+ (let [short-col-name "coun"
+ long-col-name "Total_number_of_people_from_each_state_separated_by_state_and_then_we_do_a_count"
+
+ ;; Lightly validate the native form that comes back. Resist the urge to check for exact equality.
+ validate-native-form (fn [native-form-lines]
+ (and (some #(str/includes? % short-col-name) native-form-lines)
+ (some #(str/includes? % long-col-name) native-form-lines)))
+
+ ;; Disable truncate-alias when compiling the native query to ensure we don't truncate the column.
+ ;; We want to simulate a user-defined query where the column name is long, but valid for the driver.
+ native-sub-query (with-redefs [lib.util/truncate-alias
+ (fn mock-truncate-alias
+ [ss & _] ss)]
+ (-> (mt/mbql-query people
+ {:source-table $$people
+ :aggregation [[:aggregation-options [:count] {:name short-col-name}]]
+ :breakout [[:field %state {:name long-col-name}]]
+ :limit 5})
+ qp.compile/compile
+ :query))
+
+ native-query (mt/native-query {:query native-sub-query})
+
+ ;; Let metadata-provider-with-cards-with-metadata-for-queries calculate the result-metadata.
+ metadata-provider (qp.test-util/metadata-provider-with-cards-with-metadata-for-queries [native-query])]
+ (t2.with-temp/with-temp
+ [Card card (assoc {:dataset_query native-query}
+ :result_metadata
+ (-> (lib.metadata.protocols/metadatas metadata-provider :metadata/card [1])
+ first
+ :result-metadata))]
+ (let [card-query {:database (mt/id)
+ :type "query"
+ :query {:source-table (str "card__" (u/the-id card))}}]
+ (mt/with-native-query-testing-context card-query
+ (testing "POST /api/dataset/native"
+ (is (=? {:query validate-native-form
+ :params nil}
+ (-> (mt/user-http-request :crowberto :post 200 "dataset/native" card-query)
+ (update :query #(str/split-lines (or (driver/prettify-native-form :h2 %)
+ "error: no query generated")))))))
+ (testing "POST /api/dataset"
+ (is (=?
+ {:data {:rows [["AK" 68] ["AL" 56] ["AR" 49] ["AZ" 20] ["CA" 90]]
+ :cols [{:name long-col-name
+ :display_name long-col-name
+ :field_ref ["field" long-col-name {}]
+ :source "fields"}
+ {:name short-col-name
+ :display_name short-col-name
+ :field_ref ["field" short-col-name {}]
+ :source "fields"}]
+ :native_form {:query validate-native-form
+ :params nil}
+ :results_timezone "UTC"}
+ :row_count 5
+ :status "completed"
+ :context "ad-hoc"
+ :json_query (merge query-defaults card-query)
+ :database_id (mt/id)}
+ (-> (mt/user-http-request :crowberto :post 202 "dataset" card-query)
+ (update-in [:data :native_form :query]
+ #(str/split-lines (or (driver/prettify-native-form :h2 %)
+ "error: no query generated")))))))))))))
+
(deftest formatted-results-ignore-query-constraints
(testing "POST /api/dataset/:format"
(testing "Downloading CSV/JSON/XLSX results shouldn't be subject to the default query constraints (#9831)"
diff --git a/test/metabase/query_processor_test/nested_queries_test.clj b/test/metabase/query_processor_test/nested_queries_test.clj
index 5cf8bf84018..dc48a7ec39e 100644
--- a/test/metabase/query_processor_test/nested_queries_test.clj
+++ b/test/metabase/query_processor_test/nested_queries_test.clj
@@ -15,6 +15,7 @@
[metabase.lib.schema.id :as lib.schema.id]
[metabase.lib.test-metadata :as meta]
[metabase.lib.test-util :as lib.tu]
+ [metabase.lib.util :as lib.util]
[metabase.models :refer [Field Table]]
[metabase.models.card :as card :refer [Card]]
[metabase.models.collection :as collection :refer [Collection]]
@@ -350,6 +351,60 @@
(run-native-query (str native-sub-query " -- small comment here\n")))
"Ensure trailing comments followed by a newline are trimmed and don't cause a wrapping SQL query to fail"))))
+(defn- col-max-for-driver [driver]
+ ;; There is a default impl for driver/column-name-length-limit, but apparently not all drivers implement
+ ;; table-name-length, so calling column-name-length results in an exception for some drivers.
+ (try (driver/column-name-length-limit driver)
+ (catch Exception _ nil)))
+
+(deftest card-id-native-source-query-with-long-alias-test
+ (testing "nested card with native query and long column alias (metabase##47584)"
+ (mt/test-drivers (set/intersection (mt/normal-drivers-with-feature :nested-queries)
+ (descendants driver/hierarchy :sql))
+ (let [coun-col-name "coun"
+ long-col-full-name "Total_number_of_people_from_each_state_separated_by_state_and_then_we_do_a_count"
+
+ ;; Truncate the long column to something the driver can actually execute.
+ long-col-name (subs long-col-full-name
+ 0
+ (if-let [col-max (col-max-for-driver driver/*driver*)]
+ (min col-max (count long-col-full-name))
+ (count long-col-full-name)))
+
+ ;; Disable truncate-alias when compiling the native query to ensure we don't further truncate the column.
+ ;; We want to simulate a user-defined query where the column name is long, but valid for the driver.
+ native-sub-query (with-redefs [lib.util/truncate-alias
+ (fn mock-truncate-alias
+ ([ss] ss)
+ ([ss _] ss))]
+ (-> (mt/mbql-query people
+ {:source-table $$people
+ :aggregation [[:aggregation-options [:count] {:name coun-col-name}]]
+ :breakout [[:field %state {:name long-col-name}]]
+ :limit 5})
+ qp.compile/compile
+ :query))
+
+ query (query-with-source-card 1)]
+ (qp.store/with-metadata-provider (qp.test-util/metadata-provider-with-cards-with-metadata-for-queries
+ [(mt/native-query {:query native-sub-query})])
+ (mt/with-native-query-testing-context query
+ (let [coun-col-re (re-pattern (str "(?i)" coun-col-name))
+ long-col-re (re-pattern (str "(?i)" long-col-name))]
+ (is (=? {:rows [["AK" 68] ["AL" 56] ["AR" 49] ["AZ" 20] ["CA" 90]],
+ :cols
+ [{:source :fields
+ :name long-col-re
+ :display_name long-col-re
+ :field_ref [:field long-col-re {}]}
+ {:source :fields
+ :name coun-col-re
+ :display_name coun-col-re
+ :field_ref [:field coun-col-re {}]}]}
+ (qp.test-util/rows-and-cols
+ (mt/format-rows-by [str int]
+ (qp/process-query query))))))))))))
+
(defmethod driver/database-supports? [::driver/driver ::filter-by-field-literal-test]
[_driver _feature _database]
false)
--
GitLab