From 5435028e7daff6844988d1a1cb51b4968e35776f Mon Sep 17 00:00:00 2001 From: mike12345567 Date: Tue, 20 Aug 2024 12:13:37 +0100 Subject: [PATCH 1/8] Fix for cyclic relationships, getQueryableFields allowed relationships from other tables, which can't work. --- packages/server/src/sdk/app/rows/queryUtils.ts | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/packages/server/src/sdk/app/rows/queryUtils.ts b/packages/server/src/sdk/app/rows/queryUtils.ts index a5e78b2fb9..78cf64e184 100644 --- a/packages/server/src/sdk/app/rows/queryUtils.ts +++ b/packages/server/src/sdk/app/rows/queryUtils.ts @@ -59,14 +59,15 @@ export const getQueryableFields = async ( const extractTableFields = async ( table: Table, allowedFields: string[], - fromTables: string[] + fromTables: string[], + opts: { allowRelationships?: boolean } = { allowRelationships: true } ): Promise => { const result = [] for (const field of Object.keys(table.schema).filter( f => allowedFields.includes(f) && table.schema[f].visible !== false )) { const subSchema = table.schema[field] - if (subSchema.type === FieldType.LINK) { + if (opts.allowRelationships && subSchema.type === FieldType.LINK) { if (fromTables.includes(subSchema.tableId)) { // avoid circular loops continue @@ -76,7 +77,9 @@ export const getQueryableFields = async ( const relatedFields = await extractTableFields( relatedTable, Object.keys(relatedTable.schema), - [...fromTables, subSchema.tableId] + [...fromTables, subSchema.tableId], + // don't let it recurse back and forth between relationships + { allowRelationships: false } ) result.push( From 0d389bd8d79fcbde5f79c8fc3bb05e711e9f2b0e Mon Sep 17 00:00:00 2001 From: mike12345567 Date: Tue, 20 Aug 2024 12:31:46 +0100 Subject: [PATCH 2/8] Quick set of fixes, test cases assumed that multi-depth relationships were allowed, fixing this. --- .../server/src/sdk/app/rows/queryUtils.ts | 28 ++++--- .../src/sdk/app/rows/tests/queryUtils.spec.ts | 74 ++----------------- 2 files changed, 24 insertions(+), 78 deletions(-) diff --git a/packages/server/src/sdk/app/rows/queryUtils.ts b/packages/server/src/sdk/app/rows/queryUtils.ts index 78cf64e184..65f400a1d9 100644 --- a/packages/server/src/sdk/app/rows/queryUtils.ts +++ b/packages/server/src/sdk/app/rows/queryUtils.ts @@ -16,32 +16,32 @@ export const removeInvalidFilters = ( validFields = validFields.map(f => f.toLowerCase()) for (const filterKey of Object.keys(result) as (keyof SearchFilters)[]) { - if (typeof result[filterKey] !== "object") { + const filter = result[filterKey] + if (!filter || typeof filter !== "object") { continue } if (isLogicalSearchOperator(filterKey)) { const resultingConditions: SearchFilters[] = [] - for (const condition of result[filterKey].conditions) { + for (const condition of filter.conditions) { const resultingCondition = removeInvalidFilters(condition, validFields) if (Object.keys(resultingCondition).length) { resultingConditions.push(resultingCondition) } } if (resultingConditions.length) { - result[filterKey].conditions = resultingConditions + filter.conditions = resultingConditions } else { delete result[filterKey] } continue } - const filter = result[filterKey] for (const columnKey of Object.keys(filter)) { const possibleKeys = [columnKey, db.removeKeyNumbering(columnKey)].map( c => c.toLowerCase() ) if (!validFields.some(f => possibleKeys.includes(f.toLowerCase()))) { - delete filter[columnKey] + delete filter[columnKey as keyof typeof filter] } } if (!Object.keys(filter).length) { @@ -60,18 +60,22 @@ export const getQueryableFields = async ( table: Table, allowedFields: string[], fromTables: string[], - opts: { allowRelationships?: boolean } = { allowRelationships: true } + opts?: { noRelationships?: boolean } ): Promise => { const result = [] for (const field of Object.keys(table.schema).filter( f => allowedFields.includes(f) && table.schema[f].visible !== false )) { const subSchema = table.schema[field] - if (opts.allowRelationships && subSchema.type === FieldType.LINK) { - if (fromTables.includes(subSchema.tableId)) { - // avoid circular loops - continue - } + const isRelationship = subSchema.type === FieldType.LINK + // avoid relationship loops + if ( + isRelationship && + (opts?.noRelationships || fromTables.includes(subSchema.tableId)) + ) { + continue + } + if (isRelationship) { try { const relatedTable = await sdk.tables.getTable(subSchema.tableId) const relatedFields = await extractTableFields( @@ -79,7 +83,7 @@ export const getQueryableFields = async ( Object.keys(relatedTable.schema), [...fromTables, subSchema.tableId], // don't let it recurse back and forth between relationships - { allowRelationships: false } + { noRelationships: true } ) result.push( diff --git a/packages/server/src/sdk/app/rows/tests/queryUtils.spec.ts b/packages/server/src/sdk/app/rows/tests/queryUtils.spec.ts index c156b2d5ba..aabc359484 100644 --- a/packages/server/src/sdk/app/rows/tests/queryUtils.spec.ts +++ b/packages/server/src/sdk/app/rows/tests/queryUtils.spec.ts @@ -386,35 +386,13 @@ describe("query utils", () => { expect(result).toEqual([ "_id", "name", - // deep 1 aux1 primitive props + // aux1 primitive props "aux1.name", "aux1Table.name", - // deep 2 aux1 primitive props - "aux1.aux2_1.title", - "aux1Table.aux2_1.title", - "aux1.aux2Table.title", - "aux1Table.aux2Table.title", - - // deep 2 aux2 primitive props - "aux1.aux2_2.title", - "aux1Table.aux2_2.title", - "aux1.aux2Table.title", - "aux1Table.aux2Table.title", - - // deep 1 aux2 primitive props + // aux2 primitive props "aux2.title", "aux2Table.title", - - // deep 2 aux2 primitive props - "aux2.aux1_1.name", - "aux2Table.aux1_1.name", - "aux2.aux1Table.name", - "aux2Table.aux1Table.name", - "aux2.aux1_2.name", - "aux2Table.aux1_2.name", - "aux2.aux1Table.name", - "aux2Table.aux1Table.name", ]) }) @@ -426,35 +404,17 @@ describe("query utils", () => { "_id", "name", - // deep 1 aux2_1 primitive props + // aux2_1 primitive props "aux2_1.title", "aux2Table.title", - // deep 2 aux2_1 primitive props - "aux2_1.table.name", - "aux2Table.table.name", - "aux2_1.TestTable.name", - "aux2Table.TestTable.name", - - // deep 1 aux2_2 primitive props + // aux2_2 primitive props "aux2_2.title", "aux2Table.title", - // deep 2 aux2_2 primitive props - "aux2_2.table.name", - "aux2Table.table.name", - "aux2_2.TestTable.name", - "aux2Table.TestTable.name", - - // deep 1 table primitive props + // table primitive props "table.name", "TestTable.name", - - // deep 2 table primitive props - "table.aux2.title", - "TestTable.aux2.title", - "table.aux2Table.title", - "TestTable.aux2Table.title", ]) }) @@ -466,35 +426,17 @@ describe("query utils", () => { "_id", "title", - // deep 1 aux1_1 primitive props + // aux1_1 primitive props "aux1_1.name", "aux1Table.name", - // deep 2 aux1_1 primitive props - "aux1_1.table.name", - "aux1Table.table.name", - "aux1_1.TestTable.name", - "aux1Table.TestTable.name", - - // deep 1 aux1_2 primitive props + // aux1_2 primitive props "aux1_2.name", "aux1Table.name", - // deep 2 aux1_2 primitive props - "aux1_2.table.name", - "aux1Table.table.name", - "aux1_2.TestTable.name", - "aux1Table.TestTable.name", - - // deep 1 table primitive props + // table primitive props "table.name", "TestTable.name", - - // deep 2 table primitive props - "table.aux1.name", - "TestTable.aux1.name", - "table.aux1Table.name", - "TestTable.aux1Table.name", ]) }) }) From f2beedbee6d9c4eb1997820e68db10a7b0fba95f Mon Sep 17 00:00:00 2001 From: mike12345567 Date: Tue, 20 Aug 2024 13:14:24 +0100 Subject: [PATCH 3/8] Adding a test case for primary display columns, ignore when it has been set to a relationship, instead use another column which is valid in the table. --- .../src/api/routes/tests/search.spec.ts | 64 +++++++++++++++++-- packages/server/src/db/linkedRows/index.ts | 55 +++++++++++++--- 2 files changed, 102 insertions(+), 17 deletions(-) diff --git a/packages/server/src/api/routes/tests/search.spec.ts b/packages/server/src/api/routes/tests/search.spec.ts index 9de97747e5..3607fbf329 100644 --- a/packages/server/src/api/routes/tests/search.spec.ts +++ b/packages/server/src/api/routes/tests/search.spec.ts @@ -44,14 +44,14 @@ import { DEFAULT_EMPLOYEE_TABLE_SCHEMA } from "../../../db/defaultData/datasourc import { generateRowIdField } from "../../../integrations/utils" describe.each([ - ["in-memory", undefined], - ["lucene", undefined], + // ["in-memory", undefined], + // ["lucene", undefined], ["sqs", undefined], - [DatabaseName.POSTGRES, getDatasource(DatabaseName.POSTGRES)], - [DatabaseName.MYSQL, getDatasource(DatabaseName.MYSQL)], - [DatabaseName.SQL_SERVER, getDatasource(DatabaseName.SQL_SERVER)], - [DatabaseName.MARIADB, getDatasource(DatabaseName.MARIADB)], - [DatabaseName.ORACLE, getDatasource(DatabaseName.ORACLE)], + // [DatabaseName.POSTGRES, getDatasource(DatabaseName.POSTGRES)], + // [DatabaseName.MYSQL, getDatasource(DatabaseName.MYSQL)], + // [DatabaseName.SQL_SERVER, getDatasource(DatabaseName.SQL_SERVER)], + // [DatabaseName.MARIADB, getDatasource(DatabaseName.MARIADB)], + // [DatabaseName.ORACLE, getDatasource(DatabaseName.ORACLE)], ])("search (%s)", (name, dsProvider) => { const isSqs = name === "sqs" const isLucene = name === "lucene" @@ -2762,6 +2762,56 @@ describe.each([ }) }) + describe("primaryDisplay", () => { + beforeAll(async () => { + let toRelateTable = await createTable({ + name: { + name: "name", + type: FieldType.STRING, + }, + }) + table = await config.api.table.save( + tableForDatasource(datasource, { + schema: { + name: { + name: "name", + type: FieldType.STRING, + }, + link: { + name: "link", + type: FieldType.LINK, + relationshipType: RelationshipType.MANY_TO_ONE, + tableId: toRelateTable._id!, + fieldName: "link", + }, + }, + }) + ) + toRelateTable = await config.api.table.get(toRelateTable._id!) + await config.api.table.save({ + ...toRelateTable, + primaryDisplay: "link", + }) + const relatedRows = await Promise.all([ + config.api.row.save(toRelateTable._id!, { name: "test" }), + ]) + await Promise.all([ + config.api.row.save(table._id!, { + name: "test", + link: relatedRows.map(row => row._id), + }), + ]) + }) + + it("should be able to query, primary display on related table shouldn't be used", async () => { + // this test makes sure that if a relationship has been specified as the primary display on a table + // it is ignored and another column is used instead + await expectQuery({}).toContain([ + { name: "test", link: [{ primaryDisplay: "test" }] }, + ]) + }) + }) + !isLucene && describe("$and", () => { beforeAll(async () => { diff --git a/packages/server/src/db/linkedRows/index.ts b/packages/server/src/db/linkedRows/index.ts index 87f980600a..1b3fd4f809 100644 --- a/packages/server/src/db/linkedRows/index.ts +++ b/packages/server/src/db/linkedRows/index.ts @@ -1,10 +1,10 @@ import LinkController from "./LinkController" import { getLinkDocuments, - getUniqueByProp, - getRelatedTableForField, - getLinkedTableIDs, getLinkedTable, + getLinkedTableIDs, + getRelatedTableForField, + getUniqueByProp, } from "./linkUtils" import flatten from "lodash/flatten" import { USER_METDATA_PREFIX } from "../utils" @@ -13,16 +13,25 @@ import { getGlobalUsersFromMetadata } from "../../utilities/global" import { processFormulas } from "../../utilities/rowProcessor" import { context } from "@budibase/backend-core" import { - Table, - Row, - LinkDocumentValue, - FieldType, ContextUser, + FieldType, + LinkDocumentValue, + Row, + Table, } from "@budibase/types" import sdk from "../../sdk" export { IncludeDocs, getLinkDocuments, createLinkView } from "./linkUtils" +const INVALID_DISPLAY_COLUMN_TYPE = [ + FieldType.LINK, + FieldType.ATTACHMENTS, + FieldType.ATTACHMENT_SINGLE, + FieldType.SIGNATURE_SINGLE, + FieldType.BB_REFERENCE, + FieldType.BB_REFERENCE_SINGLE, +] + /** * This functionality makes sure that when rows with links are created, updated or deleted they are processed * correctly - making sure that no stale links are left around and that all links have been made successfully. @@ -206,6 +215,34 @@ export async function attachFullLinkedDocs( return rows } +/** + * Finds a valid value for the primary display, avoiding columns which break things + * like relationships (can be circular). + * @param row The row to lift a value from for the primary display. + * @param table The related table to attempt to work out the primary display column from. + */ +function getPrimaryDisplayValue(row: Row, table?: Table) { + const primaryDisplay = table?.primaryDisplay + let invalid = true + if (primaryDisplay) { + const primaryDisplaySchema = table?.schema[primaryDisplay] + invalid = + INVALID_DISPLAY_COLUMN_TYPE.includes(primaryDisplaySchema.type) && + row[primaryDisplay] + } + if (invalid || !primaryDisplay) { + const validKey = Object.keys(table?.schema || {}).filter( + key => + table?.schema[key].type && + !INVALID_DISPLAY_COLUMN_TYPE.includes(table?.schema[key].type) && + row[key] + ) + return validKey[0] ? row[validKey[0]] : undefined + } else { + return row[primaryDisplay] + } +} + /** * This function will take the given enriched rows and squash the links to only contain the primary display field. * @param table The table from which the rows originated. @@ -232,9 +269,7 @@ export async function squashLinksToPrimaryDisplay( const linkTblId = link.tableId || getRelatedTableForField(table, column) const linkedTable = await getLinkedTable(linkTblId!, linkedTables) const obj: any = { _id: link._id } - if (linkedTable?.primaryDisplay && link[linkedTable.primaryDisplay]) { - obj.primaryDisplay = link[linkedTable.primaryDisplay] - } + obj.primaryDisplay = getPrimaryDisplayValue(link, linkedTable) newLinks.push(obj) } row[column] = newLinks From aab120b9ca3c73da1a089a0d67e7ad54037f1a1e Mon Sep 17 00:00:00 2001 From: mike12345567 Date: Tue, 20 Aug 2024 13:18:40 +0100 Subject: [PATCH 4/8] Bringing back test cases. --- .../server/src/api/routes/tests/search.spec.ts | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/packages/server/src/api/routes/tests/search.spec.ts b/packages/server/src/api/routes/tests/search.spec.ts index 3607fbf329..be7b29bb86 100644 --- a/packages/server/src/api/routes/tests/search.spec.ts +++ b/packages/server/src/api/routes/tests/search.spec.ts @@ -44,14 +44,14 @@ import { DEFAULT_EMPLOYEE_TABLE_SCHEMA } from "../../../db/defaultData/datasourc import { generateRowIdField } from "../../../integrations/utils" describe.each([ - // ["in-memory", undefined], - // ["lucene", undefined], + ["in-memory", undefined], + ["lucene", undefined], ["sqs", undefined], - // [DatabaseName.POSTGRES, getDatasource(DatabaseName.POSTGRES)], - // [DatabaseName.MYSQL, getDatasource(DatabaseName.MYSQL)], - // [DatabaseName.SQL_SERVER, getDatasource(DatabaseName.SQL_SERVER)], - // [DatabaseName.MARIADB, getDatasource(DatabaseName.MARIADB)], - // [DatabaseName.ORACLE, getDatasource(DatabaseName.ORACLE)], + [DatabaseName.POSTGRES, getDatasource(DatabaseName.POSTGRES)], + [DatabaseName.MYSQL, getDatasource(DatabaseName.MYSQL)], + [DatabaseName.SQL_SERVER, getDatasource(DatabaseName.SQL_SERVER)], + [DatabaseName.MARIADB, getDatasource(DatabaseName.MARIADB)], + [DatabaseName.ORACLE, getDatasource(DatabaseName.ORACLE)], ])("search (%s)", (name, dsProvider) => { const isSqs = name === "sqs" const isLucene = name === "lucene" From 8847a5b1461ac93436d02a8a58001f42e15a3582 Mon Sep 17 00:00:00 2001 From: mike12345567 Date: Tue, 20 Aug 2024 13:19:58 +0100 Subject: [PATCH 5/8] Disabling for old/in-memory search. --- .../src/api/routes/tests/search.spec.ts | 89 ++++++++++--------- 1 file changed, 45 insertions(+), 44 deletions(-) diff --git a/packages/server/src/api/routes/tests/search.spec.ts b/packages/server/src/api/routes/tests/search.spec.ts index be7b29bb86..bac9b6f774 100644 --- a/packages/server/src/api/routes/tests/search.spec.ts +++ b/packages/server/src/api/routes/tests/search.spec.ts @@ -2762,55 +2762,56 @@ describe.each([ }) }) - describe("primaryDisplay", () => { - beforeAll(async () => { - let toRelateTable = await createTable({ - name: { - name: "name", - type: FieldType.STRING, - }, - }) - table = await config.api.table.save( - tableForDatasource(datasource, { - schema: { - name: { - name: "name", - type: FieldType.STRING, - }, - link: { - name: "link", - type: FieldType.LINK, - relationshipType: RelationshipType.MANY_TO_ONE, - tableId: toRelateTable._id!, - fieldName: "link", - }, + isSql && + describe("primaryDisplay", () => { + beforeAll(async () => { + let toRelateTable = await createTable({ + name: { + name: "name", + type: FieldType.STRING, }, }) - ) - toRelateTable = await config.api.table.get(toRelateTable._id!) - await config.api.table.save({ - ...toRelateTable, - primaryDisplay: "link", + table = await config.api.table.save( + tableForDatasource(datasource, { + schema: { + name: { + name: "name", + type: FieldType.STRING, + }, + link: { + name: "link", + type: FieldType.LINK, + relationshipType: RelationshipType.MANY_TO_ONE, + tableId: toRelateTable._id!, + fieldName: "link", + }, + }, + }) + ) + toRelateTable = await config.api.table.get(toRelateTable._id!) + await config.api.table.save({ + ...toRelateTable, + primaryDisplay: "link", + }) + const relatedRows = await Promise.all([ + config.api.row.save(toRelateTable._id!, { name: "test" }), + ]) + await Promise.all([ + config.api.row.save(table._id!, { + name: "test", + link: relatedRows.map(row => row._id), + }), + ]) }) - const relatedRows = await Promise.all([ - config.api.row.save(toRelateTable._id!, { name: "test" }), - ]) - await Promise.all([ - config.api.row.save(table._id!, { - name: "test", - link: relatedRows.map(row => row._id), - }), - ]) - }) - it("should be able to query, primary display on related table shouldn't be used", async () => { - // this test makes sure that if a relationship has been specified as the primary display on a table - // it is ignored and another column is used instead - await expectQuery({}).toContain([ - { name: "test", link: [{ primaryDisplay: "test" }] }, - ]) + it("should be able to query, primary display on related table shouldn't be used", async () => { + // this test makes sure that if a relationship has been specified as the primary display on a table + // it is ignored and another column is used instead + await expectQuery({}).toContain([ + { name: "test", link: [{ primaryDisplay: "test" }] }, + ]) + }) }) - }) !isLucene && describe("$and", () => { From e941491d8ce9019128ddf291978885c192de0aac Mon Sep 17 00:00:00 2001 From: mike12345567 Date: Tue, 20 Aug 2024 13:24:41 +0100 Subject: [PATCH 6/8] Don't check row value - always use the column or not. --- packages/server/src/db/linkedRows/index.ts | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/packages/server/src/db/linkedRows/index.ts b/packages/server/src/db/linkedRows/index.ts index 1b3fd4f809..037e53d02f 100644 --- a/packages/server/src/db/linkedRows/index.ts +++ b/packages/server/src/db/linkedRows/index.ts @@ -226,16 +226,13 @@ function getPrimaryDisplayValue(row: Row, table?: Table) { let invalid = true if (primaryDisplay) { const primaryDisplaySchema = table?.schema[primaryDisplay] - invalid = - INVALID_DISPLAY_COLUMN_TYPE.includes(primaryDisplaySchema.type) && - row[primaryDisplay] + invalid = INVALID_DISPLAY_COLUMN_TYPE.includes(primaryDisplaySchema.type) } if (invalid || !primaryDisplay) { const validKey = Object.keys(table?.schema || {}).filter( key => table?.schema[key].type && - !INVALID_DISPLAY_COLUMN_TYPE.includes(table?.schema[key].type) && - row[key] + !INVALID_DISPLAY_COLUMN_TYPE.includes(table?.schema[key].type) ) return validKey[0] ? row[validKey[0]] : undefined } else { From ff855a677a98aeabd5df955eb156e9fd12836358 Mon Sep 17 00:00:00 2001 From: mike12345567 Date: Tue, 20 Aug 2024 13:27:20 +0100 Subject: [PATCH 7/8] PR comments. --- packages/server/src/db/linkedRows/index.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/server/src/db/linkedRows/index.ts b/packages/server/src/db/linkedRows/index.ts index 037e53d02f..2da7e212b9 100644 --- a/packages/server/src/db/linkedRows/index.ts +++ b/packages/server/src/db/linkedRows/index.ts @@ -229,12 +229,12 @@ function getPrimaryDisplayValue(row: Row, table?: Table) { invalid = INVALID_DISPLAY_COLUMN_TYPE.includes(primaryDisplaySchema.type) } if (invalid || !primaryDisplay) { - const validKey = Object.keys(table?.schema || {}).filter( + const validKey = Object.keys(table?.schema || {}).find( key => table?.schema[key].type && !INVALID_DISPLAY_COLUMN_TYPE.includes(table?.schema[key].type) ) - return validKey[0] ? row[validKey[0]] : undefined + return validKey ? row[validKey] : undefined } else { return row[primaryDisplay] } From ae49b0309dd65997bdbd25879d57371338b45157 Mon Sep 17 00:00:00 2001 From: Budibase Staging Release Bot <> Date: Tue, 20 Aug 2024 12:37:48 +0000 Subject: [PATCH 8/8] Bump version to 2.30.8 --- lerna.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lerna.json b/lerna.json index d4ea1cc162..35ba5cedb1 100644 --- a/lerna.json +++ b/lerna.json @@ -1,6 +1,6 @@ { "$schema": "node_modules/lerna/schemas/lerna-schema.json", - "version": "2.30.7", + "version": "2.30.8", "npmClient": "yarn", "packages": [ "packages/*",