Reject an unrecognized key on POST /features/:name

What does this MR do and why?

POST /api/v4/features/:name accepted any string as key. When value was boolean, an unrecognized or misspelled key was silently ignored: the response was 201 and the flag ended up fully enabled or disabled, rather than rolled out at a percentage, with nothing to indicate the key had not been applied.

This declares the allowed values on the Grape parameter, so an invalid key is rejected at the parameter layer with 400 Bad Request.

Why the validation goes on the parameter rather than in the service

update_global in app/services/admin/set_feature_flag_service.rb evaluates its branches in order, and enable? and disable? are decided purely by value:

# Note: `key` is NOT considered - setting to a percentage to 100 is the same
def enable?
  value.in?(%w[100 100.0 true])
end

With value=true the first branch matches and key is never read, whatever it contains. That ordering is deliberate, and the comments say so: a percentage of 0 is the same as disabling and 100 the same as enabling. Rejecting invalid values at the parameter layer leaves that intact and only refuses input that could never have been honoured.

The failure was value-dependent, which is why it went unnoticed. With a non-boolean value such as value=50, the first two branches do not match and an unrecognized key already fell through to UnknownOperationError. Only the enable and disable paths were silent.

This is a behaviour change

A request sending an unrecognized key receives 400 where it previously received 201. Any client relying on that would break, although it was already getting the wrong rollout silently. The endpoint is admin only (before { authenticated_as_admin! }), which bounds the exposure. This was raised in the issue discussion and judged acceptable on that basis, but it is worth a reviewer's explicit agreement.

The two valid values were already documented in doc/api/features.md, so the documentation needs no change. The documentation was correct and the implementation did not match it.

References

Closes #629413

Approach agreed in the issue discussion by https://gitlab.com/vwolanyk and https://gitlab.com/abignotti, who both preferred declaring the values on the Grape parameter over a change inside the service.

Screenshots or screen recordings

No UI change. The client-visible change is the status code and error body for an invalid key:

POST /api/v4/features/my_flag  value=true  key=percentage_of_actor

Before: 201 Created, flag fully enabled, key silently dropped
After:  400 Bad Request, "key does not have a valid value"

How to set up and validate locally

  1. As an administrator, send POST /api/v4/features/my_flag with value=true and key=percentage_of_actor, a misspelling of percentage_of_actors.
  2. Confirm the response is 400 and the flag is untouched.
  3. Repeat with key=percentage_of_actors and confirm the rollout still applies as before.

Specs, run locally and passing:

bundle exec rspec spec/requests/api/features_spec.rb
# 67 examples, 0 failures, 8 pending

The 8 pending are pre-existing skips in shared granular-token examples. The new coverage asserts both the boolean and the percentage paths, and that the flag is left untouched rather than merely that an error is returned.

MR acceptance checklist

Evaluate this MR against the MR acceptance checklist.

Merge request reports

Loading
Loading