update_descendants_with_new_ancestry in after_update

[kshnurov]

This simple code will not work as expected when you update a parent's slug - descendants won't have the slug changed.

class Model
  before_save :update_slug

  def update_slug
    self.slug = [parent.slug, self.slug].join('/')
  end
end

That is because update_descendants_with_new_ancestry is run with before_save and the parent method loads an unchanged version from the database. This PR moves update_descendants_with_new_ancestry to after_update.

There's no tests for such case right now, but I've tested it with my code and it works. Testing with update_strategy = :sql would be appreciated, though the change is pretty simple and should just work.

[kshnurov]

There's a new descendants_by_ancestry(ancestry) scope, should we add it to the docs?

[kshnurov]

@kbrock have you had a chance to look into this?

[kbrock]

@kshnurov thanks for the ping.

When I modify values in a model like a field (slug), I tend to do those in before validation. So regardless of the before_* orders, the record is complete.

If you move the code into a before_validates, does that fix it.

Also, is it possible that update_slug brings in name or tag rather than slug:

  def update_slug
    self.slug = [parent.slug, self.name].join('/')
  end

Though, having the parent in there does make it a little more expensive.

I need to document a good way to build a field that is composed of the ancestry name values rather than id values.

[kshnurov]

Hi @kbrock

When I modify values in a model like a field (slug), I tend to do those in before validation. So regardless of the before_* orders, the record is complete.

Me too, this is just an example, doesn't matter which callback you use, I use friendly_id gem, which uses both. The issue here is the parent object.

Also, is it possible that update_slug brings in name or tag rather than slug:

This is just a mockup example, a typical use would be some kind of title or name, but once again - it's not the issue here. The issue is parent.slug being incorrect, since the change in parent is not saved yet, and we load an unchanged version here.

[kbrock]

Hello @kshnurov

Thank you for the clarification. it sounds like we are on the same page.

There is definitely an issue when there are multiple version of parent loaded into memory. a test was created for similar reasons. This update case is a nice addition.

I'll take another look at this. This does look like it is just a conversion from one hook to another.

I need to document a good way to build a field that is composed of the ancestry name values rather than id values.

For this case, introducing a database only update tends to works better, but that will not update in memory nodes, so that has the potential to introduce edge cases.

/ASIDE: Wonder if this pattern is common enough to warrant introduction into the gem itself.

[kshnurov]

I'll take another look at this. This does look like it is just a conversion from one hook to another.

Yes, since we should update descendants after the record is saved, not before. I've added a simple test, it fails on the main branch and passes with this PR.

Also, there's a reason why I used before_save: before_validation is not triggered by update_attribute, which is used to update descendants. Probably should add a note to the docs about that.

For this case, introducing a database only update tends to works better, but that will not update in memory nodes, so that has the potential to introduce edge cases.

Database only update obviously won't work with this case or any other callbacks, like updating data in your search DB, and you'll end up with inconsistent data. I think they're evil and shouldn't exist, because they will break pretty much everything, including a simple cache (since updated_at isn't changed). I don't think saving a few DB transactions when changing ancestry (which sounds like a rare case) is worth the risk and extra caution required, so I'd remove them completely.

/ASIDE: Wonder if this pattern is common enough to warrant introduction into the gem itself.

Using parent values is inevitable if you have friendly urls like /a/b/c/d, which is quite common.

[kshnurov]

@kbrock as you can see tests fail with PG, since DB-only updates don't trigger callbacks. Skipped that test for sql strategy.

[kshnurov]

@kbrock is there anything holding you from merging this?

[kbrock]

@kshnurov I appreciate where you are coming from.

I'm concerned that after update will need to save the child records twice instead of once.

Is your thought that we will very rarely update the ancestry, so having that operation a double save is not bad?

Or I guess it is possible that the primary record will be changed, maybe double saved, then the followup records will only get only updated once?

[kshnurov]

@kbrock why will it save twice? It won't, child records will be updated only once - when parent ancestry is changed. Saving/touching parent without changing ancestry won't trigger anything.

If someone adds their own callback to update children on parent change, it's up to them to avoid running callbacks twice in such rare case. Anyway there won't be any double-saving to DB since all changes will happen on the first run, second run won't change the record.

[kshnurov]

@kbrock 1,5 months and this simple PR didn't move. Should we consider this gem unmaintained?

[kshnurov]

@kbrock ?

[kbrock]

Sorry for the delay.

Updating the callback order is potentially a breaking change for the hundred(s?) of projects that depend upon this gem.

I am looking into fixing this for sql based updates