Add Repo.reload/2 and Repo.reload!/2 #3416

v0idpwn · 2020-09-20T20:25:19Z

Closes #3389
I'm still polishing, got to fix some things regarding code quality. Still, the API proposal is this. Do I need to change something?

josevalim · 2020-09-20T21:39:04Z

lib/ecto/repo/queryable.ex

+    case schema.__schema__(:primary_key) do
+      [pk] ->
+        keys = Enum.map(structs, &get_pk!(&1, pk))
+        preloads = Keyword.get(opts, :preload, [])


Honestly, I don't think we need to support this option. Doing a |> Repo.preload(...) after is only very few characters more verbose than , preload: .

@josevalim wouldn't |> Repo.preload() imply another trip to the database while from(..., preload: ...) does it in one pass? Or they're always equivalent?

Preload always issues separates queries, that’s what it is about, so both forms are equivalent!

That's nice to know! I thought the form in Ecto.Query would optimize into a join of some kind

josevalim · 2020-09-20T21:42:08Z

lib/ecto/repo/queryable.ex

+    if result_count < expected_count do
+      raise Ecto.TooFewResultsError, queryable: query, min: expected_count, count: result_count
+    end
+
+    result


Unfortunately there is no guarantee result will be in the same order as structs. So you need to do something like this:

for struct <- structs do struct_pk = Map.fetch!(struct, pk) Enum.find(result, &Map.fetch!(&1, pk) == struct_pk) || raise "could not find" end

This means you don't need the Enum.count nor the exception above, as the above will guarantee all entries have a match.

If we guarantee same ordering in reload!/2, should we also do the same for reload/2?

Yes, it should be for both!

josevalim · 2020-09-20T21:42:41Z

lib/ecto/repo/queryable.ex

+        :ok
+
+      rejected ->
+        raise ArgumentError, "Expected a struct or a list of structs, received #{inspect(rejected)}"


Messages should start in lowercase.

josevalim · 2020-09-20T21:43:07Z

lib/ecto/repo/queryable.ex

+  defp is_struct?(%{__struct__: _, __meta__: _}), do: true
+  defp is_struct?(_), do: false


We typically use is_ or ? but not both. In this case, since it is not a guard, ? works better. :)

Suggested change

defp is_struct?(%{__struct__: _, __meta__: _}), do: true

defp is_struct?(_), do: false

defp schema?(%{__struct__: _, __meta__: _}), do: true

defp schema?(_), do: false

@josevalim I have a conceptual question: does Map.put(%{}, :__struct__, ...) imply that the result is a struct?

In my conception, matching on %_{__meta__: _} would be a little bit more semantic in this regard, but I really don't know if there is a practical difference

This is a subset match, so you can match as above, but you shouldn’t build the struct manually as you may miss some fields. In any case your syntax is definitely nicer!

josevalim · 2020-09-20T21:43:29Z

It is looking great! I have dropped some comments.

v0idpwn · 2020-09-20T21:47:18Z

Thanks, @josevalim, I'll fix those ASAP :)

qcam · 2020-09-21T07:48:12Z

lib/ecto/repo/queryable.ex

+    struct
+    |> Map.fetch!(pk)
+    |> case do
+      nil ->
+        raise ArgumentError, "Ecto.Repo.reload/2 expects existent structs, found a `nil` id"
+      key ->
+        key
+    end


Would this be more concise?

Suggested change

struct

|> Map.fetch!(pk)

|> case do

nil ->

raise ArgumentError, "Ecto.Repo.reload/2 expects existent structs, found a `nil` id"

key ->

key

end

case struct do

%{^pk => key} when key != nil -> key

_ -> raise ArgumentError, "Ecto.Repo.reload/2 expects existent structs"

end

qcam · 2020-09-21T07:52:42Z

lib/ecto/repo/queryable.ex

@@ -485,4 +536,28 @@ defmodule Ecto.Repo.Queryable do
      query: query,
      message: "expected a from expression with a schema"
  end
+
+  defp assert_structs!(structs) when is_list(structs) do


Shall we guard against the case when users input different struct types in the list?

Repo.reload([%Post{}, %Comment{}])

I'll cover this case

qcam · 2020-09-21T07:54:34Z

test/ecto/repo_test.exs

@@ -220,6 +220,33 @@ defmodule Ecto.RepoTest do
    end
  end

+  describe "reload" do
+    test "raises when input have a `nil` pk" do


Suggested change

test "raises when input have a `nil` pk" do

test "raises when input structs do not have valid primary keys" do

maybe?

qcam · 2020-09-21T08:04:18Z

lib/ecto/repo/queryable.ex

+    if result_count < expected_count do
+      raise Ecto.TooFewResultsError, queryable: query, min: expected_count, count: result_count
+    end
+
+    result


If we guarantee same ordering in reload!/2, should we also do the same for reload/2?

lib/ecto/repo/queryable.ex

qcam · 2020-09-21T08:11:55Z

lib/ecto/repo/queryable.ex

@@ -428,6 +459,26 @@ defmodule Ecto.Repo.Queryable do
    Query.where(queryable, [], ^Enum.to_list(clauses))
  end

+  defp query_for_reload([head| _] = structs, opts) do


Suggested change

defp query_for_reload([head| _] = structs, opts) do

defp query_for_reload([%schema{} | _] = structs, opts) do

maybe? Worse error message in case of a list of normal maps however.

v0idpwn · 2020-09-28T00:54:15Z

Hi! I just fixed most of the suggestions. I think there is room for improvement in the code, but I didn't figure out how to do it.

v0idpwn · 2020-09-28T01:00:05Z

@josevalim let me know if you wanna merge without a squash, I may improve the commits to make a better history :)

v0idpwn · 2020-10-02T14:38:02Z

Bump :)

(just in case you lost the notification)

josevalim · 2020-10-03T10:07:19Z

lib/ecto/repo.ex

+    * `:prefix` - The prefix to run the query on (such as the schema path
+      in Postgres or the database in MySQL). This will be applied to all `from`
+      and `join`s in the query that did not have a prefix previously given
+      either via the `:prefix` option on `join`/`from` or via `@schema_prefix`
+      in the schema. For more information see the "Query Prefix" section of the
+      `Ecto.Query` documentation.


I don't think we should accept this option, rather, we should use the prefix from the structs themselves. You can find it under the __meta__ field.

You can test this by using Ecto.put_meta(struct, :prefix, "another") in test/ecto/repo_test.exs and then using assert_receive to check to which prefix the query went to. :)

If that doesn't work, have an integration test that sets Ecto.put_meta(struct, :prefix, "invalid") and try to reload it and see that it will fail.

josevalim · 2020-10-03T10:08:12Z

lib/ecto/repo.ex

+  When using with lists, ordering is guaranteed to be kept. Results not found in
+  the database will be returned as `nil`.


Suggested change

When using with lists, ordering is guaranteed to be kept. Results not found in

the database will be returned as `nil`.

When using with lists, it is expected that all of the structs in the list belong

to the same schema. Ordering is guaranteed to be kept. Results not found in the

database will be returned as `nil`.

lib/ecto/repo/queryable.ex

josevalim · 2020-10-03T10:11:02Z

lib/ecto/repo/queryable.ex

+    with  [] <- Enum.reject(structs, &schema?/1),
+         true <- Enum.all?(structs, &(&1.__struct__ == head.__struct__)) do
+      :ok
+    else
+      rejected when is_list(rejected) ->
+        raise ArgumentError, "expected a struct or a list of structs, received #{inspect(rejected)}"
+
+      false ->
+        raise ArgumentError, "expected an homogenous list, received different struct types"
+    end


This can be clearer as conditionals:

if not Enum.all?(structs, &schema?/1) do raise ArgumentError, "expected a struct or a list of structs, received #{inspect(rejected)}" end if not Enum.all?(structs, &(&1.__struct__ == head.__struct__)) do raise ArgumentError, "expected an homogenous list, received different struct types" end :ok

josevalim · 2020-10-03T10:13:39Z

