Repository: elixir-ecto/ecto
Pull request #2212 (2017-09-08 09:46:31)

Validate assocs length #2211

ebert[bot]: Hello, @bartekupartek! This is your first Pull Request that will be reviewed by [Ebert](https://ebertapp.io/), an automatic Code Review service. It will leave comments on this diff with potential issues and style violations found in the code as you push new commits. You can also see all the issues found on this Pull Request on its [review page](https://ebertapp.io/github/elixir-ecto/ecto/pulls/2212). Please check our [documentation](https://docs.ebertapp.io) for more information. 
josevalim: Thanks @bartekupartek! I wonder if it would be better to set those on the cast_assoc/cast_embed functions instead of adding a new function to the API?
bartekupartek: I need it in `put_assoc` with `validate_length` cause I have to insert only existing tags, but I will check how it will behave in`cast_assoc`.
josevalim: > I need it in put_assoc with validate_length cause I have to insert only existing tags, but I will check how it will behave incast_assoc.

Hrm, that's a good point. I think what you have then is the way to go.
bartekupartek: Ok thanks, I didn't considered `:embed` relation, I will try to correct this and `filter_empty`. 

Patch:
diff --git a/guides/Getting Started.md b/guides/Getting Started.md
index 0aa5405c..b95eb1be 100644
--- a/guides/Getting Started.md
+++ b/guides/Getting Started.md
@@ -99,7 +99,6 @@ def start(_type, _args) do
 `Elixir >= 1.5.0`:
 ```elixir
 def start(_type, _args) do
-  import Supervisor.Spec
 
   children = [
     Friends.Repo,
diff --git a/lib/ecto/changeset.ex b/lib/ecto/changeset.ex
index d2909490..fc64a74a 100644
--- a/lib/ecto/changeset.ex
+++ b/lib/ecto/changeset.ex
@@ -1626,7 +1626,7 @@ defmodule Ecto.Changeset do
           {value, :graphemes} when is_binary(value) ->
             {:string, String.length(value)}
           {value, _} when is_list(value) ->
-            {:list, length(value)}
+            {:list, list_length(changeset, field, value)}
         end
 
         error = ((is = opts[:is]) && wrong_length(type, length, is, opts)) ||
@@ -1641,6 +1641,15 @@ defmodule Ecto.Changeset do
   defp codepoints_length(<<_, rest::binary>>, acc), do: codepoints_length(rest, acc + 1)
   defp codepoints_length(<<>>, acc), do: acc
 
+  defp list_length(%{types: types}, field, value) do
+    case Map.fetch(types, field) do
+      {:ok, {tag, _association}} when tag in [:embed, :assoc] ->
+        length(Relation.filter_empty(value))
+      _ ->
+        length(value)
+    end
+  end
+
   defp wrong_length(_type, value, value, _opts), do: nil
   defp wrong_length(:string, _length, value, opts), do:
     {message(opts, "should be %{count} character(s)"), count: value, validation: :length, kind: :is}
@@ -2304,9 +2313,10 @@ defmodule Ecto.Changeset do
       ...> end)
       %{title: ["should be at least 3 characters"]}
 
-  Optionally function can accept three arguments: `changeset`, `field` and error tuple `{msg, opts}`.
-  It is useful whenever you want to extract validations rules from `changeset.validations`
-  to build detailed error description.
+  Optionally function can accept three arguments: `changeset`, `field`
+  and error tuple `{msg, opts}`. It is useful whenever you want to extract
+  validations rules from `changeset.validations` to build detailed error
+  description.
   """
   @spec traverse_errors(t, (error -> String.t) | (Changeset.t, atom, error -> String.t)) :: %{atom => [String.t]}
   def traverse_errors(%Changeset{errors: errors, changes: changes, types: types} = changeset, msg_func)
diff --git a/lib/ecto/changeset/relation.ex b/lib/ecto/changeset/relation.ex
index 902a2cfe..b9a762f2 100644
--- a/lib/ecto/changeset/relation.ex
+++ b/lib/ecto/changeset/relation.ex
@@ -29,15 +29,20 @@ defmodule Ecto.Changeset.Relation do
   """
   def empty?(%{cardinality: _}, %NotLoaded{}), do: true
   def empty?(%{cardinality: :many}, []), do: true
-  def empty?(%{cardinality: :many}, changes) do
-    Enum.all?(changes, fn
-      %Changeset{action: action} when action in [:replace, :delete] -> true
-      _ -> false
-    end)
-  end
+  def empty?(%{cardinality: :many}, changes), do: filter_empty(changes) == []
   def empty?(%{cardinality: :one}, nil), do: true
   def empty?(%{}, _), do: false
 
+  @doc """
+  Filter empty changes
+  """
+  def filter_empty(changes) do
+    Enum.filter(changes, fn
+      %Changeset{action: action} when action in [:replace, :delete] -> false
+      _ -> true
+    end)
+  end
+
   @doc """
   Applies related changeset changes
   """
diff --git a/lib/ecto/migration.ex b/lib/ecto/migration.ex
index 3ee02688..57447789 100644
--- a/lib/ecto/migration.ex
+++ b/lib/ecto/migration.ex
@@ -834,6 +834,8 @@ defmodule Ecto.Migration do
   end
 
   def references(table, opts) when is_binary(table) and is_list(opts) do
+    repo_opts = Keyword.take(Runner.repo_config(:migration_primary_key, []), [:type])
+    opts = Keyword.merge(repo_opts, opts)
     reference = struct(%Reference{table: table}, opts)
 
     unless reference.on_delete in [:nothing, :delete_all, :nilify_all] do
diff --git a/lib/ecto/repo.ex b/lib/ecto/repo.ex
index bd472bfc..24443795 100644
--- a/lib/ecto/repo.ex
+++ b/lib/ecto/repo.ex
@@ -64,6 +64,12 @@ defmodule Ecto.Repo do
   The schema can be of any value. The path represents the database name
   while options are simply merged in.
 
+  URL can include query parameters to override shared and adapter-specific
+  options `ssl`, `timeout`, `pool_timeout`, `pool_size`:
+
+    config :my_app, Repo,
+        url: "ecto://postgres:postgres@localhost/ecto_simple?ssl=true&pool_size=10"
+
   In case the URL needs to be dynamically configured, for example by
   reading a system environment variable, such can be done via the
   `c:init/2` repository callback:
diff --git a/lib/ecto/repo/supervisor.ex b/lib/ecto/repo/supervisor.ex
index 9a1e5b63..8dbb75e6 100644
--- a/lib/ecto/repo/supervisor.ex
+++ b/lib/ecto/repo/supervisor.ex
@@ -3,6 +3,7 @@ defmodule Ecto.Repo.Supervisor do
   use Supervisor
 
   @defaults [timeout: 15000, pool_timeout: 5000]
+  @integer_url_query_params ["timeout", "pool_size", "pool_timeout"]
 
   @doc """
   Starts the repo supervisor.
@@ -83,7 +84,7 @@ defmodule Ecto.Repo.Supervisor do
 
   The format must be:
 
-      "ecto://username:password@hostname:port/database"
+      "ecto://username:password@hostname:port/database?ssl=true&timeout=1000"
 
   """
   def parse_url(""), do: []
@@ -102,13 +103,44 @@ defmodule Ecto.Repo.Supervisor do
     destructure [username, password], info.userinfo && String.split(info.userinfo, ":")
     "/" <> database = info.path
 
-    opts = [username: username,
-            password: password,
-            database: database,
-            hostname: info.host,
-            port:     info.port]
+    url_opts = [username: username,
+                password: password,
+                database: database,
+                hostname: info.host,
+                port:     info.port]
 
-    for {k, v} <- opts, v, do: {k, if(is_binary(v), do: URI.decode(v), else: v)}
+    query_opts = parse_uri_query(info)
+
+    for {k, v} <- url_opts ++ query_opts, not is_nil(v), do: {k, if(is_binary(v), do: URI.decode(v), else: v)}
+  end
+
+  defp parse_uri_query(%URI{query: nil}),
+    do: []
+  defp parse_uri_query(%URI{query: query} = url) do
+    query
+    |> URI.query_decoder()
+    |> Enum.reduce([], fn
+      {"ssl", "true"}, acc ->
+        [{:ssl, true}] ++ acc
+
+      {"ssl", "false"}, acc ->
+        [{:ssl, false}] ++ acc
+
+      {key, value}, acc when key in @integer_url_query_params ->
+        [{String.to_atom(key), parse_integer!(key, value, url)}] ++ acc
+
+      {key, _value}, _acc ->
+        raise Ecto.InvalidURLError, url: url, message: "unsupported query parameter `#{key}`"
+    end)
+  end
+
+  defp parse_integer!(key, value, url) do
+    case Integer.parse(value) do
+      {int, ""} ->
+        int
+      _ ->
+        raise Ecto.InvalidURLError, url: url, message: "can not parse value `#{value}` for parameter `#{key}` as an integer"
+    end
   end
 
   ## Callbacks


Test patch:
diff --git a/test/ecto/adapters/mysql_test.exs b/test/ecto/adapters/mysql_test.exs
index 3f277595..4d58a9d9 100644
--- a/test/ecto/adapters/mysql_test.exs
+++ b/test/ecto/adapters/mysql_test.exs
@@ -7,6 +7,7 @@ defmodule Ecto.Adapters.MySQLTest do
 
   alias Ecto.Queryable
   alias Ecto.Adapters.MySQL.Connection, as: SQL
+  alias Ecto.Migration.Reference
 
   defmodule Schema do
     use Ecto.Schema
@@ -549,7 +550,7 @@ defmodule Ecto.Adapters.MySQLTest do
   # DDL
 
   import Ecto.Migration, only: [table: 1, table: 2, index: 2, index: 3,
-                                references: 1, references: 2, constraint: 3]
+                                constraint: 3]
 
   test "executing a string during migration" do
     assert execute_ddl("example") == ["example"]
@@ -584,7 +585,7 @@ defmodule Ecto.Adapters.MySQLTest do
 
   test "create table with prefix" do
     create = {:create, table(:posts, prefix: :foo),
-               [{:add, :category_0, references(:categories, prefix: :foo), []}]}
+               [{:add, :category_0, %Reference{table: :categories}, []}]}
 
     assert execute_ddl(create) == ["""
     CREATE TABLE `foo`.`posts` (`category_0` BIGINT UNSIGNED,
@@ -602,11 +603,11 @@ defmodule Ecto.Adapters.MySQLTest do
   test "create table with references" do
     create = {:create, table(:posts),
                [{:add, :id, :serial, [primary_key: true]},
-                {:add, :category_0, references(:categories), []},
-                {:add, :category_1, references(:categories, name: :foo_bar), []},
-                {:add, :category_2, references(:categories, on_delete: :nothing), []},
-                {:add, :category_3, references(:categories, on_delete: :delete_all), [null: false]},
-                {:add, :category_4, references(:categories, on_delete: :nilify_all), []}]}
+                {:add, :category_0, %Reference{table: :categories}, []},
+                {:add, :category_1, %Reference{table: :categories, name: :foo_bar}, []},
+                {:add, :category_2, %Reference{table: :categories, on_delete: :nothing}, []},
+                {:add, :category_3, %Reference{table: :categories, on_delete: :delete_all}, [null: false]},
+                {:add, :category_4, %Reference{table: :categories, on_delete: :nilify_all}, []}]}
 
     assert execute_ddl(create) == ["""
     CREATE TABLE `posts` (`id` bigint unsigned not null auto_increment,
@@ -691,10 +692,10 @@ defmodule Ecto.Adapters.MySQLTest do
   test "alter table" do
     alter = {:alter, table(:posts),
                [{:add, :title, :string, [default: "Untitled", size: 100, null: false]},
-                {:add, :author_id, references(:author), []},
+                {:add, :author_id, %Reference{table: :author}, []},
                 {:modify, :price, :numeric, [precision: 8, scale: 2, null: true]},
                 {:modify, :cost, :integer, [null: false, default: nil]},
-                {:modify, :permalink_id, references(:permalinks), null: false},
+                {:modify, :permalink_id, %Reference{table: :permalinks}, null: false},
                 {:remove, :summary}]}
 
     assert execute_ddl(alter) == ["""
@@ -710,8 +711,8 @@ defmodule Ecto.Adapters.MySQLTest do
 
   test "alter table with prefix" do
     alter = {:alter, table(:posts, prefix: :foo),
-               [{:add, :author_id, references(:author), []},
-                {:modify, :permalink_id, references(:permalinks), null: false}]}
+               [{:add, :author_id, %Reference{table: :author}, []},
+                {:modify, :permalink_id, %Reference{table: :permalinks}, null: false}]}
 
     assert execute_ddl(alter) == ["""
     ALTER TABLE `foo`.`posts` ADD `author_id` BIGINT UNSIGNED,
diff --git a/test/ecto/adapters/postgres_test.exs b/test/ecto/adapters/postgres_test.exs
index a29cc803..826ae829 100644
--- a/test/ecto/adapters/postgres_test.exs
+++ b/test/ecto/adapters/postgres_test.exs
@@ -711,8 +711,9 @@ defmodule Ecto.Adapters.PostgresTest do
 
   # DDL
 
-  import Ecto.Migration, only: [table: 1, table: 2, index: 2, index: 3, references: 1,
-                                references: 2, constraint: 2, constraint: 3]
+  alias Ecto.Migration.Reference
+  import Ecto.Migration, only: [table: 1, table: 2, index: 2, index: 3,
+                                constraint: 2, constraint: 3]
 
   test "executing a string during migration" do
     assert execute_ddl("example") == ["example"]
@@ -739,7 +740,7 @@ defmodule Ecto.Adapters.PostgresTest do
 
   test "create table with prefix" do
     create = {:create, table(:posts, prefix: :foo),
-              [{:add, :category_0, references(:categories), []}]}
+              [{:add, :category_0, %Reference{table: :categories}, []}]}
 
     assert execute_ddl(create) == ["""
     CREATE TABLE "foo"."posts"
@@ -750,7 +751,7 @@ defmodule Ecto.Adapters.PostgresTest do
   test "create table with comment on columns and table" do
     create = {:create, table(:posts, comment: "comment"),
               [
-                {:add, :category_0, references(:categories), [comment: "column comment"]},
+                {:add, :category_0, %Reference{table: :categories}, [comment: "column comment"]},
                 {:add, :created_at, :timestamp, []},
                 {:add, :updated_at, :timestamp, [comment: "column comment 2"]}
               ]}
@@ -765,7 +766,7 @@ defmodule Ecto.Adapters.PostgresTest do
 
   test "create table with comment on table" do
     create = {:create, table(:posts, comment: "table comment", prefix: "foo"),
-              [{:add, :category_0, references(:categories), []}]}
+              [{:add, :category_0, %Reference{table: :categories}, []}]}
     assert execute_ddl(create) == [remove_newlines("""
     CREATE TABLE "foo"."posts"
     ("category_0" bigint CONSTRAINT "posts_category_0_fkey" REFERENCES "foo"."categories"("id"))
@@ -776,7 +777,7 @@ defmodule Ecto.Adapters.PostgresTest do
   test "create table with comment on columns" do
     create = {:create, table(:posts, prefix: "foo"),
               [
-                {:add, :category_0, references(:categories), [comment: "column comment"]},
+                {:add, :category_0, %Reference{table: :categories}, [comment: "column comment"]},
                 {:add, :created_at, :timestamp, []},
                 {:add, :updated_at, :timestamp, [comment: "column comment 2"]}
               ]}
@@ -791,15 +792,15 @@ defmodule Ecto.Adapters.PostgresTest do
   test "create table with references" do
     create = {:create, table(:posts),
               [{:add, :id, :serial, [primary_key: true]},
-               {:add, :category_0, references(:categories), []},
-               {:add, :category_1, references(:categories, name: :foo_bar), []},
-               {:add, :category_2, references(:categories, on_delete: :nothing), []},
-               {:add, :category_3, references(:categories, on_delete: :delete_all), [null: false]},
-               {:add, :category_4, references(:categories, on_delete: :nilify_all), []},
-               {:add, :category_5, references(:categories, on_update: :nothing), []},
-               {:add, :category_6, references(:categories, on_update: :update_all), [null: false]},
-               {:add, :category_7, references(:categories, on_update: :nilify_all), []},
-               {:add, :category_8, references(:categories, on_delete: :nilify_all, on_update: :update_all), [null: false]}]}
+               {:add, :category_0, %Reference{table: :categories}, []},
+               {:add, :category_1, %Reference{table: :categories, name: :foo_bar}, []},
+               {:add, :category_2, %Reference{table: :categories, on_delete: :nothing}, []},
+               {:add, :category_3, %Reference{table: :categories, on_delete: :delete_all}, [null: false]},
+               {:add, :category_4, %Reference{table: :categories, on_delete: :nilify_all}, []},
+               {:add, :category_5, %Reference{table: :categories, on_update: :nothing}, []},
+               {:add, :category_6, %Reference{table: :categories, on_update: :update_all}, [null: false]},
+               {:add, :category_7, %Reference{table: :categories, on_update: :nilify_all}, []},
+               {:add, :category_8, %Reference{table: :categories, on_delete: :nilify_all, on_update: :update_all}, [null: false]}]}
 
     assert execute_ddl(create) == ["""
     CREATE TABLE "posts" ("id" serial,
@@ -875,10 +876,10 @@ defmodule Ecto.Adapters.PostgresTest do
   test "alter table" do
     alter = {:alter, table(:posts),
              [{:add, :title, :string, [default: "Untitled", size: 100, null: false]},
-              {:add, :author_id, references(:author), []},
+              {:add, :author_id, %Reference{table: :author}, []},
               {:modify, :price, :numeric, [precision: 8, scale: 2, null: true]},
               {:modify, :cost, :integer, [null: false, default: nil]},
-              {:modify, :permalink_id, references(:permalinks), null: false},
+              {:modify, :permalink_id, %Reference{table: :permalinks}, null: false},
               {:remove, :summary}]}
 
     assert execute_ddl(alter) == ["""
@@ -901,7 +902,7 @@ defmodule Ecto.Adapters.PostgresTest do
     alter = {:alter, table(:posts, comment: "table comment"),
              [{:add, :title, :string, [default: "Untitled", size: 100, null: false, comment: "column comment"]},
               {:modify, :price, :numeric, [precision: 8, scale: 2, null: true]},
-              {:modify, :permalink_id, references(:permalinks), [null: false, comment: "column comment"]},
+              {:modify, :permalink_id, %Reference{table: :permalinks}, [null: false, comment: "column comment"]},
               {:remove, :summary}]}
 
     assert execute_ddl(alter) == [remove_newlines("""
@@ -922,8 +923,8 @@ defmodule Ecto.Adapters.PostgresTest do
 
   test "alter table with prefix" do
     alter = {:alter, table(:posts, prefix: :foo),
-             [{:add, :author_id, references(:author, prefix: :foo), []},
-              {:modify, :permalink_id, references(:permalinks, prefix: :foo), null: false}]}
+             [{:add, :author_id, %Reference{table: :author}, []},
+              {:modify, :permalink_id, %Reference{table: :permalinks}, null: false}]}
 
     assert execute_ddl(alter) == ["""
     ALTER TABLE "foo"."posts"
diff --git a/test/ecto/changeset_test.exs b/test/ecto/changeset_test.exs
index 19d2edaa..013d83a5 100644
--- a/test/ecto/changeset_test.exs
+++ b/test/ecto/changeset_test.exs
@@ -869,6 +869,15 @@ defmodule Ecto.ChangesetTest do
     assert changeset.errors == [topics: {"yada", count: 10, validation: :length, kind: :is}]
   end
 
+  test "validate_length/3 with associations" do
+    post = %Post{comments: [%Comment{id: 1}]}
+    changeset = change(post) |> put_assoc(:comments, []) |> validate_length(:comments, min: 1)
+    assert changeset.errors == [comments: {"should have at least %{count} item(s)", count: 1, validation: :length, kind: :min}]
+
+    changeset = change(post) |> put_assoc(:comments, [%Comment{id: 2}, %Comment{id: 3}]) |> validate_length(:comments, max: 2)
+    assert changeset.valid?
+  end
+
   test "validate_number/3" do
     changeset = changeset(%{"upvotes" => 3})
                 |> validate_number(:upvotes, greater_than: 0)
diff --git a/test/ecto/migration_test.exs b/test/ecto/migration_test.exs
index 1c54d60e..8296c55b 100644
--- a/test/ecto/migration_test.exs
+++ b/test/ecto/migration_test.exs
@@ -514,5 +514,16 @@ defmodule Ecto.MigrationTest do
     assert "SELECT 2" = last_command()
   end
 
+  test "references foreign keys types must be the same as primary defaults" do
+    %{runner: runner} = Process.get(:ecto_migration)
+    Agent.update(runner, fn state ->
+      config = Keyword.put(state.config, :migration_primary_key, [type: :binary_id])
+      Map.put(state, :config, config)
+    end)
+
+    assert references(:posts) ==
+           %Reference{table: "posts", column: :id, type: :binary_id}
+  end
+
   defp last_command(), do: Process.get(:last_command)
 end
diff --git a/test/ecto/repo/supervisor_test.exs b/test/ecto/repo/supervisor_test.exs
index 87fd31f9..1fddf791 100644
--- a/test/ecto/repo/supervisor_test.exs
+++ b/test/ecto/repo/supervisor_test.exs
@@ -59,10 +59,28 @@ defmodule Ecto.Repo.SupervisorTest do
     assert {:port, 12345} in url
   end
 
+  test "parse_url query string" do
+    encoded_url = URI.encode("ecto://eric:it+й@host:12345/mydb?ssl=true&pool_timeout=1000&timeout=1000&pool_size=42")
+    url = parse_url(encoded_url)
+    assert {:password, "it+й"} in url
+    assert {:username, "eric"} in url
+    assert {:hostname, "host"} in url
+    assert {:database, "mydb"} in url
+    assert {:port, 12345} in url
+    assert {:ssl, true} in url
+    assert {:timeout, 1000} in url
+    assert {:pool_timeout, 1000} in url
+    assert {:pool_size, 42} in url
+  end
+
   test "parse_url returns no config when blank" do
     assert parse_url("") == []
   end
 
+  test "parse_url keeps false values" do
+    assert {:ssl, false} in parse_url("ecto://eric:it+й@host:12345/mydb?ssl=false")
+  end
+
   test "parse_urls empty username/password" do
     url = parse_url("ecto://host:12345/mydb")
     assert !Keyword.has_key?(url, :username)
@@ -85,5 +103,15 @@ defmodule Ecto.Repo.SupervisorTest do
     assert_raise Ecto.InvalidURLError, ~r"path should be a database name", fn ->
       parse_url("ecto://eric:hunter2@host:123")
     end
+
+    assert_raise Ecto.InvalidURLError, ~r"unsupported query parameter `uknown_param`", fn ->
+      parse_url("ecto://eric:it+й@host:12345/mydb?uknown_param=value")
+    end
+
+    for key <- ["timeout", "pool_size", "pool_timeout"] do
+      assert_raise Ecto.InvalidURLError, ~r"can not parse value `not_an_int` for parameter `#{key}` as an integer", fn ->
+        parse_url("ecto://eric:it+й@host:12345/mydb?#{key}=not_an_int")
+      end
+    end
   end
 end