Thing actions with @ActionOutput annotation to be checked and fixed if necessary

[lolodomo]

Here is the list of bindings containing thing actions with @ActionOutput.
They all have to be checked and fixed if necessary as discussed in #17504

• astro => PR [a-d] Fix @ActionOutput annotations #17644
• chromecast => PR [a-d] Fix @ActionOutput annotations #17644
• deconz => PR [a-d] Fix @ActionOutput annotations #17644
• doorbird => PR [a-d] Fix @ActionOutput annotations #17644
• ecobee => PR [ecobee/flume] Fix @ActionOutput annotations #17653
• energidataservice => PR [energidataservice] Fix @ActionOutput annotations #17679
• flume => PR [ecobee/flume] Fix @ActionOutput annotations #17653
• mail => PR [m-o] Fix @ActionOutput annotations #17654
• max => PR [m-o] Fix @ActionOutput annotations #17654
• modbus/helioseasycontrols => PR [m-o] Fix @ActionOutput annotations #17654
• openwebnet => PR [m-o] Fix @ActionOutput annotations #17654
• pushbullet => PR [p-s] Fix @ActionOutput annotations #17664
• pushover => PR [p-s] Fix @ActionOutput annotations #17664
• pushsafer => PR [p-s] Fix @ActionOutput annotations #17664
• satel => PR [p-s] Fix @ActionOutput annotations #17664
• solarforecast => PR [p-s] Fix @ActionOutput annotations #17664
• telegram => PR [t*] Fix @ActionOutput annotations #17666
• tplinksmarthome => PR [t*] Fix @ActionOutput annotations #17666
• tr064 => PR [t*] Fix @ActionOutput annotations #17666
• unifi => PR [u-v] Fix @ActionOutput annotations #17670
• velux => PR [u-v] Fix @ActionOutput annotations #17670
• webexteams [webexteams] Fix @ActionOutput annotations (#17653) #17656
• x [x] Fix @ActionOutput annotations #17673

[lolodomo]

I will have to check the i18n entries for the 6 bindings using @text for the label of output to be sure that the label is "consistent": pushover, pushsafer, satel, tplinksmarthome, tr064 and velux.

And then will remain the only binding that did unusual things with the output actions: energidataservice from @jlaur ;) @florian-h05 your help will be appreciated, I have no clear idea how to annotate all these actions outputs.

[florian-h05]

And then will remain the only binding that did unusual things with the output actions: energidataservice

I will have to think about that, not so easy.

[jlaur]

I am hesitating regarding the change in core.
You could not simply use Double ?

I would prefer to stick with BigDecimal because it contains accurate financial amounts.

Or Map<String, Object>?

That would be a breaking change and make it more cumbersome to be used in rules, which, after all, is the purpose of having these actions.

In general it's nice if the UI can support some of these actions so users can try them out, but they only provide real value in rules.
So please don't put too much effort into "fixing" this - we can always remove annotation (or by some other means hide them in the UI) if it's too difficult.

Also, it's not really user-friendly anyway to have to provide e.g. timestamp as "2024-10-30T13:54:00Z" because the actions take Instant as parameter. If this should really be supported in the UI, it should probably be input as local time and converted to Instant using the configured time-zone, and there should be a datetime picker to assist. But this is of course way out of scope, I assume. 🙂 At least for Energi Data Service, it's not important at all, just nice to have.

[lolodomo]

Also, it's not really user-friendly anyway to have to provide e.g. timestamp as "2024-10-30T13:54:00Z" because the actions take Instant as parameter. If this should really be supported in the UI, it should probably be input as local time and converted to Instant using the configured time-zone, and there should be a datetime picker to assist.

Good idea, I believe we can easily do that @florian-h05 ?
In core, we could provide datetime context to UI and when running the action we could add the zone to the string parameter if not provided? This would be handled like a LocalDateTime on UI side.
@jlaur : is it just adding "Z" at the end of the formatted string? If not, what zone should be considered?

[jlaur]

In core, we could provide datetime context to UI and when running the action we could add the zone to the string parameter if not provided? This would be handled like a LocalDateTime on UI side.
@jlaur : is it just adding "Z" at the end of the formatted string? If not, what zone should be considered?

I'm not sure if zone should be taken from TimeZoneProvider or from browser - that's a UI question, I don't know how time-zone is obtain in other places in the UI. This is also one of the reasons why I haven't made progress with openhab/openhab-core#3583 for quite some time. I haven't given up yet though. 😉 See also openhab/openhab-core#2898 (comment) - "Modern browsers can provide the client's time zone upon request."

[florian-h05]

I will try to answer anything relevant, but due to the high number of messages I maybe miss something. Just ask me again in that case.

Overloaded methods taking different parameters (not just defaulted values for longer signatures).

You need to give them different labels/descriptions to let the user know which takes which params, otherwise there are multiple „same“ entries in the actions list.

How to provide lists as text (e.g. List)?

If the config description parameter has set multiple to true, the UI supports entering a list. Duration however is not supported at the moment.

if you are not showing in UI 5 actions but only 3, maybe that is because some have the same label or function name ?

The UI won‘t like that and will print a warning to the log during rendering, but it will display them anyway. Just see Jacob‘s screenshot in #17636 (comment).

So if we declare all of them as output annotation, UI will show all of them. If they are not present in the output, I assume Main UI will show "null" or something like that. @florian-h05 ?

If they are not present in the output, the UI will show nothing for them.
What is shown depends on the actual action output, the action output description is only used to get labels and descriptions for the outputs in the actual action output.

Very nice to have the actions available in the UI. The developer documentation might be extended with an example and some context as the current one does not mention this feature or actions with output.
Reference:

I am planning to do that once we are done here.

I will list in the first message the bindings with actions having unsupported outputs.
I believe only 3: energidataservice, visualcrossing and dbquery.

Add Astro to the list, as it returns QuantityType.

I am hesitating regarding the change in core.
You could not simply use Double ?
Or Map<String, Object>?

I would convert BigDecimal to a double value inside core, as this only affects action callings from the UI and if the action is used as module in UI-based rules (where the output cannot be used at the moment), so I don‘t see a problem with the decreased precision, as the output won‘t be used for any calculations.
Also add support for Map<String, BigDecimal>.

And having support for QuantityType would be nice as well.

How to provide Instant as text? Maybe just with usual string formatting like 2024-10-30T13:54:00Z?

Couldn‘t we just use the datetime context in the UI and convert the local timezone to UTC and create an Instant from that?

I'm not sure if zone should be taken from TimeZoneProvider or from browser - that's a UI question, I don't know how time-zone is obtain in other places in the UI.

We haven‘t obtained it at all until now I think, if one had to enter a time or date(time) and it was without zone information (e.g. LocalTime), it was always expected to be the server timezone. For DateTime Items, the UI adds zone information I think and then it likely is the zone of the UI. But in general I would always expect the server time zone, to let‘s use TimeZoneProvider.

[lolodomo]

Overloaded methods taking different parameters (not just defaulted values for longer signatures).

You need to give them different labels/descriptions to let the user know which takes which params, otherwise there are multiple „same“ entries in the actions list.

if you are not showing in UI 5 actions but only 3, maybe that is because some have the same label or function name ?

The UI won‘t like that and will print a warning to the log during rendering, but it will display them anyway. Just see Jacob‘s screenshot in #17636 (comment).

I just tried to create two actions with the same label/description like that:

    @RuleAction(label = "test2", description = "Test action 2")
    public @ActionOutputs({ @ActionOutput(name = "result", label = "Test result", type = "java.lang.Integer"),
            @ActionOutput(name = "instant", label = "Instant", type = "java.time.Instant") }) Map<String, Object> testAction2(
                    @ActionInput(name = "instantParam", label = "instantParam parameter", description = "Descr instantParam parameter") Instant instantParam) {
        logger.info("testAction2 {}", instantParam);
        return Map.of("result", "The value returned", "instant", instantParam.plus(5, ChronoUnit.DAYS));
    }

    @RuleAction(label = "test2", description = "Test action 2")
    public void testAction2() {
        logger.info("testAction2 v2 {}");
    }

And UI shows the two actions:

But when I click on each action, I get only the first version of my action.

It is not a Main UI bug but a bug in the REST API:

  {
    "actionUid": "astro.testAction2",
    "label": "test2",
    "description": "Test action 2",
    "inputs": [
      {
        "name": "instantParam",
        "type": "java.time.Instant",
        "label": "instantParam parameter",
        "description": "Descr instantParam parameter",
        "required": false,
        "tags": [],
        "reference": "",
        "defaultValue": ""
      }
    ],
    "inputConfigDescriptions": [
      {
        "description": "Descr instantParam parameter",
        "label": "instantParam parameter",
        "name": "instantParam",
        "required": false,
        "type": "TEXT",
        "readOnly": false,
        "multiple": false,
        "advanced": false,
        "verify": false,
        "limitToOptions": true,
        "options": [],
        "filterCriteria": []
      }
    ],
    "outputs": [
      {
        "name": "result",
        "type": "java.lang.Integer",
        "tags": [],
        "label": "Test result",
        "description": "",
        "reference": "",
        "defaultValue": ""
      },
      {
        "name": "instant",
        "type": "java.time.Instant",
        "tags": [],
        "label": "Instant",
        "description": "",
        "reference": "",
        "defaultValue": ""
      }
    ]
  },
  {
    "actionUid": "astro.testAction2",
    "label": "test2",
    "description": "Test action 2",
    "inputs": [
      {
        "name": "instantParam",
        "type": "java.time.Instant",
        "label": "instantParam parameter",
        "description": "Descr instantParam parameter",
        "required": false,
        "tags": [],
        "reference": "",
        "defaultValue": ""
      }
    ],
    "inputConfigDescriptions": [
      {
        "description": "Descr instantParam parameter",
        "label": "instantParam parameter",
        "name": "instantParam",
        "required": false,
        "type": "TEXT",
        "readOnly": false,
        "multiple": false,
        "advanced": false,
        "verify": false,
        "limitToOptions": true,
        "options": [],
        "filterCriteria": []
      }
    ],
    "outputs": [
      {
        "name": "result",
        "type": "java.lang.Integer",
        "tags": [],
        "label": "Test result",
        "description": "",
        "reference": "",
        "defaultValue": ""
      },
      {
        "name": "instant",
        "type": "java.time.Instant",
        "tags": [],
        "label": "Instant",
        "description": "",
        "reference": "",
        "defaultValue": ""
      }
    ]
  },

Now if I keep same label/description but rename my java method name, then it works well.

    @RuleAction(label = "test2", description = "Test action 2")
    public @ActionOutputs({ @ActionOutput(name = "result", label = "Test result", type = "java.lang.Integer"),
            @ActionOutput(name = "instant", label = "Instant", type = "java.time.Instant") }) Map<String, Object> testAction2(
                    @ActionInput(name = "instantParam", label = "instantParam parameter", description = "Descr instantParam parameter") Instant instantParam) {
        logger.info("testAction2 {}", instantParam);
        return Map.of("result", "The value returned", "instant", instantParam.plus(5, ChronoUnit.DAYS));
    }

    @RuleAction(label = "test2", description = "Test action 2")
    public void testMyAction() {
        logger.info("testMyAction {}");
    }

It looks like the core framework is not able to retrieve the right method in case of same label and same method name.
It would be better to find a fix for that. If not, we introduce a new constraint and we have again to check and fix all our thing actions.

[lolodomo]

How to provide lists as text (e.g. List)?

If the config description parameter has set multiple to true, the UI supports entering a list. Duration however is not supported at the moment.

Duration as input parameter is supported but List of anything is not yet supported.
If I set multiple to true, what kind of things does support Main UI ? Only List of String ? More ?
What will be the type of the input when the action will be called from UI ? A List<String> ?

As few actions have List<String> as input, so we could enhance our support to this new kind of input.

But I doubt UI can support List of anything...

[lolodomo]

In fact, we have only two bindings using List<?> as input parameter: energidataservice and mail.

