From 4d88d08bf874ae2ee1429d070d76b7d7e70555ce Mon Sep 17 00:00:00 2001 From: Abdelkader Boudih Date: Mon, 10 Aug 2026 23:45:57 +0100 Subject: [PATCH 1/5] fix: enforce models filter for all sources and stop hiding empty results rails_lens:annotate[Foo] only applied the filter to the ActiveRecord source; external sources (e.g. ActiveCypher graph models) ignored it and annotated everything on every run. The rake task also suppressed zero-count source lines and annotate_source swallowed source errors unless verbose, so a filter that matched nothing (like the misread rails_lens:annotate[models]) reported only the graph source line and looked like AR annotation silently skipped models. - filter_models_by_names enforces options[:models] centrally for every source, matching class name or table name - always print per-source counts, including zeros - warn with usage hint when a models filter matches nothing - record source-level errors in results[:failed] instead of dropping them --- lib/rails_lens/schema/annotation_manager.rb | 18 ++- lib/rails_lens/tasks/annotate.rake | 7 +- .../annotation_manager_source_filter_test.rb | 112 ++++++++++++++++++ 3 files changed, 134 insertions(+), 3 deletions(-) create mode 100644 test/rails_lens/annotation_manager_source_filter_test.rb diff --git a/lib/rails_lens/schema/annotation_manager.rb b/lib/rails_lens/schema/annotation_manager.rb index 61a72c7..632c73d 100644 --- a/lib/rails_lens/schema/annotation_manager.rb +++ b/lib/rails_lens/schema/annotation_manager.rb @@ -104,19 +104,33 @@ def self.annotate_source(source, options = {}) results = { annotated: [], skipped: [], failed: [] } begin - models = source.models(options) + models = filter_models_by_names(source.models(options), options[:models]) puts " Found #{models.size} #{source.source_name} models" if options[:verbose] models.each do |model| record_result(results, source.annotate_model(model, options)) end rescue StandardError => e - puts " Error processing #{source.source_name} source: #{e.message}" if options[:verbose] + results[:failed] << { model: "#{source.source_name} (source)", error: e.message } end results end + # Enforce the models filter for every source, including sources whose + # own #models implementation ignores options[:models]. + def self.filter_models_by_names(models, names) + names = Array(names) + return models if names.empty? + + models.select do |model| + names.any? do |name| + model.name == name || + (model.respond_to?(:table_name) && model.table_name == name) + end + end + end + # Merge source results into main results. +primary_key+ is the # source-specific success bucket (:annotated or :removed). def self.merge_results(main, source, primary_key) diff --git a/lib/rails_lens/tasks/annotate.rake b/lib/rails_lens/tasks/annotate.rake index 6929800..d854d5d 100644 --- a/lib/rails_lens/tasks/annotate.rake +++ b/lib/rails_lens/tasks/annotate.rake @@ -13,7 +13,7 @@ namespace :rails_lens do if results[:by_source]&.any? results[:by_source].each do |source_name, count| - puts "Annotated #{count} #{source_name} models" if count.positive? + puts "Annotated #{count} #{source_name} models" end else puts "Annotated #{results[:annotated].length} models" @@ -25,6 +25,11 @@ namespace :rails_lens do puts " - #{failure[:model]}: #{failure[:error]}" end end + if options[:models] && results.values_at(:annotated, :skipped, :failed).all?(&:empty?) + warn "No models matched '#{options[:models].join(', ')}'. The task argument is a " \ + 'comma-separated list of model class names, e.g. rails_lens:annotate[User,Admin::Account]. ' \ + 'Run rails_lens:annotate without arguments to annotate everything.' + end end desc 'Remove all annotations from models' diff --git a/test/rails_lens/annotation_manager_source_filter_test.rb b/test/rails_lens/annotation_manager_source_filter_test.rb new file mode 100644 index 0000000..ff0d450 --- /dev/null +++ b/test/rails_lens/annotation_manager_source_filter_test.rb @@ -0,0 +1,112 @@ +# frozen_string_literal: true + +require 'test_helper' +require 'rails_lens/schema/annotation_manager' + +class AnnotationManagerSourceFilterTest < ActiveSupport::TestCase + # A source that ignores options[:models], like external graph-model sources + # (e.g. ActiveCypher) that return their full model list regardless of filter. + class UnfilteredSource < RailsLens::ModelSource + class << self + attr_accessor :model_list, :annotated + + def models(_options = {}) + model_list + end + + def annotate_model(model, _options = {}) + self.annotated ||= [] + annotated << model.name + { status: :annotated, model: model.name } + end + + def source_name + 'Unfiltered' + end + end + end + + class ExplodingSource < RailsLens::ModelSource + class << self + def models(_options = {}) + raise 'boom' + end + + def source_name + 'Exploding' + end + end + end + + def fake_model(name, table_name = nil) + Class.new do + define_singleton_method(:name) { name } + define_singleton_method(:table_name) { table_name } if table_name + end + end + + setup do + UnfilteredSource.annotated = [] + UnfilteredSource.model_list = [ + fake_model('FakeUser', 'fake_users'), + fake_model('FakePost', 'fake_posts'), + fake_model('FakeNode') + ] + end + + def test_filter_models_by_names_returns_all_without_filter + models = UnfilteredSource.model_list + + assert_equal models, RailsLens::Schema::AnnotationManager.filter_models_by_names(models, nil) + assert_equal models, RailsLens::Schema::AnnotationManager.filter_models_by_names(models, []) + end + + def test_filter_models_by_names_matches_class_name + models = UnfilteredSource.model_list + filtered = RailsLens::Schema::AnnotationManager.filter_models_by_names(models, ['FakeUser']) + + assert_equal %w[FakeUser], filtered.map(&:name) + end + + def test_filter_models_by_names_matches_table_name + models = UnfilteredSource.model_list + filtered = RailsLens::Schema::AnnotationManager.filter_models_by_names(models, ['fake_posts']) + + assert_equal %w[FakePost], filtered.map(&:name) + end + + def test_filter_models_by_names_handles_models_without_table_name + models = UnfilteredSource.model_list + filtered = RailsLens::Schema::AnnotationManager.filter_models_by_names(models, ['FakeNode']) + + assert_equal %w[FakeNode], filtered.map(&:name) + end + + def test_annotate_source_enforces_models_filter_on_sources_that_ignore_it + results = RailsLens::Schema::AnnotationManager.annotate_source( + UnfilteredSource, { models: ['FakeUser'] } + ) + + assert_equal %w[FakeUser], results[:annotated] + assert_equal %w[FakeUser], UnfilteredSource.annotated + end + + def test_annotate_source_with_unmatched_filter_annotates_nothing + results = RailsLens::Schema::AnnotationManager.annotate_source( + UnfilteredSource, { models: ['models'] } + ) + + assert_empty results[:annotated] + assert_empty results[:skipped] + assert_empty results[:failed] + assert_empty UnfilteredSource.annotated + end + + def test_annotate_source_records_source_errors_as_failures + results = RailsLens::Schema::AnnotationManager.annotate_source(ExplodingSource, {}) + + assert_equal 1, results[:failed].length + assert_equal 'Exploding (source)', results[:failed].first[:model] + assert_equal 'boom', results[:failed].first[:error] + end +end From a7b6134387200d990972a79dc15a25700d30bd44 Mon Sep 17 00:00:00 2001 From: Abdelkader Boudih Date: Tue, 11 Aug 2026 00:04:40 +0100 Subject: [PATCH 2/5] fix: detect composite primary keys via Rails-native API Use model.primary_key (an Array for composite keys since Rails 7.1) in the CompositeKeys analyzer instead of the legacy primary_keys reader and a raw pg_index query. Schema adapters switch from connection.primary_key (returns nil for composite keys) to connection.primary_keys so composite columns are marked pk = true. Dummy model annotations regenerated accordingly. --- lib/rails_lens/analyzers/composite_keys.rb | 40 +----- lib/rails_lens/schema/adapters/base.rb | 7 +- lib/rails_lens/schema/adapters/postgresql.rb | 6 +- test/dummy/app/models/cargo_vessel.rb | 4 +- test/dummy/app/models/crew_member.rb | 2 +- test/dummy/app/models/dinosaur.rb | 2 +- test/dummy/app/models/excavation_site.rb | 2 +- test/dummy/app/models/family.rb | 2 +- test/dummy/app/models/home_planet.rb | 2 +- test/dummy/app/models/manufacturer.rb | 2 +- test/dummy/app/models/mission_waypoint.rb | 2 +- test/dummy/app/models/order_line_item.rb | 18 +-- test/dummy/app/models/product.rb | 2 +- test/dummy/app/models/product_metric.rb | 2 +- test/dummy/app/models/spatial_coordinate.rb | 2 +- test/dummy/app/models/species.rb | 2 +- .../app/models/starfleet_battle_cruiser.rb | 5 + test/dummy/app/models/tenant_setting.rb | 4 +- test/dummy/app/models/trigger.rb | 14 ++ test/dummy/app/models/vehicle_owner.rb | 2 +- .../database_specific_features_test.rb | 2 +- .../analyzers/composite_keys_test.rb | 125 ++---------------- .../schema/adapters/postgresql_test.rb | 4 +- 23 files changed, 62 insertions(+), 191 deletions(-) diff --git a/lib/rails_lens/analyzers/composite_keys.rb b/lib/rails_lens/analyzers/composite_keys.rb index be6ec37..c2d92da 100644 --- a/lib/rails_lens/analyzers/composite_keys.rb +++ b/lib/rails_lens/analyzers/composite_keys.rb @@ -4,22 +4,10 @@ module RailsLens module Analyzers class CompositeKeys < Base def analyze - # First try Rails native support - if model_class.respond_to?(:primary_keys) && model_class.primary_keys.is_a?(Array) - keys = model_class.primary_keys - return format_composite_keys(keys) if keys.length > 1 - end + pk = model_class.primary_key + return nil unless pk.is_a?(Array) - # For PostgreSQL, check the actual database constraints - if adapter_name == 'PostgreSQL' - keys = detect_composite_primary_key_from_db - return format_composite_keys(keys) if keys && keys.length > 1 - end - - nil - rescue NoMethodError => e - RailsLens.logger.debug { "Failed to analyze composite keys for #{model_class.name}: #{e.message}" } - nil + format_composite_keys(pk) rescue ActiveRecord::ConnectionNotEstablished => e RailsLens.logger.debug { "No database connection for #{model_class.name}: #{e.message}" } nil @@ -32,28 +20,6 @@ def format_composite_keys(keys) lines << "keys = #{TomlFormat.quoted_array(keys)}" lines.join("\n") end - - def detect_composite_primary_key_from_db - # Query PostgreSQL system catalogs to find composite primary keys - sql = <<~SQL.squish - SELECT a.attname - FROM pg_index i - JOIN pg_attribute a ON a.attrelid = i.indrelid AND a.attnum = ANY(i.indkey) - WHERE i.indrelid = '#{table_name}'::regclass - AND i.indisprimary - ORDER BY array_position(i.indkey, a.attnum) - SQL - - result = connection.execute(sql) - keys = result.pluck('attname') - keys.empty? ? nil : keys - rescue ActiveRecord::StatementInvalid => e - RailsLens.logger.debug { "Failed to detect composite keys from database for #{table_name}: #{e.message}" } - nil - rescue PG::Error => e - RailsLens.logger.debug { "PostgreSQL error detecting composite keys: #{e.message}" } - nil - end end end end diff --git a/lib/rails_lens/schema/adapters/base.rb b/lib/rails_lens/schema/adapters/base.rb index 3a136ff..578acf3 100644 --- a/lib/rails_lens/schema/adapters/base.rb +++ b/lib/rails_lens/schema/adapters/base.rb @@ -164,11 +164,12 @@ def format_default(default) end def primary_key?(column) - column.name == primary_key_name + primary_key_names.include?(column.name) end - def primary_key_name - @primary_key_name ||= connection.primary_key(unqualified_table_name) + # The singular connection.primary_key returns nil for composite keys. + def primary_key_names + @primary_key_names ||= connection.primary_keys(unqualified_table_name) end def column_name_width diff --git a/lib/rails_lens/schema/adapters/postgresql.rb b/lib/rails_lens/schema/adapters/postgresql.rb index 6e862f8..390e6ec 100644 --- a/lib/rails_lens/schema/adapters/postgresql.rb +++ b/lib/rails_lens/schema/adapters/postgresql.rb @@ -112,9 +112,9 @@ def fetch_foreign_keys end end - def primary_key_name - @primary_key_name ||= with_schema_in_search_path do - connection.primary_key(unqualified_table_name) + def primary_key_names + @primary_key_names ||= with_schema_in_search_path do + connection.primary_keys(unqualified_table_name) end end diff --git a/test/dummy/app/models/cargo_vessel.rb b/test/dummy/app/models/cargo_vessel.rb index 7b40b8b..cb3d320 100644 --- a/test/dummy/app/models/cargo_vessel.rb +++ b/test/dummy/app/models/cargo_vessel.rb @@ -32,7 +32,9 @@ # cargo_type = { general = "general", refrigerated = "refrigerated", hazardous = "hazardous", livestock = "livestock" } # # [callbacks] -# before_save = [{ method = "validate_cargo_weight" }] +# before_validation = [{ method = "set_defaults" }] +# before_save = [{ method = "calculate_crew_capacity" }, { method = "validate_cargo_weight" }] +# after_update = [{ method = "notify_fleet_command" }] # # notes = ["spaceship_crew_members:N_PLUS_ONE", "crew_members:N_PLUS_ONE", "missions:N_PLUS_ONE", "spatial_coordinates:N_PLUS_ONE", "comments:N_PLUS_ONE", "name:NOT_NULL", "class_type:NOT_NULL", "warp_capability:NOT_NULL", "status:NOT_NULL", "type:NOT_NULL", "cargo_capacity:NOT_NULL", "cargo_type:NOT_NULL", "battle_status:NOT_NULL", "warp_capability:DEFAULT", "status:DEFAULT", "battle_status:DEFAULT", "name:LIMIT", "class_type:LIMIT", "status:LIMIT", "type:LIMIT", "cargo_type:LIMIT", "battle_status:LIMIT", "class_type:INDEX", "status:INDEX", "type:INDEX", "cargo_type:INDEX", "battle_status:INDEX", "type:STI_NOT_NULL"] # diff --git a/test/dummy/app/models/crew_member.rb b/test/dummy/app/models/crew_member.rb index b286404..2d34b75 100644 --- a/test/dummy/app/models/crew_member.rb +++ b/test/dummy/app/models/crew_member.rb @@ -27,7 +27,7 @@ # before_update = [{ method = "prevent_status_change", if = ["deceased?"] }] # before_destroy = [{ method = "check_active_missions" }] # -# notes = ["home_planet_id:INDEX", "home_planet_id:FK_CONSTRAINT", "spaceship_crew_members:INVERSE_OF", "home_planet:INVERSE_OF", "spaceship_crew_members:N_PLUS_ONE", "spaceships:N_PLUS_ONE", "name:NOT_NULL", "rank:NOT_NULL", "species:NOT_NULL", "birth_planet:NOT_NULL", "service_record:NOT_NULL", "active:NOT_NULL", "status:NOT_NULL", "specialization:NOT_NULL", "active:DEFAULT", "status:DEFAULT", "name:LIMIT", "rank:LIMIT", "species:LIMIT", "birth_planet:LIMIT", "specialization:LIMIT", "status:INDEX", "service_record:STORAGE"] +# notes = ["home_planet_id:INDEX", "home_planet_id:FK_CONSTRAINT", "spaceship_crew_members:N_PLUS_ONE", "spaceships:N_PLUS_ONE", "name:NOT_NULL", "rank:NOT_NULL", "species:NOT_NULL", "birth_planet:NOT_NULL", "service_record:NOT_NULL", "active:NOT_NULL", "status:NOT_NULL", "specialization:NOT_NULL", "active:DEFAULT", "status:DEFAULT", "name:LIMIT", "rank:LIMIT", "species:LIMIT", "birth_planet:LIMIT", "specialization:LIMIT", "status:INDEX", "service_record:STORAGE"] # class CrewMember < ApplicationRecord # Enums diff --git a/test/dummy/app/models/dinosaur.rb b/test/dummy/app/models/dinosaur.rb index 755d980..80851ed 100644 --- a/test/dummy/app/models/dinosaur.rb +++ b/test/dummy/app/models/dinosaur.rb @@ -27,7 +27,7 @@ # before_destroy = [{ method = "archive_research_data" }, { method = "notify_researchers" }] # after_destroy = [{ method = "update_period_statistics" }, { method = "cleanup_external_references" }] # -# notes = ["fossil_discoveries:INVERSE_OF", "fossil_discoveries:N_PLUS_ONE", "name:NOT_NULL", "species:NOT_NULL", "period:NOT_NULL", "diet:NOT_NULL", "length:NOT_NULL", "weight:NOT_NULL", "fossil_count:NOT_NULL", "name:LIMIT", "species:LIMIT", "period:LIMIT", "diet:LIMIT"] +# notes = ["fossil_discoveries:N_PLUS_ONE", "name:NOT_NULL", "species:NOT_NULL", "period:NOT_NULL", "diet:NOT_NULL", "length:NOT_NULL", "weight:NOT_NULL", "fossil_count:NOT_NULL", "name:LIMIT", "species:LIMIT", "period:LIMIT", "diet:LIMIT"] # class Dinosaur < PrehistoricRecord # Enums diff --git a/test/dummy/app/models/excavation_site.rb b/test/dummy/app/models/excavation_site.rb index 4c975fa..51d654b 100644 --- a/test/dummy/app/models/excavation_site.rb +++ b/test/dummy/app/models/excavation_site.rb @@ -23,7 +23,7 @@ # rock_formation = { morrison = "morrison", hell_creek = "hell_creek", tendaguru = "tendaguru", nemegt = "nemegt", judith_river = "judith_river", solnhofen = "solnhofen", burgess_shale = "burgess_shale" } # climate_ancient = { tropical = "tropical", subtropical = "subtropical", temperate = "temperate", arid = "arid", coastal = "coastal", swamp = "swamp" } # -# notes = ["fossil_discoveries:INVERSE_OF", "fossil_discoveries:N_PLUS_ONE", "dinosaurs:N_PLUS_ONE", "name:NOT_NULL", "location:NOT_NULL", "coordinates:NOT_NULL", "depth:NOT_NULL", "soil_type:NOT_NULL", "active:NOT_NULL", "rock_formation:NOT_NULL", "climate_ancient:NOT_NULL", "active:DEFAULT", "name:LIMIT", "location:LIMIT", "coordinates:LIMIT", "soil_type:LIMIT", "soil_type:INDEX"] +# notes = ["fossil_discoveries:N_PLUS_ONE", "dinosaurs:N_PLUS_ONE", "name:NOT_NULL", "location:NOT_NULL", "coordinates:NOT_NULL", "depth:NOT_NULL", "soil_type:NOT_NULL", "active:NOT_NULL", "rock_formation:NOT_NULL", "climate_ancient:NOT_NULL", "active:DEFAULT", "name:LIMIT", "location:LIMIT", "coordinates:LIMIT", "soil_type:LIMIT", "soil_type:INDEX"] # class ExcavationSite < PrehistoricRecord # Enums diff --git a/test/dummy/app/models/family.rb b/test/dummy/app/models/family.rb index 4f65c77..319decc 100644 --- a/test/dummy/app/models/family.rb +++ b/test/dummy/app/models/family.rb @@ -31,7 +31,7 @@ # after_create = [{ method = "notify_parent_of_new_child" }] # before_destroy = [{ method = "_ct_before_destroy" }, { method = "check_for_children" }] # -# notes = ["parent_id:INDEX", "parent_id:FK_CONSTRAINT", "ancestor_hierarchies:INVERSE_OF", "descendant_hierarchies:INVERSE_OF", "species:INVERSE_OF", "children:N_PLUS_ONE", "ancestor_hierarchies:N_PLUS_ONE", "self_and_ancestors:N_PLUS_ONE", "descendant_hierarchies:N_PLUS_ONE", "self_and_descendants:N_PLUS_ONE", "species:N_PLUS_ONE", "dinosaurs:N_PLUS_ONE", "parent:COUNTER_CACHE", "name:NOT_NULL", "classification:NOT_NULL", "taxonomic_rank:NOT_NULL", "description:NOT_NULL", "name:LIMIT", "classification:LIMIT", "taxonomic_rank:LIMIT", "description:STORAGE", "family_hierarchies:COMP_INDEX", "generations:INDEX", "children:COUNTER_CACHE"] +# notes = ["parent_id:INDEX", "parent_id:FK_CONSTRAINT", "ancestor_hierarchies:INVERSE_OF", "self_and_ancestors:INVERSE_OF", "descendant_hierarchies:INVERSE_OF", "self_and_descendants:INVERSE_OF", "children:N_PLUS_ONE", "ancestor_hierarchies:N_PLUS_ONE", "self_and_ancestors:N_PLUS_ONE", "descendant_hierarchies:N_PLUS_ONE", "self_and_descendants:N_PLUS_ONE", "species:N_PLUS_ONE", "dinosaurs:N_PLUS_ONE", "parent:COUNTER_CACHE", "name:NOT_NULL", "classification:NOT_NULL", "taxonomic_rank:NOT_NULL", "description:NOT_NULL", "name:LIMIT", "classification:LIMIT", "taxonomic_rank:LIMIT", "description:STORAGE", "family_hierarchies:COMP_INDEX", "generations:INDEX", "children:COUNTER_CACHE"] # class Family < PrehistoricRecord has_closure_tree order: 'name' diff --git a/test/dummy/app/models/home_planet.rb b/test/dummy/app/models/home_planet.rb index 6a9ceec..fccad66 100644 --- a/test/dummy/app/models/home_planet.rb +++ b/test/dummy/app/models/home_planet.rb @@ -38,7 +38,7 @@ # after_save = [{ method = "_ct_after_save" }] # before_destroy = [{ method = "_ct_before_destroy" }] # -# notes = ["parent_id:INDEX", "parent_id:FK_CONSTRAINT", "ancestor_hierarchies:INVERSE_OF", "descendant_hierarchies:INVERSE_OF", "children:N_PLUS_ONE", "ancestor_hierarchies:N_PLUS_ONE", "self_and_ancestors:N_PLUS_ONE", "descendant_hierarchies:N_PLUS_ONE", "self_and_descendants:N_PLUS_ONE", "crew_members:N_PLUS_ONE", "parent:COUNTER_CACHE", "name:NOT_NULL", "galaxy:NOT_NULL", "coordinates:NOT_NULL", "habitability_score:NOT_NULL", "climate_type:NOT_NULL", "population:NOT_NULL", "classification:NOT_NULL", "hierarchy_type:NOT_NULL", "name:LIMIT", "galaxy:LIMIT", "climate_type:LIMIT", "climate_type:INDEX", "hierarchy_type:INDEX", "home_planet_hierarchies:COMP_INDEX", "generations:INDEX", "children:COUNTER_CACHE"] +# notes = ["parent_id:INDEX", "parent_id:FK_CONSTRAINT", "ancestor_hierarchies:INVERSE_OF", "self_and_ancestors:INVERSE_OF", "descendant_hierarchies:INVERSE_OF", "self_and_descendants:INVERSE_OF", "children:N_PLUS_ONE", "ancestor_hierarchies:N_PLUS_ONE", "self_and_ancestors:N_PLUS_ONE", "descendant_hierarchies:N_PLUS_ONE", "self_and_descendants:N_PLUS_ONE", "crew_members:N_PLUS_ONE", "parent:COUNTER_CACHE", "name:NOT_NULL", "galaxy:NOT_NULL", "coordinates:NOT_NULL", "habitability_score:NOT_NULL", "climate_type:NOT_NULL", "population:NOT_NULL", "classification:NOT_NULL", "hierarchy_type:NOT_NULL", "name:LIMIT", "galaxy:LIMIT", "climate_type:LIMIT", "climate_type:INDEX", "hierarchy_type:INDEX", "home_planet_hierarchies:COMP_INDEX", "generations:INDEX", "children:COUNTER_CACHE"] # class HomePlanet < ApplicationRecord # ClosureTree for hierarchical structure diff --git a/test/dummy/app/models/manufacturer.rb b/test/dummy/app/models/manufacturer.rb index 4b37b4e..d6097c8 100644 --- a/test/dummy/app/models/manufacturer.rb +++ b/test/dummy/app/models/manufacturer.rb @@ -39,7 +39,7 @@ # after_save = [{ method = "_ct_after_save" }] # before_destroy = [{ method = "_ct_before_destroy" }] # -# notes = ["parent_id:INDEX", "parent_id:FK_CONSTRAINT", "ancestor_hierarchies:INVERSE_OF", "descendant_hierarchies:INVERSE_OF", "vehicles:INVERSE_OF", "children:N_PLUS_ONE", "ancestor_hierarchies:N_PLUS_ONE", "self_and_ancestors:N_PLUS_ONE", "descendant_hierarchies:N_PLUS_ONE", "self_and_descendants:N_PLUS_ONE", "vehicles:N_PLUS_ONE", "parent:COUNTER_CACHE", "name:NOT_NULL", "country:NOT_NULL", "founded_year:NOT_NULL", "headquarters:NOT_NULL", "website:NOT_NULL", "annual_revenue:NOT_NULL", "active:NOT_NULL", "logo_url:NOT_NULL", "status:NOT_NULL", "company_type:NOT_NULL", "active:DEFAULT", "status:DEFAULT", "status:INDEX", "company_type:INDEX", "headquarters:STORAGE", "manufacturer_hierarchies:COMP_INDEX", "generations:INDEX", "children:COUNTER_CACHE"] +# notes = ["parent_id:INDEX", "parent_id:FK_CONSTRAINT", "ancestor_hierarchies:INVERSE_OF", "self_and_ancestors:INVERSE_OF", "descendant_hierarchies:INVERSE_OF", "self_and_descendants:INVERSE_OF", "children:N_PLUS_ONE", "ancestor_hierarchies:N_PLUS_ONE", "self_and_ancestors:N_PLUS_ONE", "descendant_hierarchies:N_PLUS_ONE", "self_and_descendants:N_PLUS_ONE", "vehicles:N_PLUS_ONE", "parent:COUNTER_CACHE", "name:NOT_NULL", "country:NOT_NULL", "founded_year:NOT_NULL", "headquarters:NOT_NULL", "website:NOT_NULL", "annual_revenue:NOT_NULL", "active:NOT_NULL", "logo_url:NOT_NULL", "status:NOT_NULL", "company_type:NOT_NULL", "active:DEFAULT", "status:DEFAULT", "status:INDEX", "company_type:INDEX", "headquarters:STORAGE", "manufacturer_hierarchies:COMP_INDEX", "generations:INDEX", "children:COUNTER_CACHE"] # class Manufacturer < VehicleRecord # Include ClosureTree for hierarchy diff --git a/test/dummy/app/models/mission_waypoint.rb b/test/dummy/app/models/mission_waypoint.rb index 9d5a509..1d3922d 100644 --- a/test/dummy/app/models/mission_waypoint.rb +++ b/test/dummy/app/models/mission_waypoint.rb @@ -32,7 +32,7 @@ # [callbacks] # before_validation = [{ method = "set_default_status" }] # -# notes = ["mission:INVERSE_OF", "sequence:NOT_NULL", "coordinates:NOT_NULL", "eta:NOT_NULL", "notes:NOT_NULL", "waypoint_type:NOT_NULL", "status:NOT_NULL", "status:DEFAULT", "coordinates:LIMIT", "waypoint_type:INDEX", "status:INDEX", "notes:STORAGE"] +# notes = ["sequence:NOT_NULL", "coordinates:NOT_NULL", "eta:NOT_NULL", "notes:NOT_NULL", "waypoint_type:NOT_NULL", "status:NOT_NULL", "status:DEFAULT", "coordinates:LIMIT", "waypoint_type:INDEX", "status:INDEX", "notes:STORAGE"] # class MissionWaypoint < ApplicationRecord # Enums diff --git a/test/dummy/app/models/order_line_item.rb b/test/dummy/app/models/order_line_item.rb index f4e5f54..43521d9 100644 --- a/test/dummy/app/models/order_line_item.rb +++ b/test/dummy/app/models/order_line_item.rb @@ -5,8 +5,8 @@ # database_dialect = "PostgreSQL" # # columns = [ -# { name = "order_id", type = "integer", null = false }, -# { name = "line_number", type = "integer", null = false }, +# { name = "order_id", type = "integer", pk = true, null = false }, +# { name = "line_number", type = "integer", pk = true, null = false }, # { name = "quantity", type = "integer", null = false, default = "1" }, # { name = "unit_price", type = "decimal", null = false }, # { name = "total_price", type = "decimal" }, @@ -33,17 +33,7 @@ class OrderLineItem < ApplicationRecord include Trackable - # Composite primary key using PostgreSQL - self.primary_key = [:order_id, :line_number] - - # This simulates what the composite_primary_keys gem would do - def self.primary_keys - primary_key - end - - def self.respond_to_missing?(method_name, include_private = false) - method_name == :primary_keys || super - end + self.primary_key = %i[order_id line_number] # Associations would go here # belongs_to :order @@ -54,4 +44,4 @@ def self.respond_to_missing?(method_name, include_private = false) validates :line_number, presence: true validates :quantity, presence: true, numericality: { greater_than: 0 } validates :unit_price, presence: true, numericality: { greater_than: 0 } -end \ No newline at end of file +end diff --git a/test/dummy/app/models/product.rb b/test/dummy/app/models/product.rb index 169e901..4735042 100644 --- a/test/dummy/app/models/product.rb +++ b/test/dummy/app/models/product.rb @@ -31,7 +31,7 @@ # after_update = [{ method = "audit_update" }] # after_destroy = [{ method = "audit_destruction" }] # -# notes = ["product_metrics:INVERSE_OF", "product_metrics:N_PLUS_ONE", "description:NOT_NULL", "category:NOT_NULL", "sku:NOT_NULL", "stock_quantity:NOT_NULL", "name:LIMIT", "category:LIMIT", "sku:LIMIT", "description:STORAGE"] +# notes = ["product_metrics:N_PLUS_ONE", "description:NOT_NULL", "category:NOT_NULL", "sku:NOT_NULL", "stock_quantity:NOT_NULL", "name:LIMIT", "category:LIMIT", "sku:LIMIT", "description:STORAGE"] # class Product < ApplicationRecord include Trackable diff --git a/test/dummy/app/models/product_metric.rb b/test/dummy/app/models/product_metric.rb index bdbdfd0..effffe9 100644 --- a/test/dummy/app/models/product_metric.rb +++ b/test/dummy/app/models/product_metric.rb @@ -45,7 +45,7 @@ # ELSE (0)::numeric # END" }] # -# notes = ["product:INVERSE_OF", "revenue:NOT_NULL", "conversion_rate:NOT_NULL", "average_order_value:NOT_NULL"] +# notes = ["revenue:NOT_NULL", "conversion_rate:NOT_NULL", "average_order_value:NOT_NULL"] # class ProductMetric < ApplicationRecord # Associations diff --git a/test/dummy/app/models/spatial_coordinate.rb b/test/dummy/app/models/spatial_coordinate.rb index 2c6c517..dcd04cd 100644 --- a/test/dummy/app/models/spatial_coordinate.rb +++ b/test/dummy/app/models/spatial_coordinate.rb @@ -31,7 +31,7 @@ # { column = "spaceship_id", references_table = "spaceships", references_column = "id", name = "fk_rails_69751d644a" } # ] # -# notes = ["spaceship:INVERSE_OF", "location:NOT_NULL", "coordinates:NOT_NULL", "sensor_data:NOT_NULL", "metadata:NOT_NULL", "ip_address:NOT_NULL", "altitude:NOT_NULL", "longitude:NOT_NULL", "latitude:NOT_NULL", "tracking_id:INDEX"] +# notes = ["location:NOT_NULL", "coordinates:NOT_NULL", "sensor_data:NOT_NULL", "metadata:NOT_NULL", "ip_address:NOT_NULL", "altitude:NOT_NULL", "longitude:NOT_NULL", "latitude:NOT_NULL", "tracking_id:INDEX"] # class SpatialCoordinate < ApplicationRecord belongs_to :spaceship diff --git a/test/dummy/app/models/species.rb b/test/dummy/app/models/species.rb index 3d2c9cc..40e8e16 100644 --- a/test/dummy/app/models/species.rb +++ b/test/dummy/app/models/species.rb @@ -24,7 +24,7 @@ # { column = "family_id", references_table = "families", references_column = "id" } # ] # -# notes = ["family:INVERSE_OF", "dinosaurs:N_PLUS_ONE", "family:COUNTER_CACHE", "name:NOT_NULL", "average_lifespan:NOT_NULL", "habitat:NOT_NULL", "danger_level:NOT_NULL", "locomotion:NOT_NULL", "name:LIMIT", "habitat:STORAGE"] +# notes = ["dinosaurs:N_PLUS_ONE", "family:COUNTER_CACHE", "name:NOT_NULL", "average_lifespan:NOT_NULL", "habitat:NOT_NULL", "danger_level:NOT_NULL", "locomotion:NOT_NULL", "name:LIMIT", "habitat:STORAGE"] # class Species < PrehistoricRecord belongs_to :family diff --git a/test/dummy/app/models/starfleet_battle_cruiser.rb b/test/dummy/app/models/starfleet_battle_cruiser.rb index 8ad7f30..8fdd7b8 100644 --- a/test/dummy/app/models/starfleet_battle_cruiser.rb +++ b/test/dummy/app/models/starfleet_battle_cruiser.rb @@ -31,6 +31,11 @@ # status = { active = "active", maintenance = "maintenance", decommissioned = "decommissioned", obliterated = "obliterated" } # battle_status = { standby = "standby", yellow_alert = "yellow_alert", red_alert = "red_alert", battle_stations = "battle_stations" } # +# [callbacks] +# before_validation = [{ method = "set_defaults" }] +# before_save = [{ method = "calculate_crew_capacity" }] +# after_update = [{ method = "notify_fleet_command" }] +# # notes = ["spaceship_crew_members:N_PLUS_ONE", "crew_members:N_PLUS_ONE", "missions:N_PLUS_ONE", "spatial_coordinates:N_PLUS_ONE", "comments:N_PLUS_ONE", "name:NOT_NULL", "class_type:NOT_NULL", "warp_capability:NOT_NULL", "status:NOT_NULL", "type:NOT_NULL", "cargo_capacity:NOT_NULL", "cargo_type:NOT_NULL", "battle_status:NOT_NULL", "warp_capability:DEFAULT", "status:DEFAULT", "battle_status:DEFAULT", "name:LIMIT", "class_type:LIMIT", "status:LIMIT", "type:LIMIT", "cargo_type:LIMIT", "battle_status:LIMIT", "class_type:INDEX", "status:INDEX", "type:INDEX", "cargo_type:INDEX", "battle_status:INDEX", "type:STI_NOT_NULL"] # class StarfleetBattleCruiser < Spaceship diff --git a/test/dummy/app/models/tenant_setting.rb b/test/dummy/app/models/tenant_setting.rb index da83924..47ae1cf 100644 --- a/test/dummy/app/models/tenant_setting.rb +++ b/test/dummy/app/models/tenant_setting.rb @@ -5,8 +5,8 @@ # database_dialect = "PostgreSQL" # # columns = [ -# { name = "tenant_id", type = "integer", null = false }, -# { name = "key", type = "string", null = false }, +# { name = "tenant_id", type = "integer", pk = true, null = false }, +# { name = "key", type = "string", pk = true, null = false }, # { name = "value", type = "text" }, # { name = "description", type = "text" }, # { name = "encrypted", type = "boolean", default = "false" }, diff --git a/test/dummy/app/models/trigger.rb b/test/dummy/app/models/trigger.rb index 9d18ab3..4636377 100644 --- a/test/dummy/app/models/trigger.rb +++ b/test/dummy/app/models/trigger.rb @@ -1,5 +1,19 @@ # frozen_string_literal: true +# +# table = "triggers" +# database_dialect = "PostgreSQL" +# +# columns = [ +# { name = "id", type = "integer", pk = true, null = false }, +# { name = "name", type = "string" }, +# { name = "description", type = "text" }, +# { name = "created_at", type = "datetime", null = false }, +# { name = "updated_at", type = "datetime", null = false } +# ] +# +# notes = ["name:NOT_NULL", "description:NOT_NULL", "name:LIMIT", "description:STORAGE"] +# # Test model with a table name that collides with a PostgreSQL system view # (information_schema.triggers). This is used to verify that the ModelDetector # correctly filters system schemas when checking for views. diff --git a/test/dummy/app/models/vehicle_owner.rb b/test/dummy/app/models/vehicle_owner.rb index a8c5e14..ada7044 100644 --- a/test/dummy/app/models/vehicle_owner.rb +++ b/test/dummy/app/models/vehicle_owner.rb @@ -27,7 +27,7 @@ # { column = "owner_id", references_table = "owners", references_column = "id", name = "fk_rails_f12ecc0d84" } # ] # -# notes = ["vehicle:INVERSE_OF", "owner:INVERSE_OF", "ownership_start:NOT_NULL", "ownership_end:NOT_NULL"] +# notes = ["ownership_start:NOT_NULL", "ownership_end:NOT_NULL"] # class VehicleOwner < VehicleRecord belongs_to :vehicle diff --git a/test/integration/database_specific_features_test.rb b/test/integration/database_specific_features_test.rb index 5b02440..2bb17c5 100644 --- a/test/integration/database_specific_features_test.rb +++ b/test/integration/database_specific_features_test.rb @@ -336,7 +336,7 @@ def test_postgresql_schema_qualified_table_names adapter.send(:columns) adapter.send(:fetch_indexes) adapter.send(:fetch_foreign_keys) - adapter.send(:primary_key_name) + adapter.send(:primary_key_names) end end end diff --git a/test/rails_lens/analyzers/composite_keys_test.rb b/test/rails_lens/analyzers/composite_keys_test.rb index 154f531..637f929 100644 --- a/test/rails_lens/analyzers/composite_keys_test.rb +++ b/test/rails_lens/analyzers/composite_keys_test.rb @@ -5,135 +5,28 @@ module RailsLens module Analyzers class CompositeKeysTest < ActiveSupport::TestCase - def test_analyze_with_normal_single_primary_key_models - # Test with real models that have single primary keys + def test_single_primary_key_models_return_nil [User, Post, Vehicle, Family].each do |model| - analyzer = CompositeKeys.new(model) - result = analyzer.analyze - - # Single primary key models should return nil (no composite key info) - assert_nil result, "#{model.name} should not have composite key info" + assert_nil CompositeKeys.new(model).analyze, "#{model.name} should not have composite key info" end end - def test_analyze_with_real_composite_primary_key_model - # OrderLineItem has real composite primary key [:order_id, :line_number] - analyzer = CompositeKeys.new(OrderLineItem) - result = analyzer.analyze - - assert_match(/== Composite Primary Key/, result) - assert_match(/Primary Keys: order_id, line_number/, result) - end - - def test_analyze_respects_actual_model_primary_key_behavior - # Verify that the analyzer works with real model primary key behavior - - # Real single-key models should have single primary keys - assert_equal 'id', User.primary_key - assert_equal 'id', Vehicle.primary_key - assert_equal 'id', Family.primary_key - - # OrderLineItem should have composite primary key - assert_equal %w[order_id line_number], OrderLineItem.primary_key - - # Single-key models should not support primary_keys method - assert_not User.respond_to?(:primary_keys) - assert_not Vehicle.respond_to?(:primary_keys) - assert_not Family.respond_to?(:primary_keys) - - # OrderLineItem should support primary_keys method - assert_respond_to OrderLineItem, :primary_keys - assert_equal %w[order_id line_number], OrderLineItem.primary_keys - - # So analyzer should return nil for single-key models - [User, Vehicle, Family].each do |model| - result = CompositeKeys.new(model).analyze - - assert_nil result, "#{model.name} should not have composite key analysis" - end - - # And should return composite key info for OrderLineItem + def test_composite_primary_key_emits_toml_section result = CompositeKeys.new(OrderLineItem).analyze - assert_not_nil result, 'OrderLineItem should have composite key analysis' + assert_equal "[composite_pk]\nkeys = [\"order_id\", \"line_number\"]", result end - def test_analyze_composite_key_formatting - # Test that composite key info is properly formatted - analyzer = CompositeKeys.new(OrderLineItem) - result = analyzer.analyze - - # Should have proper section header - assert_match(/^== Composite Primary Key$/, result) - - # Should have proper key listing - assert_match(/^Primary Keys: order_id, line_number$/, result) + def test_detection_uses_native_rails_primary_key + assert_equal %w[order_id line_number], OrderLineItem.primary_key + assert_equal 'id', User.primary_key end - def test_analyze_handles_nil_primary_key - # Test with model that has nil primary key + def test_nil_primary_key_returns_nil User.stub(:primary_key, nil) do - analyzer = CompositeKeys.new(User) - result = analyzer.analyze - - assert_nil result, 'Model with nil primary_key should return nil' + assert_nil CompositeKeys.new(User).analyze end end - - def test_analyze_works_across_different_databases - # Test that composite key analysis works across different database adapters - - # PostgreSQL model with composite key - pg_result = CompositeKeys.new(OrderLineItem).analyze - - assert_not_nil pg_result, 'OrderLineItem (PostgreSQL) should have composite key analysis' - assert_match(/Primary Keys: order_id, line_number/, pg_result) - - # Other database models without composite keys - mysql_result = CompositeKeys.new(Vehicle).analyze # MySQL - sqlite_result = CompositeKeys.new(Family).analyze # SQLite - - assert_nil mysql_result, 'Vehicle (MySQL) should not have composite key analysis' - assert_nil sqlite_result, 'Family (SQLite) should not have composite key analysis' - end - - def test_analyze_real_world_composite_key_behavior - # Test with actual Rails composite key behavior - - # Verify OrderLineItem has proper composite key setup - assert_kind_of Array, OrderLineItem.primary_key, 'OrderLineItem primary_key should be an array' - assert_equal 2, OrderLineItem.primary_key.length, 'OrderLineItem should have 2 primary key columns' - - # Test the analyzer detects this correctly - analyzer = CompositeKeys.new(OrderLineItem) - result = analyzer.analyze - - assert_includes result, '== Composite Primary Key' - assert_includes result, 'Primary Keys: order_id, line_number' - end - - def test_analyze_composite_key_with_database_connection - # Test that the analyzer works with the actual database table - - # OrderLineItem should exist in the database - assert_predicate OrderLineItem, :table_exists?, 'OrderLineItem table should exist' - - # Should have the expected columns - expected_columns = %w[order_id line_number quantity unit_price total_price product_name notes created_at - updated_at] - actual_columns = OrderLineItem.column_names - - expected_columns.each do |col| - assert_includes actual_columns, col, "OrderLineItem should have #{col} column" - end - - # Analyzer should work with real database connection - analyzer = CompositeKeys.new(OrderLineItem) - result = analyzer.analyze - - assert_not_nil result - assert_match(/Composite Primary Key/, result) - end end end end diff --git a/test/rails_lens/schema/adapters/postgresql_test.rb b/test/rails_lens/schema/adapters/postgresql_test.rb index bfba7d0..d4bf477 100644 --- a/test/rails_lens/schema/adapters/postgresql_test.rb +++ b/test/rails_lens/schema/adapters/postgresql_test.rb @@ -81,9 +81,9 @@ def test_schema_search_path_handling # Verify primary_key extraction works assert_nothing_raised do - pk = adapter.send(:primary_key_name) + pks = adapter.send(:primary_key_names) - assert_equal 'id', pk + assert_equal ['id'], pks end end From 5cee5bcbc30f5b03cc40ecd6c48d2c9790cceb5c Mon Sep 17 00:00:00 2001 From: Abdelkader Boudih Date: Tue, 11 Aug 2026 00:04:44 +0100 Subject: [PATCH 3/5] feat(erd): composite key rendering, per-database grouping, outage resilience - determine_keys handles Array primary/foreign keys so composite PK columns get PK/FK markers - group_by_database option writes one erd_.mmd per connection; generate_erd command reports every generated file - extracted renderable? guard: a model whose connection is down is skipped instead of aborting the whole diagram --- lib/rails_lens/commands.rb | 6 +- lib/rails_lens/erd/visualizer.rb | 71 ++++++++++--------- .../multi_database_scenarios_test.rb | 32 +++++---- test/rails_lens/cli_test.rb | 4 +- test/rails_lens/erd/visualizer_test.rb | 26 +++++++ 5 files changed, 84 insertions(+), 55 deletions(-) diff --git a/lib/rails_lens/commands.rb b/lib/rails_lens/commands.rb index 0ee9e5a..921d428 100644 --- a/lib/rails_lens/commands.rb +++ b/lib/rails_lens/commands.rb @@ -59,9 +59,9 @@ def remove_mailers(options = {}) def generate_erd(options = {}) visualizer = ERD::Visualizer.new(options: options) - filename = visualizer.generate - output.say "Entity Relationship Diagram generated at #{filename}", :green - filename + filenames = Array(visualizer.generate) + filenames.each { |filename| output.say "Entity Relationship Diagram generated at #{filename}", :green } + filenames.length == 1 ? filenames.first : filenames end def lint(options = {}) diff --git a/lib/rails_lens/erd/visualizer.rb b/lib/rails_lens/erd/visualizer.rb index e93c0b1..a97be69 100644 --- a/lib/rails_lens/erd/visualizer.rb +++ b/lib/rails_lens/erd/visualizer.rb @@ -20,7 +20,13 @@ def initialize(options: {}) def generate models = load_models - generate_mermaid(models) + if config[:group_by_database] + group_models_by_database(models).map do |db_name, group| + generate_mermaid(group, basename: "erd_#{db_name}") + end + else + generate_mermaid(models) + end end private @@ -29,11 +35,15 @@ def load_models ModelDetector.detect_models(options) end - def generate_mermaid(models) + def group_models_by_database(models) + models.group_by { |model| model.connection_pool.db_config.name } + end + + def generate_mermaid(models, basename: 'erd') if models.blank? # Still need to save the output even if no models found mermaid_output = "erDiagram\n %% No models found" - return save_output(mermaid_output, 'mmd') + return save_output(mermaid_output, 'mmd', basename: basename) end # Create new ERDiagram using mermaid-ruby gem @@ -41,25 +51,15 @@ def generate_mermaid(models) # Process models and add them to the diagram models.each do |model| - # Skip abstract models - next if model.abstract_class? - - # Skip models without valid tables/views or columns - is_view = ModelDetector.view_exists?(model) - has_data_source = is_view || (model.table_exists? && model.columns.present?) - next unless has_data_source + next unless renderable?(model) begin # Create attributes for the entity - attributes = [] - model.columns.each do |column| - type_str = format_column_type(column) - keys = determine_keys(model, column) - - attributes << { - type: type_str, + attributes = model.columns.map do |column| + { + type: format_column_type(column), name: column.name, - keys: keys + keys: determine_keys(model, column) } end @@ -74,13 +74,6 @@ def generate_mermaid(models) RailsLens.logger.debug { "Warning: Could not add entity #{model.name}: #{e.message}" } end - # Add relationships - next if model.abstract_class? - - is_view = ModelDetector.view_exists?(model) - has_data_source = is_view || (model.table_exists? && model.columns.present?) - next unless has_data_source - add_model_relationships(diagram, model, models) end @@ -88,12 +81,23 @@ def generate_mermaid(models) mermaid_output = diagram.to_mermaid # Save output - filename = save_output(mermaid_output, 'mmd') + filename = save_output(mermaid_output, 'mmd', basename: basename) RailsLens.logger.debug 'ERD generated successfully!' filename # Return the filename instead of content end + # A model renders when it is concrete and its table/view is reachable; + # an unavailable connection just drops the model from the diagram. + def renderable?(model) + return false if model.abstract_class? + + ModelDetector.view_exists?(model) || (model.table_exists? && model.columns.present?) + rescue ActiveRecord::ActiveRecordError => e + RailsLens.logger.debug { "Skipping #{model.name}: #{e.message}" } + false + end + def format_column_type(column) formatter_class = case column.sql_type when /jsonb|uuid|inet|array|tsvector/i @@ -109,13 +113,10 @@ def format_column_type(column) def determine_keys(model, column) keys = [] - keys << :PK if column.name == model.primary_key - - # Check foreign keys - if model.respond_to?(:reflect_on_all_associations) - model.reflect_on_all_associations(:belongs_to).each do |assoc| - keys << :FK if assoc.foreign_key.to_s == column.name - end + # primary_key and foreign_key are arrays for composite keys + keys << :PK if Array(model.primary_key).include?(column.name) + keys << :FK if model.reflect_on_all_associations(:belongs_to).any? do |assoc| + Array(assoc.foreign_key).map(&:to_s).include?(column.name) end # Check unique indexes - use UK which will be automatically quoted as comment @@ -181,11 +182,11 @@ def add_association_relationship(diagram, model, association, target_model) end end - def save_output(content, extension) + def save_output(content, extension, basename: 'erd') output_dir = config[:output_dir] || 'doc/erd' FileUtils.mkdir_p(output_dir) - filename = File.join(output_dir, "erd.#{extension}") + filename = File.join(output_dir, "#{basename}.#{extension}") File.write(filename, content) RailsLens.logger.debug { "ERD saved to: #{filename}" } diff --git a/test/integration/multi_database_scenarios_test.rb b/test/integration/multi_database_scenarios_test.rb index 18f19d2..39e0807 100644 --- a/test/integration/multi_database_scenarios_test.rb +++ b/test/integration/multi_database_scenarios_test.rb @@ -25,13 +25,11 @@ def test_complete_application_annotation # Get all models grouped by database models_by_database = { - 'primary' => [], - 'vehicles' => [], - 'prehistoric' => [] + 'primary' => [User, Post, Comment, Product, Spaceship, CrewMember], + 'vehicles' => [Vehicle, Manufacturer, Owner, Trip], + 'prehistoric' => [Dinosaur, Species, Family, ExcavationSite] } - skip 'Skipping due to VehicleRecord connection issues in CI' - # Annotate each database's models models_by_database.each do |db_name, models| models.each do |model| @@ -96,14 +94,18 @@ def test_database_fallback_and_recovery # Test handling when one database is temporarily unavailable # This tests resilience in multi-database environments - # Store original connection info - original_config = Vehicle.connection_db_config.configuration_hash.dup - result = nil + # Load every model while the connection is still up: lazily loading one + # inside the outage window (closure_tree reads columns at class load) + # would fail for the wrong reason. + Rails.application.eager_load! + begin - # Simulate database unavailability by removing connection - Vehicle.connection_handler.remove_connection_pool(Vehicle.connection_specification_name) + # Simulate database unavailability by removing the pool from its owner: + # Vehicle borrows VehicleRecord's connection, so remove (and later + # restore) on VehicleRecord or every sibling model stays disconnected. + VehicleRecord.connection_handler.remove_connection_pool(VehicleRecord.connection_specification_name) # Try to generate ERD with one database down visualizer = RailsLens::ERD::Visualizer.new( @@ -126,8 +128,8 @@ def test_database_fallback_and_recovery assert_match(/User|Post|Comment/, result) # Primary database assert_match(/Dinosaur|Species/, result) # Prehistoric database ensure - # Restore connection - Vehicle.establish_connection(original_config) + # Restore connection on the owner, as test_helper set it up + VehicleRecord.establish_connection(:vehicles) end end @@ -177,8 +179,6 @@ def test_cross_database_data_integrity_checks } end end - - skip 'Skipping due to VehicleRecord connection issues in CI' end def test_bulk_operations_across_databases @@ -334,7 +334,9 @@ def test_schema_dump_across_databases schema_info[db_name][:version] = versions.last end - skip 'Skipping due to VehicleRecord connection issues in CI' + config[:models].each do |model| + schema_info[db_name][:tables][model.table_name] = model.column_names + end end # Verify we collected schema info from all databases diff --git a/test/rails_lens/cli_test.rb b/test/rails_lens/cli_test.rb index 162c3f0..a967a5f 100644 --- a/test/rails_lens/cli_test.rb +++ b/test/rails_lens/cli_test.rb @@ -268,8 +268,8 @@ def test_erd_command @cli.erd output = @stdout.string - # Verify the options were passed correctly - assert_equal 'output', captured_options[:options][:output_dir] + # No --output flag: output_dir is absent, the Visualizer default applies + assert_nil captured_options[:options][:output_dir] assert_match(/Entity Relationship Diagram generated at/, output) assert_match(%r{output/erd.mmd}, output) diff --git a/test/rails_lens/erd/visualizer_test.rb b/test/rails_lens/erd/visualizer_test.rb index 85444f1..94669f7 100644 --- a/test/rails_lens/erd/visualizer_test.rb +++ b/test/rails_lens/erd/visualizer_test.rb @@ -24,6 +24,32 @@ def test_visualizer_with_custom_options assert_equal 'LR', visualizer.config[:orientation] end + def test_determine_keys_marks_composite_primary_key_columns + pk_columns = %w[order_id line_number] + + OrderLineItem.columns.each do |column| + keys = @visualizer.send(:determine_keys, OrderLineItem, column) + + if pk_columns.include?(column.name) + assert_includes keys, :PK, "#{column.name} should be marked PK" + else + assert_not_includes keys, :PK, "#{column.name} should not be marked PK" + end + end + end + + def test_determine_keys_marks_single_primary_key_column + id = User.columns_hash['id'] + + assert_includes @visualizer.send(:determine_keys, User, id), :PK + end + + def test_determine_keys_marks_belongs_to_foreign_keys + user_id = Post.columns_hash['user_id'] + + assert_includes @visualizer.send(:determine_keys, Post, user_id), :FK + end + def test_group_by_database_option visualizer = RailsLens::ERD::Visualizer.new( options: { group_by_database: true } From 67992f8da2a4e86a931f5f3ebed5f514a182f831 Mon Sep 17 00:00:00 2001 From: Abdelkader Boudih Date: Tue, 11 Aug 2026 00:04:50 +0100 Subject: [PATCH 4/5] fix: idempotent annotation writes, ErrorReporter config access, test harness - route/mailer annotators skip writing when file content is unchanged (new annotation_idempotence_test covers schema/route/mailer) - ErrorReporter read RailsLens.verbose/debug/raise_on_error which do not exist on the module; go through RailsLens.config - expose RailsLens.config_file for the loaded config path - Rakefile runs each test file in its own process: `ruby a.rb b.rb` only executes the first file, so most of the suite never ran - fix stale test assertions (TOML sections, NoteCodes, mermaid relationship lines) and un-skip the multi-database annotation test --- Rakefile | 13 +- lib/rails_lens.rb | 4 + lib/rails_lens/errors.rb | 6 +- lib/rails_lens/mailer/annotator.rb | 1 + lib/rails_lens/route/annotator.rb | 1 + .../cross_database_relationships_test.rb | 31 +-- ...ulti_database_annotation_rake_task_test.rb | 2 - .../trigger_function_annotation_test.rb | 4 +- .../analyzers/best_practices_analyzer_test.rb | 210 ++++-------------- test/rails_lens/analyzers/inheritance_test.rb | 4 + .../rails_lens/annotation_idempotence_test.rb | 37 +++ test/rails_lens/error_handling_test.rb | 24 +- 12 files changed, 131 insertions(+), 206 deletions(-) create mode 100644 test/rails_lens/annotation_idempotence_test.rb diff --git a/Rakefile b/Rakefile index 7e738c8..13e7589 100644 --- a/Rakefile +++ b/Rakefile @@ -4,13 +4,16 @@ require 'bundler/gem_tasks' desc 'Run all tests' task :test do # rubocop:disable Rails/RakeEnvironment - test_files = Dir['test/**/*_test.rb'].reject { |f| f.include?('test/dummy/') } + # One process per file: `ruby file1 file2` only executes file1 (the rest + # become ARGV), and the multi-database tests assume a fresh boot anyway. + test_files = Dir['test/**/*_test.rb'].reject { |f| f.include?('test/dummy/') }.sort - # Run minitest directly to avoid rake test runner issues - cmd = "ruby -Ilib:test #{test_files.join(' ')}" + failed = test_files.reject do |file| + puts "== #{file}" + system(Gem.ruby, '-Ilib:test', file) + end - puts "Running tests with: #{cmd}" - system(cmd) || exit(1) + abort "\n#{failed.length} test file(s) failed:\n#{failed.join("\n")}" if failed.any? end task default: :test diff --git a/lib/rails_lens.rb b/lib/rails_lens.rb index d5c8c11..9a077f9 100644 --- a/lib/rails_lens.rb +++ b/lib/rails_lens.rb @@ -31,6 +31,9 @@ module RailsLens extend Configuration class << self + # Path of the loaded config file, nil when running on defaults. + attr_reader :config_file + def logger @logger ||= config.logger || default_logger end @@ -52,6 +55,7 @@ def default_logger def load_config_file(path = '.rails-lens.yml') return unless File.exist?(path) + @config_file = path yaml = YAML.load_file(path) yaml.each do |section, settings| diff --git a/lib/rails_lens/errors.rb b/lib/rails_lens/errors.rb index f4b0ea2..dd64689 100644 --- a/lib/rails_lens/errors.rb +++ b/lib/rails_lens/errors.rb @@ -42,7 +42,7 @@ class VisualizationError < ERDError; end class ErrorReporter class << self def report(error, context = {}) - return unless RailsLens.verbose || RailsLens.debug + return unless RailsLens.config.verbose || RailsLens.config.debug message = build_error_message(error, context) @@ -50,7 +50,7 @@ def report(error, context = {}) RailsLens.logger.error message # Use kernel output for debug mode to ensure visibility - return unless RailsLens.debug + return unless RailsLens.config.debug RailsLens.logger.debug message end @@ -59,7 +59,7 @@ def handle(context = {}) yield rescue StandardError => e report(e, context) - raise if RailsLens.raise_on_error + raise if RailsLens.config.raise_on_error nil end diff --git a/lib/rails_lens/mailer/annotator.rb b/lib/rails_lens/mailer/annotator.rb index 28f63cb..8f6852b 100644 --- a/lib/rails_lens/mailer/annotator.rb +++ b/lib/rails_lens/mailer/annotator.rb @@ -201,6 +201,7 @@ def extract_formats_from_templates(templates) # @return [void] def write_content_to_file(path:, content:) return if @dry_run + return if File.exist?(path) && File.read(path) == content File.write(path, content) @changed_files << path diff --git a/lib/rails_lens/route/annotator.rb b/lib/rails_lens/route/annotator.rb index c1db947..a66140f 100644 --- a/lib/rails_lens/route/annotator.rb +++ b/lib/rails_lens/route/annotator.rb @@ -243,6 +243,7 @@ def write_to_file(path:, parsed_file:) # @return [void] def write_content_to_file(path:, content:) return if @dry_run + return if File.exist?(path) && File.read(path) == content File.write(path, content) @changed_files << path diff --git a/test/integration/cross_database_relationships_test.rb b/test/integration/cross_database_relationships_test.rb index cd50955..c259888 100644 --- a/test/integration/cross_database_relationships_test.rb +++ b/test/integration/cross_database_relationships_test.rb @@ -51,9 +51,10 @@ def test_polymorphic_associations_across_databases assert_includes annotation, 'name = "commentable_type"' assert_includes annotation, 'name = "commentable_id"' - # Check for polymorphic association note - assert_includes annotation, '== Polymorphic Associations' - assert_includes annotation, '- commentable (commentable_type/commentable_id)' + # Check for polymorphic association section (TOML format) + assert_includes annotation, '[polymorphic]' + assert_includes annotation, + 'references = [{ name = "commentable", type_col = "commentable_type", id_col = "commentable_id" }]' end def test_erd_with_join_tables_across_databases @@ -73,11 +74,11 @@ def test_erd_with_join_tables_across_databases assert_includes output, 'SpaceshipCrewMember' assert_includes output, 'VehicleOwner' - # Check relationships are properly mapped - assert_match(/Spaceship.*SpaceshipCrewMember/, output) - assert_match(/CrewMember.*SpaceshipCrewMember/, output) - assert_match(/Vehicle.*VehicleOwner/, output) - assert_match(/Owner.*VehicleOwner/, output) + # Check relationships are properly mapped (mermaid relationship lines) + assert_match(/"SpaceshipCrewMember" \}o--\|\| "Spaceship" : "spaceship"/, output) + assert_match(/"SpaceshipCrewMember" \}o--\|\| "CrewMember" : "crew_member"/, output) + assert_match(/"VehicleOwner" \}o--\|\| "Vehicle" : "vehicle"/, output) + assert_match(/"VehicleOwner" \}o--\|\| "Owner" : "owner"/, output) end def test_connection_metadata_in_relationships @@ -93,16 +94,16 @@ def test_connection_metadata_in_relationships manager = RailsLens::Schema::AnnotationManager.new(model) annotation = manager.generate_annotation - # Should include relationship information in notes - # Note: belongs_to associations get counter cache suggestions, has_many get N+1 warnings + # Should include relationship information in notes (NoteCodes format: + # belongs_to gets COUNTER_CACHE suggestions, has_many gets N_PLUS_ONE) if model == Post - assert_includes annotation, "Consider adding counter cache for 'user'" - assert_includes annotation, "Association 'comments' has N+1 query risk" + assert_includes annotation, 'user:COUNTER_CACHE' + assert_includes annotation, 'comments:N_PLUS_ONE' elsif model == Vehicle - assert_includes annotation, "Consider adding counter cache for 'manufacturer'" - assert_includes annotation, "Association 'owners' has N+1 query risk" + assert_includes annotation, 'manufacturer:COUNTER_CACHE' + assert_includes annotation, 'owners:N_PLUS_ONE' elsif model == Dinosaur - assert_includes annotation, "Association 'fossil_discoveries' has N+1 query risk" + assert_includes annotation, 'fossil_discoveries:N_PLUS_ONE' end # Get the proper dialect name diff --git a/test/integration/multi_database_annotation_rake_task_test.rb b/test/integration/multi_database_annotation_rake_task_test.rb index 6aa0c65..5d35e9c 100644 --- a/test/integration/multi_database_annotation_rake_task_test.rb +++ b/test/integration/multi_database_annotation_rake_task_test.rb @@ -41,8 +41,6 @@ class MultiDatabaseAnnotationRakeTaskTest < ActiveSupport::TestCase FileUtils.rm_rf(@temp_dir) if @temp_dir && File.exist?(@temp_dir) end - private - def remove_existing_annotations(content) # Remove rails-lens annotation blocks content.gsub(/^# .*?^# \n/m, '') diff --git a/test/integration/trigger_function_annotation_test.rb b/test/integration/trigger_function_annotation_test.rb index 27a709a..25a5b63 100644 --- a/test/integration/trigger_function_annotation_test.rb +++ b/test/integration/trigger_function_annotation_test.rb @@ -39,8 +39,8 @@ def test_application_record_includes_function_annotations annotator = RailsLens::Schema::DatabaseAnnotator.new(ApplicationRecord) annotation = annotator.generate_annotation - # Verify functions section exists - assert_includes annotation, '== Database Functions' + # Verify functions section exists (TOML format) + assert_includes annotation, '[database_functions]' assert_includes annotation, 'functions = [' # Verify the trigger function from migration diff --git a/test/rails_lens/analyzers/best_practices_analyzer_test.rb b/test/rails_lens/analyzers/best_practices_analyzer_test.rb index 0241e47..840163e 100644 --- a/test/rails_lens/analyzers/best_practices_analyzer_test.rb +++ b/test/rails_lens/analyzers/best_practices_analyzer_test.rb @@ -15,7 +15,7 @@ def initialize(table_name, columns = [], connection = nil) @connection = connection || MockConnection.new end - def self.base_class + def base_class self end @@ -26,14 +26,21 @@ def column_names # Mock connection for testing class MockConnection + def initialize(indexes = []) + @indexes = indexes + end + def indexes(_table_name) - [] + @indexes end end # Mock column for testing MockColumn = Struct.new(:name, :type, :null, :default, keyword_init: true) + # Mock index for testing + MockIndex = Struct.new(:columns, :unique, keyword_init: true) + def setup @timestamp_columns = [ MockColumn.new(name: 'id', type: :integer, null: false), @@ -42,211 +49,80 @@ def setup ] end - def test_schema_qualified_table_name_plural - # Test that schema-qualified plural table names pass validation - model = MockModel.new('ai.skills', @timestamp_columns) - analyzer = BestPracticesAnalyzer.new(model) - - notes = analyzer.analyze - - # Should not complain about plural, snake_case for "skills" - assert_not_includes notes, "Table name 'ai.skills' doesn't follow Rails conventions (should be plural, snake_case)" - end - - def test_schema_qualified_table_name_singular - # Test that schema-qualified singular table names are flagged - model = MockModel.new('cms.post', @timestamp_columns) - analyzer = BestPracticesAnalyzer.new(model) - - notes = analyzer.analyze - - # Should complain about singular "post" (should be "posts") - assert_includes notes, "Table name 'cms.post' doesn't follow Rails conventions (should be plural, snake_case)" - end - - def test_regular_table_name_plural - # Test that regular plural table names pass validation - model = MockModel.new('products', @timestamp_columns) - analyzer = BestPracticesAnalyzer.new(model) - - notes = analyzer.analyze - - assert_not_includes notes, "Table name 'products' doesn't follow Rails conventions (should be plural, snake_case)" - end - - def test_regular_table_name_singular - # Test that regular singular table names are flagged - model = MockModel.new('product', @timestamp_columns) - analyzer = BestPracticesAnalyzer.new(model) - - notes = analyzer.analyze - - assert_includes notes, "Table name 'product' doesn't follow Rails conventions (should be plural, snake_case)" - end - - def test_table_name_with_camelcase - # Test that CamelCase table names are flagged - model = MockModel.new('UserProfiles', @timestamp_columns) - analyzer = BestPracticesAnalyzer.new(model) - - notes = analyzer.analyze - - assert_includes notes, "Table name 'UserProfiles' doesn't follow Rails conventions (should be plural, snake_case)" - end - - def test_schema_qualified_table_name_with_camelcase - # Test that schema-qualified CamelCase table names are flagged - model = MockModel.new('auth.UserTokens', @timestamp_columns) - analyzer = BestPracticesAnalyzer.new(model) - - notes = analyzer.analyze + def test_timestamps_present_are_not_flagged + notes = BestPracticesAnalyzer.new(MockModel.new('users', @timestamp_columns)).analyze - assert_includes notes, "Table name 'auth.UserTokens' doesn't follow Rails conventions (should be plural, snake_case)" + assert_not_includes notes, NoteCodes::NO_TIMESTAMPS + assert_not_includes notes, NoteCodes::PARTIAL_TS end - def test_timestamp_columns_present - # Test that models with both timestamps don't get flagged - model = MockModel.new('users', @timestamp_columns) - analyzer = BestPracticesAnalyzer.new(model) - - notes = analyzer.analyze - - assert_not_includes notes, 'Missing timestamp columns (created_at, updated_at)' - end - - def test_timestamp_columns_missing - # Test that models without timestamps get flagged + def test_missing_timestamps_are_flagged columns = [ MockColumn.new(name: 'id', type: :integer, null: false), MockColumn.new(name: 'name', type: :string, null: true) ] - model = MockModel.new('users', columns) - analyzer = BestPracticesAnalyzer.new(model) - - notes = analyzer.analyze + notes = BestPracticesAnalyzer.new(MockModel.new('users', columns)).analyze - assert_includes notes, 'Missing timestamp columns (created_at, updated_at)' + assert_includes notes, NoteCodes::NO_TIMESTAMPS end - def test_partial_timestamp_columns - # Test that models with partial timestamps get flagged + def test_partial_timestamps_are_flagged columns = [ MockColumn.new(name: 'id', type: :integer, null: false), MockColumn.new(name: 'created_at', type: :datetime, null: false) ] - model = MockModel.new('users', columns) - analyzer = BestPracticesAnalyzer.new(model) - - notes = analyzer.analyze + notes = BestPracticesAnalyzer.new(MockModel.new('users', columns)).analyze - assert_includes notes, 'Has created_at but missing updated_at' + assert_includes notes, NoteCodes::PARTIAL_TS end - def test_column_with_is_prefix - # Test that columns with is_ prefix get flagged + def test_unindexed_soft_delete_column_is_flagged columns = @timestamp_columns + [ - MockColumn.new(name: 'is_active', type: :boolean, null: true) + MockColumn.new(name: 'deleted_at', type: :datetime, null: true) ] - model = MockModel.new('users', columns) - analyzer = BestPracticesAnalyzer.new(model) - - notes = analyzer.analyze + notes = BestPracticesAnalyzer.new(MockModel.new('users', columns)).analyze - assert_includes notes, "Column 'is_active' uses non-conventional prefix - consider removing 'is_' or 'has_'" + assert_includes notes, 'deleted_at:INDEX' end - def test_column_with_has_prefix - # Test that columns with has_ prefix get flagged + def test_indexed_soft_delete_column_is_not_flagged columns = @timestamp_columns + [ - MockColumn.new(name: 'has_profile', type: :boolean, null: true) + MockColumn.new(name: 'deleted_at', type: :datetime, null: true) ] - model = MockModel.new('users', columns) - analyzer = BestPracticesAnalyzer.new(model) + connection = MockConnection.new([MockIndex.new(columns: ['deleted_at'], unique: false)]) + notes = BestPracticesAnalyzer.new(MockModel.new('users', columns, connection)).analyze - notes = analyzer.analyze - - assert_includes notes, "Column 'has_profile' uses non-conventional prefix - consider removing 'is_' or 'has_'" + assert_not_includes notes, 'deleted_at:INDEX' end - def test_column_with_camelcase - # Test that CamelCase column names get flagged + def test_unindexed_nullable_sti_type_column_is_flagged columns = @timestamp_columns + [ - MockColumn.new(name: 'userId', type: :integer, null: true) + MockColumn.new(name: 'type', type: :string, null: true) ] - model = MockModel.new('users', columns) - analyzer = BestPracticesAnalyzer.new(model) - - notes = analyzer.analyze + notes = BestPracticesAnalyzer.new(MockModel.new('users', columns)).analyze - assert_includes notes, "Column 'userId' should use snake_case (e.g., 'user_id')" + assert_includes notes, 'type:INDEX' + assert_includes notes, 'type:STI_NOT_NULL' end - def test_soft_delete_column_without_index - # Test soft delete columns are checked for indexes + def test_indexed_not_null_sti_type_column_is_not_flagged columns = @timestamp_columns + [ - MockColumn.new(name: 'deleted_at', type: :datetime, null: true) + MockColumn.new(name: 'type', type: :string, null: false) ] - model = MockModel.new('users', columns) - analyzer = BestPracticesAnalyzer.new(model) - - notes = analyzer.analyze + connection = MockConnection.new([MockIndex.new(columns: ['type'], unique: false)]) + notes = BestPracticesAnalyzer.new(MockModel.new('users', columns, connection)).analyze - assert_includes notes, "Soft delete column 'deleted_at' should be indexed" + assert_not_includes notes, 'type:INDEX' + assert_not_includes notes, 'type:STI_NOT_NULL' end - def test_sti_type_column - # Test STI type column handling + def test_text_columns_get_storage_note columns = @timestamp_columns + [ - MockColumn.new(name: 'type', type: :string, null: true) + MockColumn.new(name: 'body', type: :text, null: true) ] - model = MockModel.new('users', columns) - analyzer = BestPracticesAnalyzer.new(model) - - notes = analyzer.analyze - - assert_includes notes, "STI type column 'type' should be indexed" - assert_includes notes, "STI type column 'type' should have NOT NULL constraint" - end + notes = BestPracticesAnalyzer.new(MockModel.new('users', columns)).analyze - # Integration test with real database models if available - def test_with_actual_models_if_available - skip 'Requires database connection' unless defined?(User) && User.connected? - - # Test with a real model if available - analyzer = BestPracticesAnalyzer.new(User) - notes = analyzer.analyze - - # Should return an array of notes (may be empty) - assert_kind_of Array, notes - end - - def test_schema_qualified_with_special_pluralization - # Test special pluralization cases with schema-qualified names - # PostgreSQL format is schema.table (e.g., public.users, ai.skills) - test_cases = { - 'ai.skill' => true, # singular, should be flagged (should be skills) - 'ai.skills' => false, # plural, correct - 'cms.category' => true, # singular, should be flagged (should be categories) - 'cms.categories' => false, # plural, correct - 'auth.person' => true, # singular, should be flagged (should be people) - 'auth.people' => false, # plural, correct (irregular) - 'analytics.datum' => true, # singular, should be flagged (should be data) - 'analytics.data' => false # plural, correct (irregular) - } - - test_cases.each do |table_name, should_flag| - model = MockModel.new(table_name, @timestamp_columns) - analyzer = BestPracticesAnalyzer.new(model) - notes = analyzer.analyze - - if should_flag - assert_includes notes, "Table name '#{table_name}' doesn't follow Rails conventions (should be plural, snake_case)", - "Expected #{table_name} to be flagged as non-conventional" - else - assert_not_includes notes, "Table name '#{table_name}' doesn't follow Rails conventions (should be plural, snake_case)", - "Expected #{table_name} to pass validation" - end - end + assert_includes notes, 'body:STORAGE' end end end diff --git a/test/rails_lens/analyzers/inheritance_test.rb b/test/rails_lens/analyzers/inheritance_test.rb index 202137d..2c47584 100644 --- a/test/rails_lens/analyzers/inheritance_test.rb +++ b/test/rails_lens/analyzers/inheritance_test.rb @@ -6,6 +6,10 @@ module RailsLens module Analyzers class InheritanceTest < ActiveSupport::TestCase def test_analyze_with_spaceship_sti_base_class + # Zeitwerk lazy-loads models: reference the subclasses so the + # analyzer can see them regardless of test order. + [CargoVessel, StarfleetBattleCruiser] + # Spaceship is the STI base class with type column analyzer = Inheritance.new(Spaceship) result = analyzer.analyze diff --git a/test/rails_lens/annotation_idempotence_test.rb b/test/rails_lens/annotation_idempotence_test.rb new file mode 100644 index 0000000..9d9a218 --- /dev/null +++ b/test/rails_lens/annotation_idempotence_test.rb @@ -0,0 +1,37 @@ +# frozen_string_literal: true + +require 'test_helper' + +module RailsLens + # Annotating twice must be a no-op the second time: unchanged models are + # reported as skipped and their files are not rewritten. + class AnnotationIdempotenceTest < ActiveSupport::TestCase + def test_second_pass_rewrites_nothing + models = [User, Post, Comment] + + Schema::AnnotationManager.annotate_all(models: models.map(&:name)) + + files = models.map { |m| Rails.root.join('app', 'models', "#{m.name.underscore}.rb").to_s } + before = files.index_with { |f| [File.read(f), File.mtime(f)] } + + results = Schema::AnnotationManager.annotate_all(models: models.map(&:name)) + + models.each do |model| + assert_not_includes results[:annotated], model.name, + "#{model.name} was rewritten although nothing changed" + end + files.each do |f| + assert_equal before[f], [File.read(f), File.mtime(f)], "#{f} was touched on a no-op pass" + end + end + + def test_annotate_file_returns_false_when_unchanged + manager = Schema::AnnotationManager.new(User) + path = Rails.root.join('app/models/user.rb').to_s + + manager.annotate_file(path) + + assert_not manager.annotate_file(path) + end + end +end diff --git a/test/rails_lens/error_handling_test.rb b/test/rails_lens/error_handling_test.rb index db9da84..640c3e1 100644 --- a/test/rails_lens/error_handling_test.rb +++ b/test/rails_lens/error_handling_test.rb @@ -8,18 +8,18 @@ module RailsLens class ErrorHandlingTest < ActiveSupport::TestCase def setup - @original_verbose = RailsLens.verbose - @original_debug = RailsLens.debug - @original_raise_on_error = RailsLens.raise_on_error - RailsLens.verbose = true - RailsLens.debug = false - RailsLens.raise_on_error = false + @original_verbose = RailsLens.config.verbose + @original_debug = RailsLens.config.debug + @original_raise_on_error = RailsLens.config.raise_on_error + RailsLens.config.verbose = true + RailsLens.config.debug = false + RailsLens.config.raise_on_error = false end def teardown - RailsLens.verbose = @original_verbose - RailsLens.debug = @original_debug - RailsLens.raise_on_error = @original_raise_on_error + RailsLens.config.verbose = @original_verbose + RailsLens.config.debug = @original_debug + RailsLens.config.raise_on_error = @original_raise_on_error end def test_error_reporter_logs_with_context @@ -31,7 +31,7 @@ def test_error_reporter_logs_with_context real_logger = Logger.new(log_output) real_logger.level = Logger::ERROR - Rails.stub(:logger, real_logger) do + RailsLens.stub(:logger, real_logger) do RailsLens::ErrorReporter.report(error, context) end @@ -57,7 +57,7 @@ def test_error_reporter_handle_method end def test_error_reporter_handle_method_with_raise_on_error - RailsLens.raise_on_error = true + RailsLens.config.raise_on_error = true assert_raises(StandardError) do RailsLens::ErrorReporter.handle({}) do @@ -65,7 +65,7 @@ def test_error_reporter_handle_method_with_raise_on_error end end ensure - RailsLens.raise_on_error = false + RailsLens.config.raise_on_error = false end def test_custom_error_hierarchy From b660f8843e2e8a19a48da63ec593a653eabf51ed Mon Sep 17 00:00:00 2001 From: Abdelkader Boudih Date: Tue, 11 Aug 2026 00:29:47 +0100 Subject: [PATCH 5/5] fix!: require Rails 8.0+, harden ERD generation, test against edge MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The old Rakefile only executed the first test file, so the full suite had never run on CI. Running it everywhere surfaced Rails 7.2 incompatibilities (ActiveRecord::ConnectionNotDefined, activerecord- postgis, PostGIS schema DSL); drop 7.2 instead of shimming around it. - gemspec: activerecord/railties >= 8.0.0 - CI matrix: 8.0, 8.1, edge (rails/rails main, future 8.2, non-blocking); Gemfile supports RAILS_VERSION=edge - CI: db:schema:load instead of no-op db:migrate — there are no migration files and only Rails >= 8.1 test_help loads the per-database schema dumps implicitly - ERD: an entity that fails to be added no longer crashes relationship generation; relationships only reference entities present in the diagram - ColumnTypeFormatter falls back to sql_type/'unknown' when column.type is nil (unresolvable custom types) - eager-reference lazy-loaded models in the abstract-class test so .descendants is populated regardless of test order BREAKING CHANGE: Rails 7.2 is no longer supported; require Rails 8.0+. --- .github/workflows/ci.yml | 11 ++++++++--- Gemfile | 17 ++++++++++++----- README.md | 2 +- lib/rails_lens/erd/column_type_formatter.rb | 7 ++++++- lib/rails_lens/erd/visualizer.rb | 9 +++++---- rails_lens.gemspec | 8 ++++---- test/dummy/app/models/order_line_item.rb | 2 +- test/dummy/app/models/post.rb | 4 ++-- test/dummy/app/models/product.rb | 4 ++-- test/dummy/app/models/product_metric.rb | 6 +++--- test/dummy/app/models/tenant_setting.rb | 2 +- test/dummy/app/models/vehicle.rb | 4 ++-- .../app/models/vehicle_performance_metrics.rb | 10 +++++----- .../abstract_class_and_connection_test.rb | 4 ++++ 14 files changed, 56 insertions(+), 34 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 00bcc54..83b20f8 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -28,7 +28,10 @@ jobs: strategy: fail-fast: false matrix: - rails: ['7.2', '8.0', '8.1'] + rails: ['8.0', '8.1', 'edge'] + + # Edge (rails/rails main, future 8.2) is informational, not blocking + continue-on-error: ${{ matrix.rails == 'edge' }} services: postgres: @@ -102,9 +105,11 @@ jobs: echo 'Waiting for MySQL...' sleep 2 done - # Create and migrate all databases in multi-database setup + # Create all databases and load the per-database schema dumps. + # There are no migration files, so db:migrate is a no-op; only + # Rails >= 8.1 test_help loads the schemas implicitly. bundle exec rails db:create:all RAILS_ENV=test - bundle exec rails db:migrate RAILS_ENV=test + bundle exec rails db:schema:load RAILS_ENV=test - name: Run tests run: bundle exec rake test diff --git a/Gemfile b/Gemfile index 735601b..e04f8a6 100644 --- a/Gemfile +++ b/Gemfile @@ -11,18 +11,25 @@ gem 'rake', '~> 13.0' gem 'minitest', '~> 5.17' gem 'minitest-reporters', '~> 1.6' -# Support testing against different Rails versions -if ENV['RAILS_VERSION'] +# Support testing against different Rails versions ('edge' = rails/rails main) +# rubocop:disable Bundler/DuplicatedGem -- branches are mutually exclusive +if ENV['RAILS_VERSION'] == 'edge' + git 'https://github.com/rails/rails.git', branch: 'main' do + gem 'actionmailer' + gem 'activerecord' + gem 'railties' + end +elsif ENV['RAILS_VERSION'] rails_version = ENV['RAILS_VERSION'] gem 'actionmailer', "~> #{rails_version}.0" gem 'activerecord', "~> #{rails_version}.0" gem 'railties', "~> #{rails_version}.0" else - gem 'actionmailer', '>= 7.2.0' + gem 'actionmailer', '>= 8.0.0' end +# rubocop:enable Bundler/DuplicatedGem -# PostGIS adapter only supports Rails 8+ -gem 'activerecord-postgis' if !ENV['RAILS_VERSION'] || ENV['RAILS_VERSION'].to_i >= 8 +gem 'activerecord-postgis' gem 'closure_tree' gem 'dotenv', '~> 3.0' gem 'rubocop', '~> 1.66' diff --git a/README.md b/README.md index de2abcf..6ba34be 100644 --- a/README.md +++ b/README.md @@ -120,7 +120,7 @@ erDiagram ## Requirements - Ruby >= 3.4.0 -- Rails >= 7.2.0 +- Rails >= 8.0.0 ## Installation diff --git a/lib/rails_lens/erd/column_type_formatter.rb b/lib/rails_lens/erd/column_type_formatter.rb index 650ac45..e8b1a3d 100644 --- a/lib/rails_lens/erd/column_type_formatter.rb +++ b/lib/rails_lens/erd/column_type_formatter.rb @@ -24,7 +24,12 @@ def format when :binary then 'blob' when :json, :jsonb then 'json' else - @column.type.to_s + # column.type is nil for types the adapter can't resolve (e.g. a + # PostGIS geometry column without activerecord-postgis loaded); + # the diagram gem rejects empty attribute types. + fallback = @column.type.to_s + fallback = @column.sql_type.to_s if fallback.empty? + fallback.empty? ? 'unknown' : fallback end end end diff --git a/lib/rails_lens/erd/visualizer.rb b/lib/rails_lens/erd/visualizer.rb index a97be69..d070315 100644 --- a/lib/rails_lens/erd/visualizer.rb +++ b/lib/rails_lens/erd/visualizer.rb @@ -74,7 +74,8 @@ def generate_mermaid(models, basename: 'erd') RailsLens.logger.debug { "Warning: Could not add entity #{model.name}: #{e.message}" } end - add_model_relationships(diagram, model, models) + # Relationships require both entities to exist in the diagram + add_model_relationships(diagram, model, models) if diagram.entities.key?(model.name) end # Generate mermaid syntax using the gem @@ -144,9 +145,9 @@ def add_model_relationships(diagram, model, models) next unless target_model && models.include?(target_model) - # Skip relationships to abstract models - next if target_model.abstract_class? - next unless target_model.table_exists? && target_model.columns.present? + # Skip targets whose entity was not added to the diagram + # (abstract, unreachable connection, or add_entity failed) + next unless diagram.entities.key?(target_model.name) add_association_relationship(diagram, model, association, target_model) end diff --git a/rails_lens.gemspec b/rails_lens.gemspec index a6804a5..ac73415 100644 --- a/rails_lens.gemspec +++ b/rails_lens.gemspec @@ -9,7 +9,7 @@ Gem::Specification.new do |spec| spec.email = ['terminale@gmail.com'] spec.summary = 'Comprehensive Rails application visualization and annotation' - spec.description = 'Rails Lens provides unified visualization and annotation for Rails 7.2+ applications, ' \ + spec.description = 'Rails Lens provides unified visualization and annotation for Rails 8.0+ applications, ' \ 'integrating ERD generation and model annotations.' spec.homepage = 'https://github.com/seuros/rails_lens' spec.license = 'MIT' @@ -27,8 +27,8 @@ Gem::Specification.new do |spec| spec.require_paths = ['lib'] # Rails dependencies - spec.add_dependency 'activerecord', '>= 7.2.0' - spec.add_dependency 'railties', '>= 7.2.0' + spec.add_dependency 'activerecord', '>= 8.0.0' + spec.add_dependency 'railties', '>= 8.0.0' # CLI and utilities spec.add_dependency 'ostruct' @@ -36,7 +36,7 @@ Gem::Specification.new do |spec| spec.add_dependency 'zeitwerk', '~> 2.7' # Development dependencies - spec.add_development_dependency 'actionmailer', '>= 7.2.0' + spec.add_development_dependency 'actionmailer', '>= 8.0.0' spec.add_development_dependency 'dotenv', '~> 3.0' spec.add_development_dependency 'mysql2', '~> 0.5' spec.add_development_dependency 'pg', '~> 1.5' diff --git a/test/dummy/app/models/order_line_item.rb b/test/dummy/app/models/order_line_item.rb index 43521d9..2937997 100644 --- a/test/dummy/app/models/order_line_item.rb +++ b/test/dummy/app/models/order_line_item.rb @@ -7,7 +7,7 @@ # columns = [ # { name = "order_id", type = "integer", pk = true, null = false }, # { name = "line_number", type = "integer", pk = true, null = false }, -# { name = "quantity", type = "integer", null = false, default = "1" }, +# { name = "quantity", type = "integer", null = false, default = 1 }, # { name = "unit_price", type = "decimal", null = false }, # { name = "total_price", type = "decimal" }, # { name = "product_name", type = "string" }, diff --git a/test/dummy/app/models/post.rb b/test/dummy/app/models/post.rb index 313af6c..2190327 100644 --- a/test/dummy/app/models/post.rb +++ b/test/dummy/app/models/post.rb @@ -9,10 +9,10 @@ # { name = "title", type = "string", null = false }, # { name = "content", type = "text" }, # { name = "user_id", type = "integer", null = false }, -# { name = "published", type = "boolean", default = "false" }, +# { name = "published", type = "boolean" }, # { name = "created_at", type = "datetime", null = false }, # { name = "updated_at", type = "datetime", null = false }, -# { name = "comments_count", type = "integer", null = false, default = "0" } +# { name = "comments_count", type = "integer", null = false, default = 0 } # ] # # indexes = [ diff --git a/test/dummy/app/models/product.rb b/test/dummy/app/models/product.rb index 4735042..41fbccf 100644 --- a/test/dummy/app/models/product.rb +++ b/test/dummy/app/models/product.rb @@ -10,9 +10,9 @@ # { name = "description", type = "text" }, # { name = "price", type = "decimal", null = false }, # { name = "category", type = "string" }, -# { name = "active", type = "boolean", null = false, default = "true" }, +# { name = "active", type = "boolean", null = false, default = true }, # { name = "sku", type = "string" }, -# { name = "stock_quantity", type = "integer", default = "0" }, +# { name = "stock_quantity", type = "integer", default = 0 }, # { name = "created_at", type = "datetime", null = false }, # { name = "updated_at", type = "datetime", null = false } # ] diff --git a/test/dummy/app/models/product_metric.rb b/test/dummy/app/models/product_metric.rb index effffe9..525855a 100644 --- a/test/dummy/app/models/product_metric.rb +++ b/test/dummy/app/models/product_metric.rb @@ -7,9 +7,9 @@ # columns = [ # { name = "id", type = "integer", pk = true, null = false }, # { name = "product_id", type = "integer", null = false }, -# { name = "views", type = "integer", null = false, default = "0" }, -# { name = "purchases", type = "integer", null = false, default = "0" }, -# { name = "revenue", type = "decimal", default = "0.0" }, +# { name = "views", type = "integer", null = false, default = 0 }, +# { name = "purchases", type = "integer", null = false, default = 0 }, +# { name = "revenue", type = "decimal", default = 0.0 }, # { name = "created_at", type = "datetime", null = false }, # { name = "updated_at", type = "datetime", null = false }, # { name = "conversion_rate", type = "decimal" }, diff --git a/test/dummy/app/models/tenant_setting.rb b/test/dummy/app/models/tenant_setting.rb index 47ae1cf..0dfdbdb 100644 --- a/test/dummy/app/models/tenant_setting.rb +++ b/test/dummy/app/models/tenant_setting.rb @@ -9,7 +9,7 @@ # { name = "key", type = "string", pk = true, null = false }, # { name = "value", type = "text" }, # { name = "description", type = "text" }, -# { name = "encrypted", type = "boolean", default = "false" }, +# { name = "encrypted", type = "boolean" }, # { name = "created_at", type = "datetime", null = false }, # { name = "updated_at", type = "datetime", null = false } # ] diff --git a/test/dummy/app/models/vehicle.rb b/test/dummy/app/models/vehicle.rb index 7edff35..e266f20 100644 --- a/test/dummy/app/models/vehicle.rb +++ b/test/dummy/app/models/vehicle.rb @@ -19,7 +19,7 @@ # { name = "color", type = "string" }, # { name = "vin", type = "string" }, # { name = "description", type = "text" }, -# { name = "available", type = "boolean", default = "1" }, +# { name = "available", type = "boolean", default = true }, # { name = "purchase_date", type = "date" }, # { name = "service_time", type = "time" }, # { name = "image_data", type = "binary" }, @@ -28,7 +28,7 @@ # { name = "updated_at", type = "datetime", null = false }, # { name = "vehicle_type", type = "string" }, # { name = "status", type = "string" }, -# { name = "maintenance_count", type = "integer", null = false, default = "0" } +# { name = "maintenance_count", type = "integer", null = false, default = 0 } # ] # # [enums] diff --git a/test/dummy/app/models/vehicle_performance_metrics.rb b/test/dummy/app/models/vehicle_performance_metrics.rb index 0aee0e0..c631573 100644 --- a/test/dummy/app/models/vehicle_performance_metrics.rb +++ b/test/dummy/app/models/vehicle_performance_metrics.rb @@ -7,7 +7,7 @@ # updatable = false # # columns = [ -# { name = "id", type = "integer", null = false, default = "0" }, +# { name = "id", type = "integer", null = false, default = 0 }, # { name = "name", type = "string" }, # { name = "model", type = "string" }, # { name = "year", type = "integer" }, @@ -15,10 +15,10 @@ # { name = "fuel_type", type = "string" }, # { name = "price", type = "decimal" }, # { name = "mileage", type = "integer" }, -# { name = "maintenance_events", type = "integer", null = false, default = "0" }, -# { name = "total_maintenance_cost", type = "decimal", null = false, default = "0" }, -# { name = "trip_count", type = "integer", null = false, default = "0" }, -# { name = "total_distance", type = "decimal", null = false, default = "0" }, +# { name = "maintenance_events", type = "integer", null = false, default = 0 }, +# { name = "total_maintenance_cost", type = "decimal", null = false, default = 0 }, +# { name = "trip_count", type = "integer", null = false, default = 0 }, +# { name = "total_distance", type = "decimal", null = false, default = 0 }, # { name = "cost_per_mile", type = "decimal" }, # { name = "days_owned", type = "integer" }, # { name = "maintenance_category", type = "string", null = false, default = "" }, diff --git a/test/integration/abstract_class_and_connection_test.rb b/test/integration/abstract_class_and_connection_test.rb index 43e103a..f3ad28e 100644 --- a/test/integration/abstract_class_and_connection_test.rb +++ b/test/integration/abstract_class_and_connection_test.rb @@ -263,6 +263,10 @@ def test_parallel_access_to_multiple_databases end def test_abstract_class_connection_specification + # Zeitwerk lazy-loads models: reference concrete subclasses so + # .descendants is populated regardless of test order. + [Dinosaur, Vehicle] + # Test that abstract classes properly specify connections abstract_specs = { PrehistoricRecord => 'prehistoric',