Commit 3f4b3514 authored by Vasilii Iakliushin's avatar Vasilii Iakliushin

Fix caching for pagination headers

Contributes to https://gitlab.com/gitlab-org/gitlab/-/issues/331297

**Problem**

 We set pagination headers during the request time(see
 Gitlab::Pagination:::OffsetHeaderBuilder).

But when the request is cached, then we skip the headers' setup.

**Solution**

Save headers in the cache and restore them if necessary

Changelog: fixed
parent e430a1ed
......@@ -14,6 +14,12 @@ module API
race_condition_ttl: 5.seconds
}.freeze
# @return Integer
VERSION = 1
# @return [Array]
PAGINATION_HEADERS = %w[X-Per-Page X-Page X-Next-Page X-Prev-Page Link X-Total X-Total-Pages].freeze
# This is functionally equivalent to the standard `#present` used in
# Grape endpoints, but the JSON for the object, or for each object of
# a collection, will be cached.
......@@ -72,15 +78,22 @@ module API
# @param key [Object] any object that can be converted into a cache key
# @param expires_in [ActiveSupport::Duration, Integer] an expiry time for the cache entry
# @return [Gitlab::Json::PrecompiledJson]
def cache_action(key, **cache_opts)
json = cache.fetch(key, **apply_default_cache_options(cache_opts)) do
def cache_action(key, **custom_cache_opts)
cache_opts = apply_default_cache_options(custom_cache_opts)
json, cached_headers = cache.fetch([key, VERSION], **cache_opts) do
response = yield
if response.is_a?(Gitlab::Json::PrecompiledJson)
response.to_s
else
Gitlab::Json.dump(response.as_json)
cached_body = response.is_a?(Gitlab::Json::PrecompiledJson) ? response.to_s : Gitlab::Json.dump(response.as_json)
cached_headers = header.slice(*PAGINATION_HEADERS)
[cached_body, cached_headers]
end
cached_headers.each do |key, value|
next if header.key?(key)
header key, value
end
body Gitlab::Json::PrecompiledJson.new(json)
......
......@@ -3,7 +3,7 @@
require "spec_helper"
RSpec.describe API::Helpers::Caching, :use_clean_rails_redis_caching do
subject(:instance) { Class.new.include(described_class).new }
subject(:instance) { Class.new.include(described_class, Grape::DSL::Headers).new }
let_it_be(:project) { create(:project) }
let_it_be(:user) { create(:user) }
......@@ -81,7 +81,7 @@ RSpec.describe API::Helpers::Caching, :use_clean_rails_redis_caching do
expected_kwargs = described_class::DEFAULT_CACHE_OPTIONS.merge(kwargs)
expect(expensive_thing).to receive(:do_very_expensive_action).once
expect(instance.cache).to receive(:fetch).with(cache_key, **expected_kwargs).exactly(5).times.and_call_original
expect(instance.cache).to receive(:fetch).with([cache_key, 1], **expected_kwargs).exactly(5).times.and_call_original
5.times { perform }
end
......@@ -95,6 +95,32 @@ RSpec.describe API::Helpers::Caching, :use_clean_rails_redis_caching do
expect(nested_call.to_s).to eq(subject.to_s)
end
context 'Cache for pagination headers' do
described_class::PAGINATION_HEADERS.each do |pagination_header|
context pagination_header do
before do
instance.header(pagination_header, 100)
end
it 'stores and recovers pagination headers from cache' do
expect { perform }.not_to change { instance.header[pagination_header] }
instance.header.delete(pagination_header)
expect { perform }.to change { instance.header[pagination_header] }.from(nil).to(100)
end
it 'prefers headers from request than from cache' do
expect { perform }.not_to change { instance.header[pagination_header] }
instance.header(pagination_header, 50)
expect { perform }.not_to change { instance.header[pagination_header] }.from(50)
end
end
end
end
end
describe "#cache_action_if" do
......
......@@ -75,6 +75,14 @@ RSpec.describe API::Branches do
check_merge_status(json_response)
end
it 'recovers pagination headers from cache between consecutive requests' do
2.times do
get api(route, current_user), params: base_params
expect(response.headers).to include('X-Page')
end
end
end
context 'with gitaly pagination params' do
......
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