Return the merge request when cancelling an auto-merge

What does this MR do and why?

POST /projects/:id/merge_requests/:merge_request_iid/cancel_merge_when_pipeline_succeeds promises a merge request in two places and sends one in neither.

Documented Sent before this MR
Success a full merge request {"status":"success"}
Cannot cancel 406 Can't cancel the automatic merge 201 with {"status":"error","message":"Can't cancel the automatic merge","http_status":406}

The desc block at lib/api/merge_requests.rb:930 carries success Entities::MergeRequest on line 932, and the API page prints a whole merge request as the example response and sends the reader on to the single merge request response notes. The endpoint body at lib/api/merge_requests.rb:942 ended in AutoMergeService#cancel, and Grape rendered whatever that returned.

Both halves of the contract are lost in that one line:

  • AutoMergeService#cancel (app/services/auto_merge_service.rb:54) delegates to AutoMerge::BaseService#cancel, whose value comes from clear_auto_merge (app/services/auto_merge/base_service.rb:114) and is ::BaseService#success, so a successful cancel renders {"status":"success"} and no merge request.
  • The same dispatcher returns error("Can't cancel the automatic merge", 406) when auto_merge_enabled? is false, and http_status inside a plain hash is not something Grape reads, so that reached the client as 201 carrying the error hash.

This MR presents the merge request on success and renders the service's error with its own status, which is what the annotation and the page have described all along.

Nothing published changes; the endpoint is aligned to it

The two places that already stated the contract correctly are left exactly as they are, and after this MR the endpoint finally matches both:

Where it is stated Says Touched by this MR After this MR
lib/api/merge_requests.rb:932, success Entities::MergeRequest a merge request no true
doc/api/merge_requests.md, the example response and the 201/406 table a merge request, 406 on refusal no true

So this needs no documentation update and no technical writing review: the page is already right, and the OpenAPI document generated from the desc block describes the new shape because it always described it. The only thing that moves is the handler, which is the one of the three that was wrong.

It also makes the pair symmetric. PUT :id/merge_requests/:merge_request_iid/merge sets auto-merge and presents the merge request at lib/api/merge_requests.rb:950, eight lines above. Only the endpoint that cancels it did not, so a client turning auto-merge off had no way to learn the state it left the merge request in without a second request.

Not a regression: git show v13.0.0-ee:lib/api/merge_requests.rb has the same shape, so the present has been missing for as long as the endpoint has existed in this form.

How I found it

I maintain an MCP server for GitLab, and its end-to-end suite drives the real server binary against a GitLab in Docker. This action was one of the last the suite called without asserting the answer; asserting it failed, because the field the assertion read was empty.

gitlab.com/gitlab-org/api/client-go decodes this endpoint into its MergeRequest struct and reports no error, so an SDK caller receives a zero-valued merge request rather than a failure: no IID, no state, no title, and nothing to distinguish that from a cancel that did not happen.

This is the second annotation-versus-handler disagreement I have sent from that work. The first is !254698 (merged), three desc blocks in lib/api/project_job_token_scope.rb naming entities their handlers do not present. That one corrected the annotations, because the documentation page was already right and only the annotations were wrong. Here the page and the annotation agree with each other and the handler is the odd one out, so this MR moves the handler instead.

Risk, stated plainly

This changes an observable response of a stable API, and that is a maintainer's call rather than mine.

A client reading json["status"] == "success" today stops finding that key. Against that: the documented contract, the Grape annotation and the OpenAPI document generated from it all already describe the new shape, so code written against the documentation is fixed by this rather than broken by it, and the 406 the page documents starts actually being returned.

If you would rather not move the response, the opposite change is small and I am happy to write it instead: drop success Entities::MergeRequest from the desc block, and correct doc/api/merge_requests.md to document {"status":"success"} and the 201 on failure. I did not propose that first because it makes the endpoint's answer less useful than its documentation has promised for six years, but it is the smaller change and the choice is yours.

Testing

Run locally against this branch, on PostgreSQL 17 and Ruby 3.3.11:

bundle exec rspec spec/requests/api/merge_requests_spec.rb -e "cancel_merge_when_pipeline_succeeds"
12 examples, 0 failures, 1 pending

And with the change to lib/api/merge_requests.rb reverted, the same examples on the same spec:

12 examples, 2 failures, 1 pending
  1) ... returns the merge request and not the service status hash
  2) ... when there is no automatic merge to cancel returns 406 with the reason

Those two are the ones the fix is for. The third new example, that auto-merge is off afterwards, passes either way: the 201 never changed, only the body under it.

bundle exec rubocop lib/api/merge_requests.rb spec/requests/api/merge_requests_spec.rb reports no offenses.

The examples in that block were inert, which is why nothing caught this

Writing the tests turned up why the disagreement survived. The before block at spec/requests/api/merge_requests_spec.rb:5154 asked AutoMergeService#execute for the merge_when_checks_pass strategy without arranging a pipeline for it. The strategy is only available while one is being created, so execute returned :failed, auto_merge_enabled stayed false, and every example in the block was cancelling an auto-merge that had never been set.

AutoMergeService#cancel answers Can't cancel the automatic merge for exactly that, and the endpoint handed it back under 201, which is what the single assertion in the block was checking. So it passed for years while exercising the refusal path and never the endpoint's own.

The block now calls mark_as_preparing! and Ci::PipelineCreation::Requests.start_for_merge_request first, the arrangement the auto-merge example at spec/requests/api/merge_requests_spec.rb:4246 already uses for the same reason, and asserts auto_merge_enabled is true before the examples run. That assertion is what makes the rest of them mean anything, and it is why the describe block gained :clean_gitlab_redis_shared_state.

I searched the issue tracker for this endpoint before opening this and found no report of it.

Edited by José M. Requena Plens

Merge request reports

Loading
Loading