lib/ecto/repo/queryable.ex

+    end
+  end
+
+  defp query_for_reload(struct), do: query_for_reload([struct])


I would remove this clause and call it as query_for_reload([struct]) in the reload/reload! functions. :)

josevalim · 2020-10-03T10:16:28Z

Awesome work @v0idpwn! I have added some minor comments and a note about prefix handling. In particular, I think you need to do this:

%{query | prefix: head.__meta__.prefix}

I have added some notes on how to write the test for it.

v0idpwn · 2020-10-07T07:16:21Z

Just pushed those fixes :)

josevalim · 2020-10-08T15:21:51Z

💚 💙 💜 💛 ❤️

Add Repo.reload/2 and Repo.reload!/2

0eb76c5

josevalim reviewed Sep 20, 2020

View reviewed changes

qcam reviewed Sep 21, 2020

View reviewed changes

ensure ordering, avoid receiving multiple struct types

e5bec35

Set callbacks and docs

9fbcbc8

josevalim reviewed Oct 3, 2020

View reviewed changes

lib/ecto/repo/queryable.ex Show resolved Hide resolved

josevalim reviewed Oct 3, 2020

View reviewed changes

v0idpwn added 2 commits October 7, 2020 04:03

Get prefix from struct

fcab78d

Improved code quality on assert_schema!

5cf7a92

josevalim merged commit 0ec6688 into elixir-ecto:master Oct 8, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Repo.reload/2 and Repo.reload!/2 #3416

Add Repo.reload/2 and Repo.reload!/2 #3416

v0idpwn commented Sep 20, 2020 •

edited

josevalim Sep 20, 2020

polvalente Sep 23, 2020

josevalim Sep 23, 2020

polvalente Sep 23, 2020

josevalim Sep 20, 2020

qcam Sep 21, 2020

josevalim Sep 23, 2020

josevalim Sep 20, 2020

josevalim Sep 20, 2020

polvalente Sep 23, 2020

josevalim Sep 23, 2020

josevalim commented Sep 20, 2020

v0idpwn commented Sep 20, 2020

qcam Sep 21, 2020

qcam Sep 21, 2020

v0idpwn Sep 21, 2020

qcam Sep 21, 2020

qcam Sep 21, 2020

qcam Sep 21, 2020 •

edited

v0idpwn commented Sep 28, 2020 •

edited

v0idpwn commented Sep 28, 2020

v0idpwn commented Oct 2, 2020

josevalim Oct 3, 2020

josevalim Oct 3, 2020

josevalim Oct 3, 2020

josevalim Oct 3, 2020 •

edited

josevalim Oct 3, 2020

josevalim commented Oct 3, 2020

v0idpwn commented Oct 7, 2020

josevalim commented Oct 8, 2020

		defp is_struct?(%{__struct__: _, __meta__: _}), do: true
		defp is_struct?(_), do: false

	test "raises when input have a `nil` pk" do
	test "raises when input structs do not have valid primary keys" do

	defp query_for_reload([head\| _] = structs, opts) do
	defp query_for_reload([%schema{} \| _] = structs, opts) do

		When using with lists, ordering is guaranteed to be kept. Results not found in
		the database will be returned as `nil`.

Add Repo.reload/2 and Repo.reload!/2 #3416

Add Repo.reload/2 and Repo.reload!/2 #3416

Conversation

v0idpwn commented Sep 20, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

josevalim commented Sep 20, 2020

v0idpwn commented Sep 20, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qcam Sep 21, 2020 • edited

Choose a reason for hiding this comment

v0idpwn commented Sep 28, 2020 • edited

v0idpwn commented Sep 28, 2020

v0idpwn commented Oct 2, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

josevalim Oct 3, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

josevalim commented Oct 3, 2020

v0idpwn commented Oct 7, 2020

josevalim commented Oct 8, 2020

v0idpwn commented Sep 20, 2020 •

edited

qcam Sep 21, 2020 •

edited

v0idpwn commented Sep 28, 2020 •

edited

josevalim Oct 3, 2020 •

edited