Conversation
|
|
||
| let mut ordered_items = vec![]; | ||
| for post_view in post_views { | ||
| let post_ap_id = post_view.post.ap_id.clone(); |
There was a problem hiding this comment.
This is the only place where I am passing in the object_id. I think it makes the ids for Announce and Create stable. Not sure where else I could use it.
There was a problem hiding this comment.
Also needs to be in crates/apub/activities/src/create_or_update/post.rs so both code paths generate the same id.
|
|
||
| let create_or_update = | ||
| CreateOrUpdatePage::new(post.into(), &person, &community, kind, &context).await?; | ||
| CreateOrUpdatePage::new(post.into(), &person, &community, kind, None, &context).await?; |
There was a problem hiding this comment.
| CreateOrUpdatePage::new(post.into(), &person, &community, kind, None, &context).await?; | |
| CreateOrUpdatePage::new(post.into(), &person, &community, kind, Some(post.ap_id), &context).await?; |
There was a problem hiding this comment.
Actually this can be a problem when editing the post, then each Update activity will have the same id. So you also need to hash the timestamp (published_at or updated_at).
| .into(); | ||
|
|
||
| let id = generate_activity_id(kind.clone(), &context)?; | ||
| let id = generate_activity_id(kind.clone(), None, &context)?; |
There was a problem hiding this comment.
| let id = generate_activity_id(kind.clone(), None, &context)?; | |
| let id = generate_activity_id(kind.clone(), Some(comment.ap_id), &context)?; |
There was a problem hiding this comment.
Do we need to use the timestamps here for Update as well?
| /// Generate a unique ID for an activity, in the format: | ||
| /// `http(s)://example.com/receive/create/202daf0a-1489-45df-8d2e-c8a3173fed36` | ||
| fn generate_activity_id<T>(kind: T, context: &LemmyContext) -> Result<Url, ParseError> | ||
| fn generate_activity_id<T>( |
There was a problem hiding this comment.
To avoid passing None in so many places, you could do:
fn generate_activity_id_with_object_id(kind, context) {
generate_activity_id(kind, None, context)
}
fn generate_activity_id(kind, object_id, context) {
generate_activity_id(kind, context)
}|
What is the minimum amount of info needed to create uniqueness for these generated ap_ids? Seems like |
|
That is not enough, because if you edit a post, it is federated as |
18c6db2 to
91f043f
Compare
|
Bump on this to address the comments above. As stated you'll probably need to hash the object id as well as the published / updated time. |
|
Hi, should we use timestamps for Update comments as well? Or it's not needed since I don't see any comment-related actions when making a request to the outbox of a community? |
| ) -> LemmyResult<()> { | ||
| let announce = AnnounceActivity::new(object.clone(), community, context)?; | ||
| let announce = AnnounceActivity::new(object.clone(), community, None, context)?; | ||
| let inboxes = ActivitySendTargets::to_local_community_followers(community.id); |
There was a problem hiding this comment.
Are we sure we want the activity id for Announce activities to be the hash in the outbox, and a random UUID in the sent_activity table? For Create activities, both are the same.
There was a problem hiding this comment.
The same activity must always have the same ID.
| .get(..16) // should not fail | ||
| .ok_or(UntranslatedError::CouldntGenerateHash)? | ||
| .try_into()?, | ||
| )) |
There was a problem hiding this comment.
You dont really need a Uuid but can return digest.to_string() and probably get rid of this new error. Also move this code inline as the method is only called in a single place.
| }; | ||
|
|
||
| let id = format!("{}/activities/{}/{}", hostname, kind_str, uuid); | ||
| Url::parse(&id).map_err(|e| LemmyError::from(anyhow::anyhow!(e))) |
There was a problem hiding this comment.
| Url::parse(&id).map_err(|e| LemmyError::from(anyhow::anyhow!(e))) | |
| Ok(Url::parse(&id)?) |
| seed_url.set_fragment(Some(×tamp.to_rfc3339())); | ||
| Some(&seed_url.clone()) | ||
| } | ||
| }; |
There was a problem hiding this comment.
Private message also needs the same logic. Best generate_activity_id() to take an optional timestamp param (or make a separate method), to avoid duplicate code. Theres nothing wrong with hashing the timestamp for create activities as well so it will be simpler.
And you dont need to change anything in this file, it only needs to be in CreateOrUpdatePage::new
| let timestamp = comment.updated_at.unwrap_or(comment.published_at); // use the latest timestamp | ||
| let mut seed_url = ap_id; | ||
| seed_url.set_fragment(Some(×tamp.to_rfc3339())); |
There was a problem hiding this comment.
Doesn't need to be in this PR, but these things should be trait functions.
Actors have a ApubActor trait, which defines functions like generate_local_actor_url, read_from_name, etc.
There's no reason why that couldn't also be done for the other types, and activities on them also.
For issue #6341.
Add hashing for activity ids so that the ids of the activity objects do not change on every request.