Store activitypub endpoints in database #162
Merged
dessalines
merged 2 commits from apub-endpoints-in-db into main 2 years ago
No Label
No Milestone
No Assignees
2 Participants
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: LemmyNet/lemmy#162
Loading…
Reference in new issue
There is no content yet.
Delete Branch 'apub-endpoints-in-db'
Deleting a branch is permanent. It CANNOT be undone. Continue?
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.
8e8b3951e5tobecda4162c2 years agobecda4162ctoe421e31c792 years agoWIP: Store activitypub endpoints in databaseto Store activitypub endpoints in database 2 years agoLooks 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)?)),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
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).
Ahh... okay that's fine.
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)?)),hrm... this missing followers_url?
We only have the
/c/main/followersendpoint 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 could add it later, but then we'll have to do yet another advanced_code migration when we could just handle that here.
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)?)),These should probably all be generated as a tuple or struct output, since they're all based on the
actor_idanyway. IE(followers_url, inbox_url, shared_inbox_url) = generate_bleh_urls(&actor_id)orinboxes: Inboxes = generate...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.
Okay that's fine.
}fn get_outbox_url(&self) -> Result<Url, ParseError> {assert!(self.local);This is dangerous right? assert will panic, which we shouldn't do. Maybe use a LemmyError and have this throw one of those.
Fixed.
fn get_outbox_url(&self) -> Result<Url, ParseError> {assert!(self.local);Url::parse(&format!("{}/outbox", &self.actor_id()))}These two functions, seems like its possible to make a trait with a default impl, since these are exactly the same.
That would be good, problem is that it doesnt get access to the
User.localandCommunity.localvariables in that case. I will add a method ActorType::is_local()`, that should be a bit cleaner.}/// 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>;Yeah these ones, check to see if you can do a default here.
How do you mean?
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
nm I see you got it.
))?.into(),)}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.
banner -> Nullable<Text>,followers_url -> Text,inbox_url -> Text,shared_inbox_url -> Nullable<Text>,Both of the shared inboxes should be required fields.
https://www.w3.org/TR/activitypub/#shared-inbox-delivery
mmmk.
banner -> Nullable<Text>,deleted -> Bool,inbox_url -> Text,shared_inbox_url -> Nullable<Text>,Should also have a followers_url, since we will eventually add user following.
Then we can add the
followers_urllater, I dont see any point in adding it now if we dont use it.pub banner: Option<String>,pub followers_url: Url,pub inbox_url: Url,pub shared_inbox_url: Option<Url>,^
pub banner: Option<Option<String>>,pub followers_url: Option<Url>,pub inbox_url: Option<Url>,pub shared_inbox_url: Option<Option<Url>>,^
table! {Remove this file, it gets generated by running diesel, but we don't use it.
Done.
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();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.Fixed.
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;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.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;same as for community, and also add a followers_url since we will eventually have user following.
diesel::update(user_.find(u.id)).set((inbox_url.eq(inbox_url_),shared_inbox_url.eq(shared_inbox_url_),followers here as well
Put my changes into a new commit, please squash when you merge it.
1a4e35eb50into main 2 years agoReviewers
1a4e35eb50.