Merge branch '9912-api-env-variables-update-with-env-scope' into 'master'

Add environment_scope filter to ci-variables API

See merge request gitlab-org/gitlab!34490

app/finders/ci/variables_finder.rb

+# frozen_string_literal: true
+
+module Ci
+  class VariablesFinder
+    attr_reader :project, :params
+
+    def initialize(project, params)
+      @project, @params = project, params
+
+      raise ArgumentError, 'Please provide params[:key]' if params[:key].blank?
+    end
+
+    def execute
+      variables = project.variables
+      variables = by_key(variables)
+      variables = by_environment_scope(variables)
+      variables
+    end
+
+    private
+
+    def by_key(variables)
+      variables.by_key(params[:key])
+    end
+
+    def by_environment_scope(variables)
+      environment_scope = params.dig(:filter, :environment_scope)
+      environment_scope.present? ? variables.by_environment_scope(environment_scope) : variables
+    end
+  end
+end

app/models/ci/variable.rb

@@ -18,5 +18,7 @@ module Ci
     }
 
     scope :unprotected, -> { where(protected: false) }
+    scope :by_key, -> (key) { where(key: key) }
+    scope :by_environment_scope, -> (environment_scope) { where(environment_scope: environment_scope) }
   end
 end

changelogs/unreleased/9912-api-env-variables-update-with-env-scope.yml

+---
+title: Add environment_scope filter to ci-variables API
+merge_request: 34490
+author:
+type: fixed

doc/api/project_level_variables.md

