Commit 0a5e840b authored by Stan Hu's avatar Stan Hu

Merge branch 'mc/fix/project-variables-scope-ee' into 'master'

EE Resolve "Project variables validate without any scopes disregarding environment_scope"

See merge request gitlab-org/gitlab-ee!4510
parents 02c15350 75206afb
......@@ -266,7 +266,7 @@ class Project < ActiveRecord::Base
validates :repository_storage,
presence: true,
inclusion: { in: ->(_object) { Gitlab.config.repositories.storages.keys } }
validates :variables, variable_duplicates: true
validates :variables, variable_duplicates: { scope: :environment_scope }
has_many :uploads, as: :model, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent
......
# VariableDuplicatesValidator
#
# This validtor is designed for especially the following condition
# This validator is designed for especially the following condition
# - Use `accepts_nested_attributes_for :xxx` in a parent model
# - Use `validates :xxx, uniqueness: { scope: :xxx_id }` in a child model
class VariableDuplicatesValidator < ActiveModel::EachValidator
def validate_each(record, attribute, value)
duplicates = value.reject(&:marked_for_destruction?).group_by(&:key).select { |_, v| v.many? }.map(&:first)
if options[:scope]
scoped = value.group_by do |variable|
Array(options[:scope]).map { |attr| variable.send(attr) } # rubocop:disable GitlabSecurity/PublicSend
end
scoped.each_value { |scope| validate_duplicates(record, attribute, scope) }
else
validate_duplicates(record, attribute, value)
end
end
private
def validate_duplicates(record, attribute, values)
duplicates = values.reject(&:marked_for_destruction?).group_by(&:key).select { |_, v| v.many? }.map(&:first)
if duplicates.any?
record.errors.add(attribute, "Duplicate variables: #{duplicates.join(", ")}")
error_message = "have duplicate values (#{duplicates.join(", ")})"
error_message += " for #{values.first.send(options[:scope])} scope" if options[:scope] # rubocop:disable GitlabSecurity/PublicSend
record.errors.add(attribute, error_message)
end
end
end
---
title: Fix validation of environment scope of variables
merge_request:
author:
type: fixed
......@@ -19,6 +19,32 @@ describe Project do
it { is_expected.to have_many(:audit_events).dependent(false) }
end
describe 'validations' do
let(:project) { build(:project) }
describe 'variables' do
let(:first_variable) { build(:ci_variable, key: 'test_key', value: 'first', environment_scope: 'prod', project: project) }
let(:second_variable) { build(:ci_variable, key: 'test_key', value: 'other', environment_scope: 'other', project: project) }
before do
project.variables << first_variable
project.variables << second_variable
end
context 'with duplicate variables with same environment scope' do
before do
project.variables.last.environment_scope = project.variables.first.environment_scope
end
it { expect(project).not_to be_valid }
end
context 'with same variable keys and different environment scope' do
it { expect(project).to be_valid }
end
end
end
describe '.mirrors_to_sync' do
let(:timestamp) { Time.now }
......
require 'spec_helper'
describe Projects::VariablesController do
let(:project) { create(:project) }
let(:user) { create(:user) }
before do
sign_in(user)
project.add_master(user)
allow_any_instance_of(License).to receive(:feature_available?).and_call_original
allow_any_instance_of(License).to receive(:feature_available?).with(:variable_environment_scope).and_return(true)
end
describe 'PATCH #update' do
let!(:variable) { create(:ci_variable, project: project, environment_scope: 'custom_scope') }
let(:owner) { project }
let(:variable_attributes) do
{ id: variable.id,
key: variable.key,
value: variable.value,
protected: variable.protected?.to_s,
environment_scope: variable.environment_scope }
end
let(:new_variable_attributes) do
{ key: 'new_key',
value: 'dummy_value',
protected: 'false',
environment_scope: 'new_scope' }
end
subject do
patch :update,
namespace_id: project.namespace.to_param,
project_id: project,
variables_attributes: variables_attributes,
format: :json
end
context 'with same key and different environment scope' do
let(:variables_attributes) do
[
variable_attributes,
new_variable_attributes.merge(key: variable.key)
]
end
it 'does not update the existing variable' do
expect { subject }.not_to change { variable.reload.value }
end
it 'creates the new variable' do
expect { subject }.to change { owner.variables.count }.by(1)
end
it 'returns a successful response' do
subject
expect(response).to have_gitlab_http_status(:ok)
end
it 'has all variables in response' do
subject
expect(response).to match_response_schema('variables')
end
end
context 'with same key and same environment scope' do
let(:variables_attributes) do
[
variable_attributes,
new_variable_attributes.merge(key: variable.key, environment_scope: variable.environment_scope)
]
end
it 'does not update the existing variable' do
expect { subject }.not_to change { variable.reload.value }
end
it 'does not create the new variable' do
expect { subject }.not_to change { owner.variables.count }
end
it 'returns a bad request response' do
subject
expect(response).to have_gitlab_http_status(:bad_request)
end
end
end
end
......@@ -10,7 +10,8 @@
"id": { "type": "integer" },
"key": { "type": "string" },
"value": { "type": "string" },
"protected": { "type": "boolean" }
"protected": { "type": "boolean" },
"environment_scope": { "type": "string", "optional": true }
},
"additionalProperties": false
}
......@@ -263,7 +263,7 @@ shared_examples 'variable list' do
# We check the first row because it re-sorts to alphabetical order on refresh
page.within('.js-ci-variable-list-section') do
expect(find('.js-ci-variable-error-box')).to have_content('Validation failed Variables Duplicate variables: samekey')
expect(find('.js-ci-variable-error-box')).to have_content(/Validation failed Variables have duplicate values \(.+\)/)
end
end
end
require 'spec_helper'
describe VariableDuplicatesValidator do
let(:validator) { described_class.new(attributes: [:variables], **options) }
describe '#validate_each' do
let(:project) { build(:project) }
subject { validator.validate_each(project, :variables, project.variables) }
context 'with no scope' do
let(:options) { {} }
let(:variables) { build_list(:ci_variable, 2, project: project) }
before do
project.variables << variables
end
it 'does not have any errors' do
subject
expect(project.errors.empty?).to be true
end
context 'with duplicates' do
before do
project.variables.build(key: variables.first.key, value: 'dummy_value')
end
it 'has a duplicate key error' do
subject
expect(project.errors).to have_key(:variables)
end
end
end
context 'with a scope attribute' do
let(:options) { { scope: :environment_scope } }
let(:first_variable) { build(:ci_variable, key: 'test_key', environment_scope: '*', project: project) }
let(:second_variable) { build(:ci_variable, key: 'test_key', environment_scope: 'prod', project: project) }
before do
project.variables << first_variable
project.variables << second_variable
end
it 'does not have any errors' do
subject
expect(project.errors.empty?).to be true
end
context 'with duplicates' do
before do
project.variables.build(key: second_variable.key, value: 'dummy_value', environment_scope: second_variable.environment_scope)
end
it 'has a duplicate key error' do
subject
expect(project.errors).to have_key(:variables)
end
end
end
end
end
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment