Port query description logic to MLv2

.clj-kondo/config.edn

@@ -686,9 +686,10 @@
   ;; to the new stricter rules from day 1
   metabase-lib
   {:linters
-   {:docstring-leading-trailing-whitespace {:level :warning}
-    :reduce-without-init                   {:level :warning}
-    :used-underscored-binding              {:level :warning}
-    :single-key-in                         {:level :warning}
-    :keyword-binding                       {:level :warning}
-    :shadowed-var                          {:level :warning}}}}}
+   {:docstring-leading-trailing-whitespace                 {:level :warning}
+    :reduce-without-init                                   {:level :warning}
+    :used-underscored-binding                              {:level :warning}
+    :single-key-in                                         {:level :warning}
+    :keyword-binding                                       {:level :warning}
+    :shadowed-var                                          {:level :warning}
+    :metabase/deftest-not-marked-parallel-or-synchronized  {:level :warning}}}}}

.clj-kondo/hooks/clojure/test.clj

@@ -38,6 +38,14 @@
       (hooks/reg-finding! (assoc (meta test-name)
                                  :message "Test should not be marked both ^:parallel and ^:synchronized"
                                  :type :metabase/validate-deftest)))
+    ;; only when the custom `:metabase/deftest-not-marked-parallel` is enabled: complain if tests are not explicitly
+    ;; marked `^:parallel` or `^:synchronized`. This is mostly to encourage people to mark everything `^:parallel` in
+    ;; places like `metabase.lib` tests unless there is a really good reason not to.
+    (when-not (or parallel? synchronized?)
+      (hooks/reg-finding!
+       (assoc (meta test-name)
+              :message "Test should be marked either ^:parallel or ^:synchronized"
+              :type :metabase/deftest-not-marked-parallel-or-synchronized)))
     (when parallel?
       (doseq [form body]
         (warn-about-disallowed-parallel-forms form))))

frontend/src/metabase-lib/Question.ts

@@ -2,12 +2,10 @@
 // @ts-nocheck
 import _ from "underscore";
 import { assoc, assocIn, chain, dissoc, getIn } from "icepick";
-import inflection from "inflection";
-import { t } from "ttag";
 /* eslint-disable import/order */
 // NOTE: the order of these matters due to circular dependency issues
 import slugg from "slugg";
-import { format as formatExpression } from "metabase-lib/expressions/format";
+import * as MLv2 from "cljs/metabase.lib.js";
 import StructuredQuery, {
   STRUCTURED_QUERY_TEMPLATE,
 } from "metabase-lib/queries/StructuredQuery";
@@ -27,8 +25,6 @@ import { memoizeClass, sortObject } from "metabase-lib/utils";
 
 import * as AGGREGATION from "metabase-lib/queries/utils/aggregation";
 import * as FILTER from "metabase-lib/queries/utils/filter";
-import * as DESCRIPTION from "metabase-lib/queries/utils/description";
-import * as FIELD_REF from "metabase-lib/queries/utils/field-ref";
 import * as QUERY from "metabase-lib/queries/utils/query";
 
 // TODO: remove these dependencies
@@ -1327,216 +1323,31 @@ class QuestionInner {
     return hasQueryBeenAltered ? question.markDirty() : question;
   }
 
-  generateQueryDescription(tableMetadata, options = {}) {
-    if (!tableMetadata || (this.isNative() && !this.displayName())) {
-      return "";
-    }
-
-    options = {
-      sections: [
-        "table",
-        "aggregation",
-        "breakout",
-        "filter",
-        "order-by",
-        "limit",
-      ],
-      ...options,
-    };
-
-    const sectionFns = {
-      table: this._getTableDescription.bind(this),
-      aggregation: this._getAggregationDescription.bind(this),
-      breakout: this._getBreakoutDescription.bind(this),
-      filter: this._getFilterDescription.bind(this),
-      "order-by": this._getOrderByDescription.bind(this),
-      limit: this._getLimitDescription.bind(this),
-    };
-
-    // these array gymnastics are needed to support JSX formatting
-    const query = this.datasetQuery().query;
-    const sections = options.sections
-      .map(section =>
-        _.flatten(sectionFns[section](tableMetadata, query, options)).filter(
-          s => !!s,
-        ),
-      )
-      .filter(s => s && s.length > 0);
-
-    const description = _.flatten(DESCRIPTION.joinList(sections, ", "));
-    return description.join("");
-  }
-
-  private _getFieldName(tableMetadata, field, options) {
-    try {
-      const target = FIELD_REF.getFieldTarget(field, tableMetadata);
-      const components = [];
-      if (target.path) {
-        for (const fieldDef of target.path) {
-          components.push(DESCRIPTION.formatField(fieldDef, options), " → ");
-        }
-      }
-      components.push(DESCRIPTION.formatField(target.field, options));
-      if (target.unit) {
-        components.push(` (${target.unit})`);
-      }
-      return components;
-    } catch (e) {
-      console.warn(
-        "Couldn't format field name for field",
-        field,
-        "in table",
-        tableMetadata,
-      );
-    }
-    // TODO: This is untranslated.
-    return "[Unknown Field]";
-  }
-
-  private _getTableDescription(tableMetadata) {
-    return [inflection.pluralize(tableMetadata.display_name)];
-  }
-
-  private _getAggregationDescription(tableMetadata, query, options) {
-    return DESCRIPTION.conjunctList(
-      QUERY.getAggregations(query).map(aggregation => {
-        if (AGGREGATION.hasOptions(aggregation)) {
-          if (AGGREGATION.isNamed(aggregation)) {
-            return [AGGREGATION.getName(aggregation)];
-          }
-          aggregation = AGGREGATION.getContent(aggregation);
-        }
-        if (AGGREGATION.isMetric(aggregation)) {
-          const metric = _.findWhere(tableMetadata.metrics, {
-            id: AGGREGATION.getMetric(aggregation),
-          });
-          // TODO: This is untranslated.
-          return metric ? metric.name : "[Unknown Metric]";
-        }
-        switch (aggregation[0]) {
-          case "rows":
-            return [t`Raw data`];
-          case "count":
-            return [t`Count`];
-          case "cum-count":
-            return [t`Cumulative count`];
-          case "avg":
-            return [
-              t`Average of `,
-              this._getFieldName(tableMetadata, aggregation[1], options),
-            ];
-          case "median":
-            return [
-              t`Median of `,
-              this._getFieldName(tableMetadata, aggregation[1], options),
-            ];
-          case "distinct":
-            return [
-              t`Distinct values of `,
-              this._getFieldName(tableMetadata, aggregation[1], options),
-            ];
-          case "stddev":
-            return [
-              t`Standard deviation of `,
-              this._getFieldName(tableMetadata, aggregation[1], options),
-            ];
-          case "sum":
-            return [
-              t`Sum of `,
-              this._getFieldName(tableMetadata, aggregation[1], options),
-            ];
-          case "cum-sum":
-            return [
-              t`Cumulative sum of `,
-              this._getFieldName(tableMetadata, aggregation[1], options),
-            ];
-          case "max":
-            return [
-              t`Maximum of `,
-              this._getFieldName(tableMetadata, aggregation[1], options),
-            ];
-          case "min":
-            return [
-              t`Minimum of `,
-              this._getFieldName(tableMetadata, aggregation[1], options),
-            ];
-          default:
-            return [formatExpression(aggregation, { tableMetadata })];
-        }
-      }),
-      // TODO: This is untranslated. See if there's an i18n-friendly way to do a comma-separated list.
-      "and",
-    );
-  }
-
-  private _getBreakoutDescription(tableMetadata, { breakout }, options) {
-    if (breakout && breakout.length > 0) {
-      return [
-        t`Grouped by `,
-        DESCRIPTION.joinList(
-          breakout.map(b => this._getFieldName(tableMetadata, b, options)),
-          // TODO: This is untranslated. See if there's an i18n-friendly way to do a comma-separated list.
-          " and ",
-        ),
-      ];
-    }
-  }
-
-  private _getFilterDescription(tableMetadata, query, options) {
-    // getFilters returns list of filters without the implied "and"
-    // TODO: This is untranslated. See if there's an i18n-friendly way to do a comma-separated list.
-    const filters = ["and"].concat(QUERY.getFilters(query));
-    if (filters && filters.length > 1) {
-      return [
-        t`Filtered by `,
-        this._getFilterClauseDescription(tableMetadata, filters, options),
-      ];
-    }
+  _getMLv2Query(metadata = this._metadata) {
+    return MLv2.query(this.databaseId(), metadata, this.datasetQuery());
   }
 
-  private _getFilterClauseDescription(tableMetadata, filter, options) {
-    if (filter[0] === "and" || filter[0] === "or") {
-      const clauses = filter
-        .slice(1)
-        .map(f => this._getFilterClauseDescription(tableMetadata, f, options));
-      return DESCRIPTION.conjunctList(clauses, filter[0].toLowerCase());
-    } else if (filter[0] === "segment") {
-      const segment = _.findWhere(tableMetadata.segments, { id: filter[1] });
-      return segment ? segment.name : "[Unknown Segment]";
-    } else if (filter[0] === "between" && filter[1][0] === "+") {
-      return this._getFieldName(tableMetadata, filter[1][1], options);
-    } else {
-      return this._getFieldName(tableMetadata, filter[1], options);
-    }
-  }
+  generateQueryDescription(tableMetadata) {
+    let metadata =
+      tableMetadata?.id != null
+        ? assocIn(
+            this._metadata,
+            ["tables", String(tableMetadata.id)],
+            tableMetadata,
+          )
+        : this._metadata;
 
-  private _getOrderByDescription(tableMetadata, query, options) {
-    const orderBy = query["order-by"];
-    if (orderBy && orderBy.length > 0) {
-      return [
-        t`Sorted by `,
-        DESCRIPTION.joinList(
-          orderBy.map(([direction, field]) => {
-            const name = FIELD_REF.isAggregateField(field)
-              ? this._getAggregationDescription(tableMetadata, query, options)
-              : this._getFieldName(tableMetadata, field, options);
-
-            return (
-              // TODO: This is untranslated.
-              name + " " + (direction === "asc" ? "ascending" : "descending")
-            );
-          }),
-          // TODO: This is untranslated. See if there's an i18n-friendly way to do lists.
-          " and ",
-        ),
-      ];
+    if (tableMetadata) {
+      for (const fieldMetadata of tableMetadata?.fields) {
+        metadata = assocIn(
+          metadata,
+          ["fields", String(fieldMetadata.id)],
+          fieldMetadata,
+        );
+      }
     }
-  }
 
-  private _getLimitDescription(tableMetadata, { limit }) {
-    if (limit != null) {
-      return [limit, " ", inflection.inflect("row", limit)];
-    }
+    return MLv2.suggestedName(this._getMLv2Query(metadata));
   }
 
   getUrlWithParameters(parameters, parameterValues, { objectId, clean } = {}) {

frontend/src/metabase-lib/queries/utils/description.js

@@ -145,7 +145,7 @@ export function joinList(list, joiner) {
   );
 }
 
-export function conjunctList(list, conjunction) {
+function conjunctList(list, conjunction) {
   switch (list.length) {
     case 0:
       return null;

frontend/test/metabase-lib/lib/Question.unit.spec.js

@@ -1605,47 +1605,6 @@ describe("Question", () => {
       );
     });
   });
-
-  describe("Question._getOrderByDescription", () => {
-    it("should work with fields", () => {
-      const query = {
-        "source-table": PRODUCTS.id,
-        "order-by": [["asc", ["field", PRODUCTS.CATEGORY.id, null]]],
-      };
-
-      expect(base_question._getOrderByDescription(PRODUCTS, query)).toEqual([
-        "Sorted by ",
-        ["Category ascending"],
-      ]);
-    });
-
-    it("should work with aggregations", () => {
-      const query = {
-        "source-table": PRODUCTS.id,
-        aggregation: [["count"]],
-        breakout: [["field", PRODUCTS.CATEGORY.id, null]],
-        "order-by": [["asc", ["aggregation", 0, null]]],
-      };
-      expect(base_question._getOrderByDescription(PRODUCTS, query)).toEqual([
-        "Sorted by ",
-        ["Count ascending"],
-      ]);
-    });
-
-    it("should work with expressions", () => {
-      const query = {
-        "source-table": PRODUCTS.id,
-        expressions: {
-          Foo: ["concat", "Foo ", ["field", 4, null]],
-        },
-        "order-by": [["asc", ["expression", "Foo", null]]],
-      };
-      expect(base_question._getOrderByDescription(PRODUCTS, query)).toEqual([
-        "Sorted by ",
-        ["Foo ascending"],
-      ]);
-    });
-  });
 });
 
 function parseUrl(url) {

shadow-cljs.edn

@@ -16,15 +16,16 @@
                 :compiler-options {:reader-features #{:cljs-dev}}}
    :compiler-options {:reader-features #{:cljs-release}}
    :entries    [metabase.domain-entities.queries.util
+                metabase.lib.js
                 metabase.mbql.js
-                metabase.types
                 metabase.shared.formatting.constants
                 metabase.shared.formatting.date
                 metabase.shared.formatting.numbers
                 metabase.shared.formatting.time
+                metabase.shared.parameters.parameters
                 metabase.shared.util.currency
                 metabase.shared.util.time
-                metabase.shared.parameters.parameters
+                metabase.types
                 metabase.util.devtools]}
 
   :test

shared/src/metabase/shared/parameters/parameters.cljc

@@ -66,7 +66,6 @@
            (formatted-value :date/single end locale))
       "")))
 
-;;; TODO -- sorta duplicated with [[metabase.lib.metadata.calculate.display-name/interval-display-name]] but not exactly
 (defn- translated-interval
   [interval n]
   (case interval