Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement Enum Column checker #192

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions lib/database_consistency.rb
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@
require 'database_consistency/writers/simple/enum_values_inconsistent_with_inclusion'
require 'database_consistency/writers/simple/redundant_case_insensitive_option'
require 'database_consistency/writers/simple/three_state_boolean'
require 'database_consistency/writers/simple/enum_column'
require 'database_consistency/writers/simple_writer'

require 'database_consistency/writers/autofix/helpers/migration'
Expand All @@ -60,6 +61,7 @@

require 'database_consistency/checkers/enum_checkers/enum_checker'
require 'database_consistency/checkers/enum_checkers/enum_type_checker'
require 'database_consistency/checkers/enum_checkers/enum_column_checker'

require 'database_consistency/checkers/association_checkers/association_checker'
require 'database_consistency/checkers/association_checkers/missing_index_checker'
Expand Down
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should it be in enum_checkers folder or column_checkers? What's the difference?

Currently there are column_checkers/enum_value_checker.rb and enum_checkers/enum_type_checker.rb.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should it be in enum_checkers folder or column_checkers? What's the difference?

It depends on what you want to iterate in the "processor". One is iterating through columns, another through defined_enums.

In enum_value_checker, we check that column with the enum type has corresponding Ruby values (through inclusion validator or enum). So the subject is a column.

enum_type_checker.rb focuses on enum with the corresponding column type.

Therefore, I think it should be EnumChecker as you want to ensure that every enum is covered by the enum column.

Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
# frozen_string_literal: true

module DatabaseConsistency
module Checkers
# This class checks that ActiveRecord enum is backed by enum column
class EnumColumnChecker < EnumChecker
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now, this checker intersects with EnumTypeChecker with a shorter list of "allowed" types. Before, we were okay with many types, but now we want to promote enum types.

I don't think we should have a single checker as this could make code harder, but (I believe) you would agree that having both checkers enabled simultaneously doesn't make much sense.

From one side, this looks more like a mode (strict/open). However, the gem doesn't support modes per checkers, and I'm wondering if we should have such (similar to what Rubocop has, for example).

If we don't go in that direction, maybe we should have an internal toggle system that turns off/on some checkers based on intersections. For example, if this checker is on, then EnumTypeChecker is off. However, if we go that way, I'm afraid we may end up with something too complex and unobvious.

With that said, I tend to have modes but I would like to hear your opinion on this matter.

Report = ReportBuilder.define(
DatabaseConsistency::Report,
:table_name,
:column_name
)

private

# ActiveRecord supports native enum type since version 7 and only for PostgreSQL
def preconditions
Helper.postgresql? && ActiveRecord::VERSION::MAJOR >= 7 && column.present?
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I remember we discussed enabling this checker only for PostgreSQL 15+, but let me do some benchmarks for earlier versions as well. I think we should stick to "postgres and AR >=7" condition here.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I remember we discussed enabling this checker only for PostgreSQL 15+, but let me do some benchmarks for earlier versions as well.

This would be awesome!

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should stick to "postgres and AR >=7" condition here.

Yes, we can do that 👍

end

def check
if valid?
report_template(:ok)
else
report_template(:fail, error_slug: :enum_column_type_mismatch)
end
end

def report_template(status, error_slug: nil)
Report.new(
status: status,
error_slug: error_slug,
error_message: nil,
table_name: model.table_name,
column_name: column.name,
**report_attributes
)
end

def column
@column ||= model.columns.find { |c| c.name.to_s == enum.to_s }
end

# @return [Boolean]
def valid?
column.type == :enum
end
end
end
end
3 changes: 2 additions & 1 deletion lib/database_consistency/processors/enums_processor.rb
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,8 @@ module Processors
# The class to process enums
class EnumsProcessor < BaseProcessor
CHECKERS = [
Checkers::EnumTypeChecker
Checkers::EnumTypeChecker,
Checkers::EnumColumnChecker
].freeze

private
Expand Down
22 changes: 22 additions & 0 deletions lib/database_consistency/writers/simple/enum_column.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
# frozen_string_literal: true

module DatabaseConsistency
module Writers
module Simple
class EnumColumn < Base # :nodoc:
private

def template
'column should be enum type'
end

def unique_attributes
{
table_name: report.table_name,
column_name: report.column_name
}
end
end
end
end
end
3 changes: 2 additions & 1 deletion lib/database_consistency/writers/simple_writer.rb
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,8 @@ class SimpleWriter < BaseWriter
enum_values_inconsistent_with_ar_enum: Simple::EnumValuesInconsistentWithArEnum,
enum_values_inconsistent_with_inclusion: Simple::EnumValuesInconsistentWithInclusion,
redundant_case_insensitive_option: Simple::RedundantCaseInsensitiveOption,
three_state_boolean: Simple::ThreeStateBoolean
three_state_boolean: Simple::ThreeStateBoolean,
enum_column_type_mismatch: Simple::EnumColumn
}.freeze

def write
Expand Down
137 changes: 137 additions & 0 deletions spec/checkers/enum_column_checker/postgresql_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,137 @@
# frozen_string_literal: true

RSpec.describe DatabaseConsistency::Checkers::EnumColumnChecker, :postgresql do
subject(:checker) { described_class.new(model, enum) }

let(:model) { entity_class }
let(:enum) { entity_class.defined_enums.keys.first }

context 'with enum column' do
let(:entity_class) do
define_class do |klass|
klass.enum field: { value1: 'value1', value2: 'value2' }
end
end

before do
define_database do
create_enum :field_type, %w[value1 value2]

create_table :entities do |t|
t.enum :field, enum_type: 'field_type'
end
end
end

specify do
expect(checker.report).to have_attributes(
checker_name: 'EnumColumnChecker',
table_or_model_name: entity_class.name,
column_or_attribute_name: 'field',
status: :ok,
error_message: nil,
error_slug: nil
)
end
end

context 'with integer column' do
let(:entity_class) do
define_class do |klass|
klass.enum field: %i[value1 value2]
end
end

before do
define_database do
create_table :entities do |t|
t.integer :field
end
end
end

specify do
expect(checker.report).to have_attributes(
checker_name: 'EnumColumnChecker',
table_or_model_name: entity_class.name,
column_or_attribute_name: 'field',
status: :fail,
error_message: nil,
error_slug: :enum_column_type_mismatch
)
end
end

context 'with string column' do
let(:entity_class) do
define_class do |klass|
klass.enum field: { value1: 'value1', value2: 'value2' }
end
end

before do
define_database do
create_table :entities do |t|
t.string :field
end
end
end

specify do
expect(checker.report).to have_attributes(
checker_name: 'EnumColumnChecker',
table_or_model_name: entity_class.name,
column_or_attribute_name: 'field',
status: :fail,
error_message: nil,
error_slug: :enum_column_type_mismatch
)
end
end

context 'when ActiveRecord below 7' do
toydestroyer marked this conversation as resolved.
Show resolved Hide resolved
before do
stub_const('ActiveRecord::VERSION::MAJOR', 6)
end

context 'with integer column' do
let(:entity_class) do
define_class do |klass|
klass.enum field: %i[value1 value2]
end
end

before do
define_database do
create_table :entities do |t|
t.integer :field
end
end
end

specify do
expect(checker.report).to be_nil
end
end

context 'with string column' do
let(:entity_class) do
define_class do |klass|
klass.enum field: { value1: 'value1', value2: 'value2' }
end
end

before do
define_database do
create_table :entities do |t|
t.string :field
end
end
end

specify do
expect(checker.report).to be_nil
end
end
end
end