From d6e93d44aee30f3721d5830f008fdad7d0a38ae2 Mon Sep 17 00:00:00 2001 From: rostikkkk2 Date: Mon, 23 Dec 2019 16:36:37 +0200 Subject: [PATCH 1/4] Bump ARD to 1.7.1 & fix something --- inquisition.gemspec | 2 +- lib/inquisition/active_record_doctor/runner.rb | 2 ++ spec/fixtures/files/active_record_doctor.yml | 4 +++- 3 files changed, 6 insertions(+), 2 deletions(-) diff --git a/inquisition.gemspec b/inquisition.gemspec index a4f867ab..beda1b34 100644 --- a/inquisition.gemspec +++ b/inquisition.gemspec @@ -28,7 +28,7 @@ Gem::Specification.new do |spec| spec.add_development_dependency 'simplecov', '~> 0.17.0' spec.add_development_dependency 'timecop', '~> 0.9.1' - spec.add_dependency 'active_record_doctor', '~> 1.6.0' + spec.add_dependency 'active_record_doctor', '~> 1.7', '>= 1.7.1' spec.add_dependency 'brakeman', '~> 4.6' spec.add_dependency 'bundler-audit', '~> 0.6' spec.add_dependency 'bundler-leak', '~> 0.1' diff --git a/lib/inquisition/active_record_doctor/runner.rb b/lib/inquisition/active_record_doctor/runner.rb index c5552007..7ae18a90 100644 --- a/lib/inquisition/active_record_doctor/runner.rb +++ b/lib/inquisition/active_record_doctor/runner.rb @@ -24,6 +24,8 @@ class Runner < ::Inquisition::Runner def call TASKS.each do |ard_task| ard_task.run.first.each do |table, column| + next unless table && column + @issues << Inquisition::Issue.new(Issue.new(ard_task, table, column).to_h.merge(runner: self)) end end diff --git a/spec/fixtures/files/active_record_doctor.yml b/spec/fixtures/files/active_record_doctor.yml index 0cbc063b..150ead79 100644 --- a/spec/fixtures/files/active_record_doctor.yml +++ b/spec/fixtures/files/active_record_doctor.yml @@ -3,7 +3,9 @@ - severity: :low message: 'projects has missing foreign keys, details: user_id' - severity: :low - message: 'ActiveStorage::Attachment has missing presence validation, details: name, record_type, record_id, blob_id' + message: 'projects has missing non null constraint, details: name, user_id' +- severity: :low + message: 'ActiveStorage::Attachment has missing presence validation, details: name, record_type' - severity: :low message: 'ActiveStorage::Blob has missing presence validation, details: key, filename, byte_size, checksum' - severity: :low From f9a725b06a44f1f2f259edbb45d33e007adc85e1 Mon Sep 17 00:00:00 2001 From: DELL Date: Fri, 10 Apr 2020 23:06:17 +0300 Subject: [PATCH 2/4] Split ARD tasks into separate runners --- .../extraneous_indexes_runner.rb | 17 +++++++++ lib/inquisition/active_record_doctor/issue.rb | 4 +- .../missing_foreign_keys_runner.rb | 17 +++++++++ .../missing_non_null_constraint_runner.rb | 17 +++++++++ .../missing_presence_validation_runner.rb | 17 +++++++++ .../missing_unique_indexes_runner.rb | 17 +++++++++ .../active_record_doctor/runner.rb | 37 +++++++------------ .../undefined_table_references_runner.rb | 17 +++++++++ .../unindexed_deleted_at_runner.rb | 17 +++++++++ .../unindexed_foreign_keys_runner.rb | 17 +++++++++ 10 files changed, 151 insertions(+), 26 deletions(-) create mode 100644 lib/inquisition/active_record_doctor/extraneous_indexes_runner.rb create mode 100644 lib/inquisition/active_record_doctor/missing_foreign_keys_runner.rb create mode 100644 lib/inquisition/active_record_doctor/missing_non_null_constraint_runner.rb create mode 100644 lib/inquisition/active_record_doctor/missing_presence_validation_runner.rb create mode 100644 lib/inquisition/active_record_doctor/missing_unique_indexes_runner.rb create mode 100644 lib/inquisition/active_record_doctor/undefined_table_references_runner.rb create mode 100644 lib/inquisition/active_record_doctor/unindexed_deleted_at_runner.rb create mode 100644 lib/inquisition/active_record_doctor/unindexed_foreign_keys_runner.rb diff --git a/lib/inquisition/active_record_doctor/extraneous_indexes_runner.rb b/lib/inquisition/active_record_doctor/extraneous_indexes_runner.rb new file mode 100644 index 00000000..57c0c636 --- /dev/null +++ b/lib/inquisition/active_record_doctor/extraneous_indexes_runner.rb @@ -0,0 +1,17 @@ +require_relative 'runner' +require 'active_record_doctor/tasks/extraneous_indexes' + +module Inquisition + module ActiveRecordDoctor + class ExtraneousIndexesRunner < ActiveRecordDoctor::Runner + attr_reader :issues + TASK = ::ActiveRecordDoctor::Tasks::ExtraneousIndexes + + def run + TASK.run.first.each do |table, column| + @issues << Issue.new(TASK.name, table, column).to_h.merge(runner: TASK) + end + end + end + end +end diff --git a/lib/inquisition/active_record_doctor/issue.rb b/lib/inquisition/active_record_doctor/issue.rb index 23a3272b..194090e1 100644 --- a/lib/inquisition/active_record_doctor/issue.rb +++ b/lib/inquisition/active_record_doctor/issue.rb @@ -11,7 +11,7 @@ def to_h { severity: Severity::LOW, message: create_message, - context: task.to_s.split('::').last + context: task.demodulize } end @@ -20,7 +20,7 @@ def to_h attr_reader :task, :table, :column def create_message - issue_text = task.to_s.split('::').last.split(/(?=[A-Z])/).map(&:downcase).join(' ') + issue_text = task.demodulize.gsub(/([a-z]+)([A-Z])/, '\1 \2').downcase "#{table} has #{issue_text}, details: #{column ? column.join(', ') : 'n/a'}" end end diff --git a/lib/inquisition/active_record_doctor/missing_foreign_keys_runner.rb b/lib/inquisition/active_record_doctor/missing_foreign_keys_runner.rb new file mode 100644 index 00000000..5e43f939 --- /dev/null +++ b/lib/inquisition/active_record_doctor/missing_foreign_keys_runner.rb @@ -0,0 +1,17 @@ +require_relative 'runner' +require 'active_record_doctor/tasks/missing_foreign_keys' + +module Inquisition + module ActiveRecordDoctor + class MissingForeignKeysRunner < ActiveRecordDoctor::Runner + attr_reader :issues + TASK = ::ActiveRecordDoctor::Tasks::MissingForeignKeys + + def run + TASK.run.first.each do |table, column| + @issues << Issue.new(TASK.name, table, column).to_h.merge(runner: TASK) + end + end + end + end +end diff --git a/lib/inquisition/active_record_doctor/missing_non_null_constraint_runner.rb b/lib/inquisition/active_record_doctor/missing_non_null_constraint_runner.rb new file mode 100644 index 00000000..1565ea39 --- /dev/null +++ b/lib/inquisition/active_record_doctor/missing_non_null_constraint_runner.rb @@ -0,0 +1,17 @@ +require_relative 'runner' +require 'active_record_doctor/tasks/missing_non_null_constraint' + +module Inquisition + module ActiveRecordDoctor + class MissingNonNullConstraintRunner < ActiveRecordDoctor::Runner + attr_reader :issues + TASK = ::ActiveRecordDoctor::Tasks::MissingNonNullConstraint + + def run + TASK.run.first.each do |table, column| + @issues << Issue.new(TASK.name, table, column).to_h.merge(runner: TASK) + end + end + end + end +end diff --git a/lib/inquisition/active_record_doctor/missing_presence_validation_runner.rb b/lib/inquisition/active_record_doctor/missing_presence_validation_runner.rb new file mode 100644 index 00000000..7a33f694 --- /dev/null +++ b/lib/inquisition/active_record_doctor/missing_presence_validation_runner.rb @@ -0,0 +1,17 @@ +require_relative 'runner' +require 'active_record_doctor/tasks/missing_presence_validation' + +module Inquisition + module ActiveRecordDoctor + class MissingPresenceValidationRunner < ActiveRecordDoctor::Runner + attr_reader :issues + TASK = ::ActiveRecordDoctor::Tasks::MissingPresenceValidation + + def run + TASK.run.first.each do |table, column| + @issues << Issue.new(TASK.name, table, column).to_h.merge(runner: TASK) + end + end + end + end +end diff --git a/lib/inquisition/active_record_doctor/missing_unique_indexes_runner.rb b/lib/inquisition/active_record_doctor/missing_unique_indexes_runner.rb new file mode 100644 index 00000000..8c077747 --- /dev/null +++ b/lib/inquisition/active_record_doctor/missing_unique_indexes_runner.rb @@ -0,0 +1,17 @@ +require_relative 'runner' +require 'active_record_doctor/tasks/missing_unique_indexes' + +module Inquisition + module ActiveRecordDoctor + class MissingUniqueIndexesRunner < ActiveRecordDoctor::Runner + attr_reader :issues + TASK = ::ActiveRecordDoctor::Tasks::MissingUniqueIndexes + + def run + TASK.run.first.each do |table, column| + @issues << Issue.new(TASK.name, table, column).to_h.merge(runner: TASK) + end + end + end + end +end diff --git a/lib/inquisition/active_record_doctor/runner.rb b/lib/inquisition/active_record_doctor/runner.rb index 7ae18a90..1f69813f 100644 --- a/lib/inquisition/active_record_doctor/runner.rb +++ b/lib/inquisition/active_record_doctor/runner.rb @@ -1,33 +1,22 @@ -require 'active_record_doctor/tasks/extraneous_indexes' -require 'active_record_doctor/tasks/missing_foreign_keys' -require 'active_record_doctor/tasks/unindexed_deleted_at' -require 'active_record_doctor/tasks/unindexed_foreign_keys' -require 'active_record_doctor/tasks/undefined_table_references' -require 'active_record_doctor/tasks/missing_unique_indexes' -require 'active_record_doctor/tasks/missing_presence_validation' -require 'active_record_doctor/tasks/missing_non_null_constraint' - module Inquisition module ActiveRecordDoctor class Runner < ::Inquisition::Runner - TASKS = [ - ::ActiveRecordDoctor::Tasks::ExtraneousIndexes, - ::ActiveRecordDoctor::Tasks::MissingForeignKeys, - ::ActiveRecordDoctor::Tasks::MissingNonNullConstraint, - ::ActiveRecordDoctor::Tasks::MissingPresenceValidation, - ::ActiveRecordDoctor::Tasks::MissingUniqueIndexes, - ::ActiveRecordDoctor::Tasks::UndefinedTableReferences, - ::ActiveRecordDoctor::Tasks::UnindexedDeletedAt, - ::ActiveRecordDoctor::Tasks::UnindexedForeignKeys - ].freeze + RUNNERS = %w[ExtraneousIndexesRunner + MissingForeignKeysRunner + MissingNonNullConstraintRunner + MissingPresenceValidationRunner + MissingUniqueIndexesRunner + UndefinedTableReferencesRunner + UnindexedDeletedAtRunner + UnindexedForeignKeysRunner].freeze def call - TASKS.each do |ard_task| - ard_task.run.first.each do |table, column| - next unless table && column + RUNNERS.each do |runner| + runner = "Inquisition::ActiveRecordDoctor::#{runner}".constantize.new + runner.run + issues = runner.issues - @issues << Inquisition::Issue.new(Issue.new(ard_task, table, column).to_h.merge(runner: self)) - end + issues.each { |issue| @issues << Inquisition::Issue.new(issue) } end @issues end diff --git a/lib/inquisition/active_record_doctor/undefined_table_references_runner.rb b/lib/inquisition/active_record_doctor/undefined_table_references_runner.rb new file mode 100644 index 00000000..27d7b45d --- /dev/null +++ b/lib/inquisition/active_record_doctor/undefined_table_references_runner.rb @@ -0,0 +1,17 @@ +require_relative 'runner' +require 'active_record_doctor/tasks/undefined_table_references' + +module Inquisition + module ActiveRecordDoctor + class UndefinedTableReferencesRunner < ActiveRecordDoctor::Runner + attr_reader :issues + TASK = ::ActiveRecordDoctor::Tasks::UndefinedTableReferences + + def run + TASK.run.first.each do |table, column| + @issues << Issue.new(TASK.name, table, column).to_h.merge(runner: TASK) + end + end + end + end +end diff --git a/lib/inquisition/active_record_doctor/unindexed_deleted_at_runner.rb b/lib/inquisition/active_record_doctor/unindexed_deleted_at_runner.rb new file mode 100644 index 00000000..3f72d5b0 --- /dev/null +++ b/lib/inquisition/active_record_doctor/unindexed_deleted_at_runner.rb @@ -0,0 +1,17 @@ +require_relative 'runner' +require 'active_record_doctor/tasks/unindexed_deleted_at' + +module Inquisition + module ActiveRecordDoctor + class UnindexedDeletedAtRunner < ActiveRecordDoctor::Runner + attr_reader :issues + TASK = ::ActiveRecordDoctor::Tasks::UnindexedDeletedAt + + def run + TASK.run.first.each do |table, column| + @issues << Issue.new(TASK.name, table, column).to_h.merge(runner: TASK) + end + end + end + end +end diff --git a/lib/inquisition/active_record_doctor/unindexed_foreign_keys_runner.rb b/lib/inquisition/active_record_doctor/unindexed_foreign_keys_runner.rb new file mode 100644 index 00000000..9ac00362 --- /dev/null +++ b/lib/inquisition/active_record_doctor/unindexed_foreign_keys_runner.rb @@ -0,0 +1,17 @@ +require_relative 'runner' +require 'active_record_doctor/tasks/unindexed_foreign_keys' + +module Inquisition + module ActiveRecordDoctor + class UnindexedForeignKeysRunner < ActiveRecordDoctor::Runner + attr_reader :issues + TASK = ::ActiveRecordDoctor::Tasks::UnindexedForeignKeys + + def run + TASK.run.first.each do |table, column| + @issues << Issue.new(TASK.name, table, column).to_h.merge(runner: TASK) + end + end + end + end +end From c1a87f9ae1adf5d02529bbca4b62e59191e6a90f Mon Sep 17 00:00:00 2001 From: DELL Date: Mon, 13 Apr 2020 11:28:48 +0300 Subject: [PATCH 3/4] Fixed ARD tests after modification --- lib/inquisition.rb | 8 ++++++++ spec/inquisition/active_record_doctor/issue_spec.rb | 2 +- spec/inquisition/active_record_doctor/runner_spec.rb | 3 ++- 3 files changed, 11 insertions(+), 2 deletions(-) diff --git a/lib/inquisition.rb b/lib/inquisition.rb index 5074a097..c14eba55 100644 --- a/lib/inquisition.rb +++ b/lib/inquisition.rb @@ -11,6 +11,14 @@ require 'inquisition/active_record_doctor/runner' require 'inquisition/active_record_doctor/issue' +require_relative 'inquisition/active_record_doctor/unindexed_foreign_keys_runner' +require_relative 'inquisition/active_record_doctor/extraneous_indexes_runner' +require_relative 'inquisition/active_record_doctor/missing_foreign_keys_runner' +require_relative 'inquisition/active_record_doctor/missing_non_null_constraint_runner' +require_relative 'inquisition/active_record_doctor/missing_presence_validation_runner' +require_relative 'inquisition/active_record_doctor/missing_unique_indexes_runner' +require_relative 'inquisition/active_record_doctor/undefined_table_references_runner' +require_relative 'inquisition/active_record_doctor/unindexed_deleted_at_runner' require 'inquisition/brakeman/vulnerability' require 'inquisition/brakeman/runner' require 'inquisition/rubocop' diff --git a/spec/inquisition/active_record_doctor/issue_spec.rb b/spec/inquisition/active_record_doctor/issue_spec.rb index 540099f8..b006f78e 100644 --- a/spec/inquisition/active_record_doctor/issue_spec.rb +++ b/spec/inquisition/active_record_doctor/issue_spec.rb @@ -2,7 +2,7 @@ describe '#to_h' do subject(:vulnerability) { described_class.new(task, 'table', ['column#1', 'column#2']) } - let(:task) { ActiveRecordDoctor::Tasks::UnindexedForeignKeys } + let(:task) { ActiveRecordDoctor::Tasks::UnindexedForeignKeys.name } let(:message) { 'table has unindexed foreign keys, details: column#1, column#2' } let(:options) do diff --git a/spec/inquisition/active_record_doctor/runner_spec.rb b/spec/inquisition/active_record_doctor/runner_spec.rb index e7d2704e..bdbb0923 100644 --- a/spec/inquisition/active_record_doctor/runner_spec.rb +++ b/spec/inquisition/active_record_doctor/runner_spec.rb @@ -3,6 +3,7 @@ describe '#call' do let(:runner) { described_class.new } + let(:ard_runner) { 'UnindexedForeignKeysRunner' } let(:ard_task) { ActiveRecordDoctor::Tasks::UnindexedForeignKeys } let(:warning) { { 'unindexed_table' => %w[unindexed_column_1 unindexed_column_1] } } let(:message) do @@ -12,7 +13,7 @@ before { allow(ard_task).to receive(:run).and_return([warning, true]) } it 'returns issues array with specific message' do - stub_const('Inquisition::ActiveRecordDoctor::Runner::TASKS', [ard_task]) + stub_const('Inquisition::ActiveRecordDoctor::Runner::RUNNERS', [ard_runner]) expect(runner.call.first.message).to eq(message) end end From d9f32e391208315bf9b132839dfbbad35c72b2b3 Mon Sep 17 00:00:00 2001 From: DELL Date: Wed, 15 Apr 2020 12:31:21 +0300 Subject: [PATCH 4/4] Fill rails code quality checking section --- lib/inquisition/outputter/doc/tpl/quality.rb | 7 +++ .../doc/tpl/quality/rails_best_practices.rb | 45 ++++++++++++++++++ views/doc/quality.rhtml | 46 ++++--------------- views/doc/quality/rails_best_practices.rhtml | 34 ++++++++++++++ 4 files changed, 95 insertions(+), 37 deletions(-) create mode 100644 lib/inquisition/outputter/doc/tpl/quality/rails_best_practices.rb create mode 100644 views/doc/quality/rails_best_practices.rhtml diff --git a/lib/inquisition/outputter/doc/tpl/quality.rb b/lib/inquisition/outputter/doc/tpl/quality.rb index 646a43ed..f59e8afc 100644 --- a/lib/inquisition/outputter/doc/tpl/quality.rb +++ b/lib/inquisition/outputter/doc/tpl/quality.rb @@ -18,6 +18,12 @@ def rubocop Template.new('quality/rubocop').render(Rubocop.call(@issues)) end end + + def rails_best_practices + @rails_best_practices ||= begin + Template.new('quality/rails_best_practices').render(RailsBestPractices.call(@issues)) + end + end end end end @@ -26,3 +32,4 @@ def rubocop require_relative 'quality/rubycritic' require_relative 'quality/rubocop' +require_relative 'quality/rails_best_practices' diff --git a/lib/inquisition/outputter/doc/tpl/quality/rails_best_practices.rb b/lib/inquisition/outputter/doc/tpl/quality/rails_best_practices.rb new file mode 100644 index 00000000..ac3ebb5c --- /dev/null +++ b/lib/inquisition/outputter/doc/tpl/quality/rails_best_practices.rb @@ -0,0 +1,45 @@ +module Inquisition + module Outputter + class Doc + module TPL + class Quality + class RailsBestPractices + class Wrapper < SimpleDelegator + def group + group_by(&:severity).each do |severity, collection| + yield(severity.name.to_s.capitalize, collection) + end + end + end + + def self.call(issues) + new( + Wrapper.new(Security::Collector.new(issues, ::Inquisition::RailsBestPractices::Runner).call) + ) + end + + def produce + binding + end + + def types(issues) + issues.group_by(&:message).each do |warning, collection| + yield(warning, collection.count) + end + end + + attr_reader :collection + + def initialize(collection) + @collection = collection + end + + def link + @link ||= Stack::Collector.new(['rails_best_practices']).call.first.homepage + end + end + end + end + end + end +end diff --git a/views/doc/quality.rhtml b/views/doc/quality.rhtml index c300c6ff..1d56ee39 100644 --- a/views/doc/quality.rhtml +++ b/views/doc/quality.rhtml @@ -12,41 +12,10 @@ <%= rubocop %> -
- Rails code quality checking -
    -
  • - there are 612 warnings according to the rails_best_practices (the full report can be found here). We grouped them by severity: -
      -
    • - High: -
        -
      • 24 of them related to the missing indexes in the database;
        • -
      • 137 related to the unused code;
        • -
      • 2 of them related to use model association;
        • -
      -
      • -
    • - Medium: -
        -
      • 24 of them related to the missing indexes in the database;
        • -
      • 137 related to the unused code;
        • -
      • 2 of them related to use model association;
        • -
      -
      • -
    • - Low: -
        -
      • 24 of them related to the missing indexes in the database;
        • -
      • 137 related to the unused code;
        • -
      • 2 of them related to use model association;
        • -
      -
      • -
    -
    • - +<%= rails_best_practices %> -
  • there are 41 unused routes and 10 unreachable action methods. The full report can be found here
    • +
    +
  • there are 41 unused routes and 10 unreachable action methods. The full report can be found here
  • the business logic is stored in the:
      @@ -79,7 +48,10 @@
    • workers
    • -
  • the application does not use internationalization;
    • -
  • technical documentation of the project is absent.
    • -
+
  • +
      +
    • the application does not use internationalization;
      • +
    • technical documentation of the project is absent.
      • +
    +
    • diff --git a/views/doc/quality/rails_best_practices.rhtml b/views/doc/quality/rails_best_practices.rhtml new file mode 100644 index 00000000..d02b9c7c --- /dev/null +++ b/views/doc/quality/rails_best_practices.rhtml @@ -0,0 +1,34 @@ +
    + Rails code quality checking +
      +
    • + there are <%= collection.count %> warnings according to the rails_best_practices (the full report can be found here). We grouped them by severity: + <% collection.group do |severity, issues| %> +
        +
      • + + <%= "#{severity}:" %> + +
          + <% types(issues) do |warning, count| %> +
        • + + <%= "#{count} of them related to" %> + + + + <%= warning %> + + +
          • + <% end %> +
        +
        • +
      + <% end %> +
      • +
    + + The full report can be found here. + +