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

MSC3869: Read event relations with the Widget API #3869

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

@dhenneke dhenneke force-pushed the nic/feat/widgetapi-read-relations branch from 22ad7b6 to 6d5a6a0 Compare August 16, 2022 15:21
@dhenneke dhenneke changed the title MSCXXXX: Add an MSC for a new Widget API action to read related events from a matrix room MSC3869: Add an MSC for a new Widget API action to read related events from a matrix room Aug 16, 2022
@dhenneke dhenneke changed the title MSC3869: Add an MSC for a new Widget API action to read related events from a matrix room MSC3869: Read event relations with the Widget API Aug 16, 2022
@dhenneke dhenneke marked this pull request as ready for review August 16, 2022 15:35
@robintown robintown requested review from robintown, toger5 and dbkr August 16, 2022 15:44
@turt2live turt2live added proposal A matrix spec change proposal kind:feature MSC for not-core and not-maintenance stuff widgets anything to do with widgets needs-implementation This MSC does not have a qualifying implementation for the SCT to review. The MSC cannot enter FCP. labels Aug 16, 2022
Copy link
Member

@robintown robintown left a comment

Choose a reason for hiding this comment

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

Generally looks like what I was expecting 👍

proposals/3869-widgetapi-read-event-relations.md Outdated Show resolved Hide resolved
Comment on lines 78 to 79
The `direction` parameter is used to specify the direction to search for relations. It has the same
semantic as defined by ([MSC2675](https://github.com/matrix-org/matrix-spec-proposals/blob/main/proposals/2675-aggregations-server.md)).
Copy link
Member

Choose a reason for hiding this comment

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

I'm failing to find the direction parameter in either MSC2675 or the specification.

Copy link
Author

Choose a reason for hiding this comment

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

You're right. I mixed up the MSC numbers. I meant MSC3715.

It is available as an experimental option in synapse:

https://github.com/matrix-org/synapse/blob/3dd175b628bab5638165f20de9eade36a4e88147/synapse/rest/client/relations.py#L59-L67

And also part of the fetchRelations function in the matrix-js-sdk where I come across it. Though the name of the query parameter doesn't match with the synapse implementation 🤔:

https://github.com/matrix-org/matrix-js-sdk/blob/8502759e3ee3a2b244a442b8dac2b67809d9270e/src/client.ts#L7298

We don't have a real use case for this feature, yet. So we could as well skip it for now. It also seems like the MSC in question isn't final yet.

Copy link
Member

Choose a reason for hiding this comment

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

Hm, well I guess it's ultimately up to you whether to include the parameter, since MSC3715 is fairly close to FCP

Copy link
Author

Choose a reason for hiding this comment

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

Then let's keep it in and hope that it becomes stable in synapse and get's fixed in the matrix-js-sdk so we can actually use it. It is also useful because then the relations(...) endpoint in the RoomWidgetClient will also be fully functioning.

Copy link
Member

Choose a reason for hiding this comment

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

And also part of the fetchRelations function in the matrix-js-sdk where I come across it. Though the name of the query parameter doesn't match with the synapse implementation 🤔:

matrix-org/matrix-js-sdk@8502759/src/client.ts#L7298

This is element-hq/element-web#22501, I believe.

Copy link
Author

Choose a reason for hiding this comment

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

This is vector-im/element-web#22501, I believe.

Looks quite like it. So if we are adding it to the widget api, it would probably best to then name it dir. The client can then decide whether to use the unstable or stable parameter.

proposals/3869-widgetapi-read-event-relations.md Outdated Show resolved Hide resolved
@robintown robintown removed the needs-implementation This MSC does not have a qualifying implementation for the SCT to review. The MSC cannot enter FCP. label Aug 26, 2022
… was available in synapse

Signed-off-by: Dominik Henneke <[email protected]>
Copy link
Member

@dbkr dbkr left a comment

Choose a reason for hiding this comment

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

This feels broadly along the lines of our continuing extensions of the CS API into the widget API which feels like a lot of duplication, but if it gets us to a better place in the short term it may be viable.

Copy link

@toger5 toger5 left a comment

Choose a reason for hiding this comment

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

Looks good to me.
I am very happy about the doors this will open. Widgets finally can read "unlimited" event lists!
I commented a couple of things that I think would be cool to have mentioned here (or to be more explicit) for future reference.

If the widget doesn't have appropriate permission, or an error occurs anywhere along the send path,
a standardized widget error response is returned.

If the request was successful, the client sends the following response:
Copy link

Choose a reason for hiding this comment

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

The client is obligated to return what the widget requested and also to fetch all new events?
I think that should be made extra clear because that is something which usually is not the case. The client does not need to fulfill any guarantees in the other MSC's

Copy link
Author

Choose a reason for hiding this comment

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

You mean that the client could also respond to this with a local cache of the relations? I implied that it should always get it from CS-API because it's also how it is (and was already) implemented in the matrix-js-sdk/Element. But it might be a good idea to clarify it here.


In an e2ee room, all the events must be decrypted in the client prior to applying the filters or
providing them to the widget. This can take a considerable amount of time. The widget should take
care that it selects a reasonable `limit` to not run into timeouts in the widget transport layer.
Copy link

Choose a reason for hiding this comment

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

maybe we should mention that this is the first widget api call that can force the client to trigger new cs-api calls. (all other msc's are phrased in a way, that the client can decide if it does not send the events.)

(I also think it would be nice to be more explicit what the clients responsibilities are. see other comment)

searched is minimized.

The same limitations would apply if we would consider to provide direct access to the
`GET /_matrix/client/v3/rooms/{roomId}/messages` endpoint.
Copy link

Choose a reason for hiding this comment

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

Maybe another alternative could be listed. Just to track the idea, that if the client also is capable of setting filters + use /messages custom event types could be used for a similar optimization. But I am not sure how expensive setting filters is.

Copy link
Author

Choose a reason for hiding this comment

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

Not sure what you mean here. Filters won't work for e2e encryption because one would need to read and decrypt every message in the room to decide it which is not practical. But we could add a comment to clarify it.

If the widget doesn't have appropriate permission, or an error occurs anywhere along the send path,
a standardized widget error response is returned.

If the request was successful, the client sends the following response:
Copy link

Choose a reason for hiding this comment

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

something along the lines of:

Suggested change
If the request was successful, the client sends the following response:
Widgets can rely on the complete relation lists.
The client is responsible to fetch all missing events form the matrix server.
If the request was successful, the client sends the following response:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind:feature MSC for not-core and not-maintenance stuff proposal A matrix spec change proposal widgets anything to do with widgets
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants