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

Several questions after implementing #114

Open
joachimvh opened this issue Oct 4, 2022 · 3 comments
Open

Several questions after implementing #114

joachimvh opened this issue Oct 4, 2022 · 3 comments

Comments

@joachimvh
Copy link
Contributor

Having just implemented support for notifications in CSS, I thought it might be interesting to share any thoughts and questions I had. Wasn't sure what the best way was, so feel free to split this up over new/other issues should some of it be relevant there.

While not explicitly stated every time, most questions have as additional goal to make the answer be part of the specification, because otherwise clients can't depend on that being the case.

Which events

I understand the specification wants to be general to allow many kinds of notifications,
but I do think it would help if there was a more extensive description of specific notifications,
while allowing server to expand upon this.
Because now a server can fully support the notification spec without ever actually sending out a notification.

The CSS implementation has the Create/Update/Delete activities. In the case of Delete there already was the issue that there is no state field to send along since the resource no longer exists, so that would break with the required data model from the spec.

Which permissions

Similar to the above, clearer guidelines would help there, as I see the editor's draft is going to remove the requirement to have Read permissions.

An additional issue is how to handle changing permissions on a resource. Is the server expected to do something then? Specifically, a user has Read permissions on a resource and subscribes to receive notifications on that resource. Afterwards, someone changes the ACL so they lose those permissions. But since authorization is only checked when subscribing they will still keep receiving notifications (until the subscription expires).

What to return on GET/HEAD/OPTIONS

Servers MUST accept GET, HEAD and OPTIONS requests targeting the subscription resource.

I actually missed this while implementing so the CSS version currently would not conform to that,
but what exactly should a server return as a response to those requests? The same as what was found in the resource used to discover it?

Accept feature

What exactly is the reason for this feature to exist? It feels like this is a very specific feature and most clients would be fine with just JSON-LD.

There is no way to advertise for a server which values it accepts for this field. It is also not clear when the server should error if the format is unsupported.

State feature

What activity should be used when sending out this notification? Since it's not really something that happens but just a status update. Unless the last activity that was sent out needs to be used. An additional issue is if in the meantime the target resource was removed, since it doesn't have a state then. Or the reverse, the client does not have a state value since the resource didn't exist yet when they checked, but want to receive the state notification if in the meantime it was created.

For the WebSocket implementation we send out a notification the first time a WebSocket connection is made for that subscription. But what in the case of, for example, WebHooks? Should it immediately be sent out? Can it be sent out before the server even sent its response as an answer to a subscription request (because during processing the request it notices it should send out a state notification)?

JSON-LD context

Since the https://www.w3.org/ns/solid/notification/v1 context does not exist yet, I created one internally for the server. But is it possible to create a context so that the state/rate/expiration strings in the JSON-LD discovery example are actually interpreted as URIs? The only way I could get that working is if they were prefixed.

JSON-LD discovery

There already is an issue about the JSON-LD format so won't go into that. An additional confusion is how the JSON-LD response should look like when targeting the storage description resource for discovery. Depending on the other contents in that document, it might just be impossible to have the expected format as shown in the spec, since an @graph statement might be necessary if other resources are also described in there.

@elf-pavlik
Copy link
Member

FYI solid/vocab#62 includes a draft of JSON-LD context

@csarven
Copy link
Member

csarven commented Jan 9, 2023

Thank you for implementing and providing feedback. Please note that there had been a number of changes since Version 0.1.0. See http://solid.github.io/notifications/protocol#change-log of the ED that's. Version 0.2.0 publication is currently pending approval in solid/specification. Most of the changes are data model related, and some clarifications on the discovery path, along with request to publish the Notification vocabulary and the JSON-LD context.

In https://solid.github.io/notifications/protocol#classes-of-products you'll note specific groups of conforming implementations. We're gathering feedback here on implementations for now: #141 . Please update the table as you see fit for CSS or other implementations that you are aware of, or let me know, and I can.

I'll follow-up with updates based on the following:


it would help if there was a more extensive description of specific notifications

Noted.

Because now a server can fully support the notification spec without ever actually sending out a notification.

I suppose. It just means it is not particularly useful. The NotificationSender always has that right - to send notifications at its own discretion. Although, that's beside your main point, we can clarify this.

there is no state field to send along since the resource no longer exists, so that would break with the required data model from the spec.

state is one of the optional #notification-features, and even when the #NotificationSender supports the feature, it is not required to be part of the notification-message. As you say, in some cases, it needs to be left out and this is covered by definition.

An additional issue is how to handle changing permissions on a resource.

Good consideration. A Note will help here.

what exactly should a server return as a response to those requests?

Updated to emphasise that #subscription-resource is described with the #subscription-resource-data-model .

It feels like this is a very specific feature and most clients would be fine with just JSON-LD.

I generally agree with respect to concrete RDF syntaxes. It may be useful for notifications that are preferable in other formats. However, that's still problematic considering that the #notification-message-data-model requires specific RDF vocabularies, so not sure if/how realistic it'd be to expect that to conform in non-RDF formats. Having said that, the feature can be dropped if there isn't sufficient implementations - I'd be in favour of dropping it.

It is also not clear when the server should error if the format is unsupported.

It need not error. #NotificationServer will take the suggestion into account.

State feature

Optional feature. At the moment it seems that it can be used with a single topic resource, but multiple topic resources doesn't seem well represented with a single (unstructured) value. Features may be used in any notification, but may not necessarily be applicable for each #notification-channel-type. Having said that, state is still under further discussion. We need more implementation feedback, such as yours.

For the WebSocket implementation we send out a notification the first time a WebSocket connection is made for that subscription.

Notification onopen socket / connection established is not defined in the spec, so not required. But out of curiosity, can you share an example of that notification?

JSON-LD context

Current vocab/JSON-LD context proposal is here: solid/vocab#85 . For example, state in JSON-LD context is defined with http://www.w3.org/ns/solid/notifications#state.

JSON-LD discovery

The expectation in the #description-resource would include a statement where a subject has notify:subscription or notify:channel properties. Would the graph matter? In the case where the subject is the storage resource ( https://solidproject.org/ED/protocol#storage-resource ) , we'd see subscription/channel properties. Ultimately a subscription-request could use a different value for topic - I think this applies to the storage resource where the topic resource is one of the resources in the storage. In the describedby case, the topic is more likely to be the same as the subject resource that advertises describedby.

@joachimvh
Copy link
Contributor Author

Thanks for the extensive reply. I might have more comments after going through the 0.2 specification and updating the CSS implementation accordingly, but this issue can be closed for me if there is no further discussion.

But out of curiosity, can you share an example of that notification?

The format would be the same as any other notification. At the time the socket is opened we check the status of the resource. If it exists we send out an Update notification, if it does not we send out a Delete notification.

Current vocab/JSON-LD context proposal is here: solid/vocab#85 .

I learned something new about how to write JSON-LD contexts since that does indeed fix my problem.

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