feat: support Ash query parameters in read actions#11
feat: support Ash query parameters in read actions#11dl-alexandre wants to merge 3 commits intoagentjido:mainfrom
Conversation
…or modules Add comprehensive test coverage for three modules that previously had no tests: - SensorDispatchBridge: Tests for forward/2, forward_many/2, and forward_or_ignore/2 including success cases, error handling for dead runtimes, invalid signals, and batch forwarding with error tracking. - SignalEmitter: Tests for dispatch configuration validation, signal emission with various formats, notification handling, and default signal type/source generation from resource names. - Error: Tests for all Ash error type conversions (Invalid, Forbidden, Framework, Unknown), underlying error extraction, field error mapping, and changeset error extraction. These tests improve overall test coverage and provide safety for future refactoring.
- Add filter, sort, limit, offset parameters to read action schema - Update execute_action to apply query parameters using Ash.Query - Support filter by field values, sort with +/- direction, pagination - Add query parameter helpers: maybe_apply_filter, maybe_apply_sort, maybe_apply_limit, maybe_apply_offset
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bfa24117f8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
lib/ash_jido/generator.ex
Outdated
| :destroy -> | ||
| # Destroy actions just need an id | ||
| [id: [type: :string, required: true, doc: "ID of record to destroy"]] | ||
| [id: [type: :string, required: false, doc: "ID of record to destroy"]] |
There was a problem hiding this comment.
The destroy branch now exposes id as optional in the generated schema, but destroy execution still hard-requires id and raises when it is missing. This regresses the action contract: callers relying on schema validation can submit payloads that pass validation and then fail at runtime with an execution error instead of a deterministic input-validation failure.
Useful? React with 👍 / 👎.
lib/ash_jido/generator.ex
Outdated
| {query_params, filter_params} = | ||
| Map.split(params, ["sort", "limit", "offset", :sort, :limit, :offset]) |
There was a problem hiding this comment.
In the read path, Map.split/2 only removes sort/limit/offset, so filter remains in filter_params and is passed to Ash.Query.for_read/4 as if it were an action argument. For read actions that do not declare a filter argument, this causes unknown-input validation failures before maybe_apply_filter/2 gets a chance to apply the filter, so the new filter feature breaks for normal read actions.
Useful? React with 👍 / 👎.
| Enum.map(all_attributes, fn attr -> | ||
| {attr.name, attribute_to_nimble_options(attr)} | ||
| end) |
There was a problem hiding this comment.
The filter schema reuses attribute_to_nimble_options/1 for each filter key, which carries required: true for non-null attributes. In a nested {:map, filterable_fields} definition, those required flags force callers to provide all required resource attributes whenever filter is present, rejecting common partial filters like filtering by a single field.
Useful? React with 👍 / 👎.
Fixes P1 issues from automated review: 1. Filter passed as argument to for_read (line 150): - Map.split now includes "filter" and :filter in the query_params list - filter is retrieved from query_params instead of params - Prevents filter from being passed as action argument to for_read 2. Destroy id should be required in schema (line 399): - Changed required: false to required: true for destroy action id field - Ensures validation fails early if id is missing
2 participants
Closes #4