From 76d0107d4da9ae180b3b9f030330cbe82a0d3a57 Mon Sep 17 00:00:00 2001 From: Adria Navarro Date: Tue, 15 Oct 2024 10:10:15 +0200 Subject: [PATCH 1/8] Handle empty relationships --- packages/backend-core/src/sql/sql.ts | 81 +++++++++++++++++++++------- 1 file changed, 61 insertions(+), 20 deletions(-) diff --git a/packages/backend-core/src/sql/sql.ts b/packages/backend-core/src/sql/sql.ts index b415a6f1b7..b35b2b5ec8 100644 --- a/packages/backend-core/src/sql/sql.ts +++ b/packages/backend-core/src/sql/sql.ts @@ -23,12 +23,14 @@ import { InternalSearchFilterOperator, JsonFieldMetadata, JsonTypes, + LogicalOperator, Operation, prefixed, QueryJson, QueryOptions, RangeOperator, RelationshipsJson, + SearchFilterKey, SearchFilters, SortOrder, SqlClient, @@ -96,6 +98,22 @@ function isSqs(table: Table): boolean { ) } +const allowEmptyRelationships: Record = { + [BasicOperator.EQUAL]: false, + [BasicOperator.NOT_EQUAL]: true, + [BasicOperator.EMPTY]: false, + [BasicOperator.NOT_EMPTY]: true, + [BasicOperator.FUZZY]: false, + [BasicOperator.STRING]: false, + [RangeOperator.RANGE]: false, + [ArrayOperator.CONTAINS]: false, + [ArrayOperator.NOT_CONTAINS]: true, + [ArrayOperator.CONTAINS_ANY]: false, + [ArrayOperator.ONE_OF]: false, + [LogicalOperator.AND]: false, + [LogicalOperator.OR]: false, +} + class InternalBuilder { private readonly client: SqlClient private readonly query: QueryJson @@ -405,6 +423,7 @@ class InternalBuilder { addRelationshipForFilter( query: Knex.QueryBuilder, + allowEmptyRelationships: boolean, filterKey: string, whereCb: (query: Knex.QueryBuilder) => Knex.QueryBuilder ): Knex.QueryBuilder { @@ -426,9 +445,10 @@ class InternalBuilder { relationship.to && relationship.tableName ) { - let subQuery = mainKnex + const joinTable = mainKnex .select(mainKnex.raw(1)) .from({ [toAlias]: relatedTableName }) + let subQuery = joinTable.clone() const manyToMany = validateManyToMany(relationship) if (manyToMany) { const throughAlias = @@ -440,7 +460,6 @@ class InternalBuilder { subQuery = subQuery // add a join through the junction table .innerJoin(throughTable, function () { - // @ts-ignore this.on( `${toAlias}.${manyToMany.toPrimary}`, "=", @@ -460,18 +479,33 @@ class InternalBuilder { if (this.client === SqlClient.SQL_LITE) { subQuery = this.addJoinFieldCheck(subQuery, manyToMany) } + + query = query.whereExists(whereCb(subQuery)) + if (allowEmptyRelationships) { + query = query.orWhereNotExists( + joinTable.clone().innerJoin(throughTable, function () { + this.on( + `${fromAlias}.${manyToMany.fromPrimary}`, + "=", + `${throughAlias}.${manyToMany.from}` + ) + }) + ) + } } else { + const foreignKey = `${fromAlias}.${relationship.from}` // "join" to the main table, making sure the ID matches that of the main subQuery = subQuery.where( `${toAlias}.${relationship.to}`, "=", - mainKnex.raw( - this.quotedIdentifier(`${fromAlias}.${relationship.from}`) - ) + mainKnex.raw(this.quotedIdentifier(foreignKey)) ) + + query = query.whereExists(whereCb(subQuery)) + if (allowEmptyRelationships) { + query = query.orWhereNull(foreignKey) + } } - query = query.whereExists(whereCb(subQuery)) - break } } return query @@ -502,6 +536,7 @@ class InternalBuilder { } function iterate( structure: AnySearchFilter, + operation: SearchFilterKey, fn: ( query: Knex.QueryBuilder, key: string, @@ -558,9 +593,14 @@ class InternalBuilder { if (allOr) { query = query.or } - query = builder.addRelationshipForFilter(query, updatedKey, q => { - return handleRelationship(q, updatedKey, value) - }) + query = builder.addRelationshipForFilter( + query, + allowEmptyRelationships[operation], + updatedKey, + q => { + return handleRelationship(q, updatedKey, value) + } + ) } } } @@ -592,7 +632,7 @@ class InternalBuilder { return `[${value.join(",")}]` } if (this.client === SqlClient.POSTGRES) { - iterate(mode, (q, key, value) => { + iterate(mode, ArrayOperator.CONTAINS, (q, key, value) => { const wrap = any ? "" : "'" const op = any ? "\\?| array" : "@>" const fieldNames = key.split(/\./g) @@ -610,7 +650,7 @@ class InternalBuilder { this.client === SqlClient.MARIADB ) { const jsonFnc = any ? "JSON_OVERLAPS" : "JSON_CONTAINS" - iterate(mode, (q, key, value) => { + iterate(mode, ArrayOperator.CONTAINS, (q, key, value) => { return q[rawFnc]( `${not}COALESCE(${jsonFnc}(${key}, '${stringifyArray( value @@ -619,7 +659,7 @@ class InternalBuilder { }) } else { const andOr = mode === filters?.containsAny ? " OR " : " AND " - iterate(mode, (q, key, value) => { + iterate(mode, ArrayOperator.CONTAINS, (q, key, value) => { let statement = "" const identifier = this.quotedIdentifier(key) for (let i in value) { @@ -673,6 +713,7 @@ class InternalBuilder { const fnc = allOr ? "orWhereIn" : "whereIn" iterate( filters.oneOf, + ArrayOperator.ONE_OF, (q, key: string, array) => { if (this.client === SqlClient.ORACLE) { key = this.convertClobs(key) @@ -697,7 +738,7 @@ class InternalBuilder { ) } if (filters.string) { - iterate(filters.string, (q, key, value) => { + iterate(filters.string, BasicOperator.STRING, (q, key, value) => { const fnc = allOr ? "orWhere" : "where" // postgres supports ilike, nothing else does if (this.client === SqlClient.POSTGRES) { @@ -712,10 +753,10 @@ class InternalBuilder { }) } if (filters.fuzzy) { - iterate(filters.fuzzy, like) + iterate(filters.fuzzy, BasicOperator.FUZZY, like) } if (filters.range) { - iterate(filters.range, (q, key, value) => { + iterate(filters.range, RangeOperator.RANGE, (q, key, value) => { const isEmptyObject = (val: any) => { return ( val && @@ -781,7 +822,7 @@ class InternalBuilder { }) } if (filters.equal) { - iterate(filters.equal, (q, key, value) => { + iterate(filters.equal, BasicOperator.EQUAL, (q, key, value) => { const fnc = allOr ? "orWhereRaw" : "whereRaw" if (this.client === SqlClient.MS_SQL) { return q[fnc]( @@ -801,7 +842,7 @@ class InternalBuilder { }) } if (filters.notEqual) { - iterate(filters.notEqual, (q, key, value) => { + iterate(filters.notEqual, BasicOperator.NOT_EQUAL, (q, key, value) => { const fnc = allOr ? "orWhereRaw" : "whereRaw" if (this.client === SqlClient.MS_SQL) { return q[fnc]( @@ -822,13 +863,13 @@ class InternalBuilder { }) } if (filters.empty) { - iterate(filters.empty, (q, key) => { + iterate(filters.empty, BasicOperator.EMPTY, (q, key) => { const fnc = allOr ? "orWhereNull" : "whereNull" return q[fnc](key) }) } if (filters.notEmpty) { - iterate(filters.notEmpty, (q, key) => { + iterate(filters.notEmpty, BasicOperator.NOT_EMPTY, (q, key) => { const fnc = allOr ? "orWhereNotNull" : "whereNotNull" return q[fnc](key) }) From 57da952f6944b5874fe20e4539ce33b5ac2c3055 Mon Sep 17 00:00:00 2001 From: Adria Navarro Date: Tue, 15 Oct 2024 11:34:35 +0200 Subject: [PATCH 2/8] Fix "parenthesis" --- packages/backend-core/src/sql/sql.ts | 30 +++++++++++++--------------- 1 file changed, 14 insertions(+), 16 deletions(-) diff --git a/packages/backend-core/src/sql/sql.ts b/packages/backend-core/src/sql/sql.ts index b35b2b5ec8..3e6032fc99 100644 --- a/packages/backend-core/src/sql/sql.ts +++ b/packages/backend-core/src/sql/sql.ts @@ -480,18 +480,20 @@ class InternalBuilder { subQuery = this.addJoinFieldCheck(subQuery, manyToMany) } - query = query.whereExists(whereCb(subQuery)) - if (allowEmptyRelationships) { - query = query.orWhereNotExists( - joinTable.clone().innerJoin(throughTable, function () { - this.on( - `${fromAlias}.${manyToMany.fromPrimary}`, - "=", - `${throughAlias}.${manyToMany.from}` - ) - }) - ) - } + query = query.where(q => { + q.whereExists(whereCb(subQuery)) + if (allowEmptyRelationships) { + q.orWhereNotExists( + joinTable.clone().innerJoin(throughTable, function () { + this.on( + `${fromAlias}.${manyToMany.fromPrimary}`, + "=", + `${throughAlias}.${manyToMany.from}` + ) + }) + ) + } + }) } else { const foreignKey = `${fromAlias}.${relationship.from}` // "join" to the main table, making sure the ID matches that of the main @@ -1265,12 +1267,10 @@ class InternalBuilder { }) : undefined if (!throughTable) { - // @ts-ignore query = query.leftJoin(toTableWithSchema, function () { for (let relationship of columns) { const from = relationship.from, to = relationship.to - // @ts-ignore this.orOn(`${fromAlias}.${from}`, "=", `${toAlias}.${to}`) } }) @@ -1281,7 +1281,6 @@ class InternalBuilder { for (let relationship of columns) { const fromPrimary = relationship.fromPrimary const from = relationship.from - // @ts-ignore this.orOn( `${fromAlias}.${fromPrimary}`, "=", @@ -1293,7 +1292,6 @@ class InternalBuilder { for (let relationship of columns) { const toPrimary = relationship.toPrimary const to = relationship.to - // @ts-ignore this.orOn(`${toAlias}.${toPrimary}`, `${throughAlias}.${to}`) } }) From 5782b20509e3f3ea209e692d15ce2446940d7bef Mon Sep 17 00:00:00 2001 From: Adria Navarro Date: Tue, 15 Oct 2024 11:36:29 +0200 Subject: [PATCH 3/8] Update test --- packages/server/src/api/routes/tests/search.spec.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/server/src/api/routes/tests/search.spec.ts b/packages/server/src/api/routes/tests/search.spec.ts index 3ab35c9294..23fb5a056f 100644 --- a/packages/server/src/api/routes/tests/search.spec.ts +++ b/packages/server/src/api/routes/tests/search.spec.ts @@ -2458,7 +2458,7 @@ describe.each([ }).toContainExactly([ { name: "foo", productCat: [{ _id: productCatRows[0]._id }] }, { name: "bar", productCat: [{ _id: productCatRows[1]._id }] }, - // { name: "baz", productCat: undefined }, // TODO + { name: "baz", productCat: undefined }, ]) }) @@ -2506,7 +2506,7 @@ describe.each([ }).toContainExactly([ { name: "foo", productCat: [{ _id: productCatRows[0]._id }] }, { name: "bar", productCat: [{ _id: productCatRows[1]._id }] }, - // { name: "baz", productCat: undefined }, // TODO + { name: "baz", productCat: undefined }, ]) } ) @@ -2530,7 +2530,7 @@ describe.each([ }).toContainExactly([ { name: "foo", productCat: [{ _id: productCatRows[0]._id }] }, { name: "bar", productCat: [{ _id: productCatRows[1]._id }] }, - // { name: "baz", productCat: undefined }, // TODO + { name: "baz", productCat: undefined }, ]) }) }) From 975945a23a0b9d30a2b448fc9af73ec75f1f56ae Mon Sep 17 00:00:00 2001 From: Adria Navarro Date: Tue, 15 Oct 2024 12:03:23 +0200 Subject: [PATCH 4/8] Improve tests --- .../src/api/routes/tests/search.spec.ts | 28 +++++++++++++++---- 1 file changed, 22 insertions(+), 6 deletions(-) diff --git a/packages/server/src/api/routes/tests/search.spec.ts b/packages/server/src/api/routes/tests/search.spec.ts index 23fb5a056f..798157b122 100644 --- a/packages/server/src/api/routes/tests/search.spec.ts +++ b/packages/server/src/api/routes/tests/search.spec.ts @@ -2278,12 +2278,16 @@ describe.each([ // It also can't work for in-memory searching because the related table name // isn't available. !isInMemory && - describe("relations", () => { + describe.each([ + RelationshipType.ONE_TO_MANY, + RelationshipType.MANY_TO_ONE, + RelationshipType.MANY_TO_MANY, + ])("relations", relationshipType => { let productCategoryTable: Table, productCatRows: Row[] beforeAll(async () => { const { relatedTable, tableId } = await basicRelationshipTables( - RelationshipType.ONE_TO_MANY + relationshipType ) tableOrViewId = tableId productCategoryTable = relatedTable @@ -2380,7 +2384,10 @@ describe.each([ ], }, }).toContainExactly([ - { name: "foo", productCat: [{ _id: productCatRows[0]._id }] }, + { + name: "foo", + productCat: [{ _id: productCatRows[0]._id }], + }, ]) } ) @@ -2480,7 +2487,10 @@ describe.each([ ], }, }).toContainExactly([ - { name: "foo", productCat: [{ _id: productCatRows[0]._id }] }, + { + name: "foo", + productCat: [{ _id: productCatRows[0]._id }], + }, ]) } ) @@ -2504,8 +2514,14 @@ describe.each([ ], }, }).toContainExactly([ - { name: "foo", productCat: [{ _id: productCatRows[0]._id }] }, - { name: "bar", productCat: [{ _id: productCatRows[1]._id }] }, + { + name: "foo", + productCat: [{ _id: productCatRows[0]._id }], + }, + { + name: "bar", + productCat: [{ _id: productCatRows[1]._id }], + }, { name: "baz", productCat: undefined }, ]) } From 86e5718705df0c3fb8c3eaa3aa5577435280eb6f Mon Sep 17 00:00:00 2001 From: Adria Navarro Date: Tue, 15 Oct 2024 17:40:03 +0200 Subject: [PATCH 5/8] Fix tests --- packages/server/src/integrations/tests/sqlAlias.spec.ts | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/packages/server/src/integrations/tests/sqlAlias.spec.ts b/packages/server/src/integrations/tests/sqlAlias.spec.ts index 890c8c4663..ddc84ae1ce 100644 --- a/packages/server/src/integrations/tests/sqlAlias.spec.ts +++ b/packages/server/src/integrations/tests/sqlAlias.spec.ts @@ -78,8 +78,7 @@ describe("Captures of real examples", () => { bindings: ["assembling", primaryLimit, relationshipLimit], sql: expect.stringContaining( multiline( - `where exists (select 1 from "tasks" as "b" inner join "products_tasks" as "c" on "b"."taskid" = "c"."taskid" where "c"."productid" = "a"."productid" - and (COALESCE("b"."taskname" = $1, FALSE))` + `where (exists (select 1 from "tasks" as "b" inner join "products_tasks" as "c" on "b"."taskid" = "c"."taskid" where "c"."productid" = "a"."productid" and (COALESCE("b"."taskname" = $1, FALSE)))` ) ), }) @@ -133,6 +132,8 @@ describe("Captures of real examples", () => { expect(query).toEqual({ bindings: [ + rangeValue.low, + rangeValue.high, rangeValue.low, rangeValue.high, equalValue, @@ -144,7 +145,7 @@ describe("Captures of real examples", () => { ], sql: expect.stringContaining( multiline( - `where exists (select 1 from "persons" as "c" where "c"."personid" = "a"."executorid" and ("c"."year" between $1 and $2))` + `where exists (select 1 from "persons" as "c" where "c"."personid" = "a"."executorid" and ("c"."year" between $1 and $2)) and exists (select 1 from "persons" as "c" where "c"."personid" = "a"."qaid" and ("c"."year" between $3 and $4)) and (exists (select 1 from "products" as "b" inner join "products_tasks" as "d" on "b"."productid" = "d"."productid" where "d"."taskid" = "a"."taskid" and (COALESCE("b"."productname" = $5, FALSE)))` ) ), }) From 3ea8e240e4716361850ddbb8c027d1fae968f531 Mon Sep 17 00:00:00 2001 From: Adria Navarro Date: Wed, 16 Oct 2024 14:05:48 +0200 Subject: [PATCH 6/8] Fix one-to-many --- packages/backend-core/src/sql/sql.ts | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/packages/backend-core/src/sql/sql.ts b/packages/backend-core/src/sql/sql.ts index b7b499b92c..662558415e 100644 --- a/packages/backend-core/src/sql/sql.ts +++ b/packages/backend-core/src/sql/sql.ts @@ -518,10 +518,12 @@ class InternalBuilder { mainKnex.raw(this.quotedIdentifier(foreignKey)) ) - query = query.whereExists(whereCb(updatedKey, subQuery)) - if (allowEmptyRelationships) { - query = query.orWhereNull(foreignKey) - } + query = query.where(q => { + q.whereExists(whereCb(updatedKey, subQuery)) + if (allowEmptyRelationships) { + q.orWhereNull(foreignKey) + } + }) } } } From 3fbe937bf43443647ce6709942bd32e3717045b2 Mon Sep 17 00:00:00 2001 From: Adria Navarro Date: Wed, 16 Oct 2024 14:23:58 +0200 Subject: [PATCH 7/8] Fix sqlalias test --- packages/server/src/integrations/tests/sqlAlias.spec.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/server/src/integrations/tests/sqlAlias.spec.ts b/packages/server/src/integrations/tests/sqlAlias.spec.ts index ddc84ae1ce..739d3a4aee 100644 --- a/packages/server/src/integrations/tests/sqlAlias.spec.ts +++ b/packages/server/src/integrations/tests/sqlAlias.spec.ts @@ -145,7 +145,7 @@ describe("Captures of real examples", () => { ], sql: expect.stringContaining( multiline( - `where exists (select 1 from "persons" as "c" where "c"."personid" = "a"."executorid" and ("c"."year" between $1 and $2)) and exists (select 1 from "persons" as "c" where "c"."personid" = "a"."qaid" and ("c"."year" between $3 and $4)) and (exists (select 1 from "products" as "b" inner join "products_tasks" as "d" on "b"."productid" = "d"."productid" where "d"."taskid" = "a"."taskid" and (COALESCE("b"."productname" = $5, FALSE)))` + `where (exists (select 1 from "persons" as "c" where "c"."personid" = "a"."executorid" and ("c"."year" between $1 and $2))) and (exists (select 1 from "persons" as "c" where "c"."personid" = "a"."qaid" and ("c"."year" between $3 and $4))) and (exists (select 1 from "products" as "b" inner join "products_tasks" as "d" on "b"."productid" = "d"."productid" where "d"."taskid" = "a"."taskid" and (COALESCE("b"."productname" = $5, FALSE))))` ) ), }) From b6874f52f6d8e5228b57842a9844eba28f72e9e6 Mon Sep 17 00:00:00 2001 From: Adria Navarro Date: Wed, 16 Oct 2024 16:16:39 +0200 Subject: [PATCH 8/8] Fix many-to-one --- packages/backend-core/src/sql/sql.ts | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/packages/backend-core/src/sql/sql.ts b/packages/backend-core/src/sql/sql.ts index 662558415e..949d7edf1b 100644 --- a/packages/backend-core/src/sql/sql.ts +++ b/packages/backend-core/src/sql/sql.ts @@ -510,18 +510,19 @@ class InternalBuilder { } }) } else { + const toKey = `${toAlias}.${relationship.to}` const foreignKey = `${fromAlias}.${relationship.from}` // "join" to the main table, making sure the ID matches that of the main subQuery = subQuery.where( - `${toAlias}.${relationship.to}`, + toKey, "=", mainKnex.raw(this.quotedIdentifier(foreignKey)) ) query = query.where(q => { - q.whereExists(whereCb(updatedKey, subQuery)) + q.whereExists(whereCb(updatedKey, subQuery.clone())) if (allowEmptyRelationships) { - q.orWhereNull(foreignKey) + q.orWhereNotExists(subQuery) } }) }