Add reviewer_id and reviewer_username filters to MergeRequest (!47355) · Merge requests · GitLab.org / GitLab

Sincheol (David) Kim requested to merge revert-8b5e1ae9 into master Nov 11, 2020

What does this MR do?

This MR adds ability to filter MergeRequest by reviewer_id and reviewer_username the same as assignee_id and assignee_username does except the negated multiple assignees as that doesn't seem to be in use as far as I can tell.

frontend work is required to make it accessible to the users.

We originally attempted to merge Assignee and Reviewer in !45738 (merged) and !43497 (merged), but we encountered performance issues.

This is compromised option to get Reviewer feature out the door and we'll be iterating on it.

Related to #237922 (closed)

Database

Filter: Assignee = @dskim_gitlab AND Reviewer = @dskim_gitlab

EXPLAIN
SELECT
  merge_requests.*,
  (
    SELECT
      MIN("label_priorities"."priority")
    FROM
      "labels"
    LEFT OUTER JOIN "label_priorities" ON "labels"."id" = "label_priorities"."label_id"
  INNER JOIN "label_links" ON "label_links"."label_id" = "labels"."id"
WHERE (label_priorities.project_id = merge_requests.target_project_id)
  AND (label_links.target_id = merge_requests.id)
  AND "label_links"."target_type" = 'MergeRequest') AS highest_priority,
MIN(milestones.due_date)
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
  LEFT OUTER JOIN milestones ON merge_requests.milestone_id = milestones.id
WHERE (EXISTS (
    SELECT
      1
    FROM
      "project_authorizations"
    WHERE
      "project_authorizations"."user_id" = 4901936
      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 (4901936)
      AND merge_request_id = merge_requests.id))
AND "projects"."archived" = FALSE
AND EXISTS (
  SELECT
    TRUE
  FROM
    "merge_request_reviewers"
  WHERE
    "merge_request_reviewers"."user_id" = 4901936
    AND merge_request_id = merge_requests.id)
GROUP BY
  "merge_requests"."id"
ORDER BY
  MIN(milestones.due_date) ASC NULLS LAST,
  highest_priority ASC NULLS LAST,
  "merge_requests"."id" DESC
LIMIT 20

Explain: https://explain.depesz.com/s/HfS6

Filter: Assignee = @dskim_gitlab AND Reviewer != @dskim_gitlab

EXPLAIN
SELECT
  merge_requests.*,
  (
    SELECT
      MIN("label_priorities"."priority")
    FROM
      "labels"
    LEFT OUTER JOIN "label_priorities" ON "labels"."id" = "label_priorities"."label_id"
  INNER JOIN "label_links" ON "label_links"."label_id" = "labels"."id"
WHERE (label_priorities.project_id = merge_requests.target_project_id)
  AND (label_links.target_id = merge_requests.id)
  AND "label_links"."target_type" = 'MergeRequest') AS highest_priority,
MIN(milestones.due_date)
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
  LEFT OUTER JOIN milestones ON merge_requests.milestone_id = milestones.id
WHERE (EXISTS (
    SELECT
      1
    FROM
      "project_authorizations"
    WHERE
      "project_authorizations"."user_id" = 4901936
      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 (4901936)
      AND merge_request_id = merge_requests.id))
AND "projects"."archived" = false
AND NOT (EXISTS (
    SELECT
      TRUE
    FROM
      "merge_request_reviewers"
    WHERE
      "merge_request_reviewers"."user_id" = 4901936
      AND merge_request_id = merge_requests.id))
GROUP BY
  "merge_requests"."id"
ORDER BY
  MIN(milestones.due_date) ASC NULLS LAST,
  highest_priority ASC NULLS LAST,
  "merge_requests"."id" DESC
LIMIT 20

Explain: https://explain.depesz.com/s/K1FL

Filter: Assignee = @dskim_gitlab AND Reviewer = Any

EXPLAIN
SELECT
  merge_requests.*,
  (
    SELECT
      MIN("label_priorities"."priority")
    FROM
      "labels"
    LEFT OUTER JOIN "label_priorities" ON "labels"."id" = "label_priorities"."label_id"
  INNER JOIN "label_links" ON "label_links"."label_id" = "labels"."id"
WHERE (label_priorities.project_id = merge_requests.target_project_id)
  AND (label_links.target_id = merge_requests.id)
  AND "label_links"."target_type" = 'MergeRequest') AS highest_priority,
MIN(milestones.due_date)
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
  LEFT OUTER JOIN milestones ON merge_requests.milestone_id = milestones.id
WHERE (EXISTS (
    SELECT
      1
    FROM
      "project_authorizations"
    WHERE
      "project_authorizations"."user_id" = 4901936
      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 (4901936)
      AND merge_request_id = merge_requests.id))
AND "projects"."archived" = false
AND EXISTS (
  SELECT
    TRUE
  FROM
    "merge_request_reviewers"
  WHERE
    merge_request_id = merge_requests.id)
GROUP BY
  "merge_requests"."id"
ORDER BY
  MIN(milestones.due_date) ASC NULLS LAST,
  highest_priority ASC NULLS LAST,
  "merge_requests"."id" DESC
LIMIT 20

Explain: https://explain.depesz.com/s/BAQ8

Filter: Assignee = @dskim_gitlab AND Reviewer = None

EXPLAIN
SELECT
  merge_requests.*,
  (
    SELECT
      MIN("label_priorities"."priority")
    FROM
      "labels"
    LEFT OUTER JOIN "label_priorities" ON "labels"."id" = "label_priorities"."label_id"
  INNER JOIN "label_links" ON "label_links"."label_id" = "labels"."id"
WHERE (label_priorities.project_id = merge_requests.target_project_id)
  AND (label_links.target_id = merge_requests.id)
  AND "label_links"."target_type" = 'MergeRequest') AS highest_priority,
MIN(milestones.due_date)
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
  LEFT OUTER JOIN milestones ON merge_requests.milestone_id = milestones.id
WHERE (EXISTS (
    SELECT
      1
    FROM
      "project_authorizations"
    WHERE
      "project_authorizations"."user_id" = 4901936
      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 (4901936)
      AND merge_request_id = merge_requests.id))
AND "projects"."archived" = false
AND NOT (EXISTS (
    SELECT
      TRUE
    FROM
      "merge_request_reviewers"
    WHERE
      merge_request_id = merge_requests.id))
GROUP BY
  "merge_requests"."id"
ORDER BY
  MIN(milestones.due_date) ASC NULLS LAST,
  highest_priority ASC NULLS LAST,
  "merge_requests"."id" DESC
LIMIT 20

Explain: https://explain.depesz.com/s/qc3Z

Filter: Assignee = @dskim_gitlab AND Reviewer = @dskim_gitlab AND Approved-By = @dskim_gitlab

EXPLAIN
SELECT
  merge_requests.*,
  (
    SELECT
      MIN("label_priorities"."priority")
    FROM
      "labels"
    LEFT OUTER JOIN "label_priorities" ON "labels"."id" = "label_priorities"."label_id"
  INNER JOIN "label_links" ON "label_links"."label_id" = "labels"."id"
WHERE (label_priorities.project_id = merge_requests.target_project_id)
  AND (label_links.target_id = merge_requests.id)
  AND "label_links"."target_type" = 'MergeRequest') AS highest_priority,
MIN(milestones.due_date)
FROM
  "merge_requests"
  INNER JOIN "projects" ON "projects"."id" = "merge_requests"."target_project_id"
  INNER JOIN "approvals" ON "approvals"."merge_request_id" = "merge_requests"."id"
  INNER JOIN "users" ON "users"."id" = "approvals"."user_id"
  LEFT JOIN project_features ON projects.id = project_features.project_id
  LEFT OUTER JOIN milestones ON merge_requests.milestone_id = milestones.id
WHERE (EXISTS (
    SELECT
      1
    FROM
      "project_authorizations"
    WHERE
      "project_authorizations"."user_id" = 4901936
      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 (4901936)
      AND merge_request_id = merge_requests.id))
AND "projects"."archived" = FALSE
AND "users"."username" = 'dskim_gitlab'
AND EXISTS (
  SELECT
    TRUE
  FROM
    "merge_request_reviewers"
  WHERE
    "merge_request_reviewers"."user_id" = 4901936
    AND merge_request_id = merge_requests.id)
GROUP BY
  "merge_requests"."id",
  "merge_requests"."id"
HAVING (COUNT(users.id) = 1)
ORDER BY
  MIN(milestones.due_date) ASC NULLS LAST,
  highest_priority ASC NULLS LAST,
  "merge_requests"."id" DESC
LIMIT 20

Explain: https://explain.depesz.com/s/ykcQ

Screenshots (strongly suggested)

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 Nov 13, 2020 by Sincheol (David) Kim

Add reviewer_id and reviewer_username filters to MergeRequest