If we don't call #to_a, we're relying on the members already being loaded from
elsewhere. Otherwise we'll do a separate query for each user:
[1] pry(main)> Project.first.team.members.include?(User.first)
Project Load (0.7ms) SELECT "projects".* FROM "projects" ORDER BY "projects"."id" ASC LIMIT 1
↳ (pry):3
User Load (1.8ms) SELECT "users".* FROM "users" ORDER BY "users"."id" ASC LIMIT 1
↳ (pry):3
User Exists (0.6ms) SELECT 1 AS one FROM "users" INNER JOIN "project_authorizations" ON "users"."id" = "project_authorizations"."user_id" WHERE "project_authorizations"."project_id" = $1 AND "users"."id" = $2 LIMIT 1 [["project_id", 1], ["id", 1]]
↳ (pry):3
=> true
[2] pry(main)> Project.first.team.members.to_a.include?(User.first)
Project Load (12.8ms) SELECT "projects".* FROM "projects" ORDER BY "projects"."id" ASC LIMIT 1
↳ (pry):1
User Load (9.6ms) SELECT "users".* FROM "users" INNER JOIN "project_authorizations" ON "users"."id" = "project_authorizations"."user_id" WHERE "project_authorizations"."project_id" = $1 [["project_id", 1]]
↳ (pry):1
User Load (0.6ms) SELECT "users".* FROM "users" ORDER BY "users"."id" ASC LIMIT 1
↳ (pry):1
=> true
EE checks a license, which needs RequestStore enabled to avoid N+1
queries. However, enabling RequestStore causes Gitaly to complain about N+1
invocations, which we really don't care about here.
First N+1: we may have loaded a user's notification settings already, but not
have loaded their sources. Because we're iterating through, we'd potentially
load sources that are completely unrelated, just because they belong to this
user.
Second N+1: we do a separate query for each user who could be subscribed to or
unsubcribed from the target. It's actually more efficient in this case to get
all subscriptions at once, as we will need to check most of them.
We can fix both by the slightly unpleasant means of checking IDs manually,
rather than object equality.
Even though it does modify the participants of the notification target in some
cases, this should have been safe, as different workers are responsible for
creating the notifications for each target. However, this is at best confusing,
and we should ensure we don't do that.
Sean McGivern