search

openhab / openhab-addons

Add-ons for openHAB
https://www.openhab.org/
Eclipse Public License 2.0
1.9k stars 3.59k forks source link

[energidataservice] Fix @ActionOutput annotations #17679

Closed lolodomo closed 2 weeks ago

lolodomo commented 1 month ago

Related to #17636

Depends on openhab/openhab-core/pull/4438

lolodomo commented 1 month ago

@jlaur @florian-h05

Actions 1 & 2: output annotation unchanged, I don't know what to do.

Action 3: proper output annotation. Should be ok once the new core PR is merged.

Actions 4 & 5: proper outputs annotation. Should work.

Actions 6 & 7: proper outputs annotation. Should not be visible in Main UI due to incompatible type in input parameters.

jlaur commented 4 weeks ago

@lolodomo - thanks! I'll give this a test later, hopefully in the evening.

jlaur commented 4 weeks ago

@lolodomo - just a note: You can easily test the binding by creating a Thing and selecting one of the price areas, and optionally a grid company. Example configuration:

UID: energidataservice:service:energidataservice
label: Energi Data Service
thingTypeUID: energidataservice:service
configuration:
  gridCompanyGLN: "5790001089030"
  energinetGLN: "5790000432752"
  priceArea: DK1
  currencyCode: DKK
  reducedElectricityTax: false

As file:

Thing energidataservice:service:energidataservice "Energi Data Service" [ priceArea="DK1", currencyCode="DKK", gridCompanyGLN="5790001089030" ] {
}
lolodomo commented 4 weeks ago

First, use the API explorer and the GET endpoint to know what is returned to Main UI. This will help to know if not visible actions are due to core framework or Main UI.

lolodomo commented 4 weeks ago

But I believe @florian-h05 already told us that different actions cannot have the same label. That is very probably why some are not visible in UI.

jlaur commented 4 weeks ago

But I believe @florian-h05 already told us that different actions cannot have the same label. That is very probably why some are not visible in UI.

Probably, yes. So for now there is no reason to change/annotate any of those.

lolodomo commented 4 weeks ago

We just have to change labels.

I will try to make this work during the weekend.

florian-h05 commented 4 weeks ago

But I believe @florian-h05 already told us that different actions cannot have the same label. That is very probably why some are not visible in UI.

Technically I think this should work fine, but it is quite confusing for the user.

lolodomo commented 4 weeks ago

After renaming methods to avoid same names.

image

image

image

image

With my fix in core framework and with very last version of Main UI.

lolodomo commented 4 weeks ago

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.

lolodomo commented 3 weeks ago

@jlaur : do you prefer renaming the methods (breaking change) or keeping the current method names but without the ability to run them from Main UI ?

For the two first actions (get prices) with their atypical return type, maybe the best to simply to fully remove annotations so that they are simply not shown in Main UI ?

jlaur commented 3 weeks ago

do you prefer renaming the methods (breaking change) or keeping the current method names but without the ability to run them from Main UI ?

As mentioned here, the only real use-case of these actions is to be used in rules. When I created them, I tested in both Rules DSL and JavaScript, and all actions were working. Unless support for method overloads will be removed from core, I would prefer to not change the names:

For the two first actions (get prices) with their atypical return type, maybe the best to simply to fully remove annotations so that they are simply not shown in Main UI ?

Agreed. GetPrices is useless from Main UI. It was created before time series were introduced to make prices directly available for rules. It can still be slightly useful if for some reason a user doesn't want to configure persistence for an item, although it's debable.

Price calculations could be nice as a kind of demo of the power of the actions, but besides letting users trying the actions before implementing them in rules, it's not important.

lolodomo commented 3 weeks ago

Ok so I close this PR. But I am not totally forgiving, I have an idea how we could support several actions with the same method name.

jlaur commented 3 weeks ago

Ok so I close this PR.

So what remains now is to simply remove annotations - and they are only used by the REST API?

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.
florian-h05 commented 3 weeks ago

they are only used by the REST API?

They are only used for invoking Thing actions through the UI or adding them to UI-based rules, where you can select them from the all actions list. Those annotations are not used when calling Thing actions from rules through real code.

lolodomo commented 3 weeks ago

I reopen the PR as I implemented the solution in core framework to distinguish actions with the same method name. I am going to update this PR.

jlaur commented 3 weeks ago

They are only used for invoking Thing actions through the UI or adding them to UI-based rules, where you can select them from the all actions list.

It sounds like the actions provided by this binding have been broken right from the start, since I never tested UI-based rules. 😢

florian-h05 commented 3 weeks ago

To be fair, UI support for them was added just a few weeks ago so you could not test that before.

jlaur commented 3 weeks ago

To be fair, UI support for them was added just a few weeks ago so you could not test that before.

Phew, thanks. 😀 I thought you could do the same in UI rules as in file-based, but it never crossed my mind to test rules and actions from the UI.

florian-h05 commented 3 weeks ago

When writing code in a rule, the same was possible, but this is new:

image

lolodomo commented 3 weeks ago

I pushed new code but we need the merge of core PRs first.

lsiepel commented 3 weeks ago

Could someone link the PR that this depends on?

jlaur commented 3 weeks ago

Could someone link the PR that this depends on?

For sure openhab/openhab-core#4438. I think the others are now merged.

jlaur commented 2 weeks ago

@lolodomo - I will test this again when the new milestone is available.

jlaur commented 2 weeks ago

Tests with snapshot build 4374 and binding built from this PR:

image

I guess the "get prices" method should not have been shown since they are both marked as hidden?

First "calculate cheapest period":

image

2024-11-11 12:23:09.429 [WARN ] [e.automation.util.ActionInputsHelper] - Input parameter 'duration': converting value '60' into type java.time.Duration failed! Input parameter is ignored.
2024-11-11 12:23:09.434 [ERROR] [dule.handler.AnnotationActionHandler] - Could not call method 'public java.util.Map org.openhab.binding.energidataservice.internal.action.EnergiDataServiceActions.calculateCheapestPeriod(java.time.Instant,java.time.Instant,java.time.Duration)' from module type 'energidataservice.calculateCheapestPeriod#19c6be7b3428410229b15c6588521bf0'.
java.lang.reflect.InvocationTargetException: null
    at jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method) ~[?:?]
    at jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:77) ~[?:?]
    at jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43) ~[?:?]
    at java.lang.reflect.Method.invoke(Method.java:568) ~[?:?]
    at org.openhab.core.automation.internal.module.handler.AnnotationActionHandler.execute(AnnotationActionHandler.java:127) ~[?:?]
    at org.openhab.core.automation.rest.internal.ThingActionsResource.executeThingAction(ThingActionsResource.java:258) ~[?:?]
    at jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method) ~[?:?]
    at jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:77) ~[?:?]
    at jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43) ~[?:?]
    at java.lang.reflect.Method.invoke(Method.java:568) ~[?:?]
    at org.apache.cxf.service.invoker.AbstractInvoker.performInvocation(AbstractInvoker.java:179) ~[bundleFile:3.6.2]
    at org.apache.cxf.service.invoker.AbstractInvoker.invoke(AbstractInvoker.java:96) ~[bundleFile:3.6.2]
    at org.apache.cxf.jaxrs.JAXRSInvoker.invoke(JAXRSInvoker.java:201) ~[bundleFile:3.6.2]
    at org.apache.cxf.jaxrs.JAXRSInvoker.invoke(JAXRSInvoker.java:104) ~[bundleFile:3.6.2]
    at org.apache.cxf.interceptor.ServiceInvokerInterceptor$1.run(ServiceInvokerInterceptor.java:59) ~[bundleFile:3.6.2]
    at org.apache.cxf.interceptor.ServiceInvokerInterceptor.handleMessage(ServiceInvokerInterceptor.java:96) ~[bundleFile:3.6.2]
    at org.apache.cxf.phase.PhaseInterceptorChain.doIntercept(PhaseInterceptorChain.java:307) ~[bundleFile:3.6.2]
    at org.apache.cxf.transport.ChainInitiationObserver.onMessage(ChainInitiationObserver.java:121) ~[bundleFile:3.6.2]
    at org.apache.cxf.transport.http.AbstractHTTPDestination.invoke(AbstractHTTPDestination.java:265) ~[bundleFile:3.6.2]
    at org.apache.cxf.transport.servlet.ServletController.invokeDestination(ServletController.java:234) ~[bundleFile:3.6.2]
    at org.apache.cxf.transport.servlet.ServletController.invoke(ServletController.java:208) ~[bundleFile:3.6.2]
    at org.apache.cxf.transport.servlet.ServletController.invoke(ServletController.java:160) ~[bundleFile:3.6.2]
    at org.apache.cxf.transport.servlet.CXFNonSpringServlet.invoke(CXFNonSpringServlet.java:225) ~[bundleFile:3.6.2]
    at org.apache.cxf.transport.servlet.AbstractHTTPServlet.handleRequest(AbstractHTTPServlet.java:304) ~[bundleFile:3.6.2]
    at org.apache.cxf.transport.servlet.AbstractHTTPServlet.doPost(AbstractHTTPServlet.java:217) ~[bundleFile:3.6.2]
    at javax.servlet.http.HttpServlet.service(HttpServlet.java:517) ~[bundleFile:4.0.4]
    at org.apache.cxf.transport.servlet.AbstractHTTPServlet.service(AbstractHTTPServlet.java:279) ~[bundleFile:3.6.2]
    at org.ops4j.pax.web.service.spi.servlet.OsgiInitializedServlet.service(OsgiInitializedServlet.java:102) ~[bundleFile:?]
    at org.eclipse.jetty.servlet.ServletHolder.handle(ServletHolder.java:799) ~[bundleFile:9.4.54.v20240208]
    at org.eclipse.jetty.servlet.ServletHandler$ChainEnd.doFilter(ServletHandler.java:1656) ~[bundleFile:9.4.54.v20240208]
    at org.ops4j.pax.web.service.spi.servlet.OsgiFilterChain.doFilter(OsgiFilterChain.java:113) ~[bundleFile:?]
    at org.ops4j.pax.web.service.jetty.internal.PaxWebServletHandler.doHandle(PaxWebServletHandler.java:334) ~[bundleFile:?]
    at org.eclipse.jetty.server.handler.ScopedHandler.handle(ScopedHandler.java:143) ~[bundleFile:9.4.54.v20240208]
    at org.eclipse.jetty.security.SecurityHandler.handle(SecurityHandler.java:600) ~[bundleFile:9.4.54.v20240208]
    at org.eclipse.jetty.server.handler.HandlerWrapper.handle(HandlerWrapper.java:127) ~[bundleFile:9.4.54.v20240208]
    at org.eclipse.jetty.server.handler.ScopedHandler.nextHandle(ScopedHandler.java:235) ~[bundleFile:9.4.54.v20240208]
    at org.eclipse.jetty.server.session.SessionHandler.doHandle(SessionHandler.java:1624) ~[bundleFile:9.4.54.v20240208]
    at org.eclipse.jetty.server.handler.ScopedHandler.nextHandle(ScopedHandler.java:233) ~[bundleFile:9.4.54.v20240208]
    at org.eclipse.jetty.server.handler.ContextHandler.doHandle(ContextHandler.java:1440) ~[bundleFile:9.4.54.v20240208]
    at org.eclipse.jetty.server.handler.ScopedHandler.nextScope(ScopedHandler.java:188) ~[bundleFile:9.4.54.v20240208]
    at org.eclipse.jetty.servlet.ServletHandler.doScope(ServletHandler.java:505) ~[bundleFile:9.4.54.v20240208]
    at org.eclipse.jetty.server.session.SessionHandler.doScope(SessionHandler.java:1594) ~[bundleFile:9.4.54.v20240208]
    at org.eclipse.jetty.server.handler.ScopedHandler.nextScope(ScopedHandler.java:186) ~[bundleFile:9.4.54.v20240208]
    at org.eclipse.jetty.server.handler.ContextHandler.doScope(ContextHandler.java:1355) ~[bundleFile:9.4.54.v20240208]
    at org.eclipse.jetty.server.handler.ScopedHandler.handle(ScopedHandler.java:141) ~[bundleFile:9.4.54.v20240208]
    at org.eclipse.jetty.server.handler.ContextHandlerCollection.handle(ContextHandlerCollection.java:234) ~[bundleFile:9.4.54.v20240208]
    at org.ops4j.pax.web.service.jetty.internal.PrioritizedHandlerCollection.handle(PrioritizedHandlerCollection.java:96) ~[bundleFile:?]
    at org.eclipse.jetty.server.handler.gzip.GzipHandler.handle(GzipHandler.java:722) ~[bundleFile:9.4.54.v20240208]
    at org.eclipse.jetty.server.handler.HandlerWrapper.handle(HandlerWrapper.java:127) ~[bundleFile:9.4.54.v20240208]
    at org.eclipse.jetty.server.Server.handle(Server.java:516) ~[bundleFile:9.4.54.v20240208]
    at org.eclipse.jetty.server.HttpChannel.lambda$handle$1(HttpChannel.java:487) ~[bundleFile:9.4.54.v20240208]
    at org.eclipse.jetty.server.HttpChannel.dispatch(HttpChannel.java:732) [bundleFile:9.4.54.v20240208]
    at org.eclipse.jetty.server.HttpChannel.handle(HttpChannel.java:479) [bundleFile:9.4.54.v20240208]
    at org.eclipse.jetty.server.HttpConnection.onFillable(HttpConnection.java:277) [bundleFile:9.4.54.v20240208]
    at org.eclipse.jetty.io.AbstractConnection$ReadCallback.succeeded(AbstractConnection.java:311) [bundleFile:9.4.54.v20240208]
    at org.eclipse.jetty.io.FillInterest.fillable(FillInterest.java:105) [bundleFile:9.4.54.v20240208]
    at org.eclipse.jetty.io.ChannelEndPoint$1.run(ChannelEndPoint.java:104) [bundleFile:9.4.54.v20240208]
    at org.eclipse.jetty.util.thread.strategy.EatWhatYouKill.runTask(EatWhatYouKill.java:338) [bundleFile:9.4.54.v20240208]
    at org.eclipse.jetty.util.thread.strategy.EatWhatYouKill.doProduce(EatWhatYouKill.java:315) [bundleFile:9.4.54.v20240208]
    at org.eclipse.jetty.util.thread.strategy.EatWhatYouKill.tryProduce(EatWhatYouKill.java:173) [bundleFile:9.4.54.v20240208]
    at org.eclipse.jetty.util.thread.strategy.EatWhatYouKill.run(EatWhatYouKill.java:131) [bundleFile:9.4.54.v20240208]
    at org.eclipse.jetty.util.thread.ReservedThreadExecutor$ReservedThread.run(ReservedThreadExecutor.java:409) [bundleFile:9.4.54.v20240208]
    at org.eclipse.jetty.util.thread.QueuedThreadPool.runJob(QueuedThreadPool.java:883) [bundleFile:9.4.54.v20240208]
    at org.eclipse.jetty.util.thread.QueuedThreadPool$Runner.run(QueuedThreadPool.java:1034) [bundleFile:9.4.54.v20240208]
    at java.lang.Thread.run(Thread.java:833) [?:?]
Caused by: java.lang.NullPointerException
    at java.util.Objects.requireNonNull(Objects.java:208) ~[?:?]
    at java.util.ImmutableCollections$List12.<init>(ImmutableCollections.java:556) ~[?:?]
    at java.util.List.of(List.java:812) ~[?:?]
    at org.openhab.binding.energidataservice.internal.PriceCalculator.calculateCheapestPeriod(PriceCalculator.java:109) ~[?:?]
    at org.openhab.binding.energidataservice.internal.action.EnergiDataServiceActions.calculateCheapestPeriod(EnergiDataServiceActions.java:129) ~[?:?]
    ... 65 more

Now corrected to "PT60M":

2024-11-11 12:24:46.778 [WARN ] [rnal.action.EnergiDataServiceActions] - Price missing at 2024-11-11T23:00:00Z

I then realized this is because my development time-zone is currently set to US/Alaska, so it seems these times are converted (correctly) from local datetime to UTC (Instant):

image

When having no deserialization of Instant in place, this output cannot be read by a user, so I think the method should be hidden.

The next "calculate cheapest period" method:

image

Here the prices are available, so this method makes sense to expose.

Method "calculate price" is perfect:

image

Btw, in Firefox (no time selection):

image

in Chrome:

image

FYI @florian-h05

