Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[REST] Add thing status to ThingAddedEvent #3112

Closed
spacemanspiff2007 opened this issue Oct 13, 2022 · 10 comments
Closed

[REST] Add thing status to ThingAddedEvent #3112

spacemanspiff2007 opened this issue Oct 13, 2022 · 10 comments

Comments

@spacemanspiff2007
Copy link
Contributor

spacemanspiff2007 commented Oct 13, 2022

As described by @J-N-K in #3070: the thing has always a status.
This status should be part of the ThingAddedEvent as it is the only information missing to create a full thing object on the HABApp side.

@J-N-K
Copy link
Member

J-N-K commented Oct 16, 2022

But how will this solve the linked issue? It seems that the events are in the wrong order, which results in the ThingStatusInfoEvent being emitted before the ThingAddedEvent. In this case overwriting the ThingStatusEvent with a status from the ThingAddedEvent could result in a wrong state.

When the thing is created, the status is always UNINITIALIZED, so that would be the status present in the ThingAddedEvent. I don't see any benefit in adding that to the event. All status changes are then reported with ThingStatusEvent/ThingStatusInfoChangedEvent.

@spacemanspiff2007
Copy link
Contributor Author

But how will this solve the linked issue?

Currently I receive the ThingAddedEvent. Then I query the RestAPI to get the things status. In the meanwhile I receive a ThingStatusInfoEvent for the thing I am about to add, but the query hasn't completed yet.
So I am receiving a ThingStatusInfoEvent for a thing that doesn't exist which generates a warning in the log.

If the status information would be part of the ThingAddedEvent I would not have to query openHAB for the thing status.
So the flow would be:
ThingAddedEvent -> create thing on HABApp side -> ThingStatusInfoEvent for a thing that exists -> no warning


When the thing is created, the status is always UNINITIALIZED

You know this because you are an absolute openHAB expert. It is not in the official docs and there is no lifecycle diagram where I can easily find this information.

I don't see any benefit in adding that to the event.

For somebody who doesn't know the internals it might be an important information.
And let's say it information is added to the docs that the thing always starts with UNINITIALIZED. What is the corresponding thing status info? Also maybe somehow this status changes with openHAB4 and then my code runs with false assumptions.

These are ~20bytes with neglectable overhead but they transfer required knowledge from the API consumer to the openHAB core.
Consequently if something changes there the API consumer will always have the correct information.


It's important to realize that not all API consumers are openHAB experts.
Making things a little easier for them can make quite a difference.

@spacemanspiff2007
Copy link
Contributor Author

@J-N-K Could you find it in your heart to implement this or point me in the right direction where this can be changed? I searched the source code this morning but I am really having a hard time understanding it.

The warnings are quite annoying and I think it's good to make the events more explicit (from an API perspective).
Since this is a backwards compatible change I think it's low effort high reward with no side effects.

@J-N-K
Copy link
Member

J-N-K commented Nov 5, 2022

The event is created via ThingEventFactory.createAddedEvent(Thing thing). In this method a ThingDTO is created from the thing via the ThingDTOMapper.map(Thing thing) method. The ThingDTO is then serialized for the event payload.

So to get the thing status into the event one would need to create an enriched ThingDTO and use that. There is already an EnrichedThingDTO which includes the status but also firmware information and information about linked items, so it seems it's not the ideal candidate and a new inheritor of ThingDTO would probably be a better idea.

You would also need to take care of the "inverse" method (ThingEventFactory.createAddedEvent(String topic, String payload)) that converts the serialized payload to an ThingAddedEvent.

I still think that status and entity events should in general not be mixed. Also the next question will probably be, why the ThingUpdatedEvent does not carry status information and as pointed out above, the status may already be different when the event is processed. So my position here is: I will not implement it, but I'll also not put a veto on that change if someone else would be willing to merge it.

@spacemanspiff2007
Copy link
Contributor Author

I still think that status and entity events should in general not be mixed.

I agree that it's not optimal. But when I create the thing on the client side I need to use a value for the status and for the status detail.
Both values could be documented on the thing page and I gladly provide a PR for that.
If the status and detail would be included in the event the event would be "self documenting" since all important fields would be included.

You mentioned that on creation the thing status is always UNINITIALIZED. Do you by change know what the status detail is?

@spacemanspiff2007
Copy link
Contributor Author

The thing status detail gets initialized as "NONE"

@J-N-K
Copy link
Member

J-N-K commented Nov 7, 2022

In case of a disabled item it is then immediately updated to UNINITIALIZED/DISABLED.

@spacemanspiff2007
Copy link
Contributor Author

But that will be properly reported by a ThingStatusUpdate, correct?
So it's always correct to create the Thing on the client side with UNINITIALIZED / NONE?

@J-N-K
Copy link
Member

J-N-K commented Nov 7, 2022

2x yes: there is an update event and NONE is always the first status.

@spacemanspiff2007
Copy link
Contributor Author

I must have been blind, the docs are excellent!
That's sufficient for me

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants