Help needed to understand this case .. do expression in changeset validation

Phoenix Forum Questions / Help
, ,
1

I have this working code, based on some online tutorial (question inline):

  def validate_book(changeset, field) do
    case changeset.valid? do
      true ->
        book_id = get_field(changeset, field)
        case Repo.get(Mango.Books.Book, book_id) do
          nil -> add_error(changeset, :book_id, "Book not found..")
          book -> {:ok, book}
            case book.status do
              :available -> changeset
              _ -> add_error(changeset, :book_id, "Book is not available..")
            end
        end
      _ ->
        changeset # <-- QUESTION -- what is the importance of this line .. or is it the line always returning a changeset with or without any errors?
    end
  end
2

The first case is actually an if, the last line would be it’s else part meaning the part that handles false case of changeset.valid?.

The whole thing would be more readable if we use with and some pattern matching, here’s an untested rewrite, it will probably need some fixing but should illustrate the idea:

def validate_book(changeset, field) do
    with true <- changeset.valid?, 
         book_id <- get_field(changeset, field)
    do
      handle_book(changeset, Repo.get(Mango.Books.Book, book_id)),       
    else
        _ -> changeset
    end
  end

defp handle_book(changeset, nil), do: add_error(changeset, :book_id, "Book not found..")

defp handle_book(changeset, %{status: :available}), do: changeset

defp handle_book(changeset, _book), do: add_error(changeset, :book_id, "Book is not available..")
1 Like
3

Without that line if changeset is not valid function returns nil, if valid it returns a changeset. With that line you return a changeset in every case.

1 Like
4

Changeset validations take as first argument a changeset and returns a changeset. In your case, validate_book is a custom changeset validation therefor it must take a changeset and return a changeset.

As a best practice, a changeset validations first checks to see it the changeset is valid or not. If it is valid, the code will be executed else, it will return the changeset without any changes. This is useful to prevent code form unnecessarily executing when the changeset was set to invalid in prior validations.

1 Like
5

It’s a “best practice” for code that can’t error (like an additional put_change based on other changes) or takes a lot of time to execute which is unusual for a changeset function.

In my opinion, it makes for a better UX when all errors are returned immediately (that is, there are no changeset.valid? checks and all errors from all custom changeset functions are collected), not one by one (as when there are changeset.valid? checks and only the first few errors are returned).

Also, if you look at the validate_* functions in the Ecto.Changeset module, they don’t stop working for invalid changeset and keep collecting errors.

3 Likes
6

If you wanted to keep the short-circuit check of changeset.valid? then I would put it on a separate function head:

  def validate_book(%Changeset{valid?: true} = changeset, field) do
    book_id = get_field(changeset, field)
    case Repo.get(Mango.Books.Book, book_id) do
      nil -> add_error(changeset, :book_id, "Book not found..")
      book ->
        if book.status == :available do
          changeset
        else
          add_error(changeset, :book_id, "Book is not available..")
        end
    end
  end

  def validate_book(changeset, _), do: changeset
1 Like