Gracefully handling Ecto.Repo.one exception

Questions / Help
1

In Elixir, error handling using pattern matching is generally preferred over using try/rescue construct.

However due to the specification of Ecto.Repo.one (returns a struct if single result is found, nil if no result and raises Ecto.MultipleResultsError if more than one result), it appears (unless I’m mistaken) the only way to safely implement the following requires either:

  • first restricting the query to return the first result with , limit: 1
  • resorting to try/rescue to handle the exceptional case
query = from ni in NewsItem, where: ilike(ni.link, ^"#{url}%")

case Repo.one(query) do
  %Scraper.NewsItem{id: id} -> success_path()
  _ -> alternate_path()
end

If that is the case then what are your opinions on the current spec of Ecto.Repo.one? Should the responsibility of making the query safe be left to the developer or should the limit restriction happen implicitly thus allowing safe/conventional pattern matching?

2

You forgot one option: “Let it crash”.

If you only expect one item and get multiple something went irrecoveribly wrong. Let your process crash, let your supervisor handle this and always have an eye on your logs (assuming crashes are logged properly with a reason).

If though you want explicit error handling for your case, just don’t use Repo.one/1 but Repo.all/1 instead:

case Repo.all(q) do
  [%Scraper.NewsItem{id: id} -> success_path()
  [] -> nothing_found_path()
  _ -> to_many_found_path()
end

edit

I’d even implement Repo.one/1 in terms of Repo.all/1, roughly like this:

def one(q) do
  case all(q) do
    [i] -> i
    [] -> nil
    _ -> raise Foo
  end
end

So by surrounding one with try/rescue, you then were reimplementing a slower version of all/1, because catching an exception is very expensive on the BEAM.

1 Like
3

Thanks for pointing that out. The reason I noticed it in the first place is that the process crashed and was properly logged as you suggested.

I agree that getting multiple results when expecting one points to an erroneous query but I don’t see how using Repo.all for error handling in that case would help since it would load all the results of your (erroneous) query.

Given the semantics of Repo.one described in the documentation as Fetches a single result from the query., my question was whether it should implicitly limit the result set to one or raise an exception like it currently does.

I now believe raising the exception is preferable since it points to the root cause, an inaccurate query, which would likely be a bug.

4

See ecto/lib/ecto/repo/queryable.ex at v2.2.8 · elixir-ecto/ecto · GitHub

  def one(repo, adapter, queryable, opts) do
    case all(repo, adapter, queryable, opts) do
      [one] -> one
      []    -> nil
      other -> raise Ecto.MultipleResultsError, queryable: queryable, count: length(other)
    end
  end

i.e. one is already implemented in terms of all.

I now believe raising the exception is preferable since it points to the root cause, an inaccurate query, which would likely be a bug.

In my experience, which isn’t necessarily representative, inconsistently persisted data is usually to blame - i.e. the problem is usually further upstream.

5 Likes
5

That’s interesting, I had no idea :slight_smile: