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

Add topic filter for event WebSocket #4550

Merged
merged 3 commits into from
Feb 15, 2025

Conversation

florian-h05
Copy link
Contributor

@florian-h05 florian-h05 commented Jan 9, 2025

Closes #4549.
Closes #3764.

The topic filter can be configured by sending a WS message like in following examples:

Only send all ItemCommandEvents:

{
  "type": "WebSocketEvent",
  "topic": "openhab/websocket/filter/topic",
  "payload": "[\"openhab/items/*/command\"]",
  "source": "ui-c2ead538aa"
}

Only send all ItemCommandEvents, except for the test Item:

{
  "type": "WebSocketEvent",
  "topic": "openhab/websocket/filter/topic",
  "payload": "[\"openhab/items/*/command\", \"!openhab/items/test/command\"]",
  "source": "ui-c2ead538aa"
}

Send all events, except ItemCommandEvents:

{
  "type": "WebSocketEvent",
  "topic": "openhab/websocket/filter/topic",
  "payload": "[\"!openhab/items/*/command\"]",
  "source": "ui-c2ead538aa"
}

Signed-off-by: Florian Hotze <dev@florianhotze.com>
Signed-off-by: Florian Hotze <dev@florianhotze.com>
@florian-h05 florian-h05 marked this pull request as ready for review January 9, 2025 19:05
@florian-h05 florian-h05 requested a review from a team as a code owner January 9, 2025 19:05
florian-h05 added a commit to florian-h05/openhab-webui that referenced this pull request Jan 9, 2025
Depends on openhab/openhab-core#4550.

Signed-off-by: Florian Hotze <dev@florianhotze.com>
@florian-h05
Copy link
Contributor Author

@ghys We are getting close to have the event WS support everything we need in the UI 🚀

@spacemanspiff2007
Copy link
Contributor

Would it be possible to provide a negative and positive filter here, too?
E.g. a filter that passes certain topics and a filter that rejects them?

@florian-h05
Copy link
Contributor Author

Added just a few seconds ago 👍
See the PR description for how to use that.

Signed-off-by: Florian Hotze <dev@florianhotze.com>
@spacemanspiff2007
Copy link
Contributor

Is the topic not a regular expression? Is it something openhab specific?

@florian-h05
Copy link
Contributor Author

Topics can be defined as string with the * wildcard, see SSE events.
Internally, the topic expressions are converted to RegEx, during this conversion I can check whether the topic is to include or to ! exclude.

@spacemanspiff2007
Copy link
Contributor

spacemanspiff2007 commented Jan 10, 2025

If you look at the /items endpoint some time ago there was a switch away from the openhab specific wildcard implementation to a regular expression.
Do you think it would make sense to use a normal regular expression here, too?

grafik

@florian-h05
Copy link
Contributor Author

Do you think it would make sense to use a normal regular expression here, too?

I don‘t think that the additional regex features would be a benefit here, I think the wildcard is enough.

@spacemanspiff2007
Copy link
Contributor

I don‘t think that the additional regex features would be a benefit here, I think the wildcard is enough.

But why use an custom openhab wildcard implementation which then gets internally converted into regex instead of just allowing a regex from the start instead?
This seems overly complicated and unnecessarily restrictive.

@florian-h05
Copy link
Contributor Author

It makes the filters easier to use from clients though, as you can simply specify the topic with wildcards and don't need to take care of the RegEx.
The SSE event subscription is there since at least 2019 and since then no one ever asked for RegEx support for the topic filter.

@spacemanspiff2007
Copy link
Contributor

spacemanspiff2007 commented Jan 26, 2025

I still think directly exposing the regex is a much better idea.
If you know how to programmatically create a websocket connection you most certainly know how to write the correct regex (or at least know how to look it up) instead of reading up on this custom wildcard implementation. Or we pragmatically provide an example in the docs which can just be copy pasted - that way it's easy to use while still providing flexibility.

The wildcard implementation already confused me because I thought openhab/items/*/command will match only one level of the topics yet when looking at the code it matches an arbitrary length string including "/".

If the goal is still to provide a simple topic matching mechanism then we should use the mqtt matching mechanism with + and # as wildcards instead because that's an already established and properly defined topic matching convention.

@florian-h05
Copy link
Contributor Author

@openhab/core-maintainers What is your opinion here?

The current implementation is easier to consume, especially when migrating from SSE to WS, as it works the same way it did for SSE for years.

@spacemanspiff2007
Copy link
Contributor

spacemanspiff2007 commented Feb 15, 2025

There will be no migration issue.
When providing the old format (e.g. openhab/items/*/command) no events will be sent over the websocket connection so it's immediately clear that something is wrong.

One could provide an example in the docs how to migrate from sse to websocket filter by just showing that instead of
openhab/items/*/command it should now be
openhab/items/.*/command .
I'll gladly provide a PR for the docs with two or three examples if that mitigates these hypothetical migration issues.

Copy link
Member

@kaikreuzer kaikreuzer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd say this issue is about porting the existing functionality from SSE over to WebSockets - so this PR is fine and imho can be merged.

The discussion about RegEx is independent of this. Whether that makes sense or not is debatable. My personal feeling is that RegEx is not typically used in the web, possibly due to its tendency to become very complex and as it is difficult to read. I would therefore also prefer keep things simple. But @spacemanspiff2007, please feel free to create an issue on this and if there are good reasons why RegEx would improve the usability/flexibility, this can certainly be considered.

@kaikreuzer kaikreuzer merged commit 4ec2b3c into openhab:main Feb 15, 2025
2 checks passed
@kaikreuzer kaikreuzer added the enhancement An enhancement or new feature of the Core label Feb 15, 2025
@kaikreuzer kaikreuzer added this to the 5.0 milestone Feb 15, 2025
@florian-h05 florian-h05 deleted the event-ws-topics branch February 15, 2025 15:21
@J-N-K
Copy link
Member

J-N-K commented Feb 15, 2025

I agree with @kaikreuzer

florian-h05 added a commit to florian-h05/openhab-docs that referenced this pull request Feb 20, 2025
Refs openhab/openhab-core#4550.

Signed-off-by: Florian Hotze <dev@florianhotze.com>
stefan-hoehn pushed a commit to openhab/openhab-docs that referenced this pull request Feb 20, 2025
* Add WebSocket topic filter docs

Refs openhab/openhab-core#4550.

Signed-off-by: Florian Hotze <dev@florianhotze.com>

* Minor rewording

Signed-off-by: Florian Hotze <dev@florianhotze.com>

---------

Signed-off-by: Florian Hotze <dev@florianhotze.com>
@fazasharif
Copy link

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement An enhancement or new feature of the Core
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Event WebSocket: Add topic filter Support topic filters in the WebSocket API
5 participants