The current API of the Parser imho does not make sense, especially the Status class.

My suggestion is to:

• change the Parser.importData() method to return a Set of RuleDTOs or throw a ParsingException with a list of errors (like Status.errorMap)
• remove the Status class completely (and introduce the ParsingException instead)

Events are currently completely missing.
Similar to the Thing infrastructure, we should have events for Rules like at least RULE_ADDED, RULE_UPDATED, RULE_REMOVED, RULE_STATUS_INFO.

The IntegrationTest "assert that a rule added by json is executed correctly" only sometimes gets green when executed in eclipse, but never when executed in maven. There seems to be a timing problem on startup. This has to be analyzed

Check all code to comply with https://www.eclipse.org/smarthome/documentation/development/guidelines.html.

The AbstractModuleHandler should resolve the ModuleTypeRegistry by itself during initialization. This reduces the overhead in the common implementations and makes the abstract method getModuleTypeRegistry obsolete.

I've defined a rule which contains composite trigger and composite condition (See below). It seems to fail on the trigger when it checks:

public void receive(Event event) {

 if (!event.getTopic().contains(source)) {

}

The source that I see is $itemName. It seems that the reference wasn't replaced with the item name: demo_switch_1_switch. Currently I cannot find in the source were the replacement is being done for composite trigger.

BTW, is it valid to define a rule without action (although I know that it doesn't make any sense).

The rule:

[
{
"uid": "smart.rule.composite",
"name": "SmartRule",
"tags": [
"smart",
"rule"
],
"description": "Smart Rule definition composite.",
"on": [
{
"id": "SmartTriggerID",
"type": "ItemStateChangeTrigger",
"config": {
"itemName": "demo_switch_1_switch"
}
}
],
"if": [
{
"id": "SmartConditionID",
"type": "ItemStateEvent_ON_Condition",
"config": {
"itemName": "demo_switch_1_switch"
}
}
],
"then": [
]
}
]

Move the BasicModuleHandlerFactory to the api Bundle and provide methods for common useCases here. According to Issue #48 the setter and unsetter could probably be removed as the ModuleTypeRegistry is resolved in the Handlers themselves.
Additionally the BasicModuleHandlerFactory should be able to control the lifecycle of the created ModuleHandlers.

Composite module types: CompositeConditionType, CompositeTriggerType and CompositeActionType should be supported by the automation rules

As for other components, we should have Groovy integration tests that test functionality on a running OSGi framework.

The automation module sources should not rely on Java7 classes and code constructs in order to be compatible with other JVMs

The new rule engine contribution must come with documentation

• explaining the overall concepts/ideas
• listing the ways for interaction (bundle resources, console, Java API, etc.)
• showing the JSON syntax (incl. reference to the JSON schema)
• providing examples for JSON as well as code

Hi,

See rule below. It seems that it fails "to satisfy" the condition. As far as I can see, it fails in ""EventCondition" because $event gets null in the inputs (which means event is null).

 @Override
    public boolean isSatisfied(Map<String, ?> inputs) {
        Event event = inputs.get("event") != null ? (Event) inputs.get("event") : null;
        if (event != null) {
            return isConfiguredAndMatches(TOPIC, event.getTopic()) && isConfiguredAndMatches(SOURCE, event.getSource())
                    && isConfiguredAndMatches(PAYLOAD, event.getPayload())
                    && isConfiguredAndMatches(EVENTTYPE, event.getType());
        }
        return false;
    }

The rule:

[
{
"uid": "smart.rule.composite",
"name": "SmartRule",
"tags": [
"smart",
"rule"
],
"description": "Smart Rule definition composite.",
"on": [
{
"id": "SmartTriggerID",
"type": "ItemStateChangeTrigger",
"config": {
"itemName": "demo_switch_1_switch"
}
}
],
"if": [
{
"id": "SmartConditionID",
"type": "ItemStateEvent_ON_Condition",
"config": {
"itemName": "demo_switch_1_switch"
}
}
],
"then": [
]
}
]

Currently, the rule context map is kept filled across the executions of the rule.

This can cause problems, because modules cannot know whether information is from the current execution or from previous ones.

Imagine two triggers T1 and T2, which trigger if Item A updates to ON, resp. Item B updates to ON.

If you know have:
A updates to ON -> T1 triggers, context is filled with A->ON
A updates to OFF -> No triggers, since nobody reacts on OFF
B updates to ON -> T2 triggers, context now contains A->ON, B->ON.

There are actually two problems:

1. A following module cannot determine which trigger has been triggered as both A and B are in the context.
2. The module might imply that A is ON, but this is a false assumption.

There are two problems (which may have one root cause):

1. the order of delivering the event to event handlers is not the same as the order in which the handlers were registered
2. when first event handler receives the event and queries the item state - the item state is not yet changed

The consequence of these two problems is that in some rules which are triggered on item state event and the rule condition checks the item state - this check fails because the item did not receive the event prior the rule trigger and rule condition actually checks the old item state

Here is a sample test code to check the event handling

        def eventHandler = [
            receive: { Event e ->
                logger.info("Event1: " + e.topic + " = " + e)
                logger.warn('after event1, myMotionItem.state=' + myMotionItem.getState());
            },
            getSubscribedEventTypes: {
                Sets.newHashSet(ItemStateEvent.TYPE)
            },
            getEventFilter:{ null }
        ] as EventSubscriber
        registerService(eventHandler)

///////////////////////
        def eventHandler2 = [
            receive: { Event e ->
                logger.info("Event2: " + e.topic + " = " + e)
                logger.warn('after event2, myMotionItem.state=' + myMotionItem.getState());
            },
            getSubscribedEventTypes: {
                Sets.newHashSet(ItemStateEvent.TYPE)
            },
            getEventFilter:{ null }
        ] as EventSubscriber
        registerService(eventHandler2)
///////////////////////
        def eventHandler3 = [
            receive: { Event e ->
                logger.info("Event3: " + e.topic + " = " + e)
                logger.warn('after event3, myMotionItem.state=' + myMotionItem.getState());
            },
            getSubscribedEventTypes: {
                Sets.newHashSet(ItemStateEvent.TYPE)
            },
            getEventFilter:{ null }
        ] as EventSubscriber
        registerService(eventHandler3)
///////////////////////

Then when event is posted
eventPublisher.post(ItemEventFactory.createStateEvent("myMotionItem", OnOffType.ON))
the messages printed are:

Event2: smarthome/items/myMotionItem/state = myMotionItem updated to ON
after event2, myMotionItem.state=NULL
Event1: smarthome/items/myMotionItem/state = myMotionItem updated to ON
after event1, myMotionItem.state=ON
Event3: smarthome/items/myMotionItem/state = myMotionItem updated to ON
after event3, myMotionItem.state=ON

The RuleResourceBundleImporter.setUID() method has the following problems that should be addressed:

• it uses the bundle id (vendor id), which can potentially change if a bundle is uninstalled and re-installed. Using the bundle name would imho be a better choice.
• rulesCount is declared static and will be incremented globally. This means that rules will get different UIDs if the startup order of bundle changes or a bundle in uninstalled. The count should instead be done individually per processed bundle.
• The rule name could potentially contain special characters that we would not want to have within the UID (which will e.g. be used as part of a URL in the REST API). We should therefore strip out any characters that we consider illegal for the UID.

ModuleTypeManager can return all modules by class type (getTypes(Class moduleType, Locale locale)). The issue is that if I want to get all TriggerTypes it will not return CompositeTriggerType because of the following line:

if (moduleType.getName().equals(mt.getClass().getName()))

Is it for purpose?

We currently have a very long chain:
RuleRegistry->RuleRegistryImpl->RuleManager->RuleManagerImpl->RuleEngine

Many of these simply delegate calls to the next level. We should simplify this to only have

RuleRegistry->RuleRegistryImpl->RuleEngine

The IntegrationTest "assert that a rule can be added by a ruleProvider" fails because when removing the ruleProvider the rules still exist in RuleRegistry. Expected is that the rules which are created by this RuleProvider are removed from the RuleRegistry as well.

Calling ruleRegistry.add(rule) for a rule without UID results in

Caused by: java.lang.NullPointerException: null
    at org.mapdb.BTreeMap.get(BTreeMap.java:595)
    at org.mapdb.BTreeMap.get(BTreeMap.java:591)
    at org.eclipse.smarthome.storage.mapdb.MapDbStorage.get(MapDbStorage.java:76)
    at org.eclipse.smarthome.core.common.registry.AbstractManagedProvider.add(AbstractManagedProvider.java:55)
    at org.eclipse.smarthome.core.common.registry.AbstractRegistry.add(AbstractRegistry.java:112)
    at org.eclipse.smarthome.automation.core.internal.RuleRegistryImpl.add(RuleRegistryImpl.java:76)
    at org.eclipse.smarthome.automation.core.internal.RuleRegistryImpl.add(RuleRegistryImpl.java:1)

The UID only seems to be created for the RuntimeRule, but it is not set in the Rule object itself before passing it to super.add(rule).

It should be equally fast to check the existence of the ESH-INF/automation resource to check whether a bundle has automation resources or not. Requiring a manifest header is a possible reason for problems, since people might tend to forget adding it.

The parser type should not be determined by the header, but by the resouce file extension.

Currently, the rule importer fails at startup, if the templates are not yet read from the bundle resources.
This should be changed to be stable, independent of the bundle startup order.

In the following method of AutomationCommandList class:

 protected String parseOptionsAndParameters(String[] parameterValues) {
        boolean getId = true;
        boolean getLocale = true;
        for (int i = 0; i < parameterValues.length; i++) {
            if (null == parameterValues[i]) {
                continue;
            }
            if (parameterValues[i].charAt(0) == '-') {
                if (parameterValues[i].equals(OPTION_ST)) {
                    st = true;
                    continue;
                }
                return String.format("[Automation Commands : Command \"%s\"] Unsupported option: %s", command,
                        parameterValues[i]);
            }
            if (getId) {
                id = parameterValues[i];
                getId = false;
                continue;
            }
            if (getLocale) {
                String l = parameterValues[i];
                if (l.contains("-"))
                    locale = Locale.forLanguageTag(l);
                else
                    locale = new Locale(l);
                getLocale = false;
            }
            if (getId && getLocale)
                return String.format("[Automation Commands : Command \"%s\"] Unsupported parameter: %s", command,
                        parameterValues[i]);
        }
        return SUCCESS;
    }

is used method: forLanguageTag(), which comes from jdk 1.7. I propose to remove support of language tag and the locale to be defined only by the language code.

In ModuleHandlerFactory we have to add the ruleId to the methods getHandler and ungetHandler to uniquely identify moduleHandlers. Also the implementation within the factories has to be adapted then.

The RuleRegistry must provide methods to get and set rules to enabled/disabled.
If StorageService is available, this information should be persisted in a separate Storage, otherwise it should be kept only in memory.
The "active" property should be removed from the rule JSON format. If a rule should be deactivated, this is done through a separate call to the RuleRegistry.

The IntegrationTest "assert that a rule created from a template is executed as expected" does sometimes not work because there seems to be a problem with the ResourceBundleProviders: while parsing the templates, they are validated, but the modules referenced in the template are not yet parsed. This causes that the template never will be loaded. There has to be a mechanism which tries to import failed templates again if moduleTypes are added.

Additionally here is the stacktrace:

13:29:19.753 [Automation Provider Processing Queue] ERROR o.e.s.a.p.json.TemplateJSONParser - [Template  UID : SimpleTestTemplate] Failed to validate connections of RuleTemplate with UID "SimpleTestTemplate"! Condition Type "ItemStateConditionX" does not exist!
java.lang.IllegalArgumentException: Condition Type "ItemStateConditionX" does not exist!
    at org.eclipse.smarthome.automation.core.util.ConnectionValidator.validateConditionConnections(ConnectionValidator.java:169) ~[org.eclipse.smarthome.automation.core/:na]
    at org.eclipse.smarthome.automation.core.util.ConnectionValidator.validateConnections(ConnectionValidator.java:51) ~[org.eclipse.smarthome.automation.core/:na]
    at org.eclipse.smarthome.automation.core.provider.TemplateResourceBundleProvider.importData(TemplateResourceBundleProvider.java:244) ~[org.eclipse.smarthome.automation.providers/:na]
    at org.eclipse.smarthome.automation.core.provider.AbstractResourceBundleProvider.processAutomationProvider(AbstractResourceBundleProvider.java:317) [org.eclipse.smarthome.automation.providers/:na]
    at org.eclipse.smarthome.automation.core.provider.AutomationResourceBundlesEventQueue.processBundleChanged(AutomationResourceBundlesEventQueue.java:316) [org.eclipse.smarthome.automation.providers/:na]
    at org.eclipse.smarthome.automation.core.provider.AutomationResourceBundlesEventQueue.run(AutomationResourceBundlesEventQueue.java:144) [org.eclipse.smarthome.automation.providers/:na]
    at java.lang.Thread.run(Thread.java:745) [na:1.7.0_55]

We now have the DTOs to be more or less the Java representation of our JSON structures. These DTOs will also be created by the REST API (Jersey/JAX-RS) when receiving requests. It makes sense to move the code from ManagedRuleProvider.toElement(RuleDTO) to some public helper, so that it can be used by others (e.g. the REST API).
In consequence, the Parser can be simplified to only parse into DTOs and we leave the rule construction to the caller of the parser.
Changing the Parser that way would mean it could be also independent from AutomationFactory - it really „just“ parses JSON to a Java object model.

I have a rule that gets imported through the bundle importer. This seems to happen before the according module types are loaded and I get:

13:07:04.479 [ESH-safeCall-3] INFO  smarthome.event.RuleAddedEvent - Rule 'javascript.rule1' has been added.
13:07:04.493 [Automation Provider Processing Queue] DEBUG o.e.s.a.core.internal.RuleEngine - Added rule 'javascript.rule1'
Exception in thread "Automation Provider Processing Queue" java.lang.NullPointerException
    at org.eclipse.smarthome.automation.core.internal.RuleEngine.setDefautlValues(RuleEngine.java:1137)
    at org.eclipse.smarthome.automation.core.internal.RuleEngine.resolveDefaultValues(RuleEngine.java:1127)
    at org.eclipse.smarthome.automation.core.internal.RuleEngine.setRule(RuleEngine.java:336)
    at org.eclipse.smarthome.automation.core.internal.RuleRegistryImpl.addIntoRuleEngine(RuleRegistryImpl.java:87)
    at org.eclipse.smarthome.automation.core.internal.RuleRegistryImpl.add(RuleRegistryImpl.java:78)
    at org.eclipse.smarthome.automation.core.internal.RuleRegistryImpl.add(RuleRegistryImpl.java:1)
    at org.eclipse.smarthome.automation.internal.core.provider.RuleResourceBundleImporter.importData(RuleResourceBundleImporter.java:213)
    at org.eclipse.smarthome.automation.internal.core.provider.RuleResourceBundleImporter.processAutomationProvider(RuleResourceBundleImporter.java:183)
    at org.eclipse.smarthome.automation.internal.core.provider.AutomationResourceBundlesEventQueue.processBundleChanged(AutomationResourceBundlesEventQueue.java:319)
    at org.eclipse.smarthome.automation.internal.core.provider.AutomationResourceBundlesEventQueue.run(AutomationResourceBundlesEventQueue.java:144)
    at java.lang.Thread.run(Thread.java:745)

The RuleRegistry should work without the ManagedProvider. See also comments in #63.

See discussion at #54 (comment).

I don't think that introducing an API interface (Converter) with pluggable services is the right choice for for this requirement.

Discussed in the Daily.

Pure junit tests should be added for implementation classes to get some decent code coverage.

ConnectionValidator.validateConnections() makes use of condition.getConnections().iterator() but a NullPointerException is thrown because condition.getConnections returns null instead of an empty collection.
Consequently returning empty collections instead of null will avoid those errors.

The current Localizer class should be replaced by using the I18NProvider interface, so that a localization implementation can be plugged in (for ESH in place already).

We have DTOs for almost everything, but not for ModuleTypes (e.g. no ActionTypeDTO).
From what I see, the reason is that ActionType etc. are already pojos, which are nicely serializable.
Still, this somehow brings some strange mix as the Parsers will return an object model that consists partly of DTOs and partly of classes from the API.

I see two ways to fix this:

1. Create ActionTypeDTO etc. classes that simply extend the ActionType and nothing more.
2. Remove the specific DTO classes altogether and instead use in general pojos from the API bundle. Since Action etc. are currently interfaces and not pojos, this would mean to change them to be classes (this could be easily done by moving the getters and setters from the ActionImpl etc. classes).

Looking at this, I think option 2 would very much simplify the architecture and move it towards what we had initially in mind (to have the API to be a representation of the JSON and not requiring additional DTOs). This would also go very well with option 2 of #58, since the rule registry could deal with the simply pojos and the RuleEngine could wrap them into Impl classes when instantiating them, connecting them and injecting handlers into them.

• RuleRegistry should only add a UID to a newly added rule, if none was provided.
• The BundleResourceRuleImporter should add a UID to the imported rules that is derived from bundle id, version and rule name, so that it is constructed the same way every time.
• The JSON parser must support the UID property on rules.

When adding two rules without UID, the second one fails with

java.lang.IllegalArgumentException: Cannot add element, because an element with same UID (rule_0) already exists.
    at org.eclipse.smarthome.core.common.registry.AbstractManagedProvider.add(AbstractManagedProvider.java:56)
    at org.eclipse.smarthome.core.common.registry.AbstractRegistry.add(AbstractRegistry.java:112)
    at org.eclipse.smarthome.automation.core.internal.RuleRegistryImpl.add(RuleRegistryImpl.java:81)
    at org.eclipse.smarthome.automation.core.internal.RuleRegistryImpl.add(RuleRegistryImpl.java:1)

The RuleEngine should be a proper instance that does not mix static from instance method calls, especially as the instance is disposable.

Setup an automatic build

I would like to have only the last part of the ID be treated as the ID of an customized moduleType. So that we can express a chain for BaseModuleType->CutomModuleType->MoreCustomModuleType as follows:

"BaseModuleType": {...}

"BaseModuleType:CustomModuleType":{...}

"CustomModuleType:MoreCustomModuleType":{...}

The Ids of the CustomModuleTypes should be only CustomModuleType and MoreCustomModuleType then. With this approach the CustomModuleType only needs to know its direct parent as it should be in the implementation as well.
In the rule where these moduleTypes will be used, we then only should specify the ID of the CustomModuleType as well.

The constructor of RuleImpl throws an IllegalArgumentException, if used templates or module types are not available in the system.
This can create a bunch of critical lifecycle problems:

• Rules can not be restored from the database, if the ManagedRuleProvider starts before bundles holding the templates or module types
  -The RuleBundleResourceImporter will fail the import, if a rule is declared in one bundle why the types are provided by a specific provider from another bundle.

I see two solutions to this problem:

1. Try to get the dynamic behavior under control, i.e. listen for things coming and going and retry instantiation, as soon as all dependencies are available. I think this might be quite nasty to implement.
2. Only use the DTO model within the RuleRegistry, which would cut all those dependencies. Move the instantiation problem to the RuleEngine, where the RuleImpl instances are really only required. This would allow the RuleRegistry to offer a consistent view on the rules in the system, regardless if their underlying module types are (still) available in the system or not.

I very much prefer option 2 as I think in option 1 rules can miraculously disappear from the system when module types disappear - they should rather be in an error status in this case.

Set the Execution Environment to OSGi minimal 1.2 for all automation bundles.
Keep compiler settings of IDE/Tycho at 1.7 as is.

The method SampleTriggerHandler.parse() appears to be a generally required code to resolve references in handler implementation, thus it should be moved to the AbstractModuleHandler.

I actually assume that this even applies to getSystemOutputsValues() as well?

As I understand, module type providers are added in a thread (AutomationResourceBundlesEventQueue runnable) different than the thread setRule (RuleEngine) is being called.

setRule calls ConnectionValidator which requires the providers for the rule validation process (ConnectionValidator.validateConnections). How do you make sure that providers are ready before setRule is completed?

(Mainly I refer to the case when the system starts and the rules are added via ruleRegistry.addProvider(rp);. In this case the module type providers are still not loaded).

The rule engine should come with some basic module type definitions and their handlers, like Item event triggers, thing status triggers, item command actions and item state actions.

In the package org.eclipse.smarthome.core.events;
there is a class AbstractEventFactory with dependency to com.google.gson.Gson;
this dependency should be moved outside this class

When implementing JavascriptModuleTypes, I came across a conceptional problem:

A generic "ScriptAction" type would not have any inputs defined, but it still needs access to the outputs of modules that ran before within the same rule (so that these values can be used within the script itself). As the available outputs depend on the concrete rule and its trigger modules, there is no chance for the ScriptAction type to define suitable inputs.

My suggestion would be to provide all module outputs in general as a rule context within the Map that it passed into module handlers (i.e. in execute(Map<String, ?> inputs)).
Using "." as a key within the map should hopefully prevent any clashes with the inputs.

Instead of different flags, there should be an aggregated RuleStatus (similar to ThingInfoStatus), which has primary values NOT_INITIALIZED, IDLE, DISABLED, RUNNING and sub-statuses like NONE, HANDLER_MISSING_ERROR, HANDLER_INITIALIZING_ERROR, CONFIGURATION_ERROR (see also ThingStatusDetail). Additionally, there should be a string message (like ThingStatusInfo.description), which is an english text providing details, which can be used in logs or admin user interfaces.

Internal packages should have the bundle name as a prefix, i.e. within the bundle org.eclipse.smarthome.automation.core' the packages should be 'org.eclipse.smarthome.automation.core.internal and not org.eclipse.smarthome.automation.internal.core.