Skip to content

Commit 0a5cd67

Browse files
committed
identifiable error on cache miss
refs CNVS-26368 make sure we can find instances where we're unable to find accounts by throwing a specific error (rather than counting on ActiveRecord::RecordNotFound) for that case. TEST PLAN: 1) we should start seeing AccountCacheErrors showing up in Sentry Change-Id: I9ae05df6a7ac2ca921ace70f10eeae32f11b2e70 Reviewed-on: https://gerrit.instructure.com/70468 Reviewed-by: Cody Cutrer <cody@instructure.com> Tested-by: Jenkins Product-Review: Ethan Vizitei <evizitei@instructure.com> QA-Review: Ethan Vizitei <evizitei@instructure.com>
1 parent 86ee3aa commit 0a5cd67

2 files changed

Lines changed: 19 additions & 4 deletions

File tree

app/models/account.rb

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1101,11 +1101,17 @@ def self.#{key}(force_create = false)
11011101
def precache
11021102
end
11031103

1104+
class ::Canvas::AccountCacheError < StandardError; end
1105+
11041106
def self.find_cached(id)
11051107
birth_id = Shard.relative_id_for(id, Shard.current, Shard.birth)
11061108
Shard.birth.activate do
11071109
Rails.cache.fetch(account_lookup_cache_key(birth_id)) do
1108-
account = Account.find(birth_id)
1110+
begin
1111+
account = Account.find(birth_id)
1112+
rescue ActiveRecord::RecordNotFound => e
1113+
raise ::Canvas::AccountCacheError, e.message
1114+
end
11091115
account.precache
11101116
account
11111117
end
@@ -1119,7 +1125,7 @@ def self.get_special_account(special_account_type, default_account_name, force_c
11191125
special_account_id = special_account_ids[special_account_type] ||= Setting.get("#{special_account_type}_account_id", nil)
11201126
begin
11211127
account = special_accounts[special_account_type] = Account.find_cached(special_account_id) if special_account_id
1122-
rescue ActiveRecord::RecordNotFound
1128+
rescue ::Canvas::AccountCacheError
11231129
raise unless Rails.env.test?
11241130
end
11251131
end

spec/models/account_spec.rb

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1408,6 +1408,8 @@ def account_with_admin_and_restricted_user(account, restricted_role)
14081408
specs_require_sharding
14091409

14101410
describe ".find_cached" do
1411+
let(:nonsense_id){ 987654321 }
1412+
14111413
it "works relative to a different shard" do
14121414
@shard1.activate do
14131415
a = Account.create!
@@ -1416,8 +1418,15 @@ def account_with_admin_and_restricted_user(account, restricted_role)
14161418
end
14171419

14181420
it "errors if infrastructure fails and we can't see the account" do
1419-
nonsense_id = 987654321
1420-
expect{ Account.find_cached(nonsense_id) }.to raise_error(ActiveRecord::RecordNotFound)
1421+
expect{ Account.find_cached(nonsense_id) }.to raise_error(::Canvas::AccountCacheError)
1422+
end
1423+
1424+
it "includes the account id in the error message" do
1425+
begin
1426+
Account.find_cached(nonsense_id)
1427+
rescue ::Canvas::AccountCacheError => e
1428+
expect(e.message).to eq("Couldn't find Account with id=#{nonsense_id}")
1429+
end
14211430
end
14221431
end
14231432

0 commit comments

Comments
 (0)