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

Application Service support tracking #228

Closed
33 of 40 tasks
johannescpk opened this issue May 11, 2021 · 6 comments
Closed
33 of 40 tasks

Application Service support tracking #228

johannescpk opened this issue May 11, 2021 · 6 comments
Labels
enhancement New feature or request help wanted Interested in working on the project? These are great additions we'd like to have!

Comments

@johannescpk
Copy link
Contributor

johannescpk commented May 11, 2021

The matrix-sdk-appservice crate should be considered experimental until this issue gets closed

Docs: https://matrix-org.github.io/matrix-rust-sdk/matrix_sdk_appservice/index.html

Todo

Bugs

Hard

Medium

Easy

  • Don't process transactions twice (Don't process the same transaction twice #560)
  • Proper re-export and usage of ruma appservice api instead of hidden/inlined re-export (related: The SDK should use reexported types directly  #152)
    • Using re-exported ruma from a consumer doesn't completely work since it breaks the EventContent macro
  • Switch warp dependency to crates.io version, blocked behind new warp release (after 0.3.1)
  • docs: Explain that incoming transactions are handled by the main user client (appservice: Sync room state #303)
  • Allow to drop or re-initialize cached singleton clients. Done via virtual_users returning the dashmap.
  • Mention in the docs that the appservice is basically a wrapper around the sdk Client
  • Remove outdated serde_yaml dependency from matrix-sdk crate
  • Fix typo: try_into_http_request_with_identy_assertion -> try_into_http_request_with_identity_assertion :D
  • Rename client_with_localpart to client_with_identity_assertion (appservice: Refactor API #231)
  • Move get_host_and_port_from_registration into AppserviceRegistration::get_host_and_port (appservice: Refactor API #231)
  • Check if it makes sense to introduce an InnerAppservice for Arc-purposes, or single Arcs for member fields (from feat: appservice #204 (comment))
  • Initial version (feat: appservice #204)

Refactoring

  • matrix_sdk_test: use SyncStateEvent::into_full_event instead of value_with_room_id
  • Try intermediate conversion from http::Request instead (see appservice: Add warp support #254 (comment))
  • Consider renaming Appservice to AppService (appservice: Rename Appservice to AppService #272)
  • localpart-unwrap in Appservice::client and Appservice::client_with_config should be deduplicated and re-use get_cached_client instead, but was hitting some Send + Sync roadblock
  • Using appservice in the actix examples probably wants a mutex instead of cloning, or maybe allow &mut self event handler methods
  • Make localpart in client_with_config() a non-Option, since the main appservice user config can't get overwritten anyway

Optional / Nice to have

  • Consider removing actix (appservice: Drop actix #290)
    • Reasoning: actix does a rather specific tokio runtime setup that leads to being able to easily block threads
  • Think about appservice-FFI scenarios: would spawning the webserver in a thread already help? how to sanely FFI events / event handler?
  • Support hyper: maybe merge in from matrix-appservice-rs? /cc @lieuwex
    • hyper is supported by using the warp filter as tower service
  • Consider supporting constructing clients based on server_name & well-known

Constant effort

  • Tracing
  • Test-Coverage

Other Application Service Implementations

(todo: feature matrix table?)

Bridge projects

Related projects

@MTRNord
Copy link
Contributor

MTRNord commented Jun 4, 2021

Additional to the membership Bug is that invites only go to the EventHandler::on_room_member handler when they come in via the invite branch of the timeline (See:

AnySyncStateEvent::RoomMember(e) => self.on_room_member(room, e).await,
which gets called by
for event in
room_info.timeline.events.iter().filter_map(|e| e.event.deserialize().ok())
{
self.handle_timeline_event(room.clone(), &event).await;
}
which then gets called by
handler.handle_sync(&sync_response).await;
which however gets the wrong timelines from and has a joined instead of an Invite room that gets passed from https://github.com/matrix-org/matrix-rust-sdk/blob/master/matrix_sdk_base/src/client.rs#L886-L905 ).

This doesnt seem to be an easy fix as currently a) all appservice Events come via the join Part (due to ruma) and b) the Event handler Part where this gets relayed is struct typed. So the joined room struct cant be passed along to make this work from the join Part.

Using the following code I wasnt getting any member events whatsoever.

async fn on_room_member(&self, room: Room, event: &SyncStateEvent<MemberEventContent>) {
        if !&event.state_key.contains("@server_stats:nordgedanken.dev") {
            return;
        }
        info!("Got invite");

        if let MembershipState::Invite = event.content.membership {
            let client = self.appservice.client(Some("server_stats")).await;
            client.join_room_by_id(room.room_id()).await.unwrap();
            VoyagerBot::set_direct(client, room.clone(), event).await;
            info!("Successfully joined room {}", room.room_id());
        };
    }

I am running on MTRNord@bf59b65 currently

@johannescpk
Copy link
Contributor Author

johannescpk commented Jun 4, 2021

@MTRNord This should be fixed in #254 which also includes a bugfix due to improved test coverage. The issue being that identity assertion and auth forcing didn't work as expected any longer.

@johannescpk
Copy link
Contributor Author

@MTRNord Also, about invite vs join events, please see https://docs.ruma.io/ruma/api/appservice/event/push_events/v1/struct.IncomingRequest.html#method.try_into_sync_response for the rationale. Added that text to the set_event_handler docs as well now. Hope that makes sense. If you have different ideas about it, please let me know.

@agraven
Copy link
Contributor

agraven commented Aug 16, 2021

I think item 4 in "Nice to have" can be checked since the linked PR has been merged

@johannescpk
Copy link
Contributor Author

@agraven Clarified that item's description. The linked PR was only about introducing it on the Client, I think it needs more work to support it in the appservice crate

@Hywan
Copy link
Member

Hywan commented Sep 20, 2023

I'm closing this issue since #2509 has removed matrix-sdk-appservice.

@Hywan Hywan closed this as completed Sep 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Interested in working on the project? These are great additions we'd like to have!
Projects
None yet
Development

No branches or pull requests

5 participants