Skip to content

improvement: Add prepend? opt to hooks and Ash.Subject transaction hooks#2221

Merged
zachdaniel merged 3 commits intoash-project:mainfrom
chazwatkins:improvement/subject-transaction-hooks
Jul 25, 2025
Merged

improvement: Add prepend? opt to hooks and Ash.Subject transaction hooks#2221
zachdaniel merged 3 commits intoash-project:mainfrom
chazwatkins:improvement/subject-transaction-hooks

Conversation

@chazwatkins
Copy link
Copy Markdown
Contributor

@chazwatkins chazwatkins commented Jul 24, 2025

Changes

  • Add support for prepend? option to Changeset, ActionInput, and Query hooks. Except for Ash.Query.after_action which uses @read_action_after_action_hooks_in_order?
  • Add transaction hooks to Ash.Subject since Changeset, ActionInput, and Query now all support them.
  • Update all functions to delegate to the subject type module function
  • Align type specs with subject type module types
  • Update get_argument/3 to only return the default value when :error is returned from fetch_argument/2
  • Add Ash.ActionInput.delete_argument/3
  • Add Ash.Changeset.fetch_attribute/2
  • Add Ash.Changeset.fetch_data/2
  • Add Ash.Changeset.fetch_argument_or_attribute/2
  • Update Ash.Query.fetch_argument/2 to first call Ash.Query.new(query) like other functions to ensure the subject is a query.
  • Remove set_private_argument/3 tests because there is a bug where new/1 doesn't add the arguments, but set_argument/3 is using dot syntax to access the arguments. So, there's no way to use set_private_argument/3 without getting the warning message. I'll submit another PR to fix this"

Contributor checklist

Leave anything that you believe does not apply unchecked.

  • I accept the AI Policy, or AI was not used in the creation of this PR.
  • Bug fixes include regression tests
  • Chores
  • Documentation changes
  • Features include unit/acceptance tests
  • Refactoring
  • Update dependencies

- Adds `prepend?` opt for all hooks except for `Ash.Query.after_action` since it uses `@read_action_after_action_hooks_in_order?`
- Clean up type specs
- Add before/after,around transaction hooks to Ash.Subject
@chazwatkins
Copy link
Copy Markdown
Contributor Author

I removed the default implementations and delegate to the subject type modules.

@chazwatkins chazwatkins changed the title improvement: Add prepend? opt to hooks and Ash.Subject transaction hooks DRAFT: improvement: Add prepend? opt to hooks and Ash.Subject transaction hooks Jul 24, 2025
@chazwatkins chazwatkins force-pushed the improvement/subject-transaction-hooks branch from 64e3a56 to 4ae7544 Compare July 24, 2025 20:13
- Update all functions to delegate to the subject type module function
- Align type specs with subject type module types
- Update `get_argument/3` to only return the default value when `:error` is returned from `fetch_argument/2`
- Add `Ash.ActionInput.delete_argument/3`
- Add `Ash.Changeset.fetch_attribute/2`
- Add `Ash.Changeset.fetch_data/2`
- Add `Ash.Changeset.fetch_argument_or_attribute/2`
- Update `Ash.Query.fetch_argument/2` to first call `Ash.Query.new(query)` like other functions to ensure the subject is a query.
- Remove `set_private_argument/3` tests because there is a bug where `new/1` doesn't add the `arguments`, but `set_argument/3` is using dot syntax to access the arguments.  So, there's no way to use `set_private_argument/3` without getting the warning message.  I'll submit another PR to fix this"
-
@chazwatkins chazwatkins force-pushed the improvement/subject-transaction-hooks branch from 4ae7544 to 08bf489 Compare July 24, 2025 20:31
@chazwatkins chazwatkins changed the title DRAFT: improvement: Add prepend? opt to hooks and Ash.Subject transaction hooks improvement: Add prepend? opt to hooks and Ash.Subject transaction hooks Jul 24, 2025
@chazwatkins
Copy link
Copy Markdown
Contributor Author

chazwatkins commented Jul 24, 2025

Ok, It should be good to go now. Let me know if you need anymore changes.

Few items for future PRs:

  • Fix issue where you can't call set_argument or set_private_argument for a Changeset or ActionInput when you only do new/1. This isn't intended behavior as the error message you receive when using Ash.Changeset.for_create(MyResource, :create) |> Ash.Changeset.set_private_argument(...) is that you should do Ash.Changeset.new(MyResource) |> Ash.Changeset.set_private_argument(...), but you can't because action.arguments key doesn't exist yet. This should be an easy fix.
  • Add Ash.Query.set_private_argument
  • Determine the functions that are the same between subjects and pull them out into another module
  • Break-up the subject modules into smaller modules to reduce compilation time.

@chazwatkins
Copy link
Copy Markdown
Contributor Author

@zachdaniel Thanks for the time and patience reviewing the PRs. I've been off work this week and wanted to contribute a bit.

@zachdaniel
Copy link
Copy Markdown
Contributor

Determine the functions that are the same between subjects and pull them out into another module

For this one, I think it will be better to leave them in their respective places. We'll never have another subject type, and it is possible their implementations will intentionally drift. Having the unified Ash.Subject implementation should be sufficient to simplify callers. Open to doing it differently later but for now I think the separation is best.

@zachdaniel zachdaniel merged commit b107cb0 into ash-project:main Jul 25, 2025
43 checks passed
@zachdaniel
Copy link
Copy Markdown
Contributor

🚀 Thank you for your contribution! 🚀

One small note: we want to be careful to avoid scope creep on individual PRs since it can make it a bit tougher to review.

Thanks for the time and patience reviewing the PRs. I've been off work this week and wanted to contribute a bit.

Not at all! these are great changes, so thank you!

@chazwatkins
Copy link
Copy Markdown
Contributor Author

Determine the functions that are the same between subjects and pull them out into another module

For this one, I think it will be better to leave them in their respective places. We'll never have another subject type, and it is possible their implementations will intentionally drift. Having the unified Ash.Subject implementation should be sufficient to simplify callers. Open to doing it differently later but for now I think the separation is best.

Fair enough.

For the scope creep, I definitely realized this one was getting a pretty big. I'll keep them smaller in the future to reduce the risk of y'all missing something.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants