Thing actions with missing @ActionOutput annotation

[lolodomo]

We have 4 bindings that do not declare properly the output of thing actions. The annotation @ActionOutput is missing.

• dbquery => PR [dbquery] Fix Thing Action support for DSL rules #17784
• solarforecast => PR [solarforecast] Add missing @ActionOutput annotation #17506
• telegram => PR [telegram] Add missing @ActionOutput annotation #17505
• visualcrossing => PR [visualcrossing] Add missing @ActionOutput annotation #17665

[lsiepel]

According to the linked fronius PR, it needs to be discussed wether @ActionOutput is mandatory for that PR. Accodding to @florian-h05 there is no benefti at this moment as core code does nothing with it. I'm not that familiair, but if i remember correctly this is (or in the near future) to be used to improve API / UI support for actions?

Main question her is if there is a core PR to be expect that is going to proces this in the near future.

[florian-h05]

With the current core codebase, the @ActionOutput annotation is not processed by core if it is not put inside a @ActionOutputs annotation.
According to the current docs, this annotation is only needed when returning a Map<String, Object> from the action.

Even if @ActionOutput was processed by core, IMO there is no real value in having it for methods returning a single value. If my action is annotated “get azimuth”, what return value do I expect? “azimuth” … I don’t need that annotated extra.

[lolodomo]

When I checked the add-ons codebase, all actions with a return had @ActionOutput annotation except the fourth bindings mentioned in this issue.
You say that it is useless, maybe you're right but we have to be sure before updating all the codebase and providing right guidelines to developers.

[lolodomo]

And @ActionOutputs was never used in the add-ons codebase which is also strange considering what you are saying.
But maybe due to wrong guidelines in documentation.

[florian-h05]

When I checked the add-ons codebase, all actions with a return had @ActionOutput annotation except the fourth bindings mentioned in this issue.

And for none of those bindings the Thing actions API returns anything in the outputs array — when playing around with Astro I noticed that and checked the core code. For actions returning a single value I agree with the code that there is no info needed …

And @ActionOutputs was never used in the add-ons codebase which is also strange considering what you are saying.

I am not sure if there are any actions that return a Map<String, Object> as written down in the docs.
Also see openhab/openhab-docs#2388 (comment).

But maybe due to wrong guidelines in documentation.

I think so.

FYI this is how output annotations are currently processed:

https://github.com/openhab/openhab-core/blob/a22349abf4b3106ec8a3eb9d36799e334cdfbf25/bundles/org.openhab.core.automation/src/main/java/org/openhab/core/automation/module/provider/AnnotationActionModuleTypeHelper.java#L146-L159

Maybe a bit off topic here: Core also limits the allowed return values from actions inside its code, but for scripts it is super useful to have other return values than the allowed ones. However can these mostly not be shown in the UI, so we either need to serialise them properly or hide those actions from the UI.

[florian-h05]

EDIT:
Let’s keep @ActionOutput for actions returning a single value so we optionally provide a label for the output when displaying it in the UI. I would suggest to have the output name default to „result“ because this is the default name used by core to return the return value of a single action, and we need to modify core to parse the annotation in that case - I can do that.

FYI The output class of that action should be determinable by core without having it in the annotation, this is already done for action inputs that don’t specify the type in the annotation. Will check if core does that for the output as well.

And add the @ActionOutout annotation inside the @ActionOutputs annotation for all actions returning multiple outputs through a Map<String, Object>.
These are:

cat **.java | grep @ActionOutput | grep "Map<String, Object>"
    public @ActionOutput(name = NEW_SCENE_ID_OUTPUT, type = "java.lang.Integer") Map<String, Object> createScene(
    public @ActionOutput(name = "result", type = "java.util.Map<String, Object>") Map<String, Object> calculateCheapestPeriod(
    public @ActionOutput(name = "result", type = "java.util.Map<String, Object>") Map<String, Object> calculateCheapestPeriod(
    public @ActionOutput(name = "result", type = "java.util.Map<String, Object>") Map<String, Object> calculateCheapestPeriod(
    public @ActionOutput(name = "result", type = "java.util.Map<String, Object>") Map<String, Object> calculateCheapestPeriod(
    public @ActionOutput(name = "errorMessages", type = "java.util.List<String>") @ActionOutput(name = "warningMessages", type = "java.util.List<String>") @ActionOutput(name = "infoMessages", type = "java.util.List<String>") @ActionOutput(name = "statusMessages", type = "java.util.List<String>") Map<String, Object> getMessages() {
    public @ActionOutput(name = "success", type = "java.lang.Boolean") @ActionOutput(name = "responseMessages", type = "java.util.List<String>") Map<String, Object> sendMessageWithResponse(
    public @ActionOutput(name = "index", type = "java.lang.Integer", label = "@text/actionOutputIndexLabel", description = "@text/actionOutputIndexDesc") @ActionOutput(name = "prev_index", type = "java.lang.Integer", label = "@text/actionOutputPrevIndexLabel", description = "@text/actionOutputPrevIndexDesc") @ActionOutput(name = "timestamp", type = "java.time.ZonedDateTime", label = "@text/actionOutputTimestampLabel", description = "@text/actionOutputTimestampDesc") @ActionOutput(name = "description", type = "java.lang.String", label = "@text/actionOutputDescriptionLabel", description = "@text/actionOutputDescriptionDesc") @ActionOutput(name = "details", type = "java.lang.String", label = "@text/actionOutputDetailsLabel", description = "@text/actionOutputDetailsDesc") Map<String, Object> readEvent(

WDYT?

[florian-h05]

@lolodomo I have now found a good reason to keep the single ActionOutput annotation for actions returning a single value, WDYT?

[maniac103]

add the @ActionOutout annotation inside the @ActionOutputs annotation for all actions returning multiple outputs through a Map<String, Object>.

Looking at the processing code you linked to above I wonder

• why does ActionOutput need to be added inside of ActionOutputs instead of both being present in parallel?
• why is ActionOutputs needed at all? Judging from the code, its presence doesn't change behaviour at all.

[lolodomo]

Let’s keep @ActionOutput for actions returning a single value so we optionally provide a label for the output when displaying it in the UI.

Very good argument.

I would suggest to have the output name default to „result“ because this is the default name used by core to return the return value of a single action, and we need to modify core to parse the annotation in that case - I can do that.

Yes, I agree, "result" should be used as output name in bindings when the action returns only one output.
For example, "success" was largely used in bindings when returning a boolean. We should renamed into "result" I believe.

For actions returning several outputs, they should use a Map<String, Object> and in that case the name of each output should map the entry in the Map.

And add the @ActionOutout annotation inside the @ActionOutputs annotation for all actions returning multiple outputs

I tend to agree with @maniac103 , do we really need this @ActionOutputs annotation ? Maybe we can update the parsing in core framework to consider @ActionOutput even when there is no @ActionOutputs ? This will have the big advantage to simplify the syntax and to avoid fixing all bindings. WDYT ?

[florian-h05]

For example, "success" was largely used in bindings when returning a boolean. We should renamed into "result" I believe.

I agree - Success can still be set as label so the UI displays Success true or Success false.
When modifying core, I will probably add some warning log if an action with single output not named result is detected.

For actions returning several outputs, they should use a Map<String, Object> and in that case the name of each output should map the entry in the Map.

What with actions returning e.g. a Map<Instant, QuantityType>? I see the benefit of allowing those return types for rules, but the UI cannot use them. I think we should hide them from the UI, WDYT?

I tend to agree with @maniac103 , do we really need this @ActionOutputs annotation ?

I don't see a technical reason for this, but prefer the style of having one annotation wrapping multiple annotations instead of having multiple annotations just there.
From the search I shared above, only 8 bindings would need to be modified.

[lolodomo]

I agree - Success can still be set as label so the UI displays Success true or Success false.

Yes. If you deal with core changes, I can prepare a PR for add-ons.
I am counting 108 lines in our codebase having @ActionOutput so 108 actions to check & fix if necessary.

What with actions returning e.g. a Map<Instant, QuantityType>? I see the benefit of allowing those return types for rules, but the UI cannot use them. I think we should hide them from the UI, WDYT?

Yes, I agree.

I don't see a technical reason for this, but prefer the style of having one annotation wrapping multiple annotations instead of having multiple annotations just there.
From the search I shared above, only 8 bindings would need to be modified.

We have 108 +2 actions to fix in that case ! All actions that returns something.
But as we have to check all of them anyway (due to the "result" name), why not.

[florian-h05]

We have 108 +2 actions to fix in that case ! All actions that returns something.

I meant to only wrap @ActionOutput inside @ActionOutputs IF the action is returning a Map<String, Object>. If it returns a single value only, use @ActionOutput only. So @ActionOutputs is ONLY used if there are multiple values returned.

[lolodomo]

I don't know how you count 8 bindings. I am counting 23 bindings.

• astro
• chromecast
• deconz
• doorbird
• ecobee
• energidataservice
• flume
• mail
• max
• modbus/helioseasycontrols
• openwebnet
• pushbullet
• pushover
• pushsafer
• satel
• tplinksmarthome
• tr064
• unifi
• velux
• webexteams
• x
• dbquery
• visualcrossing

[lolodomo]

I meant to only wrap @ActionOutput inside @ActionOutputs IF the action is returning a Map<String, Object>. If it returns a single value only, use @ActionOutput only. So @ActionOutputs is ONLY used if there are multiple values returned.

Ok, in that case it reduces the changes.

[florian-h05]

I don't know how you count 8 bindings. I am counting 23 bindings.

cat **.java | grep @ActionOutput | grep "Map<String, Object>"

This covers bindings that need to wrap multiple @ActionOutput annotations with @ActionOutputs. However might there be bindings completely missing the annotation.

[lolodomo]

However might there be bindings completely missing the annotation.

Yes, this is what I identified in this issue. 4 bindings were concerned, still 2 needs to be fixed.

[maniac103]

I meant to only wrap @ActionOutput inside @ActionOutputs IF the action is returning a Map<String, Object>. If it returns a single value only, use @ActionOutput only. So @ActionOutputs is ONLY used if there are multiple values returned.

That clears up my questions above, thanks 👍

[florian-h05]

@lolodomo See openhab/openhab-core#4430 for the core changes.

[lolodomo]

All completed