Commit f3e682c0 authored by Rémy Coutable's avatar Rémy Coutable

Merge branch 'request-store-wrap' into 'master'

Add RequestCache to cache via RequestStore

See merge request !12920
parents f4826455 f69c0f80
class Commit
extend ActiveModel::Naming
extend Gitlab::Cache::RequestCache
include ActiveModel::Conversion
include Noteable
......@@ -169,19 +170,9 @@ class Commit
end
def author
if RequestStore.active?
key = "commit_author:#{author_email.downcase}"
# nil is a valid value since no author may exist in the system
if RequestStore.store.key?(key)
@author = RequestStore.store[key]
else
@author = find_author_by_any_email
RequestStore.store[key] = @author
end
else
@author ||= find_author_by_any_email
end
User.find_by_any_email(author_email.downcase)
end
request_cache(:author) { author_email.downcase }
def committer
@committer ||= User.find_by_any_email(committer_email.downcase)
......@@ -368,10 +359,6 @@ class Commit
end
end
def find_author_by_any_email
User.find_by_any_email(author_email.downcase)
end
def repo_changes
changes = { added: [], modified: [], removed: [] }
......
......@@ -190,7 +190,7 @@ class Note < ActiveRecord::Base
# override to return commits, which are not active record
def noteable
if for_commit?
project.commit(commit_id)
@commit ||= project.commit(commit_id)
else
super
end
......
---
title: Add RequestCache which makes caching with RequestStore easier
merge_request: 12920
author:
module Gitlab
module Cache
# This module provides a simple way to cache values in RequestStore,
# and the cache key would be based on the class name, method name,
# optionally customized instance level values, optionally customized
# method level values, and optional method arguments.
#
# A simple example:
#
# class UserAccess
# extend Gitlab::Cache::RequestCache
#
# request_cache_key do
# [user&.id, project&.id]
# end
#
# request_cache def can_push_to_branch?(ref)
# # ...
# end
# end
#
# This way, the result of `can_push_to_branch?` would be cached in
# `RequestStore.store` based on the cache key. If RequestStore is not
# currently active, then it would be stored in a hash saved in an
# instance variable, so the cache logic would be the same.
# Here's another example using customized method level values:
#
# class Commit
# extend Gitlab::Cache::RequestCache
#
# def author
# User.find_by_any_email(author_email.downcase)
# end
# request_cache(:author) { author_email.downcase }
# end
#
# So that we could have different strategies for different methods
#
module RequestCache
def self.extended(klass)
return if klass < self
extension = Module.new
klass.const_set(:RequestCacheExtension, extension)
klass.prepend(extension)
end
def request_cache_key(&block)
if block_given?
@request_cache_key = block
else
@request_cache_key
end
end
def request_cache(method_name, &method_key_block)
const_get(:RequestCacheExtension).module_eval do
cache_key_method_name = "#{method_name}_cache_key"
define_method(method_name) do |*args|
store =
if RequestStore.active?
RequestStore.store
else
ivar_name = # ! and ? cannot be used as ivar name
"@cache_#{method_name.to_s.tr('!?', "\u2605\u2606")}"
instance_variable_get(ivar_name) ||
instance_variable_set(ivar_name, {})
end
key = __send__(cache_key_method_name, args)
store.fetch(key) { store[key] = super(*args) }
end
define_method(cache_key_method_name) do |args|
klass = self.class
instance_key = instance_exec(&klass.request_cache_key) if
klass.request_cache_key
method_key = instance_exec(&method_key_block) if method_key_block
[klass.name, method_name, *instance_key, *method_key, *args]
.join(':')
end
private cache_key_method_name
end
end
end
end
end
module Gitlab
class UserAccess
extend Gitlab::Cache::RequestCache
request_cache_key do
[user&.id, project&.id]
end
attr_reader :user, :project
def initialize(user, project: nil)
......@@ -28,7 +34,7 @@ module Gitlab
true
end
def can_create_tag?(ref)
request_cache def can_create_tag?(ref)
return false unless can_access_git?
if ProtectedTag.protected?(project, ref)
......@@ -38,7 +44,7 @@ module Gitlab
end
end
def can_delete_branch?(ref)
request_cache def can_delete_branch?(ref)
return false unless can_access_git?
if ProtectedBranch.protected?(project, ref)
......@@ -48,7 +54,7 @@ module Gitlab
end
end
def can_push_to_branch?(ref)
request_cache def can_push_to_branch?(ref)
return false unless can_access_git?
if ProtectedBranch.protected?(project, ref)
......@@ -60,7 +66,7 @@ module Gitlab
end
end
def can_merge_to_branch?(ref)
request_cache def can_merge_to_branch?(ref)
return false unless can_access_git?
if ProtectedBranch.protected?(project, ref)
......
......@@ -4,14 +4,19 @@ FactoryGirl.define do
factory :commit do
git_commit RepoHelpers.sample_commit
project factory: :empty_project
author { build(:author) }
initialize_with do
new(git_commit, project)
end
after(:build) do |commit|
allow(commit).to receive(:author).and_return build(:author)
end
trait :without_author do
author nil
after(:build) do |commit|
allow(commit).to receive(:author).and_return nil
end
end
end
end
......@@ -54,7 +54,8 @@ feature 'Member autocomplete', :js do
let(:note) { create(:note_on_commit, project: project, commit_id: project.commit.id) }
before do
allow_any_instance_of(Commit).to receive(:author).and_return(author)
allow(User).to receive(:find_by_any_email)
.with(noteable.author_email.downcase).and_return(author)
visit project_commit_path(project, noteable)
end
......
require 'spec_helper'
describe Gitlab::Cache::RequestCache do
let(:klass) do
Class.new do
extend Gitlab::Cache::RequestCache
attr_accessor :id, :name, :result, :extra
def self.name
'ExpensiveAlgorithm'
end
def initialize(id, name, result, extra = nil)
self.id = id
self.name = name
self.result = result
self.extra = nil
end
request_cache def compute(arg)
result << arg
end
request_cache def repute(arg)
result << arg
end
def dispute(arg)
result << arg
end
request_cache(:dispute) { extra }
end
end
let(:algorithm) { klass.new('id', 'name', []) }
shared_examples 'cache for the same instance' do
it 'does not compute twice for the same argument' do
algorithm.compute(true)
result = algorithm.compute(true)
expect(result).to eq([true])
end
it 'computes twice for the different argument' do
algorithm.compute(true)
result = algorithm.compute(false)
expect(result).to eq([true, false])
end
it 'computes twice for the different class name' do
algorithm.compute(true)
allow(klass).to receive(:name).and_return('CheapAlgo')
result = algorithm.compute(true)
expect(result).to eq([true, true])
end
it 'computes twice for the different method' do
algorithm.compute(true)
result = algorithm.repute(true)
expect(result).to eq([true, true])
end
context 'when request_cache_key is provided' do
before do
klass.request_cache_key do
[id, name]
end
end
it 'computes twice for the different keys, id' do
algorithm.compute(true)
algorithm.id = 'ad'
result = algorithm.compute(true)
expect(result).to eq([true, true])
end
it 'computes twice for the different keys, name' do
algorithm.compute(true)
algorithm.name = 'same'
result = algorithm.compute(true)
expect(result).to eq([true, true])
end
it 'uses extra method cache key if provided' do
algorithm.dispute(true) # miss
algorithm.extra = true
algorithm.dispute(true) # miss
result = algorithm.dispute(true) # hit
expect(result).to eq([true, true])
end
end
end
context 'when RequestStore is active', :request_store do
it_behaves_like 'cache for the same instance'
it 'computes once for different instances when keys are the same' do
algorithm.compute(true)
result = klass.new('id', 'name', algorithm.result).compute(true)
expect(result).to eq([true])
end
it 'computes twice if RequestStore starts over' do
algorithm.compute(true)
RequestStore.end!
RequestStore.clear!
RequestStore.begin!
result = algorithm.compute(true)
expect(result).to eq([true, true])
end
end
context 'when RequestStore is inactive' do
it_behaves_like 'cache for the same instance'
it 'computes twice for different instances even if keys are the same' do
algorithm.compute(true)
result = klass.new('id', 'name', algorithm.result).compute(true)
expect(result).to eq([true, true])
end
end
end
......@@ -19,17 +19,15 @@ describe Commit, models: true do
expect(commit.author).to eq(user)
end
it 'caches the author' do
allow(RequestStore).to receive(:active?).and_return(true)
it 'caches the author', :request_store do
user = create(:user, email: commit.author_email)
expect_any_instance_of(Commit).to receive(:find_author_by_any_email).and_call_original
expect(User).to receive(:find_by_any_email).and_call_original
expect(commit.author).to eq(user)
key = "commit_author:#{commit.author_email}"
key = "Commit:author:#{commit.author_email.downcase}"
expect(RequestStore.store[key]).to eq(user)
expect(commit.author).to eq(user)
RequestStore.store.clear
end
end
......
......@@ -383,7 +383,7 @@ describe NotificationService, services: true do
before do
build_team(note.project)
reset_delivered_emails!
allow_any_instance_of(Commit).to receive(:author).and_return(@u_committer)
allow(note.noteable).to receive(:author).and_return(@u_committer)
update_custom_notification(:new_note, @u_guest_custom, resource: project)
update_custom_notification(:new_note, @u_custom_global)
end
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment