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

Pass repo_options to repo.insert! to support TenantFactories #377

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 18 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -370,6 +370,24 @@ change this line in `mix.exs`:
defp elixirc_paths(:test), do: ["lib", "web", "test/support", "test/factories"]
```

## Multi-tenant support

If you are depending on query prefixes to switch between tenant schemas you can
pass `repo_options` to your factory which can be used to set the `:prefix` on your insert queries.
This allows you to create a factory per tenant.

```elixir
defmodule MyApp.TenantFactory
use ExMachina.Ecto, repo: MyApp.Repo, repo_options: [prefix: "test_tenant"]

def user_factory do
...
end

...
end
```
Copy link
Collaborator

Choose a reason for hiding this comment

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

🎉


## Custom Strategies

You can use ExMachina without Ecto, by using just the `build` functions, or you
Expand Down
4 changes: 3 additions & 1 deletion lib/ex_machina/ecto.ex
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,9 @@ defmodule ExMachina.Ecto do

quote do
use ExMachina
use ExMachina.EctoStrategy, repo: unquote(Keyword.get(opts, :repo))
use ExMachina.EctoStrategy,
repo: unquote(Keyword.get(opts, :repo)),
repo_options: unquote(Keyword.get(opts, :repo_options, []))

def params_for(factory_name, attrs \\ %{}) do
ExMachina.Ecto.params_for(__MODULE__, factory_name, attrs)
Expand Down
4 changes: 2 additions & 2 deletions lib/ex_machina/ecto_strategy.ex
Original file line number Diff line number Diff line change
Expand Up @@ -20,10 +20,10 @@ defmodule ExMachina.EctoStrategy do
"""
end

def handle_insert(%{__meta__: %{__struct__: Ecto.Schema.Metadata}} = record, %{repo: repo}) do
def handle_insert(%{__meta__: %{__struct__: Ecto.Schema.Metadata}} = record, %{repo: repo} = opts) do
record
|> cast
|> repo.insert!
|> repo.insert!(opts[:repo_options] || [])
end

def handle_insert(record, %{repo: _repo}) do
Expand Down
16 changes: 16 additions & 0 deletions test/ex_machina/ecto_test.exs
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,22 @@ defmodule ExMachina.EctoTest do
end
end

defmodule MockRepo do
def insert!(_model, opts), do: opts
end

defmodule MockedTenantFactory do
use ExMachina.Ecto, repo: MockRepo, repo_options: [prefix: "test_tenant"]

def user_factory do
%ExMachina.User{}
end
end

test "it passes repo_options to Repo.insert!" do
assert [prefix: "test_tenant"] == MockedTenantFactory.insert(:user)
Copy link
Collaborator

@germsvel germsvel Apr 27, 2020

Choose a reason for hiding this comment

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

Thanks for adding a test with this 🎉

I see that we're using the MockRepo defined above which only returns the opts back (which makes for really easy testing). But I'm wondering, how do you feel about adding a test that exercises the full insert into a separate tenant? Since it's a new feature, it would be nice to have more certainty that everything works as expected. What do you think?

Copy link
Author

@joeppeeters joeppeeters Apr 28, 2020

Choose a reason for hiding this comment

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

Mocking the repo was done in an attempt to prevent that, but I agree that changing the return spec of insert/1 makes it a bit weird.

I'm fine with creating an integration test to the database. Any advise on how to set that up? I'll try to look into it later this week. (Sorry, I'm don't know when I get to this. Someone should feel free to add the missing test.)

Copy link
Collaborator

Choose a reason for hiding this comment

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

I haven't looked deeply into how to set the multi-tenancy with Ecto either. I think all the setup for Ecto in ExMachina's tests comes from test/support/test_repo.ex and config/test.exs.

If I have time, I will also try to give this a shot, but I'm a bit swamped right now. Thank you for getting this PR this far 🎉. Let's see if someone else comes along and can finish it or if either of us frees up.

Choose a reason for hiding this comment

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

@germsvel So to merge this PR is all about to add a sample test with multi tenancy? The sample User.ex schema would be enough?

Copy link
Collaborator

@germsvel germsvel May 27, 2020

Choose a reason for hiding this comment

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

So to merge this PR is all about to add a sample test with multi tenancy?

Yes, I think if we could get a full integration test with multi-tenancy we'd be good. I just want to make sure this works correctly and that we have it tested.

The sample User.ex schema would be enough?

I don't know if you mean the User schema that coming from the factory defined above this test or something like https://github.com/thoughtbot/ex_machina/blob/master/test/support/models/user.ex. I think ideally we have a new schema for the multi-tenancy?

Copy link

Choose a reason for hiding this comment

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

@germsvel Any thoughts on @mojidabckuu's last comment? We too are in need of this feature! 🙂

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry, I missed that. Yes, that sounds like a good test to me!

Copy link

Choose a reason for hiding this comment

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

Found this while looking for tenet-support fixtures for my own project. If this PR still needs more test, and the above described acceptance criteria are still valid, let me know and maybe I can take a shot. Love using this on my other projects.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks for jumping in @zorn. Yeah, the above acceptance criteria is still what's needed. I just want to make sure we have a test testing multi-tenancy. I'd love to get this merged after that. Let me know if you need any help or if that's good enough to move forward.

Copy link

@zorn zorn Jan 16, 2021

Choose a reason for hiding this comment

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

I worked on this today but ran into the same issues I ran into a year ago when trying to use the Ecto sandbox env and dynamic tenants.

Specifically in a traditional test, the database is dropped/created/migrated before the tests run inside a db transaction (to allow for fast async tests), but when doing a dynamic tenant it's not obvious to me how to run prefixed migrations inside a the transaction-based checkouts after manually making the tenant with something like Repo.query("CREATE SCHEMA test_tenant"). I saw Ecto.Migrator.run(Repo, :up, options) and Ecto.Adapters.SQL.Sandbox.mode(Repo, :auto) which are related but I keep running into pid ownership issues no matter what I try.

The other thing to keep in mind (for anyone who picks this up) is that Postgres and MySQL do this behavior differently. In one of my projects I've leaned on Triplex for this abstraction. Not sure if ex_machina currently or wants to support MySQL.

With what I know now, on an individual project, it wouldn't be too hard to code a project's mix test to do a prefix migration with mix ecto.migrate --prefix "prefix_1" ahead of running tests and making some assumptions about a tenant name. Sadly, dynamic tenants seem to be much harder for me to get working, with or without this library. :(

This thread may also help people, but I am suspect that this works for async tests.

end

test "params_for/1 still works as expected" do
user_params = NoRepoTestFactory.params_for(:user)

Expand Down