Avoid N+1 queries on Audit Events controllers (!26245) · Merge requests · GitLab.org / GitLab

Tan Le requested to merge tle-view-audit-events-query-opt into master Mar 02, 2020

What does this MR do?

Avoid N+1 queries fetching Author (~ User) and Entity (~ User/Project/Group) for Audit Events on GitLab UI. API does not exihibit this behaviour as the JSON view doesn't include details from associated models.

In particular, we need to pull in the associated Author to decorate the path and display name. This results in a number of extra SELECT queries to the Users table. This is currently affecting Audit Events views on Instance, Group, and Project level. Similar story applies on Entity where we decorate the path and display name.

Running the test before and after this changes yields the following results

bundle exec spring rspec ee/spec/requests/admin/audit_events_spec.rb -fd

Before

=> ["SELECT \"users\".* FROM \"users\" WHERE \"users\".\"id\" = 1 ORDER BY \"users\".\"id\" ASC LIMIT 1",
 "SELECT 1 AS one FROM \"identities\" WHERE \"identities\".\"user_id\" = 1 AND (provider = 'ultraauth' AND extern_uid IS NOT NULL) LIMIT 1",
 "SELECT 1 AS one FROM \"identities\" WHERE \"identities\".\"user_id\" = 1 AND (provider LIKE 'ldap%' AND extern_uid IS NOT NULL) LIMIT 1",
 "SELECT 1 AS one FROM \"identities\" WHERE \"identities\".\"user_id\" = 1 AND (provider = 'ultraauth' AND extern_uid IS NOT NULL) LIMIT 1",
 "SELECT \"user_preferences\".* FROM \"user_preferences\" WHERE \"user_preferences\".\"user_id\" = 1 LIMIT 1",
 "SELECT \"audit_events\".* FROM \"audit_events\" ORDER BY \"audit_events\".\"id\" DESC LIMIT 26 OFFSET 0",
 "SELECT \"users\".* FROM \"users\" WHERE \"users\".\"id\" = 6 LIMIT 1",
 "SELECT \"users\".* FROM \"users\" WHERE \"users\".\"id\" = 7 LIMIT 1",
 "SELECT \"users\".* FROM \"users\" WHERE \"users\".\"id\" = 4 LIMIT 1",
 "SELECT \"users\".* FROM \"users\" WHERE \"users\".\"id\" = 5 LIMIT 1",
 "SELECT \"users\".* FROM \"users\" WHERE \"users\".\"id\" = 2 LIMIT 1",
 "SELECT \"users\".* FROM \"users\" WHERE \"users\".\"id\" = 3 LIMIT 1",
 "SELECT \"appearances\".* FROM \"appearances\" ORDER BY \"appearances\".\"id\" ASC LIMIT 1",
 "SELECT \"appearances\".* FROM \"appearances\" ORDER BY \"appearances\".\"id\" ASC LIMIT 1",
 "SELECT \"appearances\".* FROM \"appearances\" ORDER BY \"appearances\".\"id\" ASC LIMIT 1",
 "SELECT \"appearances\".* FROM \"appearances\" ORDER BY \"appearances\".\"id\" ASC LIMIT 1",
 "SELECT \"licenses\".* FROM \"licenses\" ORDER BY \"licenses\".\"id\" DESC LIMIT 1",
 "SELECT 1 AS one FROM \"geo_nodes\" LIMIT 1",
 "SELECT COUNT(*) FROM \"projects\" INNER JOIN \"namespaces\" ON \"projects\".\"namespace_id\" = \"namespaces\".\"id\" WHERE \"namespaces\".\"owner_id\" = 1 AND \"namespaces\".\"type\" IS NULL",
 "SELECT 1 AS one FROM \"users\" WHERE \"users\".\"id\" = $1 LIMIT $2",
 "SELECT \"users\".* FROM \"users\" WHERE \"users\".\"id\" = $1",
 "SELECT COUNT(*) FROM \"issues\" INNER JOIN \"projects\" ON \"projects\".\"id\" = \"issues\".\"project_id\" LEFT JOIN project_features ON projects.id = project_features.project_id WHERE (EXISTS (SELECT 1 FROM \"project_authorizations\" WHERE \"project_authorizations\".\"user_id\" = 1 AND (project_authorizations.project_id = projects.id)) OR projects.visibility_level IN (0,10,20)) AND (\"project_features\".\"issues_access_level\" > 0 OR \"project_features\".\"issues_access_level\" IS NULL) AND (\"issues\".\"state_id\" IN (1)) AND (EXISTS (SELECT true FROM \"issue_assignees\" WHERE \"issue_assignees\".\"user_id\" IN (1) AND issue_id = issues.id)) AND \"projects\".\"archived\" = $1",
 "SELECT \"users\".* FROM \"users\" WHERE \"users\".\"id\" = $1 ORDER BY \"users\".\"id\" ASC LIMIT $2",
 "SELECT COUNT(*) FROM \"merge_requests\" INNER JOIN \"projects\" ON \"projects\".\"id\" = \"merge_requests\".\"target_project_id\" LEFT JOIN project_features ON projects.id = project_features.project_id WHERE (EXISTS (SELECT 1 FROM \"project_authorizations\" WHERE \"project_authorizations\".\"user_id\" = 1 AND (project_authorizations.project_id = projects.id)) OR projects.visibility_level IN (0,10,20)) AND (\"project_features\".\"merge_requests_access_level\" > 0 OR \"project_features\".\"merge_requests_access_level\" IS NULL) AND (\"merge_requests\".\"state_id\" IN (1)) AND (EXISTS (SELECT true FROM \"merge_request_assignees\" WHERE \"merge_request_assignees\".\"user_id\" IN (1) AND merge_request_id = merge_requests.id)) AND \"projects\".\"archived\" = $1",
 "SELECT COUNT(*) FROM \"todos\" WHERE \"todos\".\"user_id\" = $1 AND (\"todos\".\"state\" IN ('pending'))",
 "SELECT \"user_statuses\".* FROM \"user_statuses\" WHERE \"user_statuses\".\"user_id\" = $1 LIMIT $2",
 "SELECT COUNT(*) FROM \"abuse_reports\"",
 "SELECT COUNT(*) FROM \"abuse_reports\"",
 "SELECT \"broadcast_messages\".* FROM \"broadcast_messages\" WHERE (ends_at > '2020-03-20 10:01:54.846624') AND \"broadcast_messages\".\"broadcast_type\" = $1 ORDER BY \"broadcast_messages\".\"id\" ASC",
 "SELECT \"broadcast_messages\".* FROM \"broadcast_messages\" WHERE (ends_at > '2020-03-20 10:01:54.850633') AND \"broadcast_messages\".\"broadcast_type\" = $1 ORDER BY \"broadcast_messages\".\"id\" ASC"]

After

=> ["SELECT \"users\".* FROM \"users\" WHERE \"users\".\"id\" = 1 ORDER BY \"users\".\"id\" ASC LIMIT 1",
 "SELECT 1 AS one FROM \"identities\" WHERE \"identities\".\"user_id\" = 1 AND (provider = 'ultraauth' AND extern_uid IS NOT NULL) LIMIT 1",
 "SELECT 1 AS one FROM \"identities\" WHERE \"identities\".\"user_id\" = 1 AND (provider LIKE 'ldap%' AND extern_uid IS NOT NULL) LIMIT 1",
 "SELECT 1 AS one FROM \"identities\" WHERE \"identities\".\"user_id\" = 1 AND (provider = 'ultraauth' AND extern_uid IS NOT NULL) LIMIT 1",
 "SELECT \"user_preferences\".* FROM \"user_preferences\" WHERE \"user_preferences\".\"user_id\" = 1 LIMIT 1",
 "SELECT \"audit_events\".* FROM \"audit_events\" ORDER BY \"audit_events\".\"id\" DESC",
 "SELECT \"audit_events\".* FROM \"audit_events\" ORDER BY \"audit_events\".\"id\" DESC LIMIT 26 OFFSET 0",
 "SELECT \"users\".* FROM \"users\" WHERE \"users\".\"id\" IN (6, 4, 2) ORDER BY \"users\".\"id\" ASC LIMIT 1000",
 "SELECT \"users\".* FROM \"users\" WHERE \"users\".\"id\" IN (7, 5, 3) ORDER BY \"users\".\"id\" ASC LIMIT 1000",
 "SELECT \"appearances\".* FROM \"appearances\" ORDER BY \"appearances\".\"id\" ASC LIMIT 1",
 "SELECT \"appearances\".* FROM \"appearances\" ORDER BY \"appearances\".\"id\" ASC LIMIT 1",
 "SELECT \"appearances\".* FROM \"appearances\" ORDER BY \"appearances\".\"id\" ASC LIMIT 1",
 "SELECT \"appearances\".* FROM \"appearances\" ORDER BY \"appearances\".\"id\" ASC LIMIT 1",
 "SELECT \"licenses\".* FROM \"licenses\" ORDER BY \"licenses\".\"id\" DESC LIMIT 1",
 "SELECT 1 AS one FROM \"geo_nodes\" LIMIT 1",
 "SELECT COUNT(*) FROM \"projects\" INNER JOIN \"namespaces\" ON \"projects\".\"namespace_id\" = \"namespaces\".\"id\" WHERE \"namespaces\".\"owner_id\" = 1 AND \"namespaces\".\"type\" IS NULL",
 "SELECT 1 AS one FROM \"users\" WHERE \"users\".\"id\" = $1 LIMIT $2",
 "SELECT \"users\".* FROM \"users\" WHERE \"users\".\"id\" = $1",
 "SELECT COUNT(*) FROM \"issues\" INNER JOIN \"projects\" ON \"projects\".\"id\" = \"issues\".\"project_id\" LEFT JOIN project_features ON projects.id = project_features.project_id WHERE (EXISTS (SELECT 1 FROM \"project_authorizations\" WHERE \"project_authorizations\".\"user_id\" = 1 AND (project_authorizations.project_id = projects.id)) OR projects.visibility_level IN (0,10,20)) AND (\"project_features\".\"issues_access_level\" > 0 OR \"project_features\".\"issues_access_level\" IS NULL) AND (\"issues\".\"state_id\" IN (1)) AND (EXISTS (SELECT true FROM \"issue_assignees\" WHERE \"issue_assignees\".\"user_id\" IN (1) AND issue_id = issues.id)) AND \"projects\".\"archived\" = $1",
 "SELECT \"users\".* FROM \"users\" WHERE \"users\".\"id\" = $1 ORDER BY \"users\".\"id\" ASC LIMIT $2",
 "SELECT COUNT(*) FROM \"merge_requests\" INNER JOIN \"projects\" ON \"projects\".\"id\" = \"merge_requests\".\"target_project_id\" LEFT JOIN project_features ON projects.id = project_features.project_id WHERE (EXISTS (SELECT 1 FROM \"project_authorizations\" WHERE \"project_authorizations\".\"user_id\" = 1 AND (project_authorizations.project_id = projects.id)) OR projects.visibility_level IN (0,10,20)) AND (\"project_features\".\"merge_requests_access_level\" > 0 OR \"project_features\".\"merge_requests_access_level\" IS NULL) AND (\"merge_requests\".\"state_id\" IN (1)) AND (EXISTS (SELECT true FROM \"merge_request_assignees\" WHERE \"merge_request_assignees\".\"user_id\" IN (1) AND merge_request_id = merge_requests.id)) AND \"projects\".\"archived\" = $1",
 "SELECT COUNT(*) FROM \"todos\" WHERE \"todos\".\"user_id\" = $1 AND (\"todos\".\"state\" IN ('pending'))",
 "SELECT \"user_statuses\".* FROM \"user_statuses\" WHERE \"user_statuses\".\"user_id\" = $1 LIMIT $2",
 "SELECT COUNT(*) FROM \"abuse_reports\"",
 "SELECT COUNT(*) FROM \"abuse_reports\"",
 "SELECT \"broadcast_messages\".* FROM \"broadcast_messages\" WHERE (ends_at > '2020-03-20 10:29:54.483296') AND \"broadcast_messages\".\"broadcast_type\" = $1 ORDER BY \"broadcast_messages\".\"id\" ASC",
 "SELECT \"broadcast_messages\".* FROM \"broadcast_messages\" WHERE (ends_at > '2020-03-20 10:29:54.486512') AND \"broadcast_messages\".\"broadcast_type\" = $1 ORDER BY \"broadcast_messages\".\"id\" ASC"]

As part of this performance improvement, I have also properly captured the Author domain model in Audit land.

Audit::UnauthenticatedAuthor: this author always has id = -1
Audit::DeletedAuthor: this author has id that is no longer existed in our DB (i.e. deleted) and we resolve the name via a the snapshot recorded in author_name attribute.

READ	WRITE

The Entity domain also need to be captured but I have left this out for future MR as this is already growing bigger than anticipated.

Does this MR meet the acceptance criteria?

Conformity

Availability and Testing

Review and add/update tests for this feature/bug. Consider all test levels. See the Test Planning Process.
Tested in all supported browsers
Informed Infrastructure department of a default or new setting change, if applicable per definition of done

Security

If this MR contains changes to processing or storing of credentials or tokens, authorization and authentication methods and other items described in the security review guidelines:

[-] Label as security and @ mention @gitlab-com/gl-security/appsec
[-] The MR includes necessary changes to maintain consistency between UI, API, email, or other methods
[-] Security reports checked/validated by a reviewer from the AppSec team

Edited Mar 20, 2020 by Tan Le

Avoid N+1 queries on Audit Events controllers