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

Add support for lazy attributes #131

Closed
wants to merge 2 commits into from

Conversation

moklett
Copy link

@moklett moklett commented May 14, 2016

You may defer calculation of your attributes, and even base one attribute upon another, by using functions:

def hero_factory do
  %{
    creature: "Bat",
    nickname: &"#{&1.creature}man",
    vehicle: &"#{&1.creature}mobile"
  }
end

build(:hero)
 # => %{creature: "Bat", nickname: "Batman", vehicle: "Batmobile"}
build(:hero, creature: "Spider", vehicle: "webs")
 # => %{creature: "Spider", nickname: "Spiderman", vehicle: "webs"}

This is especially helpful in complex object graphs where a record and its child both belong to the same parent. This is common in multi-tenant apps, where many of the records belong to the same scope. Consider a multi-site blog platform where Articles and Authors both belong to a Site, and Articles also belong to the User:

Site <---- Author
  ^          ^
  |          |
  +------- Article

Such factories would be:

def site_factory do
  %Site{
    name: "Awesome Blog"
  }
end

def author_factory do
  %Author{
    name: "Danny Tanner",
    email: "danny@example.com",
    site: build(:site)
  }
end

def article_factory do
  %Article{
    title: "The House is Full",
    site: create(:site), # Unfortunately, site creation can't be deferred or Author won't see it
    author: build(:author, site: &(&1.site))
  }
end

Without lazy attributes, this could be accomplished through something like:

def article_factory do
  # Don't do this
  site = create(:site) # Site gets created despite the attributes you pass in build/insert :(
  %Article{
    site: site,
    title: "The House is Full",
    author: build(:author, site: site)
  }
end

But with lazy attributes, we can pass in the Site, keeping your inserts to a minimum:

def article_factory do
  %Article{
    site: create(:site),
    title: "The House is Full",
    author: build(:author, site: &(&1.site))
  }
end

article_for_auto_generated_site = build(:article)
 # => %Article{site: %Site{id: 1, ...}, author: %Author{site: %Site{id: 1, ...}, ...}}

specific_site = create(:site, name: "Special Blog")
article_for_specific_site = build(:article, site: specific_site)
 # => %Article{site: %Site{id: 2, ...}, author: %Author{site: %Site{id: 2, ...}, ...}}

You may defer calculation of your attributes, and even base one attribute upon
another, by using functions:

```elixir
def hero_factory do
  %{
    creature: "Bat",
    nickname: &"#{&1.creature}man",
    vehicle: &"#{&1.creature}mobile"
  }
end

build(:hero)
 # => %{creature: "Bat", nickname: "Batman", vehicle: "Batmobile"}
build(:hero, creature: "Spider", vehicle: "webs")
 # => %{creature: "Spider", nickname: "Spiderman", vehicle: "webs"}
```

This is especially helpful in complex object graphs where a record and its
child both belong to the same parent.  This is common in multi-tenant apps,
where many of the records belong to the same scope.  Consider a multi-site blog
platform where Articles and Authors both belong to a Site, and Articles also
belong to the User:

```txt
Site <---- Author
  ^          ^
  |          |
  +------- Article
```

Such factories would be:

```elixir
def site_factory do
  %Site{
    name: "Awesome Blog"
  }
end

def author_factory do
  %Author{
    name: "Danny Tanner",
    email: "danny@example.com",
    site: build(:site)
  }
end

def article_factory do
  %Article{
    title: "The House is Full",
    site: create(:site), # Unfortunately, site creation can't be deferred or Author won't see it
    author: build(:author, site: &(&1.site))
  }
end
```

Without lazy attributes, this could be accomplished through something like:

```elixir
def article_factory do
  # Don't do this
  site = create(:site) # Site gets created despite the attributes you pass in build/insert :(
  %Article{
    site: site,
    title: "The House is Full",
    author: build(:author, site: site)
  }
end
```

But with lazy attributes, we can pass in the Site, keeping your inserts to a
minimum:

