From ecc41372d7d873347f8b78ca885825d1df8832db Mon Sep 17 00:00:00 2001 From: Andrew n marshall Date: Wed, 11 Apr 2018 10:22:30 -0700 Subject: [PATCH] Fixing issue 1760: nulls in JSON array for block definitions. (#1768) JSON array with null or undefined value will now skip the offending item, and proceed to load following items. Added tests for null and undefined array elements. * Testing handling null and undefined id in block definition. * Adding test utility function to capture console warnings. * Test assumption that creating a simple block will not cause a warning. This is assumed when later checking warning counts. --- core/blockly.js | 29 +++++--- tests/jsunit/json_test.js | 119 +++++++++++++++++++++++++++++++-- tests/jsunit/test_utilities.js | 20 ++++++ 3 files changed, 153 insertions(+), 15 deletions(-) diff --git a/core/blockly.js b/core/blockly.js index bb2366cbe..f760caa54 100644 --- a/core/blockly.js +++ b/core/blockly.js @@ -392,21 +392,28 @@ Blockly.jsonInitFactory_ = function(jsonDef) { * @param {!Array.} jsonArray An array of JSON block definitions. */ Blockly.defineBlocksWithJsonArray = function(jsonArray) { - for (var i = 0, elem; elem = jsonArray[i]; i++) { - var typename = elem.type; - if (typename == null || typename === '') { + for (var i = 0; i < jsonArray.length; i++) { + var elem = jsonArray[i]; + if (!elem) { console.warn( - 'Block definition #' + i + - ' in JSON array is missing a type attribute. Skipping.'); + 'Block definition #' + i + ' in JSON array is ' + elem + '. ' + + 'Skipping.'); } else { - if (Blockly.Blocks[typename]) { + var typename = elem.type; + if (typename == null || typename === '') { console.warn( - 'Block definition #' + i + ' in JSON array' + - ' overwrites prior definition of "' + typename + '".'); + 'Block definition #' + i + + ' in JSON array is missing a type attribute. Skipping.'); + } else { + if (Blockly.Blocks[typename]) { + console.warn( + 'Block definition #' + i + ' in JSON array' + + ' overwrites prior definition of "' + typename + '".'); + } + Blockly.Blocks[typename] = { + init: Blockly.jsonInitFactory_(elem) + }; } - Blockly.Blocks[typename] = { - init: Blockly.jsonInitFactory_(elem) - }; } } }; diff --git a/tests/jsunit/json_test.js b/tests/jsunit/json_test.js index d489022b2..6832e151d 100644 --- a/tests/jsunit/json_test.js +++ b/tests/jsunit/json_test.js @@ -26,12 +26,17 @@ function test_json_minimal() { var workspace = new Blockly.Workspace(); var block; try { - Blockly.defineBlocksWithJsonArray([{ - "type": BLOCK_TYPE - }]); + var warnings = captureWarnings(function() { + Blockly.defineBlocksWithJsonArray([{ + "type": BLOCK_TYPE + }]); + block = new Blockly.Block(workspace, BLOCK_TYPE); + }); - block = new Blockly.Block(workspace, BLOCK_TYPE); assertEquals(BLOCK_TYPE, block.type); + assertEquals( + 'Expecting no warnings when defining and creating a simple block.', + warnings.length, 0); // TODO: asserts } finally { block.dispose(); @@ -40,6 +45,40 @@ function test_json_minimal() { } } +/** + * Ensure a block without a type id fails with a warning, but does not block + * later definitions. + */ +function test_json_nullOrUndefinedBlockTypeId() { + var BLOCK_TYPE1 = 'test_json_before_bad_blocks'; + var BLOCK_TYPE2 = 'test_json_after_bad_blocks'; + + assertUndefined(Blockly.Blocks[BLOCK_TYPE1]); + assertUndefined(Blockly.Blocks[BLOCK_TYPE2]); + var blockTypeCount = Object.keys(Blockly.Blocks).length; + + try { + var warnings = captureWarnings(function() { + Blockly.defineBlocksWithJsonArray([ + {"type": BLOCK_TYPE1}, + {"type": undefined}, + {"type": null}, + {"type": BLOCK_TYPE2}]); + }); + + assertNotNullNorUndefined('Block before bad blocks should be defined.', + Blockly.Blocks[BLOCK_TYPE1]); + assertNotNullNorUndefined('Block after bad blocks should be defined.', + Blockly.Blocks[BLOCK_TYPE2]); + assertEquals(Object.keys(Blockly.Blocks).length, blockTypeCount + 2); + assertEquals( + 'Expecting 2 warnings, one for each bad block.', warnings.length, 2); + } finally { + delete Blockly.Blocks[BLOCK_TYPE1]; + delete Blockly.Blocks[BLOCK_TYPE2]; + } +} + /** Ensure message0 creates an input. */ function test_json_message0() { var BLOCK_TYPE = 'test_json_message0'; @@ -257,3 +296,75 @@ function test_json_dropdown_image() { delete Blockly.Msg['ALTTEXT']; } } + +function test_defineBlocksWithJsonArray_nullItem() { + var BLOCK_TYPE1 = 'test_block_before_null'; + var BLOCK_TYPE2 = 'test_block_after_null'; + + assertUndefined(Blockly.Blocks[BLOCK_TYPE1]); + assertUndefined(Blockly.Blocks[BLOCK_TYPE2]); + var blockTypeCount = Object.keys(Blockly.Blocks).length; + + try { + var warnings = captureWarnings(function() { + Blockly.defineBlocksWithJsonArray([ + { + "type": BLOCK_TYPE1, + "message0": 'before' + }, + null, + { + "type": BLOCK_TYPE2, + "message0": 'after' + }]); + }); + assertNotNullNorUndefined( + 'Block before null in array should be defined.', + Blockly.Blocks[BLOCK_TYPE1]); + assertNotNullNorUndefined( + 'Block after null in array should be defined.', + Blockly.Blocks[BLOCK_TYPE2]); + assertEquals(Object.keys(Blockly.Blocks).length, blockTypeCount + 2); + assertEquals('Expected 1 warning for the bad block.', warnings.length, 1); + } finally { + workspace.dispose(); + delete Blockly.Blocks[BLOCK_TYPE1]; + delete Blockly.Blocks[BLOCK_TYPE2]; + } +} + +function test_defineBlocksWithJsonArray_undefinedItem() { + var BLOCK_TYPE1 = 'test_block_before_undefined'; + var BLOCK_TYPE2 = 'test_block_after_undefined'; + + assertUndefined(Blockly.Blocks[BLOCK_TYPE1]); + assertUndefined(Blockly.Blocks[BLOCK_TYPE2]); + var blockTypeCount = Object.keys(Blockly.Blocks).length; + + try { + var warnings = captureWarnings(function() { + Blockly.defineBlocksWithJsonArray([ + { + "type": BLOCK_TYPE1, + "message0": 'before' + }, + undefined, + { + "type": BLOCK_TYPE2, + "message0": 'after' + }]); + }); + assertNotNullNorUndefined( + 'Block before undefined in array should be defined.', + Blockly.Blocks[BLOCK_TYPE1]); + assertNotNullNorUndefined( + 'Block after undefined in array should be defined.', + Blockly.Blocks[BLOCK_TYPE2]); + assertEquals(Object.keys(Blockly.Blocks).length, blockTypeCount + 2); + assertEquals('Expected 1 warning for the bad block.', warnings.length, 1); + } finally { + workspace.dispose(); + delete Blockly.Blocks[BLOCK_TYPE1]; + delete Blockly.Blocks[BLOCK_TYPE2]; + } +} diff --git a/tests/jsunit/test_utilities.js b/tests/jsunit/test_utilities.js index 0c256e6f7..381e9134e 100644 --- a/tests/jsunit/test_utilities.js +++ b/tests/jsunit/test_utilities.js @@ -141,3 +141,23 @@ function defineGetVarBlock() { function undefineGetVarBlock() { delete Blockly.Blocks['get_var_block']; } + +/** + * Capture the strings sent to console.warn() when calling a function. + * @param {function} innerFunc The function where warnings may called. + * @return {string[]} The warning messages (only the first arguments). + */ +function captureWarnings(innerFunc) { + var msgs = []; + var nativeConsoleWarn = console.warn; + try { + console.warn = function(msg) { + msgs.push(msg); + nativeConsoleWarn.apply(console, arguments); + }; + innerFunc(); + } finally { + console.warn = nativeConsoleWarn; + } + return msgs; +}