From 00b608d79ecd293ac52f62253d51fe75c6c85216 Mon Sep 17 00:00:00 2001 From: Luke Duncalfe Date: Fri, 18 Jul 2025 10:42:15 +1200 Subject: [PATCH 1/6] Shift some validations to BuiltInTool --- ee/app/models/ai/catalog/built_in_tool.rb | 15 ++++- .../catalog/built_in_tool_definitions_spec.rb | 39 ++----------- .../models/ai/catalog/built_in_tool_spec.rb | 55 +++++++++++++++++++ 3 files changed, 73 insertions(+), 36 deletions(-) diff --git a/ee/app/models/ai/catalog/built_in_tool.rb b/ee/app/models/ai/catalog/built_in_tool.rb index ad4c600ddfaa2a..dcbf04f3983842 100644 --- a/ee/app/models/ai/catalog/built_in_tool.rb +++ b/ee/app/models/ai/catalog/built_in_tool.rb @@ -12,11 +12,20 @@ class BuiltInTool attribute :description, :string validates :name, :title, :description, presence: true + validate :validate_unique_name class << self - def count - all.size - end + delegate :count, :size, to: :all + end + + private + + def validate_unique_name + return unless name.present? + + return unless self.class.all.count { |item| item.name == name } > 1 + + errors.add(:name, 'must be unique') end end end diff --git a/ee/spec/lib/ai/catalog/built_in_tool_definitions_spec.rb b/ee/spec/lib/ai/catalog/built_in_tool_definitions_spec.rb index 67b9d32a40745c..abeb758f3208ca 100644 --- a/ee/spec/lib/ai/catalog/built_in_tool_definitions_spec.rb +++ b/ee/spec/lib/ai/catalog/built_in_tool_definitions_spec.rb @@ -30,41 +30,14 @@ expect(items).not_to be_empty end - describe 'item structure' do - it 'has required keys for each item' do - expect(items).to all(include(:id, :name, :title, :description)) - end - - it 'has unique IDs' do - ids = items.pluck(:id) - expect(ids.uniq.size).to eq(ids.size) - end - - it 'has unique names' do - names = items.pluck(:name) - expect(names.uniq.size).to eq(names.size) - end - - it 'has sequential IDs starting from 1' do - ids = items.pluck(:id).sort - expect(ids).to eq((1..items.size).to_a) - end + it 'has sequential IDs starting from 1' do + ids = items.pluck(:id).sort + expect(ids).to eq((1..items.size).to_a) end - describe 'data validation' do - it 'has non-empty strings for all required fields' do - items.each do |item| - expect(item[:id]).to be_a(Integer).and be_present - expect(item[:name]).to be_a(String).and be_present - expect(item[:title]).to be_a(String).and be_present - expect(item[:description]).to be_a(String).and be_present - end - end - - it 'has positive integer IDs' do - items.each do |item| - expect(item[:id]).to be_a(Integer).and be_positive - end + it 'has positive integer IDs' do + items.each do |item| + expect(item[:id]).to be_a(Integer).and be_positive end end end diff --git a/ee/spec/models/ai/catalog/built_in_tool_spec.rb b/ee/spec/models/ai/catalog/built_in_tool_spec.rb index 3d4e750c784658..aaa91b79bdabf8 100644 --- a/ee/spec/models/ai/catalog/built_in_tool_spec.rb +++ b/ee/spec/models/ai/catalog/built_in_tool_spec.rb @@ -11,10 +11,65 @@ it { is_expected.to include(Ai::Catalog::BuiltInToolDefinitions) } end + it 'is expected to all be valid' do + expect(described_class.all).to all(be_valid) + end + describe 'validations' do it { is_expected.to validate_presence_of(:name) } it { is_expected.to validate_presence_of(:title) } it { is_expected.to validate_presence_of(:description) } + + it { is_expected.to allow_values(1, 999).for(:id) } + it { is_expected.not_to allow_values(-1, 0).for(:id) } + + describe 'custom validations' do + let(:other_tool_attributes) do + { + id: 1, + name: 'first_tool', + title: 'Test Tool', + description: 'A test tool description' + } + end + + let(:attributes) do + { + id: 2, + name: 'second_tool', + title: 'Test Tool', + description: 'A test tool description' + } + end + + before do + allow(described_class).to receive(:all).and_return( + [described_class.new(other_tool_attributes), tool] + ) + end + + subject(:tool) { described_class.new(attributes) } + + it { is_expected.to be_valid } + + describe '#validate_unique_id' do + let(:attributes) { super().merge(id: other_tool_attributes[:id]) } + + it 'is expected to add an error for duplicate ID' do + expect(tool).to be_invalid + expect(tool.errors[:id]).to include('must be unique') + end + end + + describe '#validate_unique_name' do + let(:attributes) { super().merge(name: other_tool_attributes[:name]) } + + it 'is expected to add an error for duplicate name' do + expect(tool).to be_invalid + expect(tool.errors[:name]).to include('must be unique') + end + end + end end describe '.count' do -- GitLab From 1ca846e626e687cc1a8812e008f7d7fc335f5315 Mon Sep 17 00:00:00 2001 From: Imjaydip Date: Fri, 18 Jul 2025 12:44:41 +0530 Subject: [PATCH 2/6] add common validation method for unique attribute --- ee/app/models/ai/catalog/built_in_tool.rb | 13 ++++++------- ee/spec/models/ai/catalog/built_in_tool_spec.rb | 11 +---------- 2 files changed, 7 insertions(+), 17 deletions(-) diff --git a/ee/app/models/ai/catalog/built_in_tool.rb b/ee/app/models/ai/catalog/built_in_tool.rb index dcbf04f3983842..4bc2387a33c0d6 100644 --- a/ee/app/models/ai/catalog/built_in_tool.rb +++ b/ee/app/models/ai/catalog/built_in_tool.rb @@ -12,20 +12,19 @@ class BuiltInTool attribute :description, :string validates :name, :title, :description, presence: true - validate :validate_unique_name + validate -> { validate_unique_attribute(:name) } class << self delegate :count, :size, to: :all end - private + def validate_unique_attribute(attribute) + attribute_value = read_attribute(attribute) + return unless attribute_value.present? - def validate_unique_name - return unless name.present? + return unless self.class.all.count { |item| item.read_attribute(attribute) == attribute_value } > 1 - return unless self.class.all.count { |item| item.name == name } > 1 - - errors.add(:name, 'must be unique') + errors.add(attribute, 'must be unique') end end end diff --git a/ee/spec/models/ai/catalog/built_in_tool_spec.rb b/ee/spec/models/ai/catalog/built_in_tool_spec.rb index aaa91b79bdabf8..b89e3fd020c394 100644 --- a/ee/spec/models/ai/catalog/built_in_tool_spec.rb +++ b/ee/spec/models/ai/catalog/built_in_tool_spec.rb @@ -52,7 +52,7 @@ it { is_expected.to be_valid } - describe '#validate_unique_id' do + describe '#validate_unique_attribute' do let(:attributes) { super().merge(id: other_tool_attributes[:id]) } it 'is expected to add an error for duplicate ID' do @@ -60,15 +60,6 @@ expect(tool.errors[:id]).to include('must be unique') end end - - describe '#validate_unique_name' do - let(:attributes) { super().merge(name: other_tool_attributes[:name]) } - - it 'is expected to add an error for duplicate name' do - expect(tool).to be_invalid - expect(tool.errors[:name]).to include('must be unique') - end - end end end -- GitLab From e921833242548a9e9eaf4e4bb402c42930608698 Mon Sep 17 00:00:00 2001 From: Luke Duncalfe Date: Mon, 21 Jul 2025 15:39:37 +1200 Subject: [PATCH 3/6] Validate id and name in one loop, add test --- ee/app/models/ai/catalog/built_in_tool.rb | 15 +++++++++------ .../models/ai/catalog/built_in_tool_spec.rb | 19 +++++++++++++++---- 2 files changed, 24 insertions(+), 10 deletions(-) diff --git a/ee/app/models/ai/catalog/built_in_tool.rb b/ee/app/models/ai/catalog/built_in_tool.rb index 4bc2387a33c0d6..e872f37cb2bef6 100644 --- a/ee/app/models/ai/catalog/built_in_tool.rb +++ b/ee/app/models/ai/catalog/built_in_tool.rb @@ -12,19 +12,22 @@ class BuiltInTool attribute :description, :string validates :name, :title, :description, presence: true - validate -> { validate_unique_attribute(:name) } + validate :validate_unique_name class << self delegate :count, :size, to: :all end - def validate_unique_attribute(attribute) - attribute_value = read_attribute(attribute) - return unless attribute_value.present? + private - return unless self.class.all.count { |item| item.read_attribute(attribute) == attribute_value } > 1 + def validate_unique_name + name_count = 0 - errors.add(attribute, 'must be unique') + self.class.all.each do |item| # rubocop:disable Rails/FindEach -- not an ActiveRecord model + name_count += 1 if item.read_attribute(:name) == name + end + + errors.add(:name, 'must be unique') if name_count > 1 end end end diff --git a/ee/spec/models/ai/catalog/built_in_tool_spec.rb b/ee/spec/models/ai/catalog/built_in_tool_spec.rb index b89e3fd020c394..62d117ee54b9ad 100644 --- a/ee/spec/models/ai/catalog/built_in_tool_spec.rb +++ b/ee/spec/models/ai/catalog/built_in_tool_spec.rb @@ -53,11 +53,22 @@ it { is_expected.to be_valid } describe '#validate_unique_attribute' do - let(:attributes) { super().merge(id: other_tool_attributes[:id]) } + context 'on id' do + let(:attributes) { super().merge(id: other_tool_attributes[:id]) } - it 'is expected to add an error for duplicate ID' do - expect(tool).to be_invalid - expect(tool.errors[:id]).to include('must be unique') + it 'is expected to add an error for duplicate ID' do + expect(tool).to be_invalid + expect(tool.errors[:id]).to include('must be unique') + end + end + + context 'on name' do + let(:attributes) { super().merge(name: other_tool_attributes[:name]) } + + it 'is expected to add an error for duplicate Name' do + expect(tool).to be_invalid + expect(tool.errors[:name]).to include('must be unique') + end end end end -- GitLab From 1e8e55e4726164f9da361e85212df498b8cd9a27 Mon Sep 17 00:00:00 2001 From: Luke Duncalfe Date: Tue, 22 Jul 2025 16:04:25 +1200 Subject: [PATCH 4/6] Add reviewer feedback --- ee/app/models/ai/catalog/built_in_tool.rb | 14 +------- .../models/ai/catalog/built_in_tool_spec.rb | 2 +- .../lib/active_record/fixed_items_model.rb | 1 + .../validators/uniqueness_validator.rb | 15 ++++++++ .../validators/uniqueness_validator_spec.rb | 36 +++++++++++++++++++ 5 files changed, 54 insertions(+), 14 deletions(-) create mode 100644 gems/activerecord-gitlab/lib/active_record/fixed_items_model/validators/uniqueness_validator.rb create mode 100644 gems/activerecord-gitlab/spec/active_record/fixed_items_model/validators/uniqueness_validator_spec.rb diff --git a/ee/app/models/ai/catalog/built_in_tool.rb b/ee/app/models/ai/catalog/built_in_tool.rb index e872f37cb2bef6..d38acbded3349d 100644 --- a/ee/app/models/ai/catalog/built_in_tool.rb +++ b/ee/app/models/ai/catalog/built_in_tool.rb @@ -12,23 +12,11 @@ class BuiltInTool attribute :description, :string validates :name, :title, :description, presence: true - validate :validate_unique_name + validates_with ActiveRecord::FixedItemsModel::Validators::UniquenessValidator, attributes: [:name] class << self delegate :count, :size, to: :all end - - private - - def validate_unique_name - name_count = 0 - - self.class.all.each do |item| # rubocop:disable Rails/FindEach -- not an ActiveRecord model - name_count += 1 if item.read_attribute(:name) == name - end - - errors.add(:name, 'must be unique') if name_count > 1 - end end end end diff --git a/ee/spec/models/ai/catalog/built_in_tool_spec.rb b/ee/spec/models/ai/catalog/built_in_tool_spec.rb index 62d117ee54b9ad..588b0c02973795 100644 --- a/ee/spec/models/ai/catalog/built_in_tool_spec.rb +++ b/ee/spec/models/ai/catalog/built_in_tool_spec.rb @@ -52,7 +52,7 @@ it { is_expected.to be_valid } - describe '#validate_unique_attribute' do + describe 'uniqueness validations' do context 'on id' do let(:attributes) { super().merge(id: other_tool_attributes[:id]) } diff --git a/gems/activerecord-gitlab/lib/active_record/fixed_items_model.rb b/gems/activerecord-gitlab/lib/active_record/fixed_items_model.rb index f95ff85a7b71cc..6981421d7225c5 100644 --- a/gems/activerecord-gitlab/lib/active_record/fixed_items_model.rb +++ b/gems/activerecord-gitlab/lib/active_record/fixed_items_model.rb @@ -1,4 +1,5 @@ # frozen_string_literal: true +require_relative "fixed_items_model/validators/uniqueness_validator" require_relative "fixed_items_model/model" require_relative "fixed_items_model/has_one" diff --git a/gems/activerecord-gitlab/lib/active_record/fixed_items_model/validators/uniqueness_validator.rb b/gems/activerecord-gitlab/lib/active_record/fixed_items_model/validators/uniqueness_validator.rb new file mode 100644 index 00000000000000..bd29d69b2c2d36 --- /dev/null +++ b/gems/activerecord-gitlab/lib/active_record/fixed_items_model/validators/uniqueness_validator.rb @@ -0,0 +1,15 @@ +# frozen_string_literal: true + +module ActiveRecord + module FixedItemsModel + module Validators + class UniquenessValidator < ActiveModel::EachValidator + def validate_each(record, attribute, value) + return unless record.class.where(attribute => value).size > 1 + + record.errors.add(attribute, 'must be unique') + end + end + end + end +end diff --git a/gems/activerecord-gitlab/spec/active_record/fixed_items_model/validators/uniqueness_validator_spec.rb b/gems/activerecord-gitlab/spec/active_record/fixed_items_model/validators/uniqueness_validator_spec.rb new file mode 100644 index 00000000000000..64f3b6e44a3502 --- /dev/null +++ b/gems/activerecord-gitlab/spec/active_record/fixed_items_model/validators/uniqueness_validator_spec.rb @@ -0,0 +1,36 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe ActiveRecord::FixedItemsModel::Validators::UniquenessValidator, feature_category: :shared do + let(:test_class) do + Class.new do + include ActiveModel::Model + include ActiveModel::Attributes + include ActiveRecord::FixedItemsModel::Model + + attribute :name, :string + end + end + + let(:record) { test_class.new(name: 'Name') } + let(:validator) { described_class.new(attributes: [:name]) } + + subject { validator.validate_each(record, :name, record.name) } + + it 'does not add any error if value is unique' do + allow(test_class).to receive(:all).and_return([record]) + + subject + + expect(record.errors).to be_empty + end + + it 'adds an error if value is not unique' do + allow(test_class).to receive(:all).and_return([record, test_class.new(name: 'Name')]) + + subject + + expect(record.errors[:name]).to eq(['must be unique']) + end +end -- GitLab From fae4b2592bdba1e4bd26140938c42b1683427310 Mon Sep 17 00:00:00 2001 From: Luke Duncalfe Date: Wed, 30 Jul 2025 12:51:24 +1200 Subject: [PATCH 5/6] Add reviewer feedback --- .../catalog/built_in_tool_definitions_spec.rb | 6 ----- .../models/ai/catalog/built_in_tool_spec.rb | 26 +++++-------------- .../validators/uniqueness_validator.rb | 3 ++- .../validators/uniqueness_validator_spec.rb | 8 ++++++ 4 files changed, 16 insertions(+), 27 deletions(-) diff --git a/ee/spec/lib/ai/catalog/built_in_tool_definitions_spec.rb b/ee/spec/lib/ai/catalog/built_in_tool_definitions_spec.rb index abeb758f3208ca..d419883c08fa87 100644 --- a/ee/spec/lib/ai/catalog/built_in_tool_definitions_spec.rb +++ b/ee/spec/lib/ai/catalog/built_in_tool_definitions_spec.rb @@ -34,11 +34,5 @@ ids = items.pluck(:id).sort expect(ids).to eq((1..items.size).to_a) end - - it 'has positive integer IDs' do - items.each do |item| - expect(item[:id]).to be_a(Integer).and be_positive - end - end end end diff --git a/ee/spec/models/ai/catalog/built_in_tool_spec.rb b/ee/spec/models/ai/catalog/built_in_tool_spec.rb index 588b0c02973795..e63d8961d9efda 100644 --- a/ee/spec/models/ai/catalog/built_in_tool_spec.rb +++ b/ee/spec/models/ai/catalog/built_in_tool_spec.rb @@ -20,10 +20,7 @@ it { is_expected.to validate_presence_of(:title) } it { is_expected.to validate_presence_of(:description) } - it { is_expected.to allow_values(1, 999).for(:id) } - it { is_expected.not_to allow_values(-1, 0).for(:id) } - - describe 'custom validations' do + describe 'name uniqueness validation' do let(:other_tool_attributes) do { id: 1, @@ -52,23 +49,12 @@ it { is_expected.to be_valid } - describe 'uniqueness validations' do - context 'on id' do - let(:attributes) { super().merge(id: other_tool_attributes[:id]) } - - it 'is expected to add an error for duplicate ID' do - expect(tool).to be_invalid - expect(tool.errors[:id]).to include('must be unique') - end - end - - context 'on name' do - let(:attributes) { super().merge(name: other_tool_attributes[:name]) } + context 'when name is not unique' do + let(:attributes) { super().merge(name: other_tool_attributes[:name]) } - it 'is expected to add an error for duplicate Name' do - expect(tool).to be_invalid - expect(tool.errors[:name]).to include('must be unique') - end + it 'is invalid' do + expect(tool).to be_invalid + expect(tool.errors[:name]).to include('must be unique') end end end diff --git a/gems/activerecord-gitlab/lib/active_record/fixed_items_model/validators/uniqueness_validator.rb b/gems/activerecord-gitlab/lib/active_record/fixed_items_model/validators/uniqueness_validator.rb index bd29d69b2c2d36..9276c82c713761 100644 --- a/gems/activerecord-gitlab/lib/active_record/fixed_items_model/validators/uniqueness_validator.rb +++ b/gems/activerecord-gitlab/lib/active_record/fixed_items_model/validators/uniqueness_validator.rb @@ -5,7 +5,8 @@ module FixedItemsModel module Validators class UniquenessValidator < ActiveModel::EachValidator def validate_each(record, attribute, value) - return unless record.class.where(attribute => value).size > 1 + similar = record.class.where(attribute => value) + return if similar.empty? || similar == [record] record.errors.add(attribute, 'must be unique') end diff --git a/gems/activerecord-gitlab/spec/active_record/fixed_items_model/validators/uniqueness_validator_spec.rb b/gems/activerecord-gitlab/spec/active_record/fixed_items_model/validators/uniqueness_validator_spec.rb index 64f3b6e44a3502..8f538e90671971 100644 --- a/gems/activerecord-gitlab/spec/active_record/fixed_items_model/validators/uniqueness_validator_spec.rb +++ b/gems/activerecord-gitlab/spec/active_record/fixed_items_model/validators/uniqueness_validator_spec.rb @@ -26,6 +26,14 @@ expect(record.errors).to be_empty end + it 'does not add any error if there are no matches' do + allow(test_class).to receive(:all).and_return([]) + + subject + + expect(record.errors).to be_empty + end + it 'adds an error if value is not unique' do allow(test_class).to receive(:all).and_return([record, test_class.new(name: 'Name')]) -- GitLab From 362ccb839370dd3805cf7d3fb353770ed570d33a Mon Sep 17 00:00:00 2001 From: Luke Duncalfe Date: Wed, 30 Jul 2025 15:31:50 +1200 Subject: [PATCH 6/6] Split load and validate steps up https://gitlab.com/gitlab-org/gitlab/-/merge_requests/198193#note_2657810082 --- .../active_record/fixed_items_model/model.rb | 28 +++++++++---------- .../fixed_items_model/model_spec.rb | 2 +- 2 files changed, 15 insertions(+), 15 deletions(-) diff --git a/gems/activerecord-gitlab/lib/active_record/fixed_items_model/model.rb b/gems/activerecord-gitlab/lib/active_record/fixed_items_model/model.rb index 88bbc3e9a500e8..aa9ad15292c743 100644 --- a/gems/activerecord-gitlab/lib/active_record/fixed_items_model/model.rb +++ b/gems/activerecord-gitlab/lib/active_record/fixed_items_model/model.rb @@ -35,9 +35,15 @@ module Model class_methods do def all - load_items! if storage.empty? + if storage.empty? + load_items! + validate_items! + end - storage.compact + storage + rescue StandardError + storage.clear + raise end def find(id) @@ -59,22 +65,15 @@ def storage private def load_items! - validate_items_definition! - self::ITEMS.each do |item_definition| - item = new(item_definition) - raise "Static definition in ITEMS is invalid! #{item.errors.full_messages.join(', ')}" unless item.valid? - - storage[item.id] = item + storage << new(item_definition) end end - def validate_items_definition! - unique_ids = self::ITEMS.map { |item| item[:id] }.uniq - - return if unique_ids.size == self::ITEMS.size - - raise "Static definition ITEMS has #{self::ITEMS.size - unique_ids.size} duplicated IDs!" + def validate_items! + storage.each do |item| + raise "Static definition in ITEMS is invalid! #{item.errors.full_messages.join(', ')}" unless item.valid? + end end end @@ -82,6 +81,7 @@ def validate_items_definition! attribute :id, :integer validates :id, numericality: { greater_than: 0, only_integer: true } + validates_with ActiveRecord::FixedItemsModel::Validators::UniquenessValidator, attributes: [:id] end def matches?(conditions) diff --git a/gems/activerecord-gitlab/spec/active_record/fixed_items_model/model_spec.rb b/gems/activerecord-gitlab/spec/active_record/fixed_items_model/model_spec.rb index 6f22abf426025e..154587364d82fc 100644 --- a/gems/activerecord-gitlab/spec/active_record/fixed_items_model/model_spec.rb +++ b/gems/activerecord-gitlab/spec/active_record/fixed_items_model/model_spec.rb @@ -61,7 +61,7 @@ it 'raises an error' do expect do TestStaticModel.all - end.to raise_error("Static definition ITEMS has 2 duplicated IDs!") + end.to raise_error("Static definition in ITEMS is invalid! Id must be unique") end end -- GitLab