Return a ServiceResponse from conflicts ResolveService

Marc Shawrequested to merge
630903-conflicts-resolve-service-response into master

What does this MR do and why?

First step of the epic to resolve merge request conflicts through the GraphQL API (gitlab-org#23712). It moves the checks around conflict resolution out of the conflicts controller and into MergeRequests::Conflicts::ResolveService, which now returns a ServiceResponse instead of raising. The GraphQL mutation later in the epic cannot reuse controller code, so it needs a service it can call and read errors from.

This is a pure refactor with no behaviour change: the controller maps each error reason back to the same status code and message as master.

Detailed context for AI agents

Closes #630903 (closed)

An earlier attempt at this move, !256973 (closed) , was closed unmerged. It mixed the move with several status code and message changes, which made it hard to review. This MR only moves code. Each behaviour change has its own follow-up issue:

What changed

MergeRequests::Conflicts::ResolveService#execute(current_user, params) now returns a ServiceResponse:

  • Success: ServiceResponse.success(payload: { merge_request: })
  • Error: ServiceResponse.error(message:, reason:), with the reason being a constant owned by the service: REASON_NO_PUSH_ACCESS, REASON_NOT_RESOLVABLE_IN_UI, REASON_ALREADY_RESOLVED, REASON_RESOLUTION_ERROR, REASON_PRE_RECEIVE.

The service now runs the checks the controller used to run around it, in the same order as master:

  1. Push access to the source branch, via ListService#can_be_resolved_by?
  2. ListService#can_be_resolved_in_ui?
  3. The track_resolve_conflict_action usage counter
  4. The already-resolved check, via can_be_merged?
  5. The resolve call itself

It rescues Gitlab::Git::Conflict::Resolver::ResolutionError and Gitlab::Git::PreReceiveError into error reasons.

Projects::MergeRequests::ConflictsController#resolve_conflicts now calls the service and maps reasons back to the same responses as master: no push access or not resolvable in UI produce a 404, pre-receive produces a 422 with the hook message, and everything else, resolution error or already resolved, produces a 400 with the message.

The controller before_action :authorize_can_resolve_conflicts! now skips resolve_conflicts, since the service runs the same checks in the same order. It still covers show and conflict_for_path, where it also triggers the first Gitaly call early so HandlesGitalyErrors can answer with a 503. For resolve_conflicts, a Gitaly error raised inside the service reaches the same rescue_from, so the 503 behaviour is unchanged (covered by the controller spec).

Gitaly errors are not rescued in the service yet. They still go to the controller's HandlesGitalyErrors rescue_from and answer with a 503, exactly as before. Moving that into the service is #630906 (closed) .

The already-resolved check stays unreachable, exactly as on master: can_be_resolved_in_ui? is false for a mergeable merge request, so the 404 fires first. A code comment links #630905 (closed) , which fixes the ordering.

The no-push-access message reuses the existing string Cannot push to source branch, already used by MergeRequests::RebaseService. The not-resolvable message reuses the controller's existing string. So there are no new translations and no locale/gitlab.pot change.

The service now builds its conflicts through MergeRequests::Conflicts::ListService#conflicts, instead of building Gitlab::Conflict::FileCollection.new(merge_request) directly. This adds no extra Gitaly calls: the merge request memoizes its branch heads, and FileCollection#can_be_resolved_in_ui? is cached in Redis keyed by the two commit ids, so the service's own ListService performs the same Gitaly calls the controller used to make, once.

No database change, no feature flag, no changelog since this is an internal refactor, no UI change.

Specs

spec/services/merge_requests/conflicts/resolve_service_spec.rb: existing examples now assert on the returned response instead of raise_error. The spec user gets developer access and the merge requests are created with merge_status: :cannot_be_merged, so they pass the new gates (the factory default is can_be_merged). New contexts added:

  • Success payload, and that the counter is tracked
  • No push access
  • Not resolvable in the UI, for a mergeable merge request and for unparseable conflicts
  • Already resolved, reached by stubbing can_be_resolved_in_ui?, since it is unreachable otherwise
  • Pre-receive rejection
  • A Gitaly CommandError from the resolve call still propagates, so the controller's HandlesGitalyErrors keeps answering 503

The refusal contexts assert the counter is not tracked and no commit is made.

spec/controllers/projects/merge_requests/conflicts_controller_spec.rb: one example changed and one added. The pre-receive example used to stub the service's execute to raise PreReceiveError; the service now rescues that itself, so the stub returns the REASON_PRE_RECEIVE error response instead, with the same expected 422 status and message. The added example covers the not-resolvable 404 path, which had no POST coverage. Every other example passes unchanged, which is the evidence that nothing observable changed.

Verification run locally

  • bundle exec rspec spec/services/merge_requests/conflicts/resolve_service_spec.rb: 17 examples, 0 failures.
  • bundle exec rspec spec/controllers/projects/merge_requests/conflicts_controller_spec.rb: 19 examples, 0 failures.
  • VITE_RUBY_DEV_SERVER_CONNECTION_CHECK=true bundle exec rspec spec/features/merge_request/user_resolves_conflicts_spec.rb: 16 examples, 0 failures, 1 pending (an xit already skipped on master), run locally against the GDK Vite dev server since the MR pipeline is tier-1 and does not run system specs.
  • RuboCop clean on all four changed files.

References

Screenshots or screen recordings

Not applicable, backend only, no UI change.

How to set up and validate locally

  1. Create a merge request from conflict-resolvable into conflict-start in a project using the test repository, or any merge request with conflicts that GitLab can resolve in the UI.
  2. Open the merge request, choose Resolve conflicts, pick sides and commit. It should behave exactly as on master: a merge commit on the source branch and a redirect back to the merge request.
  3. Optionally compare the JSON responses of POST .../merge_requests/:iid/conflicts/resolve_conflicts for a missing section (400 with the Gitaly resolution message), an already-mergeable merge request (404), and a user without push access (404), against master.

MR acceptance checklist

Evaluate this MR against the MR acceptance checklist. It helps you analyze changes to reduce risks in quality, performance, reliability, security, and maintainability.

Edited by Marc Shaw

Merge request reports