```elixir
def article_factory do
  %Article{
    site: create(:site),
    title: "The House is Full",
    author: build(:author, site: &(&1.site))
  }
end

article_for_auto_generated_site = build(:article)
 # => %Article{site: %Site{id: 1, ...}, author: %Author{site: %Site{id: 1, ...}, ...}}

specific_site = create(:site, name: "Special Blog")
article_for_specific_site = build(:article, site: specific_site)
 # => %Article{site: %Site{id: 2, ...}, author: %Author{site: %Site{id: 2, ...}, ...}}
```
%Article{
title: "The House is Full",
site: create(:site), # Unfortunately, site creation can't be deferred or Author won't see it
author: build(:author, site: &(&1.site))
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The above is sub-optimal, but I don't know how to solve it yet. I'd prefer to be able to do this:

site: &create(:site), # Defer creation until we know we need it, so it isn't created if it will be overwritten by attributes
author: build(:author, site: &(&1.site))

But there's a problem: if author gets resolved first, then &1.site will return a function instead of a site - there's a circular dependency. Maybe we should just add a warning in the README to avoid circular dependencies?

@paulcsmith
Copy link
Contributor

@moklett I've been a bit busy lately, but I like the idea of this PR. The one thing I'm concerned about is that I'm worried people will use abuse this and make some complicated factories. I think simplify the documentation and avoiding using it for associations would be the best bet. People can do that if they need to, but I think keeping the docs simpler will encourage simpler use cases. I think most of the time lazy attributes won't be needed for associations. What do you think?

@pablo-co
Copy link

pablo-co commented Sep 6, 2016

👍 for this PR. I also agree this probably shouldn't be on the README but rather on the documentation for people looking for more advanced uses. I was just about to open a PR with the use case of basing attributes upon others. In my case there is correlation between the values of a couple attributes, this correlation is necessary for some tests to pass and it gets a little cumbersome at times.

@jsteiner jsteiner mentioned this pull request Nov 4, 2016
@marnen
Copy link

marnen commented Dec 5, 2016

@pablo-co:

I also agree this probably shouldn't be on the README but rather on the documentation

I might agree with that, if it were not for the fact that the documentation basically points users to the README for usage examples. FactoryGirl has dependent attributes documented very clearly in the [https://github.com/thoughtbot/factory_girl/blob/master/GETTING_STARTED.md](GETTING_STARTED file); since ex_machina doesn't have anything like that, the README is probably the best place.

@marnen
Copy link

marnen commented Dec 5, 2016

@paulcsmith

The one thing I'm concerned about is that I'm worried people will use abuse this and make some complicated factories.

But when it's needed, it's really needed. Please merge this PR once conflicts are resolved...

@gmile
Copy link

gmile commented Dec 21, 2016

@moklett @paulcsmith apart from changes related to Readme.md & the branch being not up to date, is there anything specific that blocks this PR?

@paulcsmith
Copy link
Contributor

@moklett @gmile Sorry for the late response. We've been very busy and we can't really support a feature we're not sure we'd use, so for now we won't be merging this in, though we may in the future.

I think the best track forward would be to extract a library that uses ExMachina.Strategy. Maybe strategies for build_fn and insert_fn (not the best names, but I think you get the idea). That way the community can still use it, and if it gains enough traction we can consider moving it into core. What do you think?

@paulcsmith paulcsmith closed this Feb 24, 2017
@marnen
Copy link

marnen commented Feb 25, 2017

@paulcsmith:

We've been very busy and we can't really support a feature we're not sure we'd use

Um, what? Clearly there's a lot of community demand for this, which means there would probably be a lot of community support. Anyway, the way that a library like this matures is for others in the community to add features that they would use, even if the original maintainer doesn't.

If you're too busy to provide this level of maintenance, perhaps an additional maintainer can step up from the community. (I'd volunteer if I actually had any experience with Elixir; since I'm a beginner with it, I think that's probably not a great idea.)

@paulcsmith
Copy link
Contributor

@marnen The problem is two-fold. We don't have much time, and we're not sure this is the best way to handle the problem. We don't want to add it, support it, and then find out later that it's not quite right and not be able to remove it. That is why I suggested someone making another library. That way it can be maintained separately, we can gauge how useful it is, and if it gains wide community adoption we can add it to ExMachina core. If it doesn't gain wide adoption or we find problems with it, we can think of some other way to solve the problem based on what we've learned from the other library.

I hope that makes sense

@marnen
Copy link

marnen commented Feb 25, 2017

@paulcsmith I suppose it does, in the abstract, except that the idea of "not being able to remove it" seems potentially misguided to me: if code is sufficiently modular, misfeatures can always be removed (perhaps abstracted into their own libraries).

I do know that dependent attributes are one of Factory Girl's killer features as far as I'm concerned, so the idea that they might somehow not be appropriate for Ex Machina seems odd as well.

Also, I understand not wanting to break a library, but this approach seems conservative to the point of timidity. If this is not quite the right implementation of the feature, perhaps it can be improved later, but in the meantime it works, and the library becomes more useful to more people.

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

Successfully merging this pull request may close these issues.

None yet

5 participants