rework GenericEventTrigger and GenericEventCondition #3299

ccutrer · 2023-01-03T17:13:58Z

to make their interfaces and semantics match, as well as having a well defined (and useful) way of defining topic filters

fixes #3234

ccutrer · 2023-01-03T17:15:47Z

This is very much a breaking change - filtering behavior and names of configuration items. I don't think GenerictEventTrigger and GenericEventCondition are used too much though, so the impact shouldn't be huge. I'm still open to walking back, or having a back-compat mode (depending on which configuration params are used) any particular breaking change.

In light of that pending discussion, I haven't ensured all the integration tests pass yet, and don't plan on solidifying them until the direction I'm going here is confirmed.

fixes openhab#3234 to make their interfaces and semantics match, as well as having a well defined (and useful) way of defining topic filters Signed-off-by: Cody Cutrer <cody@cutrer.us>

J-N-K

I did not compare with the old implementation, just checked what I would expect. What exactly makes it incompatible with the old implementation (except that the topic filtering was not working at all for the trigger, which is clearly a bug)?

J-N-K · 2023-01-03T17:31:18Z

...n/java/org/openhab/core/automation/internal/module/handler/GenericEventConditionHandler.java

-            }
+        this.source = (String) module.getConfiguration().get(CFG_SOURCE);
+        String topic = (String) module.getConfiguration().get(CFG_TOPIC);
+        if (!topic.isEmpty()) {


I would suggest to use .isBlank here, only whitespace characters don't make sense in the topic, too.

J-N-K · 2023-01-03T17:32:50Z

...n/java/org/openhab/core/automation/internal/module/handler/GenericEventConditionHandler.java

+        } else {
+            topicFilter = null;
+        }
+        if (module.getConfiguration().get(CFG_TOPIC) != null) {


Shouldn't this be CFG_TYPES?

J-N-K · 2023-01-03T17:35:04Z

...n/java/org/openhab/core/automation/internal/module/handler/GenericEventConditionHandler.java

        } else {
-            return true;
+            payloadPattern = null;
        }
    }

    @Override
    public boolean isSatisfied(Map<String, Object> inputs) {
        Event event = inputs.get("event") != null ? (Event) inputs.get("event") : null;


Suggested change

Event event = inputs.get("event") != null ? (Event) inputs.get("event") : null;

Event event = inputs.get("event") instanceof Event ? (Event) inputs.get("event") : null;

Just to make sure we don't run into a ClassCastException here.

J-N-K · 2023-01-03T17:39:00Z

...n/java/org/openhab/core/automation/internal/module/handler/GenericEventConditionHandler.java

+        if (!types.isEmpty() && !types.contains(event.getType())) {
+            return false;
+        }
+        TopicGlobEventFilter localTopicFilter = topicFilter;


I was think about that in your previous PR. Wouldn't it be better to have a MatchAllEventFilter that always returns true? That way we could omit a lot of null checks and also make .getFilter return a non-null value.

In general since EventFilter gets the Event, a filter could also filter not only on topic but also on source or type. That brings me again to the EventFilterBuilder that builds an EventFilter .withTopicGlob(String glob), .withTopicRegex(String regex), .withType(Class<? extends Event>), .withTypes(Set<Class<? extends Event>> or .withSource(String source). The implementation than collects all those filters in a List<EventFilter> filterChain, and does a filterChain.stream().allMatch(filter -> filter.apply(event) in .apply. But this is out of scope here. Just an idea :-)

I actually really like this idea. But yeah... out of scope here.

J-N-K · 2023-01-03T17:48:05Z

...ain/java/org/openhab/core/automation/internal/module/handler/GenericEventTriggerHandler.java

+        } else {
+            topicFilter = null;
+        }
+        if (module.getConfiguration().get(CFG_TOPIC) != null) {


ccutrer · 2023-01-03T18:08:21Z

I did not compare with the old implementation, just checked what I would expect. What exactly makes it incompatible with the old implementation (except that the topic filtering was not working at all for the trigger, which is clearly a bug)?

Behavior changes (whether it's a bug or not):

Config param names for GenericEventTrigger changed (from eventTopic => topic, eventSource => source, eventType => types
Config param names for GenericEventCondition changed (from eventType => types)
GenericEventTrigger topic param now enforced (as a glob)
GenericEventTrigger source param is now checked against event source, exact match, not against the event topic as a substring
GenericEventCondition topic param is now a glob, instead of a regex (though it was never documented that I could find that it was a regex)
GenericEventCondition source param is now an exact match, instead of a regex (again, never really documented)
GenericEventCondition payload param is now a simple regex, without implicit start/end anchors (using find() instead of matches()). There are also weirdness here because the old code checked if the payload config value started with * it would leave it alone, and then process as a regex, which then is a syntax error since regexes can't start with *.

I'm mostly concerned about the GenericEventTrigger topic now being enforced. Before switching some of the internal triggers to use the TopicPrefixEventFilter, they were attempting to set a filter like openhab/*, which wouldn't match an event like openhab/items/myMotionItem1/command anymore, since the glob would need to be ** in order to match multiple levels. And itests definitely had several cases where the source param was being set to things like myMotionItem1 which definitely won't match the event source anymore. Maybe I'm just being too conservative though, and these are things that didn't work at all in the past, so shouldn't really be considered breaking.

* use isBlank() instead of isEmpty() * fix copy/paste error using wrong config param for event types Signed-off-by: Cody Cutrer <cody@cutrer.us>

ccutrer · 2023-01-03T18:32:04Z

bundles/org.openhab.core/src/main/java/org/openhab/core/events/TopicGlobEventFilter.java

+     *      Glob</a>
+     */
+    public TopicGlobEventFilter(String topicGlob) {
+        this.topicMatcher = FileSystems.getDefault().getPathMatcher("glob:" + topicGlob);


I did verify that this still works on Windows, even though this is technically a file system interface, and file systems on windows use backslash not forward slash

Yes, it works because internally it is converted to a regex and the conversion code is the same for Windows and Unix. To make sure that this is always the case in the future, please add a test that checks it, we'll have Windows and Mac builds for CI, too.

already there, as a unit test: TopicGlobEventFilterTest.

J-N-K · 2023-01-03T19:03:42Z

To me these changes make perfectly sense and I agree that most of this was probably never used. We need to add a breaking notice to openhab-distro.

I would like to hear comments from other @openhab/core-maintainers.

see openhab/openhab-core#3299 Signed-off-by: Cody Cutrer <cody@cutrer.us>

Signed-off-by: Cody Cutrer <cody@cutrer.us>

see openhab/openhab-core#3299 Signed-off-by: Cody Cutrer <cody@cutrer.us>

openhab-bot · 2023-01-09T19:54:23Z

This pull request has been mentioned on openHAB Community. There might be relevant details there:

https://community.openhab.org/t/thing-status-reporting-4-0-0/143180/2

ccutrer · 2023-01-13T19:38:37Z

Ping @openhab/core-maintainers

J-N-K · 2023-01-20T11:01:24Z

Since nobody objects, I would say we are good to go. I'll have a last look this evening.

J-N-K

Sorry for letting you wait so long. One last point: can you re-generate the i18n-files? Otherwise LGTM.

apparently this wasn't run when the timeOnly option was added to timer.DateTimeTrigger either. I just left it. Signed-off-by: Cody Cutrer <cody@cutrer.us>

J-N-K

Thanks

see openhab/openhab-core#3299 Signed-off-by: Cody Cutrer <cody@cutrer.us>

* update docs for core.GenericEventTrigger see openhab/openhab-core#3299 Signed-off-by: Cody Cutrer <cody@cutrer.us> * link to javadocs for mostly-complete list of event types Signed-off-by: Cody Cutrer <cody@cutrer.us> --------- Signed-off-by: Cody Cutrer <cody@cutrer.us>

* rework GenericEventTrigger and GenericEventCondition fixes openhab#3234 to make their interfaces and semantics match, as well as having a well defined (and useful) way of defining topic filters Signed-off-by: Cody Cutrer <cody@cutrer.us> GitOrigin-RevId: 474d24c

openhab-bot · 2023-07-30T19:40:12Z

This pull request has been mentioned on openHAB Community. There might be relevant details there:

https://community.openhab.org/t/openhab-4-migration-faq/148144/18

openhab-bot · 2023-08-03T13:47:31Z

This pull request has been mentioned on openHAB Community. There might be relevant details there:

https://community.openhab.org/t/openhab-4-0-release-discussion/147957/111

ccutrer requested a review from a team as a code owner January 3, 2023 17:13

rework GenericEventTrigger and GenericEventCondition

f3f80db

fixes openhab#3234 to make their interfaces and semantics match, as well as having a well defined (and useful) way of defining topic filters Signed-off-by: Cody Cutrer <cody@cutrer.us>

ccutrer force-pushed the generic-event-trigger branch from 18e3bfb to f3f80db Compare January 3, 2023 17:21

J-N-K requested changes Jan 3, 2023

View reviewed changes

address initial review comments

c47bb9c

* use isBlank() instead of isEmpty() * fix copy/paste error using wrong config param for event types Signed-off-by: Cody Cutrer <cody@cutrer.us>

ccutrer commented Jan 3, 2023

View reviewed changes

ccutrer added a commit to ccutrer/openhab-docs that referenced this pull request Jan 3, 2023

update docs for core.GenericEventTrigger

40db7c5

see openhab/openhab-core#3299 Signed-off-by: Cody Cutrer <cody@cutrer.us>

ccutrer mentioned this pull request Jan 3, 2023

update docs for core.GenericEventTrigger openhab/openhab-docs#1986

Merged

ccutrer added a commit to ccutrer/openhab-distro that referenced this pull request Jan 3, 2023

add breaking change alert for core.GenericEventTrigger

b69a347

see openhab/openhab-core#3299 Signed-off-by: Cody Cutrer <cody@cutrer.us>

ccutrer mentioned this pull request Jan 3, 2023

add breaking change alert for core.GenericEventTrigger openhab/openhab-distro#1456

Merged

fix integration test?

9d27e9b

Signed-off-by: Cody Cutrer <cody@cutrer.us>

ccutrer added a commit to ccutrer/openhab-docs that referenced this pull request Jan 3, 2023

update docs for core.GenericEventTrigger

0854837

see openhab/openhab-core#3299 Signed-off-by: Cody Cutrer <cody@cutrer.us>

J-N-K requested changes Feb 12, 2023

View reviewed changes

update automation i18n default translations

2d4aafd

apparently this wasn't run when the timeOnly option was added to timer.DateTimeTrigger either. I just left it. Signed-off-by: Cody Cutrer <cody@cutrer.us>

J-N-K approved these changes Feb 14, 2023

View reviewed changes

J-N-K added the enhancement An enhancement or new feature of the Core label Feb 14, 2023

J-N-K added this to the 4.0 milestone Feb 14, 2023

J-N-K merged commit 474d24c into openhab:main Feb 14, 2023

wborn pushed a commit to openhab/openhab-distro that referenced this pull request Feb 18, 2023

add breaking change alert for core.GenericEventTrigger (#1456)

00e00fa

see openhab/openhab-core#3299 Signed-off-by: Cody Cutrer <cody@cutrer.us>

rkoshak mentioned this pull request Feb 21, 2023

Changed trigger to support changes in core rkoshak/openhab-rules-tools#83

Merged

ccutrer added a commit to ccutrer/openhab-docs that referenced this pull request Mar 6, 2023

update docs for core.GenericEventTrigger

14d12a5

see openhab/openhab-core#3299 Signed-off-by: Cody Cutrer <cody@cutrer.us>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rework GenericEventTrigger and GenericEventCondition #3299

rework GenericEventTrigger and GenericEventCondition #3299

ccutrer commented Jan 3, 2023

ccutrer commented Jan 3, 2023

J-N-K left a comment

J-N-K Jan 3, 2023

J-N-K Jan 3, 2023

J-N-K Jan 3, 2023

J-N-K Jan 3, 2023

ccutrer Jan 3, 2023

J-N-K Jan 3, 2023

ccutrer commented Jan 3, 2023

ccutrer Jan 3, 2023

J-N-K Jan 3, 2023 •

edited

Loading

ccutrer Jan 3, 2023

J-N-K commented Jan 3, 2023

openhab-bot commented Jan 9, 2023

ccutrer commented Jan 13, 2023

J-N-K commented Jan 20, 2023

J-N-K left a comment

J-N-K left a comment

openhab-bot commented Jul 30, 2023

openhab-bot commented Aug 3, 2023

	Event event = inputs.get("event") != null ? (Event) inputs.get("event") : null;
	Event event = inputs.get("event") instanceof Event ? (Event) inputs.get("event") : null;

rework GenericEventTrigger and GenericEventCondition #3299

rework GenericEventTrigger and GenericEventCondition #3299

Conversation

ccutrer commented Jan 3, 2023

ccutrer commented Jan 3, 2023

J-N-K left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ccutrer commented Jan 3, 2023

Choose a reason for hiding this comment

J-N-K Jan 3, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

J-N-K commented Jan 3, 2023

openhab-bot commented Jan 9, 2023

ccutrer commented Jan 13, 2023

J-N-K commented Jan 20, 2023

J-N-K left a comment

Choose a reason for hiding this comment

J-N-K left a comment

Choose a reason for hiding this comment

openhab-bot commented Jul 30, 2023

openhab-bot commented Aug 3, 2023

J-N-K Jan 3, 2023 •

edited

Loading