From 43525a0c5d9229a70659ede8f6f5cd76b79ad4b9 Mon Sep 17 00:00:00 2001 From: Alex Pooley Date: Wed, 10 Sep 2025 13:05:24 +0800 Subject: [PATCH 01/19] Organization Isolation utilities --- lib/gitlab/crawler.rb | 92 ++++ lib/gitlab/database/relationship_mapper.rb | 35 ++ .../reflections/database/foreign_keys.rb | 41 ++ .../reflections/models/active_record.rb | 71 +++ .../relationships/active_record.rb | 106 ++++ lib/gitlab/reflections/relationships/base.rb | 50 ++ .../reflections/relationships/builder.rb | 46 ++ .../reflections/relationships/crawl_logger.rb | 112 +++++ .../reflections/relationships/crawl_tracer.rb | 90 ++++ .../reflections/relationships/crawler.rb | 190 ++++++++ .../reflections/relationships/foreign_key.rb | 72 +++ .../handlers/active_storage_handler.rb | 32 ++ .../relationships/handlers/base_handler.rb | 85 ++++ .../handlers/belongs_to_handler.rb | 45 ++ .../relationships/handlers/habtm_handler.rb | 28 ++ .../handlers/has_association_handler.rb | 81 ++++ .../polymorphic_belongs_to_handler.rb | 90 ++++ .../polymorphic_has_association_handler.rb | 108 +++++ .../reflections/relationships/predicates.rb | 13 + .../relationships/predicates/polymorphic.rb | 24 + .../relationships/predicates/type_based.rb | 18 + .../reflections/relationships/relationship.rb | 214 +++++++++ .../relationships/transformers/deduplicate.rb | 46 ++ .../transformers/expand_polymorphic.rb | 71 +++ .../transformers/filter_tables.rb | 46 ++ .../transformers/invert_relationships.rb | 76 +++ .../transformers/merge_data_sources.rb | 48 ++ .../transformers/normalize_columns.rb | 44 ++ .../relationships/transformers/pipeline.rb | 19 + .../relationships/transformers/validate.rb | 52 ++ lib/tasks/database_relationships.rake | 193 ++++++++ .../reflections/database/foreign_keys_spec.rb | 255 ++++++++++ .../reflections/models/active_record_spec.rb | 452 ++++++++++++++++++ .../polymorphic_edge_cases_spec.rb | 233 +++++++++ .../reflections/polymorphic_json_task_spec.rb | 167 +++++++ .../polymorphic_relationships_spec.rb | 159 ++++++ .../relationships/active_record_spec.rb | 443 +++++++++++++++++ .../reflections/relationships/base_spec.rb | 83 ++++ .../relationships/foreign_key_spec.rb | 392 +++++++++++++++ .../handlers/active_storage_handler_spec.rb | 447 +++++++++++++++++ .../handlers/base_handler_spec.rb | 23 + .../handlers/belongs_to_handler_spec.rb | 39 ++ .../handlers/habtm_handler_spec.rb | 396 +++++++++++++++ .../handlers/has_association_handler_spec.rb | 62 +++ .../polymorphic_belongs_to_handler_spec.rb | 79 +++ ...olymorphic_has_association_handler_spec.rb | 79 +++ 46 files changed, 5547 insertions(+) create mode 100644 lib/gitlab/crawler.rb create mode 100644 lib/gitlab/database/relationship_mapper.rb create mode 100644 lib/gitlab/reflections/database/foreign_keys.rb create mode 100644 lib/gitlab/reflections/models/active_record.rb create mode 100644 lib/gitlab/reflections/relationships/active_record.rb create mode 100644 lib/gitlab/reflections/relationships/base.rb create mode 100644 lib/gitlab/reflections/relationships/builder.rb create mode 100644 lib/gitlab/reflections/relationships/crawl_logger.rb create mode 100644 lib/gitlab/reflections/relationships/crawl_tracer.rb create mode 100644 lib/gitlab/reflections/relationships/crawler.rb create mode 100644 lib/gitlab/reflections/relationships/foreign_key.rb create mode 100644 lib/gitlab/reflections/relationships/handlers/active_storage_handler.rb create mode 100644 lib/gitlab/reflections/relationships/handlers/base_handler.rb create mode 100644 lib/gitlab/reflections/relationships/handlers/belongs_to_handler.rb create mode 100644 lib/gitlab/reflections/relationships/handlers/habtm_handler.rb create mode 100644 lib/gitlab/reflections/relationships/handlers/has_association_handler.rb create mode 100644 lib/gitlab/reflections/relationships/handlers/polymorphic_belongs_to_handler.rb create mode 100644 lib/gitlab/reflections/relationships/handlers/polymorphic_has_association_handler.rb create mode 100644 lib/gitlab/reflections/relationships/predicates.rb create mode 100644 lib/gitlab/reflections/relationships/predicates/polymorphic.rb create mode 100644 lib/gitlab/reflections/relationships/predicates/type_based.rb create mode 100644 lib/gitlab/reflections/relationships/relationship.rb create mode 100644 lib/gitlab/reflections/relationships/transformers/deduplicate.rb create mode 100644 lib/gitlab/reflections/relationships/transformers/expand_polymorphic.rb create mode 100644 lib/gitlab/reflections/relationships/transformers/filter_tables.rb create mode 100644 lib/gitlab/reflections/relationships/transformers/invert_relationships.rb create mode 100644 lib/gitlab/reflections/relationships/transformers/merge_data_sources.rb create mode 100644 lib/gitlab/reflections/relationships/transformers/normalize_columns.rb create mode 100644 lib/gitlab/reflections/relationships/transformers/pipeline.rb create mode 100644 lib/gitlab/reflections/relationships/transformers/validate.rb create mode 100644 lib/tasks/database_relationships.rake create mode 100644 spec/lib/gitlab/reflections/database/foreign_keys_spec.rb create mode 100644 spec/lib/gitlab/reflections/models/active_record_spec.rb create mode 100644 spec/lib/gitlab/reflections/polymorphic_edge_cases_spec.rb create mode 100644 spec/lib/gitlab/reflections/polymorphic_json_task_spec.rb create mode 100644 spec/lib/gitlab/reflections/polymorphic_relationships_spec.rb create mode 100644 spec/lib/gitlab/reflections/relationships/active_record_spec.rb create mode 100644 spec/lib/gitlab/reflections/relationships/base_spec.rb create mode 100644 spec/lib/gitlab/reflections/relationships/foreign_key_spec.rb create mode 100644 spec/lib/gitlab/reflections/relationships/handlers/active_storage_handler_spec.rb create mode 100644 spec/lib/gitlab/reflections/relationships/handlers/base_handler_spec.rb create mode 100644 spec/lib/gitlab/reflections/relationships/handlers/belongs_to_handler_spec.rb create mode 100644 spec/lib/gitlab/reflections/relationships/handlers/habtm_handler_spec.rb create mode 100644 spec/lib/gitlab/reflections/relationships/handlers/has_association_handler_spec.rb create mode 100644 spec/lib/gitlab/reflections/relationships/handlers/polymorphic_belongs_to_handler_spec.rb create mode 100644 spec/lib/gitlab/reflections/relationships/handlers/polymorphic_has_association_handler_spec.rb diff --git a/lib/gitlab/crawler.rb b/lib/gitlab/crawler.rb new file mode 100644 index 00000000000000..6a8a0faaec6e35 --- /dev/null +++ b/lib/gitlab/crawler.rb @@ -0,0 +1,92 @@ +# frozen_string_literal: true + +module Gitlab + # Generic crawler class that handles queue management, observer pattern, and basic crawling logic. + # This class can be extended for specific crawling needs like relationship crawling, dependency crawling, etc. + class Crawler + attr_reader :observers, :queue, :processed + + def initialize(observers: nil) + @queue = [] + @processed = Set.new + @observers = observers || default_observers + end + + # Main crawling method - template method pattern + # + # Observer events fired during crawling: + # - :crawl_start - Called when crawling begins + # - :item_crawling - Called before crawling each item (passes the item) + # - :item_crawled - Called after crawling each item (passes {item:, data:}) + # - :crawl_complete - Called when crawling finishes + def crawl + notify_observers(:crawl_start) + + initialize_queue + + while @queue.any? + current = @queue.shift + key = item_key(current) + + next if @processed.include?(key) + + @processed.add(key) + + notify_observers(:item_crawling, current) + + data = crawl_item(current) + + notify_observers(:item_crawled, { item: current, data: data }) + + next unless data.any? + + process_crawled_item(current, data) + end + + notify_observers(:crawl_complete) + end + + protected + + # Template methods to be implemented by subclasses + + # Initialize the queue with seed data + def initialize_queue + raise NotImplementedError, "Subclasses must implement initialize_queue" + end + + # Build a unique key for tracking processed items + def item_key(queue_item) + raise NotImplementedError, "Subclasses must implement item_key" + end + + # Crawl data for a queue item + def crawl_item(queue_item) + raise NotImplementedError, "Subclasses must implement crawl_item" + end + + # Process data and potentially add new items to queue + def process_crawled_item(queue_item, data) + # Default implementation does nothing - subclasses should override + end + + # Default observers - can be overridden by subclasses + def default_observers + [] + end + + # Add an item to the queue + def enqueue(item) + @queue << item + end + + private + + def notify_observers(event, *args) + @observers.each do |observer| + method_name = :"on_#{event}" + observer.__send__(method_name, *args) if observer.respond_to?(method_name) + end + end + end +end diff --git a/lib/gitlab/database/relationship_mapper.rb b/lib/gitlab/database/relationship_mapper.rb new file mode 100644 index 00000000000000..50d6460925f45b --- /dev/null +++ b/lib/gitlab/database/relationship_mapper.rb @@ -0,0 +1,35 @@ +# frozen_string_literal: true + +module Gitlab + module Database + class RelationshipMapper + def initialize + @relationship_sources = [ + Relationships::ActiveRecord.new, + Relationships::ForeignKey.new + ] + end + + def generate_map + Rails.application.eager_load! + + relationships = [] + + @relationship_sources.each do |source| + relationships.concat(source.extract_relationships) + end + + # Sort for consistent output + relationships.sort_by do |rel| + [rel[:source_model], rel[:target_model], rel[:relationship_name]] + end + end + + def save_map(output_path = 'config/relationship_map.json') + map = generate_map + File.write(output_path, Gitlab::Json.pretty_generate(map)) + map + end + end + end +end diff --git a/lib/gitlab/reflections/database/foreign_keys.rb b/lib/gitlab/reflections/database/foreign_keys.rb new file mode 100644 index 00000000000000..adbcc6aa60db68 --- /dev/null +++ b/lib/gitlab/reflections/database/foreign_keys.rb @@ -0,0 +1,41 @@ +# frozen_string_literal: true + +module Gitlab + module Reflections + module Database + # Application-aware foreign key reflection for GitLab's database schema + class ForeignKeys + # GitLab application-specific table filtering constants + INTERNAL_TABLE_NAMES = %w[schema_migrations ar_internal_metadata].freeze + + SYSTEM_TABLE_PATTERNS = [ + /^batched_background_migration/, + /^postgres_/, + /^detached_partitions$/, + /^loose_foreign_keys_deleted_records$/, + /^ghost_user_migrations$/ + ].freeze + + class << self + def for_relationships + scope = ::Gitlab::Database::PostgresForeignKey.not_inherited + + # Filter out internal tables by name + INTERNAL_TABLE_NAMES.each do |table_name| + scope = scope.where.not(constrained_table_name: table_name) + .where.not(referenced_table_name: table_name) + end + + # Filter out system tables by pattern + SYSTEM_TABLE_PATTERNS.each do |pattern| + scope = scope.where.not("constrained_table_name ~ ?", pattern.source) + .where.not("referenced_table_name ~ ?", pattern.source) + end + + scope + end + end + end + end + end +end diff --git a/lib/gitlab/reflections/models/active_record.rb b/lib/gitlab/reflections/models/active_record.rb new file mode 100644 index 00000000000000..6c9994888a4297 --- /dev/null +++ b/lib/gitlab/reflections/models/active_record.rb @@ -0,0 +1,71 @@ +# frozen_string_literal: true + +module Gitlab + module Reflections + module Models + # ActiveRecord model discovery and analysis for GitLab application + class ActiveRecord + include Singleton + + attr_reader :table_to_model_index, :model_to_table_index, :models + + def initialize + build_indexes + end + + private + + def discover_models + ::ActiveRecord::Base.descendants + end + + def filter_models(models) + processed_models = Set.new + + models.reject do |model| + skip = skip_model?(model) || processed_models.include?(model.name) + processed_models.add(model.name) unless skip + skip + end + end + + def skip_model?(model) + return true if model.abstract_class? + return true if model.name.nil? # Anonymous models + return true if model.table_name.nil? + return true if excluded_model?(model) + + false + end + + def excluded_model?(model) + # Skip test/fixture models, internal Rails models, etc. + excluded_patterns = [ + /\AActiveRecord::/, + /\AActiveStorage::/, + /Test$/, + /Fixture$/, + /\AMiniTest::/, + /\ARSpec::/ + ] + + excluded_patterns.any? { |pattern| model.name.match?(pattern) } + end + + def build_indexes + @table_to_model_index = {} + @model_to_table_index = {} + + @models = filter_models(discover_models) + + @models.each do |model| + next unless model.table_name + + @table_to_model_index[model.table_name] = model.name + @model_to_table_index[model.name] = model.table_name + end + end + end + end + end +end diff --git a/lib/gitlab/reflections/relationships/active_record.rb b/lib/gitlab/reflections/relationships/active_record.rb new file mode 100644 index 00000000000000..a43e136e1ac3a9 --- /dev/null +++ b/lib/gitlab/reflections/relationships/active_record.rb @@ -0,0 +1,106 @@ +# frozen_string_literal: true + +module Gitlab + module Reflections + module Relationships + class ActiveRecord < Base + SUPPORTED_RELATIONSHIPS = %w[ + has_many has_one belongs_to has_and_belongs_to_many + has_many_attached has_one_attached + ].freeze + + RELATIONSHIP_HANDLERS = { + 'belongs_to' => Handlers::BelongsToHandler, + 'has_many' => Handlers::HasAssociationHandler, + 'has_one' => Handlers::HasAssociationHandler, + 'has_and_belongs_to_many' => Handlers::HabtmHandler, + 'has_many_attached' => Handlers::ActiveStorageHandler, + 'has_one_attached' => Handlers::ActiveStorageHandler + }.freeze + + def initialize + super + @models_reflection = Gitlab::Reflections::Models::ActiveRecord.instance + end + + private + + def get_source_data + @models_reflection.models + end + + def filter_source_data(models) + # Filtering is now handled by the models reflection + models + end + + def map_to_relationships(models) + relationships = [] + + models.each do |model| + model.reflections.each do |name, reflection| + next unless SUPPORTED_RELATIONSHIPS.include?(reflection.macro.to_s) + + relationship_hashes = build_relationships_for_reflection(model, name, reflection, models) + relationships.concat(relationship_hashes) if relationship_hashes + end + end + + relationships + end + + def build_relationships_for_reflection(model, association_name, reflection, all_models) + handler_class = determine_handler_class(reflection) + return unless handler_class + + handler = handler_class.new(model, association_name, reflection) + + # For polymorphic relationships, we need to discover all possible target tables + if handler.polymorphic? + build_polymorphic_relationships(handler, all_models) + else + [handler.build_relationship].compact + end + end + + def build_polymorphic_relationships(handler, all_models) + relationships = [] + + # First, add the base polymorphic relationship (with target_table: nil) + base_relationship = handler.build_relationship + relationships << base_relationship if base_relationship + + # Then, discover all possible target tables for this polymorphic association + target_tables = handler.discover_targets(all_models) + + target_tables.each do |target_table| + # Create a specific relationship for each target table + specific_relationship = handler.build_relationship_with_target(target_table) + relationships << specific_relationship if specific_relationship + end + + relationships + end + + def determine_handler_class(reflection) + # Check if this is a polymorphic relationship + if polymorphic_belongs_to?(reflection) + Handlers::PolymorphicBelongsToHandler + elsif polymorphic_has_association?(reflection) + Handlers::PolymorphicHasAssociationHandler + else + RELATIONSHIP_HANDLERS[reflection.macro.to_s] + end + end + + def polymorphic_belongs_to?(reflection) + reflection.macro == :belongs_to && reflection.polymorphic? + end + + def polymorphic_has_association?(reflection) + [:has_many, :has_one].include?(reflection.macro) && reflection.options[:as] + end + end + end + end +end diff --git a/lib/gitlab/reflections/relationships/base.rb b/lib/gitlab/reflections/relationships/base.rb new file mode 100644 index 00000000000000..8e08c97154119f --- /dev/null +++ b/lib/gitlab/reflections/relationships/base.rb @@ -0,0 +1,50 @@ +# frozen_string_literal: true + +module Gitlab + module Reflections + module Relationships + class Base + # Main interface - returns relationship data + def extract_relationships + source_data = get_source_data + filtered_data = filter_source_data(source_data) + map_to_relationships(filtered_data) + end + + protected + + def build_relationship(**attributes) + # Validate required fields + required_fields = [:source_table, :target_table, :foreign_key] + missing_fields = required_fields.select { |field| attributes[field].nil? || attributes[field].to_s.empty? } + + if missing_fields.any? + Rails.logger.debug "Failed to build relationship: missing required fields #{missing_fields}" + return + end + + # Return clean relationship hash + attributes.compact + rescue StandardError => e + Rails.logger.debug "Failed to build relationship: #{e.message}" + nil + end + + private + + # Override in subclasses + def get_source_data + raise NotImplementedError, 'Subclasses must implement get_source_data' + end + + def filter_source_data(source_data) + raise NotImplementedError, 'Subclasses must implement filter_source_data' + end + + def map_to_relationships(filtered_data) + raise NotImplementedError, 'Subclasses must implement map_to_relationships' + end + end + end + end +end diff --git a/lib/gitlab/reflections/relationships/builder.rb b/lib/gitlab/reflections/relationships/builder.rb new file mode 100644 index 00000000000000..11cdc1c53cf1e7 --- /dev/null +++ b/lib/gitlab/reflections/relationships/builder.rb @@ -0,0 +1,46 @@ +# frozen_string_literal: true + +module Gitlab + module Reflections + module Relationships + # Builds a relationship array from relationship extractors + class Builder + def initialize(ar_extractor = nil, fk_extractor = nil) + @ar_extractor = ar_extractor || Gitlab::Reflections::Relationships::ActiveRecord.new + @fk_extractor = fk_extractor || Gitlab::Reflections::Relationships::ForeignKey.new + end + + def build_relationships + ar_relationships = @ar_extractor.extract_relationships + fk_relationships = @fk_extractor.extract_relationships + + # Combine and transform relationships using pipeline + combined_relationships = ar_relationships + fk_relationships + + Transformers::Pipeline.new( + Transformers::NormalizeColumns, + Transformers::MergeDataSources, + Transformers::Deduplicate, + Transformers::Validate + ).execute(combined_relationships) + end + + # Alias for backward compatibility + alias_method :build_graph, :build_relationships + + private + + def build_relationship(**attributes) + relationship = Relationship.new(**attributes) + relationship.valid? ? relationship.to_h : nil + rescue ArgumentError => e + Rails.logger.debug "Failed to build relationship: #{e.message}" + nil + end + + # Backward compatibility methods + alias_method :build_edge, :build_relationship + end + end + end +end diff --git a/lib/gitlab/reflections/relationships/crawl_logger.rb b/lib/gitlab/reflections/relationships/crawl_logger.rb new file mode 100644 index 00000000000000..d75c7dbaf5f8d2 --- /dev/null +++ b/lib/gitlab/reflections/relationships/crawl_logger.rb @@ -0,0 +1,112 @@ +# frozen_string_literal: true + +module Gitlab + module Reflections + module Relationships + class CrawlLogger + def initialize(seed_table:, seed_id:, timestamp:) + @seed_table = seed_table + @seed_id = seed_id + @timestamp = timestamp + @tables_log = Rails.root.join('tmp', "crawl_tables_#{timestamp}.log") + @tables_with_data = Set.new + end + + def on_crawl_start + puts "Starting crawl from #{@seed_table}:#{@seed_id}" + puts "Logging tables with data to: #{@tables_log}" + puts "=" * 50 + puts "Validating relationships..." + end + + def on_item_crawling(queue_item) + puts "Fetching #{queue_item.table} data..." + end + + def on_crawl_event(event_data) + case event_data[:type] + when :relationship_query + if event_data[:result_count] > 0 + log_table_with_data(event_data[:table]) + puts "✓ Found #{event_data[:result_count]} records in #{event_data[:table]}" + else + puts "- No data in #{event_data[:table]}" + end + end + end + + def on_crawl_complete(summary_data = nil) + puts "=" * 50 + puts "Crawl completed!" + + if summary_data + puts "Tables with data: #{summary_data[:tables_with_data_count]}" if summary_data[:tables_with_data_count] + puts "Total queries executed: #{summary_data[:total_queries]}" if summary_data[:total_queries] + end + + puts "Tables logged to: #{@tables_log}" + end + + def on_validation_error(error_data) + puts "⚠ Found #{error_data[:count]} invalid relationships in sample:" + error_data[:invalid_relationships].each do |invalid| + puts " [#{invalid[:index]}] #{invalid[:issues].join(', ')}: #{invalid[:relationship].inspect}" + end + end + + def on_crawl_error(error_data) + case error_data[:type] + when :query_error + puts "ERROR querying #{error_data[:table]}: #{error_data[:message]}" + when :table_not_found + puts "ERROR: Table '#{error_data[:table]}' does not exist" + when :column_not_found + puts "ERROR: Column '#{error_data[:column]}' does not exist in table '#{error_data[:table]}'" + when :polymorphic_error + puts "ERROR processing polymorphic relationship: #{error_data[:message]}" + end + end + + def on_crawl_skip(skip_data) + case skip_data[:type] + when :table_ignored + puts "SKIPPED: Table '#{skip_data[:table]}' - #{skip_data[:reason]}" + end + end + + private + + def log_table_with_data(table_name) + return if @tables_with_data.include?(table_name) + + @tables_with_data.add(table_name) + File.open(@tables_log, 'a') { |f| f.puts table_name } + end + + def print_relationship_def(relationship_def) + puts " Input relationship definition:" + puts " #{Gitlab::Json.pretty_generate(relationship_def).gsub(/\n/, "\n ")}" + + return unless relationship_def&.dig('source_info') + + source_info = relationship_def['source_info'] + puts " Derived from:" + + if source_info['has_fk_constraint'] + puts " - Foreign key constraint: #{relationship_def['source_table']}.#{relationship_def['source_column']} -> #{relationship_def['target_table']}.#{relationship_def['target_column']}" + end + + if source_info['forward_association'] + assoc = source_info['forward_association'] + puts " - ActiveRecord association: #{assoc['name']} (#{assoc['type']}) in #{assoc['class_name'] || relationship_def['source_table'].classify}" + end + + return unless source_info['reverse_association'] + + assoc = source_info['reverse_association'] + puts " - Reverse association: #{assoc['name']} (#{assoc['type']}) in #{assoc['class_name'] || relationship_def['target_table']&.classify}" + end + end + end + end +end diff --git a/lib/gitlab/reflections/relationships/crawl_tracer.rb b/lib/gitlab/reflections/relationships/crawl_tracer.rb new file mode 100644 index 00000000000000..511ba455e27d4f --- /dev/null +++ b/lib/gitlab/reflections/relationships/crawl_tracer.rb @@ -0,0 +1,90 @@ +# frozen_string_literal: true + +module Gitlab + module Reflections + module Relationships + class CrawlTracer + def initialize(seed_table:, seed_id:, timestamp:) + @seed_table = seed_table + @seed_id = seed_id + @timestamp = timestamp + @debug_log = Rails.root.join('tmp', "crawl_debug_#{timestamp}.log") + @sequence_counter = 0 + + # Initialize debug log file + initialize_debug_log + end + + def on_crawl_event(event_data) + case event_data[:type] + when :relationship_query + @sequence_counter += 1 + + # Build chain display from relationship_chain if available + chain_display = build_chain_display(event_data[:relationship_chain]) + + log_debug("RELATIONSHIP_QUERY", + "[#{@sequence_counter}] #{event_data[:table]} " \ + "(#{event_data[:result_count]} records) | " \ + "Chain: #{chain_display}") + end + end + + def on_crawl_error(error_data) + log_debug("CRAWL_ERROR", "#{error_data[:type]}: #{error_data[:table]} - #{error_data[:message]}") + end + + def on_crawl_skip(skip_data) + log_debug("CRAWL_SKIP", "#{skip_data[:type]}: #{skip_data[:table]} - #{skip_data[:reason]}") + end + + def query_count + @sequence_counter + end + + private + + def build_chain_display(relationship_chain) + return "seed" if relationship_chain.nil? || relationship_chain.empty? + + # Convert ChainStep objects to readable format + chain_parts = relationship_chain.map do |step| + if step.respond_to?(:to_s) + step.to_s # Uses ChainStep's to_s method: "table.column = value" + else + step.to_s # Fallback for any other format + end + end + + chain_parts.join(" -> ") + end + + def initialize_debug_log + File.open(@debug_log, 'w') do |f| + f.puts "=== DATABASE CRAWL DEBUG LOG ===" + f.puts "Seed: #{@seed_table}:#{@seed_id}" + f.puts "Started: #{Time.current.strftime('%Y-%m-%d %H:%M:%S')}" + f.puts "Log file: #{@debug_log}" + f.puts "Use 'tail -f #{@debug_log}' to follow in real-time" + f.puts "=" * 50 + f.flush + end + rescue StandardError => e + Gitlab::AppLogger.warn("Failed to initialize crawl debug log: #{e.message}") + end + + def log_debug(event_type, message) + timestamp = Time.current.strftime("%H:%M:%S") + log_line = "[#{timestamp}] #{event_type}: #{message}" + + File.open(@debug_log, 'a') do |f| + f.puts log_line + f.flush + end + rescue StandardError => e + Gitlab::AppLogger.warn("Failed to write to crawl debug log: #{e.message}") + end + end + end + end +end diff --git a/lib/gitlab/reflections/relationships/crawler.rb b/lib/gitlab/reflections/relationships/crawler.rb new file mode 100644 index 00000000000000..df3e384cd2af99 --- /dev/null +++ b/lib/gitlab/reflections/relationships/crawler.rb @@ -0,0 +1,190 @@ +# frozen_string_literal: true + +module Gitlab + module Reflections + module Relationships + # Relationship-specific crawler that extends the generic Crawler + # Handles relationship loading, polymorphic relationships, and relationship chain building + class Crawler < Gitlab::Crawler + # Represents a queue item for relationship crawling + QueueItem = Struct.new( + :table, :column, :value, :relationship_chain, + keyword_init: true + ) do + def to_h + super.compact + end + end + + # Represents a step in the relationship chain + ChainStep = Struct.new( + :source_table, :source_column, :source_value, :target_table, :target_column, :target_value, + keyword_init: true + ) do + def to_s + "#{source_table}.#{source_column} = #{source_value} -> #{target_table}.#{target_column} = #{target_value}" + end + end + + def initialize( + relationships_file:, seed_table:, seed_id:, observers: nil, callback_handlers: nil, + ignore_tables: nil + ) + @relationships_file = relationships_file + @seed_table = seed_table + @seed_id = seed_id + @timestamp = Time.current.strftime("%Y%m%d_%H%M%S") + + # Load and transform relationships using pipeline + raw_relationships = Gitlab::Json.parse(File.read(@relationships_file)) + @normalized_relationships = Transformers::Pipeline.new( + Transformers::ExpandPolymorphic, + Transformers::InvertRelationships + ).execute(raw_relationships) + + @tables_with_data = Set.new + # Convert to Set for efficient lookup, handle nil gracefully + @ignore_tables = Set.new(Array(ignore_tables)) + + # Initialize observers (support both old and new parameter names for backwards compatibility) + observers = observers || callback_handlers || default_observers + + super(observers: observers) + end + + # Access to observers for analysis (backwards compatibility) + attr_reader :observers, :ignore_tables + alias_method :callback_handlers, :observers + + protected + + def initialize_queue + # Initialize queue with seed query + enqueue(QueueItem.new( + table: @seed_table, + column: 'id', + value: @seed_id, + relationship_chain: [] + )) + end + + def item_key(queue_item) + "#{queue_item.table}:#{queue_item.column}:#{queue_item.value}" + end + + def crawl_item(queue_item) + sql = "SELECT * FROM #{queue_item.table} WHERE #{queue_item.column} = #{queue_item.value}" + ApplicationRecord.connection.select_all(sql) + rescue StandardError => e + notify_observers(:crawl_error, { + type: :query_error, + table: queue_item.table, + message: e.message + }) + [] + end + + def process_crawled_item(queue_item, records) + @tables_with_data.add(queue_item.table) + + notify_observers(:crawl_event, { + type: :relationship_query, + table: queue_item.table, + result_count: records.count, + relationship_chain: queue_item.relationship_chain + }) + + # Queue related tables based on this data + records.each do |record| + queue_related_tables_from_record(queue_item.table, record, queue_item.relationship_chain) + end + end + + def default_observers + [ + CrawlLogger.new(seed_table: @seed_table, seed_id: @seed_id, timestamp: @timestamp), + CrawlTracer.new(seed_table: @seed_table, seed_id: @seed_id, timestamp: @timestamp) + ] + end + + # Override enqueue to check for ignored tables + def enqueue(item) + # Skip ignored tables + if should_ignore_table?(item.table) + notify_observers(:crawl_skip, { + type: :table_ignored, + table: item.table, + reason: "in ignore list" + }) + return + end + + super + end + + private + + def should_ignore_table?(table_name) + return false if @ignore_tables.empty? + return false unless table_name + + @ignore_tables.include?(table_name) + end + + def queue_related_tables_from_record(source_table, record, relationship_chain) + record_id = record['id'] + return unless record_id + + # Process all normalized relationships in a single loop + @normalized_relationships.select { |rel| rel['source_table'] == source_table }.each do |rel| + next if should_ignore_table?(rel['target_table']) + + queue_value = record[rel['source_column']] + next unless queue_value + + # Handle polymorphic type checking + if rel['relationship_type']&.start_with?('polymorphic_expanded') + type_value = record[rel['polymorphic_type_column']] + next unless type_value == rel['polymorphic_type_value'] + end + + queue_relationship( + record: record, + relationship_chain: relationship_chain, + target_table: rel['target_table'], + target_column: rel['target_column'], + source_column: rel['source_column'], + source_table: source_table + ) + end + end + + def queue_relationship( + record:, relationship_chain:, target_table:, target_column:, source_column:, + source_table:) + record_value = record[source_column] + return unless record_value + + # Build relationship step showing the connection + relationship_step = ChainStep.new( + source_table: source_table, + source_column: source_column, + source_value: record_value, + target_table: target_table, + target_column: target_column, + target_value: record_value + ) + + new_chain = relationship_chain + [relationship_step] + + enqueue(QueueItem.new( + table: target_table, + column: target_column, + value: record_value, + relationship_chain: new_chain + )) + end + end + end + end +end diff --git a/lib/gitlab/reflections/relationships/foreign_key.rb b/lib/gitlab/reflections/relationships/foreign_key.rb new file mode 100644 index 00000000000000..9972f54000670a --- /dev/null +++ b/lib/gitlab/reflections/relationships/foreign_key.rb @@ -0,0 +1,72 @@ +# frozen_string_literal: true + +module Gitlab + module Reflections + module Relationships + class ForeignKey < Base + def initialize(fk_reflection = nil) + super() + @fk_reflection = fk_reflection || Gitlab::Reflections::Database::ForeignKeys + end + + private + + def get_source_data + table_foreign_keys = [] + + @fk_reflection.for_relationships.find_each do |pg_fk| + # Convert PostgresForeignKey to our internal format + table_foreign_keys << { + table_name: pg_fk.constrained_table_name, + foreign_key: build_fk_definition_from_pg_fk(pg_fk) + } + end + + table_foreign_keys + end + + def filter_source_data(table_foreign_keys) + # Filtering is already done by the fk_reflection scope + table_foreign_keys + end + + def map_to_relationships(filtered_items) + filtered_items.filter_map do |item| + build_foreign_key_relationship(item[:table_name], item[:foreign_key]) + end + end + + def build_foreign_key_relationship(table_name, foreign_key) + build_relationship( + source_table: foreign_key.to_table, + target_table: table_name, + relationship_type: 'one_to_many', + foreign_key: foreign_key.column, + foreign_key_table: table_name, + source_column: foreign_key.primary_key || 'id', + target_column: foreign_key.column, + has_fk_constraint: true, + fk_metadata: { + constraint_name: foreign_key.name, + on_delete: foreign_key.on_delete, + on_update: foreign_key.on_update + }, + data_sources: ['foreign_key'] + ) + end + + # Convert PostgresForeignKey to ActiveRecord::ConnectionAdapters::ForeignKeyDefinition-like object + def build_fk_definition_from_pg_fk(pg_fk) + OpenStruct.new( + column: pg_fk.constrained_columns.first, # Assuming single column FK for now + to_table: pg_fk.referenced_table_name, + primary_key: pg_fk.referenced_columns.first, + name: pg_fk.name, + on_delete: pg_fk.on_delete_action, + on_update: pg_fk.on_update_action + ) + end + end + end + end +end diff --git a/lib/gitlab/reflections/relationships/handlers/active_storage_handler.rb b/lib/gitlab/reflections/relationships/handlers/active_storage_handler.rb new file mode 100644 index 00000000000000..e2408e83a71459 --- /dev/null +++ b/lib/gitlab/reflections/relationships/handlers/active_storage_handler.rb @@ -0,0 +1,32 @@ +# frozen_string_literal: true + +module Gitlab + module Reflections + module Relationships + module Handlers + class ActiveStorageHandler < BaseHandler + def build_relationship + build_relationship_hash( + **base_attributes, + foreign_key: 'record_id', + foreign_key_table: 'active_storage_attachments' + ) + end + + private + + def determine_target_table + '**************************' + end + + def base_attributes + # Override to set the correct relationship type for Active Storage + super.merge( + relationship_type: reflection.macro == :has_many_attached ? 'one_to_many' : 'one_to_one' + ) + end + end + end + end + end +end diff --git a/lib/gitlab/reflections/relationships/handlers/base_handler.rb b/lib/gitlab/reflections/relationships/handlers/base_handler.rb new file mode 100644 index 00000000000000..6fcadd1c14e1a1 --- /dev/null +++ b/lib/gitlab/reflections/relationships/handlers/base_handler.rb @@ -0,0 +1,85 @@ +# frozen_string_literal: true + +module Gitlab + module Reflections + module Relationships + module Handlers + class BaseHandler + def initialize(model, association_name, reflection) + @model = model + @association_name = association_name + @reflection = reflection + end + + def build_relationship + raise NotImplementedError, 'Subclasses must implement build_relationship' + end + + def build_relationship_with_target(target_table) + # Default implementation for non-polymorphic relationships + build_relationship + end + + def polymorphic? + false + end + + attr_reader :model, :association_name, :reflection + + private + + def build_relationship_hash(**attributes) + required_fields = [:source_table, :target_table, :foreign_key] + missing_fields = required_fields.select { |field| attributes[field].nil? || attributes[field].to_s.empty? } + + if missing_fields.any? + Gitlab::AppLogger.debug "Failed to build relationship: missing required fields #{missing_fields}" + return + end + + # Return clean relationship hash + attributes.compact + rescue StandardError => e + Gitlab::AppLogger.debug "Failed to build relationship: #{e.message}" + nil + end + + def determine_target_table + reflection.klass.table_name + rescue NameError, ArgumentError + reflection.table_name || reflection.plural_name + end + + def base_attributes + { + source_table: model.table_name, + target_table: determine_target_table, + relationship_type: determine_relationship_type, + forward_association: { + name: association_name.to_s, + type: reflection.macro.to_s, + model: model.name + }, + has_ar_forward: true, + polymorphic: false, + data_sources: ['active_record'] + } + end + + def determine_relationship_type + case reflection.macro + when :has_many, :has_and_belongs_to_many + 'one_to_many' + when :has_one + 'one_to_one' + when :belongs_to + 'many_to_one' + else + 'unknown' + end + end + end + end + end + end +end diff --git a/lib/gitlab/reflections/relationships/handlers/belongs_to_handler.rb b/lib/gitlab/reflections/relationships/handlers/belongs_to_handler.rb new file mode 100644 index 00000000000000..96b966f816ea2a --- /dev/null +++ b/lib/gitlab/reflections/relationships/handlers/belongs_to_handler.rb @@ -0,0 +1,45 @@ +# frozen_string_literal: true + +module Gitlab + module Reflections + module Relationships + module Handlers + class BelongsToHandler < BaseHandler + def build_relationship + build_relationship_hash( + **base_attributes, + foreign_key: reflection.foreign_key, + foreign_key_table: model.table_name + ) + end + + def build_relationship_with_target(_target_table) + # Non-polymorphic belongs_to handlers don't need target-specific relationships + build_relationship + end + + private + + def base_attributes + # For belongs_to, we need to swap source/target to match database FK direction + # The referenced table (parent) should be source, referencing table (child) should be target + { + source_table: determine_target_table, # The referenced table (e.g., namespaces) + target_table: model.table_name, # The referencing table (e.g., projects) + source_column: reflection.association_primary_key || 'id', + relationship_type: 'many_to_one', + reverse_association: { + name: association_name.to_s, + type: reflection.macro.to_s, + model: model.name + }, + has_ar_reverse: true, + polymorphic: false, + data_sources: ['active_record'] + } + end + end + end + end + end +end diff --git a/lib/gitlab/reflections/relationships/handlers/habtm_handler.rb b/lib/gitlab/reflections/relationships/handlers/habtm_handler.rb new file mode 100644 index 00000000000000..a2aa68fb54a73a --- /dev/null +++ b/lib/gitlab/reflections/relationships/handlers/habtm_handler.rb @@ -0,0 +1,28 @@ +# frozen_string_literal: true + +module Gitlab + module Reflections + module Relationships + module Handlers + class HabtmHandler < BaseHandler + def build_relationship + build_relationship_hash( + **base_attributes, + foreign_key: reflection.foreign_key, + foreign_key_table: reflection.join_table, + through_table: reflection.join_table, + through_target_key: reflection.association_foreign_key + ) + end + + private + + def base_attributes + # Override to set the correct relationship type for HABTM + super.merge(relationship_type: 'many_to_many') + end + end + end + end + end +end diff --git a/lib/gitlab/reflections/relationships/handlers/has_association_handler.rb b/lib/gitlab/reflections/relationships/handlers/has_association_handler.rb new file mode 100644 index 00000000000000..de99f29402e5e9 --- /dev/null +++ b/lib/gitlab/reflections/relationships/handlers/has_association_handler.rb @@ -0,0 +1,81 @@ +# frozen_string_literal: true + +module Gitlab + module Reflections + module Relationships + module Handlers + class HasAssociationHandler < BaseHandler + def build_relationship + attributes = base_attributes.merge( + foreign_key: determine_foreign_key, + foreign_key_table: determine_foreign_key_table, + source_column: (reflection.options[:primary_key] || reflection.association_primary_key || 'id').to_s, + target_column: determine_foreign_key, + through_table: determine_through_table, + through_target_key: determine_through_target_key, + scope_conditions: extract_scope_conditions, + is_through_association: reflection.through_reflection? + ) + + build_relationship_hash(**attributes) + end + + def build_relationship_with_target(_target_table) + # Non-polymorphic has_association handlers don't need target-specific relationships + build_relationship + end + + private + + def determine_foreign_key + if reflection.through_reflection + reflection.source_reflection&.foreign_key + else + reflection.foreign_key + end + end + + def determine_foreign_key_table + if reflection.through_reflection + reflection.through_reflection.table_name + else + determine_target_table + end + rescue StandardError + nil + end + + def determine_through_table + return unless reflection.through_reflection? + + case reflection.through_reflection.macro + when :has_and_belongs_to_many + reflection.through_reflection.join_table + else + reflection.through_reflection.table_name + end + end + + def determine_through_target_key + return unless reflection.through_reflection? + + reflection.source_reflection&.foreign_key + end + + def extract_scope_conditions + return unless reflection.scope + + if reflection.options[:conditions] + reflection.options[:conditions].to_s + elsif reflection.scope.is_a?(Proc) + scope_source = reflection.scope.source_location + "# Scope defined at #{scope_source&.join(':')}" if scope_source + end + rescue StandardError + nil + end + end + end + end + end +end diff --git a/lib/gitlab/reflections/relationships/handlers/polymorphic_belongs_to_handler.rb b/lib/gitlab/reflections/relationships/handlers/polymorphic_belongs_to_handler.rb new file mode 100644 index 00000000000000..6616f3c8800900 --- /dev/null +++ b/lib/gitlab/reflections/relationships/handlers/polymorphic_belongs_to_handler.rb @@ -0,0 +1,90 @@ +# frozen_string_literal: true + +module Gitlab + module Reflections + module Relationships + module Handlers + class PolymorphicBelongsToHandler < BelongsToHandler + def build_relationship + build_relationship_hash( + **base_polymorphic_attributes, + foreign_key: reflection.foreign_key, + foreign_key_table: model.table_name + ) + end + + def build_relationship_with_target(target_table) + build_relationship_hash( + **base_attributes_with_target(target_table), + foreign_key: reflection.foreign_key, + foreign_key_table: model.table_name + ) + end + + def polymorphic? + true + end + + def discover_targets(all_models) + target_tables = [] + polymorphic_macros = [:has_many, :has_one].freeze + polymorphic_name = association_name.to_s + + all_models.each do |model| + model.reflections.each_value do |reflection| + if polymorphic_macros.include?(reflection.macro) && + reflection.options[:as]&.to_s == polymorphic_name + target_tables << model.table_name + end + end + end + + target_tables.uniq + end + + private + + def base_polymorphic_attributes + { + source_table: model.table_name, + target_table: nil, # Polymorphic relationships don't have a single target + relationship_type: 'many_to_one', + reverse_association: { + name: association_name.to_s, + type: reflection.macro.to_s, + model: model.name + }, + has_ar_reverse: true, + polymorphic: true, + polymorphic_type_column: polymorphic_type_column, + polymorphic_name: association_name.to_s, + data_sources: ['active_record'] + } + end + + def base_attributes_with_target(target_table) + { + source_table: target_table, + target_table: model.table_name, + relationship_type: 'one_to_many', + forward_association: { + name: association_name.to_s, + type: 'has_many', + model: target_table.classify + }, + has_ar_forward: true, + polymorphic: true, + polymorphic_type_column: polymorphic_type_column, + polymorphic_name: association_name.to_s, + data_sources: ['active_record'] + } + end + + def polymorphic_type_column + reflection.foreign_type || "#{association_name}_type" + end + end + end + end + end +end diff --git a/lib/gitlab/reflections/relationships/handlers/polymorphic_has_association_handler.rb b/lib/gitlab/reflections/relationships/handlers/polymorphic_has_association_handler.rb new file mode 100644 index 00000000000000..8d8d3baef7f88c --- /dev/null +++ b/lib/gitlab/reflections/relationships/handlers/polymorphic_has_association_handler.rb @@ -0,0 +1,108 @@ +# frozen_string_literal: true + +module Gitlab + module Reflections + module Relationships + module Handlers + class PolymorphicHasAssociationHandler < HasAssociationHandler + def build_relationship + attributes = base_polymorphic_attributes.merge( + foreign_key: determine_foreign_key, + foreign_key_table: determine_foreign_key_table, + source_column: reflection.association_primary_key || 'id', + target_column: determine_foreign_key, + through_table: determine_through_table, + through_target_key: determine_through_target_key, + scope_conditions: extract_scope_conditions, + is_through_association: reflection.through_reflection? + ) + + build_relationship_hash(**attributes) + end + + def build_relationship_with_target(target_table) + attributes = base_attributes_with_target(target_table).merge( + foreign_key: determine_foreign_key, + foreign_key_table: determine_foreign_key_table, + source_column: reflection.association_primary_key || 'id', + target_column: determine_foreign_key, + through_table: determine_through_table, + through_target_key: determine_through_target_key, + scope_conditions: extract_scope_conditions, + is_through_association: reflection.through_reflection? + ) + + build_relationship_hash(**attributes) + end + + def polymorphic? + true + end + + def discover_targets(all_models) + target_tables = [] + as_name = reflection.options[:as].to_s + + all_models.each do |model| + model.reflections.each do |name, reflection| + next unless reflection.macro == :belongs_to && + reflection.polymorphic? && + name.to_s == as_name + + target_tables << model.table_name + end + end + + target_tables.uniq + end + + private + + def base_polymorphic_attributes + { + source_table: model.table_name, + target_table: nil, # Polymorphic relationships don't have a single target + relationship_type: determine_relationship_type, + forward_association: { + name: association_name.to_s, + type: reflection.macro.to_s, + model: model.name + }, + has_ar_forward: true, + polymorphic: true, + polymorphic_type_column: polymorphic_type_column, + polymorphic_name: polymorphic_association_name, + data_sources: ['active_record'] + } + end + + def base_attributes_with_target(target_table) + { + source_table: model.table_name, + target_table: target_table, + relationship_type: determine_relationship_type, + forward_association: { + name: association_name.to_s, + type: reflection.macro.to_s, + model: model.name + }, + has_ar_forward: true, + polymorphic: true, + polymorphic_type_column: polymorphic_type_column, + polymorphic_name: polymorphic_association_name, + data_sources: ['active_record'] + } + end + + def polymorphic_type_column + "#{reflection.options[:as]}_type" + end + + def polymorphic_association_name + reflection.options[:as].to_s + end + end + end + end + end +end diff --git a/lib/gitlab/reflections/relationships/predicates.rb b/lib/gitlab/reflections/relationships/predicates.rb new file mode 100644 index 00000000000000..708145ab0bda4a --- /dev/null +++ b/lib/gitlab/reflections/relationships/predicates.rb @@ -0,0 +1,13 @@ +# frozen_string_literal: true + +module Gitlab + module Reflections + module Relationships + # Business logic for relationship identification and classification + module Predicates + include TypeBased + include Polymorphic + end + end + end +end diff --git a/lib/gitlab/reflections/relationships/predicates/polymorphic.rb b/lib/gitlab/reflections/relationships/predicates/polymorphic.rb new file mode 100644 index 00000000000000..a5caa6b916eb54 --- /dev/null +++ b/lib/gitlab/reflections/relationships/predicates/polymorphic.rb @@ -0,0 +1,24 @@ +# frozen_string_literal: true + +module Gitlab + module Reflections + module Relationships + module Predicates + # Predicates specific to polymorphic relationships + module Polymorphic + # Key generation functions for grouping - this provides meaningful abstraction + # for complex grouping logic that would be cumbersome to inline + def polymorphic_key(relationship) + return unless polymorphic_relationship?(relationship) + + [ + relationship[:polymorphic_type_column], + relationship[:foreign_key], + relationship[:foreign_key_table] || relationship[:target_table] + ] + end + end + end + end + end +end diff --git a/lib/gitlab/reflections/relationships/predicates/type_based.rb b/lib/gitlab/reflections/relationships/predicates/type_based.rb new file mode 100644 index 00000000000000..8d80c93161ec06 --- /dev/null +++ b/lib/gitlab/reflections/relationships/predicates/type_based.rb @@ -0,0 +1,18 @@ +# frozen_string_literal: true + +module Gitlab + module Reflections + module Relationships + module Predicates + # Predicates for determining relationship types and characteristics + module TypeBased + # Only keep the polymorphic_relationship? method as it's actually used + # and provides a meaningful abstraction for the boolean check + def polymorphic_relationship?(relationship) + relationship[:polymorphic] == true + end + end + end + end + end +end diff --git a/lib/gitlab/reflections/relationships/relationship.rb b/lib/gitlab/reflections/relationships/relationship.rb new file mode 100644 index 00000000000000..7f86543c34e433 --- /dev/null +++ b/lib/gitlab/reflections/relationships/relationship.rb @@ -0,0 +1,214 @@ +# frozen_string_literal: true + +module Gitlab + module Reflections + module Relationships + # Represents a single relationship in the relationship collection between database tables. + # A relationship captures the raw/primitive relationship metadata directly extracted from + # ActiveRecord reflections and database foreign key constraints. + # + # This class stores ONLY primitive parsed data - no derived or processed information. + # Processed data like relationship types, polymorphic targets, etc. should be computed + # in later processing steps. + # + # == Attribute Relationships and Direction + # + # The source/target and foreign_key attributes serve different purposes and are NOT redundant: + # + # - source_table/source_column: The table/column being referenced (usually the "parent" with primary key) + # - target_table/target_column: The table/column doing the referencing (usually the "child" with foreign key) + # - foreign_key/foreign_key_table: The actual physical foreign key column and which table it's in + # + # Example for users → posts relationship: + # source_table: 'users', source_column: 'id' (the referenced primary key) + # target_table: 'posts', target_column: 'user_id' (the referencing column) + # foreign_key: 'user_id', foreign_key_table: 'posts' (the actual FK constraint) + # + # This distinction matters for: + # - Through relationships: FK might be in an intermediate table + # - Complex joins: FK column name might differ from target column name + # - Polymorphic associations: Multiple target tables can share the same FK + # + # == Direction and Bidirectional Information + # + # Direction is determined by data source and association type. The system creates + # inverted information by parsing relationships from multiple sources: + # + # 1. ActiveRecord associations create relationships based on the model defining the association: + # - User.has_many :posts creates forward relationship (users → posts) with has_ar_forward: true + # - Post.belongs_to :user creates reverse relationship (users → posts) with has_ar_reverse: true + # Note: Both associations describe the same relationship direction (users → posts) + # but from different model perspectives + # + # 2. Foreign Key constraints always create relationships from referenced → referencing table: + # - FK constraint posts.user_id → users.id creates relationship (users → posts) + # + # 3. Collection merges relationships with same key (source_table, target_table, source_column, target_column): + # - When both User.has_many :posts and Post.belongs_to :user exist, they merge into + # one bidirectional relationship with both has_ar_forward: true and has_ar_reverse: true + # + # @attr source_table [String] The name of the source table in the relationship + # @attr target_table [String] The name of the target table in the relationship + # @attr source_column [String] The column in the source table that is referenced (usually primary key) + # @attr target_column [String] The column in the target table that references the source (foreign key) + # @attr foreign_key [String] The foreign key column name that establishes the relationship + # @attr foreign_key_table [String] The table that contains the foreign key column + # @attr relationship_type [String] The type of relationship directly derived from ActiveRecord macro (one_to_one, one_to_many, many_to_one, many_to_many) + # @attr forward_association [Hash] ActiveRecord association metadata for the forward direction (source -> target) + # @attr reverse_association [Hash] ActiveRecord association metadata for the reverse direction (target -> source) + # @attr through_table [String] The intermediate table name for many-to-many relationships + # @attr through_target_key [String] The key column in the through table that points to the target + # @attr polymorphic [Boolean] Whether this is a polymorphic association + # @attr polymorphic_type_column [String] The column name that stores the polymorphic type + # @attr polymorphic_name [String] The name of the polymorphic association (e.g., 'commentable', 'notable') + # @attr has_ar_forward [Boolean] Whether ActiveRecord defines a forward association for this relationship + # @attr has_ar_reverse [Boolean] Whether ActiveRecord defines a reverse association for this relationship + # @attr has_fk_constraint [Boolean] Whether a foreign key constraint exists in the database + # @attr fk_metadata [Hash] Additional metadata about the foreign key constraint + # @attr data_sources [Array] List of data sources that contributed to discovering this relationship + # @attr scope_conditions [Hash] Additional conditions or scopes that apply to this relationship + class Relationship + include ActiveModel::Model + include Gitlab::Reflections::Relationships::Predicates + + VALID_RELATIONSHIP_TYPES = %w[ + one_to_one one_to_many many_to_one many_to_many + ].freeze + + VALID_ASSOCIATION_TYPES = %w[ + has_many has_one belongs_to has_and_belongs_to_many + has_many_attached has_one_attached + ].freeze + + attr_accessor :source_table, :target_table, :foreign_key, :foreign_key_table, + :source_column, :target_column, :relationship_type, :forward_association, :reverse_association, + :through_table, :through_target_key, :polymorphic, :polymorphic_type_column, + :polymorphic_name, :has_ar_forward, :has_ar_reverse, + :has_fk_constraint, :fk_metadata, :data_sources, :scope_conditions, :is_through_association + + validates :source_table, :foreign_key, presence: true + validates :target_table, presence: true, unless: :polymorphic? + validates :relationship_type, inclusion: { in: VALID_RELATIONSHIP_TYPES }, allow_blank: true + validate :valid_associations? + + def initialize(attributes = {}) + # Ensure data_sources is always an array + attributes = attributes.dup + attributes[:data_sources] = Array(attributes[:data_sources]) if attributes.key?(:data_sources) + + # Set default values + attributes[:polymorphic] = false unless attributes.key?(:polymorphic) + attributes[:has_ar_forward] = false unless attributes.key?(:has_ar_forward) + attributes[:has_ar_reverse] = false unless attributes.key?(:has_ar_reverse) + attributes[:has_fk_constraint] = false unless attributes.key?(:has_fk_constraint) + attributes[:is_through_association] = false unless attributes.key?(:is_through_association) + attributes[:data_sources] = [] unless attributes.key?(:data_sources) + + # Set default column values if not provided + attributes[:source_column] ||= 'id' unless attributes.key?(:source_column) + if attributes[:foreign_key] && !attributes.key?(:target_column) + attributes[:target_column] ||= attributes[:foreign_key] + end + + super + end + + def through_relationship? + !!(through_table && !through_table.empty?) + end + + def polymorphic? + polymorphic + end + + # Returns all possible target models for this polymorphic relationship + # Note: This should be computed in a later processing step by analyzing + # all relationships with the same polymorphic_type_column + def polymorphic_target_models + [] + end + + # Returns the polymorphic association name (e.g., 'commentable', 'notable') + def polymorphic_association_name + return unless polymorphic? + + polymorphic_name + end + + # Returns the type column name for polymorphic relationships + def polymorphic_type_column_name + return unless polymorphic? + + polymorphic_type_column + end + + # Hash-style attribute access for compatibility with Predicates + def [](attr_name) + public_send(attr_name) + end + + def to_h + { + source_table: source_table, + target_table: target_table, + source_column: source_column, + target_column: target_column, + foreign_key: foreign_key, + foreign_key_table: foreign_key_table, + relationship_type: relationship_type, + forward_association: forward_association, + reverse_association: reverse_association, + through_table: through_table, + through_target_key: through_target_key, + polymorphic: polymorphic, + polymorphic_type_column: polymorphic_type_column, + polymorphic_name: polymorphic_name, + has_ar_forward: has_ar_forward, + has_ar_reverse: has_ar_reverse, + has_fk_constraint: has_fk_constraint, + fk_metadata: fk_metadata, + data_sources: data_sources, + scope_conditions: scope_conditions, + is_through_association: is_through_association + }.compact + end + + def ==(other) + return false unless other.is_a?(self.class) + + to_h == other.to_h + end + + alias_method :eql?, :== + + def hash + to_h.hash + end + + def inspect + "#<#{self.class.name} #{source_table} -> #{target_table} (#{relationship_type})>" + end + + private + + def valid_associations? + valid_forward = forward_association.nil? || valid_association?(forward_association) + valid_reverse = reverse_association.nil? || valid_association?(reverse_association) + + errors.add(:base, 'Invalid association types') unless valid_forward && valid_reverse + + valid_forward && valid_reverse + end + + def valid_association?(association) + return false unless association.is_a?(Hash) + + return false unless association[:name] && !association[:name].empty? && + association[:type] && !association[:type].empty? + + VALID_ASSOCIATION_TYPES.include?(association[:type]) + end + end + end + end +end diff --git a/lib/gitlab/reflections/relationships/transformers/deduplicate.rb b/lib/gitlab/reflections/relationships/transformers/deduplicate.rb new file mode 100644 index 00000000000000..f1fc43955984b7 --- /dev/null +++ b/lib/gitlab/reflections/relationships/transformers/deduplicate.rb @@ -0,0 +1,46 @@ +# frozen_string_literal: true + +module Gitlab + module Reflections + module Relationships + module Transformers + class Deduplicate + def self.call(relationships) + new(relationships).transform + end + + def initialize(relationships) + @relationships = relationships + end + + def transform + seen = Set.new + @relationships.filter_map do |rel| + key = relationship_signature(rel) + if seen.include?(key) + nil + else + seen.add(key) + rel + end + end + end + + private + + def relationship_signature(rel) + # Create a signature that uniquely identifies a relationship + [ + rel['source_table'], + rel['target_table'], + rel['source_column'], + rel['target_column'], + rel['relationship_type'], + rel['polymorphic_type_value'] + ].compact.join('|') + end + end + end + end + end +end \ No newline at end of file diff --git a/lib/gitlab/reflections/relationships/transformers/expand_polymorphic.rb b/lib/gitlab/reflections/relationships/transformers/expand_polymorphic.rb new file mode 100644 index 00000000000000..721d395b3b428d --- /dev/null +++ b/lib/gitlab/reflections/relationships/transformers/expand_polymorphic.rb @@ -0,0 +1,71 @@ +# frozen_string_literal: true + +module Gitlab + module Reflections + module Relationships + module Transformers + class ExpandPolymorphic + def self.call(relationships) + new(relationships).transform + end + + def initialize(relationships) + @relationships = relationships + end + + def transform + expanded = [] + + @relationships.each do |rel| + if polymorphic_relationship?(rel) + if rel['target_table'] # Already expanded + expanded << rel + else + # Expand base polymorphic into specific relationships + expanded.concat(expand_polymorphic_relationship(rel)) + end + else + expanded << rel + end + end + + expanded + end + + private + + def polymorphic_relationship?(rel) + rel['polymorphic'] + end + + def expand_polymorphic_relationship(rel) + # Find all relationships that reference this polymorphic association + target_tables = find_polymorphic_targets(rel) + + target_tables.map do |target_table| + rel.merge( + 'target_table' => target_table, + 'relationship_type' => 'polymorphic_expanded', + 'polymorphic_type_value' => target_table.classify + ) + end + end + + def find_polymorphic_targets(rel) + # Look through all relationships to find ones that target this polymorphic + polymorphic_name = rel['polymorphic_name'] + targets = Set.new + + @relationships.each do |other_rel| + if other_rel['polymorphic_name'] == polymorphic_name && other_rel['target_table'] + targets << other_rel['target_table'] + end + end + + targets.to_a + end + end + end + end + end +end diff --git a/lib/gitlab/reflections/relationships/transformers/filter_tables.rb b/lib/gitlab/reflections/relationships/transformers/filter_tables.rb new file mode 100644 index 00000000000000..da08760135f1e3 --- /dev/null +++ b/lib/gitlab/reflections/relationships/transformers/filter_tables.rb @@ -0,0 +1,46 @@ +# frozen_string_literal: true + +module Gitlab + module Reflections + module Relationships + module Transformers + class FilterTables + def self.call(relationships, excluded_tables: [], included_tables: nil) + new(relationships, excluded_tables: excluded_tables, included_tables: included_tables).transform + end + + def initialize(relationships, excluded_tables: [], included_tables: nil) + @relationships = relationships + @excluded_tables = Set.new(Array(excluded_tables)) + @included_tables = included_tables ? Set.new(Array(included_tables)) : nil + end + + def transform + @relationships.reject { |rel| should_filter?(rel) } + end + + private + + def should_filter?(rel) + return true if table_excluded?(rel['source_table']) + return true if table_excluded?(rel['target_table']) + return true if @included_tables && !table_included?(rel['source_table']) + return true if @included_tables && !table_included?(rel['target_table']) + + false + end + + def table_excluded?(table) + return false unless table + @excluded_tables.include?(table) + end + + def table_included?(table) + return true unless @included_tables && table + @included_tables.include?(table) + end + end + end + end + end +end \ No newline at end of file diff --git a/lib/gitlab/reflections/relationships/transformers/invert_relationships.rb b/lib/gitlab/reflections/relationships/transformers/invert_relationships.rb new file mode 100644 index 00000000000000..ae7f48ce99a202 --- /dev/null +++ b/lib/gitlab/reflections/relationships/transformers/invert_relationships.rb @@ -0,0 +1,76 @@ +# frozen_string_literal: true + +module Gitlab + module Reflections + module Relationships + module Transformers + class InvertRelationships + def self.call(relationships) + new(relationships).transform + end + + def initialize(relationships) + @relationships = relationships + end + + def transform + inverted = @relationships.dup + + @relationships.each do |rel| + next if should_skip_inversion?(rel) + + inverse_rel = create_inverse_relationship(rel) + inverted << inverse_rel if inverse_rel + end + + inverted + end + + private + + def should_skip_inversion?(rel) + # Don't invert polymorphic base relationships (without target_table) + # Don't invert if it's already an inverted relationship + (rel['polymorphic'] && !rel['target_table']) || + rel['inverted'] == true + end + + def create_inverse_relationship(rel) + return nil unless rel['source_table'] && rel['target_table'] + + { + 'source_table' => rel['target_table'], + 'target_table' => rel['source_table'], + 'source_column' => rel['target_column'], + 'target_column' => rel['source_column'], + 'relationship_type' => invert_relationship_type(rel['relationship_type']), + 'inverted' => true, + 'original_relationship' => rel.slice('source_table', 'target_table', 'relationship_type') + }.merge(preserve_polymorphic_info(rel)) + end + + def invert_relationship_type(type) + case type + when 'one_to_many' then 'many_to_one' + when 'many_to_one' then 'one_to_many' + when 'polymorphic_expanded' then 'polymorphic_expanded_inverse' + else type + end + end + + def preserve_polymorphic_info(rel) + if rel['polymorphic'] + { + 'polymorphic' => true, + 'polymorphic_type_column' => rel['polymorphic_type_column'], + 'polymorphic_type_value' => rel['polymorphic_type_value'] + } + else + {} + end + end + end + end + end + end +end \ No newline at end of file diff --git a/lib/gitlab/reflections/relationships/transformers/merge_data_sources.rb b/lib/gitlab/reflections/relationships/transformers/merge_data_sources.rb new file mode 100644 index 00000000000000..5ac6a18c6d4ee4 --- /dev/null +++ b/lib/gitlab/reflections/relationships/transformers/merge_data_sources.rb @@ -0,0 +1,48 @@ +# frozen_string_literal: true + +module Gitlab + module Reflections + module Relationships + module Transformers + class MergeDataSources + def self.call(relationships) + new(relationships).transform + end + + def initialize(relationships) + @relationships = relationships + end + + def transform + grouped = @relationships.group_by { |rel| key(rel) } + grouped.values.map { |group| merge_group(group) } + end + + private + + def key(rel) + [rel['source_table'], rel['target_table'], rel['source_column'], rel['target_column']] + end + + def merge_group(group) + return group.first if group.size == 1 + + merged = group.first.dup + group[1..].each { |rel| merge_data(merged, rel) } + merged + end + + def merge_data(base, other) + base['data_sources'] = Array(base['data_sources']) | Array(other['data_sources']) + base['has_ar_forward'] ||= other['has_ar_forward'] + base['has_ar_reverse'] ||= other['has_ar_reverse'] + base['has_fk_constraint'] ||= other['has_fk_constraint'] + base['forward_association'] ||= other['forward_association'] + base['reverse_association'] ||= other['reverse_association'] + base['fk_metadata'] ||= other['fk_metadata'] + end + end + end + end + end +end \ No newline at end of file diff --git a/lib/gitlab/reflections/relationships/transformers/normalize_columns.rb b/lib/gitlab/reflections/relationships/transformers/normalize_columns.rb new file mode 100644 index 00000000000000..00ff3bdc5ff22b --- /dev/null +++ b/lib/gitlab/reflections/relationships/transformers/normalize_columns.rb @@ -0,0 +1,44 @@ +# frozen_string_literal: true + +module Gitlab + module Reflections + module Relationships + module Transformers + class NormalizeColumns + def self.call(relationships) + new(relationships).transform + end + + def initialize(relationships) + @relationships = relationships + end + + def transform + @relationships.map { |rel| normalize_relationship(rel) } + end + + private + + def normalize_relationship(rel) + normalized = rel.dup + + # Set default source column if missing + normalized['source_column'] ||= 'id' + + # Set target column from foreign key if missing + if normalized['foreign_key'] && !normalized['target_column'] + normalized['target_column'] = normalized['foreign_key'] + end + + # Ensure foreign_key_table is set + if normalized['foreign_key'] && !normalized['foreign_key_table'] + normalized['foreign_key_table'] = normalized['target_table'] + end + + normalized + end + end + end + end + end +end \ No newline at end of file diff --git a/lib/gitlab/reflections/relationships/transformers/pipeline.rb b/lib/gitlab/reflections/relationships/transformers/pipeline.rb new file mode 100644 index 00000000000000..2be892d7763534 --- /dev/null +++ b/lib/gitlab/reflections/relationships/transformers/pipeline.rb @@ -0,0 +1,19 @@ +# frozen_string_literal: true + +module Gitlab + module Reflections + module Relationships + module Transformers + class Pipeline + def initialize(*transformers) + @transformers = transformers + end + + def execute(input) + @transformers.reduce(input) { |data, t| t.call(data) } + end + end + end + end + end +end \ No newline at end of file diff --git a/lib/gitlab/reflections/relationships/transformers/validate.rb b/lib/gitlab/reflections/relationships/transformers/validate.rb new file mode 100644 index 00000000000000..568e4b1c7a91a6 --- /dev/null +++ b/lib/gitlab/reflections/relationships/transformers/validate.rb @@ -0,0 +1,52 @@ +# frozen_string_literal: true + +module Gitlab + module Reflections + module Relationships + module Transformers + class Validate + def self.call(relationships) + new(relationships).transform + end + + def initialize(relationships) + @relationships = relationships + end + + def transform + @relationships.filter_map do |rel| + validate_relationship(rel) + end + end + + private + + def validate_relationship(rel) + # Check required fields + required_fields = ['source_table', 'target_table'] + missing_fields = required_fields.select { |field| rel[field].nil? || rel[field].to_s.empty? } + + if missing_fields.any? + Rails.logger.debug "Skipping invalid relationship: missing required fields #{missing_fields}" + return nil + end + + # Additional validation for polymorphic relationships + if rel['polymorphic'] && rel['relationship_type'] == 'polymorphic_expanded' + unless rel['polymorphic_type_column'] && rel['polymorphic_type_value'] + Rails.logger.debug "Skipping invalid polymorphic relationship: missing type information" + return nil + end + end + + # Return clean relationship hash + rel.compact + rescue StandardError => e + Rails.logger.debug "Failed to validate relationship: #{e.message}" + nil + end + end + end + end + end +end \ No newline at end of file diff --git a/lib/tasks/database_relationships.rake b/lib/tasks/database_relationships.rake new file mode 100644 index 00000000000000..5aee890a73afe1 --- /dev/null +++ b/lib/tasks/database_relationships.rake @@ -0,0 +1,193 @@ +# frozen_string_literal: true + +namespace :db do + namespace :relationships do + desc "Print relationships in JSON format" + task all: :environment do + builder = Gitlab::Reflections::Relationships::Builder.new + relationships = builder.build_relationships + + puts Gitlab::Json.pretty_generate(relationships) + end + + desc "Crawl relationships for all tables" + task crawl_map: :environment do + builder = Gitlab::Reflections::Relationships::Builder.new + all_relationships = builder.build_relationships + + # Filter out through associations for crawling - they don't represent direct database relationships + crawlable_relationships = all_relationships.reject { |rel| rel[:is_through_association] } + + # Filter to only include relationships from sharded tables + crawlable_relationships = filter_sharded_relationships(crawlable_relationships) + + # Process all relationships uniformly - polymorphic relationships are already completed + crawl_data = crawlable_relationships.map do |rel| + { + source_table: rel[:source_table], + target_table: rel[:target_table], + source_column: rel[:source_column], + target_column: rel[:target_column], + relationship_type: rel[:polymorphic] ? "polymorphic" : "regular", + # Add traceability information for debugging + source_info: { + has_ar_forward: rel[:has_ar_forward], + has_ar_reverse: rel[:has_ar_reverse], + has_fk_constraint: rel[:has_fk_constraint], + forward_association: rel[:forward_association]&.slice(:name, :type, :class_name), + reverse_association: rel[:reverse_association]&.slice(:name, :type, :class_name), + is_through_association: rel[:is_through_association] + } + }.tap do |item| + # Add polymorphic fields only if it's a polymorphic relationship + if rel[:polymorphic] + item[:polymorphic_type_column] = rel[:polymorphic_type_column] + # For polymorphic relationships with specific target tables, use the target table name + # For base polymorphic relationships (target_table: nil), keep it as nil + item[:polymorphic_type_value] = rel[:target_table]&.classify + end + end + end + + # Group relationships to show duplicates and their sources + grouped_relationships = crawl_data.group_by do |item| + [ + item[:source_table], + item[:target_table], + item[:source_column], + item[:target_column], + item[:relationship_type], + item[:polymorphic_type_column], + item[:polymorphic_type_value] + ] + end + + # Convert grouped relationships back to crawl format + final_crawl_data = grouped_relationships.map do |key, group| + if group.size == 1 + # Single relationship - use as is + group.first + else + # Multiple relationships with same key - merge source_info + base_relationship = group.first.dup + base_relationship[:source_info] = { + has_ar_forward: group.any? { |r| r[:source_info][:has_ar_forward] }, + has_ar_reverse: group.any? { |r| r[:source_info][:has_ar_reverse] }, + has_fk_constraint: group.any? { |r| r[:source_info][:has_fk_constraint] }, + forward_association: group.find do |r| + r[:source_info][:forward_association] + end&.dig(:source_info, :forward_association), + reverse_association: group.find do |r| + r[:source_info][:reverse_association] + end&.dig(:source_info, :reverse_association), + duplicate_count: group.size, + all_sources: group.map { |r| r[:source_info] } + } + base_relationship + end + end + + puts Gitlab::Json.pretty_generate(final_crawl_data) + end + + desc "Crawl database relationships starting from a seed table" + task :crawl, [:json_file, :seed_table, :seed_id, :ignore_tables] => :environment do |_task, args| + unless args[:json_file] && args[:seed_table] && args[:seed_id] + usage_msg = "Usage: rails db:relationships:crawl[/path/to/relationships.json," \ + "table_name,primary_key_id,table1,table2,...]" + puts usage_msg + puts " ignore_tables: Optional comma-separated list of tables to ignore during crawling" + exit 1 + end + + # Parse ignore_tables if provided + ignore_tables = args[:ignore_tables]&.split(',')&.map(&:strip) + + crawler = Gitlab::Reflections::Relationships::Crawler.new( + relationships_file: args[:json_file], + seed_table: args[:seed_table], + seed_id: args[:seed_id], + ignore_tables: ignore_tables + ) + + crawler.crawl + end + + desc "Print polymorphic relationships using predicates in JSON format" + task polymorphic_json: :environment do + # Include the predicate modules for easy access + include Gitlab::Reflections::Relationships::Predicates::Polymorphic + include Gitlab::Reflections::Relationships::Predicates::TypeBased + + puts "Building relationships..." + builder = Gitlab::Reflections::Relationships::Builder.new + relationships = builder.build_relationships + + puts "Filtering polymorphic relationships using predicates..." + # Work directly with Array - no wrapper needed + polymorphic_rels = relationships.select { |rel| polymorphic_relationship?(rel) } + + # Group by polymorphic key and aggregate + grouped = polymorphic_rels.group_by { |rel| polymorphic_key(rel) } + + puts "Aggregating #{grouped.size} polymorphic relationship groups..." + aggregated_polymorphic = grouped.map do |key, group| + type_column, foreign_key, foreign_key_table = key + + # Collect target tables from the group (polymorphic relationships may not have target_table) + targets = group.filter_map { |rel| rel[:target_table] }.compact.uniq.sort + + # For polymorphic relationships, we need to discover targets by analyzing related relationships + # that share the same polymorphic_type_column + if targets.empty? && type_column + # Find other relationships that might be targets for this polymorphic association + potential_targets = relationships + .select do |rel| + rel[:polymorphic_type_column] == type_column && + rel[:has_ar_forward] && + rel[:source_table] != foreign_key_table + end + .filter_map { |rel| rel[:source_table] } + .compact.uniq.sort + + targets = potential_targets + end + + { + polymorphic_type_column: type_column, + foreign_key: foreign_key, + foreign_key_table: foreign_key_table, + target_tables: targets, + has_ar_forward: group.any? { |rel| rel[:has_ar_forward] }, + has_ar_reverse: group.any? { |rel| rel[:has_ar_reverse] }, + has_fk_constraint: group.any? { |rel| rel[:has_fk_constraint] }, + relationship_count: group.size, + sample_relationship: group.first # Include a sample for debugging + } + end + + output = aggregated_polymorphic.sort_by { |rel| rel[:polymorphic_type_column] || '' } + + puts "\n" + ("=" * 80) + puts "POLYMORPHIC RELATIONSHIPS (using predicates)" + puts "=" * 80 + puts Gitlab::Json.pretty_generate(output) + end + + private + + def filter_sharded_relationships(relationships) + # Get all sharded table names + sharded_tables = Set.new + + Gitlab::Database::Dictionary.entries.each do |entry| + sharded_tables.add(entry.table_name) if entry.sharding_key.present? + end + + # Filter relationships to only include those from sharded tables + relationships.select do |rel| + sharded_tables.include?(rel[:source_table]) + end + end + end +end diff --git a/spec/lib/gitlab/reflections/database/foreign_keys_spec.rb b/spec/lib/gitlab/reflections/database/foreign_keys_spec.rb new file mode 100644 index 00000000000000..4f1b42f734cf93 --- /dev/null +++ b/spec/lib/gitlab/reflections/database/foreign_keys_spec.rb @@ -0,0 +1,255 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Gitlab::Reflections::Database::ForeignKeys, feature_category: :database do + # Mock PostgresForeignKey class to avoid database dependencies + let(:mock_postgres_foreign_key_class) do + Class.new do + def self.not_inherited + MockScope.new + end + end + end + + # Mock scope class for chaining ActiveRecord-like queries + class MockScope + def initialize(filtered_tables: [], filtered_patterns: []) + @filtered_tables = filtered_tables + @filtered_patterns = filtered_patterns + end + + def where + MockWhereClause.new(self) + end + + def to_a + # Return mock foreign key records that would pass the filters + [ + mock_foreign_key('users', 'projects', 'user_id'), + mock_foreign_key('issues', 'projects', 'project_id'), + mock_foreign_key('merge_requests', 'projects', 'target_project_id') + ].reject do |fk| + @filtered_tables.include?(fk.constrained_table_name) || + @filtered_tables.include?(fk.referenced_table_name) || + @filtered_patterns.any? { |pattern| fk.constrained_table_name.match?(pattern) } || + @filtered_patterns.any? { |pattern| fk.referenced_table_name.match?(pattern) } + end + end + + private + + def mock_foreign_key(constrained_table, referenced_table, column) + double('foreign_key', + constrained_table_name: constrained_table, + referenced_table_name: referenced_table, + constrained_column_names: [column] + ) + end + end + + # Mock where clause for building query conditions + class MockWhereClause + def initialize(scope) + @scope = scope + end + + def not(conditions) + if conditions.is_a?(Hash) + if conditions.key?(:constrained_table_name) + @scope.instance_variable_get(:@filtered_tables) << conditions[:constrained_table_name] + elsif conditions.key?(:referenced_table_name) + @scope.instance_variable_get(:@filtered_tables) << conditions[:referenced_table_name] + end + elsif conditions.is_a?(String) && conditions.include?('~') + # Handle pattern matching conditions + pattern_match = conditions.match(/(\w+_table_name) ~ \?/) + if pattern_match + # This would be handled by the calling code passing the pattern + # We'll track this in the test + end + end + @scope + end + end + + before do + # Stub the PostgresForeignKey constant + stub_const('Gitlab::Database::PostgresForeignKey', mock_postgres_foreign_key_class) + end + + describe '.for_relationships' do + subject { described_class.for_relationships } + + it 'returns a scope that filters out internal tables' do + expect(mock_postgres_foreign_key_class).to receive(:not_inherited).and_return(MockScope.new) + + result = subject + expect(result).to be_a(MockScope) + end + + it 'filters out schema_migrations table' do + scope = MockScope.new + allow(mock_postgres_foreign_key_class).to receive(:not_inherited).and_return(scope) + + # Mock the where.not chaining for schema_migrations + expect(scope).to receive(:where).and_return(double('where_clause', not: scope)).at_least(:once) + + described_class.for_relationships + end + + it 'filters out ar_internal_metadata table' do + scope = MockScope.new + allow(mock_postgres_foreign_key_class).to receive(:not_inherited).and_return(scope) + + # Mock the where.not chaining for ar_internal_metadata + expect(scope).to receive(:where).and_return(double('where_clause', not: scope)).at_least(:once) + + described_class.for_relationships + end + + context 'with system table patterns' do + it 'filters out batched_background_migration tables' do + scope = MockScope.new + allow(mock_postgres_foreign_key_class).to receive(:not_inherited).and_return(scope) + + # Mock the pattern-based filtering + expect(scope).to receive(:where).and_return(double('where_clause', not: scope)).at_least(:once) + + described_class.for_relationships + end + + it 'filters out postgres_ prefixed tables' do + scope = MockScope.new + allow(mock_postgres_foreign_key_class).to receive(:not_inherited).and_return(scope) + + expect(scope).to receive(:where).and_return(double('where_clause', not: scope)).at_least(:once) + + described_class.for_relationships + end + + it 'filters out detached_partitions table' do + scope = MockScope.new + allow(mock_postgres_foreign_key_class).to receive(:not_inherited).and_return(scope) + + expect(scope).to receive(:where).and_return(double('where_clause', not: scope)).at_least(:once) + + described_class.for_relationships + end + + it 'filters out loose_foreign_keys_deleted_records table' do + scope = MockScope.new + allow(mock_postgres_foreign_key_class).to receive(:not_inherited).and_return(scope) + + expect(scope).to receive(:where).and_return(double('where_clause', not: scope)).at_least(:once) + + described_class.for_relationships + end + + it 'filters out ghost_user_migrations table' do + scope = MockScope.new + allow(mock_postgres_foreign_key_class).to receive(:not_inherited).and_return(scope) + + expect(scope).to receive(:where).and_return(double('where_clause', not: scope)).at_least(:once) + + described_class.for_relationships + end + end + + context 'with realistic foreign key data' do + let(:mock_foreign_keys) do + [ + # Valid application foreign keys + double('fk1', constrained_table_name: 'issues', referenced_table_name: 'projects'), + double('fk2', constrained_table_name: 'merge_requests', referenced_table_name: 'users'), + + # Internal tables that should be filtered out + double('fk3', constrained_table_name: 'schema_migrations', referenced_table_name: 'projects'), + double('fk4', constrained_table_name: 'issues', referenced_table_name: 'ar_internal_metadata'), + + # System tables that should be filtered out + double('fk5', constrained_table_name: 'batched_background_migration_jobs', referenced_table_name: 'projects'), + double('fk6', constrained_table_name: 'postgres_indexes', referenced_table_name: 'users'), + double('fk7', constrained_table_name: 'detached_partitions', referenced_table_name: 'projects'), + double('fk8', constrained_table_name: 'loose_foreign_keys_deleted_records', referenced_table_name: 'users'), + double('fk9', constrained_table_name: 'ghost_user_migrations', referenced_table_name: 'projects') + ] + end + + it 'includes valid application foreign keys' do + # This test demonstrates the expected behavior without actually executing database queries + valid_tables = %w[issues merge_requests projects users] + invalid_tables = described_class::INTERNAL_TABLE_NAMES + invalid_patterns = described_class::SYSTEM_TABLE_PATTERNS + + mock_foreign_keys.each do |fk| + should_be_included = !invalid_tables.include?(fk.constrained_table_name) && + !invalid_tables.include?(fk.referenced_table_name) && + !invalid_patterns.any? { |pattern| fk.constrained_table_name.match?(pattern) } && + !invalid_patterns.any? { |pattern| fk.referenced_table_name.match?(pattern) } + + if should_be_included + expect(valid_tables).to include(fk.constrained_table_name) + expect(valid_tables).to include(fk.referenced_table_name) + end + end + end + end + end + + describe 'constants' do + describe 'INTERNAL_TABLE_NAMES' do + it 'includes schema_migrations' do + expect(described_class::INTERNAL_TABLE_NAMES).to include('schema_migrations') + end + + it 'includes ar_internal_metadata' do + expect(described_class::INTERNAL_TABLE_NAMES).to include('ar_internal_metadata') + end + + it 'contains only string values' do + described_class::INTERNAL_TABLE_NAMES.each do |table_name| + expect(table_name).to be_a(String) + end + end + end + + describe 'SYSTEM_TABLE_PATTERNS' do + it 'includes batched_background_migration pattern' do + pattern = described_class::SYSTEM_TABLE_PATTERNS.find { |p| p.source.include?('batched_background_migration') } + expect(pattern).not_to be_nil + expect('batched_background_migration_jobs').to match(pattern) + end + + it 'includes postgres_ pattern' do + pattern = described_class::SYSTEM_TABLE_PATTERNS.find { |p| p.source.include?('postgres_') } + expect(pattern).not_to be_nil + expect('postgres_indexes').to match(pattern) + end + + it 'includes detached_partitions pattern' do + pattern = described_class::SYSTEM_TABLE_PATTERNS.find { |p| p.source.include?('detached_partitions') } + expect(pattern).not_to be_nil + expect('detached_partitions').to match(pattern) + end + + it 'includes loose_foreign_keys_deleted_records pattern' do + pattern = described_class::SYSTEM_TABLE_PATTERNS.find { |p| p.source.include?('loose_foreign_keys_deleted_records') } + expect(pattern).not_to be_nil + expect('loose_foreign_keys_deleted_records').to match(pattern) + end + + it 'includes ghost_user_migrations pattern' do + pattern = described_class::SYSTEM_TABLE_PATTERNS.find { |p| p.source.include?('ghost_user_migrations') } + expect(pattern).not_to be_nil + expect('ghost_user_migrations').to match(pattern) + end + + it 'contains only Regexp objects' do + described_class::SYSTEM_TABLE_PATTERNS.each do |pattern| + expect(pattern).to be_a(Regexp) + end + end + end + end +end \ No newline at end of file diff --git a/spec/lib/gitlab/reflections/models/active_record_spec.rb b/spec/lib/gitlab/reflections/models/active_record_spec.rb new file mode 100644 index 00000000000000..006f641da8dcde --- /dev/null +++ b/spec/lib/gitlab/reflections/models/active_record_spec.rb @@ -0,0 +1,452 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Gitlab::Reflections::Models::ActiveRecord, feature_category: :database do + # Mock ActiveRecord::Base to avoid loading actual models + let(:mock_active_record_base) do + Class.new do + def self.descendants + [ + MockUserModel, + MockProjectModel, + MockIssueModel, + MockAbstractModel, + MockAnonymousModel, + MockActiveRecordInternalModel, + MockActiveStorageModel, + MockTestModel, + MockFixtureModel, + MockRSpecModel + ] + end + end + end + + # Mock application models + class MockUserModel + def self.name + 'User' + end + + def self.table_name + 'users' + end + + def self.abstract_class? + false + end + end + + class MockProjectModel + def self.name + 'Project' + end + + def self.table_name + 'projects' + end + + def self.abstract_class? + false + end + end + + class MockIssueModel + def self.name + 'Issue' + end + + def self.table_name + 'issues' + end + + def self.abstract_class? + false + end + end + + # Mock models that should be excluded + class MockAbstractModel + def self.name + 'AbstractModel' + end + + def self.table_name + 'abstract_models' + end + + def self.abstract_class? + true + end + end + + class MockAnonymousModel + def self.name + nil + end + + def self.table_name + 'anonymous_table' + end + + def self.abstract_class? + false + end + end + + class MockActiveRecordInternalModel + def self.name + 'ActiveRecord::InternalModel' + end + + def self.table_name + 'internal_table' + end + + def self.abstract_class? + false + end + end + + class MockActiveStorageModel + def self.name + 'ActiveStorage::Blob' + end + + def self.table_name + 'active_storage_blobs' + end + + def self.abstract_class? + false + end + end + + class MockTestModel + def self.name + 'UserTest' + end + + def self.table_name + 'user_tests' + end + + def self.abstract_class? + false + end + end + + class MockFixtureModel + def self.name + 'ProjectFixture' + end + + def self.table_name + 'project_fixtures' + end + + def self.abstract_class? + false + end + end + + class MockRSpecModel + def self.name + 'RSpec::TestModel' + end + + def self.table_name + 'rspec_test_models' + end + + def self.abstract_class? + false + end + end + + class MockModelWithoutTable + def self.name + 'ModelWithoutTable' + end + + def self.table_name + nil + end + + def self.abstract_class? + false + end + end + + before do + # Stub ActiveRecord::Base constant + stub_const('ActiveRecord::Base', mock_active_record_base) + + # Clear any existing singleton instance + described_class.instance_variable_set(:@singleton__instance__, nil) + end + + describe '#initialize' do + it 'builds indexes on initialization' do + instance = described_class.instance + + expect(instance.table_to_model_index).to be_a(Hash) + expect(instance.model_to_table_index).to be_a(Hash) + expect(instance.models).to be_an(Array) + end + end + + describe '#models' do + subject { described_class.instance.models } + + it 'includes valid application models' do + expect(subject).to include(MockUserModel) + expect(subject).to include(MockProjectModel) + expect(subject).to include(MockIssueModel) + end + + it 'excludes abstract models' do + expect(subject).not_to include(MockAbstractModel) + end + + it 'excludes anonymous models' do + expect(subject).not_to include(MockAnonymousModel) + end + + it 'excludes ActiveRecord internal models' do + expect(subject).not_to include(MockActiveRecordInternalModel) + end + + it 'excludes ActiveStorage models' do + expect(subject).not_to include(MockActiveStorageModel) + end + + it 'excludes test models' do + expect(subject).not_to include(MockTestModel) + end + + it 'excludes fixture models' do + expect(subject).not_to include(MockFixtureModel) + end + + it 'excludes RSpec models' do + expect(subject).not_to include(MockRSpecModel) + end + + it 'excludes models without table names' do + # Add a model without table name to the descendants + allow(mock_active_record_base).to receive(:descendants).and_return([ + MockUserModel, + MockProjectModel, + MockModelWithoutTable + ]) + + # Clear singleton to force rebuild + described_class.instance_variable_set(:@singleton__instance__, nil) + + expect(described_class.instance.models).not_to include(MockModelWithoutTable) + end + + it 'handles duplicate model names' do + # Create duplicate models with same name + duplicate_model = Class.new do + def self.name + 'User' + end + + def self.table_name + 'duplicate_users' + end + + def self.abstract_class? + false + end + end + + allow(mock_active_record_base).to receive(:descendants).and_return([ + MockUserModel, + duplicate_model + ]) + + # Clear singleton to force rebuild + described_class.instance_variable_set(:@singleton__instance__, nil) + + # Should only include one of the duplicate models + user_models = described_class.instance.models.select { |m| m.name == 'User' } + expect(user_models.length).to eq(1) + end + end + + describe '#table_to_model_index' do + subject { described_class.instance.table_to_model_index } + + it 'maps table names to model names' do + expect(subject['users']).to eq('User') + expect(subject['projects']).to eq('Project') + expect(subject['issues']).to eq('Issue') + end + + it 'does not include excluded models' do + expect(subject).not_to have_key('abstract_models') + expect(subject).not_to have_key('internal_table') + expect(subject).not_to have_key('active_storage_blobs') + expect(subject).not_to have_key('user_tests') + expect(subject).not_to have_key('project_fixtures') + expect(subject).not_to have_key('rspec_test_models') + end + + it 'does not include anonymous models' do + expect(subject).not_to have_key('anonymous_table') + end + end + + describe '#model_to_table_index' do + subject { described_class.instance.model_to_table_index } + + it 'maps model names to table names' do + expect(subject['User']).to eq('users') + expect(subject['Project']).to eq('projects') + expect(subject['Issue']).to eq('issues') + end + + it 'does not include excluded models' do + expect(subject).not_to have_key('AbstractModel') + expect(subject).not_to have_key('ActiveRecord::InternalModel') + expect(subject).not_to have_key('ActiveStorage::Blob') + expect(subject).not_to have_key('UserTest') + expect(subject).not_to have_key('ProjectFixture') + expect(subject).not_to have_key('RSpec::TestModel') + end + end + + describe 'singleton behavior' do + it 'returns the same instance on multiple calls' do + instance1 = described_class.instance + instance2 = described_class.instance + + expect(instance1).to be(instance2) + end + + it 'builds indexes only once' do + expect(mock_active_record_base).to receive(:descendants).once.and_call_original + + described_class.instance + described_class.instance + end + end + + describe 'private methods' do + let(:instance) { described_class.instance } + + describe '#discover_models' do + it 'returns ActiveRecord::Base descendants' do + models = instance.send(:discover_models) + expect(models).to include(MockUserModel) + expect(models).to include(MockProjectModel) + end + end + + describe '#skip_model?' do + it 'skips abstract models' do + expect(instance.send(:skip_model?, MockAbstractModel)).to be true + end + + it 'skips anonymous models' do + expect(instance.send(:skip_model?, MockAnonymousModel)).to be true + end + + it 'skips models without table names' do + expect(instance.send(:skip_model?, MockModelWithoutTable)).to be true + end + + it 'skips excluded models' do + expect(instance.send(:skip_model?, MockActiveRecordInternalModel)).to be true + expect(instance.send(:skip_model?, MockActiveStorageModel)).to be true + expect(instance.send(:skip_model?, MockTestModel)).to be true + expect(instance.send(:skip_model?, MockFixtureModel)).to be true + expect(instance.send(:skip_model?, MockRSpecModel)).to be true + end + + it 'does not skip valid application models' do + expect(instance.send(:skip_model?, MockUserModel)).to be false + expect(instance.send(:skip_model?, MockProjectModel)).to be false + expect(instance.send(:skip_model?, MockIssueModel)).to be false + end + end + + describe '#excluded_model?' do + it 'excludes ActiveRecord internal models' do + expect(instance.send(:excluded_model?, MockActiveRecordInternalModel)).to be true + end + + it 'excludes ActiveStorage models' do + expect(instance.send(:excluded_model?, MockActiveStorageModel)).to be true + end + + it 'excludes test models' do + expect(instance.send(:excluded_model?, MockTestModel)).to be true + end + + it 'excludes fixture models' do + expect(instance.send(:excluded_model?, MockFixtureModel)).to be true + end + + it 'excludes RSpec models' do + expect(instance.send(:excluded_model?, MockRSpecModel)).to be true + end + + it 'does not exclude application models' do + expect(instance.send(:excluded_model?, MockUserModel)).to be false + expect(instance.send(:excluded_model?, MockProjectModel)).to be false + end + end + + describe '#filter_models' do + let(:all_models) do + [ + MockUserModel, + MockProjectModel, + MockAbstractModel, + MockAnonymousModel, + MockActiveRecordInternalModel, + MockTestModel + ] + end + + it 'filters out excluded models' do + filtered = instance.send(:filter_models, all_models) + + expect(filtered).to include(MockUserModel) + expect(filtered).to include(MockProjectModel) + expect(filtered).not_to include(MockAbstractModel) + expect(filtered).not_to include(MockAnonymousModel) + expect(filtered).not_to include(MockActiveRecordInternalModel) + expect(filtered).not_to include(MockTestModel) + end + + it 'handles duplicate model names' do + duplicate_user = Class.new do + def self.name + 'User' + end + + def self.table_name + 'users' + end + + def self.abstract_class? + false + end + end + + models_with_duplicate = [MockUserModel, duplicate_user] + filtered = instance.send(:filter_models, models_with_duplicate) + + # Should only include one User model + user_models = filtered.select { |m| m.name == 'User' } + expect(user_models.length).to eq(1) + end + end + end +end \ No newline at end of file diff --git a/spec/lib/gitlab/reflections/polymorphic_edge_cases_spec.rb b/spec/lib/gitlab/reflections/polymorphic_edge_cases_spec.rb new file mode 100644 index 00000000000000..380505a188ae05 --- /dev/null +++ b/spec/lib/gitlab/reflections/polymorphic_edge_cases_spec.rb @@ -0,0 +1,233 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe 'Polymorphic Relationship Edge Cases', feature_category: :database do + describe 'complex polymorphic scenarios' do + context 'with multiple polymorphic targets' do + it 'handles models that can be targets of multiple polymorphic relationships' do + # Test that models like User can be targets of multiple polymorphic relationships + # e.g., author_type, assignee_type, etc. + + builder = Gitlab::Reflections::Relationships::Builder.new + relationships = builder.build_relationships + + # Find all relationships where User is a polymorphic target + user_polymorphic_relationships = relationships.select do |relationship| + relationship[:polymorphic] && + relationship[:polymorphic_targets]&.include?('User') + end + + expect(user_polymorphic_relationships).not_to be_empty + + # Verify that each relationship has proper polymorphic metadata + user_polymorphic_relationships.each do |relationship| + expect(relationship[:polymorphic_type_column]).to be_present + expect(relationship[:polymorphic_name]).to be_present + expect(relationship[:polymorphic_targets]).to include('User') + end + end + end + + context 'with polymorphic relationships without matching counterparts' do + it 'handles belongs_to polymorphic without corresponding has_many' do + # Create a test model with orphaned polymorphic belongs_to + test_model = Class.new(ApplicationRecord) do + self.table_name = 'test_orphaned_poly' + belongs_to :orphaned_target, polymorphic: true + end + + # Mock the reflection + reflection = double('reflection', + macro: :belongs_to, + polymorphic?: true, + foreign_key: 'orphaned_target_id', + foreign_type: 'orphaned_target_type', + options: {} + ) + + handler = Gitlab::Reflections::Relationships::Handlers::BelongsToHandler.new( + test_model, :orphaned_target, reflection + ) + + relationship = handler.build_relationship + + expect(relationship[:polymorphic]).to be true + expect(relationship[:polymorphic_type_column]).to eq('orphaned_target_type') + expect(relationship[:polymorphic_targets]).to eq([]) + end + end + + context 'with custom polymorphic column names' do + it 'handles custom foreign_type column names' do + # Test polymorphic relationships with custom type column names + test_model = Class.new(ApplicationRecord) do + self.table_name = 'test_custom_poly' + belongs_to :custom_target, polymorphic: true, foreign_type: 'custom_type_col' + end + + reflection = double('reflection', + macro: :belongs_to, + polymorphic?: true, + foreign_key: 'custom_target_id', + foreign_type: 'custom_type_col', + options: { foreign_type: 'custom_type_col' } + ) + + handler = Gitlab::Reflections::Relationships::Handlers::BelongsToHandler.new( + test_model, :custom_target, reflection + ) + + relationship = handler.build_relationship + + expect(relationship[:polymorphic_type_column]).to eq('custom_type_col') + end + end + + context 'with nested polymorphic relationships' do + it 'handles polymorphic relationships in STI hierarchies' do + # Test polymorphic relationships where the target is part of STI hierarchy + builder = Gitlab::Reflections::Relationships::Builder.new + relationships = builder.build_relationships + + # Find polymorphic relationships that involve STI models + sti_polymorphic_relationships = relationships.select do |relationship| + relationship[:polymorphic] && + relationship[:polymorphic_targets]&.any? { |target| target.include?('::') } + end + + # Verify STI models are handled correctly + sti_polymorphic_relationships.each do |relationship| + expect(relationship[:polymorphic_targets]).to be_an(Array) + expect(relationship[:polymorphic_type_column]).to be_present + end + end + end + + context 'with polymorphic through relationships' do + it 'handles has_many through with polymorphic associations' do + # Test complex relationships like has_many :something, through: :polymorphic_association + builder = Gitlab::Reflections::Relationships::Builder.new + relationships = builder.build_relationships + + # Find through relationships that involve polymorphic associations + through_polymorphic_edges = relationships.select do |edge| + edge[:through_table] && edge[:polymorphic] + end + + through_polymorphic_edges.each do |edge| + expect(edge[:through_table]).to be_present + expect(edge[:polymorphic]).to be true + expect(edge[:polymorphic_type_column]).to be_present + end + end + end + + context 'with polymorphic scoped associations' do + it 'handles polymorphic associations with scopes' do + # Test polymorphic associations that have additional scopes/conditions + builder = Gitlab::Reflections::Relationships::Builder.new + relationships = builder.build_relationships + + # Find polymorphic edges with scope conditions + scoped_polymorphic_edges = relationships.select do |edge| + edge[:polymorphic] && edge[:scope_conditions] + end + + scoped_polymorphic_edges.each do |edge| + expect(edge[:scope_conditions]).to be_present + expect(edge[:polymorphic]).to be true + end + end + end + + context 'with bidirectional polymorphic relationships' do + it 'correctly merges polymorphic belongs_to with has_many as: relationships' do + builder = Gitlab::Reflections::Relationships::Builder.new + relationships = builder.build_relationships + + # Find bidirectional polymorphic relationships + bidirectional_polymorphic = relationships.select do |edge| + edge[:polymorphic] && + edge[:has_ar_forward] && + edge[:has_ar_reverse] + end + + bidirectional_polymorphic.each do |edge| + expect(edge[:forward_association]).to be_present + expect(edge[:reverse_association]).to be_present + expect(edge[:polymorphic_type_column]).to be_present + expect(edge[:polymorphic_targets]).to be_an(Array) + end + end + end + + context 'with Member model edge cases' do + it 'handles Member polymorphic relationship correctly' do + builder = Gitlab::Reflections::Relationships::Builder.new + relationships = builder.build_relationships + + # Find Member's polymorphic relationship + member_edges = relationships.select do |edge| + edge[:source_table] == 'members' || edge[:target_table] == 'members' + end + + # Should have polymorphic edges for Member + polymorphic_member_edges = member_edges.select { |edge| edge[:polymorphic] } + expect(polymorphic_member_edges).not_to be_empty + + polymorphic_member_edges.each do |edge| + if edge[:source_table] == 'members' + # belongs_to :source, polymorphic: true + expect(edge[:polymorphic_type_column]).to eq('source_type') + expect(edge[:has_ar_reverse]).to be true + else + # has_many :members, as: :source (from Group/Project) + expect(edge[:polymorphic_type_column]).to eq('source_type') + expect(edge[:has_ar_forward]).to be true + end + end + end + + it 'identifies Group and Project as Member polymorphic targets' do + builder = Gitlab::Reflections::Relationships::Builder.new + relationships = builder.build_relationships + + # Find the specific polymorphic relationship for Member's source + member_source_edges = relationships.select do |edge| + edge[:polymorphic] && + edge[:polymorphic_type_column] == 'source_type' && + (edge[:source_table] == 'members' || edge[:target_table] == 'members') + end + + expect(member_source_edges).not_to be_empty + + # Collect all polymorphic targets from these edges + all_targets = member_source_edges.flat_map { |edge| edge[:polymorphic_targets] || [] }.uniq + + expect(all_targets).to include('Group') + expect(all_targets).to include('Project') + end + end + end + + describe 'performance considerations' do + it 'handles large numbers of polymorphic relationships efficiently' do + # Test that the system can handle many polymorphic relationships without performance issues + start_time = Time.current + + builder = Gitlab::Reflections::Relationships::Builder.new + relationships = builder.build_relationships + + end_time = Time.current + duration = end_time - start_time + + # Should complete within reasonable time (adjust threshold as needed) + expect(duration).to be < 30.seconds + + # Should find polymorphic relationships + polymorphic_edges = relationships.select { |edge| edge[:polymorphic] } + expect(polymorphic_edges).not_to be_empty + end + end +end diff --git a/spec/lib/gitlab/reflections/polymorphic_json_task_spec.rb b/spec/lib/gitlab/reflections/polymorphic_json_task_spec.rb new file mode 100644 index 00000000000000..8458b9062b520e --- /dev/null +++ b/spec/lib/gitlab/reflections/polymorphic_json_task_spec.rb @@ -0,0 +1,167 @@ +# frozen_string_literal: true + +require 'spec_helper' +require 'rake' + +RSpec.describe 'rails db:relationships:polymorphic_json task', feature_category: :database do + before_all do + Rake.application.rake_require 'tasks/database_relationships' + end + + let(:task) { Rake::Task['db:relationships:polymorphic_json'] } + + before do + task.reenable + end + + describe 'polymorphic_json task output' do + it 'includes Member model relationships in the output' do + output = capture_task_output { task.invoke } + parsed_output = Gitlab::Json.parse(output) + + # Member model should appear in the output with source_type column + expect(parsed_output).to have_key('members') + expect(parsed_output['members']).to have_key('source_type') + expect(parsed_output['members']['source_type']).to include('Group', 'Project') + end + + it 'includes all polymorphic relationships from Group model' do + output = capture_task_output { task.invoke } + parsed_output = Gitlab::Json.parse(output) + + # Groups should show up as targets for members polymorphic relationship + expect(parsed_output['members']['source_type']).to include('Group') + end + + it 'includes all polymorphic relationships from Project model' do + output = capture_task_output { task.invoke } + parsed_output = Gitlab::Json.parse(output) + + # Projects should show up as targets for members polymorphic relationship + expect(parsed_output['members']['source_type']).to include('Project') + end + + it 'correctly identifies polymorphic type columns' do + output = capture_task_output { task.invoke } + parsed_output = Gitlab::Json.parse(output) + + # Verify that common polymorphic type columns are present + expect(parsed_output).to have_key('notes') + expect(parsed_output['notes']).to have_key('noteable_type') + + expect(parsed_output).to have_key('todos') + expect(parsed_output['todos']).to have_key('target_type') + end + + it 'handles multiple polymorphic relationships on the same table' do + output = capture_task_output { task.invoke } + parsed_output = Gitlab::Json.parse(output) + + # Some tables have multiple polymorphic relationships + # For example, events table has both target_type and author_type + expect(parsed_output['events'].keys.length).to be >= 1 if parsed_output.key?('events') + end + + it 'excludes non-polymorphic relationships' do + output = capture_task_output { task.invoke } + parsed_output = Gitlab::Json.parse(output) + + # Regular foreign key relationships should not appear + # Only polymorphic type columns should be present + parsed_output.each do |table_name, columns| + columns.each do |column_name, models| + expect(column_name).to end_with('_type'), + "Expected #{table_name}.#{column_name} to be a polymorphic type column" + expect(models).to be_an(Array), + "Expected #{table_name}.#{column_name} to have an array of model names" + end + end + end + + it 'sorts model names alphabetically within each polymorphic relationship' do + output = capture_task_output { task.invoke } + parsed_output = Gitlab::Json.parse(output) + + parsed_output.each do |table_name, columns| + columns.each do |column_name, models| + expect(models).to eq(models.sort), + "Expected #{table_name}.#{column_name} models to be sorted alphabetically" + end + end + end + + context 'with Member model specifically' do + it 'correctly identifies Group and Project as polymorphic targets' do + output = capture_task_output { task.invoke } + parsed_output = Gitlab::Json.parse(output) + + expect(parsed_output).to have_key('members') + expect(parsed_output['members']).to have_key('source_type') + + member_targets = parsed_output['members']['source_type'] + expect(member_targets).to include('Group') + expect(member_targets).to include('Project') + expect(member_targets).to be_sorted + end + + it 'does not include other models as Member polymorphic targets' do + output = capture_task_output { task.invoke } + parsed_output = Gitlab::Json.parse(output) + + member_targets = parsed_output['members']['source_type'] + + # Member's source should only be Group and Project + # It should not include other models like User, Issue, etc. + expect(member_targets).to match_array(%w[Group Project]) + end + end + end + + describe 'edge cases' do + it 'handles empty polymorphic relationships gracefully' do + output = capture_task_output { task.invoke } + parsed_output = Gitlab::Json.parse(output) + + # Some polymorphic relationships might have empty arrays + # This should be handled gracefully + parsed_output.each do |table_name, columns| + columns.each do |column_name, models| + expect(models).to be_an(Array) + # Empty arrays are acceptable + end + end + end + + it 'produces valid JSON output' do + output = capture_task_output { task.invoke } + + expect { Gitlab::Json.parse(output) }.not_to raise_error + end + + it 'handles models with multiple polymorphic associations' do + output = capture_task_output { task.invoke } + parsed_output = Gitlab::Json.parse(output) + + # Some models might have multiple polymorphic associations + # Each should be handled correctly + parsed_output.each do |table_name, columns| + expect(columns).to be_a(Hash) + columns.each do |column_name, models| + expect(column_name).to be_a(String) + expect(models).to be_an(Array) + end + end + end + end + + private + + def capture_task_output + original_stdout = $stdout + $stdout = StringIO.new + yield + $stdout.string + ensure + $stdout = original_stdout + end +end diff --git a/spec/lib/gitlab/reflections/polymorphic_relationships_spec.rb b/spec/lib/gitlab/reflections/polymorphic_relationships_spec.rb new file mode 100644 index 00000000000000..4369948185b123 --- /dev/null +++ b/spec/lib/gitlab/reflections/polymorphic_relationships_spec.rb @@ -0,0 +1,159 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe 'Member polymorphic relationships in rails task', feature_category: :database do + describe 'polymorphic_json task logic' do + it 'correctly processes Member polymorphic relationships' do + # This test documents the expected behavior for Member polymorphic relationships + # in the rails db:relationships:polymorphic_json task + + # Mock edges representing Member's polymorphic relationship structure + member_belongs_to_edge = { + source_table: 'members', + target_table: nil, + polymorphic: true, + polymorphic_type_column: 'source_type', + has_ar_reverse: true, + reverse_association: { + name: 'source', + type: 'belongs_to', + model: 'GroupMember' + } + } + + # Mock Group's has_many relationship with as: :source + namespace_has_many_edge = { + source_table: 'namespaces', + target_table: 'members', + polymorphic: true, + polymorphic_type_column: 'source_type', + has_ar_forward: true, + forward_association: { + name: 'group_members', + type: 'has_many', + model: 'Group' + } + } + + # Mock Project's has_many relationship with as: :source + project_has_many_edge = { + source_table: 'projects', + target_table: 'members', + polymorphic: true, + polymorphic_type_column: 'source_type', + has_ar_forward: true, + forward_association: { + name: 'project_members', + type: 'has_many', + model: 'Project' + } + } + + edges = [member_belongs_to_edge, namespace_has_many_edge, project_has_many_edge] + + # Simulate the polymorphic_json task logic + polymorphic_data = {} + + edges.each do |edge| + next unless edge[:polymorphic] && edge[:polymorphic_type_column] + + # For has_many/has_one side (as: :something) + if edge[:has_ar_forward] && edge[:forward_association] + table_name = edge[:source_table] + type_column = edge[:polymorphic_type_column] + + polymorphic_data[table_name] ||= {} + polymorphic_data[table_name][type_column] ||= Set.new + end + + # For belongs_to side (polymorphic: true) + next unless edge[:has_ar_reverse] && edge[:reverse_association] + + type_column = edge[:polymorphic_type_column] + source_model = edge[:reverse_association][:model] + + # Look for the corresponding has_many/has_one relationships + edges.each do |other_edge| + next unless other_edge[:polymorphic] && + other_edge[:polymorphic_type_column] == type_column && + other_edge[:has_ar_forward] + + target_table = other_edge[:source_table] + + polymorphic_data[target_table] ||= {} + polymorphic_data[target_table][type_column] ||= Set.new + polymorphic_data[target_table][type_column] << source_model + end + end + + # Convert sets to sorted arrays + output = {} + polymorphic_data.each do |table_name, columns| + output[table_name] = {} + columns.each do |type_column, models_set| + output[table_name][type_column] = models_set.to_a.sort + end + end + + # Verify that Member models appear in the output + expect(output).to have_key('namespaces') + expect(output['namespaces']).to have_key('source_type') + expect(output['namespaces']['source_type']).to include('GroupMember') + + expect(output).to have_key('projects') + expect(output['projects']).to have_key('source_type') + expect(output['projects']['source_type']).to include('GroupMember') + end + end + + describe 'Member model polymorphic association' do + it 'has the expected polymorphic belongs_to :source association' do + expect(Member.reflections).to have_key('source') + + source_reflection = Member.reflections['source'] + expect(source_reflection.macro).to eq(:belongs_to) + expect(source_reflection.polymorphic?).to be true + expect(source_reflection.foreign_type).to eq('source_type') + expect(source_reflection.foreign_key).to eq('source_id') + end + end + + describe 'Group model polymorphic associations' do + it 'has has_many associations with as: :source pointing to Member subclasses' do + group_member_associations = Group.reflections.select do |name, reflection| + reflection.macro == :has_many && + reflection.options[:as] == :source && + name.include?('member') + end + + expect(group_member_associations).not_to be_empty + + # Check that at least one association points to GroupMember or Member + has_member_association = group_member_associations.any? do |name, reflection| + reflection.class_name&.include?('Member') + end + + expect(has_member_association).to be true + end + end + + describe 'Project model polymorphic associations' do + it 'has has_many associations with as: :source pointing to Member subclasses' do + project_member_associations = Project.reflections.select do |name, reflection| + reflection.macro == :has_many && + reflection.options[:as] == :source && + name.include?('member') + end + + expect(project_member_associations).not_to be_empty + + # Check that at least one association points to ProjectMember or Member + has_member_association = project_member_associations.any? do |name, reflection| + reflection.class_name&.include?('Member') + end + + expect(has_member_association).to be true + end + end +end diff --git a/spec/lib/gitlab/reflections/relationships/active_record_spec.rb b/spec/lib/gitlab/reflections/relationships/active_record_spec.rb new file mode 100644 index 00000000000000..d5564cc1c968bf --- /dev/null +++ b/spec/lib/gitlab/reflections/relationships/active_record_spec.rb @@ -0,0 +1,443 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Gitlab::Reflections::Relationships::ActiveRecord, feature_category: :database do + # Mock models reflection instance + let(:mock_models_reflection) do + double('models_reflection', models: mock_models) + end + + let(:mock_models) do + [MockUserModel, MockProjectModel, MockIssueModel] + end + + # Mock ActiveRecord models with reflections + class MockUserModel + def self.name + 'User' + end + + def self.table_name + 'users' + end + + def self.reflections + { + 'projects' => MockHasManyReflection.new(:has_many, 'projects', 'Project'), + 'profile' => MockHasOneReflection.new(:has_one, 'profile', 'Profile'), + 'avatar' => MockHasOneAttachedReflection.new(:has_one_attached, 'avatar'), + 'uploads' => MockHasManyAttachedReflection.new(:has_many_attached, 'uploads') + } + end + end + + class MockProjectModel + def self.name + 'Project' + end + + def self.table_name + 'projects' + end + + def self.reflections + { + 'owner' => MockBelongsToReflection.new(:belongs_to, 'owner', 'User'), + 'issues' => MockHasManyReflection.new(:has_many, 'issues', 'Issue'), + 'tags' => MockHabtmReflection.new(:has_and_belongs_to_many, 'tags', 'Tag') + } + end + end + + class MockIssueModel + def self.name + 'Issue' + end + + def self.table_name + 'issues' + end + + def self.reflections + { + 'project' => MockBelongsToReflection.new(:belongs_to, 'project', 'Project'), + 'author' => MockBelongsToReflection.new(:belongs_to, 'author', 'User'), + 'assignees' => MockHasManyReflection.new(:has_many, 'assignees', 'User'), + 'unsupported_relation' => MockUnsupportedReflection.new(:custom_macro, 'unsupported', 'Other') + } + end + end + + # Mock reflection classes + class MockBelongsToReflection + attr_reader :macro, :name, :class_name + + def initialize(macro, name, class_name) + @macro = macro + @name = name + @class_name = class_name + end + + def polymorphic? + false + end + + def foreign_key + "#{name}_id" + end + + def options + {} + end + end + + class MockHasManyReflection + attr_reader :macro, :name, :class_name + + def initialize(macro, name, class_name) + @macro = macro + @name = name + @class_name = class_name + end + + def polymorphic? + false + end + + def foreign_key + "#{name.singularize}_id" + end + + def options + {} + end + end + + class MockHasOneReflection + attr_reader :macro, :name, :class_name + + def initialize(macro, name, class_name) + @macro = macro + @name = name + @class_name = class_name + end + + def polymorphic? + false + end + + def foreign_key + "#{name}_id" + end + + def options + {} + end + end + + class MockHabtmReflection + attr_reader :macro, :name, :class_name + + def initialize(macro, name, class_name) + @macro = macro + @name = name + @class_name = class_name + end + + def options + { join_table: "#{name}_join_table" } + end + end + + class MockHasOneAttachedReflection + attr_reader :macro, :name + + def initialize(macro, name) + @macro = macro + @name = name + end + + def options + {} + end + end + + class MockHasManyAttachedReflection + attr_reader :macro, :name + + def initialize(macro, name) + @macro = macro + @name = name + end + + def options + {} + end + end + + class MockUnsupportedReflection + attr_reader :macro, :name, :class_name + + def initialize(macro, name, class_name) + @macro = macro + @name = name + @class_name = class_name + end + end + + # Mock handler classes + let(:mock_belongs_to_handler) { double('belongs_to_handler') } + let(:mock_has_association_handler) { double('has_association_handler') } + let(:mock_habtm_handler) { double('habtm_handler') } + let(:mock_active_storage_handler) { double('active_storage_handler') } + + before do + # Stub the models reflection + allow(Gitlab::Reflections::Models::ActiveRecord).to receive(:instance).and_return(mock_models_reflection) + + # Stub handler classes + stub_const('Gitlab::Reflections::Relationships::Handlers::BelongsToHandler', double('BelongsToHandler')) + stub_const('Gitlab::Reflections::Relationships::Handlers::HasAssociationHandler', double('HasAssociationHandler')) + stub_const('Gitlab::Reflections::Relationships::Handlers::HabtmHandler', double('HabtmHandler')) + stub_const('Gitlab::Reflections::Relationships::Handlers::ActiveStorageHandler', double('ActiveStorageHandler')) + + # Mock handler instantiation and behavior + allow(Gitlab::Reflections::Relationships::Handlers::BelongsToHandler).to receive(:new).and_return(mock_belongs_to_handler) + allow(Gitlab::Reflections::Relationships::Handlers::HasAssociationHandler).to receive(:new).and_return(mock_has_association_handler) + allow(Gitlab::Reflections::Relationships::Handlers::HabtmHandler).to receive(:new).and_return(mock_habtm_handler) + allow(Gitlab::Reflections::Relationships::Handlers::ActiveStorageHandler).to receive(:new).and_return(mock_active_storage_handler) + + # Mock handler build_relationship methods + allow(mock_belongs_to_handler).to receive(:build_relationship).and_return({ type: 'belongs_to', +source_table: 'issues', target_table: 'projects' }) + allow(mock_has_association_handler).to receive(:build_relationship).and_return({ type: 'has_many', +source_table: 'projects', target_table: 'issues' }) + allow(mock_habtm_handler).to receive(:build_relationship).and_return({ type: 'has_and_belongs_to_many', +source_table: 'projects', target_table: 'tags' }) + allow(mock_active_storage_handler).to receive(:build_relationship).and_return({ type: 'active_storage', +source_table: 'users', target_table: 'active_storage_attachments' }) + end + + describe '#initialize' do + it 'initializes with models reflection instance' do + instance = described_class.new + expect(Gitlab::Reflections::Models::ActiveRecord).to have_received(:instance) + end + end + + describe '#extract_relationships' do + subject { described_class.new.extract_relationships } + + it 'returns an array of relationships' do + expect(subject).to be_an(Array) + end + + it 'processes all supported relationship types' do + relationships = subject + + # Should process belongs_to relationships + expect(mock_belongs_to_handler).to have_received(:build_relationship).at_least(:once) + + # Should process has_many/has_one relationships + expect(mock_has_association_handler).to have_received(:build_relationship).at_least(:once) + + # Should process has_and_belongs_to_many relationships + expect(mock_habtm_handler).to have_received(:build_relationship).at_least(:once) + + # Should process Active Storage relationships + expect(mock_active_storage_handler).to have_received(:build_relationship).at_least(:once) + end + + it 'skips unsupported relationship types' do + # The unsupported_relation from MockIssueModel should be skipped + # We can verify this by checking that no handler was created for it + relationships = subject + + # Verify that only supported relationships are processed + expect(relationships).to all(be_a(Hash)) + end + + it 'filters out nil relationships' do + # Mock one handler to return nil + allow(mock_belongs_to_handler).to receive(:build_relationship).and_return(nil) + + relationships = subject + expect(relationships).not_to include(nil) + end + end + + describe 'constants' do + describe 'SUPPORTED_RELATIONSHIPS' do + it 'includes all expected relationship types' do + expected_relationships = %w[ + has_many has_one belongs_to has_and_belongs_to_many + has_many_attached has_one_attached + ] + + expect(described_class::SUPPORTED_RELATIONSHIPS).to match_array(expected_relationships) + end + + it 'contains only string values' do + described_class::SUPPORTED_RELATIONSHIPS.each do |relationship| + expect(relationship).to be_a(String) + end + end + end + + describe 'RELATIONSHIP_HANDLERS' do + it 'maps relationship types to handler classes' do + expected_mappings = { + 'belongs_to' => 'Gitlab::Reflections::Relationships::Handlers::BelongsToHandler', + 'has_many' => 'Gitlab::Reflections::Relationships::Handlers::HasAssociationHandler', + 'has_one' => 'Gitlab::Reflections::Relationships::Handlers::HasAssociationHandler', + 'has_and_belongs_to_many' => 'Gitlab::Reflections::Relationships::Handlers::HabtmHandler', + 'has_many_attached' => 'Gitlab::Reflections::Relationships::Handlers::ActiveStorageHandler', + 'has_one_attached' => 'Gitlab::Reflections::Relationships::Handlers::ActiveStorageHandler' + } + + expected_mappings.each do |relationship_type, expected_handler_name| + handler_class = described_class::RELATIONSHIP_HANDLERS[relationship_type] + expect(handler_class.to_s).to eq(expected_handler_name) + end + end + + it 'has handlers for all supported relationships' do + described_class::SUPPORTED_RELATIONSHIPS.each do |relationship| + expect(described_class::RELATIONSHIP_HANDLERS).to have_key(relationship) + end + end + end + end + + describe 'private methods' do + let(:instance) { described_class.new } + + describe '#get_source_data' do + it 'returns models from the models reflection' do + source_data = instance.send(:get_source_data) + expect(source_data).to eq(mock_models) + end + end + + describe '#filter_source_data' do + it 'returns models unchanged' do + filtered_data = instance.send(:filter_source_data, mock_models) + expect(filtered_data).to eq(mock_models) + end + end + + describe '#map_to_relationships' do + it 'processes reflections from all models' do + relationships = instance.send(:map_to_relationships, mock_models) + + expect(relationships).to be_an(Array) + expect(relationships).not_to be_empty + end + + it 'creates relationships for supported reflection types' do + relationships = instance.send(:map_to_relationships, [MockUserModel]) + + # Should create relationships for has_many, has_one, and Active Storage attachments + expect(relationships.length).to be >= 4 # projects, profile, avatar, uploads + end + + it 'skips unsupported reflection types' do + relationships = instance.send(:map_to_relationships, [MockIssueModel]) + + # Should not include the unsupported_relation + expect(relationships).to all(be_a(Hash)) + end + end + + describe '#build_relationship_for_reflection' do + it 'creates appropriate handler for belongs_to reflection' do + reflection = MockBelongsToReflection.new(:belongs_to, 'project', 'Project') + + relationship = instance.send(:build_relationship_for_reflection, MockIssueModel, 'project', reflection) + + expect(Gitlab::Reflections::Relationships::Handlers::BelongsToHandler).to have_received(:new) + .with(MockIssueModel, 'project', reflection) + expect(relationship).to eq({ type: 'belongs_to', source_table: 'issues', target_table: 'projects' }) + end + + it 'creates appropriate handler for has_many reflection' do + reflection = MockHasManyReflection.new(:has_many, 'issues', 'Issue') + + relationship = instance.send(:build_relationship_for_reflection, MockProjectModel, 'issues', reflection) + + expect(Gitlab::Reflections::Relationships::Handlers::HasAssociationHandler).to have_received(:new) + .with(MockProjectModel, 'issues', reflection) + expect(relationship).to eq({ type: 'has_many', source_table: 'projects', target_table: 'issues' }) + end + + it 'creates appropriate handler for has_one reflection' do + reflection = MockHasOneReflection.new(:has_one, 'profile', 'Profile') + + relationship = instance.send(:build_relationship_for_reflection, MockUserModel, 'profile', reflection) + + expect(Gitlab::Reflections::Relationships::Handlers::HasAssociationHandler).to have_received(:new) + .with(MockUserModel, 'profile', reflection) + expect(relationship).to eq({ type: 'has_many', source_table: 'projects', target_table: 'issues' }) + end + + it 'creates appropriate handler for has_and_belongs_to_many reflection' do + reflection = MockHabtmReflection.new(:has_and_belongs_to_many, 'tags', 'Tag') + + relationship = instance.send(:build_relationship_for_reflection, MockProjectModel, 'tags', reflection) + + expect(Gitlab::Reflections::Relationships::Handlers::HabtmHandler).to have_received(:new) + .with(MockProjectModel, 'tags', reflection) + expect(relationship).to eq({ type: 'has_and_belongs_to_many', source_table: 'projects', target_table: 'tags' }) + end + + it 'creates appropriate handler for Active Storage reflections' do + reflection = MockHasOneAttachedReflection.new(:has_one_attached, 'avatar') + + relationship = instance.send(:build_relationship_for_reflection, MockUserModel, 'avatar', reflection) + + expect(Gitlab::Reflections::Relationships::Handlers::ActiveStorageHandler).to have_received(:new) + .with(MockUserModel, 'avatar', reflection) + expect(relationship).to eq({ type: 'active_storage', source_table: 'users', +target_table: 'active_storage_attachments' }) + end + + it 'returns nil for unsupported reflection types' do + reflection = MockUnsupportedReflection.new(:custom_macro, 'unsupported', 'Other') + + relationship = instance.send(:build_relationship_for_reflection, MockIssueModel, 'unsupported', reflection) + + expect(relationship).to be_nil + end + + it 'returns nil when handler returns nil' do + reflection = MockBelongsToReflection.new(:belongs_to, 'project', 'Project') + allow(mock_belongs_to_handler).to receive(:build_relationship).and_return(nil) + + relationship = instance.send(:build_relationship_for_reflection, MockIssueModel, 'project', reflection) + + expect(relationship).to be_nil + end + end + end + + describe 'integration with handlers' do + it 'passes correct parameters to handlers' do + instance = described_class.new + instance.extract_relationships + + # Verify that handlers are called with correct parameters + expect(Gitlab::Reflections::Relationships::Handlers::BelongsToHandler).to have_received(:new) + .with(MockProjectModel, 'owner', instance_of(MockBelongsToReflection)) + + expect(Gitlab::Reflections::Relationships::Handlers::HasAssociationHandler).to have_received(:new) + .with(MockUserModel, 'projects', instance_of(MockHasManyReflection)) + + expect(Gitlab::Reflections::Relationships::Handlers::HabtmHandler).to have_received(:new) + .with(MockProjectModel, 'tags', instance_of(MockHabtmReflection)) + + expect(Gitlab::Reflections::Relationships::Handlers::ActiveStorageHandler).to have_received(:new) + .with(MockUserModel, 'avatar', instance_of(MockHasOneAttachedReflection)) + end + end +end diff --git a/spec/lib/gitlab/reflections/relationships/base_spec.rb b/spec/lib/gitlab/reflections/relationships/base_spec.rb new file mode 100644 index 00000000000000..0d02ca9a918ae7 --- /dev/null +++ b/spec/lib/gitlab/reflections/relationships/base_spec.rb @@ -0,0 +1,83 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Gitlab::Reflections::Relationships::Base do + let(:test_class) do + Class.new(described_class) do + private + + def get_source_data + [{ name: 'test_data' }] + end + + def filter_source_data(data) + data + end + + def map_to_relationships(data) + data.map do |item| + { + source_table: 'test_table', + target_table: 'target_table', + foreign_key: 'test_id', + relationship_type: 'one_to_many' + } + end + end + end + end + + describe '#extract_relationships' do + it 'returns relationship data from subclass implementation' do + extractor = test_class.new + relationships = extractor.extract_relationships + + expect(relationships).to have(1).item + expect(relationships.first[:source_table]).to eq('test_table') + expect(relationships.first[:target_table]).to eq('target_table') + expect(relationships.first[:foreign_key]).to eq('test_id') + end + end + + describe '#build_relationship' do + let(:extractor) { test_class.new } + + it 'returns a clean relationship hash for valid data' do + relationship = extractor.send(:build_relationship, + source_table: 'users', + target_table: 'posts', + foreign_key: 'user_id', + extra_field: 'value', + nil_field: nil + ) + + expect(relationship).to eq({ + source_table: 'users', + target_table: 'posts', + foreign_key: 'user_id', + extra_field: 'value' + }) + end + + it 'returns nil for missing required fields' do + relationship = extractor.send(:build_relationship, + source_table: 'users', + target_table: 'posts' + # missing foreign_key + ) + + expect(relationship).to be_nil + end + + it 'returns nil for empty required fields' do + relationship = extractor.send(:build_relationship, + source_table: 'users', + target_table: '', + foreign_key: 'user_id' + ) + + expect(relationship).to be_nil + end + end +end \ No newline at end of file diff --git a/spec/lib/gitlab/reflections/relationships/foreign_key_spec.rb b/spec/lib/gitlab/reflections/relationships/foreign_key_spec.rb new file mode 100644 index 00000000000000..179658d12d5d9b --- /dev/null +++ b/spec/lib/gitlab/reflections/relationships/foreign_key_spec.rb @@ -0,0 +1,392 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Gitlab::Reflections::Relationships::ForeignKey, feature_category: :database do + # Mock PostgresForeignKey objects + let(:mock_pg_fk_1) do + double('pg_fk_1', + constrained_table_name: 'issues', + referenced_table_name: 'projects', + constrained_columns: ['project_id'], + referenced_columns: ['id'], + name: 'fk_issues_project_id', + on_delete_action: 'cascade', + on_update_action: 'restrict' + ) + end + + let(:mock_pg_fk_2) do + double('pg_fk_2', + constrained_table_name: 'merge_requests', + referenced_table_name: 'users', + constrained_columns: ['author_id'], + referenced_columns: ['id'], + name: 'fk_merge_requests_author_id', + on_delete_action: 'set_null', + on_update_action: 'cascade' + ) + end + + let(:mock_pg_fk_3) do + double('pg_fk_3', + constrained_table_name: 'notes', + referenced_table_name: 'projects', + constrained_columns: ['project_id'], + referenced_columns: ['id'], + name: 'fk_notes_project_id', + on_delete_action: 'restrict', + on_update_action: 'restrict' + ) + end + + # Mock scope that behaves like ActiveRecord relation + let(:mock_scope) do + double('scope').tap do |scope| + allow(scope).to receive(:find_each).and_yield(mock_pg_fk_1).and_yield(mock_pg_fk_2).and_yield(mock_pg_fk_3) + end + end + + # Mock foreign key reflection + let(:mock_fk_reflection) do + double('fk_reflection', for_relationships: mock_scope) + end + + describe '#initialize' do + context 'with custom fk_reflection' do + it 'uses the provided fk_reflection' do + instance = described_class.new(mock_fk_reflection) + expect(instance.instance_variable_get(:@fk_reflection)).to eq(mock_fk_reflection) + end + end + + context 'without fk_reflection parameter' do + it 'uses default Gitlab::Reflections::Database::ForeignKeys' do + # Stub the default class + stub_const('Gitlab::Reflections::Database::ForeignKeys', mock_fk_reflection) + + instance = described_class.new + expect(instance.instance_variable_get(:@fk_reflection)).to eq(mock_fk_reflection) + end + end + end + + describe '#get_relationships' do + subject { described_class.new(mock_fk_reflection).get_relationships } + + it 'returns an array of relationships' do + expect(subject).to be_an(Array) + end + + it 'creates relationships for each foreign key' do + relationships = subject + expect(relationships.length).to eq(3) + end + + it 'creates relationships with correct structure' do + relationships = subject + + relationships.each do |relationship| + expect(relationship).to be_a(Hash) + expect(relationship).to have_key(:source_table) + expect(relationship).to have_key(:target_table) + expect(relationship).to have_key(:relationship_type) + expect(relationship).to have_key(:foreign_key) + expect(relationship).to have_key(:has_fk_constraint) + expect(relationship).to have_key(:fk_metadata) + expect(relationship).to have_key(:data_sources) + end + end + + it 'sets relationship_type to one_to_many' do + relationships = subject + + relationships.each do |relationship| + expect(relationship[:relationship_type]).to eq('one_to_many') + end + end + + it 'sets has_fk_constraint to true' do + relationships = subject + + relationships.each do |relationship| + expect(relationship[:has_fk_constraint]).to be true + end + end + + it 'includes data_sources as foreign_key' do + relationships = subject + + relationships.each do |relationship| + expect(relationship[:data_sources]).to eq(['foreign_key']) + end + end + + it 'creates correct relationship for issues -> projects' do + relationships = subject + issues_project_rel = relationships.find { |r| r[:target_table] == 'issues' && r[:source_table] == 'projects' } + + expect(issues_project_rel).not_to be_nil + expect(issues_project_rel[:foreign_key]).to eq('project_id') + expect(issues_project_rel[:foreign_key_table]).to eq('issues') + expect(issues_project_rel[:source_column]).to eq('id') + expect(issues_project_rel[:target_column]).to eq('project_id') + expect(issues_project_rel[:fk_metadata][:constraint_name]).to eq('fk_issues_project_id') + expect(issues_project_rel[:fk_metadata][:on_delete]).to eq('cascade') + expect(issues_project_rel[:fk_metadata][:on_update]).to eq('restrict') + end + + it 'creates correct relationship for merge_requests -> users' do + relationships = subject + mr_user_rel = relationships.find { |r| r[:target_table] == 'merge_requests' && r[:source_table] == 'users' } + + expect(mr_user_rel).not_to be_nil + expect(mr_user_rel[:foreign_key]).to eq('author_id') + expect(mr_user_rel[:foreign_key_table]).to eq('merge_requests') + expect(mr_user_rel[:source_column]).to eq('id') + expect(mr_user_rel[:target_column]).to eq('author_id') + expect(mr_user_rel[:fk_metadata][:constraint_name]).to eq('fk_merge_requests_author_id') + expect(mr_user_rel[:fk_metadata][:on_delete]).to eq('set_null') + expect(mr_user_rel[:fk_metadata][:on_update]).to eq('cascade') + end + + it 'creates correct relationship for notes -> projects' do + relationships = subject + notes_project_rel = relationships.find { |r| r[:target_table] == 'notes' && r[:source_table] == 'projects' } + + expect(notes_project_rel).not_to be_nil + expect(notes_project_rel[:foreign_key]).to eq('project_id') + expect(notes_project_rel[:foreign_key_table]).to eq('notes') + expect(notes_project_rel[:source_column]).to eq('id') + expect(notes_project_rel[:target_column]).to eq('project_id') + expect(notes_project_rel[:fk_metadata][:constraint_name]).to eq('fk_notes_project_id') + expect(notes_project_rel[:fk_metadata][:on_delete]).to eq('restrict') + expect(notes_project_rel[:fk_metadata][:on_update]).to eq('restrict') + end + end + + describe 'private methods' do + let(:instance) { described_class.new(mock_fk_reflection) } + + describe '#get_source_data' do + it 'converts PostgresForeignKey objects to internal format' do + source_data = instance.send(:get_source_data) + + expect(source_data).to be_an(Array) + expect(source_data.length).to eq(3) + + source_data.each do |item| + expect(item).to have_key(:table_name) + expect(item).to have_key(:foreign_key) + expect(item[:foreign_key]).to respond_to(:column) + expect(item[:foreign_key]).to respond_to(:to_table) + expect(item[:foreign_key]).to respond_to(:primary_key) + expect(item[:foreign_key]).to respond_to(:name) + expect(item[:foreign_key]).to respond_to(:on_delete) + expect(item[:foreign_key]).to respond_to(:on_update) + end + end + + it 'calls find_each on the fk_reflection scope' do + expect(mock_scope).to receive(:find_each) + instance.send(:get_source_data) + end + end + + describe '#filter_source_data' do + it 'returns data unchanged' do + test_data = [{ table_name: 'test', foreign_key: double }] + filtered_data = instance.send(:filter_source_data, test_data) + + expect(filtered_data).to eq(test_data) + end + end + + describe '#map_to_relationships' do + let(:test_items) do + [ + { + table_name: 'issues', + foreign_key: OpenStruct.new( + column: 'project_id', + to_table: 'projects', + primary_key: 'id', + name: 'fk_test', + on_delete: 'cascade', + on_update: 'restrict' + ) + } + ] + end + + it 'maps items to relationships' do + relationships = instance.send(:map_to_relationships, test_items) + + expect(relationships).to be_an(Array) + expect(relationships.length).to eq(1) + expect(relationships.first).to be_a(Hash) + end + + it 'filters out nil relationships' do + # Mock build_foreign_key_relationship to return nil for one item + allow(instance).to receive(:build_foreign_key_relationship).and_return(nil, { test: 'relationship' }) + + test_items_with_nil = [ + { table_name: 'table1', foreign_key: double }, + { table_name: 'table2', foreign_key: double } + ] + + relationships = instance.send(:map_to_relationships, test_items_with_nil) + + expect(relationships).to eq([{ test: 'relationship' }]) + end + end + + describe '#build_foreign_key_relationship' do + let(:foreign_key) do + OpenStruct.new( + column: 'project_id', + to_table: 'projects', + primary_key: 'id', + name: 'fk_issues_project_id', + on_delete: 'cascade', + on_update: 'restrict' + ) + end + + it 'builds a complete relationship hash' do + relationship = instance.send(:build_foreign_key_relationship, 'issues', foreign_key) + + expect(relationship).to be_a(Hash) + expect(relationship[:source_table]).to eq('projects') + expect(relationship[:target_table]).to eq('issues') + expect(relationship[:relationship_type]).to eq('one_to_many') + expect(relationship[:foreign_key]).to eq('project_id') + expect(relationship[:foreign_key_table]).to eq('issues') + expect(relationship[:source_column]).to eq('id') + expect(relationship[:target_column]).to eq('project_id') + expect(relationship[:has_fk_constraint]).to be true + expect(relationship[:data_sources]).to eq(['foreign_key']) + end + + it 'includes fk_metadata with constraint details' do + relationship = instance.send(:build_foreign_key_relationship, 'issues', foreign_key) + + expect(relationship[:fk_metadata]).to be_a(Hash) + expect(relationship[:fk_metadata][:constraint_name]).to eq('fk_issues_project_id') + expect(relationship[:fk_metadata][:on_delete]).to eq('cascade') + expect(relationship[:fk_metadata][:on_update]).to eq('restrict') + end + + it 'handles foreign key without explicit primary_key' do + fk_without_pk = OpenStruct.new( + column: 'user_id', + to_table: 'users', + primary_key: nil, + name: 'fk_test', + on_delete: 'restrict', + on_update: 'restrict' + ) + + relationship = instance.send(:build_foreign_key_relationship, 'issues', fk_without_pk) + + expect(relationship[:source_column]).to eq('id') + end + end + + describe '#build_fk_definition_from_pg_fk' do + it 'converts PostgresForeignKey to OpenStruct with correct attributes' do + fk_definition = instance.send(:build_fk_definition_from_pg_fk, mock_pg_fk_1) + + expect(fk_definition).to be_an(OpenStruct) + expect(fk_definition.column).to eq('project_id') + expect(fk_definition.to_table).to eq('projects') + expect(fk_definition.primary_key).to eq('id') + expect(fk_definition.name).to eq('fk_issues_project_id') + expect(fk_definition.on_delete).to eq('cascade') + expect(fk_definition.on_update).to eq('restrict') + end + + it 'handles single column foreign keys' do + # Test assumes single column FK (takes first element) + fk_definition = instance.send(:build_fk_definition_from_pg_fk, mock_pg_fk_2) + + expect(fk_definition.column).to eq('author_id') + expect(fk_definition.to_table).to eq('users') + expect(fk_definition.primary_key).to eq('id') + end + + it 'preserves constraint metadata' do + fk_definition = instance.send(:build_fk_definition_from_pg_fk, mock_pg_fk_2) + + expect(fk_definition.name).to eq('fk_merge_requests_author_id') + expect(fk_definition.on_delete).to eq('set_null') + expect(fk_definition.on_update).to eq('cascade') + end + end + end + + describe 'integration with foreign key reflection' do + it 'calls for_relationships on the fk_reflection' do + expect(mock_fk_reflection).to receive(:for_relationships).and_return(mock_scope) + + described_class.new(mock_fk_reflection).get_relationships + end + + it 'processes all foreign keys from the scope' do + expect(mock_scope).to receive(:find_each).and_yield(mock_pg_fk_1).and_yield(mock_pg_fk_2).and_yield(mock_pg_fk_3) + + relationships = described_class.new(mock_fk_reflection).get_relationships + expect(relationships.length).to eq(3) + end + end + + describe 'error handling' do + context 'when foreign key has missing data' do + let(:incomplete_pg_fk) do + double('incomplete_pg_fk', + constrained_table_name: 'issues', + referenced_table_name: nil, + constrained_columns: ['project_id'], + referenced_columns: ['id'], + name: 'fk_incomplete', + on_delete_action: 'cascade', + on_update_action: 'restrict' + ) + end + + let(:incomplete_scope) do + double('incomplete_scope').tap do |scope| + allow(scope).to receive(:find_each).and_yield(incomplete_pg_fk) + end + end + + let(:incomplete_fk_reflection) do + double('incomplete_fk_reflection', for_relationships: incomplete_scope) + end + + it 'handles foreign keys with missing referenced table' do + expect do + described_class.new(incomplete_fk_reflection).get_relationships + end.not_to raise_error + end + end + + context 'when scope is empty' do + let(:empty_scope) do + double('empty_scope').tap do |scope| + allow(scope).to receive(:find_each) + end + end + + let(:empty_fk_reflection) do + double('empty_fk_reflection', for_relationships: empty_scope) + end + + it 'returns empty array when no foreign keys exist' do + relationships = described_class.new(empty_fk_reflection).get_relationships + expect(relationships).to eq([]) + end + end + end +end \ No newline at end of file diff --git a/spec/lib/gitlab/reflections/relationships/handlers/active_storage_handler_spec.rb b/spec/lib/gitlab/reflections/relationships/handlers/active_storage_handler_spec.rb new file mode 100644 index 00000000000000..250409e69ace8b --- /dev/null +++ b/spec/lib/gitlab/reflections/relationships/handlers/active_storage_handler_spec.rb @@ -0,0 +1,447 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Gitlab::Reflections::Relationships::Handlers::ActiveStorageHandler do + # Mock ActiveRecord model class with ActiveStorage attachments + let(:mock_model_class) do + Class.new do + def self.table_name + 'users' + end + + def self.name + 'User' + end + end + end + + # Mock ActiveStorage has_one_attached reflection + let(:mock_has_one_attached_reflection) do + double('has_one_attached_reflection', + macro: :has_one_attached, + name: :avatar, + klass: mock_attachment_class, + table_name: 'active_storage_attachments', + plural_name: 'active_storage_attachments', + polymorphic?: false, + foreign_type: nil, + options: {} + ) + end + + # Mock ActiveStorage has_many_attached reflection + let(:mock_has_many_attached_reflection) do + double('has_many_attached_reflection', + macro: :has_many_attached, + name: :documents, + klass: mock_attachment_class, + table_name: 'active_storage_attachments', + plural_name: 'active_storage_attachments', + polymorphic?: false, + foreign_type: nil, + options: {} + ) + end + + # Mock ActiveStorage::Attachment class + let(:mock_attachment_class) do + Class.new do + def self.table_name + 'active_storage_attachments' + end + + def self.name + 'ActiveStorage::Attachment' + end + end + end + + # Mock reflection that raises NameError for target table determination + let(:mock_problematic_reflection) do + double('problematic_reflection', + macro: :has_one_attached, + name: :unknown_attachment, + table_name: 'active_storage_attachments', + plural_name: 'active_storage_attachments', + polymorphic?: false, + foreign_type: nil, + options: {} + ).tap do |reflection| + allow(reflection).to receive(:klass).and_raise(NameError, 'uninitialized constant ActiveStorage::Attachment') + end + end + + let(:association_name) { :avatar } + let(:handler) { described_class.new(mock_model_class, association_name, mock_has_one_attached_reflection) } + + describe '#initialize' do + it 'inherits from BaseHandler' do + expect(described_class.superclass).to eq(Gitlab::Reflections::Relationships::Handlers::BaseHandler) + end + + it 'initializes with model, association name, and reflection' do + expect(handler.instance_variable_get(:@model)).to eq(mock_model_class) + expect(handler.instance_variable_get(:@association_name)).to eq(association_name) + expect(handler.instance_variable_get(:@reflection)).to eq(mock_has_one_attached_reflection) + end + end + + describe '#build_relationship' do + context 'with has_one_attached reflection' do + it 'builds a complete ActiveStorage relationship hash' do + result = handler.build_relationship + + expect(result).to be_a(Hash) + expect(result[:source_table]).to eq('users') + expect(result[:target_table]).to eq('active_storage_attachments') + expect(result[:relationship_type]).to eq('one_to_one') + expect(result[:foreign_key]).to eq('record_id') + expect(result[:has_ar_forward]).to be true + expect(result[:polymorphic]).to be false + expect(result[:data_sources]).to eq(['active_record']) + end + + it 'includes forward association details' do + result = handler.build_relationship + + expect(result[:forward_association]).to be_a(Hash) + expect(result[:forward_association][:name]).to eq('avatar') + expect(result[:forward_association][:type]).to eq('has_one_attached') + expect(result[:forward_association][:model]).to eq('User') + end + + it 'sets the correct relationship type for has_one_attached' do + result = handler.build_relationship + expect(result[:relationship_type]).to eq('one_to_one') + end + end + + context 'with has_many_attached reflection' do + let(:many_handler) { described_class.new(mock_model_class, :documents, mock_has_many_attached_reflection) } + + it 'builds a complete ActiveStorage relationship hash' do + result = many_handler.build_relationship + + expect(result).to be_a(Hash) + expect(result[:source_table]).to eq('users') + expect(result[:target_table]).to eq('active_storage_attachments') + expect(result[:relationship_type]).to eq('one_to_many') + expect(result[:foreign_key]).to eq('record_id') + expect(result[:has_ar_forward]).to be true + expect(result[:polymorphic]).to be false + expect(result[:data_sources]).to eq(['active_record']) + end + + it 'includes forward association details' do + result = many_handler.build_relationship + + expect(result[:forward_association]).to be_a(Hash) + expect(result[:forward_association][:name]).to eq('documents') + expect(result[:forward_association][:type]).to eq('has_many_attached') + expect(result[:forward_association][:model]).to eq('User') + end + + it 'sets the correct relationship type for has_many_attached' do + result = many_handler.build_relationship + expect(result[:relationship_type]).to eq('one_to_many') + end + end + + context 'when target table determination fails' do + let(:problematic_handler) { described_class.new(mock_model_class, :unknown_attachment, mock_problematic_reflection) } + + it 'falls back to reflection table_name or plural_name' do + result = problematic_handler.build_relationship + + expect(result[:target_table]).to eq('active_storage_attachments') + expect(result[:relationship_type]).to eq('one_to_one') + end + end + + context 'when required fields are missing' do + let(:incomplete_reflection) do + double('incomplete_reflection', + macro: :has_one_attached, + name: :avatar, + klass: mock_attachment_class, + table_name: 'active_storage_attachments', + plural_name: 'active_storage_attachments', + polymorphic?: false, + foreign_type: nil, + options: {} + ) + end + + let(:incomplete_handler) { described_class.new(mock_model_class, association_name, incomplete_reflection) } + + before do + # Mock a scenario where source table is nil + allow(mock_model_class).to receive(:table_name).and_return(nil) + allow(Gitlab::AppLogger).to receive(:debug) + end + + it 'returns nil when source_table is missing' do + result = incomplete_handler.build_relationship + + expect(result).to be_nil + expect(Gitlab::AppLogger).to have_received(:debug).with(/Failed to build relationship: missing required fields/) + end + end + + context 'when an exception occurs during relationship building' do + before do + allow(mock_model_class).to receive(:table_name).and_raise(StandardError, 'Database connection error') + allow(Gitlab::AppLogger).to receive(:debug) + end + + it 'logs the error and returns nil' do + result = handler.build_relationship + + expect(result).to be_nil + expect(Gitlab::AppLogger).to have_received(:debug).with('Failed to build relationship: Database connection error') + end + end + end + + describe 'private methods' do + describe '#determine_target_table' do + it 'returns active_storage_attachments table' do + # Access private method for testing + target_table = handler.send(:determine_target_table) + expect(target_table).to eq('active_storage_attachments') + end + + context 'when klass raises NameError' do + let(:problematic_handler) { described_class.new(mock_model_class, :unknown_attachment, mock_problematic_reflection) } + + it 'falls back to reflection table_name' do + target_table = problematic_handler.send(:determine_target_table) + expect(target_table).to eq('active_storage_attachments') + end + end + end + + describe '#base_attributes' do + context 'with has_one_attached' do + it 'sets relationship_type to one_to_one' do + base_attrs = handler.send(:base_attributes) + + expect(base_attrs[:relationship_type]).to eq('one_to_one') + expect(base_attrs[:source_table]).to eq('users') + expect(base_attrs[:target_table]).to eq('active_storage_attachments') + expect(base_attrs[:has_ar_forward]).to be true + expect(base_attrs[:polymorphic]).to be false + expect(base_attrs[:data_sources]).to eq(['active_record']) + end + end + + context 'with has_many_attached' do + let(:many_handler) { described_class.new(mock_model_class, :documents, mock_has_many_attached_reflection) } + + it 'sets relationship_type to one_to_many' do + base_attrs = many_handler.send(:base_attributes) + + expect(base_attrs[:relationship_type]).to eq('one_to_many') + expect(base_attrs[:source_table]).to eq('users') + expect(base_attrs[:target_table]).to eq('active_storage_attachments') + expect(base_attrs[:has_ar_forward]).to be true + expect(base_attrs[:polymorphic]).to be false + expect(base_attrs[:data_sources]).to eq(['active_record']) + end + end + + it 'includes forward association information' do + base_attrs = handler.send(:base_attributes) + + expect(base_attrs[:forward_association]).to be_a(Hash) + expect(base_attrs[:forward_association][:name]).to eq('avatar') + expect(base_attrs[:forward_association][:type]).to eq('has_one_attached') + expect(base_attrs[:forward_association][:model]).to eq('User') + end + end + end + + # Integration tests with different ActiveStorage scenarios + describe 'integration scenarios' do + context 'with project logo attachment' do + let(:mock_project_model) do + Class.new do + def self.table_name + 'projects' + end + + def self.name + 'Project' + end + end + end + + let(:logo_reflection) do + double('logo_reflection', + macro: :has_one_attached, + name: :logo, + klass: mock_attachment_class, + table_name: 'active_storage_attachments', + plural_name: 'active_storage_attachments', + polymorphic?: false, + foreign_type: nil, + options: {} + ) + end + + let(:logo_handler) { described_class.new(mock_project_model, :logo, logo_reflection) } + + it 'correctly handles project logo attachment' do + result = logo_handler.build_relationship + + expect(result[:source_table]).to eq('projects') + expect(result[:target_table]).to eq('active_storage_attachments') + expect(result[:foreign_key]).to eq('record_id') + expect(result[:relationship_type]).to eq('one_to_one') + expect(result[:forward_association][:name]).to eq('logo') + end + end + + context 'with issue attachments' do + let(:mock_issue_model) do + Class.new do + def self.table_name + 'issues' + end + + def self.name + 'Issue' + end + end + end + + let(:attachments_reflection) do + double('attachments_reflection', + macro: :has_many_attached, + name: :attachments, + klass: mock_attachment_class, + table_name: 'active_storage_attachments', + plural_name: 'active_storage_attachments', + polymorphic?: false, + foreign_type: nil, + options: {} + ) + end + + let(:attachments_handler) { described_class.new(mock_issue_model, :attachments, attachments_reflection) } + + it 'correctly handles issue attachments' do + result = attachments_handler.build_relationship + + expect(result[:source_table]).to eq('issues') + expect(result[:target_table]).to eq('active_storage_attachments') + expect(result[:foreign_key]).to eq('record_id') + expect(result[:relationship_type]).to eq('one_to_many') + expect(result[:forward_association][:name]).to eq('attachments') + end + end + + context 'with user profile images' do + let(:profile_images_reflection) do + double('profile_images_reflection', + macro: :has_many_attached, + name: :profile_images, + klass: mock_attachment_class, + table_name: 'active_storage_attachments', + plural_name: 'active_storage_attachments', + polymorphic?: false, + foreign_type: nil, + options: {} + ) + end + + let(:profile_images_handler) { described_class.new(mock_model_class, :profile_images, profile_images_reflection) } + + it 'correctly handles user profile images' do + result = profile_images_handler.build_relationship + + expect(result[:source_table]).to eq('users') + expect(result[:target_table]).to eq('active_storage_attachments') + expect(result[:foreign_key]).to eq('record_id') + expect(result[:relationship_type]).to eq('one_to_many') + expect(result[:forward_association][:name]).to eq('profile_images') + end + end + end + + # Edge cases and error handling + describe 'edge cases' do + context 'with unknown macro type' do + let(:unknown_macro_reflection) do + double('unknown_macro_reflection', + macro: :unknown_attachment_type, + name: :unknown, + klass: mock_attachment_class, + table_name: 'active_storage_attachments', + plural_name: 'active_storage_attachments', + polymorphic?: false, + foreign_type: nil, + options: {} + ) + end + + let(:unknown_handler) { described_class.new(mock_model_class, :unknown, unknown_macro_reflection) } + + it 'defaults to unknown relationship type from base class' do + result = unknown_handler.build_relationship + + # The base class determine_relationship_type method should handle unknown macros + expect(result[:relationship_type]).to eq('unknown') + end + end + + context 'with nil attachment name' do + let(:nil_name_reflection) do + double('nil_name_reflection', + macro: :has_one_attached, + name: nil, + klass: mock_attachment_class, + table_name: 'active_storage_attachments', + plural_name: 'active_storage_attachments', + polymorphic?: false, + foreign_type: nil, + options: {} + ) + end + + let(:nil_name_handler) { described_class.new(mock_model_class, nil, nil_name_reflection) } + + it 'handles nil attachment name gracefully' do + result = nil_name_handler.build_relationship + + expect(result).to be_a(Hash) + expect(result[:forward_association][:name]).to eq('') + end + end + + context 'with empty table name' do + let(:empty_table_model) do + Class.new do + def self.table_name + '' + end + + def self.name + 'EmptyTableModel' + end + end + end + + let(:empty_table_handler) { described_class.new(empty_table_model, :avatar, mock_has_one_attached_reflection) } + + it 'handles empty table name gracefully' do + allow(Gitlab::AppLogger).to receive(:debug) + result = empty_table_handler.build_relationship + + expect(result).to be_nil + expect(Gitlab::AppLogger).to have_received(:debug).with(/Failed to build relationship: missing required fields/) + end + end + end +end \ No newline at end of file diff --git a/spec/lib/gitlab/reflections/relationships/handlers/base_handler_spec.rb b/spec/lib/gitlab/reflections/relationships/handlers/base_handler_spec.rb new file mode 100644 index 00000000000000..32ae7979950961 --- /dev/null +++ b/spec/lib/gitlab/reflections/relationships/handlers/base_handler_spec.rb @@ -0,0 +1,23 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Gitlab::Reflections::Relationships::Handlers::BaseHandler, feature_category: :database do + let(:model) { Note } + let(:association_name) { 'noteable' } + let(:reflection) { model.reflections[association_name.to_s] } + let(:handler) { described_class.new(model, association_name, reflection) } + + describe '#build_relationship' do + it 'raises NotImplementedError' do + expect { handler.build_relationship }.to raise_error(NotImplementedError) + end + end + + describe '#polymorphic?' do + it 'returns false for base handler' do + expect(handler.polymorphic?).to be false + end + end +end +end diff --git a/spec/lib/gitlab/reflections/relationships/handlers/belongs_to_handler_spec.rb b/spec/lib/gitlab/reflections/relationships/handlers/belongs_to_handler_spec.rb new file mode 100644 index 00000000000000..d78f3d44f8555f --- /dev/null +++ b/spec/lib/gitlab/reflections/relationships/handlers/belongs_to_handler_spec.rb @@ -0,0 +1,39 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Gitlab::Reflections::Relationships::Handlers::BelongsToHandler, feature_category: :database do + let(:model) { Project } + let(:association_name) { 'namespace' } + let(:reflection) { model.reflections[association_name.to_s] } + let(:handler) { described_class.new(model, association_name, reflection) } + + describe '#build_relationship' do + context 'with regular belongs_to association' do + it 'builds regular belongs_to relationship' do + relationship = handler.build_relationship + + expect(relationship).to include( + source_table: 'namespaces', # The referenced table (parent) + target_table: 'projects', # The referencing table (child) + source_column: 'id', # The referenced column + relationship_type: 'many_to_one', + polymorphic: false + ) + expect(relationship[:reverse_association]).to include( + name: 'namespace', + type: 'belongs_to', + model: 'Project' + ) + expect(relationship[:has_ar_reverse]).to be true + end + + it 'returns same relationship for build_relationship_with_target' do + original_relationship = handler.build_relationship + target_relationship = handler.build_relationship_with_target('some_table') + + expect(target_relationship).to eq(original_relationship) + end + end + end +end diff --git a/spec/lib/gitlab/reflections/relationships/handlers/habtm_handler_spec.rb b/spec/lib/gitlab/reflections/relationships/handlers/habtm_handler_spec.rb new file mode 100644 index 00000000000000..8a00b445fdc412 --- /dev/null +++ b/spec/lib/gitlab/reflections/relationships/handlers/habtm_handler_spec.rb @@ -0,0 +1,396 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Gitlab::Reflections::Relationships::Handlers::HabtmHandler do + # Mock ActiveRecord model class + let(:mock_model_class) do + Class.new do + def self.table_name + 'users' + end + + def self.name + 'User' + end + end + end + + # Mock HABTM reflection + let(:mock_habtm_reflection) do + double('habtm_reflection', + macro: :has_and_belongs_to_many, + foreign_key: 'user_id', + association_foreign_key: 'project_id', + join_table: 'projects_users', + klass: mock_target_model_class, + table_name: 'projects', + plural_name: 'projects', + polymorphic?: false, + foreign_type: nil, + options: {} + ) + end + + # Mock target model class + let(:mock_target_model_class) do + Class.new do + def self.table_name + 'projects' + end + + def self.name + 'Project' + end + end + end + + # Mock HABTM reflection with custom join table + let(:mock_custom_habtm_reflection) do + double('custom_habtm_reflection', + macro: :has_and_belongs_to_many, + foreign_key: 'user_id', + association_foreign_key: 'role_id', + join_table: 'user_roles', + klass: mock_role_model_class, + table_name: 'roles', + plural_name: 'roles', + polymorphic?: false, + foreign_type: nil, + options: {} + ) + end + + let(:mock_role_model_class) do + Class.new do + def self.table_name + 'roles' + end + + def self.name + 'Role' + end + end + end + + # Mock reflection that raises NameError for target table determination + let(:mock_problematic_reflection) do + double('problematic_reflection', + macro: :has_and_belongs_to_many, + foreign_key: 'user_id', + association_foreign_key: 'unknown_id', + join_table: 'users_unknowns', + table_name: 'unknowns', + plural_name: 'unknowns', + polymorphic?: false, + foreign_type: nil, + options: {} + ).tap do |reflection| + allow(reflection).to receive(:klass).and_raise(NameError, 'uninitialized constant Unknown') + end + end + + let(:association_name) { :projects } + let(:handler) { described_class.new(mock_model_class, association_name, mock_habtm_reflection) } + + describe '#initialize' do + it 'inherits from BaseHandler' do + expect(described_class.superclass).to eq(Gitlab::Reflections::Relationships::Handlers::BaseHandler) + end + + it 'initializes with model, association name, and reflection' do + expect(handler.instance_variable_get(:@model)).to eq(mock_model_class) + expect(handler.instance_variable_get(:@association_name)).to eq(association_name) + expect(handler.instance_variable_get(:@reflection)).to eq(mock_habtm_reflection) + end + end + + describe '#build_relationship' do + context 'with valid HABTM reflection' do + it 'builds a complete HABTM relationship hash' do + result = handler.build_relationship + + expect(result).to be_a(Hash) + expect(result[:source_table]).to eq('users') + expect(result[:target_table]).to eq('projects') + expect(result[:relationship_type]).to eq('many_to_many') + expect(result[:foreign_key]).to eq('user_id') + expect(result[:foreign_key_table]).to eq('projects_users') + expect(result[:through_table]).to eq('projects_users') + expect(result[:has_ar_forward]).to be true + expect(result[:polymorphic]).to be false + expect(result[:data_sources]).to eq(['active_record']) + end + + it 'includes forward association details' do + result = handler.build_relationship + + expect(result[:forward_association]).to be_a(Hash) + expect(result[:forward_association][:name]).to eq('projects') + expect(result[:forward_association][:type]).to eq('has_and_belongs_to_many') + expect(result[:forward_association][:model]).to eq('User') + end + + it 'sets the correct relationship type for HABTM' do + result = handler.build_relationship + expect(result[:relationship_type]).to eq('many_to_many') + end + end + + context 'with custom join table' do + let(:custom_handler) { described_class.new(mock_model_class, :roles, mock_custom_habtm_reflection) } + + it 'uses the custom join table' do + result = custom_handler.build_relationship + + expect(result[:foreign_key_table]).to eq('user_roles') + expect(result[:through_table]).to eq('user_roles') + expect(result[:target_table]).to eq('roles') + end + end + + context 'when target table determination fails' do + let(:problematic_handler) { described_class.new(mock_model_class, :unknowns, mock_problematic_reflection) } + + it 'falls back to reflection table_name or plural_name' do + result = problematic_handler.build_relationship + + expect(result[:target_table]).to eq('unknowns') + expect(result[:relationship_type]).to eq('many_to_many') + end + end + + context 'when required fields are missing' do + let(:incomplete_reflection) do + double('incomplete_reflection', + macro: :has_and_belongs_to_many, + foreign_key: nil, + association_foreign_key: 'project_id', + join_table: 'projects_users', + klass: mock_target_model_class, + table_name: 'projects', + plural_name: 'projects', + polymorphic?: false, + foreign_type: nil, + options: {} + ) + end + + let(:incomplete_handler) { described_class.new(mock_model_class, association_name, incomplete_reflection) } + + it 'returns nil when foreign_key is missing' do + allow(Gitlab::AppLogger).to receive(:debug) + result = incomplete_handler.build_relationship + + expect(result).to be_nil + expect(Gitlab::AppLogger).to have_received(:debug).with(/Failed to build relationship: missing required fields/) + end + end + + context 'when an exception occurs during relationship building' do + before do + allow(mock_habtm_reflection).to receive(:foreign_key).and_raise(StandardError, 'Database connection error') + allow(Gitlab::AppLogger).to receive(:debug) + end + + it 'logs the error and returns nil' do + result = handler.build_relationship + + expect(result).to be_nil + expect(Gitlab::AppLogger).to have_received(:debug).with('Failed to build relationship: Database connection error') + end + end + end + + describe 'private methods' do + describe '#base_attributes' do + it 'overrides relationship_type to many_to_many' do + # Access private method for testing + base_attrs = handler.send(:base_attributes) + + expect(base_attrs[:relationship_type]).to eq('many_to_many') + expect(base_attrs[:source_table]).to eq('users') + expect(base_attrs[:target_table]).to eq('projects') + expect(base_attrs[:has_ar_forward]).to be true + expect(base_attrs[:polymorphic]).to be false + expect(base_attrs[:data_sources]).to eq(['active_record']) + end + + it 'includes forward association information' do + base_attrs = handler.send(:base_attributes) + + expect(base_attrs[:forward_association]).to be_a(Hash) + expect(base_attrs[:forward_association][:name]).to eq('projects') + expect(base_attrs[:forward_association][:type]).to eq('has_and_belongs_to_many') + expect(base_attrs[:forward_association][:model]).to eq('User') + end + end + end + + # Integration tests with different HABTM scenarios + describe 'integration scenarios' do + context 'with tags and taggables HABTM relationship' do + let(:mock_tag_model) do + Class.new do + def self.table_name + 'tags' + end + + def self.name + 'Tag' + end + end + end + + let(:mock_taggable_model) do + Class.new do + def self.table_name + 'issues' + end + + def self.name + 'Issue' + end + end + end + + let(:tags_reflection) do + double('tags_reflection', + macro: :has_and_belongs_to_many, + foreign_key: 'issue_id', + association_foreign_key: 'tag_id', + join_table: 'issue_tags', + klass: mock_tag_model, + table_name: 'tags', + plural_name: 'tags', + polymorphic?: false, + foreign_type: nil, + options: {} + ) + end + + let(:tags_handler) { described_class.new(mock_taggable_model, :tags, tags_reflection) } + + it 'correctly handles tags relationship' do + result = tags_handler.build_relationship + + expect(result[:source_table]).to eq('issues') + expect(result[:target_table]).to eq('tags') + expect(result[:foreign_key]).to eq('issue_id') + expect(result[:through_table]).to eq('issue_tags') + expect(result[:relationship_type]).to eq('many_to_many') + end + end + + context 'with permissions and roles HABTM relationship' do + let(:mock_permission_model) do + Class.new do + def self.table_name + 'permissions' + end + + def self.name + 'Permission' + end + end + end + + let(:mock_role_model) do + Class.new do + def self.table_name + 'roles' + end + + def self.name + 'Role' + end + end + end + + let(:permissions_reflection) do + double('permissions_reflection', + macro: :has_and_belongs_to_many, + foreign_key: 'role_id', + association_foreign_key: 'permission_id', + join_table: 'permissions_roles', + klass: mock_permission_model, + table_name: 'permissions', + plural_name: 'permissions', + polymorphic?: false, + foreign_type: nil, + options: {} + ) + end + + let(:permissions_handler) { described_class.new(mock_role_model, :permissions, permissions_reflection) } + + it 'correctly handles permissions relationship' do + result = permissions_handler.build_relationship + + expect(result[:source_table]).to eq('roles') + expect(result[:target_table]).to eq('permissions') + expect(result[:foreign_key]).to eq('role_id') + expect(result[:through_table]).to eq('permissions_roles') + expect(result[:relationship_type]).to eq('many_to_many') + end + end + end + + # Edge cases and error handling + describe 'edge cases' do + context 'with empty join table name' do + let(:empty_join_reflection) do + double('empty_join_reflection', + macro: :has_and_belongs_to_many, + foreign_key: 'user_id', + association_foreign_key: 'project_id', + join_table: '', + klass: mock_target_model_class, + table_name: 'projects', + plural_name: 'projects', + polymorphic?: false, + foreign_type: nil, + options: {} + ) + end + + let(:empty_join_handler) { described_class.new(mock_model_class, association_name, empty_join_reflection) } + + it 'handles empty join table gracefully' do + allow(Gitlab::AppLogger).to receive(:debug) + result = empty_join_handler.build_relationship + + expect(result).to be_nil + expect(Gitlab::AppLogger).to have_received(:debug).with(/Failed to build relationship: missing required fields/) + end + end + + context 'with nil association foreign key' do + let(:nil_assoc_fk_reflection) do + double('nil_assoc_fk_reflection', + macro: :has_and_belongs_to_many, + foreign_key: 'user_id', + association_foreign_key: nil, + join_table: 'projects_users', + klass: mock_target_model_class, + table_name: 'projects', + plural_name: 'projects', + polymorphic?: false, + foreign_type: nil, + options: {} + ) + end + + let(:nil_assoc_fk_handler) { described_class.new(mock_model_class, association_name, nil_assoc_fk_reflection) } + + it 'still builds relationship without association foreign key in main hash' do + result = nil_assoc_fk_handler.build_relationship + + expect(result).to be_a(Hash) + expect(result[:source_table]).to eq('users') + expect(result[:target_table]).to eq('projects') + expect(result[:relationship_type]).to eq('many_to_many') + end + end + end +end \ No newline at end of file diff --git a/spec/lib/gitlab/reflections/relationships/handlers/has_association_handler_spec.rb b/spec/lib/gitlab/reflections/relationships/handlers/has_association_handler_spec.rb new file mode 100644 index 00000000000000..1ef95f36bbd2f5 --- /dev/null +++ b/spec/lib/gitlab/reflections/relationships/handlers/has_association_handler_spec.rb @@ -0,0 +1,62 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Gitlab::Reflections::Relationships::Handlers::HasAssociationHandler, feature_category: :database do + let(:model) { Project } + let(:association_name) { 'issues' } + let(:reflection) { model.reflections[association_name.to_s] } + let(:handler) { described_class.new(model, association_name, reflection) } + + describe '#build_relationship' do + context 'with regular has_many association' do + it 'builds regular has_many relationship' do + relationship = handler.build_relationship + + expect(relationship).to include( + source_table: 'projects', + target_table: 'issues', + relationship_type: 'one_to_many', + polymorphic: false + ) + expect(relationship[:forward_association]).to include( + name: 'issues', + type: 'has_many', + model: 'Project' + ) + expect(relationship[:has_ar_forward]).to be true + end + + it 'returns same relationship for build_relationship_with_target' do + original_relationship = handler.build_relationship + target_relationship = handler.build_relationship_with_target('some_table') + + expect(target_relationship).to eq(original_relationship) + end + end + + context 'with has_many through association' do + let(:model) { User } + let(:association_name) { 'assigned_issues' } + let(:reflection) { model.reflections[association_name.to_s] } + + it 'builds has_many through relationship' do + relationship = handler.build_relationship + + expect(relationship).to include( + source_table: 'users', + target_table: 'issues', + relationship_type: 'one_to_many', + polymorphic: false + ) + expect(relationship[:through_table]).to eq('issue_assignees') + expect(relationship[:forward_association]).to include( + name: 'assigned_issues', + type: 'has_many', + model: 'User' + ) + expect(relationship[:has_ar_forward]).to be true + end + end + end +end diff --git a/spec/lib/gitlab/reflections/relationships/handlers/polymorphic_belongs_to_handler_spec.rb b/spec/lib/gitlab/reflections/relationships/handlers/polymorphic_belongs_to_handler_spec.rb new file mode 100644 index 00000000000000..33aac750a09078 --- /dev/null +++ b/spec/lib/gitlab/reflections/relationships/handlers/polymorphic_belongs_to_handler_spec.rb @@ -0,0 +1,79 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Gitlab::Reflections::Relationships::Handlers::PolymorphicBelongsToHandler, feature_category: :database do + let(:model) { Note } + let(:association_name) { 'noteable' } + let(:reflection) { model.reflections[association_name.to_s] } + let(:handler) { described_class.new(model, association_name, reflection) } + + describe '#polymorphic?' do + it 'returns true' do + expect(handler.polymorphic?).to be true + end + end + + describe '#build_relationship' do + it 'builds a polymorphic belongs_to relationship' do + relationship = handler.build_relationship + + expect(relationship).to include( + source_table: 'notes', + target_table: nil, + relationship_type: 'many_to_one', + polymorphic: true, + polymorphic_type_column: 'noteable_type', + polymorphic_name: 'noteable' + ) + expect(relationship[:reverse_association]).to include( + name: 'noteable', + type: 'belongs_to', + model: 'Note' + ) + expect(relationship[:has_ar_reverse]).to be true + end + end + + describe '#build_relationship_with_target' do + it 'builds a specific relationship for a target table' do + relationship = handler.build_relationship_with_target('issues') + + expect(relationship).to include( + source_table: 'issues', + target_table: 'notes', + relationship_type: 'one_to_many', + polymorphic: true, + polymorphic_type_column: 'noteable_type', + polymorphic_name: 'noteable' + ) + expect(relationship[:forward_association]).to include( + name: 'noteable', + type: 'has_many', + model: 'Issue' + ) + expect(relationship[:has_ar_forward]).to be true + end + end + + describe '#discover_targets' do + it 'finds models with matching has_many/has_one as: associations' do + # Mock models that would have has_many :notes, as: :noteable + mock_models = [ + double('Issue', reflections: { + 'notes' => double('reflection', macro: :has_many, options: { as: :noteable }) + }, table_name: 'issues'), + double('MergeRequest', reflections: { + 'notes' => double('reflection', macro: :has_many, options: { as: :noteable }) + }, table_name: 'merge_requests'), + double('Project', reflections: { + 'issues' => double('reflection', macro: :has_many, options: {}) + }, table_name: 'projects') + ] + + targets = handler.discover_targets(mock_models) + + expect(targets).to contain_exactly('issues', 'merge_requests') + end + end +end diff --git a/spec/lib/gitlab/reflections/relationships/handlers/polymorphic_has_association_handler_spec.rb b/spec/lib/gitlab/reflections/relationships/handlers/polymorphic_has_association_handler_spec.rb new file mode 100644 index 00000000000000..6574b1eea45fb7 --- /dev/null +++ b/spec/lib/gitlab/reflections/relationships/handlers/polymorphic_has_association_handler_spec.rb @@ -0,0 +1,79 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Gitlab::Reflections::Relationships::Handlers::PolymorphicHasAssociationHandler, feature_category: :database do + let(:model) { Issue } + let(:association_name) { 'todos' } + let(:reflection) { model.reflections[association_name.to_s] } + let(:handler) { described_class.new(model, association_name, reflection) } + + describe '#polymorphic?' do + it 'returns true' do + expect(handler.polymorphic?).to be true + end + end + + describe '#build_relationship' do + it 'builds a polymorphic has_many relationship' do + relationship = handler.build_relationship + + expect(relationship).to include( + source_table: 'issues', + target_table: nil, + relationship_type: 'one_to_many', + polymorphic: true, + polymorphic_type_column: 'target_type', + polymorphic_name: 'target' + ) + expect(relationship[:forward_association]).to include( + name: 'todos', + type: 'has_many', + model: 'Issue' + ) + expect(relationship[:has_ar_forward]).to be true + end + end + + describe '#build_relationship_with_target' do + it 'builds a specific relationship for a target table' do + relationship = handler.build_relationship_with_target('todos') + + expect(relationship).to include( + source_table: 'issues', + target_table: 'todos', + relationship_type: 'one_to_many', + polymorphic: true, + polymorphic_type_column: 'target_type', + polymorphic_name: 'target' + ) + expect(relationship[:forward_association]).to include( + name: 'todos', + type: 'has_many', + model: 'Issue' + ) + expect(relationship[:has_ar_forward]).to be true + end + end + + describe '#discover_targets' do + it 'finds models with matching belongs_to polymorphic associations' do + # Mock models that would have belongs_to :target, polymorphic: true + mock_models = [ + double('Todo', reflections: { + 'target' => double('reflection', macro: :belongs_to, polymorphic?: true) + }, table_name: 'todos'), + double('Note', reflections: { + 'noteable' => double('reflection', macro: :belongs_to, polymorphic?: true) + }, table_name: 'notes'), + double('Project', reflections: { + 'namespace' => double('reflection', macro: :belongs_to, polymorphic?: false) + }, table_name: 'projects') + ] + + targets = handler.discover_targets(mock_models) + + expect(targets).to contain_exactly('todos') + end + end +end -- GitLab From 8b2016356db971235a980231201a2fdda30f76ae Mon Sep 17 00:00:00 2001 From: Alex Pooley Date: Thu, 11 Sep 2025 10:25:10 +0800 Subject: [PATCH 02/19] wip --- .../reflections/relationships/crawler.rb | 116 ++++++++++++++---- 1 file changed, 89 insertions(+), 27 deletions(-) diff --git a/lib/gitlab/reflections/relationships/crawler.rb b/lib/gitlab/reflections/relationships/crawler.rb index df3e384cd2af99..e3d983771e9c44 100644 --- a/lib/gitlab/reflections/relationships/crawler.rb +++ b/lib/gitlab/reflections/relationships/crawler.rb @@ -9,6 +9,17 @@ class Crawler < Gitlab::Crawler # Represents a queue item for relationship crawling QueueItem = Struct.new( :table, :column, :value, :relationship_chain, + :source_table, :target_table, :column_matchers, :relationship_info, + keyword_init: true + ) do + def to_h + super.compact + end + end + + # Represents a column matching constraint for relationships + ColumnMatcher = Struct.new( + :source_column, :source_value, :target_column, :target_value, keyword_init: true ) do def to_h @@ -18,11 +29,22 @@ def to_h # Represents a step in the relationship chain ChainStep = Struct.new( - :source_table, :source_column, :source_value, :target_table, :target_column, :target_value, + :source_table, :target_table, :column_matchers, :relationship_info, keyword_init: true ) do def to_s - "#{source_table}.#{source_column} = #{source_value} -> #{target_table}.#{target_column} = #{target_value}" + source_matchers = column_matchers.filter_map do |m| + "#{m.source_column}=#{m.source_value}" if m.source_column + end + + target_matchers = column_matchers.filter_map do |m| + "#{m.target_column}=#{m.target_value}" if m.target_column + end + + source_part = source_matchers.any? ? "#{source_table}(#{source_matchers.join(',')})" : source_table + target_part = target_matchers.any? ? "#{target_table}(#{target_matchers.join(',')})" : target_table + + "#{source_part} -> #{target_part}" end end @@ -139,40 +161,76 @@ def queue_related_tables_from_record(source_table, record, relationship_chain) @normalized_relationships.select { |rel| rel['source_table'] == source_table }.each do |rel| next if should_ignore_table?(rel['target_table']) - queue_value = record[rel['source_column']] - next unless queue_value - - # Handle polymorphic type checking - if rel['relationship_type']&.start_with?('polymorphic_expanded') - type_value = record[rel['polymorphic_type_column']] - next unless type_value == rel['polymorphic_type_value'] - end + # Convert relationship to column matcher format + column_matchers = build_column_matchers(rel, record) + next if column_matchers.empty? queue_relationship( - record: record, relationship_chain: relationship_chain, - target_table: rel['target_table'], - target_column: rel['target_column'], - source_column: rel['source_column'], - source_table: source_table + relationship: rel, + column_matchers: column_matchers ) end end - def queue_relationship( - record:, relationship_chain:, target_table:, target_column:, source_column:, - source_table:) - record_value = record[source_column] - return unless record_value + def build_column_matchers(relationship, record) + matchers = [] - # Build relationship step showing the connection + # Get the source value for the main column match + source_value = record[relationship['source_column']] + return [] unless source_value + + # Add main column matcher + matchers << ColumnMatcher.new( + source_column: relationship['source_column'], + target_column: relationship['target_column'], + source_value: source_value, + target_value: source_value + ) + + # Handle polymorphic type constraints + if relationship['relationship_type']&.start_with?('polymorphic_expanded') + type_column = relationship['polymorphic_type_column'] + type_value = relationship['polymorphic_type_value'] + + if relationship['inverted'] + # For inverted polymorphic: source has the type constraint + record_type_value = record[type_column] + return [] unless record_type_value == type_value + + matchers << ColumnMatcher.new( + source_column: type_column, + source_value: type_value + ) + else + # For normal polymorphic: target has the type constraint + matchers << ColumnMatcher.new( + source_value: type_value, + target_column: type_column + ) + end + end + + matchers + end + + def queue_relationship(relationship_chain:, relationship:, column_matchers:) + source_table = relationship['source_table'] + target_table = relationship['target_table'] + + # Find the matcher that defines the target column for queuing + target_matcher = column_matchers.find(&:target_column) + return unless target_matcher + + target_column = target_matcher.target_column + target_value = target_matcher.target_value + + # Build relationship step with all column matchers relationship_step = ChainStep.new( source_table: source_table, - source_column: source_column, - source_value: record_value, target_table: target_table, - target_column: target_column, - target_value: record_value + column_matchers: column_matchers, + relationship_info: relationship.slice('relationship_type', 'inverted', 'polymorphic') ) new_chain = relationship_chain + [relationship_step] @@ -180,8 +238,12 @@ def queue_relationship( enqueue(QueueItem.new( table: target_table, column: target_column, - value: record_value, - relationship_chain: new_chain + value: target_value, + relationship_chain: new_chain, + source_table: source_table, + target_table: target_table, + column_matchers: column_matchers, + relationship_info: relationship.slice('relationship_type', 'inverted', 'polymorphic') )) end end -- GitLab From b67835d6eb717badfe2c1eb313f4522bff920875 Mon Sep 17 00:00:00 2001 From: Alex Pooley Date: Thu, 11 Sep 2025 10:54:22 +0800 Subject: [PATCH 03/19] wip --- .../reflections/relationships/crawler.rb | 51 ++----------------- .../reflections/relationships/relationship.rb | 11 +++- .../transformers/expand_polymorphic.rb | 51 +++++++++++++++++-- 3 files changed, 61 insertions(+), 52 deletions(-) diff --git a/lib/gitlab/reflections/relationships/crawler.rb b/lib/gitlab/reflections/relationships/crawler.rb index e3d983771e9c44..807265a646825b 100644 --- a/lib/gitlab/reflections/relationships/crawler.rb +++ b/lib/gitlab/reflections/relationships/crawler.rb @@ -17,15 +17,8 @@ def to_h end end - # Represents a column matching constraint for relationships - ColumnMatcher = Struct.new( - :source_column, :source_value, :target_column, :target_value, - keyword_init: true - ) do - def to_h - super.compact - end - end + # Use the ColumnMatcher from the builder for consistency + ColumnMatcher = Transformers::ColumnMatcherBuilder::ColumnMatcher # Represents a step in the relationship chain ChainStep = Struct.new( @@ -174,44 +167,8 @@ def queue_related_tables_from_record(source_table, record, relationship_chain) end def build_column_matchers(relationship, record) - matchers = [] - - # Get the source value for the main column match - source_value = record[relationship['source_column']] - return [] unless source_value - - # Add main column matcher - matchers << ColumnMatcher.new( - source_column: relationship['source_column'], - target_column: relationship['target_column'], - source_value: source_value, - target_value: source_value - ) - - # Handle polymorphic type constraints - if relationship['relationship_type']&.start_with?('polymorphic_expanded') - type_column = relationship['polymorphic_type_column'] - type_value = relationship['polymorphic_type_value'] - - if relationship['inverted'] - # For inverted polymorphic: source has the type constraint - record_type_value = record[type_column] - return [] unless record_type_value == type_value - - matchers << ColumnMatcher.new( - source_column: type_column, - source_value: type_value - ) - else - # For normal polymorphic: target has the type constraint - matchers << ColumnMatcher.new( - source_value: type_value, - target_column: type_column - ) - end - end - - matchers + # Use the generic ColumnMatcherBuilder instead of custom logic + Transformers::ColumnMatcherBuilder.build_matchers(relationship, record) end def queue_relationship(relationship_chain:, relationship:, column_matchers:) diff --git a/lib/gitlab/reflections/relationships/relationship.rb b/lib/gitlab/reflections/relationships/relationship.rb index 7f86543c34e433..efaddd8df1b0f9 100644 --- a/lib/gitlab/reflections/relationships/relationship.rb +++ b/lib/gitlab/reflections/relationships/relationship.rb @@ -50,7 +50,13 @@ module Relationships # @attr source_table [String] The name of the source table in the relationship # @attr target_table [String] The name of the target table in the relationship # @attr source_column [String] The column in the source table that is referenced (usually primary key) + # - DEPRECATED: use source_columns # @attr target_column [String] The column in the target table that references the source (foreign key) + # - DEPRECATED: use target_columns + # @attr source_columns [Array] The columns in the source table that are referenced + # (supports composite keys and polymorphic types) + # @attr target_columns [Array] The columns in the target table that reference the source + # (supports composite keys and polymorphic types) # @attr foreign_key [String] The foreign key column name that establishes the relationship # @attr foreign_key_table [String] The table that contains the foreign key column # @attr relationship_type [String] The type of relationship directly derived from ActiveRecord macro (one_to_one, one_to_many, many_to_one, many_to_many) @@ -81,7 +87,8 @@ class Relationship ].freeze attr_accessor :source_table, :target_table, :foreign_key, :foreign_key_table, - :source_column, :target_column, :relationship_type, :forward_association, :reverse_association, + :source_column, :target_column, :source_columns, :target_columns, + :relationship_type, :forward_association, :reverse_association, :through_table, :through_target_key, :polymorphic, :polymorphic_type_column, :polymorphic_name, :has_ar_forward, :has_ar_reverse, :has_fk_constraint, :fk_metadata, :data_sources, :scope_conditions, :is_through_association @@ -153,6 +160,8 @@ def to_h target_table: target_table, source_column: source_column, target_column: target_column, + source_columns: source_columns, + target_columns: target_columns, foreign_key: foreign_key, foreign_key_table: foreign_key_table, relationship_type: relationship_type, diff --git a/lib/gitlab/reflections/relationships/transformers/expand_polymorphic.rb b/lib/gitlab/reflections/relationships/transformers/expand_polymorphic.rb index 721d395b3b428d..06ce639dec2566 100644 --- a/lib/gitlab/reflections/relationships/transformers/expand_polymorphic.rb +++ b/lib/gitlab/reflections/relationships/transformers/expand_polymorphic.rb @@ -19,13 +19,13 @@ def transform @relationships.each do |rel| if polymorphic_relationship?(rel) if rel['target_table'] # Already expanded - expanded << rel + expanded << add_column_matchers(rel) else # Expand base polymorphic into specific relationships expanded.concat(expand_polymorphic_relationship(rel)) end else - expanded << rel + expanded << add_column_matchers(rel) end end @@ -43,14 +43,44 @@ def expand_polymorphic_relationship(rel) target_tables = find_polymorphic_targets(rel) target_tables.map do |target_table| - rel.merge( + expanded_rel = rel.merge( 'target_table' => target_table, 'relationship_type' => 'polymorphic_expanded', - 'polymorphic_type_value' => target_table.classify + 'polymorphic_type_value' => target_table.classify, + 'polymorphic_type_column' => rel['polymorphic_type_column'] || infer_type_column(rel) ) + add_column_matchers(expanded_rel) end end + def add_column_matchers(relationship) + # Ensure source_columns and target_columns are arrays + source_columns = Array(relationship['source_column'] || relationship['source_columns']) + target_columns = Array(relationship['target_column'] || relationship['target_columns']) + + # Add polymorphic type column if this is a polymorphic relationship + if polymorphic_expanded?(relationship) + type_column = relationship['polymorphic_type_column'] + + if relationship['inverted'] + # For inverted polymorphic: type constraint is on source + source_columns << type_column unless source_columns.include?(type_column) + else + # For normal polymorphic: type constraint is on target + target_columns << type_column unless target_columns.include?(type_column) + end + end + + relationship.merge( + 'source_columns' => source_columns, + 'target_columns' => target_columns + ) + end + + def polymorphic_expanded?(relationship) + relationship['relationship_type']&.start_with?('polymorphic_expanded') + end + def find_polymorphic_targets(rel) # Look through all relationships to find ones that target this polymorphic polymorphic_name = rel['polymorphic_name'] @@ -64,6 +94,19 @@ def find_polymorphic_targets(rel) targets.to_a end + + def infer_type_column(rel) + # Infer the type column name from the polymorphic association + # Standard Rails convention: if foreign key is 'commentable_id', type column is 'commentable_type' + foreign_key = rel['source_column'] || rel['target_column'] + return unless foreign_key + + if foreign_key.end_with?('_id') + foreign_key.sub(/_id$/, '_type') + else + "#{foreign_key}_type" + end + end end end end -- GitLab From c7b00448f1e2dab534da9138b58352325cd72081 Mon Sep 17 00:00:00 2001 From: Alex Pooley Date: Thu, 11 Sep 2025 11:01:58 +0800 Subject: [PATCH 04/19] wip --- .../reflections/relationships/relationship.rb | 30 ++++++++----------- .../transformers/expand_polymorphic.rb | 6 ++-- .../transformers/invert_relationships.rb | 14 ++++----- 3 files changed, 23 insertions(+), 27 deletions(-) diff --git a/lib/gitlab/reflections/relationships/relationship.rb b/lib/gitlab/reflections/relationships/relationship.rb index efaddd8df1b0f9..bfa457c9e45d17 100644 --- a/lib/gitlab/reflections/relationships/relationship.rb +++ b/lib/gitlab/reflections/relationships/relationship.rb @@ -15,15 +15,20 @@ module Relationships # # The source/target and foreign_key attributes serve different purposes and are NOT redundant: # - # - source_table/source_column: The table/column being referenced (usually the "parent" with primary key) - # - target_table/target_column: The table/column doing the referencing (usually the "child" with foreign key) + # - source_table/source_columns: The table/columns being referenced (usually the "parent" with primary key) + # - target_table/target_columns: The table/columns doing the referencing (usually the "child" with foreign key) # - foreign_key/foreign_key_table: The actual physical foreign key column and which table it's in # # Example for users → posts relationship: - # source_table: 'users', source_column: 'id' (the referenced primary key) - # target_table: 'posts', target_column: 'user_id' (the referencing column) + # source_table: 'users', source_columns: ['id'] (the referenced primary key) + # target_table: 'posts', target_columns: ['user_id'] (the referencing column) # foreign_key: 'user_id', foreign_key_table: 'posts' (the actual FK constraint) # + # Example for polymorphic notes → issues relationship: + # source_table: 'notes', source_columns: ['noteable_id', 'noteable_type'] + # target_table: 'issues', target_columns: ['id', 'Issue'] + # foreign_key: 'noteable_id', foreign_key_table: 'notes' + # # This distinction matters for: # - Through relationships: FK might be in an intermediate table # - Complex joins: FK column name might differ from target column name @@ -43,16 +48,12 @@ module Relationships # 2. Foreign Key constraints always create relationships from referenced → referencing table: # - FK constraint posts.user_id → users.id creates relationship (users → posts) # - # 3. Collection merges relationships with same key (source_table, target_table, source_column, target_column): + # 3. Collection merges relationships with same key (source_table, target_table, source_columns, target_columns): # - When both User.has_many :posts and Post.belongs_to :user exist, they merge into # one bidirectional relationship with both has_ar_forward: true and has_ar_reverse: true # # @attr source_table [String] The name of the source table in the relationship # @attr target_table [String] The name of the target table in the relationship - # @attr source_column [String] The column in the source table that is referenced (usually primary key) - # - DEPRECATED: use source_columns - # @attr target_column [String] The column in the target table that references the source (foreign key) - # - DEPRECATED: use target_columns # @attr source_columns [Array] The columns in the source table that are referenced # (supports composite keys and polymorphic types) # @attr target_columns [Array] The columns in the target table that reference the source @@ -87,7 +88,7 @@ class Relationship ].freeze attr_accessor :source_table, :target_table, :foreign_key, :foreign_key_table, - :source_column, :target_column, :source_columns, :target_columns, + :source_columns, :target_columns, :relationship_type, :forward_association, :reverse_association, :through_table, :through_target_key, :polymorphic, :polymorphic_type_column, :polymorphic_name, :has_ar_forward, :has_ar_reverse, @@ -111,11 +112,8 @@ def initialize(attributes = {}) attributes[:is_through_association] = false unless attributes.key?(:is_through_association) attributes[:data_sources] = [] unless attributes.key?(:data_sources) - # Set default column values if not provided - attributes[:source_column] ||= 'id' unless attributes.key?(:source_column) - if attributes[:foreign_key] && !attributes.key?(:target_column) - attributes[:target_column] ||= attributes[:foreign_key] - end + # Set default source_columns to ['id'] if not provided + attributes[:source_columns] ||= ['id'] unless attributes.key?(:source_columns) super end @@ -158,8 +156,6 @@ def to_h { source_table: source_table, target_table: target_table, - source_column: source_column, - target_column: target_column, source_columns: source_columns, target_columns: target_columns, foreign_key: foreign_key, diff --git a/lib/gitlab/reflections/relationships/transformers/expand_polymorphic.rb b/lib/gitlab/reflections/relationships/transformers/expand_polymorphic.rb index 06ce639dec2566..2b5553244d6be9 100644 --- a/lib/gitlab/reflections/relationships/transformers/expand_polymorphic.rb +++ b/lib/gitlab/reflections/relationships/transformers/expand_polymorphic.rb @@ -55,8 +55,8 @@ def expand_polymorphic_relationship(rel) def add_column_matchers(relationship) # Ensure source_columns and target_columns are arrays - source_columns = Array(relationship['source_column'] || relationship['source_columns']) - target_columns = Array(relationship['target_column'] || relationship['target_columns']) + source_columns = Array(relationship['source_columns']) + target_columns = Array(relationship['target_columns']) # Add polymorphic type column if this is a polymorphic relationship if polymorphic_expanded?(relationship) @@ -98,7 +98,7 @@ def find_polymorphic_targets(rel) def infer_type_column(rel) # Infer the type column name from the polymorphic association # Standard Rails convention: if foreign key is 'commentable_id', type column is 'commentable_type' - foreign_key = rel['source_column'] || rel['target_column'] + foreign_key = rel['foreign_key'] return unless foreign_key if foreign_key.end_with?('_id') diff --git a/lib/gitlab/reflections/relationships/transformers/invert_relationships.rb b/lib/gitlab/reflections/relationships/transformers/invert_relationships.rb index ae7f48ce99a202..b8eee7f404f934 100644 --- a/lib/gitlab/reflections/relationships/transformers/invert_relationships.rb +++ b/lib/gitlab/reflections/relationships/transformers/invert_relationships.rb @@ -15,14 +15,14 @@ def initialize(relationships) def transform inverted = @relationships.dup - + @relationships.each do |rel| next if should_skip_inversion?(rel) - + inverse_rel = create_inverse_relationship(rel) inverted << inverse_rel if inverse_rel end - + inverted end @@ -36,13 +36,13 @@ def should_skip_inversion?(rel) end def create_inverse_relationship(rel) - return nil unless rel['source_table'] && rel['target_table'] + return unless rel['source_table'] && rel['target_table'] { 'source_table' => rel['target_table'], 'target_table' => rel['source_table'], - 'source_column' => rel['target_column'], - 'target_column' => rel['source_column'], + 'source_columns' => rel['target_columns'], + 'target_columns' => rel['source_columns'], 'relationship_type' => invert_relationship_type(rel['relationship_type']), 'inverted' => true, 'original_relationship' => rel.slice('source_table', 'target_table', 'relationship_type') @@ -73,4 +73,4 @@ def preserve_polymorphic_info(rel) end end end -end \ No newline at end of file +end -- GitLab From 9d3dca9148b067fdbb766ef674aecbde8c0a8caa Mon Sep 17 00:00:00 2001 From: Alex Pooley Date: Thu, 11 Sep 2025 11:11:29 +0800 Subject: [PATCH 05/19] wip --- .../transformers/column_matcher_builder.rb | 94 +++++++++++++++++++ 1 file changed, 94 insertions(+) create mode 100644 lib/gitlab/reflections/relationships/transformers/column_matcher_builder.rb diff --git a/lib/gitlab/reflections/relationships/transformers/column_matcher_builder.rb b/lib/gitlab/reflections/relationships/transformers/column_matcher_builder.rb new file mode 100644 index 00000000000000..099d40e3a950e7 --- /dev/null +++ b/lib/gitlab/reflections/relationships/transformers/column_matcher_builder.rb @@ -0,0 +1,94 @@ +# frozen_string_literal: true + +module Gitlab + module Reflections + module Relationships + module Transformers + # Generic utility class for building column matchers from relationships + # and record data. This extracts the column matching logic from the Crawler + # making it reusable across different contexts. + class ColumnMatcherBuilder + # Represents a column matching constraint for relationships + ColumnMatcher = Struct.new( + :source_column, :source_value, :target_column, :target_value, + keyword_init: true + ) do + def to_h + super.compact + end + end + + def self.build_matchers(relationship, record) + new(relationship, record).build + end + + def initialize(relationship, record) + @relationship = relationship + @record = record + end + + def build + matchers = [] + + # Get source and target columns + source_columns = Array(@relationship['source_columns']) + target_columns = Array(@relationship['target_columns']) + + # Build matchers for each column pair + source_columns.zip(target_columns).each do |source_col, target_col| + next unless source_col && target_col + + source_value = get_column_value(source_col) + next unless source_value + + # For polymorphic type columns, validate the value matches expected + next if polymorphic_type_column?(source_col) && !validate_polymorphic_type(source_col, source_value) + + matchers << ColumnMatcher.new( + source_column: source_col, + target_column: target_col, + source_value: source_value, + target_value: get_target_value(source_col, source_value) + ) + end + + matchers + end + + private + + def get_column_value(column) + @record[column] + end + + def get_target_value(source_column, source_value) + # For polymorphic type columns, use the expected type value + if polymorphic_type_column?(source_column) + @relationship['polymorphic_type_value'] + else + source_value + end + end + + def polymorphic_type_column?(column) + @relationship['polymorphic_type_column'] == column + end + + def validate_polymorphic_type(column, value) + return true unless polymorphic_type_column?(column) + + expected_value = @relationship['polymorphic_type_value'] + + if @relationship['inverted'] + # For inverted polymorphic: source record must have matching type + value == expected_value + else + # For normal polymorphic: we're setting the target constraint + true + end + end + end + end + end + end +end -- GitLab From 686e2574a0430d013d7ca78328e0133a50cd0a6d Mon Sep 17 00:00:00 2001 From: Alex Pooley Date: Thu, 11 Sep 2025 11:17:18 +0800 Subject: [PATCH 06/19] wip --- .../reflections/relationships/crawler.rb | 49 +++++++++- .../transformers/column_matcher_builder.rb | 94 ------------------- 2 files changed, 45 insertions(+), 98 deletions(-) delete mode 100644 lib/gitlab/reflections/relationships/transformers/column_matcher_builder.rb diff --git a/lib/gitlab/reflections/relationships/crawler.rb b/lib/gitlab/reflections/relationships/crawler.rb index 807265a646825b..c08d1875ed14dd 100644 --- a/lib/gitlab/reflections/relationships/crawler.rb +++ b/lib/gitlab/reflections/relationships/crawler.rb @@ -17,8 +17,15 @@ def to_h end end - # Use the ColumnMatcher from the builder for consistency - ColumnMatcher = Transformers::ColumnMatcherBuilder::ColumnMatcher + # Represents a column matching constraint for relationships + ColumnMatcher = Struct.new( + :source_column, :source_value, :target_column, :target_value, + keyword_init: true + ) do + def to_h + super.compact + end + end # Represents a step in the relationship chain ChainStep = Struct.new( @@ -167,8 +174,42 @@ def queue_related_tables_from_record(source_table, record, relationship_chain) end def build_column_matchers(relationship, record) - # Use the generic ColumnMatcherBuilder instead of custom logic - Transformers::ColumnMatcherBuilder.build_matchers(relationship, record) + source_columns = Array(relationship['source_columns']) + target_columns = Array(relationship['target_columns']) + matchers = [] + + # Build matchers for each column pair + source_columns.zip(target_columns).each do |source_col, target_col| + next unless source_col && target_col + + source_value = record[source_col] + next unless source_value + + # For polymorphic type columns, validate and get target value + if relationship['polymorphic_type_column'] == source_col + # Validate polymorphic type constraint + expected_value = relationship['polymorphic_type_value'] + + if relationship['inverted'] && source_value != expected_value + # For inverted polymorphic: source record must have matching type + next + end + + # Use expected type value as target value + target_value = expected_value + else + target_value = source_value + end + + matchers << ColumnMatcher.new( + source_column: source_col, + target_column: target_col, + source_value: source_value, + target_value: target_value + ) + end + + matchers end def queue_relationship(relationship_chain:, relationship:, column_matchers:) diff --git a/lib/gitlab/reflections/relationships/transformers/column_matcher_builder.rb b/lib/gitlab/reflections/relationships/transformers/column_matcher_builder.rb deleted file mode 100644 index 099d40e3a950e7..00000000000000 --- a/lib/gitlab/reflections/relationships/transformers/column_matcher_builder.rb +++ /dev/null @@ -1,94 +0,0 @@ -# frozen_string_literal: true - -module Gitlab - module Reflections - module Relationships - module Transformers - # Generic utility class for building column matchers from relationships - # and record data. This extracts the column matching logic from the Crawler - # making it reusable across different contexts. - class ColumnMatcherBuilder - # Represents a column matching constraint for relationships - ColumnMatcher = Struct.new( - :source_column, :source_value, :target_column, :target_value, - keyword_init: true - ) do - def to_h - super.compact - end - end - - def self.build_matchers(relationship, record) - new(relationship, record).build - end - - def initialize(relationship, record) - @relationship = relationship - @record = record - end - - def build - matchers = [] - - # Get source and target columns - source_columns = Array(@relationship['source_columns']) - target_columns = Array(@relationship['target_columns']) - - # Build matchers for each column pair - source_columns.zip(target_columns).each do |source_col, target_col| - next unless source_col && target_col - - source_value = get_column_value(source_col) - next unless source_value - - # For polymorphic type columns, validate the value matches expected - next if polymorphic_type_column?(source_col) && !validate_polymorphic_type(source_col, source_value) - - matchers << ColumnMatcher.new( - source_column: source_col, - target_column: target_col, - source_value: source_value, - target_value: get_target_value(source_col, source_value) - ) - end - - matchers - end - - private - - def get_column_value(column) - @record[column] - end - - def get_target_value(source_column, source_value) - # For polymorphic type columns, use the expected type value - if polymorphic_type_column?(source_column) - @relationship['polymorphic_type_value'] - else - source_value - end - end - - def polymorphic_type_column?(column) - @relationship['polymorphic_type_column'] == column - end - - def validate_polymorphic_type(column, value) - return true unless polymorphic_type_column?(column) - - expected_value = @relationship['polymorphic_type_value'] - - if @relationship['inverted'] - # For inverted polymorphic: source record must have matching type - value == expected_value - else - # For normal polymorphic: we're setting the target constraint - true - end - end - end - end - end - end -end -- GitLab From a133c3de24663c3fa43ff8dd004c6d7813ba2928 Mon Sep 17 00:00:00 2001 From: Alex Pooley Date: Thu, 11 Sep 2025 11:31:12 +0800 Subject: [PATCH 07/19] wip --- .../reflections/relationships/crawler.rb | 18 +----------------- 1 file changed, 1 insertion(+), 17 deletions(-) diff --git a/lib/gitlab/reflections/relationships/crawler.rb b/lib/gitlab/reflections/relationships/crawler.rb index c08d1875ed14dd..199199a7615977 100644 --- a/lib/gitlab/reflections/relationships/crawler.rb +++ b/lib/gitlab/reflections/relationships/crawler.rb @@ -185,27 +185,11 @@ def build_column_matchers(relationship, record) source_value = record[source_col] next unless source_value - # For polymorphic type columns, validate and get target value - if relationship['polymorphic_type_column'] == source_col - # Validate polymorphic type constraint - expected_value = relationship['polymorphic_type_value'] - - if relationship['inverted'] && source_value != expected_value - # For inverted polymorphic: source record must have matching type - next - end - - # Use expected type value as target value - target_value = expected_value - else - target_value = source_value - end - matchers << ColumnMatcher.new( source_column: source_col, target_column: target_col, source_value: source_value, - target_value: target_value + target_value: source_value ) end -- GitLab From f6159c9d7549a0a16bd9fde76dfc648ec933d3a5 Mon Sep 17 00:00:00 2001 From: Alex Pooley Date: Fri, 12 Sep 2025 07:29:32 +0800 Subject: [PATCH 08/19] wip --- .../reflections/relationships/crawler.rb | 101 ++++++++---------- 1 file changed, 46 insertions(+), 55 deletions(-) diff --git a/lib/gitlab/reflections/relationships/crawler.rb b/lib/gitlab/reflections/relationships/crawler.rb index 199199a7615977..8e6a84188064c6 100644 --- a/lib/gitlab/reflections/relationships/crawler.rb +++ b/lib/gitlab/reflections/relationships/crawler.rb @@ -9,17 +9,6 @@ class Crawler < Gitlab::Crawler # Represents a queue item for relationship crawling QueueItem = Struct.new( :table, :column, :value, :relationship_chain, - :source_table, :target_table, :column_matchers, :relationship_info, - keyword_init: true - ) do - def to_h - super.compact - end - end - - # Represents a column matching constraint for relationships - ColumnMatcher = Struct.new( - :source_column, :source_value, :target_column, :target_value, keyword_init: true ) do def to_h @@ -29,22 +18,29 @@ def to_h # Represents a step in the relationship chain ChainStep = Struct.new( - :source_table, :target_table, :column_matchers, :relationship_info, + :relationship, :source_values, :target_values, keyword_init: true ) do def to_s - source_matchers = column_matchers.filter_map do |m| - "#{m.source_column}=#{m.source_value}" if m.source_column + source_table = relationship['source_table'] + target_table = relationship['target_table'] + + # Build source part with column=value pairs + source_columns = Array(relationship['source_columns']) + source_parts = source_columns.zip(source_values).filter_map do |col, val| + "#{col}=#{val}" if col && val end - target_matchers = column_matchers.filter_map do |m| - "#{m.target_column}=#{m.target_value}" if m.target_column + # Build target part with column=value pairs + target_columns = Array(relationship['target_columns']) + target_parts = target_columns.zip(target_values).filter_map do |col, val| + "#{col}=#{val}" if col && val end - source_part = source_matchers.any? ? "#{source_table}(#{source_matchers.join(',')})" : source_table - target_part = target_matchers.any? ? "#{target_table}(#{target_matchers.join(',')})" : target_table + source_display = source_parts.any? ? "#{source_table}(#{source_parts.join(',')})" : source_table + target_display = target_parts.any? ? "#{target_table}(#{target_parts.join(',')})" : target_table - "#{source_part} -> #{target_part}" + "#{source_display} -> #{target_display}" end end @@ -161,58 +157,57 @@ def queue_related_tables_from_record(source_table, record, relationship_chain) @normalized_relationships.select { |rel| rel['source_table'] == source_table }.each do |rel| next if should_ignore_table?(rel['target_table']) - # Convert relationship to column matcher format - column_matchers = build_column_matchers(rel, record) - next if column_matchers.empty? + # Extract values for this relationship + relationship_values = extract_relationship_values(rel, record) + next if relationship_values.empty? queue_relationship( relationship_chain: relationship_chain, relationship: rel, - column_matchers: column_matchers + relationship_values: relationship_values ) end end - def build_column_matchers(relationship, record) + def extract_relationship_values(relationship, record) source_columns = Array(relationship['source_columns']) target_columns = Array(relationship['target_columns']) - matchers = [] - # Build matchers for each column pair - source_columns.zip(target_columns).each do |source_col, target_col| - next unless source_col && target_col + # Extract source values from the record + source_values = source_columns.filter_map { |col| record[col] if record[col] } - source_value = record[source_col] - next unless source_value + # For most relationships, target values are the same as source values + # (they represent the same data, just in different tables) + target_values = source_values.dup - matchers << ColumnMatcher.new( - source_column: source_col, - target_column: target_col, - source_value: source_value, - target_value: source_value - ) - end + # Return empty if we don't have all required values + return {} if source_values.length != source_columns.length - matchers + { + source_values: source_values, + target_values: target_values, + source_columns: source_columns, + target_columns: target_columns + } end - def queue_relationship(relationship_chain:, relationship:, column_matchers:) - source_table = relationship['source_table'] + def queue_relationship(relationship_chain:, relationship:, relationship_values:) target_table = relationship['target_table'] + target_columns = relationship_values[:target_columns] + target_values = relationship_values[:target_values] - # Find the matcher that defines the target column for queuing - target_matcher = column_matchers.find(&:target_column) - return unless target_matcher + # Use the first target column/value pair for queuing + # (most relationships have single columns, multi-column support can be enhanced later) + target_column = target_columns.first + target_value = target_values.first - target_column = target_matcher.target_column - target_value = target_matcher.target_value + return unless target_column && target_value - # Build relationship step with all column matchers + # Build relationship step relationship_step = ChainStep.new( - source_table: source_table, - target_table: target_table, - column_matchers: column_matchers, - relationship_info: relationship.slice('relationship_type', 'inverted', 'polymorphic') + relationship: relationship, + source_values: relationship_values[:source_values], + target_values: relationship_values[:target_values] ) new_chain = relationship_chain + [relationship_step] @@ -221,11 +216,7 @@ def queue_relationship(relationship_chain:, relationship:, column_matchers:) table: target_table, column: target_column, value: target_value, - relationship_chain: new_chain, - source_table: source_table, - target_table: target_table, - column_matchers: column_matchers, - relationship_info: relationship.slice('relationship_type', 'inverted', 'polymorphic') + relationship_chain: new_chain )) end end -- GitLab From 99ce07b5603949465e31384f826a1a72e68a3a24 Mon Sep 17 00:00:00 2001 From: Alex Pooley Date: Fri, 12 Sep 2025 09:32:33 +0800 Subject: [PATCH 09/19] wip --- .../reflections/relationships/crawler.rb | 67 ++++++++++---- .../reflections/relationships/relationship.rb | 4 +- .../transformers/expand_polymorphic.rb | 90 +++++++++++++------ 3 files changed, 114 insertions(+), 47 deletions(-) diff --git a/lib/gitlab/reflections/relationships/crawler.rb b/lib/gitlab/reflections/relationships/crawler.rb index 8e6a84188064c6..1166ffb756bddd 100644 --- a/lib/gitlab/reflections/relationships/crawler.rb +++ b/lib/gitlab/reflections/relationships/crawler.rb @@ -8,7 +8,7 @@ module Relationships class Crawler < Gitlab::Crawler # Represents a queue item for relationship crawling QueueItem = Struct.new( - :table, :column, :value, :relationship_chain, + :table, :query_conditions, :relationship_chain, keyword_init: true ) do def to_h @@ -80,18 +80,24 @@ def initialize_queue # Initialize queue with seed query enqueue(QueueItem.new( table: @seed_table, - column: 'id', - value: @seed_id, + query_conditions: { 'id' => @seed_id }, relationship_chain: [] )) end def item_key(queue_item) - "#{queue_item.table}:#{queue_item.column}:#{queue_item.value}" + # Create a unique key representing the entire query + conditions_key = queue_item.query_conditions.sort.map { |col, val| "#{col}:#{val}" }.join('&') + "#{queue_item.table}:#{conditions_key}" end def crawl_item(queue_item) - sql = "SELECT * FROM #{queue_item.table} WHERE #{queue_item.column} = #{queue_item.value}" + # Build WHERE clause from key/value conditions + where_conditions = queue_item.query_conditions.map do |col, val| + "#{col} = #{ApplicationRecord.connection.quote(val)}" + end + + sql = "SELECT * FROM #{queue_item.table} WHERE #{where_conditions.join(' AND ')}" ApplicationRecord.connection.select_all(sql) rescue StandardError => e notify_observers(:crawl_error, { @@ -176,13 +182,18 @@ def extract_relationship_values(relationship, record) # Extract source values from the record source_values = source_columns.filter_map { |col| record[col] if record[col] } - # For most relationships, target values are the same as source values - # (they represent the same data, just in different tables) - target_values = source_values.dup - # Return empty if we don't have all required values return {} if source_values.length != source_columns.length + # Check scope_conditions to see if this relationship applies to this record + if relationship['scope_conditions'].present? + return {} unless scope_conditions_match?(relationship['scope_conditions'], record) + end + + # Pipeline has already handled polymorphic expansion, so target values + # are directly mapped from source values + target_values = source_values.dup + { source_values: source_values, target_values: target_values, @@ -191,17 +202,40 @@ def extract_relationship_values(relationship, record) } end + # Evaluates scope_conditions against a record to determine if the relationship applies + # scope_conditions is now an array of SQL condition strings like ["status = 'active'", "type = 'Issue'"] + def scope_conditions_match?(scope_conditions, record) + return true unless scope_conditions.is_a?(Array) + + scope_conditions.all? do |condition_sql| + evaluate_sql_condition(condition_sql, record) + end + end + + # Simple SQL condition evaluator for common patterns + # Handles basic equality conditions like "column = 'value'" + def evaluate_sql_condition(sql_condition, record) + # Match pattern: column = 'value' or column = "value" + if sql_condition.match(/^\s*(\w+)\s*=\s*['"]([^'"]+)['"]\s*$/) + column = $1 + expected_value = $2 + record[column] == expected_value + else + # For complex conditions we can't parse, assume they match + # In a real implementation, you might want to log this or use a proper SQL parser + true + end + end + def queue_relationship(relationship_chain:, relationship:, relationship_values:) target_table = relationship['target_table'] target_columns = relationship_values[:target_columns] target_values = relationship_values[:target_values] - # Use the first target column/value pair for queuing - # (most relationships have single columns, multi-column support can be enhanced later) - target_column = target_columns.first - target_value = target_values.first + return if target_columns.empty? || target_values.empty? - return unless target_column && target_value + # Build query conditions as key/value pairs + query_conditions = target_columns.zip(target_values).to_h # Build relationship step relationship_step = ChainStep.new( @@ -214,12 +248,11 @@ def queue_relationship(relationship_chain:, relationship:, relationship_values:) enqueue(QueueItem.new( table: target_table, - column: target_column, - value: target_value, + query_conditions: query_conditions, relationship_chain: new_chain )) end end end end -end +end \ No newline at end of file diff --git a/lib/gitlab/reflections/relationships/relationship.rb b/lib/gitlab/reflections/relationships/relationship.rb index bfa457c9e45d17..8b939b0f35f096 100644 --- a/lib/gitlab/reflections/relationships/relationship.rb +++ b/lib/gitlab/reflections/relationships/relationship.rb @@ -73,7 +73,8 @@ module Relationships # @attr has_fk_constraint [Boolean] Whether a foreign key constraint exists in the database # @attr fk_metadata [Hash] Additional metadata about the foreign key constraint # @attr data_sources [Array] List of data sources that contributed to discovering this relationship - # @attr scope_conditions [Hash] Additional conditions or scopes that apply to this relationship + # @attr scope_conditions [String, Hash] Additional conditions or scopes that apply to this relationship. + # Can be a SQL string (e.g., "status = 'active'") or a hash of column-value pairs. class Relationship include ActiveModel::Model include Gitlab::Reflections::Relationships::Predicates @@ -111,6 +112,7 @@ def initialize(attributes = {}) attributes[:has_fk_constraint] = false unless attributes.key?(:has_fk_constraint) attributes[:is_through_association] = false unless attributes.key?(:is_through_association) attributes[:data_sources] = [] unless attributes.key?(:data_sources) + attributes[:scope_conditions] = nil unless attributes.key?(:scope_conditions) # Set default source_columns to ['id'] if not provided attributes[:source_columns] ||= ['id'] unless attributes.key?(:source_columns) diff --git a/lib/gitlab/reflections/relationships/transformers/expand_polymorphic.rb b/lib/gitlab/reflections/relationships/transformers/expand_polymorphic.rb index 2b5553244d6be9..be627550dad602 100644 --- a/lib/gitlab/reflections/relationships/transformers/expand_polymorphic.rb +++ b/lib/gitlab/reflections/relationships/transformers/expand_polymorphic.rb @@ -4,6 +4,16 @@ module Gitlab module Reflections module Relationships module Transformers + # Transforms polymorphic relationships into concrete has_many relationships with scope conditions. + # + # This transformer converts a single polymorphic relationship like: + # User.has_many :notes, as: :noteable + # + # Into multiple concrete has_many relationships: + # User.has_many :issue_notes, -> { where(noteable_type: 'Issue') }, class_name: 'Note' + # User.has_many :merge_request_notes, -> { where(noteable_type: 'MergeRequest') }, class_name: 'Note' + # + # The polymorphic type constraint becomes a scope_condition on each concrete relationship. class ExpandPolymorphic def self.call(relationships) new(relationships).transform @@ -19,13 +29,13 @@ def transform @relationships.each do |rel| if polymorphic_relationship?(rel) if rel['target_table'] # Already expanded - expanded << add_column_matchers(rel) + expanded << normalize_columns(rel) else - # Expand base polymorphic into specific relationships - expanded.concat(expand_polymorphic_relationship(rel)) + # Convert polymorphic relationship into multiple has_many relationships + expanded.concat(expand_into_has_many_relationships(rel)) end else - expanded << add_column_matchers(rel) + expanded << normalize_columns(rel) end end @@ -38,38 +48,64 @@ def polymorphic_relationship?(rel) rel['polymorphic'] end - def expand_polymorphic_relationship(rel) - # Find all relationships that reference this polymorphic association + # Converts a single polymorphic relationship into multiple concrete has_many relationships + # Each concrete relationship has a scope_condition that filters by the polymorphic type + def expand_into_has_many_relationships(rel) target_tables = find_polymorphic_targets(rel) target_tables.map do |target_table| - expanded_rel = rel.merge( + type_column = rel['polymorphic_type_column'] || infer_type_column(rel) + type_value = target_table.classify + + # Create a concrete has_many relationship with scope condition + concrete_relationship = rel.merge( 'target_table' => target_table, - 'relationship_type' => 'polymorphic_expanded', - 'polymorphic_type_value' => target_table.classify, - 'polymorphic_type_column' => rel['polymorphic_type_column'] || infer_type_column(rel) + 'relationship_type' => 'has_many', # Convert to has_many + 'polymorphic' => false, # No longer polymorphic + 'polymorphic_type_value' => type_value, + 'polymorphic_type_column' => type_column, + 'scope_conditions' => build_scope_condition(rel['scope_conditions'], type_column, type_value) ) - add_column_matchers(expanded_rel) + + normalize_columns(concrete_relationship) end end - def add_column_matchers(relationship) - # Ensure source_columns and target_columns are arrays - source_columns = Array(relationship['source_columns']) - target_columns = Array(relationship['target_columns']) + # Builds the scope condition for the concrete relationship + # Returns an array of SQL condition strings that can be easily combined + def build_scope_condition(existing_conditions, type_column, type_value) + conditions = [] - # Add polymorphic type column if this is a polymorphic relationship - if polymorphic_expanded?(relationship) - type_column = relationship['polymorphic_type_column'] + # Add existing conditions + conditions.concat(normalize_existing_conditions(existing_conditions)) if existing_conditions.present? - if relationship['inverted'] - # For inverted polymorphic: type constraint is on source - source_columns << type_column unless source_columns.include?(type_column) - else - # For normal polymorphic: type constraint is on target - target_columns << type_column unless target_columns.include?(type_column) - end + # Add polymorphic type condition + conditions << "#{type_column} = '#{type_value}'" + + conditions + end + + # Normalizes existing conditions to the standard array of strings format + def normalize_existing_conditions(existing_conditions) + case existing_conditions + when Array + # Already in correct format (array of strings) + existing_conditions + when String + # Split SQL string on AND + existing_conditions.split(/\s+AND\s+/i).map(&:strip) + when Hash + # Convert hash to array of SQL strings + existing_conditions.map { |col, val| "#{col} = '#{val}'" } + else + [] end + end + + def normalize_columns(relationship) + # Ensure source_columns and target_columns are arrays + source_columns = Array(relationship['source_columns']) + target_columns = Array(relationship['target_columns']) relationship.merge( 'source_columns' => source_columns, @@ -77,10 +113,6 @@ def add_column_matchers(relationship) ) end - def polymorphic_expanded?(relationship) - relationship['relationship_type']&.start_with?('polymorphic_expanded') - end - def find_polymorphic_targets(rel) # Look through all relationships to find ones that target this polymorphic polymorphic_name = rel['polymorphic_name'] -- GitLab From bad0a441329597b342609245646e7078b68d9845 Mon Sep 17 00:00:00 2001 From: Alex Pooley Date: Fri, 12 Sep 2025 11:34:55 +0800 Subject: [PATCH 10/19] wip --- .../reflections/relationships/crawler.rb | 35 ++-------- .../handlers/has_association_handler.rb | 14 ---- .../polymorphic_has_association_handler.rb | 2 - .../reflections/relationships/relationship.rb | 12 ++-- .../transformers/expand_polymorphic.rb | 65 +++++++------------ .../polymorphic_edge_cases_spec.rb | 18 ----- .../handlers/base_handler_spec.rb | 1 - 7 files changed, 33 insertions(+), 114 deletions(-) diff --git a/lib/gitlab/reflections/relationships/crawler.rb b/lib/gitlab/reflections/relationships/crawler.rb index 1166ffb756bddd..f6dc698b44e39f 100644 --- a/lib/gitlab/reflections/relationships/crawler.rb +++ b/lib/gitlab/reflections/relationships/crawler.rb @@ -185,11 +185,6 @@ def extract_relationship_values(relationship, record) # Return empty if we don't have all required values return {} if source_values.length != source_columns.length - # Check scope_conditions to see if this relationship applies to this record - if relationship['scope_conditions'].present? - return {} unless scope_conditions_match?(relationship['scope_conditions'], record) - end - # Pipeline has already handled polymorphic expansion, so target values # are directly mapped from source values target_values = source_values.dup @@ -202,31 +197,6 @@ def extract_relationship_values(relationship, record) } end - # Evaluates scope_conditions against a record to determine if the relationship applies - # scope_conditions is now an array of SQL condition strings like ["status = 'active'", "type = 'Issue'"] - def scope_conditions_match?(scope_conditions, record) - return true unless scope_conditions.is_a?(Array) - - scope_conditions.all? do |condition_sql| - evaluate_sql_condition(condition_sql, record) - end - end - - # Simple SQL condition evaluator for common patterns - # Handles basic equality conditions like "column = 'value'" - def evaluate_sql_condition(sql_condition, record) - # Match pattern: column = 'value' or column = "value" - if sql_condition.match(/^\s*(\w+)\s*=\s*['"]([^'"]+)['"]\s*$/) - column = $1 - expected_value = $2 - record[column] == expected_value - else - # For complex conditions we can't parse, assume they match - # In a real implementation, you might want to log this or use a proper SQL parser - true - end - end - def queue_relationship(relationship_chain:, relationship:, relationship_values:) target_table = relationship['target_table'] target_columns = relationship_values[:target_columns] @@ -237,6 +207,9 @@ def queue_relationship(relationship_chain:, relationship:, relationship_values:) # Build query conditions as key/value pairs query_conditions = target_columns.zip(target_values).to_h + # Add any additional conditions from the relationship (e.g., polymorphic type constraints) + query_conditions.merge!(relationship['conditions']) if relationship['conditions'] + # Build relationship step relationship_step = ChainStep.new( relationship: relationship, @@ -255,4 +228,4 @@ def queue_relationship(relationship_chain:, relationship:, relationship_values:) end end end -end \ No newline at end of file +end diff --git a/lib/gitlab/reflections/relationships/handlers/has_association_handler.rb b/lib/gitlab/reflections/relationships/handlers/has_association_handler.rb index de99f29402e5e9..02b933b8e9b0cd 100644 --- a/lib/gitlab/reflections/relationships/handlers/has_association_handler.rb +++ b/lib/gitlab/reflections/relationships/handlers/has_association_handler.rb @@ -13,7 +13,6 @@ def build_relationship target_column: determine_foreign_key, through_table: determine_through_table, through_target_key: determine_through_target_key, - scope_conditions: extract_scope_conditions, is_through_association: reflection.through_reflection? ) @@ -61,19 +60,6 @@ def determine_through_target_key reflection.source_reflection&.foreign_key end - - def extract_scope_conditions - return unless reflection.scope - - if reflection.options[:conditions] - reflection.options[:conditions].to_s - elsif reflection.scope.is_a?(Proc) - scope_source = reflection.scope.source_location - "# Scope defined at #{scope_source&.join(':')}" if scope_source - end - rescue StandardError - nil - end end end end diff --git a/lib/gitlab/reflections/relationships/handlers/polymorphic_has_association_handler.rb b/lib/gitlab/reflections/relationships/handlers/polymorphic_has_association_handler.rb index 8d8d3baef7f88c..bd692c063aa65e 100644 --- a/lib/gitlab/reflections/relationships/handlers/polymorphic_has_association_handler.rb +++ b/lib/gitlab/reflections/relationships/handlers/polymorphic_has_association_handler.rb @@ -13,7 +13,6 @@ def build_relationship target_column: determine_foreign_key, through_table: determine_through_table, through_target_key: determine_through_target_key, - scope_conditions: extract_scope_conditions, is_through_association: reflection.through_reflection? ) @@ -28,7 +27,6 @@ def build_relationship_with_target(target_table) target_column: determine_foreign_key, through_table: determine_through_table, through_target_key: determine_through_target_key, - scope_conditions: extract_scope_conditions, is_through_association: reflection.through_reflection? ) diff --git a/lib/gitlab/reflections/relationships/relationship.rb b/lib/gitlab/reflections/relationships/relationship.rb index 8b939b0f35f096..967aee2ca9d195 100644 --- a/lib/gitlab/reflections/relationships/relationship.rb +++ b/lib/gitlab/reflections/relationships/relationship.rb @@ -73,8 +73,8 @@ module Relationships # @attr has_fk_constraint [Boolean] Whether a foreign key constraint exists in the database # @attr fk_metadata [Hash] Additional metadata about the foreign key constraint # @attr data_sources [Array] List of data sources that contributed to discovering this relationship - # @attr scope_conditions [String, Hash] Additional conditions or scopes that apply to this relationship. - # Can be a SQL string (e.g., "status = 'active'") or a hash of column-value pairs. + # @attr conditions [Hash] Additional conditions that must be met for this relationship (e.g., polymorphic type constraints) + class Relationship include ActiveModel::Model include Gitlab::Reflections::Relationships::Predicates @@ -93,7 +93,8 @@ class Relationship :relationship_type, :forward_association, :reverse_association, :through_table, :through_target_key, :polymorphic, :polymorphic_type_column, :polymorphic_name, :has_ar_forward, :has_ar_reverse, - :has_fk_constraint, :fk_metadata, :data_sources, :scope_conditions, :is_through_association + :has_fk_constraint, :fk_metadata, :data_sources, :is_through_association, + :conditions validates :source_table, :foreign_key, presence: true validates :target_table, presence: true, unless: :polymorphic? @@ -112,7 +113,6 @@ def initialize(attributes = {}) attributes[:has_fk_constraint] = false unless attributes.key?(:has_fk_constraint) attributes[:is_through_association] = false unless attributes.key?(:is_through_association) attributes[:data_sources] = [] unless attributes.key?(:data_sources) - attributes[:scope_conditions] = nil unless attributes.key?(:scope_conditions) # Set default source_columns to ['id'] if not provided attributes[:source_columns] ||= ['id'] unless attributes.key?(:source_columns) @@ -175,8 +175,8 @@ def to_h has_fk_constraint: has_fk_constraint, fk_metadata: fk_metadata, data_sources: data_sources, - scope_conditions: scope_conditions, - is_through_association: is_through_association + is_through_association: is_through_association, + conditions: conditions }.compact end diff --git a/lib/gitlab/reflections/relationships/transformers/expand_polymorphic.rb b/lib/gitlab/reflections/relationships/transformers/expand_polymorphic.rb index be627550dad602..07542297a5d0c6 100644 --- a/lib/gitlab/reflections/relationships/transformers/expand_polymorphic.rb +++ b/lib/gitlab/reflections/relationships/transformers/expand_polymorphic.rb @@ -4,16 +4,17 @@ module Gitlab module Reflections module Relationships module Transformers - # Transforms polymorphic relationships into concrete has_many relationships with scope conditions. + # Transforms polymorphic relationships into concrete has_many relationships. # # This transformer converts a single polymorphic relationship like: # User.has_many :notes, as: :noteable # - # Into multiple concrete has_many relationships: + # Into multiple concrete has_many relationships with conditions: # User.has_many :issue_notes, -> { where(noteable_type: 'Issue') }, class_name: 'Note' # User.has_many :merge_request_notes, -> { where(noteable_type: 'MergeRequest') }, class_name: 'Note' # - # The polymorphic type constraint becomes a scope_condition on each concrete relationship. + # Each expanded relationship includes a 'conditions' attribute that specifies + # the polymorphic type constraint: { 'noteable_type' => 'Issue' } class ExpandPolymorphic def self.call(relationships) new(relationships).transform @@ -29,13 +30,13 @@ def transform @relationships.each do |rel| if polymorphic_relationship?(rel) if rel['target_table'] # Already expanded - expanded << normalize_columns(rel) + expanded << rel else # Convert polymorphic relationship into multiple has_many relationships expanded.concat(expand_into_has_many_relationships(rel)) end else - expanded << normalize_columns(rel) + expanded << rel end end @@ -49,7 +50,6 @@ def polymorphic_relationship?(rel) end # Converts a single polymorphic relationship into multiple concrete has_many relationships - # Each concrete relationship has a scope_condition that filters by the polymorphic type def expand_into_has_many_relationships(rel) target_tables = find_polymorphic_targets(rel) @@ -57,55 +57,36 @@ def expand_into_has_many_relationships(rel) type_column = rel['polymorphic_type_column'] || infer_type_column(rel) type_value = target_table.classify - # Create a concrete has_many relationship with scope condition + # Create a concrete has_many relationship with polymorphic condition concrete_relationship = rel.merge( 'target_table' => target_table, - 'relationship_type' => 'has_many', # Convert to has_many - 'polymorphic' => false, # No longer polymorphic + 'relationship_type' => 'one_to_many', # Convert to one_to_many + 'polymorphic' => false, # No longer polymorphic 'polymorphic_type_value' => type_value, 'polymorphic_type_column' => type_column, - 'scope_conditions' => build_scope_condition(rel['scope_conditions'], type_column, type_value) + 'conditions' => { type_column => type_value } ) - normalize_columns(concrete_relationship) + add_polymorphic_columns(concrete_relationship, type_column, type_value) end end - # Builds the scope condition for the concrete relationship - # Returns an array of SQL condition strings that can be easily combined - def build_scope_condition(existing_conditions, type_column, type_value) - conditions = [] + def add_polymorphic_columns(relationship, type_column, type_value) + # Ensure source_columns and target_columns are arrays (make copies to avoid mutation) + source_columns = Array(relationship['source_columns']).dup + target_columns = Array(relationship['target_columns']).dup - # Add existing conditions - conditions.concat(normalize_existing_conditions(existing_conditions)) if existing_conditions.present? + # Add the type column to source_columns and type_value to target_columns + type_column_index = source_columns.index(type_column) - # Add polymorphic type condition - conditions << "#{type_column} = '#{type_value}'" - - conditions - end - - # Normalizes existing conditions to the standard array of strings format - def normalize_existing_conditions(existing_conditions) - case existing_conditions - when Array - # Already in correct format (array of strings) - existing_conditions - when String - # Split SQL string on AND - existing_conditions.split(/\s+AND\s+/i).map(&:strip) - when Hash - # Convert hash to array of SQL strings - existing_conditions.map { |col, val| "#{col} = '#{val}'" } + if type_column_index + # Replace the corresponding target_columns value with the type_value + target_columns[type_column_index] = type_value else - [] + # Add type column/value to the column arrays if not already present + source_columns += [type_column] + target_columns += [type_value] end - end - - def normalize_columns(relationship) - # Ensure source_columns and target_columns are arrays - source_columns = Array(relationship['source_columns']) - target_columns = Array(relationship['target_columns']) relationship.merge( 'source_columns' => source_columns, diff --git a/spec/lib/gitlab/reflections/polymorphic_edge_cases_spec.rb b/spec/lib/gitlab/reflections/polymorphic_edge_cases_spec.rb index 380505a188ae05..aaeaa700babad9 100644 --- a/spec/lib/gitlab/reflections/polymorphic_edge_cases_spec.rb +++ b/spec/lib/gitlab/reflections/polymorphic_edge_cases_spec.rb @@ -123,24 +123,6 @@ end end - context 'with polymorphic scoped associations' do - it 'handles polymorphic associations with scopes' do - # Test polymorphic associations that have additional scopes/conditions - builder = Gitlab::Reflections::Relationships::Builder.new - relationships = builder.build_relationships - - # Find polymorphic edges with scope conditions - scoped_polymorphic_edges = relationships.select do |edge| - edge[:polymorphic] && edge[:scope_conditions] - end - - scoped_polymorphic_edges.each do |edge| - expect(edge[:scope_conditions]).to be_present - expect(edge[:polymorphic]).to be true - end - end - end - context 'with bidirectional polymorphic relationships' do it 'correctly merges polymorphic belongs_to with has_many as: relationships' do builder = Gitlab::Reflections::Relationships::Builder.new diff --git a/spec/lib/gitlab/reflections/relationships/handlers/base_handler_spec.rb b/spec/lib/gitlab/reflections/relationships/handlers/base_handler_spec.rb index 32ae7979950961..d1e5e33a620db4 100644 --- a/spec/lib/gitlab/reflections/relationships/handlers/base_handler_spec.rb +++ b/spec/lib/gitlab/reflections/relationships/handlers/base_handler_spec.rb @@ -20,4 +20,3 @@ end end end -end -- GitLab From 41b084b22247069c71e0e05bba17136a66ee4f18 Mon Sep 17 00:00:00 2001 From: Alex Pooley Date: Fri, 12 Sep 2025 11:41:39 +0800 Subject: [PATCH 11/19] wip --- .../transformers/expand_polymorphic.rb | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/lib/gitlab/reflections/relationships/transformers/expand_polymorphic.rb b/lib/gitlab/reflections/relationships/transformers/expand_polymorphic.rb index 07542297a5d0c6..b1455ae692913d 100644 --- a/lib/gitlab/reflections/relationships/transformers/expand_polymorphic.rb +++ b/lib/gitlab/reflections/relationships/transformers/expand_polymorphic.rb @@ -28,14 +28,11 @@ def transform expanded = [] @relationships.each do |rel| - if polymorphic_relationship?(rel) - if rel['target_table'] # Already expanded - expanded << rel - else - # Convert polymorphic relationship into multiple has_many relationships - expanded.concat(expand_into_has_many_relationships(rel)) - end + if polymorphic_relationship?(rel) && rel['target_table'].nil? + # Convert polymorphic relationship into multiple has_many relationships + expanded.concat(expand_into_has_many_relationships(rel)) else + # Pass through non-polymorphic relationships and concrete polymorphic relationships expanded << rel end end @@ -45,8 +42,10 @@ def transform private + # String-key version of the polymorphic_relationship? predicate + # Transformers work with string keys, while the main predicate uses symbol keys def polymorphic_relationship?(rel) - rel['polymorphic'] + rel['polymorphic'] == true end # Converts a single polymorphic relationship into multiple concrete has_many relationships -- GitLab From e4dbe04a9843ead0fdb3508dffd3779851fe35b4 Mon Sep 17 00:00:00 2001 From: Alex Pooley Date: Fri, 12 Sep 2025 11:45:22 +0800 Subject: [PATCH 12/19] wip --- .../transformers/expand_polymorphic_spec.rb | 136 ++++++++++++++++++ 1 file changed, 136 insertions(+) create mode 100644 spec/lib/gitlab/reflections/relationships/transformers/expand_polymorphic_spec.rb diff --git a/spec/lib/gitlab/reflections/relationships/transformers/expand_polymorphic_spec.rb b/spec/lib/gitlab/reflections/relationships/transformers/expand_polymorphic_spec.rb new file mode 100644 index 00000000000000..920b46c71a8ecb --- /dev/null +++ b/spec/lib/gitlab/reflections/relationships/transformers/expand_polymorphic_spec.rb @@ -0,0 +1,136 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Gitlab::Reflections::Relationships::Transformers::ExpandPolymorphic, feature_category: :database do + describe '.call' do + let(:polymorphic_relationship) do + { + 'source_table' => 'notes', + 'target_table' => nil, # Polymorphic, no specific target + 'foreign_key' => 'noteable_id', + 'foreign_key_table' => 'notes', + 'source_columns' => %w[noteable_id noteable_type], + 'target_columns' => ['id', nil], # Type value will be filled in + 'relationship_type' => 'belongs_to', + 'polymorphic' => true, + 'polymorphic_name' => 'noteable', + 'polymorphic_type_column' => 'noteable_type' + } + end + + let(:concrete_relationships) do + [ + { + 'source_table' => 'notes', + 'target_table' => 'issues', + 'foreign_key' => 'noteable_id', + 'polymorphic_name' => 'noteable', + 'polymorphic' => true + }, + { + 'source_table' => 'notes', + 'target_table' => 'merge_requests', + 'foreign_key' => 'noteable_id', + 'polymorphic_name' => 'noteable', + 'polymorphic' => true + } + ] + end + + let(:relationships) { [polymorphic_relationship] + concrete_relationships } + + subject { described_class.call(relationships) } + + it 'expands polymorphic relationships into concrete has_many relationships with conditions' do + result = subject + + # Should have 2 concrete relationships (issues and merge_requests) plus the 2 already concrete ones + expect(result.length).to eq(4) + + # Find the expanded relationships + issue_relationship = result.find { |r| r['target_table'] == 'issues' && r['conditions'] } + merge_request_relationship = result.find { |r| r['target_table'] == 'merge_requests' && r['conditions'] } + + expect(issue_relationship).to be_present + expect(merge_request_relationship).to be_present + + # Check issue relationship + expect(issue_relationship).to include( + 'source_table' => 'notes', + 'target_table' => 'issues', + 'relationship_type' => 'one_to_many', + 'polymorphic' => false, + 'polymorphic_type_column' => 'noteable_type', + 'polymorphic_type_value' => 'Issue', + 'conditions' => { 'noteable_type' => 'Issue' } + ) + + # Check merge request relationship + expect(merge_request_relationship).to include( + 'source_table' => 'notes', + 'target_table' => 'merge_requests', + 'relationship_type' => 'one_to_many', + 'polymorphic' => false, + 'polymorphic_type_column' => 'noteable_type', + 'polymorphic_type_value' => 'MergeRequest', + 'conditions' => { 'noteable_type' => 'MergeRequest' } + ) + + # Check that source_columns and target_columns include the type column/value + expect(issue_relationship['source_columns']).to include('noteable_type') + expect(issue_relationship['target_columns']).to include('Issue') + + expect(merge_request_relationship['source_columns']).to include('noteable_type') + expect(merge_request_relationship['target_columns']).to include('MergeRequest') + end + + it 'preserves non-polymorphic relationships unchanged' do + non_polymorphic = { + 'source_table' => 'users', + 'target_table' => 'projects', + 'foreign_key' => 'creator_id', + 'polymorphic' => false + } + + result = described_class.call([non_polymorphic]) + + expect(result.length).to eq(1) + expect(result.first).to include(non_polymorphic) + expect(result.first['conditions']).to be_nil + end + + it 'handles concrete polymorphic relationships (those with target_table)' do + concrete_polymorphic = { + 'source_table' => 'notes', + 'target_table' => 'issues', # Already has target_table - this is a concrete polymorphic relationship + 'polymorphic' => true, + 'polymorphic_name' => 'noteable' + } + + result = described_class.call([concrete_polymorphic]) + + expect(result.length).to eq(1) + expect(result.first).to include(concrete_polymorphic) + # Should not add conditions to concrete polymorphic relationships + expect(result.first['conditions']).to be_nil + end + + context 'when polymorphic_type_column is not provided' do + let(:polymorphic_relationship_without_type_column) do + polymorphic_relationship.except('polymorphic_type_column') + end + + let(:relationships) { [polymorphic_relationship_without_type_column] + concrete_relationships } + + it 'infers the type column from the foreign key' do + result = subject + + issue_relationship = result.find { |r| r['target_table'] == 'issues' && r['conditions'] } + + expect(issue_relationship['polymorphic_type_column']).to eq('noteable_type') + expect(issue_relationship['conditions']).to eq({ 'noteable_type' => 'Issue' }) + end + end + end +end -- GitLab From bf8af9975b3a9d4c5cb0e65c3e485240aa42c88e Mon Sep 17 00:00:00 2001 From: Alex Pooley Date: Fri, 12 Sep 2025 11:47:32 +0800 Subject: [PATCH 13/19] wip --- .../relationships/predicates/polymorphic.rb | 24 ------------------- lib/tasks/database_relationships.rake | 11 +++++++-- 2 files changed, 9 insertions(+), 26 deletions(-) delete mode 100644 lib/gitlab/reflections/relationships/predicates/polymorphic.rb diff --git a/lib/gitlab/reflections/relationships/predicates/polymorphic.rb b/lib/gitlab/reflections/relationships/predicates/polymorphic.rb deleted file mode 100644 index a5caa6b916eb54..00000000000000 --- a/lib/gitlab/reflections/relationships/predicates/polymorphic.rb +++ /dev/null @@ -1,24 +0,0 @@ -# frozen_string_literal: true - -module Gitlab - module Reflections - module Relationships - module Predicates - # Predicates specific to polymorphic relationships - module Polymorphic - # Key generation functions for grouping - this provides meaningful abstraction - # for complex grouping logic that would be cumbersome to inline - def polymorphic_key(relationship) - return unless polymorphic_relationship?(relationship) - - [ - relationship[:polymorphic_type_column], - relationship[:foreign_key], - relationship[:foreign_key_table] || relationship[:target_table] - ] - end - end - end - end - end -end diff --git a/lib/tasks/database_relationships.rake b/lib/tasks/database_relationships.rake index 5aee890a73afe1..b5a87b44c77f9d 100644 --- a/lib/tasks/database_relationships.rake +++ b/lib/tasks/database_relationships.rake @@ -116,7 +116,6 @@ namespace :db do desc "Print polymorphic relationships using predicates in JSON format" task polymorphic_json: :environment do # Include the predicate modules for easy access - include Gitlab::Reflections::Relationships::Predicates::Polymorphic include Gitlab::Reflections::Relationships::Predicates::TypeBased puts "Building relationships..." @@ -128,7 +127,15 @@ namespace :db do polymorphic_rels = relationships.select { |rel| polymorphic_relationship?(rel) } # Group by polymorphic key and aggregate - grouped = polymorphic_rels.group_by { |rel| polymorphic_key(rel) } + grouped = polymorphic_rels.group_by do |rel| + next unless polymorphic_relationship?(rel) + + [ + rel[:polymorphic_type_column], + rel[:foreign_key], + rel[:foreign_key_table] || rel[:target_table] + ] + end puts "Aggregating #{grouped.size} polymorphic relationship groups..." aggregated_polymorphic = grouped.map do |key, group| -- GitLab From 3a3625bd8b9bc5dd572ad94d1997eddf449d14f4 Mon Sep 17 00:00:00 2001 From: Alex Pooley Date: Fri, 12 Sep 2025 11:58:02 +0800 Subject: [PATCH 14/19] wip --- .../reflections/relationships/predicates.rb | 13 ------------- .../relationships/predicates/type_based.rb | 18 ------------------ .../reflections/relationships/relationship.rb | 3 +-- .../transformers/expand_polymorphic.rb | 8 +------- lib/tasks/database_relationships.rake | 7 ++----- 5 files changed, 4 insertions(+), 45 deletions(-) delete mode 100644 lib/gitlab/reflections/relationships/predicates.rb delete mode 100644 lib/gitlab/reflections/relationships/predicates/type_based.rb diff --git a/lib/gitlab/reflections/relationships/predicates.rb b/lib/gitlab/reflections/relationships/predicates.rb deleted file mode 100644 index 708145ab0bda4a..00000000000000 --- a/lib/gitlab/reflections/relationships/predicates.rb +++ /dev/null @@ -1,13 +0,0 @@ -# frozen_string_literal: true - -module Gitlab - module Reflections - module Relationships - # Business logic for relationship identification and classification - module Predicates - include TypeBased - include Polymorphic - end - end - end -end diff --git a/lib/gitlab/reflections/relationships/predicates/type_based.rb b/lib/gitlab/reflections/relationships/predicates/type_based.rb deleted file mode 100644 index 8d80c93161ec06..00000000000000 --- a/lib/gitlab/reflections/relationships/predicates/type_based.rb +++ /dev/null @@ -1,18 +0,0 @@ -# frozen_string_literal: true - -module Gitlab - module Reflections - module Relationships - module Predicates - # Predicates for determining relationship types and characteristics - module TypeBased - # Only keep the polymorphic_relationship? method as it's actually used - # and provides a meaningful abstraction for the boolean check - def polymorphic_relationship?(relationship) - relationship[:polymorphic] == true - end - end - end - end - end -end diff --git a/lib/gitlab/reflections/relationships/relationship.rb b/lib/gitlab/reflections/relationships/relationship.rb index 967aee2ca9d195..24c3718188e808 100644 --- a/lib/gitlab/reflections/relationships/relationship.rb +++ b/lib/gitlab/reflections/relationships/relationship.rb @@ -77,7 +77,6 @@ module Relationships class Relationship include ActiveModel::Model - include Gitlab::Reflections::Relationships::Predicates VALID_RELATIONSHIP_TYPES = %w[ one_to_one one_to_many many_to_one many_to_many @@ -149,7 +148,7 @@ def polymorphic_type_column_name polymorphic_type_column end - # Hash-style attribute access for compatibility with Predicates + # Hash-style attribute access def [](attr_name) public_send(attr_name) end diff --git a/lib/gitlab/reflections/relationships/transformers/expand_polymorphic.rb b/lib/gitlab/reflections/relationships/transformers/expand_polymorphic.rb index b1455ae692913d..b5389ba39a0872 100644 --- a/lib/gitlab/reflections/relationships/transformers/expand_polymorphic.rb +++ b/lib/gitlab/reflections/relationships/transformers/expand_polymorphic.rb @@ -28,7 +28,7 @@ def transform expanded = [] @relationships.each do |rel| - if polymorphic_relationship?(rel) && rel['target_table'].nil? + if rel['polymorphic'] == true && rel['target_table'].nil? # Convert polymorphic relationship into multiple has_many relationships expanded.concat(expand_into_has_many_relationships(rel)) else @@ -42,12 +42,6 @@ def transform private - # String-key version of the polymorphic_relationship? predicate - # Transformers work with string keys, while the main predicate uses symbol keys - def polymorphic_relationship?(rel) - rel['polymorphic'] == true - end - # Converts a single polymorphic relationship into multiple concrete has_many relationships def expand_into_has_many_relationships(rel) target_tables = find_polymorphic_targets(rel) diff --git a/lib/tasks/database_relationships.rake b/lib/tasks/database_relationships.rake index b5a87b44c77f9d..3b2503b09eed5d 100644 --- a/lib/tasks/database_relationships.rake +++ b/lib/tasks/database_relationships.rake @@ -115,20 +115,17 @@ namespace :db do desc "Print polymorphic relationships using predicates in JSON format" task polymorphic_json: :environment do - # Include the predicate modules for easy access - include Gitlab::Reflections::Relationships::Predicates::TypeBased - puts "Building relationships..." builder = Gitlab::Reflections::Relationships::Builder.new relationships = builder.build_relationships puts "Filtering polymorphic relationships using predicates..." # Work directly with Array - no wrapper needed - polymorphic_rels = relationships.select { |rel| polymorphic_relationship?(rel) } + polymorphic_rels = relationships.select { |rel| rel[:polymorphic] == true } # Group by polymorphic key and aggregate grouped = polymorphic_rels.group_by do |rel| - next unless polymorphic_relationship?(rel) + next unless rel[:polymorphic] == true [ rel[:polymorphic_type_column], -- GitLab From a57eb84ada2d790772acd5869d9c0962fd6cf752 Mon Sep 17 00:00:00 2001 From: Alex Pooley Date: Fri, 12 Sep 2025 13:11:06 +0800 Subject: [PATCH 15/19] wip --- .../reflections/relationships/crawl_logger.rb | 4 +++- .../reflections/relationships/foreign_key.rb | 4 ++-- .../handlers/belongs_to_handler.rb | 3 ++- .../handlers/has_association_handler.rb | 4 ++-- .../polymorphic_has_association_handler.rb | 8 +++---- .../relationships/transformers/deduplicate.rb | 6 ++--- .../transformers/merge_data_sources.rb | 9 ++++++-- .../transformers/normalize_columns.rb | 22 +++++++++++-------- 8 files changed, 36 insertions(+), 24 deletions(-) diff --git a/lib/gitlab/reflections/relationships/crawl_logger.rb b/lib/gitlab/reflections/relationships/crawl_logger.rb index d75c7dbaf5f8d2..0f33a596ddde5b 100644 --- a/lib/gitlab/reflections/relationships/crawl_logger.rb +++ b/lib/gitlab/reflections/relationships/crawl_logger.rb @@ -93,7 +93,9 @@ def print_relationship_def(relationship_def) puts " Derived from:" if source_info['has_fk_constraint'] - puts " - Foreign key constraint: #{relationship_def['source_table']}.#{relationship_def['source_column']} -> #{relationship_def['target_table']}.#{relationship_def['target_column']}" + source_columns = Array(relationship_def['source_columns']).join(', ') + target_columns = Array(relationship_def['target_columns']).join(', ') + puts " - Foreign key constraint: #{relationship_def['source_table']}.[#{source_columns}] -> #{relationship_def['target_table']}.[#{target_columns}]" end if source_info['forward_association'] diff --git a/lib/gitlab/reflections/relationships/foreign_key.rb b/lib/gitlab/reflections/relationships/foreign_key.rb index 9972f54000670a..587d7c044d4bfb 100644 --- a/lib/gitlab/reflections/relationships/foreign_key.rb +++ b/lib/gitlab/reflections/relationships/foreign_key.rb @@ -43,8 +43,8 @@ def build_foreign_key_relationship(table_name, foreign_key) relationship_type: 'one_to_many', foreign_key: foreign_key.column, foreign_key_table: table_name, - source_column: foreign_key.primary_key || 'id', - target_column: foreign_key.column, + source_columns: [foreign_key.primary_key || 'id'], + target_columns: [foreign_key.column], has_fk_constraint: true, fk_metadata: { constraint_name: foreign_key.name, diff --git a/lib/gitlab/reflections/relationships/handlers/belongs_to_handler.rb b/lib/gitlab/reflections/relationships/handlers/belongs_to_handler.rb index 96b966f816ea2a..f8fd41b321c5ce 100644 --- a/lib/gitlab/reflections/relationships/handlers/belongs_to_handler.rb +++ b/lib/gitlab/reflections/relationships/handlers/belongs_to_handler.rb @@ -26,7 +26,8 @@ def base_attributes { source_table: determine_target_table, # The referenced table (e.g., namespaces) target_table: model.table_name, # The referencing table (e.g., projects) - source_column: reflection.association_primary_key || 'id', + source_columns: [reflection.association_primary_key || 'id'], + target_columns: [reflection.foreign_key], relationship_type: 'many_to_one', reverse_association: { name: association_name.to_s, diff --git a/lib/gitlab/reflections/relationships/handlers/has_association_handler.rb b/lib/gitlab/reflections/relationships/handlers/has_association_handler.rb index 02b933b8e9b0cd..1d10695c60bcbc 100644 --- a/lib/gitlab/reflections/relationships/handlers/has_association_handler.rb +++ b/lib/gitlab/reflections/relationships/handlers/has_association_handler.rb @@ -9,8 +9,8 @@ def build_relationship attributes = base_attributes.merge( foreign_key: determine_foreign_key, foreign_key_table: determine_foreign_key_table, - source_column: (reflection.options[:primary_key] || reflection.association_primary_key || 'id').to_s, - target_column: determine_foreign_key, + source_columns: [(reflection.options[:primary_key] || reflection.association_primary_key || 'id').to_s], + target_columns: [determine_foreign_key], through_table: determine_through_table, through_target_key: determine_through_target_key, is_through_association: reflection.through_reflection? diff --git a/lib/gitlab/reflections/relationships/handlers/polymorphic_has_association_handler.rb b/lib/gitlab/reflections/relationships/handlers/polymorphic_has_association_handler.rb index bd692c063aa65e..ceb3a751fccfdc 100644 --- a/lib/gitlab/reflections/relationships/handlers/polymorphic_has_association_handler.rb +++ b/lib/gitlab/reflections/relationships/handlers/polymorphic_has_association_handler.rb @@ -9,8 +9,8 @@ def build_relationship attributes = base_polymorphic_attributes.merge( foreign_key: determine_foreign_key, foreign_key_table: determine_foreign_key_table, - source_column: reflection.association_primary_key || 'id', - target_column: determine_foreign_key, + source_columns: [reflection.association_primary_key || 'id'], + target_columns: [determine_foreign_key], through_table: determine_through_table, through_target_key: determine_through_target_key, is_through_association: reflection.through_reflection? @@ -23,8 +23,8 @@ def build_relationship_with_target(target_table) attributes = base_attributes_with_target(target_table).merge( foreign_key: determine_foreign_key, foreign_key_table: determine_foreign_key_table, - source_column: reflection.association_primary_key || 'id', - target_column: determine_foreign_key, + source_columns: [reflection.association_primary_key || 'id'], + target_columns: [determine_foreign_key], through_table: determine_through_table, through_target_key: determine_through_target_key, is_through_association: reflection.through_reflection? diff --git a/lib/gitlab/reflections/relationships/transformers/deduplicate.rb b/lib/gitlab/reflections/relationships/transformers/deduplicate.rb index f1fc43955984b7..ecd6052c97c2ff 100644 --- a/lib/gitlab/reflections/relationships/transformers/deduplicate.rb +++ b/lib/gitlab/reflections/relationships/transformers/deduplicate.rb @@ -33,8 +33,8 @@ def relationship_signature(rel) [ rel['source_table'], rel['target_table'], - rel['source_column'], - rel['target_column'], + Array(rel['source_columns']).sort.join(','), + Array(rel['target_columns']).sort.join(','), rel['relationship_type'], rel['polymorphic_type_value'] ].compact.join('|') @@ -43,4 +43,4 @@ def relationship_signature(rel) end end end -end \ No newline at end of file +end diff --git a/lib/gitlab/reflections/relationships/transformers/merge_data_sources.rb b/lib/gitlab/reflections/relationships/transformers/merge_data_sources.rb index 5ac6a18c6d4ee4..611f08985d0937 100644 --- a/lib/gitlab/reflections/relationships/transformers/merge_data_sources.rb +++ b/lib/gitlab/reflections/relationships/transformers/merge_data_sources.rb @@ -21,7 +21,12 @@ def transform private def key(rel) - [rel['source_table'], rel['target_table'], rel['source_column'], rel['target_column']] + [ + rel['source_table'], + rel['target_table'], + Array(rel['source_columns']).sort, + Array(rel['target_columns']).sort + ] end def merge_group(group) @@ -45,4 +50,4 @@ def merge_data(base, other) end end end -end \ No newline at end of file +end diff --git a/lib/gitlab/reflections/relationships/transformers/normalize_columns.rb b/lib/gitlab/reflections/relationships/transformers/normalize_columns.rb index 00ff3bdc5ff22b..dee04d356ca3e4 100644 --- a/lib/gitlab/reflections/relationships/transformers/normalize_columns.rb +++ b/lib/gitlab/reflections/relationships/transformers/normalize_columns.rb @@ -21,24 +21,28 @@ def transform def normalize_relationship(rel) normalized = rel.dup - - # Set default source column if missing - normalized['source_column'] ||= 'id' - - # Set target column from foreign key if missing - if normalized['foreign_key'] && !normalized['target_column'] - normalized['target_column'] = normalized['foreign_key'] + + # Set default source columns if missing + normalized['source_columns'] ||= ['id'] + + # Set target columns from foreign key if missing + if normalized['foreign_key'] && !normalized['target_columns'] + normalized['target_columns'] = [normalized['foreign_key']] end - + # Ensure foreign_key_table is set if normalized['foreign_key'] && !normalized['foreign_key_table'] normalized['foreign_key_table'] = normalized['target_table'] end + # Ensure columns are arrays + normalized['source_columns'] = Array(normalized['source_columns']) + normalized['target_columns'] = Array(normalized['target_columns']) + normalized end end end end end -end \ No newline at end of file +end -- GitLab From e2a888298e5e9844fc48a8ea4842cc2b19e9f158 Mon Sep 17 00:00:00 2001 From: Alex Pooley Date: Fri, 12 Sep 2025 15:48:02 +0800 Subject: [PATCH 16/19] wip --- .../reflections/relationships/crawl_logger.rb | 24 +++++++------- .../reflections/relationships/crawler.rb | 20 ++++++------ .../relationships/transformers/deduplicate.rb | 12 +++---- .../transformers/expand_polymorphic.rb | 32 +++++++++---------- .../transformers/filter_tables.rb | 12 ++++--- .../transformers/invert_relationships.rb | 28 ++++++++-------- .../transformers/merge_data_sources.rb | 22 ++++++------- .../transformers/normalize_columns.rb | 14 ++++---- .../relationships/transformers/validate.rb | 14 ++++---- 9 files changed, 89 insertions(+), 89 deletions(-) diff --git a/lib/gitlab/reflections/relationships/crawl_logger.rb b/lib/gitlab/reflections/relationships/crawl_logger.rb index 0f33a596ddde5b..d2cde21fdaa50a 100644 --- a/lib/gitlab/reflections/relationships/crawl_logger.rb +++ b/lib/gitlab/reflections/relationships/crawl_logger.rb @@ -87,26 +87,26 @@ def print_relationship_def(relationship_def) puts " Input relationship definition:" puts " #{Gitlab::Json.pretty_generate(relationship_def).gsub(/\n/, "\n ")}" - return unless relationship_def&.dig('source_info') + return unless relationship_def&.dig(:source_info) - source_info = relationship_def['source_info'] + source_info = relationship_def[:source_info] puts " Derived from:" - if source_info['has_fk_constraint'] - source_columns = Array(relationship_def['source_columns']).join(', ') - target_columns = Array(relationship_def['target_columns']).join(', ') - puts " - Foreign key constraint: #{relationship_def['source_table']}.[#{source_columns}] -> #{relationship_def['target_table']}.[#{target_columns}]" + if source_info[:has_fk_constraint] + source_columns = Array(relationship_def[:source_columns]).join(', ') + target_columns = Array(relationship_def[:target_columns]).join(', ') + puts " - Foreign key constraint: #{relationship_def[:source_table]}.[#{source_columns}] -> #{relationship_def[:target_table]}.[#{target_columns}]" end - if source_info['forward_association'] - assoc = source_info['forward_association'] - puts " - ActiveRecord association: #{assoc['name']} (#{assoc['type']}) in #{assoc['class_name'] || relationship_def['source_table'].classify}" + if source_info[:forward_association] + assoc = source_info[:forward_association] + puts " - ActiveRecord association: #{assoc[:name]} (#{assoc[:type]}) in #{assoc[:class_name] || relationship_def[:source_table].classify}" end - return unless source_info['reverse_association'] + return unless source_info[:reverse_association] - assoc = source_info['reverse_association'] - puts " - Reverse association: #{assoc['name']} (#{assoc['type']}) in #{assoc['class_name'] || relationship_def['target_table']&.classify}" + assoc = source_info[:reverse_association] + puts " - Reverse association: #{assoc[:name]} (#{assoc[:type]}) in #{assoc[:class_name] || relationship_def[:target_table]&.classify}" end end end diff --git a/lib/gitlab/reflections/relationships/crawler.rb b/lib/gitlab/reflections/relationships/crawler.rb index f6dc698b44e39f..c111a499b47cac 100644 --- a/lib/gitlab/reflections/relationships/crawler.rb +++ b/lib/gitlab/reflections/relationships/crawler.rb @@ -22,17 +22,17 @@ def to_h keyword_init: true ) do def to_s - source_table = relationship['source_table'] - target_table = relationship['target_table'] + source_table = relationship[:source_table] + target_table = relationship[:target_table] # Build source part with column=value pairs - source_columns = Array(relationship['source_columns']) + source_columns = Array(relationship[:source_columns]) source_parts = source_columns.zip(source_values).filter_map do |col, val| "#{col}=#{val}" if col && val end # Build target part with column=value pairs - target_columns = Array(relationship['target_columns']) + target_columns = Array(relationship[:target_columns]) target_parts = target_columns.zip(target_values).filter_map do |col, val| "#{col}=#{val}" if col && val end @@ -160,8 +160,8 @@ def queue_related_tables_from_record(source_table, record, relationship_chain) return unless record_id # Process all normalized relationships in a single loop - @normalized_relationships.select { |rel| rel['source_table'] == source_table }.each do |rel| - next if should_ignore_table?(rel['target_table']) + @normalized_relationships.select { |rel| rel[:source_table] == source_table }.each do |rel| + next if should_ignore_table?(rel[:target_table]) # Extract values for this relationship relationship_values = extract_relationship_values(rel, record) @@ -176,8 +176,8 @@ def queue_related_tables_from_record(source_table, record, relationship_chain) end def extract_relationship_values(relationship, record) - source_columns = Array(relationship['source_columns']) - target_columns = Array(relationship['target_columns']) + source_columns = Array(relationship[:source_columns]) + target_columns = Array(relationship[:target_columns]) # Extract source values from the record source_values = source_columns.filter_map { |col| record[col] if record[col] } @@ -198,7 +198,7 @@ def extract_relationship_values(relationship, record) end def queue_relationship(relationship_chain:, relationship:, relationship_values:) - target_table = relationship['target_table'] + target_table = relationship[:target_table] target_columns = relationship_values[:target_columns] target_values = relationship_values[:target_values] @@ -208,7 +208,7 @@ def queue_relationship(relationship_chain:, relationship:, relationship_values:) query_conditions = target_columns.zip(target_values).to_h # Add any additional conditions from the relationship (e.g., polymorphic type constraints) - query_conditions.merge!(relationship['conditions']) if relationship['conditions'] + query_conditions.merge!(relationship[:conditions]) if relationship[:conditions] # Build relationship step relationship_step = ChainStep.new( diff --git a/lib/gitlab/reflections/relationships/transformers/deduplicate.rb b/lib/gitlab/reflections/relationships/transformers/deduplicate.rb index ecd6052c97c2ff..8248030ecf392d 100644 --- a/lib/gitlab/reflections/relationships/transformers/deduplicate.rb +++ b/lib/gitlab/reflections/relationships/transformers/deduplicate.rb @@ -31,12 +31,12 @@ def transform def relationship_signature(rel) # Create a signature that uniquely identifies a relationship [ - rel['source_table'], - rel['target_table'], - Array(rel['source_columns']).sort.join(','), - Array(rel['target_columns']).sort.join(','), - rel['relationship_type'], - rel['polymorphic_type_value'] + rel[:source_table], + rel[:target_table], + Array(rel[:source_columns]).sort.join(','), + Array(rel[:target_columns]).sort.join(','), + rel[:relationship_type], + rel[:polymorphic_type_value] ].compact.join('|') end end diff --git a/lib/gitlab/reflections/relationships/transformers/expand_polymorphic.rb b/lib/gitlab/reflections/relationships/transformers/expand_polymorphic.rb index b5389ba39a0872..82da1a65bf143f 100644 --- a/lib/gitlab/reflections/relationships/transformers/expand_polymorphic.rb +++ b/lib/gitlab/reflections/relationships/transformers/expand_polymorphic.rb @@ -28,7 +28,7 @@ def transform expanded = [] @relationships.each do |rel| - if rel['polymorphic'] == true && rel['target_table'].nil? + if rel[:polymorphic] == true && rel[:target_table].nil? # Convert polymorphic relationship into multiple has_many relationships expanded.concat(expand_into_has_many_relationships(rel)) else @@ -47,17 +47,17 @@ def expand_into_has_many_relationships(rel) target_tables = find_polymorphic_targets(rel) target_tables.map do |target_table| - type_column = rel['polymorphic_type_column'] || infer_type_column(rel) + type_column = rel[:polymorphic_type_column] || infer_type_column(rel) type_value = target_table.classify # Create a concrete has_many relationship with polymorphic condition concrete_relationship = rel.merge( - 'target_table' => target_table, - 'relationship_type' => 'one_to_many', # Convert to one_to_many - 'polymorphic' => false, # No longer polymorphic - 'polymorphic_type_value' => type_value, - 'polymorphic_type_column' => type_column, - 'conditions' => { type_column => type_value } + target_table: target_table, + relationship_type: 'one_to_many', # Convert to one_to_many + polymorphic: false, # No longer polymorphic + polymorphic_type_value: type_value, + polymorphic_type_column: type_column, + conditions: { type_column => type_value } ) add_polymorphic_columns(concrete_relationship, type_column, type_value) @@ -66,8 +66,8 @@ def expand_into_has_many_relationships(rel) def add_polymorphic_columns(relationship, type_column, type_value) # Ensure source_columns and target_columns are arrays (make copies to avoid mutation) - source_columns = Array(relationship['source_columns']).dup - target_columns = Array(relationship['target_columns']).dup + source_columns = Array(relationship[:source_columns]).dup + target_columns = Array(relationship[:target_columns]).dup # Add the type column to source_columns and type_value to target_columns type_column_index = source_columns.index(type_column) @@ -82,19 +82,19 @@ def add_polymorphic_columns(relationship, type_column, type_value) end relationship.merge( - 'source_columns' => source_columns, - 'target_columns' => target_columns + source_columns: source_columns, + target_columns: target_columns ) end def find_polymorphic_targets(rel) # Look through all relationships to find ones that target this polymorphic - polymorphic_name = rel['polymorphic_name'] + polymorphic_name = rel[:polymorphic_name] targets = Set.new @relationships.each do |other_rel| - if other_rel['polymorphic_name'] == polymorphic_name && other_rel['target_table'] - targets << other_rel['target_table'] + if other_rel[:polymorphic_name] == polymorphic_name && other_rel[:target_table] + targets << other_rel[:target_table] end end @@ -104,7 +104,7 @@ def find_polymorphic_targets(rel) def infer_type_column(rel) # Infer the type column name from the polymorphic association # Standard Rails convention: if foreign key is 'commentable_id', type column is 'commentable_type' - foreign_key = rel['foreign_key'] + foreign_key = rel[:foreign_key] return unless foreign_key if foreign_key.end_with?('_id') diff --git a/lib/gitlab/reflections/relationships/transformers/filter_tables.rb b/lib/gitlab/reflections/relationships/transformers/filter_tables.rb index da08760135f1e3..6f8fedc51250de 100644 --- a/lib/gitlab/reflections/relationships/transformers/filter_tables.rb +++ b/lib/gitlab/reflections/relationships/transformers/filter_tables.rb @@ -22,25 +22,27 @@ def transform private def should_filter?(rel) - return true if table_excluded?(rel['source_table']) - return true if table_excluded?(rel['target_table']) - return true if @included_tables && !table_included?(rel['source_table']) - return true if @included_tables && !table_included?(rel['target_table']) + return true if table_excluded?(rel[:source_table]) + return true if table_excluded?(rel[:target_table]) + return true if @included_tables && !table_included?(rel[:source_table]) + return true if @included_tables && !table_included?(rel[:target_table]) false end def table_excluded?(table) return false unless table + @excluded_tables.include?(table) end def table_included?(table) return true unless @included_tables && table + @included_tables.include?(table) end end end end end -end \ No newline at end of file +end diff --git a/lib/gitlab/reflections/relationships/transformers/invert_relationships.rb b/lib/gitlab/reflections/relationships/transformers/invert_relationships.rb index b8eee7f404f934..c01fbe8f486022 100644 --- a/lib/gitlab/reflections/relationships/transformers/invert_relationships.rb +++ b/lib/gitlab/reflections/relationships/transformers/invert_relationships.rb @@ -31,21 +31,21 @@ def transform def should_skip_inversion?(rel) # Don't invert polymorphic base relationships (without target_table) # Don't invert if it's already an inverted relationship - (rel['polymorphic'] && !rel['target_table']) || - rel['inverted'] == true + (rel[:polymorphic] && !rel[:target_table]) || + rel[:inverted] == true end def create_inverse_relationship(rel) - return unless rel['source_table'] && rel['target_table'] + return unless rel[:source_table] && rel[:target_table] { - 'source_table' => rel['target_table'], - 'target_table' => rel['source_table'], - 'source_columns' => rel['target_columns'], - 'target_columns' => rel['source_columns'], - 'relationship_type' => invert_relationship_type(rel['relationship_type']), - 'inverted' => true, - 'original_relationship' => rel.slice('source_table', 'target_table', 'relationship_type') + source_table: rel[:target_table], + target_table: rel[:source_table], + source_columns: rel[:target_columns], + target_columns: rel[:source_columns], + relationship_type: invert_relationship_type(rel[:relationship_type]), + inverted: true, + original_relationship: rel.slice(:source_table, :target_table, :relationship_type) }.merge(preserve_polymorphic_info(rel)) end @@ -59,11 +59,11 @@ def invert_relationship_type(type) end def preserve_polymorphic_info(rel) - if rel['polymorphic'] + if rel[:polymorphic] { - 'polymorphic' => true, - 'polymorphic_type_column' => rel['polymorphic_type_column'], - 'polymorphic_type_value' => rel['polymorphic_type_value'] + polymorphic: true, + polymorphic_type_column: rel[:polymorphic_type_column], + polymorphic_type_value: rel[:polymorphic_type_value] } else {} diff --git a/lib/gitlab/reflections/relationships/transformers/merge_data_sources.rb b/lib/gitlab/reflections/relationships/transformers/merge_data_sources.rb index 611f08985d0937..9478cf50b26ff5 100644 --- a/lib/gitlab/reflections/relationships/transformers/merge_data_sources.rb +++ b/lib/gitlab/reflections/relationships/transformers/merge_data_sources.rb @@ -22,10 +22,10 @@ def transform def key(rel) [ - rel['source_table'], - rel['target_table'], - Array(rel['source_columns']).sort, - Array(rel['target_columns']).sort + rel[:source_table], + rel[:target_table], + Array(rel[:source_columns]).sort, + Array(rel[:target_columns]).sort ] end @@ -38,13 +38,13 @@ def merge_group(group) end def merge_data(base, other) - base['data_sources'] = Array(base['data_sources']) | Array(other['data_sources']) - base['has_ar_forward'] ||= other['has_ar_forward'] - base['has_ar_reverse'] ||= other['has_ar_reverse'] - base['has_fk_constraint'] ||= other['has_fk_constraint'] - base['forward_association'] ||= other['forward_association'] - base['reverse_association'] ||= other['reverse_association'] - base['fk_metadata'] ||= other['fk_metadata'] + base[:data_sources] = Array(base[:data_sources]) | Array(other[:data_sources]) + base[:has_ar_forward] ||= other[:has_ar_forward] + base[:has_ar_reverse] ||= other[:has_ar_reverse] + base[:has_fk_constraint] ||= other[:has_fk_constraint] + base[:forward_association] ||= other[:forward_association] + base[:reverse_association] ||= other[:reverse_association] + base[:fk_metadata] ||= other[:fk_metadata] end end end diff --git a/lib/gitlab/reflections/relationships/transformers/normalize_columns.rb b/lib/gitlab/reflections/relationships/transformers/normalize_columns.rb index dee04d356ca3e4..cafba872cc9ef9 100644 --- a/lib/gitlab/reflections/relationships/transformers/normalize_columns.rb +++ b/lib/gitlab/reflections/relationships/transformers/normalize_columns.rb @@ -23,21 +23,21 @@ def normalize_relationship(rel) normalized = rel.dup # Set default source columns if missing - normalized['source_columns'] ||= ['id'] + normalized[:source_columns] ||= ['id'] # Set target columns from foreign key if missing - if normalized['foreign_key'] && !normalized['target_columns'] - normalized['target_columns'] = [normalized['foreign_key']] + if normalized[:foreign_key] && !normalized[:target_columns] + normalized[:target_columns] = [normalized[:foreign_key]] end # Ensure foreign_key_table is set - if normalized['foreign_key'] && !normalized['foreign_key_table'] - normalized['foreign_key_table'] = normalized['target_table'] + if normalized[:foreign_key] && !normalized[:foreign_key_table] + normalized[:foreign_key_table] = normalized[:target_table] end # Ensure columns are arrays - normalized['source_columns'] = Array(normalized['source_columns']) - normalized['target_columns'] = Array(normalized['target_columns']) + normalized[:source_columns] = Array(normalized[:source_columns]) + normalized[:target_columns] = Array(normalized[:target_columns]) normalized end diff --git a/lib/gitlab/reflections/relationships/transformers/validate.rb b/lib/gitlab/reflections/relationships/transformers/validate.rb index 568e4b1c7a91a6..481b8ac95d07f5 100644 --- a/lib/gitlab/reflections/relationships/transformers/validate.rb +++ b/lib/gitlab/reflections/relationships/transformers/validate.rb @@ -23,20 +23,18 @@ def transform def validate_relationship(rel) # Check required fields - required_fields = ['source_table', 'target_table'] + required_fields = [:source_table, :target_table] missing_fields = required_fields.select { |field| rel[field].nil? || rel[field].to_s.empty? } if missing_fields.any? Rails.logger.debug "Skipping invalid relationship: missing required fields #{missing_fields}" - return nil + return end # Additional validation for polymorphic relationships - if rel['polymorphic'] && rel['relationship_type'] == 'polymorphic_expanded' - unless rel['polymorphic_type_column'] && rel['polymorphic_type_value'] - Rails.logger.debug "Skipping invalid polymorphic relationship: missing type information" - return nil - end + if rel[:polymorphic] && rel[:relationship_type] == 'polymorphic_expanded' && !(rel[:polymorphic_type_column] && rel[:polymorphic_type_value]) + Rails.logger.debug "Skipping invalid polymorphic relationship: missing type information" + return end # Return clean relationship hash @@ -49,4 +47,4 @@ def validate_relationship(rel) end end end -end \ No newline at end of file +end -- GitLab From 084b489a3415aa3fa2f545f6ad72fdf343e62262 Mon Sep 17 00:00:00 2001 From: Alex Pooley Date: Fri, 12 Sep 2025 15:50:59 +0800 Subject: [PATCH 17/19] wip --- .../transformers/merge_data_sources_spec.rb | 202 ++++++++++++++++++ 1 file changed, 202 insertions(+) create mode 100644 spec/lib/gitlab/reflections/relationships/transformers/merge_data_sources_spec.rb diff --git a/spec/lib/gitlab/reflections/relationships/transformers/merge_data_sources_spec.rb b/spec/lib/gitlab/reflections/relationships/transformers/merge_data_sources_spec.rb new file mode 100644 index 00000000000000..037cc9f0cdaaae --- /dev/null +++ b/spec/lib/gitlab/reflections/relationships/transformers/merge_data_sources_spec.rb @@ -0,0 +1,202 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Gitlab::Reflections::Relationships::Transformers::MergeDataSources, feature_category: :database do + describe '.call' do + subject { described_class.call(relationships) } + + context 'with identical relationships from different data sources' do + let(:relationships) do + [ + { + 'source_table' => 'users', + 'target_table' => 'projects', + 'source_columns' => ['id'], + 'target_columns' => ['user_id'], + 'data_sources' => ['active_record'], + 'has_ar_forward' => true, + 'has_ar_reverse' => false, + 'has_fk_constraint' => false + }, + { + 'source_table' => 'users', + 'target_table' => 'projects', + 'source_columns' => ['id'], + 'target_columns' => ['user_id'], + 'data_sources' => ['foreign_key'], + 'has_ar_forward' => false, + 'has_ar_reverse' => false, + 'has_fk_constraint' => true, + 'fk_metadata' => { 'constraint_name' => 'fk_projects_user_id' } + } + ] + end + + it 'merges relationships with the same key' do + result = subject + + expect(result.length).to eq(1) + + merged = result.first + expect(merged['source_table']).to eq('users') + expect(merged['target_table']).to eq('projects') + expect(merged['source_columns']).to eq(['id']) + expect(merged['target_columns']).to eq(['user_id']) + expect(merged['data_sources']).to contain_exactly('active_record', 'foreign_key') + expect(merged['has_ar_forward']).to be true + expect(merged['has_ar_reverse']).to be false + expect(merged['has_fk_constraint']).to be true + expect(merged['fk_metadata']).to eq({ 'constraint_name' => 'fk_projects_user_id' }) + end + end + + context 'with different relationships' do + let(:relationships) do + [ + { + 'source_table' => 'users', + 'target_table' => 'projects', + 'source_columns' => ['id'], + 'target_columns' => ['user_id'], + 'data_sources' => ['active_record'] + }, + { + 'source_table' => 'users', + 'target_table' => 'issues', + 'source_columns' => ['id'], + 'target_columns' => ['author_id'], + 'data_sources' => ['active_record'] + } + ] + end + + it 'keeps different relationships separate' do + result = subject + + expect(result.length).to eq(2) + + user_project = result.find { |r| r['target_table'] == 'projects' } + user_issue = result.find { |r| r['target_table'] == 'issues' } + + expect(user_project).to be_present + expect(user_issue).to be_present + end + end + + context 'with relationships having different column orders' do + let(:relationships) do + [ + { + 'source_table' => 'merge_requests', + 'target_table' => 'projects', + 'source_columns' => ['target_project_id', 'source_project_id'], + 'target_columns' => ['id', 'id'], + 'data_sources' => ['active_record'] + }, + { + 'source_table' => 'merge_requests', + 'target_table' => 'projects', + 'source_columns' => ['source_project_id', 'target_project_id'], + 'target_columns' => ['id', 'id'], + 'data_sources' => ['foreign_key'] + } + ] + end + + it 'merges relationships with same columns regardless of order' do + result = subject + + expect(result.length).to eq(1) + + merged = result.first + expect(merged['data_sources']).to contain_exactly('active_record', 'foreign_key') + end + end + + context 'with single relationship' do + let(:relationships) do + [ + { + 'source_table' => 'users', + 'target_table' => 'projects', + 'source_columns' => ['id'], + 'target_columns' => ['user_id'], + 'data_sources' => ['active_record'] + } + ] + end + + it 'returns the relationship unchanged' do + result = subject + + expect(result.length).to eq(1) + expect(result.first).to eq(relationships.first) + end + end + + context 'with nil and empty data_sources' do + let(:relationships) do + [ + { + 'source_table' => 'users', + 'target_table' => 'projects', + 'source_columns' => ['id'], + 'target_columns' => ['user_id'], + 'data_sources' => nil, + 'has_ar_forward' => true + }, + { + 'source_table' => 'users', + 'target_table' => 'projects', + 'source_columns' => ['id'], + 'target_columns' => ['user_id'], + 'data_sources' => [], + 'has_fk_constraint' => true + } + ] + end + + it 'handles nil and empty data_sources correctly' do + result = subject + + expect(result.length).to eq(1) + + merged = result.first + expect(merged['data_sources']).to eq([]) + expect(merged['has_ar_forward']).to be true + expect(merged['has_fk_constraint']).to be true + end + end + + context 'with string vs array columns' do + let(:relationships) do + [ + { + 'source_table' => 'users', + 'target_table' => 'projects', + 'source_columns' => 'id', + 'target_columns' => 'user_id', + 'data_sources' => ['active_record'] + }, + { + 'source_table' => 'users', + 'target_table' => 'projects', + 'source_columns' => ['id'], + 'target_columns' => ['user_id'], + 'data_sources' => ['foreign_key'] + } + ] + end + + it 'treats string and array columns as equivalent' do + result = subject + + expect(result.length).to eq(1) + + merged = result.first + expect(merged['data_sources']).to contain_exactly('active_record', 'foreign_key') + end + end + end +end \ No newline at end of file -- GitLab From 17cb8f7acdb93c2c0bfa99f807d7111e96bac9e9 Mon Sep 17 00:00:00 2001 From: Alex Pooley Date: Fri, 12 Sep 2025 16:38:45 +0800 Subject: [PATCH 18/19] wip --- .../reflections/relationships/crawler.rb | 4 +- .../reflections/relationships/relationship.rb | 4 +- lib/tasks/database_relationships.rake | 8 +-- .../relationships/foreign_key_spec.rb | 68 +++++++++---------- 4 files changed, 44 insertions(+), 40 deletions(-) diff --git a/lib/gitlab/reflections/relationships/crawler.rb b/lib/gitlab/reflections/relationships/crawler.rb index c111a499b47cac..7466c667882ced 100644 --- a/lib/gitlab/reflections/relationships/crawler.rb +++ b/lib/gitlab/reflections/relationships/crawler.rb @@ -55,10 +55,12 @@ def initialize( # Load and transform relationships using pipeline raw_relationships = Gitlab::Json.parse(File.read(@relationships_file)) + # Symbolize keys to maintain consistency with Relationship#to_h output + symbolized_relationships = raw_relationships.map(&:deep_symbolize_keys) @normalized_relationships = Transformers::Pipeline.new( Transformers::ExpandPolymorphic, Transformers::InvertRelationships - ).execute(raw_relationships) + ).execute(symbolized_relationships) @tables_with_data = Set.new # Convert to Set for efficient lookup, handle nil gracefully diff --git a/lib/gitlab/reflections/relationships/relationship.rb b/lib/gitlab/reflections/relationships/relationship.rb index 24c3718188e808..973f0d258609b9 100644 --- a/lib/gitlab/reflections/relationships/relationship.rb +++ b/lib/gitlab/reflections/relationships/relationship.rb @@ -148,8 +148,10 @@ def polymorphic_type_column_name polymorphic_type_column end - # Hash-style attribute access + # Hash-style attribute access - supports both symbol and string keys def [](attr_name) + # Convert string keys to symbols for consistency + attr_name = attr_name.to_sym if attr_name.is_a?(String) public_send(attr_name) end diff --git a/lib/tasks/database_relationships.rake b/lib/tasks/database_relationships.rake index 3b2503b09eed5d..9ed87c64553207 100644 --- a/lib/tasks/database_relationships.rake +++ b/lib/tasks/database_relationships.rake @@ -26,8 +26,8 @@ namespace :db do { source_table: rel[:source_table], target_table: rel[:target_table], - source_column: rel[:source_column], - target_column: rel[:target_column], + source_columns: rel[:source_columns], + target_columns: rel[:target_columns], relationship_type: rel[:polymorphic] ? "polymorphic" : "regular", # Add traceability information for debugging source_info: { @@ -54,8 +54,8 @@ namespace :db do [ item[:source_table], item[:target_table], - item[:source_column], - item[:target_column], + item[:source_columns], + item[:target_columns], item[:relationship_type], item[:polymorphic_type_column], item[:polymorphic_type_value] diff --git a/spec/lib/gitlab/reflections/relationships/foreign_key_spec.rb b/spec/lib/gitlab/reflections/relationships/foreign_key_spec.rb index 179658d12d5d9b..f046094a097e4e 100644 --- a/spec/lib/gitlab/reflections/relationships/foreign_key_spec.rb +++ b/spec/lib/gitlab/reflections/relationships/foreign_key_spec.rb @@ -64,7 +64,7 @@ it 'uses default Gitlab::Reflections::Database::ForeignKeys' do # Stub the default class stub_const('Gitlab::Reflections::Database::ForeignKeys', mock_fk_reflection) - + instance = described_class.new expect(instance.instance_variable_get(:@fk_reflection)).to eq(mock_fk_reflection) end @@ -85,7 +85,7 @@ it 'creates relationships with correct structure' do relationships = subject - + relationships.each do |relationship| expect(relationship).to be_a(Hash) expect(relationship).to have_key(:source_table) @@ -100,7 +100,7 @@ it 'sets relationship_type to one_to_many' do relationships = subject - + relationships.each do |relationship| expect(relationship[:relationship_type]).to eq('one_to_many') end @@ -108,7 +108,7 @@ it 'sets has_fk_constraint to true' do relationships = subject - + relationships.each do |relationship| expect(relationship[:has_fk_constraint]).to be true end @@ -116,7 +116,7 @@ it 'includes data_sources as foreign_key' do relationships = subject - + relationships.each do |relationship| expect(relationship[:data_sources]).to eq(['foreign_key']) end @@ -125,12 +125,12 @@ it 'creates correct relationship for issues -> projects' do relationships = subject issues_project_rel = relationships.find { |r| r[:target_table] == 'issues' && r[:source_table] == 'projects' } - + expect(issues_project_rel).not_to be_nil expect(issues_project_rel[:foreign_key]).to eq('project_id') expect(issues_project_rel[:foreign_key_table]).to eq('issues') - expect(issues_project_rel[:source_column]).to eq('id') - expect(issues_project_rel[:target_column]).to eq('project_id') + expect(issues_project_rel[:source_columns]).to eq(['id']) + expect(issues_project_rel[:target_columns]).to eq(['project_id']) expect(issues_project_rel[:fk_metadata][:constraint_name]).to eq('fk_issues_project_id') expect(issues_project_rel[:fk_metadata][:on_delete]).to eq('cascade') expect(issues_project_rel[:fk_metadata][:on_update]).to eq('restrict') @@ -139,12 +139,12 @@ it 'creates correct relationship for merge_requests -> users' do relationships = subject mr_user_rel = relationships.find { |r| r[:target_table] == 'merge_requests' && r[:source_table] == 'users' } - + expect(mr_user_rel).not_to be_nil expect(mr_user_rel[:foreign_key]).to eq('author_id') expect(mr_user_rel[:foreign_key_table]).to eq('merge_requests') - expect(mr_user_rel[:source_column]).to eq('id') - expect(mr_user_rel[:target_column]).to eq('author_id') + expect(mr_user_rel[:source_columns]).to eq(['id']) + expect(mr_user_rel[:target_columns]).to eq(['author_id']) expect(mr_user_rel[:fk_metadata][:constraint_name]).to eq('fk_merge_requests_author_id') expect(mr_user_rel[:fk_metadata][:on_delete]).to eq('set_null') expect(mr_user_rel[:fk_metadata][:on_update]).to eq('cascade') @@ -153,12 +153,12 @@ it 'creates correct relationship for notes -> projects' do relationships = subject notes_project_rel = relationships.find { |r| r[:target_table] == 'notes' && r[:source_table] == 'projects' } - + expect(notes_project_rel).not_to be_nil expect(notes_project_rel[:foreign_key]).to eq('project_id') expect(notes_project_rel[:foreign_key_table]).to eq('notes') - expect(notes_project_rel[:source_column]).to eq('id') - expect(notes_project_rel[:target_column]).to eq('project_id') + expect(notes_project_rel[:source_columns]).to eq(['id']) + expect(notes_project_rel[:target_columns]).to eq(['project_id']) expect(notes_project_rel[:fk_metadata][:constraint_name]).to eq('fk_notes_project_id') expect(notes_project_rel[:fk_metadata][:on_delete]).to eq('restrict') expect(notes_project_rel[:fk_metadata][:on_update]).to eq('restrict') @@ -171,10 +171,10 @@ describe '#get_source_data' do it 'converts PostgresForeignKey objects to internal format' do source_data = instance.send(:get_source_data) - + expect(source_data).to be_an(Array) expect(source_data.length).to eq(3) - + source_data.each do |item| expect(item).to have_key(:table_name) expect(item).to have_key(:foreign_key) @@ -197,7 +197,7 @@ it 'returns data unchanged' do test_data = [{ table_name: 'test', foreign_key: double }] filtered_data = instance.send(:filter_source_data, test_data) - + expect(filtered_data).to eq(test_data) end end @@ -221,7 +221,7 @@ it 'maps items to relationships' do relationships = instance.send(:map_to_relationships, test_items) - + expect(relationships).to be_an(Array) expect(relationships.length).to eq(1) expect(relationships.first).to be_a(Hash) @@ -230,14 +230,14 @@ it 'filters out nil relationships' do # Mock build_foreign_key_relationship to return nil for one item allow(instance).to receive(:build_foreign_key_relationship).and_return(nil, { test: 'relationship' }) - + test_items_with_nil = [ { table_name: 'table1', foreign_key: double }, { table_name: 'table2', foreign_key: double } ] - + relationships = instance.send(:map_to_relationships, test_items_with_nil) - + expect(relationships).to eq([{ test: 'relationship' }]) end end @@ -256,22 +256,22 @@ it 'builds a complete relationship hash' do relationship = instance.send(:build_foreign_key_relationship, 'issues', foreign_key) - + expect(relationship).to be_a(Hash) expect(relationship[:source_table]).to eq('projects') expect(relationship[:target_table]).to eq('issues') expect(relationship[:relationship_type]).to eq('one_to_many') expect(relationship[:foreign_key]).to eq('project_id') expect(relationship[:foreign_key_table]).to eq('issues') - expect(relationship[:source_column]).to eq('id') - expect(relationship[:target_column]).to eq('project_id') + expect(relationship[:source_columns]).to eq(['id']) + expect(relationship[:target_columns]).to eq(['project_id']) expect(relationship[:has_fk_constraint]).to be true expect(relationship[:data_sources]).to eq(['foreign_key']) end it 'includes fk_metadata with constraint details' do relationship = instance.send(:build_foreign_key_relationship, 'issues', foreign_key) - + expect(relationship[:fk_metadata]).to be_a(Hash) expect(relationship[:fk_metadata][:constraint_name]).to eq('fk_issues_project_id') expect(relationship[:fk_metadata][:on_delete]).to eq('cascade') @@ -287,17 +287,17 @@ on_delete: 'restrict', on_update: 'restrict' ) - + relationship = instance.send(:build_foreign_key_relationship, 'issues', fk_without_pk) - - expect(relationship[:source_column]).to eq('id') + + expect(relationship[:source_columns]).to eq(['id']) end end describe '#build_fk_definition_from_pg_fk' do it 'converts PostgresForeignKey to OpenStruct with correct attributes' do fk_definition = instance.send(:build_fk_definition_from_pg_fk, mock_pg_fk_1) - + expect(fk_definition).to be_an(OpenStruct) expect(fk_definition.column).to eq('project_id') expect(fk_definition.to_table).to eq('projects') @@ -310,7 +310,7 @@ it 'handles single column foreign keys' do # Test assumes single column FK (takes first element) fk_definition = instance.send(:build_fk_definition_from_pg_fk, mock_pg_fk_2) - + expect(fk_definition.column).to eq('author_id') expect(fk_definition.to_table).to eq('users') expect(fk_definition.primary_key).to eq('id') @@ -318,7 +318,7 @@ it 'preserves constraint metadata' do fk_definition = instance.send(:build_fk_definition_from_pg_fk, mock_pg_fk_2) - + expect(fk_definition.name).to eq('fk_merge_requests_author_id') expect(fk_definition.on_delete).to eq('set_null') expect(fk_definition.on_update).to eq('cascade') @@ -329,13 +329,13 @@ describe 'integration with foreign key reflection' do it 'calls for_relationships on the fk_reflection' do expect(mock_fk_reflection).to receive(:for_relationships).and_return(mock_scope) - + described_class.new(mock_fk_reflection).get_relationships end it 'processes all foreign keys from the scope' do expect(mock_scope).to receive(:find_each).and_yield(mock_pg_fk_1).and_yield(mock_pg_fk_2).and_yield(mock_pg_fk_3) - + relationships = described_class.new(mock_fk_reflection).get_relationships expect(relationships.length).to eq(3) end @@ -389,4 +389,4 @@ end end end -end \ No newline at end of file +end -- GitLab From e077d6c013ce692a2e0e793e1d23862deeb3c5c7 Mon Sep 17 00:00:00 2001 From: Alex Pooley Date: Fri, 12 Sep 2025 18:03:26 +0800 Subject: [PATCH 19/19] wip --- .../reflections/relationships/crawler.rb | 4 +- .../filter_through_relationships.rb | 37 +++++ .../filter_through_relationships_spec.rb | 138 ++++++++++++++++++ 3 files changed, 178 insertions(+), 1 deletion(-) create mode 100644 lib/gitlab/reflections/relationships/transformers/filter_through_relationships.rb create mode 100644 spec/lib/gitlab/reflections/relationships/transformers/filter_through_relationships_spec.rb diff --git a/lib/gitlab/reflections/relationships/crawler.rb b/lib/gitlab/reflections/relationships/crawler.rb index 7466c667882ced..0a2a296678528a 100644 --- a/lib/gitlab/reflections/relationships/crawler.rb +++ b/lib/gitlab/reflections/relationships/crawler.rb @@ -57,9 +57,11 @@ def initialize( raw_relationships = Gitlab::Json.parse(File.read(@relationships_file)) # Symbolize keys to maintain consistency with Relationship#to_h output symbolized_relationships = raw_relationships.map(&:deep_symbolize_keys) + @normalized_relationships = Transformers::Pipeline.new( Transformers::ExpandPolymorphic, - Transformers::InvertRelationships + Transformers::InvertRelationships, + Transformers::FilterThroughRelationships ).execute(symbolized_relationships) @tables_with_data = Set.new diff --git a/lib/gitlab/reflections/relationships/transformers/filter_through_relationships.rb b/lib/gitlab/reflections/relationships/transformers/filter_through_relationships.rb new file mode 100644 index 00000000000000..b529d2f1b3ca87 --- /dev/null +++ b/lib/gitlab/reflections/relationships/transformers/filter_through_relationships.rb @@ -0,0 +1,37 @@ +# frozen_string_literal: true + +module Gitlab + module Reflections + module Relationships + module Transformers + # Filters out "through" relationships that should not be included in direct relationship mapping. + # + # Through relationships use intermediate tables to establish connections between entities + # and should be handled separately from direct relationships to avoid generating + # invalid SQL queries or incorrect relationship assumptions. + class FilterThroughRelationships + def self.call(relationships) + new(relationships).transform + end + + def initialize(relationships) + @relationships = relationships + end + + def transform + @relationships.reject { |rel| through_relationship?(rel) } + end + + private + + def through_relationship?(rel) + # Check if this is a through relationship using the relationship's own indicators + rel[:is_through_association] || + (rel[:through_table] && !rel[:through_table].empty?) || + (rel.respond_to?(:through_relationship?) && rel.through_relationship?) + end + end + end + end + end +end diff --git a/spec/lib/gitlab/reflections/relationships/transformers/filter_through_relationships_spec.rb b/spec/lib/gitlab/reflections/relationships/transformers/filter_through_relationships_spec.rb new file mode 100644 index 00000000000000..1fc3079fb0c429 --- /dev/null +++ b/spec/lib/gitlab/reflections/relationships/transformers/filter_through_relationships_spec.rb @@ -0,0 +1,138 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Gitlab::Reflections::Relationships::Transformers::FilterThroughRelationships do + describe '.call' do + let(:relationships) do + [ + { + source_table: 'users', + target_table: 'projects', + source_columns: ['id'], + target_columns: ['user_id'], + is_through_association: false + }, + { + source_table: 'users', + target_table: 'groups', + source_columns: ['id'], + target_columns: ['user_id'], + through_table: 'members', + is_through_association: true + }, + { + source_table: 'projects', + target_table: 'labels', + source_columns: ['id'], + target_columns: ['project_id'], + through_table: 'project_labels' + }, + { + source_table: 'issues', + target_table: 'users', + source_columns: ['id'], + target_columns: ['issue_id'], + is_through_association: true + }, + { + source_table: 'merge_requests', + target_table: 'users', + source_columns: ['id'], + target_columns: ['merge_request_id'] + } + ] + end + + it 'filters out relationships marked as through associations' do + result = described_class.call(relationships) + + expect(result).to contain_exactly( + { + source_table: 'users', + target_table: 'projects', + source_columns: ['id'], + target_columns: ['user_id'], + is_through_association: false + }, + { + source_table: 'merge_requests', + target_table: 'users', + source_columns: ['id'], + target_columns: ['merge_request_id'] + } + ) + end + + it 'filters out relationships with through_table' do + relationships_with_through_table = [ + { + source_table: 'users', + target_table: 'projects', + source_columns: ['id'], + target_columns: ['user_id'] + }, + { + source_table: 'projects', + target_table: 'labels', + source_columns: ['id'], + target_columns: ['project_id'], + through_table: 'project_labels' + } + ] + + result = described_class.call(relationships_with_through_table) + + expect(result).to contain_exactly( + { + source_table: 'users', + target_table: 'projects', + source_columns: ['id'], + target_columns: ['user_id'] + } + ) + end + + it 'handles relationships with through_relationship? method' do + relationship_object = double('relationship') + allow(relationship_object).to receive(:respond_to?).with(:through_relationship?).and_return(true) + allow(relationship_object).to receive(:through_relationship?).and_return(true) + allow(relationship_object).to receive(:[]).with(:is_through_association).and_return(nil) + allow(relationship_object).to receive(:[]).with(:through_table).and_return(nil) + + normal_relationship = { + source_table: 'users', + target_table: 'projects', + source_columns: ['id'], + target_columns: ['user_id'] + } + + relationships_with_object = [normal_relationship, relationship_object] + + result = described_class.call(relationships_with_object) + + expect(result).to contain_exactly(normal_relationship) + end + + it 'keeps all relationships when none are through relationships' do + direct_relationships = [ + { + source_table: 'users', + target_table: 'projects', + source_columns: ['id'], + target_columns: ['user_id'] + }, + { + source_table: 'projects', + target_table: 'issues', + source_columns: ['id'], + target_columns: ['project_id'] + } + ] + + result = described_class.call(direct_relationships) + + expect(result).to eq(direct_relationships) + end + end +end -- GitLab