Exclude implicit joins when virtual card table has an explicit join (#25341)

* Exclude implicit joins when virtual card table has an explicit join

* Add unit tests

frontend/src/metabase-lib/lib/metadata/Table.ts

@@ -6,12 +6,14 @@ import Question from "../Question"; // eslint-disable-line import/order
 import { singularize } from "metabase/lib/formatting";
 import { getAggregationOperators } from "metabase/lib/schema_metadata";
 import type { TableId } from "metabase-types/types/Table";
+import { isVirtualCardId } from "metabase/lib/saved-questions/saved-questions";
 import { createLookupByProperty, memoizeClass } from "metabase-lib/lib/utils";
 import Base from "./Base";
 import type Metadata from "./Metadata";
 import type Schema from "./Schema";
 import type Field from "./Field";
 import type Database from "./Database";
+
 /**
  * @typedef { import("./metadata").SchemaName } SchemaName
  * @typedef { import("./metadata").EntityType } EntityType
@@ -33,6 +35,10 @@ class TableInner extends Base {
   metadata?: Metadata;
   db?: Database | undefined | null;
 
+  isVirtualCard() {
+    return isVirtualCardId(this.id);
+  }
+
   hasSchema() {
     return (this.schema_name && this.db && this.db.schemas.length > 1) || false;
   }

frontend/src/metabase-lib/lib/metadata/Table.unit.spec.ts

 import { PRODUCTS } from "__support__/sample_database_fixture";
 import Table from "./Table";
-import type Field from "./Field";
 
 describe("Table", () => {
   const productsTable = new Table(PRODUCTS);
@@ -29,4 +28,17 @@ describe("Table", () => {
       expect(table.connectedTables()).toEqual([productsTable]);
     });
   });
+
+  describe("isVirtualCard", () => {
+    it("should return false when the Table is not a virtual card table", () => {
+      expect(productsTable.isVirtualCard()).toBe(false);
+    });
+
+    it("should return true when the Table is a virtual card table", () => {
+      const virtualCardTable = new Table({
+        id: "card__123",
+      });
+      expect(virtualCardTable.isVirtualCard()).toBe(true);
+    });
+  });
 });

frontend/src/metabase-lib/lib/queries/StructuredQuery.ts

@@ -1270,7 +1270,16 @@ class StructuredQueryInner extends AtomicQuery {
       for (const dimension of fkDimensions) {
         const field = dimension.field();
 
-        if (field && explicitJoins.has(this._keyForFK(field, field.target))) {
+        const queryHasExplicitJoin =
+          field && explicitJoins.has(this._keyForFK(field, field.target));
+        const isNestedCardTable = table?.isVirtualCard();
+        const tableHasExplicitJoin =
+          isNestedCardTable &&
+          table.fields.find(
+            tableField => tableField.id === field.fk_target_field_id,
+          );
+
+        if (queryHasExplicitJoin || tableHasExplicitJoin) {
           continue;
         }
 

frontend/src/metabase/parameters/utils/mapping-options.js

@@ -72,8 +72,19 @@ export function getParameterMappingOptions(
   const question = new Question(card, metadata);
   const query = question.query();
   const options = [];
-
-  if (question.isStructured()) {
+  if (question.isDataset()) {
+    // treat the dataset/model question like it is already composed so that we can apply
+    // dataset/model-specific metadata to the underlying dimension options
+    const composedDatasetQuery = question.composeDataset().query();
+    options.push(
+      ...composedDatasetQuery
+        .dimensionOptions(
+          parameter ? dimensionFilterForParameter(parameter) : undefined,
+        )
+        .sections()
+        .flatMap(section => buildStructuredQuerySectionOptions(section)),
+    );
+  } else if (question.isStructured()) {
     options.push(
       ...query
         .dimensionOptions(

frontend/src/metabase/parameters/utils/mapping-options.unit.spec.js

@@ -18,6 +18,54 @@ function native(native) {
 
 describe("parameters/utils/mapping-options", () => {
   describe("getParameterMappingOptions", () => {
+    describe("Model question", () => {
+      let dataset;
+      let virtualCardTable;
+      beforeEach(() => {
+        const question = ORDERS.question();
+        dataset = question
+          .setCard({ ...question.card(), id: 123 })
+          .setDataset(true);
+
+        // create a virtual table for the card
+        // that contains fields with custom, model-specific metadata
+        virtualCardTable = ORDERS.clone();
+        virtualCardTable.id = `card__123`;
+        virtualCardTable.fields = [
+          ORDERS.CREATED_AT.clone({
+            table_id: `card__123`,
+            uniqueId: `card__123:${ORDERS.CREATED_AT.id}`,
+            display_name: "~*~Created At~*~",
+          }),
+        ];
+
+        // add instances to the `metadata` instance
+        metadata.questions[dataset.id()] = dataset;
+        metadata.tables[virtualCardTable.id] = virtualCardTable;
+        virtualCardTable.fields.forEach(f => {
+          metadata.fields[f.uniqueId] = f;
+        });
+      });
+
+      it("should return fields from the model question's virtual card table, as though it is already nested", () => {
+        const options = getParameterMappingOptions(
+          metadata,
+          { type: "date/single" },
+          dataset.card(),
+        );
+
+        expect(options).toEqual([
+          {
+            icon: "calendar",
+            isForeign: false,
+            name: "~*~Created At~*~",
+            sectionName: "Order",
+            target: ["dimension", ["field", 1, null]],
+          },
+        ]);
+      });
+    });
+
     describe("Structured Query", () => {
       it("should return field-id and fk-> dimensions", () => {
         const options = getParameterMappingOptions(

frontend/test/metabase-lib/lib/queries/StructuredQuery-nesting.unit.spec.js

-import { ORDERS } from "__support__/sample_database_fixture";
+import {
+  ORDERS,
+  PRODUCTS,
+  PEOPLE,
+  metadata,
+} from "__support__/sample_database_fixture";
 
 describe("StructuredQuery nesting", () => {
   describe("nest", () => {
@@ -130,4 +135,77 @@ describe("StructuredQuery nesting", () => {
       expect(d.mbql()).toEqual(["field", ORDERS.TOTAL.id, null]);
     });
   });
+
+  describe("model question", () => {
+    let dataset;
+    let virtualCardTable;
+    beforeEach(() => {
+      const question = ORDERS.question();
+      dataset = question
+        .setCard({ ...question.card(), id: 123 })
+        .setDataset(true);
+
+      // create a virtual table for the card
+      // that contains fields from both Orders and Products tables
+      // to imitate an explicit join of Products to Orders
+      virtualCardTable = ORDERS.clone();
+      virtualCardTable.id = `card__123`;
+      virtualCardTable.fields = virtualCardTable.fields
+        .map(f =>
+          f.clone({
+            table_id: `card__123`,
+            uniqueId: `card__123:${f.id}`,
+          }),
+        )
+        .concat(
+          PRODUCTS.fields.map(f => {
+            const field = f.clone({
+              table_id: `card__123`,
+              uniqueId: `card__123:${f.id}`,
+            });
+
+            return field;
+          }),
+        );
+
+      // add instances to the `metadata` instance
+      metadata.questions[dataset.id()] = dataset;
+      metadata.tables[virtualCardTable.id] = virtualCardTable;
+      virtualCardTable.fields.forEach(f => {
+        metadata.fields[f.uniqueId] = f;
+      });
+    });
+
+    it("should not include implicit join dimensions when the underyling question has an explicit join", () => {
+      const nestedDatasetQuery = dataset.composeDataset().query();
+      expect(
+        // get a list of all dimension options for the nested query
+        nestedDatasetQuery
+          .dimensionOptions()
+          .all()
+          .map(d => d.field().getPlainObject()),
+      ).toEqual([
+        // Order fields
+        ...ORDERS.fields.map(f =>
+          f
+            .clone({
+              table_id: `card__123`,
+              uniqueId: `card__123:${f.id}`,
+            })
+            .getPlainObject(),
+        ),
+        // Product fields from the explicit join
+        ...PRODUCTS.fields.map(f =>
+          f
+            .clone({
+              table_id: `card__123`,
+              uniqueId: `card__123:${f.id}`,
+            })
+            .getPlainObject(),
+        ),
+        // People fields from the implicit join
+        ...PEOPLE.fields.map(f => f.getPlainObject()),
+      ]);
+    });
+  });
 });