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

Using name-spaced models #199

Closed
ream88 opened this issue Sep 10, 2011 · 28 comments
Closed

Using name-spaced models #199

ream88 opened this issue Sep 10, 2011 · 28 comments

Comments

@ream88
Copy link

ream88 commented Sep 10, 2011

Hello folks,

I have Rails projects with name-spaced models. At the moment my factories look like the following with a lot of clutter, because of the name-spaced models:

# factories/user.rb
FactoryGirl.define do
  factory(:account, :class => MyProject::User::Account) do
    # ...
  end

  factory(:user, :class => MyProject::User::User) do
    # ...
  end

  # and so on ...
end

They are working, though it doesn't feel very DRY. Is (or will be) there any way to define the name-space once? Like the following?

FactoryGirl.define(:namespace => MyProject::User) do
  factory(:account) do
    # I'm a MyProject::User::Account class
  end
end
@joshuaclayton
Copy link
Contributor

There's not a way to do this in Factory Girl specifically, and I don't think it's necessary to add. If you're interested in reducing the noise from the namespacing, I'd think you could create a local variable to reference the full namespace at the top of your file and use that throughout the definitions.

user_ns = MyProject::User
FactoryGirl.define do
  factory :account, :class => user_ns::Account do
    # attributes
  end
end

@bobbus
Copy link

bobbus commented Aug 24, 2012

I think this feature could be interesting, especially when developping in a namespaced engine with Rails which I believe is now a common usage.
Without it, there is a lot of repetition :

factory :product_variation, class: Engine::ProductVariation
factory :category, class: Engine::Category

@joshuaclayton , Don't you think so?

@joshuaclayton
Copy link
Contributor

I'm still standing behind my comment about assigning a constant to a local variable to reuse; the change wouldn't be insignificant and I don't think it's a common enough problem to have FG address it specifically.

@jpfuentes2
Copy link

It's a problem for me because I have a DB with 200+ tables and they are grouped into "modules." Hence, we use namespaces in our code. Not to mention, I'd like to easily leverage factory_girl for PORO's which are often namespaced as they are logically grouped into modules.

I suppose I understand that you think it's not common enough to address it specifically, but I imagine this can't be but so hard to patch in. Would you accept a PR?

The local variable assignment of a constant is not an elegant "work around" as produces noise in your codebase. It's unconventional to see namespaces referenced as local variables.

@bobbus
Copy link

bobbus commented Oct 11, 2012

The local variable assignment of a constant is not an elegant "work around" as produces noise in your codebase. It's unconventional to see namespaces referenced as local variables.

+1 :)

@timscott
Copy link

timscott commented Nov 9, 2012

+1 for clean namespace support

@bradrobertson
Copy link

this would be useful to us also...

@0xCCD
Copy link

0xCCD commented Apr 23, 2013

+1 for clean Namespacing implementation

@joshuaclayton
Copy link
Contributor

Based on the number of requests for this feature, if there was a clean and well-tested PR, I would definitely consider getting it merged.

@tf
Copy link

tf commented Jun 20, 2014

Since I have come across this issue a couple of times now, I'll try to lead the discussion to a more technical level. I am also very interested in improving factory girl in the context of engines. So I'll split this post into two parts:

  1. A suggestion how to implement a :module option to DRY up constant usage.
  2. Some rough sketches how I could imagine this feature to fit in with further engine support improvements.

I've pushed a first draft implementation to tf/factory_girl:tf-module. I've added an acceptance test for the new behavior. If we a agree on the general direction, I'd continue with some unit level specs for Factory#build_class.

The basic idea is to a add a :module option (in alignment with the Rails router API), which is used when either

  • the class option is a String or
  • the class_name is guessed from the factory name.

In a second step, an optional default_options argument is added to FactoryGirl.define and passed to FactoryGirl::Syntax::Default::DSL to be reverse merged in every factory call.

This enables the following usage:

FactoryGirl.define module: SomeEngine do
  factory :user do
  end
end

FactoryGirl.create(:user).is_a?(SomeEngine::User)

While this change would make working with namespaced models a bit less verbose, the really interesting thing to me is reusing factories defined in gems/engines. As it stands, this is already possible, but all factories share a global namespace.

I think it would be great if factories could be defined in different scopes:

FactoryGirl.define scope: :some_gem, module: SomeGem do
  factory :user do
  end

  factory :entry do
  end
end

Just like in the Rails router a :namespace option could then be added set :module and scope at once.

There could be a default_scope option letting a gem use its factories the normal way. For dependent apps/gems I could imagine two possibilities. Either names have to be prefixed with the scope name:

FactoryGirl.create(:some_gem_user)

or there could be some sort of import API:

FactoryGirl.define do
  import :some_gem, :user
  import :some_gem, :entry, as: external_entry
end

The cleanest way to implement that would probably be to allow for different FacotryGirl::Configurations to exist for each scope. From a quick glance at the code, this might be a bigger change, since the global FactoryGirl.register_* methods are used throughout the code as a facade in front of the FactoryGirl::Configuration.

Still, I think this could really be a worthwhile endeavor, to provide great tooling for engines to supply dependent apps with first class testing facilities.

@cyril-sf
Copy link

I'm bumping into that problem with a "simple" use case: I use Spree in one of my application.

I wanted to write some tests, some requiring to manipulate Spree records. They already offer factories, but when requiring those files I run into name conflicts:

Factory already registered: address (FactoryGirl::DuplicateDefinitionError)

Namespacing would typically avoid that kind of issue.

@ReneB
Copy link

ReneB commented Aug 21, 2014

@tf Without having seen the code in your branch, as a new FactoryGirl user I think the :create call is a little ambiguous. I think it would be better if it required some explicit 'namespace' or 'module' option, even if there is only one :user factory defined. (Some context: I work on a project that makes heavy use of namespacing and has several repeated class names, and I'm currently migrating from Machinist 1 to FactoryGirl).

A major reason for requiring explicit namespacing on creation would be that adding a new User class in a different namespace would require going back to all the old create (build, etc) calls and explicitly namespace them because the factory lookup is now ambiguous.

I could envision a FactoryGirl.namespace(namespace) do ... end block in which all calls to create (build, etc) are sought in a specific scope, but I'm not sure if this would tickle anyone's fancy as much as it does mine.

@tf
Copy link

tf commented Aug 22, 2014

@ReneB well, I'd argue that this ambiguity is already there with the current state of things. Right now you'd probably have to define the two factories as:

factory :module_a_user, class: ModuleA::User do
end

factory :module_b_user, class: ModuleB::User do
end

With the suggested change you could do:

FactoryGirl.define namespace: :module_a do
  factory :user do
  end
end

FactoryGirl.define namespace: :module_b do
  factory :user do
  end
end

create(:module_a_user)

Your FactoryGirl.namespace method could even be implemented, by allowing to change the default_scope for a block. At the moment, I do not really see the value of being able to write:

create(:user, scope: :module_a)

But really there is no reason why this should not be possible.

@ReneB
Copy link

ReneB commented Aug 22, 2014

@tf I think maybe I should read your code to get things a little clearer, but just to make sure we understand each other: what I was commenting on, was this example:

FactoryGirl.define module: SomeEngine do
  factory :user do
  end
end

FactoryGirl.create(:user).is_a?(SomeEngine::User)

I think this factory should not be used when create(:user) is called, but only when either create(:some_engine_user) or some call with an explicit namespace, scope or module parameter (I have no real naming preference here). Otherwise, adding a new, global user factory would break all existing calls to factories. What I'm saying is "force the programmer to be explicit in some way". All the other suggestyions are just there to provide some ideas for ways in which they could be explicit.

@tf
Copy link

tf commented Aug 26, 2014

I totally agree that the module option alone does not help in any way with the namespacing problem. Still, I'm not sure what a sensible first step towards some concept of scope could be. Using fully qualified factory names as described in your example is possible already today. Still, from my point of view, having to repeat the module/gem/library name in every create call is also far from ideal and hinders code mobility.

The way I see it, this is one of the cases where inventing a DSL reintroduced problems that are originally solved in the host language. In ruby a class defined as ModuleA::User can be referred to as User in other classes inside ModuleA, still there is no risk of naming collision with a ModuleB::User. Re-modelling constant definition via define/factory calls, this concept got lost and now has to be reconstructed somehow by concepts like scope or prefixed names.

@ReneB
Copy link

ReneB commented Aug 27, 2014

@tf well what you're saying here makes complete sense to me and I think that maybe those semantics are really what we want:

FactoryGirl.define module: SomeEngine do
  factory :user do end  # will build a SomeEngine::User

  factory :profile do # will build a SomeEngine::Profile
    user # gets evaluated to the 'user' factory in the 'some_engine' module
  end
end

FactoryGirl.define do
  factory :profile do
    association :user, module: :some_engine
  end

  factory :wrong_profile do
    user # this won't work
  end
end

I think, in order to keep the DSL a little DRY, it should be possible to open modules, scopes, namespaces or whatchamacallit in define, factory or possibly specific FactoryGirl.scope blocks. Also, I'm not really sure what the equivalent of '::' would be - building a ::User from a SomeEngine namespace would require an explicit module escape.

@tf
Copy link

tf commented Aug 30, 2014

Your ideas resonate a lot with my thoughts about scoping. But details aside, all of this would require a major change as to allow the existence of multiple FactoryGirl::Configuration instances. Then different engines could provide factories, sequences and callbacks in isolated namespaces. At the moment though DSL manipulates the configuration via the static FactoryGirl.register_* methods which basically turns the configuration into global state.

From my point of view, refactoring towards a more decoupled solution would be an interesting improvement for the code base in itself. This would probably have to be a group afford preceded by a much more thorough discussion. Still, I'm hesitant as long as there has been no signal from the maintainers that work on such a change would be supported and is wished for. Let's not forget that our discussion takes place in a closed issue ;)

@orbanbotond
Copy link

+1 for namespace support.

I am extracting a gem from an app and I would need that feature... I will use the class: Namespace:Class option until then

@jkoisch
Copy link

jkoisch commented Nov 21, 2014

+1 for namespace support.

I am writing integration components that map from XCompany::Product to YCompany::Product. Both ends deserve testing.

@joshuaclayton
Copy link
Contributor

To follow up on this thread, there's a couple of examples people have given,
and we're now running into edge cases.

One example to define factories:

FactoryGirl.define namespace: Whatever do
  factory :user
  factory :profile
end

The questions I currently have are:

  1. How do you reference this namespaced factory? Current options I can come up with include FactoryGirl.create :whatever_user (snakecasing the namespace and prefixing the factory name) and FactoryGirl.create :user, namespace: Whatever (pretty verbose!).
  2. How are namespaces handled in associations/references/dynamic attributes?

Here's how to handle namespacing currently:

FactoryGirl.define do
  factory :user, class: "Some::Namespace::User" do
    profile
  end

  factory :profile, class: "Some::Namespace::Profile"
end

With prefixing so similarly-named objects under different namespaces coexist:

FactoryGirl.define do
  factory :some_namespace_user, class: "Some::Namespace::User" do
    some_namespace_profile
  end

  factory :some_namespace_profile, class: "Some::Namespace::Profile"

  factory :profile
  factory :user do
    profile
  end
end

I think introducing another level of abstraction by introducing prefixes is
not obvious, as it implies understanding of how namespaces modify the actual
names when building/creating them. There's currently no other purely dynamic
way of generating factories, and I'd like to continue to avoid it if possible.

That leads to a solution like this:

FactoryGirl.define namespace: "Some::Namespace" do
  factory :user do
    profile # would always reference the profile defined in this define block - and would raise if the profile factory didn't exist
  end

  factory :profile
end

FactoryGirl.define do
  factory :user do
    profile
  end

  factory :profile
end

This reduces the clutter of defining the class name for each factory but makes
it difficult to have similarly-named factories coexist:

FactoryGirl.define namespace: "Some::Namespace" do
  factory :user do
    profile # the profile would need to be defined here
  end

  factory :profile
end

# This would result in duplicate definitions
FactoryGirl.define do
  factory :user do
    profile
  end

  factory :profile
end

To be clear, I'm not opposed to more robust namespace support - but I don't
feel one exists yet which addresses all of these points and still feels like
it fits within Factory Girl.

@tf
Copy link

tf commented Dec 4, 2014

As described in my post above, the main issue, from my point of view, is that factory names currently live in a single, global scope.

It should be possible for two libraries/gems/modules to each define a :user and a :profile factory without stepping on each other's toes or having to resort to prefixed factory names. I'd suggest adding a :scope option to FactoryGirl.define which basically causes all definitions to go into a different FactoryGirl::Configuration object.

This would answer your question 2: Associations reference factories from the same scope the enclosing factory is defined in. Same goes for sequences.

Regarding question 1: Referencing factories from different scopes via prefixing surely would require dynamic resolution/generation of factory names. To avoid this, one could introduce an explicit import mechanism.

# some_gem/spec/factories/users.rb
FactoryGirl.define scope: :some_gem, module: 'SomeGem' do
  factory :user do
    profile
  end
end

# some_gem/spec/factories/profiles.rb
FactoryGirl.define scope: :some_gem, module: 'SomeGem' do
  factory :profile do
  end
end

# some_app/spec/factories/some_gem.rb
FactoryGirl.define do
  import_factory :some_gem, :user, as: :some_gem_user
  #or
  factory :some_gem_user do
    import :some_gem, :user
  end
end

Now when running the specs of some_gem the default scope could be configured like this, allowing unprefixed usage:

# some_gem/spec/spec_helper.rb
FactoryGirl.default_scope = :some_gem

# some_gem/spec/some_spec.rb
create(:user).is_a?(SomeGem::User)
create(:profile).is_a?(SomeGem::Profile)

while in the depending app there is an explicitly defined factory that imports the definition from the gem:

# some_app/spec/some_spec.rb
create(:some_gem_user).is_a?(SomeGem::User)

The scope/default scope concept would also be completely backward compatible: Supplying no :scope option to FactoryGirl.define is the same as scope: :default which is also the default value of FactoryGirl.default_scope.

Does this make sense?

@djnawara
Copy link

FYI, given this class:

module ModuleA
  class Foo
    attr_accessor :uuid
  end
end

Then this works for me:

module ModuleA
  FactoryGirl.define do
    factory :module_a_foo, class: Foo do
      sequence(:uuid) { UUIDTools::UUID.random_create }
    end
  end
end

So, in theory, you could do something like this:

module ModuleA
  FactoryGirl.define(factory_name_prefix: :module_a) do
    factory :foo, class: Foo do
      sequence(:uuid) { UUIDTools::UUID.random_create }
    end
  end
end

FactoryGirl.build(:module_a_foo)

Just a random thought.

@tf
Copy link

tf commented Jan 14, 2015

Yes, but this does not solve the problem of having to repeat the, for example, gem name all over the gem codebase. Always having to write FactoryGirl.build(:some_engine_post) hinders code mobility and makes extracting engines hard when things originally started out as FactoryGirl.build(:post). This is the sort of duplication I'd like to prevent by adding the scope concept.

@cec
Copy link

cec commented Sep 17, 2015

@joshuaclayton I believe that having this in FG would benefit not only engine creators, since namespaced models are not uncommon in non-trivial apps.

@tf Thank you for leading this discussion.
I believe the scoping idea to be the way to go.
When writing specs for a namespaced models, it would be nice to be able to set the default_scope for those specs individually.

module NameSpace
  describe MyModel, type: model, factories_scope: NameSpace do
    #....
  end
end

Implementing this would be trivial.

@mooreniemi
Copy link

I wish I had this now, as I am struggling with a large project with many name-spaces.

@joshuaclayton
Copy link
Contributor

joshuaclayton commented Jun 7, 2016

I still have a number of concerns around edge cases, implied global state around RSpec blocks, and overhead (see #199 (comment) for history).

I'd love to see people implement this without modifying FG and see what they run into. A few different implementations might all help us understand what issues might arise.

As an example of namespace support with zero modification of FG, please take a look at 652818b. This demonstrates default namespaces, namespaces wrapping multiple factories, and automatic name prefixing in instances where the same class name exists under multiple namespaces.

Note that this doesn't handle association prefixes, which I demonstrated in the Bar namespace.

@evgen3d
Copy link

evgen3d commented Mar 5, 2017

Any progress with it?
I really cant understand why this in discussion a few years already.
Rails Engines is most powerful encapsulation tool and I cant find any solution to make FactoryGirl work with namespaces.
Joshua, please provide working example of it!
This is not working in my case.
This is in mainapp/spec/factories.rb:

FactoryGirl.define do
  factory :eb_user, class: Eb::User do
   # ----
  end
end
NameError:
       uninitialized constant Eb::User

I spend two days already to make it work without any success.
Could you advice any solution?
Thanks

@joshuaclayton
Copy link
Contributor

@evgen3d without access to the codebase, I'm afraid I can't offer much help. Seems this is related to load order (especially if you expect that constant to be available), but I don't have specific guidance here, apart from ensuring the engine is loaded before the factories file is.

zspencer added a commit to zinc-collective/factory_bot that referenced this issue Nov 24, 2020
See: thoughtbot#199

I have a number of clients who use FactoryBot and are working through
the difficult challene of decomposing a large monolith into either
domain-driven namespaces; or small gems/engines that are "plugged into"
a larger monolith.

In most cases, we've been able to get away with keeping the factories
bundled with their extracted modules and using distinct names.

However there's also been a bit of copy-pasting of factory code back into the monolith in cases where the names really _do_ need to be shared.

This is an attempt to bring @joshuaclayton's example of namespacing into
the core FactoryBot DSL.

If you'd prefer I packaged this as it's own gem, I'd be happy to do so;
but I'd be stoked to see this in the core FactoryBot package!
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests