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

fields_for/params_for and belongs_to associations #104

Closed
geofflane opened this issue Mar 17, 2016 · 5 comments
Closed

fields_for/params_for and belongs_to associations #104

geofflane opened this issue Mar 17, 2016 · 5 comments

Comments

@geofflane
Copy link
Contributor

fields_for (now params_for) doesn't populate the IDs of relations when you build them.

Imagine you're modeling a play in a sport...

  schema "plays" do
    belongs_to :game, Namespace.Game
    belongs_to :possession_team, Namespace.Team
...
  end

With

def factory(:play) do
  %Namespace.Play{
    game: create(:game),
    possession_team: create(:team),
  }
end

When you do fields_for(:play) game_id and possession_team_id are nil. It makes it harder to use for sending data to an API, for example. It would be nice if those could be populated.

Technically when you do build(:play) the same is true. It seems like Ecto doesn't populate those IDs until after it's saved?

@paulcsmith
Copy link
Contributor

@geofflane This is a good point. This is actually expected behavior. Since Ecto doesn't have an id until the record is inserted we can't set the id. Since params_for calls build and not create, it does not get the id.

We could call insert when using params_for, but that could cause performance issues and bugs. Since we would insert the record and the association it would require more database inserts, and it would also insert the record. So if you're testing a create endpoint you would already have those records in the database, making it impossible (or very difficult) to test.

I do see your point though. We have code in one of our apps that does something like this

def play_params do
  game = create(:game)
  posession_team = create(:posession_team)
  params_for(:play, game_id: game.id, posession_team: posession_team.id)
end

Maybe it would be useful to have an option for creating associations when using params_for, but I'm not sure how that would look, or if it would be more trouble than it's worth. params_for_with_associations(:user)?

For now I think it would be best to write your own function like the one above, but I'm open to ideas!

@geofflane
Copy link
Contributor Author

@paulcsmith I mainly put this issue in because it was unexpected behavior to me. The create "does the right thing" and builds the graph of objects in the DB, but the build/params only kind of does. (That params_for model is exactly what I came up with too. I found that pain because I added a constraint that caused a bunch of my tests to fail so I had to go add those associations manually in a bunch of places.)

The use case, for me, is testing an API POST to create a new instance of a thing. I wanted a way to easily construct an instance, but also to have dependent relations exist already. There might be a hybrid in there somewhere between create and build/params_for maybe to support that case?

@paulcsmith
Copy link
Contributor

@geofflane Sorry I probably should have phrased that better. What I meant by "expected behavior" was that it is not a bug, but I can definitely see how it is unexpected. One thing I would recommend is to add the params function to your factory or in a module that you import in your tests, that way when you need to change how it works you only need to do it in one place.

As for the params_for function, I doubt that we'll change it to insert associations by default because that could cause unintended side effects. I'm not opposed to adding another function like params_with_association_ids_for(:user), or full_params_for(:user) or something like that.

What do you think @jsteiner. Also @derekprior, we had to create a custom function for handling relationships on our project. What do you think of having a function that inserts associations and sets their ids?

@geofflane
Copy link
Contributor Author

@paulcsmith No apologies needed, I understood what you meant. :)

@paulcsmith
Copy link
Contributor

Thanks @geofflane! I created another issue #105 for implementing this. Feel free to tackle it if you have time :) If not I can get it in a few weeks

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

2 participants