From ceaf7b514676a6e4a682c299cf0ca6418c0a6238 Mon Sep 17 00:00:00 2001 From: Ben Irvin Date: Fri, 27 Jan 2023 13:58:10 +0100 Subject: [PATCH 01/10] improve messaging --- packages/core/data-transfer/src/engine/index.ts | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/packages/core/data-transfer/src/engine/index.ts b/packages/core/data-transfer/src/engine/index.ts index 2787953bc7..23a68ba19a 100644 --- a/packages/core/data-transfer/src/engine/index.ts +++ b/packages/core/data-transfer/src/engine/index.ts @@ -346,7 +346,7 @@ class TransferEngine< keys.forEach((key) => { const sourceSchema = sourceSchemas[key]; const destinationSchema = destinationSchemas[key]; - const schemaDiffs = compareSchemas(destinationSchema, sourceSchema, strategy); + const schemaDiffs = compareSchemas(sourceSchema, destinationSchema, strategy); if (schemaDiffs.length) { diffs[key] = schemaDiffs; @@ -364,15 +364,15 @@ class TransferEngine< const path = diff.path.join('.'); if (diff.kind === 'added') { - return `Added "${path}": "${diff.value}" (${diff.type})`; + return `${path} exists in destination schema but not in source schema`; } if (diff.kind === 'deleted') { - return `Removed "${path}"`; + return `${path} exists in source schema but not in destination schema`; } if (diff.kind === 'modified') { - return `Modified "${path}": "${diff.values[0]}" (${diff.types[0]}) => "${diff.values[1]}" (${diff.types[1]})`; + return `Schema value changed at "${path}": "${diff.values[0]}" (${diff.types[0]}) => "${diff.values[1]}" (${diff.types[1]})`; } throw new TransferEngineValidationError(`Invalid diff found for "${uid}"`, { From 94898aba59c9747625f1f520f81ad95f58a0bcbb Mon Sep 17 00:00:00 2001 From: Ben Irvin Date: Fri, 27 Jan 2023 13:58:41 +0100 Subject: [PATCH 02/10] add check for loose equality --- .../src/engine/validation/schemas/index.ts | 19 +++--- packages/core/data-transfer/src/utils/json.ts | 59 ++++++++++++++----- 2 files changed, 55 insertions(+), 23 deletions(-) diff --git a/packages/core/data-transfer/src/engine/validation/schemas/index.ts b/packages/core/data-transfer/src/engine/validation/schemas/index.ts index 4d9f096235..c2720a6061 100644 --- a/packages/core/data-transfer/src/engine/validation/schemas/index.ts +++ b/packages/core/data-transfer/src/engine/validation/schemas/index.ts @@ -9,15 +9,18 @@ const strategies = { // Diffs allowed on specific attributes properties strict(diffs: Diff[]) { - const isIgnorableDiff = ({ path }: Diff) => { + const isIgnorableDiff = (diff: Diff) => { return ( - path.length === 3 && - // Root property must be attributes - path[0] === 'attributes' && - // Need a valid string attribute name - typeof path[1] === 'string' && - // The diff must be on ignorable attribute properties - ['private', 'required', 'configurable'].includes(path[2]) + // Ignore cases where one field is missing and the other is falsey + (diff.kind === 'dataType' && diff.types.includes('undefined')) || + // Ignore cases where... + (diff.path.length === 3 && + // Root property must be attributes + diff.path[0] === 'attributes' && + // Need a valid string attribute name + typeof diff.path[1] === 'string' && + // The diff must be on ignorable attribute properties + ['private', 'required', 'configurable'].includes(diff.path[2])) ); }; diff --git a/packages/core/data-transfer/src/utils/json.ts b/packages/core/data-transfer/src/utils/json.ts index d305ee1cab..c0bec3e997 100644 --- a/packages/core/data-transfer/src/utils/json.ts +++ b/packages/core/data-transfer/src/utils/json.ts @@ -19,12 +19,12 @@ export const diff = (a: unknown, b: unknown, ctx: Context = createContext()): Di // Define helpers const added = () => { - diffs.push({ kind: 'added', path, type: bType, value: b }); + diffs.push({ kind: 'added', path, types: [aType, bType], values: [a, b] }); return diffs; }; const deleted = () => { - diffs.push({ kind: 'deleted', path, type: aType, value: a }); + diffs.push({ kind: 'deleted', path, types: [aType, bType], values: [a, b] }); return diffs; }; @@ -38,13 +38,23 @@ export const diff = (a: unknown, b: unknown, ctx: Context = createContext()): Di return diffs; }; - if (aType === 'undefined') { - return added(); - } + const dataType = () => { + diffs.push({ + kind: 'dataType', + path, + types: [aType, bType], + values: [a, b], + }); + return diffs; + }; - if (bType === 'undefined') { - return deleted(); - } + const isLooselyEqual = () => { + // eslint-disable-next-line eqeqeq + if (a == b) { + return true; + } + return false; + }; if (isArray(a) && isArray(b)) { let k = 0; @@ -77,17 +87,29 @@ export const diff = (a: unknown, b: unknown, ctx: Context = createContext()): Di } if (!isEqual(a, b)) { + if (isLooselyEqual()) { + return dataType(); + } + + if (aType === 'undefined') { + return added(); + } + + if (bType === 'undefined') { + return deleted(); + } + return modified(); } return diffs; }; -export interface AddedDiff { +export interface AddedDiff { kind: 'added'; path: string[]; - type: string; - value: T; + types: [string, string]; + values: [T, P]; } export interface ModifiedDiff { @@ -97,14 +119,21 @@ export interface ModifiedDiff { values: [T, P]; } -export interface DeletedDiff { +export interface DeletedDiff { kind: 'deleted'; path: string[]; - type: string; - value: T; + types: [string, string]; + values: [T, P]; } -export type Diff = AddedDiff | ModifiedDiff | DeletedDiff; +export interface DataTypeDiff { + kind: 'dataType'; + path: string[]; + types: [string, string]; + values: [T, P]; +} + +export type Diff = AddedDiff | ModifiedDiff | DeletedDiff | DataTypeDiff; export interface Context { path: string[]; From 168ee1e2dd14cb1dce94fc5c434961c5cedbb596 Mon Sep 17 00:00:00 2001 From: Ben Irvin Date: Fri, 27 Jan 2023 14:06:33 +0100 Subject: [PATCH 03/10] add message for dataType diff --- packages/core/data-transfer/src/engine/index.ts | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/packages/core/data-transfer/src/engine/index.ts b/packages/core/data-transfer/src/engine/index.ts index 23a68ba19a..5666e9a853 100644 --- a/packages/core/data-transfer/src/engine/index.ts +++ b/packages/core/data-transfer/src/engine/index.ts @@ -375,6 +375,10 @@ class TransferEngine< return `Schema value changed at "${path}": "${diff.values[0]}" (${diff.types[0]}) => "${diff.values[1]}" (${diff.types[1]})`; } + if (diff.kind === 'dataType') { + return `Schema has differing data types at "${path}": "${diff.values[0]}" (${diff.types[0]}) => "${diff.values[1]}" (${diff.types[1]})`; + } + throw new TransferEngineValidationError(`Invalid diff found for "${uid}"`, { check: `schema on ${uid}`, }); From f9fa911bf85aabfe04cfdf10d59447e33a565e9b Mon Sep 17 00:00:00 2001 From: Ben Irvin Date: Fri, 27 Jan 2023 14:12:40 +0100 Subject: [PATCH 04/10] revert change to AddedDiff and DeletedDiff --- packages/core/data-transfer/src/utils/json.ts | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/packages/core/data-transfer/src/utils/json.ts b/packages/core/data-transfer/src/utils/json.ts index c0bec3e997..024612c233 100644 --- a/packages/core/data-transfer/src/utils/json.ts +++ b/packages/core/data-transfer/src/utils/json.ts @@ -105,11 +105,11 @@ export const diff = (a: unknown, b: unknown, ctx: Context = createContext()): Di return diffs; }; -export interface AddedDiff { +export interface AddedDiff { kind: 'added'; path: string[]; - types: [string, string]; - values: [T, P]; + type: string; + value: T; } export interface ModifiedDiff { @@ -119,11 +119,11 @@ export interface ModifiedDiff { values: [T, P]; } -export interface DeletedDiff { +export interface DeletedDiff { kind: 'deleted'; path: string[]; - types: [string, string]; - values: [T, P]; + type: string; + value: T; } export interface DataTypeDiff { From 7be8fc4d45ff4d1ae91d7de163028326651ef4db Mon Sep 17 00:00:00 2001 From: Ben Irvin Date: Fri, 27 Jan 2023 14:32:39 +0100 Subject: [PATCH 05/10] cleanup --- packages/core/data-transfer/src/utils/json.ts | 22 +++++++++---------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/packages/core/data-transfer/src/utils/json.ts b/packages/core/data-transfer/src/utils/json.ts index 024612c233..f804b380db 100644 --- a/packages/core/data-transfer/src/utils/json.ts +++ b/packages/core/data-transfer/src/utils/json.ts @@ -2,6 +2,14 @@ import { isArray, isObject, zip, isEqual, uniq } from 'lodash/fp'; const createContext = (): Context => ({ path: [] }); +const isLooselyEqual = (a: unknown, b: unknown) => { + // eslint-disable-next-line eqeqeq + if (a == b) { + return true; + } + return false; +}; + /** * Compute differences between two JSON objects and returns them * @@ -19,12 +27,12 @@ export const diff = (a: unknown, b: unknown, ctx: Context = createContext()): Di // Define helpers const added = () => { - diffs.push({ kind: 'added', path, types: [aType, bType], values: [a, b] }); + diffs.push({ kind: 'added', path, type: aType, value: a }); return diffs; }; const deleted = () => { - diffs.push({ kind: 'deleted', path, types: [aType, bType], values: [a, b] }); + diffs.push({ kind: 'deleted', path, type: bType, value: b }); return diffs; }; @@ -48,14 +56,6 @@ export const diff = (a: unknown, b: unknown, ctx: Context = createContext()): Di return diffs; }; - const isLooselyEqual = () => { - // eslint-disable-next-line eqeqeq - if (a == b) { - return true; - } - return false; - }; - if (isArray(a) && isArray(b)) { let k = 0; @@ -87,7 +87,7 @@ export const diff = (a: unknown, b: unknown, ctx: Context = createContext()): Di } if (!isEqual(a, b)) { - if (isLooselyEqual()) { + if (isLooselyEqual(a, b)) { return dataType(); } From 12b0807384d76962beee69dc8b7a10017edd6337 Mon Sep 17 00:00:00 2001 From: Ben Irvin Date: Fri, 27 Jan 2023 14:34:07 +0100 Subject: [PATCH 06/10] fix value of added and deleted --- packages/core/data-transfer/src/utils/json.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/core/data-transfer/src/utils/json.ts b/packages/core/data-transfer/src/utils/json.ts index f804b380db..583d2c88dc 100644 --- a/packages/core/data-transfer/src/utils/json.ts +++ b/packages/core/data-transfer/src/utils/json.ts @@ -27,12 +27,12 @@ export const diff = (a: unknown, b: unknown, ctx: Context = createContext()): Di // Define helpers const added = () => { - diffs.push({ kind: 'added', path, type: aType, value: a }); + diffs.push({ kind: 'added', path, type: bType, value: b }); return diffs; }; const deleted = () => { - diffs.push({ kind: 'deleted', path, type: bType, value: b }); + diffs.push({ kind: 'deleted', path, type: aType, value: a }); return diffs; }; From e027d73fe38655d0e2f6b90f6c6beaa770febe91 Mon Sep 17 00:00:00 2001 From: Ben Irvin Date: Mon, 30 Jan 2023 10:15:47 +0100 Subject: [PATCH 07/10] remove datatypediff --- .../core/data-transfer/src/engine/index.ts | 9 +++--- .../src/engine/validation/schemas/index.ts | 16 +++++----- packages/core/data-transfer/src/utils/json.ts | 31 +------------------ 3 files changed, 13 insertions(+), 43 deletions(-) diff --git a/packages/core/data-transfer/src/engine/index.ts b/packages/core/data-transfer/src/engine/index.ts index 5666e9a853..72c39170d5 100644 --- a/packages/core/data-transfer/src/engine/index.ts +++ b/packages/core/data-transfer/src/engine/index.ts @@ -372,11 +372,12 @@ class TransferEngine< } if (diff.kind === 'modified') { - return `Schema value changed at "${path}": "${diff.values[0]}" (${diff.types[0]}) => "${diff.values[1]}" (${diff.types[1]})`; - } + // eslint-disable-next-line eqeqeq + if (diff.values[0] == diff.values[1]) { + return `Schema has differing data types at "${path}": "${diff.values[0]}" (${diff.types[0]}) => "${diff.values[1]}" (${diff.types[1]})`; + } - if (diff.kind === 'dataType') { - return `Schema has differing data types at "${path}": "${diff.values[0]}" (${diff.types[0]}) => "${diff.values[1]}" (${diff.types[1]})`; + return `Schema value changed at "${path}": "${diff.values[0]}" (${diff.types[0]}) => "${diff.values[1]}" (${diff.types[1]})`; } throw new TransferEngineValidationError(`Invalid diff found for "${uid}"`, { diff --git a/packages/core/data-transfer/src/engine/validation/schemas/index.ts b/packages/core/data-transfer/src/engine/validation/schemas/index.ts index c2720a6061..830b6701dd 100644 --- a/packages/core/data-transfer/src/engine/validation/schemas/index.ts +++ b/packages/core/data-transfer/src/engine/validation/schemas/index.ts @@ -11,16 +11,14 @@ const strategies = { strict(diffs: Diff[]) { const isIgnorableDiff = (diff: Diff) => { return ( - // Ignore cases where one field is missing and the other is falsey - (diff.kind === 'dataType' && diff.types.includes('undefined')) || // Ignore cases where... - (diff.path.length === 3 && - // Root property must be attributes - diff.path[0] === 'attributes' && - // Need a valid string attribute name - typeof diff.path[1] === 'string' && - // The diff must be on ignorable attribute properties - ['private', 'required', 'configurable'].includes(diff.path[2])) + diff.path.length === 3 && + // Root property must be attributes + diff.path[0] === 'attributes' && + // Need a valid string attribute name + typeof diff.path[1] === 'string' && + // The diff must be on ignorable attribute properties + ['private', 'required', 'configurable'].includes(diff.path[2]) ); }; diff --git a/packages/core/data-transfer/src/utils/json.ts b/packages/core/data-transfer/src/utils/json.ts index 583d2c88dc..42001f48a2 100644 --- a/packages/core/data-transfer/src/utils/json.ts +++ b/packages/core/data-transfer/src/utils/json.ts @@ -2,14 +2,6 @@ import { isArray, isObject, zip, isEqual, uniq } from 'lodash/fp'; const createContext = (): Context => ({ path: [] }); -const isLooselyEqual = (a: unknown, b: unknown) => { - // eslint-disable-next-line eqeqeq - if (a == b) { - return true; - } - return false; -}; - /** * Compute differences between two JSON objects and returns them * @@ -46,16 +38,6 @@ export const diff = (a: unknown, b: unknown, ctx: Context = createContext()): Di return diffs; }; - const dataType = () => { - diffs.push({ - kind: 'dataType', - path, - types: [aType, bType], - values: [a, b], - }); - return diffs; - }; - if (isArray(a) && isArray(b)) { let k = 0; @@ -87,10 +69,6 @@ export const diff = (a: unknown, b: unknown, ctx: Context = createContext()): Di } if (!isEqual(a, b)) { - if (isLooselyEqual(a, b)) { - return dataType(); - } - if (aType === 'undefined') { return added(); } @@ -126,14 +104,7 @@ export interface DeletedDiff { value: T; } -export interface DataTypeDiff { - kind: 'dataType'; - path: string[]; - types: [string, string]; - values: [T, P]; -} - -export type Diff = AddedDiff | ModifiedDiff | DeletedDiff | DataTypeDiff; +export type Diff = AddedDiff | ModifiedDiff | DeletedDiff; export interface Context { path: string[]; From 71a4e6ed290d8ec7fb215b5e022b796b4fcf8c7a Mon Sep 17 00:00:00 2001 From: Ben Irvin Date: Mon, 30 Jan 2023 10:22:20 +0100 Subject: [PATCH 08/10] revert strict --- .../src/engine/validation/schemas/index.ts | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/packages/core/data-transfer/src/engine/validation/schemas/index.ts b/packages/core/data-transfer/src/engine/validation/schemas/index.ts index 830b6701dd..4d9f096235 100644 --- a/packages/core/data-transfer/src/engine/validation/schemas/index.ts +++ b/packages/core/data-transfer/src/engine/validation/schemas/index.ts @@ -9,16 +9,15 @@ const strategies = { // Diffs allowed on specific attributes properties strict(diffs: Diff[]) { - const isIgnorableDiff = (diff: Diff) => { + const isIgnorableDiff = ({ path }: Diff) => { return ( - // Ignore cases where... - diff.path.length === 3 && + path.length === 3 && // Root property must be attributes - diff.path[0] === 'attributes' && + path[0] === 'attributes' && // Need a valid string attribute name - typeof diff.path[1] === 'string' && + typeof path[1] === 'string' && // The diff must be on ignorable attribute properties - ['private', 'required', 'configurable'].includes(diff.path[2]) + ['private', 'required', 'configurable'].includes(path[2]) ); }; From 29d6b02ecff46e0ecdd39ad4f0f2212897076789 Mon Sep 17 00:00:00 2001 From: Ben Irvin Date: Mon, 30 Jan 2023 10:32:41 +0100 Subject: [PATCH 09/10] clean up logic --- packages/core/data-transfer/src/engine/index.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/core/data-transfer/src/engine/index.ts b/packages/core/data-transfer/src/engine/index.ts index 72c39170d5..4c15ed2960 100644 --- a/packages/core/data-transfer/src/engine/index.ts +++ b/packages/core/data-transfer/src/engine/index.ts @@ -373,11 +373,11 @@ class TransferEngine< if (diff.kind === 'modified') { // eslint-disable-next-line eqeqeq - if (diff.values[0] == diff.values[1]) { - return `Schema has differing data types at "${path}": "${diff.values[0]}" (${diff.types[0]}) => "${diff.values[1]}" (${diff.types[1]})`; + if (diff.types[0] === diff.types[1]) { + return `Schema value changed at "${path}": "${diff.values[0]}" (${diff.types[0]}) => "${diff.values[1]}" (${diff.types[1]})`; } - return `Schema value changed at "${path}": "${diff.values[0]}" (${diff.types[0]}) => "${diff.values[1]}" (${diff.types[1]})`; + return `Schema has differing data types at "${path}": "${diff.values[0]}" (${diff.types[0]}) => "${diff.values[1]}" (${diff.types[1]})`; } throw new TransferEngineValidationError(`Invalid diff found for "${uid}"`, { From 26e044a58b3e9a7babbcfb9230396ec76182c6bc Mon Sep 17 00:00:00 2001 From: Ben Irvin Date: Mon, 30 Jan 2023 10:34:46 +0100 Subject: [PATCH 10/10] remove eslint ignore --- packages/core/data-transfer/src/engine/index.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/core/data-transfer/src/engine/index.ts b/packages/core/data-transfer/src/engine/index.ts index 4c15ed2960..2f04bce86e 100644 --- a/packages/core/data-transfer/src/engine/index.ts +++ b/packages/core/data-transfer/src/engine/index.ts @@ -372,7 +372,6 @@ class TransferEngine< } if (diff.kind === 'modified') { - // eslint-disable-next-line eqeqeq if (diff.types[0] === diff.types[1]) { return `Schema value changed at "${path}": "${diff.values[0]}" (${diff.types[0]}) => "${diff.values[1]}" (${diff.types[1]})`; }