diff --git a/packages/compass-data-modeling/src/components/diagram-editor.tsx b/packages/compass-data-modeling/src/components/diagram-editor.tsx index a6128564a9e..0fe597fafdc 100644 --- a/packages/compass-data-modeling/src/components/diagram-editor.tsx +++ b/packages/compass-data-modeling/src/components/diagram-editor.tsx @@ -128,7 +128,6 @@ const DiagramContent: React.FunctionComponent<{ isNewlyCreatedDiagram?: boolean; model: StaticModel | null; isInRelationshipDrawingMode: boolean; - editErrors?: string[]; newCollection?: string; onAddFieldToObjectField: (ns: string, parentPath: string[]) => void; onAddNewFieldToCollection: (ns: string) => void; @@ -526,7 +525,6 @@ export default connect( const { diagram, step } = state; return { step: step, - editErrors: diagram?.editErrors, diagramId: diagram?.id, }; }, diff --git a/packages/compass-data-modeling/src/store/apply-edit.ts b/packages/compass-data-modeling/src/store/apply-edit.ts index 2e34d273a39..0baa0846d55 100644 --- a/packages/compass-data-modeling/src/store/apply-edit.ts +++ b/packages/compass-data-modeling/src/store/apply-edit.ts @@ -32,6 +32,21 @@ function renameFieldInRelationshipSide( }; } +/** + * @param collections + * @param ns + * @throws Will throw an error if the namespace is not found in the collections. + */ +function assertCollectionExists( + collections: DataModelCollection[], + ns: string +): void { + const collection = collections.find((c) => c.ns === ns); + if (!collection) { + throw new Error(`Collection '${ns}' not found`); + } +} + export function applyEdit(edit: Edit, model?: StaticModel): StaticModel { if (edit.type === 'SetModel') { return edit.model; @@ -81,6 +96,7 @@ export function applyEdit(edit: Edit, model?: StaticModel): StaticModel { }; } case 'MoveCollection': { + assertCollectionExists(model.collections, edit.ns); return { ...model, collections: model.collections.map((collection) => { @@ -95,6 +111,7 @@ export function applyEdit(edit: Edit, model?: StaticModel): StaticModel { }; } case 'RemoveCollection': { + assertCollectionExists(model.collections, edit.ns); return { ...model, // Remove any relationships involving the collection being removed. @@ -109,6 +126,7 @@ export function applyEdit(edit: Edit, model?: StaticModel): StaticModel { }; } case 'RenameCollection': { + assertCollectionExists(model.collections, edit.fromNS); return { ...model, // Update relationships to point to the renamed namespace. @@ -137,6 +155,7 @@ export function applyEdit(edit: Edit, model?: StaticModel): StaticModel { }; } case 'UpdateCollectionNote': { + assertCollectionExists(model.collections, edit.ns); return { ...model, collections: model.collections.map((collection) => { @@ -151,6 +170,7 @@ export function applyEdit(edit: Edit, model?: StaticModel): StaticModel { }; } case 'AddField': { + assertCollectionExists(model.collections, edit.ns); return { ...model, collections: model.collections.map((collection) => { @@ -169,6 +189,7 @@ export function applyEdit(edit: Edit, model?: StaticModel): StaticModel { }; } case 'RemoveField': { + assertCollectionExists(model.collections, edit.ns); return { ...model, // Remove any relationships involving the field being removed. @@ -193,6 +214,7 @@ export function applyEdit(edit: Edit, model?: StaticModel): StaticModel { }; } case 'RenameField': { + assertCollectionExists(model.collections, edit.ns); return { ...model, // Update any relationships involving the field being renamed. @@ -227,6 +249,7 @@ export function applyEdit(edit: Edit, model?: StaticModel): StaticModel { }; } case 'ChangeFieldType': { + assertCollectionExists(model.collections, edit.ns); return { ...model, collections: model.collections.map((collection) => { diff --git a/packages/compass-data-modeling/src/store/diagram.spec.ts b/packages/compass-data-modeling/src/store/diagram.spec.ts index f17ae1d7bff..5c7d1f99bb5 100644 --- a/packages/compass-data-modeling/src/store/diagram.spec.ts +++ b/packages/compass-data-modeling/src/store/diagram.spec.ts @@ -18,6 +18,7 @@ import type { StaticModel, } from '../services/data-model-storage'; import { UUID } from 'bson'; +import Sinon from 'sinon'; const model: StaticModel = { collections: [ @@ -67,9 +68,11 @@ const loadedDiagram: MongoDBDataModelDescription = { describe('Data Modeling store', function () { let store: DataModelingStore; + let openToastSpy: Sinon.SinonSpy; beforeEach(function () { - store = setupStore(); + openToastSpy = Sinon.spy(); + store = setupStore({}, undefined, openToastSpy); }); describe('New Diagram', function () { @@ -157,7 +160,7 @@ describe('Data Modeling store', function () { const state = store.getState(); const diagram = getCurrentDiagramFromState(state); - expect(state.diagram?.editErrors).to.be.undefined; + expect(openToastSpy).not.to.have.been.called; expect(diagram.edits).to.have.length(2); expect(diagram.edits[0]).to.deep.equal(loadedDiagram.edits[0]); expect(diagram.edits[1]).to.deep.include(edit); @@ -193,7 +196,7 @@ describe('Data Modeling store', function () { const state = store.getState(); const diagram = getCurrentDiagramFromState(state); - expect(state.diagram?.editErrors).to.be.undefined; + expect(openToastSpy).not.to.have.been.called; expect(diagram.edits).to.have.length(2); expect(diagram.edits[0]).to.deep.equal(loadedDiagram.edits[0]); expect(diagram.edits[1]).to.deep.include({ @@ -217,9 +220,8 @@ describe('Data Modeling store', function () { } as unknown as Edit; store.dispatch(applyEdit(edit)); - const editErrors = store.getState().diagram?.editErrors; - expect(editErrors).to.have.length(1); - expect(editErrors && editErrors[0]).to.equal( + expect(openToastSpy).to.have.been.calledOnce; + expect(openToastSpy.firstCall.args[1].description).to.include( "'relationship,relationship' is required" ); const diagram = getCurrentDiagramFromState(store.getState()); @@ -353,7 +355,7 @@ describe('Data Modeling store', function () { const state = store.getState(); const diagram = getCurrentDiagramFromState(state); - expect(state.diagram?.editErrors).to.be.undefined; + expect(openToastSpy).not.to.have.been.called; expect(diagram.edits).to.have.length(2); expect(diagram.edits[0]).to.deep.equal(loadedDiagram.edits[0]); expect(diagram.edits[1]).to.deep.include(edit); @@ -373,12 +375,22 @@ describe('Data Modeling store', function () { } as unknown as Edit; store.dispatch(applyEdit(edit)); - const editErrors = store.getState().diagram?.editErrors; - expect(editErrors).to.have.length(1); - expect(editErrors && editErrors[0]).to.equal("'newPosition' is required"); + expect(openToastSpy).to.have.been.calledOnce; + expect(openToastSpy.firstCall.args[1].description).to.include( + "'newPosition' is required" + ); const diagram = getCurrentDiagramFromState(store.getState()); expect(diagram.edits).to.deep.equal(loadedDiagram.edits); }); + + it('should handle an invalid RenameCollection edit', function () { + store.dispatch(openDiagram(loadedDiagram)); + store.dispatch(renameCollection('nonExisting', 'newName')); + expect(openToastSpy).to.have.been.calledOnce; + expect(openToastSpy.firstCall.args[1].description).to.include( + "Collection 'nonExisting' not found" + ); + }); }); it('undo & redo', function () { diff --git a/packages/compass-data-modeling/src/store/diagram.ts b/packages/compass-data-modeling/src/store/diagram.ts index a6dc2849d77..cc8e4c85ab2 100644 --- a/packages/compass-data-modeling/src/store/diagram.ts +++ b/packages/compass-data-modeling/src/store/diagram.ts @@ -18,7 +18,7 @@ import { memoize } from 'lodash'; import type { DataModelingState, DataModelingThunkAction } from './reducer'; import { getCoordinatesForNewNode, - openToast, + type openToast as _openToast, showConfirmation, showPrompt, } from '@mongodb-js/compass-components'; @@ -59,7 +59,6 @@ export type DiagramState = current: [Edit, ...Edit[]]; next: Edit[][]; }; - editErrors?: string[]; selectedItems: SelectedItems | null; isNewlyCreated: boolean; draftCollection?: string; @@ -72,9 +71,9 @@ export enum DiagramActionTypes { RENAME_DIAGRAM = 'data-modeling/diagram/RENAME_DIAGRAM', APPLY_INITIAL_LAYOUT = 'data-modeling/diagram/APPLY_INITIAL_LAYOUT', APPLY_EDIT = 'data-modeling/diagram/APPLY_EDIT', - APPLY_EDIT_FAILED = 'data-modeling/diagram/APPLY_EDIT_FAILED', UNDO_EDIT = 'data-modeling/diagram/UNDO_EDIT', REDO_EDIT = 'data-modeling/diagram/REDO_EDIT', + REVERT_FAILED_EDIT = 'data-modeling/diagram/REVERT_FAILED_EDIT', COLLECTION_SELECTED = 'data-modeling/diagram/COLLECTION_SELECTED', RELATIONSHIP_SELECTED = 'data-modeling/diagram/RELATIONSHIP_SELECTED', FIELD_SELECTED = 'data-modeling/diagram/FIELD_SELECTED', @@ -102,15 +101,14 @@ export type ApplyEditAction = { edit: Edit; }; -export type ApplyEditFailedAction = { - type: DiagramActionTypes.APPLY_EDIT_FAILED; - errors: string[]; -}; - export type UndoEditAction = { type: DiagramActionTypes.UNDO_EDIT; }; +export type RevertFailedEditAction = { + type: DiagramActionTypes.REVERT_FAILED_EDIT; +}; + export type RedoEditAction = { type: DiagramActionTypes.REDO_EDIT; }; @@ -140,7 +138,7 @@ export type DiagramActions = | DeleteDiagramAction | RenameDiagramAction | ApplyEditAction - | ApplyEditFailedAction + | RevertFailedEditAction | UndoEditAction | RedoEditAction | CollectionSelectedAction @@ -227,7 +225,6 @@ export const diagramReducer: Reducer = ( state.draftCollection, action.edit.toNS ), - editErrors: undefined, updatedAt: new Date().toISOString(), selectedItems: { type: 'collection', @@ -244,7 +241,6 @@ export const diagramReducer: Reducer = ( current: [...state.edits.current, action.edit] as [Edit, ...Edit[]], next: [], }, - editErrors: undefined, updatedAt: new Date().toISOString(), selectedItems: updateSelectedItemsFromAppliedEdit( state.selectedItems, @@ -254,23 +250,23 @@ export const diagramReducer: Reducer = ( action.edit.type === 'AddCollection' ? action.edit.ns : undefined, }; } - if (isAction(action, DiagramActionTypes.APPLY_EDIT_FAILED)) { - return { - ...state, - editErrors: action.errors, - }; - } - if (isAction(action, DiagramActionTypes.UNDO_EDIT)) { + if ( + isAction(action, DiagramActionTypes.UNDO_EDIT) || + isAction(action, DiagramActionTypes.REVERT_FAILED_EDIT) + ) { const newCurrent = state.edits.prev.pop() || []; if (!isNonEmptyArray(newCurrent)) { return state; } + const next = isAction(action, DiagramActionTypes.REVERT_FAILED_EDIT) + ? state.edits.next + : [...state.edits.next, state.edits.current]; return { ...state, edits: { prev: [...state.edits.prev], current: newCurrent, - next: [...state.edits.next, state.edits.current], + next, }, updatedAt: new Date().toISOString(), }; @@ -503,7 +499,7 @@ export function redoEdit(): DataModelingThunkAction { export function onAddNestedField( ns: string, parentFieldPath: string[] -): DataModelingThunkAction { +): DataModelingThunkAction { return (dispatch, getState) => { const modelState = selectCurrentModelFromState(getState()); @@ -534,7 +530,7 @@ export function onAddNestedField( export function addNewFieldToCollection( ns: string -): DataModelingThunkAction { +): DataModelingThunkAction { return (dispatch, getState) => { const modelState = selectCurrentModelFromState(getState()); @@ -563,7 +559,7 @@ export function addNewFieldToCollection( export function moveCollection( ns: string, newPosition: [number, number] -): DataModelingThunkAction { +): DataModelingThunkAction { const edit: Omit< Extract, 'id' | 'timestamp' @@ -580,7 +576,7 @@ export function renameCollection( toNS: string ): DataModelingThunkAction< void, - ApplyEditAction | ApplyEditFailedAction | CollectionSelectedAction + ApplyEditAction | RevertFailedEditAction | CollectionSelectedAction > { const edit: Omit< Extract, @@ -594,27 +590,53 @@ export function renameCollection( return applyEdit(edit); } +function handleError( + openToast: typeof _openToast, + title: string, + messages: string[] +) { + openToast('data-modeling-error', { + variant: 'warning', + title, + description: messages.join(' '), + }); +} + +/** + * Not intended to be called directly, only exported for testing. + */ export function applyEdit( rawEdit: EditAction -): DataModelingThunkAction { - return (dispatch, getState, { dataModelStorage }) => { +): DataModelingThunkAction { + return (dispatch, getState, { dataModelStorage, openToast }) => { const edit = { ...rawEdit, id: new UUID().toString(), timestamp: new Date().toISOString(), }; - const { result: isValid, errors } = validateEdit(edit); - if (!isValid) { - dispatch({ - type: DiagramActionTypes.APPLY_EDIT_FAILED, - errors, - }); + const { result, errors } = validateEdit(edit); + let isValid = result; + if (!result) { + handleError(openToast, 'Could not apply changes', errors); return isValid; } dispatch({ type: DiagramActionTypes.APPLY_EDIT, edit, }); + + // try to build the model with the latest edit + try { + selectCurrentModelFromState(getState()); + } catch (e) { + handleError(openToast, 'Could not apply changes', [ + 'Something went wrong when applying the changes.', + (e as Error).message, + ]); + dispatch({ type: DiagramActionTypes.REVERT_FAILED_EDIT }); + isValid = false; + } + void dataModelStorage.save(getCurrentDiagramFromState(getState())); return isValid; }; @@ -672,7 +694,7 @@ export function renameDiagram( export function openDiagramFromFile( file: File ): DataModelingThunkAction, OpenDiagramAction> { - return async (dispatch, getState, { dataModelStorage, track }) => { + return async (dispatch, getState, { dataModelStorage, track, openToast }) => { try { const { name, edits } = await getDiagramContentsFromFile(file); @@ -692,18 +714,16 @@ export function openDiagramFromFile( track('Data Modeling Diagram Imported', {}); void dataModelStorage.save(diagram); } catch (error) { - openToast('data-modeling-file-read-error', { - variant: 'warning', - title: 'Error opening diagram', - description: (error as Error).message, - }); + handleError(openToast, 'Error opening diagram', [ + (error as Error).message, + ]); } }; } export function updateRelationship( relationship: Relationship -): DataModelingThunkAction { +): DataModelingThunkAction { return applyEdit({ type: 'UpdateRelationship', relationship, @@ -731,7 +751,7 @@ export function deleteRelationship( export function deleteCollection( ns: string -): DataModelingThunkAction { +): DataModelingThunkAction { return (dispatch, getState, { track }) => { track('Data Modeling Collection Removed', { source: 'side_panel', @@ -744,14 +764,14 @@ export function deleteCollection( export function updateCollectionNote( ns: string, note: string -): DataModelingThunkAction { +): DataModelingThunkAction { return applyEdit({ type: 'UpdateCollectionNote', ns, note }); } export function removeField( ns: string, field: FieldPath -): DataModelingThunkAction { +): DataModelingThunkAction { return (dispatch, getState, { track }) => { track('Data Modeling Field Removed', { source: 'side_panel', @@ -765,7 +785,7 @@ export function renameField( ns: string, field: FieldPath, newName: string -): DataModelingThunkAction { +): DataModelingThunkAction { return (dispatch, getState, { track }) => { track('Data Modeling Field Renamed', { source: 'side_panel', @@ -801,7 +821,7 @@ export function changeFieldType( ns: string, fieldPath: FieldPath, newTypes: string[] -): DataModelingThunkAction { +): DataModelingThunkAction { return (dispatch, getState, { track }) => { const collectionSchema = selectCurrentModelFromState( getState() @@ -869,7 +889,7 @@ export function addCollection( position?: [number, number] ): DataModelingThunkAction< void, - ApplyEditAction | ApplyEditFailedAction | CollectionSelectedAction + ApplyEditAction | RevertFailedEditAction | CollectionSelectedAction > { return (dispatch, getState, { track }) => { const existingCollections = selectCurrentModelFromState( diff --git a/packages/compass-data-modeling/src/store/index.ts b/packages/compass-data-modeling/src/store/index.ts index eb36b8b91a2..0896eddea4a 100644 --- a/packages/compass-data-modeling/src/store/index.ts +++ b/packages/compass-data-modeling/src/store/index.ts @@ -9,8 +9,11 @@ import reducer from './reducer'; import type { DataModelingExtraArgs } from './reducer'; import thunk from 'redux-thunk'; import type { ActivateHelpers } from '@mongodb-js/compass-app-registry'; +import { openToast as _openToast } from '@mongodb-js/compass-components'; -export type DataModelingStoreOptions = Record; +export type DataModelingStoreOptions = { + openToast?: typeof _openToast; +}; export type DataModelingStoreServices = { preferences: PreferencesAccess; @@ -22,7 +25,7 @@ export type DataModelingStoreServices = { }; export function activateDataModelingStore( - _: DataModelingStoreOptions, + { openToast = _openToast }: DataModelingStoreOptions, services: DataModelingStoreServices, { cleanup }: ActivateHelpers ) { @@ -35,6 +38,7 @@ export function activateDataModelingStore( ...services, cancelAnalysisControllerRef, cancelExportControllerRef, + openToast, }) ) ); diff --git a/packages/compass-data-modeling/src/store/reducer.ts b/packages/compass-data-modeling/src/store/reducer.ts index d7de974b1b2..2dafb926e99 100644 --- a/packages/compass-data-modeling/src/store/reducer.ts +++ b/packages/compass-data-modeling/src/store/reducer.ts @@ -20,6 +20,7 @@ import type { ExportDiagramActions, } from './export-diagram'; import { exportDiagramReducer } from './export-diagram'; +import { type openToast as _openToast } from '@mongodb-js/compass-components'; const reducer = combineReducers({ step: stepReducer, @@ -46,6 +47,7 @@ export type DataModelingState = ReturnType; export type DataModelingExtraArgs = DataModelingStoreServices & { cancelAnalysisControllerRef: { current: AbortController | null }; cancelExportControllerRef: { current: AbortController | null }; + openToast: typeof _openToast; }; export type DataModelingThunkAction = ThunkAction< diff --git a/packages/compass-data-modeling/test/setup-store.tsx b/packages/compass-data-modeling/test/setup-store.tsx index ae36d9d4eec..1b0d89ea509 100644 --- a/packages/compass-data-modeling/test/setup-store.tsx +++ b/packages/compass-data-modeling/test/setup-store.tsx @@ -10,6 +10,7 @@ import { activateDataModelingStore } from '../src/store'; import type { DataModelingStoreServices } from '../src/store'; import { noopDataModelStorageService } from '../src/provider'; import { Provider } from 'react-redux'; +import { openToast as _openToast } from '@mongodb-js/compass-components'; type ConnectionInfoWithMockData = ConnectionInfo & { databases: Array<{ @@ -136,10 +137,13 @@ const testConnections = [ export const setupStore = ( services: Partial = {}, - connections: ConnectionInfoWithMockData[] = testConnections + connections: ConnectionInfoWithMockData[] = testConnections, + openToast: typeof _openToast = _openToast ) => { return activateDataModelingStore( - {}, + { + openToast, + }, { logger: createNoopLogger('TEST'), track: createNoopTrack(),