openhab-core icon indicating copy to clipboard operation
openhab-core copied to clipboard

Introduce a generic scripting profile

Open J-N-K opened this issue 2 years ago • 23 comments

Closes #2201 Depends on #2852

This introduces a profile that can be used on all channel types and all item types with all available script languages. Input/Output values can either be State, Command or String. In the latter case the profile tries to convert the String to an acceptable State/Command for channel/item.

J-N-K avatar Mar 26 '22 09:03 J-N-K

Do you think these script profiles make script engine based transformations like transform.javascript obsolete? I'm asking this because of https://github.com/openhab/openhab-addons/issues/12359.

I think a generic "SCRIPT" transformation provided by the core probably still makes sense so you can pick any scripting language of your chosing for labels and within rules of other scripting languages (i.e. bullets 1,2,3 in the Transformation Usage docs).

wborn avatar Mar 26 '22 12:03 wborn

Good idea. I need to check how this can be implemented.

J-N-K avatar Mar 27 '22 10:03 J-N-K

I re-added WIP because I would like to make use of the new transformation registry for providing the scripts.

J-N-K avatar Apr 16 '22 07:04 J-N-K

@openhab/core-maintainers It would appreciate if this could make it in the next milestone so we have the chance to remove bugs before the release.

J-N-K avatar May 26 '22 07:05 J-N-K

I agree and planned to look at it the next days!

kaikreuzer avatar May 26 '22 16:05 kaikreuzer

In fact, it's not a script you put there but the reference to a script:

  • js:myscript.js for a JavaScript script in conf/transform (provided by the FileTransformationConfigurationProvider)
  • js:config:js:myscript for a managed JavaScript script (provided by the ManagedTransformationConfigurationProvider, which unfortunately has no UI support yet - this is where I would expect the script editor)

This makes it easy to re-use the same script at different places (e.g. as transformation in the sitemap or a rule and also as a profile). An example could be a mathematical calculation like a common logarithm or a script for complex formatting of a numeric value to a string.

The information of available scripts that can be used is already available in the transformation service (similar to what the MAP transformation/profile does), so the whole code in the transformation would need to be duplicated in the profile. IMO this doesn't make much sense, it's better to have it only at a single place with good test coverage.

J-N-K avatar May 29 '22 21:05 J-N-K

I am not convinced that we should further promote transformation services and their syntax here. Transformations are a construct from openHAB 1.x, mainly useful for textual configurations in item or sitemap files. They never made it to be supported in the UI. Instead, script support in the UI was done as shown above in the rule editor, which is imho pretty user friendly. For those reasons, I'd want to avoid adding now a dependency from profile to transformation here. It also feels wrong to me to put the profile scripts in the "conf/transform" folder.

I'd be interested in the opinions from @rkoshak and @ghys (and of course the other @openhab/core-maintainers) from a user perspective here.

kaikreuzer avatar May 30 '22 05:05 kaikreuzer

I don't think abandoning transformations is the way to go. Transformations are very common in a lot of places: some bindings (like MQTT, HTTP probably being the most popular), in sitemaps and in rules. They should have full UI support instead and with #2821 we added core-support to create transformation configurations in UI.

Also I don't think that adding the same script in a lot of places is user-friendly, as it requires manually changing the script at a lot of places. E.g. a transformation that formats a number of seconds to "HH:MM" format could be easily re-used and if you would like to change from "09:32" to "9:32", this should be super-easy to change it in one place.

Looking at the place where to add the files: I would prefer not to have to add a file at all (i.e. UI support as pointed out above), but having the profile "configuration" in the transform folder is an already known concept: org.openhab.transform.javascript, org.openhab.transform.map, org.openhab.transform.scale already use it. Removing that would not be easy to explain.

J-N-K avatar May 30 '22 06:05 J-N-K

Transformations are critical for certain low level bindings like MQTT, HTTP, Serial, etc. where the data comes in as a big XML, JSON, or some other structured blob of text that needs to be parsed to pull out the desired values.

