From 271e2372e40f0f7e35fef8ff4750a7e939598502 Mon Sep 17 00:00:00 2001
From: Alexander Polyankin <alexander.polyankin@metabase.com>
Date: Mon, 22 May 2023 15:07:53 +0300
Subject: [PATCH] Fix Field.id for cases where it's FieldReference (#30899)
---
.../metabase-lib/metadata/Table.unit.spec.ts | 10 ++-
frontend/src/metabase-lib/metadata/mocks.ts | 32 -------
.../metadata/utils/fields.unit.spec.ts | 88 ++++++++++++-------
.../src/metabase-lib/metadata/utils/models.ts | 3 +-
frontend/src/metabase-types/api/field.ts | 9 +-
.../ModelDetailPage.unit.spec.tsx | 5 +-
.../parameters/utils/formatting.unit.spec.ts | 21 +++--
.../metabase-lib/lib/Dimension.unit.spec.js | 10 +--
8 files changed, 87 insertions(+), 91 deletions(-)
delete mode 100644 frontend/src/metabase-lib/metadata/mocks.ts
diff --git a/frontend/src/metabase-lib/metadata/Table.unit.spec.ts b/frontend/src/metabase-lib/metadata/Table.unit.spec.ts
index 58ac1aa1eea..1a802d70630 100644
--- a/frontend/src/metabase-lib/metadata/Table.unit.spec.ts
+++ b/frontend/src/metabase-lib/metadata/Table.unit.spec.ts
@@ -9,14 +9,16 @@ const TABLE_ORIGIN_ID = 1;
const TABLE_DESTINATION_ID = 2;
const TABLE_EMPTY_ID = 3;
const TABLE_VIRTUAL_ID = "card__1";
+const FIELD_ORIGIN_ID = 1;
+const FIELD_DESTINATION_ID = 2;
const FIELD_ORIGIN = createMockField({
- id: 1,
+ id: FIELD_ORIGIN_ID,
table_id: TABLE_ORIGIN_ID,
});
const FIELD_DESTINATION = createMockField({
- id: 2,
+ id: FIELD_DESTINATION_ID,
table_id: TABLE_DESTINATION_ID,
});
@@ -31,9 +33,9 @@ const TABLE_DESTINATION = createMockTable({
fks: [
createMockForeignKey({
origin: FIELD_ORIGIN,
- origin_id: FIELD_ORIGIN.id,
+ origin_id: FIELD_ORIGIN_ID,
destination: FIELD_DESTINATION,
- destination_id: FIELD_DESTINATION.id,
+ destination_id: FIELD_DESTINATION_ID,
}),
],
});
diff --git a/frontend/src/metabase-lib/metadata/mocks.ts b/frontend/src/metabase-lib/metadata/mocks.ts
deleted file mode 100644
index c62e9e04467..00000000000
--- a/frontend/src/metabase-lib/metadata/mocks.ts
+++ /dev/null
@@ -1,32 +0,0 @@
-import { createMockField } from "metabase-types/api/mocks/field";
-import { Field as ApiField } from "metabase-types/api";
-import Field from "./Field";
-
-export function createMockConcreteField({
- apiOpts,
- instanceOpts,
-}: {
- apiOpts?: Partial<ApiField>;
- instanceOpts?: Partial<Field>;
-}) {
- const mockField = createMockField(apiOpts);
- const instance = new Field(mockField);
-
- Object.assign(instance, instanceOpts);
-
- return instance;
-}
-
-export function createMockVirtualField({
- constructorOpts,
- instanceOpts,
-}: {
- constructorOpts?: Partial<Field>;
- instanceOpts?: Partial<Field>;
-}) {
- const instance = new Field(constructorOpts);
-
- Object.assign(instance, instanceOpts);
-
- return instance;
-}
diff --git a/frontend/src/metabase-lib/metadata/utils/fields.unit.spec.ts b/frontend/src/metabase-lib/metadata/utils/fields.unit.spec.ts
index d941771f120..abbe9ccb039 100644
--- a/frontend/src/metabase-lib/metadata/utils/fields.unit.spec.ts
+++ b/frontend/src/metabase-lib/metadata/utils/fields.unit.spec.ts
@@ -1,62 +1,90 @@
-import { createMockConcreteField, createMockVirtualField } from "../mocks";
+import { FieldReference } from "metabase-types/api";
+import { createMockField, createMockTable } from "metabase-types/api/mocks";
+import { createMockMetadata } from "__support__/metadata";
import { getUniqueFieldId } from "./fields";
-const structuredVirtualCardField = createMockConcreteField({
- apiOpts: {
- id: 1,
- table_id: "card__123",
- name: "foo",
- },
+const DB_TABLE_ID = 1;
+const CARD_TABLE_ID = "card__1";
+
+const DB_FIELD_ID = 1;
+const CARD_FIELD_ID = 2;
+const NESTED_DB_FIELD_ID: FieldReference = ["field", 3, null];
+const NATIVE_CARD_FIELD_ID: FieldReference = [
+ "field",
+ "foo",
+ { "base-type": "type/Integer" },
+];
+
+const DB_FIELD = createMockField({
+ id: DB_FIELD_ID,
+ table_id: DB_TABLE_ID,
+ name: "foo",
+ display_name: "foo",
+});
+
+const NESTED_DB_FIELD = createMockField({
+ id: NESTED_DB_FIELD_ID,
});
-const nativeVirtualCardField = createMockConcreteField({
- apiOpts: {
- id: undefined,
- table_id: "card__123",
- name: "foo",
- },
+const CARD_FIELD = createMockField({
+ id: CARD_FIELD_ID,
+ table_id: CARD_TABLE_ID,
+ name: "foo",
+ display_name: "foo",
});
-const concreteTableField = createMockConcreteField({
- apiOpts: {
- id: 1,
- table_id: 123,
- name: "foo",
- },
+const NATIVE_CARD_FIELD = createMockField({
+ id: NATIVE_CARD_FIELD_ID,
+ table_id: CARD_TABLE_ID,
+ name: "foo",
+ display_name: "foo",
});
-const fieldWithFieldRefId = createMockVirtualField({
- constructorOpts: {
- id: ["field", 1, null],
- table_id: 123,
- },
+const DB_TABLE = createMockTable({
+ id: DB_TABLE_ID,
+ fields: [DB_FIELD],
});
+const CARD_TABLE = createMockTable({
+ id: CARD_TABLE_ID,
+ fields: [CARD_FIELD, NESTED_DB_FIELD, NATIVE_CARD_FIELD],
+});
+
+const setup = () => {
+ return createMockMetadata({ tables: [DB_TABLE, CARD_TABLE] });
+};
+
describe("metabase-lib/metadata/utils", () => {
describe("getUniqueFieldId", () => {
describe("when the given field is from a concrete table", () => {
it("should return the field's id", () => {
- expect(getUniqueFieldId(concreteTableField)).toEqual(1);
+ const metadata = setup();
+ const field = metadata.field(DB_FIELD_ID);
+ expect(field && getUniqueFieldId(field)).toEqual(DB_FIELD_ID);
});
});
describe("when the given field is from a structured virtual card table", () => {
it("should return a combination of the field's id and table_id", () => {
- expect(getUniqueFieldId(structuredVirtualCardField)).toBe(
- "card__123:1",
- );
+ const metadata = setup();
+ const field = metadata.field(CARD_FIELD_ID, CARD_TABLE_ID);
+ expect(field && getUniqueFieldId(field)).toBe("card__1:2");
});
});
describe("when the given field is from a native virtual card table", () => {
it("should return a combination of the field's name and table_id", () => {
- expect(getUniqueFieldId(nativeVirtualCardField)).toBe("card__123:foo");
+ const metadata = setup();
+ const field = metadata.field(NATIVE_CARD_FIELD_ID, CARD_TABLE_ID);
+ expect(field && getUniqueFieldId(field)).toBe("card__1:foo");
});
});
describe("when the given field has a field ref id", () => {
it("should return the field ref id", () => {
- expect(getUniqueFieldId(fieldWithFieldRefId)).toBe(1);
+ const metadata = setup();
+ const field = metadata.field(NESTED_DB_FIELD_ID);
+ expect(field && getUniqueFieldId(field)).toBe(NESTED_DB_FIELD_ID[1]);
});
});
});
diff --git a/frontend/src/metabase-lib/metadata/utils/models.ts b/frontend/src/metabase-lib/metadata/utils/models.ts
index e7bd5b572c0..102dceb6eef 100644
--- a/frontend/src/metabase-lib/metadata/utils/models.ts
+++ b/frontend/src/metabase-lib/metadata/utils/models.ts
@@ -6,6 +6,7 @@ import {
TableColumnOrderSetting,
TemplateTag,
StructuredDatasetQuery,
+ FieldId,
} from "metabase-types/api";
import { getQuestionVirtualTableId } from "metabase-lib/metadata/utils/saved-questions";
import Database from "metabase-lib/metadata/Database";
@@ -15,7 +16,7 @@ import { isSameField } from "metabase-lib/queries/utils/field-ref";
import { isStructured } from "metabase-lib/queries/utils";
type FieldMetadata = {
- id?: number | string;
+ id?: FieldId | FieldReference;
name: string;
display_name: string;
description?: string | null;
diff --git a/frontend/src/metabase-types/api/field.ts b/frontend/src/metabase-types/api/field.ts
index c67e159824f..c3b5a185515 100644
--- a/frontend/src/metabase-types/api/field.ts
+++ b/frontend/src/metabase-types/api/field.ts
@@ -1,4 +1,5 @@
import { RowValue } from "./dataset";
+import { FieldReference } from "./query";
import { Table, TableId } from "./table";
export type FieldId = number;
@@ -65,8 +66,8 @@ export type FieldDimensionOption = {
type: string;
};
-export interface ConcreteField {
- id: FieldId;
+export interface Field {
+ id: FieldId | FieldReference;
table_id: TableId;
table?: Table;
@@ -118,10 +119,6 @@ export interface FieldValues {
has_more_values: boolean;
}
-export type Field = Omit<ConcreteField, "id"> & {
- id?: FieldId;
-};
-
export interface FieldFormattingSettings {
currency?: string;
}
diff --git a/frontend/src/metabase/models/containers/ModelDetailPage/ModelDetailPage.unit.spec.tsx b/frontend/src/metabase/models/containers/ModelDetailPage/ModelDetailPage.unit.spec.tsx
index 52f0937330b..2ebf5dcd9fe 100644
--- a/frontend/src/metabase/models/containers/ModelDetailPage/ModelDetailPage.unit.spec.tsx
+++ b/frontend/src/metabase/models/containers/ModelDetailPage/ModelDetailPage.unit.spec.tsx
@@ -80,8 +80,9 @@ const TEST_FIELD = createMockField({
});
const TEST_FK_TABLE_1_ID = 2;
+const TEST_FK_FIELD_ID = 4;
const TEST_FK_FIELD = createMockField({
- id: 4,
+ id: TEST_FK_FIELD_ID,
table_id: TEST_FK_TABLE_1_ID,
});
@@ -97,7 +98,7 @@ const TEST_FIELDS = [
display_name: "Field 3",
table_id: TEST_TABLE_ID,
semantic_type: TYPE.FK,
- fk_target_field_id: TEST_FK_FIELD.id,
+ fk_target_field_id: TEST_FK_FIELD_ID,
target: TEST_FK_FIELD,
}),
];
diff --git a/frontend/src/metabase/parameters/utils/formatting.unit.spec.ts b/frontend/src/metabase/parameters/utils/formatting.unit.spec.ts
index 67af9bb52a3..1bb52ac0f64 100644
--- a/frontend/src/metabase/parameters/utils/formatting.unit.spec.ts
+++ b/frontend/src/metabase/parameters/utils/formatting.unit.spec.ts
@@ -1,28 +1,31 @@
import { createMockMetadata } from "__support__/metadata";
import { checkNotNull } from "metabase/core/utils/types";
+import { createMockField } from "metabase-types/api/mocks";
import {
createSampleDatabase,
PRODUCTS,
ORDERS,
} from "metabase-types/api/mocks/presets";
-
import { createMockUiParameter } from "metabase-lib/parameters/mock";
-import Field from "metabase-lib/metadata/Field";
import { formatParameterValue } from "./formatting";
+const REMAPPED_FIELD_ID = 100;
+
const metadata = createMockMetadata({
databases: [createSampleDatabase()],
+ fields: [
+ createMockField({
+ id: REMAPPED_FIELD_ID,
+ base_type: "type/Text",
+ remappings: [[123456789, "A"]],
+ }),
+ ],
});
const numberField = checkNotNull(metadata.field(ORDERS.TOTAL));
const textField = checkNotNull(metadata.field(PRODUCTS.TITLE));
const categoryField = checkNotNull(metadata.field(PRODUCTS.CATEGORY));
-
-const remappedField = new Field({
- base_type: "type/Text",
- human_readable_field_id: numberField.id,
- remapping: new Map([[123456789, 0]]),
-});
+const remappedField = checkNotNull(metadata.field(REMAPPED_FIELD_ID));
describe("metabase/parameters/utils/formatting", () => {
describe("formatParameterValue", () => {
@@ -129,7 +132,7 @@ describe("metabase/parameters/utils/formatting", () => {
type: "number/=",
fields: [remappedField],
});
- expect(formatParameterValue(123456789, parameter)).toEqual(0);
+ expect(formatParameterValue(123456789, parameter)).toEqual("A");
});
});
});
diff --git a/frontend/test/metabase-lib/lib/Dimension.unit.spec.js b/frontend/test/metabase-lib/lib/Dimension.unit.spec.js
index 74ff2ec4c83..24650a42c2d 100644
--- a/frontend/test/metabase-lib/lib/Dimension.unit.spec.js
+++ b/frontend/test/metabase-lib/lib/Dimension.unit.spec.js
@@ -942,14 +942,10 @@ describe("Dimension", () => {
describe("field", () => {
it("should return the cached Field instance", () => {
const category = metadata.field(PRODUCTS.CATEGORY);
- const fieldFromEndpoint = new Field({
- ...category.getPlainObject(),
- _comesFromEndpoint: true,
- });
- const fieldDimension = fieldFromEndpoint.dimension();
- expect(fieldDimension._fieldInstance).toBe(fieldFromEndpoint);
- expect(fieldDimension.field()).toBe(fieldFromEndpoint);
+ const fieldDimension = category.dimension();
+ expect(fieldDimension._fieldInstance).toBe(category);
+ expect(fieldDimension.field()).toBe(category);
});
});
});
--
GitLab