@@ -43,6 +43,7 @@ GET /projects/:id/variables/:key
 |-----------|---------|----------|-----------------------|
 | `id`      | integer/string | yes      | The ID of a project or [urlencoded NAMESPACE/PROJECT_NAME of the project](README.md#namespaced-path-encoding) owned by the authenticated user   |
 | `key`     | string  | yes      | The `key` of a variable |
+| `filter`  | hash    | no       | Available filters: `[environment_scope]`. See the [`filter` parameter details](#the-filter-parameter). |
 
 ```shell
 curl --header "PRIVATE-TOKEN: <your_access_token>" "https://gitlab.example.com/api/v4/projects/1/variables/TEST_VARIABLE_1"
@@ -108,6 +109,7 @@ PUT /projects/:id/variables/:key
 | `protected`         | boolean | no       | Whether the variable is protected |
 | `masked`            | boolean | no       | Whether the variable is masked |
 | `environment_scope` | string  | no       | The `environment_scope` of the variable |
+| `filter`            | hash    | no       | Available filters: `[environment_scope]`. See the [`filter` parameter details](#the-filter-parameter). |
 
 ```shell
 curl --request PUT --header "PRIVATE-TOKEN: <your_access_token>" "https://gitlab.example.com/api/v4/projects/1/variables/NEW_VARIABLE" --form "value=updated value"
@@ -136,7 +138,40 @@ DELETE /projects/:id/variables/:key
 |-----------|---------|----------|-------------------------|
 | `id`      | integer/string | yes      | The ID of a project or [urlencoded NAMESPACE/PROJECT_NAME of the project](README.md#namespaced-path-encoding) owned by the authenticated user     |
 | `key`     | string  | yes      | The `key` of a variable |
+| `filter`  | hash    | no       | Available filters: `[environment_scope]`. See the [`filter` parameter details](#the-filter-parameter). |
 
 ```shell
 curl --request DELETE --header "PRIVATE-TOKEN: <your_access_token>" "https://gitlab.example.com/api/v4/projects/1/variables/VARIABLE_1"
 ```
+
+## The `filter` parameter
+
+> - [Introduced](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/34490) in GitLab 13.2.
+> - It's deployed behind a feature flag, disabled by default.
+> - It's disabled on GitLab.com.
+> - To use it in GitLab self-managed instances, ask a GitLab administrator to enable it.
+
+This parameter is used for filtering by attributes, such as `environment_scope`.
+
+Example usage:
+
+```shell
+curl --request DELETE --header "PRIVATE-TOKEN: <your_access_token>" "https://gitlab.example.com/api/v4/projects/1/variables/VARIABLE_1?filter[environment_scope]=production"
+```
+
+### Enable or disable
+
+[GitLab administrators with access to the GitLab Rails console](../administration/feature_flags.md)
+can enable it for your instance.
+
+To enable it:
+
+```ruby
+Feature.enable(:ci_variables_api_filter_environment_scope)
+```
+
+To disable it:
+
+```ruby
+Feature.disable(:ci_variables_api_filter_environment_scope)
+```

lib/api/variables.rb

@@ -13,6 +13,15 @@ module API
         # parameters, without having to modify the source code directly.
         params
       end
+
+      def find_variable(params)
+        variables = ::Ci::VariablesFinder.new(user_project, params).execute.to_a
+
+        return variables.first unless ::Gitlab::Ci::Features.variables_api_filter_environment_scope?
+        return variables.first unless variables.many? # rubocop: disable CodeReuse/ActiveRecord
+
+        conflict!("There are multiple variables with provided parameters. Please use 'filter[environment_scope]'")
+      end
     end
 
     params do
@@ -39,10 +48,8 @@ module API
       end
       # rubocop: disable CodeReuse/ActiveRecord
       get ':id/variables/:key' do
-        key = params[:key]
-        variable = user_project.variables.find_by(key: key)
-
-        break not_found!('Variable') unless variable
+        variable = find_variable(params)
+        not_found!('Variable') unless variable
 
         present variable, with: Entities::Variable
       end
@@ -82,14 +89,14 @@ module API
         optional :masked, type: Boolean, desc: 'Whether the variable is masked'
         optional :variable_type, type: String, values: ::Ci::Variable.variable_types.keys, desc: 'The type of variable, must be one of env_var or file'
         optional :environment_scope, type: String, desc: 'The environment_scope of the variable'
+        optional :filter, type: Hash, desc: 'Available filters: [environment_scope]. Example: filter[environment_scope]=production'
       end
       # rubocop: disable CodeReuse/ActiveRecord
       put ':id/variables/:key' do
-        variable = user_project.variables.find_by(key: params[:key])
-
-        break not_found!('Variable') unless variable
+        variable = find_variable(params)
+        not_found!('Variable') unless variable
 
-        variable_params = declared_params(include_missing: false).except(:key)
+        variable_params = declared_params(include_missing: false).except(:key, :filter)
         variable_params = filter_variable_parameters(variable_params)
 
         if variable.update(variable_params)
@@ -105,10 +112,11 @@ module API
       end
       params do
         requires :key, type: String, desc: 'The key of the variable'
+        optional :filter, type: Hash, desc: 'Available filters: [environment_scope]. Example: filter[environment_scope]=production'
       end
       # rubocop: disable CodeReuse/ActiveRecord
       delete ':id/variables/:key' do
-        variable = user_project.variables.find_by(key: params[:key])
+        variable = find_variable(params)
         not_found!('Variable') unless variable
 
         # Variables don't have a timestamp. Therefore, destroy unconditionally.

lib/gitlab/ci/features.rb

@@ -50,6 +50,11 @@ module Gitlab
       def self.store_pipeline_messages?(project)
         ::Feature.enabled?(:ci_store_pipeline_messages, project, default_enabled: true)
       end
+
+      # Remove in https://gitlab.com/gitlab-org/gitlab/-/issues/227052
+      def self.variables_api_filter_environment_scope?
+        ::Feature.enabled?(:ci_variables_api_filter_environment_scope, default_enabled: false)
+      end
     end
   end
 end

spec/finders/ci/variables_finder_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Ci::VariablesFinder do
+  let!(:project) { create(:project) }
+  let!(:params) { {} }
+
+  let!(:var1) { create(:ci_variable, project: project, key: 'key1', environment_scope: 'staging') }
+  let!(:var2) { create(:ci_variable, project: project, key: 'key2', environment_scope: 'staging') }
+  let!(:var3) { create(:ci_variable, project: project, key: 'key2', environment_scope: 'production') }
+
+  describe '#initialize' do
+    subject { described_class.new(project, params) }
+
+    context 'without key filter' do
+      let!(:params) { {} }
+
+      it 'raises an error' do
+        expect { subject }.to raise_error(ArgumentError, 'Please provide params[:key]')
+      end
+    end
+  end
+
+  describe '#execute' do
+    subject { described_class.new(project.reload, params).execute }
+
+    context 'with key filter' do
+      let!(:params) { { key: 'key1' } }
+
+      it 'returns var1' do
+        expect(subject).to contain_exactly(var1)
+      end
+    end
+
+    context 'with key and environment_scope filter' do
+      let!(:params) { { key: 'key2', filter: { environment_scope: 'staging' } } }
+
+      it 'returns var2' do
+        expect(subject).to contain_exactly(var2)
+      end
+    end
+  end
+end

spec/requests/api/variables_spec.rb

@@ -54,6 +54,59 @@ RSpec.describe API::Variables do
 
         expect(response).to have_gitlab_http_status(:not_found)
       end
+
+      context 'when there are two variables with the same key on different env' do
+        let!(:var1) { create(:ci_variable, project: project, key: 'key1', environment_scope: 'staging') }
+        let!(:var2) { create(:ci_variable, project: project, key: 'key1', environment_scope: 'production') }
+
+        context 'when filter[environment_scope] is not passed' do
+          context 'FF ci_variables_api_filter_environment_scope is enabled' do
+            it 'returns 409' do
+              get api("/projects/#{project.id}/variables/key1", user)
+
+              expect(response).to have_gitlab_http_status(:conflict)
+            end
+          end
+
+          context 'FF ci_variables_api_filter_environment_scope is disabled' do
+            before do
+              stub_feature_flags(ci_variables_api_filter_environment_scope: false)
+            end
+
+            it 'returns random one' do
+              get api("/projects/#{project.id}/variables/key1", user)
+
+              expect(response).to have_gitlab_http_status(:ok)
+              expect(json_response['key']).to eq('key1')
+            end
+          end
+        end
+
+        context 'when filter[environment_scope] is passed' do
+          it 'returns the variable' do
+            get api("/projects/#{project.id}/variables/key1", user), params: { 'filter[environment_scope]': 'production' }
+
+            expect(response).to have_gitlab_http_status(:ok)
+            expect(json_response['value']).to eq(var2.value)
+          end
+        end
+
+        context 'when wrong filter[environment_scope] is passed' do
+          it 'returns not_found' do
+            get api("/projects/#{project.id}/variables/key1", user), params: { 'filter[environment_scope]': 'invalid' }
+
+            expect(response).to have_gitlab_http_status(:not_found)
+          end
+
+          context 'when there is only one variable with provided key' do
+            it 'returns not_found' do
+              get api("/projects/#{project.id}/variables/#{variable.key}", user), params: { 'filter[environment_scope]': 'invalid' }
+
+              expect(response).to have_gitlab_http_status(:not_found)
+            end
+          end
+        end
+      end
     end
 
     context 'authorized user with invalid permissions' do
@@ -173,6 +226,52 @@ RSpec.describe API::Variables do
 
         expect(response).to have_gitlab_http_status(:not_found)
       end
+
+      context 'when there are two variables with the same key on different env' do
+        let!(:var1) { create(:ci_variable, project: project, key: 'key1', environment_scope: 'staging') }
+        let!(:var2) { create(:ci_variable, project: project, key: 'key1', environment_scope: 'production') }
+
+        context 'when filter[environment_scope] is not passed' do
+          context 'FF ci_variables_api_filter_environment_scope is enabled' do
+            it 'returns 409' do
+              get api("/projects/#{project.id}/variables/key1", user)
+
+              expect(response).to have_gitlab_http_status(:conflict)
+            end
+          end
+
+          context 'FF ci_variables_api_filter_environment_scope is disabled' do
+            before do
+              stub_feature_flags(ci_variables_api_filter_environment_scope: false)
+            end
+
+            it 'updates random one' do
+              put api("/projects/#{project.id}/variables/key1", user), params: { value: 'new_val' }
+
+              expect(response).to have_gitlab_http_status(:ok)
+              expect(json_response['value']).to eq('new_val')
+            end
+          end
+        end
+
+        context 'when filter[environment_scope] is passed' do
+          it 'updates the variable' do
+            put api("/projects/#{project.id}/variables/key1", user), params: { value: 'new_val', 'filter[environment_scope]': 'production' }
+
+            expect(response).to have_gitlab_http_status(:ok)
+            expect(var1.reload.value).not_to eq('new_val')
+            expect(var2.reload.value).to eq('new_val')
+          end
+        end
+
+        context 'when wrong filter[environment_scope] is passed' do
+          it 'returns not_found' do
+            put api("/projects/#{project.id}/variables/key1", user), params: { value: 'new_val', 'filter[environment_scope]': 'invalid' }
+
+            expect(response).to have_gitlab_http_status(:not_found)
+          end
+        end
+      end
     end
 
     context 'authorized user with invalid permissions' do
@@ -207,6 +306,56 @@ RSpec.describe API::Variables do
 
         expect(response).to have_gitlab_http_status(:not_found)
       end
+
+      context 'when there are two variables with the same key on different env' do
+        let!(:var1) { create(:ci_variable, project: project, key: 'key1', environment_scope: 'staging') }
+        let!(:var2) { create(:ci_variable, project: project, key: 'key1', environment_scope: 'production') }
+
+        context 'when filter[environment_scope] is not passed' do
+          context 'FF ci_variables_api_filter_environment_scope is enabled' do
+            it 'returns 409' do
+              get api("/projects/#{project.id}/variables/key1", user)
+
+              expect(response).to have_gitlab_http_status(:conflict)
+            end
+          end
+
+          context 'FF ci_variables_api_filter_environment_scope is disabled' do
+            before do
+              stub_feature_flags(ci_variables_api_filter_environment_scope: false)
+            end
+
+            it 'deletes random one' do
+              expect do
+                delete api("/projects/#{project.id}/variables/key1", user), params: { 'filter[environment_scope]': 'production' }
+
+                expect(response).to have_gitlab_http_status(:no_content)
+              end.to change {project.variables.count}.by(-1)
+            end
+          end
+        end
+
+        context 'when filter[environment_scope] is passed' do
+          it 'deletes the variable' do
+            expect do
+              delete api("/projects/#{project.id}/variables/key1", user), params: { 'filter[environment_scope]': 'production' }
+
+              expect(response).to have_gitlab_http_status(:no_content)
+            end.to change {project.variables.count}.by(-1)
+
+            expect(var1.reload).to be_present
+            expect { var2.reload }.to raise_error(ActiveRecord::RecordNotFound)
+          end
+        end
+
+        context 'when wrong filter[environment_scope] is passed' do
+          it 'returns not_found' do
+            delete api("/projects/#{project.id}/variables/key1", user), params: { 'filter[environment_scope]': 'invalid' }
+
+            expect(response).to have_gitlab_http_status(:not_found)
+          end
+        end
+      end
     end
 
     context 'authorized user with invalid permissions' do