WIP: Simplify ProjectsFinder query (!10869) · Merge requests · GitLab.org / GitLab FOSS

username-removed-840794 requested to merge 0xbsec/gitlab-ce:31255-simplify-project-finder-query into master Apr 22, 2017

What does this MR do?

Remove unnecessary conditions for admin user from ProjectsFinder query.
Replace UNION with OR conditions and WHERE IN with WHERE EXISTS when in user authorization query.
Remove duplicate condition from calls to Project#self.with_feature_available_for_user.

This will affect other finders queries (ref #22145 (moved)).

Updated query for getting events in user profile (logged as a normal user with id: 669):

SELECT "events"."id"
-- ...
FROM "events"
LEFT OUTER JOIN "projects" ON "projects"."id" = "events"."project_id"
AND "projects"."pending_delete" = 'f'
LEFT OUTER JOIN "routes" ON "routes"."source_id" = "projects"."id"
AND "routes"."source_type" = 'Project'
LEFT OUTER JOIN "namespaces" ON "namespaces"."id" = "projects"."namespace_id"
AND "namespaces"."deleted_at" IS NULL
LEFT OUTER JOIN "users" ON "users"."id" = "events"."author_id"
WHERE ("events"."author_id" IS NOT NULL)
  AND "events"."author_id" = 669
  AND "projects"."pending_delete" = 'f'
  AND ("projects"."visibility_level" IN (20,
                                         10)
       OR EXISTS
         (SELECT 1
          FROM "project_authorizations"
          WHERE "projects"."id" = "project_authorizations"."project_id"
            AND "project_authorizations"."user_id" = 669))
ORDER BY "events"."id" DESC LIMIT 20
OFFSET 0

Original -- normal user:

SELECT "events"."id"
-- ...
FROM "events"
LEFT OUTER JOIN "projects" ON "projects"."id" = "events"."project_id"
AND "projects"."pending_delete" = 'f'
LEFT OUTER JOIN "routes" ON "routes"."source_id" = "projects"."id"
AND "routes"."source_type" = 'Project'
LEFT OUTER JOIN "namespaces" ON "namespaces"."id" = "projects"."namespace_id"
AND "namespaces"."deleted_at" IS NULL
LEFT OUTER JOIN "users" ON "users"."id" = "events"."author_id"
WHERE ("events"."author_id" IS NOT NULL)
  AND "events"."author_id" = 669
  AND "projects"."pending_delete" = 'f'
  AND (projects.id IN
         (SELECT "projects"."id"
          FROM "projects"
          INNER JOIN "project_authorizations" ON "projects"."id" = "project_authorizations"."project_id"
          WHERE "projects"."pending_delete" = 'f'
            AND "project_authorizations"."user_id" = 669
          UNION SELECT "projects"."id"
          FROM "projects"
          WHERE "projects"."visibility_level" IN (20,
                                                  10)))
ORDER BY "events"."id" DESC LIMIT 20
OFFSET 0

Updated -- admin

SELECT "events"."id"
-- ...
FROM "events"
LEFT OUTER JOIN "projects" ON "projects"."id" = "events"."project_id"
AND "projects"."pending_delete" = 'f'
LEFT OUTER JOIN "routes" ON "routes"."source_id" = "projects"."id"
AND "routes"."source_type" = 'Project'
LEFT OUTER JOIN "namespaces" ON "namespaces"."id" = "projects"."namespace_id"
AND "namespaces"."deleted_at" IS NULL
LEFT OUTER JOIN "users" ON "users"."id" = "events"."author_id"
WHERE ("events"."author_id" IS NOT NULL)
  AND "events"."author_id" = 669
  AND "projects"."pending_delete" = 'f'
ORDER BY "events"."id" DESC LIMIT 20
OFFSET 0

Original -- admin

SELECT "events"."id"
-- ...
FROM "events"
LEFT OUTER JOIN "projects" ON "projects"."id" = "events"."project_id"
AND "projects"."pending_delete" = 'f'
LEFT OUTER JOIN "routes" ON "routes"."source_id" = "projects"."id"
AND "routes"."source_type" = 'Project'
LEFT OUTER JOIN "namespaces" ON "namespaces"."id" = "projects"."namespace_id"
AND "namespaces"."deleted_at" IS NULL
LEFT OUTER JOIN "users" ON "users"."id" = "events"."author_id"
WHERE ("events"."author_id" IS NOT NULL)
  AND "events"."author_id" = 660
  AND "projects"."pending_delete" = 'f'
  AND (projects.id IN
         (SELECT "projects"."id"
          FROM "projects"
          INNER JOIN "project_authorizations" ON "projects"."id" = "project_authorizations"."project_id"
          WHERE "projects"."pending_delete" = 'f'
            AND "project_authorizations"."user_id" = 1
          UNION SELECT "projects"."id"
          FROM "projects"))
ORDER BY "events"."id" DESC LIMIT 20
OFFSET 0

Some affected queries by removing duplicate condition for union query in Project#self.with_feature_available_for_user (with/without unscopeing):

http://localhost:3000/search?utf8=%E2%9C%93&snippets=&scope=&search=seed

Issue: https://www.diffchecker.com/uy42QCGD
MergeRequest: https://www.diffchecker.com/1QVRiHIZ

Are there points in the code the reviewer needs to double check?

There are some issues I had to workaround:

missing binding values in some queries: http://gitlab.com/0xbsec/gitlab-ce/blob/31255-simplify-project-finder-query/app/models/project.rb#L293-L295 (to reproduce run spec/finders/issues_finder_spec.rb spec without unscoping). Idealy this should be fixed from the root cause (the query should stay unscoped).
calling user.refresh_authorized_projects: https://gitlab.com/0xbsec/gitlab-ce/blob/31255-simplify-project-finder-query/app/models/project.rb#L280 (reproduce in spec: spec/finders/projects_finder_spec.rb:96`)

Why was this MR needed?

Screenshots (if relevant)

Does this MR meet the acceptance criteria?

Changelog entry added, if necessary
Documentation created/updated
API support added
Tests
- Added for this feature/bug
- All builds are passing
Conform by the merge request performance guides
Conform by the style guides
Branch has no merge conflicts with master (if it does - rebase it please)
Squashed related commits together

What are the relevant issue numbers?

Closes #31255 (moved)

Admin message