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

Review of HTTP Webhook Profile #375

Open
26 of 30 tasks
benfrancis opened this issue Mar 22, 2023 · 1 comment
Open
26 of 30 tasks

Review of HTTP Webhook Profile #375

benfrancis opened this issue Mar 22, 2023 · 1 comment

Comments

@benfrancis
Copy link
Member

benfrancis commented Mar 22, 2023

I have been evaluating the latest draft of the Webhook Profile with a view to potential implementation in WebThings Gateway (Producer) and WebThings Cloud (Consumer) and I think it needs some more work before it's in an implementable state. I've provided my feedback section by section below, with action items as task list items which can be turned into separate issues if necessary.

10 HTTP Webhook Profile

This section defines the HTTP Webhook Profile, including a Protocol Binding for observing properties and listening for events using WebHooks.

So far there's only a protocol binding for events.

10.1 Introduction

Suggested refinements:

  • "Webhooks are a mechanism for Consumers to subscribe to events emitted by a Web Thing." -> "Webhooks can be used as a mechanism for Consumers to subscribe to events and observe properties of a Web Thing."
  • "Consumers implement a Webhook listener that is handling the event streams generated by Things." -> "Consumers implement a Webhook listener which handles events emitted by Things." (Webhooks do not use streams, they emit each event in a separate HTTP request which requires its own TCP connection. Some payload formats allow for multiple events to be batched into a one payload, but that is not "streaming".)
  • "The client behavior is used for the initial subscription operation, whereas the server behavior is used to accept the event streams sent by the Thing." -> "The client behavior is used for the initial subscription operation, whereas the server behavior is used to accept the events sent by the Thing."
  • Integration with CloudEvents #387
  • Multiple subscriptions from the same consumer to the same endpoint? #388

10.1.1 Webhook Example

I think this example could use work. I'm not sure why the fireAlarm event would be fired when the button has been re-armed, and I'm not sure how a Consumer would know anything about sprinkler status. The cancellation payload format and unsubscribe protocol binding need updating to match the protocol binding in the specification text, i.e. DELETE instead of POST and provide the subscription ID in the URL rather than the body (a DELETE request does not usually contain a body).

10.4.1.1 subscribeevent

  • Add Content-Type header to list of features of the subscription request, as in the example

This involves the Web Thing initially sending an HTTP response to the Consumer with:

  • Status code set to 200
  • Content-Type header set to application/json
  • Accept header set to application/json

EXAMPLE 46

HTTP/1.1 200 OK
Content-Type: application/json
{
    subscriptionId: 1234-4544-1211
}

Although it's not mentioned in the normative text, the example shows the subscription ID being provided in the body of the HTTP response to the subscribeevent request. The problem with only including the ID in the body (as opposed to a URL) is that it requires a fixed URL design in order to know the subscription URL to which a Consumer should send an unsubscribe DELETE request.

I suggest that the response to the subscribe POST request should instead be 201 Created (since it creates a new subscription resource), and the URL of the new subscription resource should be included in a Location header. The unsubscribeevent operation can then send a DELETE request to that URL (see below). This would be consistent with what we do for asynchronous actions in the HTTP Basic Profile.

  • Remove Accept header from subscribeevent operation request since it need not respond with a body
  • Require a Content-Type header with value application/json in the POST request since it contains JSON
  • Change response to subscribeevent operation to a 201 Created with the subscription URL in a Location header
  • Remove the body from the response to the subscribeevent operation

E.g.

HTTP/1.1 201 Created
Content-Type: application/json
Location: /things/lamp/events/overheated/1234-4544-1211

10.4.1.2 unsubscribeevent

  • Change the unsubscribeevent operation to a DELETE request on the subscription URL provided in the Location header of the response to the subscribeevent request, instead of prescribing a fixed URL format
  • Remove the Accept header from the DELETE request
  • Specify a 204 No Content response to the DELETE request
  • Provide an example response to the DELETE request

10.4.1.3 subscribeallevents

  • Remove the Accept header from the subscription POST request, as with subscribeevent
  • Add a Content-Type header with value application/json to the subscription POST request, since it contains JSON
  • Change response to subscribeallevents operation to a 201 Created with the subscription URL in a Location header
  • Remove the body from the response to the subscribeevent operation

E.g.

HTTP/1.1 201 CREATED
Content-Type: application/json
Location: /things/lamp/events/1234-4544-1211

10.4.1.4 unsubscribeallevents

URL set to the URL of the Event resource

DELETE /things/lamp/events looks like it's trying to delete the events endpoints altogether rather than deleting a subscription to the events. Currently this operation also doesn't use the subscription ID provided in the subscribeallevents operation, so if there are multiple subscriptions the Web Thing won't know which one to remove.

  • Change the unsubscribeallevents operation to a DELETE request on the subscription URL provided in the Location header of the response to the subscribeallevents request, instead of on the events URL
  • Remove the Accept header from the DELETE request
  • Specify a 204 No Content response to the DELETE request
  • Provide an example response to the DELETE request

10.4.2 Event Connections

  • Consider renaming "Event Connections" to "Event Notifications"
  • Remove the assertion "If the connection between the Web Thing and the Consumer drops, the Web Thing MUST attempt to re-establish the connection." This is not how Webhooks work, they require a separate connection for each event notification.
  • Remove the assertion "Once the connection is re-established, the Web Thing SHOULD, if possible, send any missed events which occurred since the last successful event notification." This is implied by the assertion "If a Consumer becomes unavailable and the Web Thing cannot successfully transmit event messages to the consumer, it SHOULD attempt several retries at increasing intervals."
  • Remove the assertion: "The Web Thing MAY reuse an existing connection to a listener for subsequent message traffic, or it MAY establish a new connection for each message." This is not how Webhooks work, they require a separate connection for each event notification.
  • Add an assertion to specify that events must be sent with an HTTP POST request with a Content-Type header set to application/json
  • Consider changing the assertion 'When an event message has been received, the Consumer MUST respond with a "HTTP 200 OK" message.' to 'When an event message has been received, the Consumer MUST respond with an HTTP 200 OK response code (if the response contains a body) or an HTTP 204 No Content response code (if the response does not contain a payload).' This is not essential but would help Things to know whether there is a response body to process.

Missing Features

The payload format for events still needs to be specified, but that is being discussed in #258. My proposal is at #258 (comment).

The observe property protocol bindings need to be specified - I have filed #376.

There are also some additional features I think may help with scalability, for which I have filed separate issues:

@mlagally
Copy link
Contributor

Remove the sentence: "Depending on the deployment scenarios and integration requirements for existing consumers, it may be required to use specific data payload formats (e.g. Cloud Events)." (The HTTP Webhook Profile needs to define a single payload format for events in order to guarantee out-of-the-box interoperability. Compatibility with existing consumers should be a non-goal.)

I'm really concerned if we would agree on a policy of having a non-goal to be compatible with the world as it exists.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants