Define `states` parameter in Environment API interface
The following discussion from !87023 (merged) should be addressed:
-
@shinya.maeda started a discussion: @ahmed.hemdan Actually, it seems
states
is an undefined parameter in this API interface (Oddly, we already document this behavior). I think this is the reason why we don't see the coverage in the corresponding test file.This works fine because we pass the
params
to the finder, however, we should not allow users to pass random parameters in terms of consistency and security aspect. To fix this problem, we can do:diff --git a/lib/api/environments.rb b/lib/api/environments.rb index 31c8ae73a0c..b4a0c58d7cc 100644 --- a/lib/api/environments.rb +++ b/lib/api/environments.rb @@ -22,12 +22,13 @@ class Environments < ::API::Base use :pagination optional :name, type: String, desc: 'Returns the environment with this name' optional :search, type: String, desc: 'Returns list of environments matching the search criteria' + optional :states, type: Array[String], desc: 'Filter by the environment states' mutually_exclusive :name, :search, message: 'cannot be used together' end get ':id/environments' do authorize! :read_environment, user_project - environments = ::Environments::EnvironmentsFinder.new(user_project, current_user, params).execute + environments = ::Environments::EnvironmentsFinder.new(user_project, current_user, declared_params(include_missing: false)).execute present paginate(environments), with: Entities::Environment, current_user: current_user end
Even better, Grape has the built-in
values
validator, so technically we don't need to catch theInvalidStatesError
exception, like:optional :states, type: Array[String], desc: 'Filter by the environment states', values: Environment.states.keys
This effectively avoid 500. Example.
Since this is not in the original scope, I'll create a follow-up issue to resolve this discussion.