Fix mutually_exclusive typo in pull mirror API parameter
What does this MR do and why?
ee/lib/api/project_mirror.rb (line 156) declares this validation on PUT /projects/:id/mirror/pull ("Update project pull mirroring settings"):
mutually_exclusive :only_protected_branches, :mirror_branch_regexThe endpoint's actual parameter is only_mirror_protected_branches — the mirror_ segment is missing from the declared name. Grape's MutualExclusionValidator resolves declared names against the keys actually present in the request (MultipleParamsBase#keys_in_common intersects the declared list with resource_params.keys). Since only_protected_branches is never a key on this endpoint, the intersection never reached two entries, and the rule never fired. It has been no-op since it was introduced in !167901 (merged).
The likely origin is a copy-paste from the push-mirror helper ee/lib/ee/api/helpers/remote_mirrors_helpers.rb (line 13), which contains the same line. There it is correct, because push mirrors use the column name only_protected_branches. Pull mirrors use a different column name for the equivalent setting.
Why these two parameters are in fact the mutually exclusive pair:
ee/app/models/concerns/mirror_configuration.rbdefinesmirror_branches_setting, which returns exactly one ofall,protected, orregex. The two settings encode a single tri-state choice, so they are mutually exclusive by construction.- That concern defines an abstract hook,
only_mirror_protected_branches_column, resolved per model:Projectreturnsonly_mirror_protected_branches,RemoteMirrorreturnsonly_protected_branches. Same conceptual field, two column names — which is how the copy-paste went wrong. - Column ownership differs:
projects.only_mirror_protected_branchesversusremote_mirrors.only_protected_branches. ee/app/models/ee/project_setting.rb(line 26) independently validates the pull-mirror pair at the model layer:validates :mirror_branch_regex, absence: true, if: -> { project&.only_mirror_protected_branches? }.- The sibling endpoint
PUT /projects/:idalready declares the correct pair, atee/lib/ee/api/helpers/projects_helpers.rb(line 48). only_protected_branchesis not an attribute onProjectorProjectSettingat all, so the typo could not have referred to some other real field.
What actually happened before this fix (this corrects wording used in the issue): the issue describes both settings as being "persisted" when both parameters are sent. They were not. Repositories::PullMirrors::UpdateService funnels attributes through Repositories::PullMirrors::Attributes#allowed (ee/app/models/repositories/pull_mirrors/attributes.rb, lines 27-28), which silently forces only_mirror_protected_branches to false whenever mirror_branch_regex is present. So the real prior behavior was 200 OK plus silent, unreported coercion of a setting the caller explicitly asked for. Running the new spec against the unmodified source confirmed this: a request sending only_mirror_protected_branches=true together with a regex returned 200 with a body containing "only_mirror_protected_branches":false,"mirror_branch_regex":"main".
Second impact — a phantom parameter in the published OpenAPI spec: the gitlab-grape-openapi gem reads mutually_exclusive declarations and appends a prose note to each named parameter's description, since OpenAPI 3.0 has no native keyword for cross-parameter exclusion. It trusted the declared (wrong) name, so doc/api/openapi/openapi_v3.yaml advertised a request-body property only_protected_branches with type: string and a description consisting only of the generated note — a parameter this endpoint does not accept and silently ignores. Its type came out as string rather than boolean because Grape records no type for a validator-only name.
What this MR changes (4 files):
ee/lib/api/project_mirror.rb— the one-word fix::only_protected_branchesbecomes:only_mirror_protected_branches.ee/spec/requests/api/project_mirror_spec.rb— two new examples in thePUT /projects/:id/mirror/pullblock: one asserting that sending both parameters returns400with the exact messageonly_mirror_protected_branches, mirror_branch_regex are mutually exclusive, and one asserting that sendingmirror_branch_regexalone still succeeds (guarding against over-rejection). There was no coverage of this constraint before, which is why the typo went unnoticed for nearly two years.doc/api/project_pull_mirroring.md— a version history entry recording the behavior change, plus corrected wording on the two affected attribute rows.doc/api/openapi/openapi_v3.yaml— regenerated withGITLAB_SIMULATE_SAAS=false bundle exec rake gitlab:openapi:v3:generate. Drops the phantom property and retargets the exclusion notes to the correct parameter name.
Behavior change and risk: callers currently sending both parameters together will start receiving 400 Bad Request — this switches on a validation that has been inert for nearly two years. Grape's mutual exclusion triggers on parameter key presence, not value, so sending only_mirror_protected_branches=false together with mirror_branch_regex also now returns 400; callers must omit the key entirely rather than sending false. This is why the documentation wording needed correcting — the old text said mirror_branch_regex "Requires only_mirror_protected_branches to be disabled," which wrongly implied sending false was acceptable. Mitigating factor: this endpoint carries route_setting :lifecycle, :experiment, so it sits outside GA stability guarantees, and the change is recorded in the docs version history rather than staged as a deprecation.
Verification already performed:
- The new
400example was run against the unmodified source first and failed with200, confirming it is a real regression guard, not a tautological test. bundle exec rspec ee/spec/requests/api/project_mirror_spec.rb— 73 examples, 0 failures, 5 pre-existing pending.bundle exec rspec ee/spec/models/repositories/pull_mirrors/attributes_spec.rb ee/spec/services/repositories/pull_mirrors/update_service_spec.rb— 24 examples, 0 failures.GITLAB_SIMULATE_SAAS=false bundle exec rake gitlab:openapi:v3:check_docs— reports "OpenAPI v3 documentation is up to date".bundle exec rubocopon both changed Ruby files — no offenses.- Vale on the changed documentation file — 0 errors, 0 warnings.
References
- Issue being resolved: #627615 (closed)
- The merge request that introduced the typo: !167901 (merged)
- The issue that MR contributed to: #494294 (closed)
How to set up and validate locally
-
Ensure your local GDK has an EE license applied (Premium or Ultimate) — pull mirroring is a licensed feature.
-
Create or use an existing project that can reach a mirror source, and note its project ID and a personal access token.
-
Before applying this MR's fix, confirm the bug: send both parameters together and observe a
200that silently coercesonly_mirror_protected_branchestofalse. -
After applying the fix, run the same request and confirm it now returns
400 Bad Request:curl --request PUT \ --header "PRIVATE-TOKEN: <your_access_token>" \ --url "http://gdk.test:3000/api/v4/projects/<project_id>/mirror/pull" \ --data "only_mirror_protected_branches=true" \ --data "mirror_branch_regex=^release/.*" -
Confirm sending
mirror_branch_regexalone (withoutonly_mirror_protected_branches) still succeeds. -
Run the request specs to exercise both new examples and confirm no regressions:
bundle exec rspec ee/spec/requests/api/project_mirror_spec.rb.
Related to #627615 (closed)