Ecto: Repo.get should allow preloading associations

[BjRo]

I expected to be able to do a Repo.get(Model, id, preload: [:association]), but currently that doesn't seem to be supported. Is there a reason why Repo.get doesn't allow for preloading associations?

In my playground app im currently doing this instead Repo.all(from e in MODULE, where: e.id == ^id, preload: [:association]) |> hd
which feels a bit forced.

I think there should be a way for loading a model together with all specified associations without having to resort to the 'from' syntax.

What do guys think?

[josevalim]

:+1: from me.

[josevalim]

Btw, I think you are able to do:

from(e in MODULE, preload: [:association]) |> Repo.get(1)

But omitting the query sounds like a good idea to me.

[BjRo]

That's looks better, but I guess preload is currently not allowed in that querey. As long as preload is in there it gives me a ** (Ecto.QueryError) query can only havewhereexpressions error :-/

[josevalim]

Ah, we would have to fix that. Let's wait for Eric's reply regardless. :)

[ericmj]

I actually don't think we should allow queries to Repo.get at all. Today we seem to allow where queries, which doesn't really make much sense. If you need to filter the results you have a non-unique primary key. With a non-unique primary key Repo.get will give you inconsistent results because we don't order on the primary key. Repo.get is supposed to error with Ecto.NotSingleResult in the case when multiple rows are returned, but that doesn't seem to happen because we also add limit: 1 to the query :/.

To get back to the original issue, I would rather make use of the proposed Repo.preload here, issue #102. It would look something like this:

MyRepo.get(Model, id)
|> MyRepo.preload([:association])

[BjRo]

I think removing the queries from Repo.get is a good idea to make things clearer. I wasn't even aware that you could do that with Repo.get.

I wonder though, what speaks against MyRepo.get(Model, id, preload: [:association])? Treating preloading as an option of the get method?

[ericmj]

I'm not sure about adding it to Repo.get. I don't want to add another optional argument to Repo.get. Today we have MyRepo.get(Model, id, timeout: 5000). Should we add :preload to the same kw-list even if they do different things?

/cc @josevalim

[josevalim]

Should we add :preload to the same kw-list even if they do different things?

That's a good point.

[BjRo]

Should we add :preload to the same kw-list even if they do different things?

I think having some interface consistency between get and all would be beneficial from a consumer perspective.

Do you mean those two things would need to be implemented differently when you say do different things? If that is the case, I would argue does/should the client care? For me that looks like an implementation detail ...

Another question: How would preloading work for a single primary key lookup when you split it up into two methods like Repo.get(Model, id) |> preload(:association)? Lazy execution?

[josevalim]

@BjRo preload is always done as a separate step after the records/entities are loaded, so |> preload(:association) should work just fine. Furthermore, keep in mind Repo.all does not accept a preload option, but queries (which support preload). :)

[BjRo]

@josevalim Ok that changes the picture quite a bit. If it's executed separately anyway, making it explicit seems like a good idea. Wasn't aware of that :)

[ericmj]

We have Repo.one now that accepts a query. We also have an open issue for Repo.preload #102.