Merge pull request #14630 from Budibase/fix/mysql-correlated-queries

MariaDB - avoid using correlated sub-queries

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

@@ -150,6 +150,7 @@ class InternalBuilder {
         return `"${str}"`
       case SqlClient.MS_SQL:
         return `[${str}]`
+      case SqlClient.MARIADB:
       case SqlClient.MY_SQL:
         return `\`${str}\``
     }
@@ -559,7 +560,10 @@ class InternalBuilder {
             )}${wrap}, FALSE)`
           )
         })
-      } else if (this.client === SqlClient.MY_SQL) {
+      } else if (
+        this.client === SqlClient.MY_SQL ||
+        this.client === SqlClient.MARIADB
+      ) {
         const jsonFnc = any ? "JSON_OVERLAPS" : "JSON_CONTAINS"
         iterate(mode, (q, key, value) => {
           return q[rawFnc](
@@ -930,7 +934,8 @@ class InternalBuilder {
       }
       const relatedTable = meta.tables?.[toTable]
       const toAlias = aliases?.[toTable] || toTable,
-        fromAlias = aliases?.[fromTable] || fromTable
+        fromAlias = aliases?.[fromTable] || fromTable,
+        throughAlias = (throughTable && aliases?.[throughTable]) || throughTable
       let toTableWithSchema = this.tableNameWithSchema(toTable, {
         alias: toAlias,
         schema: endpoint.schema,
@@ -957,38 +962,36 @@ class InternalBuilder {
       const primaryKey = `${toAlias}.${toPrimary || toKey}`
       let subQuery: Knex.QueryBuilder = knex
         .from(toTableWithSchema)
-        .limit(getRelationshipLimit())
         // add sorting to get consistent order
         .orderBy(primaryKey)
 
-      // many-to-many relationship with junction table
+      const isManyToMany = throughTable && toPrimary && fromPrimary
-      if (throughTable && toPrimary && fromPrimary) {
+      let correlatedTo = isManyToMany
-        const throughAlias = aliases?.[throughTable] || throughTable
+          ? `${throughAlias}.${fromKey}`
+          : `${toAlias}.${toKey}`,
+        correlatedFrom = isManyToMany
+          ? `${fromAlias}.${fromPrimary}`
+          : `${fromAlias}.${fromKey}`
+      // many-to-many relationship needs junction table join
+      if (isManyToMany) {
         let throughTableWithSchema = this.tableNameWithSchema(throughTable, {
           alias: throughAlias,
           schema: endpoint.schema,
         })
-        subQuery = subQuery
+        subQuery = subQuery.join(throughTableWithSchema, function () {
-          .join(throughTableWithSchema, function () {
+          this.on(`${toAlias}.${toPrimary}`, "=", `${throughAlias}.${toKey}`)
-            this.on(`${toAlias}.${toPrimary}`, "=", `${throughAlias}.${toKey}`)
+        })
-          })
-          .where(
-            `${throughAlias}.${fromKey}`,
-            "=",
-            knex.raw(this.quotedIdentifier(`${fromAlias}.${fromPrimary}`))
-          )
-      }
-      // one-to-many relationship with foreign key
-      else {
-        subQuery = subQuery.where(
-          `${toAlias}.${toKey}`,
-          "=",
-          knex.raw(this.quotedIdentifier(`${fromAlias}.${fromKey}`))
-        )
       }
 
+      // add the correlation to the overall query
+      subQuery = subQuery.where(
+        correlatedTo,
+        "=",
+        knex.raw(this.quotedIdentifier(correlatedFrom))
+      )
+
       const standardWrap = (select: string): Knex.QueryBuilder => {
-        subQuery = subQuery.select(`${toAlias}.*`)
+        subQuery = subQuery.select(`${toAlias}.*`).limit(getRelationshipLimit())
         // @ts-ignore - the from alias syntax isn't in Knex typing
         return knex.select(knex.raw(select)).from({
           [toAlias]: subQuery,
@@ -1008,11 +1011,15 @@ class InternalBuilder {
             `json_agg(json_build_object(${fieldList}))`
           )
           break
-        case SqlClient.MY_SQL:
+        case SqlClient.MARIADB:
+          // can't use the standard wrap due to correlated sub-query limitations in MariaDB
           wrapperQuery = subQuery.select(
-            knex.raw(`json_arrayagg(json_object(${fieldList}))`)
+            knex.raw(
+              `json_arrayagg(json_object(${fieldList}) LIMIT ${getRelationshipLimit()})`
+            )
           )
           break
+        case SqlClient.MY_SQL:
         case SqlClient.ORACLE:
           wrapperQuery = standardWrap(
             `json_arrayagg(json_object(${fieldList}))`
@@ -1024,7 +1031,9 @@ class InternalBuilder {
               .select(`${fromAlias}.*`)
               // @ts-ignore - from alias syntax not TS supported
               .from({
-                [fromAlias]: subQuery.select(`${toAlias}.*`),
+                [fromAlias]: subQuery
+                  .select(`${toAlias}.*`)
+                  .limit(getRelationshipLimit()),
               })} FOR JSON PATH))`
           )
           break
@@ -1179,7 +1188,8 @@ class InternalBuilder {
     if (
       this.client === SqlClient.POSTGRES ||
       this.client === SqlClient.SQL_LITE ||
-      this.client === SqlClient.MY_SQL
+      this.client === SqlClient.MY_SQL ||
+      this.client === SqlClient.MARIADB
     ) {
       const primary = this.table.primary
       if (!primary) {
@@ -1326,12 +1336,11 @@ class SqlQueryBuilder extends SqlTableQueryBuilder {
   _query(json: QueryJson, opts: QueryOptions = {}): SqlQuery | SqlQuery[] {
     const sqlClient = this.getSqlClient()
     const config: Knex.Config = {
-      client: sqlClient,
+      client: this.getBaseSqlClient(),
     }
     if (sqlClient === SqlClient.SQL_LITE || sqlClient === SqlClient.ORACLE) {
       config.useNullAsDefault = true
     }
-
     const client = knex(config)
     let query: Knex.QueryBuilder
     const builder = new InternalBuilder(sqlClient, client, json)
@@ -1440,7 +1449,10 @@ class SqlQueryBuilder extends SqlTableQueryBuilder {
       let id
       if (sqlClient === SqlClient.MS_SQL) {
         id = results?.[0].id
-      } else if (sqlClient === SqlClient.MY_SQL) {
+      } else if (
+        sqlClient === SqlClient.MY_SQL ||
+        sqlClient === SqlClient.MARIADB
+      ) {
         id = results?.insertId
       }
       row = processFn(

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

@@ -210,16 +210,27 @@ function buildDeleteTable(knex: SchemaBuilder, table: Table): SchemaBuilder {
 
 class SqlTableQueryBuilder {
   private readonly sqlClient: SqlClient
+  private extendedSqlClient: SqlClient | undefined
 
   // pass through client to get flavour of SQL
   constructor(client: SqlClient) {
     this.sqlClient = client
   }
 
-  getSqlClient(): SqlClient {
+  getBaseSqlClient(): SqlClient {
     return this.sqlClient
   }
 
+  getSqlClient(): SqlClient {
+    return this.extendedSqlClient || this.sqlClient
+  }
+
+  // if working in a database like MySQL with many variants (MariaDB)
+  // we can set another client which overrides the base one
+  setExtendedSqlClient(client: SqlClient) {
+    this.extendedSqlClient = client
+  }
+
   /**
    * @param json the input JSON structure from which an SQL query will be built.
    * @return the operation that was found in the JSON.

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

@@ -39,9 +39,10 @@ import tk from "timekeeper"
 import { encodeJSBinding } from "@budibase/string-templates"
 import { dataFilters } from "@budibase/shared-core"
 import { Knex } from "knex"
-import { structures } from "@budibase/backend-core/tests"
+import { generator, structures } from "@budibase/backend-core/tests"
 import { DEFAULT_EMPLOYEE_TABLE_SCHEMA } from "../../../db/defaultData/datasource_bb_default"
 import { generateRowIdField } from "../../../integrations/utils"
+import { cloneDeep } from "lodash/fp"
 
 describe.each([
   ["in-memory", undefined],
@@ -66,6 +67,36 @@ describe.each([
   let table: Table
   let rows: Row[]
 
+  async function basicRelationshipTables(type: RelationshipType) {
+    const relatedTable = await createTable(
+      {
+        name: { name: "name", type: FieldType.STRING },
+      },
+      generator.guid().substring(0, 10)
+    )
+    table = await createTable(
+      {
+        name: { name: "name", type: FieldType.STRING },
+        //@ts-ignore - API accepts this structure, will build out rest of definition
+        productCat: {
+          type: FieldType.LINK,
+          relationshipType: type,
+          name: "productCat",
+          fieldName: "product",
+          tableId: relatedTable._id!,
+          constraints: {
+            type: "array",
+          },
+        },
+      },
+      generator.guid().substring(0, 10)
+    )
+    return {
+      relatedTable: await config.api.table.get(relatedTable._id!),
+      table,
+    }
+  }
+
   beforeAll(async () => {
     await withCoreEnv({ TENANT_FEATURE_FLAGS: "*:SQS" }, () => config.init())
     if (isLucene) {
@@ -201,6 +232,7 @@ describe.each([
     // rows returned by the query will also cause the assertion to fail.
     async toMatchExactly(expectedRows: any[]) {
       const response = await this.performSearch()
+      const cloned = cloneDeep(response)
       const foundRows = response.rows
 
       // eslint-disable-next-line jest/no-standalone-expect
@@ -211,7 +243,7 @@ describe.each([
           expect.objectContaining(this.popRow(expectedRow, foundRows))
         )
       )
-      return response
+      return cloned
     }
 
     // Asserts that the query returns rows matching exactly the set of rows
@@ -219,6 +251,7 @@ describe.each([
     // cause the assertion to fail.
     async toContainExactly(expectedRows: any[]) {
       const response = await this.performSearch()
+      const cloned = cloneDeep(response)
       const foundRows = response.rows
 
       // eslint-disable-next-line jest/no-standalone-expect
@@ -231,7 +264,7 @@ describe.each([
           )
         )
       )
-      return response
+      return cloned
     }
 
     // Asserts that the query returns some property values - this cannot be used
@@ -239,6 +272,7 @@ describe.each([
     // typing for this has to be any, Jest doesn't expose types for matchers like expect.any(...)
     async toMatch(properties: Record<string, any>) {
       const response = await this.performSearch()
+      const cloned = cloneDeep(response)
       const keys = Object.keys(properties) as Array<keyof SearchResponse<Row>>
       for (let key of keys) {
         // eslint-disable-next-line jest/no-standalone-expect
@@ -248,17 +282,18 @@ describe.each([
           expect(response[key]).toEqual(properties[key])
         }
       }
-      return response
+      return cloned
     }
 
     // Asserts that the query doesn't return a property, e.g. pagination parameters.
     async toNotHaveProperty(properties: (keyof SearchResponse<Row>)[]) {
       const response = await this.performSearch()
+      const cloned = cloneDeep(response)
       for (let property of properties) {
         // eslint-disable-next-line jest/no-standalone-expect
         expect(response[property]).toBeUndefined()
       }
-      return response
+      return cloned
     }
 
     // Asserts that the query returns rows matching the set of rows passed in.
@@ -266,6 +301,7 @@ describe.each([
     // assertion to fail.
     async toContain(expectedRows: any[]) {
       const response = await this.performSearch()
+      const cloned = cloneDeep(response)
       const foundRows = response.rows
 
       // eslint-disable-next-line jest/no-standalone-expect
@@ -276,7 +312,7 @@ describe.each([
           )
         )
       )
-      return response
+      return cloned
     }
 
     async toFindNothing() {
@@ -2196,28 +2232,10 @@ describe.each([
       let productCategoryTable: Table, productCatRows: Row[]
 
       beforeAll(async () => {
-        productCategoryTable = await createTable(
+        const { relatedTable } = await basicRelationshipTables(
-          {
+          RelationshipType.ONE_TO_MANY
-            name: { name: "name", type: FieldType.STRING },
-          },
-          "productCategory"
-        )
-        table = await createTable(
-          {
-            name: { name: "name", type: FieldType.STRING },
-            productCat: {
-              type: FieldType.LINK,
-              relationshipType: RelationshipType.ONE_TO_MANY,
-              name: "productCat",
-              fieldName: "product",
-              tableId: productCategoryTable._id!,
-              constraints: {
-                type: "array",
-              },
-            },
-          },
-          "product"
         )
+        productCategoryTable = relatedTable
 
         productCatRows = await Promise.all([
           config.api.row.save(productCategoryTable._id!, { name: "foo" }),
@@ -2250,7 +2268,7 @@ describe.each([
 
       it("should be able to filter by relationship using table name", async () => {
         await expectQuery({
-          equal: { ["productCategory.name"]: "foo" },
+          equal: { [`${productCategoryTable.name}.name`]: "foo" },
         }).toContainExactly([
           { name: "foo", productCat: [{ _id: productCatRows[0]._id }] },
         ])
@@ -2262,6 +2280,36 @@ describe.each([
         }).toContainExactly([{ name: "baz", productCat: undefined }])
       })
     })
+
+  isSql &&
+    describe("big relations", () => {
+      beforeAll(async () => {
+        const { relatedTable } = await basicRelationshipTables(
+          RelationshipType.MANY_TO_ONE
+        )
+        const mainRow = await config.api.row.save(table._id!, {
+          name: "foo",
+        })
+        for (let i = 0; i < 11; i++) {
+          await config.api.row.save(relatedTable._id!, {
+            name: i,
+            product: [mainRow._id!],
+          })
+        }
+      })
+
+      it("can only pull 10 related rows", async () => {
+        await withCoreEnv({ SQL_MAX_RELATED_ROWS: "10" }, async () => {
+          const response = await expectQuery({}).toContain([{ name: "foo" }])
+          expect(response.rows[0].productCat).toBeArrayOfSize(10)
+        })
+      })
+
+      it("can pull max rows when env not set (defaults to 500)", async () => {
+        const response = await expectQuery({}).toContain([{ name: "foo" }])
+        expect(response.rows[0].productCat).toBeArrayOfSize(11)
+      })
+    })
   ;(isSqs || isLucene) &&
     describe("relations to same table", () => {
       let relatedTable: Table, relatedRows: Row[]

packages/server/src/integrations/mysql.ts

@@ -241,6 +241,16 @@ class MySQLIntegration extends Sql implements DatasourcePlus {
 
   async connect() {
     this.client = await mysql.createConnection(this.config)
+    const res = await this.internalQuery(
+      {
+        sql: "SELECT VERSION();",
+      },
+      { connect: false }
+    )
+    const version = res?.[0]?.["VERSION()"]
+    if (version?.toLowerCase().includes("mariadb")) {
+      this.setExtendedSqlClient(SqlClient.MARIADB)
+    }
   }
 
   async disconnect() {

packages/server/src/sdk/app/tables/external/index.ts

@@ -198,12 +198,15 @@ export async function save(
       }
     }
     generateRelatedSchema(schema, relatedTable, tableToSave, relatedColumnName)
+    tables[relatedTable.name] = relatedTable
     schema.main = true
   }
 
   // add in the new table for relationship purposes
   tables[tableToSave.name] = tableToSave
-  cleanupRelationships(tableToSave, tables, oldTable)
+  if (oldTable) {
+    cleanupRelationships(tableToSave, tables, { oldTable })
+  }
 
   const operation = tableId ? Operation.UPDATE_TABLE : Operation.CREATE_TABLE
   await makeTableRequest(
@@ -231,7 +234,10 @@ export async function save(
   // remove the rename prop
   delete tableToSave._rename
 
-  datasource.entities[tableToSave.name] = tableToSave
+  datasource.entities = {
+    ...datasource.entities,
+    ...tables,
+  }
 
   // store it into couch now for budibase reference
   await db.put(populateExternalTableSchemas(datasource))
@@ -255,7 +261,7 @@ export async function destroy(datasourceId: string, table: Table) {
   const operation = Operation.DELETE_TABLE
   if (tables) {
     await makeTableRequest(datasource, operation, table, tables)
-    cleanupRelationships(table, tables)
+    cleanupRelationships(table, tables, { deleting: true })
     delete tables[table.name]
     datasource.entities = tables
   }

packages/server/src/sdk/app/tables/external/utils.ts

@@ -20,14 +20,26 @@ import { cloneDeep } from "lodash/fp"
 export function cleanupRelationships(
   table: Table,
   tables: Record<string, Table>,
-  oldTable?: Table
+  opts: { oldTable: Table }
-) {
+): void
+export function cleanupRelationships(
+  table: Table,
+  tables: Record<string, Table>,
+  opts: { deleting: boolean }
+): void
+export function cleanupRelationships(
+  table: Table,
+  tables: Record<string, Table>,
+  opts?: { oldTable?: Table; deleting?: boolean }
+): void {
+  const oldTable = opts?.oldTable
   const tableToIterate = oldTable ? oldTable : table
   // clean up relationships in couch table schemas
   for (let [key, schema] of Object.entries(tableToIterate.schema)) {
     if (
       schema.type === FieldType.LINK &&
-      (!oldTable || table.schema[key] == null)
+      (opts?.deleting || oldTable?.schema[key] != null) &&
+      table.schema[key] == null
     ) {
       const schemaTableId = schema.tableId
       const relatedTable = Object.values(tables).find(

packages/server/src/tests/utilities/structures.ts

@@ -600,10 +600,10 @@ export function fullSchemaWithoutLinks({
   allRequired,
 }: {
   allRequired?: boolean
-}) {
+}): {
-  const schema: {
+  [type in Exclude<FieldType, FieldType.LINK>]: FieldSchema & { type: type }
-    [type in Exclude<FieldType, FieldType.LINK>]: FieldSchema & { type: type }
+} {
-  } = {
+  return {
     [FieldType.STRING]: {
       name: "string",
       type: FieldType.STRING,
@@ -741,8 +741,6 @@ export function fullSchemaWithoutLinks({
       },
     },
   }
-
-  return schema
 }
 export function basicAttachment() {
   return {

packages/types/src/sdk/search.ts

@@ -195,6 +195,7 @@ export enum SqlClient {
   MS_SQL = "mssql",
   POSTGRES = "pg",
   MY_SQL = "mysql2",
+  MARIADB = "mariadb",
   ORACLE = "oracledb",
   SQL_LITE = "sqlite3",
 }