Generated test for change_user/1 failing when attempting to generate association with put_assoc

My setup

These are the files I think are useful, please let me know if you need to see other setup. The repo itself is private so I can’t link to it.

lib/site/user/user.ex

  schema "user" do
    field :first_name, :string
    field :last_name, :string
    field :full_name, :string
    belongs_to :countries, Site.Countries.Country, foreign_key: :countries_id, on_replace: :nilify
    belongs_to :currencies, Site.Countries.Currency, foreign_key: :currencies_id, on_replace: :nilify

    timestamps()
  end

  def changeset(employee, attrs) do
    employee
    |> cast(attrs, [:first_name, :last_name, :full_name])
    |> validate_required([:first_name, :last_name])
    |> put_assoc(:countries,
    Site.Countries.get_country_by_name!(attrs["country_name"] || attrs[:country_name]))
    |> put_assoc(:currencies,
    Site.Countries.get_currency_by_code!(attrs["currency_code"] || attrs[:currency_code]))
    |> generate_full_name()
  end

  defp generate_full_name(changeset) do
    case changeset do
      %Ecto.Changeset{valid?: true, changes: %{first_name: first_name, last_name: last_name}} ->
        put_change(changeset, :full_name, "#{first_name} #{last_name}")
      _ -> changeset
    end
  end

lib/be_exercise/countries.ex

...
  def get_currency_by_code!(code) do
    query =
      from(c in Currency,
        where: c.code == ^code
      )

    Repo.one!(query)
  end

  def get_country_by_name!(name) do
    query =
      from(c in Country,
        where: (c.name == ^name)
      )

    Repo.one!(query)
  end

lib/site/countries/country.ex

  schema "countries" do
    field :code, :string
    field :name, :string
    field :currency_id, :id
    has_many :users, Site.Users.User, foreign_key: :countries_id

    timestamps()
  end

  @doc false
  def changeset(country, attrs) do
    country
    |> cast(attrs, [:name, :code, :currency_id])
    |> validate_required([:name, :code])
    |> unique_constraint(:name)
    |> unique_constraint(:code)
  end

lib/site/countries/currency.ex

  schema "currencies" do
    field :code, :string
    field :name, :string
    field :symbol, :string
    has_many :users, Site.User.Users, foreign_key: :currencies_id

    timestamps()
  end

  @doc false
  def changeset(currency, attrs) do
    currency
    |> cast(attrs, [:code, :name, :symbol])
    |> validate_required([:code, :name, :symbol])
    |> unique_constraint(:name)
    |> unique_constraint(:code)
  end

test/support/fixtures/users_fixtures.ex

  import Site.CountriesFixtures

  def user_fixture(attrs \\ %{}) do
    currency = currency_fixture()
    country = country_fixture()

    {:ok, user} =
      attrs
      |> Enum.into(%{
        first_name: Faker.Person.En.first_name(),
        last_name: Faker.Person.En.last_name(),
        country_name: country.name,
        currency_code: currency.code
      })
      |>Site.User.create_employee()

    user
  end

The problem

I originally used mix phx.gen context to generate the User context, at the time I did not create any foreign keys on the table. After I added in the country and currency associations, and updated the schema to use put_assoc to generate the associations, the unit tests started to fail. I’ve been working my through them and have fixed all except the last one:

    test "change_user/1 returns a user changeset" do
      user = user_fixture()
      assert %Ecto.Changeset{} = Users.change_user(user)
    end

this is change_user/1

  def change_user(%User{} = user, attrs \\ %{}) do
    User.changeset(user, attrs)
  end

The error:

  1) test user change_user/1 returns a user changeset (Site.UsersTest)
     test/site/userss_test.exs:122
     ** (ArgumentError) comparison with nil is forbidden as it is unsafe. If you want to check if a value is nil, use is_nil/1 instead
     code: assert %Ecto.Changeset{} = Users.change_user(user)
     stacktrace:
       (ecto 3.7.2) lib/ecto/query/builder.ex:958: Ecto.Query.Builder.not_nil!/1
       (site 0.1.0) lib/site/countries.ex:176: Site.Countries.get_country_by_name!/1
       (site0.1.0) lib/site/users/user.ex:26: Site.Users.User.changeset/2
       test/site/users_test.exs:124: (test)

I have tried

  • Using IEx.pry to inspect the attrs in the changeset and in get_country_by_name(). When I stop use the breakpoint in the changeset this line:

put_assoc(:countries, Site.Countries.get_country_by_name!(attrs["country_name"] || attrs[:country_name]))

does NOT error.

Also when I investigate get_country_by_name!/1 the name parameter is NOT nil and the query returns a country.

  • IO.inspect(user) from the unit test gives me:
%Site.Users.User{
  __meta__: #Ecto.Schema.Metadata<:loaded, "euser">,
  countries: %Site.Countries.Country{
    __meta__: #Ecto.Schema.Metadata<:loaded, "countries">,
    code: "BS",
    currency_id: nil,
    users: #Ecto.Association.NotLoaded<association :users is not loaded>,
    id: 566,
    inserted_at: ~N[2022-03-24 15:53:51],
    name: "Montenegro",
    updated_at: ~N[2022-03-24 15:53:51]
  },
  countries_id: 566,
  country_name: "Montenegro",
  currencies: %Site.Countries.Currency{
    __meta__: #Ecto.Schema.Metadata<:loaded, "currencies">,
    code: "EUR",
    users: #Ecto.Association.NotLoaded<association :users is not loaded>,
    id: 560,
    inserted_at: ~N[2022-03-24 15:53:51],
    name: "Norfolk Island currency",
    symbol: "¢",
    updated_at: ~N[2022-03-24 15:53:51]
  },
  currencies_id: 560,
  currency_code: "EUR",
  first_name: "Margaretta",
  full_name: "Margaretta Padberg",
  id: 165,
  inserted_at: ~N[2022-03-24 15:53:51],
  last_name: "Padberg",
  updated_at: ~N[2022-03-24 15:53:51]
}
  • I also tried manually building an %User{} object to pass to change_user/1. When I do this from an iex session I successfully get back a changeset but when I do it from the test I get the same ArgumentError.

  • The countries_name and currencies_code fields did not initially exist in the Users table, I was just using them to find a given Country and Currency and did not need to persist them. One of my attempts to fix this error was to create those fields in the User table and to update the schema accordingly but that didn’t help.

I know this is a very verbose post but I have really tried a lot of things and any advice this community can provide would be much appreciated!

In the right-hand side of the assert, Users.change_user(user) will call User.changeset(user, %{}) - Site.Countries.get_country_by_name! will be called with nil, causing the error you’re encountering.


You could keep them as virtual attributes and then use get_change to only update the associations when needed:

  def changeset(employee, attrs) do
    employee
    |> cast(attrs, [:first_name, :last_name, :full_name, :country_name, :currency_code])
    |> validate_required([:first_name, :last_name])
    |> maybe_set_currencies()
    |> maybe_set_countries()
    |> generate_full_name()
  end

  defp maybe_set_countries(%{valid?: false} = changeset), do: changeset
  defp maybe_set_countries(changeset) do
    case get_change(changeset, :country_name) do
      nil ->
        changeset

      country_name ->
        country = Site.Countries.get_country_by_name!(country_name)
        put_assoc(changeset, :countries, country)
    end
  end

  # similar for currencies

There are some places where you may want different behavior:

  • the head that matches on valid? is useful if get_country_by_name! is expensive to call
  • the case branch for nil doesn’t distinguish between "not changing country_name" and "changing country_name to nil"

Some additional thoughts:

  • belongs_to associations are usually named with singular nouns. Future readers may get confused when accessing some_user.currencies returns a %Currency{} instead of a list.
  • raising an exception on invalid input in a changeset is somewhat user-hostile; consider using add_error instead to tell the caller they passed a bogus country name
2 Likes

Thanks so much @al2o3cr and apologies for the long delay in responding.

I want to say I can’t believe I missed something so blatant but it’s actually pretty believable given how close I was to the code (for a long time) and also since I’m still experimenting/learning.

Thank you for the other notes and suggestions as well. They’re super useful and I’ve taken them on board :slightly_smiling_face: