From 9d3b6db348b9fd090ac91b0ce58dea787bc90ca7 Mon Sep 17 00:00:00 2001 From: Jorge Tarrero Date: Thu, 22 Oct 2020 11:11:45 +0200 Subject: [PATCH 1/5] Migrate ::Logs model to AR --- app/models/carto/geocoding.rb | 4 +- app/models/carto/log.rb | 295 +++++++++++++++--- app/models/carto/user_migration_export.rb | 4 +- app/models/carto/user_migration_import.rb | 3 +- app/models/carto/visualization_export.rb | 2 +- app/models/data_import.rb | 16 +- app/models/geocoding.rb | 12 +- app/models/log.rb | 198 ------------ app/models/synchronization/member.rb | 7 +- .../importer/lib/importer/connector_runner.rb | 3 +- services/importer/lib/importer/job.rb | 3 +- services/importer/lib/importer/runner.rb | 4 +- .../lib/synchronizer/collection.rb | 1 - spec/factories/log.rb | 5 +- spec/factories/synchronizations.rb | 2 +- spec/models/carto/log_spec.rb | 229 ++++++++++++++ spec/models/carto/user_migration_base_spec.rb | 28 +- .../carto/user_migration_import_spec.rb | 2 +- spec/models/data_import_spec.rb | 4 +- spec/models/log_spec.rb | 235 -------------- spec/requests/api/imports_spec.rb | 2 +- 21 files changed, 521 insertions(+), 538 deletions(-) delete mode 100644 app/models/log.rb create mode 100644 spec/models/carto/log_spec.rb delete mode 100644 spec/models/log_spec.rb diff --git a/app/models/carto/geocoding.rb b/app/models/carto/geocoding.rb index 85560b60bb13..76d9f5371324 100644 --- a/app/models/carto/geocoding.rb +++ b/app/models/carto/geocoding.rb @@ -44,9 +44,9 @@ def remaining_quota user.remaining_geocoding_quota end - # TODO: Properly migrate log to AR and remove this def log - CartoDB::Log[log_id] + Carto::Log.find(log_id) end + end end diff --git a/app/models/carto/log.rb b/app/models/carto/log.rb index 160b04617d0d..905ba10d996b 100644 --- a/app/models/carto/log.rb +++ b/app/models/carto/log.rb @@ -1,72 +1,271 @@ -class Carto::Log < ActiveRecord::Base +module Carto + class Log < ActiveRecord::Base - has_one :data_import, class_name: Carto::DataImport, foreign_key: :logger + has_one :data_import, class_name: Carto::DataImport, foreign_key: :logger - MAX_ENTRY_LENGTH = 256 + after_initialize :after_initialize_callback - ENTRY_POSTFIX = "\n" - ENTRY_FORMAT = "%s: %s#{ENTRY_POSTFIX}" + MAX_ENTRY_LENGTH = 256 + MAX_LOG_ENTRIES = 1000 - TYPE_USER_CREATION = 'user_creation'.freeze + ENTRY_POSTFIX = "\n".freeze - # INFO: disable ActiveRecord inheritance column - self.inheritance_column = :_type + ENTRY_FORMAT = "%s: %s#{ENTRY_POSTFIX}".freeze + ENTRY_REHYDRATED_FORMAT = "%s#{ENTRY_POSTFIX}".freeze - def self.new_user_creation - log = Carto::Log.new - log.entries = 'New user creation' - log.type = TYPE_USER_CREATION - log - end + HALF_OF_LOG_MARK = "===LOG HALF===\n".freeze + END_OF_LOG_MARK = '===LOG END==='.freeze - def append(line, truncate = true, timestamp = Time.now.utc) - line.slice!(MAX_ENTRY_LENGTH..-1) if truncate + TYPE_DATA_IMPORT = 'import'.freeze + TYPE_SYNCHRONIZATION = 'sync'.freeze + TYPE_USER_CREATION = 'user_creation'.freeze + TYPE_GEOCODING = 'geocoding'.freeze + TYPE_USER_MIGRATION_EXPORT = 'user_migration_export'.freeze + TYPE_USER_MIGRATION_IMPORT = 'user_migration_import'.freeze + TYPE_VISUALIZATION_EXPORT = 'visualization_export'.freeze - entry = ENTRY_FORMAT % [ timestamp, line.slice(0..MAX_ENTRY_LENGTH) ] - self.entries = "#{self.entries}#{entry}" - self.save - end + SUPPORTED_TYPES = [ + TYPE_DATA_IMPORT, + TYPE_SYNCHRONIZATION, + TYPE_USER_CREATION, + TYPE_GEOCODING, + TYPE_USER_MIGRATION_EXPORT, + TYPE_USER_MIGRATION_IMPORT, + TYPE_VISUALIZATION_EXPORT + ].freeze - def append_exception(line, exception:, truncate: true, timestamp: Time.now.utc) - append("#{line}: #{exception_to_string(exception)}", truncate, timestamp) - end + # INFO: disable ActiveRecord inheritance column + self.inheritance_column = :_type - def store - self.save - end + def after_initialize_callback + if MAX_LOG_ENTRIES < 2 || MAX_LOG_ENTRIES.odd? + raise StandardError, 'MAX_LOG_ENTRIES must be greater than 2 and an even number' + end - def logger - @logger ||= LogWriter.new(self) - end + @dirty = true if new_record? - private + clear_entries + rehydrate_entries_from_string(entries) + end - def exception_to_string(error) - error.inspect + "\n" + error.backtrace.join("\n") + "\n" - end + def logger + @logger ||= LogWriter.new(self) + end - # A logger implementation that logs to this Carto::Log - class LogWriter < ::Logger - SAVE_BLOCK = 10 # Save the changes to DB every X lines + def self.new_visualization_export + Carto::Log.new( + entries: '', + type: TYPE_VISUALIZATION_EXPORT + ) + end - def initialize(log) - @log = log - @stored_entries = 0 + def self.new_user_creation + Carto::Log.new( + entries: 'New user creation', + type: TYPE_USER_CREATION + ) end - def add(_severity, _progname = nil, message = nil) - if message.present? - @log.entries += message.to_s + "\n" - @stored_entries += 1 - if @stored_entries >= SAVE_BLOCK - @stored_entries = 0 - @log.save + def self.new_user_migration_export + Carto::Log.new( + entries: '', + type: TYPE_USER_MIGRATION_EXPORT + ) + end + + def self.new_user_migration_import + Carto::Log.new( + entries: '', + type: TYPE_USER_MIGRATION_IMPORT + ) + end + + def self.new_data_import(user_id = nil) + Carto::Log.new( + entries: '', + type: TYPE_DATA_IMPORT, + user_id: user_id + ) + end + + def self.new_synchronization(user_id = nil) + Carto::Log.new( + entries: '', + type: TYPE_SYNCHRONIZATION, + user_id: user_id + ) + end + + def self.new_geocoding(user_id = nil) + Carto::Log.new( + entries: '', + type: TYPE_GEOCODING, + user_id: user_id + ) + end + + def collect_entries + (@fixed_entries_half + ordered_circular_entries_half).join('') + end + + def append(content, truncate = true, timestamp = Time.now.utc) + @dirty = true + content.slice!(MAX_ENTRY_LENGTH..-1) if truncate + add_to_entries(format(ENTRY_FORMAT, timestamp, content)) + store + end + + def append_and_store(content, truncate = true, timestamp = Time.now.utc) + reload + # Sync content from other processes. Ie. geocoding cancel + rehydrate_entries_from_string(entries) + append(content, truncate, timestamp) + store + end + + def append_exception(line, exception:, truncate: true, timestamp: Time.now.utc) + append("#{line}: #{exception_to_string(exception)}", truncate, timestamp) + end + + def clear + @dirty = true + end + + def store + if @dirty + self.entries = collect_entries + save + @dirty = false + end + rescue StandardError => e + CartoDB.notify_error( + 'Error appending log, likely an encoding issue', + { error_info: "id: #{id}. #{inspect} --------- #{e.backtrace.join}" } + ) + begin + fix_entries_encoding + save + rescue StandardError => e + CartoDB.notify_exception( + e, + { message: 'Error saving fallback log info.', error_info: "id: #{id}" } + ) + begin + self.entries = "Previous log entries stripped because of an error, check Rollbar. Id: #{id}\n" + + END_OF_LOG_MARK + save + rescue StandardError => e + CartoDB.notify_exception( + e, + { message: 'Error saving stripped fallback log info.', error_info: "id: #{id}" } + ) end end end - def close - @log.save + def to_s + # Just as a safeguard, no real store will be done if there's no data to flush + store + + list = @fixed_entries_half + circular_half = ordered_circular_entries_half + list += [HALF_OF_LOG_MARK] unless circular_half.empty? + (list + circular_half + [END_OF_LOG_MARK]).join('') + end + + private + + def add_to_entries(content) + if @fixed_entries_half.length < half_max_size + @fixed_entries_half << content + else + @circular_entries_half[@circular_index] = content + @circular_index = (@circular_index + 1) % half_max_size + end + end + + def ordered_circular_entries_half + (@circular_entries_half[@circular_index..-1] + @circular_entries_half[0...@circular_index]).compact + end + + def rehydrate_entries_from_string(source) + existing_entries = source.nil? ? [] : source.split(ENTRY_POSTFIX.to_s) + + # If rehydrated data, assume nothing changed yet + @dirty = false unless existing_entries.empty? + + @fixed_entries_half = existing_entries.slice!(0, half_max_size) + .map { |entry| format(ENTRY_REHYDRATED_FORMAT, entry) } + @fixed_entries_half = [] if @fixed_entries_half.nil? # Log was empty + + return if existing_entries.empty? + + index = existing_entries.length > half_max_size ? -half_max_size : -existing_entries.length + @circular_entries_half = existing_entries.slice(index, half_max_size) + .map { |entry| format(ENTRY_REHYDRATED_FORMAT, entry) } + # Fill circular part + if @circular_entries_half.length < half_max_size + @circular_index = @circular_entries_half.length + @circular_entries_half += Array.new(half_max_size - @circular_entries_half.length) + else + @circular_index = 0 + end + end + + def clear_entries + @fixed_entries_half = [] + @circular_entries_half = Array.new(half_max_size) + @circular_index = 0 + '' + end + + def exception_to_string(error) + error.inspect + "\n" + error.backtrace.join("\n") + "\n" + end + + def half_max_size + MAX_LOG_ENTRIES / 2 + end + + def fix_entries_encoding + @fixed_entries_half = @fixed_entries_half.map do |entry| + entry&.encode('UTF-8', 'binary', invalid: :replace, undef: :replace, replace: '?????') + end + @circular_entries_half = @circular_entries_half.map do |entry| + entry&.encode('UTF-8', 'binary', invalid: :replace, undef: :replace, replace: '?????') + end + @dirty = true + + self.entries = collect_entries + end + + # A logger implementation that logs to this Carto::Log + class LogWriter < ::Logger + + SAVE_BLOCK = 100 # Save the changes to DB every X lines + + def initialize(log) + @log = log + @stored_entries = 0 + end + + def add(_severity, _progname = nil, message = nil) + return unless message.present? + + @log.append(message.to_s) + @stored_entries += 1 + + return unless @stored_entries >= SAVE_BLOCK + + @stored_entries = 0 + @log.store + end + + def close + @log.store + end + end + end end diff --git a/app/models/carto/user_migration_export.rb b/app/models/carto/user_migration_export.rb index 168622529ae7..90a1d53b90c2 100644 --- a/app/models/carto/user_migration_export.rb +++ b/app/models/carto/user_migration_export.rb @@ -145,7 +145,9 @@ def validate_export_data end def set_defaults - self.log = Carto::Log.create(type: 'user_migration_export') unless log + self.log = Carto::Log.new_user_migration_export unless log + log.save + self.state = STATE_PENDING unless state save end diff --git a/app/models/carto/user_migration_import.rb b/app/models/carto/user_migration_import.rb index 287042562578..2d89db6ca5e0 100644 --- a/app/models/carto/user_migration_import.rb +++ b/app/models/carto/user_migration_import.rb @@ -219,7 +219,8 @@ def import_job_arguments(data_dir) end def set_defaults - self.log = Carto::Log.create(type: 'user_migration_import') unless log + self.log = Carto::Log.new_user_migration_import unless log + log.save self.state = STATE_PENDING unless state save end diff --git a/app/models/carto/visualization_export.rb b/app/models/carto/visualization_export.rb index b02a90752e94..5c5cf4df9186 100644 --- a/app/models/carto/visualization_export.rb +++ b/app/models/carto/visualization_export.rb @@ -30,7 +30,7 @@ def exporter_folder end def run_export!(file_upload_helper: default_file_upload_helper, download_path: nil) - logger = Carto::Log.new(type: 'visualization_export') + logger = Carto::Log.new_visualization_export logger.append('Exporting') update_attributes(state: STATE_EXPORTING, log: logger) diff --git a/app/models/data_import.rb b/app/models/data_import.rb index f320b910c49f..035771cc49a7 100644 --- a/app/models/data_import.rb +++ b/app/models/data_import.rb @@ -2,7 +2,6 @@ require 'fileutils' require_relative './user' require_relative './table' -require_relative './log' require_relative './visualization/member' require_relative './table_registrar' require_relative './quota_checker' @@ -385,14 +384,11 @@ def before_destroy def instantiate_log uuid = logger - if uuid?(uuid) - self.log = CartoDB::Log.where(id: uuid.to_s).first - else - self.log = CartoDB::Log.new( - type: CartoDB::Log::TYPE_DATA_IMPORT, - user_id: user_id - ) - end + self.log = if uuid?(uuid) + Carto::Log.find(uuid.to_s) + else + Carto::Log.new_data_import(user_id) + end end def uploaded_file @@ -833,7 +829,7 @@ def update_visualization_id(importer) def update_synchronization(importer) if synchronization_id - log.type = CartoDB::Log::TYPE_SYNCHRONIZATION + log.type = Carto::Log::TYPE_SYNCHRONIZATION log.store log.append("synchronization_id: #{synchronization_id}") synchronization = CartoDB::Synchronization::Member.new(id: synchronization_id).fetch diff --git a/app/models/geocoding.rb b/app/models/geocoding.rb index 90dbbedd5f0d..747d5e40f7ba 100644 --- a/app/models/geocoding.rb +++ b/app/models/geocoding.rb @@ -4,7 +4,6 @@ require_relative '../../services/table-geocoder/lib/mail_geocoder' require_relative '../../services/geocoder/lib/geocoder_config' require_relative '../../lib/cartodb/metrics' -require_relative 'log' require_relative '../../lib/cartodb/stats/geocoding' require_dependency 'carto/configuration' @@ -321,16 +320,13 @@ def table_service end def instantiate_log - if self.log_id - @log = CartoDB::Log.where(id: log_id).first + if log_id + @log = Carto::Log.find(log_id) else - @log = CartoDB::Log.new({ - type: CartoDB::Log::TYPE_GEOCODING, - user_id: user.id - }) + @log = Carto::Log.new_geocoding(user.id) @log.store self.log_id = @log.id - self.save + save end @log end diff --git a/app/models/log.rb b/app/models/log.rb deleted file mode 100644 index 628676b4dee1..000000000000 --- a/app/models/log.rb +++ /dev/null @@ -1,198 +0,0 @@ -module CartoDB - # Motivation: Logs usually are quite small, but in some scenarios, or due to encoding errors, parsing errors, etc. - # they can grow and even end with 1 line per source row. Thus the need of having a log that hits as less as possible - # the DB, and that stores critical parts of the log events; this is, the beggining (data recognized, etc.) and the end - # (how it ended, and last 'errored rows' if present). The middle area is not important and for small logs will still - # be included - class Log < Sequel::Model - - MAX_ENTRY_LENGTH = 256 - MAX_LOG_ENTRIES = 1000 - - ENTRY_POSTFIX = "\n" - - ENTRY_FORMAT = "%s: %s#{ENTRY_POSTFIX}" - ENTRY_REHYDRATED_FORMAT = "%s#{ENTRY_POSTFIX}" - - HALF_OF_LOG_MARK = "===LOG HALF===\n".freeze - END_OF_LOG_MARK = '===LOG END==='.freeze - - TYPE_DATA_IMPORT = 'import'.freeze - TYPE_SYNCHRONIZATION = 'sync'.freeze - TYPE_USER_CREATION = 'user_creation'.freeze - TYPE_GEOCODING = 'geocoding'.freeze - - SUPPORTED_TYPES = [ - TYPE_DATA_IMPORT, - TYPE_SYNCHRONIZATION, - TYPE_USER_CREATION, - TYPE_GEOCODING - ] - - # @param id Numeric - # @param type String - # @param user_id String (uuid) - # @param created_at DateTime - # @param updated_at DateTime - # @param entries String - - def after_initialize - super - - if (MAX_LOG_ENTRIES < 2 || MAX_LOG_ENTRIES % 2 != 0) - raise StandardError.new('MAX_LOG_ENTRIES must be greater than 2 and an even number') - end - - @dirty = true if new? - - clear_entries # Reset internal lists - rehydrate_entries_from_string(self.entries) # And now load if proceeds - end - - def clear - @dirty = true - end - - def to_s - # Just as a safeguard, no real store will be done if there's no data to flush - # TODO: Check all usages of Log to see if could be removed - store - - # Extra decoration only for string presentation - list = @fixed_entries_half - circular_half = ordered_circular_entries_half - if circular_half.length > 0 - list = list + [HALF_OF_LOG_MARK] - end - list = (list + circular_half + [END_OF_LOG_MARK]).join('') - end - - def store - if @dirty - self.entries = collect_entries - save - @dirty = false - end - rescue StandardError => e - CartoDB.notify_error("Error appending log, likely an encoding issue", - { - error_info: "id: #{id}. #{self.inspect} --------- #{e.backtrace.join}" - }) - begin - fix_entries_encoding - self.save - rescue StandardError => e2 - CartoDB.notify_exception(e2, - { - message: "Error saving fallback log info.", - error_info: "id: #{id}" - }) - begin - self.entries = "Previous log entries stripped because of an error, check Rollbar. Id: #{id}\n" + - END_OF_LOG_MARK - self.save - rescue StandardError => e3 - CartoDB.notify_exception(e3, - { - message: "Error saving stripped fallback log info.", - error_info: "id: #{id}" - }) - end - end - end - - # INFO: Does not store log, only appends in-memory - def append(content, truncate = true, timestamp = Time.now.utc) - @dirty = true - content.slice!(MAX_ENTRY_LENGTH..-1) if truncate - add_to_entries(ENTRY_FORMAT % [ timestamp, content ]) - end - - def append_and_store(content, truncate = true, timestamp = Time.now.utc) - refresh - # Sync content from other processes. Ie. geocoding cancel - rehydrate_entries_from_string(entries) - append(content, truncate, timestamp) - store - end - - private - - def add_to_entries(content) - if @fixed_entries_half.length < half_max_size - @fixed_entries_half << content - else - @circular_entries_half[@circular_index] = content - @circular_index = (@circular_index + 1) % half_max_size - end - end - - def clear_entries - @fixed_entries_half = [] - @circular_entries_half = Array.new(half_max_size) - @circular_index = 0 - '' - end - - def rehydrate_entries_from_string(source) - existing_entries = source.nil? ? [] : source.split("#{ENTRY_POSTFIX}") - - # If rehydrated data, assume nothing changed yet - @dirty = false if existing_entries.length > 0 - - @fixed_entries_half = existing_entries.slice!(0, half_max_size) - .map { |entry| ENTRY_REHYDRATED_FORMAT % [entry] } - @fixed_entries_half = [] if @fixed_entries_half.nil? # Log was empty - - if (existing_entries.length > 0) - index = existing_entries.length > half_max_size ? -half_max_size : -existing_entries.length - @circular_entries_half = existing_entries.slice(index, half_max_size) - .map { |entry| ENTRY_REHYDRATED_FORMAT % [entry] } - # Fill circular part - if @circular_entries_half.length < half_max_size - @circular_index = @circular_entries_half.length - @circular_entries_half = @circular_entries_half + Array.new(half_max_size - @circular_entries_half.length) - else - @circular_index = 0 - end - end - end - - def ordered_circular_entries_half - # Reorder circular buffer: entries 0 to @circular_index-1 have been more recently overwritten - (@circular_entries_half[@circular_index..-1] + @circular_entries_half[0...@circular_index]).compact - end - - def collect_entries - (@fixed_entries_half + ordered_circular_entries_half).join('') - end - - # INFO: To ease testing - def half_max_size - @half_max_entries_size ||= MAX_LOG_ENTRIES / 2 - end - - def fix_entries_encoding - @fixed_entries_half = @fixed_entries_half.map { |entry| - entry.encode('UTF-8', 'binary', invalid: :replace, undef: :replace, replace: '?????') unless entry.nil? - } - @circular_entries_half = @circular_entries_half.map { |entry| - entry.encode('UTF-8', 'binary', invalid: :replace, undef: :replace, replace: '?????') unless entry.nil? - } - @dirty = true - - self.entries = collect_entries - end - - def validate - super - errors.add(:type, 'unsupported type') unless SUPPORTED_TYPES.include?(self.type) - end - - def before_save - super - self.updated_at = Time.now - end - - end -end diff --git a/app/models/synchronization/member.rb b/app/models/synchronization/member.rb index cd8910f379f3..a5cd0a8d1010 100644 --- a/app/models/synchronization/member.rb +++ b/app/models/synchronization/member.rb @@ -4,7 +4,6 @@ require_relative '../visualization/collection' require_relative '../../../services/importer/lib/importer/datasource_downloader' require_relative '../../../services/datasources/lib/datasources' -require_relative '../log' require_relative '../../../services/importer/lib/importer/unp' require_relative '../../../services/importer/lib/importer/post_import_handler' require_relative '../../../services/importer/lib/importer/overviews' @@ -169,12 +168,12 @@ def run # First import is a "normal import" so still has no id, then run gets called and will get log first time # but we need this to fix old logs if log.nil? - @log = CartoDB::Log.new(type: CartoDB::Log::TYPE_SYNCHRONIZATION, user_id: user.id) + @log = Carto::Log.new_synchronization(user.id) @log.store self.log_id = @log.id store else - @log.type = CartoDB::Log::TYPE_SYNCHRONIZATION + @log.type = Carto::Log::TYPE_SYNCHRONIZATION @log.clear @log.store end @@ -554,7 +553,7 @@ def log log_attributes.merge(user_id: user.id) if user - @log = CartoDB::Log.where(log_attributes).first + @log = Carto::Log.find_by(log_attributes) end def get_datasource(datasource_name) diff --git a/services/importer/lib/importer/connector_runner.rb b/services/importer/lib/importer/connector_runner.rb index 79d38a01b39a..e5727f54e4ce 100644 --- a/services/importer/lib/importer/connector_runner.rb +++ b/services/importer/lib/importer/connector_runner.rb @@ -45,7 +45,6 @@ def initialize(connector_source, options = {}) def run(tracker = nil) @tracker = tracker @job.log "ConnectorRunner #{@json_params.except('connection').to_json}" - # TODO: logging with CartoDB::Logger table_name = @job.table_name updated = false if !should_import?(@connector.table_name) @@ -140,7 +139,7 @@ def result_for(schema, table_name, error = nil) end def new_logger - CartoDB::Log.new(type: CartoDB::Log::TYPE_DATA_IMPORT) + Carto::Log.new_data_import end def new_job(log, pg_options) diff --git a/services/importer/lib/importer/job.rb b/services/importer/lib/importer/job.rb index 9933f209c9d8..b406435fb965 100644 --- a/services/importer/lib/importer/job.rb +++ b/services/importer/lib/importer/job.rb @@ -10,7 +10,6 @@ class Job def initialize(attributes={}) @id = attributes.fetch(:id, Carto::UUIDHelper.random_uuid) @logger = attributes.fetch(:logger, nil) - # Avoid calling new_logger (and thus, requiring CartoDB::Log) if param comes @logger = new_logger if @logger.nil? @pg_options = attributes.fetch(:pg_options, {}) @schema = attributes.fetch(:schema, DEFAULT_IMPORT_SCHEMA) @@ -28,7 +27,7 @@ def new_table_name end def new_logger - CartoDB::Log.new(type: CartoDB::Log::TYPE_DATA_IMPORT) + Carto::Log.new_data_import end def log(message, truncate = true) diff --git a/services/importer/lib/importer/runner.rb b/services/importer/lib/importer/runner.rb index 4ecc8f730a05..ed12e4f3b59e 100644 --- a/services/importer/lib/importer/runner.rb +++ b/services/importer/lib/importer/runner.rb @@ -35,7 +35,7 @@ class Runner # { # :pg Hash { ... } # :downloader CartoDB::Importer2::DatasourceDownloader|CartoDB::Importer2::Downloader - # :log CartoDB::Log|nil + # :log Carto::Log|nil # :job CartoDB::Importer2::Job|nil # :user ::User|nil # :unpacker Unp|nil @@ -80,7 +80,7 @@ def set_importer_stats_host_info(queue_id) end def new_logger - CartoDB::Log.new(type: CartoDB::Log::TYPE_DATA_IMPORT) + Carto::Log.new_data_import end def include_additional_errors_mapping(additional_errors) diff --git a/services/synchronizer/lib/synchronizer/collection.rb b/services/synchronizer/lib/synchronizer/collection.rb index dff87d2d2480..60ae89404fa1 100644 --- a/services/synchronizer/lib/synchronizer/collection.rb +++ b/services/synchronizer/lib/synchronizer/collection.rb @@ -1,6 +1,5 @@ require 'yaml' require 'resque' -require_relative '../../../../app/models/log' require_relative '../../../../app/models/synchronization/member' require_relative '../../../../lib/resque/synchronization_jobs' require_relative '../../../../services/platform-limits/platform_limits' diff --git a/spec/factories/log.rb b/spec/factories/log.rb index 65290f359e99..73e177817064 100644 --- a/spec/factories/log.rb +++ b/spec/factories/log.rb @@ -1,7 +1,4 @@ FactoryGirl.define do - factory :log, class: CartoDB::Log do - end - - factory :carto_log, class: Carto::Log do + factory :log, class: Carto::Log do end end diff --git a/spec/factories/synchronizations.rb b/spec/factories/synchronizations.rb index b724ff4f2d56..247b010f2ec0 100644 --- a/spec/factories/synchronizations.rb +++ b/spec/factories/synchronizations.rb @@ -12,7 +12,7 @@ ran_at Time.now after(:build) do |sync| - sync.log = FactoryGirl.build(:carto_log, type: 'sync') + sync.log = FactoryGirl.build(:log, type: 'sync') end trait :enqueued do diff --git a/spec/models/carto/log_spec.rb b/spec/models/carto/log_spec.rb new file mode 100644 index 000000000000..4ab03531cdf1 --- /dev/null +++ b/spec/models/carto/log_spec.rb @@ -0,0 +1,229 @@ +require_relative '../../spec_helper' + +describe Carto::Log do + describe '#basic' do + it 'checks basic operations' do + type = Carto::Log::TYPE_DATA_IMPORT + text1 = 'test' + text2 = 'anothertest' + timestamp = Time.now.utc + + log = Carto::Log.new({ type: type }) + log.append(text1, false, timestamp) + log.append(text2, false, timestamp) + # Only stores now upon explicit change + log.store + log_id = log.id + + log = Carto::Log.find(log_id) + log.id.should eq log_id + log.type.should eq type + log.to_s.should eq (Carto::Log::ENTRY_FORMAT % [timestamp, text1]) + + (Carto::Log::ENTRY_FORMAT % [timestamp, text2]) + + Carto::Log::END_OF_LOG_MARK + end + end + + describe '#circular buffer logic' do + it 'checks that buffer half works as expected' do + max_entries_per_half = 2 + + timestamp = Time.now.utc + text1 = '1' + text2 = '2' + text3 = '3' + text4 = '4' + text5 = '5' + text6 = '6' + + Carto::Log.any_instance.stubs(:half_max_size).returns(max_entries_per_half) + + log = Carto::Log.new_data_import + + # Fixed half + log.append(text1, false, timestamp) + log.append(text2, false, timestamp) + # circular half + 10.times do + log.append('to be deleted', false, timestamp) + end + log.append(text3, false, timestamp) + log.append(text4, false, timestamp) + + log.to_s.should eq (Carto::Log::ENTRY_FORMAT % [timestamp, text1]) + + (Carto::Log::ENTRY_FORMAT % [timestamp, text2]) + + Carto::Log::HALF_OF_LOG_MARK + + (Carto::Log::ENTRY_FORMAT % [timestamp, text3]) + + (Carto::Log::ENTRY_FORMAT % [timestamp, text4]) + + Carto::Log::END_OF_LOG_MARK + + log.append(text5, false, timestamp) + log.to_s.should eq (Carto::Log::ENTRY_FORMAT % [timestamp, text1]) + + (Carto::Log::ENTRY_FORMAT % [timestamp, text2]) + + Carto::Log::HALF_OF_LOG_MARK + + (Carto::Log::ENTRY_FORMAT % [timestamp, text4]) + + (Carto::Log::ENTRY_FORMAT % [timestamp, text5]) + + Carto::Log::END_OF_LOG_MARK + + log.append(text6, false, timestamp) + log.to_s.should eq (Carto::Log::ENTRY_FORMAT % [timestamp, text1]) + + (Carto::Log::ENTRY_FORMAT % [timestamp, text2]) + + Carto::Log::HALF_OF_LOG_MARK + + (Carto::Log::ENTRY_FORMAT % [timestamp, text5]) + + (Carto::Log::ENTRY_FORMAT % [timestamp, text6]) + + Carto::Log::END_OF_LOG_MARK + end + + it 'checks that loading a log with existing entries works' do + max_entries_per_half = 2 + + timestamp = Time.now.utc + text1 = 'aaa' + text2 = 'bbb' + text3 = '3.4' + text4 = '5 6 7 8' + text5 = 'five' + text6 = 'six' + + Carto::Log.any_instance.stubs(:half_max_size).returns(max_entries_per_half) + + log = Carto::Log.new_data_import + + log.append(text1, false, timestamp) + log.append(text2, false, timestamp) + log.append(text3, false, timestamp) + log.append(text4, false, timestamp) + log.store + + # reload + log = Carto::Log.find(log.id) + + # collect doesn't beautifies + log.send(:collect_entries).should eq (Carto::Log::ENTRY_FORMAT % [timestamp, text1]) + + (Carto::Log::ENTRY_FORMAT % [timestamp, text2]) + + (Carto::Log::ENTRY_FORMAT % [timestamp, text3]) + + (Carto::Log::ENTRY_FORMAT % [timestamp, text4]) + + log.to_s.should eq (Carto::Log::ENTRY_FORMAT % [timestamp, text1]) + + (Carto::Log::ENTRY_FORMAT % [timestamp, text2]) + + Carto::Log::HALF_OF_LOG_MARK + + (Carto::Log::ENTRY_FORMAT % [timestamp, text3]) + + (Carto::Log::ENTRY_FORMAT % [timestamp, text4]) + + Carto::Log::END_OF_LOG_MARK + + # More tests + log = Carto::Log.new_data_import + log.append(text1, false, timestamp) + log.append(text2, false, timestamp) + log.append(text3, false, timestamp) + log.store + log = Carto::Log.find(log.id) + + log.send(:collect_entries).should eq (Carto::Log::ENTRY_FORMAT % [timestamp, text1]) + + (Carto::Log::ENTRY_FORMAT % [timestamp, text2]) + + (Carto::Log::ENTRY_FORMAT % [timestamp, text3]) + log.to_s.should eq (Carto::Log::ENTRY_FORMAT % [timestamp, text1]) + + (Carto::Log::ENTRY_FORMAT % [timestamp, text2]) + + Carto::Log::HALF_OF_LOG_MARK + + (Carto::Log::ENTRY_FORMAT % [timestamp, text3]) + + Carto::Log::END_OF_LOG_MARK + + # Check that new entries are added correctly + log.append(text4, false, timestamp) + log.send(:collect_entries).should eq (Carto::Log::ENTRY_FORMAT % [timestamp, text1]) + + (Carto::Log::ENTRY_FORMAT % [timestamp, text2]) + + (Carto::Log::ENTRY_FORMAT % [timestamp, text3]) + + (Carto::Log::ENTRY_FORMAT % [timestamp, text4]) + log.to_s.should eq (Carto::Log::ENTRY_FORMAT % [timestamp, text1]) + + (Carto::Log::ENTRY_FORMAT % [timestamp, text2]) + + Carto::Log::HALF_OF_LOG_MARK + + (Carto::Log::ENTRY_FORMAT % [timestamp, text3]) + + (Carto::Log::ENTRY_FORMAT % [timestamp, text4]) + + Carto::Log::END_OF_LOG_MARK + log.append(text5, false, timestamp) + log.append(text6, false, timestamp) + log.send(:collect_entries).should eq (Carto::Log::ENTRY_FORMAT % [timestamp, text1]) + + (Carto::Log::ENTRY_FORMAT % [timestamp, text2]) + + (Carto::Log::ENTRY_FORMAT % [timestamp, text5]) + + (Carto::Log::ENTRY_FORMAT % [timestamp, text6]) + log.to_s.should eq (Carto::Log::ENTRY_FORMAT % [timestamp, text1]) + + (Carto::Log::ENTRY_FORMAT % [timestamp, text2]) + + Carto::Log::HALF_OF_LOG_MARK + + (Carto::Log::ENTRY_FORMAT % [timestamp, text5]) + + (Carto::Log::ENTRY_FORMAT % [timestamp, text6]) + + Carto::Log::END_OF_LOG_MARK + + log = Carto::Log.new_data_import + log.append(text1, false, timestamp) + log.store + log = Carto::Log.find(log.id) + + log.send(:collect_entries).should eq(Carto::Log::ENTRY_FORMAT % [timestamp, text1]) + + log.to_s.should eq (Carto::Log::ENTRY_FORMAT % [timestamp, text1]) + + Carto::Log::END_OF_LOG_MARK + + # This test checks that old logs with more lines than accepted get truncated correctly + log = Carto::Log.new_data_import + log.append(text1, false, timestamp) + log.append(text2, false, timestamp) + log.append('fill', false, timestamp) + log.append('fill more', false, timestamp) + # This goes to the circular area + log.append('fill even more', false, timestamp) + log.append(text3, false, timestamp) + log.append(text4, false, timestamp) + log.store + + log = Carto::Log.find(log.id) + + log.send(:collect_entries).should eq (Carto::Log::ENTRY_FORMAT % [timestamp, text1]) + + (Carto::Log::ENTRY_FORMAT % [timestamp, text2]) + + (Carto::Log::ENTRY_FORMAT % [timestamp, text3]) + + (Carto::Log::ENTRY_FORMAT % [timestamp, text4]) + log.to_s.should eq (Carto::Log::ENTRY_FORMAT % [timestamp, text1]) + + (Carto::Log::ENTRY_FORMAT % [timestamp, text2]) + + Carto::Log::HALF_OF_LOG_MARK + + (Carto::Log::ENTRY_FORMAT % [timestamp, text3]) + + (Carto::Log::ENTRY_FORMAT % [timestamp, text4]) + + Carto::Log::END_OF_LOG_MARK + + # Nothing should change with this + log.send(:fix_entries_encoding) + log.send(:collect_entries).should eq (Carto::Log::ENTRY_FORMAT % [timestamp, text1]) + + (Carto::Log::ENTRY_FORMAT % [timestamp, text2]) + + (Carto::Log::ENTRY_FORMAT % [timestamp, text3]) + + (Carto::Log::ENTRY_FORMAT % [timestamp, text4]) + log.to_s.should eq (Carto::Log::ENTRY_FORMAT % [timestamp, text1]) + + (Carto::Log::ENTRY_FORMAT % [timestamp, text2]) + + Carto::Log::HALF_OF_LOG_MARK + + (Carto::Log::ENTRY_FORMAT % [timestamp, text3]) + + (Carto::Log::ENTRY_FORMAT % [timestamp, text4]) + + Carto::Log::END_OF_LOG_MARK + # Check that new entries are added correctly + log.append(text5, false, timestamp) + log.append(text6, false, timestamp) + log.send(:collect_entries).should eq (Carto::Log::ENTRY_FORMAT % [timestamp, text1]) + + (Carto::Log::ENTRY_FORMAT % [timestamp, text2]) + + (Carto::Log::ENTRY_FORMAT % [timestamp, text5]) + + (Carto::Log::ENTRY_FORMAT % [timestamp, text6]) + log.to_s.should eq (Carto::Log::ENTRY_FORMAT % [timestamp, text1]) + + (Carto::Log::ENTRY_FORMAT % [timestamp, text2]) + + Carto::Log::HALF_OF_LOG_MARK + + (Carto::Log::ENTRY_FORMAT % [timestamp, text5]) + + (Carto::Log::ENTRY_FORMAT % [timestamp, text6]) + + Carto::Log::END_OF_LOG_MARK + end + + it 'checks zero case of a new log' do + log = Carto::Log.new_data_import + log.to_s.should eq Carto::Log::END_OF_LOG_MARK + log = Carto::Log.find(log.id) + log.to_s.should eq Carto::Log::END_OF_LOG_MARK + + # Forcing call without entries + log.send(:fix_entries_encoding) + log.to_s.should eq Carto::Log::END_OF_LOG_MARK + end + end +end diff --git a/spec/models/carto/user_migration_base_spec.rb b/spec/models/carto/user_migration_base_spec.rb index c8a9b00a072f..d817dee6f17f 100644 --- a/spec/models/carto/user_migration_base_spec.rb +++ b/spec/models/carto/user_migration_base_spec.rb @@ -60,7 +60,7 @@ def teardown_mock_plpython_function(user) export = create(:user_migration_export, user_id: carto_user.id, export_data: false) export.run_export - puts export.log.entries if export.state != Carto::UserMigrationExport::STATE_COMPLETE + puts export.log.collect_entries if export.state != Carto::UserMigrationExport::STATE_COMPLETE expect(export.state).to eq(Carto::UserMigrationExport::STATE_COMPLETE) remove_user(carto_user) @@ -77,7 +77,7 @@ def teardown_mock_plpython_function(user) import.run_import - puts import.log.entries if import.state != Carto::UserMigrationImport::STATE_COMPLETE + puts import.log.collect_entries if import.state != Carto::UserMigrationImport::STATE_COMPLETE expect(import.state).to eq(Carto::UserMigrationImport::STATE_COMPLETE) carto_user = Carto::User.find(user_attributes['id']) @@ -128,7 +128,7 @@ def teardown_mock_plpython_function(user) export = create(:user_migration_export, user_id: carto_user.id, export_data: false) export.run_export - puts export.log.entries if export.state != Carto::UserMigrationExport::STATE_COMPLETE + puts export.log.collect_entries if export.state != Carto::UserMigrationExport::STATE_COMPLETE expect(export.state).to eq(Carto::UserMigrationExport::STATE_COMPLETE) remove_user(carto_user) @@ -162,7 +162,7 @@ def teardown_mock_plpython_function(user) export = create(:user_migration_export, organization_id: @carto_organization.id, export_data: false) export.run_export - puts export.log.entries if export.state != Carto::UserMigrationExport::STATE_COMPLETE + puts export.log.collect_entries if export.state != Carto::UserMigrationExport::STATE_COMPLETE expect(export.state).to eq(Carto::UserMigrationExport::STATE_COMPLETE) database_host = @carto_organization.owner.database_host @@ -182,7 +182,7 @@ def teardown_mock_plpython_function(user) import.run_import - puts import.log.entries if import.state != Carto::UserMigrationImport::STATE_COMPLETE + puts import.log.collect_entries if import.state != Carto::UserMigrationImport::STATE_COMPLETE expect(import.state).to eq(Carto::UserMigrationImport::STATE_COMPLETE) @carto_organization.users.each do |u| @@ -195,7 +195,7 @@ def teardown_mock_plpython_function(user) export = create(:user_migration_export, organization_id: @carto_organization.id, export_data: false) export.run_export - puts export.log.entries if export.state != Carto::UserMigrationExport::STATE_COMPLETE + puts export.log.collect_entries if export.state != Carto::UserMigrationExport::STATE_COMPLETE expect(export.state).to eq(Carto::UserMigrationExport::STATE_COMPLETE) database_host = @carto_organization.owner.database_host @@ -242,7 +242,7 @@ def teardown_mock_plpython_function(user) export = create(:user_migration_export, user_id: carto_user.id, export_metadata: true) export.run_export - puts export.log.entries if export.state != Carto::UserMigrationExport::STATE_COMPLETE + puts export.log.collect_entries if export.state != Carto::UserMigrationExport::STATE_COMPLETE expect(export.state).to eq(Carto::UserMigrationExport::STATE_COMPLETE) carto_user.client_application.destroy @@ -260,7 +260,7 @@ def teardown_mock_plpython_function(user) import.stubs(:assert_user_does_not_exist) import.run_import - puts import.log.entries if import.state != Carto::UserMigrationImport::STATE_COMPLETE + puts import.log.collect_entries if import.state != Carto::UserMigrationImport::STATE_COMPLETE expect(import.state).to eq(Carto::UserMigrationImport::STATE_COMPLETE) carto_user = Carto::User.find(user_attributes['id']) @@ -291,7 +291,7 @@ def teardown_mock_plpython_function(user) export = create(:user_migration_export, user_id: carto_user.id, export_metadata: true) export.run_export - puts export.log.entries if export.state != Carto::UserMigrationExport::STATE_COMPLETE + puts export.log.collect_entries if export.state != Carto::UserMigrationExport::STATE_COMPLETE expect(export.state).to eq(Carto::UserMigrationExport::STATE_COMPLETE) user.destroy_cascade @@ -308,7 +308,7 @@ def teardown_mock_plpython_function(user) import.stubs(:assert_user_does_not_exist) import.run_import - puts import.log.entries if import.state != Carto::UserMigrationImport::STATE_COMPLETE + puts import.log.collect_entries if import.state != Carto::UserMigrationImport::STATE_COMPLETE expect(export.state).to eq(Carto::UserMigrationImport::STATE_COMPLETE) imported_user = Carto::User.find(user_attributes['id']) imported_user.visualizations.count.should eq 3 @@ -341,7 +341,7 @@ def teardown_mock_plpython_function(user) import.run_import expect(import.state).to eq(Carto::UserMigrationImport::STATE_FAILURE) - expect(import.log.entries).to include('DB already exists at DB host') + expect(import.log.collect_entries).to include('DB already exists at DB host') # DB exists, otherwise this would fail user.in_database.run("select 1;") @@ -367,7 +367,7 @@ def teardown_mock_plpython_function(user) export = create(:user_migration_export, user_id: carto_user.id, export_metadata: true) export.run_export - export.log.entries.should_not include("Cannot export if tables aren't synched with db. Please run ghost tables.") + export.log.collect_entries.should_not include("Cannot export if tables aren't synched with db. Please run ghost tables.") expect(export.state).to eq(Carto::UserMigrationExport::STATE_COMPLETE) export.destroy @@ -425,7 +425,7 @@ def remove_user(carto_user) it "fails" do expect(export.state).to eq(Carto::UserMigrationImport::STATE_FAILURE) - expect(export.log.entries).to include("Can't migrate custom plpython2 functions") + expect(export.log.collect_entries).to include("Can't migrate custom plpython2 functions") teardown_mock_plpython_function(user) end @@ -437,7 +437,7 @@ def remove_user(carto_user) it "fails" do expect(export.state).to eq(Carto::UserMigrationImport::STATE_FAILURE) - expect(export.log.entries).to include("Can't migrate custom plpython2 functions") + expect(export.log.collect_entries).to include("Can't migrate custom plpython2 functions") teardown_mock_plpython_function(user) end diff --git a/spec/models/carto/user_migration_import_spec.rb b/spec/models/carto/user_migration_import_spec.rb index 64af5c3eb75e..1925347cd932 100644 --- a/spec/models/carto/user_migration_import_spec.rb +++ b/spec/models/carto/user_migration_import_spec.rb @@ -13,7 +13,7 @@ ) import.run_import.should eq false import.state.should eq 'failure' - import.log.entries.should match(/Dry dry cannot be true while import_metadata is true/) + import.log.collect_entries.should match(/Dry dry cannot be true while import_metadata is true/) end it 'fails saving a migration with dry and import_metadata' do diff --git a/spec/models/data_import_spec.rb b/spec/models/data_import_spec.rb index 426f3ab975ac..5c78b3fdb6be 100644 --- a/spec/models/data_import_spec.rb +++ b/spec/models/data_import_spec.rb @@ -603,12 +603,12 @@ def create_import_from_query(user: @user, overwrite:, from_query: '') end describe 'log' do - it 'is initialized to a CartoDB::Log instance' do + it 'is initialized to a Carto::Log instance' do data_import = DataImport.create( user_id: @user.id, data_source: "http://mydatasource.cartodb.wadus.com/foo.csv" ) - data_import.log.should be_instance_of CartoDB::Log + data_import.log.should be_instance_of Carto::Log end it 'allows messages to be appended' do diff --git a/spec/models/log_spec.rb b/spec/models/log_spec.rb deleted file mode 100644 index ad0b72e68fc1..000000000000 --- a/spec/models/log_spec.rb +++ /dev/null @@ -1,235 +0,0 @@ -require_relative '../spec_helper' - -include CartoDB - -describe CartoDB::Log do - - describe '#basic' do - it 'checks basic operations' do - user_id = Carto::UUIDHelper.random_uuid - type = Log::TYPE_DATA_IMPORT - text1 = 'test' - text2 = 'anothertest' - timestamp = Time.now.utc - - log = Log.new({ type: type }) - log.append(text1, false, timestamp) - log.append(text2, false, timestamp) - # Only stores now upon explicit change - log.store - log_id = log.id - - log = Log.where(id:log_id).first - log.id.should eq log_id - log.type.should eq type - log.to_s.should eq (Log::ENTRY_FORMAT % [ timestamp, text1 ]) + - (Log::ENTRY_FORMAT % [ timestamp, text2 ]) + - Log::END_OF_LOG_MARK - end - end - - describe '#circular buffer logic' do - it 'checks that buffer half works as expected' do - max_entries_per_half = 2 - - timestamp = Time.now.utc - text1 = "1" - text2 = "2" - text3 = "3" - text4 = "4" - text5 = "5" - text6 = "6" - - Log.any_instance.stubs(:half_max_size).returns(max_entries_per_half) - - log = Log.new({ type: Log::TYPE_DATA_IMPORT }) - - # Fixed half - log.append(text1, false, timestamp) - log.append(text2, false, timestamp) - # circular half - 10.times do - log.append('to be deleted', false, timestamp) - end - log.append(text3, false, timestamp) - log.append(text4, false, timestamp) - - log.to_s.should eq (Log::ENTRY_FORMAT % [ timestamp, text1 ]) + - (Log::ENTRY_FORMAT % [ timestamp, text2 ]) + - Log::HALF_OF_LOG_MARK + - (Log::ENTRY_FORMAT % [ timestamp, text3 ]) + - (Log::ENTRY_FORMAT % [ timestamp, text4 ]) + - Log::END_OF_LOG_MARK - - log.append(text5, false, timestamp) - log.to_s.should eq (Log::ENTRY_FORMAT % [ timestamp, text1 ]) + - (Log::ENTRY_FORMAT % [ timestamp, text2 ]) + - Log::HALF_OF_LOG_MARK + - (Log::ENTRY_FORMAT % [ timestamp, text4 ]) + - (Log::ENTRY_FORMAT % [ timestamp, text5 ]) + - Log::END_OF_LOG_MARK - - log.append(text6, false, timestamp) - log.to_s.should eq (Log::ENTRY_FORMAT % [ timestamp, text1 ]) + - (Log::ENTRY_FORMAT % [ timestamp, text2 ]) + - Log::HALF_OF_LOG_MARK + - (Log::ENTRY_FORMAT % [ timestamp, text5 ]) + - (Log::ENTRY_FORMAT % [ timestamp, text6 ]) + - Log::END_OF_LOG_MARK - end - - it 'checks that loading a log with existing entries works' do - max_entries_per_half = 2 - - timestamp = Time.now.utc - text1 = "aaa" - text2 = "bbb" - text3 = "3.4" - text4 = "5 6 7 8" - text5 = "five" - text6 = "six" - - Log.any_instance.stubs(:half_max_size).returns(max_entries_per_half) - - log = Log.new({ type: Log::TYPE_DATA_IMPORT }) - - log.append(text1, false, timestamp) - log.append(text2, false, timestamp) - log.append(text3, false, timestamp) - log.append(text4, false, timestamp) - log.store - - #reload - log = Log.where(id: log.id).first - - # collect doesn't beautifies - log.send(:collect_entries).should eq (Log::ENTRY_FORMAT % [ timestamp, text1 ]) + - (Log::ENTRY_FORMAT % [ timestamp, text2 ]) + - (Log::ENTRY_FORMAT % [ timestamp, text3 ]) + - (Log::ENTRY_FORMAT % [ timestamp, text4 ]) - - log.to_s.should eq (Log::ENTRY_FORMAT % [ timestamp, text1 ]) + - (Log::ENTRY_FORMAT % [ timestamp, text2 ]) + - Log::HALF_OF_LOG_MARK + - (Log::ENTRY_FORMAT % [ timestamp, text3 ]) + - (Log::ENTRY_FORMAT % [ timestamp, text4 ]) + - Log::END_OF_LOG_MARK - - # More tests - log = Log.new({ type: Log::TYPE_DATA_IMPORT }) - log.append(text1, false, timestamp) - log.append(text2, false, timestamp) - log.append(text3, false, timestamp) - log.store - log = Log.where(id: log.id).first - - log.send(:collect_entries).should eq (Log::ENTRY_FORMAT % [ timestamp, text1 ]) + - (Log::ENTRY_FORMAT % [ timestamp, text2 ]) + - (Log::ENTRY_FORMAT % [ timestamp, text3 ]) - log.to_s.should eq (Log::ENTRY_FORMAT % [ timestamp, text1 ]) + - (Log::ENTRY_FORMAT % [ timestamp, text2 ]) + - Log::HALF_OF_LOG_MARK + - (Log::ENTRY_FORMAT % [ timestamp, text3 ]) + - Log::END_OF_LOG_MARK - - # Check that new entries are added correctly - log.append(text4, false, timestamp) - log.send(:collect_entries).should eq (Log::ENTRY_FORMAT % [ timestamp, text1 ]) + - (Log::ENTRY_FORMAT % [ timestamp, text2 ]) + - (Log::ENTRY_FORMAT % [ timestamp, text3 ]) + - (Log::ENTRY_FORMAT % [ timestamp, text4 ]) - log.to_s.should eq (Log::ENTRY_FORMAT % [ timestamp, text1 ]) + - (Log::ENTRY_FORMAT % [ timestamp, text2 ]) + - Log::HALF_OF_LOG_MARK + - (Log::ENTRY_FORMAT % [ timestamp, text3 ]) + - (Log::ENTRY_FORMAT % [ timestamp, text4 ]) + - Log::END_OF_LOG_MARK - log.append(text5, false, timestamp) - log.append(text6, false, timestamp) - log.send(:collect_entries).should eq (Log::ENTRY_FORMAT % [timestamp, text1]) + - (Log::ENTRY_FORMAT % [timestamp, text2]) + - (Log::ENTRY_FORMAT % [timestamp, text5]) + - (Log::ENTRY_FORMAT % [timestamp, text6]) - log.to_s.should eq (Log::ENTRY_FORMAT % [timestamp, text1]) + - (Log::ENTRY_FORMAT % [timestamp, text2]) + - Log::HALF_OF_LOG_MARK + - (Log::ENTRY_FORMAT % [timestamp, text5]) + - (Log::ENTRY_FORMAT % [timestamp, text6]) + - Log::END_OF_LOG_MARK - - log = Log.new({ type: Log::TYPE_DATA_IMPORT }) - log.append(text1, false, timestamp) - log.store - log = Log.where(id: log.id).first - - log.send(:collect_entries).should eq (Log::ENTRY_FORMAT % [ timestamp, text1 ]) - - log.to_s.should eq (Log::ENTRY_FORMAT % [ timestamp, text1 ]) + - Log::END_OF_LOG_MARK - - # This test checks that old logs with more lines than accepted get truncated correctly - log = Log.new({ type: Log::TYPE_DATA_IMPORT }) - log.append(text1, false, timestamp) - log.append(text2, false, timestamp) - log.append('filll', false, timestamp) - log.append('filll more', false, timestamp) - # This goes to the circular area - log.append('filll even more', false, timestamp) - log.append(text3, false, timestamp) - log.append(text4, false, timestamp) - log.store - - log = Log.where(id: log.id).first - - log.send(:collect_entries).should eq (Log::ENTRY_FORMAT % [ timestamp, text1 ]) + - (Log::ENTRY_FORMAT % [ timestamp, text2 ]) + - (Log::ENTRY_FORMAT % [ timestamp, text3 ]) + - (Log::ENTRY_FORMAT % [ timestamp, text4 ]) - log.to_s.should eq (Log::ENTRY_FORMAT % [ timestamp, text1 ]) + - (Log::ENTRY_FORMAT % [ timestamp, text2 ]) + - Log::HALF_OF_LOG_MARK + - (Log::ENTRY_FORMAT % [ timestamp, text3 ]) + - (Log::ENTRY_FORMAT % [ timestamp, text4 ]) + - Log::END_OF_LOG_MARK - - #Nothing should change with this - log.send(:fix_entries_encoding) - log.send(:collect_entries).should eq (Log::ENTRY_FORMAT % [ timestamp, text1 ]) + - (Log::ENTRY_FORMAT % [ timestamp, text2 ]) + - (Log::ENTRY_FORMAT % [ timestamp, text3 ]) + - (Log::ENTRY_FORMAT % [ timestamp, text4 ]) - log.to_s.should eq (Log::ENTRY_FORMAT % [ timestamp, text1 ]) + - (Log::ENTRY_FORMAT % [ timestamp, text2 ]) + - Log::HALF_OF_LOG_MARK + - (Log::ENTRY_FORMAT % [ timestamp, text3 ]) + - (Log::ENTRY_FORMAT % [ timestamp, text4 ]) + - Log::END_OF_LOG_MARK - # Check that new entries are added correctly - log.append(text5, false, timestamp) - log.append(text6, false, timestamp) - log.send(:collect_entries).should eq (Log::ENTRY_FORMAT % [timestamp, text1]) + - (Log::ENTRY_FORMAT % [timestamp, text2]) + - (Log::ENTRY_FORMAT % [timestamp, text5]) + - (Log::ENTRY_FORMAT % [timestamp, text6]) - log.to_s.should eq (Log::ENTRY_FORMAT % [ timestamp, text1 ]) + - (Log::ENTRY_FORMAT % [timestamp, text2]) + - Log::HALF_OF_LOG_MARK + - (Log::ENTRY_FORMAT % [timestamp, text5]) + - (Log::ENTRY_FORMAT % [timestamp, text6]) + - Log::END_OF_LOG_MARK - - end - - it 'checks zero case of a new log' do - log = Log.new({ type: Log::TYPE_DATA_IMPORT }) - log.to_s.should eq Log::END_OF_LOG_MARK - log = Log.where(id: log.id).first - log.to_s.should eq Log::END_OF_LOG_MARK - - # Forcing call without entries - log.send(:fix_entries_encoding) - log.to_s.should eq Log::END_OF_LOG_MARK - end - end - -end diff --git a/spec/requests/api/imports_spec.rb b/spec/requests/api/imports_spec.rb index 63af91a187ad..bfb2c7d4f672 100644 --- a/spec/requests/api/imports_spec.rb +++ b/spec/requests/api/imports_spec.rb @@ -167,7 +167,7 @@ def auth_params last_import.tables_created_count.should be_nil last_import.state.should be == 'failure' last_import.error_code.should be == 8002 - last_import.log.entries.should include('Results would set overquota') + last_import.log.collect_entries.should include('Results would set overquota') @user.reload.tables.count.should == 0 end From bb9819eba7a5753e66c90b0a136927621259393c Mon Sep 17 00:00:00 2001 From: Jorge Tarrero Date: Thu, 22 Oct 2020 13:35:30 +0200 Subject: [PATCH 2/5] Fix tests and linter --- NEWS.md | 1 + app/models/carto/log.rb | 9 +- app/models/carto/user_creation.rb | 2 +- spec/models/carto/log_spec.rb | 140 +++++++++--------- spec/models/carto/user_migration_base_spec.rb | 4 +- 5 files changed, 76 insertions(+), 80 deletions(-) diff --git a/NEWS.md b/NEWS.md index 60bab9af2f50..23725eb9ad54 100644 --- a/NEWS.md +++ b/NEWS.md @@ -14,6 +14,7 @@ Development * Migrate `ClientApplication` model to `ActiveRecord` [#15886](https://github.com/CartoDB/cartodb/pull/15886) * Avoid delegating special methods in presenters [#15889](https://github.com/CartoDB/cartodb/pull/15889) * Fix Dashboard/Data navigation for free users. Update Data preview texts [#15892](https://github.com/CartoDB/cartodb/pull/15892) +* Migrate `Log` model to `ActiveRecord` [#15896](https://github.com/CartoDB/cartodb/pull/15896) 4.42.0 (2020-09-28) ------------------- diff --git a/app/models/carto/log.rb b/app/models/carto/log.rb index 905ba10d996b..e9e6da13d310 100644 --- a/app/models/carto/log.rb +++ b/app/models/carto/log.rb @@ -54,7 +54,6 @@ def logger def self.new_visualization_export Carto::Log.new( - entries: '', type: TYPE_VISUALIZATION_EXPORT ) end @@ -68,21 +67,18 @@ def self.new_user_creation def self.new_user_migration_export Carto::Log.new( - entries: '', type: TYPE_USER_MIGRATION_EXPORT ) end def self.new_user_migration_import Carto::Log.new( - entries: '', type: TYPE_USER_MIGRATION_IMPORT ) end def self.new_data_import(user_id = nil) Carto::Log.new( - entries: '', type: TYPE_DATA_IMPORT, user_id: user_id ) @@ -90,7 +86,6 @@ def self.new_data_import(user_id = nil) def self.new_synchronization(user_id = nil) Carto::Log.new( - entries: '', type: TYPE_SYNCHRONIZATION, user_id: user_id ) @@ -98,7 +93,6 @@ def self.new_synchronization(user_id = nil) def self.new_geocoding(user_id = nil) Carto::Log.new( - entries: '', type: TYPE_GEOCODING, user_id: user_id ) @@ -112,7 +106,6 @@ def append(content, truncate = true, timestamp = Time.now.utc) @dirty = true content.slice!(MAX_ENTRY_LENGTH..-1) if truncate add_to_entries(format(ENTRY_FORMAT, timestamp, content)) - store end def append_and_store(content, truncate = true, timestamp = Time.now.utc) @@ -220,7 +213,7 @@ def clear_entries end def exception_to_string(error) - error.inspect + "\n" + error.backtrace.join("\n") + "\n" + "#{error.inspect}\n#{error.backtrace.join("\n")}\n" end def half_max_size diff --git a/app/models/carto/user_creation.rb b/app/models/carto/user_creation.rb index 59fe9c2a66bd..c49304899d1e 100644 --- a/app/models/carto/user_creation.rb +++ b/app/models/carto/user_creation.rb @@ -204,7 +204,7 @@ def log_transition_end end def log_transition(prefix) - self.log.append("#{prefix}: State: #{self.state}") + log.append("#{prefix}: State: #{self.state}") end def initialize_user diff --git a/spec/models/carto/log_spec.rb b/spec/models/carto/log_spec.rb index 4ab03531cdf1..1723ea61f926 100644 --- a/spec/models/carto/log_spec.rb +++ b/spec/models/carto/log_spec.rb @@ -18,8 +18,8 @@ log = Carto::Log.find(log_id) log.id.should eq log_id log.type.should eq type - log.to_s.should eq (Carto::Log::ENTRY_FORMAT % [timestamp, text1]) + - (Carto::Log::ENTRY_FORMAT % [timestamp, text2]) + + log.to_s.should eq format(Carto::Log::ENTRY_FORMAT, timestamp, text1) + + format(Carto::Log::ENTRY_FORMAT, timestamp, text2) + Carto::Log::END_OF_LOG_MARK end end @@ -50,27 +50,27 @@ log.append(text3, false, timestamp) log.append(text4, false, timestamp) - log.to_s.should eq (Carto::Log::ENTRY_FORMAT % [timestamp, text1]) + - (Carto::Log::ENTRY_FORMAT % [timestamp, text2]) + + log.to_s.should eq format(Carto::Log::ENTRY_FORMAT, timestamp, text1) + + format(Carto::Log::ENTRY_FORMAT, timestamp, text2) + Carto::Log::HALF_OF_LOG_MARK + - (Carto::Log::ENTRY_FORMAT % [timestamp, text3]) + - (Carto::Log::ENTRY_FORMAT % [timestamp, text4]) + + format(Carto::Log::ENTRY_FORMAT, timestamp, text3) + + format(Carto::Log::ENTRY_FORMAT, timestamp, text4) + Carto::Log::END_OF_LOG_MARK log.append(text5, false, timestamp) - log.to_s.should eq (Carto::Log::ENTRY_FORMAT % [timestamp, text1]) + - (Carto::Log::ENTRY_FORMAT % [timestamp, text2]) + + log.to_s.should eq format(Carto::Log::ENTRY_FORMAT, timestamp, text1) + + format(Carto::Log::ENTRY_FORMAT, timestamp, text2) + Carto::Log::HALF_OF_LOG_MARK + - (Carto::Log::ENTRY_FORMAT % [timestamp, text4]) + - (Carto::Log::ENTRY_FORMAT % [timestamp, text5]) + + format(Carto::Log::ENTRY_FORMAT, timestamp, text4) + + format(Carto::Log::ENTRY_FORMAT, timestamp, text5) + Carto::Log::END_OF_LOG_MARK log.append(text6, false, timestamp) - log.to_s.should eq (Carto::Log::ENTRY_FORMAT % [timestamp, text1]) + - (Carto::Log::ENTRY_FORMAT % [timestamp, text2]) + + log.to_s.should eq format(Carto::Log::ENTRY_FORMAT, timestamp, text1) + + format(Carto::Log::ENTRY_FORMAT, timestamp, text2) + Carto::Log::HALF_OF_LOG_MARK + - (Carto::Log::ENTRY_FORMAT % [timestamp, text5]) + - (Carto::Log::ENTRY_FORMAT % [timestamp, text6]) + + format(Carto::Log::ENTRY_FORMAT, timestamp, text5) + + format(Carto::Log::ENTRY_FORMAT, timestamp, text6) + Carto::Log::END_OF_LOG_MARK end @@ -99,16 +99,16 @@ log = Carto::Log.find(log.id) # collect doesn't beautifies - log.send(:collect_entries).should eq (Carto::Log::ENTRY_FORMAT % [timestamp, text1]) + - (Carto::Log::ENTRY_FORMAT % [timestamp, text2]) + - (Carto::Log::ENTRY_FORMAT % [timestamp, text3]) + - (Carto::Log::ENTRY_FORMAT % [timestamp, text4]) + log.send(:collect_entries).should eq format(Carto::Log::ENTRY_FORMAT, timestamp, text1) + + format(Carto::Log::ENTRY_FORMAT, timestamp, text2) + + format(Carto::Log::ENTRY_FORMAT, timestamp, text3) + + format(Carto::Log::ENTRY_FORMAT, timestamp, text4) - log.to_s.should eq (Carto::Log::ENTRY_FORMAT % [timestamp, text1]) + - (Carto::Log::ENTRY_FORMAT % [timestamp, text2]) + + log.to_s.should eq format(Carto::Log::ENTRY_FORMAT, timestamp, text1) + + format(Carto::Log::ENTRY_FORMAT, timestamp, text2) + Carto::Log::HALF_OF_LOG_MARK + - (Carto::Log::ENTRY_FORMAT % [timestamp, text3]) + - (Carto::Log::ENTRY_FORMAT % [timestamp, text4]) + + format(Carto::Log::ENTRY_FORMAT, timestamp, text3) + + format(Carto::Log::ENTRY_FORMAT, timestamp, text4) + Carto::Log::END_OF_LOG_MARK # More tests @@ -119,38 +119,38 @@ log.store log = Carto::Log.find(log.id) - log.send(:collect_entries).should eq (Carto::Log::ENTRY_FORMAT % [timestamp, text1]) + - (Carto::Log::ENTRY_FORMAT % [timestamp, text2]) + - (Carto::Log::ENTRY_FORMAT % [timestamp, text3]) - log.to_s.should eq (Carto::Log::ENTRY_FORMAT % [timestamp, text1]) + - (Carto::Log::ENTRY_FORMAT % [timestamp, text2]) + + log.send(:collect_entries).should eq format(Carto::Log::ENTRY_FORMAT, timestamp, text1) + + format(Carto::Log::ENTRY_FORMAT, timestamp, text2) + + format(Carto::Log::ENTRY_FORMAT, timestamp, text3) + log.to_s.should eq format(Carto::Log::ENTRY_FORMAT, timestamp, text1) + + format(Carto::Log::ENTRY_FORMAT, timestamp, text2) + Carto::Log::HALF_OF_LOG_MARK + - (Carto::Log::ENTRY_FORMAT % [timestamp, text3]) + + format(Carto::Log::ENTRY_FORMAT, timestamp, text3) + Carto::Log::END_OF_LOG_MARK # Check that new entries are added correctly log.append(text4, false, timestamp) - log.send(:collect_entries).should eq (Carto::Log::ENTRY_FORMAT % [timestamp, text1]) + - (Carto::Log::ENTRY_FORMAT % [timestamp, text2]) + - (Carto::Log::ENTRY_FORMAT % [timestamp, text3]) + - (Carto::Log::ENTRY_FORMAT % [timestamp, text4]) - log.to_s.should eq (Carto::Log::ENTRY_FORMAT % [timestamp, text1]) + - (Carto::Log::ENTRY_FORMAT % [timestamp, text2]) + + log.send(:collect_entries).should eq format(Carto::Log::ENTRY_FORMAT, timestamp, text1) + + format(Carto::Log::ENTRY_FORMAT, timestamp, text2) + + format(Carto::Log::ENTRY_FORMAT, timestamp, text3) + + format(Carto::Log::ENTRY_FORMAT, timestamp, text4) + log.to_s.should eq format(Carto::Log::ENTRY_FORMAT, timestamp, text1) + + format(Carto::Log::ENTRY_FORMAT, timestamp, text2) + Carto::Log::HALF_OF_LOG_MARK + - (Carto::Log::ENTRY_FORMAT % [timestamp, text3]) + - (Carto::Log::ENTRY_FORMAT % [timestamp, text4]) + + format(Carto::Log::ENTRY_FORMAT, timestamp, text3) + + format(Carto::Log::ENTRY_FORMAT, timestamp, text4) + Carto::Log::END_OF_LOG_MARK log.append(text5, false, timestamp) log.append(text6, false, timestamp) - log.send(:collect_entries).should eq (Carto::Log::ENTRY_FORMAT % [timestamp, text1]) + - (Carto::Log::ENTRY_FORMAT % [timestamp, text2]) + - (Carto::Log::ENTRY_FORMAT % [timestamp, text5]) + - (Carto::Log::ENTRY_FORMAT % [timestamp, text6]) - log.to_s.should eq (Carto::Log::ENTRY_FORMAT % [timestamp, text1]) + - (Carto::Log::ENTRY_FORMAT % [timestamp, text2]) + + log.send(:collect_entries).should eq format(Carto::Log::ENTRY_FORMAT, timestamp, text1) + + format(Carto::Log::ENTRY_FORMAT, timestamp, text2) + + format(Carto::Log::ENTRY_FORMAT, timestamp, text5) + + format(Carto::Log::ENTRY_FORMAT, timestamp, text6) + log.to_s.should eq format(Carto::Log::ENTRY_FORMAT, timestamp, text1) + + format(Carto::Log::ENTRY_FORMAT, timestamp, text2) + Carto::Log::HALF_OF_LOG_MARK + - (Carto::Log::ENTRY_FORMAT % [timestamp, text5]) + - (Carto::Log::ENTRY_FORMAT % [timestamp, text6]) + + format(Carto::Log::ENTRY_FORMAT, timestamp, text5) + + format(Carto::Log::ENTRY_FORMAT, timestamp, text6) + Carto::Log::END_OF_LOG_MARK log = Carto::Log.new_data_import @@ -158,9 +158,9 @@ log.store log = Carto::Log.find(log.id) - log.send(:collect_entries).should eq(Carto::Log::ENTRY_FORMAT % [timestamp, text1]) + log.send(:collect_entries).should eq format(Carto::Log::ENTRY_FORMAT, timestamp, text1) - log.to_s.should eq (Carto::Log::ENTRY_FORMAT % [timestamp, text1]) + + log.to_s.should eq format(Carto::Log::ENTRY_FORMAT, timestamp, text1) + Carto::Log::END_OF_LOG_MARK # This test checks that old logs with more lines than accepted get truncated correctly @@ -177,41 +177,41 @@ log = Carto::Log.find(log.id) - log.send(:collect_entries).should eq (Carto::Log::ENTRY_FORMAT % [timestamp, text1]) + - (Carto::Log::ENTRY_FORMAT % [timestamp, text2]) + - (Carto::Log::ENTRY_FORMAT % [timestamp, text3]) + - (Carto::Log::ENTRY_FORMAT % [timestamp, text4]) - log.to_s.should eq (Carto::Log::ENTRY_FORMAT % [timestamp, text1]) + - (Carto::Log::ENTRY_FORMAT % [timestamp, text2]) + + log.send(:collect_entries).should eq format(Carto::Log::ENTRY_FORMAT, timestamp, text1) + + format(Carto::Log::ENTRY_FORMAT, timestamp, text2) + + format(Carto::Log::ENTRY_FORMAT, timestamp, text3) + + format(Carto::Log::ENTRY_FORMAT, timestamp, text4) + log.to_s.should eq format(Carto::Log::ENTRY_FORMAT, timestamp, text1) + + format(Carto::Log::ENTRY_FORMAT, timestamp, text2) + Carto::Log::HALF_OF_LOG_MARK + - (Carto::Log::ENTRY_FORMAT % [timestamp, text3]) + - (Carto::Log::ENTRY_FORMAT % [timestamp, text4]) + + format(Carto::Log::ENTRY_FORMAT, timestamp, text3) + + format(Carto::Log::ENTRY_FORMAT, timestamp, text4) + Carto::Log::END_OF_LOG_MARK # Nothing should change with this log.send(:fix_entries_encoding) - log.send(:collect_entries).should eq (Carto::Log::ENTRY_FORMAT % [timestamp, text1]) + - (Carto::Log::ENTRY_FORMAT % [timestamp, text2]) + - (Carto::Log::ENTRY_FORMAT % [timestamp, text3]) + - (Carto::Log::ENTRY_FORMAT % [timestamp, text4]) - log.to_s.should eq (Carto::Log::ENTRY_FORMAT % [timestamp, text1]) + - (Carto::Log::ENTRY_FORMAT % [timestamp, text2]) + + log.send(:collect_entries).should eq format(Carto::Log::ENTRY_FORMAT, timestamp, text1) + + format(Carto::Log::ENTRY_FORMAT, timestamp, text2) + + format(Carto::Log::ENTRY_FORMAT, timestamp, text3) + + format(Carto::Log::ENTRY_FORMAT, timestamp, text4) + log.to_s.should eq format(Carto::Log::ENTRY_FORMAT, timestamp, text1) + + format(Carto::Log::ENTRY_FORMAT, timestamp, text2) + Carto::Log::HALF_OF_LOG_MARK + - (Carto::Log::ENTRY_FORMAT % [timestamp, text3]) + - (Carto::Log::ENTRY_FORMAT % [timestamp, text4]) + + format(Carto::Log::ENTRY_FORMAT, timestamp, text3) + + format(Carto::Log::ENTRY_FORMAT, timestamp, text4) + Carto::Log::END_OF_LOG_MARK # Check that new entries are added correctly log.append(text5, false, timestamp) log.append(text6, false, timestamp) - log.send(:collect_entries).should eq (Carto::Log::ENTRY_FORMAT % [timestamp, text1]) + - (Carto::Log::ENTRY_FORMAT % [timestamp, text2]) + - (Carto::Log::ENTRY_FORMAT % [timestamp, text5]) + - (Carto::Log::ENTRY_FORMAT % [timestamp, text6]) - log.to_s.should eq (Carto::Log::ENTRY_FORMAT % [timestamp, text1]) + - (Carto::Log::ENTRY_FORMAT % [timestamp, text2]) + + log.send(:collect_entries).should eq format(Carto::Log::ENTRY_FORMAT, timestamp, text1) + + format(Carto::Log::ENTRY_FORMAT, timestamp, text2) + + format(Carto::Log::ENTRY_FORMAT, timestamp, text5) + + format(Carto::Log::ENTRY_FORMAT, timestamp, text6) + log.to_s.should eq format(Carto::Log::ENTRY_FORMAT, timestamp, text1) + + format(Carto::Log::ENTRY_FORMAT, timestamp, text2) + Carto::Log::HALF_OF_LOG_MARK + - (Carto::Log::ENTRY_FORMAT % [timestamp, text5]) + - (Carto::Log::ENTRY_FORMAT % [timestamp, text6]) + + format(Carto::Log::ENTRY_FORMAT, timestamp, text5) + + format(Carto::Log::ENTRY_FORMAT, timestamp, text6) + Carto::Log::END_OF_LOG_MARK end diff --git a/spec/models/carto/user_migration_base_spec.rb b/spec/models/carto/user_migration_base_spec.rb index d817dee6f17f..df12d6a9c3d8 100644 --- a/spec/models/carto/user_migration_base_spec.rb +++ b/spec/models/carto/user_migration_base_spec.rb @@ -367,7 +367,9 @@ def teardown_mock_plpython_function(user) export = create(:user_migration_export, user_id: carto_user.id, export_metadata: true) export.run_export - export.log.collect_entries.should_not include("Cannot export if tables aren't synched with db. Please run ghost tables.") + export.log.collect_entries.should_not include( + "Cannot export if tables aren't in sync with db. Please run ghost tables." + ) expect(export.state).to eq(Carto::UserMigrationExport::STATE_COMPLETE) export.destroy From 2c6d5226d89d58adb82669a28c77b2f25b0844e3 Mon Sep 17 00:00:00 2001 From: Jorge Tarrero Date: Thu, 22 Oct 2020 13:47:49 +0200 Subject: [PATCH 3/5] Fixing small linter error --- app/models/carto/user_creation.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/models/carto/user_creation.rb b/app/models/carto/user_creation.rb index c49304899d1e..4f0d7e6c211b 100644 --- a/app/models/carto/user_creation.rb +++ b/app/models/carto/user_creation.rb @@ -204,7 +204,7 @@ def log_transition_end end def log_transition(prefix) - log.append("#{prefix}: State: #{self.state}") + log.append("#{prefix}: State: #{state}") end def initialize_user From f8395f099d5b3e5655e9a24c7da3eaf7138f3173 Mon Sep 17 00:00:00 2001 From: Jorge Tarrero Date: Thu, 22 Oct 2020 14:40:11 +0200 Subject: [PATCH 4/5] Fix test --- spec/models/carto/user_migration_api_key_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/models/carto/user_migration_api_key_spec.rb b/spec/models/carto/user_migration_api_key_spec.rb index 6fe9eeb5a301..6ccb17b4a59f 100644 --- a/spec/models/carto/user_migration_api_key_spec.rb +++ b/spec/models/carto/user_migration_api_key_spec.rb @@ -313,7 +313,7 @@ class DummyTester export = Carto::UserMigrationExport.create(organization: @carto_organization, export_metadata: true) export.run_export - export.log.entries.should_not include("Cannot export if tables aren't synched with db. Please run ghost tables.") + export.log.collect_entries.should_not include("Cannot export if tables aren't synched with db. Please run ghost tables.") expect(export.state).to eq(Carto::UserMigrationExport::STATE_COMPLETE) export.destroy end From 2489b27db671e24eb4a68af4754b9469b2d04c1f Mon Sep 17 00:00:00 2001 From: Jorge Tarrero Date: Fri, 23 Oct 2020 13:52:44 +0200 Subject: [PATCH 5/5] Apply CR suggestions --- app/models/carto/log.rb | 21 +++++++------------ app/models/carto/user_migration_export.rb | 2 +- .../carto/user_migration_api_key_spec.rb | 4 +++- spec/models/carto/user_migration_base_spec.rb | 20 +++++++++--------- 4 files changed, 21 insertions(+), 26 deletions(-) diff --git a/app/models/carto/log.rb b/app/models/carto/log.rb index e9e6da13d310..3129ada9d18d 100644 --- a/app/models/carto/log.rb +++ b/app/models/carto/log.rb @@ -1,6 +1,8 @@ module Carto class Log < ActiveRecord::Base + include ::LoggerHelper + has_one :data_import, class_name: Carto::DataImport, foreign_key: :logger after_initialize :after_initialize_callback @@ -131,27 +133,18 @@ def store @dirty = false end rescue StandardError => e - CartoDB.notify_error( - 'Error appending log, likely an encoding issue', - { error_info: "id: #{id}. #{inspect} --------- #{e.backtrace.join}" } - ) + log_error(message: 'Error appending log, likely an encoding issue', exception: e, log_id: id) begin fix_entries_encoding save rescue StandardError => e - CartoDB.notify_exception( - e, - { message: 'Error saving fallback log info.', error_info: "id: #{id}" } - ) + log_error(message: 'Error saving fallback log info.', exception: e, log_id: id) begin self.entries = "Previous log entries stripped because of an error, check Rollbar. Id: #{id}\n" + END_OF_LOG_MARK save rescue StandardError => e - CartoDB.notify_exception( - e, - { message: 'Error saving stripped fallback log info.', error_info: "id: #{id}" } - ) + log_error(message: 'Error saving stripped fallback log info.', exception: e, log_id: id) end end end @@ -162,7 +155,7 @@ def to_s list = @fixed_entries_half circular_half = ordered_circular_entries_half - list += [HALF_OF_LOG_MARK] unless circular_half.empty? + list += [HALF_OF_LOG_MARK] if circular_half.any? (list + circular_half + [END_OF_LOG_MARK]).join('') end @@ -185,7 +178,7 @@ def rehydrate_entries_from_string(source) existing_entries = source.nil? ? [] : source.split(ENTRY_POSTFIX.to_s) # If rehydrated data, assume nothing changed yet - @dirty = false unless existing_entries.empty? + @dirty = false if existing_entries.any? @fixed_entries_half = existing_entries.slice!(0, half_max_size) .map { |entry| format(ENTRY_REHYDRATED_FORMAT, entry) } diff --git a/app/models/carto/user_migration_export.rb b/app/models/carto/user_migration_export.rb index 90a1d53b90c2..8b51e0ade61e 100644 --- a/app/models/carto/user_migration_export.rb +++ b/app/models/carto/user_migration_export.rb @@ -145,7 +145,7 @@ def validate_export_data end def set_defaults - self.log = Carto::Log.new_user_migration_export unless log + self.log ||= Carto::Log.new_user_migration_export log.save self.state = STATE_PENDING unless state diff --git a/spec/models/carto/user_migration_api_key_spec.rb b/spec/models/carto/user_migration_api_key_spec.rb index 6ccb17b4a59f..13cca031d43a 100644 --- a/spec/models/carto/user_migration_api_key_spec.rb +++ b/spec/models/carto/user_migration_api_key_spec.rb @@ -313,7 +313,9 @@ class DummyTester export = Carto::UserMigrationExport.create(organization: @carto_organization, export_metadata: true) export.run_export - export.log.collect_entries.should_not include("Cannot export if tables aren't synched with db. Please run ghost tables.") + export.log.collect_entries.should_not include( + "Cannot export if tables aren't synched with db. Please run ghost tables." + ) expect(export.state).to eq(Carto::UserMigrationExport::STATE_COMPLETE) export.destroy end diff --git a/spec/models/carto/user_migration_base_spec.rb b/spec/models/carto/user_migration_base_spec.rb index df12d6a9c3d8..fb0431713b11 100644 --- a/spec/models/carto/user_migration_base_spec.rb +++ b/spec/models/carto/user_migration_base_spec.rb @@ -60,7 +60,7 @@ def teardown_mock_plpython_function(user) export = create(:user_migration_export, user_id: carto_user.id, export_data: false) export.run_export - puts export.log.collect_entries if export.state != Carto::UserMigrationExport::STATE_COMPLETE + export.log.collect_entries if export.state != Carto::UserMigrationExport::STATE_COMPLETE expect(export.state).to eq(Carto::UserMigrationExport::STATE_COMPLETE) remove_user(carto_user) @@ -77,7 +77,7 @@ def teardown_mock_plpython_function(user) import.run_import - puts import.log.collect_entries if import.state != Carto::UserMigrationImport::STATE_COMPLETE + import.log.collect_entries if import.state != Carto::UserMigrationImport::STATE_COMPLETE expect(import.state).to eq(Carto::UserMigrationImport::STATE_COMPLETE) carto_user = Carto::User.find(user_attributes['id']) @@ -128,7 +128,7 @@ def teardown_mock_plpython_function(user) export = create(:user_migration_export, user_id: carto_user.id, export_data: false) export.run_export - puts export.log.collect_entries if export.state != Carto::UserMigrationExport::STATE_COMPLETE + export.log.collect_entries if export.state != Carto::UserMigrationExport::STATE_COMPLETE expect(export.state).to eq(Carto::UserMigrationExport::STATE_COMPLETE) remove_user(carto_user) @@ -162,7 +162,7 @@ def teardown_mock_plpython_function(user) export = create(:user_migration_export, organization_id: @carto_organization.id, export_data: false) export.run_export - puts export.log.collect_entries if export.state != Carto::UserMigrationExport::STATE_COMPLETE + export.log.collect_entries if export.state != Carto::UserMigrationExport::STATE_COMPLETE expect(export.state).to eq(Carto::UserMigrationExport::STATE_COMPLETE) database_host = @carto_organization.owner.database_host @@ -182,7 +182,7 @@ def teardown_mock_plpython_function(user) import.run_import - puts import.log.collect_entries if import.state != Carto::UserMigrationImport::STATE_COMPLETE + import.log.collect_entries if import.state != Carto::UserMigrationImport::STATE_COMPLETE expect(import.state).to eq(Carto::UserMigrationImport::STATE_COMPLETE) @carto_organization.users.each do |u| @@ -195,7 +195,7 @@ def teardown_mock_plpython_function(user) export = create(:user_migration_export, organization_id: @carto_organization.id, export_data: false) export.run_export - puts export.log.collect_entries if export.state != Carto::UserMigrationExport::STATE_COMPLETE + export.log.collect_entries if export.state != Carto::UserMigrationExport::STATE_COMPLETE expect(export.state).to eq(Carto::UserMigrationExport::STATE_COMPLETE) database_host = @carto_organization.owner.database_host @@ -242,7 +242,7 @@ def teardown_mock_plpython_function(user) export = create(:user_migration_export, user_id: carto_user.id, export_metadata: true) export.run_export - puts export.log.collect_entries if export.state != Carto::UserMigrationExport::STATE_COMPLETE + export.log.collect_entries if export.state != Carto::UserMigrationExport::STATE_COMPLETE expect(export.state).to eq(Carto::UserMigrationExport::STATE_COMPLETE) carto_user.client_application.destroy @@ -260,7 +260,7 @@ def teardown_mock_plpython_function(user) import.stubs(:assert_user_does_not_exist) import.run_import - puts import.log.collect_entries if import.state != Carto::UserMigrationImport::STATE_COMPLETE + import.log.collect_entries if import.state != Carto::UserMigrationImport::STATE_COMPLETE expect(import.state).to eq(Carto::UserMigrationImport::STATE_COMPLETE) carto_user = Carto::User.find(user_attributes['id']) @@ -291,7 +291,7 @@ def teardown_mock_plpython_function(user) export = create(:user_migration_export, user_id: carto_user.id, export_metadata: true) export.run_export - puts export.log.collect_entries if export.state != Carto::UserMigrationExport::STATE_COMPLETE + export.log.collect_entries if export.state != Carto::UserMigrationExport::STATE_COMPLETE expect(export.state).to eq(Carto::UserMigrationExport::STATE_COMPLETE) user.destroy_cascade @@ -308,7 +308,7 @@ def teardown_mock_plpython_function(user) import.stubs(:assert_user_does_not_exist) import.run_import - puts import.log.collect_entries if import.state != Carto::UserMigrationImport::STATE_COMPLETE + import.log.collect_entries if import.state != Carto::UserMigrationImport::STATE_COMPLETE expect(export.state).to eq(Carto::UserMigrationImport::STATE_COMPLETE) imported_user = Carto::User.find(user_attributes['id']) imported_user.visualizations.count.should eq 3