Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Fix #10856, #10789: `::Logger#silence` and `LoggerSilence` troubles #10859

Closed
wants to merge 2 commits into from

1 participant

Alexey Chernenkov
Alexey Chernenkov
  • Synchronization has been added to LoggerSilence#silence.

  • Fix #10856: search pending migrations among all known migrations, not
    only the latest one. ActiveRecord::Migrator.needs_migration? works
    as expected now.

  • Fix #10789: LoggerSilence included into ::Logger.

  • Revert 3970432: it is very bad idea to use mtime to check if
    there are pending migrations. Suppose you did db:migrate/db:rollback
    several times: checking will stop working at all (db:migrate:status has
    changed but mtime is not).

907th added some commits
Alexey Chernenkov 907th Test for wrong ActiveRecord::Migrator.needs_migration? result. 1e1cb2a
Alexey Chernenkov 907th Fix #10856, #10789: `::Logger#silence` and `LoggerSilence` troubles
* Synchronization has been added to `LoggerSilence#silence`.

* Fix #10856: search pending migrations among all known migrations, not
  only the latest one. `ActiveRecord::Migrator.needs_migration?` works
  as expected now.

* Fix #10789: `LoggerSilence` included into `::Logger`.

* Revert 3970432fcb6a0: it is very bad idea to use mtime to check if
  there are pending migrations. Suppose you did `db:migrate/db:rollback`
  several times: checking will stop work at all (`db:migrate:status` has
  changed but mtime is not).
424dee5
Alexey Chernenkov 907th closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Jun 5, 2013
  1. Alexey Chernenkov
  2. Alexey Chernenkov

    Fix #10856, #10789: `::Logger#silence` and `LoggerSilence` troubles

    907th authored
    * Synchronization has been added to `LoggerSilence#silence`.
    
    * Fix #10856: search pending migrations among all known migrations, not
      only the latest one. `ActiveRecord::Migrator.needs_migration?` works
      as expected now.
    
    * Fix #10789: `LoggerSilence` included into `::Logger`.
    
    * Revert 3970432fcb6a0: it is very bad idea to use mtime to check if
      there are pending migrations. Suppose you did `db:migrate/db:rollback`
      several times: checking will stop work at all (`db:migrate:status` has
      changed but mtime is not).
This page is out of date. Refresh to see the latest.
35 activerecord/lib/active_record/migration.rb
View
@@ -357,14 +357,11 @@ class Migration
class CheckPending
def initialize(app)
@app = app
- @last_check = 0
end
def call(env)
- mtime = ActiveRecord::Migrator.last_migration.mtime.to_i
- if @last_check < mtime
+ ActiveRecord::Base.logger.silence do
ActiveRecord::Migration.check_pending!
- @last_check = mtime
end
@app.call(env)
end
@@ -722,16 +719,6 @@ def load_migration
end
- class NullMigration < MigrationProxy #:nodoc:
- def initialize
- super(nil, 0, nil, nil)
- end
-
- def mtime
- 0
- end
- end
-
class Migrator#:nodoc:
class << self
attr_writer :migrations_paths
@@ -785,28 +772,28 @@ def schema_migrations_table_name
end
def get_all_versions
- SchemaMigration.all.map { |x| x.version.to_i }.sort
- end
-
- def current_version
sm_table = schema_migrations_table_name
if Base.connection.table_exists?(sm_table)
- get_all_versions.max || 0
+ SchemaMigration.all.map { |x| x.version.to_i }.sort
else
- 0
+ []
end
end
+ def current_version
+ get_all_versions.max || 0
+ end
+
def needs_migration?
- current_version < last_version
+ (all_known_versions - get_all_versions).any?
end
def last_version
- last_migration.version
+ all_known_versions.max || 0
end
- def last_migration #:nodoc:
- migrations(migrations_paths).last || NullMigration.new
+ def all_known_versions
+ migrations(migrations_paths).map(&:version).sort
end
def proper_table_name(name)
23 activerecord/test/cases/migration_test.rb
View
@@ -70,12 +70,33 @@ def test_migrator_versions
assert_equal 3, ActiveRecord::Migrator.last_version
assert_equal false, ActiveRecord::Migrator.needs_migration?
- ActiveRecord::Migrator.down(MIGRATIONS_ROOT + "/valid")
+ ActiveRecord::Migrator.down(migrations_path)
assert_equal 0, ActiveRecord::Migrator.current_version
assert_equal 3, ActiveRecord::Migrator.last_version
assert_equal true, ActiveRecord::Migrator.needs_migration?
end
+ def test_need_migration
+ migrations_path = MIGRATIONS_ROOT + "/valid"
+ ActiveRecord::Migrator.migrations_paths = migrations_path
+
+ ActiveRecord::Migrator.up migrations_path
+ assert_equal [1, 2, 3], ActiveRecord::Migrator.get_all_versions
+
+ Reminder.reset_column_information
+ assert Reminder.table_exists?
+
+ ActiveRecord::Migrator.run :down, migrations_path, 2
+ assert_equal [1, 3], ActiveRecord::Migrator.get_all_versions
+
+ Reminder.reset_column_information
+ assert !Reminder.table_exists?
+
+ assert_equal 3, ActiveRecord::Migrator.current_version
+ assert_equal 3, ActiveRecord::Migrator.last_version
+ assert_equal true, ActiveRecord::Migrator.needs_migration?
+ end
+
def test_create_table_with_force_true_does_not_drop_nonexisting_table
if Person.connection.table_exists?(:testings2)
Person.connection.drop_table :testings2
18 activesupport/lib/active_support/logger.rb
View
@@ -2,10 +2,19 @@
require 'active_support/logger_silence'
require 'logger'
+class Logger
+ include LoggerSilence
+
+ # Overwrite initialize to set a default formatter.
+ alias :old_initialize :initialize
+ def initialize(*args)
+ old_initialize(*args)
+ self.formatter = ActiveSupport::Logger::SimpleFormatter.new
+ end
+end
+
module ActiveSupport
class Logger < ::Logger
- include LoggerSilence
-
# Broadcasts logs to multiple loggers.
def self.broadcast(logger) # :nodoc:
Module.new do
@@ -41,11 +50,6 @@ def self.broadcast(logger) # :nodoc:
end
end
- def initialize(*args)
- super
- @formatter = SimpleFormatter.new
- end
-
# Simple formatter which only displays the message.
class SimpleFormatter < ::Logger::Formatter
# This method is invoked when a log event occurs
27 activesupport/lib/active_support/logger_silence.rb
View
@@ -1,8 +1,9 @@
require 'active_support/concern'
+require 'thread'
module LoggerSilence
extend ActiveSupport::Concern
-
+
included do
cattr_accessor :silencer
self.silencer = true
@@ -10,15 +11,23 @@ module LoggerSilence
# Silences the logger for the duration of the block.
def silence(temporary_level = Logger::ERROR)
- if silencer
- begin
- old_logger_level, self.level = level, temporary_level
+ mutex.synchronize do
+ if silencer
+ begin
+ old_logger_level, self.level = level, temporary_level
+ yield self
+ ensure
+ self.level = old_logger_level
+ end
+ else
yield self
- ensure
- self.level = old_logger_level
end
- else
- yield self
end
end
-end
+
+ private
+
+ def mutex
+ @mutex ||= Mutex.new # It seems to be threadsafe in MRI only.
+ end
+end
2  activesupport/test/tagged_logging_test.rb
View
@@ -16,7 +16,7 @@ def flush(*)
test 'sets logger.formatter if missing and extends it with a tagging API' do
logger = Logger.new(StringIO.new)
- assert_nil logger.formatter
+ logger.formatter = nil
ActiveSupport::TaggedLogging.new(logger)
assert_not_nil logger.formatter
assert logger.formatter.respond_to?(:tagged)
1  railties/test/application/configuration_test.rb
View
@@ -62,7 +62,6 @@ def teardown
require "#{app_path}/config/environment"
ActiveRecord::Migrator.stubs(:needs_migration?).returns(true)
- ActiveRecord::NullMigration.any_instance.stubs(:mtime).returns(1)
get "/foo"
assert_equal 500, last_response.status
Something went wrong with that request. Please try again.