Detaching a record via an association calls delete_all, skipping ActiveRecord callbacks, instead of destroy.

[carbonatedcoder]

Describe the bug

Detaching an association record calls delete_all instead of destroy or destroy_all. This skips any ActiveRecord callbacks that may exist on the association join record.

Steps to Reproduce

1. Setup some basic rails models like the example below for a top level model called Facility.
2. Generate the Avo resources and add the has_many associations so that you can attach/detach facilities on the user resource.
3. Detach a facility from a user. The callback before_destroy is not hit within FacilityUser. Rails console logs also show FacilityUser Delete All instead of something like FacilityUser Destroy.

Expected behavior & Actual behavior

Although delete_all is more performant, we would expect destroy or destroy_all to be called upon detaching so that any ActiveRecord callbacks are hit. Is there a reason that deletion was chosen?

Models and resource files

# Facility
has_many :facility_users, dependent: :destroy
has_many :users, through: :facility_users

# User
has_many :facility_users, dependent: :destroy
has_many :facilities, through: :facility_users

# FacilityUser
before_destroy :do_something

belongs_to :facility
belongs_to :user

def do_something
 ...
end

System configuration

Avo version: 3.4.1
Rails version: 7.0.8
Ruby version: 3.2.2

License type:

• Community
• Pro
• Advanced

Are you using Avo monkey patches, overriding views or view components?

• Yes. If so, please post code samples.
• No

Additional context

Impact

• High impact (It makes my app un-usable.)
• Medium impact (I'm annoyed, but I'll live.)
• Low impact (It's really a tiny thing that I could live with.)

Urgency

• High urgency (I can't continue development without it.)
• Medium urgency (I found a workaround, but I'd love to have it fixed.)
• Low urgency (It can wait. I just wanted you to know about it.)

[Paul-Bob]

Hello @carbonatedcoder

Thanks for reporting this issue. We use indeed delete when detaching, using destroy despite the fact that it triggers the callbacks it will actually also destroy the detached record ignoring the :dependent option and we don't want that to happen on most cases. That unwanted behavior is actually the same as clicking directly on destroy.

We're open to suggestions here.

Related PRs:

• fix: don't render detach button when can't detach record #1856
• fix: detach should not destroy attachment #2034

[carbonatedcoder]

@Paul-Bob, thanks for the reply, much appreciated!

I apologize if I misunderstood, but continuing the example from my original post, calling facility_user.destroy should not implicitly destroy attached records (facility and user). The only way that should happen is if a dependent: :destroy is included on those in the model definitions within FacilityUser models.

Given that, it still seems like destroy should be the appropriate method called in this case. With a join record like FacilityUser (linking a user to a facility), detaching a user from a facility is destroying the join record. Destroying the join record should/would not have downstream dependent destroy affects unless specifically defined in its models.

It seems like Avo should defer to what users define in the models to happen on destroy as users may have custom definitions or ActiveRecord callback methods that need to be respected upon detachment. By calling facility_user.delete, those are getting bypassed at the moment.

[Paul-Bob]

calling facility_user.destroy should not implicitly destroy attached records (facility and user).

Agree but it does (not both, only the detached record). Check this: https://edgeapi.rubyonrails.org/classes/ActiveRecord/Associations/CollectionProxy.html#method-i-destroy

Destroys the records supplied and removes them from the collection. This method will always remove record from the database ignoring the :dependent option.

Notice that we don't do the delete on the record itself but on the association https://github.com/avo-hq/avo/blob/main/app/controllers/avo/associations_controller.rb#L82:
@user.facility_users.destroy @facility_user this line would destroy @facility_user without taking consideration of :dependent option.

[carbonatedcoder]

@Paul-Bob, thanks for the additional context.

Is there a suggested way (or perhaps maybe an option that we could specify on the field/resource) to handle the situation outlined in the original post where we need to make sure that ActiveRecord callbacks (or validations), defined in the models, are hit upon a record getting removed via a detachment? I'd imagine this would be a relatively common need for Avo users.

For example, when detaching a User from a Facility we'd want to verify some condition (i.e. before_save) and potentially prevent the destroy/delete if the condition is met. This type of check is defined in the models but right now with the delete call, detaching bypasses the check entirely.

Thanks!

[coffenbacher]

Just restating the scenario here to see if I've got it right:

class User < ApplicationRecord
  has_many :facility_users
  has_many :facilities, through: :facility_users
end

class Facility < ApplicationRecord
  has_many :facility_users
  has_many :users, through: :facility_users
end

class FacilityUser < ApplicationRecord
  belongs_to :user
  belongs_to :facility

  before_destroy :custom_cleanup

  private

  def custom_cleanup
    puts "Running custom cleanup for FacilityUser #{id}"
  end
end

Given this scenario, when you click "Detach" on a facility associated with a user, I think the current behavior (if I'm understanding right) is based off of this code:

@model.send(association_name).delete @attachment_model

If I'm reading this correctly, I think that ends up as user.facilities.destroy facility_user_attachment_model. (where facility_user_attachment_model is an instance of FacilityUser that joins the facility and user records. We can't change it back to destroy because then that goes after the dependent records, which most people would consider a bug.

What do you think about avoiding the association altogether and focusing on destroying the attachment model itself?

@attachment_model.destroy

[Paul-Bob]

@Paul-Bob, thanks for the additional context.

Is there a suggested way (or perhaps maybe an option that we could specify on the field/resource) to handle the situation outlined in the original post where we need to make sure that ActiveRecord callbacks (or validations), defined in the models, are hit upon a record getting removed via a detachment? I'd imagine this would be a relatively common need for Avo users.

For example, when detaching a User from a Facility we'd want to verify some condition (i.e. before_save) and potentially prevent the destroy/delete if the condition is met. This type of check is defined in the models but right now with the delete call, detaching bypasses the check entirely.

Thanks!

@carbonatedcoder just to clarify, we definitely understand the problem and want to add support for detach callbacks, any PR in that direction is welcome!

I did some research and can't find a straight forward method on rails that does the detach, runs the callbacks and respect the dependent option. Are you aware of any way of detaching the association record without deleting it and running the callbacks?

[Paul-Bob]

@coffenbacher you're right, in this situation destroying the join record should do the trick but there are situation without join tables and then we cant just destroy the attachment record.

We need to tailor a solution that fits for all use cases

[coffenbacher]

Ah, yes great point @Paul-Bob, zooming out to other scenarios I'm thinking through this...

      if reflection_class == "HasManyReflection"
        @model.send(association_name).delete @attachment_model
      else
        @model.send("#{association_name}=", nil)
      end

Am I correct in understanding in pseudocode this is basically like this? I'm trying to think through what the scenarios are here...

if (has_many_through_table? || has_many_directly?)
   @model.send(association_name).delete @attachment_model
elsif (belongs_to?)
   @model.send("#{association_name}=", nil)
end

Maybe we can add a third path if there's a way to identify the scenarios in the reflections?

if has_many_through_table?
    @attachment_model.destroy
elsif has_many_directly?
    @model.send(association_name).delete @attachment_model
elsif belongs_to?
    @model.send("#{association_name}=", nil)
end

[Paul-Bob]

Yes @coffenbacher my point is that it will work only on has many through, the solution should be implemented so callbacks get triggered even if is a direct has many. And we don't want to end up with a solution with so many ifs, checking each relation type, if dependent is destroy or nullify etc... I think that's rails responsibility.

[Paul-Bob]

Just to ensure that we're all on the same page:

# Break the association without running callbaks
@record.send(association_name).delete @attachment_record

# Break the association run callbaks and destroy @attachment_record even if dependent is nullify      
@record.send(association_name).destroy @attachment_record 

What we want:

@record.send(association_name).magic_method @attachment_record

Where the magic_method break the association, runs the callbacks and respect :dependent option on the @attachment_record...

Wonder if we should create a rails issue to aboard this question

[coffenbacher]

Edit - woops, sorry, responded out of order here without seeing your latest comment. Your magic_method suggestion would be perfect if you think they'd implement it.

If not, and I don't know how hard things are to get added to Rails (if this is a necessary add), here's what I had written prior about that scenario:

--

Yeah totally agree, I think leveraging the Rails behavior whenever possible is ideal.

In this case, however, I think there's a semantic mismatch between the behavior that makes sense when we click Avo's 'Detach' button and the tools that Rails exposes, at least that I'm aware of. Those docs you shared for the Rails association delete/destroy is the closest I know of, but the behavior on those methods is rough enough that I think it can basically be considered a bug when connected to the concept of 'Detach'. The current implementation is as good as it gets with the Rails basics I think, but as we ran into here, it's not as good as it could be.

It's somewhat surprising that they don't have a better match for 'Detach', but I can kind of see why after thinking about it. In application code, it rarely comes up because it's always so specific what people want to do - destroy a through record, destroy a child record, nullify a belongs_to relationship etc... so the ActiveRecord tools work there.

I think given Avo's position as a foundational tool, having a small chunk of logic on top of Rails supporting the Avo conception of 'Detach' may make sense??

I think the full tree might look like this? Just as an example trying to wrap my head around the intended behavior in the different scenarios, I'm sure there would be a cleaner way to represent it.

if has_many_through_table?
    @attachment_model.destroy
elsif has_many_directly? && detachable?
    @attachment_model.update("#{reverse_association_name}=", nil) # pseudocode
elsif has_many_directly? && !detachable?
    raise 'Cannot detach, must destroy attached record'
elsif belongs_to?
    @model.send("#{association_name}=", nil)
end

[github-actions]

This issue has been marked as stale because there was no activity for the past 15 days.

[krystof-k]

I just bumped into this. I'm using a through model association. It would be great if something like @coffenbacher suggest – calling destroy for such cases to run callbacks.

However, it can be bypassed now by adding the model itself as Avo resource and deleting the association there, not great though.

However I believe this should be mentioned in the docs as it is potentially destructive in the terms of data integrity. I (as everbody?) expected Avo to use exclusively destroys and not deletes.

Also I just discovered that it is even more complex issue in Rails, as there is a difference between Subscription.subscription_items = […] or Subscription.subscription_items.create(…) and SubscriptionItem.create(subscription: …, …). The first runs callbacks on both the Subscription and SubscriptionItem, the second only on SubscriptionItems. Also this is worth noting:

Automatic deletion of join models is direct, no destroy callbacks are triggered.

Probably not creating/deleting the join model directly ever is the correct approach.

[Paul-Bob]

Approach

Add one conditional check (if) around here for through associations like has_many, through: ...

When that conditional check is valid find the join record and apply destroy! on it.

I have some code that isn't working in all cases, but it might provide some inspiration.

# spec/dummy/app/models/team_membership.rb

# == Schema Information
#
# Table name: team_memberships
#
#  id         :bigint           not null, primary key
#  team_id    :bigint           not null
#  user_id    :bigint           not null
#  level      :string
#  created_at :datetime         not null
#  updated_at :datetime         not null
#
class TeamMembership < ApplicationRecord
  belongs_to :team
  belongs_to :user
  before_destroy :raise_here

  def name
    "#{team&.name} - #{user&.name}"
  end

  def raise_here
    raise "here"
  end
end

# app/controllers/avo/associations_controller.rb

    def destroy
      association_name = BaseResource.valid_association_name(@record, @field.for_attribute || params[:related_name])

      if reflection.class.eql?(ActiveRecord::Reflection::ThroughReflection)
        join_table = reflection.join_table

        source_foreign_key = reflection.source_reflection.foreign_key
        through_foreign_key = reflection.through_reflection.foreign_key

        join_record = join_table.camelize.singularize.constantize.find_by(source_foreign_key => @record.id, through_foreign_key => @attachment_record.id)

        join_record.destroy!
      elsif reflection_class == "HasManyReflection"
        @record.send(association_name).delete @attachment_record
      else
        @record.send(:"#{association_name}=", nil)
      end

      respond_to do |format|
        format.html { redirect_to params[:referrer] || resource_view_response_path, notice: t("avo.attachment_class_detached", attachment_class: @attachment_class) }
      end
    end

    def reflection
      @reflection ||= @record._reflections.with_indifferent_access[association_from_params]
    end

@binarygit let's talk about this one tomorrow.