Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a generic script transformation #2883

Merged
merged 3 commits into from
Apr 24, 2022

Conversation

J-N-K
Copy link
Member

@J-N-K J-N-K commented Mar 30, 2022

Depends on #2821
Related to #2872

Signed-off-by: Jan N. Klug [email protected]

@J-N-K J-N-K requested a review from a team as a code owner March 30, 2022 07:50
@wborn wborn added the enhancement An enhancement or new feature of the Core label Apr 1, 2022
@wborn wborn added awaiting other PR Depends on another PR rebuild Triggers the Jenkins PR build and removed awaiting other PR Depends on another PR rebuild Triggers the Jenkins PR build labels Apr 10, 2022
@J-N-K J-N-K force-pushed the feature-scriptingtransformation branch 2 times, most recently from 255c8b2 to ca8c400 Compare April 11, 2022 17:06
@J-N-K J-N-K closed this Apr 11, 2022
@J-N-K J-N-K reopened this Apr 11, 2022
@J-N-K J-N-K closed this Apr 12, 2022
@J-N-K
Copy link
Member Author

J-N-K commented Apr 12, 2022

I don‘t understand why this happens. Every second local build fails if I don‘t use -DwithResolver=true and the same bundles are changed back and forth with every new resolving build.

@J-N-K J-N-K reopened this Apr 12, 2022
Copy link
Member

@wborn wborn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR! I finally got some time to test it and got it working with my JSScripting (Nashorn) add-on on Java 17. 👍 I did find a small issue while testing:

@wborn
Copy link
Member

wborn commented Apr 24, 2022

One additional functionality that the existing JavaScript transformation has is that you can supply additional parameters used in scripts.

See also the examples documentation:

As input name is reserved for transformed data, it can't be used in query parameters. Also ? and & characters are reserved, but if they need to passed as additional data, they can be escaped according to URI syntax.

transform/scale.js:

(function(data, cf, d) {
return parseFloat(data) * parseFloat(cf) / parseFloat(d);
})(input, correctionFactor, divider)
transform/scale.js?correctionFactor=1.1&divider=10

Maybe it is also possible to implement this so this new transformation has feature parity and there is no good reason to keep using the existing JavaScript transformation. 😉

@J-N-K
Copy link
Member Author

J-N-K commented Apr 24, 2022

Good idea. The nice thing is that this also works with other languages.

@wborn
Copy link
Member

wborn commented Apr 24, 2022

Yes it works very well! So far I've tested Groovy, JS (Graal) and JS (Nashorn) transformations in parallel on Java 17. 🙂

J-N-K added 3 commits April 24, 2022 18:23
Signed-off-by: Jan N. Klug <[email protected]>
Signed-off-by: Jan N. Klug <[email protected]>
@J-N-K J-N-K force-pushed the feature-scriptingtransformation branch from 9d4827f to fbdd2c9 Compare April 24, 2022 16:23
@J-N-K
Copy link
Member Author

J-N-K commented Apr 24, 2022

There is one other feature missing, but I'm not sure if we should implement that: inline scripts.

@wborn
Copy link
Member

wborn commented Apr 24, 2022

Maybe that is something for a follow up PR.
We could for instance use the inline script when the function has a positive index for "|" which is smaller as ":"

E.g. JS(| input / 10) would become SCRIPT(js| inputString / 10)

In that case the function value could be used as key for the scriptCache. But it would probably also be a good idea to then track if scripts are still in use... otherwise the cache will likely become yet another memory leak. 😮

Copy link
Member

@wborn wborn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Many thanks! 👍

@wborn wborn merged commit a192a1d into openhab:main Apr 24, 2022
@wborn wborn added this to the 3.3 milestone Apr 24, 2022
@J-N-K
Copy link
Member Author

J-N-K commented Apr 24, 2022

Your last comment brought my attention to potential leaking memory when script engines are not properly disposed. In ScriptEngineManagerImpl we check if the ScriptEngine implements AutoClosable and call close in that case. I was just implementing that, when you pressed "merge". Do you think it's worth making a new PR for that?

@wborn
Copy link
Member

wborn commented Apr 24, 2022

Yes it would certainly help to prevent any potential memory leak. 👍

@J-N-K J-N-K deleted the feature-scriptingtransformation branch April 24, 2022 17:09
@florian-h05
Copy link
Contributor

Is this actually documented anywhere?

@J-N-K
Copy link
Member Author

J-N-K commented Dec 2, 2022

Unfortunately there is no good place where to put that documentation. I can provide the content if someone finds a place to put it.

@rkoshak
Copy link

rkoshak commented Dec 2, 2022

I would expect it to find information about this in the Transformations page under Configuration, unless I'm misunderstanding what documentation is being asked about here. That's at least the first place I'd personally look for documentation about transformation capabilities provided by core.

@J-N-K
Copy link
Member Author

J-N-K commented Dec 2, 2022

IMO it doesn't fit very well here because this is only a general documentation that does not go into detail about specific transformations. And it can't be added to this page because that is generated automatically from the openhab-addons repository.

@rkoshak
Copy link

rkoshak commented Dec 3, 2022

Just because the Transformations Configuration is generic now doesn't mean we can't go as specific as necessary. I don't think this warrants inventing a whole new page to support it. I agree that we can't put it under add-ons not only because it's generated automatically but also because this isn't an add-on, it's part of core.

