w3c / wot-profile

Web of Things (WoT) Profile
http://w3c.github.io/wot-profile/
Other
16 stars 8 forks source link

webhook properties section #382

Closed mlagally closed 1 year ago

mlagally commented 1 year ago

resolves #376


Preview | Diff

mlagally commented 1 year ago

Ready for review.

benfrancis commented 1 year ago

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:

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:

(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".)

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

mlagally commented 1 year ago

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

mlagally commented 1 year ago

Approve to merge in Profile call on April 19th