Skip to content

Commit 30261f9

Browse files
committed
refactor(delta): simplify after multi-agent review pass
Net: 77 fewer lines, same behavior, same tests green. Code reuse ---------- * `Reducer.add_to_coll/5`, `remove_from_coll/4`, `replace_in_coll/5` — collapse 14 near-duplicate Add/Remove/Alter/Rename apply_op clauses into uniform one-liners. Each helper accepts a thunk for the error message so inspect-heavy strings only build on the failure path. * `MigrationGenerator.snapshot_filename?/2` and `dev_snapshot_filename?/1` — single source of truth for the `YYYYMMDDHHMMSS[_dev].json` convention. Previously defined inline in 5 places (2× squash, 1× migrate_snapshots, 2× migration_generator). * `ash_postgres.squash_snapshots.ex apply_squash(..., :delta)` — drop the `maybe_hydrate_context/2` helper. `Reducer.apply_op(CreateTable)` already sets table/schema/multitenancy/repo/create_table_options, so the hand-hydration was both redundant and less complete than what the Reducer does. * `Codec.decode_atom/1` — subsumes the identical `decode_optional_atom/1`. Quality / efficiency -------------------- * `Reducer`: drop the dead `state_empty?/1` final-override pass on `load_reduced_state/2`. Inline `empty?` writes already track correctness for every consumer of the returned state (`pkey_operations/4`). * `Reducer.list_delta_files/2`: replace the 3-syscall `File.exists?`/`File.dir?`/`File.ls!` dance with a single `File.ls/1` pattern match. Also closes the TOCTOU gap. * `ash_postgres.migrate_snapshots.ex find_legacy_directories/1`: return pre-classified `{dir, all_files, legacy_files}` tuples so `migrate_directory/5` doesn't re-read every file via `v2?/1`. Each file is now read exactly once during discovery. * `migration_generator.ex move_rename_snapshots/3`: drop the `File.exists?(old_dir)` + `File.exists?(dst)` guards in favor of a single `File.ls/1`. Rename detection guarantees `new_dir` is empty, so the per-file guard was a defensive no-op.
1 parent ae1f9be commit 30261f9

5 files changed

Lines changed: 218 additions & 295 deletions

File tree

lib/migration_generator/migration_generator.ex

Lines changed: 42 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -201,6 +201,28 @@ defmodule AshPostgres.MigrationGenerator do
201201
end
202202
end
203203

204+
@snapshot_name_regex ~r/^\d{14}\.json$/
205+
@dev_snapshot_name_regex ~r/^\d{14}_dev\.json$/
206+
207+
@doc false
208+
# Does `name` (a basename, not a full path) match the snapshot file
209+
# convention `YYYYMMDDHHMMSS.json`? If `include_dev?` is true, also matches
210+
# the `*_dev.json` variant.
211+
def snapshot_filename?(name, include_dev? \\ false)
212+
213+
def snapshot_filename?(name, false),
214+
do: String.match?(name, @snapshot_name_regex)
215+
216+
def snapshot_filename?(name, true),
217+
do:
218+
String.match?(name, @snapshot_name_regex) or
219+
String.match?(name, @dev_snapshot_name_regex)
220+
221+
@doc false
222+
# Does `name` (a basename, not a full path) match a *_dev.json snapshot?
223+
def dev_snapshot_filename?(name),
224+
do: String.match?(name, @dev_snapshot_name_regex)
225+
204226
defp snapshot_path(%{snapshot_path: snapshot_path}, _) when not is_nil(snapshot_path),
205227
do: snapshot_path
206228

@@ -3252,10 +3274,7 @@ defmodule AshPostgres.MigrationGenerator do
32523274
if File.exists?(snapshot_path) do
32533275
snapshot_path
32543276
|> File.ls!()
3255-
|> Enum.filter(
3256-
&(String.match?(&1, ~r/^\d{14}\.json$/) or
3257-
(opts.dev and String.match?(&1, ~r/^\d{14}\_dev.json$/)))
3258-
)
3277+
|> Enum.filter(&snapshot_filename?(&1, opts.dev))
32593278
|> case do
32603279
[] ->
32613280
get_old_snapshot(folder, snapshot)
@@ -3306,11 +3325,9 @@ defmodule AshPostgres.MigrationGenerator do
33063325

33073326
if File.exists?(snapshot_dir) do
33083327
snapshot_files =
3309-
File.ls!(snapshot_dir)
3310-
|> Enum.filter(
3311-
&(String.match?(&1, ~r/^\d{14}\.json$/) or
3312-
(opts.dev and String.match?(&1, ~r/^\d{14}\_dev\.json$/)))
3313-
)
3328+
snapshot_dir
3329+
|> File.ls!()
3330+
|> Enum.filter(&snapshot_filename?(&1, opts.dev))
33143331

33153332
case snapshot_files do
33163333
[] ->
@@ -3683,27 +3700,22 @@ defmodule AshPostgres.MigrationGenerator do
36833700
multitenancy: snap.multitenancy
36843701
}
36853702

3686-
old_folder = get_snapshot_folder(old_snapshot, opts)
3687-
new_folder = get_snapshot_folder(new_snapshot, opts)
3688-
old_dir = get_snapshot_path(old_snapshot, old_folder)
3689-
new_dir = get_snapshot_path(new_snapshot, new_folder)
3690-
3691-
if File.exists?(old_dir) do
3692-
File.mkdir_p!(new_dir)
3693-
3694-
old_dir
3695-
|> File.ls!()
3696-
|> Enum.each(fn file ->
3697-
src = Path.join(old_dir, file)
3698-
dst = Path.join(new_dir, file)
3699-
3700-
# Skip if destination already exists — should not happen given
3701-
# rename detection only fires when the new dir is empty, but be
3702-
# defensive to avoid clobbering.
3703-
if !File.exists?(dst) do
3704-
File.rename!(src, dst)
3705-
end
3706-
end)
3703+
old_dir = get_snapshot_path(old_snapshot, get_snapshot_folder(old_snapshot, opts))
3704+
new_dir = get_snapshot_path(new_snapshot, get_snapshot_folder(new_snapshot, opts))
3705+
3706+
# Single File.ls — if old_dir is missing/unreadable we treat it as
3707+
# "nothing to move". Rename detection guarantees new_dir is empty,
3708+
# so no defensive per-file existence check is needed.
3709+
case File.ls(old_dir) do
3710+
{:ok, files} ->
3711+
File.mkdir_p!(new_dir)
3712+
3713+
Enum.each(files, fn file ->
3714+
File.rename!(Path.join(old_dir, file), Path.join(new_dir, file))
3715+
end)
3716+
3717+
{:error, _} ->
3718+
:ok
37073719
end
37083720
end)
37093721
end

lib/migration_generator/operation/codec.ex

Lines changed: 2 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -935,15 +935,12 @@ defmodule AshPostgres.MigrationGenerator.Operation.Codec do
935935

936936
def decode_multitenancy(mt) do
937937
%{
938-
strategy: mt |> Map.get(:strategy) |> decode_optional_atom(),
939-
attribute: mt |> Map.get(:attribute) |> decode_optional_atom(),
938+
strategy: mt |> Map.get(:strategy) |> decode_atom(),
939+
attribute: mt |> Map.get(:attribute) |> decode_atom(),
940940
global: Map.get(mt, :global)
941941
}
942942
end
943943

944-
defp decode_optional_atom(nil), do: nil
945-
defp decode_optional_atom(value), do: maybe_to_atom(value)
946-
947944
defp decode_atom(nil), do: nil
948945
defp decode_atom(value), do: maybe_to_atom(value)
949946

0 commit comments

Comments
 (0)