eclipse-archived / smarthome

Eclipse SmartHome™ project
https://www.eclipse.org/smarthome/
Eclipse Public License 2.0
864 stars 782 forks source link

Cannot create rule based on template #3050

Closed kaikreuzer closed 7 years ago

kaikreuzer commented 7 years ago

I have this template defined:

[  
   {  
      "uid":"demo:energymeter",
      "label":"Energy Meter",
      "description":"Visualizes the current energy consumption.",
      "configDescriptions":[  
         {  
            "name":"consumption",
            "type":"TEXT",
            "context":"item",
            "label":"Consumption Item",
            "description":"Data source for current consumption",
            "required":true
         },
         {  
            "name":"light",
            "type":"TEXT",
            "context":"item",
            "label":"Color Item",
            "description":"Color light to use for visualisation",
            "required":true
         },
         {  
            "name":"max",
            "type":"INTEGER",
            "label":"Max. consumption",
            "description":"Maximum value (in W) for red light",
            "required":true,
            "defaultValue":1500
         }
      ],
  "triggers": [
    {
      "id": "trigger",
      "label": "Current consumption changes",
      "description": "Triggers whenever the current consumption changes its value",
      "configuration": {
        "itemName": "${consumption}"
      },
      "type": "core.ItemStateChangeTrigger"
    }
  ],
  "conditions": [],
  "actions": [
    {
      "inputs": {},
      "id": "setcolor",
      "label": "Change the light color",
      "description": "Sets the color to a value in the range from green (low consumption) to red (high consumption)",
      "configuration": {
        "type": "application/javascript",
        "script": "var power = trigger.event.getItemState().toString()\nvar percent = power / (${max} / 100) \nif(percent < 0) percent = 0\nvar hue = 120 - percent\nevents.sendCommand('${light}', hue +',100,100')"
      },
      "type": "script.ScriptAction"
    }
  ]
  }
]

When creating a rule through the REST API (using the Paper UI), this is what ends up in the rule registry:

[
  {
    "enabled": false,
    "status": {
      "status": "DISABLED",
      "statusDetail": "NONE"
    },
    "triggers": [
      {
        "id": "trigger",
        "configuration": {
          "itemName": "${consumption}"
        },
        "type": "core.ItemStateChangeTrigger"
      }
    ],
    "conditions": [],
    "actions": [
      {
        "inputs": {},
        "id": "setcolor",
        "label": "Change the light color",
        "description": "Sets the color to a value in the range from green (low consumption) to red (high consumption)",
        "configuration": {
          "type": "application/javascript",
          "script": "var power = trigger.event.getItemState().toString()\nvar percent = power / (${max} / 100) \nif(percent < 0) percent = 0\nvar hue = 120 - percent\nevents.sendCommand('${light}', hue +',100,100')"
        },
        "type": "script.ScriptAction"
      }
    ],
    "configuration": {
      "consumption": "Window_FF_Office_Door",
      "light": "Window_FF_Office_Door",
      "max": 1500
    },
    "configDescriptions": [
      {
        "name": "consumption",
        "type": "TEXT",
        "required": true,
        "readOnly": false,
        "multiple": false,
        "context": "item",
        "label": "Consumption Item",
        "description": "Data source for current consumption",
        "options": [],
        "filterCriteria": [],
        "limitToOptions": false,
        "advanced": false
      },
      {
        "name": "light",
        "type": "TEXT",
        "required": true,
        "readOnly": false,
        "multiple": false,
        "context": "item",
        "label": "Color Item",
        "description": "Color light to use for visualisation",
        "options": [],
        "filterCriteria": [],
        "limitToOptions": false,
        "advanced": false
      },
      {
        "name": "max",
        "type": "INTEGER",
        "required": true,
        "readOnly": false,
        "multiple": false,
        "defaultValue": "1500",
        "label": "Max. consumption",
        "description": "Maximum value (in W) for red light",
        "options": [],
        "filterCriteria": [],
        "limitToOptions": false,
        "advanced": false
      }
    ],
    "uid": "rule_1",
    "name": "Energy Meter",
    "tags": [],
    "visibility": "VISIBLE",
    "description": "Visualizes the current energy consumption."
  }
]

Note that

kaikreuzer commented 7 years ago

@danchom I didn't use templates for a while, so maybe I am missing something - would be great to have your feedback!

danchom commented 7 years ago

HI Kai, I know that there is a problem with templates (i.e. it is not resolved when the template is appeared after creating a rule) and i'm going to check them. @kaikreuzer could you please add the source of rule which uses this template and produce the wrong rule. Also is the problem appears only through the REST api or in any template usage?

kaikreuzer commented 7 years ago

add the source of rule

This is what the Paper UI sends as a POST to /rest/rules when doing a create "rule from template":

{
  "templateUID":"demo:energymeter",
  "name":"Energy Meter",
  "description":"Visualizes the current energy consumption.",
  "configuration":
  {
    "consumption":"Window_FF_Office_Door",
    "light":"Window_FF_Office_Door",
    "max":1500
  }
}
danchom commented 7 years ago

the placeholders are not replaced with the config values (or is this already a new feature?)

Yes this is a new feature. The rule registry persists rules as they are added. The references are resolved by the RuleEngine. In that way you always get what you have added and simplifies normalization and reference resolution. In the future this will give opportunity to update the rule just with update of its configuration.

the rule contains the config params and config descriptions

yes because they are part of added rule and if we remove them it will be impossible rule engine to resolve the reference to configuration properties and normalize them

the trigger is missing label and description

this is bug which I will fix

kaikreuzer commented 7 years ago

yes because they are part of added rule and if we remove them it will be impossible rule engine to resolve the reference to configuration properties and normalize them

Ok, I can see the reasoning behind it. BUT this means that UI editors are broken - they cannot handle that as the configurations do not have any valid value, but merely the placeholder.

I wonder if we might require to differentiate betweeen template configurations that should go hardcoded into the rule and the ones that should be kept as a rule configuration value.

We could also try to dynamically determine this: If a module is marked as HIDDEN, we could keep the placeholder, while any module that is meant to be displayed in the UI should also have "real" config values and no placeholder anymore. Not sure if this will cover all use cases, though.

What do you think?

kaikreuzer commented 7 years ago

@danchom Can you point me to the PR which introduced that feature? I would suggest to revert it for the moment, so that it is in sync again with the rule editor of the Paper UI - I will actually need it working again for demos by the end of this week... We can then discuss how to best deal with the different situations (hidden/visible) modules and implement support in the Paper UI at the same time.

danchom commented 7 years ago

Please try the fix from Ani, and check if it will fix the problem. I'm not sure if I catch the problem. When a rule is created by template and added, it can't return its configuration values, because the rule registry was stored them before the resolving procedure? I think the Ani's fix will help in that case, but we can't apply different configuration to this rule anymore. For example we can't update the rule applying different configuration to template. May be we can extend REST api with method returning resolved rules.

aounhaider1 commented 7 years ago

The update event after creation of a rule from template still contains the placeholders instead of actual configuration values. I tried debugging it and found that values are not replaced before the event is dispatched. Here is a screenshot:

screen shot 2017-03-01 at 14 10 21
kaikreuzer commented 7 years ago

@danchom @adimova Could you have a look at how to solve that issue?

danchom commented 7 years ago

Yes it is correct to send event when the template is resolved. At this point the rule is updated in rule registry - the remplateUID is removed and trigger condition and actions are added, but at this point the references are not resolved yet. When they are resolved there is no event, because the runtime rule is available only in memory and managed provider is not changed

kaikreuzer commented 7 years ago

trigger condition and actions are added, but at this point the references are not resolved yet.

Well, this is clearly a bug then (at least as long as we have #3090 in place).

kaikreuzer commented 7 years ago

Note: All I am trying to do is to get the existing rule editor working again for now - and for this, there must be no placeholders anywhere in visible modules.

danchom commented 7 years ago

Ok I'll try to fix this.

danchom commented 7 years ago

Could you explain what does not work in PaperUI, because I've created a rule from template? How can I reproduce the problem?

kaikreuzer commented 7 years ago

create a rule from a template and DON'T do any page reload - the very first version the Paper UI shows you is the one received as an event. But you can also simply set a breakpoint when sending the event and you will see the problem just like in https://github.com/eclipse/smarthome/issues/3050#issuecomment-283335929.

danchom commented 7 years ago

I think the PR#3097 will fix the problem. But IMHO this should be a temporary change.

kaikreuzer commented 7 years ago

I think the PR#3097 will fix the problem.

Thanks.

But IMHO this should be a temporary change.

Yes, so we should continue discussing a proper solution. I did a suggestion above already:

We could also try to dynamically determine this: If a module is marked as HIDDEN, we could keep the placeholder, while any module that is meant to be displayed in the UI should also have "real" config values and no placeholder anymore.

This would allow us to do "black box templates", i.e. create rules from templates, which do not expose their inner workings, but merely offer their configuration to the user. The hidden, internal modules would still have the placeholders, which are only replaced for the RuntimeRule. On the other hand, it also allows templates as a one-time skeleton for a complex rule, which is created in all its glory and left for further customization to the user - he might change modules, their configuration, etc and thus there must be no placeholders in the module configuration anymore.

What do you think about that?