public api for removing factories

[jrochkind]

FG already has a way to modify existing factories (FactoryGirl.modify),
which is great when there are factories supplied by a gem you need
to tweak.

But sometimes you need to remove entirely a factory supplied by a gem.
Often because you can't do things like remove an attribute or
callback from an existing factory. It's best just to remove it, and
start over from scratch with your own define.

This adds some simple public API for un-registering factories.

[houndci-bot]

Line is too long. [85/80]
Space missing to the left of {.

[houndci-bot]

Inconsistent indentation detected.

[jrochkind]

I have no idea how to make hound happy with code that makes sense here, sorry.

[joshuaclayton]

@jrochkind interested in the specific case(s) where this needed to happen. If you'd prefer not to share publicly and have a Twitter account, mind sending me a DM at the same Twitter handle as my GH handle?

[jrochkind]

Thanks for the response, I don't mind sharing publicly.

We develop apps against Spree (and lately now prefer solidus, a fork). It has a pretty complicated data model. It makes factories available to consuming apps, which is great and totally crucial for us to be able to write our own tests for our own local func that also involves the complicated schemas from solidus.

To do this effectively, we use FactoryGirl.modify extensively -- this is a great feature, thanks for it, it works well for us. We can add our own attributes, associations, and hooks, to deal with local constriants or features we've added to our own app(s) using Solidus, and still create appropriate factory objects.

But sometimes we need to remove an attribute, association, or feature from the solidus-provided factory. There's no good way to do this with modify. You can over-ride it to set it to nil, but setting to nil isn't always the same as if it wasn't there at all -- we may have actually removed the attribute entirely in our local app, so trying to set to nil will raise. Or it may be a model that has logic such that setting an attribute to nil isn't the same as if it were never set at all. And there's no way to remove after_create or other hooks at all with modify, they are there doing what they do, even if they're doing something inappropriate for our local app.

A specific example is the solidus user factory. The factory from spree has a password_confirmation attribute. Our local auth implementation uses clearance instead of devise, and does not use a password_confirmation field. Over-riding it in modify to be nil wouldn't help, User#password_confirmation= isn't a method in our model at all, it would still raise. We could add a no-op password_confirmation= method in our app just to make the factory happy, but that's obviously undesirable.

So we've ended up using the power of ruby's enough rope to remove the user factory entirely, FactoryGirl.factories.instance_variable_get('@items').delete(:users). It works, and has worked out well for us. Because FactoryGirl is so cleanly implemented, reaching in like this seems to work fine. But obviously it would be better to be using public API to do this, not reaching into an under-the-hood instance variable implementation.

While in this case it was an attribute, we could see the need to remove an association or hook too. An alternative to this API to remove a factory, we could imagine API to, in a FactoryGirl.modify, remove a particular attribute, association, or hook. That would be more targetted to what we really need, and would actually serve our needs even better. But it would require much more extensive lines of code changes, and especially in the case of hooks not sure if there's even a good way to make it so. This PR was very clean and straightforward, and is just what we are already doing (and is working out for us) made into FG API.

You might say, why can't you just not load the user factory from solidus at all? Then you won't need to remove it, you can just define your own from scratch? Because we need other solidus factories that depend on a user factory existing, and (appropriately IMO) require the user factory accordingly, so loading them loads the user factory from solidus, the one we need to remove an attribute from.

You might say, well, can solidus just provide API to control what it includes in factories? Maybe surround the password_confirmation { password } with a guard block, of some kind, like that factory is actually already doing for authentication_token. Sure, that's possible. But it's hard to predict in advance everything someone might want to omit, or to surround every single attribute/association/hook with a guard making the the factory source a mess. And the same argument really applies to FactoryGirl.modify, you wouldn't need it at all if gems providing factories just made everything super configurable instead. But modify is a lot cleaner code.

The ability to remove a factory can meet a host of unexpected/weird conditions where you need to remove an attribute/association/hook from a gem-supplied factory, when there's no API to do that, and it would be quite complicated or even infeasible to provide API to do that, but it's pretty straightforward to provide a "remove factory" API, as a last resort for meeting these needs. If you don't provide the public API, we will nonetheless keep doing the unfortunate FactoryGirl.factories.instance_variable_get('@items') thing, cause it is what we needed.

So that's my brief! :)

[jrochkind]

This is actually getting increasingly painful as I work on upgrading a particular spree/solidus app.

Any thoughts?

I can certainly keep using the internal API hackily.

[joshuaclayton]

@jrochkind thanks for the context; it helps a ton. The "pull in one, pull in all" aspect, especially given the tightly coupled data model, makes it tough, agreed.

I'd consider this a fairly impactful change; let me think on it/see if I can poke any holes in the logic/reason about other cases or issues that might come up. In the meantime, FG doesn't have a lot of active development so what you've got shouldn't break in the near-term.

[joshuaclayton]

Thoughts on whether this should raise if you attempt to delete something that doesn't exist, similar to find?

[jrochkind]

I think it should not raise. If you try to delete a factory that was never there in the first place, the outcome is the same either way -- the factory is no longer there. If it were to raise if not there, then you'd need a way to check if it was there first, but i don't think it's neccesary, it should happily no-op if not there. I forget if there was a test for that, I guess there should be.

[joshuaclayton]

My question then is, "Why would you, as a developer using factories from another library, attempt to delete something unless you know it's causing problems?" Is there something other than name that you're using to dictate whether something will be deleted, or is there significant churn in the provided Spree/Solidus factories file?

I imagine if you attempt to delete something that's not there, it's probably a line of code you wouldn't want to exist in the codebase, since it arguably has no value.

[jrochkind]

I would not want to delete something unless I knew it's causing problems, but if a future release of that dependency removes the factory altogether, I don't know if I really want it raising -- the same side effects are there either way, there is no factory present with that name after the line of code is executed.

But I see what you're saying, if you feel strongly about it I can make it raise. Which would eliminate the other question over what it should return, probably.

[composerinteralia]

Since there hasn't been any activity on this for quite some time now, I am going to close. We can pick this up down the line if it comes up again.