diff --git a/lib/ruby-pg-extras.rb b/lib/ruby-pg-extras.rb index c450351..736618b 100644 --- a/lib/ruby-pg-extras.rb +++ b/lib/ruby-pg-extras.rb @@ -7,6 +7,7 @@ require "ruby_pg_extras/diagnose_data" require "ruby_pg_extras/diagnose_print" require "ruby_pg_extras/detect_fk_column" +require "ruby_pg_extras/ignore_list" require "ruby_pg_extras/missing_fk_indexes" require "ruby_pg_extras/missing_fk_constraints" require "ruby_pg_extras/index_info" @@ -195,7 +196,7 @@ def self.missing_fk_indexes(args: {}, in_format: :display_table) end def self.missing_fk_constraints(args: {}, in_format: :display_table) - RubyPgExtras::MissingFkConstraints.call(args[:table_name]) + RubyPgExtras::MissingFkConstraints.call(args[:table_name], ignore_list: args[:ignore_list]) end def self.display_result(result, title:, in_format:) diff --git a/lib/ruby_pg_extras/detect_fk_column.rb b/lib/ruby_pg_extras/detect_fk_column.rb index c6a500c..fe2b277 100644 --- a/lib/ruby_pg_extras/detect_fk_column.rb +++ b/lib/ruby_pg_extras/detect_fk_column.rb @@ -34,16 +34,25 @@ def self.call(column_name, tables) end def call(column_name, tables) - return false unless column_name =~ /_id$/ - table_name = column_name.split("_").first - table_name = pluralize(table_name) - tables.include?(table_name) + # Heuristic: Rails-style foreign keys are usually named `_id`. + # We accept underscores in the prefix (e.g. `account_user_id` -> `account_users`). + match = /\A(?.+)_id\z/i.match(column_name.to_s) + return false unless match + + table_singular = match[:table_singular] + return false if table_singular.empty? + + tables.include?(pluralize(table_singular)) end def pluralize(word) - return word if UNCOUNTABLE.include?(word.downcase) - return IRREGULAR[word] if IRREGULAR.key?(word) - return IRREGULAR.invert[word] if IRREGULAR.value?(word) + # Table names from Postgres are typically lowercase. Normalize before applying rules. + word = word.to_s.downcase + + return word if UNCOUNTABLE.include?(word) + return IRREGULAR.fetch(word) if IRREGULAR.key?(word) + # If the word is already an irregular plural (e.g. "people"), keep it as-is. + return word if IRREGULAR.value?(word) PLURAL_RULES.reverse.each do |(rule, replacement)| return word.gsub(rule, replacement) if word.match?(rule) diff --git a/lib/ruby_pg_extras/ignore_list.rb b/lib/ruby_pg_extras/ignore_list.rb new file mode 100644 index 0000000..baa75d4 --- /dev/null +++ b/lib/ruby_pg_extras/ignore_list.rb @@ -0,0 +1,47 @@ +# frozen_string_literal: true + +module RubyPgExtras + # Parses and matches ignore patterns like: + # - "*" (ignore everything) + # - "posts.*" (ignore all columns on a table) + # - "category_id" (ignore this column name on all tables) + # - "posts.topic_id" (ignore a specific table+column) + class IgnoreList + def initialize(ignore_list) + @rules = normalize(ignore_list) + end + + def ignored?(table:, column_name:) + @rules.any? do |rule| + next true if rule == "*" + next true if rule == "#{table}.*" + next true if rule == column_name + next true if rule == "#{table}.#{column_name}" + false + end + end + + private + + def normalize(ignore_list) + entries = + case ignore_list + when nil + [] + when String + ignore_list.split(",") + when Array + ignore_list + else + raise ArgumentError, "ignore_list must be a String or Array" + end + + entries + .map { |v| v.to_s.strip } + .reject(&:empty?) + .uniq + end + end +end + + diff --git a/lib/ruby_pg_extras/missing_fk_constraints.rb b/lib/ruby_pg_extras/missing_fk_constraints.rb index cf6d859..03e199e 100644 --- a/lib/ruby_pg_extras/missing_fk_constraints.rb +++ b/lib/ruby_pg_extras/missing_fk_constraints.rb @@ -2,42 +2,68 @@ module RubyPgExtras class MissingFkConstraints - def self.call(table_name) - new.call(table_name) + # ignore_list: array (or comma-separated string) of entries like: + # - "posts.category_id" (ignore a specific table+column) + # - "category_id" (ignore this column name for all tables) + # - "posts.*" (ignore all columns on a table) + # - "*" (ignore everything) + def self.call(table_name, ignore_list: nil) + new.call(table_name, ignore_list: ignore_list) end - def call(table_name) - tables = if table_name + def call(table_name, ignore_list: nil) + ignore_list_matcher = IgnoreList.new(ignore_list) + + tables = + if table_name [table_name] else all_tables end - schemas = query_module.table_schemas(in_format: :hash) - foreign_keys = query_module.foreign_keys(in_format: :hash) + schemas_by_table = query_module + .table_schemas(in_format: :hash) + .group_by { |row| row.fetch("table_name") } - tables.reduce([]) do |agg, table| - schema = schemas.select { |row| row.fetch("table_name") == table } - fk_columns = foreign_keys.select { |row| row.fetch("table_name") == table } + fk_columns_by_table = query_module + .foreign_keys(in_format: :hash) + .group_by { |row| row.fetch("table_name") } + .transform_values { |rows| rows.map { |row| row.fetch("column_name") } } - fk_columns = schema.filter_map do |row| - if DetectFkColumn.call(row.fetch("column_name"), all_tables) - row.fetch("column_name") - end - end + tables.each_with_object([]) do |table, agg| + schema = schemas_by_table.fetch(table, []) + fk_columns_for_table = fk_columns_by_table.fetch(table, []) + schema_column_names = schema.map { |row| row.fetch("column_name") } + + candidate_fk_columns = schema.filter_map do |row| + column_name = row.fetch("column_name") + + # Skip columns explicitly excluded via ignore list. + next if ignore_list_matcher.ignored?(table: table, column_name: column_name) - fk_columns.each do |column_name| - if foreign_keys.none? { |row| row.fetch("column_name") == column_name } - agg.push( - { - table: table, - column_name: column_name, - } - ) - end + # Skip columns that already have a foreign key constraint on this table. + next if fk_columns_for_table.include?(column_name) + + # Skip columns that don't look like an FK candidate based on naming conventions. + next unless DetectFkColumn.call(column_name, all_tables) + + # Rails polymorphic associations use _id + _type and can't have FK constraints. + candidate_prefix = column_name.delete_suffix("_id") + polymorphic_type_column = "#{candidate_prefix}_type" + # Skip polymorphic associations (cannot be expressed as a real FK constraint). + next if schema_column_names.include?(polymorphic_type_column) + + column_name end - agg + candidate_fk_columns.each do |column_name| + agg.push( + { + table: table, + column_name: column_name, + } + ) + end end end diff --git a/lib/ruby_pg_extras/missing_fk_indexes.rb b/lib/ruby_pg_extras/missing_fk_indexes.rb index 84d7f4a..257c441 100644 --- a/lib/ruby_pg_extras/missing_fk_indexes.rb +++ b/lib/ruby_pg_extras/missing_fk_indexes.rb @@ -7,26 +7,22 @@ def self.call(table_name) end def call(table_name) + indexes_info = query_module.indexes(in_format: :hash) + foreign_keys = query_module.foreign_keys(in_format: :hash) + tables = if table_name [table_name] else - all_tables + foreign_keys.map { |row| row.fetch("table_name") }.uniq end - indexes_info = query_module.indexes(in_format: :hash) - schemas = query_module.table_schemas(in_format: :hash) - tables.reduce([]) do |agg, table| index_info = indexes_info.select { |row| row.fetch("tablename") == table } - schema = schemas.select { |row| row.fetch("table_name") == table } + table_fks = foreign_keys.select { |row| row.fetch("table_name") == table } - fk_columns = schema.filter_map do |row| - if DetectFkColumn.call(row.fetch("column_name"), all_tables) - row.fetch("column_name") - end - end + table_fks.each do |fk| + column_name = fk.fetch("column_name") - fk_columns.each do |column_name| if index_info.none? { |row| row.fetch("columns").split(",").first == column_name } agg.push( { @@ -43,10 +39,6 @@ def call(table_name) private - def all_tables - @_all_tables ||= query_module.table_size(in_format: :hash).map { |row| row.fetch("name") } - end - def query_module RubyPgExtras end diff --git a/spec/detect_fk_column_spec.rb b/spec/detect_fk_column_spec.rb index 52e47bb..6100ca7 100644 --- a/spec/detect_fk_column_spec.rb +++ b/spec/detect_fk_column_spec.rb @@ -34,5 +34,41 @@ expect(result).to eq(true) end end + + context "matching table with underscored prefix" do + let(:column_name) { "account_user_id" } + let(:tables) { ["account_users", "users"] } + + it "returns true" do + expect(result).to eq(true) + end + end + + context "matching table with uncountable noun" do + let(:column_name) { "fish_id" } + let(:tables) { ["fish", "users"] } + + it "returns true" do + expect(result).to eq(true) + end + end + + context "matching table with mixed-case column name" do + let(:column_name) { "User_id" } + let(:tables) { ["users", "posts"] } + + it "returns true" do + expect(result).to eq(true) + end + end + + context "matching table with irregular plural already pluralized" do + let(:column_name) { "people_id" } + let(:tables) { ["people", "users"] } + + it "returns true" do + expect(result).to eq(true) + end + end end end diff --git a/spec/ignore_list_spec.rb b/spec/ignore_list_spec.rb new file mode 100644 index 0000000..abd294c --- /dev/null +++ b/spec/ignore_list_spec.rb @@ -0,0 +1,60 @@ +# frozen_string_literal: true + +require "spec_helper" +require "ruby-pg-extras" + +describe RubyPgExtras::IgnoreList do + describe "#ignored?" do + it "returns false when ignore_list is nil" do + list = described_class.new(nil) + expect(list.ignored?(table: "posts", column_name: "topic_id")).to eq(false) + end + + it "supports '*' to ignore everything" do + list = described_class.new(["*"]) + expect(list.ignored?(table: "posts", column_name: "topic_id")).to eq(true) + expect(list.ignored?(table: "users", column_name: "customer_id")).to eq(true) + end + + it "supports 'table.*' to ignore all columns for a table" do + list = described_class.new(["posts.*"]) + expect(list.ignored?(table: "posts", column_name: "topic_id")).to eq(true) + expect(list.ignored?(table: "posts", column_name: "user_id")).to eq(true) + expect(list.ignored?(table: "users", column_name: "topic_id")).to eq(false) + end + + it "supports 'column' to ignore a column name globally" do + list = described_class.new(["topic_id"]) + expect(list.ignored?(table: "posts", column_name: "topic_id")).to eq(true) + expect(list.ignored?(table: "users", column_name: "topic_id")).to eq(true) + expect(list.ignored?(table: "posts", column_name: "user_id")).to eq(false) + end + + it "supports 'table.column' to ignore a specific table+column" do + list = described_class.new(["posts.topic_id"]) + expect(list.ignored?(table: "posts", column_name: "topic_id")).to eq(true) + expect(list.ignored?(table: "users", column_name: "topic_id")).to eq(false) + expect(list.ignored?(table: "posts", column_name: "user_id")).to eq(false) + end + + it "accepts ignore_list as a comma-separated string" do + list = described_class.new("posts.topic_id, customer_id") + expect(list.ignored?(table: "posts", column_name: "topic_id")).to eq(true) + expect(list.ignored?(table: "users", column_name: "customer_id")).to eq(true) + expect(list.ignored?(table: "users", column_name: "topic_id")).to eq(false) + end + + it "strips whitespace, drops empty entries and de-duplicates" do + list = described_class.new([" posts.topic_id ", "", "posts.topic_id", " "]) + expect(list.ignored?(table: "posts", column_name: "topic_id")).to eq(true) + expect(list.ignored?(table: "users", column_name: "topic_id")).to eq(false) + end + + it "raises ArgumentError for unsupported ignore_list types" do + expect { described_class.new(123) }.to raise_error(ArgumentError) + expect { described_class.new({}) }.to raise_error(ArgumentError) + end + end +end + + diff --git a/spec/missing_fk_constraints_spec.rb b/spec/missing_fk_constraints_spec.rb index bffaa68..b395427 100644 --- a/spec/missing_fk_constraints_spec.rb +++ b/spec/missing_fk_constraints_spec.rb @@ -6,21 +6,51 @@ describe "#missing_fk_constraints" do it "detects missing foreign keys for all tables" do result = RubyPgExtras.missing_fk_constraints(in_format: :hash) - expect(result.size).to eq(2) - expect(result[0]).to eq({ - table: "users", column_name: "company_id", - }) - expect(result[1]).to eq({ - table: "posts", column_name: "topic_id", - }) + expect(result).to match_array([ + { table: "users", column_name: "customer_id" }, + { table: "posts", column_name: "category_id" }, + ]) end it "detects missing foreign_keys for a specific table" do result = RubyPgExtras.missing_fk_constraints(args: { table_name: "posts" }, in_format: :hash) - expect(result.size).to eq(1) - expect(result[0]).to eq({ - table: "posts", column_name: "topic_id", - }) + expect(result).to eq([ + { table: "posts", column_name: "category_id" }, + ]) + end + + it "does not report columns that already have foreign key constraints" do + result = RubyPgExtras.missing_fk_constraints(args: { table_name: "users" }, in_format: :hash) + expect(result).to eq([ + { table: "users", column_name: "customer_id" }, + ]) + end + + it "does not report polymorphic associations (_id with _type)" do + result = RubyPgExtras.missing_fk_constraints(args: { table_name: "events" }, in_format: :hash) + expect(result).to eq([]) + end + + it "supports ignoring a specific table+column via args" do + result = RubyPgExtras.missing_fk_constraints( + args: { ignore_list: ["posts.category_id"] }, + in_format: :hash + ) + + expect(result).to eq([ + { table: "users", column_name: "customer_id" }, + ]) + end + + it "supports ignoring a column name globally via args" do + result = RubyPgExtras.missing_fk_constraints( + args: { ignore_list: ["customer_id"] }, + in_format: :hash + ) + + expect(result).to eq([ + { table: "posts", column_name: "category_id" }, + ]) end end diff --git a/spec/missing_fk_indexes_spec.rb b/spec/missing_fk_indexes_spec.rb index d903642..462b068 100644 --- a/spec/missing_fk_indexes_spec.rb +++ b/spec/missing_fk_indexes_spec.rb @@ -6,20 +6,16 @@ describe "missing_fk_indexes" do it "detects missing indexes for all tables" do result = RubyPgExtras.missing_fk_indexes(in_format: :hash) - expect(result.size).to eq(2) - expect(result[0]).to eq({ - table: "users", column_name: "company_id", - }) - expect(result[1]).to eq({ - table: "posts", column_name: "topic_id", - }) + expect(result).to match_array([ + { table: "users", column_name: "company_id" }, + { table: "posts", column_name: "topic_id" }, + ]) end it "detects missing indexes for specific table" do result = RubyPgExtras.missing_fk_indexes(args: { table_name: "posts" }, in_format: :hash) - expect(result.size).to eq(1) - expect(result[0]).to eq({ - table: "posts", column_name: "topic_id", - }) + expect(result).to eq([ + { table: "posts", column_name: "topic_id" }, + ]) end end diff --git a/spec/smoke_spec.rb b/spec/smoke_spec.rb index b301ec1..a0599ff 100644 --- a/spec/smoke_spec.rb +++ b/spec/smoke_spec.rb @@ -33,7 +33,7 @@ describe "table_foreign_keys" do it "returns a correct fk info" do result = RubyPgExtras.table_foreign_keys(args: { table_name: :posts }, in_format: :hash) - expect(result.size).to eq(1) + expect(result.size).to eq(2) expect(result[0].keys).to eq(["table_name", "constraint_name", "column_name", "foreign_table_name", "foreign_column_name"]) end @@ -47,7 +47,7 @@ describe "table_schema" do it "returns a correct schema" do result = RubyPgExtras.table_schema(args: { table_name: :users }, in_format: :hash) - expect(result.size).to eq(3) + expect(result.size).to eq(4) expect(result[0].keys).to eq(["column_name", "data_type", "is_nullable", "column_default"]) end diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index c2d72f2..90bf341 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -21,36 +21,65 @@ RSpec.configure do |config| config.before(:suite) do DB_SCHEMA = <<-SQL +DROP TABLE IF EXISTS categories; +DROP TABLE IF EXISTS customers; +DROP TABLE IF EXISTS events; +DROP TABLE IF EXISTS subjects; DROP TABLE IF EXISTS posts; DROP TABLE IF EXISTS users; DROP TABLE IF EXISTS topics; DROP TABLE IF EXISTS companies; +CREATE TABLE companies ( + id SERIAL PRIMARY KEY, + name VARCHAR(255) +); + CREATE TABLE users ( id SERIAL PRIMARY KEY, email VARCHAR(255), - company_id INTEGER + company_id INTEGER, + customer_id INTEGER, + CONSTRAINT fk_users_company FOREIGN KEY (company_id) REFERENCES companies(id) ON DELETE CASCADE +); + +CREATE TABLE topics ( + id SERIAL PRIMARY KEY, + title VARCHAR(255) ); CREATE TABLE posts ( id SERIAL PRIMARY KEY, user_id INTEGER NOT NULL, topic_id INTEGER, + category_id INTEGER, external_id INTEGER, title VARCHAR(255), - CONSTRAINT fk_posts_user FOREIGN KEY (user_id) REFERENCES users(id) ON DELETE CASCADE + CONSTRAINT fk_posts_user FOREIGN KEY (user_id) REFERENCES users(id) ON DELETE CASCADE, + CONSTRAINT fk_posts_topic FOREIGN KEY (topic_id) REFERENCES topics(id) ON DELETE CASCADE ); -CREATE TABLE topics ( +CREATE TABLE customers ( id SERIAL PRIMARY KEY, - title VARCHAR(255) + name VARCHAR(255) ); -CREATE TABLE companies ( +CREATE TABLE categories ( + id SERIAL PRIMARY KEY, + name VARCHAR(255) +); + +CREATE TABLE subjects ( id SERIAL PRIMARY KEY, name VARCHAR(255) ); +CREATE TABLE events ( + id SERIAL PRIMARY KEY, + subject_id INTEGER, + subject_type VARCHAR(255) +); + CREATE INDEX index_posts_on_user_id ON posts(user_id, topic_id); SQL