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

Bindings with several classes with the same thing action scope #17785

Open
5 tasks
lolodomo opened this issue Nov 22, 2024 · 31 comments
Open
5 tasks

Bindings with several classes with the same thing action scope #17785

lolodomo opened this issue Nov 22, 2024 · 31 comments
Labels
bug An unexpected problem or unintended behavior of an add-on

Comments

@lolodomo
Copy link
Contributor

It is not allowed to have several classes with the same thing action scope.
Here are the bindings not respecting this rule:

  • caddx
  • deconz
  • freeboxos
  • hue
  • openwebnet
@lolodomo lolodomo added the bug An unexpected problem or unintended behavior of an add-on label Nov 22, 2024
@clinique
Copy link
Contributor

I assume the correct way is like it is done in max for example ? "max-cube", "max-device"... ?
Will provide a fix for freeboxos

@clinique
Copy link
Contributor

Well @lolodomo , while I was on it I did it all.

@lolodomo
Copy link
Contributor Author

Note that changing the scope is a breaking change for users.

For hue binding for example, I believe one is for API v1 and the other for API v2. @andrewfg can you please confirm.
We could then decide to not change the scope for the second and only change the scope of the first. Users of API v1 are now probably less than API v2.

@lolodomo
Copy link
Contributor Author

@clinique : bindings README should be updated as the new scope has to be mentioned for each action.

@clinique
Copy link
Contributor

This seems a bit counter-intuitive, I thought getActions was defined:
getActions(String bindingId, String thingUid)

but it's
getActions(String scope, String thingUid)

It's fine when there's only one action class that can define scope = bindingID

then for Caddx it would give:
val actions = getActions("caddx-zone","caddx:zone:thebridge:zone4")

correct ?

@lolodomo
Copy link
Contributor Author

Yes, that's my understanding.

@lolodomo
Copy link
Contributor Author

Please make a test with freeboxos binding for example to confirm.

@clinique
Copy link
Contributor

I use rebootPlayer and rebootServer in Freeboxos, thus they had the same some and it used to work fine.

@lolodomo
Copy link
Contributor Author

lolodomo commented Nov 22, 2024

It is not allowed to have several classes with the same thing action scope.

This information is coming from @J-N-K . Please comment and confirm to avoid making useless changes.

@andrewfg
Copy link
Contributor

andrewfg commented Nov 22, 2024

I believe one is for API v1 and the other for API v2. @andrewfg can you please confirm

Yes. I think that is the case. I agree that if we should 'break' something, it should be rather in v1 than v2. But let me check in detail tomorrow..


EDIT: I confirm the above.

@lolodomo
Copy link
Contributor Author

lolodomo commented Nov 23, 2024

I use rebootPlayer and rebootServer in Freeboxos, thus they had the same some and it used to work fine.

That's a good reason I would prefer to have first a GO from @J-N-K before applying these breaking changes.

@lolodomo
Copy link
Contributor Author

For information, the only binding currently using different scopes is the max binding.

@lsiepel
Copy link
Contributor

lsiepel commented Nov 23, 2024

Slighlty offtopic; i'm implementing some actions for the telegram binding and want to use an array as parameter. It's not working, might be something different. But the question is if i can find some documentation on allowed/supported parameters.

The method with this signature does not show up in main UI (4.3.0M4) as other methods do. I don;t see a problem with the scope, the method name is unique

    @RuleAction(label = "send a test message", description = "Send a Telegram using the Telegram API.")
    public @ActionOutput(label = "Success", type = "java.lang.Boolean") boolean sendTelegramTo(
            @ActionInput(name = "chatIds") @Nullable Long @Nullable [] chatIds,
            @ActionInput(name = "message") @Nullable String message,
            @ActionInput(name = "replyMessageId") @Nullable Integer replyMessageId,
            @ActionInput(name = "silent") @Nullable Boolean silent,
            @ActionInput(name = "messageThreadId") @Nullable Integer messageThreadId,
            @ActionInput(name = "args") @Nullable Object... args) {

@lolodomo
Copy link
Contributor Author

The type of your parameters chatIds and args are not supported by Main UI.

The supported types are listed in this PR:
openhab/openhab-core#4392

String, boolean, Boolean, byte, Byte, short, Short, int, Integer, long, Long, float, Float, double, Double, Number, DecimalType, QuantityType<?>, LocalDateTime, LocalDate, LocalTime, ZonedDateTime, Date, Instant and Duration

Ps how a variable list of java Object could be...

@Nadahar
Copy link
Contributor

Nadahar commented Nov 30, 2024

I'm confused about the @ThingActionsScope. I need different Actions for different Thing-Types, so as far as I can understand, that means using different ThingActions implementations and "bind" them to the different thing-type handlers. This requires different scopes, but as soon as I change the scope from being just "bindingid" to "bindingid-xxx", the Actions stop working from MainUI. They show up, but when you try to execute them, the UI just "hangs" with "Executing..." and the actual Action method is never called.

The documentation merely states

@ThingActionsScope(name = "mqtt") // Your bindings id is usually the scope

I'm lost - I can't get it to work.

@lolodomo
Copy link
Contributor Author

In fact we are all a little confused, it is still not so clear. I hoped we could discuss with @J-N-K but he does not answer.

Regarding run of action from Main UI, it might be that you need to reload Main UI or maybe a bug in Main UI ?
Use API Explorer from Main UI and request all thing actions from your thing and see what scope is returned for each action.

image

@Nadahar
Copy link
Contributor

Nadahar commented Nov 30, 2024

I've tried reloading many times, I've restarted, "cleaned" the project and everything I can think of to "clear caches", but the issue persists. MainUI does list the Actions though, so it might be a bug in MainUI.

When running one of the actions from Rules DSL using `getActions(, ) it does work, which probably further hints that the problem is with MainUI.

@lolodomo
Copy link
Contributor Author

Pinging @florian-h05 as there is a potential issue in Main UI.
I am asking myself if the problem is not in the REST API. Do we allow the "-" ?

@Nadahar
Copy link
Contributor

Nadahar commented Nov 30, 2024

Do we allow the "-"

I've tried with "." and ":" too - same result.

@lolodomo
Copy link
Contributor Author

lolodomo commented Nov 30, 2024

I believe the problem is here in the REST API, we do not accept a "-" in the first part of the action id:

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

@Nadahar
Copy link
Contributor

Nadahar commented Nov 30, 2024

I can't see any information about scope at all when running /actions/{thingUID} from the API Explorer.

@lolodomo
Copy link
Contributor Author

Try using bindingidxxx instead of bindingid-xxx just to confirm my hypothesis.

@lolodomo
Copy link
Contributor Author

lolodomo commented Nov 30, 2024

actionid in the REST API is scope followed by a "." and followed by the method name like for example astro.getAzimut

@Nadahar
Copy link
Contributor

Nadahar commented Nov 30, 2024

Try using bindingidxxx instead of bindingid-xxx just to confirm my hypothesis.

That works 👍

It's kind of "messy" to not have any separator at all though, so something should probably be changed. And the documentation must definitely explain how to use the scope much better.

@lolodomo
Copy link
Contributor Author

As you can see with this issue, this is something new and we have to fix it.
We have only one binding in the official distribution using something else than the binding id as scope, the max binding.
And calling actions from REST API and Main UI is something rrecent, to not say very recent.

So as soon as we are all sure that bindingid cannot be used for different action classes, we will have to fix the REST API also.

@florian-h05
Copy link
Contributor

It is not allowed to have several classes with the same thing action scope.

I rather think that it is not allowed for a single ThingType to have multiple action classes in the same scope. A binding may habe multiple action class for different ThingTypes.

@J-N-K
Copy link
Member

J-N-K commented Dec 1, 2024

@florian-h05 Correct. In fact the restriction is "one action class with the same scope per THING", not per ThingType. The key is build from the thing UID and the scope. In principle it could be that the ThingHandler selects from several action classes with the same scope and return only one in getServices but I believe there are not many use-cases for that and the risk of failure is high. So "one action class with the same scope per thing-type" seems like a good policy.

@florian-h05
Copy link
Contributor

Thanks for the feedback.
I am already working on improving the developer docs for Thing actions, I will make sure to put that in there.

@florian-h05
Copy link
Contributor

In principle it could be that the ThingHandler selects from several action classes with the same scope and return only one in getServices but I believe there are not many use-cases for that and the risk of failure is high.

That would also make using Thing actions from scripts very difficult, as the user would have to check which ThingActions implementation is returned.

@J-N-K
Copy link
Member

J-N-K commented Dec 1, 2024

The use-case I can imagine is that a ThingHandler is implemented for a generic thing type (e.g. binding:actuator) and the available channels for the specific device are created dynamically at runtime. The available actions could also depend on the specific device, the developer could create separate ThingActions implementations for each device and the ThingHandler selects and returns the only correct one. The user knows what device he has, so the available actions could be retrieved from the documentation, but he could always use the same scope (e.g. the binding id).

@lolodomo
Copy link
Contributor Author

lolodomo commented Dec 1, 2024

Thank you @J-N-K for clarification.

If we want to allow bindingname-xxx as scope, we should update the REST API to allow the "-" character. Until that, actions from the max binding are certainly not runnable from REST API.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug An unexpected problem or unintended behavior of an add-on
Projects
None yet
Development

No branches or pull requests

7 participants