-
Notifications
You must be signed in to change notification settings - Fork 990
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
Fixes #36466 - Allow gathering information on deleted records #9725
Fixes #36466 - Allow gathering information on deleted records #9725
Conversation
Issues: #36466 |
da78b43
to
581fc30
Compare
Sorry for the delay, I finally got around to reading through it properly and taking it for a spin. The only thing I'm not all the fond of is that it pulls in a lot of stuff just in case someone might need it. Yes, I know we don't know up front what people will need, but it is still a lot of stiff. It would be nice if we could only preload associations that are exposed in the object's jail, but I'm not sure if that's feasible. |
One of the first iterations on this PR was combination of filters against all object's associations based on Jail methods. But I couldn't come up with something, so I decided to drop that. But now since I've got something actually working I'll try to add the Jail based filter against this solution. In theory this will work, I just need to find a way :) |
581fc30
to
750fa9a
Compare
@adamruzicka, after looking at this more, I've noticed that some methods are wrapped or renamed (such as P.S. Pushed a refactor only. |
I'm still worried about loading so many things even if it is not needed. /me puts on the wild idea hat It would be a somewhat bigger change than what you're proposing here, but it could be reasonably cheap during runtime. It is quite possible I'm missing something so please bear with me :) How does this sound? |
@adamruzicka, I'm not quite sure if it's doable?.. Surely I'm missing something, so here are my thoughts:
As of now, the only my concern is the step |
Oh, that's probably the part I was missing. Right now we fire an active job for every event (and webhook) that renders the payload and sends it out? My initial line of thinking was that the ActiveSupport::Notification::Event stuff just runs a bunch of callbacks (the subscribers), these callbacks are processed sequentially and synchronously so we could just render everything there, which I must admit is a somewhat poor solution. If we try to parallelize the rendering, then things would indeed get hairy real quick. It would be fairly straightforward if we explicitly backed everything with dynflow as katello does, but that would be a big detour right now. |
@adamruzicka, WDYT, do we leave it this way or try to "backed everything with dynflow"? :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, sometimes wild ideas should remain in the idea stage. Let's get this is as is. Could you please squash?
750fa9a
to
2058f71
Compare
@adamruzicka, done. |
Thank you @ofedoren ! |
Foreman allows subscriptions to the events that occur on ActiveRecord objects (such as an object being created). The problem is that when an object is destroyed, the dependent bonds are being destroyed also with relational links, and can't be referenced afterwards.
Mostly this change can be tested either via foreman_webhooks plugin, or in Rails console :/
The fix is simple: try to load the ActiveModel object with references to dependent objects before the actual deletion. For example, host's interface gets deleted before the host and apparently with the references, so
host.interfaces
on a deleted host will always return an empty array.I've tried to do preloading automatical, so devs don't need to specify what needs to be in
Model.includes
, but just in case I've made this to be hybrid: most of the scopes for preloading are being created automatically, but there is possibility to override or add explicitly scopes in the model itself viaself.preload_scopes; super + [:scope]; end
. And since some dependent models can be created by plugins, there is a plugin DSL entry to add scopes from plugins as well (usage is:extend_preload_scopes(Model, [:scope])
). Automatical scope builder takes in consideration plugins as well.@adamruzicka, could you take a look at this as well?