getCachedState() returns null, failing the requests

[benbucksch]

The below code fails for me with

Cannot read property 'modelid' of null

so it's clear that the getCachedState() function returns null in my case.

This was at the very first setup. I don't know which exact situation is needed to reproduce this, but I got this error as soon as the Hue API started working for me for the first time.

I hope this is enough indication for you to find the bug in the code. null errors should be simple enough to fix.

[benbucksch]

Stack:

TypeError: Cannot read property 'modelid' of null
    at /mnt/compile/source/voice/pia/node_modules/node-hue-api/lib/api/Users.js:79:33
    at async Hue.createCredentials (myapp)

This change makes it work for me:

diff --git a/lib/api/Users.js b/lib/api/Users.js
index 0a42ba6..e0cdc4f 100644
--- a/lib/api/Users.js
+++ b/lib/api/Users.js
@@ -72,13 +72,7 @@ module.exports = class Users extends ApiDefinition {
    * @returns {Promise<any>>}
    */
   createUser(appName, deviceName) {
-    return this.hueApi.getCachedState()
-      .then(state => {
-        //TODO may need to combine the modelid and API version, but am assuming that all newer bridges are kept up to
-        // date, as do not know the specific version number of the introduction of the generateclientkey parameter.
-        const oldBridge = state.modelid === 'BSB001';
-        return this.execute(configurationApi.createUser, {appName: appName, deviceName: deviceName, generateKey: !oldBridge});
-      });
+    return this.execute(configurationApi.createUser, {appName: appName, deviceName: deviceName, generateKey: true});
   }

Of course that's not a proper fix. But I hope it helps you see the bug.

[peter-murray]

The bridge getCachedState() returns a promise for the bridge that should only resolve once the initial connection and configuration has been loaded from the bridge. When you call the connectLocal() function there is an async request sent to the bridge in the background to get the configuration.

It might be possible null was returned from the bridge, or there is some scenario I have not captured in my tests so far that you are hitting.

Can you elaborate on the scenario that you have when you encountered this? Did you have new bridge with no users? If so I could imagine that might be a problem that I have not tested so far.

With Signify deprecating the older bridges, this check will be able to be removed in the future, but for now, I need to validate what bridge/API version I am talking to as the older bridge does not support the streaming client key.

[benbucksch]

the older bridge does not support the streaming client key.

Any chance to do that:
a) at a later time, when the streaming APIs are actually used (I didn't need them and probably will never need them, so this makes a network request on every startup that's useless in my use case)
b) in another way, for example just assuming that it's the new version and if it fails on older devices, detect that failure and handle it as an error at this point?

[benbucksch]

Did you have new bridge with no users?

The bridge is about 1.5 years old, fully configured with lights and rooms and so on using the Philips Hue app and a third party app, and I even had it connected to node red. I think that means the answer to your question is no, but I don't know enough about Hue.

[peter-murray]

The API is different for the creation of the users with respect to the parameters, that is what the check is doing at the point that it is trying to connect. The older bridges error on the connection being made as the API will support the extra parameter for the clientkey. Effectively we have two different sets of parameters between the old and new bridges, which is why the check is made there.

That said the connection to the bridge spins off an async request to get the configuration, there are two aspects to this, one where you pass in a username and another where you are establishing a connection as an unauthenticated user (i.e. no user). The call to get the cached state should not resolve until this promise is resolved to an object, but there are a couple of failure scanrios that could result and cause this to be null. I have created a branch with a fix for one of these scanrios, but also added an error output on the failure condition. This is present in a branch, but I messed up on the base from an old working copy of the repo I had (just been moving to a new computer).

A possible fix is in the commit eaf3c88 but even if that is not a fix, it might also reveal an error condition I was not aware of.

The question I was asking about your scenario was more around the connection you are making, is it an existing user you are connecting with, is it a local connection (or remote), etc...? Just something to give me more context in coming up with a test case to validate the failure, as this code has been working for a little while now to resolve the issues between 1st and 2nd genreation bridges.

[peter-murray]

@benbucksch Do you have anything more on this? If not I will close out this issue as I cannot reproduce it.

[benbucksch]

Hey peter, sorry, I was on vacation, and wasn't able to respond.

Even though you cannot reproduce it, it doesn't negate the fact that it did happen here.

It should be trivial enough to fix, no? Of course, my change was gross, but how about this?

    return this.hueApi.getCachedState()
      .then(state => {
        //TODO may need to combine the modelid and API version, but am assuming that all newer bridges are kept up to
        // date, as do not know the specific version number of the introduction of the generateclientkey parameter.
-        const oldBridge = state.modelid === 'BSB001';
+        const oldBridge = state && state.modelid === 'BSB001';
        return this.execute(configurationApi.createUser, {appName: appName, deviceName: deviceName, generateKey: !oldBridge});
      });
   }

So, if we run into this bug and the cache state is null, then oldBridge become false, making things work. This change should keep things working as before, and also fix the bug I ran into.

[benbucksch]

(Please reopen)

[benbucksch]

is it an existing user you are connecting with, is it a local connection (or remote), etc...?

The hub is on a different local network, but reachable via routing.

IIRC, a new dedicated user with new credentials was being created.

My situation is described in #171 Reproduction. See the createLocal() call there. I ran into this bug #172 here right after I had worked around bug #171.

[benbucksch]

(Please note that I'll be on vacation again and not able to answer questions for the next 3 weeks.)

[peter-murray]

Your fix that you suggest just pushes problems further down and other stuff will blow up later in use (I cannot see how we don't get a cache loaded). The cache is a requirement to load the known configuration and to store things like the lights that are known so that various validations can be performed later, getting past this current check just stores up later failures that will happen if you call certain functions in the API.

This ticket was closed as I provided some changes to get feedback to narrow down as to the problem at hand. There was no response from that and those changes added that I asked for feedback on.

If I cannot reproduce an issue, I cannot test it. If I cannot reproduce and test it, I cannot release a fix to it that I am confident in it not impacting other users.

Signify has ended software updates for the v1 of the bridge, so the need for this check is goign to go away as people retire the older bridges, removing the divergence in the API parameters.

I am currently working on a 5.0 release of the library which is removing axios for a fetch based implementation (to better support browser use cases and remove some of the bloat and proxy support issues of axios).

[benbucksch]

I have recent Hue bridge, IIRC late 2018. I think that's version 2. FYI, when I tested, it was blowing up only at this one spot. With the hacky patch, it worked as expected. I could enumerate lights and rooms, and switch them on and off. However, it's possible that this does unnecessary work on startup. I perfectly understand the need to test the fixes. If you need me to test something specific with master, I can do that. However, I'm currently only vacation, and otherwise have to run 2 companies, so I cannot guarantee it. Re: axing axios (pun intended): Yay! Thanks! (replying by email) Am 17. August 2020 10:12:23 MESZ schrieb Peter Murray <notifications@github.com>:

…

Your fix that you suggest just pushes problems further down and other stuff will blow up later in use (I cannot see how we don't get a cache loaded). The cache is a requirement to load the known configuration and to store things like the lights that are known so that various validations can be performed later, getting past this current check just stores up later failures that will happen if you call certain functions in the API. This ticket was closed as I provided some changes to get feedback to narrow down as to the problem at hand. There was no response from that and those changes added that I asked for feedback on. If I cannot reproduce an issue, I cannot test it. If I cannot reproduce and test it, I cannot release a fix to it that I am confident in it not impacting other users. Signify has ended software updates for the v1 of the bridge, so the need for this check is goign to go away as people retire the older bridges, removing the divergence in the API parameters. I am currently working on a `5.0` release of the library which is removing axios for a fetch based implementation (to better support browser use cases and remove some of the bloat and proxy support issues of axios). -- You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: #172 (comment)

-- Sent from my cell phone. Please excuse the brevity.