FactoryGirl blowing up spec due to foreign key in model

Question

I've got a model Foo which has state_code as a foreign key. The States table is a (more or less) static table created to hold the codes and names for the 50 states, as well as other US postal codes (e.g. "PR" for Puerto Rico). I opted to use state_code as the primary key on States and as the foreign key on Foo, rather than something like state_id. It reads better to humans, and simplifies view logic where I want to call the state code. (EDIT - just to clarify: I don't mean calling code to access the model from the view; I mean that displaying the state as @foo.state_code seems simpler than @foo.state.state_code.)

Foo also has a has_many relationship with model Bar. Both model specs pass a spec for valid factories but for some reason when running a feature spec that builds an instance of Bar, the test blows up due to a foreign key issue related to state_code

I get passing model specs for all of my models, including the test for a valid factory. However, I'm running into trouble whenever I try to create a test object for 'Bar'. Using build blows up on a foreign key error for state_code in Foo (despite fact that the Foo factory explicitly specifies a value that is confirmed to exist as a state_code in States). Using build_stubbed for the Bar object doesn't seem to persist the object.

The models:

# models/foo.rb
class Foo < ActiveRecord
  belongs_to :state, foreign_key: 'state_code', primary_key: 'state_code'
  has_many :bars
  validates :state_code, presence: true, length: { is: 2 }

  # other code omitted...
end

# models/state.rb
class State < ActiveRecord
  self.primary_key = 'state_code'
  has_many :foos, foreign_key: 'state_code'
  validates :state_code, presence: true, uniqueness: true, length: { is: 2 } 

  # other code omitted...
end

# models/bar.rb
class Bar < ActiveRecord
  belongs_to :foo

  # other code omitted
end

The factory below passes green for my Foo and Bar models, so from the model point of view the factories seem fine:

# spec/factores/foo_bar_factory.rb
require 'faker'
require 'date'

FactoryGirl.define do
  factory :foo do
    name { Faker::Company.name }
    city { Faker::Address.city }
    website { Faker::Internet.url }
    state_code { 'AZ' } # Set code for Arizona b/c doesn't matter which state
  end

  factory :bar do
    name { Faker::Name.name }
    website_url { Faker::Internet.url }
    # other columns omitted
    association :foo
  end
end

...where the basic specs are:

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

describe Foo, type: :model do
  let(:foo) { build(:foo) }

  it "has a valid factory" do
    expect(foo).to be_valid
  end

  # code omitted...
end

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

describe Bar, type: :model do
  let(:bar) { build_stubbed(:bar) } # have to build_stubbed - build causes error

  it "has a valid factory" do
    expect(bar).to be_valid
  end
end

This spec passes, with no issues. But if I use build(:bar) for Bar instead of build_stubbed, I get an error on foreign key:

1) Bar has a valid factory
     Failure/Error: let(:bar) { build(:bar) }
ActiveRecord::InvalidForeignKey:
       PG::ForeignKeyViolation: ERROR:  insert or update on table "bars" violates foreign key constraint "fk_rails_3dd3a7c4c3"
       DETAIL:  Key (state_code)=(AZ) is not present in table "states".

The code 'AZ' is definitely in the states table, so I'm unclear why it fails.

In a feature spec I'm attempting to create instances of bar that persist in the database, so I can test they are appearing correctly in #index, #show, and #edit actions. However I can't seem to get it working correctly. The feature spec fails: # spec/features/bar_pages_spec.rb require 'rails_helper'

feature "Bar pages" do
  context "when signed in as admin" do
    let!(:bar_1) { build_stubbed(:bar) }
    let!(:bar_2) { build_stubbed(:bar) }
    let!(:bar_3) { build_stubbed(:bar) }

  # code omitted...

   scenario "clicking manage bar link shows all bars" do
     visit root_path
     click_link "Manage bars"
     save_and_open_page

     expect(page).to have_css("tr td a", text: bar_1.name)
     expect(page).to have_css("tr td a", text: bar_2.name)
     expect(page).to have_css("tr td a", text: bar_3.name)
   end
 end

This spec fails with a message indicating no matches. Using save_and_open_page doesn't show the expected items in the view. (I have a working page with development data though, so I know that the logic actually works as expected). The thoughtbot post on build_stubbed indicates that it should persist objects:

It makes objects look look like they’ve been persisted, creates associations with the build_stubbed strategy (whereas build still uses create), and stubs out a handful of methods that interact with the database and raises if you call them.

...but it doesn't appear to be doing so in my spec. Attempting to use build in lieu of build_stubbed in this spec generates the same foreign key error noted above.

I'm really stuck here. The models appear to have valid factories and pass all specs. But feature specs either blow up the foreign key relationship or don't seem to persist the build_stubbed object between views. It feels like a mess but I can't figure out the right approach to fix it. I have actual, working views in practice, that do what I expect - but I'd like to have test coverage that works.

UPDATE

I went back and updated all of the model code to remove the natural key for state_code. I followed all of @Max's recommendations. The Foo table now uses state_id as the foreign key for states; I copied in the code for app/models/concerns/belongs_to_state.rb as recommended, etc.

Updated schema.rb:

create_table "foos", force: :cascade do |t|
  # columns omitted
  t.integer  "state_id"
end

create_table "states", force: :cascade do |t|
  t.string   "code",       null: false
  t.string   "name"
end

add_foreign_key "foos", "states"

The model specs passed, and some of my simpler feature specs passed. I now realize that the problem is only when more than one Foo object gets created. When this happens, the second object fails due to the uniqueness constraint on the column :code

Failure/Error: let!(:foo_2) { create(:foo) }
     ActiveRecord::RecordInvalid:
       Validation failed: Code has already been taken

I've tried to set the :state_id column directly in the factory for :foo to avoid calling the :state factory. E.g.

# in factory for foo:
state_id { 1 }

# generates following error on run:
Failure/Error: let!(:foo_1) { create(:foo) }
     ActiveRecord::InvalidForeignKey:
       PG::ForeignKeyViolation: ERROR:  insert or update on table "foos" violates foreign key constraint "fk_rails_5f3d3f12c3"
       DETAIL:  Key (state_id)=(1) is not present in table "states".

Obviously state_id isn't in states, since it's id on states, and state_id in foos. Another approach:

# in factory for foo:
state { 1 }    # alternately w/ same error ->  state 1

ActiveRecord::AssociationTypeMismatch:
   State(#70175500844280) expected, got Fixnum(#70175483679340)

Or:

# in factory for foo:
state { State.first }

ActiveRecord::RecordInvalid:
   Validation failed: State can't be blank

All I really want to do is create an instance of the Foo object and have it include the relationship to one of the states from the states table. I don't anticipate doing a lot of changes to the states table - it's really just a reference.

I DON'T need to create a new state. I just need to populate the foreign key state_id on the Foo object with one of the 66 values in the :id column on the states table. Conceptually, the factory for :foo would ideally just pick an integer value between 1 and 66 for the :state_id. It works in console:

irb(main):001:0> s = Foo.new(name: "Test", state_id: 1)
=> #<Foo id: nil, name: "Test", city: nil, created_at: nil, updated_at: nil,  zip_code: nil, state_id: 1>
irb(main):002:0> s.valid?
  State Load (0.6ms)  SELECT  "states".* FROM "states" WHERE "states"."id" = $1 LIMIT 1  [["id", 1]]
  State Exists (0.8ms)  SELECT  1 AS one FROM "states" WHERE ("states"."code" = 'AL' AND "states"."id" != 1) LIMIT 1
=> true

Only way forward I can see right now is to get rid of the uniqueness constraint on :code column in states. Or - remove the foreign key constraint between foos and states, and let Rails enforce the relationship.

Sorry for the massive post...

Answer 1

I'm going to be a pain in the *rse and argue that conventions might be more important than developer convenience and perceived readability.

One of the great things with Rails is that the strong conventions allow us to open up any project and figure out what is going on pretty fast (provided the original author is not a total hack). Try that with a PHP project.

One of these conventions is that foreign keys are postfixed with _id. Many other components such as FactoryGirl rely on these conventions.

I would also argue that using the state code as a primary ID will cause issues if your app ever finds use beyond the US. What happens when you need to keep track of Canadian provinces or Indian states and territories? How are you going to deal with the unavoidable conflicts? Even if you think that this might not be the deal today remember that requirements change with time.

I would model it as:

create_table "countries", force: :cascade do |t|
    t.string   "code",       null: false # ISO 3166-1 alpha-2 or alpha-3
    t.datetime "created_at", null: false
    t.datetime "updated_at", null: false
end

add_index "countries", ["code"], name: "index_countries_on_code"

create_table "states", force: :cascade do |t|
    t.integer  "country_id"
    t.string   "code",       null: false
    t.string   "name",       null: false
    t.datetime "created_at", null: false
    t.datetime "updated_at", null: false
end

add_index "states", ["code"], name: "index_states_on_code"
add_index "states", ["country_id", "code"], name: "index_states_on_country_id_and_code"
add_index "states", ["country_id"], name: "index_states_on_country_id"

"and simplifies view logic where I want to call the state code"

I would argue that you should not be doing database calls at all from your views if it is avoidable. Query upfront from your controller and pass data to your views. It makes it much simpler to optimise queries and avoid N+1 issues.

Use presenters or helper methods to help manage complexity. The slight inconvenience of having to do State.find_by(code: 'AZ') instead of State.find('AZ') is most likely not as important as you think.

added:

This is how you would use associations properly in FactoryGirl. Consider the simplicity in this solution a final argument why your custom foreign key arrangement may be causing more grief than convenience.

models:

class State < ActiveRecord::Base
  # Only the State model should be validating its attributes.
  # You have a major violation of concerns.
  validates_uniqueness_of :state_code
  validates_length_of :state_code, is: 2
end

---

# app/models/concerns/belongs_to_state.rb
module BelongsToState

  extend ActiveSupport::Concern

  included do
    belongs_to :state
    validates :state, presence: true
    validates_associated :state # will not let you save a Foo or Bar if the state is invalid.
  end

  def state_code
    state.state_code
  end

  def state_code= code
    self.assign_attributes(state: State.find_by!(state_code: code))
  end
end

---

class Foo < ActiveRecord::Base
  include BelongsToState
end

---

class Bar < ActiveRecord::Base
  include BelongsToState
end

Factories:

# spec/factories/foos.rb
require 'faker'
FactoryGirl.define do
  factory :foo do
    name { Faker::Company.name }
    city { Faker::Address.city }
    website { Faker::Internet.url }
    state
  end
end

# spec/factories/states.rb
FactoryGirl.define do
  factory :state do
    state_code "AZ"
    name "Arizona"
  end
end

These specs use shoulda-matchers for the extremely succint validation examples:

require 'rails_helper'

RSpec.describe Foo, type: :model do

  let(:foo) { build(:foo) }
  it { should validate_presence_of :state }

  it 'validates the associated state' do
    foo.state.state_code = 'XYZ'
    foo.valid?
    expect(foo.errors).to have_key :state
  end

  describe '#state_code' do
    it 'returns the state code' do
      expect(foo.state_code).to eq 'AZ'
    end
  end

  describe '#state_code=' do
    let!(:vt) { State.create(state_code: 'VT') }
    it 'allows you to set the state with a string' do
      foo.state_code = 'VT'
      expect(foo.state).to eq vt
    end
  end
end

---

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

RSpec.describe State, type: :model do
  it { should validate_length_of(:state_code).is_equal_to(2) }
  it { should validate_uniqueness_of(:state_code) }
end

https://github.com/maxcal/sandbox/tree/31773581

---

Also, in your feature, controller or integration specs you need to use FactoryGirl.create not build_stubbed. build_stubbed does not persist models to the database and in these cases you need your controllers to be able to load the records from the database.

Also you should avoid using CSS selectors in your feature specs if possible. Feature specs should describe your application from a user's POV.

feature "Bar management" do
  context "as an Admin" do
    let!(:bars){ 3.times.map { create(:bar) } }

    background do
      visit root_path
      click_link "Manage bars"
    end

    scenario "I should see all the bars on the management page" do
      # just testing a sampling is usually good enough
      expect(page).to have_link bars.first.name 
      expect(page).to have_link bars.last.name 
    end  

    scenario "I should be able to edit a Bar" do
      click_link bars.first.name
      fill_in('Name', with: 'Moe´s tavern')
      # ...
    end  
  end
end

Answer 2

There was a lot going on here, but with respect to the FactoryGirl issue blowing up on the foreign key relationship between Foo and State, I've figured it out.

@Max was spot on about the problem with using a natural key for the primary key on the states table. It doesn't follow Rails convention, and led to some mixing of concerns, such as potentially having to validate the foreign key (e.g. length 2) on the Foo table.

But even after fixing that to link the tables on a Rails-friendly key (:state_id as foreign key on foos, and :id as primary key on states) -- I still could not find any way to create more than a single instance of a Foo object using the :foo factory. It either failed when I tried to "plug" an integer value into state_id, or the :state factory would fail on the second instance, stating that the code already existed. (See my update in the question for details on the attempts and related fails).

The only way around seemed to be removing the uniqueness validation on State, or eliminating the foreign key relationship at the database layer (Postgres 9.4). I decided I didn't want to do the former. And in thinking about the latter, I realized that I really don't need the foreign key constraint in the database. The states table was intended just to provide a consistent list of state codes as a reference point. If I were to delete this table for some reason, it's not true that I'd want to destroy all of the Foo records. They essentially stand alone, with the state just an attribute of Foo. I briefly considered putting the state info into a constant, but meh.

Deleting the database-level foreign key constraint fixed things for me.

bin/rails g migration RemoveForeignKeyStatesFromFoos

class RemoveForeignKeyStatesFromFoos < ActiveRecord::Migration
  def change
    remove_foreign_key :foos, :states
  end
end

This left the :state_id column intact on my foos table, but removed the line add_foreign_key "foos", "states" from my schema.rb

bin/rails g migration AddIndexToStateIdInFoos

class AddIndexToStateIdInFoos < ActiveRecord::Migration
  def change
    add_index :foos, :state_id
  end
end

...added the line add_index "foos", ["state_id"], name: "index_foos_on_state_id", using: :btree to my schema.

After migrating both, I initially made the mistake of deleting the :state factory, thinking that I didn't need to create new states. After some headaches in test I realized that the test database isn't normally seeded with rake db:seed - so my tests were failing due to strange errors for Module::DelegationError. Rather than build a script to seed the test dB with states, I just modified the factory, and kept the association on the :foo factory.

# spec/factories/foo_factory.rb
FactoryGirl.define do
  factory :foo do
    # columns omitted
    state
  end

  factory :state do
    code { Faker::Address.state_abbr }
    code { Faker::Address.state }
  end
end

At this point, Rails still successfully validates the has_many and belongs_to relationships in the model (which were unchanged).

I understand the add_foreign_key method is relatively new to Rails, as of 4.2. I bit off on it by conflating the fact of the relationship with the need to establish an actual foreign key constraint at the database layer.

From the Rails Guide for ActiveRecord Associations:

You are responsible for maintaining your database schema to match your associations. In practice, this means two things, depending on what sort of associations you are creating. For belongs_to associations you need to create foreign keys, and for has_and_belongs_to_many associations you need to create the appropriate join table.

The use of the term "foreign keys" in this case appears to mean a different thing for Rails, versus Postgres. Rails seems perfectly happy so long as there is a column in the belongs_to table that matches the convention [parent_table_name]_id. This can be achieved by explicitly adding the column or by using references in a migration:

Using t.integer :supplier_id makes the foreign key naming obvious and explicit. In current versions of Rails, you can abstract away this implementation detail by using t.references :supplier instead

In my case, this was plenty sufficient -- an actual foreign key was not necessary.