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

Add ActivityStreams2 URL field to Accounts and populate it during WebFinger lookup. #3827

Closed
wants to merge 2 commits into from

Conversation

evanminto
Copy link
Contributor

Fairly simple. The FollowRemoteAccountService now looks for a rel-self value in the WebFinger response and if it's present, assigns it to a new Account field, activitystreams2_url. I'm relatively new to this area of the code, so let me know if I missed any gotchas!

@evanminto evanminto requested a review from Gargron June 17, 2017 18:41
@@ -36,6 +36,7 @@
# followers_count :integer default(0), not null
# following_count :integer default(0), not null
# last_webfingered_at :datetime
# activitystreams2_url :string
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe activitypub_url?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I thought about that option, but technically AP is only the protocol for server/client communication, built on top of AS2 representations. The URL is the user's AS2 representation (which happens to implement AP), so that seemed like the more accurate name. Not a big deal either way though.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I understand correctly AS2 could also be used for OStatus, so this might be confusing.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm for activitystrams2_url because Webfinger does not tell if it has ActivityPub extensions such as inbox and outbox. Methods referring to this column must NOT assume it has ActivityPub extensions and must do some checks. I'm worrying that naming it as activitypub_url would promote such mistakes.

@akihikodaki akihikodaki added the api REST API, Streaming API, Web Push API label Jun 19, 2017
@Gargron
Copy link
Member

Gargron commented Jun 24, 2017

I'm thinking about all the URLs we need to store for an AP user:

  • URL of the actor representation, so we can update all other URLs and profile data
  • inbox URL
  • publicInbox URL ???? It feels wasteful to duplicate that value over potentially thousands of users...
  • outbox URL
  • followers collection URL
  • following collection URL (potentially used for mutuals-only addressing?)

Things that are the same for both AP and OStatus users:

  • public/private keys (not technically a URL but vitally important for interacting with user)
  • avatar URL
  • header URL

I am not sure if it makes sense to create new columns for each of these, or perhaps create some serialized field?

@akihikodaki
Copy link
Contributor

Actually we may need to consider about variables other than activitystreams2_url to consolidate the idea.

publicInbox URL ???? It feels wasteful to duplicate that value over potentially thousands of users...

However, the specification does not say publicInbox would have a common value for all accounts in a server. It would be the safest way to save publicInbox URL for each accounts.

I am not sure if it makes sense to create new columns for each of these, or perhaps create some serialized field?

Obviously publicInbox should not be. publicInbox will have an index which would be used when publishing statuses. For other columns, I do not think it matters, but it would be consistent with publicIndex to have columns for each.

@Gargron
Copy link
Member

Gargron commented Jun 24, 2017

@akihikodaki

However, the specification does not say publicInbox would have a common value for all accounts in a server

In fact, the purpose of the publicInbox URL is to consolidate payloads to a common server when multiple recipients share one: https://www.w3.org/TR/activitypub/#public-inbox-delivery

@akihikodaki
Copy link
Contributor

@Gargron But it is still possible to have different values in an instance. For example, an instance may want to distribute the load of the inbox server and then have multiple publicInbox servers, assigning different accounts for each.

@Gargron Gargron added the activitypub Protocol-related changes, federation label Jul 11, 2017
@Gargron
Copy link
Member

Gargron commented Jul 17, 2017

Existing attribute Meaning in OStatus Equivalent in ActivityPub
remote_url Atom feed, where items reside Outbox URL
salmon_url Salmon endpoint, where to deliver personal payloads Inbox URL
url URL of HTML profile URL of HTML profile
uri URI of account. Traditionally a URL, but can be some other string Actor URL
hub_url Where to subscribe to updates, generally same for every account on same domain Shared Inbox URL? Has nothing to do with subscriptions, but is also "same for all domain"

As you can see there is some overlap in meaning, attributes could be renamed more generically, we could add an "account type" enum which is either ostatus or activitypub, so we would handle the same attributes differently. There is some concern about backwards-compatibility, especially with value of uri, however perhaps the webfinger code could upgrade that account data once it discovers the account supports ActivityPub, because once that's the case there is no reason to use OStatus data of the account anymore.

Anyway, just some ideas.

There are also some definitely new attributes, such as followers_url, it could also be cool to cache followers_remote_count/following_remote_count/statuses_remote_count because that information (totalItems) is provided by the respective collection endpoints.

@ThibG @evanminto @akihikodaki

@evanminto
Copy link
Contributor Author

I don't like the semantics of overloading the OStatus columns. Renaming them would help, but I think it's still a bit messy to try to merge the two standards together.

That said, it looks like we could do a bit of mixing and matching, so that each account would have the following columns (not an exhaustive list):

  • remote_url
  • salmon_url
  • inbox_url
  • outbox_url
  • uri (same for both OStatus and AP)
  • url (same for both OStatus and AP)
  • hub_url
  • public_inbox

An account type enum seems useful either way, though maybe we could have that be generated on the fly based on which columns are set.

Also I agree it's a good idea to cache the follower/following/statuses counts as they'll be requested quite a lot.

@akihikodaki
Copy link
Contributor

I'm for renaming, but public inbox should have different column because it is different from hub_url in terms of semantics. Even though the column can be shared, that is excessive optimization IMHO.

So we could have the following change:

Existing column New column
remote_url outbox_url
salmon_url inbox_url
url url
uri uri
hub_url hub_url
N/A public_inbox_url
N/A protocol

@nightpool
Copy link
Member

nightpool commented Jul 17, 2017

I'm very 👎 on adding an account type enum or overloading existing OStatus fields, since it completely closes the door on any possible gradual migration path between ostatus and activitypub for mastodon. (For example, using AP for private posts while still federating public posts using ostatus)

It seems very bad to switch completely from ostatus to AP in a single version or pull request—I would be much happier if we did a gradual implementation that got some of this code shipping before we migrated everything. It seems a lot less likely to be "biting off more then we can chew", as well.

@akihikodaki
Copy link
Contributor

Additional code will be required to support gradual migration anyway. We can add columns then if we need the feature.

@Gargron
Copy link
Member

Gargron commented Jul 17, 2017

@nightpool These fields are for remote accounts.... You don't need to know them to publish OStatus content via PubSubHubbub....

And when a remote account is ActivityPub-ready, what's the point of still using OStatus to send stuff to it?

@nightpool
Copy link
Member

And when a remote account is ActivityPub-ready, what's the point of still using OStatus to send stuff to it?

@Gargron because that assumes that we're going to implement AP in one fell swoop, rather then incrementally over a series of changes? One makes it very hard to test and use this code, and it ends up with writing lots and lots of code that will never see use in the codebase until we "switch it on".

@Gargron
Copy link
Member

Gargron commented Jul 17, 2017

@nightpool It is not possible to have partial ActivityPub in my opinion.

@evanminto
Copy link
Contributor Author

Unless I missed something, it looks like this is superseded by #4216. I can close this unless you think there's still something worth doing in this PR @Gargron.

@Gargron
Copy link
Member

Gargron commented Aug 8, 2017

You're right this is superseded by that

@Gargron Gargron closed this Aug 8, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
activitypub Protocol-related changes, federation api REST API, Streaming API, Web Push API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants