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

webhook properties section #382

Merged
merged 18 commits into from
Apr 19, 2023
Merged

Conversation

mlagally
Copy link
Contributor

@mlagally mlagally commented Mar 24, 2023

resolves #376


Preview | Diff

@mlagally mlagally changed the title WIP: initial draft of webhook properties section Draft: initial draft of webhook properties section Apr 5, 2023
@mlagally
Copy link
Contributor Author

mlagally commented Apr 5, 2023

Ready for review.

Copy link
Contributor

@egekorkan egekorkan left a comment

Choose a reason for hiding this comment

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

Mostly minor operation names

index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
@benfrancis
Copy link
Member

benfrancis commented Apr 6, 2023

Many of the points from my review of the events protocol binding (#375) also apply to this proposed properties binding, since the assertions have been copied across from there. I would suggest resolving those points before landing this and duplicating all of the same issues.

In particular, I would recommend:

  • Change observeproperty and observeallproperties responses to 201 with a subscription URI in a header rather than an ID in the body. Add a Content-type header and remove the Accept header on observeproperty and observeallproperties requests to reflect this.
  • Change unobserveproperty and unobserveallproperties requests to a DELETE on the subscription URL, with a 204 response and an example

I think these changes are important so that the protocol binding doesn't rely on a fixed URL structure (which we have so far avoided in all of the other protocol bindings). This would also resolve the current issue with two different unsubscribe mechanisms, one of which has strange semantics (POST to /properties/level is subscribe and POST to properties/level/123-456-789 is unsubscribe), and the other is ambiguous (both operations are POST requests on the same URL, so presumably would have to be differentiated by the content in the body though that isn't shown). From a REST point of view it makes more sense to me to create a subscription resource with the ID in its URI to represent a subscription, then delete that resource to cancel the subscription. This would be consistent with what we do in the actions protocol binding for cancelling something.

The subscription URI could alternatively be included in both a header and the body of the observeproperty response (which was the compromise we came up with for the actions protocol binding), but it should definitely be a URI and not just an ID string.

Much of my feedback on the Event Connections section in #375 also applies here:

  • Remove the assertion "If the connection between the Web Thing and the Consumer drops, the Web Thing MUST re-establish the connection."
  • Remove the assertion "Once the connection is re-established the Web Thing SHOULD, if possible, send any missed notifications which occurred since the last successful notification."

(These assertions have been copied and pasted from the SSE protocol binding, but they don't make sense for Webhooks where every property reading would be sent in a separate HTTP request, and therefore a separate TCP "connection".)

  • Add assertions to say that property readings must be sent from the Thing to the Consumer with an HTTP POST request with a Content-Type header set to application/json, with a 200 or 204 response.

My suggestions about batched event payloads, event rate limiting, mutual authentication and abuse protection also apply here, but can be addressed separately.

@mlagally mlagally changed the title Draft: initial draft of webhook properties section Draft: webhook properties section Apr 17, 2023
index.html Outdated Show resolved Hide resolved
@mlagally mlagally changed the title Draft: webhook properties section webhook properties section Apr 17, 2023
@mlagally
Copy link
Contributor Author

@benfrancis
thanks for your thorough review. I addressed most points, also see corresponding PRs #390 #391 and #392

@mlagally
Copy link
Contributor Author

Approve to merge in Profile call on April 19th

@mlagally mlagally merged commit 593bffa into main Apr 19, 2023
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.

Specify observe property protocol bindings for HTTP Webhook Profile
4 participants