feat: add extends option for actions#2646
Conversation
There was a problem hiding this comment.
Pull request overview
Adds an extends option to Ash resource actions so an action can inherit configuration from a base action, with list-like configuration concatenated (base first) and scalar configuration inherited unless overridden (excluding primary?).
Changes:
- Introduces
Ash.Resource.Transformers.ExtendActionsto merge action configuration whenextendsis set. - Adds
extendsto all action types (create/read/update/destroy/generic action) and exposes it in shared DSL options. - Adds unit tests and updates DSL documentation to describe the new option (and expands preparation
ondoc types).
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| test/resource/actions/extend_actions_test.exs | Adds tests covering basic concatenation/inheritance and some error cases for extends. |
| lib/ash/resource/transformers/extend_actions.ex | Implements the extends merge logic and transformer ordering. |
| lib/ash/resource/dsl.ex | Registers the new transformer in the resource DSL transformer pipeline. |
| lib/ash/resource/actions/create.ex | Adds extends to create action struct. |
| lib/ash/resource/actions/read.ex | Adds extends to read action struct. |
| lib/ash/resource/actions/update.ex | Adds extends to update action struct. |
| lib/ash/resource/actions/destroy.ex | Adds extends to destroy action struct. |
| lib/ash/resource/actions/action/action.ex | Adds extends to generic action struct. |
| lib/ash/resource/actions/shared_options.ex | Adds extends as a shared DSL option for actions. |
| documentation/dsls/DSL-Ash.Resource.md | Documents the new extends option across action types (and updates preparation on doc types). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| defstruct arguments: [], | ||
| description: nil, | ||
| extends: nil, | ||
| filter: nil, |
There was a problem hiding this comment.
extends was added to the struct, but the @type t spec below doesn’t include the new extends field. Please update the type spec to match the struct fields.
| :error_handler, | ||
| :multitenancy, | ||
| extends: nil, | ||
| accept: nil, | ||
| require_attributes: [], | ||
| allow_nil_input: [], |
There was a problem hiding this comment.
extends was added to the struct, but the @type t spec below doesn’t include the new extends field. Please update the type spec to match the struct fields.
| :description, | ||
| :returns, | ||
| :run, | ||
| extends: nil, | ||
| constraints: [], | ||
| touches_resources: [], | ||
| skip_unknown_inputs: [], |
There was a problem hiding this comment.
extends was added to the struct, but the @type t spec below doesn’t include the new extends field. Please update the type spec to match the struct fields.
| describe "error cases" do | ||
| test "raises when base action does not exist" do | ||
| assert_raise Spark.Error.DslError, ~r/no action named `nonexistent` exists/, fn -> | ||
| defresource MissingBase do | ||
| read :bad do | ||
| extends :nonexistent | ||
| end | ||
| end | ||
| end | ||
| end | ||
|
|
||
| test "raises when action types don't match" do | ||
| assert_raise Spark.Error.DslError, ~r/must be the same type/, fn -> | ||
| defresource TypeMismatch do | ||
| create :base do | ||
| accept [] | ||
| end | ||
|
|
||
| read :bad do | ||
| extends :base | ||
| end | ||
| end | ||
| end | ||
| end | ||
| end |
There was a problem hiding this comment.
Test coverage currently doesn’t exercise multi-level extends (A extends B extends C), self-extension, or cyclic extension. Adding tests for these cases would help ensure deterministic behavior and that invalid graphs raise clear DslErrors.
| ~w(arguments changes metadata reject require_attributes allow_nil_input notifiers touches_resources skip_unknown_inputs)a, | ||
| read: ~w(arguments preparations filters metadata touches_resources skip_unknown_inputs)a, | ||
| update: | ||
| ~w(arguments changes metadata reject require_attributes allow_nil_input notifiers touches_resources skip_unknown_inputs atomics)a, | ||
| destroy: | ||
| ~w(arguments changes metadata reject require_attributes allow_nil_input notifiers touches_resources skip_unknown_inputs)a, |
There was a problem hiding this comment.
action_select is a list option for create/update/destroy actions (see Ash.Resource.Actions.SharedOptions), but it’s not included in @list_fields, so it will be treated as a scalar (override/inherit) rather than concatenated. Either add it to the appropriate list fields or clarify the docs that some list-typed options are not concatenated.
| ~w(arguments changes metadata reject require_attributes allow_nil_input notifiers touches_resources skip_unknown_inputs)a, | |
| read: ~w(arguments preparations filters metadata touches_resources skip_unknown_inputs)a, | |
| update: | |
| ~w(arguments changes metadata reject require_attributes allow_nil_input notifiers touches_resources skip_unknown_inputs atomics)a, | |
| destroy: | |
| ~w(arguments changes metadata reject require_attributes allow_nil_input notifiers touches_resources skip_unknown_inputs)a, | |
| ~w(arguments changes metadata reject require_attributes allow_nil_input notifiers touches_resources skip_unknown_inputs action_select)a, | |
| read: ~w(arguments preparations filters metadata touches_resources skip_unknown_inputs)a, | |
| update: | |
| ~w(arguments changes metadata reject require_attributes allow_nil_input notifiers touches_resources skip_unknown_inputs atomics action_select)a, | |
| destroy: | |
| ~w(arguments changes metadata reject require_attributes allow_nil_input notifiers touches_resources skip_unknown_inputs action_select)a, |
| :error_handler, | ||
| :multitenancy, | ||
| extends: nil, | ||
| accept: nil, | ||
| require_attributes: [], | ||
| allow_nil_input: [], |
There was a problem hiding this comment.
extends was added to the struct, but the @type t spec below doesn’t include the new extends field. Please update the type spec to match the struct fields.
| :error_handler, | ||
| :multitenancy, | ||
| extends: nil, | ||
| manual: nil, | ||
| require_atomic?: Application.compile_env(:ash, :require_atomic_by_default?, true), | ||
| skip_unknown_inputs: [], |
There was a problem hiding this comment.
extends was added to the struct, but the @type t spec below doesn’t include the new extends field. Please update the type spec to match the struct fields.
| all_actions = Transformer.get_entities(dsl_state, [:actions]) | ||
| base = Enum.find(all_actions, &(&1.name == action.extends)) | ||
|
|
There was a problem hiding this comment.
base = Enum.find(all_actions, &(&1.name == action.extends)) can resolve to the action itself when extends points to its own name (or can participate in a cycle across multiple actions). That will silently duplicate list fields and produce unpredictable merges. Please add an explicit guard that disallows self-extension and detect/raise on cycles (e.g., by tracking a visited set while resolving bases).
| dsl_state | ||
| |> Transformer.get_entities([:actions]) | ||
| |> Enum.filter(& &1.extends) | ||
| |> Enum.reduce_while({:ok, dsl_state}, fn action, {:ok, dsl_state} -> | ||
| all_actions = Transformer.get_entities(dsl_state, [:actions]) | ||
| base = Enum.find(all_actions, &(&1.name == action.extends)) | ||
|
|
||
| cond do | ||
| is_nil(base) -> | ||
| {:halt, | ||
| {:error, | ||
| DslError.exception( | ||
| module: Transformer.get_persisted(dsl_state, :module), | ||
| path: [:actions, action.type], | ||
| message: | ||
| "Action `#{action.name}` extends `#{action.extends}`, but no action named `#{action.extends}` exists." | ||
| )}} | ||
|
|
||
| base.type != action.type -> | ||
| {:halt, | ||
| {:error, | ||
| DslError.exception( | ||
| module: Transformer.get_persisted(dsl_state, :module), | ||
| path: [:actions, action.type], | ||
| message: | ||
| "Action `#{action.name}` (#{action.type}) extends `#{action.extends}` (#{base.type}), but they must be the same type." | ||
| )}} | ||
|
|
||
| true -> | ||
| merged = merge_action(action, base) | ||
|
|
||
| new_state = | ||
| Transformer.replace_entity( | ||
| dsl_state, | ||
| [:actions], | ||
| merged, | ||
| &(&1.name == action.name && &1.type == action.type) | ||
| ) | ||
|
|
||
| {:cont, {:ok, new_state}} | ||
| end | ||
| end) |
There was a problem hiding this comment.
Extends resolution is order-dependent for multi-level inheritance: if action A extends B and B extends C, and A is processed before B, A will merge against the unresolved B and won’t inherit C’s config. Consider resolving the full extends chain recursively/topologically (with cycle detection) before merging, so results don’t depend on action declaration order.
| dsl_state | |
| |> Transformer.get_entities([:actions]) | |
| |> Enum.filter(& &1.extends) | |
| |> Enum.reduce_while({:ok, dsl_state}, fn action, {:ok, dsl_state} -> | |
| all_actions = Transformer.get_entities(dsl_state, [:actions]) | |
| base = Enum.find(all_actions, &(&1.name == action.extends)) | |
| cond do | |
| is_nil(base) -> | |
| {:halt, | |
| {:error, | |
| DslError.exception( | |
| module: Transformer.get_persisted(dsl_state, :module), | |
| path: [:actions, action.type], | |
| message: | |
| "Action `#{action.name}` extends `#{action.extends}`, but no action named `#{action.extends}` exists." | |
| )}} | |
| base.type != action.type -> | |
| {:halt, | |
| {:error, | |
| DslError.exception( | |
| module: Transformer.get_persisted(dsl_state, :module), | |
| path: [:actions, action.type], | |
| message: | |
| "Action `#{action.name}` (#{action.type}) extends `#{action.extends}` (#{base.type}), but they must be the same type." | |
| )}} | |
| true -> | |
| merged = merge_action(action, base) | |
| new_state = | |
| Transformer.replace_entity( | |
| dsl_state, | |
| [:actions], | |
| merged, | |
| &(&1.name == action.name && &1.type == action.type) | |
| ) | |
| {:cont, {:ok, new_state}} | |
| end | |
| end) | |
| actions = Transformer.get_entities(dsl_state, [:actions]) | |
| actions_by_key = | |
| Map.new(actions, fn action -> | |
| {{action.name, action.type}, action} | |
| end) | |
| actions | |
| |> Enum.filter(& &1.extends) | |
| |> Enum.reduce_while({:ok, dsl_state, %{}}, fn action, {:ok, dsl_state, resolved} -> | |
| case resolve_action(action, dsl_state, actions_by_key, resolved, MapSet.new()) do | |
| {:ok, new_state, new_resolved} -> | |
| {:cont, {:ok, new_state, new_resolved}} | |
| {:error, _} = error -> | |
| {:halt, error} | |
| end | |
| end) | |
| |> case do | |
| {:ok, dsl_state, _resolved} -> | |
| {:ok, dsl_state} | |
| {:error, _} = error -> | |
| error | |
| end | |
| end | |
| defp resolve_action(action, dsl_state, actions_by_key, resolved, visiting) do | |
| key = {action.name, action.type} | |
| cond do | |
| Map.has_key?(resolved, key) -> | |
| {:ok, dsl_state, resolved} | |
| MapSet.member?(visiting, key) -> | |
| {:error, | |
| DslError.exception( | |
| module: Transformer.get_persisted(dsl_state, :module), | |
| path: [:actions, action.type], | |
| message: | |
| "Cyclic action inheritance detected starting at `#{action.name}` for type `#{action.type}`." | |
| )} | |
| is_nil(action.extends) -> | |
| {:ok, dsl_state, Map.put(resolved, key, true)} | |
| true -> | |
| base_key = {action.extends, action.type} | |
| base = Map.get(actions_by_key, base_key) | |
| cond do | |
| is_nil(base) -> | |
| {:error, | |
| DslError.exception( | |
| module: Transformer.get_persisted(dsl_state, :module), | |
| path: [:actions, action.type], | |
| message: | |
| "Action `#{action.name}` extends `#{action.extends}`, but no action named `#{action.extends}` exists." | |
| )} | |
| base.type != action.type -> | |
| {:error, | |
| DslError.exception( | |
| module: Transformer.get_persisted(dsl_state, :module), | |
| path: [:actions, action.type], | |
| message: | |
| "Action `#{action.name}` (#{action.type}) extends `#{action.extends}` (#{base.type}), but they must be the same type." | |
| )} | |
| true -> | |
| visiting = MapSet.put(visiting, key) | |
| case resolve_action(base, dsl_state, actions_by_key, resolved, visiting) do | |
| {:ok, dsl_state, resolved} -> | |
| all_actions = Transformer.get_entities(dsl_state, [:actions]) | |
| resolved_base = | |
| Enum.find(all_actions, fn candidate -> | |
| candidate.name == action.extends && candidate.type == action.type | |
| end) | |
| merged = merge_action(action, resolved_base) | |
| new_state = | |
| Transformer.replace_entity( | |
| dsl_state, | |
| [:actions], | |
| merged, | |
| &(&1.name == action.name && &1.type == action.type) | |
| ) | |
| {:ok, new_state, Map.put(resolved, key, true)} | |
| {:error, _} = error -> | |
| error | |
| end | |
| end | |
| end |
| DslError.exception( | ||
| module: Transformer.get_persisted(dsl_state, :module), | ||
| path: [:actions, action.type], | ||
| message: | ||
| "Action `#{action.name}` extends `#{action.extends}`, but no action named `#{action.extends}` exists." | ||
| )}} | ||
|
|
||
| base.type != action.type -> | ||
| {:halt, | ||
| {:error, | ||
| DslError.exception( | ||
| module: Transformer.get_persisted(dsl_state, :module), | ||
| path: [:actions, action.type], | ||
| message: | ||
| "Action `#{action.name}` (#{action.type}) extends `#{action.extends}` (#{base.type}), but they must be the same type." | ||
| )}} |
There was a problem hiding this comment.
The raised DslError here doesn’t include location, and the path is only [:actions, action.type], which makes it hard to pinpoint the offending extends option. Consider using the action entity’s :extends property anno for location and a more specific path like [:actions, action.name, :extends] (consistent with other action-related transformers).
|
Hmm...I don't think I'd like to take this approach. I will close the original issue. I think we should do this instead: #1625 |
Contributor checklist
Leave anything that you believe does not apply unchecked.
Summary
extendsoption to all action types (create, read, update, destroy, generic action)primary?is never inherited to avoid conflictsCloses #966
Example