Allow factories to be modified after they've been defined.

This adds `FactoryGirl.modify`, which allows for reopening of factories
that've been defined elsewhere. Modifying a factory won't remove or
change callbacks, only attributes.

GETTING_STARTED.md

@@ -403,6 +403,51 @@ Calling FactoryGirl.create will invoke both after_build and after_create callbac
 
 Also, like standard attributes, child factories will inherit (and can also define) callbacks from their parent factory.
 
+Modifying factories
+-------------------
+
+If you're given a set of factories (say, from a gem developer) but want to change them to fit into your application better, you can
+modify that factory instead of creating a child factory and adding attributes there.
+
+If a gem were to give you a User factory:
+
+    FactoryGirl.define do
+      factory :user do
+        full_name "John Doe"
+        sequence(:username) {|n| "user#{n}" }
+        password "password"
+      end
+    end
+
+Instead of creating a child factory that added additional attributes:
+
+    FactoryGirl.define do
+      factory :application_user, :parent => :user do
+        full_name     { Faker::Name.name }
+        date_of_birth { 21.years.ago }
+        gender        "Female"
+        health        90
+      end
+    end
+
+You could modify that factory instead.
+
+    FactoryGirl.modify do
+      factory :user do
+        full_name     { Faker::Name.name }
+        date_of_birth { 21.years.ago }
+        gender        "Female"
+        health        90
+      end
+    end
+
+When modifying a factory, you can change any of the attributes you want (aside from callbacks).
+
+`FactoryGirl.modify` must be called outside of a `FactoryGirl.define` block as it operates on factories differently.
+
+A couple caveats: you can only modify factories (not sequences or traits) and callbacks *still compound as they normally would*. So, if
+the factory you're modifying defines an `after_create` callback, you defining an `after_create` won't override it, it'll just get run after the first callback.
+
 Building or Creating Multiple Records
 -------------------------------------
 

lib/factory_girl/attribute.rb

@@ -42,6 +42,10 @@ module FactoryGirl
       self.priority <=> another.priority
     end
 
+    def ==(another)
+      self.object_id == another.object_id
+    end
+
     private
 
     def ensure_non_attribute_writer!

lib/factory_girl/attribute_list.rb

@@ -3,11 +3,12 @@ module FactoryGirl
     include Enumerable
 
     def initialize
-      @attributes = {}
+      @attributes  = {}
+      @overridable = false
     end
 
     def define_attribute(attribute)
-      if attribute_defined?(attribute.name)
+      if !overridable? && attribute_defined?(attribute.name)
         raise AttributeDefinitionError, "Attribute already defined: #{attribute.name}"
       end
 
@@ -27,30 +28,34 @@ module FactoryGirl
     end
 
     def attribute_defined?(attribute_name)
-      !@attributes.values.flatten.detect do |attribute|
-        attribute.name == attribute_name &&
-          !attribute.is_a?(FactoryGirl::Attribute::Callback)
-      end.nil?
+      !!find_attribute(attribute_name)
     end
 
     def apply_attributes(attributes_to_apply)
       new_attributes = []
 
       attributes_to_apply.each do |attribute|
-        if attribute_defined?(attribute.name)
-          @attributes.each_value do |attributes|
-            attributes.delete_if do |attrib|
-              new_attributes << attrib.clone if attrib.name == attribute.name
-            end
-          end
+        new_attribute = if !overridable? && defined_attribute = find_attribute(attribute.name)
+          defined_attribute
         else
-          new_attributes << attribute.clone
+          attribute
         end
+
+        delete_attribute(attribute.name)
+        new_attributes << new_attribute
       end
 
       prepend_attributes new_attributes
     end
 
+    def overridable
+      @overridable = true
+    end
+
+    def overridable?
+      @overridable
+    end
+
     private
 
     def valid_callback_names
@@ -58,6 +63,8 @@ module FactoryGirl
     end
 
     def add_attribute(attribute)
+      delete_attribute(attribute.name) if overridable?
+
       @attributes[attribute.priority] ||= []
       @attributes[attribute.priority] << attribute
       attribute
@@ -76,5 +83,20 @@ module FactoryGirl
         result
       end.flatten
     end
+
+    def find_attribute(attribute_name)
+      @attributes.values.flatten.detect do |attribute|
+        attribute.name == attribute_name &&
+          !attribute.is_a?(FactoryGirl::Attribute::Callback)
+      end
+    end
+
+    def delete_attribute(attribute_name)
+      if attribute_defined?(attribute_name)
+        @attributes.each_value do |attributes|
+          attributes.delete_if {|attrib| attrib.name == attribute_name }
+        end
+      end
+    end
   end
 end

lib/factory_girl/factory.rb

@@ -34,18 +34,37 @@ module FactoryGirl
 
     def initialize(name, options = {}) #:nodoc:
       assert_valid_options(options)
-      @name           = factory_name_for(name)
-      @parent         = options[:parent]
-      @options        = options
-      @attribute_list = AttributeList.new
-      @traits         = []
+      @name                     = factory_name_for(name)
+      @parent                   = options[:parent]
+      @options                  = options
+      @traits                   = []
+      @children                 = []
+      @attribute_list           = AttributeList.new
+      @inherited_attribute_list = AttributeList.new
+    end
+
+    def allow_overrides
+      @attribute_list.overridable
+      @inherited_attribute_list.overridable
+      self
+    end
+
+    def allow_overrides?
+      @attribute_list.overridable?
     end
 
     def inherit_from(parent) #:nodoc:
       @options[:class]            ||= parent.class_name
       @options[:default_strategy] ||= parent.default_strategy
 
-      apply_attributes(parent.attributes)
+      allow_overrides if parent.allow_overrides?
+      parent.add_child(self)
+
+      @inherited_attribute_list.apply_attributes(parent.attributes)
+    end
+
+    def add_child(factory)
+      @children << factory unless @children.include?(factory)
     end
 
     def apply_traits(traits) #:nodoc:
@@ -63,7 +82,7 @@ module FactoryGirl
         raise AssociationDefinitionError, "Self-referencing association '#{attribute.name}' in factory '#{self.name}'"
       end
 
-      @attribute_list.define_attribute(attribute)
+      @attribute_list.define_attribute(attribute).tap { update_children }
     end
 
     def define_trait(trait)
@@ -75,13 +94,17 @@ module FactoryGirl
     end
 
     def attributes
-      @attribute_list.to_a
+      AttributeList.new.tap do |list|
+        list.apply_attributes(@attribute_list)
+        list.apply_attributes(@inherited_attribute_list)
+      end.to_a
     end
 
     def run(proxy_class, overrides) #:nodoc:
       proxy = proxy_class.new(build_class)
       overrides = symbolize_keys(overrides)
-      @attribute_list.each do |attribute|
+
+      attributes.each do |attribute|
         factory_overrides = overrides.select { |attr, val| attribute.aliases_for?(attr) }
         if factory_overrides.empty?
           attribute.add_to(proxy)
@@ -146,6 +169,10 @@ module FactoryGirl
 
     private
 
+    def update_children
+      @children.each { |child| child.inherit_from(self) }
+    end
+
     def class_for (class_or_to_s)
       if class_or_to_s.respond_to?(:to_sym)
         class_name = variable_name_to_class_name(class_or_to_s)

lib/factory_girl/syntax/default.rb

@@ -7,6 +7,10 @@ module FactoryGirl
         DSL.run(block)
       end
 
+      def modify(&block)
+        ModifyDSL.run(block)
+      end
+
       class DSL
         def self.run(block)
           new.instance_eval(&block)
@@ -39,6 +43,18 @@ module FactoryGirl
           FactoryGirl.register_trait(Trait.new(name, &block))
         end
       end
+
+      class ModifyDSL
+        def self.run(block)
+          new.instance_eval(&block)
+        end
+
+        def factory(name, options = {}, &block)
+          factory = FactoryGirl.factory_by_name(name).allow_overrides
+          proxy = FactoryGirl::DefinitionProxy.new(factory)
+          proxy.instance_eval(&block)
+        end
+      end
     end
   end
 

spec/acceptance/create_spec.rb

@@ -89,4 +89,3 @@ describe "a custom create" do
     FactoryGirl.create(:user).should be_persisted
   end
 end
-

spec/acceptance/modify_factories_spec.rb

@@ -0,0 +1,184 @@
+require "spec_helper"
+
+describe "modifying factories" do
+  include FactoryGirl::Syntax::Methods
+
+  before do
+    define_model('User', :name => :string, :admin => :boolean, :email => :string, :login => :string)
+
+    FactoryGirl.define do
+      sequence(:email) {|n| "user#{n}@example.com" }
+
+      factory :user do
+        email
+
+        after_create do |user|
+          user.login = user.name.upcase if user.name
+        end
+
+        factory :admin do
+          admin true
+        end
+      end
+    end
+  end
+
+  context "simple modification" do
+    before do
+      FactoryGirl.modify do
+        factory :user do
+          name "Great User"
+        end
+      end
+    end
+
+    subject     { create(:user) }
+    its(:name)  { should == "Great User" }
+    its(:login) { should == "GREAT USER" }
+
+    it "doesn't allow the factory to be subsequently defined" do
+      expect do
+        FactoryGirl.define { factory :user }
+      end.to raise_error(FactoryGirl::DuplicateDefinitionError)
+    end
+
+    it "does allow the factory to be subsequently modified" do
+      FactoryGirl.modify do
+        factory :user do
+          name "Overridden again!"
+        end
+      end
+
+      create(:user).name.should == "Overridden again!"
+    end
+  end
+
+  context "adding callbacks" do
+    before do
+      FactoryGirl.modify do
+        factory :user do
+          name "Great User"
+          after_create do |user|
+            user.name = user.name.downcase
+            user.login = nil
+          end
+        end
+      end
+    end
+
+    subject { create(:user) }
+
+    its(:name)  { should == "great user" }
+    its(:login) { should be_nil }
+  end
+
+  context "reusing traits" do
+    before do
+      FactoryGirl.define do
+        trait :rockstar do
+          name "Johnny Rockstar!!!"
+        end
+      end
+
+      FactoryGirl.modify do
+        factory :user do
+          rockstar
+          email { "#{name}@example.com" }
+        end
+      end
+    end
+
+    subject     { create(:user) }
+
+    its(:name)  { should == "Johnny Rockstar!!!" }
+    its(:email) { should == "Johnny Rockstar!!!@example.com" }
+    its(:login) { should == "JOHNNY ROCKSTAR!!!" }
+  end
+
+  context "redefining attributes" do
+    before do
+      FactoryGirl.modify do
+        factory :user do
+          email { "#{name}-modified@example.com" }
+          name "Great User"
+        end
+      end
+    end
+
+    context "creating user" do
+      context "without overrides" do
+        subject     { create(:user) }
+
+        its(:name)  { should == "Great User" }
+        its(:email) { should == "Great User-modified@example.com" }
+      end
+
+      context "overriding dynamic attributes" do
+        subject     { create(:user, :email => "perfect@example.com") }
+
+        its(:name)  { should == "Great User" }
+        its(:email) { should == "perfect@example.com" }
+      end
+
+      context "overriding static attributes" do
+        subject     { create(:user, :name => "wonderful") }
+
+        its(:name)  { should == "wonderful" }
+        its(:email) { should == "wonderful-modified@example.com" }
+      end
+    end
+
+    context "creating admin" do
+      context "without overrides" do
+        subject     { create(:admin) }
+
+        its(:name)  { should == "Great User" }
+        its(:email) { should == "Great User-modified@example.com" }
+        its(:admin) { should be_true }
+      end
+
+      context "overriding dynamic attributes" do
+        subject     { create(:admin, :email => "perfect@example.com") }
+
+        its(:name)  { should == "Great User" }
+        its(:email) { should == "perfect@example.com" }
+        its(:admin) { should be_true }
+      end
+
+      context "overriding static attributes" do
+        subject     { create(:admin, :name => "wonderful") }
+
+        its(:name)  { should == "wonderful" }
+        its(:email) { should == "wonderful-modified@example.com" }
+        its(:admin) { should be_true }
+      end
+    end
+  end
+
+  it "doesn't overwrite already defined child's attributes" do
+    FactoryGirl.modify do
+      factory :user do
+        admin false
+      end
+    end
+    create(:admin).should be_admin
+  end
+
+  it "allows for overriding child classes" do
+    FactoryGirl.modify do
+      factory :admin do
+        admin false
+      end
+    end
+
+    create(:admin).should_not be_admin
+  end
+
+  it "raises an exception if the factory was not defined before" do
+    lambda {
+      FactoryGirl.modify do
+        factory :unknown_factory
+      end
+    }.should raise_error(ArgumentError)
+  end
+end

spec/factory_girl/attribute_list_spec.rb

@@ -1,5 +1,14 @@
 require "spec_helper"
 
+describe FactoryGirl::AttributeList, "overridable" do
+  it { should_not be_overridable }
+
+  it "can set itself as overridable" do
+    subject.overridable
+    subject.should be_overridable
+  end
+end
+
 describe FactoryGirl::AttributeList, "#define_attribute" do
   let(:static_attribute)  { FactoryGirl::Attribute::Static.new(:full_name, "value") }
   let(:dynamic_attribute) { FactoryGirl::Attribute::Dynamic.new(:email, lambda {|u| "#{u.full_name}@example.com" }) }
@@ -22,6 +31,18 @@ describe FactoryGirl::AttributeList, "#define_attribute" do
       2.times { subject.define_attribute(static_attribute) }
     }.to raise_error(FactoryGirl::AttributeDefinitionError, "Attribute already defined: full_name")
   end
+
+  context "when set as overridable" do
+    let(:static_attribute_with_same_name) { FactoryGirl::Attribute::Static.new(:full_name, "overridden value") }
+    before { subject.overridable }
+
+    it "redefines the attribute if the name already exists" do
+      subject.define_attribute(static_attribute)
+      subject.define_attribute(static_attribute_with_same_name)
+
+      subject.to_a.should == [static_attribute_with_same_name]
+    end
+  end
 end
 
 describe FactoryGirl::AttributeList, "#attribute_defined?" do
@@ -109,11 +130,42 @@ describe FactoryGirl::AttributeList, "#apply_attributes" do
     subject.to_a.should == [city_attribute, full_name_attribute, email_attribute, login_attribute]
   end
 
-  it "overwrites attributes that are already defined" do
+  it "doesn't overwrite attributes that are already defined" do
     subject.define_attribute(full_name_attribute)
     attribute_with_same_name = FactoryGirl::Attribute::Static.new(:full_name, "Benjamin Franklin")
 
     subject.apply_attributes([attribute_with_same_name])
-    subject.to_a.should == [attribute_with_same_name]
+    subject.to_a.should == [full_name_attribute]
+  end
+
+  context "when set as overridable" do
+    before { subject.overridable }
+
+    it "prepends applied attributes" do
+      subject.define_attribute(full_name_attribute)
+      subject.apply_attributes([city_attribute])
+      subject.to_a.should == [city_attribute, full_name_attribute]
+    end
+
+    it "moves non-static attributes to the end of the list" do
+      subject.define_attribute(full_name_attribute)
+      subject.apply_attributes([city_attribute, email_attribute])
+      subject.to_a.should == [city_attribute, full_name_attribute, email_attribute]
+    end
+
+    it "maintains order of non-static attributes" do
+      subject.define_attribute(full_name_attribute)
+      subject.define_attribute(login_attribute)
+      subject.apply_attributes([city_attribute, email_attribute])
+      subject.to_a.should == [city_attribute, full_name_attribute, email_attribute, login_attribute]
+    end
+
+    it "overwrites attributes that are already defined" do
+      subject.define_attribute(full_name_attribute)
+      attribute_with_same_name = FactoryGirl::Attribute::Static.new(:full_name, "Benjamin Franklin")
+
+      subject.apply_attributes([attribute_with_same_name])
+      subject.to_a.should == [attribute_with_same_name]
+    end
   end
 end

spec/factory_girl/factory_spec.rb

@@ -308,15 +308,17 @@ describe FactoryGirl::Factory, "human names" do
 end
 
 describe FactoryGirl::Factory, "running a factory" do
-  subject             { FactoryGirl::Factory.new(:user) }
-  let(:attribute)     { stub("attribute", :name => :name, :ignored => false, :add_to => nil, :aliases_for? => true) }
-  let(:proxy)         { stub("proxy", :result => "result", :set => nil) }
+  subject              { FactoryGirl::Factory.new(:user) }
+  let(:attribute)      { stub("attribute", :name => :name, :ignored => false, :add_to => nil, :aliases_for? => true) }
+  let(:attribute_list) { [attribute] }
+  let(:proxy)          { stub("proxy", :result => "result", :set => nil) }
 
   before do
     define_model("User", :name => :string)
     FactoryGirl::Attribute::Static.stubs(:new => attribute)
     FactoryGirl::Proxy::Build.stubs(:new => proxy)
-    FactoryGirl::AttributeList.stubs(:new => [attribute])
+    attribute_list.stubs(:apply_attributes)
+    FactoryGirl::AttributeList.stubs(:new => attribute_list)
   end
 
   it "creates the right proxy using the build class when running" do