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

Improve support for interrelated model associations? #1063

Open
rlue opened this issue Oct 25, 2017 · 3 comments

Comments

@rlue
Copy link

commented Oct 25, 2017

This one's a doozy but the short version is that I can't seem to find a way to set up a factory with multiple associations that are themselves related to each other. (For instance, a character belongs to a book, and both belong to an author — how can I create a character factory that sets associations appropriately?)

The problems I've run into are subtle, so I figure it's best to outline the process:

The Setup

There's a hierarchy in my application where an Account has multiple subordinate associations:

Account has_many :ctas
Account has_many :links
Cta has_many :links

So if a link exists, it belongs to both a cta and an account. Importantly, link.account == link.cta.account.

Link Factory v1: attributes_for doesn't work

This was my first attempt:

# spec/factories/links.rb

factory :link do
  account { cta.account }
  cta
end

The factory above worked fine for most cases, but borked on attributes_for:

>> FactoryGirl.attributes_for(:link)
NoMethodError: undefined method `account' for nil:NilClass

Of course, the documentation makes no mention of being able to define associations with a dynamic attribute block, so I decided to take another approach.

Link Factory v2: build_stubbed doesn't work

This time around, I decided to make a change to the Link model:

# app/models/link.rb

before_validation :bind_to_cta_account

def bind_to_cta_account
  self.account = cta.account if account.blank? && cta.present?
end

With this, I removed the account association line from the Link factory altogether. Now, attributes_for works just fine, but build_stubbed isn't running the validation callback automatically:

>> foo = FactoryGirl.build_stubbed(:link)
=> #<Link id: 1004, uid: nil, account_id: nil, cta_id: 1003, token: nil, url: "http://schmidt.io/janelle", title: nil, deleted_at: nil, created_at: nil, updated_at: nil, favicon: nil, name: nil, meta_tags: nil, display_type: "iframe", amp_url: nil>
>> foo.account
=> nil
>> foo.valid?
=> true
>> foo.account
=> #<Account id: 1002, uid: nil, user_id: 1001, time_zone: "UTC", stripe_customer_id: nil, subscription: "free", deleted_at: nil, created_at: nil, updated_at: nil, links_count: 0, ctas_count: 0>

Another problem with this approach is that FactoryBot has to override certain conflicting methods (like counter_cache) to make build_stubbed work, but it can only do this when the associations are defined in the factory. If account keeps a counter_cache of its links, and is removed from the link factory, build_stubbed fails again:

>> FactoryGirl.build_stubbed(:link)
RuntimeError: stubbed models are not allowed to access the database - Account#increment!

Link Factory v3: works, but hacky

For the time being, the best I can do is define a separate before(:stub) callback in addition to the before_validation callback in the model:

after(:stub) do |link|
  link.account = link.cta.account
end

Surely there's a better way?


(I should clarify I'm using FactoryBot with Rails/ActiveRecord.)

Is there a built-in way to get these kinds of interdependent associations working in FactoryBot? If there isn't, is the team open to adding it? My preferred approach would be the first one I described, and I'd be happy to take a stab at a PR in a month or so if I had the team's blessing.

@joshuaclayton

This comment has been minimized.

Copy link
Member

commented Oct 25, 2017

@rlue You're running into what is one of the most difficult areas of the gem, primarily because there's no hard-and-fast rules around ORMs and relationships here.

For attributes_for, the error is resulting from the fact that it doesn't build/create relationships, and for build_stubbed, it chooses not to attempt relationship persistence in a traditional sense, resulting in the values being available in some cases but not others, dependent on the ORM and how it interacts with its internal cache vs data that's available via a db query.

I've approached this in a couple of ways:

  1. Use Ruby to construct a potentially deeply-nested and inter-related set of records, and include that in the test harness available for object construction. Check out this example. In it, you can see it uses FactoryBot to create four records and do state transition on the created order, but it's wrapped behind a DSL.
  2. Create a factory that, via transient attributes, handles relationship creation a bit more aggressively (by using create in a block/dynamic attribute instead of using the built-in associations).

Both have their drawbacks; the first introduces additional complexity and moves it out of FB. The second has the potential to slow down the test suite, depending on frequency of stubs and how many records end up getting "force"-created.

Due to AR's behavior around associations, this is a particularly tough nut to crack, and there's no ultimate solution to this. It reminds me of #458, but I recall attempting a spike and running into issues.

The considerations I've had for any solution are:

  1. Factoring in the type of build strategy, and how associations are handled for each
  2. Understanding that FB is in no way directly coupled to ActiveRecord; it makes assumptions for stubbed methods in the stub strategy, and it assumes for each attribute, there's a setter of the same name, as well as #save! (which can even be overridden)

While there's no hard dependency on ActiveRecord, it means FB defaults to as simple a set of operations as possible on the objects - specifically, calling new without arguments, a setter per attribute, and #save!. Ideally, that's the extent of the interface we really muck with, too, which makes coming up with a solution tailored to AR tricky. Maybe strategies per persistence layer/ORM might make sense, but we've avoided it thus far.

I hope this helps provide some context on the state of things. I'm definitely not opposed to moving towards a solution, but I do want to ensure that whatever direction is taken, it works for AR and non-AR ORMs/ODMs alike.

@calleerlandsson

This comment has been minimized.

Copy link

commented Nov 16, 2017

Hello!

I stumbled across this issue when trying to solve a similar problem in an app I'm working on.

I might be completely missing the point here (please tell me if I am) but I figured I'd share what worked for me:

My app has these models:

class Organization < ApplicationRecord
  has_and_belongs_to_many :users
end

class User < ApplicationRecord
  has_and_belongs_to_many :organizations
  has_many :entries
end

class Project < ApplicationRecord
  belongs_to :organization
  has_many :entries
end

class Entry < ApplicationRecord
  belongs_to :organization
  belongs_to :user
  belongs_to :project
end

For entries, it is important that self.organization == project.organization && user.organizations.include?(self.organization).

Setting up factories for the first three models was straight-forward:

FactoryBot.define do
  factory :organization do
  end

  factory :user do
  end

  factory :project do
    organization
  end
end

When writing the entry factory, I tried to find documentation on how to set up dependent associations and found this issue as well as #458 and #426. After not finding a clear answer I tried this, and it seems to work:

FactoryBot.define do
  factory :entry do
    organization
    user { association :user, organizations: [organization] }
    project { association :project, organization: organization }
  end
end

In your case, @rlue, would this work?

factory :link do
  account
  cta { association :cta, account: account }
end

@joshuaclayton, Is calling association within a dynamic attribute block completely crazy?

@papa-cool

This comment has been minimized.

Copy link

commented Nov 17, 2017

Hello!
Working on the same issue this day, I found the same solution as @calleerlandsson and it works perfectly.
It would be great to have official access to FactoryGirl::Evaluator::association.

In addition, it could be nice to have an official access to the instance variable @instance in FactoryGirl::Evaluator.
I need it to declare the association in both side.
With the following model,

class User < ApplicationRecord
  belongs_to :school
  has_one :profile
end

class Profile < ApplicationRecord
  belongs_to :school
  belongs_to :user
end

class School < ApplicationRecord
  has_many :users
  has_many :profiles
end

I have the following factories.

FactoryBot.define do
  factory :user do
    school
    profile { association :profile, user: @instance, school: school }
  end

  factory :profile do
    school
    user { association :user, profile: @instance, school: school }
  end

  factory :school do
  end
end

With the use of @instance, the association is define in both side. Then we can use both FactoryBot.create(:user) and FactoryBot.create(:profile) without having SystemStackError: stack level too deep.
Without the use of @instance, FactoryBot.build(:user).profile.user_id == nil.

@joshuaclayton, am I using FactoryBot in a bad way?
Thanks for your help.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
4 participants
You can’t perform that action at this time.