From 91922aa571e615e4fb6e143a58964937166d9507 Mon Sep 17 00:00:00 2001 From: Beka Westberg Date: Tue, 17 Aug 2021 13:55:25 +0000 Subject: [PATCH] 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 | 5 +- core/serialization/blocks.js | 66 +++++-- core/serialization/exceptions.js | 128 ++++++++++++++ tests/deps.js | 3 +- tests/mocha/block_test.js | 4 +- tests/mocha/jso_deserialization_test.js | 219 +++++++++++++++++++++++- tests/mocha/test_helpers.js | 1 + 7 files changed, 411 insertions(+), 15 deletions(-) create mode 100644 core/serialization/exceptions.js diff --git a/core/connection.js b/core/connection.js index ae7ad8051..f2f8c3fd8 100644 --- a/core/connection.js +++ b/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(); }; /** diff --git a/core/serialization/blocks.js b/core/serialization/blocks.js index 9f55c4091..4267c295f 100644 --- a/core/serialization/blocks.js +++ b/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']); }; /** diff --git a/core/serialization/exceptions.js b/core/serialization/exceptions.js new file mode 100644 index 000000000..0ddb021a2 --- /dev/null +++ b/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; diff --git a/tests/deps.js b/tests/deps.js index 8fe9efacb..a96fa877c 100644 --- a/tests/deps.js +++ b/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'}); diff --git a/tests/mocha/block_test.js b/tests/mocha/block_test.js index 19d2f750b..5d07b0d88 100644 --- a/tests/mocha/block_test.js +++ b/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'); }); diff --git a/tests/mocha/jso_deserialization_test.js b/tests/mocha/jso_deserialization_test.js index 56670f849..f248dca3f 100644 --- a/tests/mocha/jso_deserialization_test.js +++ b/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); + }); + }); + }); }); diff --git a/tests/mocha/test_helpers.js b/tests/mocha/test_helpers.js index 62e9252b5..66b3ffb75 100644 --- a/tests/mocha/test_helpers.js +++ b/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