From 7e70d8c1e86e058ac1bc99ce0c117788fc51eb1a Mon Sep 17 00:00:00 2001 From: Emad Shaaban Date: Tue, 18 Jun 2024 18:22:55 +0300 Subject: [PATCH 1/6] fix: ensure index keys are atoms in generated migrations --- .../migration_generator.ex | 22 ++++++++++-- lib/migration_generator/operation.ex | 35 ++++++++++--------- 2 files changed, 39 insertions(+), 18 deletions(-) diff --git a/lib/migration_generator/migration_generator.ex b/lib/migration_generator/migration_generator.ex index 2d96b5d5..a1f3c1e0 100644 --- a/lib/migration_generator/migration_generator.ex +++ b/lib/migration_generator/migration_generator.ex @@ -2884,14 +2884,14 @@ defmodule AshPostgres.MigrationGenerator do Enum.map(keys, fn key -> case Ash.Resource.Info.field(resource, key) do %Ash.Resource.Attribute{} = attribute -> - to_string(attribute.source || attribute.name) + attribute.source || attribute.name %Ash.Resource.Calculation{} -> sql = AshPostgres.DataLayer.Info.calculation_to_sql(resource, key) || raise "Must define an entry for :#{key} in `postgres.calculations_to_sql`, or skip this identity with `postgres.skip_unique_indexes`" - "(" <> sql <> ")" + {:sql, "(" <> sql <> ")"} end end), where: @@ -3000,6 +3000,17 @@ defmodule AshPostgres.MigrationGenerator do %{index | fields: fields} end) end) + |> Map.update!(:identities, fn identities -> + Enum.map(identities, fn identity -> + keys = + Enum.map(identity.keys, fn + {:sql, value} when is_binary(value) -> ["sql", value] + key -> key + end) + + %{identity | keys: keys} + end) + end) |> Jason.encode!(pretty: true) end @@ -3251,6 +3262,13 @@ defmodule AshPostgres.MigrationGenerator do |> Map.put_new(:all_tenants?, false) |> Map.put_new(:where, nil) |> Map.put_new(:nils_distinct?, true) + |> Map.update!( + :keys, + &Enum.map(&1, fn + ["sql", value] when is_binary(value) -> {:sql, value} + key -> maybe_to_atom(key) + end) + ) end defp add_index_name(%{name: name} = index, table) do diff --git a/lib/migration_generator/operation.ex b/lib/migration_generator/operation.ex index 4198baa3..a3ed422b 100644 --- a/lib/migration_generator/operation.ex +++ b/lib/migration_generator/operation.ex @@ -30,6 +30,9 @@ defmodule AshPostgres.MigrationGenerator.Operation do # sobelow_skip ["DOS.StringToAtom"] def as_atom(value), do: Macro.inspect_atom(:remote_call, String.to_atom(value)) + def index_key({:sql, value}) when is_binary(value), do: inspect(value) + def index_key(value) when is_binary(value) or is_atom(value), do: inspect(value) + def option(key, value) when key in [:nulls_distinct, "nulls_distinct"] do if !value do "#{as_atom(key)}: #{inspect(value)}" @@ -819,20 +822,20 @@ defmodule AshPostgres.MigrationGenerator.Operation do base_filter && where -> where = "(#{where}) AND (#{base_filter})" - "create unique_index(:#{as_atom(table)}, [#{Enum.map_join(keys, ", ", &inspect/1)}], #{join(["name: \"#{index_name}\"", option("prefix", schema), option("nulls_distinct", nils_distinct?), option("where", where)])})" + "create unique_index(:#{as_atom(table)}, [#{Enum.map_join(keys, ", ", &index_key/1)}], #{join(["name: \"#{index_name}\"", option("prefix", schema), option("nulls_distinct", nils_distinct?), option("where", where)])})" base_filter -> base_filter = "(#{base_filter})" - "create unique_index(:#{as_atom(table)}, [#{Enum.map_join(keys, ", ", &inspect/1)}], where: \"#{base_filter}\", #{join(["name: \"#{index_name}\"", option("prefix", schema), option("nulls_distinct", nils_distinct?)])})" + "create unique_index(:#{as_atom(table)}, [#{Enum.map_join(keys, ", ", &index_key/1)}], where: \"#{base_filter}\", #{join(["name: \"#{index_name}\"", option("prefix", schema), option("nulls_distinct", nils_distinct?)])})" where -> where = "(#{where})" - "create unique_index(:#{as_atom(table)}, [#{Enum.map_join(keys, ", ", &inspect/1)}], #{join(["name: \"#{index_name}\"", option("prefix", schema), option("nulls_distinct", nils_distinct?), option("where", where)])})" + "create unique_index(:#{as_atom(table)}, [#{Enum.map_join(keys, ", ", &index_key/1)}], #{join(["name: \"#{index_name}\"", option("prefix", schema), option("nulls_distinct", nils_distinct?), option("where", where)])})" true -> - "create unique_index(:#{as_atom(table)}, [#{Enum.map_join(keys, ", ", &inspect/1)}], #{join(["name: \"#{index_name}\"", option("prefix", schema), option("nulls_distinct", nils_distinct?)])})" + "create unique_index(:#{as_atom(table)}, [#{Enum.map_join(keys, ", ", &index_key/1)}], #{join(["name: \"#{index_name}\"", option("prefix", schema), option("nulls_distinct", nils_distinct?)])})" end end @@ -853,7 +856,7 @@ defmodule AshPostgres.MigrationGenerator.Operation do index_name = index_name || "#{table}_#{name}_index" - "drop_if_exists unique_index(:#{as_atom(table)}, [#{Enum.map_join(keys, ", ", &inspect/1)}], #{join(["name: \"#{index_name}\"", option("prefix", schema)])})" + "drop_if_exists unique_index(:#{as_atom(table)}, [#{Enum.map_join(keys, ", ", &index_key/1)}], #{join(["name: \"#{index_name}\"", option("prefix", schema)])})" end end @@ -939,9 +942,9 @@ defmodule AshPostgres.MigrationGenerator.Operation do ]) if opts == "" do - "create index(:#{as_atom(table)}, [#{Enum.map_join(keys, ", ", &inspect/1)}])" + "create index(:#{as_atom(table)}, [#{Enum.map_join(keys, ", ", &index_key/1)}])" else - "create index(:#{as_atom(table)}, [#{Enum.map_join(keys, ", ", &inspect/1)}], #{opts})" + "create index(:#{as_atom(table)}, [#{Enum.map_join(keys, ", ", &index_key/1)}], #{opts})" end end @@ -960,9 +963,9 @@ defmodule AshPostgres.MigrationGenerator.Operation do ]) if opts == "" do - "drop_if_exists index(:#{as_atom(table)}, [#{Enum.map_join(keys, ", ", &inspect/1)}])" + "drop_if_exists index(:#{as_atom(table)}, [#{Enum.map_join(keys, ", ", &index_key/1)}])" else - "drop_if_exists index(:#{as_atom(table)}, [#{Enum.map_join(keys, ", ", &inspect/1)}], #{opts})" + "drop_if_exists index(:#{as_atom(table)}, [#{Enum.map_join(keys, ", ", &index_key/1)}], #{opts})" end end end @@ -991,9 +994,9 @@ defmodule AshPostgres.MigrationGenerator.Operation do ]) if opts == "" do - "create index(:#{as_atom(table)}, [#{Enum.map_join(keys, ", ", &inspect/1)}])" + "create index(:#{as_atom(table)}, [#{Enum.map_join(keys, ", ", &index_key/1)}])" else - "create index(:#{as_atom(table)}, [#{Enum.map_join(keys, ", ", &inspect/1)}], #{opts})" + "create index(:#{as_atom(table)}, [#{Enum.map_join(keys, ", ", &index_key/1)}], #{opts})" end end @@ -1011,9 +1014,9 @@ defmodule AshPostgres.MigrationGenerator.Operation do ]) if opts == "" do - "drop_if_exists index(:#{as_atom(table)}, [#{Enum.map_join(keys, ", ", &inspect/1)}])" + "drop_if_exists index(:#{as_atom(table)}, [#{Enum.map_join(keys, ", ", &index_key/1)}])" else - "drop_if_exists index(:#{as_atom(table)}, [#{Enum.map_join(keys, ", ", &inspect/1)}], #{opts})" + "drop_if_exists index(:#{as_atom(table)}, [#{Enum.map_join(keys, ", ", &index_key/1)}], #{opts})" end end end @@ -1158,7 +1161,7 @@ defmodule AshPostgres.MigrationGenerator.Operation do index_name = index_name || "#{table}_#{name}_index" - "drop_if_exists unique_index(:#{as_atom(table)}, [#{Enum.map_join(keys, ", ", &inspect/1)}], #{join(["name: \"#{index_name}\"", option(:prefix, schema)])})" + "drop_if_exists unique_index(:#{as_atom(table)}, [#{Enum.map_join(keys, ", ", &index_key/1)}], #{join(["name: \"#{index_name}\"", option(:prefix, schema)])})" end def down(%{ @@ -1179,9 +1182,9 @@ defmodule AshPostgres.MigrationGenerator.Operation do index_name = index_name || "#{table}_#{name}_index" if base_filter do - "create unique_index(:#{as_atom(table)}, [#{Enum.map_join(keys, ", ", &inspect/1)}], where: \"#{base_filter}\", #{join(["name: \"#{index_name}\"", option(:prefix, schema)])})" + "create unique_index(:#{as_atom(table)}, [#{Enum.map_join(keys, ", ", &index_key/1)}], where: \"#{base_filter}\", #{join(["name: \"#{index_name}\"", option(:prefix, schema)])})" else - "create unique_index(:#{as_atom(table)}, [#{Enum.map_join(keys, ", ", &inspect/1)}], #{join(["name: \"#{index_name}\"", option(:prefix, schema)])})" + "create unique_index(:#{as_atom(table)}, [#{Enum.map_join(keys, ", ", &index_key/1)}], #{join(["name: \"#{index_name}\"", option(:prefix, schema)])})" end end end From 66e868b8b3463aeb3d9028b2710033f5d92476da Mon Sep 17 00:00:00 2001 From: Zach Daniel Date: Tue, 18 Jun 2024 16:37:32 -0400 Subject: [PATCH 2/6] improvement: better type handling using new type inference --- .credo.exs | 2 +- lib/sql_implementation.ex | 67 ++++++++++++++++++++++--------- mix.exs | 2 +- mix.lock | 2 +- test/load_test.exs | 2 +- test/migration_generator_test.exs | 2 +- 6 files changed, 53 insertions(+), 24 deletions(-) diff --git a/.credo.exs b/.credo.exs index 9442b952..fa9affad 100644 --- a/.credo.exs +++ b/.credo.exs @@ -123,7 +123,7 @@ {Credo.Check.Refactor.MatchInCondition, []}, {Credo.Check.Refactor.NegatedConditionsInUnless, []}, {Credo.Check.Refactor.NegatedConditionsWithElse, []}, - {Credo.Check.Refactor.Nesting, [max_nesting: 5]}, + {Credo.Check.Refactor.Nesting, [max_nesting: 7]}, {Credo.Check.Refactor.UnlessWithElse, []}, {Credo.Check.Refactor.WithClauses, []}, diff --git a/lib/sql_implementation.ex b/lib/sql_implementation.ex index 57e973ec..d037d2d9 100644 --- a/lib/sql_implementation.ex +++ b/lib/sql_implementation.ex @@ -229,8 +229,10 @@ defmodule AshPostgres.SqlImplementation do true -> [:any] end - |> Enum.concat(Map.keys(Ash.Query.Operator.operator_overloads(name) || %{})) - |> Enum.map(fn types -> + |> then(fn types -> + Enum.concat(Map.keys(Ash.Query.Operator.operator_overloads(name) || %{}), types) + end) + |> Enum.flat_map(fn types -> case types do :same -> types = @@ -238,32 +240,58 @@ defmodule AshPostgres.SqlImplementation do :same end - closest_fitting_type(types, values) + [closest_fitting_type(types, values)] :any -> - for _ <- values do - :any - end + [] types -> - closest_fitting_type(types, values) + [types] end end) + # this doesn't seem right to me |> Enum.filter(fn types -> Enum.all?(types, &(vagueness(&1) == 0)) end) |> case do - [type] -> - if type == :any || type == {:in, :any} do - nil - else - type - end - - # There are things we could likely do here - # We only say "we know what types these are" when we explicitly know - _ -> - Enum.map(values, fn _ -> nil end) + [types] -> + types + + types -> + Enum.find_value(types, Enum.map(values, fn _ -> nil end), fn types -> + if length(types) == length(values) do + types + |> Enum.zip(values) + |> Enum.reduce_while([], fn {type, value}, vals -> + type = Ash.Type.get_type(type) + # this means its a known type + if Ash.Type.ash_type?(type) do + {type, constraints} = + case type do + {type, constraints} -> {type, constraints} + type -> {type, []} + end + + case value do + %Ash.Query.Function.Type{arguments: [_, ^type | _]} -> + {:cont, vals ++ [:any]} + + %Ash.Query.Ref{attribute: %{type: ^type}} -> + {:cont, vals ++ [:any]} + + _ -> + if Ash.Type.matches_type?(type, value, constraints) do + {:cont, vals ++ [parameterized_type(type, constraints)]} + else + {:halt, nil} + end + end + else + {:halt, nil} + end + end) + end + end) end end @@ -370,7 +398,8 @@ defmodule AshPostgres.SqlImplementation do end end - defp fill_in_known_type({type, value}), do: {array_to_in(type), value} + defp fill_in_known_type({type, value}), + do: {array_to_in(type), value} defp array_to_in({:array, v}), do: {:in, array_to_in(v)} diff --git a/mix.exs b/mix.exs index 47f8977c..e53450e2 100644 --- a/mix.exs +++ b/mix.exs @@ -162,7 +162,7 @@ defmodule AshPostgres.MixProject do # Run "mix help deps" to learn about dependencies. defp deps do [ - {:ash, ash_version("~> 3.0 and >= 3.0.13")}, + {:ash, ash_version("~> 3.0 and >= 3.0.15")}, {:ash_sql, ash_sql_version("~> 0.2 and >= 0.2.6")}, {:ecto_sql, "~> 3.9"}, {:ecto, "~> 3.9"}, diff --git a/mix.lock b/mix.lock index 21d65d6a..560cd842 100644 --- a/mix.lock +++ b/mix.lock @@ -1,5 +1,5 @@ %{ - "ash": {:hex, :ash, "3.0.13", "9111fa58362f82fd6687635c12ea96ce1958d24772e3c773fbc8b0893a7e7d47", [:mix], [{:comparable, "~> 1.0", [hex: :comparable, repo: "hexpm", optional: false]}, {:decimal, "~> 2.0", [hex: :decimal, repo: "hexpm", optional: false]}, {:ecto, "~> 3.7", [hex: :ecto, repo: "hexpm", optional: false]}, {:ets, "~> 0.8", [hex: :ets, repo: "hexpm", optional: false]}, {:jason, ">= 1.0.0", [hex: :jason, repo: "hexpm", optional: false]}, {:picosat_elixir, "~> 0.2", [hex: :picosat_elixir, repo: "hexpm", optional: true]}, {:plug, ">= 0.0.0", [hex: :plug, repo: "hexpm", optional: true]}, {:reactor, ">= 0.8.1 and < 1.0.0-0", [hex: :reactor, repo: "hexpm", optional: false]}, {:simple_sat, ">= 0.1.1 and < 1.0.0-0", [hex: :simple_sat, repo: "hexpm", optional: true]}, {:spark, ">= 2.1.18 and < 3.0.0-0", [hex: :spark, repo: "hexpm", optional: false]}, {:splode, "~> 0.2", [hex: :splode, repo: "hexpm", optional: false]}, {:stream_data, "~> 1.0", [hex: :stream_data, repo: "hexpm", optional: false]}, {:telemetry, "~> 1.1", [hex: :telemetry, repo: "hexpm", optional: false]}], "hexpm", "b5c1a9c428eac939a313bfea5e4509e8ac39beaa4707bd0deb5f7f310b1022c3"}, + "ash": {:hex, :ash, "3.0.15", "1cea8ca799dc8281d09e316a189fddfb27a2a29a0b0e5af369aa83d6f731ca75", [:mix], [{:comparable, "~> 1.0", [hex: :comparable, repo: "hexpm", optional: false]}, {:decimal, "~> 2.0", [hex: :decimal, repo: "hexpm", optional: false]}, {:ecto, "~> 3.7", [hex: :ecto, repo: "hexpm", optional: false]}, {:ets, "~> 0.8", [hex: :ets, repo: "hexpm", optional: false]}, {:jason, ">= 1.0.0", [hex: :jason, repo: "hexpm", optional: false]}, {:picosat_elixir, "~> 0.2", [hex: :picosat_elixir, repo: "hexpm", optional: true]}, {:plug, ">= 0.0.0", [hex: :plug, repo: "hexpm", optional: true]}, {:reactor, ">= 0.8.1 and < 1.0.0-0", [hex: :reactor, repo: "hexpm", optional: false]}, {:simple_sat, ">= 0.1.1 and < 1.0.0-0", [hex: :simple_sat, repo: "hexpm", optional: true]}, {:spark, ">= 2.1.18 and < 3.0.0-0", [hex: :spark, repo: "hexpm", optional: false]}, {:splode, "~> 0.2", [hex: :splode, repo: "hexpm", optional: false]}, {:stream_data, "~> 1.0", [hex: :stream_data, repo: "hexpm", optional: false]}, {:telemetry, "~> 1.1", [hex: :telemetry, repo: "hexpm", optional: false]}], "hexpm", "97abfed57f2bf29c3889c51f0dfa23b382a1361a9d70e7d82d7e59a2be6bdc73"}, "ash_sql": {:hex, :ash_sql, "0.2.5", "8b50c3178776263b912e1b60e161e2bcf08a907a38abf703edf8a8a0a51b3fe2", [:mix], [{:ash, "~> 3.0", [hex: :ash, repo: "hexpm", optional: false]}, {:ecto, "~> 3.9", [hex: :ecto, repo: "hexpm", optional: false]}, {:ecto_sql, "~> 3.9", [hex: :ecto_sql, repo: "hexpm", optional: false]}], "hexpm", "0d5d8606738a17c4e8c0be4244623df721abee5072cee69d31c2711c36d0548f"}, "benchee": {:hex, :benchee, "1.3.1", "c786e6a76321121a44229dde3988fc772bca73ea75170a73fd5f4ddf1af95ccf", [:mix], [{:deep_merge, "~> 1.0", [hex: :deep_merge, repo: "hexpm", optional: false]}, {:statistex, "~> 1.0", [hex: :statistex, repo: "hexpm", optional: false]}, {:table, "~> 0.1.0", [hex: :table, repo: "hexpm", optional: true]}], "hexpm", "76224c58ea1d0391c8309a8ecbfe27d71062878f59bd41a390266bf4ac1cc56d"}, "bunt": {:hex, :bunt, "1.0.0", "081c2c665f086849e6d57900292b3a161727ab40431219529f13c4ddcf3e7a44", [:mix], [], "hexpm", "dc5f86aa08a5f6fa6b8096f0735c4e76d54ae5c9fa2c143e5a1fc7c1cd9bb6b5"}, diff --git a/test/load_test.exs b/test/load_test.exs index 200e1f29..4ab6d325 100644 --- a/test/load_test.exs +++ b/test/load_test.exs @@ -1,6 +1,6 @@ defmodule AshPostgres.Test.LoadTest do use AshPostgres.RepoCase, async: false - alias AshPostgres.Test.{Author, Comment, Post, Record, TempEntity, User, StatefulPostFollower} + alias AshPostgres.Test.{Author, Comment, Post, Record, StatefulPostFollower, TempEntity, User} require Ash.Query diff --git a/test/migration_generator_test.exs b/test/migration_generator_test.exs index 1a323202..804ac50b 100644 --- a/test/migration_generator_test.exs +++ b/test/migration_generator_test.exs @@ -164,7 +164,7 @@ defmodule AshPostgres.MigrationGeneratorTest do # the migration creates unique_indexes using the `source` on the attributes of the identity on the resource assert file_contents =~ - ~S{create unique_index(:posts, ["t_w_s", "title"], name: "posts_thing_with_source_index")} + ~S{create unique_index(:posts, ["title", "t_w_s"], name: "posts_thing_with_source_index")} end end From 0a20f2ca471c81e4b308dcdc7fff80b29f375e10 Mon Sep 17 00:00:00 2001 From: Zach Daniel Date: Tue, 18 Jun 2024 16:37:56 -0400 Subject: [PATCH 3/6] chore: release version v2.0.10 --- CHANGELOG.md | 19 +++++++++++++++++++ mix.exs | 2 +- 2 files changed, 20 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6f226053..84b5becf 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,25 @@ See [Conventional Commits](Https://conventionalcommits.org) for commit guideline +## [v2.0.10](https://github.com/ash-project/ash_postgres/compare/v2.0.9...v2.0.10) (2024-06-18) + + + + +### Bug Fixes: + +* update ash_sql to fix query generation issues + +* ensure that parens are always added to calculation generated SQL + +* properly get calculation sql + +### Improvements: + +* better type handling using new type inference + +* identities w/ calculations and where clauses in upserts + ## [v2.0.9](https://github.com/ash-project/ash_postgres/compare/v2.0.8...v2.0.9) (2024-06-13) diff --git a/mix.exs b/mix.exs index e53450e2..d219fc15 100644 --- a/mix.exs +++ b/mix.exs @@ -5,7 +5,7 @@ defmodule AshPostgres.MixProject do The PostgreSQL data layer for Ash Framework """ - @version "2.0.9" + @version "2.0.10" def project do [ From 1af3f7d273da89183373e44f3a5273bec80f3af7 Mon Sep 17 00:00:00 2001 From: Zach Daniel Date: Tue, 18 Jun 2024 16:38:19 -0400 Subject: [PATCH 4/6] chore: update ash_sql --- mix.lock | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/mix.lock b/mix.lock index 560cd842..864f1461 100644 --- a/mix.lock +++ b/mix.lock @@ -1,6 +1,6 @@ %{ "ash": {:hex, :ash, "3.0.15", "1cea8ca799dc8281d09e316a189fddfb27a2a29a0b0e5af369aa83d6f731ca75", [:mix], [{:comparable, "~> 1.0", [hex: :comparable, repo: "hexpm", optional: false]}, {:decimal, "~> 2.0", [hex: :decimal, repo: "hexpm", optional: false]}, {:ecto, "~> 3.7", [hex: :ecto, repo: "hexpm", optional: false]}, {:ets, "~> 0.8", [hex: :ets, repo: "hexpm", optional: false]}, {:jason, ">= 1.0.0", [hex: :jason, repo: "hexpm", optional: false]}, {:picosat_elixir, "~> 0.2", [hex: :picosat_elixir, repo: "hexpm", optional: true]}, {:plug, ">= 0.0.0", [hex: :plug, repo: "hexpm", optional: true]}, {:reactor, ">= 0.8.1 and < 1.0.0-0", [hex: :reactor, repo: "hexpm", optional: false]}, {:simple_sat, ">= 0.1.1 and < 1.0.0-0", [hex: :simple_sat, repo: "hexpm", optional: true]}, {:spark, ">= 2.1.18 and < 3.0.0-0", [hex: :spark, repo: "hexpm", optional: false]}, {:splode, "~> 0.2", [hex: :splode, repo: "hexpm", optional: false]}, {:stream_data, "~> 1.0", [hex: :stream_data, repo: "hexpm", optional: false]}, {:telemetry, "~> 1.1", [hex: :telemetry, repo: "hexpm", optional: false]}], "hexpm", "97abfed57f2bf29c3889c51f0dfa23b382a1361a9d70e7d82d7e59a2be6bdc73"}, - "ash_sql": {:hex, :ash_sql, "0.2.5", "8b50c3178776263b912e1b60e161e2bcf08a907a38abf703edf8a8a0a51b3fe2", [:mix], [{:ash, "~> 3.0", [hex: :ash, repo: "hexpm", optional: false]}, {:ecto, "~> 3.9", [hex: :ecto, repo: "hexpm", optional: false]}, {:ecto_sql, "~> 3.9", [hex: :ecto_sql, repo: "hexpm", optional: false]}], "hexpm", "0d5d8606738a17c4e8c0be4244623df721abee5072cee69d31c2711c36d0548f"}, + "ash_sql": {:hex, :ash_sql, "0.2.6", "097a191b138af6bd5104ae0e166db5deb443fe3a4616b349fffe98120382765d", [:mix], [{:ash, "~> 3.0", [hex: :ash, repo: "hexpm", optional: false]}, {:ecto, "~> 3.9", [hex: :ecto, repo: "hexpm", optional: false]}, {:ecto_sql, "~> 3.9", [hex: :ecto_sql, repo: "hexpm", optional: false]}], "hexpm", "076abb07d7762537880a12699b756c7c86c1a4e98fcd0dc8c8059de93f7e9265"}, "benchee": {:hex, :benchee, "1.3.1", "c786e6a76321121a44229dde3988fc772bca73ea75170a73fd5f4ddf1af95ccf", [:mix], [{:deep_merge, "~> 1.0", [hex: :deep_merge, repo: "hexpm", optional: false]}, {:statistex, "~> 1.0", [hex: :statistex, repo: "hexpm", optional: false]}, {:table, "~> 0.1.0", [hex: :table, repo: "hexpm", optional: true]}], "hexpm", "76224c58ea1d0391c8309a8ecbfe27d71062878f59bd41a390266bf4ac1cc56d"}, "bunt": {:hex, :bunt, "1.0.0", "081c2c665f086849e6d57900292b3a161727ab40431219529f13c4ddcf3e7a44", [:mix], [], "hexpm", "dc5f86aa08a5f6fa6b8096f0735c4e76d54ae5c9fa2c143e5a1fc7c1cd9bb6b5"}, "castore": {:hex, :castore, "1.0.7", "b651241514e5f6956028147fe6637f7ac13802537e895a724f90bf3e36ddd1dd", [:mix], [], "hexpm", "da7785a4b0d2a021cd1292a60875a784b6caef71e76bf4917bdee1f390455cf5"}, From aa40778cc46971a42fdac3e6b96c49de007b364e Mon Sep 17 00:00:00 2001 From: Emad Shaaban Date: Tue, 18 Jun 2024 18:22:55 +0300 Subject: [PATCH 5/6] fix: ensure index keys are atoms in generated migrations --- lib/migration_generator/migration_generator.ex | 2 +- lib/migration_generator/operation.ex | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/migration_generator/migration_generator.ex b/lib/migration_generator/migration_generator.ex index a1f3c1e0..4628f2db 100644 --- a/lib/migration_generator/migration_generator.ex +++ b/lib/migration_generator/migration_generator.ex @@ -3266,7 +3266,7 @@ defmodule AshPostgres.MigrationGenerator do :keys, &Enum.map(&1, fn ["sql", value] when is_binary(value) -> {:sql, value} - key -> maybe_to_atom(key) + key -> key end) ) end diff --git a/lib/migration_generator/operation.ex b/lib/migration_generator/operation.ex index a3ed422b..27cd917f 100644 --- a/lib/migration_generator/operation.ex +++ b/lib/migration_generator/operation.ex @@ -31,7 +31,7 @@ defmodule AshPostgres.MigrationGenerator.Operation do def as_atom(value), do: Macro.inspect_atom(:remote_call, String.to_atom(value)) def index_key({:sql, value}) when is_binary(value), do: inspect(value) - def index_key(value) when is_binary(value) or is_atom(value), do: inspect(value) + def index_key(value) when is_binary(value) or is_atom(value), do: ":#{as_atom(value)}" def option(key, value) when key in [:nulls_distinct, "nulls_distinct"] do if !value do From 39e2cfd14941208eedc12f7ff5fe365114f4c3ff Mon Sep 17 00:00:00 2001 From: Zach Daniel Date: Wed, 19 Jun 2024 12:41:24 -0400 Subject: [PATCH 6/6] fix: consider old/new formats for generating migrations --- .../migration_generator.ex | 20 ++++++++++++---- lib/migration_generator/operation.ex | 2 +- test/migration_generator_test.exs | 24 +++++++++---------- 3 files changed, 28 insertions(+), 18 deletions(-) diff --git a/lib/migration_generator/migration_generator.ex b/lib/migration_generator/migration_generator.ex index 4628f2db..b586f6d6 100644 --- a/lib/migration_generator/migration_generator.ex +++ b/lib/migration_generator/migration_generator.ex @@ -2891,7 +2891,7 @@ defmodule AshPostgres.MigrationGenerator do AshPostgres.DataLayer.Info.calculation_to_sql(resource, key) || raise "Must define an entry for :#{key} in `postgres.calculations_to_sql`, or skip this identity with `postgres.skip_unique_indexes`" - {:sql, "(" <> sql <> ")"} + "(" <> sql <> ")" end end), where: @@ -3004,8 +3004,8 @@ defmodule AshPostgres.MigrationGenerator do Enum.map(identities, fn identity -> keys = Enum.map(identity.keys, fn - {:sql, value} when is_binary(value) -> ["sql", value] - key -> key + value when is_binary(value) -> %{"type" => "string", "value" => value} + value when is_atom(value) -> %{"type" => "atom", "value" => value} end) %{identity | keys: keys} @@ -3265,8 +3265,18 @@ defmodule AshPostgres.MigrationGenerator do |> Map.update!( :keys, &Enum.map(&1, fn - ["sql", value] when is_binary(value) -> {:sql, value} - key -> key + %{type: "atom", value: value} -> + maybe_to_atom(value) + + %{type: "string", value: value} -> + value + + value -> + if String.contains?(value, "(") do + value + else + maybe_to_atom(value) + end end) ) end diff --git a/lib/migration_generator/operation.ex b/lib/migration_generator/operation.ex index 27cd917f..a9fef7ff 100644 --- a/lib/migration_generator/operation.ex +++ b/lib/migration_generator/operation.ex @@ -30,7 +30,7 @@ defmodule AshPostgres.MigrationGenerator.Operation do # sobelow_skip ["DOS.StringToAtom"] def as_atom(value), do: Macro.inspect_atom(:remote_call, String.to_atom(value)) - def index_key({:sql, value}) when is_binary(value), do: inspect(value) + def index_key(value) when is_binary(value), do: inspect(value) def index_key(value) when is_binary(value) or is_atom(value), do: ":#{as_atom(value)}" def option(key, value) when key in [:nulls_distinct, "nulls_distinct"] do diff --git a/test/migration_generator_test.exs b/test/migration_generator_test.exs index 804ac50b..40c8ea3e 100644 --- a/test/migration_generator_test.exs +++ b/test/migration_generator_test.exs @@ -156,15 +156,15 @@ defmodule AshPostgres.MigrationGeneratorTest do # the migration creates unique_indexes based on the identities of the resource assert file_contents =~ - ~S{create unique_index(:posts, ["title"], name: "posts_title_index")} + ~S{create unique_index(:posts, [:title], name: "posts_title_index")} # the migration creates unique_indexes based on the identities of the resource assert file_contents =~ - ~S{create unique_index(:posts, ["title", "second_title"], name: "posts_thing_index")} + ~S{create unique_index(:posts, [:title, :second_title], name: "posts_thing_index")} # the migration creates unique_indexes using the `source` on the attributes of the identity on the resource assert file_contents =~ - ~S{create unique_index(:posts, ["title", "t_w_s"], name: "posts_thing_with_source_index")} + ~S{create unique_index(:posts, [:title, :t_w_s], name: "posts_thing_with_source_index")} end end @@ -241,7 +241,7 @@ defmodule AshPostgres.MigrationGeneratorTest do assert file_contents =~ ~S{create index(:posts, ["id"]} assert file_contents =~ - ~S{create unique_index(:posts, ["second_title"], name: "posts_second_title_index", prefix: "example", nulls_distinct: false, where: "((second_title like '%foo%'))")} + ~S{create unique_index(:posts, [:second_title], name: "posts_second_title_index", prefix: "example", nulls_distinct: false, where: "((second_title like '%foo%'))")} # the migration adds the id, with its default assert file_contents =~ @@ -255,7 +255,7 @@ defmodule AshPostgres.MigrationGeneratorTest do # the migration creates unique_indexes based on the identities of the resource assert file_contents =~ - ~S{create unique_index(:posts, ["title"], name: "posts_title_index", prefix: "example")} + ~S{create unique_index(:posts, [:title], name: "posts_title_index", prefix: "example")} end end @@ -535,11 +535,11 @@ defmodule AshPostgres.MigrationGeneratorTest do assert [file_before, _] = String.split( File.read!(file2), - ~S{create unique_index(:posts, ["title"], name: "posts_title_index", where: "(title != 'fred' and title != 'george')")} + ~S{create unique_index(:posts, [:title], name: "posts_title_index", where: "(title != 'fred' and title != 'george')")} ) assert file_before =~ - ~S{drop_if_exists unique_index(:posts, ["title"], name: "posts_title_index")} + ~S{drop_if_exists unique_index(:posts, [:title], name: "posts_title_index")} end test "when adding a field, it adds the field" do @@ -753,18 +753,18 @@ defmodule AshPostgres.MigrationGeneratorTest do file1_content = File.read!(file1) assert file1_content =~ - "create unique_index(:posts, [\"title\"], name: \"posts_title_index\")" + "create unique_index(:posts, [:title], name: \"posts_title_index\")" file2_content = File.read!(file2) assert file2_content =~ - "drop_if_exists unique_index(:posts, [\"title\"], name: \"posts_title_index\")" + "drop_if_exists unique_index(:posts, [:title], name: \"posts_title_index\")" assert file2_content =~ - "create unique_index(:posts, [\"name\"], name: \"posts_unique_name_index\")" + "create unique_index(:posts, [:name], name: \"posts_unique_name_index\")" assert file2_content =~ - "create unique_index(:posts, [\"title\"], name: \"posts_unique_title_index\")" + "create unique_index(:posts, [:title], name: \"posts_unique_title_index\")" end test "when an attribute exists only on some of the resources that use the same table, it isn't marked as null: false" do @@ -1241,7 +1241,7 @@ defmodule AshPostgres.MigrationGeneratorTest do assert [file] = Path.wildcard("test_migration_path/**/*_migrate_resources*.exs") assert File.read!(file) =~ - ~S{create unique_index(:users, ["name"], name: "users_unique_name_index")} + ~S{create unique_index(:users, [:name], name: "users_unique_name_index")} end test "when modified, the foreign key is dropped before modification" do