Avoid N+1 queries coming from #version_limit when destroying records

[davidrunger]

Fixes #1510

Thank you for your contribution!

Check the following boxes:

• Wrote good commit messages.
• Feature branch is up-to-date with master (if not - rebase it).
• Squashed related commits together.
• Added tests.
• Added an entry to the Changelog if the new
  code introduces user-observable changes.
• The PR relates to only one subject with a clear title
  and description in grammatically correct, complete sentences.

Avoiding N+1 queries when destroying a record with a has_many dependent: :destroy association

This change avoids N+1 queries that otherwise currently occur on the master branch when destroying a record with a has_many dependent: :destroy association where the has_many model is tracked by PaperTrail.

My thoughts about this PR

Some of the things mentioned below are somewhat questionable/hacky. I understand if a PR in this form isn't really desired. But I believe that this change does fix the bug linked above, and I personally think that the changes herein are beneficial improvements to the gem and to its test suite.

Background context

I believe that the N+1 queries that this PR aims to remove were introduced by this change, and this current change essentially undoes that specific linked change in #1309 (though this current change handles the fact that item_subtype is an optional column in a slightly different way than did the original code prior to that change that I believe introduced this N+1 query).

Adding n_plus_one_control gem

In order to test this change, this PR adds the n_plus_one_control gem as a development dependency. I understand that adding dependencies (even development-only dependencies) is a somewhat significant change, and that we might not want to add that gem for the sake of (at least for now) a single test. Nevertheless, it seemed to me like an effective way to test this change, and also that having the n_plus_one_control gem available might also be useful to write other specs in the future related to N+1 queries (or, rather, to a lack thereof), and so I'm including that gem in this proposed change.

Using an instance variable in the spec

I think that the nature of how that gem works (maybe specifically in conjunction with the fact that the spec being added herein destroys the Customer record that is somewhat the subject of the spec?) necessitates capturing the Customer record with an instance variable, so I disabled the RSpec/InstanceVariable cop for that spec.

Hacking PostVersion to act like it has no item_subtype method

In working on this change, in one iteration, I wrote code that assumes that the object into which the PaperTrail::VersionConcern module has been included will respond to an item_subtype message (i.e. have such a method). Specifically, I had written (item_subtype || item_type).constantize. Although this code will fail in any project that uses PaperTrail but which does not add an item_subtype column for the versions table, all specs nevertheless passed (when executed with DB=sqlite bundle exec appraisal rails-8.0 rspec). This implied to me that there is not currently any spec coverage/awareness of the fact that item_subtype is an optional column that will not be available in all projects.

I believe that this is the observation that was being made by @jaredbeck here:

Looking at our SimpleCov report, It seems we are missing a test for when item_subtype is absent? That seems important because item_subtype is an optional column.

The reason that the test suite doesn't have coverage of this possibility (not having an item_subtype method) is because all of the version models in the test suite inherit from PaperTrail::Version, which, in the spec setup, uses the versions table, which is defined with an item_subtype column. So all of the version models in the test suite respond to item_subtype, even if their respective tables (e.g. post_versions) do not have an item_subtype column.

This differs from the situation of real applications using paper_trail, where in many (most?) of those applications the PaperTrail version model(s) will not respond to an item_subtype method.

To try to give the test suite some awareness of this possibility, I hacked one of the version models -- PostVersion -- to raise an error if #item_subtype is called and to return false when queried with respond_to?(:item_subtype). This is ugly/hacky, but it does at least have the beneficial effect that the problematic iteration of my code change that I mentioned above would be caught by some failing specs.

[davidrunger]

Specs are failing for reasons not related to this change. I submitted this PR to fix them: #1512.

[davidrunger]

Woops, I'm sorry, this branch and PR are currently in a bad state, because I made some changes to it for the sake of merging them into my own fork, forgetting about the fact that the branch was also being used for this PR. 😥 I will fix the branch up (and pull in the recently merged fix for specs failing on master, #1512), and then push it back up.

[davidrunger]

Woops, I'm sorry, this branch and PR are currently in a bad state, because I made some changes to it for the sake of merging them into my own fork, forgetting about the fact that the branch was also being used for this PR. 😥 I will fix the branch up (and pull in the recently merged fix for specs failing on master, #1512), and then push it back up.

Done. The diff is back to the intended much, much smaller size, and, having pulled in #1512, all GitHub Actions checks are now passing.

[davidrunger]

Calling item here is essentially what was causing the N+1 query, since instantiating the version's item requires querying the database for the item's attributes.

This change avoids that database query by instead just referencing the item_subtype or item_type fields of the version record, which does not require any additional database query.