Merge pull request #7533 from SuperTux88/add-indexes

Add missing indexes from #7234

Changelog.md

@@ -32,6 +32,7 @@ If so, please delete it since it will prevent the federation from working proper
 * Refactoring single post view interactions [#7182](https://github.com/diaspora/diaspora/pull/7182)
 * Update help pages [#7528](https://github.com/diaspora/diaspora/pull/7528)
 * Disable rendering logging in production [#7529](https://github.com/diaspora/diaspora/pull/7529)
+* Add some missing indexes and cleanup the database if needed [#7533](https://github.com/diaspora/diaspora/pull/7533)
 
 ## Bug fixes
 

app/models/account_deletion.rb

@@ -5,28 +5,20 @@
 class AccountDeletion < ApplicationRecord
   include Diaspora::Federated::Base
 
-  scope :uncompleted, -> { where('completed_at is null') }
+  scope :uncompleted, -> { where("completed_at is null") }
 
   belongs_to :person
-  after_commit :queue_delete_account, :on => :create
+  after_commit :queue_delete_account, on: :create
 
-  def person=(person)
-    self[:diaspora_handle] = person.diaspora_handle
-    self[:person_id] = person.id
-  end
-
-  def diaspora_handle=(diaspora_handle)
-    self[:diaspora_handle] = diaspora_handle
-    self[:person_id] ||= Person.find_by_diaspora_handle(diaspora_handle).id
-  end
+  delegate :diaspora_handle, to: :person
 
   def queue_delete_account
-    Workers::DeleteAccount.perform_async(self.id)
+    Workers::DeleteAccount.perform_async(id)
   end
 
   def perform!
     Diaspora::Federation::Dispatcher.build(person.owner, self).dispatch if person.local?
-    AccountDeleter.new(diaspora_handle).perform!
+    AccountDeleter.new(person).perform!
   end
 
   def subscribers

app/models/user.rb

@@ -507,7 +507,7 @@ class User < ApplicationRecord
   def close_account!
     self.person.lock_access!
     self.lock_access!
-    AccountDeletion.create(:person => self.person)
+    AccountDeletion.create(person: person)
   end
 
   def closed_account?

db/migrate/20170813141631_cleanup_account_deletions_and_add_unique_index.rb

@@ -0,0 +1,19 @@
+class CleanupAccountDeletionsAndAddUniqueIndex < ActiveRecord::Migration[5.1]
+  def up
+    remove_column :account_deletions, :diaspora_handle
+
+    duplicate_query = "WHERE a1.person_id = a2.person_id AND a1.id > a2.id"
+    if AppConfig.postgres?
+      execute("DELETE FROM account_deletions AS a1 USING account_deletions AS a2 #{duplicate_query}")
+    else
+      execute("DELETE a1 FROM account_deletions a1, account_deletions a2 #{duplicate_query}")
+    end
+
+    add_index :account_deletions, :person_id, name: :index_account_deletions_on_person_id, unique: true
+  end
+
+  def down
+    remove_index :account_deletions, name: :index_account_deletions_on_person_id
+    add_column :account_deletions, :diaspora_handle, :string
+  end
+end

db/migrate/20170813153048_add_missing_indexes.rb

@@ -0,0 +1,7 @@
+class AddMissingIndexes < ActiveRecord::Migration[5.1]
+  def change
+    add_index :photos, :author_id
+    add_index :user_preferences, %i[user_id email_type], length: {email_type: 190}
+    add_index :locations, :status_message_id
+  end
+end

db/migrate/20170813160104_cleanup_aspects_and_add_unique_index.rb

@@ -0,0 +1,21 @@
+class CleanupAspectsAndAddUniqueIndex < ActiveRecord::Migration[5.1]
+  class Aspect < ApplicationRecord
+  end
+
+  def up
+    cleanup_aspects
+    add_index :aspects, %i[user_id name], name: :index_aspects_on_user_id_and_name, length: {name: 190}, unique: true
+  end
+
+  def down
+    remove_index :aspects, name: :index_aspects_on_user_id_and_name
+  end
+
+  def cleanup_aspects
+    Aspect.where(user_id: 0).delete_all
+    Aspect.joins("INNER JOIN aspects as a2 ON aspects.user_id = a2.user_id AND aspects.name = a2.name")
+          .where("aspects.id > a2.id").each do |aspect|
+      aspect.update_attributes(name: "#{aspect.name}_#{UUID.generate(:compact)}")
+    end
+  end
+end

db/migrate/20170813164435_add_missing_unique_indexes.rb

@@ -0,0 +1,41 @@
+class AddMissingUniqueIndexes < ActiveRecord::Migration[5.1]
+  def up
+    cleanup
+
+    remove_index :aspect_visibilities, name: :shareable_and_aspect_id
+    add_index :aspect_visibilities, %i[shareable_id shareable_type aspect_id],
+              name: :index_aspect_visibilities_on_shareable_and_aspect_id,
+              length: {shareable_type: 189}, unique: true
+
+    add_index :blocks, %i[user_id person_id], name: :index_blocks_on_user_id_and_person_id, unique: true
+
+    add_index :roles, %i[person_id name], name: :index_roles_on_person_id_and_name, length: {name: 190}, unique: true
+  end
+
+  def down
+    remove_index :aspect_visibilities, name: :index_aspect_visibilities_on_shareable_and_aspect_id
+    add_index :aspect_visibilities, %i[shareable_id shareable_type aspect_id], name: :shareable_and_aspect_id,
+              length: {shareable_type: 189}, unique: true
+
+    remove_index :blocks, name: :index_blocks_on_user_id_and_person_id
+
+    remove_index :roles, name: :index_roles_on_person_id_and_name
+  end
+
+  def cleanup
+    aspect_visibilities_where = "WHERE a1.shareable_id = a2.shareable_id AND a1.shareable_type = a2.shareable_type " \
+                                "AND a1.aspect_id = a2.aspect_id AND a1.id > a2.id"
+    blocks_where = "WHERE b1.user_id = b2.user_id AND b1.person_id = b2.person_id AND b1.id > b2.id"
+    roles_where = "WHERE r1.person_id = r2.person_id AND r1.name = r2.name AND r1.id > r2.id"
+
+    if AppConfig.postgres?
+      execute "DELETE FROM aspect_visibilities AS a1 USING aspect_visibilities AS a2 #{aspect_visibilities_where}"
+      execute "DELETE FROM blocks AS b1 USING blocks AS b2 #{blocks_where}"
+      execute "DELETE FROM roles AS r1 USING roles AS r2 #{roles_where}"
+    else
+      execute "DELETE a1 FROM aspect_visibilities a1, aspect_visibilities a2 #{aspect_visibilities_where}"
+      execute "DELETE b1 FROM blocks b1, blocks b2 #{blocks_where}"
+      execute "DELETE r1 FROM roles r1, roles r2 #{roles_where}"
+    end
+  end
+end

lib/account_deleter.rb

@@ -17,9 +17,9 @@ class AccountDeleter
 
   attr_accessor :person, :user
 
-  def initialize(diaspora_handle)
-    self.person = Person.where(:diaspora_handle => diaspora_handle).first
-    self.user = self.person.owner
+  def initialize(person)
+    self.person = person
+    self.user = person.owner
   end
 
   def perform!
@@ -34,7 +34,6 @@ class AccountDeleter
         #user deletion methods
         remove_share_visibilities_on_contacts_posts
         delete_standard_user_associations
-        disconnect_contacts
         tombstone_user
       end
 
@@ -44,17 +43,17 @@ class AccountDeleter
 
   #user deletions
   def normal_ar_user_associates_to_delete
-    %i(tag_followings services aspects user_preferences
-       notifications blocks authorizations o_auth_applications pairwise_pseudonymous_identifiers)
+    %i[tag_followings services aspects user_preferences
+       notifications blocks authorizations o_auth_applications pairwise_pseudonymous_identifiers]
   end
 
   def special_ar_user_associations
-    %i(person profile contacts auto_follow_back_aspect)
+    %i[person profile contacts auto_follow_back_aspect]
   end
 
   def ignored_ar_user_associations
-    %i(followed_tags invited_by contact_people aspect_memberships
-       ignored_people share_visibilities conversation_visibilities conversations reports)
+    %i[followed_tags invited_by contact_people aspect_memberships
+       ignored_people share_visibilities conversation_visibilities conversations reports]
   end
 
   def delete_standard_user_associations
@@ -69,10 +68,6 @@ class AccountDeleter
     end
   end
 
-  def disconnect_contacts
-    user.contacts.destroy_all
-  end
-
   # Currently this would get deleted due to the db foreign key constrainsts,
   # but we'll keep this method here for completeness
   def remove_share_visibilities_on_contacts_posts
@@ -97,7 +92,7 @@ class AccountDeleter
   end
 
   def normal_ar_person_associates_to_delete
-    %i(posts photos mentions participations roles)
+    %i[posts photos mentions participations roles]
   end
 
   def ignored_or_special_ar_person_associations
@@ -106,6 +101,6 @@ class AccountDeleter
   end
 
   def mark_account_deletion_complete
-    AccountDeletion.where(:diaspora_handle => self.person.diaspora_handle).where(:person_id => self.person.id).update_all(["completed_at = ?", Time.now])
+    AccountDeletion.find_by(person: person)&.update_attributes(completed_at: Time.now.utc)
   end
 end

lib/diaspora/federation/receive.rb

@@ -8,7 +8,7 @@ module Diaspora
       end
 
       def self.account_deletion(entity)
-        AccountDeletion.create!(person: author_of(entity), diaspora_handle: entity.author)
+        AccountDeletion.create!(person: author_of(entity))
       end
 
       def self.comment(entity)

spec/factories.rb

@@ -51,9 +51,6 @@ FactoryGirl.define do
 
   factory :account_deletion do
     association :person
-    after(:build) do |delete|
-      delete.diaspora_handle = delete.person.diaspora_handle
-    end
   end
 
   factory :like do

spec/integration/account_deletion_spec.rb

@@ -1,11 +1,12 @@
 describe "deleteing account", type: :request do
   def account_removal_method
-    AccountDeleter.new(subject.diaspora_handle).perform!
+    AccountDeleter.new(person).perform!
     subject.reload
   end
 
   context "of local user" do
     subject(:user) { FactoryGirl.create(:user_with_aspect) }
+    let(:person) { user.person }
 
     before do
       DataGenerator.create(subject, :generic_user_data)
@@ -29,9 +30,7 @@ describe "deleteing account", type: :request do
         }.to(eq([true] * user.send(:clearable_fields).count)))
     end
 
-    it_behaves_like "it removes the person associations" do
-      subject(:person) { user.person }
-    end
+    it_behaves_like "it removes the person associations"
   end
 
   context "of remote person" do

spec/integration/federation/receive_federation_messages_spec.rb

@@ -14,18 +14,19 @@ describe "Receive federation messages feature" do
   context "with public receive" do
     let(:recipient) { nil }
 
+    context "account deletion" do
       it "receives account deletion correctly" do
-      post_message(generate_payload(DiasporaFederation::Entities::AccountDeletion.new(diaspora_id: sender_id), sender))
+        post_message(generate_payload(DiasporaFederation::Entities::AccountDeletion.new(author: sender_id), sender))
 
-      expect(AccountDeletion.exists?(diaspora_handle: sender_id)).to be_truthy
+        expect(AccountDeletion.exists?(person: sender.person)).to be_truthy
       end
 
-    it "rejects account deletion with wrong diaspora_id" do
+      it "rejects account deletion with wrong author" do
         delete_id = Fabricate.sequence(:diaspora_id)
-      post_message(generate_payload(DiasporaFederation::Entities::AccountDeletion.new(diaspora_id: delete_id), sender))
-
-      expect(AccountDeletion.exists?(diaspora_handle: delete_id)).to be_falsey
-      expect(AccountDeletion.exists?(diaspora_handle: sender_id)).to be_falsey
+        expect {
+          post_message(generate_payload(DiasporaFederation::Entities::AccountDeletion.new(author: delete_id), sender))
+        }.not_to change(AccountDeletion, :count)
+      end
     end
 
     context "reshare" do

spec/lib/account_deleter_spec.rb

@@ -4,28 +4,28 @@
 
 describe AccountDeleter do
   before do
-    @account_deletion = AccountDeleter.new(bob.person.diaspora_handle)
+    @account_deletion = AccountDeleter.new(bob.person)
     @account_deletion.user = bob
   end
 
   it "attaches the user" do
-    expect(AccountDeleter.new(bob.person.diaspora_handle).user).to eq(bob)
-    expect(AccountDeleter.new(remote_raphael.diaspora_handle).user).to eq(nil)
+    expect(AccountDeleter.new(bob.person).user).to eq(bob)
+    expect(AccountDeleter.new(remote_raphael).user).to eq(nil)
   end
 
   describe '#perform' do
-    user_removal_methods = %i(
+    user_removal_methods = %i[
       delete_standard_user_associations
       remove_share_visibilities_on_contacts_posts
-      disconnect_contacts tombstone_user
-    )
+      tombstone_user
+    ]
 
-    person_removal_methods = %i(
+    person_removal_methods = %i[
       delete_contacts_of_me
       delete_standard_person_associations
       tombstone_person_and_profile
       remove_conversation_visibilities
-    )
+    ]
 
     context "user deletion" do
       after do
@@ -42,7 +42,7 @@ describe AccountDeleter do
 
     context "profile deletion" do
       before do
-        @profile_deletion = AccountDeleter.new(remote_raphael.diaspora_handle)
+        @profile_deletion = AccountDeleter.new(remote_raphael)
         @profile = remote_raphael.profile
       end
 
@@ -57,7 +57,7 @@ describe AccountDeleter do
 
     context "person deletion" do
       before do
-        @person_deletion = AccountDeleter.new(remote_raphael.diaspora_handle)
+        @person_deletion = AccountDeleter.new(remote_raphael)
       end
 
       after do
@@ -109,13 +109,6 @@ describe AccountDeleter do
   end
 
   context 'person associations' do
-    describe '#disconnect_contacts' do
-      it "deletes all of user's contacts" do
-        expect(bob.contacts).to receive(:destroy_all)
-        @account_deletion.disconnect_contacts
-      end
-    end
-
     describe '#delete_contacts_of_me' do
       it 'deletes all the local contact objects where deleted account is the person' do
         contacts = double

spec/lib/diaspora/federation/receive_spec.rb

@@ -8,9 +8,7 @@ describe Diaspora::Federation::Receive do
     it "saves the account deletion" do
       Diaspora::Federation::Receive.account_deletion(account_deletion_entity)
 
-      account_deletion = AccountDeletion.find_by!(diaspora_handle: sender.diaspora_handle)
-
-      expect(account_deletion.person).to eq(sender)
+      expect(AccountDeletion.exists?(person: sender)).to be_truthy
     end
   end
 

spec/models/account_deletion_spec.rb

@@ -16,7 +16,7 @@ describe AccountDeletion, type: :model do
 
   describe "#perform!" do
     it "creates a deleter" do
-      expect(AccountDeleter).to receive(:new).with(alice.person.diaspora_handle).and_return(double(perform!: true))
+      expect(AccountDeleter).to receive(:new).with(alice.person).and_return(double(perform!: true))
       account_deletion.perform!
     end