-
-
Notifications
You must be signed in to change notification settings - Fork 916
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Wrap attributedTo for group to fix Peertube federation #5509
Wrap attributedTo for group to fix Peertube federation #5509
Conversation
Unfortunately it still cant fetch the channel. You can try with
The error is |
crates/apub/src/objects/post.rs
Outdated
if !(page.kind == PageType::Video) { | ||
to_local_url(url.as_str(), context).await.or(Some(url)) | ||
} else { | ||
Some(url) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is needed to prevent a stack overflow from happening, as this will endlessly recurse otherwise. But now you can resolve a PeerTube video without issue.
curl http://localhost:8536/api/v4/resolve_object?q=https://tilvids.com/videos/watch/251f4ba5-2bb6-4b26-be3b-d7e7c0a93084
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, to_local_url
really needs some kind of stack to keep track of what's already being resolved as it's trivial to cause a stack overflow with it. E.g:
curl http://localhost:8536/api/v4/resolve_object?q=https://lemmy.dbzer0.com/u/Blaze
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True, opened #5536 for that.
|
||
pub fn truncate_description(string: String) -> String { | ||
truncate_for_db(string, SITE_DESCRIPTION_MAX_LENGTH) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if there is any way to get the max_length
attributes from schema.rs, then we dont have to define them twice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pry not worth it anyway.
.filter(|p| p.kind == PersonOrGroupType::Person) | ||
.map(|p| ObjectId::<ApubPerson>::from(p.id.clone().into_inner())) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's also a filter_map
function that can combine these 2 steps:
.filter(|p| p.kind == PersonOrGroupType::Person) | |
.map(|p| ObjectId::<ApubPerson>::from(p.id.clone().into_inner())) | |
.filter_map(|p| (p.kind == PersonOrGroupType::Person) | |
.then(|| ObjectId::<ApubPerson>::from(p.id.clone().into_inner())) | |
) |
If the type that p.id
implements Copy
, you can also omit the call to clone
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Clippy changes this back to a filter
and map
. https://rust-lang.github.io/rust-clippy/master/index.html#filter_map_bool_then
match self { | ||
AttributedTo::Lemmy(l) => Some(l.moderators().into()), | ||
AttributedTo::Peertube(_) => None, | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick: could be:
match self { | |
AttributedTo::Lemmy(l) => Some(l.moderators().into()), | |
AttributedTo::Peertube(_) => None, | |
} | |
if let AttributedTo::Lemmy(l) = self { | |
Some(l.moderators().into()) | |
} else { | |
None | |
} |
I'll leave it to the other reviewers to decide which style is preferred.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Match has less lines which makes it easier to read, plus we might add more variants in the future. So leave it as is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
Peertube uses
attributedTo
onGroup
s differently to Lemmy, which has the knock-on effect of making Lemmy reject videos from Peertube (more details). To get around this, we need to wrapattributedTo
forGroup
like we do forPage
.