Put your tests to good use, Refactor with confidence

May 24, 2017

Put your tests to good use, Refactor with confidence

In a previous post I wrote about using RSpec to test a feature that needs to be built, however it is a very rare thing that when you need to build something in software that you won’t have to change it, either it’s implementation or it’s behaviour. With the rate limiter example, it’s very feasible that you might need, or want, to change the backend for it from the database to an alternative data store, most likely redis. Before testing, this would be fraught with stress, how do you know that the changes that you are making aren’t going to break the features that you have already built?

Luckily, we tested them. Now we can alter the implementation of our solution but still know that it works when we are ready to ship it to production. This is good to know, but how do we go about achieving it?

Adding Redis

The first thing we need to do is add redis to our application as it is the data store that we want to implement it. To do this, let’s add redis-namespace to the Gemfile:

# Gemfile
source 'https://rubygems.org'

git_source(:github) do |repo_name|
  repo_name = "#{repo_name}/#{repo_name}" unless repo_name.include?("/")
  "https://github.com/#{repo_name}.git"
end

gem 'rails', '~> 5.0.2'
gem 'pg', '~> 0.18'
gem 'redis-namespace'

# Code omitted

The reason that we are adding in redis-namespace and not just redis is that now we can scope our keys that we are going to store in redis by our Rails environment, development, test, what have you, and then we can use it in whatever rails environment we need at any given time. By including redis-namespace bundler will automatically bundle in redis as a dependency. Now that we have made this change we are good to run bundle:

> bundle
# output omitted
Using redis 3.3.3
# output omitted
Using redis-namespace 1.5.3
# output omitted
Bundle complete! 12 Gemfile dependencies, 62 gems now installed.
Use `bundle show [gemname]` to see where a bundled gem is installed.

So as we can see, redis and redis-namespace are installed, and we are ready to start our refactor.

Refactoring the code

Now we need to set about changing our code to support the redis implementation. First things we first we need to remove ActiveRecord from our Request class, this means also ditching our tests that rely on it as well. This is what we have from our implementation that uses the database:

# spec/models/requests_spec.rb
require 'rails_helper'

describe Request do

  it 'requires an ip address' do
    request = Request.new
    request.requested_at = Time.zone.now
    expect(request).to_not be_valid
    request.ip_address = '127.0.0.1'
    expect(request.save).to eq(true)
  end

  it 'requires the time a request was made' do
    request = Request.new
    request.ip_address = '127.0.0.1'
    expect(request).to_not be_valid
    request.requested_at = Time.zone.now
    expect(request.save).to eq(true)
  end

  context do
    let(:request_ip) { '127.0.0.1' }
    let(:other_ip) { '10.0.0.10' }
    let!(:previous_requests) { (1..10).each { |number| Request.create(ip_address: request_ip, requested_at: number.minutes.ago)} }
    let!(:other_requests) { (1..10).each { |number| Request.create(ip_address: other_ip, requested_at: number.minutes.ago)} }

    it 'orders the requests by requested at, newest to oldest' do
      requests = Request.requests_for_ip_during_time_period(request_ip)
      requests.each_with_index do |request, counter|
        if counter < (requests.size - 1) # Don't try to compare the last one against something outside the array
          expect(request.requested_at > requests[counter + 1].requested_at).to eq(true)
        end
      end
    end

    it 'can count up requests for over a time period' do
      expect(Request.within_interval(60.minutes.ago).size).to eq(20)
    end

    it 'can count up requests for a requesting ip address' do
      expect(Request.for_ip_address(request_ip).size).to eq(10)
      expect(Request.for_ip_address(other_ip).size).to eq(10)
    end

    it 'can count requests for a requesting ip address over a time period' do
      expect(Request.requests_for_ip_during_time_period(request_ip).size).to eq(10)
      expect(Request.requests_for_ip_during_time_period(request_ip, time_period: 6.minutes.ago).size).to eq(5)
    end

  end
end

First of all, let’s remove the ActiveRecord class from Request, it will become a Plain Old Ruby Object, a PORO, which we will need to back with Redis.

# app/models/request.rb
class Request

  validates :ip_address, :requested_at, presence: true

  scope :within_interval, -> (threshold_time) { where("requested_at > ?", threshold_time) }
  scope :for_ip_address, -> (requesting_ip) { where(ip_address: requesting_ip)}

  def self.requests_for_ip_during_time_period(ip_address, time_period: 60.minutes.ago)
    for_ip_address(ip_address).within_interval(time_period).order('requested_at desc')
  end

end

Now we can run our requests spec, and we can see what behaviour was dependent on the ActiveRecord functionality.

> bundle exec rspec spec/models/requests_spec
NoMethodError: undefined method `validates' for Request:Class
  /Users/chris/projects/mine/rate-limiter/app/models/request.rb:3:in `<class:Request>'

As we can see, the validates method is provided by ActiveRecord, so let’s remove the call to that:

# app/models/request.rb
class Request

  scope :within_interval, -> (threshold_time) { where("requested_at > ?", threshold_time) }
  scope :for_ip_address, -> (requesting_ip) { where(ip_address: requesting_ip)}

  def self.requests_for_ip_during_time_period(ip_address, time_period: 60.minutes.ago)
    for_ip_address(ip_address).within_interval(time_period).order('requested_at desc')
  end

end

Now we can try the tests again:

> bundle exec rspec spec/models/requests_spec
bundler: failed to load command: rspec (/Users/chris/.rbenv/versions/2.4.0/bin/rspec)
NoMethodError: undefined method `scope' for Request:Class

So scope is also provided by ActiveRecord, so we will need to remove those too. Let’s just comment these out for the time being as we are going to need to replace this functionality for our Request class as these two methods are vital to how the rate limiter currently works. By temporarily just commenting them out we can refer to them when we have to redo them.

# app/models/request.rb
class Request

  # scope :within_interval, -> (threshold_time) { where("requested_at > ?", threshold_time) }
  # scope :for_ip_address, -> (requesting_ip) { where(ip_address: requesting_ip)}

  def self.requests_for_ip_during_time_period(ip_address, time_period: 60.minutes.ago)
    for_ip_address(ip_address).within_interval(time_period).order('requested_at desc')
  end

end

Let’s run our tests again:

> bundle exec rspec spec/models/requests_spec.rb
FFFFFF

Failures:

  1) Request requires an ip address
     Failure/Error: request.requested_at = Time.zone.now

     NoMethodError:
       undefined method `requested_at=' for #<Request:0x007fe6cbb21628>
     # ./spec/models/requests_spec.rb:7:in `block (2 levels) in <top (required)>'

  2) Request requires the time a request was made
     Failure/Error: request.ip_address = '127.0.0.1'

     NoMethodError:
       undefined method `ip_address=' for #<Request:0x007fe6cbb11f98>
     # ./spec/models/requests_spec.rb:15:in `block (2 levels) in <top (required)>'

  3) Request  orders the requests by requested at, newest to oldest
     Failure/Error: let!(:previous_requests) { (1..10).each { |number| Request.create(ip_address: request_ip, requested_at: number.minutes.ago)} }

     NoMethodError:
       undefined method `create' for Request:Class
     # ./spec/models/requests_spec.rb:24:in `block (4 levels) in <top (required)>'
     # ./spec/models/requests_spec.rb:24:in `block (3 levels) in <top (required)>'

  4) Request  can count up requests for over a time period
     Failure/Error: let!(:previous_requests) { (1..10).each { |number| Request.create(ip_address: request_ip, requested_at: number.minutes.ago)} }

     NoMethodError:
       undefined method `create' for Request:Class
     # ./spec/models/requests_spec.rb:24:in `block (4 levels) in <top (required)>'
     # ./spec/models/requests_spec.rb:24:in `block (3 levels) in <top (required)>'

  5) Request  can count up requests for a requesting ip address
     Failure/Error: let!(:previous_requests) { (1..10).each { |number| Request.create(ip_address: request_ip, requested_at: number.minutes.ago)} }

     NoMethodError:
       undefined method `create' for Request:Class
     # ./spec/models/requests_spec.rb:24:in `block (4 levels) in <top (required)>'
     # ./spec/models/requests_spec.rb:24:in `block (3 levels) in <top (required)>'

  6) Request  can count requests for a requesting ip address over a time period
     Failure/Error: let!(:previous_requests) { (1..10).each { |number| Request.create(ip_address: request_ip, requested_at: number.minutes.ago)} }

     NoMethodError:
       undefined method `create' for Request:Class
     # ./spec/models/requests_spec.rb:24:in `block (4 levels) in <top (required)>'
     # ./spec/models/requests_spec.rb:24:in `block (3 levels) in <top (required)>'

