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

Federation Gem Step 1: Webfinger/HCard generation #6151

Merged
merged 5 commits into from Jul 12, 2015

Conversation

Projects
None yet
8 participants
@SuperTux88
Copy link
Member

SuperTux88 commented Jul 1, 2015

I'm working on a new federation gem. This is built as a rails engine, so it can simply be mounted to a rails-app and provide the URLs that are needed for the federation. Some parts are based on @Raven24's old federation gem. After the diaspora_federation gem is complete, we can move this to the diaspora organization.

I also integrate the new gem into diaspora and I want to create several small pull requests for this, so that it is better to review with small changes.

In the first step I replaced the host-meta, webfinger and hcard generation with the new diaspora_federation gem. Everything else is still working as before.

I wrote a new spec, which generates the fixtures with the new controllers. So the existing parser is tested with the new generator. This can be removed again when the parsing is also done in the gem.

Also done in this PR:

  • refactored the atom_url to contain the .atom suffix
  • dropped caches_page, this fixes #5143
  • removed unused publics_helper, this was unused since 2010
  • don't get person.url from the hcard, this is already set from the webfinger

@denschub reviews my work at the gem on a regular basis, I think he can also review this PR :)

@@ -4,7 +4,7 @@

module Workers
class PublishToHub < Base
def perform(sender_public_url)
def perform(sender_atom_url)

This comment has been minimized.

@houndci-bot

houndci-bot Jul 1, 2015

Unused method argument - sender_atom_url. If it's necessary, use _ or _sender_atom_url as an argument name to indicate that it won't be used. You can also write as perform(*) if you want the method to accept any arguments but don't care about them.

pubkey: serialized_public_key,
searchable: searchable,
first_name: first_name,
last_name: last_name

This comment has been minimized.

@houndci-bot

houndci-bot Jul 1, 2015

Unnecessary spacing detected.

photo_small_url: profile.image_url_small,
pubkey: serialized_public_key,
searchable: searchable,
first_name: first_name,

This comment has been minimized.

@houndci-bot

houndci-bot Jul 1, 2015

Unnecessary spacing detected.

photo_medium_url: profile.image_url_medium,
photo_small_url: profile.image_url_small,
pubkey: serialized_public_key,
searchable: searchable,

This comment has been minimized.

@houndci-bot

houndci-bot Jul 1, 2015

Unnecessary spacing detected.

photo_large_url: image_url,
photo_medium_url: profile.image_url_medium,
photo_small_url: profile.image_url_small,
pubkey: serialized_public_key,

This comment has been minimized.

@houndci-bot

houndci-bot Jul 1, 2015

Unnecessary spacing detected.

url: url,
photo_large_url: image_url,
photo_medium_url: profile.image_url_medium,
photo_small_url: profile.image_url_small,

This comment has been minimized.

@houndci-bot

houndci-bot Jul 1, 2015

Unnecessary spacing detected.

This comment has been minimized.

@jhass

jhass Jul 1, 2015

Member

That feels inconsistent, either further add to the delegation and use only the delegated helpers, or use all fields from the profile explicitly

nickname: username,
full_name: "#{first_name} #{last_name}",
url: url,
photo_large_url: image_url,

This comment has been minimized.

@houndci-bot

houndci-bot Jul 1, 2015

Unnecessary spacing detected.

guid: guid,
nickname: username,
full_name: "#{first_name} #{last_name}",
url: url,

This comment has been minimized.

@houndci-bot

houndci-bot Jul 1, 2015

Unnecessary spacing detected.

{
guid: guid,
nickname: username,
full_name: "#{first_name} #{last_name}",

This comment has been minimized.

@houndci-bot

houndci-bot Jul 1, 2015

Unnecessary spacing detected.

This comment has been minimized.

@jhass

jhass Jul 1, 2015

Member

Both fields are optional, so this may end up to be the empty space. If that's not relevant because the other side never uses that field, then I think we already have a full_name method somewhere that can be used. Else this should probably have a .strip

def hcard_profile_hash
{
guid: guid,
nickname: username,

This comment has been minimized.

@houndci-bot

houndci-bot Jul 1, 2015

Unnecessary spacing detected.


def hcard_profile_hash
{
guid: guid,

This comment has been minimized.

@houndci-bot

houndci-bot Jul 1, 2015

Unnecessary spacing detected.

atom_url: atom_url,
salmon_url: receive_url,
guid: guid,
pubkey: serialized_public_key

This comment has been minimized.

@houndci-bot

houndci-bot Jul 1, 2015

Unnecessary spacing detected.

profile_url: profile_url,
atom_url: atom_url,
salmon_url: receive_url,
guid: guid,

This comment has been minimized.

@houndci-bot

houndci-bot Jul 1, 2015

Unnecessary spacing detected.

seed_url: url,
profile_url: profile_url,
atom_url: atom_url,
salmon_url: receive_url,

This comment has been minimized.

@houndci-bot

houndci-bot Jul 1, 2015

Unnecessary spacing detected.

This comment has been minimized.

@jhass

jhass Jul 1, 2015

Member

How about cleaning up all the URL generation a bit and making it more robust:

def uri
  @uri ||= URI.parse(url)
  @uri.dup
end

def url_to(path)
  uri.tap {|uri| uri.path = path }.to_s
end

And then while at it also cleaning up clean_url (still used, can usages be replaced?) and update_url below.

This comment has been minimized.

@SuperTux88

SuperTux88 Jul 1, 2015

Member

@jhass: Do you know why we need the rescue in the url-method? do we have invalid URLs anywhere? Or can I expect parseable URLs here?

This comment has been minimized.

@jhass

jhass Jul 1, 2015

Member

No I don't know, I'd assume defensive coding.

This comment has been minimized.

@SuperTux88

SuperTux88 Jul 1, 2015

Member

I'll have a look at it. But I think that should be a valid URL anyway, or the other generated URLs wouldn't work ...

hcard_url: url[0...-1] + DiasporaFederation::Engine.routes.url_helpers.hcard_path(guid),
seed_url: url,
profile_url: profile_url,
atom_url: atom_url,

This comment has been minimized.

@houndci-bot

houndci-bot Jul 1, 2015

Unnecessary spacing detected.

acct_uri: "acct:#{diaspora_handle}",
alias_url: "#{url}people/#{guid}",
hcard_url: url[0...-1] + DiasporaFederation::Engine.routes.url_helpers.hcard_path(guid),
seed_url: url,

This comment has been minimized.

@houndci-bot

houndci-bot Jul 1, 2015

Unnecessary spacing detected.

{
acct_uri: "acct:#{diaspora_handle}",
alias_url: "#{url}people/#{guid}",
hcard_url: url[0...-1] + DiasporaFederation::Engine.routes.url_helpers.hcard_path(guid),

This comment has been minimized.

@houndci-bot

houndci-bot Jul 1, 2015

Unnecessary spacing detected.

def webfinger_hash
{
acct_uri: "acct:#{diaspora_handle}",
alias_url: "#{url}people/#{guid}",

This comment has been minimized.

@houndci-bot

houndci-bot Jul 1, 2015

Unnecessary spacing detected.


def webfinger_hash
{
acct_uri: "acct:#{diaspora_handle}",

This comment has been minimized.

@houndci-bot

houndci-bot Jul 1, 2015

Unnecessary spacing detected.

@username ||= owner ? owner.username : diaspora_handle.split("@")[0]
end

def webfinger_hash

This comment has been minimized.

@houndci-bot

houndci-bot Jul 1, 2015

Assignment Branch Condition size for webfinger_hash is too high. [16/15]

@@ -32,7 +32,7 @@ def load_javascript_locales(section = 'javascripts')

def current_user_atom_tag
return unless @person.present?
content_tag(:link, '', :rel => 'alternate', :href => "#{@person.public_url}.atom", :type => "application/atom+xml", :title => t('.public_feed', :name => @person.name))
content_tag(:link, "", rel: "alternate", href: @person.atom_url, type: "application/atom+xml", title: t(".public_feed", name: @person.name))

This comment has been minimized.

@houndci-bot

houndci-bot Jul 1, 2015

Line is too long. [144/120]

@SuperTux88 SuperTux88 force-pushed the SuperTux88:federation-gem branch from 0d2fdf9 to 77cff17 Jul 1, 2015

@denschub denschub self-assigned this Jul 1, 2015

first_name: first_name,
last_name: last_name
}
end

This comment has been minimized.

@denschub

denschub Jul 1, 2015

Member

I am not sure if I like those hashes. It would be nicer to just have a list of attributes that are included.

m = double()
describe ".perform" do
it "calls pubsubhubbub" do
url = "http://publiczone.com/public/username.atom"

This comment has been minimized.

@denschub

denschub Jul 1, 2015

Member

Please use example.com everywhere.

@Flaburgan

This comment has been minimized.

Copy link
Member

Flaburgan commented Jul 1, 2015

Wow, awesome!

Gemfile Outdated
@@ -15,6 +10,10 @@ gem "responders", "2.1.0"

gem "unicorn", "4.9.0", require: false

# Federation

gem "diaspora_federation", "0.0.1"

This comment has been minimized.

@jaywink

jaywink Jul 1, 2015

Contributor

🍪 👍

def self.by_account_identifier(identifier)
identifier = identifier.strip.downcase.gsub('acct:', '')
self.where(:diaspora_handle => identifier).first
identifier = identifier.strip.downcase.gsub("acct:", "")

This comment has been minimized.

@jhass

jhass Jul 1, 2015

Member

A simple sub should be enough and communicate that we only expect one occurrence.

config.server_uri = AppConfig.pod_uri

# the class to be used for a person
config.person_class = Person.to_s

This comment has been minimized.

@jhass

jhass Jul 1, 2015

Member

I think the accessor should duck type here and call to_s on its own. Assigning the actual classes is just too common, especially in the Rails world.

This comment has been minimized.

@SuperTux88

SuperTux88 Jul 1, 2015

Member

The Rails documentation says it should be done with a String: http://guides.rubyonrails.org/engines.html#configuring-an-engine (the red block)

It's very important here to use the String version of the class, rather than the class itself. If you were to use the class, Rails would attempt to load that class and then reference the related table. This could lead to problems if the table wasn't already existing. Therefore, a String should be used and then converted to a class using constantize in the engine later on.

So, because it's red, I thought it's important to use a String here ;) But I can move the .to_s to the gem and still hold it as a String in the gem.

This comment has been minimized.

@jhass

jhass Jul 1, 2015

Member

Yes, I'm just saying it's safer to do in the accessor, since you then 1) don't need that knowledge 2) aren't confused by it since it's common not to in the Rails API (e.g. has_many :foo, class: Bar)

person = Person.find_local_by_guid(guid)
if person
DiasporaFederation::WebFinger::HCard.new(
guid: person.guid,

This comment has been minimized.

@houndci-bot

houndci-bot Jul 10, 2015

Unnecessary spacing detected.

atom_url: person.atom_url,
salmon_url: person.receive_url,
guid: person.guid,
public_key: person.serialized_public_key

This comment has been minimized.

@houndci-bot

houndci-bot Jul 10, 2015

Unnecessary spacing detected.

profile_url: person.profile_url,
atom_url: person.atom_url,
salmon_url: person.receive_url,
guid: person.guid,

This comment has been minimized.

@houndci-bot

houndci-bot Jul 10, 2015

Unnecessary spacing detected.

seed_url: AppConfig.pod_uri,
profile_url: person.profile_url,
atom_url: person.atom_url,
salmon_url: person.receive_url,

This comment has been minimized.

@houndci-bot

houndci-bot Jul 10, 2015

Unnecessary spacing detected.

hcard_url: url_to(DiasporaFederation::Engine.routes.url_helpers.hcard_path(person.guid)),
seed_url: AppConfig.pod_uri,
profile_url: person.profile_url,
atom_url: person.atom_url,

This comment has been minimized.

@houndci-bot

houndci-bot Jul 10, 2015

Unnecessary spacing detected.

acct_uri: "acct:#{person.diaspora_handle}",
alias_url: url_to("/people/#{person.guid}"),
hcard_url: url_to(DiasporaFederation::Engine.routes.url_helpers.hcard_path(person.guid)),
seed_url: AppConfig.pod_uri,

This comment has been minimized.

@houndci-bot

houndci-bot Jul 10, 2015

Unnecessary spacing detected.

DiasporaFederation::WebFinger::WebFinger.new(
acct_uri: "acct:#{person.diaspora_handle}",
alias_url: url_to("/people/#{person.guid}"),
hcard_url: url_to(DiasporaFederation::Engine.routes.url_helpers.hcard_path(person.guid)),

This comment has been minimized.

@houndci-bot

houndci-bot Jul 10, 2015

Unnecessary spacing detected.

if person
DiasporaFederation::WebFinger::WebFinger.new(
acct_uri: "acct:#{person.diaspora_handle}",
alias_url: url_to("/people/#{person.guid}"),

This comment has been minimized.

@houndci-bot

houndci-bot Jul 10, 2015

Unnecessary spacing detected.

This comment has been minimized.

@jhass

jhass Jul 10, 2015

Member

I would consider moving this into a model method, like we already have profile_url and such.

person = Person.find_local_by_diaspora_handle(handle)
if person
DiasporaFederation::WebFinger::WebFinger.new(
acct_uri: "acct:#{person.diaspora_handle}",

This comment has been minimized.

@houndci-bot

houndci-bot Jul 10, 2015

Unnecessary spacing detected.

@SuperTux88

This comment has been minimized.

Copy link
Member

SuperTux88 commented Jul 11, 2015

I have moved the url_to to the AppConfig and wrote some tests for the callbacks.

@SuperTux88 SuperTux88 force-pushed the SuperTux88:federation-gem branch from 277faa7 to 0ffe513 Jul 11, 2015

@denschub denschub merged commit 0ffe513 into diaspora:develop Jul 12, 2015

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
hound 18 violations found.

denschub added a commit that referenced this pull request Jul 12, 2015

Merge pull request #6151 from SuperTux88/federation-gem
Federation Gem Step 1: Webfinger/HCard generation
@denschub

This comment has been minimized.

Copy link
Member

denschub commented Jul 12, 2015

Okay, I merged this.

Thank you very much for your ongoing work, looking forward to the upcoming pull request. ❤️

@denschub denschub added this to the 0.6.0.0 milestone Jul 12, 2015

@Flaburgan

This comment has been minimized.

Copy link
Member

Flaburgan commented Jul 13, 2015

Yay! This is awesome, definitely the most important work on diaspora* for months. Thank you so much for what you're doing.

@Fensterbank

This comment has been minimized.

Copy link
Contributor

Fensterbank commented Jul 13, 2015

That's definitely great! Keep up the good work. 👍

denschub added a commit that referenced this pull request Jul 14, 2015

@denschub

This comment has been minimized.

Copy link
Member

denschub commented Jul 14, 2015

As discussed with @SuperTux88, I backported this to 0.5.2.0. We merged the other federation refactorings into minors as well and I see no harm in having additional production time here.

This breaks 2.0 builds. @SuperTux88 is aware of that and I expect a fix soonish.

@denschub denschub modified the milestones: 0.5.2.0, 0.6.0.0 Jul 14, 2015

@svbergerem svbergerem referenced this pull request Jul 20, 2015

Closed

Integration of diaspora-federation gem #5622

1 of 5 tasks complete

@denschub denschub removed this from the 0.5.2.0 milestone Jul 27, 2015

@SuperTux88 SuperTux88 deleted the SuperTux88:federation-gem branch Aug 9, 2015

@denschub denschub removed their assignment Aug 25, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment