Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow write_options to be specified for FaradayMiddleware::Caching #155

Merged
merged 2 commits into from
Jul 27, 2017
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 13 additions & 2 deletions lib/faraday_middleware/response/caching.rb
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,9 @@ class Caching < Faraday::Middleware
# :ignore_params - String name or Array names of query params
# that should be ignored when forming the cache
# key (default: []).
# :write_options - Hash of settings that should be passed as the third
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did consider other names, but this felt like the best one, being the most explicit about where exactly these options go

# options parameter to the cache's #write method. If not
# specified, no options parameter will be passed.
#
# Yields if no cache is given. The block should return a cache object.
def initialize(app, cache = nil, options = {})
Expand All @@ -48,7 +51,11 @@ def call(env)
response = @app.call(env)

if CACHEABLE_STATUS_CODES.include?(response.status)
cache.write(key, response)
if @options[:write_options]
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added this if statement with backwards compatibility in mind - if people are using ActiveSupport::Cache::Store-confirming objects, this will work fine, but people may not be, especially in tests, so we shouldn't start passing options if the user hasn't set some.

cache.write(key, response, @options[:write_options])
else
cache.write(key, response)
end
end
end
finalize_response(response, env)
Expand Down Expand Up @@ -81,7 +88,11 @@ def cache_on_complete(env)
# response.status is nil at this point, any checks need to be done inside on_complete block
@app.call(env).on_complete do |response_env|
if CACHEABLE_STATUS_CODES.include?(response_env.status)
cache.write(key, response_env.response)
if @options[:write_options]
cache.write(key, response_env.response, @options[:write_options])
else
cache.write(key, response_env.response)
end
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lines 90-95 are exactly the same as lines 53-58, the only difference is the variable they use: response_env.response for the former, response for the latter (but they should be exactly the same).
Please refactor these lines into a method

end
response_env
end
Expand Down
22 changes: 21 additions & 1 deletion spec/unit/caching_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -78,14 +78,34 @@
end
end

context ":write_options" do
let(:options) { {:write_options => {:expires_in => 9000 } } }

it "passes on the options when writing to the cache" do
expect(@cache).to receive(:write).with("/",
instance_of(Faraday::Response),
options[:write_options])
get('/')
end

context "with no :write_options" do
let(:options) { {} }

it "doesn't pass a third options parameter to the cache's #write" do
expect(@cache).to receive(:write).with("/", instance_of(Faraday::Response))
get('/')
end
end
end

class TestCache < Hash
def read(key)
if cached = self[key]
Marshal.load(cached)
end
end

def write(key, data)
def write(key, data, options = nil)
self[key] = Marshal.dump(data)
end

Expand Down