In addition, one of the few things that profiles are good for (controversial opinion, I dislike profiles because there is too much overlap between them and rules which causes confusion on when it's appropriate to use one or the other) is taking an event from a Channel and transforming it to some other type of Item. Thus, one can, for example, change those read-only Switches from the Network Binding to Contacts (like they should have been in the first place) or add units or strip units from a numerical value and the like.

If I were to redesign the transformation service, I'd take what's done in the MQTT/HTTP binding with the filtering and chaining, pull that into the transform profile, and remove the binding/binding custom implementation of transformations so that all channels on all bindings can be transformed and it works the same everywhere. But that's a different discussion and I'm sure would cause other problems.

As for this PR, my biggest concern is it widens the overlap between rules and profiles. It will be used as a replacement for rules by some users, guaranteed. But I don't think that outweighs the benefits of having a scripting profile like this. There are many use cases where a profile really is the right way to go and being able to harness more than just JavaScript would be incredibly useful.

rkoshak avatar May 30 '22 14:05 rkoshak

@rkoshak I didn't want to trigger any discussion about dropping transformations here. My point was rather that profiles are closer to rules than to transformations and they especially require decent UI support. Having to put js:config:js:true into a plain text input field as a configuration vs. putting true into a script editor in the UI imho makes a huge difference. I'd assume users to be confused, if ScriptActions in rules need a completely different configuration syntax then what the Script profile expects. If we were to focus on textual configuration in DSLs, I'd be fine with transformation syntax, but I'd think in the UI we should do better.

kaikreuzer avatar May 30 '22 14:05 kaikreuzer

My point was rather that profiles are closer to rules than to transformations and they especially require decent UI support.

From the end user's perspective ignorant of any of the implementation details, I'm not sure that's the case. Except for the follow profile, all the rest are primarily focused on transforming a channel event in some way before it gets to the Item (e.g. taking any event and transforming it into a date time, performing hysteresis, out of range detection, etc.). It's a different kind of transformation than what we'd traditionally have in the transformation service since some really complicated behaviors are handled (e.g. converting a number value to an ON/OFF based on a hysteresis) but it's still a transformation when all is said and done (except for the pesky follow profile which isn't and I'm not sure what to make of that).

I agree that good UI support will be needed but I don't really see opening it up to where any of the installed scripting languages can be used instead of just ECMAScript 5.1 only (i.e. JavaScript Trnsformation) changes things fundamentally. In fact it helps since end users won't have to learn a new language from what they are using in rules.

As for the confusion, the confusion already exists because of the JS Transformation. I don't see this making that any worse. I also don't see it making that any better either. Of course, anything that can be done to make these script profiles look (from a UI perspective) like a rule Script Action the better from an end user's perspective.

rkoshak avatar May 30 '22 15:05 rkoshak

Let me add some points regarding the configuration:

  1. I have removed the need for the first js:, this information is already present. I have creates #2987 for that.
  2. For a script in files, you only have to put the filename in the configuration, nothing else. This is exactly what we have for the other profiles.
  3. For managed configuration (which as I said above already has core support, but no UI support yet), the value is config:<scripttype>:<some_id>. My guess would be that someone who wants to configure a profile in an .items file also puts his scripts in files, so (2) is more likely what would be used.
  4. For UI configuration of the profile, a drop-down list is presented with all available configurations, that is what the ConfigOptionProvider is used for, so nothing has to be entered manually at all, just selected from the list.

J-N-K avatar May 30 '22 15:05 J-N-K

Thank you both for the insights, that's ok for me.

For UI configuration of the profile, a drop-down list is presented with all available configurations, that is what the ConfigOptionProvider is used for, so nothing has to be entered manually at all, just selected from the list.

This might actually be the reason for my confusion: I don't see any drop-downs, but as mentioned above just a plain text input field:

Screenshot 2022-05-30 at 22 55 55

Without any further documentation, I as a user am completely lost here of what I should enter and in which format. What kind of values should I see here in the dropdown?

kaikreuzer avatar May 30 '22 20:05 kaikreuzer

If you put

var restStunden = Math.floor(inputString / 3600)
var restMinuten = Math.floor((inputString - restStunden * 3600) / 60)
restStunden + ":" + ("00" + restMinuten).slice(-2)

into a file time.js in the conf/transform folder and copy it once it should look like this:

Bildschirmfoto 2022-05-30 um 23 27 59

~I guess it needs some fine-tuning in the configuration description, because currently you cannot set "none" after you select an option.~ If you point me where to put the documentation, I'll add something there (this also needs to be done for the script transformation).

I also found your issue: If no config option is present, the input is rendered like in your example. @ghys, is there any way to force the UI to display an empty list instead of the text input box?

J-N-K avatar May 30 '22 21:05 J-N-K

If you put ... into a file time.js in the conf/transform folder ...

Can I do the same with a DSL file? What would be the file extension for that?

@ghys, is there any way to force the UI to display an empty list instead of the text input box? For managed configuration (which as I said above already has core support, but no UI support yet), the value is config::<some_id>.

I don't understand yet, how you envision the UI support here. We can either have a script context that allows storing scripts (managed) with the profile or we can have options that provide files that are found in the textual configuration (transform folder). Offering both side by side for the same config parameter sounds difficult to me. And if we'd want to do so, I think we should align it with the ScriptAction and ScriptCondition. Note that the "script context" of a config parameter not only offers the script editor for any installed language, but also Blockly as a choice for users.

kaikreuzer avatar Jun 01 '22 18:06 kaikreuzer

After #2990 is merged, you can also do it with DSL and the extension is .dsl. In fact files with that extension are already shown in the list if they are found (because that is reported by the DSLScriptEngineFactory as script type), but it does not work because inputString is not available when the script is parsed. I missed that when I implemented #2883.

It is already possible to use UID and filename in the same input, that is what was implemented in #2821 (configurations supplied from files have the filename as UID, managed configurations have their given UID in the format config:<type>:<id>. It'll only fail in the rare case that a filename would be something like config:map:openclose.map (which is not a valid filename on windows, anyway). But I doubt that someone will use filenames like that. There are two providers that feed the TransformationConfigurationRegistry, one for files in conf/transformation, one for managed configurations stored in a JSON database. They can all be accessed from the REST API under transformations.

My UI idea would be a new section Transformations below Items and above Pages in the Configuration section. If you open that, a list of all transformation configurations (file-based and managed) is shown (similar to the list of items on the Items page). You can then

  • show the file base configuration
  • show/edit a managed configuration

in a view similar to what you see when you edit script actions on the rules page. There should also be a "+" button to add a new managed configuration. if you add a managed configuration you have to select the type (either the mime-type or the file extension), the id part of the UID (which should be suggested by the UI like for adding a new thing), a label and of course the content. The UID is then constructed with config:<type>:<id> like the UID is constructed for things with <binding>:<type>:<id>. The type should be freely selectable, but have pre-defined values for known extensions/types like in the script action.

Except the UI part for adding the managed configuration, this is technically already implemented for the script transformation and the MAP transformation and works.

If you PUT

{
  "uid": "config:js:time",
  "label": "Seconds To Hour Minute Converter",
  "type": "js",
  "content": "var restStunden = Math.floor(inputString / 3600)\nvar restMinuten = Math.floor((inputString - restStunden * 3600) / 60)\nrestStunden + \":\" + (\"00\" + restMinuten).slice(-2)\n"
}

with a UID config:js:time to transformations in the API Explorer, you'll see another option labelled "Seconds To Hour Minute Converter". In the same way you can add configurations for the MAP transformation.

J-N-K avatar Jun 01 '22 19:06 J-N-K

Sleeping over it, I think I now understand that we are both having different things in mind and thus have problems following the arguments of each other.

If I am not mistaken, what you actually implemented is not a scripting profile, but a generic transformation profile. Also, when you talk about "managed" stuff, you are actually talking about managed transformations, not managed scripts.

As @rkoshak pointed out here, it is already possible to use the existing script transformation services for profiles. Why exactly would we need a second mechanism for this? The main difference that I see is that the current ones only allow one direction and not both, but that could probably be easily changed. Also, we could enhance those transformation profiles to allow referring to managed transformations. I could imagine that we introduce a new context "transformation", which the UI could then support and allows selecting transformations from the managed provider as well as from what is found in the "conf/transform" folder.

My interpretation of a "ScriptProfile" is what I sketched out here: "simply takes scripts as configuration parameters (similar to the ScriptActions of the rule engine). [...] create such flexible profiles directly in the UI and using its script editor." I simply assumed that this is what you implemented with this PR and was thus very confused about the solution.

I am not against a generic transformation profile, but I think it can exist besides a ScriptProfile as both refer to different concepts (while having an intersection by the fact that there are scripting transformations available). And I'd like to understand, what benefit the generic transformation profile has over the already existing specific ones - or would it be meant as a replacement and we remove the specific profiles from the transformation services?

kaikreuzer avatar Jun 02 '22 08:06 kaikreuzer

That may well be :-)

As far as I can see there is no generic transformation profile, instead all transformations implement their own profile, e.g.:

  • https://github.com/openhab/openhab-addons/tree/main/bundles/org.openhab.transform.map/src/main/java/org/openhab/transform/map/internal/profiles
  • https://github.com/openhab/openhab-addons/tree/main/bundles/org.openhab.transform.javascript/src/main/java/org/openhab/transform/javascript/internal/profiles
  • https://github.com/openhab/openhab-addons/tree/main/bundles/org.openhab.transform.regex/src/main/java/org/openhab/transform/regex/internal/profiles

This is what I did here. The profile here only works for script transformation configurations (and only those will be displayed). Consolidating all of these profiles into a TRANSFORM profile would be preferable, but a completely different task. But you have a point there: the ProfileUID should be aligned with the other transformation profiles, so the transition to the generic transformation profile can take place without breaking user configurations.

The benefit of this profile: there currently is only one scripting profile, this is the https://github.com/openhab/openhab-addons/tree/main/bundles/org.openhab.transform.javascript transformation with its associated profile. However, this will not be available when we switch to supporting Java 17 as it depends on Nashorn. This profile (and its associated transformation) allows to use every scripting language. So it is meant to replace the JavaScript transformation and extend it to all other available languages (including DSL).

Regarding the configuration: I think we can have both: If the UI renders an "Add new configuration" button next to the configuration selection list, you can immediately add a new (managed) script there and use it. The only difference would be that you need to add a name to the script.

Edit: In fact I believe that the generic transformation profile you propose in combination with the "Add new configuration" button would be a big step forward. There would be a single profile which can make use of all installed transformations and there would be UI support for adding new configurations independent of the transformation type. I'll put that on my to-do list, but I fear this can't be completed before 3.3.0 is out. To allow the transition from the old JavaScript profile to this profile (which can be consolidated without user interaction to the new transformation) it would be preferable to have it in 3.3.0.

J-N-K avatar Jun 02 '22 08:06 J-N-K

Very sorry for answering so late, but I fell sick inbetween and was unable so sort a single thought in my head...

there is no generic transformation profile [...] all transformations implement their own profile

Right, that's why I called your PR here to be a "generic" solution, while I called the existing ones the "specific" solutions.

This profile [...] is meant to replace the JavaScript transformation and extend it to all other available languages

We should avoid mixing the notions of profiles and transformations. I get the point that the current JS transformation is limited to Nashorn and it makes sense to replace it by a broader "scripting transformation". With this, we should probably take special care on how to deal with filename extensions in the conf/transform folder, because the current convention is that the file extension corresponds to the applicable transformation service. This folder imho should not end up as yet another place to put scripts (we already have a few).

Making transformations available as profiles, I think a generic transformation profile that supports any kind of transformation and not just scripted transformation makes a lot of sense. I wouldn't see a good reason to exclude certain transformation types from it.

I fear this can't be completed before 3.3.0 is out.

I agree, especially as I think we should role this only out in a release, when we also have the UI support in place for it. I'd therefore suggest to target this for 3.4, which gains us time to do it in a nice way and have some user feedback rounds as well.

To allow the transition from the old JavaScript profile to this profile it would be preferable to have it in 3.3.0

The current JS profile is tight to the JS transformation and it will still be there - and it comes with suitable UI support already. I would hence prefer to stick to the above plan and only ship the final solution in an official release. This reduces the risk of having to roll out breaking changes to the users too often. Would you be fine with that?

kaikreuzer avatar Jun 04 '22 10:06 kaikreuzer

I agree that profiles and transformations are two different things. In case of JavaScript Transformation/Profile and the (already merged) Script transformation and the profile proposed here, they are somewhat linked.

To be more precise:

  • The script transformation merged in #2883 is intended to replace the JavaScript transformation and extend it to all other scripting languages.
  • The profile proposed here is intended to replace the JavaScript Profile (which is located in the same bundle as the transformation) and extend it to all other scripting languages.
  • Both (the JavaScript Profile and the Script Profile) are internally using the associated transformation service to reduce duplicated code.

So my point why it is like it is:

  • I don't see any good reason not to re-use existing, working, well-tested code instead of implementing exactly the same functionality a second time.
  • Re-using the transformation configurations as configurations in a profile is a concept that is already implemented for all other transformations and well accepted and understood by the user.

I also fail to see where the exiting JavaScript transformation has better UI support than what is proposed here:

Bildschirmfoto 2022-06-04 um 19 47 27

This is exactly the same, the only difference is that it ALSO allows in-line scripts when the configuration starts with '|'. That could be easily added.

I'm okay with moving it to 3.4 but I doubt that this will bring us any closer to a solution.

J-N-K avatar Jun 04 '22 17:06 J-N-K

Latest code renders the UI like this:

Bildschirmfoto 2022-06-04 um 22 16 09

Two configurations (the ones with ".script") are added via files, the other one was added manually via the REST API. The red dots mark the place where I would like to see a "Add new script" button, to add new managed configurations. The editor should be the same you see when you add a script action to an UI based rule.

J-N-K avatar Jun 04 '22 20:06 J-N-K

The script transformation merged in https://github.com/openhab/openhab-core/pull/2883 is intended to replace the JavaScript transformation

At the moment it exists in co-existence, but in contrast to the JS transformation without any documentation (since it is not implemented as an add-on in the add-ons repo, although transformation services are an add-on type, so we have a pretty weird situation here, which we might want to clean up).

We should also agree on what "intended to replace" means: Do we want them to coexist or do we want it to replace it immediately to not have two competing implementations in place at the same time. In case of the latter, we need to communicate this to the user and should provide some migration path (either automatic or manual).

The profile proposed here is intended to replace the JavaScript Profile

As discussed above: I fully agree that it should replace the JS (transformation) profile, but with it not only this, but all other specific transformation profiles, so that the new one is a full replacement, i.e. a generic transformation profile. Here again, we will need to find a way how to migrate existing users from the specific profiles over to the generic profile.

Besides this new transformation profile in this PR, I would imagine to have additionally a new Script profile as it had been suggested in https://github.com/openhab/openhab-core/issues/2201, i.e. using UI script editor, Blockly, etc. But this can clearly be implemented fully independently and won't raise any issues wrt migration as this does not yet exist.

It is mainly the question on how to properly migrate users to the new transformation profile and to make sure that the new format is stable and won't require another breaking change after releasing, which makes me think that it'll be safer to not rush anything right now, but do it cleanly and with a proper plan for 3.4.

kaikreuzer avatar Jun 05 '22 17:06 kaikreuzer

"Replacement" here means: The JSTransformation/Profile will automatically be removed once we switch to Java 17 (because it depends on Nashorn). I would prefer to have this one in place BEFORE this happens, so users don't have to migrate immediately when upgrading, but can do that on their own schedule. But as I said before: I'm fine with delaying it to a time after the release. #2990 is independent from that and should be merged.

I still don't understand what the difference to your script profile is. It's just an UI issue to present the correct editor for adding new managed configurations in the right place. Then it's exactly the same like adding a script action to a rule in UI, it can even be blocky since blocky in the end creates a JS script which can be used here. The blockly editor just needs to be extended to allow using the input variable.

I did check if it is possible to make a generic transformation profile that can be used with all transformations (like MAP, SCALE, REGEX, XPATH, SCRIPT). It's not easy to do that (especially for UI), because depending on the selected transformation the UI would need to render different configuration parameters (e.g. MAP and SCALE provide a list of options, REGEX needs a regular expression, XPATH needs a path and a source format, SCRIPT needs the script type and provides a list of options).

J-N-K avatar Jun 05 '22 18:06 J-N-K

@kaikreuzer Any chance for this to make it into 3.4?

J-N-K avatar Oct 27 '22 05:10 J-N-K

