From bd838ca49b4b9291bc84e138100fa8ebe89931db Mon Sep 17 00:00:00 2001
From: Paul Rosenzweig <paulrosenzweig@users.noreply.github.com>
Date: Thu, 19 Dec 2019 12:34:15 -0500
Subject: [PATCH] Treat missing "template-tags" as equivalent to
"template-tags": {} (#11517)
---
.../src/metabase/query_builder/selectors.js | 42 ++++--
.../query_builder/selectors.unit.spec.js | 138 ++++++++++--------
2 files changed, 107 insertions(+), 73 deletions(-)
diff --git a/frontend/src/metabase/query_builder/selectors.js b/frontend/src/metabase/query_builder/selectors.js
index 355e1beba51..d9820abcafc 100644
--- a/frontend/src/metabase/query_builder/selectors.js
+++ b/frontend/src/metabase/query_builder/selectors.js
@@ -143,6 +143,30 @@ const getNextRunParameterValues = createSelector(
parameters.map(parameter => parameter.value).filter(p => p !== undefined),
);
+// Certain differences in a query should be ignored. `normalizeQuery`
+// standardizes the query before comparision in `getIsResultDirty`.
+function normalizeQuery(query, tableMetadata) {
+ if (!query) {
+ return query;
+ }
+ if (query.query && tableMetadata) {
+ query = updateIn(query, ["query", "fields"], fields => {
+ fields = fields
+ ? // if the query has fields, copy them before sorting
+ [...fields]
+ : // if the fields aren't set, we get them from the table metadata
+ tableMetadata.fields.map(({ id }) => ["field-id", id]);
+ return fields.sort((a, b) =>
+ JSON.stringify(b).localeCompare(JSON.stringify(a)),
+ );
+ });
+ }
+ if (query.native && query.native["template-tags"] == null) {
+ query.native["template-tags"] = {};
+ }
+ return query;
+}
+
export const getIsResultDirty = createSelector(
[
getLastRunDatasetQuery,
@@ -158,22 +182,8 @@ export const getIsResultDirty = createSelector(
nextParameters,
tableMetadata,
) => {
- // this function sorts fields so that reordering doesn't dirty the result
- const queryWithSortedFields = query =>
- query && query.query && tableMetadata
- ? updateIn(query, ["query", "fields"], fields => {
- fields = fields
- ? // if the query has fields, copy them before sorting
- [...fields]
- : // if the fields aren't set, we get them from the table metadata
- tableMetadata.fields.map(({ id }) => ["field-id", id]);
- return fields.sort((a, b) =>
- JSON.stringify(b).localeCompare(JSON.stringify(a)),
- );
- })
- : query;
- lastDatasetQuery = queryWithSortedFields(lastDatasetQuery);
- nextDatasetQuery = queryWithSortedFields(nextDatasetQuery);
+ lastDatasetQuery = normalizeQuery(lastDatasetQuery, tableMetadata);
+ nextDatasetQuery = normalizeQuery(nextDatasetQuery, tableMetadata);
return (
!Utils.equals(lastDatasetQuery, nextDatasetQuery) ||
!Utils.equals(lastParameters, nextParameters)
diff --git a/frontend/test/metabase/query_builder/selectors.unit.spec.js b/frontend/test/metabase/query_builder/selectors.unit.spec.js
index b92558b3e64..eda0dee442e 100644
--- a/frontend/test/metabase/query_builder/selectors.unit.spec.js
+++ b/frontend/test/metabase/query_builder/selectors.unit.spec.js
@@ -2,70 +2,94 @@ import { getIsResultDirty } from "metabase/query_builder/selectors";
import { state as sampleState } from "__support__/sample_dataset_fixture";
describe("getIsResultDirty", () => {
- function getState(q1, q2) {
- const card = query => ({
- dataset_query: { database: 1, type: "query", query },
+ describe("structure query", () => {
+ function getState(q1, q2) {
+ const card = query => ({
+ dataset_query: { database: 1, type: "query", query },
+ });
+ const qb = { lastRunCard: card(q1), card: card(q2) };
+ return { ...sampleState, qb };
+ }
+
+ it("should not be dirty for empty queries", () => {
+ const state = getState({}, {});
+ expect(getIsResultDirty(state)).toBe(false);
});
- const qb = { lastRunCard: card(q1), card: card(q2) };
- return { ...sampleState, qb };
- }
- it("should not be dirty for empty queries", () => {
- const state = getState({}, {});
- expect(getIsResultDirty(state)).toBe(false);
- });
+ it("should be dirty if the table was changed", () => {
+ const state = getState({ "source-table": 1 }, { "source-table": 2 });
+ expect(getIsResultDirty(state)).toBe(true);
+ });
- it("should be dirty if the table was changed", () => {
- const state = getState({ "source-table": 1 }, { "source-table": 2 });
- expect(getIsResultDirty(state)).toBe(true);
- });
+ it("should be dirty if the fields were changed", () => {
+ const state = getState(
+ { "source-table": 1, fields: [["field-id", 1]] },
+ { "source-table": 1, fields: [["field-id", 2]] },
+ );
+ expect(getIsResultDirty(state)).toBe(true);
+ });
- it("should be dirty if the fields were changed", () => {
- const state = getState(
- { "source-table": 1, fields: [["field-id", 1]] },
- { "source-table": 1, fields: [["field-id", 2]] },
- );
- expect(getIsResultDirty(state)).toBe(true);
- });
+ it("should not be dirty if the fields were reordered", () => {
+ const state = getState(
+ { "source-table": 1, fields: [["field-id", 1], ["field-id", 2]] },
+ { "source-table": 1, fields: [["field-id", 2], ["field-id", 1]] },
+ );
+ expect(getIsResultDirty(state)).toBe(false);
+ });
- it("should not be dirty if the fields were reordered", () => {
- const state = getState(
- { "source-table": 1, fields: [["field-id", 1], ["field-id", 2]] },
- { "source-table": 1, fields: [["field-id", 2], ["field-id", 1]] },
- );
- expect(getIsResultDirty(state)).toBe(false);
- });
+ it("should not be dirty if fields with fk refs were reordered", () => {
+ const state = getState(
+ {
+ "source-table": 1,
+ fields: [["fk->", ["field-id", 1], ["field-id", 2]], ["field-id", 1]],
+ },
+ {
+ "source-table": 1,
+ fields: [["field-id", 1], ["fk->", ["field-id", 1], ["field-id", 2]]],
+ },
+ );
+ expect(getIsResultDirty(state)).toBe(false);
+ });
- it("should not be dirty if fields with fk refs were reordered", () => {
- const state = getState(
- {
- "source-table": 1,
- fields: [["fk->", ["field-id", 1], ["field-id", 2]], ["field-id", 1]],
- },
- {
- "source-table": 1,
- fields: [["field-id", 1], ["fk->", ["field-id", 1], ["field-id", 2]]],
- },
- );
- expect(getIsResultDirty(state)).toBe(false);
+ it("should not be dirty if fields were just made explicit", () => {
+ const state = getState(
+ { "source-table": 1 },
+ {
+ "source-table": 1,
+ fields: [
+ ["field-id", 1],
+ ["field-id", 2],
+ ["field-id", 3],
+ ["field-id", 4],
+ ["field-id", 5],
+ ["field-id", 6],
+ ["field-id", 7],
+ ],
+ },
+ );
+ expect(getIsResultDirty(state)).toBe(false);
+ });
});
+ describe("native query", () => {
+ function getState(q1, q2) {
+ const card = native => ({
+ dataset_query: { database: 1, type: "query", native },
+ });
+ const qb = { lastRunCard: card(q1), card: card(q2) };
+ return { ...sampleState, qb };
+ }
- it("should not be dirty if fields were just made explicit", () => {
- const state = getState(
- { "source-table": 1 },
- {
- "source-table": 1,
- fields: [
- ["field-id", 1],
- ["field-id", 2],
- ["field-id", 3],
- ["field-id", 4],
- ["field-id", 5],
- ["field-id", 6],
- ["field-id", 7],
- ],
- },
- );
- expect(getIsResultDirty(state)).toBe(false);
+ it("should not be dirty if template-tags is empty vs an empty object", () => {
+ const state = getState({}, { "template-tags": {} });
+ expect(getIsResultDirty(state)).toBe(false);
+ });
+
+ it("should be dirty if template-tags differ", () => {
+ const state = getState(
+ { "template-tags": { foo: {} } },
+ { "template-tags": { bar: {} } },
+ );
+ expect(getIsResultDirty(state)).toBe(true);
+ });
});
});
--
GitLab