$ find */src -name "*.java" -exec grep -H "@ActionInput" {} \; | grep "List<"
org.openhab.binding.energidataservice/src/main/java/org/openhab/binding/energidataservice/internal/action/EnergiDataServiceActions.java:            @ActionInput(name = "durationPhases", type = "java.util.List<java.time.Duration>") List<Duration> durationPhases,
org.openhab.binding.energidataservice/src/main/java/org/openhab/binding/energidataservice/internal/action/EnergiDataServiceActions.java:            @ActionInput(name = "durationPhases", type = "java.util.List<java.time.Duration>") List<Duration> durationPhases,
org.openhab.binding.energidataservice/src/main/java/org/openhab/binding/energidataservice/internal/action/EnergiDataServiceActions.java:            @ActionInput(name = "powerPhases", type = "java.util.List<QuantityType<Power>>") List<QuantityType<Power>> powerPhases) {
org.openhab.binding.mail/src/main/java/org/openhab/binding/mail/internal/action/SendMailActions.java:            @ActionInput(name = "urlList", type = "List<String>") @Nullable List<String> urlList) {
org.openhab.binding.mail/src/main/java/org/openhab/binding/mail/internal/action/SendMailActions.java:            @ActionInput(name = "urlList", type = "List<String>") @Nullable List<String> urlList) {

energidataservice uses List<Duration> and List<QuantityType<Power>> while mail uses only List<String>.

[florian-h05]

If I set multiple to true, what kind of things does support Main UI ? Only List of String ? More ?

Currently only raw strings and selection of Items, Things etc.
So List<String> should work, List<Duration> should work as well as long as there is special handling of Duration.

What will be the type of the input when the action will be called from UI ? A List ?

I would think yes.

[lolodomo]

It looks like the core framework is not able to retrieve the right method in case of same label and same method name.
It would be better to find a fix for that. If not, we introduce a new constraint and we have again to check and fix all our thing actions.

It is not possible to have several actions with the same method name and the same scope because they will have same id in the registry.

https://github.com/openhab/openhab-core/blob/728c7376b650c2c1cd271128f39d300cbacbf212/bundles/org.openhab.core.automation.rest/src/main/java/org/openhab/core/automation/rest/internal/ThingActionsResource.java#L175

@florian-h05 : it is a serious problem as several bindings including energidataservice, chromecast, ecobee, ... etc use sometimes the same method name for different actions.
Only one action with a certain name is stored in the module registry. The actions REST API returns several actions but they all points to one unique action.

Renaming methods is certainly a breaking change for use in rules ?

[lolodomo]

@florian-h05 : I know that you plan to update documentation, the constraint on different method names should be clearly documented.

[florian-h05]

The actions REST API returns several actions but they all points to one unique action.

Can you fix the API?

Renaming methods is certainly a breaking change for use in rules ?

Definitely.

[lolodomo]

The actions REST API returns several actions but they all points to one unique action.

Can you fix the API?

I don't know how !
This would probably require to change the way we build ID for each action in the registry.

[lolodomo]

The actions REST API returns several actions but they all points to one unique action.

Can you fix the API?

I don't know how ! This would probably require to change the way we build ID for each action in the registry.

One option could be to introduce an optional "name" to the thing action annotation. If set, it will override the method name.
Like that, developers wanting to have multiple actions with the same method name could do it by just setting a different name in the annotation.
This would also allow us to fix existing actions without any breaking change. WDYT ?

[florian-h05]

This would also allow us to fix existing actions without any breaking change. WDYT ?

Sounds like a good idea 👍

[jlaur]

What remains for Energi Data Service is to fix these warnings, probably by removing annotations?

2024-11-02 16:53:35.075 [DEBUG] [rnal.action.EnergiDataServiceActions] - bundle org.openhab.binding.energidataservice:4.3.0.202411021553 (253)[org.openhab.binding.energidataservice.internal.action.EnergiDataServiceActions(420)] : Changed state from active to active
2024-11-02 16:53:35.076 [WARN ] [der.AnnotationActionModuleTypeHelper] - Method EnergiDataServiceActions::getPrices has a single output but does not use the default output name in the ActionOutput annotation. This should be fixed in the binding.
2024-11-02 16:53:35.077 [WARN ] [der.AnnotationActionModuleTypeHelper] - Method EnergiDataServiceActions::getPrices has a single output but does not use the default output name in the ActionOutput annotation. This should be fixed in the binding.
2024-11-02 16:53:35.078 [WARN ] [der.AnnotationActionModuleTypeHelper] - Method EnergiDataServiceActions::calculateCheapestPeriod returns a Map<String, Object> but is not annotated with ActionOutputs. This should be fixed in the binding.
2024-11-02 16:53:35.078 [WARN ] [der.AnnotationActionModuleTypeHelper] - Method EnergiDataServiceActions::calculateCheapestPeriod returns a Map<String, Object> but is not annotated with ActionOutputs. This should be fixed in the binding.
2024-11-02 16:53:35.079 [WARN ] [der.AnnotationActionModuleTypeHelper] - Method EnergiDataServiceActions::calculateCheapestPeriod returns a Map<String, Object> but is not annotated with ActionOutputs. This should be fixed in the binding.
2024-11-02 16:53:35.080 [WARN ] [der.AnnotationActionModuleTypeHelper] - Method EnergiDataServiceActions::calculateCheapestPeriod returns a Map<String, Object> but is not annotated with ActionOutputs. This should be fixed in the binding.
2024-11-02 16:53:35.080 [WARN ] [der.AnnotationActionModuleTypeHelper] - Method EnergiDataServiceActions::calculatePrice has a single output but does not use the default output name in the ActionOutput annotation. This should be fixed in the binding.

[lolodomo]

There is apparently also a visibility field so we could hide some actions by setting it to HIDDEN. To be investigated.

[lolodomo]

What remains for Energi Data Service is to fix these warnings, probably by removing annotations?

2024-11-02 16:53:35.075 [DEBUG] [rnal.action.EnergiDataServiceActions] - bundle org.openhab.binding.energidataservice:4.3.0.202411021553 (253)[org.openhab.binding.energidataservice.internal.action.EnergiDataServiceActions(420)] : Changed state from active to active
2024-11-02 16:53:35.076 [WARN ] [der.AnnotationActionModuleTypeHelper] - Method EnergiDataServiceActions::getPrices has a single output but does not use the default output name in the ActionOutput annotation. This should be fixed in the binding.
2024-11-02 16:53:35.077 [WARN ] [der.AnnotationActionModuleTypeHelper] - Method EnergiDataServiceActions::getPrices has a single output but does not use the default output name in the ActionOutput annotation. This should be fixed in the binding.
2024-11-02 16:53:35.078 [WARN ] [der.AnnotationActionModuleTypeHelper] - Method EnergiDataServiceActions::calculateCheapestPeriod returns a Map<String, Object> but is not annotated with ActionOutputs. This should be fixed in the binding.
2024-11-02 16:53:35.078 [WARN ] [der.AnnotationActionModuleTypeHelper] - Method EnergiDataServiceActions::calculateCheapestPeriod returns a Map<String, Object> but is not annotated with ActionOutputs. This should be fixed in the binding.
2024-11-02 16:53:35.079 [WARN ] [der.AnnotationActionModuleTypeHelper] - Method EnergiDataServiceActions::calculateCheapestPeriod returns a Map<String, Object> but is not annotated with ActionOutputs. This should be fixed in the binding.
2024-11-02 16:53:35.080 [WARN ] [der.AnnotationActionModuleTypeHelper] - Method EnergiDataServiceActions::calculateCheapestPeriod returns a Map<String, Object> but is not annotated with ActionOutputs. This should be fixed in the binding.
2024-11-02 16:53:35.080 [WARN ] [der.AnnotationActionModuleTypeHelper] - Method EnergiDataServiceActions::calculatePrice has a single output but does not use the default output name in the ActionOutput annotation. This should be fixed in the binding.

We still have "Method EnergiDataServiceActions::getPrices has a single output but does not use the default output name in the ActionOutput annotation. This should be fixed in the binding." with my current PR. Either we avoid this log in core framework when the action is hidden, or I propose to remove action annotations for these 2 methods. WDYT ?

[florian-h05]

I would suggest to fix the annotation, see #17679 (comment).

[lolodomo]

All bindings have been fixed.