diff --git a/.rubocop.yml b/.rubocop.yml index 4b50794..a26ad56 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -16,3 +16,6 @@ Metrics/BlockLength: Naming/FileName: Exclude: - 'lib/rubocop-sequel.rb' + +RSpec/ExampleLength: + Enabled: false diff --git a/config/default.yml b/config/default.yml index 7701aef..51a7d02 100644 --- a/config/default.yml +++ b/config/default.yml @@ -11,6 +11,13 @@ Sequel/ConcurrentIndex: VersionAdded: 0.0.1 VersionChanged: 0.3.3 +Sequel/EnforceNotValidConstraint: + Description: 'Ensures that new constraints are created with the `not_valid: true` option.' + Reference: https://www.rubydoc.info/gems/rubocop-sequel/RuboCop/Cop/Sequel/EnforceNotValidConstraint + Enabled: false + VersionAdded: '0.3.4' + VersionChanged: '0.3.4' + Sequel/JSONColumn: Description: >- Advocates the use of the `jsonb` column type over `json` or `hstore` for performance diff --git a/lib/rubocop-sequel.rb b/lib/rubocop-sequel.rb index 0ca6e2e..a0a5c10 100644 --- a/lib/rubocop-sequel.rb +++ b/lib/rubocop-sequel.rb @@ -12,3 +12,4 @@ require 'rubocop/cop/sequel/migration_name' require 'rubocop/cop/sequel/save_changes' require 'rubocop/cop/sequel/partial_constraint' +require 'rubocop/cop/sequel/enforce_not_valid_constraint' diff --git a/lib/rubocop/cop/sequel/enforce_not_valid_constraint.rb b/lib/rubocop/cop/sequel/enforce_not_valid_constraint.rb new file mode 100644 index 0000000..23438c7 --- /dev/null +++ b/lib/rubocop/cop/sequel/enforce_not_valid_constraint.rb @@ -0,0 +1,108 @@ +# frozen_string_literal: true + +module RuboCop + module Cop + module Sequel + # Ensures that new constraints are created with the `not_valid: true` option. + # + # This cop checks database migration files for `add_foreign_key` and + # `add_constraint` operations to ensure that new constraints are created + # with the `not_valid: true` option, which is necessary for deferred validation. + # + # This cop can be configured to exclude or include specific tables through + # the `ExcludeTables` and `IncludeTables` options. + # + # By using the not_valid: true option, the constraint is created but not enforced + # on existing data. This allows the database to continue operating normally without + # the overhead of immediate validation. You can then validate the existing data + # in a controlled manner using a separate process or during off-peak hours, + # minimizing the impact on your application’s performance. This approach ensures + # that the new constraint is applied to new data immediately while providing + # flexibility in handling existing data, promoting better performance and + # stability during schema changes. Also it is a good + # + # @example + # # bad + # add_foreign_key :users, :orders, column: :user_id + # + # # good + # add_foreign_key :users, :orders, column: :user_id, not_valid: true + # + # # bad + # alter_table :users do + # add_constraint(:user_email_check) { "email IS NOT NULL" } + # end + # + # # good + # alter_table :users do + # add_constraint(:user_email_check, not_valid: true) { "email IS NOT NULL" } + # end + # + # @example Configuration + # Sequel/EnforceNotValidConstraint: + # Enabled: true + # Include: + # - "db/migrations" + # ExcludeTables: + # - users + # IncludeTables: + # - comments + class EnforceNotValidConstraint < Base + MSG = "Add the 'not_valid: true' option when creating new constraints to ensure " \ + 'they are not validated immediately.' + + # Matcher for alter_table blocks + def_node_matcher :alter_table_block?, <<-MATCHER + (block + (send _ :alter_table ({str sym} $_)) + _ + $... + ) + MATCHER + + # Matcher for constraint operations like add_foreign_key and add_constraint + def_node_search :constraint_operations, <<~MATCHER + (send _ {:add_foreign_key :add_constraint} ...) + MATCHER + + # Matcher for check if the not_valid: true argument is present + def_node_search :not_valid_constraint?, <<~MATCHER + (pair (_ {:not_valid "not_valid"}) true) + MATCHER + + # Check constraint operations inside the alter_table block + # + # @param node [RuboCop::AST::Node] the AST node being checked + def on_block(node) + alter_table_block?(node) do |table_name, _block_nodes| + next if exclude_table?(table_name) + + constraint_operations(node).each do |block_node| + validate_node(block_node) + end + end + end + + private + + # Validate constraint operations + # + # @param node [RuboCop::AST::Node] the AST node being checked + def validate_node(node) + add_offense(node, message: MSG) unless not_valid_constraint?(node) + end + + # Skip the table if it is excluded or not included + # + # @param table_name [String,Symbol] the name of the table + # @return [Boolean] whether the table should be skipped + def exclude_table?(table_name) + return true if cop_config['ExcludeTables']&.include?(table_name.to_s) + return false unless cop_config.key?('IncludeTables') + + !cop_config['IncludeTables'].include?(table_name.to_s) + end + end + end + end +end diff --git a/spec/project_spec.rb b/spec/project_spec.rb index 66d2da6..bf6089d 100644 --- a/spec/project_spec.rb +++ b/spec/project_spec.rb @@ -15,7 +15,7 @@ expect(description.include?("\n")).to be(false) end - it 'stars from a verb' do # rubocop:disable RSpec/ExampleLength + it 'stars from a verb' do description = config.dig(cop_name, 'Description') start_with_subject = description.match(/\AThis cop (?.+?) .*/) suggestion = start_with_subject[:verb]&.capitalize if start_with_subject @@ -97,7 +97,7 @@ end end - it 'sorts cop names alphabetically' do # rubocop:disable RSpec/ExampleLength + it 'sorts cop names alphabetically' do previous_key = '' config_default = YAML.load_file('config/default.yml') diff --git a/spec/rubocop/cop/sequel/enforce_not_valid_constraint_spec.rb b/spec/rubocop/cop/sequel/enforce_not_valid_constraint_spec.rb new file mode 100644 index 0000000..2d7fda8 --- /dev/null +++ b/spec/rubocop/cop/sequel/enforce_not_valid_constraint_spec.rb @@ -0,0 +1,162 @@ +# frozen_string_literal: true + +RSpec.describe RuboCop::Cop::Sequel::EnforceNotValidConstraint do + subject(:cop) { described_class.new(config) } + + let(:cop_config) { { 'Enabled' => true } } + let(:config) { RuboCop::Config.new('Sequel/EnforceNotValidConstraint' => cop_config) } + + context 'when add_constraint is used' do + it 'registers an offense when used without `not_valid: true` argument' do + offenses = inspect_source(<<~RUBY) + alter_table :foo do + add_constraint :not_valid, Sequel.lit("jsonb_typeof(column) = 'object'") + add_constraint({name: "not_valid"}, Sequel.lit("jsonb_typeof(column) = 'object'")) + end + RUBY + + expect(offenses.size).to eq(2) + end + + it 'registers an offense when used with `not_valid: false` argument' do + offenses = inspect_source(<<~RUBY) + alter_table :foo do + add_constraint({name: "valid", not_valid: false}, Sequel.lit("jsonb_typeof(column) = 'object'")) + end + RUBY + + expect(offenses.size).to eq(1) + end + + it 'does not register an offense when using `not_valid: true` argument' do + offenses = inspect_source(<<~RUBY) + alter_table :foo do + add_constraint({name: "name", not_valid: true }, Sequel.lit("jsonb_typeof(column) = 'object'")) + end + RUBY + + expect(offenses.size).to eq(0) + end + + context 'when wrap with additional block' do + it 'registers an offense when used with `not_valid: false` argument' do + offenses = inspect_source(<<~RUBY) + alter_table :foo do + begin + add_constraint({name: "valid", not_valid: false}, Sequel.lit("jsonb_typeof(column) = 'object'")) + end + end + RUBY + + expect(offenses.size).to eq(1) + end + end + + context 'when hash-rockets is used to define argument' do + it "does not register an offense when using `'not_valid' => true` argument" do + offenses = inspect_source(<<~RUBY) + alter_table :foo do + add_constraint({name: "name", "not_valid" => true }, Sequel.lit("jsonb_typeof(column) = 'object'")) + end + RUBY + + expect(offenses.size).to eq(0) + end + end + end + + context 'when add_foreign_key is used' do + it 'registers an offense when used without `not_valid: true` argument' do + offenses = inspect_source(<<~RUBY) + alter_table :foo do + add_foreign_key(:not_valid, :table) + add_foreign_key(:not_valid, :table, name: "not_valid") + end + RUBY + + expect(offenses.size).to eq(2) + end + + it 'registers an offense when used with `not_valid: false` argument' do + offenses = inspect_source(<<~RUBY) + alter_table :foo do + add_foreign_key(:valid, :table, name: "not_valid", not_valid: false) + end + RUBY + + expect(offenses.size).to eq(1) + end + + it 'does not register an offense when using `not_valid: true` argument' do + offenses = inspect_source(<<~RUBY) + alter_table :foo do + add_foreign_key(:not_valid, :table, name: "not_valid", not_valid: true) + end + RUBY + + expect(offenses.size).to eq(0) + end + end + + context 'with ExcludeTables setting' do + let(:cop_config) do + { + 'Enabled' => true, + 'ExcludeTables' => ['excluded_table'] + } + end + + it 'does not register an offense when using `not_valid: true` argument inside excluded table migration' do + offenses = inspect_source(<<~RUBY) + alter_table :excluded_table do + add_foreign_key(:not_valid, :table) + add_foreign_key(:not_valid, :table, name: "not_valid") + end + RUBY + + expect(offenses.size).to eq(0) + end + + it 'register an offense when using `not_valid: true` argument for not excluded table' do + offenses = inspect_source(<<~RUBY) + alter_table :not_excluded_table do + add_foreign_key(:not_valid, :table) + add_foreign_key(:not_valid, :table, name: "not_valid") + end + RUBY + + expect(offenses.size).to eq(2) + end + end + + context 'with IncludeTables setting' do + let(:cop_config) do + { + 'Enabled' => true, + 'IncludeTables' => ['included_table'] + } + end + + it 'does not register an offense when using `not_valid: true` argument inside excluded table migration' do + offenses = inspect_source(<<~RUBY) + alter_table :included_table do + add_foreign_key(:not_valid, :table) + add_foreign_key(:not_valid, :table, name: "not_valid") + end + RUBY + + expect(offenses.size).to eq(2) + end + + it 'register an offense when using `not_valid: true` argument for not excluded table' do + offenses = inspect_source(<<~RUBY) + alter_table :not_included_table do + add_foreign_key(:not_valid, :table) + add_foreign_key(:not_valid, :table, name: "not_valid") + end + RUBY + + expect(offenses.size).to eq(0) + end + end +end