I look at this similarly to Ephemeris. The docs for it didn't really "belong" anywhere either. But short of creating a new page just for it, placing the docs for it on the Actions page was the best we could manage. I'm still not happy with that but it was the best we could do.

I think this is a similar situation. It doesn't really fit the page now but there really isn't anywhere else to put it. And it's not wholly unexpected to look there. The docs shouldn't need to go into to too many specific details on syntax. We can push that on the references for the specific languages I think. But the generic stuff like "it supports all the languages installed", the files need to go into the transformation folder, the tranformation should be wrapped in a function that takes one argument and returns the result as Strings (is this true?), stuff like that. An example or two would be nice but may not even be necessary.

@florian-h05
Copy link
Contributor

I think this is a similar situation. It doesn't really fit the page now but there really isn't anywhere else to put it. And it's not wholly unexpected to look there. The docs shouldn't need to go into to too many specific details on syntax.

I totally agree on Rich’s opinion here, IMO it is not the perfect place. This feature is just to good to keep it undocumented because it’s docs do not really fit into the existing docs. So, let’s put a general, language-independent section about the generic script transformation with one/two examples there, I can then take care of adding a example to the JS Scripting add-on docs.

@florian-h05
Copy link
Contributor

@J-N-K Does the SCRIPT transformation work for you when you follow your docs? For me it is always failing with Failed transforming the state 'hello' on item 'test_string' with pattern 'SCRIPT(dsl:stringlength):%s': Could not get script for UID 'stringlength'.

@J-N-K
Copy link
Member Author

J-N-K commented Dec 4, 2022

No. But SCRIPT(dsl:stringlength.script):%s does.

@florian-h05
Copy link
Contributor

florian-h05 commented Dec 4, 2022

Thx, now it‘s working. The docs were not clear enough to me, maybe I should improve them a little bit.

The SCRIPT transformation is using Nashorn, right?
How can I use JS Scripting with it?

@J-N-K
Copy link
Member Author

J-N-K commented Dec 4, 2022

Use application/javascript;version=ECMAScript-2021 as script type. Maybe an alias would be nice, but that needs to be implemented in the add-on (here). Something like graaljs would be a good fit IMO.

@wborn
Copy link
Member

wborn commented Dec 4, 2022

The SCRIPT transformation is using Nashorn, right?
How can I use JS Scripting with it?

If you use Java 17 there will be no Nashorn. 😉

@florian-h05
Copy link
Contributor

florian-h05 commented Dec 4, 2022

Use application/javascript;version=ECMAScript-2021 as script type.

I could have come up with that myself, but the alias for RhinoJS confused me a bit.
You are right that an alias would be nice, but I‘d have to check if introducing one would have any unwanted side effects (such as JS Scripting being listed two times in the script creation wizard in MainUI).

If you use Java 17 there will be no Nashorn. 😉

When we switch to Java 17 and Nashorn leaves us, JS Scripting can take over the js script type.
Given the simplicity of transformation scripts only relying on native JS methods, they should work without any modification required.

@J-N-K
Copy link
Member Author

J-N-K commented Dec 4, 2022

But it works fine with your Nashorn-addon (which should IMO be merged to openhab-addons after we switch to Java 17).

The Groovy add-on also accepts groovy and a MIME-type. The reason is that most engines return a list which is merged from the list of accepted file-extensions and accepted MIME-types. This will not result in double-listing.

@florian-h05
Copy link
Contributor

Thats the question: After switching to Java 17, should we continue to use Nashorn for transformation or install JS Scripting by default and use JS Scripting for the js script type of the SCRIPT transformation.

@J-N-K
Copy link
Member Author

J-N-K commented Dec 4, 2022

IMO the alias will not affect that decision, but would be much easier for all those using 3.4 (which as last version of 3.x will probably be around for quite a while, if we look at the number of questions we still receive for 2.5).

@florian-h05
Copy link
Contributor

IMO the alias will not affect that decision, but would be much easier for all those using 3.4

You are right, I‘ll have a look at introducing an alias, let‘s take your proposal graaljs.

@wborn
Copy link
Member

wborn commented Dec 4, 2022

The extra mime type would certainly be nice for people who transition from Nashorn to GraalJS... or from GraalJS to something else whenever that goes EOL. I'm all for adding JSScripting (Nashorn) to the addons repo so it is easier to optionally install it after an upgrade. 🙂

florian-h05 added a commit to florian-h05/openhab-js that referenced this pull request Dec 5, 2022

Verified

This commit was signed with the committer’s verified signature.
florian-h05 Florian Hotze
Reference openhab/openhab-core#2883.

The "graaljs" MIME type is introduced in openhab/openhab-addons#13851.

Please note that the transformation docs link might not work now, as the doc updates aren't published yet.

Signed-off-by: Florian Hotze <[email protected]>
florian-h05 added a commit to openhab/openhab-js that referenced this pull request Dec 5, 2022

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
* README: Add `SCRIPT` transformation
Reference openhab/openhab-core#2883.
The "graaljs" MIME type is introduced in openhab/openhab-addons#13851.
Please note that the transformation docs link might not work now, as the doc updates aren't published yet.

* README: Use var instead of const and let in examples
Usage of const and let lead to problems in UI scripts and therefore the
examples should not use them.

Signed-off-by: Florian Hotze <[email protected]>
splatch pushed a commit to ConnectorIO/copybara-hab-core that referenced this pull request Jul 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement An enhancement or new feature of the Core
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants