Swallowing ActiveRecord::RecordNotDestroyed exceptions

[ybakos]

Hi and thanks for all things acts_as_tenant. I am trying it out on an existing legacy project, and I seem to be observing behavior that is somehow interfering with the default ActiveRecord exception throwing. It appears like acts_as_tenant is swallowing an ActiveRecord::RecordNotDestroyed exception.

Here's the model for my tenant: a School.

class School < ApplicationRecord
  has_many :activities, dependent: :destroy
  has_many :registrations, dependent: :destroy
  has_many :teachers, dependent: :destroy
  has_many :users, dependent: :destroy
end

In the PostgreSQL schema, I also have FK constraints on the foreign keys for each of the above associations.

In addition, one of those associated models, Teacher, has a dependent: :restrict_with_error on its other associations.

class Teacher < ApplicationRecord
  acts_as_tenant(:school)
  has_many :students, class_name: 'User', dependent: :restrict_with_error
  has_many :registrations, dependent: :restrict_with_error

In short, I can destroy a School and anything associated with it, as long as an associated Teacher does not have students or registrations associated with it.

Consider the destroy method in my SchoolsController:

def destroy
  @school = School.find(params[:id])
  @school.destroy!
  redirect_to sys_admin_schools_path, notice: "School deleted."
rescue ActiveRecord::RecordNotDestroyed => error
  redirect_to sys_admin_school_path(@school), alert: error.record.errors.full_messages
end

Before I introduced acts_as_tenant, destroying a School that is not destroyable, due to some associated restrictions/constraints, would raise an ActiveRecord::RecordNotDestroyed exception.

After I introduced acts_as_tenant, this method does not raise the ActiveRecord::RecordNotDestroyed exception. Instead, the underlying ActiveRecord::InvalidForeignKey exception gets raised. I would expect to see this exception be raised due to a deletion that violates the FK constraints - this would occur if I did not declare the dependent options on the associations.

If I change the destroy action to ignore the tenant:

def destroy
    ActsAsTenant.without_tenant do
      @school = School.find(params[:id])
      @school.destroy!
    end
    redirect_to sys_admin_schools_path, notice: "School deleted."
rescue ActiveRecord::RecordNotDestroyed => error
  redirect_to sys_admin_school_path(@school), alert: error.record.errors.full_messages
end

Then I indeed see the ActiveRecord::RecordNotDestroyed exception.

I wonder - might acts_as_tenant be somehow swallowing the RecordNotDestroyed exception, or somehow interfering with my dependent options? This is all under test, and I believe that in my test I am indeed using fixtures that include only tenant-specific models. (But I am triple-checking this now.)

[ybakos]

Perhaps acts_as_tenant has a before_destroy callback that is not handling cases of throwing :abort ?

https://api.rubyonrails.org/classes/ActiveRecord/Persistence.html#method-i-destroy-21

[ybakos]

Some more information. I replicated this error surfaced by a system test in a unit test. Sooo...

1. Although my controller is finding the School, (which is the tenant model and therefore itself does not declare acts_as_tenant nor subject to the constraints of the current_tenant)
2. destroying any School, regardless of the current tenant, works fine, as long as no dependent: constraints are encountered (destroying a School and its Teachers works as long as associated Teachers do not have associated Students) [1]
3. ActiveRecord generates queries to check for dependent: :restrict_with_error violations, and these queries are subject to the influence of the current_tenant.
4. Those queries return no associated Teachers and/or associated Students for those Teachers
5. So the AR check for :restrict_with_error doesn't raise an error, but
6. Postgres raises an error due to the abandoned foreign key, violating the fk constraint

Hence, I see the InvalidForeignKey exception and not the originally anticipated RecordNotDestroyed exception.

By turning off the tenant:

ActsAsTenant.without_tenant { @school.destroy! }

I indeed see the RecordNotDestroyed exception again.

In addition, [1] I likely need to have the tenant turned off anyway in order for my cascading deletes to occur! I don't yet have this situation tested but am going to add it to verify what I am observing here.