Finished in 0.01017 seconds (files took 1.2 seconds to load)
6 examples, 6 failures

Right, so now we are at the point where the tests run, but fail, as opposed to just erroring out. So let’s start thinking about how it all has to change. We’ve taken out ActiveRecord, so let’s remove the validation tests that rely on it:

# spec/models/requests_spec.rb
it 'requires an ip address' do
  request = Request.new
  request.requested_at = Time.zone.now
  expect(request).to_not be_valid
  request.ip_address = '127.0.0.1'
  expect(request.save).to eq(true)
end

it 'requires the time a request was made' do
  request = Request.new
  request.ip_address = '127.0.0.1'
  expect(request).to_not be_valid
  request.requested_at = Time.zone.now
  expect(request.save).to eq(true)
end

And run our tests again:

> bundle exec rspec spec/models/requests_spec.rb
FFFF

Failures:

  1) Request  orders the requests by requested at, newest to oldest
     Failure/Error: let!(:previous_requests) { (1..10).each { |number| Request.create(ip_address: request_ip, requested_at: number.minutes.ago)} }

     NoMethodError:
       undefined method `create' for Request:Class
     # ./spec/models/requests_spec.rb:8:in `block (4 levels) in <top (required)>'
     # ./spec/models/requests_spec.rb:8:in `block (3 levels) in <top (required)>'

  2) Request  can count up requests for over a time period
     Failure/Error: let!(:previous_requests) { (1..10).each { |number| Request.create(ip_address: request_ip, requested_at: number.minutes.ago)} }

     NoMethodError:
       undefined method `create' for Request:Class
     # ./spec/models/requests_spec.rb:8:in `block (4 levels) in <top (required)>'
     # ./spec/models/requests_spec.rb:8:in `block (3 levels) in <top (required)>'

  3) Request  can count up requests for a requesting ip address
     Failure/Error: let!(:previous_requests) { (1..10).each { |number| Request.create(ip_address: request_ip, requested_at: number.minutes.ago)} }

     NoMethodError:
       undefined method `create' for Request:Class
     # ./spec/models/requests_spec.rb:8:in `block (4 levels) in <top (required)>'
     # ./spec/models/requests_spec.rb:8:in `block (3 levels) in <top (required)>'

  4) Request  can count requests for a requesting ip address over a time period
     Failure/Error: let!(:previous_requests) { (1..10).each { |number| Request.create(ip_address: request_ip, requested_at: number.minutes.ago)} }

     NoMethodError:
       undefined method `create' for Request:Class
     # ./spec/models/requests_spec.rb:8:in `block (4 levels) in <top (required)>'
     # ./spec/models/requests_spec.rb:8:in `block (3 levels) in <top (required)>'

Now, we can see that we can’t create a Request to use for our tests. That is because create is a method provided by ActiveRecord. Since we are not using ActiveRecord anymore, we need to get Redis connected to our Request class, and then we need to add functionality to store the requests. First, let’s hook up redis

# app/models/request.rb
class Request
  # code omitted
  private

  def self.redis_connection
    @redis_connection = Redis.new
  end

  def self.redis
    namespace = "requests_#{Rails.env}".to_sym
    @redis = Redis::Namespace.new(namespace, redis: redis_connection)
  end

end

We want to use redis-namespace, so we need two methods, one to make a new Redis object, def self.redis_connection, and another to use that connection with the namespace, which we will just call def self.redis' since that is what we need to know, by using that method, we are communicating with our redis instance where we want to store each instance of Request`.

Adding Requests to Redis

Now that we have a connection to our Redis instance, we can start putting requests in there. To do this, we need a method to replace our ActiveRecord create method. Let’s add a method called add to our Request class. First of all, we’ll need to change our tests to use that method:

# spec/models/requests_spec.rb
require 'rails_helper'

describe Request do

  let(:request_ip) { '127.0.0.1' }
  let(:other_ip) { '10.0.0.10' }
  let!(:previous_requests) { (1..10).each { |number| Request.add(ip_address: request_ip, requested_at: number.minutes.ago)} }
  let!(:other_requests) { (1..10).each { |number| Request.add(ip_address: other_ip, requested_at: number.minutes.ago)} }

  it 'orders the requests by requested at, newest to oldest' do
    requests = Request.requests_for_ip_during_time_period(request_ip)
    requests.each_with_index do |request, counter|
      if counter < (requests.size - 1) # Don't try to compare the last one against something outside the array
        expect(request.requested_at > requests[counter + 1].requested_at).to eq(true)
      end
    end
  end

  it 'can count up requests for over a time period' do
    expect(Request.within_interval(60.minutes.ago).size).to eq(20)
  end

  it 'can count up requests for a requesting ip address' do
    expect(Request.for_ip_address(request_ip).size).to eq(10)
    expect(Request.for_ip_address(other_ip).size).to eq(10)
  end

  it 'can count requests for a requesting ip address over a time period' do
    expect(Request.requests_for_ip_during_time_period(request_ip).size).to eq(10)
    expect(Request.requests_for_ip_during_time_period(request_ip, time_period: 6.minutes.ago).size).to eq(5)
  end


end

So we’ve changed what we are trying to call with our Request class, we will now be calling an add method, this method will take an IP address as the first parameter and a requested_at time, which will default to the current time. In our implementation, it is likely that we will not have to use anything other than the default, but it does give us some flexibility and helps us to test the method easier. So lets’ run those tests:

> bundle exec rspec spec/models/requests_spec.rb
FFFF

Failures:

  1) Request orders the requests by requested at, newest to oldest
     Failure/Error: for_ip_address(ip_address).within_interval(time_period).order('requested_at desc')

     NoMethodError:
       undefined method `for_ip_address' for Request:Class
     # ./app/models/request.rb:16:in `requests_for_ip_during_time_period'
     # ./spec/models/requests_spec.rb:11:in `block (2 levels) in <top (required)>'

  2) Request can count up requests for over a time period
     Failure/Error: expect(Request.within_interval(60.minutes.ago).size).to eq(20)

     NoMethodError:
       undefined method `within_interval' for Request:Class
     # ./spec/models/requests_spec.rb:20:in `block (2 levels) in <top (required)>'

  3) Request can count up requests for a requesting ip address
     Failure/Error: expect(Request.for_ip_address(request_ip).size).to eq(10)

     NoMethodError:
       undefined method `for_ip_address' for Request:Class
     # ./spec/models/requests_spec.rb:24:in `block (2 levels) in <top (required)>'

  4) Request can count requests for a requesting ip address over a time period
     Failure/Error: for_ip_address(ip_address).within_interval(time_period).order('requested_at desc')

     NoMethodError:
       undefined method `for_ip_address' for Request:Class
     # ./app/models/request.rb:16:in `requests_for_ip_during_time_period'
     # ./spec/models/requests_spec.rb:29:in `block (2 levels) in <top (required)>'

So now we have our class trying to look up requests from Redis, but not being able to find anything because we had to take away our scope calls that we were using to look up data. Now we have two things that we need to take care of. We need to be able to put data into Redis, and we need to be able to query it to check how many requests have been made from an IP address. Let’s add both pieces of code first then have a look at what they do:

# app/models/request.rb
class Request

  def self.for_ip_address(requesting_ip)
    JSON.parse(redis.get(requesting_ip) || [].to_json).map { |requested_at| Time.zone.parse(requested_at) }
  end

  def self.add(requesting_ip, requested_at: Time.zone.now)
    requests = JSON.parse(redis.get(requesting_ip) || [].to_json)
    requests << requested_at
    requests = requests.sort.reverse
    redis.set(requesting_ip, requests.to_json)
  end

  def self.requests_for_ip_during_time_period(ip_address, time_period: 60.minutes.ago)
    for_ip_address(ip_address).select { |requested_at| requested_at > time_period }
  end

  private

  def self.redis_connection
    @redis_connection = Redis.new
  end

  def self.redis
    namespace = "requests_#{Rails.env}".to_sym
    redis = Redis::Namespace.new(namespace, redis: redis_connection)
  end

end

So we’ve added a bit of code here. Let’s go through each of them, first up:

# app/models/request.rb
def self.add(requesting_ip, requested_at: Time.zone.now)
  requests = JSON.parse(redis.get(requesting_ip) || [].to_json)
  requests << requested_at
  requests = requests.sort.reverse
  redis.set(requesting_ip, requests.to_json)
end

This is our method to add a request to redis. Now we have lost a lot of our built in functionality of ActiveRecord so we need to do a little extra ourselves. In redis we want to store an array of hashes, with the ip address being the key of the hash, and the value being an array of times that requests were made:

"127.0.0.1": [Time, Time, Time]

In Redis our stuff is stored as JSON. When we go into the add method, we need to get the requests for that IP address that has already been made. If no requests haven been made we need to make an array for our requests to be stored in. All of that is taken care of by this line

requests = JSON.parse(redis.get(requesting_ip) || [].to_json)

Once we have our array of requests, we need to add the requested_at time to it:

requests << requested_at

We then need to make sure that the first item in the array is the newest request:

requests = requests.sort.reverse

And once we have done all that we need to persist our new array back to redis:

redis.set(requesting_ip, requests.to_json)

Now we can store data in redis for our IP address that is making a request.

Retrieving requests from Redis

Now that we are adding requests, it is time to look at our other piece of code that was added, the code to retrieve requests made by an IP address:

# app/models/request.rb
def self.for_ip_address(requesting_ip)
  JSON.parse(redis.get(requesting_ip) || [].to_json).map { |requested_at| Time.zone.parse(requested_at) }
end

In this call we are calling redis and asking for the array of requests that are being stored for an IP address:

redis.get(requesting_ip)

And if we don’t have it, return an empty array to work with, we are expecting an array back:

[].json

Then once we have our array, return it as a bunch of time Time objects for us to use:

.map { |requested_at| Time.zone.parse(requested_at)}

Now we can receive back an array that allows us to see the requests made by an IP address at what times.

We have one last change that was made:

# app/models/request.rb
def self.requests_for_ip_during_time_period(ip_address, time_period: 60.minutes.ago)
  for_ip_address(ip_address).select { |requested_at| requested_at > time_period }
end

We used to have two methods for retrieving requests, one by IP address and one by time interval, and then combined the two to allow us to look up the relevant data. This was because by using a relational database we were able to just put all requests into the database and then be able to look up by either column. However, Redis is not a relational database, it is a key value store, so we have lost some of that flexibility now, and we are only able to look up by the keys that we are storing requests for. Since we are only able to look up by the ip_address, to get the ip address and time period, we now use our for_ip_address call as normal inside self.requests_for_ip_during_time_period but we just select the requests that have happened since the time_period threshold time, in our case, the default 60 minutes. Those are the requests that we are checking against in our rolling 60 minute period that we are rate limiting for.

What about the tests

We’ve gone and changed a bunch of code. We need to make sure our tests are still valid for this code. At the moment, it won’t be, but let’s run them:

> bundle exec rspec spec/models/requests_spec.rb
FFFF

Failures:

  1) Request orders the requests by requested at, newest to oldest
     Failure/Error: expect(request.requested_at > requests[counter + 1].requested_at).to eq(true)

     NoMethodError:
       undefined method `requested_at' for Wed, 24 May 2017 11:55:21 UTC +00:00:Time
       Did you mean?  require_relative
       Did you mean?  require_relative
     # ./spec/models/requests_spec.rb:14:in `block (3 levels) in <top (required)>'
     # ./spec/models/requests_spec.rb:12:in `each'
     # ./spec/models/requests_spec.rb:12:in `each_with_index'
     # ./spec/models/requests_spec.rb:12:in `block (2 levels) in <top (required)>'
     # ------------------
     # --- Caused by: ---
     # NoMethodError:
     #   undefined method `requested_at' for 2017-05-24 11:55:21 UTC:Time
     #   Did you mean?  require_relative
     #   ./spec/models/requests_spec.rb:14:in `block (3 levels) in <top (required)>'

  2) Request can count up requests for over a time period
     Failure/Error: expect(Request.within_interval(60.minutes.ago).size).to eq(20)

     NoMethodError:
       undefined method `within_interval' for Request:Class
     # ./spec/models/requests_spec.rb:20:in `block (2 levels) in <top (required)>'

  3) Request can count up requests for a requesting ip address
     Failure/Error: expect(Request.for_ip_address(request_ip).size).to eq(10)

       expected: 10
            got: 300

       (compared using ==)
     # ./spec/models/requests_spec.rb:24:in `block (2 levels) in <top (required)>'

  4) Request can count requests for a requesting ip address over a time period
     Failure/Error: expect(Request.requests_for_ip_during_time_period(request_ip).size).to eq(10)

       expected: 10
            got: 194

       (compared using ==)
     # ./spec/models/requests_spec.rb:29:in `block (2 levels) in <top (required)>'

