Add throwing exceptions during deserialization (#5200)

* Add exception definitions

* Add tests for throwing errors during deserialization

* Add actually throwing exceptions

* Cleanup

* Cleanup

* Fix tests

* fix: PR Comments

core/connection.js

@@ -280,11 +280,12 @@ Connection.prototype.onFailedConnect = function(_otherConnection) {
 /**
  * Connect this connection to another connection.
  * @param {!Connection} otherConnection Connection to connect to.
+ * @return {boolean} Whether the the blocks are now connected or not.
  */
 Connection.prototype.connect = function(otherConnection) {
   if (this.targetConnection == otherConnection) {
     // Already connected together.  NOP.
-    return;
+    return true;
   }
 
   const checker = this.getConnectionChecker();
@@ -305,6 +306,8 @@ Connection.prototype.connect = function(otherConnection) {
       Events.setGroup(false);
     }
   }
+
+  return this.isConnected();
 };
 
 /**

core/serialization/blocks.js

@@ -18,6 +18,8 @@ const Block = goog.requireType('Blockly.Block');
 // eslint-disable-next-line no-unused-vars
 const Connection = goog.requireType('Blockly.Connection');
 const Events = goog.require('Blockly.Events');
+const {MissingBlockType, MissingConnection, BadConnectionCheck} =
+    goog.require('Blockly.serialization.exceptions');
 // eslint-disable-next-line no-unused-vars
 const Workspace = goog.requireType('Blockly.Workspace');
 const Xml = goog.require('Blockly.Xml');
@@ -311,16 +313,15 @@ exports.load = load;
  * @return {!Block} The block that was just loaded.
  */
 const loadInternal = function(state, workspace, parentConnection = undefined) {
+  if (!state['type']) {
+    throw new MissingBlockType(state);
+  }
+
   const block = workspace.newBlock(state['type'], state['id']);
   loadCoords(block, state);
   loadAttributes(block, state);
   loadExtraState(block, state);
-  if (parentConnection &&
-      (block.outputConnection || block.previousConnection)) {
-    parentConnection.connect(
-        /** @type {!Connection} */
-        (block.outputConnection || block.previousConnection));
-  }
+  tryToConnectParent(parentConnection, block, state);
   // loadIcons(block, state);
   loadFields(block, state);
   loadInputBlocks(block, state);
@@ -383,6 +384,49 @@ const loadExtraState = function(block, state) {
   block.loadExtraState(state['extraState']);
 };
 
+/**
+ * Attempts to connect the block to the parent connection, if it exists.
+ * @param {(!Connection|undefined)} parentConnection The parent connnection to
+ *     try to connect the block to.
+ * @param {!Block} child The block to try to conecnt to the parent.
+ * @param {!State} state The state which defines the given block
+ */
+const tryToConnectParent = function(parentConnection, child, state) {
+  if (!parentConnection) {
+    return;
+  }
+  
+  let connected = false;
+  let childConnection;
+  if (parentConnection.type == inputTypes.VALUE) {
+    childConnection = child.outputConnection;
+    if (!childConnection) {
+      throw new MissingConnection('output', child, state);
+    }
+    connected = parentConnection.connect(childConnection);
+  } else { // Statement type.
+    childConnection = child.previousConnection;
+    if (!childConnection) {
+      throw new MissingConnection('previous', child, state);
+    }
+    connected = parentConnection.connect(childConnection);
+  }
+
+  if (!connected) {
+    const checker = child.workspace.connectionChecker;
+    throw new BadConnectionCheck(
+        checker.getErrorMessage(
+            checker.canConnectWithReason(
+                childConnection, parentConnection, false),
+            childConnection,
+            parentConnection),
+        parentConnection.type == inputTypes.VALUE ?
+            'output connection' : 'previous connection',
+        child,
+        state);
+  }
+};
+
 /**
  * Applies any field information available on the state object to the block.
  * @param {!Block} block The block to set the field state of.
@@ -420,9 +464,10 @@ const loadInputBlocks = function(block, state) {
   for (let i = 0; i < keys.length; i++) {
     const inputName = keys[i];
     const input = block.getInput(inputName);
-    if (input && input.connection) {
-      loadConnection(input.connection, state['inputs'][inputName]);
+    if (!input || !input.connection) {
+      throw new MissingConnection(inputName, block, state);
     }
+    loadConnection(input.connection, state['inputs'][inputName]);
   }
 };
 
@@ -436,9 +481,10 @@ const loadNextBlocks = function(block, state) {
   if (!state['next']) {
     return;
   }
-  if (block.nextConnection) {
-    loadConnection(block.nextConnection, state['next']);
+  if (!block.nextConnection) {
+    throw new MissingConnection('next', block, state);
   }
+  loadConnection(block.nextConnection, state['next']);
 };
 
 /**

core/serialization/exceptions.js

@@ -0,0 +1,128 @@
+
+/**
+ * @license
+ * Copyright 2021 Google LLC
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+/**
+ * @fileoverview Contains custom errors thrown by the serialization system.
+ */
+'use strict';
+
+goog.module('Blockly.serialization.exceptions');
+goog.module.declareLegacyNamespace();
+
+
+// eslint-disable-next-line no-unused-vars
+const {State} = goog.requireType('Blockly.serialization.blocks');
+
+class DeserializationError extends Error { }
+exports.DeserializationError = DeserializationError;
+
+/**
+ * Represents an error where the serialized state is expected to provide a
+ * block type, but it is not provided.
+ */
+class MissingBlockType extends DeserializationError {
+  /**
+   * @param {!State} state The state object which is missing the block type.
+   */
+  constructor(state) {
+    super(`Expected to find a 'type' property, defining the block type`);
+
+    /**
+     * The state object containing the bad name.
+     * @type {!State}
+     */
+    this.state = state;
+  }
+}
+exports.MissingBlockType = MissingBlockType;
+
+/**
+ * Represents an error where deserialization encountered a block that did
+ * not have a connection that was defined in the serialized state.
+ */
+class MissingConnection extends DeserializationError {
+  /**
+   * @param {string} connection The name of the connection that is missing. E.g.
+   *     'IF0', or 'next'.
+   * @param {!Blockly.Block} block The block missing the connection.
+   * @param {!State} state The state object containing the bad connection.
+   */
+  constructor(connection, block, state) {
+    super(`The block ${block.toDevString()} is missing a(n) ${connection}
+connection`);
+
+    /**
+     * The block missing the connection.
+     * @type {!Blockly.Block}
+     */
+    this.block = block;
+
+    /**
+     * The state object containing the bad name.
+     * @type {!State}
+     */
+    this.state = state;
+  }
+}
+exports.MissingConnection = MissingConnection;
+
+/**
+ * Represents an error where deserialization tried to connect two connections
+ * that were not compatible.
+ */
+class BadConnectionCheck extends DeserializationError {
+  /**
+   * @param {string} reason The reason the connections were not compatible.
+   * @param {string} childConnection The name of the incompatible child
+   *     connection. E.g. 'output' or 'previous'.
+   * @param {!Blockly.Block} childBlock The child block that could not connect
+   *     to its parent.
+   * @param {!State} childState The state object representing the child block.
+   */
+  constructor(reason, childConnection, childBlock, childState) {
+    super(`The block ${childBlock.toDevString()} could not connect its
+${childConnection} to its parent, because: ${reason}`);
+
+    /**
+     * The block that could not connect to its parent.
+     * @type {!Blockly.Block}
+     */
+    this.childBlock = childBlock;
+
+    /**
+     * The state object representing the block that could not connect to its
+     * parent.
+     * @type {!State}
+     */
+    this.childState = childState;
+  }
+}
+exports.BadConnectionCheck = BadConnectionCheck;
+
+/**
+ * Represents an error where deserialization encountered a real block as it
+ * was deserializing children of a shadow.
+ * This is an error because it is an invariant of Blockly that shadow blocks
+ * do not have real children.
+ */
+class RealChildOfShadow extends DeserializationError {
+  /**
+   * @param {!State} state The state object representing the real block.
+   */
+  constructor(state) {
+    super(`Encountered a real block which is defined as a child of a shadow
+block. It is an invariant of Blockly that shadow blocks only have shadow
+children`);
+
+    /**
+     * The state object representing the real block.
+     * @type {!State}
+     */
+    this.state = state;
+  }
+}
+exports.RealChildOfShadow = RealChildOfShadow;

tests/deps.js

@@ -196,7 +196,8 @@ goog.addDependency('../../core/renderers/zelos/renderer.js', ['Blockly.zelos.Ren
 goog.addDependency('../../core/requires.js', ['Blockly.requires'], ['Blockly', 'Blockly.Comment', 'Blockly.ContextMenuItems', 'Blockly.FieldAngle', 'Blockly.FieldCheckbox', 'Blockly.FieldColour', 'Blockly.FieldDropdown', 'Blockly.FieldImage', 'Blockly.FieldLabelSerializable', 'Blockly.FieldMultilineInput', 'Blockly.FieldNumber', 'Blockly.FieldTextInput', 'Blockly.FieldVariable', 'Blockly.FlyoutButton', 'Blockly.Generator', 'Blockly.HorizontalFlyout', 'Blockly.Mutator', 'Blockly.ShortcutItems', 'Blockly.Themes.Classic', 'Blockly.Toolbox', 'Blockly.Trashcan', 'Blockly.VariablesDynamic', 'Blockly.VerticalFlyout', 'Blockly.Warning', 'Blockly.ZoomControls', 'Blockly.geras.Renderer', 'Blockly.serialization.workspaces', 'Blockly.thrasos.Renderer', 'Blockly.zelos.Renderer']);
 goog.addDependency('../../core/scrollbar.js', ['Blockly.Scrollbar'], ['Blockly.Touch', 'Blockly.browserEvents', 'Blockly.utils', 'Blockly.utils.Coordinate', 'Blockly.utils.Svg', 'Blockly.utils.dom'], {'lang': 'es6', 'module': 'goog'});
 goog.addDependency('../../core/scrollbar_pair.js', ['Blockly.ScrollbarPair'], ['Blockly.Events', 'Blockly.Scrollbar', 'Blockly.utils.Svg', 'Blockly.utils.dom'], {'lang': 'es6', 'module': 'goog'});
-goog.addDependency('../../core/serialization/blocks.js', ['Blockly.serialization.blocks'], ['Blockly.Events', 'Blockly.Xml', 'Blockly.inputTypes'], {'lang': 'es6', 'module': 'goog'});
+goog.addDependency('../../core/serialization/blocks.js', ['Blockly.serialization.blocks'], ['Blockly.Events', 'Blockly.Xml', 'Blockly.inputTypes', 'Blockly.serialization.exceptions'], {'lang': 'es6', 'module': 'goog'});
+goog.addDependency('../../core/serialization/exceptions.js', ['Blockly.serialization.exceptions'], [], {'lang': 'es6', 'module': 'goog'});
 goog.addDependency('../../core/serialization/variables.js', ['Blockly.serialization.variables'], ['Blockly.Events'], {'lang': 'es6', 'module': 'goog'});
 goog.addDependency('../../core/serialization/workspaces.js', ['Blockly.serialization.workspaces'], ['Blockly.Events', 'Blockly.Workspace', 'Blockly.serialization.blocks', 'Blockly.serialization.variables', 'Blockly.utils.dom'], {'lang': 'es6', 'module': 'goog'});
 goog.addDependency('../../core/shortcut_items.js', ['Blockly.ShortcutItems'], ['Blockly.Gesture', 'Blockly.ShortcutRegistry', 'Blockly.clipboard', 'Blockly.common', 'Blockly.utils.KeyCodes'], {'lang': 'es6', 'module': 'goog'});

tests/mocha/block_test.js

@@ -1990,7 +1990,7 @@ suite('Blocks', function() {
       var recordUndoDuringInit;
       Blockly.Blocks['init_test_block'].init = function() {
         initCalled = true;
-        recordUndoDuringInit = Blockly.Events.recordUndo;
+        recordUndoDuringInit = Blockly.Events.getRecordUndo();
         throw new Error();
       };
       chai.assert.throws(function() {
@@ -1998,7 +1998,7 @@ suite('Blocks', function() {
       }.bind(this));
       chai.assert.isFalse(recordUndoDuringInit,
           'recordUndo should be false during block init function');
-      chai.assert.isTrue(Blockly.Events.recordUndo,
+      chai.assert.isTrue(Blockly.Events.getRecordUndo(),
           'recordUndo should be reset to true after init');
       chai.assert.isTrue(initCalled, 'expected init function to be called');
     });

tests/mocha/jso_deserialization_test.js

@@ -16,7 +16,6 @@ suite('JSO Deserialization', function() {
   });
 
   teardown(function() {
-    workspaceTeardown.call(this, this.workspace);
     sharedTestTeardown.call(this);
   });
 
@@ -464,4 +463,222 @@ suite('JSO Deserialization', function() {
       });
     });
   });
+
+  suite('Exceptions', function() {
+    setup(function() {
+      this.assertThrows = function(state, error) {
+        chai.assert.throws(() => {
+          Blockly.serialization.workspaces.load(state, this.workspace);
+        }, error);
+      };
+    });
+
+    suite('Undefined block type', function() {
+      test('Name', function() {
+        const state = {
+          'blocks': {
+            'blocks': [
+              {
+                'type': 'not_a_real_block',
+              }
+            ]
+          }
+        };
+        this.assertThrows(state, TypeError);
+      });
+
+      test('Casing', function() {
+        const state = {
+          'blocks': {
+            'blocks': [
+              {
+                'type': 'MATH_NUMBER',
+              }
+            ]
+          }
+        };
+        this.assertThrows(state, TypeError);
+      });
+    });
+
+    suite('Missing connection', function() {
+      test('Input name', function() {
+        const state = {
+          'blocks': {
+            'blocks': [
+              {
+                'type': 'logic_compare',
+                'inputs': {
+                  'not_an_input': {
+                    'block': {
+                      'type': 'logic_boolean'
+                    }
+                  }
+                }
+              }
+            ]
+          }
+        };
+        this.assertThrows(
+            state, Blockly.serialization.exceptions.MissingConnection);
+      });
+
+      test('Input casing', function() {
+        const state = {
+          'blocks': {
+            'blocks': [
+              {
+                'type': 'logic_compare',
+                'inputs': {
+                  'a': {
+                    'block': {
+                      'type': 'logic_boolean'
+                    }
+                  }
+                }
+              }
+            ]
+          }
+        };
+        this.assertThrows(
+            state, Blockly.serialization.exceptions.MissingConnection);
+      });
+
+      test('Next', function() {
+        const state = {
+          'blocks': {
+            'blocks': [
+              {
+                'type': 'logic_compare',
+                'next': {
+                  'block': {
+                    'type': 'text_print',
+                  }
+                }
+              }
+            ]
+          }
+        };
+        this.assertThrows(
+            state, Blockly.serialization.exceptions.MissingConnection);
+      });
+
+      test('Previous', function() {
+        const state = {
+          'blocks': {
+            'blocks': [
+              {
+                'type': 'text_print',
+                'next': {
+                  'block': {
+                    'type': 'logic_compare',
+                  }
+                }
+              }
+            ]
+          }
+        };
+        this.assertThrows(
+            state, Blockly.serialization.exceptions.MissingConnection);
+      });
+
+      test('Output', function() {
+        const state = {
+          'blocks': {
+            'blocks': [
+              {
+                'type': 'logic_compare',
+                'inputs': {
+                  'A': {
+                    'block': {
+                      'type': 'text_print'
+                    }
+                  }
+                }
+              }
+            ]
+          }
+        };
+        this.assertThrows(
+            state, Blockly.serialization.exceptions.MissingConnection);
+      });
+    });
+
+    suite('Bad connection check', function() {
+      test('Bad checks', function() {
+        const state = {
+          'blocks': {
+            'blocks': [
+              {
+                'type': 'logic_operation',
+                'inputs': {
+                  'A': {
+                    'block': {
+                      'type': 'math_number'
+                    }
+                  }
+                }
+              }
+            ]
+          }
+        };
+        this.assertThrows(
+            state, Blockly.serialization.exceptions.BadConnectionCheck);
+      });
+    });
+
+    suite.skip('Real child of shadow', function() {
+      test('Input', function() {
+        const state = {
+          'blocks': {
+            'blocks': [
+              {
+                'type': 'logic_compare',
+                'inputs': {
+                  'A': {
+                    'shadow': {
+                      'type': 'logic_compare',
+                      'inputs': {
+                        'A': {
+                          'block': {
+                            'type': 'logic_boolean',
+                          }
+                        }
+                      }
+                    }
+                  }
+                }
+              }
+            ]
+          }
+        };
+        this.assertThrows(
+            state, Blockly.serialization.exceptions.RealChildOfShadow);
+      });
+
+      test('Next', function() {
+        const state = {
+          'blocks': {
+            'blocks': [
+              {
+                'type': 'text_print',
+                'next': {
+                  'shadow': {
+                    'type': 'text_input',
+                    'next': {
+                      'block': {
+                        'type': 'text_input',
+                      }
+                    }
+                  }
+                }
+              }
+            ]
+          }
+        };
+        this.assertThrows(
+            state, Blockly.serialization.exceptions.RealChildOfShadow);
+      });
+    });
+  });
 });

tests/mocha/test_helpers.js

@@ -233,6 +233,7 @@ function sharedTestTeardown() {
     // Clear Blockly.Event state.
     Blockly.Events.setGroup(false);
     Blockly.Events.disabled_ = 0;
+    Blockly.Events.setRecordUndo(true);
     if (Blockly.Events.FIRE_QUEUE_.length) {
       // If this happens, it may mean that some previous test is missing cleanup
       // (i.e. a previous test added an event to the queue on a timeout that