From b3cca864c2be178069bb601bb5b7f07d493ab68e Mon Sep 17 00:00:00 2001 From: Justin Edmund Date: Wed, 15 Jan 2025 17:43:17 -0800 Subject: [PATCH] Importer now displays validation errors This will help people debug errors before submitting their PR. --- lib/granblue/importers/base_importer.rb | 209 ++++++++++++++++++++---- lib/granblue/importers/import_error.rb | 31 ++++ lib/logging_helper.rb | 2 +- lib/post_deployment/manager.rb | 50 +++++- lib/post_deployment/search_indexer.rb | 2 +- 5 files changed, 260 insertions(+), 34 deletions(-) create mode 100644 lib/granblue/importers/import_error.rb diff --git a/lib/granblue/importers/base_importer.rb b/lib/granblue/importers/base_importer.rb index 9c283bb..3c24538 100644 --- a/lib/granblue/importers/base_importer.rb +++ b/lib/granblue/importers/base_importer.rb @@ -1,5 +1,7 @@ # frozen_string_literal: true +require_relative 'import_error' + module Granblue module Importers class BaseImporter @@ -28,42 +30,27 @@ module Granblue CSV.foreach(@file_path, headers: true) do |row| attributes = build_attributes(row) + + # Check if record exists before doing any validation existing_record = model_class.find_by(granblue_id: attributes[:granblue_id]) if existing_record - # For updates, only include non-nil attributes - update_attributes = attributes.compact - would_update = update_attributes.any? { |key, value| existing_record[key] != value } - - if would_update - log_test_update(existing_record, attributes) - simulated_updated[type] << { - granblue_id: attributes[:granblue_id], - name_en: attributes[:name_en] || existing_record.name_en, - attributes: update_attributes, - operation: :update - } - end + simulate_update(existing_record, attributes, simulated_updated, type) else - log_test_creation(attributes) - simulated_new[type] << { - granblue_id: attributes[:granblue_id], - name_en: attributes[:name_en], - attributes: attributes, - operation: :create - } + validate_required_attributes(attributes) + simulate_create(attributes, simulated_new, type) end end { new: simulated_new, updated: simulated_updated } + rescue StandardError => e + handle_error(e) end private def import_row(row) attributes = build_attributes(row) - # Remove nil values from attributes hash for updates - # Keep them for new records to ensure proper defaults record = find_or_create_record(attributes) track_record(record) if record end @@ -76,7 +63,6 @@ module Granblue log_test_update(existing_record, attributes) nil else - # For updates, only include non-nil attributes update_attributes = attributes.compact was_updated = update_attributes.any? { |key, value| existing_record[key] != value } existing_record.update!(update_attributes) if was_updated @@ -87,12 +73,138 @@ module Granblue log_test_creation(attributes) nil else - # For new records, use all attributes including nil values [model_class.create!(attributes), false] end end end + def simulate_create(attributes, simulated_new, type) + test_record = model_class.new(attributes) + validate_record(test_record) + + log_test_creation(attributes) + simulated_new[type] << { + granblue_id: attributes[:granblue_id], + name_en: attributes[:name_en], + attributes: attributes, + operation: :create + } + end + + def simulate_update(existing_record, attributes, simulated_updated, type) + update_attributes = attributes.compact + would_update = update_attributes.any? { |key, value| existing_record[key] != value } + + if would_update + # Create a test record with existing data + test_record = existing_record.dup + + # Validate only the columns being updated + validate_update_attributes(update_attributes) + + # Apply the updates and validate the resulting record + test_record.assign_attributes(update_attributes) + validate_record(test_record) + + log_test_update(existing_record, attributes) + simulated_updated[type] << { + granblue_id: attributes[:granblue_id], + name_en: attributes[:name_en] || existing_record.name_en, + attributes: update_attributes, + operation: :update + } + end + end + + def validate_required_attributes(attributes) + required_columns = model_class.columns.select { |c| !c.null }.map(&:name) + + missing_columns = required_columns.select do |column| + attributes[column.to_sym].nil? && + !model_class.column_defaults[column] && + !%w[id created_at updated_at].include?(column) + end + + if missing_columns.any? + details = [ + "Missing required columns:", + missing_columns.map { |col| " • #{col}" }, + "", + "Affected model: #{model_class.name}" + ].flatten.join("\n") + + raise ImportError.new( + file_name: File.basename(@file_path), + details: details + ) + end + end + + def validate_update_attributes(update_attributes) + # Get the list of columns that cannot be null in the database + required_columns = model_class.columns.select { |c| !c.null }.map(&:name) + + # For updates, we only need to validate the attributes that are being updated + update_columns = update_attributes.keys.map(&:to_s) + + # Only check required columns that are included in the update + missing_columns = required_columns.select do |column| + update_columns.include?(column) && + update_attributes[column.to_sym].nil? && + !model_class.column_defaults[column] && + !%w[id created_at updated_at].include?(column) + end + + if missing_columns.any? + details = [ + "Missing required values for update:", + missing_columns.map { |col| " • #{col}" }, + "", + "Affected model: #{model_class.name}" + ].flatten.join("\n") + + raise ImportError.new( + file_name: File.basename(@file_path), + details: details + ) + end + end + + def validate_record(record) + unless record.valid? + raise ImportError.new( + file_name: File.basename(@file_path), + details: format_validation_error(ActiveRecord::RecordInvalid.new(record)) + ) + end + + begin + ActiveRecord::Base.transaction(requires_new: true) do + record.save! + raise ActiveRecord::Rollback + end + rescue ActiveRecord::StatementInvalid => e + if e.message.include?('violates not-null constraint') + column = e.message.match(/column "([^"]+)"/)[1] + details = [ + "Database constraint violation:", + " • Column '#{column}' cannot be null", + "", + "Affected model: #{model_class.name}" + ].join("\n") + + raise ImportError.new( + file_name: File.basename(@file_path), + details: details + ) + end + raise ImportError.new( + file_name: File.basename(@file_path), + details: format_standard_error(e) + ) + end + end + def track_record(result) record, was_updated = result type = model_class.name.demodulize.downcase @@ -119,17 +231,19 @@ module Granblue end def log_test_update(record, attributes) - # For test mode, show only the attributes that would be updated update_attributes = attributes.compact - @logger&.log_step("Updating #{model_class.name} #{record.granblue_id}...") + @logger&.log_step("\nUpdate #{model_class.name} #{record.granblue_id}:") + @logger&.log_verbose("Current values:") + @logger&.log_verbose(format_attributes(record.attributes.symbolize_keys)) + @logger&.log_verbose("\nNew values:") @logger&.log_verbose(format_attributes(update_attributes)) - @logger&.log_step("\n\n") if @verbose + @logger&.log_step("\n") end def log_test_creation(attributes) - @logger&.log_step("Creating #{model_class.name}...") + @logger&.log_step("\nCreate #{model_class.name}:") @logger&.log_verbose(format_attributes(attributes)) - @logger&.log_step("\n\n") if @verbose + @logger&.log_step("\n") end def log_new_record(record) @@ -187,6 +301,43 @@ module Granblue def build_attributes(row) raise NotImplementedError, 'Subclasses must define build_attributes' end + + def handle_error(error) + details = case error + when ActiveRecord::RecordInvalid + format_validation_error(error) + else + format_standard_error(error) + end + + raise ImportError.new( + file_name: File.basename(@file_path), + details: details + ) + end + + def format_validation_error(error) + [ + "Validation failed:", + error.record.errors.full_messages.map { |msg| " • #{msg}" }, + "", + "Record attributes:", + format_attributes(error.record.attributes.symbolize_keys) + ].flatten.join("\n") + end + + def format_standard_error(error) + if @verbose && error.respond_to?(:backtrace) + [ + error.message, + "", + "Backtrace:", + error.backtrace.take(3).map { |line| " #{line}" } + ].flatten.join("\n") + else + error.message.to_s + end + end end end end diff --git a/lib/granblue/importers/import_error.rb b/lib/granblue/importers/import_error.rb new file mode 100644 index 0000000..f036bdc --- /dev/null +++ b/lib/granblue/importers/import_error.rb @@ -0,0 +1,31 @@ +module Granblue + module Importers + class ImportError < StandardError + attr_reader :file_name, :details + + def initialize(file_name:, details:) + @file_name = file_name + @details = details + super(build_message) + end + + private + + def build_message + "Error importing #{file_name}: #{details}" + end + end + + def format_attributes(attributes) + attributes.map do |key, value| + formatted_value = case value + when Array + value.empty? ? '[]' : value.inspect + else + value.inspect + end + " #{key}: #{formatted_value}" + end.join("\n") + end + end +end diff --git a/lib/logging_helper.rb b/lib/logging_helper.rb index 07ec949..9e54a32 100644 --- a/lib/logging_helper.rb +++ b/lib/logging_helper.rb @@ -10,7 +10,7 @@ module LoggingHelper end def log_error(message) - puts "❌ #{message}" + puts "#{message}" end def log_warning(message) diff --git a/lib/post_deployment/manager.rb b/lib/post_deployment/manager.rb index 63184cb..f5ec3c1 100644 --- a/lib/post_deployment/manager.rb +++ b/lib/post_deployment/manager.rb @@ -127,10 +127,54 @@ module PostDeployment end def handle_error(error) - log_error("\nError during deployment: #{error.message}") - log_error(error.backtrace.take(10).join("\n")) if @verbose + error_message = format_error_message(error) + log_formatted_error(error_message) @test_transaction&.rollback - raise error + exit(1) + end + + def format_error_message(error) + sections = [] + + # Add header section + sections << [ + "═" * 60, + "❌ Error during deployment", + "═" * 60 + ] + + # Add main error message + sections << format_main_error(error) + + # Add stack trace if verbose + if @verbose && error.respond_to?(:backtrace) + sections << [ + "Stack trace:", + error.backtrace.take(5).map { |line| " #{line}" } + ].flatten + end + + sections.flatten.join("\n") + end + + def format_main_error(error) + case error + when Granblue::Importers::ImportError + [ + "File: #{error.file_name}", + "-" * 80, + error.details + ] + else + error.message.to_s + end + end + + def log_formatted_error(message) + # Split message into lines and log each with error prefix + message.split("\n").each do |line| + log_error line + end end def all_records_empty? diff --git a/lib/post_deployment/search_indexer.rb b/lib/post_deployment/search_indexer.rb index b133ff7..8fee55d 100644 --- a/lib/post_deployment/search_indexer.rb +++ b/lib/post_deployment/search_indexer.rb @@ -55,7 +55,7 @@ module PostDeployment end def log_error(message) - puts "❌ #{message}" + puts "#{message}" end end end