Merge pull request #14292 from Budibase/fix/sql-pagination-fixes

SQL pagination improvements

packages/backend-core/src/sql/sql.ts

@@ -37,10 +37,12 @@ import { helpers } from "@budibase/shared-core"
 
 type QueryFunction = (query: SqlQuery | SqlQuery[], operation: Operation) => any
 
-const envLimit = environment.SQL_MAX_ROWS
+function getBaseLimit() {
-  ? parseInt(environment.SQL_MAX_ROWS)
+  const envLimit = environment.SQL_MAX_ROWS
-  : null
+    ? parseInt(environment.SQL_MAX_ROWS)
-const BASE_LIMIT = envLimit || 5000
+    : null
+  return envLimit || 5000
+}
 
 // Takes a string like foo and returns a quoted string like [foo] for SQL Server
 // and "foo" for Postgres.
@@ -838,7 +840,7 @@ class SqlQueryBuilder extends SqlTableQueryBuilder {
   private readonly limit: number
 
   // pass through client to get flavour of SQL
-  constructor(client: SqlClient, limit: number = BASE_LIMIT) {
+  constructor(client: SqlClient, limit: number = getBaseLimit()) {
     super(client)
     this.limit = limit
   }
@@ -882,7 +884,7 @@ class SqlQueryBuilder extends SqlTableQueryBuilder {
         query = builder.read(client, json, {
           limits: {
             query: this.limit,
-            base: BASE_LIMIT,
+            base: getBaseLimit(),
           },
         })
         break

packages/server/src/api/controllers/row/ExternalRequest.ts

@@ -66,9 +66,14 @@ export interface RunConfig {
   includeSqlRelationships?: IncludeRelationship
 }
 
+export type ExternalReadRequestReturnType = {
+  rows: Row[]
+  rawResponseSize: number
+}
+
 export type ExternalRequestReturnType<T extends Operation> =
   T extends Operation.READ
-    ? Row[]
+    ? ExternalReadRequestReturnType
     : T extends Operation.COUNT
     ? number
     : { row: Row; table: Table }
@@ -741,9 +746,11 @@ export class ExternalRequest<T extends Operation> {
     )
     // if reading it'll just be an array of rows, return whole thing
     if (operation === Operation.READ) {
-      return (
+      const rows = Array.isArray(output) ? output : [output]
-        Array.isArray(output) ? output : [output]
+      return {
-      ) as ExternalRequestReturnType<T>
+        rows,
+        rawResponseSize: responseRows.length,
+      } as ExternalRequestReturnType<T>
     } else {
       return { row: output[0], table } as ExternalRequestReturnType<T>
     }

packages/server/src/api/controllers/row/external.ts

@@ -136,7 +136,7 @@ export async function fetchEnrichedRow(ctx: UserCtx) {
     includeSqlRelationships: IncludeRelationship.INCLUDE,
   })
   const table: Table = tables[tableName]
-  const row = response[0]
+  const row = response.rows[0]
   // this seems like a lot of work, but basically we need to dig deeper for the enrich
   // for a single row, there is probably a better way to do this with some smart multi-layer joins
   for (let [fieldName, field] of Object.entries(table.schema)) {
@@ -163,10 +163,14 @@ export async function fetchEnrichedRow(ctx: UserCtx) {
       },
       includeSqlRelationships: IncludeRelationship.INCLUDE,
     })
-    row[fieldName] = await outputProcessing(linkedTable, relatedRows, {
+    row[fieldName] = await outputProcessing<Row[]>(
-      squash: true,
+      linkedTable,
-      preserveLinks: true,
+      relatedRows.rows,
-    })
+      {
+        squash: true,
+        preserveLinks: true,
+      }
+    )
   }
   return row
 }

packages/server/src/api/routes/tests/search.spec.ts

@@ -53,6 +53,7 @@ describe.each([
   const isLucene = name === "lucene"
   const isInMemory = name === "in-memory"
   const isInternal = isSqs || isLucene || isInMemory
+  const isSql = !isInMemory && !isLucene
   const config = setup.getConfig()
 
   let envCleanup: (() => void) | undefined
@@ -192,7 +193,8 @@ describe.each([
     // different to the one passed in will cause the assertion to fail.  Extra
     // rows returned by the query will also cause the assertion to fail.
     async toMatchExactly(expectedRows: any[]) {
-      const { rows: foundRows } = await this.performSearch()
+      const response = await this.performSearch()
+      const foundRows = response.rows
 
       // eslint-disable-next-line jest/no-standalone-expect
       expect(foundRows).toHaveLength(expectedRows.length)
@@ -202,13 +204,15 @@ describe.each([
           expect.objectContaining(this.popRow(expectedRow, foundRows))
         )
       )
+      return response
     }
 
     // Asserts that the query returns rows matching exactly the set of rows
     // passed in. The order of the rows is not important, but extra rows will
     // cause the assertion to fail.
     async toContainExactly(expectedRows: any[]) {
-      const { rows: foundRows } = await this.performSearch()
+      const response = await this.performSearch()
+      const foundRows = response.rows
 
       // eslint-disable-next-line jest/no-standalone-expect
       expect(foundRows).toHaveLength(expectedRows.length)
@@ -220,6 +224,7 @@ describe.each([
           )
         )
       )
+      return response
     }
 
     // Asserts that the query returns some property values - this cannot be used
@@ -236,6 +241,7 @@ describe.each([
           expect(response[key]).toEqual(properties[key])
         }
       }
+      return response
     }
 
     // Asserts that the query doesn't return a property, e.g. pagination parameters.
@@ -245,13 +251,15 @@ describe.each([
         // eslint-disable-next-line jest/no-standalone-expect
         expect(response[property]).toBeUndefined()
       }
+      return response
     }
 
     // Asserts that the query returns rows matching the set of rows passed in.
     // The order of the rows is not important. Extra rows will not cause the
     // assertion to fail.
     async toContain(expectedRows: any[]) {
-      const { rows: foundRows } = await this.performSearch()
+      const response = await this.performSearch()
+      const foundRows = response.rows
 
       // eslint-disable-next-line jest/no-standalone-expect
       expect([...foundRows]).toEqual(
@@ -261,6 +269,7 @@ describe.each([
           )
         )
       )
+      return response
     }
 
     async toFindNothing() {
@@ -2608,4 +2617,79 @@ describe.each([
         }).toContainExactly([row])
       })
     })
+
+  isSql &&
+    describe("pagination edge case with relationships", () => {
+      let mainRows: Row[] = []
+
+      beforeAll(async () => {
+        const toRelateTable = await createTable({
+          name: {
+            name: "name",
+            type: FieldType.STRING,
+          },
+        })
+        table = await createTable({
+          name: {
+            name: "name",
+            type: FieldType.STRING,
+          },
+          rel: {
+            name: "rel",
+            type: FieldType.LINK,
+            relationshipType: RelationshipType.MANY_TO_ONE,
+            tableId: toRelateTable._id!,
+            fieldName: "rel",
+          },
+        })
+        const relatedRows = await Promise.all([
+          config.api.row.save(toRelateTable._id!, { name: "tag 1" }),
+          config.api.row.save(toRelateTable._id!, { name: "tag 2" }),
+          config.api.row.save(toRelateTable._id!, { name: "tag 3" }),
+          config.api.row.save(toRelateTable._id!, { name: "tag 4" }),
+          config.api.row.save(toRelateTable._id!, { name: "tag 5" }),
+          config.api.row.save(toRelateTable._id!, { name: "tag 6" }),
+        ])
+        mainRows = await Promise.all([
+          config.api.row.save(table._id!, {
+            name: "product 1",
+            rel: relatedRows.map(row => row._id),
+          }),
+          config.api.row.save(table._id!, {
+            name: "product 2",
+            rel: [],
+          }),
+          config.api.row.save(table._id!, {
+            name: "product 3",
+            rel: [],
+          }),
+        ])
+      })
+
+      it("can still page when the hard limit is hit", async () => {
+        await config.withCoreEnv(
+          {
+            SQL_MAX_ROWS: "6",
+          },
+          async () => {
+            const params: Omit<RowSearchParams, "tableId"> = {
+              query: {},
+              paginate: true,
+              limit: 3,
+              sort: "name",
+              sortType: SortType.STRING,
+              sortOrder: SortOrder.ASCENDING,
+            }
+            const page1 = await expectSearch(params).toContain([mainRows[0]])
+            expect(page1.hasNextPage).toBe(true)
+            expect(page1.bookmark).toBeDefined()
+            const page2 = await expectSearch({
+              ...params,
+              bookmark: page1.bookmark,
+            }).toContain([mainRows[1], mainRows[2]])
+            expect(page2.hasNextPage).toBe(false)
+          }
+        )
+      })
+    })
 })

packages/server/src/sdk/app/rows/external.ts

@@ -21,7 +21,8 @@ export async function getRow(
       ? IncludeRelationship.INCLUDE
       : IncludeRelationship.EXCLUDE,
   })
-  return response ? response[0] : response
+  const rows = response?.rows || []
+  return rows[0]
 }
 
 export async function save(

packages/server/src/sdk/app/rows/search/external.ts

@@ -47,7 +47,7 @@ function getPaginationAndLimitParameters(
       limit: limit + 1,
     }
     if (bookmark) {
-      paginateObj.offset = limit * bookmark
+      paginateObj.offset = bookmark
     }
   } else if (limit) {
     paginateObj = {
@@ -105,37 +105,37 @@ export async function search(
       paginate: paginateObj as PaginationJson,
       includeSqlRelationships: IncludeRelationship.INCLUDE,
     }
-    const queries: Promise<Row[] | number>[] = []
+    const [{ rows, rawResponseSize }, totalRows] = await Promise.all([
-    queries.push(handleRequest(Operation.READ, tableId, parameters))
+      handleRequest(Operation.READ, tableId, parameters),
-    if (countRows) {
+      countRows
-      queries.push(handleRequest(Operation.COUNT, tableId, parameters))
+        ? handleRequest(Operation.COUNT, tableId, parameters)
-    }
+        : Promise.resolve(undefined),
-    const responses = await Promise.all(queries)
+    ])
-    let rows = responses[0] as Row[]
-    const totalRows =
-      responses.length > 1 ? (responses[1] as number) : undefined
 
-    let hasNextPage = false
+    let processed = await outputProcessing<Row[]>(table, rows, {
-    // remove the extra row if it's there
-    if (paginate && limit && rows.length > limit) {
-      rows.pop()
-      hasNextPage = true
-    }
-
-    if (options.fields) {
-      const fields = [...options.fields, ...PROTECTED_EXTERNAL_COLUMNS]
-      rows = rows.map((r: any) => pick(r, fields))
-    }
-
-    rows = await outputProcessing<Row[]>(table, rows, {
       preserveLinks: true,
       squash: true,
     })
 
+    let hasNextPage = false
+    // if the raw rows is greater than the limit then we likely need to paginate
+    if (paginate && limit && rawResponseSize > limit) {
+      hasNextPage = true
+      // processed rows has merged relationships down, this might not be more than limit
+      if (processed.length > limit) {
+        processed.pop()
+      }
+    }
+
+    if (options.fields) {
+      const fields = [...options.fields, ...PROTECTED_EXTERNAL_COLUMNS]
+      processed = processed.map((r: any) => pick(r, fields))
+    }
+
     // need wrapper object for bookmarks etc when paginating
-    const response: SearchResponse<Row> = { rows, hasNextPage }
+    const response: SearchResponse<Row> = { rows: processed, hasNextPage }
     if (hasNextPage && bookmark != null) {
-      response.bookmark = bookmark + 1
+      response.bookmark = bookmark + processed.length
     }
     if (totalRows != null) {
       response.totalRows = totalRows
@@ -255,24 +255,21 @@ export async function exportRows(
 }
 
 export async function fetch(tableId: string): Promise<Row[]> {
-  const response = await handleRequest<Operation.READ>(
+  const response = await handleRequest(Operation.READ, tableId, {
-    Operation.READ,
+    includeSqlRelationships: IncludeRelationship.INCLUDE,
-    tableId,
+  })
-    {
-      includeSqlRelationships: IncludeRelationship.INCLUDE,
-    }
-  )
   const table = await sdk.tables.getTable(tableId)
-  return await outputProcessing<Row[]>(table, response, {
+  return await outputProcessing<Row[]>(table, response.rows, {
     preserveLinks: true,
     squash: true,
   })
 }
 
 export async function fetchRaw(tableId: string): Promise<Row[]> {
-  return await handleRequest<Operation.READ>(Operation.READ, tableId, {
+  const response = await handleRequest(Operation.READ, tableId, {
     includeSqlRelationships: IncludeRelationship.INCLUDE,
   })
+  return response.rows
 }
 
 export async function fetchView(viewName: string) {

packages/server/src/sdk/app/rows/search/sqs.ts

@@ -46,6 +46,7 @@ import { isSearchingByRowID } from "./utils"
 import tracer from "dd-trace"
 
 const builder = new sql.Sql(SqlClient.SQL_LITE)
+const SQLITE_COLUMN_LIMIT = 2000
 const MISSING_COLUMN_REGEX = new RegExp(`no such column: .+`)
 const MISSING_TABLE_REGX = new RegExp(`no such table: .+`)
 const DUPLICATE_COLUMN_REGEX = new RegExp(`duplicate column name: .+`)
@@ -56,12 +57,14 @@ function buildInternalFieldList(
   opts?: { relationships?: RelationshipsJson[] }
 ) {
   let fieldList: string[] = []
-  const addJunctionFields = (relatedTable: Table, fields: string[]) => {
+  const getJunctionFields = (relatedTable: Table, fields: string[]) => {
+    const junctionFields: string[] = []
     fields.forEach(field => {
-      fieldList.push(
+      junctionFields.push(
         `${generateJunctionTableID(table._id!, relatedTable._id!)}.${field}`
       )
     })
+    return junctionFields
   }
   fieldList = fieldList.concat(
     PROTECTED_INTERNAL_COLUMNS.map(col => `${table._id}.${col}`)
@@ -71,18 +74,22 @@ function buildInternalFieldList(
     if (!opts?.relationships && isRelationship) {
       continue
     }
-    if (isRelationship) {
+    if (!isRelationship) {
+      fieldList.push(`${table._id}.${mapToUserColumn(col.name)}`)
+    } else {
       const linkCol = col as RelationshipFieldMetadata
       const relatedTable = tables.find(table => table._id === linkCol.tableId)
-      // no relationships provided, don't go more than a layer deep
+      if (!relatedTable) {
-      if (relatedTable) {
+        continue
-        fieldList = fieldList.concat(
-          buildInternalFieldList(relatedTable, tables)
-        )
-        addJunctionFields(relatedTable, ["doc1.fieldName", "doc2.fieldName"])
       }
-    } else {
+      const relatedFields = buildInternalFieldList(relatedTable, tables).concat(
-      fieldList.push(`${table._id}.${mapToUserColumn(col.name)}`)
+        getJunctionFields(relatedTable, ["doc1.fieldName", "doc2.fieldName"])
+      )
+      // break out of the loop if we have reached the max number of columns
+      if (relatedFields.length + fieldList.length > SQLITE_COLUMN_LIMIT) {
+        break
+      }
+      fieldList = fieldList.concat(relatedFields)
     }
   }
   return [...new Set(fieldList)]
@@ -320,25 +327,19 @@ export async function search(
     paginate = true
     request.paginate = {
       limit: params.limit + 1,
-      offset: bookmark * params.limit,
+      offset: bookmark,
     }
   }
 
   try {
-    const queries: Promise<Row[] | number>[] = []
+    const [rows, totalRows] = await Promise.all([
-    queries.push(runSqlQuery(request, allTables, relationships))
+      runSqlQuery(request, allTables, relationships),
-    if (options.countRows) {
+      options.countRows
-      // get the total count of rows
+        ? runSqlQuery(request, allTables, relationships, {
-      queries.push(
+            countTotalRows: true,
-        runSqlQuery(request, allTables, relationships, {
+          })
-          countTotalRows: true,
+        : Promise.resolve(undefined),
-        })
+    ])
-      )
-    }
-    const responses = await Promise.all(queries)
-    let rows = responses[0] as Row[]
-    const totalRows =
-      responses.length > 1 ? (responses[1] as number) : undefined
 
     // process from the format of tableId.column to expected format also
     // make sure JSON columns corrected
@@ -350,10 +351,13 @@ export async function search(
     )
 
     // check for pagination final row
-    let nextRow: Row | undefined
+    let nextRow: boolean = false
     if (paginate && params.limit && rows.length > params.limit) {
       // remove the extra row that confirmed if there is another row to move to
-      nextRow = processed.pop()
+      nextRow = true
+      if (processed.length > params.limit) {
+        processed.pop()
+      }
     }
 
     // get the rows
@@ -377,7 +381,7 @@ export async function search(
     // check for pagination
     if (paginate && nextRow) {
       response.hasNextPage = true
-      response.bookmark = bookmark + 1
+      response.bookmark = bookmark + processed.length
     }
     if (paginate && !nextRow) {
       response.hasNextPage = false