search

solid / notifications

Solid Notifications Technical Reports
https://solid.github.io/notifications/protocol
MIT License
11 stars 7 forks source link

Spec queries found while testing #98

Open edwardsph opened 2 years ago

edwardsph commented 2 years ago

I'm starting this issue as somewhere to capture comments on the spec that have come up whilst I was looking at building conformance tests.

  1. Without a defined discovery mechanism we obviously can't test section 2.1 but also have to potentially support implementations' own versions of discovery in order to test any further in the spec. The sooner this can be added, the better for testing purposes.
  2. The spec:statement at https://solid.github.io/notifications/protocol#server-subscription-access-controls does not contain the start of the requirement.
  3. How does https://solid.github.io/notifications/protocol#server-subscription-access-read "A client MUST be permitted to read the resource to which it subscribes" add anything to the previous requirement? If it is simply saying you can only subscribe to a resource to which you have access, then I think the sentence doesn't parse well as it almost suggested that subscribing grants access. Perhaps "A client MUST have read access to a resource in order to subscribe to it"

Then I went on to look at the WebsocketSubscription2021 spec: https://solidproject.org/TR/websocket-subscription-2021. This spec says an API must conform to the notification protocol spec above, then lists some requirements but I think there are some issues:

  1. https://solidproject.org/TR/websocket-subscription-2021#client-subscription-type duplicates the requirement in the notification protocol spec and is therefore redundant
  2. https://solidproject.org/TR/websocket-subscription-2021#client-subscription-feature is also a duplication but additionally the IRI of the requirement is client-subscription-feature where I think it should be client-subscription-topic
  3. I think the language of this requirement could be clearer as it is in the notification protocol spec but if it is removed as redundant that solves that concern.
edwardsph commented 2 years ago

Currently the notification data model does not contain any addressable requirements which makes this hard to include in specification tests.

csarven commented 2 years ago

Thanks for this feedback.

re Notifications Protocol:

  1. is now detailed in https://solid.github.io/notifications/protocol#discovery
  2. Fixed as part of update to https://solid.github.io/notifications/protocol#authentication-authorization
  3. Same as above.

re WebSocketSubscription2021 issues, @acoburn can you look into this as part of next set of updates? This issue can be closed once those concerns are resolved.

re Notifications Data Model, will leave it to https://github.com/solid/notifications/pull/60 or https://github.com/solid/notifications/pull/124 and possibly https://github.com/solid/notifications/issues/101 to address this.