Jrule when refactoring #61

querdenker2k · 2022-10-13T14:53:28Z

Has to wait for: #52

Refactors the JRuleWhen Annotations into separate Annotations and did a little bit refactoring.
@seaside1 @seime But you can still have a look please.

…e_when_refactoring

querdenker2k · 2022-10-13T14:56:33Z

And one more thing to Testing:
I created a simple openhab rules project, where i test again a docker-container. Doesn't it make sense to add something like that for testing JRule?
This could be extended to make just Rest Calls to the openHAB API and verify that the rules are getting executed via Item-Changes inside the rule and verify again via Rest Calls that all is working.

# Conflicts: # src/main/java/org/openhab/automation/jrule/internal/engine/JRuleEngine.java # src/main/java/org/openhab/automation/jrule/internal/engine/JRuleExecutionContext.java # src/main/java/org/openhab/automation/jrule/rules/JRuleWhenItemChange.java # src/main/java/org/openhab/automation/jrule/rules/event/JRuleItemEvent.java

querdenker2k · 2022-10-16T19:31:39Z

#53

seaside1 · 2022-10-18T19:26:04Z

@querdenker2k Can you resolve the conflicts? I'm planning to review pr59 first and then start looking into this one.

querdenker2k · 2022-10-20T06:07:27Z

Not the full README is updated, just this parts which i added to my automated test-project.
I will not change anything anymore in the branch, except the README and maybe fixing something if it does not work in my tests.

querdenker2k · 2022-10-21T20:40:44Z

It's done.
I think i have everything tested. The most of it here:
https://github.com/querdenker2k/openhab-jrule-test
README is updated as well.

seime

Lets say there are 2 or more types of triggers on a single method, ie
JRuleWhenItemChange and JRuleWhenChannelTrigger, and there is an event parameter on the rule method.

How can we handle that? If the metod parameter is a JRuleItemEvent, will it blow up when the channel triggers?

seime · 2022-10-22T09:24:27Z

Another thing is to determine which @JRuleWhenXXX got triggered if there are multiple.

Maybe we should add an ID to each trigger and pass that to the rule method?

querdenker2k · 2022-10-22T09:28:37Z

No, the method parameter must be ever the abstract class and at runtime you get the correct concrete instance of it. You have to cast.

querdenker2k · 2022-10-22T11:02:27Z

You mean multiple on the same method or on different for the same trigger?
You can determine this via the concrete instance of JRuleEvent at runtime.

…n JRuleWhenItemReceivedCommand annotation

seime · 2022-10-22T12:53:07Z

Great work @querdenker2k!

Here are some findings that I think we should address:

The PR requires a lot of rule reworking. We should have both a migration guide to ease the job and possibly backwards compatibility for JRuleWhen and JRulePrecondition (map from old to new annotations and then treat as appropriate). Then the user can migrate over time
I'd like support for specifying concrete classes of JRuleEvent as method parameter - if and only if there are no conflicting triggers. In that case, require JRuleEvent abstract class and log ERROR. The current approach requires a lot of casting that can be avoided in most scenarios, and also allows for ClassCastExceptions at runtime.

I pushed a few commits also.

querdenker2k · 2022-10-24T17:59:56Z

@seaside1 Whats needed here to merge before other MR are coming (Merging this is ever manually because the change is too big)
Is a migration guide needed if all samples in the README are updated? (as they currently are)

seaside1 · 2022-10-24T18:19:04Z

There are some conflicts, could you rebase?

querdenker2k · 2022-10-24T18:51:56Z

there are no conflicts?

seaside1 · 2022-10-24T19:26:08Z

Can't rebase and merge from the tool (github). But manually no conflict when I rebase. I can merge it.
I have some comments on the documentation I will update that. I'll test this out a bit as well
I do think this make the rules better, but it is also a breaking change so want to test it out a bit more.

Some things to be added to the doc (just for my reference

JRuleWhenReccievedCommand with On, off example
JRuleEvent should be replaced with JRuleItemEvent etc
First rule example need to be fixed

I'll see if I find more when I test it.

@seime Have you tested it out with your rules?

seaside1 · 2022-10-24T19:28:14Z

seaside1 · 2022-10-24T20:02:03Z

JRuleContactTrigger need to be updated with Open / Close values

querdenker2k · 2022-10-24T20:16:18Z

I think all the JRuleCommonTrigger could be removed, right? Looks like they are deprecated.

seaside1 · 2022-10-24T20:17:23Z

Rename @JRuleWhenItemReceivedUpdate to field to "update" to be more inline with JRuleWhenItemReceivedCommand "command"

seaside1 · 2022-10-24T20:18:40Z

I think all the JRuleCommonTrigger could be removed, right? Looks like they are deprecated.

Yes I think so. Since you will have to refactor all your JRuleWhen we might as well remove all those triggers.

querdenker2k · 2022-10-24T20:24:47Z

Rename @JRuleWhenItemReceivedUpdate to field to "update" to be more inline with JRuleWhenItemReceivedCommand "command"

I would suggest renaming to "state"

querdenker2k · 2022-10-25T16:05:56Z

Rename @JRuleWhenItemReceivedUpdate to field to "update" to be more inline with JRuleWhenItemReceivedCommand "command"

I would suggest renaming to "state"

i renamed it to "state" because in OH core its named "state" as well.

…e JRuleXXXEvent as method parameters

querdenker2k · 2022-10-25T16:17:53Z

@seaside1 all findings fixed/done

seime · 2022-10-25T18:58:24Z

I've been running this branch for a few days now, and seems to work quite well.

Two things I have noticed:

Event parameters for rules. If it isn't JRuleEvent, but a concrete class, this fails at runtime.
Same goes for casting to wrong type in the method body (ie JRuleTimerEvent when a JRuleItem event actually arrived).

Both can be mitigated as mentioned here: #61 (comment)

If we're to drop JRuleWhen-support I truly think we need a section in the changelog that deals with the necessary steps to migrate. I think I half a day migrating my rules and correcting the mistakes.

Overall a great step in the right direction IMHO, thanks again @querdenker2k

When this branch is merged I'll try to create a JUnit setup in this repo.

querdenker2k · 2022-10-25T19:28:27Z

I've been running this branch for a few days now, and seems to work quite well.

Two things I have noticed:

Event parameters for rules. If it isn't JRuleEvent, but a concrete class, this fails at runtime.

Same goes for casting to wrong type in the method body (ie JRuleTimerEvent when a JRuleItem event actually arrived).

Both can be mitigated as mentioned here: #61 (comment)

If we're to drop JRuleWhen-support I truly think we need a section in the changelog that deals with the necessary steps to migrate. I think I half a day migrating my rules and correcting the mistakes.

Overall a great step in the right direction IMHO, thanks again @querdenker2k

When this branch is merged I'll try to create a JUnit setup in this repo.

First point: Event parameters is fixed 3 hours ago, tested with this rule

@JRuleWhenItemChange(item = _MyTemperatureSensor.ITEM, condition = @JRuleCondition(lte = 20))
    public synchronized void turnOnFanIfTemperatureIsLow(JRuleItemEvent event) {
...

But what to do if somebody casts to a wrong type? Everybody has to check if event could be of multiple types.

@seime I already created a test project with some tests. Just in case if you would like to do it the same way to not to do everything again. Or will promote this as a first approach. #https://github.com/querdenker2k/openhab-jrule-test

querdenker2k · 2022-10-25T20:16:49Z

@seime @seaside1 Added a MIGRATE.md with a few examples

seaside1 · 2022-10-25T20:39:39Z

I agree with Seime it's very good PR and it make the rules a lot cleaner, so good work! :)
I feel somewhat ready to merge, I've noticed I get a loop when using

  @JRuleName("testTImeTrigger")
    @JRuleWhenTimeTrigger(hours = 6)
    public synchronized void testTimeTrigger(JRuleEvent event) {
        logInfo("Time Trigger triggered");
}

Does this work on your systems?

seaside1 · 2022-10-25T20:52:14Z

Never the less, I'll merge it now and we can report bugs if we find stuff.

seime · 2022-10-26T06:40:12Z

I've noticed I get a loop when using
    @JRuleWhenTimeTrigger(hours = 6)

Could this be related to #28 ? If you are running on a fast computer, it might be able to reschedule the timer within the same ms causing it to trigger again.

seaside1 · 2022-10-26T18:37:33Z

Could this be related to #28 ? If you are running on a fast computer, it might be able to reschedule the timer within the same ms causing it to trigger again.

Don't think so. I don't get this with the pre JruleWhen refactoring. I do have a relatively fast device that I run openHAB on.
I'll see if I can get more info about it.

querdenker2k · 2022-10-26T18:56:53Z

my test with a cron with every 5 seconds does not have a problem.
you have a endless loop?
we should create an issue to track this.

seime · 2022-10-31T18:07:49Z

Found this in my log;

java.lang.NumberFormatException: Invalid BigDecimal value: UNDEF
        at org.openhab.core.library.types.QuantityType.<init>(QuantityType.java:130) ~[?:?]
        at org.openhab.core.library.types.QuantityType.<init>(QuantityType.java:106) ~[?:?]
        at org.openhab.core.library.types.QuantityType.valueOf(QuantityType.java:206) ~[?:?]
        at org.openhab.automation.jrule.internal.engine.excutioncontext.JRuleItemExecutionContext.lambda$4(JRuleItemExecutionContext.java:67) ~[?:?]
        at java.util.Optional.filter(Optional.java:223) ~[?:?]
        at org.openhab.automation.jrule.internal.engine.excutioncontext.JRuleItemExecutionContext.matchCondition(JRuleItemExecutionContext.java:67) ~[?:?]
        at org.openhab.automation.jrule.internal.engine.excutioncontext.JRuleItemChangeExecutionContext.match(JRuleItemChangeExecutionContext.java:51) ~[?:?]
        at org.openhab.automation.jrule.internal.engine.JRuleEngine.lambda$46(JRuleEngine.java:267) ~[?:?]

querdenker2k · 2022-10-31T20:16:21Z

This is handled (or even not) by openhab. But the problem must exist already before. We could handle UNDEF before trying to parse it with QuantityType.
Please create an Issue for that.

seime · 2022-11-02T17:15:27Z

#71

seime · 2022-11-02T17:17:40Z

Could this be related to #28 ? If you are running on a fast computer, it might be able to reschedule the timer within the same ms causing it to trigger again.

Don't think so. I don't get this with the pre JruleWhen refactoring. I do have a relatively fast device that I run openHAB on. I'll see if I can get more info about it.

#69

querdenker2k added 4 commits October 5, 2022 21:47

add event for channel and cleanup

ac892ff

add event trigger for channel

d713fbd

Merge remote-tracking branch 'origin/add_event_for_channel' into jrul…

66916f7

…e_when_refactoring

add addon action handler

36f1e89

querdenker2k added 2 commits October 15, 2022 07:36

merge manually QuantityType fix

a1dbaf1

querdenker2k marked this pull request as ready for review October 15, 2022 05:37

update

ce7f0e0

update

a678061

querdenker2k added 2 commits October 18, 2022 21:49

updated some readme, add tests for test-project

c9515e0

merge

24d6e91

querdenker2k added 4 commits October 20, 2022 08:14

merge

4e9413f

update

ed49a07

Merge remote-tracking branch 'origin/main' into jrule_when_refactoring

dc9a5c3

bugfix, complete README change

28f64a0

querdenker2k added 3 commits October 21, 2022 22:55

bugfix

392ca51

removed some unused class

009afdc

removed some unused class

be1bddf

seime reviewed Oct 22, 2022

View reviewed changes

seime added 2 commits October 22, 2022 13:25

Corrected type of openhab event to check for

0197233

Add some more checks on the rule methods. Renamed to() to command() i…

6aca138

…n JRuleWhenItemReceivedCommand annotation

one small change

956aeaf

removed unused triggers, unify constructor calls, allow using concret…

d13888b

…e JRuleXXXEvent as method parameters

add migration readme

a667107

seaside1 merged commit 30b7d71 into seaside1:main Oct 25, 2022

seaside1 mentioned this pull request Oct 25, 2022

Split JRuleWhen into multiple triggers #53

Closed

querdenker2k deleted the jrule_when_refactoring branch October 26, 2022 02:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Jrule when refactoring #61

Jrule when refactoring #61

querdenker2k commented Oct 13, 2022

querdenker2k commented Oct 13, 2022

querdenker2k commented Oct 16, 2022

seaside1 commented Oct 18, 2022

querdenker2k commented Oct 20, 2022

querdenker2k commented Oct 21, 2022

seime left a comment

seime commented Oct 22, 2022

querdenker2k commented Oct 22, 2022

querdenker2k commented Oct 22, 2022

seime commented Oct 22, 2022

querdenker2k commented Oct 24, 2022

seaside1 commented Oct 24, 2022

querdenker2k commented Oct 24, 2022

seaside1 commented Oct 24, 2022

seaside1 commented Oct 24, 2022

seaside1 commented Oct 24, 2022

querdenker2k commented Oct 24, 2022

seaside1 commented Oct 24, 2022

seaside1 commented Oct 24, 2022

querdenker2k commented Oct 24, 2022

querdenker2k commented Oct 25, 2022

querdenker2k commented Oct 25, 2022

seime commented Oct 25, 2022

querdenker2k commented Oct 25, 2022

querdenker2k commented Oct 25, 2022

seaside1 commented Oct 25, 2022

seaside1 commented Oct 25, 2022

seime commented Oct 26, 2022 •

edited

Loading

seaside1 commented Oct 26, 2022

querdenker2k commented Oct 26, 2022

seime commented Oct 31, 2022

querdenker2k commented Oct 31, 2022

seime commented Nov 2, 2022

seime commented Nov 2, 2022

Jrule when refactoring #61

Jrule when refactoring #61

Conversation

querdenker2k commented Oct 13, 2022

querdenker2k commented Oct 13, 2022

querdenker2k commented Oct 16, 2022

seaside1 commented Oct 18, 2022

querdenker2k commented Oct 20, 2022

querdenker2k commented Oct 21, 2022

seime left a comment

Choose a reason for hiding this comment

seime commented Oct 22, 2022

querdenker2k commented Oct 22, 2022

querdenker2k commented Oct 22, 2022

seime commented Oct 22, 2022

querdenker2k commented Oct 24, 2022

seaside1 commented Oct 24, 2022

querdenker2k commented Oct 24, 2022

seaside1 commented Oct 24, 2022

seaside1 commented Oct 24, 2022

seaside1 commented Oct 24, 2022

querdenker2k commented Oct 24, 2022

seaside1 commented Oct 24, 2022

seaside1 commented Oct 24, 2022

querdenker2k commented Oct 24, 2022

querdenker2k commented Oct 25, 2022

querdenker2k commented Oct 25, 2022

seime commented Oct 25, 2022

querdenker2k commented Oct 25, 2022

querdenker2k commented Oct 25, 2022

seaside1 commented Oct 25, 2022

seaside1 commented Oct 25, 2022

seime commented Oct 26, 2022 • edited Loading

seaside1 commented Oct 26, 2022

querdenker2k commented Oct 26, 2022

seime commented Oct 31, 2022

querdenker2k commented Oct 31, 2022

seime commented Nov 2, 2022

seime commented Nov 2, 2022

seime commented Oct 26, 2022 •

edited

Loading