Big refactoring of the Inbox
#443
Sapludināts
elegaanz
sapludināja 37 revīzijas no inbox-refactor
uz master
pirms 5 gadiem
Recenzenti
Pieprasīt recenziju
Nav recenzentu
Etiķetes
Noņemt etiķetes
Related to the REST API
Code running on the server
Stuff related to Federation
Related to the front-end
Translations, and related code
More about project management or code than the project itself
The building, or installation process of Plume
Something isn't working
We need to talk
New feature or request
This is a new feature
Compatibility with different browsers, readers and OS
Related to an external package that Plume uses
UI/UX related issues and PRs
Good for newcomers
Extra attention is needed
Issues affecting only mobile UX
How elements're rendered out for the end user
Something else needs to be fixed first
This issue or pull request already exists
This PR is not complete yet
Issues concern a limited number of instances
This doesn't seem right
Need to be discussed by the community (on Loomio)
This PR is ready to be reviewed
Proposed ideas worth considering
This is issue has been created after a vote on Loomio
This will not be worked on
Apstiprināt etiķetes
A: API
Related to the REST API
A: Backend
Code running on the server
A: Federation
Stuff related to Federation
A: Front-End
Related to the front-end
A: I18N
Translations, and related code
A: Meta
More about project management or code than the project itself
A: Security
Build
The building, or installation process of Plume
C: Bug
Something isn't working
C: Discussion
We need to talk
C: Enhancement
New feature or request
C: Feature
This is a new feature
Compatibility
Compatibility with different browsers, readers and OS
Dependency
Related to an external package that Plume uses
Design
UI/UX related issues and PRs
Documentation
Good first issue
Good for newcomers
Help welcome
Extra attention is needed
Mobile
Issues affecting only mobile UX
Rendering
How elements're rendered out for the end user
S: Blocked
Something else needs to be fixed first
S: Duplicate
This issue or pull request already exists
S: Incomplete
This PR is not complete yet
S: Instance specific
Issues concern a limited number of instances
S: Invalid
This doesn't seem right
S: Needs Voting/Discussion
Need to be discussed by the community (on Loomio)
S: Ready for review
This PR is ready to be reviewed
Suggestion
Proposed ideas worth considering
S: Voted on Loomio
This is issue has been created after a vote on Loomio
S: Wontfix
This will not be worked on
Nav etiķešu
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
Atskaites punkts
Uzstādīt atskaites punktu
Notīrīt atskaites punktus
Nav neviena ieraksta
Nav atskaites punktu
Atbildīgie
Piešķirt lietotājus
Noņemt atbildīgo
Nav atbildīgo
2 dalībnieki
Paziņojumi
Izpildes termiņš
Datums līdz nav korekts. Izmantojiet formātu 'gggg-mm-dd'.
Izpildes termiņš nav uzstādīts.
Atkarības
Nav atkarību.
Atsaucas uz: Plume/Plume#443
Atsaukties uz šo jaunā problēmā
Vēl nav satura.
Dzēst atzaru 'inbox-refactor'
Atzara dzēšana ir neatgriezeniska, kā arī tā ir NEATGRIEZENISKA. Vai turpināt?
Nē
Jā
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:
AsActor
andAsObject
) 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
use
statement 🤔@ -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
where
clause here mean? I've never seen+ Debug
@_@given that this
match
only has one match and else,other
, maybe useif let
here.@ -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_id
This mean that E must Implement
From<InboxError<E>>
andDebug
(Debug
allow 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
Context
we 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: mastodon
andservice: pleroma
to 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 instances
long_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 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)),
},
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_create
of 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_then
be 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
url
field 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
Receiving
all right
Federation with itself
pleroma
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! 😊
Recenzenti
12efe721cc
.Solis 1:
Projekta repozitorijā izveidojiet jaunu jaunu atzaru un pārbaudiet savas izmaiņas.Solis 2:
Sapludināt izmaiņas un atjaunot tās Forgejo.