diff --git a/Changelog.md b/Changelog.md index b3209cb59..86ca0f7da 100644 --- a/Changelog.md +++ b/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 diff --git a/app/models/account_deletion.rb b/app/models/account_deletion.rb index cc9445310..b929279bb 100644 --- a/app/models/account_deletion.rb +++ b/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 diff --git a/app/models/user.rb b/app/models/user.rb index 629649d4e..3d98b5eb3 100644 --- a/app/models/user.rb +++ b/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? diff --git a/db/migrate/20170813141631_cleanup_account_deletions_and_add_unique_index.rb b/db/migrate/20170813141631_cleanup_account_deletions_and_add_unique_index.rb new file mode 100644 index 000000000..6d188278d --- /dev/null +++ b/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 diff --git a/db/migrate/20170813153048_add_missing_indexes.rb b/db/migrate/20170813153048_add_missing_indexes.rb new file mode 100644 index 000000000..9da177a30 --- /dev/null +++ b/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 diff --git a/db/migrate/20170813160104_cleanup_aspects_and_add_unique_index.rb b/db/migrate/20170813160104_cleanup_aspects_and_add_unique_index.rb new file mode 100644 index 000000000..324f43a09 --- /dev/null +++ b/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 diff --git a/db/migrate/20170813164435_add_missing_unique_indexes.rb b/db/migrate/20170813164435_add_missing_unique_indexes.rb new file mode 100644 index 000000000..4c853fbf1 --- /dev/null +++ b/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 diff --git a/lib/account_deleter.rb b/lib/account_deleter.rb index a466a3f2e..8172510a0 100644 --- a/lib/account_deleter.rb +++ b/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 diff --git a/lib/diaspora/federation/receive.rb b/lib/diaspora/federation/receive.rb index a3e8cadd4..7b1faea26 100644 --- a/lib/diaspora/federation/receive.rb +++ b/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) diff --git a/spec/factories.rb b/spec/factories.rb index d3fc152d8..02519966a 100644 --- a/spec/factories.rb +++ b/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 diff --git a/spec/integration/account_deletion_spec.rb b/spec/integration/account_deletion_spec.rb index 7c9346204..e9938c50c 100644 --- a/spec/integration/account_deletion_spec.rb +++ b/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 diff --git a/spec/integration/federation/receive_federation_messages_spec.rb b/spec/integration/federation/receive_federation_messages_spec.rb index 651c65150..f689d55d2 100644 --- a/spec/integration/federation/receive_federation_messages_spec.rb +++ b/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 } - it "receives account deletion correctly" do - post_message(generate_payload(DiasporaFederation::Entities::AccountDeletion.new(diaspora_id: sender_id), sender)) + context "account deletion" do + it "receives account deletion correctly" do + post_message(generate_payload(DiasporaFederation::Entities::AccountDeletion.new(author: sender_id), sender)) - expect(AccountDeletion.exists?(diaspora_handle: sender_id)).to be_truthy - end + expect(AccountDeletion.exists?(person: sender.person)).to be_truthy + end - it "rejects account deletion with wrong diaspora_id" 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 + it "rejects account deletion with wrong author" do + delete_id = Fabricate.sequence(:diaspora_id) + expect { + post_message(generate_payload(DiasporaFederation::Entities::AccountDeletion.new(author: delete_id), sender)) + }.not_to change(AccountDeletion, :count) + end end context "reshare" do diff --git a/spec/lib/account_deleter_spec.rb b/spec/lib/account_deleter_spec.rb index e67920157..46d431d72 100644 --- a/spec/lib/account_deleter_spec.rb +++ b/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 diff --git a/spec/lib/diaspora/federation/receive_spec.rb b/spec/lib/diaspora/federation/receive_spec.rb index 720e3c574..d8b0dcaa5 100644 --- a/spec/lib/diaspora/federation/receive_spec.rb +++ b/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 diff --git a/spec/models/account_deletion_spec.rb b/spec/models/account_deletion_spec.rb index 972435818..7fc069984 100644 --- a/spec/models/account_deletion_spec.rb +++ b/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