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

@@ -392,21 +392,28 @@ Blockly.jsonInitFactory_ = function(jsonDef) {
  * @param {!Array.<!Object>} 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)
-      };
     }
   }
 };

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];
+  }
+}

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;
+}