Swallowing ActiveRecord::RecordNotDestroyed exceptions
ybakos opened this issue · 11 comments
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.)
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
Some more information. I replicated this error surfaced by a system test in a unit test. Sooo...
- Although my controller is
find
ing the School, (which is the tenant model and therefore itself does not declareacts_as_tenant
nor subject to the constraints of thecurrent_tenant
) destroy
ing any School, regardless of the current tenant, works fine, as long as nodependent:
constraints are encountered (destroying a School and its Teachers works as long as associated Teachers do not have associated Students) [1]- ActiveRecord generates queries to check for
dependent: :restrict_with_error
violations, and these queries are subject to the influence of thecurrent_tenant
. - Those queries return no associated Teachers and/or associated Students for those Teachers
- So the AR check for :restrict_with_error doesn't raise an error, but
- 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.
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.)
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.
Is Registration
also an acts_as_tenant
model?
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
That's why then since the default scope will set it to the second school automatically.
@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.
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.
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:
- We invoke School#destroy.
- Because a School
has_many :activities, dependent: :destroy
, ARfind
s all associated Activities in a manner equivalent to School#activities. - 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?) - Second-degree
has_many :foo, dependent: :destroy
finders will scope to the tenant, because they acts_as_tenant.
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:
- I only want to see records for School 2
- Delete School 1 and dependent records
- 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.