Repo.insert, Repo.update and Repo.delete all return {:ok, resource} or {:error, changeset}.
But Repo.get and Repo.get_by return resource or nil.
I also noticed a decent amount of custom code (such as the Programming Phoenix Rumbl example app) prefers to use the tuple style, such as the RumblWeb.Auth.login_by_email_and_pass function returning {:ok, conn} or {:error, _reason, conn}.
That made me very curious. Why doesn’t Repo.get return {:ok, resource} or {:error, :resource_not_found}?
If it’s an API oversight and too late to change, what do you think about adding this custom function to your lib/myapp/repo.ex and always using this instead of get_by?
def fetch_by(queryable, clauses, opts \\ []) do
case get_by(queryable, clauses, opts) do
nil ->
{:error, :resource_not_found}
resource ->
{:ok, resource}
end
end
The fetch function (not included here) would do the same thing, except wrap get.
Edit: For clarity, it would be useful to have this because when pattern matching on else when using with it’s a lot more obvious to see what went wrong when you match on something like {:error, :resource_not_found} vs matching on nil.
There seems to be a convention of get functions directly returning a value or nil (or some supplied default) while the fetch functions return result tuples:
Yup. I add this to all my apps repo modules usually:
def fetch(query) do
case all(query) do
[] -> {:error, query}
[obj] -> {:ok, obj}
_ -> raise "Expected one or no items, got many items #{inspect(query)}"
end
end
Yes, but often times I don’t want to raise. I just want to be able to pattern match on a record existing or not, and using nil inside of a withelse can get very weird because you don’t know what function returned nil.
That is really cool, and definitely makes it easier to see what was nil.
But if you’re going to go that far, why not just wrap it up into a custom fetch_by function and auto-set the tuple behind the scenes so you don’t have to do it in every with statement?
Yeah there’s a reason my fetch uses all internally because you can also run into issues if your query selects just one column which can be nil.
value = User |> select([u], u.banned_at) |> Repo.get(id)
If value is nil, was the user never banned? or do they not exist? The fetch function I wrote will return {:ok, nil} for a user that exists but the column is nil, and {:error, query} if they don’t exist at all.
Ah, I never thought about it from the POV of wanting to return just a specific column. Normally I deal with returning the whole row.
But in your fetch case, you lose the ability to do this right? Repo.fetch(User, 4) or Repo.fetch(User, email: "foo@bar.com"). Basically the convenience of using get and get_by.
def fetch_by(query, args) do
query
|> where(^args)
|> fetch
end
def fetch(query, id) do
query
|> where(id: ^id)
|> fetch
end
def fetch(query) do
case all(query) do
[] -> {:error, query}
[obj] -> {:ok, obj}
_ -> raise "Expected one or no items, got many items #{inspect(query)}"
end
end
Excellent, thanks. Is there any reason you’re using the long form syntax in your first 2 functions instead of calling out to Repo.get and Repo.get_by? Is it mainly to leverage the third function’s tuple response so you don’t need to duplicate it?
Also, in the case of an error pattern in fetch, how would you pattern match on a query object? What made you choose that instead of something like {:error, :resource_not_found}?
And that generic not_found_msg(query) function would look at the query and potentially build up an atom like :#{resource}_not_found where resource is a variable you created by parsing it from query (I’m guessing that info would be available in the query)?
Right, and this was the objection that was raised by the Ecto team. Perhaps my fetch would be better if it returned {:not_found, query}?
Fundamentally, I still think there is a meaningful need here that isn’t addressed by the get API.
Ambiguity with queries that select a single column
Related to #1, if you start selecting individual columns or sets of columns you have to start pattern matching for whatever that specific column type is.
Hard to use with with. The “not found” value for the get api is nil, which doesn’t tell you what wasn’t found. You have to annotate this yourself.
Obviously none of these are deal breakers, but since you’re passing an ecto query to fetch, it can handle all of this for you.