fix: added bulk_action_ref for changeset lookups #2411
Conversation
DGollings
force-pushed
the
main
branch
3 times, most recently
from
ce86c98 to
fa4f2cb
Compare
|
I believe the last thing we want to do is make sure that this key is cleared from any resulting record's metadata. |
done plus test |
wip formatting and dialyzer wip removed find_changeset_by_id_fallback linting
|
We're close! Thank you so much for bearing with me on this one. |
DGollings
force-pushed
the
main
branch
3 times, most recently
from
e0018e2 to
c2136df
Compare
Appreciate (and understand) the thorough review :) Updated and also opened ash-project/ash_postgres#645 and ash-project/ash_sqlite#189 ( It passes my repro test, please review |
lib/ash/actions/update/bulk.ex
Outdated
| |> Ash.Changeset.prepare_changeset_for_action(action, opts) | ||
| |> Ash.Changeset.set_private_arguments_for_action(opts[:private_arguments] || %{}) | ||
| |> Ash.Changeset.put_context(context_key, %{index: index}) | ||
| |> Ash.Changeset.put_context(:bulk_update, %{index: index, ref: ref}) |
There was a problem hiding this comment.
We need to use the context_key variable here and make sure we aren't hardcoding it.
There was a problem hiding this comment.
makes sense, changed to
- |> Ash.Changeset.put_context(:bulk_update, %{index: index, ref: ref})
+ |> Ash.Changeset.put_context(context_key, %{index: index, ref: ref})
lib/ash/actions/update/bulk.ex
Outdated
| :destroy -> | ||
| {:bulk_destroy, :bulk_destroy_index} | ||
| end | ||
| context_key = :bulk_update |
There was a problem hiding this comment.
We still need to switch on action.type so that this uses the action type for these three values. For destroys the context key, metadata key, and ref_metadata_key need to be based on the action type.
There was a problem hiding this comment.
this one I was less sure about, but I think you mean:
- context_key = :bulk_update
- metadata_key = :bulk_update_index
- ref_metadata_key = :bulk_action_ref
+ {context_key, metadata_key, ref_metadata_key} =
+ case action.type do
+ :destroy ->
+ {:bulk_destroy, :bulk_destroy_index, :bulk_action_ref}
+
+ _ ->
+ {:bulk_update, :bulk_update_index, :bulk_action_ref}
+ endCorrect?
removed unnecessary find_changeset_by_index
|
Updated @zachdaniel , all tests (still) pass (although now I'm wondering if we need an extra test or two?) |
lib/ash/actions/update/bulk.ex
Outdated
| if ref_metadata_key do | ||
| ref_key = result.__metadata__[ref_metadata_key] | ||
|
|
||
| changesets_by_ref |
There was a problem hiding this comment.
we shouldn't need this check.
|
Feel bad that I keep doing a drive by on this PR and finding small things. With the existing data layers currently returning indexes, I'm pretty sure this needs to handle that properly. i.e if we get back a record with a bulk index but not a ref, we should look up the ref by index in a map of index -> ref. Someday down the line we can undo that call. This could be tested at least in the interim by having the ETS data layer only return an index and seeing how it breaks. |
|
And here I am impressed by your restraint, not simply making the necessary changes yourself and merging it in :) I intentionally had zero backwards compatibility, but makes sense to have some, added in |
DGollings
force-pushed
the
main
branch
2 times, most recently
from
dd93e8c to
e437c5c
Compare
…ly index metadata
|
🚀 Thank you for your contribution! 🚀 |
|
thought we'd never get this merged 😅 |
2 participants
Contributor checklist
Leave anything that you believe does not apply unchecked.
Otherwise as discussed @zachdaniel (I presume)