Finished in 0.28458 seconds (files took 1.21 seconds to load)
4 examples, 4 failures

Yep, looks bad, but it’s not that bad. Looking at the first failure, it’s just that we used to be looking at an object that had a requested_at time on it, now the array only stores times, and the second one is for a method within_interval that we don’t even have anymore, so we can delete that, so let’s fix those up first, get the easy wins.

# spec/models/requests_spec.rb
require 'rails_helper'

describe Request do

  let(:request_ip) { '127.0.0.1' }
  let(:other_ip) { '10.0.0.10' }
  let!(:previous_requests) { (1..10).each { |number| Request.add(request_ip, requested_at: number.minutes.ago)} }
  let!(:other_requests) { (1..10).each { |number| Request.add(other_ip, requested_at: number.minutes.ago)} }

  it 'orders the requests by requested at, newest to oldest' do
    requests = Request.requests_for_ip_during_time_period(request_ip)
    requests.each_with_index do |request, counter|
      if counter < (requests.size - 1) # Don't try to compare the last one against something outside the array
        expect(request > requests[counter + 1]).to eq(true)
      end
    end
  end

  it 'can count up requests for a requesting ip address' do
    expect(Request.for_ip_address(request_ip).size).to eq(10)
    expect(Request.for_ip_address(other_ip).size).to eq(10)
  end

  it 'can count requests for a requesting ip address over a time period' do
    expect(Request.requests_for_ip_during_time_period(request_ip).size).to eq(10)
    expect(Request.requests_for_ip_during_time_period(request_ip, time_period: 6.minutes.ago).size).to eq(5)
  end

end

Now let’s run them again:

> bundle exec rspec spec/models/requests_spec.rb
spec/models/requests_spec.rb
.FF

Failures:

  1) Request can count up requests for a requesting ip address
     Failure/Error: expect(Request.for_ip_address(request_ip).size).to eq(10)

       expected: 10
            got: 330

       (compared using ==)
     # ./spec/models/requests_spec.rb:20:in `block (2 levels) in <top (required)>'

  2) Request can count requests for a requesting ip address over a time period
     Failure/Error: expect(Request.requests_for_ip_during_time_period(request_ip).size).to eq(10)

       expected: 10
            got: 216

       (compared using ==)
     # ./spec/models/requests_spec.rb:25:in `block (2 levels) in <top (required)>'

Finished in 0.24174 seconds (files took 1.24 seconds to load)
3 examples, 2 failures

Those worked, but the number of requests that are being stored just keep going up. That is because we have lost another piece of functionality that we take for granted. Cleaning of the database between test runs. Now that we have switched to redis as the data store we are going to need to clean that for ourselves between each test. For this kind of thing that we want to run on every request in our test suite, we will make a modification to our spec/rails_helper.rb file:

# spec/rails_helper.rb
RSpec.configure do |config|
  # omitted code
  config.before(:each) do
    redis_connection = Redis.new
    namespace = "requests_#{Rails.env}".to_sym
    redis = Redis::Namespace.new(namespace, redis: redis_connection)
    keys = redis.keys
    redis.del(*keys) unless keys.empty?
    expect(redis.keys.size).to eq(0)
  end
end

In the RSpec configure block we want to add code to before block that runs before each test. In that block shown above, we are connecting to Redis using a test environment namespace. We are pulling all the keys form redis, and then we are deleting them all unless the keys array is empty, then we are setting an expectation that the size of the redis keys array is 0.

Now that we have this in place we can run our tests again:

> bundle exec rspec spec/models/requests_spec.rb
spec/models/requests_spec.rb
...

Finished in 0.06439 seconds (files took 1.21 seconds to load)
3 examples, 0 failures

Great, now our Request class and it’s tests look to be where we want them.

Not quite done

That’s the guts of our feature changed with the Request class being refactored, but we are not quite done yet. Let’s run all of our tests to see.

> bundle exec rspec spec/models/rate_limit_spec.rb
.FFFF

Failures:

  1) RateLimiter  can get the requests for a time period
     Failure/Error: let!(:previous_requests) { (1..10).each { |number| Request.create(ip_address: request_ip, requested_at: number.minutes.ago)} }

     NoMethodError:
       undefined method `create' for Request:Class
     # ./spec/models/rate_limit_spec.rb:14:in `block (4 levels) in <top (required)>'
     # ./spec/models/rate_limit_spec.rb:14:in `block (3 levels) in <top (required)>'

  2) RateLimiter  reports if a request is permitted
     Failure/Error: let!(:previous_requests) { (1..10).each { |number| Request.create(ip_address: request_ip, requested_at: number.minutes.ago)} }

     NoMethodError:
       undefined method `create' for Request:Class
     # ./spec/models/rate_limit_spec.rb:14:in `block (4 levels) in <top (required)>'
     # ./spec/models/rate_limit_spec.rb:14:in `block (3 levels) in <top (required)>'

  3) RateLimiter  reports if a request is not permitted
     Failure/Error: let!(:previous_requests) { (1..10).each { |number| Request.create(ip_address: request_ip, requested_at: number.minutes.ago)} }

     NoMethodError:
       undefined method `create' for Request:Class
     # ./spec/models/rate_limit_spec.rb:14:in `block (4 levels) in <top (required)>'
     # ./spec/models/rate_limit_spec.rb:14:in `block (3 levels) in <top (required)>'

  4) RateLimiter  reports how long until a request from a non permitted ip can be made
     Failure/Error: let!(:previous_requests) { (1..10).each { |number| Request.create(ip_address: request_ip, requested_at: number.minutes.ago)} }

     NoMethodError:
       undefined method `create' for Request:Class
     # ./spec/models/rate_limit_spec.rb:14:in `block (4 levels) in <top (required)>'
     # ./spec/models/rate_limit_spec.rb:14:in `block (3 levels) in <top (required)>'

Finished in 0.01631 seconds (files took 1.27 seconds to load)
5 examples, 4 failures

Of course, we need to change our create calls to add, so let’s make that change:

# spec/models/rate_limit_spec.rb
let!(:previous_requests) { (1..10).each { |number| Request.add(request_ip, requested_at: number.minutes.ago)} }
let!(:other_requests) { (1..10).each { |number| Request.add(other_ip, requested_at: number.minutes.ago)} }

And we run them:

> bundle exec rspec spec/models/rate_limit_spec.rb
spec/models/rate_limit_spec.rb
.FFFF

Failures:

  1) RateLimiter  can get the requests for a time period
     Failure/Error: @requests ||= Request.requests_for_ip_during_time_period(ip_address, time_period: time_period).limit(maximum_requests)

     NoMethodError:
       undefined method `limit' for #<Array:0x007fd7c45bb0a0>
     # ./app/models/rate_limit.rb:13:in `requests'
     # ./spec/models/rate_limit_spec.rb:19:in `block (3 levels) in <top (required)>'

# IDENTICAL FAILURES omitted

We still have one call here that was from ActiveRecord, we need to remove our limit call from our RateLimit class. Let’s do that

# app/models/rate_limit.rb
def requests
  @requests ||= Request.requests_for_ip_during_time_period(ip_address, time_period: time_period)
end

Now we can run our tests again and see where we are at:

> bundle exec rspec spec/models/rate_limit_spec.rb
....F

Failures:

  1) RateLimiter  reports how long until a request from a non permitted ip can be made
     Failure/Error: next_request_at = (requests.last.requested_at + interval)

     NoMethodError:
       undefined method `requested_at' for Wed, 24 May 2017 12:22:24 UTC +00:00:Time

Ah, again we just have to move from using an object with a requested_at time to just dealing with a time, let’s alter our method in RateLimit

# app/models/rate_limit.rb
def time_until_next_request_permitted
  interval = Time.zone.now - time_period # Gets the period we are limiting for in seconds
  next_request_at = (requests.last + interval)
  next_request_in = (next_request_at - Time.zone.now).to_i
  "Rate limit exceeded, please try again in #{next_request_in} seconds"
end

And again, our tests:

> bundle exec rspec spec/models/rate_limit_spec.rb
spec/models/rate_limit_spec.rb
.....

Finished in 0.08353 seconds (files took 1.22 seconds to load)
5 examples, 0 failures

And now our RateLimit class is good to go as well. Now we just need to check our behaviour of our feature:

> bundle exec rspec spec/requests/home_controller_spec.rb
Failures:

  1) Home Controller renders a 200 for a request
     Failure/Error: Request.create(ip_address: request.remote_ip, requested_at: Time.zone.now)

     NoMethodError:
       undefined method `create' for Request:Class
     # ./app/controllers/home_controller.rb:17:in `record_rate_limit_request'
     # ./spec/requests/home_spec.rb:6:in `block (2 levels) in <top (required)>'

  2) Home Controller takes record of a successful request
     Failure/Error: expect(Request.count).to eq(0)

     NoMethodError:
       undefined method `count' for Request:Class
     # ./spec/requests/home_spec.rb:12:in `block (2 levels) in <top (required)>'

  3) Home Controller hitting the rate limit renders a 429 if the rate limit threshold has been hit
     Failure/Error: (1..99).each { |number| Request.create(ip_address: '127.0.0.1', requested_at: number.seconds.ago )}

     NoMethodError:
       undefined method `create' for Request:Class
     # ./spec/requests/home_spec.rb:19:in `block (4 levels) in <top (required)>'
     # ./spec/requests/home_spec.rb:19:in `block (3 levels) in <top (required)>'

As we can see, again we just need to change create to our add method, but we also (if we want to count requests) need to add a replacement for the ActiveRecord count method. It’s not hard to do, so let’s add it and fix up our create calls:

# spec/requests/home_controller_spec.rb
context 'hitting the rate limit' do
  let!(:requests) do
    (1..99).each { |number| Request.add('127.0.0.1', requested_at: number.seconds.ago )}
  end

# app/models/request.rb
class Request
  # omitted

  def self.count
    redis.keys.size
  end

  # omitted
end

# app/controllers/home_controller.rb
def record_rate_limit_request
  Request.add(request.remote_ip, requested_at: Time.zone.now)
end

That should be all the changes tht we need to make. Of course though, we’ll run the tests to see:

> bundle exec rspec spec/requests/home_controller_spec.rb
...

Finished in 0.29222 seconds (files took 1.21 seconds to load)
3 examples, 0 failures

> bundle exec rspec spec
...........

Finished in 0.51232 seconds (files took 1.22 seconds to load)
11 examples, 0 failures

There we have it. Our data store has been changed from our database to redis, and the behaviour of our application has been maintained.

One thing that you might have noticed is that model specs needed a lot more changing than our request specs. This will often be the case when you refactoring an implementation but you want the behaviour itself to not change. Depending on your specs, the model specs will likely change as you have to depend on different things to drive your behaviour, but as you want the changes to be completely invisible to your end user, the request specs should remain largely the same, just possibly a different way to setup your data that you are using to construct your scenarios.

I hope that going through this has helped illustrate for you the advantage of having well tested features, and how that can be powerful in allowing you to make confident changes to your code. It’s a lot of steps to go through, but stick with it, it’ll help you write better software that you can be confident in sending to production, and I’m sure that’s what we all want, to write code and applications that people use every day.

If part of the process left you unsure, or you just have any comments or questions, please leave a comment, or send me an email, I’m always happy to help and/or discuss approaches and code.

If you want to see the code in full for this project, it’s on my github.

Comments?

I've changed how I run my blog now and have decided to not integrate comments into this new version. I am happy to answer any questions though, feel free to send me an email through the link at the top of the screen. Happy Programming.