Skip to content
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

Compliance with ActivityPub standard #1220

Closed
13 of 15 tasks
Nutomic opened this issue Oct 22, 2020 · 14 comments
Closed
13 of 15 tasks

Compliance with ActivityPub standard #1220

Nutomic opened this issue Oct 22, 2020 · 14 comments
Labels
area: federation support federation via activitypub enhancement New feature or request

Comments

@Nutomic
Copy link
Member

Nutomic commented Oct 22, 2020

These should be the most important items, but I left out some of the more complicated requirements.

@Nutomic Nutomic added enhancement New feature or request area: federation support federation via activitypub labels Oct 22, 2020
Nutomic added a commit that referenced this issue Oct 23, 2020
Nutomic added a commit that referenced this issue Oct 23, 2020
Nutomic added a commit that referenced this issue Oct 26, 2020
Nutomic added a commit that referenced this issue Nov 9, 2020
more refactoring with tons of changes:

- inbox functions return LemmyError instead of HttpResponse
- announce is done directly in community inbox
- reorganized functions for handling inbox activities
- additional checks for private messages
- moved inbox handler functions for post, comment, vote into separete file
- ensure that posts, comments etc are addressed to public (ref #1220)
- probably more
dessalines pushed a commit that referenced this issue Nov 25, 2020
Nutomic pushed a commit that referenced this issue Dec 17, 2020
@Nutomic Nutomic pinned this issue Jan 5, 2021
@Nutomic
Copy link
Member Author

Nutomic commented Jan 18, 2021

I'm pretty much out of ideas on the status codes (most importantly, returning http 404 for diesel::result::Error::NotFound). Here is the best solution I could come up with:

#[derive(Debug)]
pub struct LemmyError {
  pub inner: anyhow::Error,
  pub status_code: Option<StatusCode>
}

impl<T> From<T> for LemmyError
where
  T: Into<anyhow::Error>,
{
  default fn from(t: T) -> Self {
    LemmyError { inner: t.into(), status_code: None }
  }
}

impl From<diesel::result::Error> for LemmyError {
  fn from(e: diesel::result::Error) -> Self {
    let status_code = match e {
      diesel::result::Error::NotFound => Some(StatusCode::NOT_FOUND),
      _ => None
    };
    LemmyError { inner: e.into(), status_code }
  }
}

impl actix_web::error::ResponseError for LemmyError {
  fn status_code(&self) -> StatusCode {
    self.status_code.unwrap_or(StatusCode::INTERNAL_SERVER_ERROR)
  }
}

But there are two problems with this code:

  • It requires #![feature(min_specialization)], and for that reason only works on nightly
  • It means that utils depends on diesel, which can slow down the compilation

@dessalines
Copy link
Member

I like this solution... why does it say that feature is necessary?

@Nutomic
Copy link
Member Author

Nutomic commented Jan 19, 2021

Without that feature, the compilation fails with this error:

error[E0119]: conflicting implementations of trait `std::convert::From<diesel::result::Error>` for type `LemmyError`:
--> lemmy_utils/src/lib.rs:72:1
|
57 | / impl<T> From<T> for LemmyError
58 | | where
59 | |   T: Into<anyhow::Error>,
60 | | {
...  |
63 | |   }
64 | | }
| |_- first implementation here
...
72 |   impl From<diesel::result::Error> for LemmyError {
|   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ conflicting implementation for `LemmyError`

error: aborting due to previous error

For more information about this error, try `rustc --explain E0119`.
error: could not compile `lemmy_utils`

@Nutomic
Copy link
Member Author

Nutomic commented Feb 5, 2021

This issue should be finished, as far as I can tell we are now fully compatible with ActivityPub.

@dessalines
Copy link
Member

Nice! We can re-open if other issues of being out of spec arise.

dessalines pushed a commit that referenced this issue Feb 5, 2021
@Nutomic
Copy link
Member Author

Nutomic commented Feb 8, 2021

@vpzomtrrfrt @lanodan We fixed the ActivityPub problems that you reported, and some others as well. The fixes are already live on our test and production servers. So please give it a try, and let us know if you find any additional federation problems.

@dessalines dessalines unpinned this issue Feb 9, 2021
@lanodan
Copy link

lanodan commented Feb 9, 2021

I'm guessing lemmy.ml is up-to-date enough:

  • curl -v -H 'Accept: application/activity+json' https://lemmy.ml/post/52475/comment/34246 | jq . → 404 even though the post is visible without AP
  • curl -v -H 'Accept: application/activity+json' https://lemmy.ml/u/dessalines/outbox | jq . → An empty outbox
  • Accept: application/ld+json,application/activity+json,application/json Doesn't works, not sure if some production software uses multiple accept formats but I have it in my script for fetching AP data, seems like an HTTP server/framework issue that isn't able to fallback or manage multi-formats.
  • Page (in example https://lemmy.ml/post/52763) is missing https://www.w3.org/ns/activitystreams#Public in to or cc so it ends up being hidden like a direct message to it's Group (in example https://lemmy.ml/c/lemmy) rather than a public post.
  • Page's content field seems to still be in markdown, we already have a conversion because of Peertube but I'd rather keep that as the exception than the rule as markdown format is seriously implementation-defined.

@Nutomic
Copy link
Member Author

Nutomic commented Feb 10, 2021

curl -v -H 'Accept: application/activity+json' https://lemmy.ml/post/52475/comment/34246 | jq . → 404 even though the post is visible without AP

This is an inconsistency between HTTP and JSON routes, the comment ID is actually https://lemmy.ml/comment/34246. Part of #698

curl -v -H 'Accept: application/activity+json' https://lemmy.ml/u/dessalines/outbox | jq . → An empty outbox

We only have community outboxes implemented for now (and they only contain the last 20 posts, nothing else). We would implement it as part of #752

Accept: application/ld+json,application/activity+json,application/json Doesn't works, not sure if some production software uses multiple accept formats but I have it in my script for fetching AP data, seems like an HTTP server/framework issue that isn't able to fallback or manage multi-formats.

I opened an issue at #1420

Page (in example https://lemmy.ml/post/52763) is missing https://www.w3.org/ns/activitystreams#Public in to or cc so it ends up being hidden like a direct message to it's Group (in example https://lemmy.ml/c/lemmy) rather than a public post.

Thanks, we weren't sure if that should be set on the activities or on the objects, sounds like it should be on both. I added it in !167

Page's content field seems to still be in markdown, we already have a conversion because of Peertube but I'd rather keep that as the exception than the rule as markdown format is seriously implementation-defined.

This is for compatibility with older Lemmy versions, but I already have a commit ready to change it to HTML. We can merge that any time, but its a breaking change and not urgent for now, so we are waiting to release it together with some other breaking changes.

@lanodan
Copy link

lanodan commented Feb 10, 2021 via email

@Nutomic
Copy link
Member Author

Nutomic commented Feb 10, 2021

@lanodan I think this page has everything: https://join.lemmy.ml/docs/en/about/guide.html#markdown-guide

And FYI, we use different libraries for markdown in the frontend (for html) and in the backend (for activitypub. So its possible that there are also differences between them.

dessalines pushed a commit that referenced this issue Feb 10, 2021
@lanodan
Copy link

lanodan commented Feb 19, 2021

So I tested federation between lemmy(using lemmy.ml) and pleroma(using queer.hacktivis.me) and discovered:

Also after looking at your markdown guide, we're missing:

  • horizontal rule (probably HTML filtering)
  • spoiler (we don't have support for <details><summary></summary></details> in HTML but that could be added)
  • subscript/superscript (supported in HTML)

Btw the way we do previews in pleroma is by sending a false request to the backend and in our experience it's not too slow.

@Nutomic
Copy link
Member Author

Nutomic commented Feb 22, 2021

Your page objects are using the summary field instead of the name field, meaning that we don't get a proper title in pleroma, maybe could be just a fix on our side (like when name is null/empty summary is moved to it)

Is there any difference between the two? The description in Activitystreams Vocabulary is almost identical.

I can't seem to be able to follow a user, maybe this is normal for now but if it's by design maybe there should be a field in your Person actor advertising that?

We haven't implemented user following yet. Which field can we use for that?

I can't seem to be able to comment. Tried with https://queer.hacktivis.me/objects/5f62890d-62d7-4684-b519-6b19d29a3cbc and https://queer.hacktivis.me/objects/5b40c4e4-e77e-44c7-91bc-79266fd50d21

It prints this error when fetching your user: Failed to resolve search query as activitypub ID: value too long for type character varying(20). Must be because of the name field which has 24 chars. You could shorten that and try again. What is the max name length in Pleroma? (ping @dessalines)

Btw you can also open a new issue for federation with Pleroma ;)

@lanodan
Copy link

lanodan commented Feb 22, 2021

Your page objects are using the summary field instead of the name field, meaning that we don't get a proper title in pleroma, maybe could be just a fix on our side (like when name is null/empty summary is moved to it)

Is there any difference between the two? The description in Activitystreams Vocabulary is almost identical.

Well de-facto summary is used as a smaller version of "content" that can be used for a spoiler-tag, like the <summary> element in HTML and name is used as a title (articles, peertube video, friendica posts).

I can't seem to be able to follow a user, maybe this is normal for now but if it's by design maybe there should be a field in your Person actor advertising that?

We haven't implemented user following yet. Which field can we use for that?

None in ActivityStreams but you can extend it. That said if it's a "yet" I don't think it would be worth it to add a new one.

I can't seem to be able to comment. Tried with https://queer.hacktivis.me/objects/5f62890d-62d7-4684-b519-6b19d29a3cbc and https://queer.hacktivis.me/objects/5b40c4e4-e77e-44c7-91bc-79266fd50d21

It prints this error when fetching your user: Failed to resolve search query as activitypub ID: value too long for type character varying(20). Must be because of the name field which has 24 chars. You could shorten that and try again. What is the max name length in Pleroma? (ping @dessalines)

The default limits in pleroma are 100 for the name, 5000 for the bio, and 5000 for the post payload ((name+summary+content) < 5000).
When it's longer than that we simply truncate the content.
Usually a rule of thumb that I have for field limits -specially when hardcoded- is to pick something usual and multiply that by about two as a margin.

Btw you can also open a new issue for federation with Pleroma ;)

Ack, will do for next ones

@Nutomic
Copy link
Member Author

Nutomic commented Feb 22, 2021

Well de-facto summary is used as a smaller version of "content" that can be used for a spoiler-tag, like the

element in HTML and name is used as a title (articles, peertube video, friendica posts).

In fact we used summary to be compatible with Mastodon's spoiler tag, but Peertube videos or articles are probably the more similar one. I implemented a fix in !173

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: federation support federation via activitypub enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants