Store activitypub endpoints in database #162

Merged
dessalines merged 2 commits from apub-endpoints-in-db into main 2 years 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 2 years ago
nutomic force-pushed apub-endpoints-in-db from 8e8b3951e5 to becda4162c 2 years ago
nutomic force-pushed apub-endpoints-in-db from becda4162c to e421e31c79 2 years ago
nutomic changed title from WIP: Store activitypub endpoints in database to Store activitypub endpoints in database 2 years ago
dessalines requested changes 2 years ago
dessalines left a comment
Owner

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.

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)?)),
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).
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)?)),
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.
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)?)),
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.
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);
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()))
}
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>;
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?
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
Owner

nm I see you got it.

nm I see you got it.
dessalines marked this conversation as resolved
))?
.into(),
)
}
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>,
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
Owner

mmmk.

mmmk.
dessalines marked this conversation as resolved
banner -> Nullable<Text>,
deleted -> Bool,
inbox_url -> Text,
shared_inbox_url -> Nullable<Text>,
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>,
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>>,
Owner

^

^
dessalines marked this conversation as resolved
table! {
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();
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;
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;
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_),
Owner

followers here as well

followers here as well
dessalines marked this conversation as resolved
nutomic added 1 commit 2 years 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 2 years ago

Reviewers

dessalines requested changes 2 years 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

No dependencies set.

Reference: LemmyNet/lemmy#162
Loading…
There is no content yet.