From 4c6f7f25c27192564b9be2c606b554cdd9add581 Mon Sep 17 00:00:00 2001 From: Sam Rose Date: Tue, 9 Jul 2024 11:45:01 +0100 Subject: [PATCH 1/8] Fix bug in oneOf search. --- .../src/api/routes/tests/search.spec.ts | 52 +++++++++++++++++++ packages/server/src/sdk/app/rows/search.ts | 27 +--------- packages/shared-core/src/filters.ts | 37 +++++++++++-- 3 files changed, 87 insertions(+), 29 deletions(-) diff --git a/packages/server/src/api/routes/tests/search.spec.ts b/packages/server/src/api/routes/tests/search.spec.ts index d9036b22fb..ab4c28a90d 100644 --- a/packages/server/src/api/routes/tests/search.spec.ts +++ b/packages/server/src/api/routes/tests/search.spec.ts @@ -780,6 +780,32 @@ describe.each([ it("fails to find nonexistent row", async () => { await expectQuery({ oneOf: { name: ["none"] } }).toFindNothing() }) + + it("can have multiple values for same column", async () => { + await expectQuery({ + oneOf: { + name: ["foo", "bar"], + }, + }).toContainExactly([{ name: "foo" }, { name: "bar" }]) + }) + + it("splits comma separated strings", async () => { + await expectQuery({ + oneOf: { + // @ts-ignore + name: "foo,bar", + }, + }).toContainExactly([{ name: "foo" }, { name: "bar" }]) + }) + + it("trims whitespace", async () => { + await expectQuery({ + oneOf: { + // @ts-ignore + name: "foo, bar", + }, + }).toContainExactly([{ name: "foo" }, { name: "bar" }]) + }) }) describe("fuzzy", () => { @@ -1002,6 +1028,32 @@ describe.each([ it("fails to find nonexistent row", async () => { await expectQuery({ oneOf: { age: [2] } }).toFindNothing() }) + + // I couldn't find a way to make this work in Lucene and given that + // we're getting rid of Lucene soon I wasn't inclined to spend time on + // it. + !isLucene && + it("can convert from a string", async () => { + await expectQuery({ + oneOf: { + // @ts-ignore + age: "1", + }, + }).toContainExactly([{ age: 1 }]) + }) + + // I couldn't find a way to make this work in Lucene and given that + // we're getting rid of Lucene soon I wasn't inclined to spend time on + // it. + !isLucene && + it("can find multiple values for same column", async () => { + await expectQuery({ + oneOf: { + // @ts-ignore + age: "1,10", + }, + }).toContainExactly([{ age: 1 }, { age: 10 }]) + }) }) describe("range", () => { diff --git a/packages/server/src/sdk/app/rows/search.ts b/packages/server/src/sdk/app/rows/search.ts index 286a88054c..e2ec743d0e 100644 --- a/packages/server/src/sdk/app/rows/search.ts +++ b/packages/server/src/sdk/app/rows/search.ts @@ -66,37 +66,12 @@ export function removeEmptyFilters(filters: SearchFilters) { return filters } -// The frontend can send single values for array fields sometimes, so to handle -// this we convert them to arrays at the controller level so that nothing below -// this has to worry about the non-array values. -function fixupFilterArrays(filters: SearchFilters) { - const arrayFields = [ - SearchFilterOperator.ONE_OF, - SearchFilterOperator.CONTAINS, - SearchFilterOperator.NOT_CONTAINS, - SearchFilterOperator.CONTAINS_ANY, - ] - for (const searchField of arrayFields) { - const field = filters[searchField] - if (field == null) { - continue - } - - for (const key of Object.keys(field)) { - if (!Array.isArray(field[key])) { - field[key] = [field[key]] - } - } - } - return filters -} - export async function search( options: RowSearchParams ): Promise> { const isExternalTable = isExternalTableID(options.tableId) options.query = removeEmptyFilters(options.query || {}) - options.query = fixupFilterArrays(options.query) + options.query = dataFilters.fixupFilterArrays(options.query) if ( !dataFilters.hasFilters(options.query) && options.query.onEmptyFilter === EmptyFilterOption.RETURN_NONE diff --git a/packages/shared-core/src/filters.ts b/packages/shared-core/src/filters.ts index 3c6901e195..d4d5918bbb 100644 --- a/packages/shared-core/src/filters.ts +++ b/packages/shared-core/src/filters.ts @@ -327,6 +327,35 @@ export const buildQuery = (filter: SearchFilter[]) => { return query } +// The frontend can send single values for array fields sometimes, so to handle +// this we convert them to arrays at the controller level so that nothing below +// this has to worry about the non-array values. +export function fixupFilterArrays(filters: SearchFilters) { + const arrayFields = [ + SearchFilterOperator.ONE_OF, + SearchFilterOperator.CONTAINS, + SearchFilterOperator.NOT_CONTAINS, + SearchFilterOperator.CONTAINS_ANY, + ] + for (const searchField of arrayFields) { + const field = filters[searchField] + if (field == null) { + continue + } + + for (const key of Object.keys(field)) { + if (!Array.isArray(field[key])) { + if (typeof field[key] !== "string") { + field[key] = [field[key]] + } else { + field[key] = field[key].split(",").map(x => x.trim()) + } + } + } + } + return filters +} + export const search = ( docs: Record[], query: RowSearchParams @@ -360,6 +389,7 @@ export const runQuery = (docs: Record[], query: SearchFilters) => { } query = cleanupQuery(query) + query = fixupFilterArrays(query) if ( !hasFilters(query) && @@ -528,9 +558,10 @@ export const runQuery = (docs: Record[], query: SearchFilters) => { (docValue: any, testValue: any) => { if (typeof testValue === "string") { testValue = testValue.split(",") - if (typeof docValue === "number") { - testValue = testValue.map((item: string) => parseFloat(item)) - } + } + + if (typeof docValue === "number") { + testValue = testValue.map((item: string) => parseFloat(item)) } if (!Array.isArray(testValue)) { From 102bd28980a266a5aa57a513609019cfd80a47f4 Mon Sep 17 00:00:00 2001 From: Sam Rose Date: Tue, 9 Jul 2024 11:52:20 +0100 Subject: [PATCH 2/8] Fix lint. --- packages/server/src/sdk/app/rows/search.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/server/src/sdk/app/rows/search.ts b/packages/server/src/sdk/app/rows/search.ts index e2ec743d0e..7bcd26806c 100644 --- a/packages/server/src/sdk/app/rows/search.ts +++ b/packages/server/src/sdk/app/rows/search.ts @@ -2,7 +2,6 @@ import { EmptyFilterOption, Row, RowSearchParams, - SearchFilterOperator, SearchFilters, SearchResponse, SortOrder, From b9c32b7068d65fdac1aefdea05a63bc0032f5415 Mon Sep 17 00:00:00 2001 From: Sam Rose Date: Wed, 10 Jul 2024 09:14:15 +0100 Subject: [PATCH 3/8] Fix tests? --- packages/shared-core/src/filters.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/shared-core/src/filters.ts b/packages/shared-core/src/filters.ts index d4d5918bbb..8298f7929a 100644 --- a/packages/shared-core/src/filters.ts +++ b/packages/shared-core/src/filters.ts @@ -348,7 +348,7 @@ export function fixupFilterArrays(filters: SearchFilters) { if (typeof field[key] !== "string") { field[key] = [field[key]] } else { - field[key] = field[key].split(",").map(x => x.trim()) + field[key] = field[key].split(",").map((x: string) => x.trim()) } } } From 114edc895482ed865776e0e0c2494dbc30745426 Mon Sep 17 00:00:00 2001 From: Sam Rose Date: Wed, 10 Jul 2024 10:50:05 +0100 Subject: [PATCH 4/8] Respond to PR feedback. --- packages/shared-core/src/filters.ts | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/packages/shared-core/src/filters.ts b/packages/shared-core/src/filters.ts index de4ee1897a..f7c7aa0922 100644 --- a/packages/shared-core/src/filters.ts +++ b/packages/shared-core/src/filters.ts @@ -18,7 +18,7 @@ import { import dayjs from "dayjs" import { OperatorOptions, SqlNumberTypeRangeMap } from "./constants" import { deepGet, schema } from "./helpers" -import _ from "lodash" +import { isPlainObject, isEmpty, isArray } from "lodash" const HBS_REGEX = /{{([^{].*?)}}/g @@ -335,7 +335,7 @@ export function fixupFilterArrays(filters: SearchFilters) { ] for (const searchField of arrayFields) { const field = filters[searchField] - if (field == null) { + if (field == null || !isPlainObject(field)) { continue } @@ -444,11 +444,11 @@ export const runQuery = (docs: Record[], query: SearchFilters) => { return false } - if (_.isObject(testValue.low) && _.isEmpty(testValue.low)) { + if (isPlainObject(testValue.low) && isEmpty(testValue.low)) { testValue.low = undefined } - if (_.isObject(testValue.high) && _.isEmpty(testValue.high)) { + if (isPlainObject(testValue.high) && isEmpty(testValue.high)) { testValue.high = undefined } From 093579a34189a8c4dd30ffcaaf0ee83350d894fc Mon Sep 17 00:00:00 2001 From: Sam Rose Date: Wed, 10 Jul 2024 11:05:16 +0100 Subject: [PATCH 5/8] Respond to PR feedback. --- packages/shared-core/src/filters.ts | 78 +++++++++++++---------------- packages/types/src/sdk/search.ts | 40 +++++++++------ 2 files changed, 58 insertions(+), 60 deletions(-) diff --git a/packages/shared-core/src/filters.ts b/packages/shared-core/src/filters.ts index f7c7aa0922..34e107562a 100644 --- a/packages/shared-core/src/filters.ts +++ b/packages/shared-core/src/filters.ts @@ -6,6 +6,7 @@ import { SearchFilter, SearchFilters, SearchQueryFields, + ArrayOperator, SearchFilterOperator, SortType, FieldConstraints, @@ -14,11 +15,13 @@ import { EmptyFilterOption, SearchResponse, Table, + BasicOperator, + RangeOperator, } from "@budibase/types" import dayjs from "dayjs" import { OperatorOptions, SqlNumberTypeRangeMap } from "./constants" import { deepGet, schema } from "./helpers" -import { isPlainObject, isEmpty, isArray } from "lodash" +import { isPlainObject, isEmpty } from "lodash" const HBS_REGEX = /{{([^{].*?)}}/g @@ -327,13 +330,7 @@ export const buildQuery = (filter: SearchFilter[]) => { // this we convert them to arrays at the controller level so that nothing below // this has to worry about the non-array values. export function fixupFilterArrays(filters: SearchFilters) { - const arrayFields = [ - SearchFilterOperator.ONE_OF, - SearchFilterOperator.CONTAINS, - SearchFilterOperator.NOT_CONTAINS, - SearchFilterOperator.CONTAINS_ANY, - ] - for (const searchField of arrayFields) { + for (const searchField of Object.values(ArrayOperator)) { const field = filters[searchField] if (field == null || !isPlainObject(field)) { continue @@ -341,10 +338,12 @@ export function fixupFilterArrays(filters: SearchFilters) { for (const key of Object.keys(field)) { if (!Array.isArray(field[key])) { - if (typeof field[key] !== "string") { - field[key] = [field[key]] + if (typeof field[key] === "string") { + field[key] = (field[key] as string) + .split(",") + .map((x: string) => x.trim()) } else { - field[key] = field[key].split(",").map((x: string) => x.trim()) + field[key] = [field[key]] } } } @@ -412,7 +411,7 @@ export const runQuery = (docs: Record[], query: SearchFilters) => { } const stringMatch = match( - SearchFilterOperator.STRING, + BasicOperator.STRING, (docValue: any, testValue: any) => { if (!(typeof docValue === "string")) { return false @@ -425,7 +424,7 @@ export const runQuery = (docs: Record[], query: SearchFilters) => { ) const fuzzyMatch = match( - SearchFilterOperator.FUZZY, + BasicOperator.FUZZY, (docValue: any, testValue: any) => { if (!(typeof docValue === "string")) { return false @@ -438,7 +437,7 @@ export const runQuery = (docs: Record[], query: SearchFilters) => { ) const rangeMatch = match( - SearchFilterOperator.RANGE, + RangeOperator.RANGE, (docValue: any, testValue: any) => { if (docValue == null || docValue === "") { return false @@ -527,11 +526,8 @@ export const runQuery = (docs: Record[], query: SearchFilters) => { (...args: T): boolean => !f(...args) - const equalMatch = match(SearchFilterOperator.EQUAL, _valueMatches) - const notEqualMatch = match( - SearchFilterOperator.NOT_EQUAL, - not(_valueMatches) - ) + const equalMatch = match(BasicOperator.EQUAL, _valueMatches) + const notEqualMatch = match(BasicOperator.NOT_EQUAL, not(_valueMatches)) const _empty = (docValue: any) => { if (typeof docValue === "string") { @@ -546,27 +542,24 @@ export const runQuery = (docs: Record[], query: SearchFilters) => { return docValue == null } - const emptyMatch = match(SearchFilterOperator.EMPTY, _empty) - const notEmptyMatch = match(SearchFilterOperator.NOT_EMPTY, not(_empty)) + const emptyMatch = match(BasicOperator.EMPTY, _empty) + const notEmptyMatch = match(BasicOperator.NOT_EMPTY, not(_empty)) - const oneOf = match( - SearchFilterOperator.ONE_OF, - (docValue: any, testValue: any) => { - if (typeof testValue === "string") { - testValue = testValue.split(",") - } - - if (typeof docValue === "number") { - testValue = testValue.map((item: string) => parseFloat(item)) - } - - if (!Array.isArray(testValue)) { - return false - } - - return testValue.some(item => _valueMatches(docValue, item)) + const oneOf = match(ArrayOperator.ONE_OF, (docValue: any, testValue: any) => { + if (typeof testValue === "string") { + testValue = testValue.split(",") } - ) + + if (typeof docValue === "number") { + testValue = testValue.map((item: string) => parseFloat(item)) + } + + if (!Array.isArray(testValue)) { + return false + } + + return testValue.some(item => _valueMatches(docValue, item)) + }) const _contains = (f: "some" | "every") => (docValue: any, testValue: any) => { @@ -593,7 +586,7 @@ export const runQuery = (docs: Record[], query: SearchFilters) => { } const contains = match( - SearchFilterOperator.CONTAINS, + ArrayOperator.CONTAINS, (docValue: any, testValue: any) => { if (Array.isArray(testValue) && testValue.length === 0) { return true @@ -602,7 +595,7 @@ export const runQuery = (docs: Record[], query: SearchFilters) => { } ) const notContains = match( - SearchFilterOperator.NOT_CONTAINS, + ArrayOperator.NOT_CONTAINS, (docValue: any, testValue: any) => { // Not sure if this is logically correct, but at the time this code was // written the search endpoint behaved this way and we wanted to make this @@ -613,10 +606,7 @@ export const runQuery = (docs: Record[], query: SearchFilters) => { return not(_contains("every"))(docValue, testValue) } ) - const containsAny = match( - SearchFilterOperator.CONTAINS_ANY, - _contains("some") - ) + const containsAny = match(ArrayOperator.CONTAINS_ANY, _contains("some")) const docMatch = (doc: Record) => { const filterFunctions = { diff --git a/packages/types/src/sdk/search.ts b/packages/types/src/sdk/search.ts index ccb73a7fba..5607efece8 100644 --- a/packages/types/src/sdk/search.ts +++ b/packages/types/src/sdk/search.ts @@ -3,20 +3,28 @@ import { Row, Table, DocumentType } from "../documents" import { SortOrder, SortType } from "../api" import { Knex } from "knex" -export enum SearchFilterOperator { - STRING = "string", - FUZZY = "fuzzy", - RANGE = "range", +export enum BasicOperator { EQUAL = "equal", NOT_EQUAL = "notEqual", EMPTY = "empty", NOT_EMPTY = "notEmpty", - ONE_OF = "oneOf", + FUZZY = "fuzzy", + STRING = "string", +} + +export enum ArrayOperator { CONTAINS = "contains", NOT_CONTAINS = "notContains", CONTAINS_ANY = "containsAny", + ONE_OF = "oneOf", } +export enum RangeOperator { + RANGE = "range", +} + +export type SearchFilterOperator = BasicOperator | ArrayOperator | RangeOperator + export enum InternalSearchFilterOperator { COMPLEX_ID_OPERATOR = "_complexIdOperator", } @@ -52,17 +60,17 @@ export interface SearchFilters { // allows just fuzzy to be or - all the fuzzy/like parameters fuzzyOr?: boolean onEmptyFilter?: EmptyFilterOption - [SearchFilterOperator.STRING]?: BasicFilter - [SearchFilterOperator.FUZZY]?: BasicFilter - [SearchFilterOperator.RANGE]?: RangeFilter - [SearchFilterOperator.EQUAL]?: BasicFilter - [SearchFilterOperator.NOT_EQUAL]?: BasicFilter - [SearchFilterOperator.EMPTY]?: BasicFilter - [SearchFilterOperator.NOT_EMPTY]?: BasicFilter - [SearchFilterOperator.ONE_OF]?: ArrayFilter - [SearchFilterOperator.CONTAINS]?: ArrayFilter - [SearchFilterOperator.NOT_CONTAINS]?: ArrayFilter - [SearchFilterOperator.CONTAINS_ANY]?: ArrayFilter + [BasicOperator.STRING]?: BasicFilter + [BasicOperator.FUZZY]?: BasicFilter + [RangeOperator.RANGE]?: RangeFilter + [BasicOperator.EQUAL]?: BasicFilter + [BasicOperator.NOT_EQUAL]?: BasicFilter + [BasicOperator.EMPTY]?: BasicFilter + [BasicOperator.NOT_EMPTY]?: BasicFilter + [ArrayOperator.ONE_OF]?: ArrayFilter + [ArrayOperator.CONTAINS]?: ArrayFilter + [ArrayOperator.NOT_CONTAINS]?: ArrayFilter + [ArrayOperator.CONTAINS_ANY]?: ArrayFilter // specific to SQS/SQLite search on internal tables this can be used // to make sure the documents returned are always filtered down to a // specific document type (such as just rows) From 5356cfdce5f9f674aa0c81ad7913cab7b3e33f66 Mon Sep 17 00:00:00 2001 From: Sam Rose Date: Wed, 10 Jul 2024 11:08:11 +0100 Subject: [PATCH 6/8] Fix uses of SearchFilterOperator. --- packages/backend-core/src/users/users.ts | 9 +++++---- packages/server/src/api/routes/tests/viewV2.spec.ts | 12 ++++++------ 2 files changed, 11 insertions(+), 10 deletions(-) diff --git a/packages/backend-core/src/users/users.ts b/packages/backend-core/src/users/users.ts index 7d62a6ef39..0c994d8287 100644 --- a/packages/backend-core/src/users/users.ts +++ b/packages/backend-core/src/users/users.ts @@ -18,9 +18,10 @@ import { CouchFindOptions, DatabaseQueryOpts, SearchFilters, - SearchFilterOperator, SearchUsersRequest, User, + BasicOperator, + ArrayOperator, } from "@budibase/types" import * as context from "../context" import { getGlobalDB } from "../context" @@ -46,9 +47,9 @@ function removeUserPassword(users: User | User[]) { export function isSupportedUserSearch(query: SearchFilters) { const allowed = [ - { op: SearchFilterOperator.STRING, key: "email" }, - { op: SearchFilterOperator.EQUAL, key: "_id" }, - { op: SearchFilterOperator.ONE_OF, key: "_id" }, + { op: BasicOperator.STRING, key: "email" }, + { op: BasicOperator.EQUAL, key: "_id" }, + { op: ArrayOperator.ONE_OF, key: "_id" }, ] for (let [key, operation] of Object.entries(query)) { if (typeof operation !== "object") { diff --git a/packages/server/src/api/routes/tests/viewV2.spec.ts b/packages/server/src/api/routes/tests/viewV2.spec.ts index 43a6d39172..93ce912472 100644 --- a/packages/server/src/api/routes/tests/viewV2.spec.ts +++ b/packages/server/src/api/routes/tests/viewV2.spec.ts @@ -9,7 +9,6 @@ import { QuotaUsageType, Row, SaveTableRequest, - SearchFilterOperator, SortOrder, SortType, StaticQuotaName, @@ -19,6 +18,7 @@ import { ViewUIFieldMetadata, ViewV2, SearchResponse, + BasicOperator, } from "@budibase/types" import { generator, mocks } from "@budibase/backend-core/tests" import { DatabaseName, getDatasource } from "../../../integrations/tests/utils" @@ -149,7 +149,7 @@ describe.each([ primaryDisplay: "id", query: [ { - operator: SearchFilterOperator.EQUAL, + operator: BasicOperator.EQUAL, field: "field", value: "value", }, @@ -561,7 +561,7 @@ describe.each([ ...view, query: [ { - operator: SearchFilterOperator.EQUAL, + operator: BasicOperator.EQUAL, field: "newField", value: "thatValue", }, @@ -589,7 +589,7 @@ describe.each([ primaryDisplay: "Price", query: [ { - operator: SearchFilterOperator.EQUAL, + operator: BasicOperator.EQUAL, field: generator.word(), value: generator.word(), }, @@ -673,7 +673,7 @@ describe.each([ tableId: generator.guid(), query: [ { - operator: SearchFilterOperator.EQUAL, + operator: BasicOperator.EQUAL, field: "newField", value: "thatValue", }, @@ -1194,7 +1194,7 @@ describe.each([ name: generator.guid(), query: [ { - operator: SearchFilterOperator.EQUAL, + operator: BasicOperator.EQUAL, field: "two", value: "bar2", }, From 79b4d260f11cef0545c3ded050d8f74d4adf738f Mon Sep 17 00:00:00 2001 From: Sam Rose Date: Wed, 10 Jul 2024 11:17:59 +0100 Subject: [PATCH 7/8] Fix more fucky wucky typey wipey stuff. --- .../src/components/FilterBuilder.svelte | 4 ++-- packages/shared-core/src/filters.ts | 17 +++++++++-------- 2 files changed, 11 insertions(+), 10 deletions(-) diff --git a/packages/frontend-core/src/components/FilterBuilder.svelte b/packages/frontend-core/src/components/FilterBuilder.svelte index c0bc328a4e..5b6b6b4c86 100644 --- a/packages/frontend-core/src/components/FilterBuilder.svelte +++ b/packages/frontend-core/src/components/FilterBuilder.svelte @@ -11,7 +11,7 @@ Label, Multiselect, } from "@budibase/bbui" - import { FieldType, SearchFilterOperator } from "@budibase/types" + import { ArrayOperator, FieldType } from "@budibase/types" import { generate } from "shortid" import { QueryUtils, Constants } from "@budibase/frontend-core" import { getContext } from "svelte" @@ -268,7 +268,7 @@ {:else if [FieldType.STRING, FieldType.LONGFORM, FieldType.NUMBER, FieldType.BIGINT, FieldType.FORMULA].includes(filter.type)} - {:else if filter.type === FieldType.ARRAY || (filter.type === FieldType.OPTIONS && filter.operator === SearchFilterOperator.ONE_OF)} + {:else if filter.type === FieldType.ARRAY || (filter.type === FieldType.OPTIONS && filter.operator === ArrayOperator.ONE_OF)} x.trim()) - } else { - field[key] = [field[key]] - } + if (Array.isArray(field[key])) { + continue + } + + const value = field[key] as any + if (typeof value === "string") { + field[key] = value.split(",").map((x: string) => x.trim()) + } else { + field[key] = [value] } } } From d0c3de63eba8fd42e0fc986e9bde7ba0a9593c52 Mon Sep 17 00:00:00 2001 From: Sam Rose Date: Wed, 10 Jul 2024 11:25:53 +0100 Subject: [PATCH 8/8] Fix sqs image reference. --- .github/workflows/budibase_ci.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/budibase_ci.yml b/.github/workflows/budibase_ci.yml index 288a0462e7..d63596f08f 100644 --- a/.github/workflows/budibase_ci.yml +++ b/.github/workflows/budibase_ci.yml @@ -108,7 +108,7 @@ jobs: - name: Pull testcontainers images run: | docker pull testcontainers/ryuk:0.5.1 & - docker pull budibase/couchdb:v3.2.1-sql & + docker pull budibase/couchdb:v3.2.1-sqs & docker pull redis & wait $(jobs -p)