From 3a519338010e2d73c10e319998d776c5483952bc Mon Sep 17 00:00:00 2001 From: mike12345567 Date: Wed, 1 Feb 2023 19:09:36 +0000 Subject: [PATCH] Re-working the error handling for the SQL relationship modal, as well as adding some better defaults for the majority of the options to make the UI a bit easier to use. --- .../Datasources/CreateEditRelationship.svelte | 263 ++++++++---------- .../backend/Datasources/relationshipErrors.js | 86 ++++++ 2 files changed, 208 insertions(+), 141 deletions(-) create mode 100644 packages/builder/src/components/backend/Datasources/relationshipErrors.js diff --git a/packages/builder/src/components/backend/Datasources/CreateEditRelationship.svelte b/packages/builder/src/components/backend/Datasources/CreateEditRelationship.svelte index e43437d756..68b51ef3a1 100644 --- a/packages/builder/src/components/backend/Datasources/CreateEditRelationship.svelte +++ b/packages/builder/src/components/backend/Datasources/CreateEditRelationship.svelte @@ -10,17 +10,17 @@ } from "@budibase/bbui" import { tables } from "stores/backend" import { Helpers } from "@budibase/bbui" + import { RelationshipErrorChecker } from "./relationshipErrors" + import { onMount } from "svelte" export let save export let datasource export let plusTables = [] export let fromRelationship = {} export let toRelationship = {} + export let selectedFromTable export let close - const colNotSet = "Please specify a column name" - const relationshipAlreadyExists = - "A relationship between these tables already exists." const relationshipTypes = [ { label: "One to Many", @@ -42,8 +42,11 @@ ) let tableOptions + let errorChecker = new RelationshipErrorChecker( + invalidThroughTable, + relationshipExists + ) let errors = {} - let hasClickedSave = !!fromRelationship.relationshipType let fromPrimary, fromForeign, fromTable, @@ -51,12 +54,19 @@ throughTable, fromColumn, toColumn - let fromId, toId, throughId, throughToKey, throughFromKey + let fromId = selectedFromTable?._id, + toId, + throughId, + throughToKey, + throughFromKey let isManyToMany, isManyToOne, relationshipType + let hasValidated = false $: { if (!fromPrimary) { fromPrimary = fromRelationship.foreignKey + } + if (!fromForeign) { fromForeign = toRelationship.foreignKey } if (!fromColumn && !errors.fromColumn) { @@ -77,7 +87,8 @@ throughToKey = fromRelationship.throughTo } if (!relationshipType) { - relationshipType = fromRelationship.relationshipType + relationshipType = + fromRelationship.relationshipType || RelationshipTypes.MANY_TO_ONE } } @@ -85,7 +96,7 @@ label: table.name, value: table._id, })) - $: valid = getErrorCount(errors) === 0 || !hasClickedSave + $: valid = getErrorCount(errors) === 0 && allRequiredAttributesSet() $: isManyToMany = relationshipType === RelationshipTypes.MANY_TO_MANY $: isManyToOne = relationshipType === RelationshipTypes.MANY_TO_ONE @@ -95,10 +106,20 @@ $: toRelationship.relationshipType = fromRelationship?.relationshipType - const getErrorCount = errors => - Object.entries(errors) + function getErrorCount(errors) { + return Object.entries(errors) .filter(entry => !!entry[1]) .map(entry => entry[0]).length + } + + function allRequiredAttributesSet() { + const base = fromTable && toTable && fromColumn && toColumn + if (isManyToOne) { + return base && fromPrimary && fromForeign + } else { + return base && throughTable && throughFromKey && throughToKey + } + } function invalidThroughTable() { // need to know the foreign key columns to check error @@ -118,93 +139,48 @@ } function validate() { - const isMany = relationshipType === RelationshipTypes.MANY_TO_MANY - const tableNotSet = "Please specify a table" - const foreignKeyNotSet = "Please pick a foreign key" + if (!allRequiredAttributesSet() && !hasValidated) { + return + } + hasValidated = true + errorChecker.setType(relationshipType) const errObj = {} - if (!relationshipType) { - errObj.relationshipType = "Please specify a relationship type" - } - if (!fromTable) { - errObj.fromTable = tableNotSet - } - if (!toTable) { - errObj.toTable = tableNotSet - } - if (isMany && !throughTable) { - errObj.throughTable = tableNotSet - } - if (isMany && !throughFromKey) { - errObj.throughFromKey = foreignKeyNotSet - } - if (isMany && !throughToKey) { - errObj.throughToKey = foreignKeyNotSet - } - if (invalidThroughTable()) { - errObj.throughTable = - "Ensure non-key columns are nullable or auto-generated" - } - if (!isMany && !fromForeign) { - errObj.fromForeign = foreignKeyNotSet - } - if (!fromColumn) { - errObj.fromColumn = colNotSet - } - if (!toColumn) { - errObj.toColumn = colNotSet - } - if (!isMany && !fromPrimary) { - errObj.fromPrimary = "Please pick the primary key" - } - if (isMany && relationshipExists()) { - errObj.fromTable = relationshipAlreadyExists - errObj.toTable = relationshipAlreadyExists - } - + errObj.relationshipType = errorChecker.relationshipTypeSet(relationshipType) + errObj.fromTable = errorChecker.tableSet(fromTable) + errObj.toTable = errorChecker.tableSet(toTable) + errObj.throughTable = errorChecker.throughTableSet(throughTable) + errObj.throughFromKey = errorChecker.manyForeignKeySet(throughFromKey) + errObj.throughToKey = errorChecker.manyForeignKeySet(throughToKey) + errObj.throughTable = errorChecker.throughIsNullable() + errObj.fromForeign = errorChecker.foreignKeySet(fromForeign) + errObj.fromPrimary = errorChecker.foreignKeySet(fromPrimary) + errObj.fromTable = errorChecker.doesRelationshipExists() + errObj.toTable = errorChecker.doesRelationshipExists() // 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)) { - errObj.fromTable = tableError - } - if (toTable && (toTable === fromTable || toTable === throughTable)) { - errObj.toTable = tableError - } - if ( - throughTable && - (throughTable === fromTable || throughTable === toTable) - ) { - errObj.throughTable = tableError - } - const colError = "Column name cannot be an existing column" - if (isColumnNameBeingUsed(toTable, fromColumn, originalFromColumnName)) { - errObj.fromColumn = colError - } - if (isColumnNameBeingUsed(fromTable, toColumn, originalToColumnName)) { - errObj.toColumn = colError - } - - let fromType, toType - if (fromPrimary && fromForeign) { - fromType = fromTable?.schema[fromPrimary]?.type - toType = toTable?.schema[fromForeign]?.type - } - if (fromType && toType && fromType !== toType) { - errObj.fromForeign = - "Column type of the foreign key must match the primary key" - } - + errObj.fromTable = errorChecker.differentTables(fromId, toId, throughId) + errObj.toTable = errorChecker.differentTables(toId, fromId, throughId) + errObj.throughTable = errorChecker.differentTables(throughId, fromId, toId) + errObj.fromColumn = errorChecker.columnBeingUsed( + toTable, + fromColumn, + originalFromColumnName + ) + errObj.toColumn = errorChecker.columnBeingUsed( + fromTable, + toColumn, + originalToColumnName + ) + errObj.fromForeign = errorChecker.typeMismatch( + fromTable, + toTable, + fromPrimary, + fromForeign + ) + console.log(errObj) errors = errObj return getErrorCount(errors) === 0 } - function isColumnNameBeingUsed(table, columnName, originalName) { - if (!table || !columnName || columnName === originalName) { - return false - } - const keys = Object.keys(table.schema).map(key => key.toLowerCase()) - return keys.indexOf(columnName.toLowerCase()) !== -1 - } - function buildRelationships() { const id = Helpers.uuid() //Map temporary variables @@ -320,7 +296,6 @@ } async function saveRelationship() { - hasClickedSave = true if (!validate()) { return false } @@ -342,6 +317,20 @@ await tables.fetch() close() } + + function changed(fn) { + if (fn && typeof fn === "function") { + fn() + } + validate() + } + + onMount(() => { + if (selectedFromTable) { + fromColumn = selectedFromTable.name + fromPrimary = selectedFromTable?.primary[0] || null + } + }) (errors.relationshipType = null)} + on:change={changed} />
Tables
- + changed(() => { + const table = plusTables.find(tbl => tbl._id === e.detail) + fromColumn = table?.name || "" + fromPrimary = table?.primary?.[0] + })} + /> + {/if} {#if isManyToOne && fromTable} { - toColumn = tableOptions.find(opt => opt.value === e.detail)?.label || "" - if (errors.toTable === relationshipAlreadyExists) { - errors.fromColumn = null - } - errors.toTable = null - errors.toColumn = null - errors.fromTable = null - errors.throughTable = null - }} + on:change={e => + changed(() => { + const table = plusTables.find(tbl => tbl._id === e.detail) + toColumn = table.name || "" + fromForeign = null + })} /> {#if isManyToMany} { - if (throughFromKey === e.detail) { - throughFromKey = null - } - errors.throughToKey = null - }} + on:change={e => + changed(() => { + if (throughFromKey === e.detail) { + throughFromKey = null + } + })} /> (errors.toColumn = e.detail?.length > 0 ? null : colNotSet)} + on:change={changed} />
{#if originalFromColumnName != null} diff --git a/packages/builder/src/components/backend/Datasources/relationshipErrors.js b/packages/builder/src/components/backend/Datasources/relationshipErrors.js new file mode 100644 index 0000000000..71fb634ada --- /dev/null +++ b/packages/builder/src/components/backend/Datasources/relationshipErrors.js @@ -0,0 +1,86 @@ +import { RelationshipTypes } from "constants/backend" + +export const typeMismatch = + "Column type of the foreign key must match the primary key" +export const columnCantExist = "Column name cannot be an existing column" +export const mustBeDifferentTables = "From/to/through tables must be different" +export const primaryKeyNotSet = "Please pick the primary key" +export const throughNotNullable = + "Ensure non-key columns are nullable or auto-generated" +export const noRelationshipType = "Please specify a relationship type" +export const tableNotSet = "Please specify a table" +export const foreignKeyNotSet = "Please pick a foreign key" +export const relationshipAlreadyExists = + "A relationship between these tables already exists" + +function isColumnNameBeingUsed(table, columnName, originalName) { + if (!table || !columnName || columnName === originalName) { + return false + } + const keys = Object.keys(table.schema).map(key => key.toLowerCase()) + return keys.indexOf(columnName.toLowerCase()) !== -1 +} + +export class RelationshipErrorChecker { + constructor(invalidThroughTableFn, relationshipExistsFn) { + this.invalidThroughTable = invalidThroughTableFn + this.relationshipExists = relationshipExistsFn + } + + setType(type) { + this.type = type + } + + isMany() { + return this.type === RelationshipTypes.MANY_TO_MANY + } + + relationshipTypeSet(type) { + return !type ? noRelationshipType : null + } + + tableSet(table) { + return !table ? tableNotSet : null + } + + throughTableSet(table) { + return this.isMany() && !table ? tableNotSet : null + } + + manyForeignKeySet(key) { + return this.isMany() && !key ? foreignKeyNotSet : null + } + + foreignKeySet(key) { + return !this.isMany() && !key ? foreignKeyNotSet : null + } + + throughIsNullable() { + return this.invalidThroughTable() ? throughNotNullable : null + } + + doesRelationshipExists() { + return this.isMany() && this.relationshipExists() + ? relationshipAlreadyExists + : null + } + + differentTables(table1, table2, table3) { + // currently don't support relationships back onto the table itself, needs to relate out + const error = table1 && (table1 === table2 || (table3 && table1 === table3)) + return error ? mustBeDifferentTables : null + } + + columnBeingUsed(table, column, ogName) { + return isColumnNameBeingUsed(table, column, ogName) ? columnCantExist : null + } + + typeMismatch(fromTable, toTable, primary, foreign) { + let fromType, toType + if (primary && foreign) { + fromType = fromTable?.schema[primary]?.type + toType = toTable?.schema[foreign]?.type + } + return fromType && toType && fromType !== toType ? typeMismatch : null + } +}