diff --git a/ee/app/models/ai/catalog/built_in_tool.rb b/ee/app/models/ai/catalog/built_in_tool.rb index ad4c600ddfaa2a4e15cdbad5b7a2dd9137120c09..d38acbded3349de3363ca5c1a2dea9e614c16f64 100644 --- a/ee/app/models/ai/catalog/built_in_tool.rb +++ b/ee/app/models/ai/catalog/built_in_tool.rb @@ -12,11 +12,10 @@ class BuiltInTool attribute :description, :string validates :name, :title, :description, presence: true + validates_with ActiveRecord::FixedItemsModel::Validators::UniquenessValidator, attributes: [:name] class << self - def count - all.size - end + delegate :count, :size, to: :all 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 67b9d32a40745c78df0251d13752104239fa3f23..d419883c08fa87a84b8eb65d1a3c4fd4554853bc 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,42 +30,9 @@ 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 - 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 - end + it 'has sequential IDs starting from 1' do + ids = items.pluck(:id).sort + expect(ids).to eq((1..items.size).to_a) 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 3d4e750c7846589b4383ca21ee5b33f33824bf11..e63d8961d9efda3574333ffe0f8f0a0d8b144198 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,53 @@ 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) } + + describe 'name uniqueness validation' 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 } + + context 'when name is not unique' do + let(:attributes) { super().merge(name: other_tool_attributes[:name]) } + + it 'is invalid' do + expect(tool).to be_invalid + expect(tool.errors[:name]).to include('must be unique') + end + end + end end describe '.count' do 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 f95ff85a7b71cc94c48a1094345862b096ffa557..6981421d7225c51ec3d75aae0210766af91d894e 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/model.rb b/gems/activerecord-gitlab/lib/active_record/fixed_items_model/model.rb index 88bbc3e9a500e8ec531f5ab775a2b5d5a92c4f2c..aa9ad15292c7438a59ec1e6ad65f76715eadc097 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/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 0000000000000000000000000000000000000000..9276c82c71376127c5061bcf7243229d686edd63 --- /dev/null +++ b/gems/activerecord-gitlab/lib/active_record/fixed_items_model/validators/uniqueness_validator.rb @@ -0,0 +1,16 @@ +# frozen_string_literal: true + +module ActiveRecord + module FixedItemsModel + module Validators + class UniquenessValidator < ActiveModel::EachValidator + def validate_each(record, attribute, value) + similar = record.class.where(attribute => value) + return if similar.empty? || similar == [record] + + record.errors.add(attribute, 'must be unique') + end + end + end + end +end 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 6f22abf426025e8e16feaaa7c1cab4b4a112fa9e..154587364d82fcd976730965dbd5753ef929d5b9 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 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 0000000000000000000000000000000000000000..8f538e90671971261ab390c203edc21e59f05f5a --- /dev/null +++ b/gems/activerecord-gitlab/spec/active_record/fixed_items_model/validators/uniqueness_validator_spec.rb @@ -0,0 +1,44 @@ +# 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 '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')]) + + subject + + expect(record.errors[:name]).to eq(['must be unique']) + end +end