Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Fix issues in Ash.Generator.generate_many/2 #1703

Merged
merged 5 commits into from
Jan 9, 2025

Conversation

sevenseacat
Copy link
Contributor

@sevenseacat sevenseacat commented Jan 9, 2025

When testing out generate_many/2 I found there were a couple of issues with it:

  • It would error if after_action: nil was passed to bulk_create
  • It would error if the full action struct was passed to bulk_create (instead of just the action name)
  • It would error if the action passed to bulk_create required authorization (no actor being passed in)
  • It would raise warnings if the action used in bulk_create generated any notifications (tbh I'm not sure why the action I'm testing with is generating notifications, but that's a different story)
  • It would ignore partial success results, leading to random test failures due to intermittent erroneous generator data

With these changes I can successfully call generate_many like generate_many(artist(), 2) and get two Artist records back.

…rating many records

This prevents warnings in tests when using `generate_many` -

`[warning] Missed <x> notifications in action <action>`
…n `generate_many` wasn't a complete success

A partial success would mean less records were inserted than were requested,
definitely leading to buggy/flaky tests
@@ -505,12 +505,27 @@ defmodule Ash.Generator do
fn _changeset, record ->
{:ok, after_action.(record)}
end
else
# Do nothing
fn _changeset, record -> {:ok, record} end
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll merge but I'm going to figure out how to make this not necessary, as adding hooks that aren't needed can prevent internal optimizations etc.

@zachdaniel zachdaniel merged commit 0d49b61 into ash-project:main Jan 9, 2025
36 checks passed
@zachdaniel
Copy link
Contributor

🚀 Thank you for your contribution! 🚀

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