From 7ab53f221a3d66a0bbc94b36d847b7bcf62fbd16 Mon Sep 17 00:00:00 2001
From: abcang <abcang1015@gmail.com>
Date: Mon, 1 Feb 2021 05:24:57 +0900
Subject: Improved performance of notification preloading (#15640)

* Improved performance of notification preloading

* Remove Cacheable from Notification

* Fix test
---
 spec/controllers/application_controller_spec.rb |   4 -
 spec/models/notification_spec.rb                | 123 ++++++++++++++++++------
 2 files changed, 95 insertions(+), 32 deletions(-)

(limited to 'spec')

diff --git a/spec/controllers/application_controller_spec.rb b/spec/controllers/application_controller_spec.rb
index 63ae27a92..e73a08a0e 100644
--- a/spec/controllers/application_controller_spec.rb
+++ b/spec/controllers/application_controller_spec.rb
@@ -349,10 +349,6 @@ describe ApplicationController, type: :controller do
       expect(C.new.cache_collection(raw, Object)).to eq raw
     end
 
-    context 'Notification' do
-      include_examples 'cacheable', :notification, Notification
-    end
-
     context 'Status' do
       include_examples 'cacheable', :status, Status
     end
diff --git a/spec/models/notification_spec.rb b/spec/models/notification_spec.rb
index 0a045904b..1e9e45d8d 100644
--- a/spec/models/notification_spec.rb
+++ b/spec/models/notification_spec.rb
@@ -56,47 +56,114 @@ RSpec.describe Notification, type: :model do
     end
   end
 
-  describe '.reload_stale_associations!' do
-    context 'account_ids are empty' do
-      let(:cached_items) { [] }
+  describe '.preload_cache_collection_target_statuses' do
+    subject do
+      described_class.preload_cache_collection_target_statuses(notifications) do |target_statuses|
+        # preload account for testing instead of using cache_collection
+        Status.preload(:account).where(id: target_statuses.map(&:id))
+      end
+    end
 
-      subject { described_class.reload_stale_associations!(cached_items) }
+    context 'notifications are empty' do
+      let(:notifications) { [] }
 
-      it 'returns nil' do
-        is_expected.to be nil
+      it 'returns []' do
+        is_expected.to eq []
       end
     end
 
-    context 'account_ids are present' do
+    context 'notifications are present' do
       before do
-        allow(accounts_with_ids).to receive(:[]).with(stale_account1.id).and_return(account1)
-        allow(accounts_with_ids).to receive(:[]).with(stale_account2.id).and_return(account2)
-        allow(Account).to receive_message_chain(:where, :includes, :index_by).and_return(accounts_with_ids)
+        notifications.each(&:reload)
       end
 
-      let(:cached_items) do
+      let(:mention) { Fabricate(:mention) }
+      let(:status) { Fabricate(:status) }
+      let(:reblog) { Fabricate(:status, reblog: Fabricate(:status)) }
+      let(:follow) { Fabricate(:follow) }
+      let(:follow_request) { Fabricate(:follow_request) }
+      let(:favourite) { Fabricate(:favourite) }
+      let(:poll) { Fabricate(:poll) }
+
+      let(:notifications) do
         [
-          Fabricate(:notification, activity: Fabricate(:status)),
-          Fabricate(:notification, activity: Fabricate(:follow)),
+          Fabricate(:notification, type: :mention, activity: mention),
+          Fabricate(:notification, type: :status, activity: status),
+          Fabricate(:notification, type: :reblog, activity: reblog),
+          Fabricate(:notification, type: :follow, activity: follow),
+          Fabricate(:notification, type: :follow_request, activity: follow_request),
+          Fabricate(:notification, type: :favourite, activity: favourite),
+          Fabricate(:notification, type: :poll, activity: poll),
         ]
       end
 
-      let(:stale_account1) { cached_items[0].from_account }
-      let(:stale_account2) { cached_items[1].from_account }
-
-      let(:account1) { Fabricate(:account) }
-      let(:account2) { Fabricate(:account) }
-
-      let(:accounts_with_ids) { { account1.id => account1, account2.id => account2 } }
-
-      it 'reloads associations' do
-        expect(cached_items[0].from_account).to be stale_account1
-        expect(cached_items[1].from_account).to be stale_account2
-
-        described_class.reload_stale_associations!(cached_items)
+      it 'preloads target status' do
+        # mention
+        expect(subject[0].type).to eq :mention
+        expect(subject[0].association(:mention)).to be_loaded
+        expect(subject[0].mention.association(:status)).to be_loaded
+
+        # status
+        expect(subject[1].type).to eq :status
+        expect(subject[1].association(:status)).to be_loaded
+
+        # reblog
+        expect(subject[2].type).to eq :reblog
+        expect(subject[2].association(:status)).to be_loaded
+        expect(subject[2].status.association(:reblog)).to be_loaded
+
+        # follow: nothing
+        expect(subject[3].type).to eq :follow
+        expect(subject[3].target_status).to be_nil
+
+        # follow_request: nothing
+        expect(subject[4].type).to eq :follow_request
+        expect(subject[4].target_status).to be_nil
+
+        # favourite
+        expect(subject[5].type).to eq :favourite
+        expect(subject[5].association(:favourite)).to be_loaded
+        expect(subject[5].favourite.association(:status)).to be_loaded
+
+        # poll
+        expect(subject[6].type).to eq :poll
+        expect(subject[6].association(:poll)).to be_loaded
+        expect(subject[6].poll.association(:status)).to be_loaded
+      end
 
-        expect(cached_items[0].from_account).to be account1
-        expect(cached_items[1].from_account).to be account2
+      it 'replaces to cached status' do
+        # mention
+        expect(subject[0].type).to eq :mention
+        expect(subject[0].target_status.association(:account)).to be_loaded
+        expect(subject[0].target_status).to eq mention.status
+
+        # status
+        expect(subject[1].type).to eq :status
+        expect(subject[1].target_status.association(:account)).to be_loaded
+        expect(subject[1].target_status).to eq status
+
+        # reblog
+        expect(subject[2].type).to eq :reblog
+        expect(subject[2].target_status.association(:account)).to be_loaded
+        expect(subject[2].target_status).to eq reblog.reblog
+
+        # follow: nothing
+        expect(subject[3].type).to eq :follow
+        expect(subject[3].target_status).to be_nil
+
+        # follow_request: nothing
+        expect(subject[4].type).to eq :follow_request
+        expect(subject[4].target_status).to be_nil
+
+        # favourite
+        expect(subject[5].type).to eq :favourite
+        expect(subject[5].target_status.association(:account)).to be_loaded
+        expect(subject[5].target_status).to eq favourite.status
+
+        # poll
+        expect(subject[6].type).to eq :poll
+        expect(subject[6].target_status.association(:account)).to be_loaded
+        expect(subject[6].target_status).to eq poll.status
       end
     end
   end
-- 
cgit 


From c8d11b8bdb97a1a2f8aaf5deca5f1c6c7c0d2688 Mon Sep 17 00:00:00 2001
From: Shubhendra Singh Chauhan <withshubh@gmail.com>
Date: Mon, 1 Feb 2021 01:56:09 +0530
Subject: Fixed code quality issues (#15541)

* Added .deepsource.toml

* Removed bad use of `alias`

* Fixed operand order in the binary expression

* Prefixed unused method arguments with an underscore

* Replaced the old OpenSSL algorithmic constants with the newer strings initializers.

* Removed unnecessary UTF-8 encoding comment
---
 .deepsource.toml                                   | 18 ++++++++++++++++++
 config/initializers/fast_blank.rb                  |  2 +-
 config/initializers/simple_form.rb                 |  4 ++--
 config/initializers/twitter_regex.rb               |  2 +-
 lib/json_ld/identity.rb                            |  1 -
 lib/json_ld/security.rb                            |  1 -
 lib/mastodon/email_domain_blocks_cli.rb            |  2 +-
 lib/paperclip/color_extractor.rb                   |  4 ++--
 spec/lib/activitypub/linked_data_signature_spec.rb |  2 +-
 9 files changed, 26 insertions(+), 10 deletions(-)
 create mode 100644 .deepsource.toml

(limited to 'spec')

diff --git a/.deepsource.toml b/.deepsource.toml
new file mode 100644
index 000000000..73d38901c
--- /dev/null
+++ b/.deepsource.toml
@@ -0,0 +1,18 @@
+version = 1
+
+test_patterns = ["/app/javascript/mastodon/**/__tests__/**"]
+
+[[analyzers]]
+name = "ruby"
+enabled = true
+
+[[analyzers]]
+name = "javascript"
+enabled = true
+
+  [analyzers.meta]
+  environment = [
+    "nodejs",
+    "browser",
+    "jest"
+  ]
diff --git a/config/initializers/fast_blank.rb b/config/initializers/fast_blank.rb
index 174ea7664..f0b7cac78 100644
--- a/config/initializers/fast_blank.rb
+++ b/config/initializers/fast_blank.rb
@@ -1,5 +1,5 @@
 if String.method_defined?(:blank_as?)
   class String
-    alias_method :blank?, :blank_as?
+    alias blank? blank_as?
   end
 end
diff --git a/config/initializers/simple_form.rb b/config/initializers/simple_form.rb
index 3dc48ef08..3a2097d2f 100644
--- a/config/initializers/simple_form.rb
+++ b/config/initializers/simple_form.rb
@@ -1,7 +1,7 @@
 # Use this setup block to configure all options available in SimpleForm.
 
 module AppendComponent
-  def append(wrapper_options = nil)
+  def append(_wrapper_options = nil)
     @append ||= begin
       options[:append].to_s.html_safe if options[:append].present?
     end
@@ -9,7 +9,7 @@ module AppendComponent
 end
 
 module RecommendedComponent
-  def recommended(wrapper_options = nil)
+  def recommended(_wrapper_options = nil)
     return unless options[:recommended]
     options[:label_text] = ->(raw_label_text, _required_label_text, _label_present) { safe_join([raw_label_text, ' ', content_tag(:span, I18n.t('simple_form.recommended'), class: 'recommended')]) }
     nil
diff --git a/config/initializers/twitter_regex.rb b/config/initializers/twitter_regex.rb
index 7f99a0005..aca85dd43 100644
--- a/config/initializers/twitter_regex.rb
+++ b/config/initializers/twitter_regex.rb
@@ -75,7 +75,7 @@ module Twitter
     # XMPP or magnet URIs an empty array will be returned.
     #
     # If a block is given then it will be called for each XMPP URI.
-    def extract_extra_uris_with_indices(text, options = {}) # :yields: uri, start, end
+    def extract_extra_uris_with_indices(text, _options = {}) # :yields: uri, start, end
       return [] unless text && text.index(":")
       urls = []
 
diff --git a/lib/json_ld/identity.rb b/lib/json_ld/identity.rb
index 4fb3f8e9d..f41899150 100644
--- a/lib/json_ld/identity.rb
+++ b/lib/json_ld/identity.rb
@@ -1,4 +1,3 @@
-# -*- encoding: utf-8 -*-
 # frozen_string_literal: true
 # This file generated automatically from http://w3id.org/identity/v1
 require 'json/ld'
diff --git a/lib/json_ld/security.rb b/lib/json_ld/security.rb
index a6fbce95f..ef5391340 100644
--- a/lib/json_ld/security.rb
+++ b/lib/json_ld/security.rb
@@ -1,4 +1,3 @@
-# -*- encoding: utf-8 -*-
 # frozen_string_literal: true
 # This file generated automatically from http://w3id.org/security/v1
 require 'json/ld'
diff --git a/lib/mastodon/email_domain_blocks_cli.rb b/lib/mastodon/email_domain_blocks_cli.rb
index 55a637d68..f79df302a 100644
--- a/lib/mastodon/email_domain_blocks_cli.rb
+++ b/lib/mastodon/email_domain_blocks_cli.rb
@@ -113,7 +113,7 @@ module Mastodon
         result = entry.destroy
 
         if result
-          processed += 1 + children_count
+          processed += children_count + 1
         else
           say("#{domain} could not be unblocked.", :red)
           failed += 1
diff --git a/lib/paperclip/color_extractor.rb b/lib/paperclip/color_extractor.rb
index a70a3d21f..d3b8e1022 100644
--- a/lib/paperclip/color_extractor.rb
+++ b/lib/paperclip/color_extractor.rb
@@ -55,7 +55,7 @@ module Paperclip
       # If we don't have enough colors for accent and foreground, generate
       # new ones by manipulating the background color
       (2 - foreground_colors.size).times do |i|
-        foreground_colors << lighten_or_darken(background_color, 35 + (15 * i))
+        foreground_colors << lighten_or_darken(background_color, 35 + (i * 15))
       end
 
       # We want the color with the highest contrast to background to be the foreground one,
@@ -147,7 +147,7 @@ module Paperclip
         g = l.to_f
         b = l.to_f # achromatic
       else
-        q = l < 0.5 ? l * (1 + s) : l + s - l * s
+        q = l < 0.5 ? l * (s + 1) : l + s - l * s
         p = 2 * l - q
         r = hue_to_rgb(p, q, h + 1 / 3.0)
         g = hue_to_rgb(p, q, h)
diff --git a/spec/lib/activitypub/linked_data_signature_spec.rb b/spec/lib/activitypub/linked_data_signature_spec.rb
index 1f413eec9..2222c46fb 100644
--- a/spec/lib/activitypub/linked_data_signature_spec.rb
+++ b/spec/lib/activitypub/linked_data_signature_spec.rb
@@ -81,6 +81,6 @@ RSpec.describe ActivityPub::LinkedDataSignature do
     options_hash   = Digest::SHA256.hexdigest(canonicalize(options.merge('@context' => ActivityPub::LinkedDataSignature::CONTEXT)))
     document_hash  = Digest::SHA256.hexdigest(canonicalize(document))
     to_be_verified = options_hash + document_hash
-    Base64.strict_encode64(from_account.keypair.sign(OpenSSL::Digest::SHA256.new, to_be_verified))
+    Base64.strict_encode64(from_account.keypair.sign(OpenSSL::Digest.new('SHA256'), to_be_verified))
   end
 end
-- 
cgit 


From a044ddac5b3c0e2012c0e91bfbc07aa256a0684f Mon Sep 17 00:00:00 2001
From: ThibG <thib@sitedethib.com>
Date: Tue, 2 Feb 2021 14:49:57 +0100
Subject: Fix race conditions on account migration creation (#15597)

* Atomically check for processing lock in Move handler

* Prevent race condition when creating account migrations

Fixes #15595

* Add tests

Co-authored-by: Claire <claire.github-309c@sitedethib.com>
---
 app/lib/activitypub/activity/move.rb               | 11 +--
 app/models/account_migration.rb                    | 14 ++-
 .../settings/migrations_controller_spec.rb         | 37 +++++++-
 spec/lib/activitypub/activity/move_spec.rb         | 99 +++++++++++++++++-----
 4 files changed, 127 insertions(+), 34 deletions(-)

(limited to 'spec')

diff --git a/app/lib/activitypub/activity/move.rb b/app/lib/activitypub/activity/move.rb
index 7e073f64d..8576ceccd 100644
--- a/app/lib/activitypub/activity/move.rb
+++ b/app/lib/activitypub/activity/move.rb
@@ -4,9 +4,8 @@ class ActivityPub::Activity::Move < ActivityPub::Activity
   PROCESSING_COOLDOWN = 7.days.seconds
 
   def perform
-    return if origin_account.uri != object_uri || processed?
-
-    mark_as_processing!
+    return if origin_account.uri != object_uri
+    return unless mark_as_processing!
 
     target_account = ActivityPub::FetchRemoteAccountService.new.call(target_uri)
 
@@ -35,12 +34,8 @@ class ActivityPub::Activity::Move < ActivityPub::Activity
     value_or_id(@json['target'])
   end
 
-  def processed?
-    redis.exists?("move_in_progress:#{@account.id}")
-  end
-
   def mark_as_processing!
-    redis.setex("move_in_progress:#{@account.id}", PROCESSING_COOLDOWN, true)
+    redis.set("move_in_progress:#{@account.id}", true, nx: true, ex: PROCESSING_COOLDOWN)
   end
 
   def unmark_as_processing!
diff --git a/app/models/account_migration.rb b/app/models/account_migration.rb
index 4fae98ed7..ded32c9c6 100644
--- a/app/models/account_migration.rb
+++ b/app/models/account_migration.rb
@@ -14,6 +14,8 @@
 #
 
 class AccountMigration < ApplicationRecord
+  include Redisable
+
   COOLDOWN_PERIOD = 30.days.freeze
 
   belongs_to :account
@@ -39,7 +41,13 @@ class AccountMigration < ApplicationRecord
 
     return false unless errors.empty?
 
-    save
+    RedisLock.acquire(lock_options) do |lock|
+      if lock.acquired?
+        save
+      else
+        raise Mastodon::RaceConditionError
+      end
+    end
   end
 
   def cooldown_at
@@ -75,4 +83,8 @@ class AccountMigration < ApplicationRecord
   def validate_migration_cooldown
     errors.add(:base, I18n.t('migrations.errors.on_cooldown')) if account.migrations.within_cooldown.exists?
   end
+
+  def lock_options
+    { redis: redis, key: "account_migration:#{account.id}" }
+  end
 end
diff --git a/spec/controllers/settings/migrations_controller_spec.rb b/spec/controllers/settings/migrations_controller_spec.rb
index 36e4ba86e..048d9de8d 100644
--- a/spec/controllers/settings/migrations_controller_spec.rb
+++ b/spec/controllers/settings/migrations_controller_spec.rb
@@ -51,7 +51,7 @@ describe Settings::MigrationsController do
       it_behaves_like 'authenticate user'
     end
 
-    context 'when user is sign in' do
+    context 'when user is signed in' do
       subject { post :create, params: { account_migration: { acct: acct, current_password: '12345678' } } }
 
       let(:user) { Fabricate(:user, password: '12345678') }
@@ -67,12 +67,45 @@ describe Settings::MigrationsController do
         end
       end
 
-      context 'when acct is a current account' do
+      context 'when acct is the current account' do
         let(:acct) { user.account }
 
         it 'renders show' do
           is_expected.to render_template :show
         end
+
+        it 'does not update the moved account' do
+          expect(user.account.reload.moved_to_account_id).to be_nil
+        end
+      end
+
+      context 'when target account does not reference the account being moved from' do
+        let(:acct) { Fabricate(:account, also_known_as: []) }
+
+        it 'renders show' do
+          is_expected.to render_template :show
+        end
+
+        it 'does not update the moved account' do
+          expect(user.account.reload.moved_to_account_id).to be_nil
+        end
+      end
+
+      context 'when a recent migration already exists ' do
+        let(:acct) { Fabricate(:account, also_known_as: [ActivityPub::TagManager.instance.uri_for(user.account)]) }
+
+        before do
+          moved_to = Fabricate(:account, also_known_as: [ActivityPub::TagManager.instance.uri_for(user.account)])
+          user.account.migrations.create!(acct: moved_to.acct)
+        end
+
+        it 'renders show' do
+          is_expected.to render_template :show
+        end
+
+        it 'does not update the moved account' do
+          expect(user.account.reload.moved_to_account_id).to be_nil
+        end
       end
     end
   end
diff --git a/spec/lib/activitypub/activity/move_spec.rb b/spec/lib/activitypub/activity/move_spec.rb
index 3574f273a..2d1d276c5 100644
--- a/spec/lib/activitypub/activity/move_spec.rb
+++ b/spec/lib/activitypub/activity/move_spec.rb
@@ -1,23 +1,11 @@
 require 'rails_helper'
 
 RSpec.describe ActivityPub::Activity::Move do
-  let(:follower)    { Fabricate(:account) }
-  let(:old_account) { Fabricate(:account) }
-  let(:new_account) { Fabricate(:account) }
-
-  before do
-    follower.follow!(old_account)
-
-    old_account.update!(uri: 'https://example.org/alice', domain: 'example.org', protocol: :activitypub, inbox_url: 'https://example.org/inbox')
-    new_account.update!(uri: 'https://example.com/alice', domain: 'example.com', protocol: :activitypub, inbox_url: 'https://example.com/inbox', also_known_as: [old_account.uri])
-
-    stub_request(:post, 'https://example.org/inbox').to_return(status: 200)
-    stub_request(:post, 'https://example.com/inbox').to_return(status: 200)
-
-    service_stub = double
-    allow(ActivityPub::FetchRemoteAccountService).to receive(:new).and_return(service_stub)
-    allow(service_stub).to receive(:call).and_return(new_account)
-  end
+  let(:follower)         { Fabricate(:account) }
+  let(:old_account)      { Fabricate(:account, uri: 'https://example.org/alice', domain: 'example.org', protocol: :activitypub, inbox_url: 'https://example.org/inbox') }
+  let(:new_account)      { Fabricate(:account, uri: 'https://example.com/alice', domain: 'example.com', protocol: :activitypub, inbox_url: 'https://example.com/inbox', also_known_as: also_known_as) }
+  let(:also_known_as)    { [old_account.uri] }
+  let(:returned_account) { new_account }
 
   let(:json) do
     {
@@ -30,6 +18,17 @@ RSpec.describe ActivityPub::Activity::Move do
     }.with_indifferent_access
   end
 
+  before do
+    follower.follow!(old_account)
+
+    stub_request(:post, old_account.inbox_url).to_return(status: 200)
+    stub_request(:post, new_account.inbox_url).to_return(status: 200)
+
+    service_stub = double
+    allow(ActivityPub::FetchRemoteAccountService).to receive(:new).and_return(service_stub)
+    allow(service_stub).to receive(:call).and_return(returned_account)
+  end
+
   describe '#perform' do
     subject { described_class.new(json, old_account) }
 
@@ -37,16 +36,70 @@ RSpec.describe ActivityPub::Activity::Move do
       subject.perform
     end
 
-    it 'sets moved account on old account' do
-      expect(old_account.reload.moved_to_account_id).to eq new_account.id
+    context 'when all conditions are met' do
+      it 'sets moved account on old account' do
+        expect(old_account.reload.moved_to_account_id).to eq new_account.id
+      end
+
+      it 'makes followers unfollow old account' do
+        expect(follower.following?(old_account)).to be false
+      end
+
+      it 'makes followers follow-request the new account' do
+        expect(follower.requested?(new_account)).to be true
+      end
     end
 
-    it 'makes followers unfollow old account' do
-      expect(follower.following?(old_account)).to be false
+    context "when the new account can't be resolved" do
+      let(:returned_account) { nil }
+
+      it 'does not set moved account on old account' do
+        expect(old_account.reload.moved_to_account_id).to be_nil
+      end
+
+      it 'does not make followers unfollow old account' do
+        expect(follower.following?(old_account)).to be true
+      end
+
+      it 'does not make followers follow-request the new account' do
+        expect(follower.requested?(new_account)).to be false
+      end
     end
 
-    it 'makes followers follow-request the new account' do
-      expect(follower.requested?(new_account)).to be true
+    context 'when the new account does not references the old account' do
+      let(:also_known_as) { [] }
+
+      it 'does not set moved account on old account' do
+        expect(old_account.reload.moved_to_account_id).to be_nil
+      end
+
+      it 'does not make followers unfollow old account' do
+        expect(follower.following?(old_account)).to be true
+      end
+
+      it 'does not make followers follow-request the new account' do
+        expect(follower.requested?(new_account)).to be false
+      end
+    end
+
+    context 'when a Move has been recently processed' do
+      around do |example|
+        Redis.current.set("move_in_progress:#{old_account.id}", true, nx: true, ex: 7.days.seconds)
+        example.run
+        Redis.current.del("move_in_progress:#{old_account.id}")
+      end
+
+      it 'does not set moved account on old account' do
+        expect(old_account.reload.moved_to_account_id).to be_nil
+      end
+
+      it 'does not make followers unfollow old account' do
+        expect(follower.following?(old_account)).to be true
+      end
+
+      it 'does not make followers follow-request the new account' do
+        expect(follower.requested?(new_account)).to be false
+      end
     end
   end
 end
-- 
cgit