I see a lot of good progress - cool!

florian-h05 commented 2 weeks ago

The visibility field is not used by the UI yet, thanks for reminding me to implement that.

Wrt to proper Instant serialisation: This should be added IMO.

Wrt to Firefox not displaying a time picker: This is a Firefox bug, see https://bugzilla.mozilla.org/show_bug.cgi?id=1726108.

jlaur commented 2 weeks ago

Wrt to proper Instant serialisation: This should be added IMO.

Do you think anything should be added for providing/selecting and/or validating Duration? I guess it depends if there is any UI component that could be used for this.

florian-h05 commented 2 weeks ago

Hmm AFAIK nothing is provided by Framework7 or the browser, so we would have to build this ourselves. This should be possible (the cron picker is built by Yannick), but I am not sure if there is enough use to justify the amount of work to needed to implement this with proper validation etc.

jlaur commented 2 weeks ago

Hmm AFAIK nothing is provided by Framework7 or the browser, so we would have to build this ourselves. This should be possible (the cron picker is built by Yannick), but I am not sure if there is enough use to justify the amount of work to needed to implement this with proper validation etc.

I agree, but had to ask. 😄 From a rule context, e.g. from JavaScript, it's straight-forward and logical to provide a time.Duration of something, e.g. time.Duration.ofHours(1). But from an input field with the label "Duration" it's not that obvious for a user that you have to enter something like "PT60M". Anyway, since the action works when providing a correct ISO-8601 duration, I don't think we should hide it in this case as it can be meaningful and convenient. I can add something to the documentation.

florian-h05 commented 2 weeks ago

Hmm, our date time library in the UI, dayjs(), supports Duration construction and parsing. We could provide a picker where one could select seconds, minutes, hours, days, weeks, months (whatever) and build a duration string from it or parse a duration string to validate it, see https://day.js.org/docs/en/durations/creating.