Thanks for being my duck! Will close once I verify.

[ybakos]

Hmmm... interesting. Per [1] marked in the above comment, I am actually seeing some unexpected behavior, so I will describe it here, see if others chime in, and ya'll can decide to close this or open a more accurate issue.

With acts_as_tenant, a destroy on the tenant model will cause a cascading delete on associated models, if they are specified with dependent: :destroy, and the underlying queries for these associated records will use the id of the object upon which the destroy is invoked. But the underlying queries for the secondary associations will use the current tenant id instead!

Consider a School that has_many Activities:

class School < ApplicationRecord
  has_many :activities, dependent: :destroy

And an Activity that has_many Registrations:

class Activity < ApplicationRecord
  has_many :registrations, dependent: :destroy

Here is a test, don't worry about the context/purpose, just notice that it sets a tenant, then invokes a delete on a different object:

test 'cannot be destroyed when it has teachers that have students/registrations' do
    ActsAsTenant.with_tenant(schools(:second)) do
      school = schools(:first)
      refute school.destroy
      assert_raises(ActiveRecord::RecordNotDestroyed) { school.destroy! }
    end
  end

Notice the SQL in the test.log output:

Notice how the Activity Load query uses the id of schools(:second), and not the id of the current tenant.

Notice how the Registration Load query uses the id of the current tenant, and not the id of schools(:second).

My question is: have I uncovered a bug in acts_as_tenant? (If so, please create a new issue and close this one.)

If this is expected behavior, can anyone explain why? (Thank you.)

[ybakos]

What I am suspicious of, if this is a bug, is that acts_as_tenant is either not properly scoping SELECTs for first-degree associated records upon delete.

Or that acts_as_tenant should not scope SELECTS for second-degree associated records upon delete.

[excid3]

Is Registration also an acts_as_tenant model?

[ybakos]

Yep.

class Registration < ApplicationRecord
  belongs_to :activity, counter_cache: true
  belongs_to :creator, class_name: 'User'
  acts_as_tenant(:school)
  belongs_to :student, class_name: 'User'
  belongs_to :teacher

[excid3]

That's why then since the default scope will set it to the second school automatically.

[ybakos]

@excid3 Right.... but I am surprised that, in the query log screenshotted above, while the "Registration Load" does use the tenant, the query for "Activity Load" doesn't use the tenant.

[excid3]

Is Activity also acts_as_tenant?

If you say school.activities, it will set the school_id in the query normally in Rails. That may be overriding the default scope from ActsAsTenant.

Then the destroy would call activity.registrations and it would only set the activity_id on the query not the school_id, so that would be set from ActsAsTenant's default_scope.

[ybakos]

Yep, Activity is also acts_as_tenant:

class Activity < ApplicationRecord
  acts_as_tenant(:school)
  has_many :registrations, dependent: :destroy

I agree with what you are observing. My question is: isn't this "surprising" or "inconsistent" behavior that acts_as_tenant may have overlooked in its implementation?

Or... I think I see what's happening:

1. We invoke School#destroy.
2. Because a School has_many :activities, dependent: :destroy, AR finds all associated Activities in a manner equivalent to School#activities.
3. Because a School does not acts_as_tenant for itself, none of the finders for its has_many records will ever be scoped to the current tenant. (right?)
4. Second-degree has_many :foo, dependent: :destroy finders will scope to the tenant, because they acts_as_tenant.

[excid3]

Yes and no. The default_scope implementation can only do so much and I think ActsAsTenant should be strict so as not to leak records between tenants.

I think it makes sense that if you expect to destroy a tenant and it's records, it makes sense that you'd want to be in that tenant and not another one.

What you're doing is saying:

1. I only want to see records for School 2
2. Delete School 1 and dependent records
3. Since we're only allowed to view School 2 records, we cannot delete School 1 with all it's dependent records

I agree that it may seem a bit unexpected, but if this were relaxed then you could view School 1's records from the School 2 tenant by using the associations to access records. That would be bad.