RangerMauve / mqtt-emitter

Emit MQTT events to listeners using mqtt-regex for routing
MIT License
10 stars 6 forks source link

Cannot read property '#' of undefined #12

Open encojosh opened 4 years ago

encojosh commented 4 years ago

We have been getting this error on occasion and have been unable to figure it out. I'm not sure if it is due to a newer version of node or the older verion of mqtt-store that mqtt-emitter is using. Any ideas?

Node v12.10.0 mqtt-emitter v4.0.1 mqtt-store v1.0.4

Cannot read property '#' of undefined at get_tree_values (\node_modules\mqtt-emitter\node_modules\mqtt-store\index.js:94:27) at get_matching (\node_modules\mqtt-emitter\node_modules\mqtt-store\index.js:102:10) at \node_modules\mqtt-emitter\node_modules\mqtt-store\index.js:120:35 at Array.reduce (<anonymous>) at get_matching (\node_modules\mqtt-emitter\node_modules\mqtt-store\index.js:119:4) at \node_modules\mqtt-emitter\node_modules\mqtt-store\index.js:120:35 at Array.reduce (<anonymous>) at get_matching (\node_modules\mqtt-emitter\node_modules\mqtt-store\index.js:119:4) at \node_modules\mqtt-emitter\node_modules\mqtt-store\index.js:120:35 at Array.reduce (<anonymous>)

RangerMauve commented 4 years ago

Thanks for the bug report. Not sure what could be causing this off the top of my head.

Have you been able to put together a minimal reproducible test case?

encojosh commented 4 years ago
var MqttEmitter = require("mqtt-emitter");
var mqttEmitter = new MqttEmitter();
mqttEmitter.on("application/+appId/device/+devEUI/+type", function (message, params, topic) {
   console.log(message);
   console.log(params);
   console.log(topic);
});
mqttEmitter.emit("application/2/device/0004a30b00206495/join", "test");
RangerMauve commented 4 years ago

I tried running the example and while it didn't output to the console, it didn't throw any exceptions either. 🤔

Was it supposed to throw the stack trace you showed in your first comment?

encojosh commented 4 years ago

Yes, are you using the same versions of node and mqtt-emitter (and mqtt-store)?

encojosh commented 4 years ago

Also, I'm running on Windows. That may be an issue as well, but I have other colleagues with the same error on Mac.

RangerMauve commented 4 years ago

I'm on Windows with node 11.15.0 I'll try updating later to see if that helps.

It'd be kinda weird if it's related to the node version. 🤔🤔🤔🤔

jeromevalentin commented 4 years ago

We had the same issue this morning, this is due to the fact that you have a part of your topic which is containing an array method name 'join' ... we faced the problem with 'shift'.

The issue is due to the miss of hasOwnProperty call prior to access the corresponding entry in your tree node in mqtt-store!

Currently you have:

function get_matching(path, tree) {
    if (!path.length) {
        return get_tree_values(tree);
    }

    var multi = tree.children["#"];
    var single = tree.children["+"];

    var next = path[0];
    var next_tree = tree.children[next];

    var rest = path.slice(1);

    var results = [];

    if (multi && multi.value)
        results.push(multi.value);

    return results.concat([single, next_tree]
        .reduce(function (all, subtree) {
            if (subtree) return all.concat(get_matching(rest, subtree));
            return all;
        }, []));
}

while you should have:

function get_matching(path, tree) {
    if (!path.length) {
        return get_tree_values(tree);
    }

    var multi = tree.children["#"];
    var single = tree.children["+"];

    var next = path[0];
    var next_tree = tree.children.hasOwnProperty(next) ? tree.children[next] : undefined;

    var rest = path.slice(1);

    var results = [];

    if (multi && multi.value)
        results.push(multi.value);

    return results.concat([single, next_tree]
        .reduce(function (all, subtree) {
            if (subtree) return all.concat(get_matching(rest, subtree));
            return all;
        }, []));
}

Note the difference on the 'next_tree' line.

Hope this will be fix quickly.

RangerMauve commented 4 years ago

Thank you for looking into this! I'm a bit swamped by other projects at the moment. Would you be interested in submitting a PR with a fix?

jeromevalentin commented 4 years ago

It appears that the issue is present in the mqtt-store version 1.0.4, which is the one loaded by npm when loading the last version of mqtt-emitter.

But the latest version of mqtt-store is 2.1.0. Unfortunately this version is really different (major changes in API).

Fixing the issue on 1.0.4 would lead to create a branch with version 1.0.5 while the 2.X would be really different.

Another option is to upgrade mqtt-emitter to use mqtt-store in version 2.1.0, but this is definitely not a simple fix and would require more time that I don't have too.

So prior to create a PR, please let me know your opinion on that subject.

jeromevalentin commented 4 years ago

Finally I've done a PR!

  1. I reproduced the problem in a test.
  2. I upgraded mqtt-store dependency to the last version 2.1.0
  3. I upgrade mqtt-emitter accordingly by using the new mqtt-store API without any impact on mqtt-emitter API
  4. All tests are now passing successfully

NOTE: I upgraded also the dependency of browserify accordingly to npm audit recommandations.

Hope this PR will be integrated quickly and a new version will be available soon ;)

Arf! I referenced the #11 and not #12 issue in my commits that's why they do not appear here!

RangerMauve commented 4 years ago

Thank you for the fix! Its' out in version 4.1.0! 😁