Skip to content

Commit ae1f9be

Browse files
committed
chore(delta): address review suggestions — error handling, dead code, style
Follow-up to 3c178e1 applying the remaining suggestions from the /review pass: * Codec.decode_delta/1: replace Jason.decode!/2 with a case on Jason.decode/2 that wraps malformed input in ArgumentError and rejects non-object top-level JSON. Adds tests for both cases. * Reducer.state_to_initial_delta/1: remove. Dead code — callers use MigrationGenerator.initial_operations_for_state/1 directly. * do_fetch_operations/4: use Reducer.empty_state/1 instead of the third inline copy of the baseline state map, so the empty-state schema has one source of truth. * Mix.Tasks.AshPostgres.SquashSnapshots.run/1: add missing @impl Mix.Task. * apply_squash(..., :full): drop the `_ = folder` suppressor and destructure as `_folder` instead. * Codec.maybe_to_atom/1: add `sobelow_skip ["DOS.StringToAtom"]` pragma, matching the convention already used throughout the codebase (operation.ex, custom_index.ex, resource_generator/spec.ex).
1 parent 3c178e1 commit ae1f9be

5 files changed

Lines changed: 52 additions & 35 deletions

File tree

lib/migration_generator/migration_generator.ex

Lines changed: 10 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -2352,23 +2352,16 @@ defmodule AshPostgres.MigrationGenerator do
23522352

23532353
defp do_fetch_operations(snapshot, nil, opts, acc) do
23542354
if resource_has_meaningful_content?(snapshot) do
2355-
empty_snapshot = %{
2356-
attributes: [],
2357-
identities: [],
2358-
schema: nil,
2359-
custom_indexes: [],
2360-
custom_statements: [],
2361-
check_constraints: [],
2362-
table: snapshot.table,
2363-
repo: snapshot.repo,
2364-
base_filter: nil,
2365-
empty?: true,
2366-
multitenancy: %{
2367-
attribute: nil,
2368-
strategy: nil,
2369-
global: nil
2370-
}
2371-
}
2355+
# Use the Reducer's canonical empty state so this and delta reduction
2356+
# share one definition of "empty". `empty_state/1` produces a schema-less
2357+
# baseline with all the scalar-fields (base_filter, create_table_options,
2358+
# …) explicitly defaulted.
2359+
empty_snapshot =
2360+
AshPostgres.MigrationGenerator.Reducer.empty_state(%{
2361+
table: snapshot.table,
2362+
schema: nil,
2363+
repo: snapshot.repo
2364+
})
23722365

23732366
do_fetch_operations(snapshot, empty_snapshot, opts, [
23742367
%Operation.CreateTable{

lib/migration_generator/operation/codec.ex

Lines changed: 22 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -159,11 +159,27 @@ defmodule AshPostgres.MigrationGenerator.Operation.Codec do
159159
`:previous_hash`, `:resulting_hash`, `:migration`, `:generated_at`,
160160
`:operations` (list of operation structs).
161161
162-
Raises `ArgumentError` if the version field is missing or not equal to 2 —
163-
legacy full-state files must be migrated via `mix ash_postgres.migrate_snapshots`.
162+
Raises `ArgumentError` if:
163+
* the JSON is malformed
164+
* the top-level value is not a JSON object
165+
* the version field is missing or not equal to #{@delta_version}
166+
— legacy full-state files must be migrated via
167+
`mix ash_postgres.migrate_snapshots`.
164168
"""
165169
def decode_delta(json) when is_binary(json) do
166-
decoded = Jason.decode!(json, keys: :atoms!)
170+
decoded =
171+
case Jason.decode(json, keys: :atoms!) do
172+
{:ok, map} when is_map(map) ->
173+
map
174+
175+
{:ok, other} ->
176+
raise ArgumentError,
177+
"Expected delta snapshot to be a JSON object, got #{inspect(other, limit: :infinity, printable_limit: 200)}"
178+
179+
{:error, %Jason.DecodeError{} = err} ->
180+
raise ArgumentError,
181+
"Could not parse delta snapshot JSON: #{Exception.message(err)}"
182+
end
167183

168184
case Map.get(decoded, :version) do
169185
@delta_version ->
@@ -960,6 +976,9 @@ defmodule AshPostgres.MigrationGenerator.Operation.Codec do
960976
end
961977

962978
defp maybe_to_atom(value) when is_atom(value), do: value
979+
# sobelow_skip ["DOS.StringToAtom"]
980+
# Snapshot files are developer-committed and reviewed. This matches the
981+
# pattern already skipped in migration_generator.ex.
963982
defp maybe_to_atom(value), do: String.to_atom(value)
964983

965984
defp iso8601_now do

lib/migration_generator/reducer.ex

Lines changed: 0 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -75,19 +75,6 @@ defmodule AshPostgres.MigrationGenerator.Reducer do
7575
end
7676
end
7777

78-
@doc """
79-
Produce a list of operation structs that, when reduced from an empty state,
80-
reconstruct the given existing state.
81-
82-
Uses the same machinery the live generator uses for brand-new tables — we
83-
invoke the migration generator's empty-snapshot diff branch so the ops are
84-
byte-identical with what `do_fetch_operations/4` would emit on first-time
85-
table creation.
86-
"""
87-
def state_to_initial_delta(state) do
88-
AshPostgres.MigrationGenerator.initial_operations_for_state(state)
89-
end
90-
9178
@doc "The empty starting-state for a given snapshot (path + multitenancy context)."
9279
def empty_state(snapshot) do
9380
%{

lib/mix/tasks/ash_postgres.squash_snapshots.ex

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -58,6 +58,7 @@ defmodule Mix.Tasks.AshPostgres.SquashSnapshots do
5858
@timestamp_regex ~r/^\d{14}\.json$/
5959
@dev_regex ~r/^\d{14}_dev\.json$/
6060

61+
@impl Mix.Task
6162
def run(args) do
6263
{opts, []} = OptionParser.parse!(args, strict: @switches)
6364

@@ -189,7 +190,7 @@ defmodule Mix.Tasks.AshPostgres.SquashSnapshots do
189190
"""
190191
end
191192

192-
defp apply_squash({folder, snapshots, last_snapshot, into_snapshot, :full}, _opts) do
193+
defp apply_squash({_folder, snapshots, last_snapshot, into_snapshot, :full}, _opts) do
193194
for snapshot <- snapshots, snapshot != last_snapshot do
194195
File.rm!(snapshot)
195196
end
@@ -198,7 +199,6 @@ defmodule Mix.Tasks.AshPostgres.SquashSnapshots do
198199
File.rename!(last_snapshot, into_snapshot)
199200
end
200201

201-
_ = folder
202202
:ok
203203
end
204204

test/snapshot_delta_test.exs

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -174,6 +174,24 @@ defmodule AshPostgres.SnapshotDeltaTest do
174174
refute Codec.delta?(legacy_json)
175175
end
176176

177+
test "rejects malformed JSON with a parseable error message" do
178+
assert_raise ArgumentError, ~r/Could not parse delta snapshot JSON/, fn ->
179+
Codec.decode_delta("not valid json {[")
180+
end
181+
182+
refute Codec.delta?("not valid json {[")
183+
end
184+
185+
test "rejects JSON that isn't a top-level object" do
186+
assert_raise ArgumentError, ~r/Expected delta snapshot to be a JSON object/, fn ->
187+
Codec.decode_delta("[1, 2, 3]")
188+
end
189+
190+
assert_raise ArgumentError, ~r/Expected delta snapshot to be a JSON object/, fn ->
191+
Codec.decode_delta("\"a string\"")
192+
end
193+
end
194+
177195
test "pseudo-ops (SetBaseFilter, OptOutDropTable) round-trip" do
178196
ops = [
179197
%Operation.SetBaseFilter{

0 commit comments

Comments
 (0)