Store activitypub endpoints in database #162

Merged
dessalines merged 2 commits from apub-endpoints-in-db into main 12 months ago
Owner

Should be good to merge. But have a good look at the migrations and database changes, cause I dont really know what i'm doing there.

Should be good to merge. But have a good look at the migrations and database changes, cause I dont really know what i'm doing there.
nutomic added 1 commit 12 months ago
nutomic force-pushed apub-endpoints-in-db from 8e8b3951e5 to becda4162c 12 months ago
nutomic force-pushed apub-endpoints-in-db from becda4162c to e421e31c79 12 months ago
nutomic changed title from WIP: Store activitypub endpoints in database to Store activitypub endpoints in database 12 months ago
dessalines requested changes 12 months ago
dessalines left a comment

Looks good! Good job on both the DB and advanced_migrations, just a few suggestions. Read through all my comments over once before making changes.

published: None,
followers_url: Some(generate_followers_url(&community_actor_id)?),
inbox_url: Some(generate_inbox_url(&community_actor_id)?),
shared_inbox_url: Some(Some(generate_shared_inbox_url(&community_actor_id)?)),
Poster
Owner

Why Some(Some( for this one but not the others? I think since this is a required column, some(some( doesn't make sense. IE you can never use some(none

Why Some(Some( for this one but not the others? I think since this is a required column, some(some( doesn't make sense. IE you can never use some(none
Poster
Owner

shared_inbox is optional in activitypub, so i also made it optional in our code. So this new code should work fine with software that doesnt have shared_inbox (though I didnt test that).

shared_inbox is optional in activitypub, so i also made it optional in our code. So this new code should work fine with software that doesnt have shared_inbox (though I didnt test that).
Poster
Owner

Ahh... okay that's fine.

Ahh... okay that's fine.
dessalines marked this conversation as resolved
public_key: Some(user_keypair.public_key),
last_refreshed_at: None,
inbox_url: Some(generate_inbox_url(&user_actor_id)?),
shared_inbox_url: Some(Some(generate_shared_inbox_url(&user_actor_id)?)),
Poster
Owner

hrm... this missing followers_url?

hrm... this missing followers_url?
Poster
Owner

We only have the /c/main/followers endpoint for communities but not for users, because we dont support user following (yet). So I dont see any reason to add it at this time.

We only have the `/c/main/followers` endpoint for communities but not for users, because we dont support user following (yet). So I dont see any reason to add it at this time.
Poster
Owner

We could add it later, but then we'll have to do yet another advanced_code migration when we could just handle that here.

We could add it later, but then we'll have to do yet another advanced_code migration when we could just handle that here.
dessalines marked this conversation as resolved
banner: None,
followers_url: Some(generate_followers_url(&actor_id)?),
inbox_url: Some(generate_inbox_url(&actor_id)?),
shared_inbox_url: Some(Some(generate_shared_inbox_url(&actor_id)?)),
Poster
Owner

These should probably all be generated as a tuple or struct output, since they're all based on the actor_id anyway. IE (followers_url, inbox_url, shared_inbox_url) = generate_bleh_urls(&actor_id) or inboxes: Inboxes = generate...

These should probably all be generated as a tuple or struct output, since they're all based on the `actor_id` anyway. IE `(followers_url, inbox_url, shared_inbox_url) = generate_bleh_urls(&actor_id)` or `inboxes: Inboxes = generate...`
Poster
Owner

I dont think that would be better. Right now the code is neatly split into separate functions, and its very clear which one does what. If we put it into a single method, thats gonna be a rather long one. And tuples suck as return values, especially when all items have the same type. It would be very confusing to figure out which value is where. Plus in some places (like advanced migrations) we dont need to generate the actor_id.

I dont think that would be better. Right now the code is neatly split into separate functions, and its very clear which one does what. If we put it into a single method, thats gonna be a rather long one. And tuples suck as return values, especially when all items have the same type. It would be very confusing to figure out which value is where. Plus in some places (like advanced migrations) we dont need to generate the actor_id.
Poster
Owner

Okay that's fine.

Okay that's fine.
dessalines marked this conversation as resolved
}
fn get_outbox_url(&self) -> Result<Url, ParseError> {
assert!(self.local);
Poster
Owner

This is dangerous right? assert will panic, which we shouldn't do. Maybe use a LemmyError and have this throw one of those.

This is dangerous right? assert will panic, which we shouldn't do. Maybe use a LemmyError and have this throw one of those.
Poster
Owner

Fixed.

Fixed.
dessalines marked this conversation as resolved
fn get_outbox_url(&self) -> Result<Url, ParseError> {
assert!(self.local);
Url::parse(&format!("{}/outbox", &self.actor_id()))
}
Poster
Owner

These two functions, seems like its possible to make a trait with a default impl, since these are exactly the same.

These two functions, seems like its possible to make a trait with a default impl, since these are exactly the same.
Poster
Owner

That would be good, problem is that it doesnt get access to the User.local and Community.local variables in that case. I will add a method ActorType::is_local()`, that should be a bit cleaner.

That would be good, problem is that it doesnt get access to the `User.local` and `Community.local` variables in that case. I will add a method ActorType::is_local()`, that should be a bit cleaner.
dessalines marked this conversation as resolved
}
/// Outbox URL is not generally used by Lemmy, so it can be generated on the fly (but only for
/// local actors).
fn get_outbox_url(&self) -> Result<Url, ParseError>;
Poster
Owner

Yeah these ones, check to see if you can do a default here.

Yeah these ones, check to see if you can do a default here.
Poster
Owner

How do you mean?

How do you mean?
Poster
Owner

You should be able to do a default function on the trait, so you don't have to do impls twice if its doing the same thing: https://doc.rust-lang.org/book/ch10-02-traits.html#default-implementations

You should be able to do a default function on the trait, so you don't have to do impls twice if its doing the same thing: https://doc.rust-lang.org/book/ch10-02-traits.html#default-implementations
Poster
Owner

nm I see you got it.

nm I see you got it.
dessalines marked this conversation as resolved
))?
.into(),
)
}
Poster
Owner

This function here could generate a tuple rather than splitting them out. I don't think you have to use every value in the tuple.

This function here could generate a tuple rather than splitting them out. I don't think you have to use every value in the tuple.
dessalines marked this conversation as resolved
banner -> Nullable<Text>,
followers_url -> Text,
inbox_url -> Text,
shared_inbox_url -> Nullable<Text>,
Poster
Owner

Both of the shared inboxes should be required fields.

Both of the shared inboxes should be required fields.
Poster
Owner

Origin servers sending publicly addressed activities to sharedInbox endpoints MUST still deliver to actors and collections otherwise addressed (through to, bto, cc, bcc, and audience) which do not have a sharedInbox and would not otherwise receive the activity through the sharedInbox mechanism.

https://www.w3.org/TR/activitypub/#shared-inbox-delivery

>Origin servers sending publicly addressed activities to sharedInbox endpoints MUST still deliver to actors and collections otherwise addressed (through to, bto, cc, bcc, and audience) which do not have a sharedInbox and would not otherwise receive the activity through the sharedInbox mechanism. https://www.w3.org/TR/activitypub/#shared-inbox-delivery
Poster
Owner

mmmk.

mmmk.
dessalines marked this conversation as resolved
banner -> Nullable<Text>,
deleted -> Bool,
inbox_url -> Text,
shared_inbox_url -> Nullable<Text>,
Poster
Owner

Should also have a followers_url, since we will eventually add user following.

Should also have a followers_url, since we will eventually add user following.
Poster
Owner

Then we can add the followers_url later, I dont see any point in adding it now if we dont use it.

Then we can add the `followers_url` later, I dont see any point in adding it now if we dont use it.
dessalines marked this conversation as resolved
pub banner: Option<String>,
pub followers_url: Url,
pub inbox_url: Url,
pub shared_inbox_url: Option<Url>,
Poster
Owner

^

^
dessalines marked this conversation as resolved
pub banner: Option<Option<String>>,
pub followers_url: Option<Url>,
pub inbox_url: Option<Url>,
pub shared_inbox_url: Option<Option<Url>>,
Poster
Owner

^

^
dessalines marked this conversation as resolved
table! {
Poster
Owner

Remove this file, it gets generated by running diesel, but we don't use it.

Remove this file, it gets generated by running diesel, but we don't use it.
Poster
Owner

Done.

Done.
dessalines marked this conversation as resolved
ALTER TABLE community ADD COLUMN followers_url TEXT NOT NULL DEFAULT generate_unique_changeme();
ALTER TABLE community ADD COLUMN inbox_url TEXT NOT NULL DEFAULT generate_unique_changeme();
Poster
Owner

I wouldn't do TEXT here, have it match the actor ID column, I think a varchar(255) . If we find we need extremely long domains we can change these all later at the same time.

I wouldn't do TEXT here, have it match the actor ID column, I think a `varchar(255)` . If we find we need extremely long domains we can change these all later at the same time.
Poster
Owner

Fixed.

Fixed.
dessalines marked this conversation as resolved
ALTER TABLE community ADD COLUMN followers_url TEXT NOT NULL DEFAULT generate_unique_changeme();
ALTER TABLE community ADD COLUMN inbox_url TEXT NOT NULL DEFAULT generate_unique_changeme();
ALTER TABLE community ADD COLUMN shared_inbox_url TEXT;
Poster
Owner

Have this one also not null defaults like above, its required as well. Since its not unique, you don't have to have the generate_changeme() if you don't want to.

Have this one also not null defaults like above, its required as well. Since its not unique, you don't have to have the `generate_changeme()` if you don't want to.
dessalines marked this conversation as resolved
ALTER TABLE community ADD COLUMN shared_inbox_url TEXT;
ALTER TABLE user_ ADD COLUMN inbox_url TEXT NOT NULL DEFAULT generate_unique_changeme();
ALTER TABLE user_ ADD COLUMN shared_inbox_url TEXT;
Poster
Owner

same as for community, and also add a followers_url since we will eventually have user following.

same as for community, and also add a followers_url since we will eventually have user following.
dessalines marked this conversation as resolved
diesel::update(user_.find(u.id))
.set((
inbox_url.eq(inbox_url_),
shared_inbox_url.eq(shared_inbox_url_),
Poster
Owner

followers here as well

followers here as well
dessalines marked this conversation as resolved
nutomic added 1 commit 12 months ago
Poster
Owner

Put my changes into a new commit, please squash when you merge it.

Put my changes into a new commit, please squash when you merge it.
dessalines merged commit 1a4e35eb50 into main 12 months ago

Reviewers

dessalines requested changes 12 months ago
The pull request has been merged as 1a4e35eb50.
Sign in to join this conversation.
No reviewers
No Label
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.