diff --git a/packages/backend-core/src/sql/sql.ts b/packages/backend-core/src/sql/sql.ts index a67da7bc10..76c86b2b62 100644 --- a/packages/backend-core/src/sql/sql.ts +++ b/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 - ? parseInt(environment.SQL_MAX_ROWS) - : null -const BASE_LIMIT = envLimit || 5000 +function getBaseLimit() { + const envLimit = environment.SQL_MAX_ROWS + ? parseInt(environment.SQL_MAX_ROWS) + : 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 diff --git a/packages/server/src/api/controllers/row/ExternalRequest.ts b/packages/server/src/api/controllers/row/ExternalRequest.ts index 2ecdf9a4cb..6538e7347a 100644 --- a/packages/server/src/api/controllers/row/ExternalRequest.ts +++ b/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.READ - ? Row[] + ? ExternalReadRequestReturnType : T extends Operation.COUNT ? number : { row: Row; table: Table } @@ -741,9 +746,11 @@ export class ExternalRequest { ) // if reading it'll just be an array of rows, return whole thing if (operation === Operation.READ) { - return ( - Array.isArray(output) ? output : [output] - ) as ExternalRequestReturnType + const rows = Array.isArray(output) ? output : [output] + return { + rows, + rawResponseSize: responseRows.length, + } as ExternalRequestReturnType } else { return { row: output[0], table } as ExternalRequestReturnType } diff --git a/packages/server/src/api/controllers/row/external.ts b/packages/server/src/api/controllers/row/external.ts index 06013d230c..8b95d9c2f3 100644 --- a/packages/server/src/api/controllers/row/external.ts +++ b/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, { - squash: true, - preserveLinks: true, - }) + row[fieldName] = await outputProcessing( + linkedTable, + relatedRows.rows, + { + squash: true, + preserveLinks: true, + } + ) } return row } diff --git a/packages/server/src/api/routes/tests/search.spec.ts b/packages/server/src/api/routes/tests/search.spec.ts index 2c5756efe4..bc3cdccf18 100644 --- a/packages/server/src/api/routes/tests/search.spec.ts +++ b/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 = { + 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) + } + ) + }) + }) }) diff --git a/packages/server/src/sdk/app/rows/external.ts b/packages/server/src/sdk/app/rows/external.ts index 9ab1362606..f81d67f621 100644 --- a/packages/server/src/sdk/app/rows/external.ts +++ b/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( diff --git a/packages/server/src/sdk/app/rows/search/external.ts b/packages/server/src/sdk/app/rows/search/external.ts index e7fd2888de..3ce1013b85 100644 --- a/packages/server/src/sdk/app/rows/search/external.ts +++ b/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[] = [] - queries.push(handleRequest(Operation.READ, tableId, parameters)) - if (countRows) { - queries.push(handleRequest(Operation.COUNT, tableId, parameters)) - } - const responses = await Promise.all(queries) - let rows = responses[0] as Row[] - const totalRows = - responses.length > 1 ? (responses[1] as number) : undefined + const [{ rows, rawResponseSize }, totalRows] = await Promise.all([ + handleRequest(Operation.READ, tableId, parameters), + countRows + ? handleRequest(Operation.COUNT, tableId, parameters) + : Promise.resolve(undefined), + ]) - let hasNextPage = false - // 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(table, rows, { + let processed = await outputProcessing(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 = { rows, hasNextPage } + const response: SearchResponse = { 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 { - const response = await handleRequest( - Operation.READ, - tableId, - { - includeSqlRelationships: IncludeRelationship.INCLUDE, - } - ) + const response = await handleRequest(Operation.READ, tableId, { + includeSqlRelationships: IncludeRelationship.INCLUDE, + }) const table = await sdk.tables.getTable(tableId) - return await outputProcessing(table, response, { + return await outputProcessing(table, response.rows, { preserveLinks: true, squash: true, }) } export async function fetchRaw(tableId: string): Promise { - return await handleRequest(Operation.READ, tableId, { + const response = await handleRequest(Operation.READ, tableId, { includeSqlRelationships: IncludeRelationship.INCLUDE, }) + return response.rows } export async function fetchView(viewName: string) { diff --git a/packages/server/src/sdk/app/rows/search/sqs.ts b/packages/server/src/sdk/app/rows/search/sqs.ts index 8a61684020..f6154cf70c 100644 --- a/packages/server/src/sdk/app/rows/search/sqs.ts +++ b/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) { - fieldList = fieldList.concat( - buildInternalFieldList(relatedTable, tables) - ) - addJunctionFields(relatedTable, ["doc1.fieldName", "doc2.fieldName"]) + if (!relatedTable) { + continue } - } else { - fieldList.push(`${table._id}.${mapToUserColumn(col.name)}`) + const relatedFields = buildInternalFieldList(relatedTable, tables).concat( + 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[] = [] - queries.push(runSqlQuery(request, allTables, relationships)) - if (options.countRows) { - // get the total count of rows - queries.push( - runSqlQuery(request, allTables, relationships, { - countTotalRows: true, - }) - ) - } - const responses = await Promise.all(queries) - let rows = responses[0] as Row[] - const totalRows = - responses.length > 1 ? (responses[1] as number) : undefined + const [rows, totalRows] = await Promise.all([ + runSqlQuery(request, allTables, relationships), + options.countRows + ? runSqlQuery(request, allTables, relationships, { + countTotalRows: true, + }) + : Promise.resolve(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