Skip to content
Snippets Groups Projects
Unverified Commit e0d5922e authored by Braden Shepherdson's avatar Braden Shepherdson Committed by GitHub
Browse files

[MLv2] Drop non-standard normalization in QB selectors (#29830) 

Use the canonical `metabase.mbql.js/normalize` instead, with a bit of
customization for the query builder's needs. In the long run this should
be fully unified; for now at least the logic is inside MLv2 now.

All the test cases from the existing code are still exercised between
`selectors.unit.spec` and `metabase.mbql.js_test`.
parent 5bfa9916
No related merge requests found
Hide whitespace changes
Inline Side-by-side
Showing
with 191 additions and 274 deletions
frontend/src/metabase-lib/comparison.ts 0 → 100644
+ 10
− 0
View file @ e0d5922e
import * as ML from "cljs/metabase.lib.js";
import type { DatasetQuery } from "metabase-types/api";
export function areLegacyQueriesEqual(
query1: DatasetQuery,
query2: DatasetQuery,
fieldIds?: number[],
): boolean {
return ML.query_EQ_(query1, query2, fieldIds);
}
frontend/src/metabase-lib/v2.ts
+ 1
− 0
View file @ e0d5922e
// Note: only metabase-lib v2 exports should be added here
export * from "./comparison";
export * from "./metadata";
export * from "./limit";
export * from "./order_by";
......
frontend/src/metabase/query_builder/selectors.js
+ 13
− 49
View file @ e0d5922e
......@@ -4,7 +4,7 @@ import d3 from "d3";
import { createSelector } from "reselect";
import createCachedSelector from "re-reselect";
import _ from "underscore";
import { assocIn, getIn, merge, updateIn } from "icepick";
import { getIn, merge, updateIn } from "icepick";
// Needed due to wrong dependency resolution order
// eslint-disable-next-line no-unused-vars
......@@ -32,6 +32,7 @@ import {
import ObjectMode from "metabase/modes/components/modes/ObjectMode";
import { LOAD_COMPLETE_FAVICON } from "metabase/hoc/Favicon";
import * as ML from "metabase-lib/v2";
import { getCardUiParameters } from "metabase-lib/parameters/utils/cards";
import {
normalizeParameters,
......@@ -348,53 +349,16 @@ export const getQuestionFromCard = createCachedSelector(
(metadata, card) => new Question(card, metadata),
)((_state, card) => card.id);
function normalizeClause(clause) {
return typeof clause?.raw === "function" ? clause.raw() : clause;
}
// Certain differences in a query should be ignored. `normalizeQuery`
// standardizes the query before comparison in `getIsResultDirty`.
export function normalizeQuery(query, tableMetadata) {
if (!query) {
return query;
}
if (query.query) {
if (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, null]);
return fields.sort((a, b) =>
JSON.stringify(b).localeCompare(JSON.stringify(a)),
);
});
}
["aggregation", "breakout", "filter", "joins", "order-by"].forEach(
clauseList => {
if (query.query[clauseList]) {
query = updateIn(query, ["query", clauseList], clauses =>
clauses.map(normalizeClause),
);
}
},
);
}
if (query.native && query.native["template-tags"] == null) {
query = assocIn(query, ["native", "template-tags"], {});
}
return query;
}
function isQuestionEditable(question) {
return !question?.query().readOnly();
}
function areQueriesEqual(queryA, queryB, tableMetadata) {
const normalizedQueryA = normalizeQuery(queryA, tableMetadata);
const normalizedQueryB = normalizeQuery(queryB, tableMetadata);
return _.isEqual(normalizedQueryA, normalizedQueryB);
function areLegacyQueriesEqual(queryA, queryB, tableMetadata) {
return ML.areLegacyQueriesEqual(
queryA,
queryB,
tableMetadata?.fields.map(({ id }) => id),
);
}
// Model questions may be composed via the `composeDataset` method.
......@@ -413,12 +377,12 @@ function areModelsEquivalent({
const composedOriginal = originalQuestion.composeDataset();
const isLastRunComposed = areQueriesEqual(
const isLastRunComposed = areLegacyQueriesEqual(
lastRunQuestion.datasetQuery(),
composedOriginal.datasetQuery(),
tableMetadata,
);
const isCurrentComposed = areQueriesEqual(
const isCurrentComposed = areLegacyQueriesEqual(
currentQuestion.datasetQuery(),
composedOriginal.datasetQuery(),
tableMetadata,
......@@ -426,7 +390,7 @@ function areModelsEquivalent({
const isLastRunEquivalentToCurrent =
isLastRunComposed &&
areQueriesEqual(
areLegacyQueriesEqual(
currentQuestion.datasetQuery(),
originalQuestion.datasetQuery(),
tableMetadata,
......@@ -434,7 +398,7 @@ function areModelsEquivalent({
const isCurrentEquivalentToLastRun =
isCurrentComposed &&
areQueriesEqual(
areLegacyQueriesEqual(
lastRunQuestion.datasetQuery(),
originalQuestion.datasetQuery(),
tableMetadata,
......@@ -450,7 +414,7 @@ function areQueriesEquivalent({
tableMetadata,
}) {
return (
areQueriesEqual(
areLegacyQueriesEqual(
lastRunQuestion?.datasetQuery(),
currentQuestion?.datasetQuery(),
tableMetadata,
......
frontend/src/metabase/query_builder/selectors.unit.spec.js
+ 44
− 1
View file @ e0d5922e
......@@ -6,8 +6,17 @@ import {
getNativeEditorSelectedText,
getQuestionDetailsTimelineDrawerState,
} from "metabase/query_builder/selectors";
import { state as sampleState } from "__support__/sample_database_fixture";
import {
ORDERS,
PRODUCTS,
state as sampleState,
} from "__support__/sample_database_fixture";
import Question from "metabase-lib/Question";
import Aggregation from "metabase-lib/queries/structured/Aggregation";
import Breakout from "metabase-lib/queries/structured/Breakout";
import Filter from "metabase-lib/queries/structured/Filter";
import Join from "metabase-lib/queries/structured/Join";
import OrderBy from "metabase-lib/queries/structured/OrderBy";
function getBaseState({ uiControls = {}, ...state } = {}) {
return {
......@@ -133,6 +142,40 @@ describe("getIsResultDirty", () => {
expect(getIsResultDirty(state)).toBe(true);
});
it("converts clauses into plain MBQL objects", () => {
const aggregation = ["count"];
const breakout = ORDERS.CREATED_AT.reference();
const filter = [">=", ORDERS.TOTAL.reference(), 20];
const orderBy = ["asc", ["aggregation", 0]];
const join = {
alias: "Products",
condition: [
"=",
["field", ORDERS.PRODUCT_ID.id, null],
["field", PRODUCTS.ID.id, null],
],
};
const state = getState(
{
aggregation: [new Aggregation(aggregation)],
breakout: [new Breakout(breakout)],
filter: [new Filter(filter)],
joins: [new Join(join)],
"order-by": [new OrderBy(orderBy)],
},
{
aggregation: [aggregation],
breakout: [breakout],
filter: [filter],
joins: [join],
"order-by": [orderBy],
},
);
expect(getIsResultDirty(state)).toBe(false);
});
it("should not be dirty if the fields were reordered", () => {
const state = getState(
{
......
frontend/src/metabase/query_builder/utils.unit.spec.js deleted 100644 → 0
+ 0
− 222
View file @ 5bfa9916
import {
SAMPLE_DATABASE,
ORDERS,
PRODUCTS,
metadata,
} from "__support__/sample_database_fixture";
import Question from "metabase-lib/Question";
import Aggregation from "metabase-lib/queries/structured/Aggregation";
import Breakout from "metabase-lib/queries/structured/Breakout";
import Filter from "metabase-lib/queries/structured/Filter";
import Join from "metabase-lib/queries/structured/Join";
import OrderBy from "metabase-lib/queries/structured/OrderBy";
import { normalizeQuery } from "./selectors";
function toFieldRef(field) {
return ["field", field.id, null];
}
function sortFields(f1, f2) {
return JSON.stringify(f2).localeCompare(JSON.stringify(f1));
}
function getTableFields(tableId) {
const table = SAMPLE_DATABASE.tables.find(table => table.id === tableId);
return table.fields.map(toFieldRef).sort(sortFields);
}
function getQuestion({ type = "query", query = {} } = {}) {
const queryObjectKey = type === "query" ? "query" : "native";
let queryObject = {};
if (type === "query") {
queryObject = {
...query,
"source-table": ORDERS.id,
};
} else {
queryObject = query;
}
return new Question({
display: "table",
visualization_settings: {},
dataset_query: {
type,
database: SAMPLE_DATABASE.id,
[queryObjectKey]: queryObject,
},
});
}
function setup(questionOpts) {
const question = getQuestion(questionOpts);
const query = question.query();
const tableMetadata = question.isStructured()
? metadata.table(query.sourceTableId())
: null;
return { question, query, datasetQuery: query.datasetQuery(), tableMetadata };
}
const FEW_ORDERS_TABLE_FIELDS = [
ORDERS.ID,
ORDERS.TOTAL,
ORDERS.CREATED_AT,
].map(toFieldRef);
const TEST_CLAUSE = {
AGGREGATION: ["count"],
BREAKOUT: toFieldRef(ORDERS.CREATED_AT),
FILTER: [">=", toFieldRef(ORDERS.TOTAL), 20],
ORDER_BY: ["asc", ["aggregation", 0]],
JOIN: {
alias: "Products",
condition: ["=", toFieldRef(ORDERS.PRODUCT_ID), toFieldRef(PRODUCTS.ID)],
},
};
describe("normalizeQuery", () => {
it("does nothing if query is nullish", () => {
expect(normalizeQuery(null)).toBe(null);
expect(normalizeQuery(undefined)).toBe(undefined);
});
describe("structured query", () => {
it("handles null in filter clauses", () => {
const FILTER_WITH_NULL = ["=", ["field", ORDERS.TOTAL, null], null];
const { datasetQuery } = setup({
query: {
filter: FILTER_WITH_NULL,
},
});
const { query: normalizedQuery } = normalizeQuery(datasetQuery);
expect(normalizedQuery).toEqual({
...datasetQuery.query,
filter: FILTER_WITH_NULL,
});
});
it("adds explicit list of fields if missing", () => {
const { datasetQuery, query, tableMetadata } = setup();
const expectedFields = getTableFields(query.sourceTableId());
const normalizedQuery = normalizeQuery(datasetQuery, tableMetadata);
expect(normalizedQuery.query).toEqual(
expect.objectContaining({
fields: expectedFields,
}),
);
});
it("sorts query fields if they're set explicitly", () => {
const { datasetQuery, tableMetadata } = setup({
query: { fields: FEW_ORDERS_TABLE_FIELDS },
});
const normalizedQuery = normalizeQuery(datasetQuery, tableMetadata);
expect(normalizedQuery.query.fields).toEqual(
FEW_ORDERS_TABLE_FIELDS.sort(sortFields),
);
});
it("does nothing to query fields if table metadata is not provided", () => {
const { datasetQuery } = setup({
query: { fields: FEW_ORDERS_TABLE_FIELDS },
});
const normalizedQuery = normalizeQuery(datasetQuery);
expect(normalizedQuery).toEqual(datasetQuery);
});
it("converts clauses into plain MBQL objects", () => {
const { datasetQuery } = setup({
query: {
aggregation: [new Aggregation(TEST_CLAUSE.AGGREGATION)],
breakout: [new Breakout(TEST_CLAUSE.BREAKOUT)],
filter: [new Filter(TEST_CLAUSE.FILTER)],
joins: [new Join(TEST_CLAUSE.JOIN)],
"order-by": [new OrderBy(TEST_CLAUSE.ORDER_BY)],
},
});
const { query: normalizedQuery } = normalizeQuery(datasetQuery);
expect(normalizedQuery).toEqual({
...datasetQuery.query,
aggregation: [TEST_CLAUSE.AGGREGATION],
breakout: [TEST_CLAUSE.BREAKOUT],
filter: [TEST_CLAUSE.FILTER],
joins: [TEST_CLAUSE.JOIN],
"order-by": [TEST_CLAUSE.ORDER_BY],
});
expect(normalizedQuery.aggregation[0]).not.toBeInstanceOf(Aggregation);
expect(normalizedQuery.breakout[0]).not.toBeInstanceOf(Breakout);
expect(normalizedQuery.filter[0]).not.toBeInstanceOf(Filter);
expect(normalizedQuery.joins[0]).not.toBeInstanceOf(Join);
expect(normalizedQuery["order-by"][0]).not.toBeInstanceOf(OrderBy);
});
it("does nothing to clauses if they're plain MBQL already", () => {
const { datasetQuery } = setup({
query: {
aggregation: [TEST_CLAUSE.AGGREGATION],
breakout: [TEST_CLAUSE.BREAKOUT],
filter: [TEST_CLAUSE.FILTER],
joins: [TEST_CLAUSE.JOIN],
"order-by": [TEST_CLAUSE.ORDER_BY],
},
});
const { query: normalizedQuery } = normalizeQuery(datasetQuery);
expect(normalizedQuery).toEqual(datasetQuery.query);
expect(normalizedQuery.aggregation[0]).not.toBeInstanceOf(Aggregation);
expect(normalizedQuery.breakout[0]).not.toBeInstanceOf(Breakout);
expect(normalizedQuery.filter[0]).not.toBeInstanceOf(Filter);
expect(normalizedQuery.joins[0]).not.toBeInstanceOf(Join);
expect(normalizedQuery["order-by"][0]).not.toBeInstanceOf(OrderBy);
});
});
describe("native query", () => {
it("assigns empty object to template tags if missing", () => {
const { datasetQuery } = setup({
type: "native",
});
const normalizedQuery = normalizeQuery(datasetQuery);
expect(normalizedQuery).toEqual({
...datasetQuery,
native: {
...datasetQuery.native,
"template-tags": {},
},
});
});
it("does nothing to template tags if they're set explicitly", () => {
const { datasetQuery } = setup({
type: "native",
query: {
"template-tags": {
total: {
name: "total",
type: "dimension",
},
},
},
});
const normalizedQuery = normalizeQuery(datasetQuery);
expect(normalizedQuery).toEqual(datasetQuery);
});
});
});
shared/src/metabase/mbql/js.cljs
+ 26
− 2
View file @ e0d5922e
......@@ -4,7 +4,31 @@
[metabase.mbql.normalize :as mbql.normalize]
[metabase.util :as u]))
(defn- unwrap
"Sometimes JS queries are passed in with a `Join` or `Aggregation` clause object instead of a simple Array.
These clauses `extend Array` so `Array.isArray(x)` is true, but they're treated as opaque by `js->clj`.
This recurses over the whole query, unwrapping these values to their `.raw()` form."
[x]
(cond
;; (object? x) only matches for things that are plain objects. eg. `(object? (js/Date.))` is false.
;; This matches anything that descends from `Object`, like `Join` clause, and has a `.raw()` method.
(and x
(instance? js/Object x)
(fn? (.-raw x))) (-> x (.raw) js->clj unwrap)
(map? x) (update-vals x unwrap)
(sequential? x) (mapv unwrap x)
:else x))
(defn normalize-cljs
"Normalize an MBQL query, and convert it to the latest and greatest version of MBQL.
Returns the CLJS form of the normalized query. Use [[normalize]] for the JS form."
[query]
(-> query js->clj unwrap mbql.normalize/normalize))
(defn ^:export normalize
"Normalize an MBQL query, and convert it to the latest and greatest version of MBQL."
"Normalize an MBQL query, and convert it to the latest and greatest version of MBQL.
Returns the JS form of the normalized query. Use [[normalize-cljs]] for the CLJS form."
[query]
(-> query js->clj mbql.normalize/normalize (clj->js :keyword-fn u/qualified-name)))
(-> query normalize-cljs (clj->js :keyword-fn u/qualified-name)))
src/metabase/lib/js.cljs
+ 31
− 0
View file @ e0d5922e
(ns metabase.lib.js
"JavaScript-friendly interface to the entire Metabase lib? This stuff will probably change a bit as MLv2 evolves."
(:require
[medley.core :as m]
[metabase.lib.convert :as convert]
[metabase.lib.core :as lib.core]
[metabase.lib.js.metadata :as js.metadata]
......@@ -9,6 +10,7 @@
[metabase.lib.normalize :as lib.normalize]
[metabase.lib.order-by :as lib.order-by]
[metabase.lib.query :as lib.query]
[metabase.mbql.js :as mbql.js]
[metabase.mbql.normalize :as mbql.normalize]
[metabase.util :as u]
[metabase.util.log :as log]))
......@@ -156,3 +158,32 @@
a-query stage-number
(lib.normalize/normalize (js->clj target-clause :keywordize-keys true))
(lib.normalize/normalize (js->clj new-clause :keywordize-keys true)))))
(defn- prep-query-for-equals [a-query field-ids]
(-> a-query
mbql.js/normalize-cljs
;; If `:native` exists, but it doesn't have `:template-tags`, add it.
(m/update-existing :native #(merge {:template-tags {}} %))
(m/update-existing :query (fn [inner-query]
(let [fields (or (:fields inner-query)
(for [id field-ids]
[:field id nil]))]
;; We ignore the order of the fields in the lists, but need to make sure any dupes
;; match up. Therefore de-dupe with `frequencies` rather than simply `set`.
(assoc inner-query :fields (frequencies fields)))))))
(defn ^:export query=
"Returns whether the provided queries should be considered equal.
If `field-ids` is specified, an input MBQL query without `:fields` set defaults to the `field-ids`.
Currently this works only for legacy queries in JS form!
It duplicates the logic formerly found in `query_builder/selectors.js`.
TODO: This should evolve into a more robust, pMBQL-based sense of equality over time.
For now it pulls logic that touches query internals into `metabase.lib`."
([query1 query2] (query= query1 query2 nil))
([query1 query2 field-ids]
(let [n1 (prep-query-for-equals query1 field-ids)
n2 (prep-query-for-equals query2 field-ids)]
(= n1 n2))))
test/metabase/lib/js_test.cljs 0 → 100644
+ 66
− 0
View file @ e0d5922e
(ns metabase.lib.js-test
(:require
[clojure.test :refer [deftest is testing]]
[metabase.lib.js :as lib.js]))
(deftest query=-test
(doseq [q1 [nil js/undefined]
q2 [nil js/undefined]]
(is (lib.js/query= q1 q2)))
(testing "explicit fields vs. implied fields"
(let [q1 #js {"query" #js {"source-table" 1}}
q2 #js {"query" #js {"source-table" 1
"fields" #js [#js ["field" 1 nil]
#js ["field" 2 nil]
#js ["field" 3 nil]
#js ["field" 4 nil]
#js ["field" 4 nil] ; duplicates are okay
#js ["field" 4 nil]
#js ["field" 5 nil]
#js ["field" 6 nil]
#js ["field" 7 nil]]}}
;; Note that the order is not relevant; they get grouped.
;; Duplicates are okay, and are tracked.
field-ids #js [1 2 6 7 3 5 4 4 4]]
(is (not (lib.js/query= q1 q2))
"the field-ids must be provided to populate q1")
(is (lib.js/query= q1 q1 field-ids))
(is (not (lib.js/query= q1 q2 (conj (vec field-ids) 2)))
"duplicates are tracked, so an extra dupe breaks it")))
(testing "missing and extra fields"
(let [q1 #js {"query" #js {"source-table" 1
"fields" #js [#js ["field" 1 nil]
#js ["field" 2 nil]]}}
;; Same fields, different order.
q2 #js {"query" #js {"source-table" 1
"fields" #js [#js ["field" 2 nil]
#js ["field" 1 nil]]}}
;; Different fields
q3 #js {"query" #js {"source-table" 1
"fields" #js [#js ["field" 3 nil]
#js ["field" 1 nil]]}}]
(is (lib.js/query= q1 q2))
(is (not (lib.js/query= q1 q3)))
(is (not (lib.js/query= q2 q3))))))
(deftype FakeJoin [guts]
Object
(raw [_this] guts))
(deftest query=-unwrapping-test
(testing "JS wrapper types like Join get unwrapped"
;; This doesn't use the real Join classes, just pretends it has one.
(let [join #js {"alias" "Products"
"condition" #js ["=" #js ["field" 7 nil] #js ["field" 19 nil]]}
join-class (FakeJoin. join)
basic-query #js {"type" "query"
"query" #js {"joins" #js [join]}}
classy-query #js {"type" "query"
"query" #js {"joins" #js [join-class]}}
]
(is (not= join join-class))
(is (not= (js->clj join) (js->clj join-class)))
(is (lib.js/query= basic-query classy-query)))))
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment