Let community announce posts #36

Manually merged
nutomic merged 1 commits from announce-posts into federation 2020-06-03 21:56:57 +00:00
Owner

Not finished yet, and the tests are failing.

Not finished yet, and the tests are failing.
nutomic reviewed 2020-05-07 17:09:22 +00:00
@ -291,3 +281,1 @@
"".to_string()
}
})
.map(|c| get_shared_inbox(&c.user_actor_id))
Author
Owner

Honestly I have no clue what your code is doing, but I dont think its needed.

Honestly I have no clue what your code is doing, but I dont think its needed.
nutomic changed title from Let community announce posts to WIP: Let community announce posts 2020-05-14 17:01:39 +00:00
Author
Owner

Need to change this so that comments, votes and undos are federated (probably by having the community announce all of them?).

Need to change this so that comments, votes and undos are federated (probably by having the community announce all of them?).
Owner

Yeah, everything in the receive needs to be "forwarded" / announced to the other correct servers.

To test this correctly, we should also make sure the federation-test adds a third server.

Yeah, everything in the receive needs to be "forwarded" / announced to the other correct servers. To test this correctly, we should also make sure the federation-test adds a third server.
nutomic reviewed 2020-05-28 18:10:14 +00:00
@ -125,3 +125,3 @@
for cpost in &incorrect_posts {
Post::update_ap_id(&conn, cpost.id)?;
Post::update_ap_id(&conn, cpost.id, todo!())?;
Author
Owner

Dont forget to implement this properly.

Dont forget to implement this properly.
Author
Owner

I'm not sure where exactly MentionsAndAddresses::inboxes was used before, but I'm getting warnings now that its unused. Any advice how to integrate it again?

Edit: I think I got it, will add another commit.

Edit 2: Done, in the remote case comment notifications will be handled just like before, but I'm not sure if the local case is handled correctly.

I'm not sure where exactly `MentionsAndAddresses::inboxes` was used before, but I'm getting warnings now that its unused. Any advice how to integrate it again? Edit: I think I got it, will add another commit. Edit 2: Done, in the remote case comment notifications will be handled just like before, but I'm not sure if the local case is handled correctly.
nutomic changed title from WIP: Let community announce posts to Let community announce posts 2020-05-30 17:49:17 +00:00
nutomic reviewed 2020-05-30 17:50:36 +00:00
@ -71,1 +73,3 @@
Ok(Activity::create(&conn, &activity_form)?)
debug!("inserting activity for user {}, data {:?}", user_id, data);
// TODO: this is broken
//Activity::create(&conn, &activity_form)?;
Author
Owner

As I wrote in the chat, this is breaking with errors like Key (user_id)=(-1) is not present in table "user_". We should probably add foreign keys user_id and community_id and require one of them to be correct. Or instead a column like actor_id?

As I wrote in the chat, this is breaking with errors like `Key (user_id)=(-1) is not present in table "user_"`. We should probably add foreign keys user_id and community_id and require one of them to be correct. Or instead a column like `actor_id`?
Owner

I'll read through again, the user_id shouldn't ever be -1, but should be the local DB's id for that user.

I'll read through again, the user_id shouldn't ever be -1, but should be the local DB's id for that user.
Author
Owner

That doesnt work if the action is taken by the community (eg announce).

That doesnt work if the action is taken by the community (eg announce).
Owner

One level inside announce tho, the activity will have the creator / user that did the action.

One level inside announce tho, the activity will have the creator / user that did the action.
Author
Owner

Okay I went with community.creator_id, which is the same thing we use for the accept follow activity. Still seems wrong.

Okay I went with community.creator_id, which is the same thing we use for the accept follow activity. Still seems wrong.
Owner

Oh also, the federation tests did pass, but we really didn't test out 3 instance thing which this really requires. I can write those tests tho, probably will be able to get to it tmrw.

Oh also, the federation tests did pass, but we really didn't test out 3 instance thing which this really requires. I can write those tests tho, probably will be able to get to it tmrw.
dessalines reviewed 2020-05-31 23:06:10 +00:00
@ -483,0 +486,4 @@
send_activity(&activity, creator, to)?;
}
Ok(())
}
Owner

This is the same as what's in the impl Post, and since it doesn't use self, you could move this up to mod.rs

This is the same as what's in the `impl Post`, and since it doesn't use `self`, you could move this up to `mod.rs`
Author
Owner

Right, good catch. I moved it to activities::send_activity_to_community().

Right, good catch. I moved it to activities::send_activity_to_community().
dessalines reviewed 2020-05-31 23:08:32 +00:00
@ -393,0 +392,4 @@
where
A: Activity + Base + Serialize + Debug,
{
insert_activity(&conn, -1, &activity, is_local_activity)?;
Owner

There's a user_id that's doing the activity, so that needs to get passed through. Instead of sender: &str, use ``creator: &User_`, then you'll have both the actor_id, and the user_id.

There's a user_id that's doing the activity, so that needs to get passed through. Instead of `sender: &str`, use ``creator: &User_`, then you'll have both the actor_id, and the user_id.
Owner

Also just to add, part of my thinking in making this a necessary column in the activity table, is to ensure that federated users also have local ids.

Also just to add, part of my thinking in making this a necessary column in the activity table, is to ensure that federated users also have local ids.
dessalines reviewed 2020-05-31 23:11:51 +00:00
dessalines reviewed 2020-05-31 23:13:30 +00:00
@ -393,0 +410,4 @@
// dont send to the instance where the activity originally came from, because that would result
// in a database error (same data inserted twice)
let mut to = community.get_follower_inboxes(&conn)?;
let sending_user = get_or_fetch_and_upsert_remote_user(&sender, conn)?;
Owner

Now you won't have to do this, because you already have the sending user, which already implements ActorType and you can get the shared_inbox that way.

Now you won't have to do this, because you already have the sending user, which already implements `ActorType` and you can get the shared_inbox that way.
Author
Owner

Right, done. There is a similar issue in shared_inbox but that is trickier to fix (I left a todo).

Right, done. There is a similar issue in shared_inbox but that is trickier to fix (I left a todo).
dessalines reviewed 2020-05-31 23:15:54 +00:00
@ -393,0 +405,4 @@
.set_actor_xsd_any_uri(community.actor_id.to_owned())?
.set_object_base_box(BaseBox::from_concrete(activity)?)?;
insert_activity(&conn, -1, &announce, true)?;
Owner

Thinking about it, this is correct, it does make sense to insert both the activity, and the announce.

Thinking about it, this is correct, it does make sense to insert both the activity, and the announce.
dessalines reviewed 2020-05-31 23:17:07 +00:00
@ -56,0 +57,4 @@
return Err(format_err!(
"Received activity is addressed to remote community {}",
&community.actor_id
));
Owner

Good call, I didn't think about this.

Good call, I didn't think about this.
dessalines reviewed 2020-05-31 23:31:33 +00:00
@ -68,0 +99,4 @@
.unwrap()
.next()
.unwrap()
.to_string()
Owner

I have to think about this one. The first item of the array is community/followers, but the other ones might be mentions and such.

I'm pretty sure in my code I just scan the comment text again anyway to handle mentions, but it would be good to make sure this returns the full array.

I have to think about this one. The first item of the array is `community/followers`, but the other ones might be mentions and such. I'm pretty sure in my code I just scan the comment text again anyway to handle mentions, but it would be good to make sure this returns the full array.
dessalines reviewed 2020-05-31 23:32:17 +00:00
@ -68,0 +86,4 @@
// TODO: there is probably an easier way to do this
let oprops = match self {
SharedAcceptedObjects::Create(c) => &c.object_props,
SharedAcceptedObjects::Update(u) => &u.object_props,
Owner

There's gotta be a cleaner way to do all of these, I'll play around a bit.

There's gotta be a cleaner way to do all of these, I'll play around a bit.
Author
Owner

I hope the new apub library will make it unnecessary.

I hope the new apub library will make it unnecessary.
dessalines reviewed 2020-05-31 23:35:47 +00:00
@ -85,0 +128,4 @@
let c = get_or_fetch_and_upsert_remote_community(&sender.to_string(), &conn)?;
verify(&request, &c)
}
}?;
Owner

I can't think of a good cross-compatible way to do this either. We can't use urls because other future group-implementers will probably not use /c/...

I can't think of a good cross-compatible way to do this either. We can't use urls because other future group-implementers will probably not use `/c/`...
Author
Owner

We could move the check into the match because announce is the only activity that comes from a community. But then we probably have to duplicate the verify a lot more times. Lets see.

We could move the check into the match because announce is the only activity that comes from a community. But then we probably have to duplicate the verify a lot more times. Lets see.
dessalines reviewed 2020-05-31 23:38:53 +00:00
@ -137,0 +220,4 @@
if !community.local {
// ignore this object
}
Community::do_announce(activity, &community, sender, conn, false)
Owner

Shouldn't this go in if community.local {

Shouldn't this go in `if community.local {`
Author
Owner

Right, I think I forgot to finish this.

Right, I think I forgot to finish this.
dessalines reviewed 2020-05-31 23:45:06 +00:00
@ -137,0 +233,4 @@
.get_object_base_box()
.unwrap()
.to_owned();
// TODO: too much copy paste
Owner

I'll have to play around and see if there's a cleaner way to do it too, but I doubt there is.

I'll have to play around and see if there's a cleaner way to do it too, but I doubt there is.
Author
Owner

I guess we can leave the refactoring until after we switch to the new library version.

I guess we can leave the refactoring until after we switch to the new library version.
Owner

Good call.

Good call.
nutomic closed this pull request 2020-06-03 21:56:57 +00:00
Sign in to join this conversation.
No reviewers
No Label
No Milestone
No Assignees
2 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: LemmyNet/lemmy#36
No description provided.