Yesterday I wrote #3135 without realizing this PR existed (even though I've been pointed to it before! Whoops, I must have a bad short term memory.) My PR doesn't reuse transformations, but instead reuses the rule engine infrastructure. I've looked over this one a bit, and want to make some comparisons:

  • Because this one uses the transformation service, you can only pass in one variable (and not even a named one). Passing in the callback, as well as the ProfileContext (or at least the configuration from the ProfileContext) would allow the script to be far more useful and configurable. Also, in this PR's current iteration there's no way for the script to tell if it's being passed a command or a state update (though that might not be a problem - I'll admit I don't fully understand how/why a binding can send a command towards the framework; an item sending an update is more common, but most profiles don't do anything with that, though some do).
  • Because transformations can only return Strings, there's an extra step of converting back to a command (or even worse a state, with extra special cased code). This round-trip isn't necessarily lossless - especially for StringItems where you can't tell if NULL is supposed to be UnDefType or StringType.
  • I really like the script picker in the UI from your screenshot in https://github.com/openhab/openhab-core/pull/2872#issuecomment-1146680107. I don't see how you enabled that though.
  • When using a script, MainUI already has full UI support for creating and editing them.
  • Why don't we just implement a TransformProfile, that allows using any transformation as a profile, allowing us to remove all of the *TransformationProfiles? That would also include the script transformation (which, agreed, needs better documented. Maybe I'll go make a commit to openhab-docs for that, since I explored using it already with ruby). I could see having both a ScriptProfile (my PR) that is more flexible for the reasons I listed above, and a TransformationProfile that accomplishes what this PR is looking to do (and more). The only downside is possibly a slightly more clunky UI. But if transformations are going to stay around, it'd be nice to have better UI support for them anyway. Especially when they're used on channels like in the MQTT binding as @rkoshak mentioned at https://github.com/openhab/openhab-core/pull/2872#issuecomment-1141223733.

ccutrer avatar Oct 27 '22 14:10 ccutrer

@ccutrer As I said above: It's very difficult to create a generic transformation profile, even if I would prefer to do that. The reason is that the transformations have different parameters and UI would need to be adapted in a way that depending on the selected transformation different configurations are shown.

J-N-K avatar Nov 01 '22 08:11 J-N-K

@kaikreuzer Any chance for this to make it into 3.4?

Sure, let's try that. But I first have to get my head around what exactly "this" is now supposed to be. 😄

I could see having both a ScriptProfile (my PR) that is more flexible for the reasons I listed above, and a TransformationProfile that accomplishes what this PR is looking to do (and more).

This is more or less what I argued above as well, although as a ScriptProfile, I would prefer the "inline" version over #3135, but this could possibly be best discussed in #3135 then. In consequence, this PR here should not be about a "generic scripting profile", but about a "generic transformation profile".

I see your point @J-N-K that a full UI support of such a profile can be difficult to achieve. But a first step could be a simple string representation. After all, this is what all transformation add-ons describe in their documentation and how transformations are used within DSLs as well as in scripts. Having this string syntax being parsed and supported by the UI would for me be a (nice-to-have) second step that should not prevent the introduction of such a profile. Wdyt?

kaikreuzer avatar Nov 03 '22 19:11 kaikreuzer

If this shall now be a generic transformation profile, then that is a completely different PR. And good UI support is why I created it the way it is now.

J-N-K avatar Nov 03 '22 20:11 J-N-K

@ccutrer As I said above: It's very difficult to create a generic transformation profile, even if I would prefer to do that. The reason is that the transformations have different parameters and UI would need to be adapted in a way that depending on the selected transformation different configurations are shown.

Considering there is not currently deep UI support for transformations anywhere, it seems we could have two choices: just build a generic transformation profile with limited UI support. I.e. the same as the MQTT binding's transformation editor, where it just gives you a text box, and you have to know transformation syntax yourself. Or we build a deep UI experience for transformations concurrently with a generic transformation profile, and later we could have other parts of the UI use that (state patterns, MQTT binding configuration, etc.). It feels like you're pushing that we shouldn't release a transformation profile without a deep UI experience though?

ccutrer avatar Nov 03 '22 22:11 ccutrer

This is clearly a step backward. Currently the file-based transformations are provided by a ConfigOptionProvider to the profile. That is not possible any longer if we merge all in one profile because the profile cannot decide which options to show as it has no knowledge which is the correct ConfigOptionProvider.

J-N-K avatar Nov 04 '22 15:11 J-N-K