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

Allow belongs_to via changesets #1102

Closed
josevalim opened this issue Nov 27, 2015 · 5 comments
Closed

Allow belongs_to via changesets #1102

josevalim opened this issue Nov 27, 2015 · 5 comments
Milestone

Comments

@josevalim
Copy link
Member

The main blocker for this feature in the past was how callbacks would work. Given callbacks are on the way out, this feature will be straight-forward to implement. It is pending on callbacks being effectively removed.

@josevalim josevalim added this to the v2.0 milestone Nov 28, 2015
@josevalim josevalim changed the title Allow belongs_to via changesets and auto inserts Allow belongs_to via changesets Nov 28, 2015
@josevalim
Copy link
Member Author

We should also support on_delete for belongs_to once we add this.

@josevalim
Copy link
Member Author

This is now in master.

@paulcsmith, could you please check if, when using Ecto master, ExMachina no longer needs any of the Ecto code? For example, we support inserting the whole tree directly, like here:

https://github.com/elixir-lang/ecto/blob/6f1971f4120b84e1a441792feb77ba451c4fc783/integration_test/cases/repo.exs#L720-L743

@paulcsmith
Copy link
Contributor

@josevalim This will be so awesome! I was able to remove all of the Ecto code and still have almost all the tests passing

It almost worked. These two tests failed when the association was %Ecto.Association.NotLoaded{}

https://github.com/thoughtbot/ex_machina/blob/4443b72b677387a09fbc6c1351b29c526a0ca061/test/ex_machina/ecto_has_many_test.exs#L120

https://github.com/thoughtbot/ex_machina/blob/4443b72b677387a09fbc6c1351b29c526a0ca061/test/ex_machina/ecto_test.exs#L142

Here's the stack trace:

** (FunctionClauseError) no function clause matching in Ecto.Changeset.Relation.do_empty/1
     stacktrace:
       (ecto) lib/ecto/changeset/relation.ex:24: Ecto.Changeset.Relation.do_empty(nil)
       (ecto) lib/ecto/repo/schema.ex:377: anonymous fn/4 in Ecto.Repo.Schema.surface_changes/4
       (elixir) lib/enum.ex:1473: Enum."-reduce/3-lists^foldl/2-0-"/3
       (ecto) lib/ecto/repo/schema.ex:366: Ecto.Repo.Schema.surface_changes/4
       (ecto) lib/ecto/repo/schema.ex:128: Ecto.Repo.Schema.do_insert/4
       (ecto) lib/ecto/repo/schema.ex:76: Ecto.Repo.Schema.insert!/4
       test/ex_machina/ecto_test.exs:141

I'll pull the latest Ecto and see if it's a problem in Ecto or in ExMachina and Ecto together. I'll open a PR if there's a problem in Ecto

@paulcsmith
Copy link
Contributor

@josevalim So Ecto is totally fine. It fails if you explicitly set an Ecto.Association.NotLoaded without a cardinality, which would never happen in real life. I just did it in the test to make sure it handled unloaded associations.

So I just need to update the tests in ExMachina once Ecto 2.0 drops. Ecto side looks great 👍

@josevalim
Copy link
Member Author

That's so awesome, I am so excited! :D

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants