-
-
Notifications
You must be signed in to change notification settings - Fork 443
[rest] Support sending Item command/state as JSON #4760
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
Conversation
This pull request has been mentioned on openHAB Community. There might be relevant details there: https://community.openhab.org/t/integrating-openhab-and-garmin/155748/22 |
efbb7c4
to
c913990
Compare
What about state updates? If commands are available these should work, too. Are you aware of the webhook binding? |
I’m the original requester for this change. The Webhook binding also doesn’t natively support sending commands to items. You need to create a custom Webhook setup, which involves a relatively complex configuration using a JEXL script in the Thing and additional JavaScript processing. To make the Garmin app accessible to a wider audience, it would be much better if the needed API support were provided out of the box. |
@TheNinth7 All I'm saying is that
|
I didn’t notice the method for this, I will add that.
I think there is some architectural difference here. The webhook binding won’t be allowed to directly control items as it is a binding, it provides a way to specify custom webhooks. I think its functionality being a binding only is fine, though it would be nice to be part of the distribution.
That’s a good point, some clients only support posting JSON. It shouldn’t be a problem supporting multiple media types for the POST/PUT endpoints. |
- Adds support for sending command/state as JSON to the existing plain text endpoints. - Adds new or extends existing endpoints where command/state can be passed through query params, allowing to send them from webhooks. Signed-off-by: Florian Hotze <[email protected]>
c913990
to
a6c1f6b
Compare
So i am finished with adding the same functionality for states and JSON support for the existing send command/state endpoints. |
This pull request has been mentioned on openHAB Community. There might be relevant details there: |
Just a generic comment around the merit - for rest interfaces GET requests should issue read requests (as verb suggests), rather than write. |
This is the reason why I always argued against such a "feature" - it completely goes against the core principles of a REST API and GET requests being safe methods. I'm therefore still not a fan of adding that to our REST API. As @spacemanspiff2007 there was a "hidden" workaround in the past by having the Classic UI offering such an endpoint, which was not part of the REST API. If the main use case is the support of webhooks, the approach with the webhook bindings looks much cleaner to me. Just my two cents on the topic. |
What about splitting out the GET webhook functionality into a new core webhook API? |
From the perspective of the openHAB app I’m developing for Garmin, I can support both |
@kaikreuzer Back in the days I had an old webcam on the only way to get notifications from it into openHAB was through a GET request because that was all the webcam offered. So it was either making that GET request work or nothing. So I suggest that there is at least something or some way to provide more flexibility to ingest data, even though it's not the "proper" API. Accepting json for commands / updates is imho a good idea in any case. |
Yes, I see the recurring request and see that there are many external services that don't follow REST principles, so I won't block any solution for this in core. But I wouldn't advice to dilute the existing REST API and its endpoints by simply adding non-REST behavior to it like here. |
Signed-off-by: Florian Hotze <[email protected]>
06cfbb4
to
b4630ce
Compare
@kaikreuzer I have removed the new GET endpoints from this PR, it now only contains JSON support for the existing plain text command/state endpoints. Can you please review it? |
I also like the idea of a JSON-RPC endpoint for this, its simple, follows a specification, and keeps the REST api clean. I also would definitely use this, i use Caddy to proxy and rewrite requests from a few devices who only support get requests for events. Would be nice to remove those rules. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
@Consumes(MediaType.APPLICATION_JSON) | ||
public Response postItemCommandJson(@PathParam("itemname") String itemname, ValueContainer valueContainer) { | ||
return sendItemCommandInternal(itemname, valueContainer.value()); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lolodomo this made me think of you. I know we decided not to do it, but isn't this what you were trying to do but having trouble to do it? i.e. trying to accept either plain text and json.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Professing both MIME types is relatively simple, the most difficult part is getting Swagger work.
Signed-off-by: Florian Hotze <[email protected]> (cherry picked from commit 8ff69a3)
Adds support for sending command/state through media type
application/json
to the existing plain text endpoints.