From 015b2a8f5667688d252f70849546350ae54fe93f Mon Sep 17 00:00:00 2001
From: mike12345567 <me@michaeldrury.co.uk>
Date: Tue, 26 Oct 2021 20:03:54 +0100
Subject: [PATCH] Fixing #2820 - SQL system no longer includes tables without a
 primary key constraint and it displays an error to handle this.

---
 packages/bbui/package.json                    |  1 +
 .../bbui/src/InlineAlert/InlineAlert.svelte   | 48 +++++++++++
 packages/bbui/src/index.js                    |  1 +
 packages/bbui/yarn.lock                       |  5 ++
 .../[selectedDatasource]/index.svelte         | 19 ++++-
 .../builder/src/stores/backend/datasources.js | 81 ++++++++-----------
 .../server/src/api/controllers/datasource.js  | 41 +++++++---
 packages/server/src/constants/index.js        |  4 +
 .../src/integrations/base/datasourcePlus.ts   |  8 ++
 packages/server/src/integrations/mysql.ts     | 17 ++--
 packages/server/src/integrations/postgres.ts  | 16 ++--
 packages/server/src/integrations/utils.ts     | 28 +++++--
 12 files changed, 188 insertions(+), 81 deletions(-)
 create mode 100644 packages/bbui/src/InlineAlert/InlineAlert.svelte
 create mode 100644 packages/server/src/integrations/base/datasourcePlus.ts

diff --git a/packages/bbui/package.json b/packages/bbui/package.json
index e244fefa4b..6383ddf845 100644
--- a/packages/bbui/package.json
+++ b/packages/bbui/package.json
@@ -51,6 +51,7 @@
     "@spectrum-css/fieldlabel": "^3.0.1",
     "@spectrum-css/icon": "^3.0.1",
     "@spectrum-css/illustratedmessage": "^3.0.2",
+    "@spectrum-css/inlinealert": "^2.0.1",
     "@spectrum-css/inputgroup": "^3.0.2",
     "@spectrum-css/label": "^2.0.10",
     "@spectrum-css/link": "^3.1.1",
diff --git a/packages/bbui/src/InlineAlert/InlineAlert.svelte b/packages/bbui/src/InlineAlert/InlineAlert.svelte
new file mode 100644
index 0000000000..d3775ca7fd
--- /dev/null
+++ b/packages/bbui/src/InlineAlert/InlineAlert.svelte
@@ -0,0 +1,48 @@
+<script>
+  import "@spectrum-css/inlinealert/dist/index-vars.css"
+  import Button from "../Button/Button.svelte"
+
+  export let type = "info"
+  export let header = ""
+  export let message = ""
+  export let onConfirm = undefined
+
+  $: icon = selectIcon(type)
+
+  function selectIcon(alertType) {
+    switch (alertType) {
+      case "error":
+      case "negative":
+        return "Alert"
+      case "success":
+        return "CheckmarkCircle"
+      case "help":
+        return "Help"
+      default:
+        return "Info"
+    }
+  }
+</script>
+
+<div
+  style="--spectrum-semantic-negative-border-color: #e34850;
+    --spectrum-semantic-positive-border-color: #2d9d78;
+    --spectrum-semantic-positive-icon-color: #2d9d78;
+    --spectrum-semantic-negative-icon-color: #e34850;"
+  class="spectrum-InLineAlert spectrum-InLineAlert--{type}"
+>
+  <svg
+    class="spectrum-Icon spectrum-Icon--sizeM spectrum-InLineAlert-icon"
+    focusable="false"
+    aria-hidden="true"
+  >
+    <use xlink:href="#spectrum-icon-18-{icon}" />
+  </svg>
+  <div class="spectrum-InLineAlert-header">{header}</div>
+  <div class="spectrum-InLineAlert-content">{message}</div>
+  {#if onConfirm}
+    <div class="spectrum-InLineAlert-footer">
+      <Button on:click={onConfirm}>OK</Button>
+    </div>
+  {/if}
+</div>
diff --git a/packages/bbui/src/index.js b/packages/bbui/src/index.js
index 08ece6b93c..377d451604 100644
--- a/packages/bbui/src/index.js
+++ b/packages/bbui/src/index.js
@@ -58,6 +58,7 @@ export { default as Pagination } from "./Pagination/Pagination.svelte"
 export { default as Badge } from "./Badge/Badge.svelte"
 export { default as StatusLight } from "./StatusLight/StatusLight.svelte"
 export { default as ColorPicker } from "./ColorPicker/ColorPicker.svelte"
+export { default as InlineAlert } from "./InlineAlert/InlineAlert.svelte"
 
 // Typography
 export { default as Body } from "./Typography/Body.svelte"
diff --git a/packages/bbui/yarn.lock b/packages/bbui/yarn.lock
index a492c83266..a102e6c148 100644
--- a/packages/bbui/yarn.lock
+++ b/packages/bbui/yarn.lock
@@ -136,6 +136,11 @@
   resolved "https://registry.yarnpkg.com/@spectrum-css/illustratedmessage/-/illustratedmessage-3.0.2.tgz#6a480be98b027e050b086e7899e40d87adb0a8c0"
   integrity sha512-dqnE8X27bGcO0HN8+dYx8O4o0dNNIAqeivOzDHhe2El+V4dTzMrNIerF6G0NLm3GjVf6XliwmitsZK+K6FmbtA==
 
+"@spectrum-css/inlinealert@^2.0.1":
+  version "2.0.1"
+  resolved "https://registry.yarnpkg.com/@spectrum-css/inlinealert/-/inlinealert-2.0.1.tgz#7521f88f6c845806403cc7d925773c7414e204a2"
+  integrity sha512-Xy5RCOwgurqUXuGQCsEDUduDd5408bmEpmFg+feynG7VFUgLFZWBeylSENB/OqjlFtO76PHXNVdHkhDscPIHTA==
+
 "@spectrum-css/inputgroup@^3.0.2":
   version "3.0.2"
   resolved "https://registry.yarnpkg.com/@spectrum-css/inputgroup/-/inputgroup-3.0.2.tgz#f1b13603832cbd22394f3d898af13203961f8691"
diff --git a/packages/builder/src/pages/builder/app/[application]/data/datasource/[selectedDatasource]/index.svelte b/packages/builder/src/pages/builder/app/[application]/data/datasource/[selectedDatasource]/index.svelte
index fc82e3a11c..e3d495f86d 100644
--- a/packages/builder/src/pages/builder/app/[application]/data/datasource/[selectedDatasource]/index.svelte
+++ b/packages/builder/src/pages/builder/app/[application]/data/datasource/[selectedDatasource]/index.svelte
@@ -1,6 +1,14 @@
 <script>
   import { goto } from "@roxi/routify"
-  import { Button, Heading, Body, Divider, Layout, Modal } from "@budibase/bbui"
+  import {
+    Button,
+    Heading,
+    Body,
+    Divider,
+    Layout,
+    Modal,
+    InlineAlert,
+  } from "@budibase/bbui"
   import { datasources, integrations, queries, tables } from "stores/backend"
   import { notifications } from "@budibase/bbui"
   import IntegrationConfigForm from "components/backend/DatasourceNavigator/TableIntegrationMenu/IntegrationConfigForm.svelte"
@@ -19,6 +27,7 @@
     ? Object.values(datasource.entities || {})
     : []
   $: relationships = getRelationships(plusTables)
+  $: schemaError = $datasources.schemaError
 
   function getRelationships(tables) {
     if (!tables || !Array.isArray(tables)) {
@@ -171,6 +180,14 @@
           your tables directly from the database and you can use them without
           having to write any queries at all.
         </Body>
+        {#if schemaError}
+          <InlineAlert
+            type="error"
+            header="Error fetching tables"
+            message={schemaError}
+            onConfirm={datasources.removeSchemaError}
+          />
+        {/if}
         <div class="query-list">
           {#each plusTables as table}
             <div class="query-list-item" on:click={() => onClickTable(table)}>
diff --git a/packages/builder/src/stores/backend/datasources.js b/packages/builder/src/stores/backend/datasources.js
index 7c74074e03..91719ae9b7 100644
--- a/packages/builder/src/stores/backend/datasources.js
+++ b/packages/builder/src/stores/backend/datasources.js
@@ -5,12 +5,35 @@ import api from "../../builderStore/api"
 export const INITIAL_DATASOURCE_VALUES = {
   list: [],
   selected: null,
+  schemaError: null,
 }
 
 export function createDatasourcesStore() {
   const store = writable(INITIAL_DATASOURCE_VALUES)
   const { subscribe, update, set } = store
 
+  async function updateDatasource(response) {
+    if (response.status !== 200) {
+      throw new Error(await response.text())
+    }
+
+    const { datasource, error } = await response.json()
+    update(state => {
+      const currentIdx = state.list.findIndex(ds => ds._id === datasource._id)
+
+      const sources = state.list
+
+      if (currentIdx >= 0) {
+        sources.splice(currentIdx, 1, datasource)
+      } else {
+        sources.push(datasource)
+      }
+
+      return { list: sources, selected: datasource._id, schemaError: error }
+    })
+    return datasource
+  }
+
   return {
     subscribe,
     update,
@@ -46,61 +69,20 @@ export function createDatasourcesStore() {
       let url = `/api/datasources/${datasource._id}/schema`
 
       const response = await api.post(url)
-      const json = await response.json()
-
-      if (response.status !== 200) {
-        throw new Error(json.message)
-      }
-
-      update(state => {
-        const currentIdx = state.list.findIndex(ds => ds._id === json._id)
-
-        const sources = state.list
-
-        if (currentIdx >= 0) {
-          sources.splice(currentIdx, 1, json)
-        } else {
-          sources.push(json)
-        }
-
-        return { list: sources, selected: json._id }
-      })
-      return json
+      return updateDatasource(response)
     },
-    save: async (datasource, fetchSchema = false) => {
+    save: async (body, fetchSchema = false) => {
       let response
-      if (datasource._id) {
-        response = await api.put(
-          `/api/datasources/${datasource._id}`,
-          datasource
-        )
+      if (body._id) {
+        response = await api.put(`/api/datasources/${body._id}`, body)
       } else {
         response = await api.post("/api/datasources", {
-          datasource: datasource,
+          datasource: body,
           fetchSchema,
         })
       }
 
-      const json = await response.json()
-
-      if (response.status !== 200) {
-        throw new Error(json.message)
-      }
-
-      update(state => {
-        const currentIdx = state.list.findIndex(ds => ds._id === json._id)
-
-        const sources = state.list
-
-        if (currentIdx >= 0) {
-          sources.splice(currentIdx, 1, json)
-        } else {
-          sources.push(json)
-        }
-
-        return { list: sources, selected: json._id }
-      })
-      return json
+      return updateDatasource(response)
     },
     delete: async datasource => {
       const response = await api.delete(
@@ -115,6 +97,11 @@ export function createDatasourcesStore() {
 
       return response
     },
+    removeSchemaError: () => {
+      update(state => {
+        return { ...state, schemaError: null }
+      })
+    },
   }
 }
 
diff --git a/packages/server/src/api/controllers/datasource.js b/packages/server/src/api/controllers/datasource.js
index 2ff7c7f9b8..e49a1c2465 100644
--- a/packages/server/src/api/controllers/datasource.js
+++ b/packages/server/src/api/controllers/datasource.js
@@ -7,6 +7,7 @@ const {
   BudibaseInternalDB,
   getTableParams,
 } = require("../../db/utils")
+const { BuildSchemaErrors } = require("../../constants")
 const { integrations } = require("../../integrations")
 const { makeExternalQuery } = require("./row/utils")
 
@@ -43,13 +44,17 @@ exports.buildSchemaFromDb = async function (ctx) {
   const db = new CouchDB(ctx.appId)
   const datasource = await db.get(ctx.params.datasourceId)
 
-  const tables = await buildSchemaHelper(datasource)
+  const { tables, error } = await buildSchemaHelper(datasource)
   datasource.entities = tables
 
-  const response = await db.put(datasource)
-  datasource._rev = response.rev
+  const dbResp = await db.put(datasource)
+  datasource._rev = dbResp.rev
 
-  ctx.body = datasource
+  const response = { datasource }
+  if (error) {
+    response.error = error
+  }
+  ctx.body = response
 }
 
 exports.update = async function (ctx) {
@@ -85,13 +90,15 @@ exports.save = async function (ctx) {
     ...ctx.request.body.datasource,
   }
 
+  let schemaError = null
   if (fetchSchema) {
-    let tables = await buildSchemaHelper(datasource)
+    const { tables, error } = await buildSchemaHelper(datasource)
+    schemaError = error
     datasource.entities = tables
   }
 
-  const response = await db.put(datasource)
-  datasource._rev = response.rev
+  const dbResp = await db.put(datasource)
+  datasource._rev = dbResp.rev
 
   // Drain connection pools when configuration is changed
   if (datasource.source) {
@@ -101,9 +108,11 @@ exports.save = async function (ctx) {
     }
   }
 
-  ctx.status = 200
-  ctx.message = "Datasource saved successfully."
-  ctx.body = datasource
+  const response = { datasource }
+  if (schemaError) {
+    response.error = schemaError
+  }
+  ctx.body = response
 }
 
 exports.destroy = async function (ctx) {
@@ -143,5 +152,15 @@ const buildSchemaHelper = async datasource => {
   await connector.buildSchema(datasource._id, datasource.entities)
   datasource.entities = connector.tables
 
-  return connector.tables
+  const errors = connector.schemaErrors
+  let error = null
+  if (errors && Object.keys(errors).length > 0) {
+    const noKeyTables = Object.entries(errors)
+      .filter(entry => entry[1] === BuildSchemaErrors.NO_KEY)
+      .map(([name]) => name)
+    error = `No primary key constraint found for the following: ${noKeyTables.join(
+      ", "
+    )}`
+  }
+  return { tables: connector.tables, error }
 }
diff --git a/packages/server/src/constants/index.js b/packages/server/src/constants/index.js
index 8c194e109c..7a8958c36a 100644
--- a/packages/server/src/constants/index.js
+++ b/packages/server/src/constants/index.js
@@ -152,5 +152,9 @@ exports.MetadataTypes = {
   AUTOMATION_TEST_HISTORY: "automationTestHistory",
 }
 
+exports.BuildSchemaErrors = {
+  NO_KEY: "no_key",
+}
+
 // pass through the list from the auth/core lib
 exports.ObjectStoreBuckets = ObjectStoreBuckets
diff --git a/packages/server/src/integrations/base/datasourcePlus.ts b/packages/server/src/integrations/base/datasourcePlus.ts
new file mode 100644
index 0000000000..371671afc0
--- /dev/null
+++ b/packages/server/src/integrations/base/datasourcePlus.ts
@@ -0,0 +1,8 @@
+import { Table } from "../../definitions/common"
+
+export interface DatasourcePlus {
+  tables: Record<string, Table>
+  schemaErrors: Record<string, string>
+
+  buildSchema(datasourceId: string, entities: Record<string, Table>): any
+}
diff --git a/packages/server/src/integrations/mysql.ts b/packages/server/src/integrations/mysql.ts
index 45c0bb6454..eba288da56 100644
--- a/packages/server/src/integrations/mysql.ts
+++ b/packages/server/src/integrations/mysql.ts
@@ -8,6 +8,7 @@ import {
 } from "../definitions/datasource"
 import { Table, TableSchema } from "../definitions/common"
 import { getSqlQuery } from "./utils"
+import { DatasourcePlus } from "./base/datasourcePlus"
 
 module MySQLModule {
   const mysql = require("mysql2")
@@ -15,7 +16,7 @@ module MySQLModule {
   const {
     buildExternalTableId,
     convertType,
-    copyExistingPropsOver,
+    finaliseExternalTables,
   } = require("./utils")
   const { FieldTypes } = require("../constants")
 
@@ -131,9 +132,11 @@ module MySQLModule {
     })
   }
 
-  class MySQLIntegration extends Sql {
+  class MySQLIntegration extends Sql implements DatasourcePlus {
     private config: MySQLConfig
     private readonly client: any
+    public tables: Record<string, Table> = {}
+    public schemaErrors: Record<string, string> = {}
 
     constructor(config: MySQLConfig) {
       super("mysql")
@@ -185,10 +188,6 @@ module MySQLModule {
             constraints,
           }
         }
-        // for now just default to first column
-        if (primaryKeys.length === 0) {
-          primaryKeys.push(descResp[0].Field)
-        }
         if (!tables[tableName]) {
           tables[tableName] = {
             _id: buildExternalTableId(datasourceId, tableName),
@@ -197,12 +196,12 @@ module MySQLModule {
             schema,
           }
         }
-
-        copyExistingPropsOver(tableName, tables, entities)
       }
 
       this.client.end()
-      this.tables = tables
+      const final = finaliseExternalTables(tables, entities)
+      this.tables = final.tables
+      this.schemaErrors = final.errors
     }
 
     async create(query: SqlQuery | string) {
diff --git a/packages/server/src/integrations/postgres.ts b/packages/server/src/integrations/postgres.ts
index abd1fa9286..dd7ecd5762 100644
--- a/packages/server/src/integrations/postgres.ts
+++ b/packages/server/src/integrations/postgres.ts
@@ -7,6 +7,7 @@ import {
 } from "../definitions/datasource"
 import { Table } from "../definitions/common"
 import { getSqlQuery } from "./utils"
+import { DatasourcePlus } from "./base/datasourcePlus"
 
 module PostgresModule {
   const { Pool } = require("pg")
@@ -15,7 +16,7 @@ module PostgresModule {
   const {
     buildExternalTableId,
     convertType,
-    copyExistingPropsOver,
+    finaliseExternalTables,
   } = require("./utils")
   const { escapeDangerousCharacters } = require("../utilities")
 
@@ -132,10 +133,12 @@ module PostgresModule {
     }
   }
 
-  class PostgresIntegration extends Sql {
+  class PostgresIntegration extends Sql implements DatasourcePlus {
     static pool: any
     private readonly client: any
     private readonly config: PostgresConfig
+    public tables: Record<string, Table> = {}
+    public schemaErrors: Record<string, string> = {}
 
     COLUMNS_SQL =
       "select * from information_schema.columns where not table_schema = 'information_schema' and not table_schema = 'pg_catalog'"
@@ -207,7 +210,7 @@ module PostgresModule {
         if (!tables[tableName] || !tables[tableName].schema) {
           tables[tableName] = {
             _id: buildExternalTableId(datasourceId, tableName),
-            primary: tableKeys[tableName] || ["id"],
+            primary: tableKeys[tableName] || [],
             name: tableName,
             schema: {},
           }
@@ -232,10 +235,9 @@ module PostgresModule {
         }
       }
 
-      for (let tableName of Object.keys(tables)) {
-        copyExistingPropsOver(tableName, tables, entities)
-      }
-      this.tables = tables
+      const final = finaliseExternalTables(tables, entities)
+      this.tables = final.tables
+      this.schemaErrors = final.errors
     }
 
     async create(query: SqlQuery | string) {
diff --git a/packages/server/src/integrations/utils.ts b/packages/server/src/integrations/utils.ts
index 07e4e950f8..4d139fa28b 100644
--- a/packages/server/src/integrations/utils.ts
+++ b/packages/server/src/integrations/utils.ts
@@ -1,8 +1,8 @@
 import { SqlQuery } from "../definitions/datasource"
-import { Datasource } from "../definitions/common"
+import { Datasource, Table } from "../definitions/common"
 import { SourceNames } from "../definitions/datasource"
 const { DocumentTypes, SEPARATOR } = require("../db/utils")
-const { FieldTypes } = require("../constants")
+const { FieldTypes, BuildSchemaErrors } = require("../constants")
 
 const DOUBLE_SEPARATOR = `${SEPARATOR}${SEPARATOR}`
 const ROW_ID_REGEX = /^\[.*]$/g
@@ -102,14 +102,14 @@ export function isIsoDateString(str: string) {
 }
 
 // add the existing relationships from the entities if they exist, to prevent them from being overridden
-export function copyExistingPropsOver(
+function copyExistingPropsOver(
   tableName: string,
-  tables: { [key: string]: any },
+  table: Table,
   entities: { [key: string]: any }
 ) {
   if (entities && entities[tableName]) {
     if (entities[tableName].primaryDisplay) {
-      tables[tableName].primaryDisplay = entities[tableName].primaryDisplay
+      table.primaryDisplay = entities[tableName].primaryDisplay
     }
     const existingTableSchema = entities[tableName].schema
     for (let key in existingTableSchema) {
@@ -117,8 +117,24 @@ export function copyExistingPropsOver(
         continue
       }
       if (existingTableSchema[key].type === "link") {
-        tables[tableName].schema[key] = existingTableSchema[key]
+        table.schema[key] = existingTableSchema[key]
       }
     }
   }
+  return table
+}
+
+export function finaliseExternalTables(tables: { [key: string]: any }, entities: { [key: string]: any }) {
+  const finalTables: { [key: string]: any } = {}
+  const errors: { [key: string]: string } = {}
+  for (let [name, table] of Object.entries(tables)) {
+    // make sure every table has a key
+    if (table.primary == null || table.primary.length === 0) {
+      errors[name] = BuildSchemaErrors.NO_KEY
+      continue
+    }
+    // make sure all previous props have been added back
+    finalTables[name] = copyExistingPropsOver(name, table, entities)
+  }
+  return { tables: finalTables, errors }
 }