From c5476ce3b3e53317de8044d85a3901e4834f395b Mon Sep 17 00:00:00 2001 From: Adria Navarro Date: Wed, 21 Aug 2024 12:14:40 +0200 Subject: [PATCH 1/7] Test both allOr and allAnd --- .../src/api/routes/tests/viewV2.spec.ts | 139 ++++++++++-------- 1 file changed, 74 insertions(+), 65 deletions(-) diff --git a/packages/server/src/api/routes/tests/viewV2.spec.ts b/packages/server/src/api/routes/tests/viewV2.spec.ts index da38112e2b9..41f4cebf17f 100644 --- a/packages/server/src/api/routes/tests/viewV2.spec.ts +++ b/packages/server/src/api/routes/tests/viewV2.spec.ts @@ -1493,82 +1493,90 @@ describe.each([ }) isLucene && - it("in lucene, cannot override a view filter", async () => { - await config.api.row.save(table._id!, { - one: "foo", - two: "bar", - }) - const two = await config.api.row.save(table._id!, { - one: "foo2", - two: "bar2", - }) + it.each([true, false])( + "in lucene, cannot override a view filter", + async allOr => { + await config.api.row.save(table._id!, { + one: "foo", + two: "bar", + }) + const two = await config.api.row.save(table._id!, { + one: "foo2", + two: "bar2", + }) - const view = await config.api.viewV2.create({ - tableId: table._id!, - name: generator.guid(), - query: [ - { - operator: BasicOperator.EQUAL, - field: "two", - value: "bar2", + const view = await config.api.viewV2.create({ + tableId: table._id!, + name: generator.guid(), + query: [ + { + operator: BasicOperator.EQUAL, + field: "two", + value: "bar2", + }, + ], + schema: { + id: { visible: true }, + one: { visible: false }, + two: { visible: true }, }, - ], - schema: { - id: { visible: true }, - one: { visible: false }, - two: { visible: true }, - }, - }) + }) - const response = await config.api.viewV2.search(view.id, { - query: { - equal: { - two: "bar", + const response = await config.api.viewV2.search(view.id, { + query: { + allOr, + equal: { + two: "bar", + }, }, - }, - }) - expect(response.rows).toHaveLength(1) - expect(response.rows).toEqual([ - expect.objectContaining({ _id: two._id }), - ]) - }) + }) + expect(response.rows).toHaveLength(1) + expect(response.rows).toEqual([ + expect.objectContaining({ _id: two._id }), + ]) + } + ) !isLucene && - it("can filter a view without a view filter", async () => { - const one = await config.api.row.save(table._id!, { - one: "foo", - two: "bar", - }) - await config.api.row.save(table._id!, { - one: "foo2", - two: "bar2", - }) + it.each([true, false])( + "can filter a view without a view filter", + async allOr => { + const one = await config.api.row.save(table._id!, { + one: "foo", + two: "bar", + }) + await config.api.row.save(table._id!, { + one: "foo2", + two: "bar2", + }) - const view = await config.api.viewV2.create({ - tableId: table._id!, - name: generator.guid(), - schema: { - id: { visible: true }, - one: { visible: false }, - two: { visible: true }, - }, - }) + const view = await config.api.viewV2.create({ + tableId: table._id!, + name: generator.guid(), + schema: { + id: { visible: true }, + one: { visible: false }, + two: { visible: true }, + }, + }) - const response = await config.api.viewV2.search(view.id, { - query: { - equal: { - two: "bar", + const response = await config.api.viewV2.search(view.id, { + query: { + allOr, + equal: { + two: "bar", + }, }, - }, - }) - expect(response.rows).toHaveLength(1) - expect(response.rows).toEqual([ - expect.objectContaining({ _id: one._id }), - ]) - }) + }) + expect(response.rows).toHaveLength(1) + expect(response.rows).toEqual([ + expect.objectContaining({ _id: one._id }), + ]) + } + ) !isLucene && - it("cannot bypass a view filter", async () => { + it.each([true, false])("cannot bypass a view filter", async allOr => { await config.api.row.save(table._id!, { one: "foo", two: "bar", @@ -1597,6 +1605,7 @@ describe.each([ const response = await config.api.viewV2.search(view.id, { query: { + allOr, equal: { two: "bar", }, From ff9d934f87b4fe70988fbc1a6069d87d18ea27a0 Mon Sep 17 00:00:00 2001 From: Adria Navarro Date: Wed, 21 Aug 2024 12:58:39 +0200 Subject: [PATCH 2/7] Allow filtering via allOr --- packages/server/src/api/controllers/row/views.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/server/src/api/controllers/row/views.ts b/packages/server/src/api/controllers/row/views.ts index c38a415aa20..b9ebf3cc71b 100644 --- a/packages/server/src/api/controllers/row/views.ts +++ b/packages/server/src/api/controllers/row/views.ts @@ -38,7 +38,6 @@ export async function searchView( let query = dataFilters.buildQuery(view.query || []) if (body.query) { // Delete extraneous search params that cannot be overridden - delete body.query.allOr delete body.query.onEmptyFilter if (!isExternalTableID(view.tableId) && !db.isSqsEnabledForTenant()) { From e536ec50935972269b6615c47cbf2175fac15ca5 Mon Sep 17 00:00:00 2001 From: Adria Navarro Date: Wed, 21 Aug 2024 12:58:46 +0200 Subject: [PATCH 3/7] Fix conditions --- packages/backend-core/src/sql/sql.ts | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 deletions(-) diff --git a/packages/backend-core/src/sql/sql.ts b/packages/backend-core/src/sql/sql.ts index ebae09e156a..21baa5adcc4 100644 --- a/packages/backend-core/src/sql/sql.ts +++ b/packages/backend-core/src/sql/sql.ts @@ -465,20 +465,16 @@ class InternalBuilder { if (filters.$and) { const { $and } = filters - query = query.where(x => { - for (const condition of $and.conditions) { - x = this.addFilters(x, condition, opts) - } - }) + for (const condition of $and.conditions) { + query = query.andWhere(b => this.addFilters(b, condition, opts)) + } } if (filters.$or) { const { $or } = filters - query = query.where(x => { - for (const condition of $or.conditions) { - x = this.addFilters(x, { ...condition, allOr: true }, opts) - } - }) + for (const condition of $or.conditions) { + query = query.orWhere(b => this.addFilters(b, condition, opts)) + } } if (filters.oneOf) { From 9f05804c67d30e156dd225ec4f27d71d069907c5 Mon Sep 17 00:00:00 2001 From: Adria Navarro Date: Wed, 21 Aug 2024 12:59:28 +0200 Subject: [PATCH 4/7] Add extra tests --- .../src/api/routes/tests/viewV2.spec.ts | 102 ++++++++++++++++++ 1 file changed, 102 insertions(+) diff --git a/packages/server/src/api/routes/tests/viewV2.spec.ts b/packages/server/src/api/routes/tests/viewV2.spec.ts index 41f4cebf17f..ea7ebed0b62 100644 --- a/packages/server/src/api/routes/tests/viewV2.spec.ts +++ b/packages/server/src/api/routes/tests/viewV2.spec.ts @@ -1492,6 +1492,108 @@ describe.each([ ) }) + !isLucene && + it("can query on top of the view filters", async () => { + await config.api.row.save(table._id!, { + one: "foo", + two: "bar", + }) + await config.api.row.save(table._id!, { + one: "foo2", + two: "bar2", + }) + const three = await config.api.row.save(table._id!, { + one: "foo3", + two: "bar3", + }) + + const view = await config.api.viewV2.create({ + tableId: table._id!, + name: generator.guid(), + query: [ + { + operator: BasicOperator.NOT_EQUAL, + field: "two", + value: "bar2", + }, + ], + schema: { + id: { visible: true }, + one: { visible: false }, + two: { visible: true }, + }, + }) + + const response = await config.api.viewV2.search(view.id, { + query: { + [BasicOperator.NOT_EQUAL]: { + two: "bar", + }, + [BasicOperator.NOT_EMPTY]: { + two: null, + }, + }, + }) + expect(response.rows).toHaveLength(1) + expect(response).toEqual({ + rows: expect.arrayContaining([ + expect.objectContaining({ _id: three._id }), + ]), + }) + }) + + !isLucene && + it("can query on top of the view filters (using or filters)", async () => { + const one = await config.api.row.save(table._id!, { + one: "foo", + two: "bar", + }) + await config.api.row.save(table._id!, { + one: "foo2", + two: "bar2", + }) + const three = await config.api.row.save(table._id!, { + one: "foo3", + two: "bar3", + }) + + const view = await config.api.viewV2.create({ + tableId: table._id!, + name: generator.guid(), + query: [ + { + operator: BasicOperator.NOT_EQUAL, + field: "two", + value: "bar2", + }, + ], + schema: { + id: { visible: true }, + one: { visible: false }, + two: { visible: true }, + }, + }) + + const response = await config.api.viewV2.search(view.id, { + query: { + allOr: true, + [BasicOperator.NOT_EQUAL]: { + two: "bar", + }, + [BasicOperator.NOT_EMPTY]: { + two: null, + }, + }, + }) + expect(response.rows).toHaveLength(2) + expect(response).toEqual({ + rows: expect.arrayContaining([ + expect.objectContaining({ _id: one._id }), + expect.objectContaining({ _id: three._id }), + ]), + }) + }) + isLucene && it.each([true, false])( "in lucene, cannot override a view filter", From 8c3f1c39c8189dee23524687861b98cc7d23ceae Mon Sep 17 00:00:00 2001 From: Adria Navarro Date: Wed, 21 Aug 2024 13:33:02 +0200 Subject: [PATCH 5/7] Fix test expect --- .../server/src/api/routes/tests/viewV2.spec.ts | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/packages/server/src/api/routes/tests/viewV2.spec.ts b/packages/server/src/api/routes/tests/viewV2.spec.ts index ea7ebed0b62..149369785ca 100644 --- a/packages/server/src/api/routes/tests/viewV2.spec.ts +++ b/packages/server/src/api/routes/tests/viewV2.spec.ts @@ -1535,11 +1535,11 @@ describe.each([ }, }) expect(response.rows).toHaveLength(1) - expect(response).toEqual({ - rows: expect.arrayContaining([ + expect(response.rows).toEqual( + expect.arrayContaining([ expect.objectContaining({ _id: three._id }), - ]), - }) + ]) + ) }) !isLucene && @@ -1586,12 +1586,12 @@ describe.each([ }, }) expect(response.rows).toHaveLength(2) - expect(response).toEqual({ - rows: expect.arrayContaining([ + expect(response.rows).toEqual( + expect.arrayContaining([ expect.objectContaining({ _id: one._id }), expect.objectContaining({ _id: three._id }), - ]), - }) + ]) + ) }) isLucene && From 7c06f1a40750fd866df3a4c888d7405171bb060b Mon Sep 17 00:00:00 2001 From: Adria Navarro Date: Wed, 21 Aug 2024 13:40:17 +0200 Subject: [PATCH 6/7] Run tests for lucene as well --- .../src/api/routes/tests/viewV2.spec.ts | 176 +++++++++--------- 1 file changed, 86 insertions(+), 90 deletions(-) diff --git a/packages/server/src/api/routes/tests/viewV2.spec.ts b/packages/server/src/api/routes/tests/viewV2.spec.ts index 149369785ca..4401efc480d 100644 --- a/packages/server/src/api/routes/tests/viewV2.spec.ts +++ b/packages/server/src/api/routes/tests/viewV2.spec.ts @@ -1492,107 +1492,103 @@ describe.each([ ) }) - !isLucene && - it("can query on top of the view filters", async () => { - await config.api.row.save(table._id!, { - one: "foo", - two: "bar", - }) - await config.api.row.save(table._id!, { - one: "foo2", - two: "bar2", - }) - const three = await config.api.row.save(table._id!, { - one: "foo3", - two: "bar3", - }) + it("can query on top of the view filters", async () => { + await config.api.row.save(table._id!, { + one: "foo", + two: "bar", + }) + await config.api.row.save(table._id!, { + one: "foo2", + two: "bar2", + }) + const three = await config.api.row.save(table._id!, { + one: "foo3", + two: "bar3", + }) - const view = await config.api.viewV2.create({ - tableId: table._id!, - name: generator.guid(), - query: [ - { - operator: BasicOperator.NOT_EQUAL, - field: "two", - value: "bar2", - }, - ], - schema: { - id: { visible: true }, - one: { visible: false }, - two: { visible: true }, + const view = await config.api.viewV2.create({ + tableId: table._id!, + name: generator.guid(), + query: [ + { + operator: BasicOperator.NOT_EQUAL, + field: "one", + value: "foo2", }, - }) + ], + schema: { + id: { visible: true }, + one: { visible: true }, + two: { visible: true }, + }, + }) - const response = await config.api.viewV2.search(view.id, { - query: { - [BasicOperator.NOT_EQUAL]: { - two: "bar", - }, - [BasicOperator.NOT_EMPTY]: { - two: null, - }, + const response = await config.api.viewV2.search(view.id, { + query: { + [BasicOperator.EQUAL]: { + two: "bar3", }, - }) - expect(response.rows).toHaveLength(1) - expect(response.rows).toEqual( - expect.arrayContaining([ - expect.objectContaining({ _id: three._id }), - ]) - ) + [BasicOperator.NOT_EMPTY]: { + two: null, + }, + }, }) + expect(response.rows).toHaveLength(1) + expect(response.rows).toEqual( + expect.arrayContaining([expect.objectContaining({ _id: three._id })]) + ) + }) - !isLucene && - it("can query on top of the view filters (using or filters)", async () => { - const one = await config.api.row.save(table._id!, { - one: "foo", - two: "bar", - }) - await config.api.row.save(table._id!, { - one: "foo2", - two: "bar2", - }) - const three = await config.api.row.save(table._id!, { - one: "foo3", - two: "bar3", - }) + it("can query on top of the view filters (using or filters)", async () => { + const one = await config.api.row.save(table._id!, { + one: "foo", + two: "bar", + }) + await config.api.row.save(table._id!, { + one: "foo2", + two: "bar2", + }) + const three = await config.api.row.save(table._id!, { + one: "foo3", + two: "bar3", + }) - const view = await config.api.viewV2.create({ - tableId: table._id!, - name: generator.guid(), - query: [ - { - operator: BasicOperator.NOT_EQUAL, - field: "two", - value: "bar2", - }, - ], - schema: { - id: { visible: true }, - one: { visible: false }, - two: { visible: true }, + const view = await config.api.viewV2.create({ + tableId: table._id!, + name: generator.guid(), + query: [ + { + operator: BasicOperator.NOT_EQUAL, + field: "two", + value: "bar2", }, - }) + ], + schema: { + id: { visible: true }, + one: { visible: false }, + two: { visible: true }, + }, + }) - const response = await config.api.viewV2.search(view.id, { - query: { - allOr: true, - [BasicOperator.NOT_EQUAL]: { - two: "bar", - }, - [BasicOperator.NOT_EMPTY]: { - two: null, - }, + const response = await config.api.viewV2.search(view.id, { + query: { + allOr: true, + [BasicOperator.NOT_EQUAL]: { + two: "bar", }, - }) - expect(response.rows).toHaveLength(2) - expect(response.rows).toEqual( - expect.arrayContaining([ - expect.objectContaining({ _id: one._id }), - expect.objectContaining({ _id: three._id }), - ]) - ) + [BasicOperator.NOT_EMPTY]: { + two: null, + }, + }, }) + expect(response.rows).toHaveLength(2) + expect(response.rows).toEqual( + expect.arrayContaining([ + expect.objectContaining({ _id: one._id }), + expect.objectContaining({ _id: three._id }), + ]) + ) + }) isLucene && it.each([true, false])( From dc5a7dbc621d92d9862c6a7c19860de1205a1777 Mon Sep 17 00:00:00 2001 From: Adria Navarro Date: Wed, 21 Aug 2024 14:25:48 +0200 Subject: [PATCH 7/7] Fixes --- packages/backend-core/src/sql/sql.ts | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/packages/backend-core/src/sql/sql.ts b/packages/backend-core/src/sql/sql.ts index 21baa5adcc4..a837ce61efc 100644 --- a/packages/backend-core/src/sql/sql.ts +++ b/packages/backend-core/src/sql/sql.ts @@ -337,7 +337,7 @@ class InternalBuilder { if (!filters) { return query } - filters = this.parseFilters(filters) + filters = this.parseFilters({ ...filters }) const aliases = this.query.tableAliases // if all or specified in filters, then everything is an or const allOr = filters.allOr @@ -466,15 +466,21 @@ class InternalBuilder { if (filters.$and) { const { $and } = filters for (const condition of $and.conditions) { - query = query.andWhere(b => this.addFilters(b, condition, opts)) + query = query.where(b => { + this.addFilters(b, condition, opts) + }) } } if (filters.$or) { const { $or } = filters - for (const condition of $or.conditions) { - query = query.orWhere(b => this.addFilters(b, condition, opts)) - } + query = query.where(b => { + for (const condition of $or.conditions) { + b.orWhere(c => + this.addFilters(c, { ...condition, allOr: true }, opts) + ) + } + }) } if (filters.oneOf) {