#443 Big refactoring of the Inbox

Merged
elegaanz merged 37 commits from inbox-refactor into master 1 year ago
elegaanz commented 1 year ago (Migrated from github.com)

We now have a type that routes an activity through the registered handlers
until one of them matches.

Each Actor/Activity/Object combination is represented by an implementation of AsObject

These combinations are then registered on the Inbox type, which will try to deserialize
the incoming activity in the requested types.

Advantages:

  • nicer syntax: the final API is clearer and more idiomatic
  • more generic: only two traits (AsActor and AsObject) instead of one for each kind of activity
  • we always try to dereference unknown objects
  • it is easier to see which activities we handle and which one we don’t

(sorry for the big diff once again 😕)

We now have a type that routes an activity through the registered handlers until one of them matches. Each Actor/Activity/Object combination is represented by an implementation of AsObject These combinations are then registered on the Inbox type, which will try to deserialize the incoming activity in the requested types. Advantages: - nicer syntax: the final API is clearer and more idiomatic - more generic: only two traits (`AsActor` and `AsObject`) instead of one for each kind of activity - we always try to dereference unknown objects - it is easier to see which activities we handle and which one we don't (sorry for the big diff once again :confused:)
codecov[bot] commented 1 year ago (Migrated from github.com)
Owner

Codecov Report

Merging #443 into master will increase coverage by 12.81%.
The diff coverage is 54.19%.

@@             Coverage Diff             @@
##           master     #443       +/-   ##
===========================================
+ Coverage   29.66%   42.48%   +12.81%     
===========================================
  Files          66       68        +2     
  Lines        7190     9314     +2124     
===========================================
+ Hits         2133     3957     +1824     
- Misses       5057     5357      +300
# [Codecov](https://codecov.io/gh/Plume-org/Plume/pull/443?src=pr&el=h1) Report > Merging [#443](https://codecov.io/gh/Plume-org/Plume/pull/443?src=pr&el=desc) into [master](https://codecov.io/gh/Plume-org/Plume/commit/7645fdbf35ed5a53521fe4af60203eae66354134?src=pr&el=desc) will **increase** coverage by `12.81%`. > The diff coverage is `54.19%`. ```diff @@ Coverage Diff @@ ## master #443 +/- ## =========================================== + Coverage 29.66% 42.48% +12.81% =========================================== Files 66 68 +2 Lines 7190 9314 +2124 =========================================== + Hits 2133 3957 +1824 - Misses 5057 5357 +300 ```
trinity-1686a reviewed 1 year ago
trinity-1686a left a comment

first batch of comments, I’ll run some tests this evening or tomorrow

trinity-1686a commented 1 year ago

as it is more of a lib kind, I don’t think it is a good idea. Returning an Option and .expect()ing on it would be better if possible

as it is more of a lib kind, I don't think it is a good idea. Returning an Option<String> and `.expect()`ing on it would be better if possible
trinity-1686a commented 1 year ago

is it calling itself recursively? a {"id":{"id":{"id":{"id":"something"}}} would be considered valid and having an id if so, and as it is cloning, with a big enough json, memory usage could grow quadratic to the size of the original payload

is it calling itself recursively? a `{"id":{"id":{"id":{"id":"something"}}}` would be considered valid and having an id if so, and as it is cloning, with a big enough json, memory usage could grow quadratic to the size of the original payload
@@ -691,3 +680,4 @@
let conn = &*r.conn;
conn.test_transaction::<_, (), _>(|| {
fill_database(conn);

trinity-1686a commented 1 year ago

why was it 0 before and 1 now?

why was it 0 before and 1 now?
trinity-1686a commented 1 year ago

this could use a use statement 🤔

this could use a `use` statement :thinking:
@@ -344,3 +378,4 @@ pub mod schema;
pub mod search;
pub mod tags;
pub mod users;
pub use plume_rocket::PlumeRocket;
trinity-1686a commented 1 year ago

does this mean no more warnings when building with pg backend?

does this mean no more warnings when building with pg backend?
elegaanz (Migrated from github.com) reviewed 1 year ago
@@ -691,3 +680,4 @@
let conn = &*r.conn;
conn.test_transaction::<_, (), _>(|| {
fill_database(conn);

Plume_migration_agent commented 1 year ago

This function is never used I think, so we didn’t noticed this bug before, but the first ID in databases is 1, never 0.

This function is never used I think, so we didn't noticed this bug before, but the first ID in databases is 1, never 0.
elegaanz (Migrated from github.com) reviewed 1 year ago
@@ -344,3 +378,4 @@ pub mod schema;
pub mod search;
pub mod tags;
pub mod users;
pub use plume_rocket::PlumeRocket;
Plume_migration_agent commented 1 year ago

Normally, yes. 🙂

Normally, yes. :slightly_smiling_face:
elegaanz (Migrated from github.com) reviewed 1 year ago
Plume_migration_agent commented 1 year ago

I guess I temporally “disabled” it to see what errors I got or something like that, and I forgot to completely remove it. 😬 (I should probably review my own code before pushing it)

I guess I temporally "disabled" it to see what errors I got or something like that, and I forgot to completely remove it. :grimacing: (I should probably review my own code before pushing it)
trinity-1686a reviewed 1 year ago
trinity-1686a left a comment

I’m trying to test each activities. Sending/receiving delete seems to not be working. Sending like/reshare/follow works, but not receiving them. I haven’t tested some Undo because I wasn’t able to create the related activity in the first place

@@ -23,0 +9,4 @@
use rocket::{data::*, http::Status, response::status, Outcome::*, Request};
use rocket_contrib::json::*;
use serde::Deserialize;
use std::io::Read;
trinity-1686a commented 1 year ago

Deleting comment seems to return a Shared inbox error: Inbox(InvalidObject)

Deleting comment seems to return a `Shared inbox error: Inbox(InvalidObject)`
trinity-1686a commented 1 year ago

this doesn’t seems to work

this doesn't seems to work
trinity-1686a commented 1 year ago

this doesn’t seems to work

this doesn't seems to work
trinity-1686a commented 1 year ago

Deleting post seems to return a Shared inbox error: Inbox(InvalidObject)

Deleting post seems to return a `Shared inbox error: Inbox(InvalidObject)`
trinity-1686a commented 1 year ago

this doesn’t seems to work

this doesn't seems to work
@@ -106,3 +100,4 @@
&(&*conn, &rockets.intl.catalog, Some(user.clone())),
post.clone(),
blog,
&*form,
trinity-1686a commented 1 year ago

this line seems to be failing on my local instance. I haven’t searched why yet

this line seems to be failing on my local instance. I haven't searched why yet
trinity-1686a commented 1 year ago

This seems to be failing too

This seems to be failing too
elegaanz (Migrated from github.com) reviewed 1 year ago
@@ -106,3 +100,4 @@
&(&*conn, &rockets.intl.catalog, Some(user.clone())),
post.clone(),
blog,
&*form,
Plume_migration_agent commented 1 year ago

If it is exactly on this line, the only thing that can fail is the JSON serialization I think. But that doesn’t make sense?? I’ll see if I can reproduce.

If it is exactly on this line, the only thing that can fail is the JSON serialization I think. But that doesn't make sense?? I'll see if I can reproduce.
elegaanz commented 1 year ago (Migrated from github.com)
Owner

Thanks for your review @fdb-hiroshima. I’ll try to reproduce your issues and fix them, and maybe add tests for it in plume-models too.

Thanks for your review @fdb-hiroshima. I'll try to reproduce your issues and fix them, and maybe add tests for it in plume-models too.
trinity-1686a reviewed 1 year ago
@@ -106,3 +100,4 @@
&(&*conn, &rockets.intl.catalog, Some(user.clone())),
post.clone(),
blog,
&*form,
trinity-1686a commented 1 year ago

I’ve added a debug print on the line above and the line under, I get one but not the other (I consider with::<...> and done to be on the same line, they are the same statement)
Edit: dbg!() says done() return Err(Inbox(InvalidObject))

I've added a debug print on the line above and the line under, I get one but not the other (I consider with::<...> and done to be on the same line, they are the same statement) Edit: dbg!() says done() return `Err(Inbox(InvalidObject))`
igalic (Migrated from github.com) reviewed 1 year ago
Plume_migration_agent commented 1 year ago

what does the where clause here mean? I’ve never seen + Debug @_@

what does the `where` clause here mean? I've never seen `+ Debug` @_@
Plume_migration_agent commented 1 year ago

given that this match only has one match and else, other, maybe use if let here.

given that this `match` only has one match and else, `other`, maybe use `if let` here.
@@ -49,0 +350,4 @@
/// # impl FromId<()> for Account {
/// # type Error = ();
/// # type Object = Person;
/// #
Plume_migration_agent commented 1 year ago

is this missing an implementation, or is this it? 😜

is this missing an implementation, or is this it? 😜
@@ -691,3 +680,4 @@
let conn = &*r.conn;
conn.test_transaction::<_, (), _>(|| {
fill_database(conn);

Plume_migration_agent commented 1 year ago

Instance should probably have a method that returns the local instance_id

Instance should probably have a method that returns the local `instance_id`
trinity-1686a reviewed 1 year ago
trinity-1686a commented 1 year ago

This mean that E must Implement From<InboxError<E>> and Debug (Debug allow ugly display via println!("{:?}", yourStruct)) for the functions in this impl block to be available

This mean that E must Implement `From<InboxError<E>>` and `Debug` (`Debug` allow ugly display via `println!("{:?}", yourStruct)`) for the functions in this impl block to be available
elegaanz (Migrated from github.com) reviewed 1 year ago
@@ -49,0 +350,4 @@
/// # impl FromId<()> for Account {
/// # type Error = ();
/// # type Object = Person;
/// #
Plume_migration_agent commented 1 year ago

No it is the default implementation: unless the implementation specifies a shared inbox URL, it is None (but in reality I think all our implementations override this behavior)

No it is the default implementation: unless the implementation specifies a shared inbox URL, it is `None` (but in reality I think all our implementations override this behavior)
elegaanz commented 1 year ago (Migrated from github.com)
Owner

I fixed all the conflicts, and now instead of using Context we use PlumeRockets. I also deployed this branch to https://baptiste.gelez.xyz to make testing easier if needed.

I fixed all the conflicts, and now instead of using `Context` we use `PlumeRockets`. I also deployed this branch to https://baptiste.gelez.xyz to make testing easier if needed.
igalic (Migrated from github.com) reviewed 1 year ago
igalic (Migrated from github.com) left a comment

weak 👀

Plume_migration_agent commented 1 year ago

is there such a thing as if not let? so we can move this whole block out by one indent, by returning early?

is there such a thing as `if not let`? so we can move this whole block out by one indent, by returning early?
Plume_migration_agent commented 1 year ago

debug?

debug?
Plume_migration_agent commented 1 year ago

if author != reiten Err

and remove the entire of indent, pls

if author != reiten Err and remove the entire of indent, pls
elegaanz (Migrated from github.com) reviewed 1 year ago
Plume_migration_agent commented 1 year ago

I don’t think so…

I don't think so…
elegaanz (Migrated from github.com) reviewed 1 year ago
trinity-1686a reviewed 1 year ago
trinity-1686a commented 1 year ago

There is match

There is match
elegaanz commented 1 year ago (Migrated from github.com)
Owner

So I did some testing, and here are the results:

Mastodon Pleroma Plume
Announce ✔️ ✔️ ✔️
Create Note ✔️ ✔️ ✔️
Create Article - - ✔️
Delete Note ✔️ ✔️ ✔️
Delete Article - - ✔️
Follow ✔️ ✔️ ✔️
Like ✔️ ✔️ ✔️
Undo Announce ✔️ Works if the Pleroma user is followed ✔️
Undo Follow ✔️ ✔️ ✔️
Undo Like ✔️ Works if the Pleroma user is followed ✔️
Update Article - - ✔️
  1. The CW is not here
  2. The notification is not deleted (see #499)
  3. There is no error, but after refreshing the Mastodon page, the follow is not accepted ( icon)

I will try to debug a bit more the errors I encountered.

So I did some testing, and here are the results: | | Mastodon | Pleroma | Plume | |:--------------:|:--------:|:---------------------------------------:|:-:| | Announce | ✔️ | ✔️ | ✔️ | | Create Note | ✔️ | ✔️ | ✔️ | | Create Article | - | - | ✔️ | | Delete Note | ✔️ | ✔️ | ✔️ | | Delete Article | - | - | ✔️ | | Follow | ✔️ | ✔️ | ✔️ | | Like | ✔️ | ✔️ | ✔️ | | Undo Announce | ✔️ | Works if the Pleroma user is followed | ✔️ | | Undo Follow | ✔️ | ✔️ | ✔️ | | Undo Like | ✔️ | Works if the Pleroma user is followed | ✔️ | | Update Article | - | - | ✔️ | 1. ~~The CW is not here~~ 2. ~~The notification is not deleted (see #499)~~ 3. ~~There is no error, but after refreshing the Mastodon page, the follow is not accepted (:hourglass: icon)~~ I will try to debug a bit more the errors I encountered.
igalic commented 1 year ago (Migrated from github.com)
Owner

it would be cool if we could add a service: mastodon and service: pleroma to Travis 😅

it would be cool if we could add a `service: mastodon` and `service: pleroma` to Travis 😅
elegaanz commented 1 year ago (Migrated from github.com)
Owner

I think if we were on Gitlab we could just pull their Docker images… I really miss Gitlab CI.

I think if we were on Gitlab we could just pull their Docker images… I really miss Gitlab CI.
trinity-1686a commented 1 year ago
Owner

can’t we do it the ugly way with some bash script?

can't we do it the ugly way with some bash script?
elegaanz commented 1 year ago (Migrated from github.com)
Owner

We could, but it would take a lot of time to install all the dependencies, and the CI already takes so long to run.

We could, but it would take a lot of time to install all the dependencies, and the CI already takes so long to run.
igalic commented 1 year ago (Migrated from github.com)
Owner

i think we’re starting to discuss integration tests, and it’s not really fruitful in this PR.

i think we're starting to discuss integration tests, and it's not really fruitful in this PR.
igalic (Migrated from github.com) reviewed 1 year ago
igalic (Migrated from github.com) left a comment

👀

@@ -811,0 +727,4 @@
local: false,
// We don't really care about all the following for remote instances
long_description: SafeString::new(""),
short_description: SafeString::new(""),
Plume_migration_agent commented 1 year ago

and space.

and space.
elegaanz commented 1 year ago (Migrated from github.com)
Owner

So, I fixed most issues (and updated the table above). I just need to understand why Pleroma is failing to undo like/boost, fix the space issue @igalic found (even if it is not directly related to this PR, it was here before), and do some optimizations (the current algorithm can dereference the same URL multiple times while handling the same activity for the moment). After that, I think this PR will be ready for real.

So, I fixed most issues (and updated the table above). I just need to understand why Pleroma is failing to undo like/boost, fix the space issue @igalic found (even if it is not directly related to this PR, it was here before), and do some optimizations (the current algorithm can dereference the same URL multiple times while handling the same activity for the moment). After that, I think this PR will be ready for real.
elegaanz commented 1 year ago (Migrated from github.com)
Owner

Update: I optimized the algorithm a little bit to avoid dereferencing the same object twice, fixed the space-in-username issue, and found why it was not possible to Undo Announce and Like from Pleroma. It was just not sending the activity to Plume because no one was following the Pleroma account on the Plume instance. It works fine if I follow my Pleroma account from Plume. I still created a bug report.

So I think this PR is ready for another review. 😄

Update: I optimized the algorithm a little bit to avoid dereferencing the same object twice, fixed the space-in-username issue, and found why it was not possible to Undo Announce and Like from Pleroma. It was just not sending the activity to Plume because no one was following the Pleroma account on the Plume instance. It works fine if I follow my Pleroma account from Plume. I still created a [bug report](https://git.pleroma.social/pleroma/pleroma/issues/779). So I think this PR is ready for another review. :smile:
igalic (Migrated from github.com) reviewed 1 year ago
Plume_migration_agent commented 1 year ago

fmt will not be happy with this :P

fmt will not be happy with this :P
elegaanz (Migrated from github.com) reviewed 1 year ago
Plume_migration_agent commented 1 year ago

Yes, I saw that. Fixed it. ^^

Yes, I saw that. Fixed it. ^^
trinity-1686a reviewed 1 year ago
trinity-1686a left a comment

👀

trinity-1686a commented 1 year ago

I don’t think it can actually panic? Also the Otherwise we return its 'id' field, if it is a string is a bit unclear

I don't think it can actually panic? Also the `Otherwise we return its 'id' field, if it is a string` is a bit unclear
@@ -40,0 +273,4 @@
_ => match object {
Some(o) => Self::from_activity(ctx, o).map_err(|e| (None, e)),
None => Self::from_activity(ctx, Self::deref(id)?).map_err(|e| (None, e)),
},
trinity-1686a commented 1 year ago

Isn’t Sized implied by default?

Isn't Sized implied by default?
elegaanz (Migrated from github.com) reviewed 1 year ago
Plume_migration_agent commented 1 year ago

Yeah, I think I forgot to update the docs.

Yeah, I think I forgot to update the docs.
elegaanz (Migrated from github.com) reviewed 1 year ago
@@ -40,0 +273,4 @@
_ => match object {
Some(o) => Self::from_activity(ctx, o).map_err(|e| (None, e)),
None => Self::from_activity(ctx, Self::deref(id)?).map_err(|e| (None, e)),
},
Plume_migration_agent commented 1 year ago

IIRC, the compiler threw errors when it wasn’t here. 🤷‍♀️

IIRC, the compiler threw errors when it wasn't here. :woman_shrugging:
elegaanz (Migrated from github.com) reviewed 1 year ago
@@ -40,0 +273,4 @@
_ => match object {
Some(o) => Self::from_activity(ctx, o).map_err(|e| (None, e)),
None => Self::from_activity(ctx, Self::deref(id)?).map_err(|e| (None, e)),
},
Plume_migration_agent commented 1 year ago

I’ll try to remove it just to be sure.

I'll try to remove it just to be sure.
elegaanz (Migrated from github.com) reviewed 1 year ago
@@ -40,0 +273,4 @@
_ => match object {
Some(o) => Self::from_activity(ctx, o).map_err(|e| (None, e)),
None => Self::from_activity(ctx, Self::deref(id)?).map_err(|e| (None, e)),
},
Plume_migration_agent commented 1 year ago

It was explicitly needed here, to be able to have Result<Self, _> here, but was not actually required on the two other types.

It was explicitly needed here, to be able to have `Result<Self, _>` here, but was not actually required on the two other types.
trinity-1686a reviewed 1 year ago
@@ -40,0 +273,4 @@
_ => match object {
Some(o) => Self::from_activity(ctx, o).map_err(|e| (None, e)),
None => Self::from_activity(ctx, Self::deref(id)?).map_err(|e| (None, e)),
},
trinity-1686a commented 1 year ago

ok, I did not know

ok, I did not know
trinity-1686a reviewed 1 year ago
trinity-1686a left a comment

👀

trinity-1686a commented 1 year ago

out of scope for this pr, but we should consider a find_or_create of some kind instead of find_by_domain.or_else(insert), so to not tie to much code from one part of the model to another

out of scope for this pr, but we should consider a `find_or_create` of some kind instead of `find_by_domain.or_else(insert)`, so to not tie to much code from one part of the model to another
trinity-1686a commented 1 year ago

is this wanted or leftover?

is this wanted or leftover?
trinity-1686a commented 1 year ago

wouldn’t a and_then be more appropriate?

wouldn't a and_then be more appropriate?
trinity-1686a commented 1 year ago

I think we should try to chase clones everywhere, a lot of theme could be as_ref, which more efficient in term of both memory and computation

I think we should try to chase clones everywhere, a lot of theme could be as_ref, which more efficient in term of both memory and computation
trinity-1686a commented 1 year ago

wouldn’t a and_then be more appropriate?

wouldn't a `and_then` be more appropriate?
@@ -1079,0 +1234,4 @@
&article.object.object_props.id_string().unwrap()
);
}
}
trinity-1686a commented 1 year ago

is this related to #296 ?

is this related to #296 ?
trinity-1686a commented 1 year ago

aren’t ~ files backup some editor make?

aren't ~ files backup some editor make?
elegaanz (Migrated from github.com) reviewed 1 year ago
elegaanz (Migrated from github.com) reviewed 1 year ago
@@ -1079,0 +1234,4 @@
&article.object.object_props.id_string().unwrap()
);
}
}
Plume_migration_agent commented 1 year ago

More or less I think. Adding a url field to posts could solve both issues.

More or less I think. Adding a `url` field to posts could solve both issues.
elegaanz (Migrated from github.com) reviewed 1 year ago
Plume_migration_agent commented 1 year ago

These files are generated by the gettext command line tools, but they are normally gitignored. I’ll remove them.

These files are generated by the gettext command line tools, but they are normally gitignored. I'll remove them.
trinity-1686a commented 1 year ago
Owner

Federation with old Plume

Sending activities

  • ~Unsubscribing from a user does not seem to delete the entry in database~
  • ~Sending comment don’t seem to propagate CW (maybe bug from old Plume)~ This is a bug from old Plume

Receiving

all right

Federation with itself

  • ~Unsubscribing from a user does not seem to delete the entry in database~
  • ~Sending/Receiving Post Update does not seems to update post cover-picture~

pleroma

Sending activities

  • boost (announce) post doesn’t seems to work (unboost untested)
  • follow does not seems to work, but it might be my pleroma having problems (unfollow untested)

Receiving

  • again issues with follow, but again maybe my pleroma
### Federation with old Plume #### Sending activities - ~Unsubscribing from a user does not seem to delete the entry in database~ - ~Sending comment don't seem to propagate CW (maybe bug from old Plume)~ This is a bug from old Plume #### Receiving all right ### Federation with itself - ~Unsubscribing from a user does not seem to delete the entry in database~ - ~Sending/Receiving Post Update does not seems to update post cover-picture~ ### pleroma #### Sending activities - boost (announce) post doesn't seems to work (unboost untested) - follow does not seems to work, but it might be my pleroma having problems (unfollow untested) #### Receiving - again issues with follow, but again maybe my pleroma
trinity-1686a reviewed 1 year ago
trinity-1686a commented 1 year ago

wow I missed how ugly this thing was

wow I missed how ugly this thing was
trinity-1686a commented 1 year ago
Owner

https://github.com/Plume-org/Plume/pull/443#issuecomment-476341573 by the way if we manage to get some certificate we can use in tests, this is now probably possible with circleci

https://github.com/Plume-org/Plume/pull/443#issuecomment-476341573 by the way if we manage to get some certificate we can use in tests, this is now probably possible with circleci
elegaanz commented 1 year ago (Migrated from github.com)
Owner

@fdb-hiroshima The issues with Pleroma are normally fixed now.

@fdb-hiroshima The issues with Pleroma are normally fixed now.
trinity-1686a approved these changes 1 year ago
trinity-1686a left a comment

🚀

elegaanz commented 1 year ago (Migrated from github.com)
Owner

Yay, Finally! Thank you @igalic and @fdb-hiroshima for reviewing this big PR! 😊

Yay, Finally! Thank you @igalic and @fdb-hiroshima for reviewing this big PR! :blush:

Reviewers

trinity-1686a approved these changes 1 year ago
The pull request has been merged as 12efe721cc.
Sign in to join this conversation.
No reviewers
No Milestone
No Assignees
2 Participants
Notifications
Due Date

No due date set.

Dependencies

This pull request currently doesn't have any dependencies.

Loading…
There is no content yet.