Ecto: Repo.get returns ambiguous responses

[benwilson512]

I mentioned this in this post on the mailing list but I think it's worth covering here.

Given a schema of posts that include a description field that can be null, the following query returns an ambiguous result:

Post |> select([p], p.description) |> Repo.get(123)

If this returns nil, does Post 123 not exist? or does it exist but it has a nil description? There's no way to tell without using get! or altering the query, and neither are good substitutes.

I wouldn't really mind this ambiguity for Repo.get if an alternative function existed that could give us the same choice that Map.get and Map.fetch give us.

[josevalim]

Thanks @benwilson512. In this case, we recommend using Repo.all or Repo.one. get and get_by are meant to be convenience functions after all. :)

[benwilson512]

Repo.one suffers from exactly the same problem.

Post
|> where([p], p.id == 123)
|> select([p], p.description)
|> Repo.one

If this returns nil, there's still no way to know if it's because no records were found or if a record was found but the selected values were nil.

Sure I can do

Post
|> where([p], p.id == 123)
|> select([p], p.description)
|> Repo.all
|> case do
  [] -> missing
  [item] -> found(item)
end

The issue isn't that it's impossible to construct a query where the difference becomes clear, it's that Repo.one conflates no record with a record that has a nil column. Those aren't valid cases to conflate. Repo.one! treats them differently, but I shouldn't need to switch over to using exception handling to tell that from the outside.

*apologies for the minor edits, wanted to make the latter code example clearer.

[josevalim]

Right, so Repo.all it is. :)

[benwilson512]

Fair enough :)

[josevalim]

To expand a little bit. Folks can easily implement a fetch function in the Repository on top of all. get, get_by and one were meant to be convenience functions. There is a tension on how many convenience functions we should have. We don't want to unnecessarily pollute the repository.

My impression today is that Repo.get and Repo.get_by are largely useful today despite the downsides listed here. But maybe one is not that useful? Maybe one would be better served by something like fetch!? Then that's a discussion worth having.

[benwilson512]

I think you're right about get and get_by, and your general concern about a proliferation of convenience functions is quite reasonable as well.

Right now in my Repo I have:

  def fetch(query) do
    case all(query) do
      [] -> {:error, query}
      [obj] -> {:ok, obj}
    end
  end

This is exceptionally useful when used alongside with, because now I have a set of functions (insert, update, fetch) that all return useful information in the event something doesn't go as planned. This is important with with because by the time you're looking at the error cases you no longer know the context within which a given case was created.

To give a rough sketch of the use case

with {:ok, user} <- Repo.fetch(user_query),
  {:ok, account} <- Repo.fetch(authorized_account_query),
  {:ok, membership} <- Repo.insert(membership_changeset(user, account)) do
    send_welcome_email(membership)
    {:ok, membership}
end
|> handle_errors

defp handle_errors({:error, %Ecto.Changeset{} = changeset}) do
  format_changeset_error(changeset)
end
defp handle_errors({:error, %Ecto.Query{} = query}) do
  format_not_found(query)
end
defp handle_errors(other), do: other

This doesn't go through all the steps needed to make it nice but you get the idea.I'm also not sold 100% on the idea that the error case for fetch should be simply {:error, query}.

Maybe there's a query.errors that gets set to something?

[paulcsmith]

Ecto in it's current form allows for pattern matching with get/get_by/one on nil, but I think it looks no idomatic, and also makes it so that you have to check the nil case first, or pattern match against the returned struct. It also lets nil silently though when you don't explicitly check for it.

The way I think about it is: use Repo.get(_by) when I want to handle the response if there is no record, and use Repo.get! as the more "convenient" function since it will blow up if something goes wrong and won't just continue with an unexpected nil (those can be pretty hard to debug). With this in mind, I would prefer it if the non bang versions returned a tagged tuple.

# Typically I've seen most people match the happy path first, so this looks strange
case Repo.get(Thing, 123) do
  nil -> IO.puts "Whoops!"
  record -> record
end

# Or this, which I don't think is as clear as if there were a tagged tuple
case Repo.get(Thing, 123) do
  %Thing{} = record -> record
  _ -> IO.puts "Whoops!"
end

# With a tagged tuple (my personal pref). I think this is much more idiomatic and nicer to read
case Repo.get(Thing, 123) do
  {:ok, record} -> record
  {:error, _} -> IO.puts "Whoops!"
end

# Using Repo.all seems strange to me in this case
# It works, but semantically it seems off to me. Could be just preference.
case Thing |> where(id: 123) |> Repo.all do
  [thing] -> thing
  _ -> IO.puts("Whoops")
end

# If I just want the convenience and don't need to do anything special when it does not exist.
# These errors are much easier to catch than a random nil somewhere IMO.
thing = Repo.get!(Thing, 123) 
do_stuff(thing)

Drawbacks

It would break existing apps and would require that people change their case statements or switch to the bang versions. This is definitely a big consideration.

It also may be less convenient if all you wanted to do was the following, but I think matching with a case is more concise, and I believe is generally preferred anyway.

if thing = Repo.get(Thing, 123) do
  thing
else
  IO.puts "Whoops"
end

Another drawback would be if people did something like Repo.get(Thing, 123) || other_thing. I don't think I've needed that or seen that in use. Maybe this is common for other devs though?

[josevalim]

Thanks for the feedback @paulcsmith!

I don't think we are being fair here. We write code like this in Elixir every day: opts[:key] || "default". Get is no different. Here is a find_or_initialize:

Repo.get_by(User, username: "jose") || %User{username: "jose"}

Or dynamically:

Repo.get_by(User, attrs) || struct(User, attrs)

Or a find_or_insert:

Repo.get_by(User, username: "jose") || Repo.insert(%User{username: "jose"})

You don't want to handle nils? Call get!.

I am pretty confident get and get_by are largely useful as it is today. The only scenario where they are not enough so far are the ones above: when you are using with or when you need to select one particular column (which may be nil). For those cases, you can either use all or add a convenience function.

So I would prefer to hear about more use cases of fetch being used than somehow tag get/get_by as non idiomatic. :)

[paulcsmith]

@josevalim Sorry I did not mean to say that get/get_byare not idiomatic, I meant that using them _to pattern match_ didn't seem idiomatic to me, but I don't really know. That's why I gave the examples. Thanks for clearing that up. I wasn't trying to bash Ecto as being non idiomatic :D

You are right that those use cases make a lot of sense, and I'm glad you mentioned it because I haven't done that before! It looks very useful

As for the fetch function, what would that do? I saw what Ben's fetch function did, would what you're proposing be similar or something different?

If fetch returned a tagged tuple (or something along those lines( then I think that would be _awesome_!

[josevalim]

Oh, no worries! I didn't think you were saying Ecto is non-idiomatic, at all. :) But I agree with you, I wouldn't pattern match on them most of the time. My point is that I would focus on the merits of something like fetch than try to change get. :)

[paulcsmith]

Right on, yeah that makes a lot of sense. I think a Repo.fetch/1 that returned one record with {:ok, record} and {:error, reason} (like @benwilson512's) would be super awesome.

I think Repo.fetch would be great because I think pattern matching would be nicer to use and read, similar to the examples with the tagged tuple in my comment above.

I'm not sure if this would be too much, but a Repo.fetch/2 that worked like Repo.get/2 but returned a tagged tuple would be helpful as well, but that might be making the public API too big, and I know you're trying to avoid that :P

[tramjoe]

I just hit that exact same issue that @benwilson512 reported as well.
Here is the use case:

def update_password(domain, login, password) do
    Repo.get_by(Account, domain: domain, login: login)
    |> Account.changeset(%{password: password})
    |> Repo.update
  end

If the Account does not exist, Repo.get_by returns nil, and Account.changeset rises having no function clause for that case. That code needs to be fixed by using either one of the suggested options on that thread, I won't list them all again.

I sure would like to have an idiomatic Ecto way of dealing with this.
I'll certainly use a Repo.fetch/1 as described above, and will for now use some version of this in my code.

Having this in Ecto seems useful, and if asked, I'd happily throw Repo.one out to make some room :-)

Just the 2c of an average Ecto user.