Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
Already on GitHub? Sign in to your account
Add partial implementation for the sync endpoint. #106
Conversation
|
However you'd like to do it is fine. I'm OK with having only some parts of it implemented in the first PR. |
|
Wouldn't be better to move the todo list into the sync issue #8 and break it into smaller ones instead of a giant PR? |
|
At this moment thought about only implement the batch system and rooms/join/account_data for the start. |
|
@mujx Could you look into? |
| + set_presence = PresenceState::Unavailable; | ||
| + } | ||
| + ("timeout", value) => { | ||
| + timeout = u64::from_str_radix(value, 10).map_err(|_| ApiError::unknown(None))?; |
mujx
Oct 20, 2016
Member
You can use a more detailed explanation about the error.
timeout = u64::from_str_radix(value, 10)
.map_err(|err| ApiError::invalid_param("timeout", err.description()))?;| + ("timeout", value) => { | ||
| + timeout = u64::from_str_radix(value, 10).map_err(|_| ApiError::unknown(None))?; | ||
| + } | ||
| + _ => (), |
mujx
Oct 20, 2016
Member
Each parameter matching should be extensive resulting to an invalid_param error if no match was found. Here you drop back to the defaults silently. Also add tests for invalid options.
| @@ -0,0 +1,148 @@ | ||
| +//! Matrix filter. |
mujx
Oct 20, 2016
Member
Not sure if ruma is the best place for these structs. @jimmycuadra ? Also the structs from src/sync.rs.
| + } | ||
| + | ||
| + /// Parse a string to a `Batch`. | ||
| + pub fn parse(str: &str) -> Result<Batch, ApiError> { |
mujx
Oct 20, 2016
•
Member
You can simplify like so
pub fn parse(str: &str) -> Result<Batch, String> {
let values: Vec<&str> = str.split("_").collect();
if values.len() != 2 {
return Err(String::from("Wrong number of tokens"));
}
let room_key = u64::from_str_radix(values[0], 10)
.map_err(|err| err.to_string())?;
let presence_key = u64::from_str_radix(values[1], 10)
.map_err(|err| err.to_string())?;
Ok(Batch::new(room_key, presence_key))
}and then call
let batch = Batch::parse(value)
.map_err(|err| ApiError::invalid_param("since", &err))?;to return any errors.
Generally, use a descriptive message instead of None. It will be useful later for debugging.
| + let v: Vec<&str> = str.split("_").collect(); | ||
| + let mut room_key = 0; | ||
| + let mut presence_key = 0; | ||
| + if v.len() != 2 { |
| +} | ||
| + | ||
| +#[test] | ||
| +fn batch_parse_non_number_should_fail() { |
mujx
Oct 20, 2016
Member
You can remove the should_fail suffix. Add an assert with the error message to explain the test.
farodin91
added
status/needs review
and removed
status/work in progress
labels
Nov 11, 2016
|
I'll split |
farodin91
requested a review
from jimmycuadra
Dec 9, 2016
| + let value = match event.event_type.as_ref() { | ||
| + "m.room.member" => { | ||
| + let event = event.clone(); | ||
| + let member_event: MemberEvent = (*event).try_into()?; |
|
This needs the conflicts resolved once more. Sorry I've taken so long to review this! I'll get to it right away once it's ready to go. |
jimmycuadra
requested changes
Dec 23, 2016
This is really, really fantastic work. Thank you so much for doing this! I left a few comments. All minor stuff.
| @@ -13,7 +13,7 @@ use models::filter::{Filter as DataFilter}; | ||
| use models::user::User; | ||
| use modifier::SerializableResponse; | ||
| -/// Filter | ||
| +/// `Filter` |
jimmycuadra
Dec 23, 2016
Owner
I should've noticed this on the other filter PR, but this needs a real docstring that explains what the type is for.
| @@ -36,7 +36,7 @@ pub struct Filter { | ||
| pub not_senders: Vec<UserId>, | ||
| } | ||
| -/// RoomEventFilter | ||
| +/// `RoomEventFilter` |
| @@ -84,7 +84,7 @@ fn is_false(test: &bool) -> bool { | ||
| } | ||
| -/// RoomFilter | ||
| +/// `RoomFilter` |
| @@ -113,7 +113,7 @@ pub struct RoomFilter { | ||
| pub rooms: Vec<RoomId>, | ||
| } | ||
| -/// EventFormat | ||
| +/// `EventFormat` |
| @@ -160,7 +160,7 @@ impl ::serde::Deserialize for EventFormat { | ||
| } | ||
| } | ||
| -/// FilterResponse | ||
| +/// `FilterResponse` |
| pub use self::versions::Versions; | ||
| pub use self::filter::{GetFilter, PostFilter}; | ||
| mod account; | ||
| mod directory; | ||
| mod event_creation; | ||
| -mod filter; | ||
| +pub mod filter; |
jimmycuadra
Dec 23, 2016
Owner
It feels odd to have this one sub-module be public. We could move the shared types out to a top-level module, but thinking farther ahead, these types are just gonna get pulled out into ruma-client-api, so maybe this is fine for now.
| + use iron::status::Status; | ||
| + use ruma_identifiers::EventId; | ||
| + | ||
| + /// [https://github.com/matrix-org/sytest/blob/develop/tests/31sync/03joined.pl#L3] |
jimmycuadra
Dec 23, 2016
Owner
Let's use a specific commit hash rather than just the "develop" branch so that this URL will continue to point to the right thing even when that file is modified in the future.
| + assert_eq!(room, None); | ||
| + } | ||
| + | ||
| + /// [https://github.com/matrix-org/sytest/blob/develop/tests/31sync/03joined.pl#L43] |
| + Test::assert_json_keys(room.find("ephemeral").unwrap(), vec!["events"]); | ||
| + } | ||
| + | ||
| + /// [https://github.com/matrix-org/sytest/blob/develop/tests/31sync/03joined.pl#L81] |
| + assert_eq!(room, None); | ||
| + } | ||
| + | ||
| + /// [https://github.com/matrix-org/sytest/blob/develop/tests/31sync/04timeline.pl#L1] |
| + assert_eq!(EventId::try_from(event.find("event_id").unwrap().as_str().unwrap()).unwrap().opaque_id(), event_id_2); | ||
| + } | ||
| + | ||
| + /// [https://github.com/matrix-org/sytest/blob/develop/tests/31sync/04timeline.pl#L223] |
| + | ||
| +/// A State Ordering. | ||
| +#[derive(Debug, Clone)] | ||
| +pub struct Batch { |
jimmycuadra
Dec 23, 2016
Owner
Is Batch going to be used in other places besides the sync endpoint? If not, this type could go in the sync module.
| + } | ||
| + | ||
| + /// Make a String from a `Batch`. | ||
| + pub fn to_str(&self) -> String { |
jimmycuadra
Dec 23, 2016
Owner
Let's impl Display for Batch and then use to_string instead. https://doc.rust-lang.org/stable/std/string/trait.ToString.html
| + } | ||
| + | ||
| + /// Parse a string to a `Batch`. | ||
| + pub fn parse(str: &str) -> Result<Batch, String> { |
| @@ -37,7 +37,7 @@ macro_rules! middleware_chain { | ||
| }; | ||
| } | ||
| -/// MiddlewareChain | ||
| +/// `MiddlewareChain` |
| + (access_token, room_id) | ||
| + } | ||
| + | ||
| + pub fn get_next_batch(response: &Response) -> String { |
| + .to_string() | ||
| + } | ||
| + | ||
| + /// Sync. |
| + response | ||
| + } | ||
| + | ||
| + pub fn assert_json_keys(json: &Value, keys: Vec<&str>) { |
jimmycuadra
requested changes
Dec 23, 2016
I requested a few more small changes.
I'll push a new commit to master soon with an update to the Docker image. You can wait until that and then rebase and squash commits.
If you wanna discuss what to rename sync::Sync, let's chat on Matrix.
| +/// The `/sync` endpoint. | ||
| +pub struct GetSync; | ||
| + | ||
| +middleware_chain!(GetSync, [AccessTokenAuth]); |
jimmycuadra
Dec 23, 2016
Owner
I'm not crazy about the name GetSync for the handler. I think it should stay as Sync. I'm not sure what a better name for sync::Sync should be, though. The API endpoint is for a client to sync, but what sync::Sync does is actually just query a bunch of data with various filters, so maybe we could change the sync module into a more generic query module? Again, I'm not sure what that module's Sync type should be named. What do you think?
mujx
Dec 23, 2016
Member
My personal preference would be to keep the Sync endpoint as it is and move the sync structs and functions into models/events.rs and r0/sync.rs. The ideal solution would be to import these structs from ruma-client-api.
| @@ -37,7 +37,7 @@ macro_rules! middleware_chain { | ||
| }; | ||
| } | ||
| -/// `MiddlewareChain` | ||
| +/// `MiddlewareChain` ensures the all endpoints has a chain function. |
| + | ||
| + Ok(Batch::new(room_key, presence_key)) | ||
| + } | ||
| + |
| @@ -257,3 +305,29 @@ impl Sync { | ||
| })) | ||
| } | ||
| } | ||
| + | ||
| + |
| @@ -314,7 +315,7 @@ impl Test { | ||
| } | ||
| - /// Sync. | ||
| + /// Query sync with multiple parameters. |
jimmycuadra
Dec 23, 2016
Owner
This is the same docstring as the sync function above. Did you mean "query sync since a specific point, with multiple parameters"?
| + let response = self.get(&response_path); | ||
| + assert_eq!(response.status, Status::Ok); | ||
| + response | ||
| + |
| + ("timeout", value) => { | ||
| + timeout = u64::from_str_radix(value, 10).map_err(|err| ApiError::invalid_param("timeout", err.description()))?; | ||
| + } | ||
| + _ => (), |
| + let room = response.json().find_path(&["rooms", "join", room_id.as_ref()]).unwrap(); | ||
| + assert!(room.is_object()); | ||
| + | ||
| + let response = test.sync_again(&access_token, None, false, &next_batch); |
mujx
Dec 23, 2016
Member
It would more readable if those functions take a struct as a parameter so we don't have to read the signature to know which value goes where. You can use SyncOptions.
| @@ -37,7 +37,7 @@ macro_rules! middleware_chain { | ||
| }; | ||
| } | ||
| -/// MiddlewareChain | ||
| +/// `MiddlewareChain` ensures that all endpoints have. |
| + /// Return `RoomEvent`'s by `RoomId` and since. | ||
| + pub fn find_room_events(connection: &PgConnection, room_id: &RoomId, since: i64) -> Result<Vec<Event>, ApiError> { | ||
| + let events: Vec<Event> = events::table | ||
| + .filter(events::event_type.like("m.room.%")) |
farodin91
Dec 23, 2016
Member
This could be extend in the future. My implementation only support member and message.
| + Ok(state) | ||
| + } | ||
| + | ||
| + fn convert_events_to_timeline(events: Vec<Event>, timeline_filter: &Option<RoomEventFilter>) -> Result<(i64, Timeline), ApiError> { |
| + })) | ||
| + } | ||
| + | ||
| + /// Handle sync rooms |
| @@ -252,6 +253,88 @@ impl Test { | ||
| .unwrap() | ||
| .to_string() | ||
| } | ||
| + | ||
| + /// Send a message to room. | ||
| + pub fn message(&self, access_token: &str, room_id: &str, message: &str) -> Response { |
| + } | ||
| + | ||
| + /// Create a User and Room. | ||
| + pub fn initial_fixtures(&self) -> (String, String) { |
mujx
Dec 23, 2016
Member
These initial_fixtures hide a lot of info when reading the tests. The gain is only one line and they are not configurable. If you keep them add a parameter for the name of the user and the room options.
| + } | ||
| + | ||
| + /// Query sync with multiple parameters included since. | ||
| + pub fn sync_again(&self, access_token: &str, filter: Option<&str>, full_state: bool, since: &str) -> Response { |
| + let test = Test::new(); | ||
| + let (access_token, room_id) = test.initial_fixtures("carl", true); | ||
| + | ||
| + let options = SyncOptions::new( |
mujx
Dec 23, 2016
Member
A constructor still hides the fields. Just create a struct so the fields and their values are visible.
let options = SyncOptions {
since: ...,
full_state: ...,
...
}| + } | ||
| + | ||
| + /// Create a User and Room. | ||
| + pub fn initial_fixtures(&self, username: &str, public: bool) -> (String, String) { |
mujx
Dec 23, 2016
Member
I meant to pass the whole room options like "{"visibility": "public"}" etc. It's more general and this way it's clear what you mean.
farodin91
changed the title from
Add basic implementation for the sync endpoint.
to
Add partial implementation for the sync endpoint.
Dec 26, 2016
jimmycuadra
merged commit bf4fd61
into
ruma:master
Dec 28, 2016
1 check passed
|
|
farodin91 commentedOct 15, 2016
•
edited
Spec
https://matrix.org/docs/spec/client_server/r0.2.0.html#get-matrix-client-r0-sync
Parameters
State
Details
timeline.events,timeline.limitedtimeline.prev_batchtimeline.events(m.room.member,m.room.message,m.room.history_visibility)timelinelimiting byfiltersince,full_staterooms.join,rooms.leave,rooms.invitesince,filter,full_state,set_presence,timeoutset_presence,timeout