Big refactoring of the Inbox #443
Nav izskatītāju
trinity-1686a
Iezīmes
Nav iezīmju
A: API
A: Backend
A: Federation
A: Front-End
A: I18N
A: Meta
A: Security
Build
C: Bug
C: Discussion
C: Enhancement
C: Feature
Compatibility
Dependency
Design
Documentation
Good first issue
Help welcome
Mobile
Rendering
S: Blocked
S: Duplicate
S: Incomplete
S: Instance specific
S: Invalid
S: Needs Voting/Discussion
S: Ready for review
Suggestion
S: Voted on Loomio
S: Wontfix
Nav atskaites punkta
Nav projektu
Nav atbildīgo
2 dalībnieki
Paziņojumi
Izpildes datums
Nav uzstādīts izpildes datums.
Atkarības
Nav atkarību.
Atsauce: Plume/Plume#443
Notiek ielāde…
Pievienot tabulu
Atsaukties jaunā pieteikumā
Nav sniegts apraksts.
Izdzēst zaru "inbox-refactor"
Zara izdzēšana ir neatgriezeniska. Kaut arī izdzēstais zars neilgu laiku var turpināt pastāvēt, pirms tas patiešām tiek noņemts, to vairumā gadījumu NEVAR atsaukt. Turpināt?
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:
AsActorandAsObject) instead of one for each kind of activity(sorry for the big diff once again 😕)
Codecov Report
first batch of comments, I'll run some tests this evening or tomorrow
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 possibleis 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);why was it 0 before and 1 now?
this could use a
usestatement 🤔@ -344,3 +378,4 @@ pub mod schema;pub mod search;pub mod tags;pub mod users;pub use plume_rocket::PlumeRocket;does this mean no more warnings when building with pg backend?
?
@ -691,3 +680,4 @@let conn = &*r.conn;conn.test_transaction::<_, (), _>(|| {fill_database(conn);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.
@ -344,3 +378,4 @@ pub mod schema;pub mod search;pub mod tags;pub mod users;pub use plume_rocket::PlumeRocket;Normally, yes. 🙂
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'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;Deleting comment seems to return a
Shared inbox error: Inbox(InvalidObject)this doesn't seems to work
this doesn't seems to work
Deleting post seems to return a
Shared inbox error: Inbox(InvalidObject)this doesn't seems to work
@ -106,3 +100,4 @@&(&*conn, &rockets.intl.catalog, Some(user.clone())),post.clone(),blog,&*form,this line seems to be failing on my local instance. I haven't searched why yet
This seems to be failing too
@ -106,3 +100,4 @@&(&*conn, &rockets.intl.catalog, Some(user.clone())),post.clone(),blog,&*form,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.
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.
@ -106,3 +100,4 @@&(&*conn, &rockets.intl.catalog, Some(user.clone())),post.clone(),blog,&*form,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))what does the
whereclause here mean? I've never seen+ Debug@_@given that this
matchonly has one match and else,other, maybe useif lethere.@ -49,0 +350,4 @@/// # impl FromId<()> for Account {/// # type Error = ();/// # type Object = Person;/// #is this missing an implementation, or is this it? 😜
@ -691,3 +680,4 @@let conn = &*r.conn;conn.test_transaction::<_, (), _>(|| {fill_database(conn);Instance should probably have a method that returns the local
instance_idThis mean that E must Implement
From<InboxError<E>>andDebug(Debugallow ugly display viaprintln!("{:?}", yourStruct)) for the functions in this impl block to be available@ -49,0 +350,4 @@/// # impl FromId<()> for Account {/// # type Error = ();/// # type Object = Person;/// #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)I fixed all the conflicts, and now instead of using
Contextwe usePlumeRockets. I also deployed this branch to https://baptiste.gelez.xyz to make testing easier if needed.weak 👀
is there such a thing as
if not let? so we can move this whole block out by one indent, by returning early?debug?
if author != reiten Err
and remove the entire of indent, pls
I don't think so…
Oops.
There is match
So I did some testing, and here are the results:
The CW is not hereThe notification is not deleted (see #499)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.
it would be cool if we could add a
service: mastodonandservice: pleromato Travis 😅I think if we were on Gitlab we could just pull their Docker images… I really miss Gitlab CI.
can't we do it the ugly way with some bash script?
We could, but it would take a lot of time to install all the dependencies, and the CI already takes so long to run.
i think we're starting to discuss integration tests, and it's not really fruitful in this PR.
👀
@ -811,0 +727,4 @@local: false,// We don't really care about all the following for remote instanceslong_description: SafeString::new(""),short_description: SafeString::new(""),and space.
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.
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. 😄
fmt will not be happy with this :P
Yes, I saw that. Fixed it. ^^
👀
I don't think it can actually panic? Also the
Otherwise we return its 'id' field, if it is a stringis 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)),},Isn't Sized implied by default?
Yeah, I think I forgot to update the docs.
@ -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)),},IIRC, the compiler threw errors when it wasn't here. 🤷♀️
@ -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)),},I'll try to remove it just to be sure.
@ -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)),},It was explicitly needed here, to be able to have
Result<Self, _>here, but was not actually required on the two other types.@ -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)),},ok, I did not know
👀
out of scope for this pr, but we should consider a
find_or_createof some kind instead offind_by_domain.or_else(insert), so to not tie to much code from one part of the model to anotheris this wanted or leftover?
wouldn't a and_then be more appropriate?
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
wouldn't a
and_thenbe more appropriate?@ -1079,0 +1234,4 @@&article.object.object_props.id_string().unwrap());}}is this related to #296 ?
aren't ~ files backup some editor make?
leftover…
@ -1079,0 +1234,4 @@&article.object.object_props.id_string().unwrap());}}More or less I think. Adding a
urlfield to posts could solve both issues.These files are generated by the gettext command line tools, but they are normally gitignored. I'll remove them.
Federation with old Plume
Sending activities
Unsubscribing from a user does not seem to delete the entry in databaseSending comment don't seem to propagate CW (maybe bug from old Plume)This is a bug from old PlumeReceiving
all right
Federation with itself
Unsubscribing from a user does not seem to delete the entry in databaseSending/Receiving Post Update does not seems to update post cover-picturepleroma
Sending activities
Receiving
wow I missed how ugly this thing was
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
@fdb-hiroshima The issues with Pleroma are normally fixed now.
🚀
Yay, Finally! Thank you @igalic and @fdb-hiroshima for reviewing this big PR! 😊