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

[energidataservice] Fix @ActionOutput annotations #17679

Merged
merged 4 commits into from
Nov 11, 2024

Conversation

lolodomo
Copy link
Contributor

@lolodomo lolodomo commented Oct 30, 2024

Related to #17636

Depends on openhab/openhab-core/pull/4438

@lolodomo lolodomo requested a review from jlaur as a code owner October 30, 2024 23:45
@lolodomo
Copy link
Contributor Author

lolodomo commented Oct 30, 2024

@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.

@lolodomo lolodomo added rebuild Triggers Jenkins PR build and removed rebuild Triggers Jenkins PR build labels Oct 30, 2024
@jlaur jlaur added the additional testing preferred The change works for the pull request author. A test from someone else is preferred though. label Oct 31, 2024
@jlaur
Copy link
Contributor

jlaur commented Oct 31, 2024

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

Copy link
Contributor

@florian-h05 florian-h05 left a comment

Choose a reason for hiding this comment

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

Annotations look good to me, just one question.

Wrt to actions 1 & 2: If core serialises the return value to a object with Instants as keys and prices as values, the UI will render it. It should be serialisable, but not sure how to do that exactly in core.

@jlaur
Copy link
Contributor

jlaur commented Oct 31, 2024

@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" ] {
}

@jlaur jlaur added the awaiting other PR Depends on another PR label Oct 31, 2024
@lolodomo
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Contributor

jlaur commented Oct 31, 2024

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
Copy link
Contributor Author

We just have to change labels.

I will try to make this work during the weekend.

@florian-h05
Copy link
Contributor

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
Copy link
Contributor Author

lolodomo commented Nov 1, 2024

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
Copy link
Contributor Author

lolodomo commented Nov 1, 2024

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
Copy link
Contributor Author

lolodomo commented Nov 2, 2024

@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
Copy link
Contributor

jlaur commented Nov 2, 2024

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:

  • As you mentioned, it would be a breaking change.
  • The names would become a bit artificial/redundant, since the only difference is the provided input data. Other than that, the actions are doing the same.

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
Copy link
Contributor Author

lolodomo commented Nov 2, 2024

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.

@lolodomo lolodomo closed this Nov 2, 2024
@jlaur
Copy link
Contributor

jlaur commented Nov 2, 2024

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
Copy link
Contributor

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
Copy link
Contributor Author

lolodomo commented Nov 2, 2024

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.

@lolodomo lolodomo reopened this Nov 2, 2024
@jlaur
Copy link
Contributor

jlaur commented Nov 2, 2024

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
Copy link
Contributor

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

@jlaur
Copy link
Contributor

jlaur commented Nov 2, 2024

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
Copy link
Contributor

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

image

@lolodomo
Copy link
Contributor Author

lolodomo commented Nov 2, 2024

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

@lsiepel
Copy link
Contributor

lsiepel commented Nov 6, 2024

Could someone link the PR that this depends on?

@jlaur
Copy link
Contributor

jlaur commented Nov 6, 2024

Could someone link the PR that this depends on?

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

Signed-off-by: Laurent Garnier <[email protected]>
Signed-off-by: Laurent Garnier <[email protected]>
@jlaur
Copy link
Contributor

jlaur commented Nov 10, 2024

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

@jlaur jlaur removed the awaiting other PR Depends on another PR label Nov 11, 2024
@jlaur
Copy link
Contributor

jlaur commented Nov 11, 2024

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
Copy link
Contributor

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
Copy link
Contributor

jlaur commented Nov 11, 2024

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
Copy link
Contributor

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
Copy link
Contributor

jlaur commented Nov 11, 2024

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
Copy link
Contributor

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.

Copy link
Contributor

@jlaur jlaur left a comment

Choose a reason for hiding this comment

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

LGTM

@jlaur jlaur merged commit 972351b into openhab:main Nov 11, 2024
5 checks passed
@jlaur jlaur removed the additional testing preferred The change works for the pull request author. A test from someone else is preferred though. label Nov 11, 2024
@lolodomo lolodomo deleted the fix_actionOutput_8 branch November 11, 2024 16:34
@holgerfriedrich holgerfriedrich added this to the 4.3 milestone Nov 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants