Is this `seeds.exs` idiomatic Elixir?

I don’t think the log level should be overwritten. Either the system is configured to info level and info is logged or it isn’t. I’d also consider changing the IO.puts to use logger.

The book ids could be returned from the Repo.insert_all calls with the :returning option + a flat_map.

And as with all code generating data: This “works”, but it doesn’t really do much in terms of making sure it continues to work over time and in the face of changes to the system it seeds. There’s a reason tests commonly use factories instead of code like this. Though this is one piece of code, tests are usually quite many, so it’s less of a problem.

1. Seeds should be IMO idempotent which means that you should have corresponding Repo.delete or Repo.delete_all before inserts. This of course assumes that you can in fact identify the data that must be seeded and remove it at all which in many cases is not possible i.e. I worked in places where certain tenants / companies / users must always exist and some even had hardcoded IDs. You can still achieve idempotency by simply not deleting and not inserting anything if you detect that these special records that must be always exist are already there.

2. I would not rely on the current datetime; I’d hardcode now as a fixed datetime in the past. To me determinism trumps almost all other concerns. However that too is not a hard rule because there are businesses where certain records are only valid if they’ve been created no more than f.ex. 6 months ago. So use your best judgement but still hardcode as much as you can if you can get away with it. Again – determinism.

Disagreed, seeds are practically their own universe and the only thing they share with the main app is the storage (in most cases a relational DB). It’s quite OK to override various runtime properties like logging in there. I even stuffed OTel tracing in one project’s seeds because they were taking mysteriously long (spoiler alert: one table was too big and the seeds scanned for records + did various levels of locking; we fixed it after).

+1 here, the only thing would do differently is to use upserts instead of delete/ insert. Which I think is what you meant by:

You can still achieve idempotency by simply not deleting and not inserting anything if you detect that these special records that must be always exist are already there.

Pretty much agree with the rest as well

Yep. I didn’t say it clearly but that’s what I had in mind.

I’d be okay if this script is just a temporary solution. The code itself is good enough. My concerns are not about style of Elixir code but more about approach in general. It works for simple cases, but as the project grows, I’d not put

* Seeding is fast

to the requirements.
What is more important for me is correctness of data. The approach of inserting raw data will quickly become a mess on several dozen tables. I’m a proponent of using business functions in seed scripts. I’m okay to wait a bit longer. Inserting seed data can be sped up by parallelization (Task.async_stream or flow can help). Because of that, having progress bars in seeds is also a good thing (owl can help).

The number of inserted data must be configurable, so it is possible to specify tiny numbers to run seeds as a part of the test suit. Just create 1 record for each record type to ensure that the script doesn’t fail. Most likely, you won’t be happy when the script stops working when you need it the most

On one of the projects, we have modules for creating seeds defined in lib as a regular .ex file because we want these modules to be available in release. We run them when we launch a new server for testing. This happens quite often thanks to CI/CD. So, removing seeds.exs completely is OK, if someone is struggling with this just because the file is shipped with the default phoenix setup.

It’s always been my understanding (though I’ve been known to have a problem with understanding) that the particular seeds mechanism in Phoenix (though I’m taking this understanding from Rails from years ago) is just for data that your app requires to run on initial set up (like anything in a settings table, an initial user, etc), not for seeding dev/test data. Of course I see it used this way a lot. I like the idea of flags if one was going to use it this way so that it can cover all situations—I always create a whole separate task for adding “dev data.” I guess that essentially the same thing now that I write that out, it’s just keeping the concepts separate. Potato/potato, perhaps.

I also do upsets for this reason.

Your comments led me to make changes to both the exercise description and the example solution of the Book Club exercise.

For example, I added a motivation for the requirement that the database can be seeded.

"""
Chris is a member of a book club and wants to develop a simple web API to streamline their group activities. His vision is to create a system that stores books and tracks the pages currently being read and discussed. The book club reads multiple books simultaneously. Although there is already a system in place for tracking who is reading which books, there is no easy way to find the current page being read in any given book.

Chris shares his idea with other members who are also developers. They brainstorm additional features for future stages of the API that they could build together. For now, Chris will create the first version of the API. He and the other developers agreed to include functionality to seed the database with books and pages in this initial version, allowing Chris to easily demonstrate the API on his machine.
"""

Here are some of the changes I have made to the seeds.exs file of the example solution.

• Added concurrent database inserts, which improved seeding time drastically.
• Clearing all books and pages before any inserts.
• Return book IDs from Repo.insert_all/3, instead of querying them separately.

Some of the community suggestions were great (!), but not directly applicable to this Elixirland exercise. However, let me know if I messed something up in the new version of the seeds.exs and/or exercise description.

This is the current version.

require Logger
alias BookClub.Repo
alias BookClub.Books.{Book, Page}

# Setup
initial_log_level = Logger.level()
# Comment the next line to not override the log level configuration. The default
# log level in applications generated with `mix phx.server` is `:debug`, which
# produces verbose output.
Logger.configure(level: :info)
Logger.info("Start database seeding")
start_time = System.os_time(:millisecond)

# Clear data
Repo.delete_all(Page)
Repo.delete_all(Book)

# Constants
n_books = 4_000
n_pages_per_book = 400
inserted_at = ~N[2000-01-01 12:00:00]
batch_size = 800
max_concurrency = 8

# Batch insert books
books =
  for _ <- 1..n_books do
    %{
      title: XlFaker.generate_title(),
      inserted_at: inserted_at,
      updated_at: inserted_at
    }
  end

insert_batch = fn batch -> Repo.insert_all(Book, batch, returning: [:id]) end

book_ids =
  Enum.chunk_every(books, batch_size)
  |> Task.async_stream(insert_batch, max_concurrency: max_concurrency)
  |> Enum.flat_map(fn {:ok, {_, books}} -> books end)
  |> Enum.map(fn %{id: id} -> id end)

# Batch insert multiple pages for each book.
# Let some books have an active page.
build_pages_for_book =
  fn book_id ->
    for i <- 1..n_pages_per_book do
      %{
        book_id: book_id,
        number: i,
        status: :inactive,
        content: XlFaker.generate_page(),
        inserted_at: inserted_at,
        updated_at: inserted_at
      }
    end
    |> List.update_at(
      :rand.uniform(n_pages_per_book) - 1,
      &Map.put(&1, :status, Enum.random([:active, :inactive]))
    )
  end

Enum.chunk_every(book_ids, batch_size)
|> Task.async_stream(
  fn batch ->
    Enum.each(
      batch,
      &Repo.insert_all(Page, build_pages_for_book.(&1))
    )
  end,
  max_concurrency: max_concurrency
)

# Teardown
end_time = System.os_time(:millisecond)
run_time = end_time - start_time
Logger.info("Finish database seeding")
Logger.info("Seeded #{n_books} books and #{n_books * n_pages_per_book} pages in #{run_time}ms")
Logger.configure(level: initial_log_level)

What is the point of doing that? You want your seeds to be idenpotent, what if you have data besides those seeds in those tables?

Well, maybe I messed up!

But here’s the reasoning. As far as the exercise is concerned, only books and pages exist. If you run mix ecto.reset there is no point to clearing the data first. But if you run the seeding script separately then you’re making sure you end up with the same number of books and pages each time.

There are by the way no specific books or pages required to be in the seeded data. All content is “random”. Upserts are not necessary.

What you are describing is more fitting to a custom mix task and not seeds. I’m not sure what would be the point of seeds that do destructive operations.