-
Notifications
You must be signed in to change notification settings - Fork 146
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 opts to Repo.insert! (add function-level opts to strategies) #411
Conversation
record = ExMachina.build(__MODULE__, factory_name, attrs) | ||
|
||
unquote(function_name)(record) | ||
end | ||
|
||
def unquote(function_name)(factory_name) do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We now handle each case directly in a separate function (instead of having optional attrs \\ %{}
) because having the optional args conflicts with other versions of insert/2
and insert/3
.
CREATE TRIGGER gen_db_value | ||
BEFORE INSERT ON users | ||
FOR EACH ROW | ||
EXECUTE FUNCTION set_db_value(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Created this function and trigger so that we could properly test returning: true
. Maybe it's testing too much? But it seemed like a good test that we're indeed passing the values to Repo.insert!/2
as it expects them.
What changed? ============= We add the ability for a strategy to receive more options as a third argument. These options could be a function-level override of the strategy's options, or some other function-level options that we want to pass to the underlying strategy implementation. For example, the EctoStrategy can use those options and pass them to the `Repo.insert/2` function: ```elixir insert(:user, [name: "gandalf"], returning: true) build(:user, name: "gandalf") |> insert(returning: true) ``` That allows people to have more control over inserting the factory into the database. For example, we can now automatically get db-generated values by passing `returning: true`. Or we can use `prefix` or `on_conflict`. For a full list of insert options, see [Repo.insert docs]. [Repo.insert docs]: https://hexdocs.pm/ecto/Ecto.Repo.html#c:insert/2-options Known less-than-good ergonomics ------------------------------- Because both the second and third arguments are lists or maps, we need to add the surrounding keyword list `[]` to the second argument when we're using `insert/3`. In the worst case, when we don't pass args to the factory but we pass options to the repo, we're required to define an empty list: ```elixir insert(:user, [], returning: true) ``` We also have to wrap previously unwrapped lists _when_ we use the third opts argument: ```elixir insert(:user, [name: "Aragorn"], returning: true) ``` That's just the nature of having two arguments that are both lists or maps (factory args and repo opts). It reads more nicely when it's an already built struct: ```elixir build(:user) |> insert(returning :true) build(:user, name: "james") |> insert(returning :true) insert(%User{}, returning: true) ``` Also available for [strategy_func]_pair and _list functions ------------------------------------------------------------- We also update the strategy's _pair and _list function generators to allow for `opts` to be passed, since presumably users might want to do something like this: ```elixir insert_pair(:user, [name: "jane"], returning: true) ``` Current strategies in the wild that don't support this will not break. If they implement a `handle_[function_name]/3`, then they can also accept the third arguments, and their users can pass the third argument. Otherwise, they can continue using the strategies as they already have.
e1a3800
to
c436611
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🙌🏽
`ExMachina.Ecto` uses | ||
[`Repo.insert!/2`](https://hexdocs.pm/ecto/Ecto.Repo.html#c:insert!/2) to | ||
insert records into the database. Sometimes you may want to pass options to deal | ||
with multi-tenancy or return some values generated by the database. In those |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How would you feel about being more explicit here about the use case for returning
? Specifically it's not just values generated by the database, but values for all columns regardless of if those columns are specified in the factory (and by proxy, in the insert
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know that I want to go into full detail for what returning
does. Rather, I was hoping to push people to look up what is available in the Repo.insert!/2
docs. I just happened to know that multi-tenancy and returning are two things people have asked for. Do you have a suggestion on how to write this in a way that communicates that better? Maybe something like this?
Sometimes you may want to pass options to Repo's insert function (e.g.
returning: true
orprefix: "prefix"
). In those cases...
can -> will (via Wil) :) Co-authored-by: Wil Hall <[email protected]>
Co-authored-by: Wil Hall <[email protected]>
Closes: #273
Related to: #382, #280, #377
What changed?
We add the ability for a strategy to receive more options as a third argument. These options could be a function-level override of the strategy's options, or some other function-level options that we want to pass to the underlying strategy implementation.
For example, the EctoStrategy can use those options and pass them to the
Repo.insert/2
function:That allows people to have more control over inserting the factory into the database. For example, we can now automatically get db-generated values by passing
returning: true
. Or we can useprefix
oron_conflict
.For a full list of insert options, see Repo.insert docs.
Known less-than-good ergonomics
Because both the second and third arguments are lists or maps, we need to add the surrounding keyword list
[]
to the second argument when we're usinginsert/3
.In the worst case, when we don't pass args to the factory but we pass options to the repo, we're required to define an empty list:
We also have to wrap previously unwrapped lists when we use the third opts argument:
That's just the nature of having two arguments that are both lists or maps (factory args and repo opts).
It reads more nicely when it's an already built struct:
Also available for [strategy_func]_pair and _list functions
We also update the strategy's _pair and _list function generators to allow for
opts
to be passed, since presumably users might want to do something like this:Current strategies in the wild that don't support this will not break. If they implement a
handle_[function_name]/3
, then they can also accept the third arguments, and their users can pass the third argument. Otherwise, they can continue using the strategies as they already have.