Reduce the number of requests when reifying items.

[Nike0]

Hello,

During development, I've found that when we are trying to reify the destroyed record from the versions table, it tries to load this record from the original table twice.
I've added an additional condition for the destroy event and done some code refactoring (Rubocop didn't pass).

I don't know which type is this fix (bug or feature), so I didn't add information to the changelog file.

[gurgelrenan]

Hello @Nike0 . First, thanks for your time and for this P.R! \o/

I just made a comment in a line of code and I want to know another detail:

Did you know how much queries do you avoid? Like from X queries to Y?

Again, thanks for your time to contribute to PaperTrail!

[gurgelrenan]

The previous verification to return klass.new was:

if options[:dup] || (item_found = klass.unscoped.where(find_cond).first).nil?

I'm not sure that destroy_event?(version) is the same thing here. Could you explain?

[Nike0]

Hi @gurgelrenan. I try to explain my changes.
First of all, the count of queries for reifying destroyed objects without option :dup was reduced from 2 to 0.
Here is the first place, when we try to load not existed records from the original table:

if options[:dup] != true && version.item

Second place was here:

if options[:dup] || (item_found = klass.unscoped.where(find_cond).first).nil?

Again, we try to find not existed record from the original table.

The previous verification to return klass.new was:

if options[:dup] || (item_found = klass.unscoped.where(find_cond).first).nil?

I'm not sure that destroy_event?(version) is the same thing here. Could you explain?

Yes, destroy_event?(version) is not the same thing. It was added as a new condition. We don't need to load not existed records. The old condition was moved below into else block.

In the esle block we trying to load item directly, then without scope and if nothing was found, return a new object.

If you want I can create a table to demonstrate all possible test cases for my fix and old solution. My fix affects only items with destroy action. For the create and update actions logic still the same.

[gurgelrenan]

@Nike0 thanks for your explanation. This LGTM, but I would like @jaredbeck take a look.

[jaredbeck]

Hi Ilya, thanks for contributing this optimization. Before I review the code changes, what is a good test I can run that demonstrates the unnecessary queries? For example, animal_spec.rb:63 seems to select from animals twice.

  Animal Load (0.2ms)  SELECT "animals".* FROM "animals" WHERE "animals"."id" = $1 LIMIT $2  
[["id", 27], ["LIMIT", 1]]
  Cat Load (0.2ms)  SELECT "animals".* FROM "animals" WHERE "animals"."species" = $1 AND "animals"."id" = $2 ORDER BY "animals"."id" ASC LIMIT $3  
[["species", "Cat"], ["id", 27], ["LIMIT", 1]]

Is this a good example? Is there a better example? Thanks.

[Nike0]

Hi Ilya, thanks for contributing this optimization. Before I review the code changes, what is a good test I can run that demonstrates the unnecessary queries? For example, animal_spec.rb:63 seems to select from animals twice.

  Animal Load (0.2ms)  SELECT "animals".* FROM "animals" WHERE "animals"."id" = $1 LIMIT $2  
[["id", 27], ["LIMIT", 1]]
  Cat Load (0.2ms)  SELECT "animals".* FROM "animals" WHERE "animals"."species" = $1 AND "animals"."id" = $2 ORDER BY "animals"."id" ASC LIMIT $3  
[["species", "Cat"], ["id", 27], ["LIMIT", 1]]

Is this a good example? Is there a better example? Thanks.

Hi Jared.
Yes, this is a good example. There will no longer be unnecessary requests from the animals table with my changes.

[jaredbeck]

LGTM. I've examined some test logs, and there are, as expected, fewer queries after this optimization. Nice work, Ilya!

After merging, I may make some stylistic changes to this. In particular, I might move destroy_event? to VersionConcern.