Merge pull request #1932 from Budibase/fix/sql-relationship-validation

Adding validation to SQL relationship modal

packages/builder/src/pages/builder/app/[application]/data/datasource/[selectedDatasource]/CreateEditRelationship/CreateEditRelationship.svelte

@@ -1,8 +1,16 @@
 <script>
   import { RelationshipTypes } from "constants/backend"
-  import { Button, Input, ModalContent, Select, Detail } from "@budibase/bbui"
+  import {
+    Button,
+    Input,
+    ModalContent,
+    Select,
+    Detail,
+    Body,
+  } from "@budibase/bbui"
   import { tables } from "stores/backend"
   import { uuid } from "builderStore/uuid"
+  import { writable } from "svelte/store"
 
   export let save
   export let datasource
@@ -14,16 +22,68 @@
   let originalFromName = fromRelationship.name,
     originalToName = toRelationship.name
 
-  function isValid(relationship) {
+  function inSchema(table, prop, ogName) {
-    if (
+    if (!table || !prop || prop === ogName) {
-      relationship.relationshipType === RelationshipTypes.MANY_TO_MANY &&
-      !relationship.through
-    ) {
       return false
     }
-    return (
+    const keys = Object.keys(table.schema).map(key => key.toLowerCase())
-      relationship.name && relationship.tableId && relationship.relationshipType
+    return keys.indexOf(prop.toLowerCase()) !== -1
-    )
+  }
+
+  const touched = writable({})
+
+  function checkForErrors(
+    fromTable,
+    toTable,
+    throughTable,
+    fromRelate,
+    toRelate
+  ) {
+    const isMany =
+      fromRelate.relationshipType === RelationshipTypes.MANY_TO_MANY
+    const tableNotSet = "Please specify a table"
+    const errors = {}
+    if ($touched.from && !fromTable) {
+      errors.from = tableNotSet
+    }
+    if ($touched.to && !toTable) {
+      errors.to = tableNotSet
+    }
+    if ($touched.through && isMany && !fromRelate.through) {
+      errors.through = tableNotSet
+    }
+    if ($touched.foreign && !isMany && !fromRelate.fieldName) {
+      errors.foreign = "Please pick the foreign key"
+    }
+    const colNotSet = "Please specify a column name"
+    if ($touched.fromCol && !fromRelate.name) {
+      errors.fromCol = colNotSet
+    }
+    if ($touched.toCol && !toRelate.name) {
+      errors.toCol = colNotSet
+    }
+    // currently don't support relationships back onto the table itself, needs to relate out
+    const tableError = "From/to/through tables must be different"
+    if (fromTable && (fromTable === toTable || fromTable === throughTable)) {
+      errors.from = tableError
+    }
+    if (toTable && (toTable === fromTable || toTable === throughTable)) {
+      errors.to = tableError
+    }
+    if (
+      throughTable &&
+      (throughTable === fromTable || throughTable === toTable)
+    ) {
+      errors.through = tableError
+    }
+    const colError = "Column name cannot be an existing column"
+    if (inSchema(fromTable, fromRelate.name, originalFromName)) {
+      errors.fromCol = colError
+    }
+    if (inSchema(toTable, toRelate.name, originalToName)) {
+      errors.toCol = colError
+    }
+    return errors
   }
 
   $: tableOptions = plusTables.map(table => ({
@@ -33,7 +93,15 @@
   $: fromTable = plusTables.find(table => table._id === toRelationship?.tableId)
   $: toTable = plusTables.find(table => table._id === fromRelationship?.tableId)
   $: through = plusTables.find(table => table._id === fromRelationship?.through)
-  $: valid = toTable && fromTable && isValid(fromRelationship)
+  $: errors = checkForErrors(
+    fromTable,
+    toTable,
+    through,
+    fromRelationship,
+    toRelationship
+  )
+  $: valid =
+    Object.keys(errors).length === 0 && Object.keys($touched).length !== 0
   $: linkTable = through || toTable
   $: relationshipTypes = [
     {
@@ -155,31 +223,55 @@
   <Select
     label="Select from table"
     options={tableOptions}
+    on:change={() => ($touched.from = true)}
+    bind:error={errors.from}
     bind:value={toRelationship.tableId}
   />
   <Select
     label={"Select to table"}
     options={tableOptions}
+    on:change={() => ($touched.to = true)}
+    bind:error={errors.to}
     bind:value={fromRelationship.tableId}
   />
   {#if fromRelationship?.relationshipType === RelationshipTypes.MANY_TO_MANY}
     <Select
       label={"Through"}
       options={tableOptions}
+      on:change={() => ($touched.through = true)}
+      bind:error={errors.through}
       bind:value={fromRelationship.through}
     />
-  {:else if toTable}
+  {:else if fromRelationship?.relationshipType && toTable}
     <Select
       label={`Foreign Key (${toTable?.name})`}
-      options={Object.keys(toTable?.schema)}
+      options={Object.keys(toTable?.schema).filter(
+        field => toTable?.primary.indexOf(field) === -1
+      )}
+      on:change={() => ($touched.foreign = true)}
+      bind:error={errors.foreign}
       bind:value={fromRelationship.fieldName}
     />
   {/if}
   <div class="headings">
     <Detail>Column names</Detail>
   </div>
-  <Input label="From table column" bind:value={fromRelationship.name} />
+  <Body>
-  <Input label="To table column" bind:value={toRelationship.name} />
+    Budibase manages SQL relationships as a new column in the table, please
+    provide a name for these columns.
+  </Body>
+  <Input
+    on:blur={() => ($touched.fromCol = true)}
+    bind:error={errors.fromCol}
+    label="From table column"
+    bind:value={fromRelationship.name}
+  />
+  <Input
+    on:blur={() => ($touched.toCol = true)}
+    bind:error={errors.toCol}
+    label="To table column"
+    bind:value={toRelationship.name}
+  />
   <div slot="footer">
     {#if originalFromName != null}
       <Button warning text on:click={deleteRelationship}>Delete</Button>

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

@@ -162,7 +162,7 @@ module External {
         manyRelationships: ManyRelationship[] = []
       for (let [key, field] of Object.entries(table.schema)) {
         // if set already, or not set just skip it
-        if (!row[key] || newRow[key]) {
+        if (!row[key] || newRow[key] || field.autocolumn) {
           continue
         }
         // if its not a link then just copy it over

packages/server/src/integrations/mysql.ts

@@ -163,7 +163,7 @@ module MySQLModule {
         )
         for (let column of descResp) {
           const columnName = column.Field
-          if (column.Key === "PRI") {
+          if (column.Key === "PRI" && primaryKeys.indexOf(column.Key) === -1) {
             primaryKeys.push(columnName)
           }
           const constraints = {

packages/server/src/integrations/postgres.ts

@@ -147,7 +147,11 @@ module PostgresModule {
           if (!tableKeys[tableName]) {
             tableKeys[tableName] = []
           }
-          tableKeys[tableName].push(table.column_name || table.primary_key)
+          const key = table.column_name || table.primary_key
+          // only add the unique keys
+          if (key && tableKeys[tableName].indexOf(key) === -1) {
+            tableKeys[tableName].push(key)
+          }
         }
       } catch (err) {
         tableKeys = {}

packages/server/src/integrations/tests/postgres.spec.js

@@ -17,7 +17,7 @@ describe("Postgres Integration", () => {
 
   it("calls the create method with the correct params", async () => {
     const sql = "insert into users (name, age) values ('Joe', 123);"
-    const response = await config.integration.create({ 
+    await config.integration.create({
       sql
     })
     expect(pg.queryMock).toHaveBeenCalledWith(sql, {})
@@ -25,7 +25,7 @@ describe("Postgres Integration", () => {
 
   it("calls the read method with the correct params", async () => {
     const sql = "select * from users;"
-    const response = await config.integration.read({ 
+    await config.integration.read({
       sql
     })
     expect(pg.queryMock).toHaveBeenCalledWith(sql, {})