Report unresolvable reviewer usernames in setReviewers

Marc Shawrequested to merge
611232-set-reviewers-unresolvable-usernames into master

mergeRequestSetReviewers returned errors: [] for a username that doesn't resolve to a visible user, and left the merge request untouched, so a caller couldn't tell that apart from a real assignment. It now resolves what it can, applies it, and reports the rest in errors rather than rejecting the whole call. An earlier revision of this MR did reject the call outright, but that would have broken the reviewer sidebar for any merge request with a blocked, banned or LDAP-blocked reviewer already assigned.

Fixes #611232 (closed)

Detail

Why it was broken

  • UsersFinder.new(current_user, username: usernames).execute silently drops usernames that match no visible user.
  • With operationMode: APPEND, the resulting id set then equals the merge request's existing reviewers.
  • MergeRequests::UpdateReviewersService#execute (app/services/merge_requests/update_reviewers_service.rb:15) sees no change and returns early, so nothing is written.
  • errors: [] comes back, indistinguishable from a real assignment.

The change

  • Resolve the usernames once, diff against what came back, and add the unresolvable ones to errors.
  • The usernames that did resolve are still applied. The call is not rejected as a whole.
  • Comparison is case-insensitive, matching User.by_username's iwhere (app/models/user.rb:728), so JDoe still resolves to jdoe.
  • Duplicate usernames are reported once, matched case-insensitively, since route paths are unique case-insensitively (app/models/route.rb:27) so Foo and foo cannot be two different users.
  • An empty username list still clears all reviewers, unchanged.
  • Reused the existing errors array rather than adding a payload field. field :errors (app/graphql/mutations/base_mutation.rb:15) already declares scopes: [:api, :read_api, :ai_workflows], so an ai_workflows-scoped caller can read it, whereas Types::MergeRequestType#reviewers (app/graphql/types/merge_request_type.rb:247) defaults to scopes: [:api, :read_api] and can't be read back by that caller.

Why it applies partially instead of rejecting the call

  • An earlier revision of this change rejected the whole call when any username was unresolvable. That's a breaking change under GitLab's definition: "Any change counts as a breaking change if customers need to take action to ensure their GitLab workflows aren't disrupted" (doc/development/deprecation_guidelines/_index.md:12).
  • Concrete case: the reviewer sidebar sends the whole selected reviewer list in REPLACE mode (app/assets/javascripts/merge_requests/components/reviewers/reviewer_dropdown.vue:341). For a non-admin, UsersFinder's base scope is User.without_forbidden_states (app/models/user.rb:781), where FORBIDDEN_SEARCH_STATES is blocked, banned, ldap_blocked (app/models/user.rb:75). On a merge request that already has a reviewer in one of those states, for example someone who left the company and is ldap_blocked, every reviewer change from the sidebar would have been refused. The dropdown doesn't read errors, so the user would have seen the change silently revert.
  • Verified against a local GDK as a non-admin Developer. With the rejecting version, REPLACE ["evia", "codeowner"] where evia is blocked returned Reviewers not found: evia and left the reviewers as ["evia"]. With partial application it returns the same error and the reviewers become ["codeowner"].
  • Partial application is what the GraphQL style guide prescribes. errors "may be populated on success" (doc/development/api_graphql_styleguide.md:2028), and it gives a near identical example: "if a user uploads 10 files and 3 of them fail and the rest succeed, the errors for the failures can be made available to the user, alongside the information about the successes" (doc/development/api_graphql_styleguide.md:2094).
  • Precedent for partial application is DesignManagementUpload (app/graphql/mutations/design_management/upload.rb:20), which applies what it can and reports the rest. The all-or-nothing precedents (ee/app/graphql/mutations/incident_management/oncall_rotation/base.rb:79 and ee/app/graphql/mutations/incident_management/escalation_policy/base.rb:81) raise a top-level argument error, and they exist where partial state is incoherent, such as half an escalation policy. Reviewer assignment is set membership, where partial application is coherent.

Behaviour change to weigh

  • A call that previously returned errors: [] now returns the unresolvable usernames in errors. The same reviewers are assigned as before, so no caller that succeeds today starts failing.
  • REPLACE with only unresolvable usernames still clears all reviewers, which is unchanged, but now says why.
  • The reviewer sidebar doesn't read errors, so there's no UI impact.

Who this affects

Any GraphQL API caller. The motivating case is the Duo Agent Platform recommend_reviewers flow, which calls this mutation through a new add_merge_request_reviewers tool (gitlab-org/modelops/applied-ml/code-suggestions/ai-assist!6040 (merged)). Because failure was unreportable, that tool could only echo back the usernames it was asked to add. It can now report what was actually assigned.

Related, not fixed here

  • Without the multiple_merge_request_reviewers licensed feature, reviewer_ids truncates with .first(1) (the EE override at ee/app/services/ee/merge_requests/update_reviewers_service.rb:10 skips truncation only when licensed). With APPEND, existing reviewers sort first, so the truncation drops the one being added, another silent no-change. Noted in the issue.
  • Mutations::Assignable (app/graphql/mutations/concerns/mutations/assignable.rb:42) has the same silent-drop shape for assignee_usernames. It backs MergeRequestSetAssignees and IssueSetAssignees.

Testing

  • Added to spec/graphql/mutations/merge_requests/set_reviewers_spec.rb and spec/requests/api/graphql/mutations/merge_requests/set_reviewers_spec.rb: an unresolvable username is reported while the ones that resolved are still assigned; an existing reviewer in a state UsersFinder excludes is reported and the rest of the change still applies; APPEND reports the unresolvable username rather than reporting success; a username differing only by case still resolves.
  • Passing locally: both of the above plus ee/spec/graphql/mutations/merge_requests/set_reviewers_spec.rb. Rubocop clean. GraphQL reference docs and both introspection schemas regenerated.
  • Verified end-to-end against a local GDK through the real GraphQL API: REPLACE, APPEND and REMOVE with an unresolvable username, case-insensitive resolution, duplicate usernames reported once including case variants of the same name, empty list still clears, and the blocked-reviewer sidebar case as a non-admin Developer.
Edited by Marc Shaw

Merge request reports