Repository: elixir-ecto/ecto
Pull request #2286 (2017-10-26 03:41:17)

allow more options for migration_primary_key config
Right now configuration for `:migration_primary_key` doesn't allow passing any additional options to the `add` function. This PR allows other options to be passed. Of the available options the only one that makes sense right now would be `default` but this change will keep things flexible for different options that might be rolled out in the future.

An example of how this might be utilized:
On a product my team is working on, sometimes it's convenient for our client-side developers to add a record directly to the dev database. We like to add a default to the `id` fields to make that manual entry easier.

`config :app, App.Repo, migration_primary_key: [id: :uuid, type: :binary_id, default: {:fragment, "gen_random_uuid()"}]`

used the `timestamps` function as a style reference

ebert[bot]: Hello, @dallincrane! 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/2286). Please check our [documentation](https://docs.ebertapp.io) for more information. 
josevalim: Can you please add a test showing that custom options also work? Look at the existing tests for this feature for inspiration! Thanks!

Patch:
diff --git a/lib/ecto/migration.ex b/lib/ecto/migration.ex
index 9b6cb3df..86e2d991 100644
--- a/lib/ecto/migration.ex
+++ b/lib/ecto/migration.ex
@@ -308,7 +308,12 @@ defmodule Ecto.Migration do
 
       if table.primary_key do
         opts = Runner.repo_config(:migration_primary_key, [])
-        add(opts[:name] || :id, opts[:type] || :bigserial, primary_key: true)
+        opts = Keyword.put(opts, :primary_key, true)
+
+        {name, opts} = Keyword.pop(opts, :name, :id)
+        {type, opts} = Keyword.pop(opts, :type, :bigserial)
+
+        add(name, type, opts)
       end
 
       unquote(block)
diff --git a/lib/ecto/query.ex b/lib/ecto/query.ex
index f65beba3..5e6d4f0d 100644
--- a/lib/ecto/query.ex
+++ b/lib/ecto/query.ex
@@ -1127,7 +1127,8 @@ defmodule Ecto.Query do
   is given as an argument to an aggregate function.
 
   `group_by` also accepts a list of atoms where each atom refers to
-  a field in source.
+  a field in source. For more complicated queries you can access fields
+  directly instead of atoms.
 
   ## Keywords examples
 
@@ -1139,6 +1140,11 @@ defmodule Ecto.Query do
       # Using atoms
       from(p in Post, group_by: :category, select: {p.category, count(p.id)})
 
+      # Using direct fields access
+      from(p in Post,
+        join: c in assoc(p, :category)
+        group_by: [p.id, c.name])
+
   ## Expressions example
 
       Post |> group_by([p], p.category) |> select([p], count(p.id))


Test patch:
diff --git a/test/ecto/migration_test.exs b/test/ecto/migration_test.exs
index 3b26027d..11310b47 100644
--- a/test/ecto/migration_test.exs
+++ b/test/ecto/migration_test.exs
@@ -156,6 +156,16 @@ defmodule Ecto.MigrationTest do
            {:create, table, [{:add, :uuid, :uuid, [primary_key: true]}]}
   end
 
+  @tag repo_config: [migration_primary_key: [type: :uuid, default: {:fragment, "gen_random_uuid()"}]]
+  test "forward: create a table with custom primary key options" do
+    create(table = table(:posts)) do
+    end
+    flush()
+
+    assert last_command() ==
+           {:create, table, [{:add, :id, :uuid, [primary_key: true, default: {:fragment, "gen_random_uuid()"}]}]}
+  end
+
   @tag repo_config: [migration_timestamps: [type: :utc_datetime, null: true]]
   test "forward: create a table with timestamps" do
     create(table = table(:posts)) do