From 3dc2782b4c2b5fb64a7d3b47560c767db7d01ae0 Mon Sep 17 00:00:00 2001 From: Andrey Subbota Date: Tue, 5 Mar 2024 18:03:48 +0100 Subject: [PATCH 1/5] Register offenses for constraint creation without `not_valid:true` option Scanning a large table to verify a new foreign key or check constraint can take a long time, and other updates to the table are locked out until the `ALTER TABLE ADD CONSTRAINT` command is committed. Using `NOT VALID` constraint option is reduce the imact of adding a new constraint on concurrent updates. With `NOT VALID`, the `ADD CONSTRAINT` command does not scan the table and can be committed immediately. --- lib/rubocop-sequel.rb | 1 + .../cop/sequel/not_valid_constraint.rb | 33 ++++++++++++++++++ .../cop/sequel/not_valid_constraint_spec.rb | 34 +++++++++++++++++++ 3 files changed, 68 insertions(+) create mode 100644 lib/rubocop/cop/sequel/not_valid_constraint.rb create mode 100644 spec/rubocop/cop/sequel/not_valid_constraint_spec.rb diff --git a/lib/rubocop-sequel.rb b/lib/rubocop-sequel.rb index 0ca6e2e..081c6b0 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/not_valid_constraint' diff --git a/lib/rubocop/cop/sequel/not_valid_constraint.rb b/lib/rubocop/cop/sequel/not_valid_constraint.rb new file mode 100644 index 0000000..c891a11 --- /dev/null +++ b/lib/rubocop/cop/sequel/not_valid_constraint.rb @@ -0,0 +1,33 @@ +# frozen_string_literal: true + +module RuboCop + module Cop + module Sequel + # Constraint created without not_valid option + class NotValidConstraint < Base + MSG = 'Prefer creating new constraint with the `not_valid: true` option.' + RESTRICT_ON_SEND = %i[add_foreign_key add_constraint].freeze + + def_node_matcher :add_constraint_without_not_valid?, <<-MATCHER + (send _ {:add_foreign_key :add_constraint} $...) + MATCHER + + def on_send(node) + add_constraint_without_not_valid?(node) do |args| + add_offense(node, message: MSG) unless not_valid_option_present?(args) + end + end + + def not_valid_option_present?(args) + args.any? do |arg| + arg.hash_type? && arg.pairs.any? do |pair| + key = pair.key + value = pair.value + key.sym_type? && key.children.first == :not_valid && value.true_type? + end + end + end + end + end + end +end diff --git a/spec/rubocop/cop/sequel/not_valid_constraint_spec.rb b/spec/rubocop/cop/sequel/not_valid_constraint_spec.rb new file mode 100644 index 0000000..f6e997f --- /dev/null +++ b/spec/rubocop/cop/sequel/not_valid_constraint_spec.rb @@ -0,0 +1,34 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe RuboCop::Cop::Sequel::NotValidConstraint do + subject(:cop) { described_class.new } + + it 'registers an offense when add_constraint used without valid option' do + offenses = inspect_source(<<~RUBY) + add_constraint :not_valid, Sequel.lit("jsonb_typeof(column) = 'object'") + add_constraint({name: "not_valid"}, Sequel.lit("jsonb_typeof(column) = 'object'")) + RUBY + + expect(offenses.size).to eq(2) + end + + it 'registers an offense when add_foregn_key used without valid option' do + offenses = inspect_source(<<~RUBY) + add_foreign_key(:not_valid, :table) + add_foreign_key(:not_valid, :table, name: "not_valid") + RUBY + + expect(offenses.size).to eq(2) + end + + it 'does not register an offense when using not valid option' do + offenses = inspect_source(<<~RUBY) + add_constraint({name: "name", not_valid: true }, Sequel.lit("jsonb_typeof(column) = 'object'")) + add_foreign_key(:not_valid, :table, name: "not_valid", not_valid: true) + RUBY + + expect(offenses.size).to eq(0) + end +end From c0f9522585c43326a9b05b06150adc89ad028c59 Mon Sep 17 00:00:00 2001 From: Andrey Subbota Date: Wed, 6 Mar 2024 23:53:14 +0100 Subject: [PATCH 2/5] Refactor specs --- .../cop/sequel/not_valid_constraint_spec.rb | 51 +++++++++++-------- 1 file changed, 31 insertions(+), 20 deletions(-) diff --git a/spec/rubocop/cop/sequel/not_valid_constraint_spec.rb b/spec/rubocop/cop/sequel/not_valid_constraint_spec.rb index f6e997f..be48bfd 100644 --- a/spec/rubocop/cop/sequel/not_valid_constraint_spec.rb +++ b/spec/rubocop/cop/sequel/not_valid_constraint_spec.rb @@ -5,30 +5,41 @@ describe RuboCop::Cop::Sequel::NotValidConstraint do subject(:cop) { described_class.new } - it 'registers an offense when add_constraint used without valid option' do - offenses = inspect_source(<<~RUBY) - add_constraint :not_valid, Sequel.lit("jsonb_typeof(column) = 'object'") - add_constraint({name: "not_valid"}, Sequel.lit("jsonb_typeof(column) = 'object'")) - RUBY - - expect(offenses.size).to eq(2) + context 'when add_constraint used' do + it 'registers an offense when used without `not_valid: true` option' do + offenses = inspect_source(<<~RUBY) + add_constraint :not_valid, Sequel.lit("jsonb_typeof(column) = 'object'") + add_constraint({name: "not_valid"}, Sequel.lit("jsonb_typeof(column) = 'object'")) + RUBY + + expect(offenses.size).to eq(2) + end + + it 'does not register an offense when using `not_valid: true` option' do + offenses = inspect_source(<<~RUBY) + add_constraint({name: "name", not_valid: true }, Sequel.lit("jsonb_typeof(column) = 'object'")) + RUBY + + expect(offenses.size).to eq(0) + end end - it 'registers an offense when add_foregn_key used without valid option' do - offenses = inspect_source(<<~RUBY) - add_foreign_key(:not_valid, :table) - add_foreign_key(:not_valid, :table, name: "not_valid") - RUBY + context 'when add_foreign_key used' do + it 'registers an offense when used without `not_valid: true` option' do + offenses = inspect_source(<<~RUBY) + add_foreign_key(:not_valid, :table) + add_foreign_key(:not_valid, :table, name: "not_valid") + RUBY - expect(offenses.size).to eq(2) - end + expect(offenses.size).to eq(2) + end - it 'does not register an offense when using not valid option' do - offenses = inspect_source(<<~RUBY) - add_constraint({name: "name", not_valid: true }, Sequel.lit("jsonb_typeof(column) = 'object'")) - add_foreign_key(:not_valid, :table, name: "not_valid", not_valid: true) - RUBY + it 'does not register an offense when using `not_valid: true` option' do + offenses = inspect_source(<<~RUBY) + add_foreign_key(:not_valid, :table, name: "not_valid", not_valid: true) + RUBY - expect(offenses.size).to eq(0) + expect(offenses.size).to eq(0) + end end end From 91e70afc222d31e7924f196167a1a696f61b8d2f Mon Sep 17 00:00:00 2001 From: Andrey Subbota Date: Tue, 14 May 2024 23:02:06 +0200 Subject: [PATCH 3/5] Fix specs --- spec/rubocop/cop/sequel/not_valid_constraint_spec.rb | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/spec/rubocop/cop/sequel/not_valid_constraint_spec.rb b/spec/rubocop/cop/sequel/not_valid_constraint_spec.rb index be48bfd..b635447 100644 --- a/spec/rubocop/cop/sequel/not_valid_constraint_spec.rb +++ b/spec/rubocop/cop/sequel/not_valid_constraint_spec.rb @@ -1,8 +1,6 @@ # frozen_string_literal: true -require 'spec_helper' - -describe RuboCop::Cop::Sequel::NotValidConstraint do +RSpec.describe RuboCop::Cop::Sequel::NotValidConstraint do subject(:cop) { described_class.new } context 'when add_constraint used' do From fc925d9f2c56a06f40731cd835ca00817acad893 Mon Sep 17 00:00:00 2001 From: Andrey Subbota Date: Wed, 15 May 2024 10:18:19 +0200 Subject: [PATCH 4/5] Rename cop. Add default settings. Add documentation. --- .rubocop.yml | 3 + config/default.yml | 7 + lib/rubocop-sequel.rb | 2 +- .../sequel/enforce_not_valid_constraint.rb | 123 ++++++++++++++++++ .../cop/sequel/not_valid_constraint.rb | 33 ----- spec/project_spec.rb | 4 +- .../enforce_not_valid_constraint_spec.rb | 116 +++++++++++++++++ .../cop/sequel/not_valid_constraint_spec.rb | 43 ------ 8 files changed, 252 insertions(+), 79 deletions(-) create mode 100644 lib/rubocop/cop/sequel/enforce_not_valid_constraint.rb delete mode 100644 lib/rubocop/cop/sequel/not_valid_constraint.rb create mode 100644 spec/rubocop/cop/sequel/enforce_not_valid_constraint_spec.rb delete mode 100644 spec/rubocop/cop/sequel/not_valid_constraint_spec.rb 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 081c6b0..a0a5c10 100644 --- a/lib/rubocop-sequel.rb +++ b/lib/rubocop-sequel.rb @@ -12,4 +12,4 @@ require 'rubocop/cop/sequel/migration_name' require 'rubocop/cop/sequel/save_changes' require 'rubocop/cop/sequel/partial_constraint' -require 'rubocop/cop/sequel/not_valid_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..3550490 --- /dev/null +++ b/lib/rubocop/cop/sequel/enforce_not_valid_constraint.rb @@ -0,0 +1,123 @@ +# 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. + # + # @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} $_)) + _ + (begin + $... + ) + ) + MATCHER + + # Matcher for constraint operations like add_foreign_key and add_constraint + def_node_matcher :constraint_operation?, <<-MATCHER + (send _ {:add_foreign_key :add_constraint} $...) + 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 skip_table?(table_name) + + block_nodes.each { |block_node| validate_node(block_node) } + end + end + + private + + # Validate constraint operations + # + # @param node [RuboCop::AST::Node] the AST node being checked + def validate_node(node) + constraint_operation?(node) do |args| + next if not_valid_option_present?(args) + + add_offense(node, message: MSG) + end + end + + # Check if the not_valid: true option is present + # + # @param args [Array] the operation arguments + # @return [Boolean] whether the not_valid: true option is present + def not_valid_option_present?(args) + args.any? do |arg| + arg.hash_type? && arg.pairs.any? do |pair| + key = pair.key + value = pair.value + key.sym_type? && key.children.first == :not_valid && value.true_type? + end + end + end + + # Skip the table if it is excluded or not included + # + # @param table_name [String] the name of the table + # @return [Boolean] whether the table should be skipped + def skip_table?(table_name) + return true if excluded_table?(table_name.to_s) + + !included_table?(table_name.to_s) + end + + def excluded_table?(table_name) + return false unless cop_config.key?('ExcludeTables') + + cop_config['ExcludeTables'].include?(table_name) + end + + def included_table?(table_name) + return true unless cop_config.key?('IncludeTables') + + cop_config['IncludeTables'].include?(table_name) + end + end + end + end +end diff --git a/lib/rubocop/cop/sequel/not_valid_constraint.rb b/lib/rubocop/cop/sequel/not_valid_constraint.rb deleted file mode 100644 index c891a11..0000000 --- a/lib/rubocop/cop/sequel/not_valid_constraint.rb +++ /dev/null @@ -1,33 +0,0 @@ -# frozen_string_literal: true - -module RuboCop - module Cop - module Sequel - # Constraint created without not_valid option - class NotValidConstraint < Base - MSG = 'Prefer creating new constraint with the `not_valid: true` option.' - RESTRICT_ON_SEND = %i[add_foreign_key add_constraint].freeze - - def_node_matcher :add_constraint_without_not_valid?, <<-MATCHER - (send _ {:add_foreign_key :add_constraint} $...) - MATCHER - - def on_send(node) - add_constraint_without_not_valid?(node) do |args| - add_offense(node, message: MSG) unless not_valid_option_present?(args) - end - end - - def not_valid_option_present?(args) - args.any? do |arg| - arg.hash_type? && arg.pairs.any? do |pair| - key = pair.key - value = pair.value - key.sym_type? && key.children.first == :not_valid && value.true_type? - end - end - 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..a8b653b --- /dev/null +++ b/spec/rubocop/cop/sequel/enforce_not_valid_constraint_spec.rb @@ -0,0 +1,116 @@ +# 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 used' do + it 'registers an offense when used without `not_valid: true` option' 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 'does not register an offense when using `not_valid: true` option' 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 + + context 'when add_foreign_key used' do + it 'registers an offense when used without `not_valid: true` option' 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 'does not register an offense when using `not_valid: true` option' 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 option' do + let(:cop_config) do + { + 'Enabled' => true, + 'ExcludeTables' => ['excluded_table'] + } + end + + it 'does not register an offense when using `not_valid: true` option 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` option 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 option' do + let(:cop_config) do + { + 'Enabled' => true, + 'IncludeTables' => ['included_table'] + } + end + + it 'does not register an offense when using `not_valid: true` option 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` option 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 diff --git a/spec/rubocop/cop/sequel/not_valid_constraint_spec.rb b/spec/rubocop/cop/sequel/not_valid_constraint_spec.rb deleted file mode 100644 index b635447..0000000 --- a/spec/rubocop/cop/sequel/not_valid_constraint_spec.rb +++ /dev/null @@ -1,43 +0,0 @@ -# frozen_string_literal: true - -RSpec.describe RuboCop::Cop::Sequel::NotValidConstraint do - subject(:cop) { described_class.new } - - context 'when add_constraint used' do - it 'registers an offense when used without `not_valid: true` option' do - offenses = inspect_source(<<~RUBY) - add_constraint :not_valid, Sequel.lit("jsonb_typeof(column) = 'object'") - add_constraint({name: "not_valid"}, Sequel.lit("jsonb_typeof(column) = 'object'")) - RUBY - - expect(offenses.size).to eq(2) - end - - it 'does not register an offense when using `not_valid: true` option' do - offenses = inspect_source(<<~RUBY) - add_constraint({name: "name", not_valid: true }, Sequel.lit("jsonb_typeof(column) = 'object'")) - RUBY - - expect(offenses.size).to eq(0) - end - end - - context 'when add_foreign_key used' do - it 'registers an offense when used without `not_valid: true` option' do - offenses = inspect_source(<<~RUBY) - add_foreign_key(:not_valid, :table) - add_foreign_key(:not_valid, :table, name: "not_valid") - RUBY - - expect(offenses.size).to eq(2) - end - - it 'does not register an offense when using `not_valid: true` option' do - offenses = inspect_source(<<~RUBY) - add_foreign_key(:not_valid, :table, name: "not_valid", not_valid: true) - RUBY - - expect(offenses.size).to eq(0) - end - end -end From 76ebdc987b325e357a493ff8836ab6ecc4fe5790 Mon Sep 17 00:00:00 2001 From: Andrey Subbota Date: Wed, 15 May 2024 15:42:06 +0200 Subject: [PATCH 5/5] Use rubocop matcher for not_valid check --- .../sequel/enforce_not_valid_constraint.rb | 71 ++++++++----------- .../enforce_not_valid_constraint_spec.rb | 70 ++++++++++++++---- 2 files changed, 86 insertions(+), 55 deletions(-) diff --git a/lib/rubocop/cop/sequel/enforce_not_valid_constraint.rb b/lib/rubocop/cop/sequel/enforce_not_valid_constraint.rb index 3550490..23438c7 100644 --- a/lib/rubocop/cop/sequel/enforce_not_valid_constraint.rb +++ b/lib/rubocop/cop/sequel/enforce_not_valid_constraint.rb @@ -12,6 +12,15 @@ module Sequel # 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 @@ -47,25 +56,30 @@ class EnforceNotValidConstraint < Base (block (send _ :alter_table ({str sym} $_)) _ - (begin - $... - ) + $... ) MATCHER # Matcher for constraint operations like add_foreign_key and add_constraint - def_node_matcher :constraint_operation?, <<-MATCHER - (send _ {:add_foreign_key :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 skip_table?(table_name) + alter_table_block?(node) do |table_name, _block_nodes| + next if exclude_table?(table_name) - block_nodes.each { |block_node| validate_node(block_node) } + constraint_operations(node).each do |block_node| + validate_node(block_node) + end end end @@ -75,47 +89,18 @@ def on_block(node) # # @param node [RuboCop::AST::Node] the AST node being checked def validate_node(node) - constraint_operation?(node) do |args| - next if not_valid_option_present?(args) - - add_offense(node, message: MSG) - end - end - - # Check if the not_valid: true option is present - # - # @param args [Array] the operation arguments - # @return [Boolean] whether the not_valid: true option is present - def not_valid_option_present?(args) - args.any? do |arg| - arg.hash_type? && arg.pairs.any? do |pair| - key = pair.key - value = pair.value - key.sym_type? && key.children.first == :not_valid && value.true_type? - end - end + 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] the name of the table + # @param table_name [String,Symbol] the name of the table # @return [Boolean] whether the table should be skipped - def skip_table?(table_name) - return true if excluded_table?(table_name.to_s) - - !included_table?(table_name.to_s) - end - - def excluded_table?(table_name) - return false unless cop_config.key?('ExcludeTables') - - cop_config['ExcludeTables'].include?(table_name) - end - - def included_table?(table_name) - return true unless cop_config.key?('IncludeTables') + 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) + !cop_config['IncludeTables'].include?(table_name.to_s) end end end diff --git a/spec/rubocop/cop/sequel/enforce_not_valid_constraint_spec.rb b/spec/rubocop/cop/sequel/enforce_not_valid_constraint_spec.rb index a8b653b..2d7fda8 100644 --- a/spec/rubocop/cop/sequel/enforce_not_valid_constraint_spec.rb +++ b/spec/rubocop/cop/sequel/enforce_not_valid_constraint_spec.rb @@ -6,8 +6,8 @@ let(:cop_config) { { 'Enabled' => true } } let(:config) { RuboCop::Config.new('Sequel/EnforceNotValidConstraint' => cop_config) } - context 'when add_constraint used' do - it 'registers an offense when used without `not_valid: true` option' do + 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'") @@ -18,7 +18,17 @@ expect(offenses.size).to eq(2) end - it 'does not register an offense when using `not_valid: true` option' do + 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'")) @@ -27,10 +37,36 @@ 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 used' do - it 'registers an offense when used without `not_valid: true` option' do + 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) @@ -41,7 +77,17 @@ expect(offenses.size).to eq(2) end - it 'does not register an offense when using `not_valid: true` option' do + 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) @@ -52,7 +98,7 @@ end end - context 'with ExcludeTables option' do + context 'with ExcludeTables setting' do let(:cop_config) do { 'Enabled' => true, @@ -60,7 +106,7 @@ } end - it 'does not register an offense when using `not_valid: true` option inside excluded table migration' do + 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) @@ -71,7 +117,7 @@ expect(offenses.size).to eq(0) end - it 'register an offense when using `not_valid: true` option for not excluded table' do + 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) @@ -83,7 +129,7 @@ end end - context 'with IncludeTables option' do + context 'with IncludeTables setting' do let(:cop_config) do { 'Enabled' => true, @@ -91,7 +137,7 @@ } end - it 'does not register an offense when using `not_valid: true` option inside excluded table migration' do + 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) @@ -102,7 +148,7 @@ expect(offenses.size).to eq(2) end - it 'register an offense when using `not_valid: true` option for not excluded table' do + 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)