feat: add function and module-based topic generation to PubSub #2139
Conversation
This addresses GitHub issue #2081
barnabasJ
force-pushed
the
feature/relationship-based-pubsub-topics
branch
from
03bbe74 to
63ff8dc
Compare
|
So, I think there is one particular thing we should do before we support this, to solve for bulk actions, which is that we'd add a @callback required_loads :: %{term() => load_statement()}The key of that map would be to disambiguate for the notifier if multiple separate notifications trigger on that action. So in the read action, we'd add calculations like Writing this all up has pointed out how complex the problem space is 😆. However, it is the pattern we'd need for notifiers to automatically load their dependencies. We can potentially side-line this for now, but I worry about having this pattern in place in a way that plays so poorly with bulk actions. |
…tion Add optional required_loads/0 callback to TopicGenerator behavior to enable automatic relationship preloading and eliminate N+1 queries in bulk operations. - Add required_loads/0 callback to TopicGenerator behavior - Create Helpers module to extract loads and add them to changesets - Integrate load injection into create and bulk create actions - Add comprehensive tests demonstrating N+1 elimination - Maintain full backwards compatibility with existing topic generators This addresses the performance issue where bulk operations with relationship- dependent topic generation would trigger individual loads per notification. Topic generators can now declare their load requirements and the system automatically preloads the necessary relationships. Example usage: ```elixir defmodule MyTopicGenerator do use Ash.Notifier.PubSub.TopicGenerator @impl true def required_loads, do: [author: :followers] @impl true def generate_topics(notification, _opts) do # Relationships are already loaded followers = notification.data.author.followers Enum.map(followers, &"feed:#{&1.id}") end end ```
| | [ | ||
| String.t() | ||
| ] | ||
| @callback required_loads() :: term() |
There was a problem hiding this comment.
This should be a callback on Ash.Notifier I think.
There was a problem hiding this comment.
Ah, nvm. This makes sense here. But my other comment about having a callback on the notifier itself to drive this is the thing.
| @@ -21,6 +21,7 @@ defmodule Ash.Actions.Create do | |||
| else | |||
| {changeset, opts} = Ash.Actions.Helpers.set_context_and_get_opts(domain, changeset, opts) | |||
| changeset = Helpers.apply_opts_load(changeset, opts) | |||
| changeset = Ash.Notifier.PubSub.Helpers.add_topic_generator_loads(changeset) | |||
There was a problem hiding this comment.
We shouldn't have behavior for a specific notifier here, it should be calling a callback on all of the notifiers for this resource and action.
| Ash.Changeset.load(changeset, load_list) | ||
|
|
||
| %Ash.Query{} = query -> | ||
| Ash.Query.load(query, load_list) |
There was a problem hiding this comment.
We can't just load the things here, because the caller of the action could request the same load key but with different parameters which can lead to non deterministic behavior. This is what I was getting at with needing to add a custom calculation.
This addresses GitHub issue #2081
Contributor checklist
Leave anything that you believe does not apply unchecked.