Why does Repo.get and Repo.get_by return the resource or nil instead of an ok or error tuple?

[benwilson512]

@massimo

could return NULL ( nil converted to Elixir) or :not_found

No, it would return {:ok, nil}.

    case all(query) do
      [] -> {:error, query}
      [obj] -> {:ok, obj}
      _ -> raise "Expected one or no items, got many items #{inspect(query)}"
    end

all(query) would return [nil] which matches the second case. This is exactly why fetch handles null columns better than get. get treats the column being null and 0 return rows as the same thing, whereas fetch disambiguates.

If it helps, think of it as :found | :not_found vs :ok | :error. It isn’t trying to make a value judgement that :not_found is bad. It’s just trying to make it easier to determine “I got a row with my query” vs “I did not get a row with my query”.

[peerreynders]

@massimo

A query that does not return data is not really an error.

The semantics of fetch imply the expectation of there being a record similar to one!.

[benwilson512]

Right, this is the counter argument. No rows isn’t an error from an SQL perspective sure, but we can layer functions on top of the actual all call that give it whatever semantics we want. This is already what Ecto does with get_by! and one! which treat no results as enough of a problem to raise an exception. This isn’t even ecto specific, as you see the same thing with Map functions.

[peerreynders]

Could recycle the effort that went into Ecto.NoResultsError and Ecto.MultipleResultsError.

  defp multiple_results_error(queryable, count) do
    info = Ecto.MultipleResultsError.exception(queryable: queryable, count: count)
    {:error, {info.__struct__, info.message}}
  end

  defp no_results_error(queryable) do
    info = Ecto.NoResultsError.exception(queryable: queryable)
    {:error, {info.__struct__, info.message}}
  end

---

defmodule RepoX do
  import Ecto.Query
  alias MusicDB.Repo

  # Separate module to avoid clash with future Ecto.Repo functions

  # shameless ripoff
  # https://github.com/elixir-ecto/ecto/blob/master/lib/ecto/repo/queryable.ex

  defp assert_schema!(%{from: %{source: {_source, schema}}}) when schema != nil, do: schema

  defp assert_schema!(query) do
    raise Ecto.QueryError,
      query: query,
      message: "expected a from expression with a schema"
  end

  defp query_for_fetch(_queryable, nil) do
    raise ArgumentError, "cannot perform RepoX.fetch/3 because the given value is nil"
  end

  defp query_for_fetch(queryable, id) do
    query = Ecto.Queryable.to_query(queryable)
    schema = assert_schema!(query)

    case schema.__schema__(:primary_key) do
      [pk] ->
        from(x in query, where: field(x, ^pk) == ^id)

      pks ->
        raise ArgumentError,
              "RepoX.fetch/3 requires the schema #{inspect(schema)} " <>
                "to have exactly one primary key, got: #{inspect(pks)}"
    end
  end

  defp query_for_fetch_by(queryable, clauses),
    do: where(queryable, [], ^Enum.to_list(clauses))

  defp multiple_results_error(queryable, count) do
    info = Ecto.MultipleResultsError.exception(queryable: queryable, count: count)
    {:error, {info.__struct__, info.message}}
  end

  defp no_results_error(queryable) do
    info = Ecto.NoResultsError.exception(queryable: queryable)
    {:error, {info.__struct__, info.message}}
  end

  def fetch_one(queryable, opts \\ []) do
    case Repo.all(queryable, opts) do
      [one] -> {:ok, one}
      [] -> no_results_error(queryable)
      other -> multiple_results_error(queryable, length(other))
    end
  end

  def fetch(queryable, id, opts \\ []),
    do: fetch_one(query_for_fetch(queryable, id), opts)

  def fetch_by(queryable, clauses, opts \\ []),
    do: fetch_one(query_for_fetch_by(queryable, clauses), opts)
end

defmodule Playground do
  import Ecto.Query
  alias MusicDB.Repo
  alias MusicDB.{Album, Artist}

  defp by_pk(%{from: %{source: {_source, schema}}} = queryable, id) do
    query = Ecto.Queryable.to_query(queryable)
    [pk] = schema.__schema__(:primary_key)
    from(x in query, where: field(x, ^pk) == ^id)
  end

  defp by_clauses(queryable, clauses),
    do: where(queryable, [], ^Enum.to_list(clauses))

  defp one(queryable) do
    with {:all, [one]} <- {:all, Repo.all(queryable)} do
      {:ok, one}
    else
      {:all, [_ | _] = results} -> {:error, {:many, results}}
      {:all, []} -> {:error, :none}
    end
  end

  def play do
    noResultId = 0
    someResultId = 1
    queryOne = from(a in Album, where: a.id == 1)
    queryOneNoResults = from(a in Album, where: a.id == 0)
    queryOneMultipleResults = from(a in Album, where: a.id > 0)

    IO.puts(inspect(RepoX.fetch_one(queryOne)))
    IO.puts(inspect(RepoX.fetch_one(queryOneNoResults)))
    IO.puts(inspect(RepoX.fetch_one(queryOneMultipleResults)))
    # {:error, {Ecto.MultipleResultsError,
    #   "expected at most one result but got 5 in query:
    #
    #    from a0 in MusicDB.Album,
    #    where: a0.id > 0
    #   "}}

    IO.puts(inspect(RepoX.fetch(Album, noResultId)))
    IO.puts(inspect(RepoX.fetch(Album, someResultId)))

    IO.puts(inspect(RepoX.fetch_by(Album, id: noResultId)))
    IO.puts(inspect(RepoX.fetch_by(Album, id: someResultId)))

    queryNotNilData = from(r in Artist, select: r.inserted_at)
    queryNilData = from(r in Artist, select: r.death_date)

    IO.puts(inspect(RepoX.fetch(queryNotNilData, someResultId)))
    # {:ok, ~N[2019-05-03 00:16:32]}
    IO.puts(inspect(RepoX.fetch(queryNilData, someResultId)))
    # {:ok, nil}
    IO.puts(inspect(RepoX.fetch(queryNotNilData, noResultId)))
    # {:error, {Ecto.NoResultsError,
    #    "expected at least one result but got none in query:
    #
    #     from a0 in MusicDB.Artist,
    #     where: a0.id == ^0,
    #     select: a0.inserted_at
    #    "}}

    queryOneMultipleResults
    |> one()
    |> inspect
    |> IO.puts()
    # {:error, {:many, [...]}}

    queryNotNilData
    |> by_pk(someResultId)
    |> one()
    |> inspect
    |> IO.puts()
    # {:ok, ~N[2019-05-03 00:16:32]}

    queryNilData
    |> by_clauses(id: someResultId)
    |> one()
    |> inspect
    |> IO.puts()
    # {:ok, nil}

    queryNotNilData
    |> by_pk(noResultId)
    |> one()
    |> inspect
    |> IO.puts()
    # {:error, :none}

    :ok
  end
end

[adkron]

I like the convention of {:error, exception} since the exceptions often have the message you want, they have extra data that can be useful, they are simple to pattern match on even if the message changes and there is an option for the user to raise the error if they wish.

[michalmuskala]

To play a bit of a devil’s advocate - the return value of %Struct{} | nil isn’t really that much harder to match in a with. That’s the great thing about with that it is not limited to just ok/error tuples.

with %{} = foo <- Repo.get(Foo, foo_id),
     %{} = bar <- Repo.get(Bar, bar_id) do
  # ... all fine
else
  _ -> # not found
end

[peerreynders]

The core of @benwilson512’s argument is the nil doesn’t differentiate the “no results” vs "nil data" case e.g:

    IO.puts(inspect(RepoX.fetch(queryNilData, someResultId)))
    # {:ok, nil}
    IO.puts(inspect(RepoX.fetch(queryNotNilData, noResultId)))
    # {:error, {Ecto.NoResultsError,
    #    "expected at least one result but got none in query:
    #
    #     from a0 in MusicDB.Artist,
    #     where: a0.id == ^0,
    #     select: a0.inserted_at
    #    "}}

Sticking to essentials - stick with all/2 (and with)

• [_|_] = results -> many
• [one] -> i.e. one could be nil data
• [] -> none, i.e. no results

[benwilson512]

@michalmuskala

To play a bit of a devil’s advocate

What would the devil have to say about these points specifically then? Why does Repo.get and Repo.get_by return the resource or nil instead of an ok or error tuple? - #19 by benwilson512

Specifically, your example gets problematic if you start selecting specific columns:

1. You can no longer just match on %{} you have to change your pattern depending on the column type.
2. It fails to distinguish between “We did find what you asked for, and it’s nil” vs “not found”.
3. If you do an else, how do you tell what wasn’t found? Sure, you can tag all this yourself, but if we solve all of these ourselves then we have to do:

with {%{} = foo, _} <- {Repo.get(Foo, foo_id), {:not_found, Foo}},
     {[val], _} <- {Repo.all(query_selecting_column), {:not_found, query_selecting_column} do
  blah(foo, bar)
else
  {_, {:not_found, query}} ->
    queryable = Ecto.Queryable.to_query(query)
    not_found_message(queryable)
end

Which is hideously verbose. It also treats returning too many rows the same as returning no rows.

[michalmuskala]

I only talked about the full schema case, which is probably about 90% of all ecto queries and there it works just fine. About returning the query with the error - I see the usefulness, but I also don’t agree it’s a good default implementation. By that same logic you could argue Map.fetch should return {:error, map, key} on failure and similar. I think those are all rather fringe use cases and I’m completely fine if you need to add a 3 line function to one module to handle them.

[cnck1387]

@michalmuskala

To play a bit of a devil’s advocate - the return value of %Struct{} | nil isn’t really that much harder to match in a with . That’s the great thing about with that it is not limited to just ok / error tuples.

with %{} = foo <- Repo.get(Foo, foo_id),
     %{} = bar <- Repo.get(Bar, bar_id) do
  # ... all fine
else
  _ -> # not found
end

What if you wanted to differentiate the “not found” to be “foo not found” or “bar not found” depending on which one wasn’t found?

[michalmuskala]

I’d argue that if you need to differentiate this, than with is a wrong tool for that job - nested cases will be more readable. With is great, but it’s also, IMHO, abused a lot - similar to the pipe operator and similar to use. “Too much of a good thing, …” and all that.

To further make the point, this is how the example @benwilson512 posted would look like without with:

case Repo.get(Foo, foo_id) do
  %Foo{} ->
    case Repo.all(query_selecting_column) do
      [bar] -> blah(foo, bar)
      _ -> {:not_found, "bar"}
  _ -> {:not_found, "foo"}
end

[peerreynders]

If you insist on sticking with with

with {:foo, %{} = foo} <- {:foo, Repo.get(Foo, foo_id)},
     {:bar, %{} = bar} <- {:bar, Repo.get(Bar, bar_id)} do
  # ... all fine
else
  {:foo, _} -> # foo not found
  {:bar, _} -> # bar not found
end

[benwilson512]

@michalmuskala

By that same logic you could argue Map.fetch should return {:error, map, key} on failure and similar.

I think folks are bit hung up on the names. If you made it lookup and returned {:found, value} | {:not_found, query} I’d be just as happy. I think there is empirically a lot of concrete use case for using the query value being returned because it holds a lot more complex information than just a key. For example if you do Repo.fetch(queryable, id) and get {:error, query}, the query is not the same as queryable. This is a key difference from the Map case.

@michalmuskala

I only talked about the full schema case, which is probably about 90% of all ecto queries and there it works just fine

Sure, but in every one of those cases I also want to have a generic “not found” handler. Consider how well it works with Action Fallback. You can have a controller that just uses with and lookups and then have the result create really great pages for if records weren’t found.

Notably your example there doesn’t do the raise if there are multiple results thing. It’ll treat multiple results as if it wasn’t found.

with {:ok, foo} <- Repo.fetch(Foo, foo_id),
  {:ok, bar} <- Repo.fetch(query_selecting_column) do
  Repo.insert(blah(foo, bar))
end

If you do it this way you have either {:ok, value} | {:error, Ecto.Query | Ecto.Changeset} to work with, which is a fantastically rich set of responses to work with. I just don’t see the draw back given the hoops you have to jump to try to wrangle all of these guarantees manually.

[adkron]

What about wrapping the calls to the query? The function could return the thing or {:not_found, query}.

  # terrible name, and probably not the function I would create,
  # but the return types are the real point
  @spec by_id(mod, integer) :: %mod{} | {:not_found, mod}, when mod: var

I see this is a lot of APIs, both internal and external. My question is, why do we fight to avoid making new functions that fit our use case?

[cnck1387]

I thought with was mainly for running through a bunch of functions, doing something if everything was a success, and on error being able to determine which function failed and then acting accordingly?

It seems very reasonable if you were doing 2 get lookups to want to be able to flash different errors in a Phoenix app based on which one failed.

Or, who knows what you were trying to do. It’s also quite possible other functions you were trying to run successful return nil on failure / wasn’t abe to find something.

[peerreynders]

@cnck1387

on error being able to determine which function failed and then acting accordingly?

With your initial proposal fetch_by was only going to return {:error, :resource_not_found} - which wouldn’t allow you to differentiate between two fetch_bys in a with - so you wouldn’t be any further ahead.

want to be able to flash different errors in a Phoenix app based on which one failed.

Not necessarily. In general details should be logged. User errors should be helpful while revealing as little as possible about your actual implementation.

[cnck1387]

@peerreynders

With your initial proposal fetch_by was only going to return {:error, :resource_not_found} - which wouldn’t allow you to differentiate between two fetch_by s in a with - so you wouldn’t be any further ahead.

Correct, but you should be able to pick out the schema name from that function and use that as a variable so you’d be able to get :foo_not_found right? This would be implemented at the fetch_by level, so it doesn’t leak out to each with statement.

[benwilson512]

@peerreynders

In general details should be logged.

Suppose you wanted to build a generic detailed logger. Returning {:error, query} or {:not_found, query} lets you do this, while also still generating a generic and more minimalist error for the user. You can always take a rich data structure and make it smaller, you can’t take a datastructure with no information, like nil, and make it more informative.

[peerreynders]

@cnck1387

you should be able to pick out the schema

A general function may not do that.

However if you want to clean up a with it’s usually easiest accomplished with custom wrapper functions, e.g.:

  defp my_get(queryable, id) do
    query = Ecto.Queryable.to_query(queryable)
    %{from: %{source: {_source, schema}}} = query
    [pk] = schema.__schema__(:primary_key)
    q = from(x in query, where: field(x, ^pk) == ^id)

    case Repo.all(q) do
      [one] ->
        one # I'd use {:ok, one}

      [] ->
        {:error, {:none, schema}}

      [_ | _] ->
        # unlikely
        {:error, {:too_many, schema}}
    end
  end

  defp my_fun(album_id, artist_id) do
    with %MusicDB.Album{} = album <- my_get(Album, album_id),
         %MusicDB.Artist{} = artist <- my_get(Artist, artist_id) do
      {:ok, {album, artist}}
    else
      error -> error
    end
  end

[OvermindDL1]

@massimo

A query that does not return data is not really an error.

In erlang that is usually (in elixir syntax) {:ok, value}/:undefined (still don’t know why nil in elixir isn’t an :undefined atom to match better with erlang…).

@benwilson512

Perhaps my fetch would be better if it returned {:not_found, query} ?

Yep yep! Explicit is always better than implicit!

But yeah, I really really dislike the ambiguity of value/nil when value is at all even remotely possible to be nil, like it is in Repo and other places around in Elixir, it is disconcerting.

@michalmuskala

To play a bit of a devil’s advocate - the return value of %Struct{} | nil isn’t really that much harder to match in a with . That’s the great thing about with that it is not limited to just ok / error tuples.

That only works if you are returning a struct/map however, a query could easily return a specific value massaged into a variety of formats, even nil could be a valid return value.

@peerreynders

The core of @benwilson512’s argument is the nil doesn’t differentiate the “no results” vs " nil data" case e.g:

+1

@michalmuskala

which is probably about 90% of all ecto queries and there it works just fine.

Huh? Returning a schema straight is actually pretty rare at my work. Significantly less than 90% o.O

@michalmuskala

By that same logic you could argue Map.fetch should return {:error, map, key} on failure and similar.

This is why I quite like the exceptional libraries style, return value|%SomeException{}, which is trivial yo match out even in the anti-case with map_get in a guard now (although the exceptional libraries helpers simplify that far more). And yes it supports mixing and matching in the tuple forms too, so {:ok, value}|%SomeExcption{} is entirely valid too (and is the form I use in my project). Ah if only we had static types. ^.^

@peerreynders

If you insist on sticking with with

Yep, I use this tagging form a lot, the old happy_path library I used supported that more simply too.

@benwilson512

Suppose you wanted to build a generic detailed logger. Returning {:error, query} or {:not_found, query}

Yeah I significantly prefer tagged results, like for this example returning:

{:ok, value}
{:not_found, blah}
{:error, unknown_error}

Make sense, and in a statically typed system would correspond with the type of:

type 'value * 'query =
| Get of 'value
| NotFound of 'query
| UnknownError of Error.t

And if you notice in Erlang, near all of its types and return values follow what a static typing system does. It still boggles me that Elixir doesn’t, but I’m guessing that was some Ruby’isms in its initial API designs. Following strong static typing systems, even when the language is not a strong statically typed language, makes for significantly more robust and well designed API’s then ad-hockedly returning duck-typing everywhere (even python has optional typing now!), and Erlang did it decently well most of the time.

I really wish dialyzer was capable of detecting conflicting types, so something with a type of 'any | nil (which is essentially the return value of a few Ecto.Repo’s functions) would error as inherently conflicting.