Merge pull request #14120 from Budibase/budi-7065-csv-export-fails-for-table-with-composite-primary-key

CSV export fails for table with composite primary key
2024-09-20 03:08:18 +12:00 · 2024-07-09 16:42:28 +02:00 · 2024-07-09 16:42:28 +02:00 · 8341de6f2a
commit 8341de6f2a
parent 26874cb03c db71eab692
6 changed files with 137 additions and 87 deletions
--- a/packages/backend-core/src/sql/sql.ts
+++ b/packages/backend-core/src/sql/sql.ts
@ -11,10 +11,12 @@ import {
 import { SqlStatements } from "./sqlStatements"
 import SqlTableQueryBuilder from "./sqlTable"
 import {
+  AnySearchFilter,
  BBReferenceFieldMetadata,
  FieldSchema,
  FieldType,
  INTERNAL_TABLE_SOURCE_ID,
+  InternalSearchFilterOperator,
  JsonFieldMetadata,
  JsonTypes,
  Operation,
@ -40,7 +42,7 @@ const envLimit = environment.SQL_MAX_ROWS
  : null
 const BASE_LIMIT = envLimit || 5000

-function likeKey(client: string, key: string): string {
+function likeKey(client: string | string[], key: string): string {
  let start: string, end: string
  switch (client) {
    case SqlClient.MY_SQL:
@ -206,17 +208,32 @@ class InternalBuilder {
      return alias || name
    }
    function iterate(
-      structure: { [key: string]: any },
-      fn: (key: string, value: any) => void
+      structure: AnySearchFilter,
+      fn: (key: string, value: any) => void,
+      complexKeyFn?: (key: string[], value: any) => void
    ) {
-      for (let [key, value] of Object.entries(structure)) {
+      for (const key in structure) {
+        const value = structure[key]
        const updatedKey = dbCore.removeKeyNumbering(key)
        const isRelationshipField = updatedKey.includes(".")
-        if (!opts.relationship && !isRelationshipField) {
+
+        let castedTypeValue
+        if (
+          key === InternalSearchFilterOperator.COMPLEX_ID_OPERATOR &&
+          (castedTypeValue = structure[key]) &&
+          complexKeyFn
+        ) {
+          const alias = getTableAlias(tableName)
+          complexKeyFn(
+            castedTypeValue.id.map((x: string) =>
+              alias ? `${alias}.${x}` : x
+            ),
+            castedTypeValue.values
+          )
+        } else if (!opts.relationship && !isRelationshipField) {
          const alias = getTableAlias(tableName)
          fn(alias ? `${alias}.${updatedKey}` : updatedKey, value)
-        }
-        if (opts.relationship && isRelationshipField) {
+        } else if (opts.relationship && isRelationshipField) {
          const [filterTableName, property] = updatedKey.split(".")
          const alias = getTableAlias(filterTableName)
          fn(alias ? `${alias}.${property}` : property, value)
@ -239,7 +256,7 @@ class InternalBuilder {
      }
    }

-    const contains = (mode: object, any: boolean = false) => {
+    const contains = (mode: AnySearchFilter, any: boolean = false) => {
      const rawFnc = allOr ? "orWhereRaw" : "whereRaw"
      const not = mode === filters?.notContains ? "NOT " : ""
      function stringifyArray(value: Array<any>, quoteStyle = '"'): string {
@ -251,7 +268,7 @@ class InternalBuilder {
        return `[${value.join(",")}]`
      }
      if (this.client === SqlClient.POSTGRES) {
-        iterate(mode, (key: string, value: Array<any>) => {
+        iterate(mode, (key, value) => {
          const wrap = any ? "" : "'"
          const op = any ? "\\?| array" : "@>"
          const fieldNames = key.split(/\./g)
@ -266,7 +283,7 @@ class InternalBuilder {
        })
      } else if (this.client === SqlClient.MY_SQL) {
        const jsonFnc = any ? "JSON_OVERLAPS" : "JSON_CONTAINS"
-        iterate(mode, (key: string, value: Array<any>) => {
+        iterate(mode, (key, value) => {
          query = query[rawFnc](
            `${not}COALESCE(${jsonFnc}(${key}, '${stringifyArray(
              value
@ -275,7 +292,7 @@ class InternalBuilder {
        })
      } else {
        const andOr = mode === filters?.containsAny ? " OR " : " AND "
-        iterate(mode, (key: string, value: Array<any>) => {
+        iterate(mode, (key, value) => {
          let statement = ""
          for (let i in value) {
            if (typeof value[i] === "string") {
@ -299,10 +316,16 @@ class InternalBuilder {
    }

    if (filters.oneOf) {
-      iterate(filters.oneOf, (key, array) => {
-        const fnc = allOr ? "orWhereIn" : "whereIn"
-        query = query[fnc](key, Array.isArray(array) ? array : [array])
-      })
+      const fnc = allOr ? "orWhereIn" : "whereIn"
+      iterate(
+        filters.oneOf,
+        (key: string, array) => {
+          query = query[fnc](key, Array.isArray(array) ? array : [array])
+        },
+        (key: string[], array) => {
+          query = query[fnc](key, Array.isArray(array) ? array : [array])
+        }
+      )
    }
    if (filters.string) {
      iterate(filters.string, (key, value) => {
@ -744,6 +767,7 @@ class InternalBuilder {

 class SqlQueryBuilder extends SqlTableQueryBuilder {
  private readonly limit: number
+
  // pass through client to get flavour of SQL
  constructor(client: string, limit: number = BASE_LIMIT) {
    super(client)
--- a/packages/server/src/api/controllers/row/ExternalRequest.ts
+++ b/packages/server/src/api/controllers/row/ExternalRequest.ts
@ -7,6 +7,7 @@ import {
  FieldType,
  FilterType,
  IncludeRelationship,
+  InternalSearchFilterOperator,
  isManyToOne,
  OneToManyRelationshipFieldMetadata,
  Operation,
@ -189,15 +190,22 @@ export class ExternalRequest<T extends Operation> {
    if (filters) {
      // need to map over the filters and make sure the _id field isn't present
      let prefix = 1
-      for (let operator of Object.values(filters)) {
-        for (let field of Object.keys(operator || {})) {
+      for (const operator of Object.values(filters)) {
+        for (const field of Object.keys(operator || {})) {
          if (dbCore.removeKeyNumbering(field) === "_id") {
            if (primary) {
              const parts = breakRowIdField(operator[field])
-              for (let field of primary) {
-                operator[`${prefix}:${field}`] = parts.shift()
+              if (primary.length > 1) {
+                operator[InternalSearchFilterOperator.COMPLEX_ID_OPERATOR] = {
+                  id: primary,
+                  values: parts[0],
+                }
+              } else {
+                for (let field of primary) {
+                  operator[`${prefix}:${field}`] = parts.shift()
+                }
+                prefix++
              }
-              prefix++
            }
            // make sure this field doesn't exist on any filter
            delete operator[field]
--- a/packages/server/src/api/routes/tests/row.spec.ts
+++ b/packages/server/src/api/routes/tests/row.spec.ts
@ -1428,22 +1428,6 @@ describe.each([
      expect(row._id).toEqual(existing._id)
    })

-    it("should return an error on composite keys", async () => {
-      const existing = await config.api.row.save(table._id!, {})
-      await config.api.row.exportRows(
-        table._id!,
-        {
-          rows: [`['${existing._id!}']`, "['d001', '10111']"],
-        },
-        {
-          status: 400,
-          body: {
-            message: "Export data does not support composite keys.",
-          },
-        }
-      )
-    })
-
    it("should return an error if no table is found", async () => {
      const existing = await config.api.row.save(table._id!, {})
      await config.api.row.exportRows(
@ -1452,6 +1436,46 @@ describe.each([
        { status: 404 }
      )
    })
+
+    // MSSQL needs a setting called IDENTITY_INSERT to be set to ON to allow writing
+    // to identity columns. This is not something Budibase does currently.
+    providerType !== DatabaseName.SQL_SERVER &&
+      it("should handle filtering by composite primary keys", async () => {
+        const tableRequest = saveTableRequest({
+          primary: ["number", "string"],
+          schema: {
+            string: {
+              type: FieldType.STRING,
+              name: "string",
+            },
+            number: {
+              type: FieldType.NUMBER,
+              name: "number",
+            },
+          },
+        })
+        delete tableRequest.schema.id
+
+        const table = await config.api.table.save(tableRequest)
+
+        const rows = await Promise.all(
+          generator
+            .unique(
+              () => ({
+                string: generator.word({ length: 30 }),
+                number: generator.integer({ min: 0, max: 10000 }),
+              }),
+              10
+            )
+            .map(d => config.api.row.save(table._id!, d))
+        )
+
+        const res = await config.api.row.exportRows(table._id!, {
+          rows: _.sampleSize(rows, 3).map(r => r._id!),
+        })
+        const results = JSON.parse(res)
+        expect(results.length).toEqual(3)
+      })
  })

  let o2mTable: Table
--- a/packages/server/src/sdk/app/rows/search/external.ts
+++ b/packages/server/src/sdk/app/rows/search/external.ts
@ -158,10 +158,7 @@ export async function exportRows(
        _id: rowIds.map((row: string) => {
          const ids = breakRowIdField(row)
          if (ids.length > 1) {
-            throw new HTTPError(
-              "Export data does not support composite keys.",
-              400
-            )
+            return ids
          }
          return ids[0]
        }),
--- a/packages/shared-core/src/filters.ts
+++ b/packages/shared-core/src/filters.ts
@ -310,16 +310,12 @@ export const buildQuery = (filter: SearchFilter[]) => {
          query.equal = query.equal || {}
          query.equal[field] = true
        } else {
-          query[queryOperator] = {
-            ...query[queryOperator],
-            [field]: value,
-          }
+          query[queryOperator] ??= {}
+          query[queryOperator]![field] = value
        }
      } else {
-        query[queryOperator] = {
-          ...query[queryOperator],
-          [field]: value,
-        }
+        query[queryOperator] ??= {}
+        query[queryOperator]![field] = value
      }
    }
  })
--- a/packages/types/src/sdk/search.ts
+++ b/packages/types/src/sdk/search.ts
@ -17,51 +17,52 @@ export enum SearchFilterOperator {
  CONTAINS_ANY = "containsAny",
 }

+export enum InternalSearchFilterOperator {
+  COMPLEX_ID_OPERATOR = "_complexIdOperator",
+}
+
+type BasicFilter<T = any> = Record<string, T> & {
+  [InternalSearchFilterOperator.COMPLEX_ID_OPERATOR]?: never
+}
+
+type ArrayFilter = Record<string, any[]> & {
+  [InternalSearchFilterOperator.COMPLEX_ID_OPERATOR]?: {
+    id: string[]
+    values: string[]
+  }
+}
+
+type RangeFilter = Record<
+  string,
+  | {
+      high: number | string
+      low: number | string
+    }
+  | { high: number | string }
+  | { low: number | string }
+> & {
+  [InternalSearchFilterOperator.COMPLEX_ID_OPERATOR]?: never
+}
+
+export type AnySearchFilter = BasicFilter | ArrayFilter | RangeFilter
+
 export interface SearchFilters {
  allOr?: boolean
  // TODO: this is just around for now - we need a better way to do or/and
  // allows just fuzzy to be or - all the fuzzy/like parameters
  fuzzyOr?: boolean
  onEmptyFilter?: EmptyFilterOption
-  [SearchFilterOperator.STRING]?: {
-    [key: string]: string
-  }
-  [SearchFilterOperator.FUZZY]?: {
-    [key: string]: string
-  }
-  [SearchFilterOperator.RANGE]?: {
-    [key: string]:
-      | {
-          high: number | string
-          low: number | string
-        }
-      | { high: number | string }
-      | { low: number | string }
-  }
-  [SearchFilterOperator.EQUAL]?: {
-    [key: string]: any
-  }
-  [SearchFilterOperator.NOT_EQUAL]?: {
-    [key: string]: any
-  }
-  [SearchFilterOperator.EMPTY]?: {
-    [key: string]: any
-  }
-  [SearchFilterOperator.NOT_EMPTY]?: {
-    [key: string]: any
-  }
-  [SearchFilterOperator.ONE_OF]?: {
-    [key: string]: any[]
-  }
-  [SearchFilterOperator.CONTAINS]?: {
-    [key: string]: any[]
-  }
-  [SearchFilterOperator.NOT_CONTAINS]?: {
-    [key: string]: any[]
-  }
-  [SearchFilterOperator.CONTAINS_ANY]?: {
-    [key: string]: any[]
-  }
+  [SearchFilterOperator.STRING]?: BasicFilter<string>
+  [SearchFilterOperator.FUZZY]?: BasicFilter<string>
+  [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
  // 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)