Fix N+1 problem on Api::TaxonsController#index

[stem]

With the help of associate_parents defined by awesome_nested_set, we can associate the parent of a flat collection.
That way, we can preload all parents for a defined set of taxons with just 1 SQL query.

Closes #3004

[kennyadsl]

Thanks @stem! I'm trying to analyze the result of this PR with bullet but I see the same results as before without PR. Can you please help me understand what happens and why this should fix the N+1?

I'm also trying to see some documentation in awesome_nested_set about the associate_parents method but it really seems like an internal one so I'm a bit concerned on using it directly into Solidus.

[stem]

I understand your concerns about associate_parents but it's publicly defined on the model using act_as_nested_set, so I don't think this method is intended to be internal.

Concerning bullet, I don't know how to use it.
Because the product's page on the backend use the taxons api, I've logged the SQL queries for a product with 8 taxons :

without this PR :

Started GET "/api/taxons?ids=7%2C9%2C3%2C4%2C5%2C6%2C8%2C10&per_page=8&without_children=true" for 127.0.0.1 at 2018-12-24 09:08:36 +0100
Processing by Spree::Api::TaxonsController#index as JSON
  Parameters: {"ids"=>"7,9,3,4,5,6,8,10", "per_page"=>"8", "without_children"=>"true"}
  Spree::User Load (0.3ms)  SELECT  "spree_users".* FROM "spree_users" WHERE "spree_users"."deleted_at" IS NULL AND "spree_users"."spree_api_key" = ? LIMIT ?  [["spree_api_key", "ac228f7e112cf85bb4f238563d63831b472140d87eef4771"], ["LIMIT", 1]]
   (0.2ms)  SELECT "spree_roles"."name" FROM "spree_roles" INNER JOIN "spree_roles_users" ON "spree_roles"."id" = "spree_roles_users"."role_id" WHERE "spree_roles_users"."user_id" = ?  [["user_id", 1]]
  Spree::Role Load (0.2ms)  SELECT "spree_roles".* FROM "spree_roles" INNER JOIN "spree_roles_users" ON "spree_roles"."id" = "spree_roles_users"."role_id" WHERE "spree_roles_users"."user_id" = ?  [["user_id", 1]]

   (0.2ms)  SELECT COUNT(*) FROM (SELECT  1 AS one FROM "spree_taxons" WHERE "spree_taxons"."id" IN (?, ?, ?, ?, ?, ?, ?, ?) LIMIT ? OFFSET ?) subquery_for_count  [["id", 7], ["id", 9], ["id", 3], ["id", 4], ["id", 5], ["id", 6], ["id", 8], ["id", 10], ["LIMIT", 8], ["OFFSET", 0]]
   (0.2ms)  SELECT COUNT(*) FROM "spree_taxons" WHERE "spree_taxons"."id" IN (?, ?, ?, ?, ?, ?, ?, ?)  [["id", 7], ["id", 9], ["id", 3], ["id", 4], ["id", 5], ["id", 6], ["id", 8], ["id", 10]]

  Spree::Taxon Load (0.2ms)  SELECT  "spree_taxons".* FROM "spree_taxons" WHERE "spree_taxons"."id" IN (?, ?, ?, ?, ?, ?, ?, ?) LIMIT ? OFFSET ?  [["id", 7], ["id", 9], ["id", 3], ["id", 4], ["id", 5], ["id", 6], ["id", 8], ["id", 10], ["LIMIT", 8], ["OFFSET", 0]]

  Spree::Taxon Load (0.2ms)  SELECT "spree_taxons".* FROM "spree_taxons" WHERE "spree_taxons"."lft" <= 2 AND "spree_taxons"."rgt" >= 3 AND ("spree_taxons"."id" != 3) ORDER BY "spree_taxons"."lft" ASC
  Spree::Taxon Load (0.2ms)  SELECT "spree_taxons".* FROM "spree_taxons" WHERE "spree_taxons"."lft" <= 4 AND "spree_taxons"."rgt" >= 5 AND ("spree_taxons"."id" != 4) ORDER BY "spree_taxons"."lft" ASC
  Spree::Taxon Load (0.2ms)  SELECT "spree_taxons".* FROM "spree_taxons" WHERE "spree_taxons"."lft" <= 6 AND "spree_taxons"."rgt" >= 11 AND ("spree_taxons"."id" != 5) ORDER BY "spree_taxons"."lft" ASC
  Spree::Taxon Load (0.2ms)  SELECT "spree_taxons".* FROM "spree_taxons" WHERE "spree_taxons"."lft" <= 7 AND "spree_taxons"."rgt" >= 8 AND ("spree_taxons"."id" != 6) ORDER BY "spree_taxons"."lft" ASC
  Spree::Taxon Load (0.2ms)  SELECT "spree_taxons".* FROM "spree_taxons" WHERE "spree_taxons"."lft" <= 9 AND "spree_taxons"."rgt" >= 10 AND ("spree_taxons"."id" != 7) ORDER BY "spree_taxons"."lft" ASC
  Spree::Taxon Load (0.2ms)  SELECT "spree_taxons".* FROM "spree_taxons" WHERE "spree_taxons"."lft" <= 14 AND "spree_taxons"."rgt" >= 15 AND ("spree_taxons"."id" != 8) ORDER BY "spree_taxons"."lft" ASC
  Spree::Taxon Load (0.2ms)  SELECT "spree_taxons".* FROM "spree_taxons" WHERE "spree_taxons"."lft" <= 16 AND "spree_taxons"."rgt" >= 17 AND ("spree_taxons"."id" != 9) ORDER BY "spree_taxons"."lft" ASC
  Spree::Taxon Load (0.2ms)  SELECT "spree_taxons".* FROM "spree_taxons" WHERE "spree_taxons"."lft" <= 18 AND "spree_taxons"."rgt" >= 19 AND ("spree_taxons"."id" != 10) ORDER BY "spree_taxons"."lft" ASC
Completed 200 OK in 134ms (Views: 123.7ms | ActiveRecord: 2.9ms)

with this PR :

Started GET "/api/taxons?ids=7%2C9%2C3%2C4%2C5%2C6%2C8%2C10&per_page=8&without_children=true" for 127.0.0.1 at 2018-12-24 09:07:21 +0100
Processing by Spree::Api::TaxonsController#index as JSON
  Parameters: {"ids"=>"7,9,3,4,5,6,8,10", "per_page"=>"8", "without_children"=>"true"}
  Spree::User Load (0.4ms)  SELECT  "spree_users".* FROM "spree_users" WHERE "spree_users"."deleted_at" IS NULL AND "spree_users"."spree_api_key" = ? LIMIT ?  [["spree_api_key", "ac228f7e112cf85bb4f238563d63831b472140d87eef4771"], ["LIMIT", 1]]
   (0.2ms)  SELECT "spree_roles"."name" FROM "spree_roles" INNER JOIN "spree_roles_users" ON "spree_roles"."id" = "spree_roles_users"."role_id" WHERE "spree_roles_users"."user_id" = ?  [["user_id", 1]]
  Spree::Role Load (0.2ms)  SELECT "spree_roles".* FROM "spree_roles" INNER JOIN "spree_roles_users" ON "spree_roles"."id" = "spree_roles_users"."role_id" WHERE "spree_roles_users"."user_id" = ?  [["user_id", 1]]

  Spree::Taxon Load (0.4ms)  SELECT  "spree_taxons".* FROM "spree_taxons" WHERE "spree_taxons"."id" IN (?, ?, ?, ?, ?, ?, ?, ?) LIMIT ? OFFSET ?  [["id", 7], ["id", 9], ["id", 3], ["id", 4], ["id", 5], ["id", 6], ["id", 8], ["id", 10], ["LIMIT", 8], ["OFFSET", 0]]

  Spree::Taxon Load (0.2ms)  SELECT "spree_taxons".* FROM "spree_taxons" WHERE ((((((("spree_taxons"."lft" < 2 AND "spree_taxons"."rgt" > 3 OR "spree_taxons"."lft" < 4 AND "spree_taxons"."rgt" > 5) OR "spree_taxons"."lft" < 6 AND "spree_taxons"."rgt" > 11) OR "spree_taxons"."lft" < 7 AND "spree_taxons"."rgt" > 8) OR "spree_taxons"."lft" < 9 AND "spree_taxons"."rgt" > 10) OR "spree_taxons"."lft" < 14 AND "spree_taxons"."rgt" > 15) OR "spree_taxons"."lft" < 16 AND "spree_taxons"."rgt" > 17) OR "spree_taxons"."lft" < 18 AND "spree_taxons"."rgt" > 19)

   (0.2ms)  SELECT COUNT(*) FROM (SELECT  1 AS one FROM "spree_taxons" WHERE "spree_taxons"."id" IN (?, ?, ?, ?, ?, ?, ?, ?) LIMIT ? OFFSET ?) subquery_for_count  [["id", 7], ["id", 9], ["id", 3], ["id", 4], ["id", 5], ["id", 6], ["id", 8], ["id", 10], ["LIMIT", 8], ["OFFSET", 0]]
   (0.2ms)  SELECT COUNT(*) FROM "spree_taxons" WHERE "spree_taxons"."id" IN (?, ?, ?, ?, ?, ?, ?, ?)  [["id", 7], ["id", 9], ["id", 3], ["id", 4], ["id", 5], ["id", 6], ["id", 8], ["id", 10]]
Completed 200 OK in 157ms (Views: 143.0ms | ActiveRecord: 1.8ms)

Of course, the other queries are the same, but in this case, the total number came down by 7 queries (8 taxon's ancestors that are loaded in 1 query instead of 8)

If you point me to a way you wanna test it, I can write a spec to ensure the N+1 doesn't reappear.

[kennyadsl]

Thanks for the explanation, now the context is more clear. I left a comment on a possible improvement to avoid declaring a new scope.

Also, I've made some experiment testing the query count to avoid N+1 here: https://gist.github.com/kennyadsl/4443c3bce12e8d2ef2eb0740d61d2344.

Let me know if both make sense!

[kennyadsl]

@stem 👋Hey there! Any update here?

[stem]

@stem 👋Hey there! Any update here?

@kennyadsl, I'll look into it now that I'm back from vacations 🏖

Your experiment seems to do the job so I'll take that road and update this PR to add theses tests

[stem]

FYI, I've found and fixed another N+1 with the taxon's children while ensuring that the tests were OK.

That helps us moving from 11 queries to 5 for the 2 leafs in this simple example :

- Brand
  - Ruby
    - Rails
      - 3.2.2
  - Python
    - 3.0

[kennyadsl]

A last, small thing. Can you rebase and squash some commits (I'm looking at the Hound violation commit in particular, that could go into the Refactor Taxon.pretty_name one). Thanks again!

[stem]

@kennyadsl I've rebased and squash some commits. Let me know if I can do something more to improve this PR.

[stem]

@kennyadsl is there anything I can do to improve or merge this one ?

[kennyadsl]

Thanks!

[jacobherrington]

I'm good with this! Thanks!

[gmacdougall]

Thanks for putting this together. I'm interested by the query count spec but am concerned it would be fragile and likely to cause potential failures in the future.

What do you think about removing the query count matcher portion of this as I think the rest of this is good to go as is.

[kennyadsl]

In the core team meeting we talked about this and I agree with @gmacdougall here (sorry, I pushed to add that spec). There could be a lot of things (rails/act_as_nested_set updates or specs execution order) that would make this test fail. It's probably better to remove it now that we are sure we removed the N+1 there.

[stem]

@kennyadsl I've removed the query_counter part, but kept the rspec matcher in case we need to investigate other N+1. Let me know if you think I should also remove that.

[kennyadsl]

@stem yes, I think it's better to remove this module since we don't use it and we could keep code in the repository that could no longer work in the future.

[stem]

@kennyadsl done !

[kennyadsl]

Thanks @stem !