diff --git a/.gitignore b/.gitignore index d6b49a9..55f23a1 100644 --- a/.gitignore +++ b/.gitignore @@ -3,7 +3,6 @@ .bundle .config .yardoc -Gemfile.lock InstalledFiles _yardoc coverage diff --git a/Gemfile.lock b/Gemfile.lock new file mode 100644 index 0000000..375ba5b --- /dev/null +++ b/Gemfile.lock @@ -0,0 +1,67 @@ +PATH + remote: . + specs: + pause (0.0.6) + redis + +GEM + remote: https://rubygems.org/ + specs: + celluloid (0.15.2) + timers (~> 1.1.0) + coderay (1.1.0) + diff-lcs (1.2.5) + fakeredis (0.5.0) + redis (~> 3.0) + ffi (1.9.3) + formatador (0.2.5) + guard (2.6.1) + formatador (>= 0.2.4) + listen (~> 2.7) + lumberjack (~> 1.0) + pry (>= 0.9.12) + thor (>= 0.18.1) + guard-rspec (4.2.9) + guard (~> 2.1) + rspec (>= 2.14, < 4.0) + listen (2.7.5) + celluloid (>= 0.15.2) + rb-fsevent (>= 0.9.3) + rb-inotify (>= 0.9) + lumberjack (1.0.5) + method_source (0.8.2) + pry (0.9.12.6) + coderay (~> 1.0) + method_source (~> 0.8) + slop (~> 3.4) + rb-fsevent (0.9.4) + rb-inotify (0.9.4) + ffi (>= 0.5.0) + redis (3.1.0) + rspec (3.0.0) + rspec-core (~> 3.0.0) + rspec-expectations (~> 3.0.0) + rspec-mocks (~> 3.0.0) + rspec-core (3.0.3) + rspec-support (~> 3.0.0) + rspec-expectations (3.0.3) + diff-lcs (>= 1.2.0, < 2.0) + rspec-support (~> 3.0.0) + rspec-mocks (3.0.3) + rspec-support (~> 3.0.0) + rspec-support (3.0.3) + slop (3.5.0) + thor (0.19.1) + timecop (0.7.1) + timers (1.1.0) + +PLATFORMS + ruby + +DEPENDENCIES + fakeredis + guard-rspec + pause! + rb-fsevent + rspec (~> 3.0) + timecop diff --git a/lib/pause/action.rb b/lib/pause/action.rb index c2edfef..59c3cbc 100644 --- a/lib/pause/action.rb +++ b/lib/pause/action.rb @@ -66,9 +66,8 @@ def rate_limited? end def ok? - Pause.analyzer.check(self).nil? + !Pause.adapter.rate_limited?(key) && Pause.analyzer.check(self).nil? rescue ::Redis::CannotConnectError => e - $stderr.puts "Error connecting to redis: #{e.inspect}" false end diff --git a/lib/pause/analyzer.rb b/lib/pause/analyzer.rb index a89b5ed..777bfa2 100644 --- a/lib/pause/analyzer.rb +++ b/lib/pause/analyzer.rb @@ -4,30 +4,57 @@ module Pause class Analyzer include Pause::Helper::Timing - def check(action) - timestamp = period_marker(Pause.config.resolution, Time.now.to_i) - set = adapter.key_history(action.key) - action.checks.each do |period_check| + class BlockTTLChecker < Struct.new(:period_check, :timestamp, :set, :action) + def check start_time = timestamp - period_check.period_seconds set.reverse.inject(0) do |sum, element| break if element.ts < start_time sum += element.count if sum >= period_check.max_allowed - adapter.rate_limit!(action.key, period_check.block_ttl) + Pause.adapter.rate_limit!(action.key, period_check.block_ttl) # Note that Time.now is different from period_marker(resolution, Time.now), which # rounds down to the nearest (resolution) seconds return Pause::RateLimitedEvent.new(action, period_check, sum, Time.now.to_i) end sum end + nil end - nil end - private + class PeriodTTLChecker < Struct.new(:period_check, :timestamp, :set, :action) + def check + start_time = timestamp - period_check.period_seconds + set.select!{|element| element.ts >= start_time} + set.inject(0) do |sum, element| + sum += element.count + if sum >= period_check.max_allowed + block_ttl = set.first.ts + period_check.period_seconds - element.ts + Pause.adapter.rate_limit!(action.key, block_ttl) + # Note that Time.now is different from period_marker(resolution, Time.now), which + # rounds down to the nearest (resolution) seconds + return Pause::RateLimitedEvent.new(action, period_check, sum, Time.now.to_i) + end + sum + end + nil + end + end + + def check(action) + timestamp = period_marker(Pause.config.resolution, Time.now.to_i) + set = Pause.adapter.key_history(action.key) + action.checks.each do |period_check| + checker = if period_check.block_ttl + BlockTTLChecker.new(period_check, timestamp, set, action) + else + PeriodTTLChecker.new(period_check, timestamp, set, action) + end - def adapter - Pause.adapter + result = checker.check + return result if result + end + nil end end end diff --git a/pause.gemspec b/pause.gemspec index 0933320..edd5112 100644 --- a/pause.gemspec +++ b/pause.gemspec @@ -19,7 +19,7 @@ Gem::Specification.new do |gem| gem.add_dependency 'redis' - gem.add_development_dependency 'rspec' + gem.add_development_dependency 'rspec', '~> 3.0' gem.add_development_dependency 'fakeredis' gem.add_development_dependency 'timecop' gem.add_development_dependency 'guard-rspec' diff --git a/spec/pause/action_spec.rb b/spec/pause/action_spec.rb index 4d5233b..316947f 100644 --- a/spec/pause/action_spec.rb +++ b/spec/pause/action_spec.rb @@ -5,19 +5,19 @@ include Pause::Helper::Timing class MyNotification < Pause::Action - scope "ipn:follow" + scope "ipn:mynotification" check period_seconds: 20, max_allowed: 5, block_ttl: 40 check period_seconds: 40, max_allowed: 7, block_ttl: 40 end - let(:resolution) { 10 } + let(:resolution) { 1 } let(:history) { 60 } let(:configuration) { Pause::Configuration.new } before do - Pause.stub(:config).and_return(configuration) - Pause.config.stub(:resolution).and_return(resolution) - Pause.config.stub(:history).and_return(history) + allow(Pause).to receive(:config).and_return(configuration) + allow(configuration).to receive(:resolution).and_return(resolution) + allow(configuration).to receive(:history).and_return(history) end let(:action) { MyNotification.new("1237612") } @@ -27,7 +27,7 @@ class MyNotification < Pause::Action it "should increment" do time = Time.now Timecop.freeze time do - Pause.adapter.should_receive(:increment).with(action.key, time.to_i, 1) + expect(Pause.adapter).to receive(:increment).with(action.key, time.to_i, 1) action.increment! end end @@ -39,45 +39,51 @@ class MyNotification < Pause::Action Timecop.freeze time do 4.times do action.increment! - action.ok?.should be_true + expect(action.ok?).to eq(true) end action.increment! - action.ok?.should be_false + expect(action.ok?).to eq(false) end end it "should successfully consider different period checks" do - time = period_marker(resolution, Time.now.to_i) + time = Time.now.to_i - action.increment! 4, time - 25 - action.ok?.should be_true + Timecop.freeze time - 25 do + action.increment! 4 + expect(action.ok?).to eq(true) + end - action.increment! 2, time - 3 - action.ok?.should be_true + Timecop.freeze time - 3 do + action.increment! 2 + expect(action.ok?).to eq(true) + end - action.increment! 1, time + Timecop.freeze time do + action.increment! 1 - action.ok?.should be_false + expect(action.ok?).to eq(false) + end end it "should return false and silently fail if redis is not available" do - Redis.any_instance.stub(:zrange) { raise Redis::CannotConnectError } + allow_any_instance_of(Redis).to receive(:zrange) { raise Redis::CannotConnectError } time = period_marker(resolution, Time.now.to_i) action.increment! 4, time - 25 - action.ok?.should be_false + expect(action.ok?).to eq(false) end end describe "#analyze" do - context "action should not be rate limited" do + describe "action not to be rate limited" do it "returns nil" do - action.analyze.should be_nil + expect(action.analyze).to be_nil end end - context "action should be rate limited" do + describe "action to be rate limited" do it "returns a RateLimitedEvent object" do time = Time.now rate_limit = nil @@ -89,11 +95,11 @@ class MyNotification < Pause::Action expected_rate_limit = Pause::RateLimitedEvent.new(action, action.checks[0], 7, time.to_i) - rate_limit.should be_a(Pause::RateLimitedEvent) - rate_limit.identifier.should == expected_rate_limit.identifier - rate_limit.sum.should == expected_rate_limit.sum - rate_limit.period_check.should == expected_rate_limit.period_check - rate_limit.timestamp.should == expected_rate_limit.timestamp + expect(rate_limit).to be_a(Pause::RateLimitedEvent) + expect(rate_limit.identifier).to eq(expected_rate_limit.identifier) + expect(rate_limit.sum).to eq(expected_rate_limit.sum) + expect(rate_limit.period_check).to eq(expected_rate_limit.period_check) + expect(rate_limit.timestamp).to eq(expected_rate_limit.timestamp) end end end @@ -106,8 +112,8 @@ class MyNotification < Pause::Action action.ok? other_action.ok? - MyNotification.tracked_identifiers.should include(action.identifier) - MyNotification.tracked_identifiers.should include(other_action.identifier) + expect(MyNotification.tracked_identifiers).to include(action.identifier) + expect(MyNotification.tracked_identifiers).to include(other_action.identifier) end end @@ -119,8 +125,8 @@ class MyNotification < Pause::Action action.ok? other_action.ok? - MyNotification.rate_limited_identifiers.should include(action.identifier) - MyNotification.rate_limited_identifiers.should include(other_action.identifier) + expect(MyNotification.rate_limited_identifiers).to include(action.identifier) + expect(MyNotification.rate_limited_identifiers).to include(other_action.identifier) end end @@ -132,13 +138,13 @@ class MyNotification < Pause::Action action.ok? other_action.ok? - MyNotification.tracked_identifiers.should include(action.identifier, other_action.identifier) - MyNotification.rate_limited_identifiers.should == [action.identifier] + expect(MyNotification.tracked_identifiers).to include(action.identifier, other_action.identifier) + expect(MyNotification.rate_limited_identifiers).to eq([action.identifier]) MyNotification.unblock_all - MyNotification.rate_limited_identifiers.should be_empty - MyNotification.tracked_identifiers.should == [other_action.identifier] + expect(MyNotification.rate_limited_identifiers).to be_empty + expect(MyNotification.tracked_identifiers).to eq([other_action.identifier]) end end @@ -146,11 +152,11 @@ class MyNotification < Pause::Action it 'unblocks the specified id' do 10.times { action.increment! } - expect(action.ok?).to be_false + expect(action.ok?).to eq(false) action.unblock - expect(action.ok?).to be_true + expect(action.ok?).to eq(true) end end end @@ -171,13 +177,13 @@ class ActionWithHashChecks < Pause::Action end it "should define a period check on new instances" do - ActionWithCheck.new("id").checks.should == [ + expect(ActionWithCheck.new("id").checks).to eq [ Pause::PeriodCheck.new(100, 150, 200) ] end it "should define a period check on new instances" do - ActionWithMultipleChecks.new("id").checks.should == [ + expect(ActionWithMultipleChecks.new("id").checks).to eq [ Pause::PeriodCheck.new(100, 150, 200), Pause::PeriodCheck.new(200, 150, 200), Pause::PeriodCheck.new(300, 150, 200) @@ -185,7 +191,7 @@ class ActionWithHashChecks < Pause::Action end it "should accept hash arguments" do - ActionWithHashChecks.new("id").checks.should == [ + expect(ActionWithHashChecks.new("id").checks).to eq [ Pause::PeriodCheck.new(50, 100, 60) ] end @@ -197,9 +203,9 @@ class UndefinedScopeAction < Pause::Action end it "should raise if scope is not defined" do - lambda { + expect { UndefinedScopeAction.new("1.2.3.4").scope - }.should raise_error("Should implement scope. (Ex: ipn:follow)") + }.to raise_error("Should implement scope. (Ex: ipn:follow)") end class DefinedScopeAction < Pause::Action @@ -207,7 +213,7 @@ class DefinedScopeAction < Pause::Action end it "should set scope on class" do - DefinedScopeAction.new("1.2.3.4").scope.should == "my:scope" + expect(DefinedScopeAction.new("1.2.3.4").scope).to eq("my:scope") end end @@ -217,38 +223,39 @@ class BlockedAction < Pause::Action check 10, 0, 10 end + let(:configuration) { Pause::Configuration.new } + before do - Pause.configure do |c| - c.resolution = 10 - c.history = 10 - end + allow(Pause).to receive(:config).and_return(configuration) + allow(Pause.config).to receive(:resolution).and_return(10) + allow(Pause.config).to receive(:history).and_return(10) end let(:action) { BlockedAction } describe "#disable" do before do - action.should be_enabled - action.should_not be_disabled + expect(action).to be_enabled + expect(action).to_not be_disabled action.disable end it "disables the action" do - action.should be_disabled - action.should_not be_enabled + expect(action).to be_disabled + expect(action).to_not be_enabled end end describe "#enable" do before do action.disable - action.should_not be_enabled + expect(action).to_not be_enabled action.enable end it "enables the action" do - action.should be_enabled - action.should_not be_disabled + expect(action).to be_enabled + expect(action).to_not be_disabled end end end diff --git a/spec/pause/analyzer_spec.rb b/spec/pause/analyzer_spec.rb index eed7763..28d18aa 100644 --- a/spec/pause/analyzer_spec.rb +++ b/spec/pause/analyzer_spec.rb @@ -5,19 +5,24 @@ include Pause::Helper::Timing class FollowPushNotification < Pause::Action - scope "ipn:follow" + scope "ipn:followpn" check 20, 5, 12 check 40, 7, 12 end - let(:resolution) { 10 } + class UberSimpleCheck < Pause::Action + scope "usc" + check period_seconds: 10, max_allowed: 3 + end + + let(:resolution) { 1 } let(:history) { 60 } let(:configuration) { Pause::Configuration.new } before do - Pause.stub(:config).and_return(configuration) - Pause.config.stub(:resolution).and_return(resolution) - Pause.config.stub(:history).and_return(history) + allow(Pause).to receive(:config).and_return(configuration) + allow(Pause.config).to receive(:resolution).and_return(resolution) + allow(Pause.config).to receive(:history).and_return(history) end let(:analyzer) { Pause.analyzer } @@ -27,7 +32,7 @@ class FollowPushNotification < Pause::Action describe "#analyze" do it "checks and blocks if max_allowed is reached" do time = Time.now - adapter.should_receive(:rate_limit!).once.with(action.key, 12) + expect(adapter).to receive(:rate_limit!).once.with(action.key, 12) Timecop.freeze time do 5.times do action.increment! @@ -35,11 +40,63 @@ class FollowPushNotification < Pause::Action end end end + + context "checks without block_ttl" do + let(:action) { UberSimpleCheck.new("1243123") } + + it "blocks only for the amount of time until an action would be allowed again" do + expect(adapter).to receive(:rate_limit!).once.with(action.key, 3) + + now = Time.now.to_i + + Timecop.freeze now - 10 do + action.increment! + end + + Timecop.freeze now - 5 do + action.increment! + end + + Timecop.freeze now - 3 do + action.increment! + end + + Timecop.freeze now do + analyzer.check(action) + end + end + end + + context "with a set element with a higher count" do + let(:action) { UberSimpleCheck.new("1243123") } + + it "blocks for a period appropriate to when the limit was exceeded" do + expect(adapter).to receive(:rate_limit!).once.with(action.key, 5) + + now = Time.now.to_i + + Timecop.freeze now - 10 do + action.increment! + end + + Timecop.freeze now - 5 do + action.increment!(2) + end + + Timecop.freeze now - 3 do + action.increment! + end + + Timecop.freeze now do + analyzer.check(action) + end + end + end end describe "#check" do it "should return nil if action is NOT blocked" do - analyzer.check(action).should be_nil + expect(analyzer.check(action)).to be_nil end it "should return blocked action if action is blocked" do @@ -47,7 +104,7 @@ class FollowPushNotification < Pause::Action 5.times do action.increment! end - analyzer.check(action).should be_a(Pause::RateLimitedEvent) + expect(analyzer.check(action)).to be_a(Pause::RateLimitedEvent) end end end diff --git a/spec/pause/configuration_spec.rb b/spec/pause/configuration_spec.rb index b412166..6cf8ca7 100644 --- a/spec/pause/configuration_spec.rb +++ b/spec/pause/configuration_spec.rb @@ -14,12 +14,12 @@ c.history = 6000 end - subject.redis_host.should == "128.23.12.8" - subject.redis_port.should == 2134 - subject.redis_db.should == "13" + expect(subject.redis_host).to eq("128.23.12.8") + expect(subject.redis_port).to eq(2134) + expect(subject.redis_db).to eq("13") - subject.resolution.should == 5000 - subject.history.should == 6000 + expect(subject.resolution).to eq(5000) + expect(subject.history).to eq(6000) end it "should provide redis defaults" do @@ -27,10 +27,10 @@ # do nothing end - subject.redis_host.should == "127.0.0.1" - subject.redis_port.should == 6379 - subject.redis_db.should == "1" - subject.resolution.should == 600 # 10 minutes - subject.history.should == 86400 # one day + expect(subject.redis_host).to eq("127.0.0.1") + expect(subject.redis_port).to eq(6379) + expect(subject.redis_db).to eq("1") + expect(subject.resolution).to eq(600) # 10 minutes + expect(subject.history).to eq(86400) # one day end end diff --git a/spec/pause/redis/adapter_spec.rb b/spec/pause/redis/adapter_spec.rb index 7c0ff92..888b81b 100644 --- a/spec/pause/redis/adapter_spec.rb +++ b/spec/pause/redis/adapter_spec.rb @@ -9,9 +9,9 @@ let(:configuration) { Pause::Configuration.new } before do - Pause.stub(:config).and_return(configuration) - Pause.config.stub(:resolution).and_return(resolution) - Pause.config.stub(:history).and_return(history) + allow(Pause).to receive(:config).and_return(configuration) + allow(Pause.config).to receive(:resolution).and_return(resolution) + allow(Pause.config).to receive(:history).and_return(history) redis_conn.flushall end @@ -24,14 +24,14 @@ it "should add key to a redis set" do adapter.increment(key, Time.now.to_i) set = redis_conn.zrange(adapter.send(:white_key, key), 0, -1, :with_scores => true) - set.should_not be_empty - set.size.should eql(1) - set[0].size.should eql(2) + expect(set).to_not be_empty + expect(set.size).to eql(1) + expect(set[0].size).to eql(2) end it "should remove old key from a redis set" do time = Time.now - redis_conn.should_receive(:zrem).with(adapter.send(:white_key, key), [adapter.period_marker(resolution, time)]) + expect(redis_conn).to receive(:zrem).with(adapter.send(:white_key, key), [adapter.period_marker(resolution, time)]) adapter.time_blocks_to_keep = 1 Timecop.freeze time do @@ -43,7 +43,7 @@ end it "sets expiry on key" do - redis_conn.should_receive(:expire).with(adapter.send(:white_key, key), history) + expect(redis_conn).to receive(:expire).with(adapter.send(:white_key, key), history) adapter.increment(key, Time.now.to_i) end end @@ -55,8 +55,8 @@ it "saves ip to redis with expiration" do adapter.rate_limit!(key, ttl) - redis_conn.get(blocked_key).should_not be_nil - redis_conn.ttl(blocked_key).should == ttl + expect(redis_conn.get(blocked_key)).to_not be_nil + expect(redis_conn.ttl(blocked_key)).to eq(ttl) end end @@ -67,38 +67,38 @@ it "should return true if blocked" do adapter.rate_limit!(key, ttl) - (!!redis_conn.get(blocked_key).should) == adapter.rate_limited?(key) + expect(redis_conn.get(blocked_key) ? true : false).to eq(adapter.rate_limited?(key)) end end describe "#white_key" do it "prefixes key" do - adapter.send(:white_key, "abc").should == "i:abc" + expect(adapter.send(:white_key, "abc")).to eq("i:abc") end end describe '#enable' do it 'deletes the disabled flag in redis' do adapter.disable("boom") - expect(adapter.disabled?("boom")).to be_true + expect(adapter.disabled?("boom")).to eq(true) adapter.enable("boom") - expect(adapter.disabled?("boom")).to be_false + expect(adapter.disabled?("boom")).to eq(false) end end describe '#disable' do it 'sets the disabled flag in redis' do - expect(adapter.enabled?("boom")).to be_true + expect(adapter.enabled?("boom")).to eq(true) adapter.disable("boom") - expect(adapter.enabled?("boom")).to be_false + expect(adapter.enabled?("boom")).to eq(false) end end describe '#rate_limit!' do it 'rate limits a key for a specific ttl' do - expect(adapter.rate_limited?('1')).to be_false + expect(adapter.rate_limited?('1')).to eq(false) adapter.rate_limit!('1', 10) - expect(adapter.rate_limited?('1')).to be_true + expect(adapter.rate_limited?('1')).to eq(true) end end @@ -107,13 +107,13 @@ adapter.rate_limit!('boom:1', 10) adapter.rate_limit!('boom:2', 10) - expect(adapter.rate_limited?('boom:1')).to be_true - expect(adapter.rate_limited?('boom:2')).to be_true + expect(adapter.rate_limited?('boom:1')).to eq(true) + expect(adapter.rate_limited?('boom:2')).to eq(true) adapter.delete_rate_limited_keys('boom') - expect(adapter.rate_limited?('boom:1')).to be_false - expect(adapter.rate_limited?('boom:2')).to be_false + expect(adapter.rate_limited?('boom:1')).to eq(false) + expect(adapter.rate_limited?('boom:2')).to eq(false) end end @@ -122,13 +122,13 @@ adapter.rate_limit!('boom:1', 10) adapter.rate_limit!('boom:2', 10) - expect(adapter.rate_limited?('boom:1')).to be_true - expect(adapter.rate_limited?('boom:2')).to be_true + expect(adapter.rate_limited?('boom:1')).to eq(true) + expect(adapter.rate_limited?('boom:2')).to eq(true) adapter.delete_rate_limited_key('boom', '1') - expect(adapter.rate_limited?('boom:1')).to be_false - expect(adapter.rate_limited?('boom:2')).to be_true + expect(adapter.rate_limited?('boom:1')).to eq(false) + expect(adapter.rate_limited?('boom:2')).to eq(true) end end end diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index b403cd8..596d04b 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -12,7 +12,6 @@ require 'support/fakeredis' RSpec.configure do |config| - config.treat_symbols_as_metadata_keys_with_true_values = true config.run_all_when_everything_filtered = true config.filter_run :focus @@ -24,5 +23,6 @@ config.before :each do Redis.new.flushall + Pause.instance_variable_set(:@adapter, nil) end end