[nuki] Fixed configuration reload on initialization

[janvyb]

Fixed bug mentionend in https://community.openhab.org/t/new-openhab2-binding-for-nuki-smart-locks/25940/186.

When user changes configuration property (unlatch in this case), openhab calls initialize() on Thing, but since configuration was only loaded in constructor, any changes user made will not take effect until openhab restart or reinstallation of binding.

I've also added logging of request received from bridge when handling callback, since I noticed some weird NullpointerExceptions in my logfile which were probably caused by invalid request, but without seeing the data sent it's hard to debug.

[lolodomo]

Probably more something only for you as developer to find a potential bug.

[janvyb]

I mean, yes, that's exactly why I added it. If someone reports problem with callback I won't be able to debug it if I don't know what was sent.

[lolodomo]

But do you have an example of exception by a user?
Why not rather analyzing the current code and only catch more specific exceptions?
In case this change makes sense, logging at warn level would be more appropriate.

[janvyb]

As I've said I've added it because I found some Nullpointers in logfile which I can't replicate (which means it could happen to someone else) and knowing what data were sent would really help. I agree that warn would be better.

[lolodomo]

Why not fixing the NPE in this case, rather than catching the exception?

[janvyb]

I've fixed the one place it throws NPE but it still would be helpful to see the data sent since that boolean flag should not be null

[janvyb]

How about this? I renamed the method, I didn't like the loadConfiguration() name since it did too much and you would not expect that method to have any side effects (like also changing thing configuration).

[lolodomo]

That looks even better now.
But please reload the configuration at first in initialize because I am not sure the load in the constructor is a guarantee to have the proper configuration. Normally, the loading is done in initialize.

[lolodomo]

Just to be clear, here is what I was suggesting:

    public void initialize() {
        this.configuration = getConfigAs(getConfigurationClass());
        if (migrateConfiguration()) {
            this.configuration = getConfigAs(getConfigurationClass());
        }
        scheduler.execute(this::initializeHandler);
    }

Note that when the thing configuration is updated, I believe the core framework will dispose/initialize again the thing handler. So I believe the proper code should be:

    public void initialize() {
        this.configuration = getConfigAs(getConfigurationClass());
        if (migrateConfiguration()) {
            return;
        }
        scheduler.execute(this::initializeHandler);
    }

but that would be better if you can test it (migration) because I am not 100% sure.

[janvyb]

I think the first one is correct, the handler needs to be initialized again every time config is updated, initializeHandler is not a framework method, it's a private method of nuki handler which reloads it's state.

[lolodomo]

LGTM