Skip to content

Magic alias ignoring is blocking set of attribute :foo_id if :foo is an override #311

Closed
ijcd opened this Issue Mar 9, 2012 · 7 comments

2 participants

@ijcd
ijcd commented Mar 9, 2012

I'm trying to use FactoryGirl to create ActiveResource mocks (where there aren't real associations), but AttributeAssigner#alias_names_to_ignore has some magic in it that's preventing me from doing the following:

ignore do
  merchant { Factory(:merchant) }
end

merchant_name { merchant.name }
merchant_id   { merchant.id   }

Where the idea is:

Factory(:resource, :merchant => Factory(:merchant, ...))

The problem is that merchant_id gets silently ignored when :merchant is passed in as an override. Is there a way to override this behavior somehow? I can supply a patch if enlightened as to the purpose of this code.

@joshuaclayton
thoughtbot, inc. member

Are you currently using Factory.alias in your code? FactoryGirl shouldn't interfere with what you're trying to do...

@ijcd
ijcd commented Mar 9, 2012

No, not directly, but I think it gets triggered no matter what. From AttributeAssigner#attribute_names_to_assign (which is used in all assignments, I believe):

def attribute_names_to_assign
  non_ignored_attribute_names + override_names - ignored_attribute_names - alias_names_to_ignore
end

notice the "- alias_names_to_ignore", which is:

def alias_names_to_ignore
  @attribute_list.reject(&:ignored).map do |attribute|
    override_names.map {|override| attribute.name if attribute.alias_for?(override) && attribute.name != override }
  end.flatten.compact
end

This will include attribute names (merchant_id) if they match the override as an alias (merchant). I don't see how needing to use Factory.alias triggers this.

What is the purpose of this code here (checking overrides against aliases?) I see a test for "doesn't assign both an association and its foreign key". Is it related to that?

Perhaps the check here needs to also confirm that it is dealing with an association, and not just an override, since it's assuming that merchant/merchant_id are implicitly tied (true for association, but maybe not otherwise)

I can code up a minimal example if this is unconvincing.

@joshuaclayton
thoughtbot, inc. member

Yeah, if you could fork the code, create a branch, and write a failing test within spec/acceptance, that'd be amazingly helpful here. Thanks!

@joshuaclayton
thoughtbot, inc. member

Actually, I see what you're saying re: alias_for? here.

You're right, it's related to the test for assigning both the object and its FK; we let ActiveRecord (or whatever ORM) handle that.

You could just change the ignored object to not be named merchant, or just allow merchant be assigned and not assign merchant_id (likely this logic should be in the model anyway). merchant_name should continue to work fine.

@ijcd
ijcd commented Mar 9, 2012

This isn't an ORM. I'm just using, essentially, bare objects (ActiveResource). I need to assign merchant_name and merchant_id and it's convenient to pass in a :merchant and use the transient feature (since this is what it seems to be designed for). It seems wrong to me that FactoryGirl would presume I don't want to assign merchant_id if I pass in merchant. I most certainly do!

I'll come up with a failing test, and perhaps, a patch.

@joshuaclayton
thoughtbot, inc. member

I think the behavior would be that FactoryGirl only removes whatever_id if whatever is not ignored; when it's ignored, it should give precedence to whatever_id and not worry about replacing it, since it'll never actually get set on the model anyways. After talking this out and hearing the use case, I think this sounds great. I'd definitely love to see a pull req. for it.

@ijcd ijcd added a commit to ijcd/factory_girl that referenced this issue Mar 14, 2012
@ijcd ijcd Do not ignore alias names of transient attributes. (Fixes #311) 0d7520e
@ijcd
ijcd commented Mar 14, 2012

Pull request sent: #315

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.