Fix variable characters

src/engine/target.js

@@ -7,6 +7,7 @@ const uid = require('../util/uid');
 const {Map} = require('immutable');
 const log = require('../util/log');
 const StringUtil = require('../util/string-util');
+const VariableUtil = require('../util/variable-util');
 
 /**
  * @fileoverview
@@ -514,13 +515,7 @@ class Target extends EventEmitter {
             // for all references for a given ID instead of doing the below..?
             this.blocks.getAllVariableAndListReferences()[idToBeMerged];
 
-        referencesToChange.map(ref => {
-            ref.referencingField.id = idToMergeWith;
-            if (optNewName) {
-                ref.referencingField.value = optNewName;
-            }
-            return ref;
-        });
+        VariableUtil.updateVariableIdentifiers(referencesToChange, idToMergeWith, optNewName);
     }
 
     /**

src/serialization/sb2.js

@@ -199,7 +199,7 @@ const parseScripts = function (scripts, blocks, addBroadcastMsg, getVariableId,
  */
 const generateVariableIdGetter = (function () {
     let globalVariableNameMap = {};
-    const namer = (targetId, name, type) => `${targetId}-${name}-${type}`;
+    const namer = (targetId, name, type) => `${targetId}-${StringUtil.replaceUnsafeChars(name)}-${type}`;
     return function (targetId, topLevel) {
         // Reset the global variable map if topLevel
         if (topLevel) globalVariableNameMap = {};

src/serialization/sb3.js

@@ -14,6 +14,8 @@ const StageLayering = require('../engine/stage-layering');
 const log = require('../util/log');
 const uid = require('../util/uid');
 const MathUtil = require('../util/math-util');
+const StringUtil = require('../util/string-util');
+const VariableUtil = require('../util/variable-util');
 
 const {loadCostume} = require('../import/load-costume.js');
 const {loadSound} = require('../import/load-sound.js');
@@ -1075,6 +1077,12 @@ const deserializeMonitor = function (monitorData, runtime, targets, extensions)
         monitorBlockInfo && monitorBlockInfo.isSpriteSpecific) {
         monitorData.id = monitorBlockInfo.getId(
             monitorData.targetId, fields);
+    } else {
+        // Replace unsafe characters in monitor ID, if there are any.
+        // These would have come from projects that were originally 2.0 projects
+        // that had unsafe characters in the variable name (and then the name was
+        // used as part of the variable ID when importing the project).
+        monitorData.id = StringUtil.replaceUnsafeChars(monitorData.id);
     }
 
     // If the runtime already has a monitor block for this monitor's id,
@@ -1128,6 +1136,35 @@ const deserializeMonitor = function (monitorData, runtime, targets, extensions)
     runtime.requestAddMonitor(MonitorRecord(monitorData));
 };
 
+// Replace variable IDs throughout the project with
+// xml-safe versions.
+// This is to fix up projects imported from 2.0 where xml-unsafe names
+// were getting added to the variable ids.
+const replaceUnsafeCharsInVariableIds = function (targets) {
+    const allVarRefs = VariableUtil.getAllVarRefsForTargets(targets);
+    // Re-id the variables in the actual targets
+    targets.forEach(t => {
+        Object.keys(t.variables).forEach(id => {
+            const newId = StringUtil.replaceUnsafeChars(id);
+            if (newId === id) return;
+            t.variables[id].id = newId;
+            t.variables[newId] = t.variables[id];
+            delete t.variables[id];
+        });
+    });
+
+    // Replace the IDs in the blocks refrencing variables or lists
+    for (const id in allVarRefs) {
+        const newId = StringUtil.replaceUnsafeChars(id);
+        if (id === newId) continue; // ID was already safe, skip
+        // We're calling this on the stage target because we need a
+        // target to call on but this shouldn't matter because we're passing
+        // in all the varRefs we want to operate on
+        VariableUtil.updateVariableIdentifiers(allVarRefs[id], newId);
+    }
+    return targets;
+};
+
 /**
  * Deserialize the specified representation of a VM runtime and loads it into the provided runtime instance.
  * @param  {object} json - JSON representation of a VM runtime.
@@ -1171,6 +1208,7 @@ const deserialize = function (json, runtime, zip, isSingleSprite) {
                 delete t.targetPaneOrder;
                 return t;
             }))
+        .then(targets => replaceUnsafeCharsInVariableIds(targets))
         .then(targets => {
             monitorObjects.map(monitorDesc => deserializeMonitor(monitorDesc, runtime, targets, extensions));
             return targets;

src/util/string-util.js

@@ -57,6 +57,25 @@ class StringUtil {
             return value;
         });
     }
+    /**
+     * A function to replace unsafe characters (not allowed in XML) with safe ones. This is used
+     * in cases where we're replacing non-user facing strings (e.g. variable IDs).
+     * When replacing user facing strings, the xmlEscape utility function should be used
+     * instead so that the user facing string does not change how it displays.
+     * @param {!string} unsafe Unsafe string possibly containing unicode control characters.
+     * @return {string} String with control characters replaced.
+     */
+    static replaceUnsafeChars (unsafe) {
+        return unsafe.replace(/[<>&'"]/g, c => {
+            switch (c) {
+            case '<': return 'lt';
+            case '>': return 'gt';
+            case '&': return 'amp';
+            case '\'': return 'apos';
+            case '"': return 'quot';
+            }
+        });
+    }
 }
 
 module.exports = StringUtil;

src/util/variable-util.js

@@ -0,0 +1,47 @@
+class VariableUtil {
+    static _mergeVarRefObjects (accum, obj2) {
+        for (const id in obj2) {
+            if (accum[id]) {
+                accum[id] = accum[id].concat(obj2[id]);
+            } else {
+                accum[id] = obj2[id];
+            }
+        }
+        return accum;
+    }
+
+    /**
+     * Get all variable/list references in the given list of targets
+     * in the project.
+     * @param {Array.<Target>} targets The list of targets to get the variable
+     * and list references from.
+     * @return {object} An object with variable ids as the keys and a list of block fields referencing
+     * the variable.
+     */
+    static getAllVarRefsForTargets (targets) {
+        return targets
+            .map(t => t.blocks.getAllVariableAndListReferences())
+            .reduce(VariableUtil._mergeVarRefObjects, {});
+    }
+
+    /**
+     * Give all variable references provided a new id and possibly new name.
+     * @param {Array<object>} referencesToUpdate Context of the change, the object containing variable
+     * references to update.
+     * @param {string} newId ID of the variable that the old references should be replaced with
+     * @param {?string} optNewName New variable name to merge with. The old
+     * variable name in the references being updated should be replaced with this new name.
+     * If this parameter is not provided or is '', no name change occurs.
+     */
+    static updateVariableIdentifiers (referencesToUpdate, newId, optNewName) {
+        referencesToUpdate.map(ref => {
+            ref.referencingField.id = newId;
+            if (optNewName) {
+                ref.referencingField.value = optNewName;
+            }
+            return ref;
+        });
+    }
+}
+
+module.exports = VariableUtil;

test/integration/variable_special_chars_sb2.js

@@ -0,0 +1,135 @@
+const path = require('path');
+const test = require('tap').test;
+const makeTestStorage = require('../fixtures/make-test-storage');
+const readFileToBuffer = require('../fixtures/readProjectFile').readFileToBuffer;
+const VirtualMachine = require('../../src/index');
+const Variable = require('../../src/engine/variable');
+const StringUtil = require('../../src/util/string-util');
+const VariableUtil = require('../../src/util/variable-util');
+
+const projectUri = path.resolve(__dirname, '../fixtures/variable_characters.sb2');
+const project = readFileToBuffer(projectUri);
+
+test('importing sb2 project with special chars in variable names', t => {
+    const vm = new VirtualMachine();
+    vm.attachStorage(makeTestStorage());
+
+    // Evaluate playground data and exit
+    vm.on('playgroundData', e => {
+        const threads = JSON.parse(e.threads);
+        // All monitors should create threads that finish during the step and
+        // are revoved from runtime.threads.
+        t.equal(threads.length, 0);
+
+        // we care that the last step updated the right number of monitors
+        // we don't care whether the last step ran other threads or not
+        const lastStepUpdatedMonitorThreads = vm.runtime._lastStepDoneThreads.filter(thread => thread.updateMonitor);
+        t.equal(lastStepUpdatedMonitorThreads.length, 3);
+
+        t.equal(vm.runtime.targets.length, 3);
+
+        const stage = vm.runtime.targets[0];
+        const cat = vm.runtime.targets[1];
+        const bananas = vm.runtime.targets[2];
+
+        const allVarListFields = VariableUtil.getAllVarRefsForTargets(vm.runtime.targets);
+
+        const abVarId = Object.keys(stage.variables).filter(k => stage.variables[k].name === 'a&b')[0];
+        const abVar = stage.variables[abVarId];
+        const abMonitor = vm.runtime._monitorState.get(abVarId);
+        // Check for unsafe characters, replaceUnsafeChars should just result in the original string
+        // (e.g. there was nothing to replace)
+        // Check that the variable ID does not have any unsafe characters
+        t.equal(StringUtil.replaceUnsafeChars(abVarId), abVarId);
+        // Check that the monitor record ID does not have any unsafe characters
+        t.equal(StringUtil.replaceUnsafeChars(abMonitor.id), abMonitor.id);
+
+        // Check that the variable still has the correct info
+        t.equal(StringUtil.replaceUnsafeChars(abVar.id), abVar.id);
+        t.equal(abVar.id, abVarId);
+        t.equal(abVar.type, Variable.LIST_TYPE);
+        t.equal(abVar.value[0], 'thing');
+        t.equal(abVar.value[1], 'thing\'1');
+
+        // Find all the references for this list, and verify they have the correct ID
+        // There should be 3 fields, 2 on the stage, and one on the cat
+        t.equal(allVarListFields[abVarId].length, 3);
+        const stageBlocks = Object.keys(stage.blocks._blocks).map(blockId => stage.blocks._blocks[blockId]);
+        const stageListBlocks = stageBlocks.filter(block => block.fields.hasOwnProperty('LIST'));
+        t.equal(stageListBlocks.length, 2);
+        t.equal(stageListBlocks[0].fields.LIST.id, abVarId);
+        t.equal(stageListBlocks[1].fields.LIST.id, abVarId);
+        const catBlocks = Object.keys(cat.blocks._blocks).map(blockId => cat.blocks._blocks[blockId]);
+        const catListBlocks = catBlocks.filter(block => block.fields.hasOwnProperty('LIST'));
+        t.equal(catListBlocks.length, 1);
+        t.equal(catListBlocks[0].fields.LIST.id, abVarId);
+
+        const fooVarId = Object.keys(stage.variables).filter(k => stage.variables[k].name === '"foo')[0];
+        const fooVar = stage.variables[fooVarId];
+        const fooMonitor = vm.runtime._monitorState.get(fooVarId);
+        // Check for unsafe characters, replaceUnsafeChars should just result in the original string
+        // (e.g. there was nothing to replace)
+        // Check that the variable ID does not have any unsafe characters
+        t.equal(StringUtil.replaceUnsafeChars(fooVarId), fooVarId);
+        // Check that the monitor record ID does not have any unsafe characters
+        t.equal(StringUtil.replaceUnsafeChars(fooMonitor.id), fooMonitor.id);
+
+        // Check that the variable still has the correct info
+        t.equal(StringUtil.replaceUnsafeChars(fooVar.id), fooVar.id);
+        t.equal(fooVar.id, fooVarId);
+        t.equal(fooVar.type, Variable.SCALAR_TYPE);
+        t.equal(fooVar.value, 'foo');
+
+        // Find all the references for this variable, and verify they have the correct ID
+        // There should be only two, one on the stage and one on bananas
+        t.equal(allVarListFields[fooVarId].length, 2);
+        const stageVarBlocks = stageBlocks.filter(block => block.fields.hasOwnProperty('VARIABLE'));
+        t.equal(stageVarBlocks.length, 1);
+        t.equal(stageVarBlocks[0].fields.VARIABLE.id, fooVarId);
+        const catVarBlocks = catBlocks.filter(block => block.fields.hasOwnProperty('VARIABLE'));
+        t.equal(catVarBlocks.length, 1);
+        t.equal(catVarBlocks[0].fields.VARIABLE.id, fooVarId);
+
+        const ltPerfectVarId = Object.keys(bananas.variables).filter(k => bananas.variables[k].name === '< Perfect')[0];
+        const ltPerfectVar = bananas.variables[ltPerfectVarId];
+        const ltPerfectMonitor = vm.runtime._monitorState.get(ltPerfectVarId);
+        // Check for unsafe characters, replaceUnsafeChars should just result in the original string
+        // (e.g. there was nothing to replace)
+        // Check that the variable ID does not have any unsafe characters
+        t.equal(StringUtil.replaceUnsafeChars(ltPerfectVarId), ltPerfectVarId);
+        // Check that the monitor record ID does not have any unsafe characters
+        t.equal(StringUtil.replaceUnsafeChars(ltPerfectMonitor.id), ltPerfectMonitor.id);
+
+        // Check that the variable still has the correct info
+        t.equal(StringUtil.replaceUnsafeChars(ltPerfectVar.id), ltPerfectVar.id);
+        t.equal(ltPerfectVar.id, ltPerfectVarId);
+        t.equal(ltPerfectVar.type, Variable.SCALAR_TYPE);
+        t.equal(ltPerfectVar.value, '> perfect');
+
+        // Find all the references for this variable, and verify they have the correct ID
+        // There should be one
+        t.equal(allVarListFields[ltPerfectVarId].length, 1);
+        const bananasBlocks = Object.keys(bananas.blocks._blocks).map(blockId => bananas.blocks._blocks[blockId]);
+        const bananasVarBlocks = bananasBlocks.filter(block => block.fields.hasOwnProperty('VARIABLE'));
+        t.equal(bananasVarBlocks.length, 1);
+        t.equal(bananasVarBlocks[0].fields.VARIABLE.id, ltPerfectVarId);
+
+        t.end();
+        process.nextTick(process.exit);
+    });
+
+    // Start VM, load project, and run
+    t.doesNotThrow(() => {
+        vm.start();
+        vm.clear();
+        vm.setCompatibilityMode(false);
+        vm.setTurboMode(false);
+        vm.loadProject(project).then(() => {
+            vm.greenFlag();
+            setTimeout(() => {
+                vm.getPlaygroundData();
+                vm.stopAll();
+            }, 100);
+        });
+    });
+});