From b3420f5a2e3c38e5efc2b3d995354c39af09569e Mon Sep 17 00:00:00 2001 From: Jonathan Viney Date: Sun, 31 Aug 2008 21:09:16 +1200 Subject: Implement savepoints. --- .../abstract/database_statements.rb | 16 ++- .../connection_adapters/abstract_adapter.rb | 15 +++ .../connection_adapters/mysql_adapter.rb | 11 ++ .../connection_adapters/postgresql_adapter.rb | 11 ++ activerecord/lib/active_record/fixtures.rb | 2 + activerecord/lib/active_record/transactions.rb | 12 +- activerecord/test/cases/transactions_test.rb | 132 +++++++++++++++++++++ 7 files changed, 185 insertions(+), 14 deletions(-) diff --git a/activerecord/lib/active_record/connection_adapters/abstract/database_statements.rb b/activerecord/lib/active_record/connection_adapters/abstract/database_statements.rb index 97c6cd4331..f23fe5a90c 100644 --- a/activerecord/lib/active_record/connection_adapters/abstract/database_statements.rb +++ b/activerecord/lib/active_record/connection_adapters/abstract/database_statements.rb @@ -55,12 +55,14 @@ module ActiveRecord end # Wrap a block in a transaction. Returns result of block. - def transaction(start_db_transaction = true) + def transaction(start_db_transaction = false) + start_db_transaction ||= open_transactions.zero? || (open_transactions == 1 && transactional_fixtures) transaction_open = false begin if block_given? if start_db_transaction - begin_db_transaction + open_transactions.zero? ? begin_db_transaction : create_savepoint + increment_open_transactions transaction_open = true end yield @@ -68,21 +70,23 @@ module ActiveRecord rescue Exception => database_transaction_rollback if transaction_open transaction_open = false - rollback_db_transaction + decrement_open_transactions + open_transactions.zero? ? rollback_db_transaction : rollback_to_savepoint end raise unless database_transaction_rollback.is_a? ActiveRecord::Rollback end ensure if transaction_open + decrement_open_transactions begin - commit_db_transaction + open_transactions.zero? ? commit_db_transaction : release_savepoint rescue Exception => database_transaction_rollback - rollback_db_transaction + open_transactions.zero? ? rollback_db_transaction : rollback_to_savepoint raise end end end - + # Begins the transaction (and turns off auto-committing). def begin_db_transaction() end diff --git a/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb b/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb index c5183357a1..cb5c5740c3 100755 --- a/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb +++ b/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb @@ -159,6 +159,21 @@ module ActiveRecord def decrement_open_transactions @open_transactions -= 1 end + + def create_savepoint + end + + def rollback_to_savepoint + end + + def release_savepoint + end + + def current_savepoint_name + "rails_savepoint_#{open_transactions}" + end + + attr_accessor :transactional_fixtures def log_info(sql, name, seconds) if @logger && @logger.debug? diff --git a/activerecord/lib/active_record/connection_adapters/mysql_adapter.rb b/activerecord/lib/active_record/connection_adapters/mysql_adapter.rb index 1e452ae88a..721b365bc7 100644 --- a/activerecord/lib/active_record/connection_adapters/mysql_adapter.rb +++ b/activerecord/lib/active_record/connection_adapters/mysql_adapter.rb @@ -343,6 +343,17 @@ module ActiveRecord # Transactions aren't supported end + def create_savepoint + execute("SAVEPOINT #{current_savepoint_name}") + end + + def rollback_to_savepoint + execute("ROLLBACK TO SAVEPOINT #{current_savepoint_name}") + end + + def release_savepoint + execute("RELEASE SAVEPOINT #{current_savepoint_name}") + end def add_limit_offset!(sql, options) #:nodoc: if limit = options[:limit] diff --git a/activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb b/activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb index 60ec01b95e..f34093f0ff 100644 --- a/activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb +++ b/activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb @@ -567,6 +567,17 @@ module ActiveRecord end end + def create_savepoint + execute("SAVEPOINT #{current_savepoint_name}") + end + + def rollback_to_savepoint + execute("ROLLBACK TO SAVEPOINT #{current_savepoint_name}") + end + + def release_savepoint(savepoint_number) + execute("RELEASE SAVEPOINT #{current_savepoint_name}") + end # SCHEMA STATEMENTS ======================================== diff --git a/activerecord/lib/active_record/fixtures.rb b/activerecord/lib/active_record/fixtures.rb index 114141a646..3d8b0e40a9 100644 --- a/activerecord/lib/active_record/fixtures.rb +++ b/activerecord/lib/active_record/fixtures.rb @@ -932,6 +932,7 @@ module Test #:nodoc: end ActiveRecord::Base.connection.increment_open_transactions ActiveRecord::Base.connection.begin_db_transaction + ActiveRecord::Base.connection.transactional_fixtures = true # Load fixtures for every test. else Fixtures.reset_cache @@ -954,6 +955,7 @@ module Test #:nodoc: if use_transactional_fixtures? && ActiveRecord::Base.connection.open_transactions != 0 ActiveRecord::Base.connection.rollback_db_transaction ActiveRecord::Base.connection.decrement_open_transactions + ActiveRecord::Base.connection.transactional_fixtures = false end ActiveRecord::Base.clear_active_connections! end diff --git a/activerecord/lib/active_record/transactions.rb b/activerecord/lib/active_record/transactions.rb index 27b5aca18f..56b62474d9 100644 --- a/activerecord/lib/active_record/transactions.rb +++ b/activerecord/lib/active_record/transactions.rb @@ -122,14 +122,10 @@ module ActiveRecord # One should restart the entire transaction if a StatementError occurred. module ClassMethods # See ActiveRecord::Transactions::ClassMethods for detailed documentation. - def transaction(&block) - connection.increment_open_transactions - - begin - connection.transaction(connection.open_transactions == 1, &block) - ensure - connection.decrement_open_transactions - end + def transaction(options = {}, &block) + options.assert_valid_keys :force + + connection.transaction(options[:force], &block) end end diff --git a/activerecord/test/cases/transactions_test.rb b/activerecord/test/cases/transactions_test.rb index b12ec36455..27cc841dd3 100644 --- a/activerecord/test/cases/transactions_test.rb +++ b/activerecord/test/cases/transactions_test.rb @@ -213,6 +213,99 @@ class TransactionTest < ActiveRecord::TestCase assert Topic.find(2).approved?, "Second should still be approved" end + def test_invalid_keys_for_transaction + assert_raises ArgumentError do + Topic.transaction :forced => true do + end + end + end + + def test_force_savepoint_in_nested_transaction + Topic.transaction do + @first.approved = true + @second.approved = false + @first.save! + @second.save! + + begin + Topic.transaction :force => true do + @first.happy = false + @first.save! + raise + end + rescue + end + end + + assert @first.reload.approved? + assert !@second.reload.approved? + end + + def test_no_savepoint_in_nested_transaction_without_force + Topic.transaction do + @first.approved = true + @second.approved = false + @first.save! + @second.save! + + begin + Topic.transaction do + @first.approved = false + @first.save! + raise + end + rescue + end + end + + assert !@first.reload.approved? + assert !@second.reload.approved? + end + + def test_many_savepoints + Topic.transaction do + @first.content = "One" + @first.save! + + begin + Topic.transaction :force => true do + @first.content = "Two" + @first.save! + + begin + Topic.transaction :force => true do + @first.content = "Three" + @first.save! + + begin + Topic.transaction :force => true do + @first.content = "Four" + @first.save! + raise + end + rescue + end + + @three = @first.reload.content + raise + end + rescue + end + + @two = @first.reload.content + raise + end + rescue + end + + @one = @first.reload.content + end + + assert_equal "One", @one + assert_equal "Two", @two + assert_equal "Three", @three + end + uses_mocha 'mocking connection.commit_db_transaction' do def test_rollback_when_commit_raises Topic.connection.expects(:begin_db_transaction) @@ -282,6 +375,45 @@ class TransactionTest < ActiveRecord::TestCase end end +class TransactionsWithTransactionalFixturesTest < ActiveRecord::TestCase + self.use_transactional_fixtures = true + fixtures :topics + + def test_automatic_savepoint_in_outer_transaction + @first = Topic.find(1) + + begin + Topic.transaction do + @first.approved = true + @first.save! + raise + end + rescue + assert !@first.reload.approved? + end + end + + def test_no_automatic_savepoint_for_inner_transaction + @first = Topic.find(1) + + Topic.transaction do + @first.approved = true + @first.save! + + begin + Topic.transaction do + @first.approved = false + @first.save! + raise + end + rescue + end + end + + assert !@first.reload.approved? + end +end + if current_adapter?(:PostgreSQLAdapter) class ConcurrentTransactionTest < TransactionTest use_concurrent_connections -- cgit v1.2.3 From b1b204abbb6d010284120598aec30cf5dcd096f6 Mon Sep 17 00:00:00 2001 From: Jonathan Viney Date: Sun, 31 Aug 2008 22:19:59 +1200 Subject: Fix what looks like a Mysql bug with transactions, savepoints, and create table. --- activerecord/test/cases/defaults_test.rb | 47 ++++++++++++++++++-------------- 1 file changed, 26 insertions(+), 21 deletions(-) diff --git a/activerecord/test/cases/defaults_test.rb b/activerecord/test/cases/defaults_test.rb index ee84cb8af8..3c4309753e 100644 --- a/activerecord/test/cases/defaults_test.rb +++ b/activerecord/test/cases/defaults_test.rb @@ -48,8 +48,33 @@ class DefaultTest < ActiveRecord::TestCase ensure klass.connection.drop_table(klass.table_name) rescue nil end + end + + if current_adapter?(:PostgreSQLAdapter, :SQLServerAdapter, :FirebirdAdapter, :OpenBaseAdapter, :OracleAdapter) + def test_default_integers + default = Default.new + assert_instance_of Fixnum, default.positive_integer + assert_equal 1, default.positive_integer + assert_instance_of Fixnum, default.negative_integer + assert_equal -1, default.negative_integer + assert_instance_of BigDecimal, default.decimal_number + assert_equal BigDecimal.new("2.78"), default.decimal_number + end + end + + if current_adapter?(:PostgreSQLAdapter) + def test_multiline_default_text + # older postgres versions represent the default with escapes ("\\012" for a newline) + assert ( "--- []\n\n" == Default.columns_hash['multiline_default'].default || + "--- []\\012\\012" == Default.columns_hash['multiline_default'].default) + end + end +end - +if current_adapter?(:MysqlAdapter) + class DefaultsTestWithoutTransactionalFixtures < ActiveRecord::TestCase + self.use_transactional_fixtures = false + # MySQL uses an implicit default 0 rather than NULL unless in strict mode. # We use an implicit NULL so schema.rb is compatible with other databases. def test_mysql_integer_not_null_defaults @@ -77,24 +102,4 @@ class DefaultTest < ActiveRecord::TestCase klass.connection.drop_table(klass.table_name) rescue nil end end - - if current_adapter?(:PostgreSQLAdapter, :SQLServerAdapter, :FirebirdAdapter, :OpenBaseAdapter, :OracleAdapter) - def test_default_integers - default = Default.new - assert_instance_of Fixnum, default.positive_integer - assert_equal 1, default.positive_integer - assert_instance_of Fixnum, default.negative_integer - assert_equal -1, default.negative_integer - assert_instance_of BigDecimal, default.decimal_number - assert_equal BigDecimal.new("2.78"), default.decimal_number - end - end - - if current_adapter?(:PostgreSQLAdapter) - def test_multiline_default_text - # older postgres versions represent the default with escapes ("\\012" for a newline) - assert ( "--- []\n\n" == Default.columns_hash['multiline_default'].default || - "--- []\\012\\012" == Default.columns_hash['multiline_default'].default) - end - end end -- cgit v1.2.3 From 3f4a8b6d0a6c1ddab417b8d19affdc1e9f9209ba Mon Sep 17 00:00:00 2001 From: Jonathan Viney Date: Sun, 31 Aug 2008 22:34:54 +1200 Subject: Fix assert_queries failures by ignoring savepoint sql. --- activerecord/test/cases/helper.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/activerecord/test/cases/helper.rb b/activerecord/test/cases/helper.rb index f7bdac8013..0c03d29dec 100644 --- a/activerecord/test/cases/helper.rb +++ b/activerecord/test/cases/helper.rb @@ -31,7 +31,7 @@ rescue LoadError end ActiveRecord::Base.connection.class.class_eval do - IGNORED_SQL = [/^PRAGMA/, /^SELECT currval/, /^SELECT CAST/, /^SELECT @@IDENTITY/, /^SELECT @@ROWCOUNT/] + IGNORED_SQL = [/^PRAGMA/, /^SELECT currval/, /^SELECT CAST/, /^SELECT @@IDENTITY/, /^SELECT @@ROWCOUNT/, /^SAVEPOINT/, /^ROLLBACK TO SAVEPOINT/, /^RELEASE SAVEPOINT/] def execute_with_query_record(sql, name = nil, &block) $queries_executed ||= [] -- cgit v1.2.3 From 47b594cc5a2adc86e14a6a3d331583edde56a22f Mon Sep 17 00:00:00 2001 From: "Hongli Lai (Phusion)" Date: Thu, 9 Oct 2008 14:31:23 +0200 Subject: Improve documentation for DatabaseStatements#transactions and AbstractAdapter#transactional_fixtures, especially with regard to support for nested transactions. --- .../abstract/database_statements.rb | 75 ++++++++++++++++++++-- .../connection_adapters/abstract_adapter.rb | 3 + activerecord/lib/active_record/transactions.rb | 2 + 3 files changed, 74 insertions(+), 6 deletions(-) diff --git a/activerecord/lib/active_record/connection_adapters/abstract/database_statements.rb b/activerecord/lib/active_record/connection_adapters/abstract/database_statements.rb index f23fe5a90c..bd434f2efc 100644 --- a/activerecord/lib/active_record/connection_adapters/abstract/database_statements.rb +++ b/activerecord/lib/active_record/connection_adapters/abstract/database_statements.rb @@ -54,14 +54,65 @@ module ActiveRecord delete_sql(sql, name) end - # Wrap a block in a transaction. Returns result of block. + # Runs the given block in a database transaction, and returns the result + # of the block. + # + # == Nested transactions support + # + # Most databases don't support true nested transactions. At the time of + # writing, the only database that supports true nested transactions that + # we're aware of, is MS-SQL. + # + # In order to get around this problem, #transaction will emulate the effect + # of nested transactions, by using savepoints: + # http://dev.mysql.com/doc/refman/5.0/en/savepoints.html + # Savepoints are supported by MySQL and PostgreSQL, but not SQLite3. + # + # It is safe to call this method if a database transaction is already open, + # i.e. if #transaction is called within another #transaction block. In case + # of a nested call, #transaction will behave as follows: + # + # - The block will be run without doing anything. All database statements + # that happen within the block are effectively appended to the already + # open database transaction. + # - However, if +start_db_transaction+ is set to true, then the block will + # be run inside a new database savepoint, effectively making the block + # a sub-transaction. + # - If the #transactional_fixtures attribute is set to true, then the first + # nested call to #transaction will create a new savepoint instead of + # doing nothing. This makes it possible for toplevel transactions in unit + # tests to behave like real transactions, even though a database + # transaction has already been opened. + # + # === Caveats + # + # MySQL doesn't support DDL transactions. If you perform a DDL operation, + # then any created savepoints will be automatically released. For example, + # if you've created a savepoint, then you execute a CREATE TABLE statement, + # then the savepoint that was created will be automatically released. + # + # This means that, on MySQL, you shouldn't execute DDL operations inside + # a #transaction call that you know might create a savepoint. Otherwise, + # #transaction will raise exceptions when it tries to release the + # already-automatically-released savepoints: + # + # Model.connection.transaction do # BEGIN + # Model.connection.transaction(true) do # CREATE SAVEPOINT rails_savepoint_1 + # Model.connection.create_table(...) + # # rails_savepoint_1 now automatically released + # end # RELEASE savepoint rails_savepoint_1 <--- BOOM! database error! + # end def transaction(start_db_transaction = false) - start_db_transaction ||= open_transactions.zero? || (open_transactions == 1 && transactional_fixtures) + start_db_transaction ||= open_transactions == 0 || (open_transactions == 1 && transactional_fixtures) transaction_open = false begin if block_given? if start_db_transaction - open_transactions.zero? ? begin_db_transaction : create_savepoint + if open_transactions == 0 + begin_db_transaction + else + create_savepoint + end increment_open_transactions transaction_open = true end @@ -71,7 +122,11 @@ module ActiveRecord if transaction_open transaction_open = false decrement_open_transactions - open_transactions.zero? ? rollback_db_transaction : rollback_to_savepoint + if open_transactions == 0 + rollback_db_transaction + else + rollback_to_savepoint + end end raise unless database_transaction_rollback.is_a? ActiveRecord::Rollback end @@ -79,9 +134,17 @@ module ActiveRecord if transaction_open decrement_open_transactions begin - open_transactions.zero? ? commit_db_transaction : release_savepoint + if open_transactions == 0 + commit_db_transaction + else + release_savepoint + end rescue Exception => database_transaction_rollback - open_transactions.zero? ? rollback_db_transaction : rollback_to_savepoint + if open_transactions == 0 + rollback_db_transaction + else + rollback_to_savepoint + end raise end end diff --git a/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb b/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb index cb5c5740c3..45a6cff346 100755 --- a/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb +++ b/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb @@ -173,6 +173,9 @@ module ActiveRecord "rails_savepoint_#{open_transactions}" end + # Whether this AbstractAdapter is currently being used inside a unit test + # with transactional fixtures turned on. See DatabaseStatements#transaction + # for more information about the effect of this option. attr_accessor :transactional_fixtures def log_info(sql, name, seconds) diff --git a/activerecord/lib/active_record/transactions.rb b/activerecord/lib/active_record/transactions.rb index 56b62474d9..4bac7b39d9 100644 --- a/activerecord/lib/active_record/transactions.rb +++ b/activerecord/lib/active_record/transactions.rb @@ -125,6 +125,8 @@ module ActiveRecord def transaction(options = {}, &block) options.assert_valid_keys :force + # See the API documentation for ConnectionAdapters::DatabaseStatements#transaction + # for useful information. connection.transaction(options[:force], &block) end end -- cgit v1.2.3 From f48703e8c6ad8497795a67708ace4eb507a91119 Mon Sep 17 00:00:00 2001 From: "Hongli Lai (Phusion)" Date: Thu, 9 Oct 2008 14:35:42 +0200 Subject: Fix the final MySQL unit test failure that's related to savepoint support. --- activerecord/test/cases/defaults_test.rb | 67 +++++++++++++++++++------------- 1 file changed, 39 insertions(+), 28 deletions(-) diff --git a/activerecord/test/cases/defaults_test.rb b/activerecord/test/cases/defaults_test.rb index 3c4309753e..875b7f8dbc 100644 --- a/activerecord/test/cases/defaults_test.rb +++ b/activerecord/test/cases/defaults_test.rb @@ -20,34 +20,7 @@ class DefaultTest < ActiveRecord::TestCase if current_adapter?(:MysqlAdapter) - #MySQL 5 and higher is quirky with not null text/blob columns. - #With MySQL Text/blob columns cannot have defaults. If the column is not null MySQL will report that the column has a null default - #but it behaves as though the column had a default of '' - def test_mysql_text_not_null_defaults - klass = Class.new(ActiveRecord::Base) - klass.table_name = 'test_mysql_text_not_null_defaults' - klass.connection.create_table klass.table_name do |t| - t.column :non_null_text, :text, :null => false - t.column :non_null_blob, :blob, :null => false - t.column :null_text, :text, :null => true - t.column :null_blob, :blob, :null => true - end - assert_equal '', klass.columns_hash['non_null_blob'].default - assert_equal '', klass.columns_hash['non_null_text'].default - - assert_equal nil, klass.columns_hash['null_blob'].default - assert_equal nil, klass.columns_hash['null_text'].default - - assert_nothing_raised do - instance = klass.create! - assert_equal '', instance.non_null_text - assert_equal '', instance.non_null_blob - assert_nil instance.null_text - assert_nil instance.null_blob - end - ensure - klass.connection.drop_table(klass.table_name) rescue nil - end + end if current_adapter?(:PostgreSQLAdapter, :SQLServerAdapter, :FirebirdAdapter, :OpenBaseAdapter, :OracleAdapter) @@ -73,8 +46,46 @@ end if current_adapter?(:MysqlAdapter) class DefaultsTestWithoutTransactionalFixtures < ActiveRecord::TestCase + # ActiveRecord::Base#create! (and #save and other related methods) will + # open a new transaction. When in transactional fixtures mode, this will + # cause ActiveRecord to create a new savepoint. However, since MySQL doesn't + # support DDL transactions, creating a table will result in any created + # savepoints to be automatically released. This in turn causes the savepoint + # release code in AbstractAdapter#transaction to fail. + # + # We don't want that to happen, so we disable transactional fixtures here. self.use_transactional_fixtures = false + # MySQL 5 and higher is quirky with not null text/blob columns. + # With MySQL Text/blob columns cannot have defaults. If the column is not + # null MySQL will report that the column has a null default + # but it behaves as though the column had a default of '' + def test_mysql_text_not_null_defaults + klass = Class.new(ActiveRecord::Base) + klass.table_name = 'test_mysql_text_not_null_defaults' + klass.connection.create_table klass.table_name do |t| + t.column :non_null_text, :text, :null => false + t.column :non_null_blob, :blob, :null => false + t.column :null_text, :text, :null => true + t.column :null_blob, :blob, :null => true + end + assert_equal '', klass.columns_hash['non_null_blob'].default + assert_equal '', klass.columns_hash['non_null_text'].default + + assert_equal nil, klass.columns_hash['null_blob'].default + assert_equal nil, klass.columns_hash['null_text'].default + + assert_nothing_raised do + instance = klass.create! + assert_equal '', instance.non_null_text + assert_equal '', instance.non_null_blob + assert_nil instance.null_text + assert_nil instance.null_blob + end + ensure + klass.connection.drop_table(klass.table_name) rescue nil + end + # MySQL uses an implicit default 0 rather than NULL unless in strict mode. # We use an implicit NULL so schema.rb is compatible with other databases. def test_mysql_integer_not_null_defaults -- cgit v1.2.3 From e383835e738a30cd992c322eff126380bd15094f Mon Sep 17 00:00:00 2001 From: "Hongli Lai (Phusion)" Date: Thu, 9 Oct 2008 14:47:43 +0200 Subject: Revert "PostgreSQL: introduce transaction_active? rather than tracking activity ourselves" This commit conflicts with savepoint support. This reverts commit 045713ee240fff815edb5962b25d668512649478. Conflicts: activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb --- .../connection_adapters/postgresql_adapter.rb | 38 ---------------------- 1 file changed, 38 deletions(-) diff --git a/activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb b/activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb index f34093f0ff..98501db816 100644 --- a/activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb +++ b/activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb @@ -529,44 +529,6 @@ module ActiveRecord execute "ROLLBACK" end - # ruby-pg defines Ruby constants for transaction status, - # ruby-postgres does not. - PQTRANS_IDLE = defined?(PGconn::PQTRANS_IDLE) ? PGconn::PQTRANS_IDLE : 0 - - # Check whether a transaction is active. - def transaction_active? - @connection.transaction_status != PQTRANS_IDLE - end - - # Wrap a block in a transaction. Returns result of block. - def transaction(start_db_transaction = true) - transaction_open = false - begin - if block_given? - if start_db_transaction - begin_db_transaction - transaction_open = true - end - yield - end - rescue Exception => database_transaction_rollback - if transaction_open && transaction_active? - transaction_open = false - rollback_db_transaction - end - raise unless database_transaction_rollback.is_a? ActiveRecord::Rollback - end - ensure - if transaction_open && transaction_active? - begin - commit_db_transaction - rescue Exception => database_transaction_rollback - rollback_db_transaction - raise - end - end - end - def create_savepoint execute("SAVEPOINT #{current_savepoint_name}") end -- cgit v1.2.3 From e981eaaf342d06e399b5138553c964adcfadd87c Mon Sep 17 00:00:00 2001 From: "Hongli Lai (Phusion)" Date: Thu, 9 Oct 2008 14:52:02 +0200 Subject: Fix a stale typo in the PostgreSQL adapter. Fix a stale mock expection in transaction_test. --- .../lib/active_record/connection_adapters/postgresql_adapter.rb | 2 +- activerecord/test/cases/transactions_test.rb | 1 - 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb b/activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb index 98501db816..cfeef30d06 100644 --- a/activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb +++ b/activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb @@ -537,7 +537,7 @@ module ActiveRecord execute("ROLLBACK TO SAVEPOINT #{current_savepoint_name}") end - def release_savepoint(savepoint_number) + def release_savepoint execute("RELEASE SAVEPOINT #{current_savepoint_name}") end diff --git a/activerecord/test/cases/transactions_test.rb b/activerecord/test/cases/transactions_test.rb index 27cc841dd3..4bbcd272b7 100644 --- a/activerecord/test/cases/transactions_test.rb +++ b/activerecord/test/cases/transactions_test.rb @@ -309,7 +309,6 @@ class TransactionTest < ActiveRecord::TestCase uses_mocha 'mocking connection.commit_db_transaction' do def test_rollback_when_commit_raises Topic.connection.expects(:begin_db_transaction) - Topic.connection.expects(:transaction_active?).returns(true) if current_adapter?(:PostgreSQLAdapter) Topic.connection.expects(:commit_db_transaction).raises('OH NOES') Topic.connection.expects(:rollback_db_transaction) -- cgit v1.2.3 From 885c11b8f9e18f34b12076023455e72166365f00 Mon Sep 17 00:00:00 2001 From: "Hongli Lai (Phusion)" Date: Thu, 9 Oct 2008 15:41:56 +0200 Subject: Make SQLite3 pass the unit tests for savepoints. --- .../lib/active_record/connection_adapters/abstract_adapter.rb | 6 ++++++ .../lib/active_record/connection_adapters/mysql_adapter.rb | 4 ++++ .../lib/active_record/connection_adapters/postgresql_adapter.rb | 4 ++++ activerecord/test/cases/transactions_test.rb | 8 ++++---- 4 files changed, 18 insertions(+), 4 deletions(-) diff --git a/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb b/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb index 45a6cff346..81260eeecc 100755 --- a/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb +++ b/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb @@ -65,6 +65,12 @@ module ActiveRecord def supports_ddl_transactions? false end + + # Does this adapter support savepoints? PostgreSQL and MySQL do, SQLite + # does not. + def supports_savepoints? + false + end # Should primary key values be selected from their corresponding # sequence before the insert statement? If true, next_sequence_value diff --git a/activerecord/lib/active_record/connection_adapters/mysql_adapter.rb b/activerecord/lib/active_record/connection_adapters/mysql_adapter.rb index 721b365bc7..76ade2a980 100644 --- a/activerecord/lib/active_record/connection_adapters/mysql_adapter.rb +++ b/activerecord/lib/active_record/connection_adapters/mysql_adapter.rb @@ -205,6 +205,10 @@ module ActiveRecord def supports_migrations? #:nodoc: true end + + def supports_savepoints? #:nodoc: + true + end def native_database_types #:nodoc: NATIVE_DATABASE_TYPES diff --git a/activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb b/activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb index cfeef30d06..828f321767 100644 --- a/activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb +++ b/activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb @@ -272,6 +272,10 @@ module ActiveRecord def supports_ddl_transactions? true end + + def supports_savepoints? + true + end # Returns the configured supported identifier length supported by PostgreSQL, # or report the default of 63 on PostgreSQL 7.x. diff --git a/activerecord/test/cases/transactions_test.rb b/activerecord/test/cases/transactions_test.rb index 4bbcd272b7..3c6bfd7e22 100644 --- a/activerecord/test/cases/transactions_test.rb +++ b/activerecord/test/cases/transactions_test.rb @@ -239,7 +239,7 @@ class TransactionTest < ActiveRecord::TestCase assert @first.reload.approved? assert !@second.reload.approved? - end + end if Topic.connection.supports_savepoints? def test_no_savepoint_in_nested_transaction_without_force Topic.transaction do @@ -260,7 +260,7 @@ class TransactionTest < ActiveRecord::TestCase assert !@first.reload.approved? assert !@second.reload.approved? - end + end if Topic.connection.supports_savepoints? def test_many_savepoints Topic.transaction do @@ -304,7 +304,7 @@ class TransactionTest < ActiveRecord::TestCase assert_equal "One", @one assert_equal "Two", @two assert_equal "Three", @three - end + end if Topic.connection.supports_savepoints? uses_mocha 'mocking connection.commit_db_transaction' do def test_rollback_when_commit_raises @@ -411,7 +411,7 @@ class TransactionsWithTransactionalFixturesTest < ActiveRecord::TestCase assert !@first.reload.approved? end -end +end if Topic.connection.supports_savepoints? if current_adapter?(:PostgreSQLAdapter) class ConcurrentTransactionTest < TransactionTest -- cgit v1.2.3 From e916aa7ea1613be966959c05ad41d13fee55a683 Mon Sep 17 00:00:00 2001 From: "Hongli Lai (Phusion)" Date: Thu, 9 Oct 2008 16:24:15 +0200 Subject: Rename ActiveRecord::Base#transaction's :force option to :nest. Improve documentation for nested transactions. --- activerecord/lib/active_record/transactions.rb | 59 +++++++++++++++++++++++++- activerecord/test/cases/transactions_test.rb | 10 ++--- 2 files changed, 62 insertions(+), 7 deletions(-) diff --git a/activerecord/lib/active_record/transactions.rb b/activerecord/lib/active_record/transactions.rb index 4bac7b39d9..1e0185d39c 100644 --- a/activerecord/lib/active_record/transactions.rb +++ b/activerecord/lib/active_record/transactions.rb @@ -120,14 +120,69 @@ module ActiveRecord # end # # One should restart the entire transaction if a StatementError occurred. + # + # == Nested transactions + # + # #transaction calls can be nested. By default, this makes all database + # statements in the nested transaction block become part of the parent + # transaction. For example: + # + # User.transaction do + # User.create(:username => 'Kotori') + # User.transaction do + # User.create(:username => 'Nemu') + # raise ActiveRecord::Rollback + # end + # end + # + # User.find(:all) # => empty + # + # It is also possible to treat a certain #transaction call as its own + # sub-transaction, by passing :nest => true to #transaction. If + # anything goes wrong inside that transaction block, then the parent + # transaction will remain unaffected. For example: + # + # User.transaction do + # User.create(:username => 'Kotori') + # User.transaction(:nest => true) do + # User.create(:username => 'Nemu') + # raise ActiveRecord::Rollback + # end + # end + # + # User.find(:all) # => Returns only Kotori + # + # Most databases don't support true nested transactions. At the time of + # writing, the only database that we're aware of that supports true nested + # transactions, is MS-SQL. Because of this, Active Record emulates nested + # transactions by using savepoints. See + # http://dev.mysql.com/doc/refman/5.0/en/savepoints.html + # for more information about savepoints. + # + # === Caveats + # + # If you're on MySQL, then do not use DDL operations in nested transactions + # blocks that are emulated with savepoints. That is, do not execute statements + # like 'CREATE TABLE' inside such blocks. This is because MySQL automatically + # releases all savepoints upon executing a DDL operation. When #transaction + # is finished and tries to release the savepoint it created earlier, a + # database error will occur because the savepoint has already been + # automatically released. The following example demonstrates the problem: + # + # Model.connection.transaction do # BEGIN + # Model.connection.transaction(true) do # CREATE SAVEPOINT rails_savepoint_1 + # Model.connection.create_table(...) # rails_savepoint_1 now automatically released + # end # RELEASE savepoint rails_savepoint_1 + # # ^^^^ BOOM! database error! + # end module ClassMethods # See ActiveRecord::Transactions::ClassMethods for detailed documentation. def transaction(options = {}, &block) - options.assert_valid_keys :force + options.assert_valid_keys :nest # See the API documentation for ConnectionAdapters::DatabaseStatements#transaction # for useful information. - connection.transaction(options[:force], &block) + connection.transaction(options[:nest], &block) end end diff --git a/activerecord/test/cases/transactions_test.rb b/activerecord/test/cases/transactions_test.rb index 3c6bfd7e22..069ba9d6c5 100644 --- a/activerecord/test/cases/transactions_test.rb +++ b/activerecord/test/cases/transactions_test.rb @@ -215,7 +215,7 @@ class TransactionTest < ActiveRecord::TestCase def test_invalid_keys_for_transaction assert_raises ArgumentError do - Topic.transaction :forced => true do + Topic.transaction :nested => true do end end end @@ -228,7 +228,7 @@ class TransactionTest < ActiveRecord::TestCase @second.save! begin - Topic.transaction :force => true do + Topic.transaction :nest => true do @first.happy = false @first.save! raise @@ -268,17 +268,17 @@ class TransactionTest < ActiveRecord::TestCase @first.save! begin - Topic.transaction :force => true do + Topic.transaction :nest => true do @first.content = "Two" @first.save! begin - Topic.transaction :force => true do + Topic.transaction :nest => true do @first.content = "Three" @first.save! begin - Topic.transaction :force => true do + Topic.transaction :nest => true do @first.content = "Four" @first.save! raise -- cgit v1.2.3 From fb2325e35855d62abd2c76ce03feaa3ca7992e4f Mon Sep 17 00:00:00 2001 From: "Hongli Lai (Phusion)" Date: Thu, 9 Oct 2008 17:57:49 +0200 Subject: Reimplement Jeremy's PostgreSQL automatic transaction state introspection code. - Fixed compatibility with the old 'postgres' driver which doesn't support transaction state introspection. - Added unit tests for it. --- .../abstract/database_statements.rb | 20 +++++++++++-- .../connection_adapters/postgresql_adapter.rb | 10 +++++++ activerecord/test/cases/transactions_test.rb | 34 ++++++++++++++++++++++ 3 files changed, 62 insertions(+), 2 deletions(-) diff --git a/activerecord/lib/active_record/connection_adapters/abstract/database_statements.rb b/activerecord/lib/active_record/connection_adapters/abstract/database_statements.rb index bd434f2efc..a9a63e5a9f 100644 --- a/activerecord/lib/active_record/connection_adapters/abstract/database_statements.rb +++ b/activerecord/lib/active_record/connection_adapters/abstract/database_statements.rb @@ -53,6 +53,20 @@ module ActiveRecord def delete(sql, name = nil) delete_sql(sql, name) end + + # Checks whether there is currently no transaction active. This is done + # by querying the database driver, and does not use the transaction + # house-keeping information recorded by #increment_open_transactions and + # friends. + # + # Returns true if there is no transaction active, false if there is a + # transaction active, and nil if this information is unknown. + # + # Not all adapters supports transaction state introspection. Currently, + # only the PostgreSQL adapter supports this. + def outside_transaction? + nil + end # Runs the given block in a database transaction, and returns the result # of the block. @@ -119,7 +133,7 @@ module ActiveRecord yield end rescue Exception => database_transaction_rollback - if transaction_open + if transaction_open && !outside_transaction? transaction_open = false decrement_open_transactions if open_transactions == 0 @@ -131,7 +145,9 @@ module ActiveRecord raise unless database_transaction_rollback.is_a? ActiveRecord::Rollback end ensure - if transaction_open + if outside_transaction? + @open_transactions = 0 + elsif transaction_open decrement_open_transactions begin if open_transactions == 0 diff --git a/activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb b/activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb index 828f321767..c4ef2be82e 100644 --- a/activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb +++ b/activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb @@ -532,6 +532,16 @@ module ActiveRecord def rollback_db_transaction execute "ROLLBACK" end + + if PGconn.public_method_defined?(:transaction_status) + # ruby-pg defines Ruby constants for transaction status, + # ruby-postgres does not. + PQTRANS_IDLE = defined?(PGconn::PQTRANS_IDLE) ? PGconn::PQTRANS_IDLE : 0 + + def outside_transaction? + @connection.transaction_status == PQTRANS_IDLE + end + end def create_savepoint execute("SAVEPOINT #{current_savepoint_name}") diff --git a/activerecord/test/cases/transactions_test.rb b/activerecord/test/cases/transactions_test.rb index 069ba9d6c5..0c69fee8f2 100644 --- a/activerecord/test/cases/transactions_test.rb +++ b/activerecord/test/cases/transactions_test.rb @@ -310,6 +310,7 @@ class TransactionTest < ActiveRecord::TestCase def test_rollback_when_commit_raises Topic.connection.expects(:begin_db_transaction) Topic.connection.expects(:commit_db_transaction).raises('OH NOES') + Topic.connection.expects(:outside_transaction?).returns(false) Topic.connection.expects(:rollback_db_transaction) assert_raise RuntimeError do @@ -319,6 +320,39 @@ class TransactionTest < ActiveRecord::TestCase end end end + + if current_adapter?(:PostgreSQLAdapter) && PGconn.public_method_defined?(:transaction_status) + def test_outside_transaction_works + Topic.logger.info("-------------") + assert Topic.connection.outside_transaction? + Topic.connection.begin_db_transaction + assert !Topic.connection.outside_transaction? + Topic.connection.rollback_db_transaction + assert Topic.connection.outside_transaction? + end + + uses_mocha 'mocking connection.rollback_db_transaction' do + def test_rollback_wont_be_executed_if_no_transaction_active + assert_raise RuntimeError do + Topic.transaction do + Topic.connection.rollback_db_transaction + Topic.connection.expects(:rollback_db_transaction).never + raise "Rails doesn't scale!" + end + end + end + end + + def test_open_transactions_count_is_reset_to_zero_if_no_transaction_active + Topic.transaction do + Topic.transaction do + Topic.connection.rollback_db_transaction + end + assert_equal 0, Topic.connection.open_transactions + end + assert_equal 0, Topic.connection.open_transactions + end + end def test_sqlite_add_column_in_transaction_raises_statement_invalid return true unless current_adapter?(:SQLite3Adapter, :SQLiteAdapter) -- cgit v1.2.3 From 92dbf5ba832c2c4d8f6fda8b151090069cd701f3 Mon Sep 17 00:00:00 2001 From: Joshua Peek Date: Sat, 10 Jan 2009 11:32:38 -0600 Subject: Refactor request multipart params parsing tests --- .../test/controller/integration_upload_test.rb | 65 -------- .../request/multipart_params_parsing_test.rb | 167 +++++++++++++++++++++ actionpack/test/controller/request_test.rb | 104 ------------- 3 files changed, 167 insertions(+), 169 deletions(-) delete mode 100644 actionpack/test/controller/integration_upload_test.rb create mode 100644 actionpack/test/controller/request/multipart_params_parsing_test.rb diff --git a/actionpack/test/controller/integration_upload_test.rb b/actionpack/test/controller/integration_upload_test.rb deleted file mode 100644 index d579980c19..0000000000 --- a/actionpack/test/controller/integration_upload_test.rb +++ /dev/null @@ -1,65 +0,0 @@ -require 'abstract_unit' - -unless defined? ApplicationController - class ApplicationController < ActionController::Base - end -end - -class UploadTestController < ActionController::Base - def update - SessionUploadTest.last_request_type = ActionController::Base.param_parsers[request.content_type] - render :text => "got here" - end - - def read - render :text => "File: #{params[:uploaded_data].read}" - end -end - -class SessionUploadTest < ActionController::IntegrationTest - FILES_DIR = File.dirname(__FILE__) + '/../fixtures/multipart' - - class << self - attr_accessor :last_request_type - end - - def test_upload_and_read_file - with_test_routing do - post '/read', :uploaded_data => fixture_file_upload(FILES_DIR + "/hello.txt", "text/plain") - assert_equal "File: Hello", response.body - end - end - - # The lint wrapper is used in integration tests - # instead of a normal StringIO class - InputWrapper = Rack::Lint::InputWrapper - - def test_post_with_upload_with_unrewindable_input - InputWrapper.any_instance.expects(:rewind).raises(Errno::ESPIPE) - - with_test_routing do - post '/read', :uploaded_data => fixture_file_upload(FILES_DIR + "/hello.txt", "text/plain") - assert_equal "File: Hello", response.body - end - end - - def test_post_with_upload_with_params_parsing - with_test_routing do - params = { :uploaded_data => fixture_file_upload(FILES_DIR + "/mona_lisa.jpg", "image/jpg") } - post '/update', params, :location => 'blah' - assert_equal(:multipart_form, SessionUploadTest.last_request_type) - end - end - - private - def with_test_routing - with_routing do |set| - set.draw do |map| - map.update 'update', :controller => "upload_test", :action => "update", :method => :post - map.read 'read', :controller => "upload_test", :action => "read", :method => :post - end - - yield - end - end -end diff --git a/actionpack/test/controller/request/multipart_params_parsing_test.rb b/actionpack/test/controller/request/multipart_params_parsing_test.rb new file mode 100644 index 0000000000..03ab164972 --- /dev/null +++ b/actionpack/test/controller/request/multipart_params_parsing_test.rb @@ -0,0 +1,167 @@ +require 'abstract_unit' + +class MultipartParamsParsingTest < ActionController::IntegrationTest + class TestController < ActionController::Base + class << self + attr_accessor :last_request_parameters, :last_request_type + end + + def parse + self.class.last_request_type = ActionController::Base.param_parsers[request.content_type] + self.class.last_request_parameters = request.request_parameters + head :ok + end + + def read + render :text => "File: #{params[:uploaded_data].read}" + end + end + + FIXTURE_PATH = File.dirname(__FILE__) + '/../../fixtures/multipart' + + def teardown + TestController.last_request_parameters = nil + TestController.last_request_type = nil + end + + test "parses single parameter" do + assert_equal({ 'foo' => 'bar' }, parse_multipart('single_parameter')) + end + + test "parses bracketed parameters" do + assert_equal({ 'foo' => { 'baz' => 'bar'}}, parse_multipart('bracketed_param')) + end + + test "parses text file" do + params = parse_multipart('text_file') + assert_equal %w(file foo), params.keys.sort + assert_equal 'bar', params['foo'] + + file = params['file'] + assert_kind_of StringIO, file + assert_equal 'file.txt', file.original_filename + assert_equal "text/plain", file.content_type + assert_equal 'contents', file.read + end + + test "parses boundary problem file" do + params = parse_multipart('boundary_problem_file') + assert_equal %w(file foo), params.keys.sort + + file = params['file'] + foo = params['foo'] + + assert_kind_of Tempfile, file + + assert_equal 'file.txt', file.original_filename + assert_equal "text/plain", file.content_type + + assert_equal 'bar', foo + end + + test "parses large text file" do + params = parse_multipart('large_text_file') + assert_equal %w(file foo), params.keys.sort + assert_equal 'bar', params['foo'] + + file = params['file'] + + assert_kind_of Tempfile, file + + assert_equal 'file.txt', file.original_filename + assert_equal "text/plain", file.content_type + assert ('a' * 20480) == file.read + end + + test "parses binary file" do + params = parse_multipart('binary_file') + assert_equal %w(file flowers foo), params.keys.sort + assert_equal 'bar', params['foo'] + + file = params['file'] + assert_kind_of StringIO, file + assert_equal 'file.csv', file.original_filename + assert_nil file.content_type + assert_equal 'contents', file.read + + file = params['flowers'] + assert_kind_of StringIO, file + assert_equal 'flowers.jpg', file.original_filename + assert_equal "image/jpeg", file.content_type + assert_equal 19512, file.size + end + + test "parses mixed files" do + params = parse_multipart('mixed_files') + assert_equal %w(files foo), params.keys.sort + assert_equal 'bar', params['foo'] + + # Ruby CGI doesn't handle multipart/mixed for us. + files = params['files'] + assert_kind_of String, files + files.force_encoding('ASCII-8BIT') if files.respond_to?(:force_encoding) + assert_equal 19756, files.size + end + + test "uploads and parses parameters" do + with_test_routing do + params = { :uploaded_data => fixture_file_upload(FIXTURE_PATH + "/mona_lisa.jpg", "image/jpg") } + post '/parse', params, :location => 'blah' + assert_equal(:multipart_form, TestController.last_request_type) + end + end + + test "uploads and reads file" do + with_test_routing do + post '/read', :uploaded_data => fixture_file_upload(FIXTURE_PATH + "/hello.txt", "text/plain") + assert_equal "File: Hello", response.body + end + end + + # The lint wrapper is used in integration tests + # instead of a normal StringIO class + InputWrapper = Rack::Lint::InputWrapper + + test "parses unwindable stream" do + InputWrapper.any_instance.expects(:rewind).raises(Errno::ESPIPE) + params = parse_multipart('large_text_file') + assert_equal %w(file foo), params.keys.sort + assert_equal 'bar', params['foo'] + end + + test "uploads and reads file with unwindable input" do + InputWrapper.any_instance.expects(:rewind).raises(Errno::ESPIPE) + + with_test_routing do + post '/read', :uploaded_data => fixture_file_upload(FIXTURE_PATH + "/hello.txt", "text/plain") + assert_equal "File: Hello", response.body + end + end + + private + def fixture(name) + File.open(File.join(FIXTURE_PATH, name), 'rb') do |file| + { "rack.input" => file.read, + "CONTENT_TYPE" => "multipart/form-data; boundary=AaB03x", + "CONTENT_LENGTH" => file.stat.size.to_s } + end + end + + def parse_multipart(name) + with_test_routing do + headers = fixture(name) + post "/parse", headers.delete("rack.input"), headers + assert_response :ok + TestController.last_request_parameters + end + end + + def with_test_routing + with_routing do |set| + set.draw do |map| + map.connect ':action', :controller => "multipart_params_parsing_test/test" + end + yield + end + end +end diff --git a/actionpack/test/controller/request_test.rb b/actionpack/test/controller/request_test.rb index c53f1bc2d9..3256774b6d 100644 --- a/actionpack/test/controller/request_test.rb +++ b/actionpack/test/controller/request_test.rb @@ -606,107 +606,3 @@ class UrlEncodedRequestParameterParsingTest < ActiveSupport::TestCase assert_equal expected, ActionController::RequestParser.parse_request_parameters(input) end end - -class MultipartRequestParameterParsingTest < ActiveSupport::TestCase - FIXTURE_PATH = File.dirname(__FILE__) + '/../fixtures/multipart' - - def test_single_parameter - params = parse_multipart('single_parameter') - assert_equal({ 'foo' => 'bar' }, params) - end - - def test_bracketed_param - assert_equal({ 'foo' => { 'baz' => 'bar'}}, parse_multipart('bracketed_param')) - end - - def test_text_file - params = parse_multipart('text_file') - assert_equal %w(file foo), params.keys.sort - assert_equal 'bar', params['foo'] - - file = params['file'] - assert_kind_of StringIO, file - assert_equal 'file.txt', file.original_filename - assert_equal "text/plain", file.content_type - assert_equal 'contents', file.read - end - - def test_boundary_problem_file - params = parse_multipart('boundary_problem_file') - assert_equal %w(file foo), params.keys.sort - - file = params['file'] - foo = params['foo'] - - assert_kind_of Tempfile, file - - assert_equal 'file.txt', file.original_filename - assert_equal "text/plain", file.content_type - - assert_equal 'bar', foo - end - - def test_large_text_file - params = parse_multipart('large_text_file') - assert_equal %w(file foo), params.keys.sort - assert_equal 'bar', params['foo'] - - file = params['file'] - - assert_kind_of Tempfile, file - - assert_equal 'file.txt', file.original_filename - assert_equal "text/plain", file.content_type - assert ('a' * 20480) == file.read - end - - uses_mocha "test_no_rewind_stream" do - def test_no_rewind_stream - # Ensures that parse_multipart_form_parameters works with streams that cannot be rewound - file = File.open(File.join(FIXTURE_PATH, 'large_text_file'), 'rb') - file.expects(:rewind).raises(Errno::ESPIPE) - params = ActionController::RequestParser.parse_multipart_form_parameters(file, 'AaB03x', file.stat.size, {}) - assert_not_equal 0, file.pos # file was not rewound after reading - end - end - - def test_binary_file - params = parse_multipart('binary_file') - assert_equal %w(file flowers foo), params.keys.sort - assert_equal 'bar', params['foo'] - - file = params['file'] - assert_kind_of StringIO, file - assert_equal 'file.csv', file.original_filename - assert_nil file.content_type - assert_equal 'contents', file.read - - file = params['flowers'] - assert_kind_of StringIO, file - assert_equal 'flowers.jpg', file.original_filename - assert_equal "image/jpeg", file.content_type - assert_equal 19512, file.size - #assert_equal File.read(File.dirname(__FILE__) + '/../../../activerecord/test/fixtures/flowers.jpg'), file.read - end - - def test_mixed_files - params = parse_multipart('mixed_files') - assert_equal %w(files foo), params.keys.sort - assert_equal 'bar', params['foo'] - - # Ruby CGI doesn't handle multipart/mixed for us. - files = params['files'] - assert_kind_of String, files - files.force_encoding('ASCII-8BIT') if files.respond_to?(:force_encoding) - assert_equal 19756, files.size - end - - private - def parse_multipart(name) - File.open(File.join(FIXTURE_PATH, name), 'rb') do |file| - params = ActionController::RequestParser.parse_multipart_form_parameters(file, 'AaB03x', file.stat.size, {}) - assert_equal 0, file.pos # file was rewound after reading - params - end - end -end -- cgit v1.2.3 From 9fe69b225cfbf12c02ee1433adf3a5aa17bcdf59 Mon Sep 17 00:00:00 2001 From: Joshua Peek Date: Sat, 10 Jan 2009 11:39:57 -0600 Subject: Moved query string parsing tests into the request tests folder --- .../test/controller/query_string_parsing_test.rb | 120 --------------------- .../request/query_string_parsing_test.rb | 120 +++++++++++++++++++++ 2 files changed, 120 insertions(+), 120 deletions(-) delete mode 100644 actionpack/test/controller/query_string_parsing_test.rb create mode 100644 actionpack/test/controller/request/query_string_parsing_test.rb diff --git a/actionpack/test/controller/query_string_parsing_test.rb b/actionpack/test/controller/query_string_parsing_test.rb deleted file mode 100644 index a31e326ddf..0000000000 --- a/actionpack/test/controller/query_string_parsing_test.rb +++ /dev/null @@ -1,120 +0,0 @@ -require 'abstract_unit' - -class QueryStringParsingTest < ActionController::IntegrationTest - class TestController < ActionController::Base - class << self - attr_accessor :last_query_parameters - end - - def parse - self.class.last_query_parameters = request.query_parameters - head :ok - end - end - - def teardown - TestController.last_query_parameters = nil - end - - test "query string" do - assert_parses( - {"action" => "create_customer", "full_name" => "David Heinemeier Hansson", "customerId" => "1"}, - "action=create_customer&full_name=David%20Heinemeier%20Hansson&customerId=1" - ) - end - - test "deep query string" do - assert_parses( - {'x' => {'y' => {'z' => '10'}}}, - "x[y][z]=10" - ) - end - - test "deep query string with array" do - assert_parses({'x' => {'y' => {'z' => ['10']}}}, 'x[y][z][]=10') - assert_parses({'x' => {'y' => {'z' => ['10', '5']}}}, 'x[y][z][]=10&x[y][z][]=5') - end - - test "deep query string with array of hash" do - assert_parses({'x' => {'y' => [{'z' => '10'}]}}, 'x[y][][z]=10') - assert_parses({'x' => {'y' => [{'z' => '10', 'w' => '10'}]}}, 'x[y][][z]=10&x[y][][w]=10') - assert_parses({'x' => {'y' => [{'z' => '10', 'v' => {'w' => '10'}}]}}, 'x[y][][z]=10&x[y][][v][w]=10') - end - - test "deep query string with array of hashes with one pair" do - assert_parses({'x' => {'y' => [{'z' => '10'}, {'z' => '20'}]}}, 'x[y][][z]=10&x[y][][z]=20') - end - - test "deep query string with array of hashes with multiple pairs" do - assert_parses( - {'x' => {'y' => [{'z' => '10', 'w' => 'a'}, {'z' => '20', 'w' => 'b'}]}}, - 'x[y][][z]=10&x[y][][w]=a&x[y][][z]=20&x[y][][w]=b' - ) - end - - test "query string with nil" do - assert_parses( - { "action" => "create_customer", "full_name" => ''}, - "action=create_customer&full_name=" - ) - end - - test "query string with array" do - assert_parses( - { "action" => "create_customer", "selected" => ["1", "2", "3"]}, - "action=create_customer&selected[]=1&selected[]=2&selected[]=3" - ) - end - - test "query string with amps" do - assert_parses( - { "action" => "create_customer", "name" => "Don't & Does"}, - "action=create_customer&name=Don%27t+%26+Does" - ) - end - - test "query string with many equal" do - assert_parses( - { "action" => "create_customer", "full_name" => "abc=def=ghi"}, - "action=create_customer&full_name=abc=def=ghi" - ) - end - - test "query string without equal" do - assert_parses({ "action" => nil }, "action") - end - - test "query string with empty key" do - assert_parses( - { "action" => "create_customer", "full_name" => "David Heinemeier Hansson" }, - "action=create_customer&full_name=David%20Heinemeier%20Hansson&=Save" - ) - end - - test "query string with many ampersands" do - assert_parses( - { "action" => "create_customer", "full_name" => "David Heinemeier Hansson"}, - "&action=create_customer&&&full_name=David%20Heinemeier%20Hansson" - ) - end - - test "unbalanced query string with array" do - assert_parses( - {'location' => ["1", "2"], 'age_group' => ["2"]}, - "location[]=1&location[]=2&age_group[]=2" - ) - end - - private - def assert_parses(expected, actual) - with_routing do |set| - set.draw do |map| - map.connect ':action', :controller => "query_string_parsing_test/test" - end - - get "/parse", actual - assert_response :ok - assert_equal(expected, TestController.last_query_parameters) - end - end -end diff --git a/actionpack/test/controller/request/query_string_parsing_test.rb b/actionpack/test/controller/request/query_string_parsing_test.rb new file mode 100644 index 0000000000..a31e326ddf --- /dev/null +++ b/actionpack/test/controller/request/query_string_parsing_test.rb @@ -0,0 +1,120 @@ +require 'abstract_unit' + +class QueryStringParsingTest < ActionController::IntegrationTest + class TestController < ActionController::Base + class << self + attr_accessor :last_query_parameters + end + + def parse + self.class.last_query_parameters = request.query_parameters + head :ok + end + end + + def teardown + TestController.last_query_parameters = nil + end + + test "query string" do + assert_parses( + {"action" => "create_customer", "full_name" => "David Heinemeier Hansson", "customerId" => "1"}, + "action=create_customer&full_name=David%20Heinemeier%20Hansson&customerId=1" + ) + end + + test "deep query string" do + assert_parses( + {'x' => {'y' => {'z' => '10'}}}, + "x[y][z]=10" + ) + end + + test "deep query string with array" do + assert_parses({'x' => {'y' => {'z' => ['10']}}}, 'x[y][z][]=10') + assert_parses({'x' => {'y' => {'z' => ['10', '5']}}}, 'x[y][z][]=10&x[y][z][]=5') + end + + test "deep query string with array of hash" do + assert_parses({'x' => {'y' => [{'z' => '10'}]}}, 'x[y][][z]=10') + assert_parses({'x' => {'y' => [{'z' => '10', 'w' => '10'}]}}, 'x[y][][z]=10&x[y][][w]=10') + assert_parses({'x' => {'y' => [{'z' => '10', 'v' => {'w' => '10'}}]}}, 'x[y][][z]=10&x[y][][v][w]=10') + end + + test "deep query string with array of hashes with one pair" do + assert_parses({'x' => {'y' => [{'z' => '10'}, {'z' => '20'}]}}, 'x[y][][z]=10&x[y][][z]=20') + end + + test "deep query string with array of hashes with multiple pairs" do + assert_parses( + {'x' => {'y' => [{'z' => '10', 'w' => 'a'}, {'z' => '20', 'w' => 'b'}]}}, + 'x[y][][z]=10&x[y][][w]=a&x[y][][z]=20&x[y][][w]=b' + ) + end + + test "query string with nil" do + assert_parses( + { "action" => "create_customer", "full_name" => ''}, + "action=create_customer&full_name=" + ) + end + + test "query string with array" do + assert_parses( + { "action" => "create_customer", "selected" => ["1", "2", "3"]}, + "action=create_customer&selected[]=1&selected[]=2&selected[]=3" + ) + end + + test "query string with amps" do + assert_parses( + { "action" => "create_customer", "name" => "Don't & Does"}, + "action=create_customer&name=Don%27t+%26+Does" + ) + end + + test "query string with many equal" do + assert_parses( + { "action" => "create_customer", "full_name" => "abc=def=ghi"}, + "action=create_customer&full_name=abc=def=ghi" + ) + end + + test "query string without equal" do + assert_parses({ "action" => nil }, "action") + end + + test "query string with empty key" do + assert_parses( + { "action" => "create_customer", "full_name" => "David Heinemeier Hansson" }, + "action=create_customer&full_name=David%20Heinemeier%20Hansson&=Save" + ) + end + + test "query string with many ampersands" do + assert_parses( + { "action" => "create_customer", "full_name" => "David Heinemeier Hansson"}, + "&action=create_customer&&&full_name=David%20Heinemeier%20Hansson" + ) + end + + test "unbalanced query string with array" do + assert_parses( + {'location' => ["1", "2"], 'age_group' => ["2"]}, + "location[]=1&location[]=2&age_group[]=2" + ) + end + + private + def assert_parses(expected, actual) + with_routing do |set| + set.draw do |map| + map.connect ':action', :controller => "query_string_parsing_test/test" + end + + get "/parse", actual + assert_response :ok + assert_equal(expected, TestController.last_query_parameters) + end + end +end -- cgit v1.2.3 From ab0ce052ba23a4cce7a84ecade0d00d9cc518ebd Mon Sep 17 00:00:00 2001 From: Jeremy Kemper Date: Sat, 10 Jan 2009 13:36:09 -0800 Subject: Introduce transaction_joinable flag to mark that the fixtures transaction can't joined, a new savepoint is required even if :requires_new is not set. Use :requires_new option instead of :nest. Update changelog. [#383 state:committed] --- activerecord/CHANGELOG | 2 ++ .../abstract/database_statements.rb | 31 +++++++++++----------- .../connection_adapters/abstract_adapter.rb | 19 +++++++------ activerecord/lib/active_record/fixtures.rb | 5 ++-- activerecord/lib/active_record/transactions.rb | 27 +++++++++---------- activerecord/test/cases/transactions_test.rb | 8 +++--- 6 files changed, 45 insertions(+), 47 deletions(-) diff --git a/activerecord/CHANGELOG b/activerecord/CHANGELOG index c750f486f9..35172ae110 100644 --- a/activerecord/CHANGELOG +++ b/activerecord/CHANGELOG @@ -1,5 +1,7 @@ *2.3.0/3.0* +* Support nested transactions using database savepoints. #383 [Jonathan Viney, Hongli Lai] + * Added dynamic scopes ala dynamic finders #1648 [Yaroslav Markin] * Fixed that ActiveRecord::Base#new_record? should return false (not nil) for existing records #1219 [Yaroslav Markin] diff --git a/activerecord/lib/active_record/connection_adapters/abstract/database_statements.rb b/activerecord/lib/active_record/connection_adapters/abstract/database_statements.rb index cecbc6b3ac..39118583bd 100644 --- a/activerecord/lib/active_record/connection_adapters/abstract/database_statements.rb +++ b/activerecord/lib/active_record/connection_adapters/abstract/database_statements.rb @@ -89,14 +89,8 @@ module ActiveRecord # - The block will be run without doing anything. All database statements # that happen within the block are effectively appended to the already # open database transaction. - # - However, if +start_db_transaction+ is set to true, then the block will - # be run inside a new database savepoint, effectively making the block - # a sub-transaction. - # - If the #transactional_fixtures attribute is set to true, then the first - # nested call to #transaction will create a new savepoint instead of - # doing nothing. This makes it possible for toplevel transactions in unit - # tests to behave like real transactions, even though a database - # transaction has already been opened. + # - However, if +requires_new+ is set, the block will be wrapped in a + # database savepoint acting as a sub-transaction. # # === Caveats # @@ -111,20 +105,25 @@ module ActiveRecord # already-automatically-released savepoints: # # Model.connection.transaction do # BEGIN - # Model.connection.transaction(true) do # CREATE SAVEPOINT rails_savepoint_1 + # Model.connection.transaction(:requires_new => true) do # CREATE SAVEPOINT active_record_1 # Model.connection.create_table(...) - # # rails_savepoint_1 now automatically released - # end # RELEASE savepoint rails_savepoint_1 <--- BOOM! database error! + # # active_record_1 now automatically released + # end # RELEASE SAVEPOINT active_record_1 <--- BOOM! database error! # end - def transaction(start_db_transaction = false) - start_db_transaction ||= open_transactions == 0 || (open_transactions == 1 && transactional_fixtures) + def transaction(options = {}) + options.assert_valid_keys :requires_new, :joinable + + last_transaction_joinable, @transaction_joinable = + @transaction_joinable, options[:joinable] || true + requires_new = options[:requires_new] || !last_transaction_joinable + transaction_open = false begin if block_given? - if start_db_transaction + if requires_new || open_transactions == 0 if open_transactions == 0 begin_db_transaction - else + elsif requires_new create_savepoint end increment_open_transactions @@ -145,6 +144,8 @@ module ActiveRecord raise unless database_transaction_rollback.is_a? ActiveRecord::Rollback end ensure + @transaction_joinable = last_transaction_joinable + if outside_transaction? @open_transactions = 0 elsif transaction_open diff --git a/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb b/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb index 5137b0f78c..a8cd9f033b 100755 --- a/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb +++ b/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb @@ -165,24 +165,23 @@ module ActiveRecord def decrement_open_transactions @open_transactions -= 1 end - + + def transaction_joinable=(joinable) + @transaction_joinable = joinable + end + def create_savepoint end - + def rollback_to_savepoint end - + def release_savepoint end - + def current_savepoint_name - "rails_savepoint_#{open_transactions}" + "active_record_#{open_transactions}" end - - # Whether this AbstractAdapter is currently being used inside a unit test - # with transactional fixtures turned on. See DatabaseStatements#transaction - # for more information about the effect of this option. - attr_accessor :transactional_fixtures def log_info(sql, name, ms) if @logger && @logger.debug? diff --git a/activerecord/lib/active_record/fixtures.rb b/activerecord/lib/active_record/fixtures.rb index 039d5a4e8e..0131d9fac5 100644 --- a/activerecord/lib/active_record/fixtures.rb +++ b/activerecord/lib/active_record/fixtures.rb @@ -516,7 +516,7 @@ class Fixtures < (RUBY_VERSION < '1.9' ? YAML::Omap : Hash) all_loaded_fixtures.update(fixtures_map) - connection.transaction(connection.open_transactions.zero?) do + connection.transaction(:requires_new => true) do fixtures.reverse.each { |fixture| fixture.delete_existing_fixtures } fixtures.each { |fixture| fixture.insert_fixtures } @@ -937,8 +937,8 @@ module ActiveRecord @@already_loaded_fixtures[self.class] = @loaded_fixtures end ActiveRecord::Base.connection.increment_open_transactions + ActiveRecord::Base.connection.transaction_joinable = false ActiveRecord::Base.connection.begin_db_transaction - ActiveRecord::Base.connection.transactional_fixtures = true # Load fixtures for every test. else Fixtures.reset_cache @@ -961,7 +961,6 @@ module ActiveRecord if run_in_transaction? && ActiveRecord::Base.connection.open_transactions != 0 ActiveRecord::Base.connection.rollback_db_transaction ActiveRecord::Base.connection.decrement_open_transactions - ActiveRecord::Base.connection.transactional_fixtures = false end ActiveRecord::Base.clear_active_connections! end diff --git a/activerecord/lib/active_record/transactions.rb b/activerecord/lib/active_record/transactions.rb index aaa298dc49..0b6e52c79b 100644 --- a/activerecord/lib/active_record/transactions.rb +++ b/activerecord/lib/active_record/transactions.rb @@ -137,14 +137,14 @@ module ActiveRecord # # User.find(:all) # => empty # - # It is also possible to treat a certain #transaction call as its own - # sub-transaction, by passing :nest => true to #transaction. If - # anything goes wrong inside that transaction block, then the parent - # transaction will remain unaffected. For example: + # It is also possible to requires a sub-transaction by passing + # :requires_new => true. If anything goes wrong, the + # database rolls back to the beginning of the sub-transaction + # without rolling back the parent transaction. For example: # # User.transaction do # User.create(:username => 'Kotori') - # User.transaction(:nest => true) do + # User.transaction(:requires_new => true) do # User.create(:username => 'Nemu') # raise ActiveRecord::Rollback # end @@ -169,20 +169,17 @@ module ActiveRecord # database error will occur because the savepoint has already been # automatically released. The following example demonstrates the problem: # - # Model.connection.transaction do # BEGIN - # Model.connection.transaction(true) do # CREATE SAVEPOINT rails_savepoint_1 - # Model.connection.create_table(...) # rails_savepoint_1 now automatically released - # end # RELEASE savepoint rails_savepoint_1 - # # ^^^^ BOOM! database error! + # Model.connection.transaction do # BEGIN + # Model.connection.transaction(:requires_new => true) do # CREATE SAVEPOINT active_record_1 + # Model.connection.create_table(...) # active_record_1 now automatically released + # end # RELEASE savepoint active_record_1 + # # ^^^^ BOOM! database error! # end module ClassMethods # See ActiveRecord::Transactions::ClassMethods for detailed documentation. def transaction(options = {}, &block) - options.assert_valid_keys :nest - - # See the API documentation for ConnectionAdapters::DatabaseStatements#transaction - # for useful information. - connection.transaction(options[:nest], &block) + # See the ConnectionAdapters::DatabaseStatements#transaction API docs. + connection.transaction(options, &block) end end diff --git a/activerecord/test/cases/transactions_test.rb b/activerecord/test/cases/transactions_test.rb index 0c69fee8f2..f07fad1828 100644 --- a/activerecord/test/cases/transactions_test.rb +++ b/activerecord/test/cases/transactions_test.rb @@ -228,7 +228,7 @@ class TransactionTest < ActiveRecord::TestCase @second.save! begin - Topic.transaction :nest => true do + Topic.transaction :requires_new => true do @first.happy = false @first.save! raise @@ -268,17 +268,17 @@ class TransactionTest < ActiveRecord::TestCase @first.save! begin - Topic.transaction :nest => true do + Topic.transaction :requires_new => true do @first.content = "Two" @first.save! begin - Topic.transaction :nest => true do + Topic.transaction :requires_new => true do @first.content = "Three" @first.save! begin - Topic.transaction :nest => true do + Topic.transaction :requires_new => true do @first.content = "Four" @first.save! raise -- cgit v1.2.3 From 18cb0493d1ec1990a45000b1f3e6d9714a849690 Mon Sep 17 00:00:00 2001 From: Joshua Peek Date: Sat, 10 Jan 2009 16:02:03 -0600 Subject: Refactor request urlencoded params parsing tests --- .../request/url_encoded_params_parsing_test.rb | 171 ++++++++++++++++++ actionpack/test/controller/request_test.rb | 201 --------------------- 2 files changed, 171 insertions(+), 201 deletions(-) create mode 100644 actionpack/test/controller/request/url_encoded_params_parsing_test.rb diff --git a/actionpack/test/controller/request/url_encoded_params_parsing_test.rb b/actionpack/test/controller/request/url_encoded_params_parsing_test.rb new file mode 100644 index 0000000000..26a4538ca6 --- /dev/null +++ b/actionpack/test/controller/request/url_encoded_params_parsing_test.rb @@ -0,0 +1,171 @@ +require 'abstract_unit' + +class UrlEncodedParamsParsingTest < ActionController::IntegrationTest + class TestController < ActionController::Base + class << self + attr_accessor :last_request_parameters, :last_request_type + end + + def parse + self.class.last_request_parameters = request.request_parameters + head :ok + end + end + + def teardown + TestController.last_request_parameters = nil + end + + test "parses unbalanced query string with array" do + assert_parses( + {'location' => ["1", "2"], 'age_group' => ["2"]}, + "location[]=1&location[]=2&age_group[]=2" + ) + end + + test "parses nested hash" do + query = [ + "note[viewers][viewer][][type]=User", + "note[viewers][viewer][][id]=1", + "note[viewers][viewer][][type]=Group", + "note[viewers][viewer][][id]=2" + ].join("&") + + expected = { "note" => { "viewers"=>{"viewer"=>[{ "id"=>"1", "type"=>"User"}, {"type"=>"Group", "id"=>"2"} ]} } } + assert_parses(expected, query) + end + + test "parses more complex nesting" do + query = [ + "customers[boston][first][name]=David", + "customers[boston][first][url]=http://David", + "customers[boston][second][name]=Allan", + "customers[boston][second][url]=http://Allan", + "something_else=blah", + "something_nil=", + "something_empty=", + "products[first]=Apple Computer", + "products[second]=Pc", + "=Save" + ].join("&") + + expected = { + "customers" => { + "boston" => { + "first" => { + "name" => "David", + "url" => "http://David" + }, + "second" => { + "name" => "Allan", + "url" => "http://Allan" + } + } + }, + "something_else" => "blah", + "something_empty" => "", + "something_nil" => "", + "products" => { + "first" => "Apple Computer", + "second" => "Pc" + } + } + + assert_parses expected, query + end + + test "parses params with array" do + query = "selected[]=1&selected[]=2&selected[]=3" + expected = { "selected" => [ "1", "2", "3" ] } + assert_parses expected, query + end + + test "parses params with non alphanumeric name" do + query = "a/b[c]=d" + expected = { "a/b" => { "c" => "d" }} + assert_parses expected, query + end + + test "parses params with single brackets in the middle" do + query = "a/b[c]d=e" + expected = { "a/b" => {} } + assert_parses expected, query + end + + test "parses params with separated brackets" do + query = "a/b@[c]d[e]=f" + expected = { "a/b@" => { }} + assert_parses expected, query + end + + test "parses params with separated brackets and array" do + query = "a/b@[c]d[e][]=f" + expected = { "a/b@" => { }} + assert_parses expected, query + end + + test "parses params with unmatched brackets and array" do + query = "a/b@[c][d[e][]=f" + expected = { "a/b@" => { "c" => { }}} + assert_parses expected, query + end + + test "parses params with nil key" do + query = "=&test2=value1" + expected = { "test2" => "value1" } + assert_parses expected, query + end + + test "parses params with array prefix and hashes" do + query = "a[][b][c]=d" + expected = {"a" => [{"b" => {"c" => "d"}}]} + assert_parses expected, query + end + + test "parses params with complex nesting" do + query = "a[][b][c][][d][]=e" + expected = {"a" => [{"b" => {"c" => [{"d" => ["e"]}]}}]} + assert_parses expected, query + end + + test "parses params with file path" do + query = [ + "customers[boston][first][name]=David", + "something_else=blah", + "logo=#{File.expand_path(__FILE__)}" + ].join("&") + + expected = { + "customers" => { + "boston" => { + "first" => { + "name" => "David" + } + } + }, + "something_else" => "blah", + "logo" => File.expand_path(__FILE__), + } + + assert_parses expected, query + end + + + private + def with_test_routing + with_routing do |set| + set.draw do |map| + map.connect ':action', :controller => "url_encoded_params_parsing_test/test" + end + yield + end + end + + def assert_parses(expected, actual) + with_test_routing do + post "/parse", actual + assert_response :ok + assert_equal(expected, TestController.last_request_parameters) + end + end +end diff --git a/actionpack/test/controller/request_test.rb b/actionpack/test/controller/request_test.rb index 3256774b6d..7097d08076 100644 --- a/actionpack/test/controller/request_test.rb +++ b/actionpack/test/controller/request_test.rb @@ -405,204 +405,3 @@ class RequestTest < ActiveSupport::TestCase @request.request_method(true) end end - -class UrlEncodedRequestParameterParsingTest < ActiveSupport::TestCase - def test_unbalanced_query_string_with_array - assert_equal( - {'location' => ["1", "2"], 'age_group' => ["2"]}, - ActionController::RequestParser.parse_request_parameters({'location[]' => ["1", "2"], 'age_group[]' => ["2"]}) - ) - end - - def test_request_hash_parsing - query = { - "note[viewers][viewer][][type]" => ["User", "Group"], - "note[viewers][viewer][][id]" => ["1", "2"] - } - - expected = { "note" => { "viewers"=>{"viewer"=>[{ "id"=>"1", "type"=>"User"}, {"type"=>"Group", "id"=>"2"} ]} } } - - assert_equal(expected, ActionController::RequestParser.parse_request_parameters(query)) - end - - def test_parse_params - input = { - "customers[boston][first][name]" => [ "David" ], - "customers[boston][first][url]" => [ "http://David" ], - "customers[boston][second][name]" => [ "Allan" ], - "customers[boston][second][url]" => [ "http://Allan" ], - "something_else" => [ "blah" ], - "something_nil" => [ nil ], - "something_empty" => [ "" ], - "products[first]" => [ "Apple Computer" ], - "products[second]" => [ "Pc" ], - "" => [ 'Save' ] - } - - expected_output = { - "customers" => { - "boston" => { - "first" => { - "name" => "David", - "url" => "http://David" - }, - "second" => { - "name" => "Allan", - "url" => "http://Allan" - } - } - }, - "something_else" => "blah", - "something_empty" => "", - "something_nil" => "", - "products" => { - "first" => "Apple Computer", - "second" => "Pc" - } - } - - assert_equal expected_output, ActionController::RequestParser.parse_request_parameters(input) - end - - UploadedStringIO = ActionController::UploadedStringIO - class MockUpload < UploadedStringIO - def initialize(content_type, original_path, *args) - self.content_type = content_type - self.original_path = original_path - super *args - end - end - - def test_parse_params_from_multipart_upload - file = MockUpload.new('img/jpeg', 'foo.jpg') - ie_file = MockUpload.new('img/jpeg', 'c:\\Documents and Settings\\foo\\Desktop\\bar.jpg') - non_file_text_part = MockUpload.new('text/plain', '', 'abc') - - input = { - "something" => [ UploadedStringIO.new("") ], - "array_of_stringios" => [[ UploadedStringIO.new("One"), UploadedStringIO.new("Two") ]], - "mixed_types_array" => [[ UploadedStringIO.new("Three"), "NotStringIO" ]], - "mixed_types_as_checkboxes[strings][nested]" => [[ file, "String", UploadedStringIO.new("StringIO")]], - "ie_mixed_types_as_checkboxes[strings][nested]" => [[ ie_file, "String", UploadedStringIO.new("StringIO")]], - "products[string]" => [ UploadedStringIO.new("Apple Computer") ], - "products[file]" => [ file ], - "ie_products[string]" => [ UploadedStringIO.new("Microsoft") ], - "ie_products[file]" => [ ie_file ], - "text_part" => [non_file_text_part] - } - - expected_output = { - "something" => "", - "array_of_stringios" => ["One", "Two"], - "mixed_types_array" => [ "Three", "NotStringIO" ], - "mixed_types_as_checkboxes" => { - "strings" => { - "nested" => [ file, "String", "StringIO" ] - }, - }, - "ie_mixed_types_as_checkboxes" => { - "strings" => { - "nested" => [ ie_file, "String", "StringIO" ] - }, - }, - "products" => { - "string" => "Apple Computer", - "file" => file - }, - "ie_products" => { - "string" => "Microsoft", - "file" => ie_file - }, - "text_part" => "abc" - } - - params = ActionController::RequestParser.parse_request_parameters(input) - assert_equal expected_output, params - - # Lone filenames are preserved. - assert_equal 'foo.jpg', params['mixed_types_as_checkboxes']['strings']['nested'].first.original_filename - assert_equal 'foo.jpg', params['products']['file'].original_filename - - # But full Windows paths are reduced to their basename. - assert_equal 'bar.jpg', params['ie_mixed_types_as_checkboxes']['strings']['nested'].first.original_filename - assert_equal 'bar.jpg', params['ie_products']['file'].original_filename - end - - def test_parse_params_with_file - input = { - "customers[boston][first][name]" => [ "David" ], - "something_else" => [ "blah" ], - "logo" => [ File.new(File.dirname(__FILE__) + "/rack_test.rb").path ] - } - - expected_output = { - "customers" => { - "boston" => { - "first" => { - "name" => "David" - } - } - }, - "something_else" => "blah", - "logo" => File.new(File.dirname(__FILE__) + "/rack_test.rb").path, - } - - assert_equal expected_output, ActionController::RequestParser.parse_request_parameters(input) - end - - def test_parse_params_with_array - input = { "selected[]" => [ "1", "2", "3" ] } - - expected_output = { "selected" => [ "1", "2", "3" ] } - - assert_equal expected_output, ActionController::RequestParser.parse_request_parameters(input) - end - - def test_parse_params_with_non_alphanumeric_name - input = { "a/b[c]" => %w(d) } - expected = { "a/b" => { "c" => "d" }} - assert_equal expected, ActionController::RequestParser.parse_request_parameters(input) - end - - def test_parse_params_with_single_brackets_in_middle - input = { "a/b[c]d" => %w(e) } - expected = { "a/b" => {} } - assert_equal expected, ActionController::RequestParser.parse_request_parameters(input) - end - - def test_parse_params_with_separated_brackets - input = { "a/b@[c]d[e]" => %w(f) } - expected = { "a/b@" => { }} - assert_equal expected, ActionController::RequestParser.parse_request_parameters(input) - end - - def test_parse_params_with_separated_brackets_and_array - input = { "a/b@[c]d[e][]" => %w(f) } - expected = { "a/b@" => { }} - assert_equal expected , ActionController::RequestParser.parse_request_parameters(input) - end - - def test_parse_params_with_unmatched_brackets_and_array - input = { "a/b@[c][d[e][]" => %w(f) } - expected = { "a/b@" => { "c" => { }}} - assert_equal expected, ActionController::RequestParser.parse_request_parameters(input) - end - - def test_parse_params_with_nil_key - input = { nil => nil, "test2" => %w(value1) } - expected = { "test2" => "value1" } - assert_equal expected, ActionController::RequestParser.parse_request_parameters(input) - end - - def test_parse_params_with_array_prefix_and_hashes - input = { "a[][b][c]" => %w(d) } - expected = {"a" => [{"b" => {"c" => "d"}}]} - assert_equal expected, ActionController::RequestParser.parse_request_parameters(input) - end - - def test_parse_params_with_complex_nesting - input = { "a[][b][c][][d][]" => %w(e) } - expected = {"a" => [{"b" => {"c" => [{"d" => ["e"]}]}}]} - assert_equal expected, ActionController::RequestParser.parse_request_parameters(input) - end -end -- cgit v1.2.3 From 296ca4da1700eb27a7043112d22027444ea0e548 Mon Sep 17 00:00:00 2001 From: Nicholas Dainty Date: Sat, 10 Jan 2009 14:09:29 +0000 Subject: TimeWithZone#xmlschema accepts optional fraction_digits argument [#1725 state:resolved] --- activesupport/CHANGELOG | 2 ++ activesupport/lib/active_support/time_with_zone.rb | 8 ++++++-- activesupport/test/core_ext/time_with_zone_test.rb | 9 +++++++++ 3 files changed, 17 insertions(+), 2 deletions(-) diff --git a/activesupport/CHANGELOG b/activesupport/CHANGELOG index 757cb1da04..fed977775e 100644 --- a/activesupport/CHANGELOG +++ b/activesupport/CHANGELOG @@ -1,5 +1,7 @@ *2.3.0 [Edge]* +* TimeWithZone#xmlschema accepts optional fraction_digits argument [#1725 state:resolved] [Nicholas Dainty] + * Object#tap shim for Ruby < 1.8.7. Similar to Object#returning, tap yields self then returns self. [Jeremy Kemper] array.select { ... }.tap(&:inspect).map { ... } diff --git a/activesupport/lib/active_support/time_with_zone.rb b/activesupport/lib/active_support/time_with_zone.rb index 99be89fdf0..ec28f9801a 100644 --- a/activesupport/lib/active_support/time_with_zone.rb +++ b/activesupport/lib/active_support/time_with_zone.rb @@ -99,8 +99,12 @@ module ActiveSupport "#{time.strftime('%a, %d %b %Y %H:%M:%S')} #{zone} #{formatted_offset}" end - def xmlschema - "#{time.strftime("%Y-%m-%dT%H:%M:%S")}#{formatted_offset(true, 'Z')}" + def xmlschema(fraction_digits = 0) + fraction = if fraction_digits > 0 + ".%i" % time.usec.to_s[0, fraction_digits] + end + + "#{time.strftime("%Y-%m-%dT%H:%M:%S")}#{fraction}#{formatted_offset(true, 'Z')}" end alias_method :iso8601, :xmlschema diff --git a/activesupport/test/core_ext/time_with_zone_test.rb b/activesupport/test/core_ext/time_with_zone_test.rb index 7c8fb7dd94..4dc1fec559 100644 --- a/activesupport/test/core_ext/time_with_zone_test.rb +++ b/activesupport/test/core_ext/time_with_zone_test.rb @@ -105,6 +105,15 @@ class TimeWithZoneTest < Test::Unit::TestCase end end + def test_xmlschema_with_fractional_seconds + silence_warnings do # silence warnings raised by tzinfo gem + @twz += 0.123456 # advance the time by a fraction of a second + assert_equal "1999-12-31T19:00:00.123-05:00", @twz.xmlschema(3) + assert_equal "1999-12-31T19:00:00.123456-05:00", @twz.xmlschema(6) + assert_equal "1999-12-31T19:00:00.123456-05:00", @twz.xmlschema(12) + end + end + def test_to_yaml silence_warnings do # silence warnings raised by tzinfo gem assert_equal "--- 1999-12-31 19:00:00 -05:00\n", @twz.to_yaml -- cgit v1.2.3 From 5339f813be99012aba01586743d8b24f065e7034 Mon Sep 17 00:00:00 2001 From: Pratik Naik Date: Tue, 13 Jan 2009 03:28:32 +0000 Subject: Change Object#try to raise NoMethodError on private methods and always return nil when Object is nil [Pratik Naik, Lawrence Pit] --- actionpack/lib/action_controller/test_process.rb | 3 ++- .../lib/active_support/core_ext/object/misc.rb | 2 +- .../test/core_ext/object_and_class_ext_test.rb | 19 ++++++++++--------- 3 files changed, 13 insertions(+), 11 deletions(-) diff --git a/actionpack/lib/action_controller/test_process.rb b/actionpack/lib/action_controller/test_process.rb index 8180d4ee93..22b97fc157 100644 --- a/actionpack/lib/action_controller/test_process.rb +++ b/actionpack/lib/action_controller/test_process.rb @@ -484,7 +484,8 @@ module ActionController #:nodoc: # # post :change_avatar, :avatar => fixture_file_upload('/files/spongebob.png', 'image/png', :binary) def fixture_file_upload(path, mime_type = nil, binary = false) - ActionController::TestUploadedFile.new("#{ActionController::TestCase.try(:fixture_path)}#{path}", mime_type, binary) + fixture_path = ActionController::TestCase.send(:fixture_path) if ActionController::TestCase.respond_to?(:fixture_path) + ActionController::TestUploadedFile.new("#{fixture_path}#{path}", mime_type, binary) end # A helper to make it easier to test different route configurations. diff --git a/activesupport/lib/active_support/core_ext/object/misc.rb b/activesupport/lib/active_support/core_ext/object/misc.rb index 4570570bbc..c0a109ecf3 100644 --- a/activesupport/lib/active_support/core_ext/object/misc.rb +++ b/activesupport/lib/active_support/core_ext/object/misc.rb @@ -102,6 +102,6 @@ class Object # Person.try(:find, 1) # @people.try(:map) {|p| p.name} def try(method, *args, &block) - send(method, *args, &block) if respond_to?(method, true) + send(method, *args, &block) unless self.nil? end end diff --git a/activesupport/test/core_ext/object_and_class_ext_test.rb b/activesupport/test/core_ext/object_and_class_ext_test.rb index 2f79b6f67f..0bdbd14f33 100644 --- a/activesupport/test/core_ext/object_and_class_ext_test.rb +++ b/activesupport/test/core_ext/object_and_class_ext_test.rb @@ -256,21 +256,13 @@ class ObjectTryTest < Test::Unit::TestCase def test_nonexisting_method method = :undefined_method assert !@string.respond_to?(method) - assert_nil @string.try(method) + assert_raises(NoMethodError) { @string.try(method) } end def test_valid_method assert_equal 5, @string.try(:size) end - def test_valid_private_method - class << @string - private :size - end - - assert_equal 5, @string.try(:size) - end - def test_argument_forwarding assert_equal 'Hey', @string.try(:sub, 'llo', 'y') end @@ -278,4 +270,13 @@ class ObjectTryTest < Test::Unit::TestCase def test_block_forwarding assert_equal 'Hey', @string.try(:sub, 'llo') { |match| 'y' } end + + def test_nil_to_type + assert_nil nil.try(:to_s) + assert_nil nil.try(:to_i) + end + + def test_false_try + assert_equal 'false', false.try(:to_s) + end end -- cgit v1.2.3 From c99ef814b0ce5d6b6a677ee6116edac03c8a35b3 Mon Sep 17 00:00:00 2001 From: Pratik Naik Date: Tue, 13 Jan 2009 16:13:42 +0000 Subject: Revert "HTTP Digest authentication [#1230 state:resolved]" This reverts commit 45dee3842d68359a189fe7c0729359bd5a905ea4. Reasons : 1. The code is not working in it's current state 2. Should not be using exceptions for flow control --- .../lib/action_controller/http_authentication.rb | 191 +-------------------- actionpack/lib/action_controller/integration.rb | 82 --------- .../controller/http_digest_authentication_test.rb | 73 -------- actionpack/test/controller/integration_test.rb | 88 ---------- 4 files changed, 2 insertions(+), 432 deletions(-) delete mode 100644 actionpack/test/controller/http_digest_authentication_test.rb diff --git a/actionpack/lib/action_controller/http_authentication.rb b/actionpack/lib/action_controller/http_authentication.rb index 3cb5829eca..2ed810db7d 100644 --- a/actionpack/lib/action_controller/http_authentication.rb +++ b/actionpack/lib/action_controller/http_authentication.rb @@ -55,31 +55,7 @@ module ActionController # end # end # - # Simple Digest example. Note the block must return the user's password so the framework - # can appropriately hash it to check the user's credentials. Returning nil will cause authentication to fail. - # - # class PostsController < ApplicationController - # Users = {"dhh" => "secret"} - # - # before_filter :authenticate, :except => [ :index ] - # - # def index - # render :text => "Everyone can see me!" - # end - # - # def edit - # render :text => "I'm only accessible if you know the password" - # end - # - # private - # def authenticate - # authenticate_or_request_with_http_digest(realm) do |user_name| - # Users[user_name] - # end - # end - # end - # - # + # # In your integration tests, you can do something like this: # # def test_access_granted_from_xml @@ -132,10 +108,7 @@ module ActionController end def decode_credentials(request) - # Properly decode credentials spanning a new-line - auth = authorization(request) - auth.slice!('Basic ') - ActiveSupport::Base64.decode64(auth || '') + ActiveSupport::Base64.decode64(authorization(request).split.last || '') end def encode_credentials(user_name, password) @@ -147,165 +120,5 @@ module ActionController controller.__send__ :render, :text => "HTTP Basic: Access denied.\n", :status => :unauthorized end end - - module Digest - extend self - - module ControllerMethods - def authenticate_or_request_with_http_digest(realm = "Application", &password_procedure) - begin - authenticate_with_http_digest!(realm, &password_procedure) - rescue ActionController::HttpAuthentication::Error => e - msg = e.message - msg = "#{msg} expected '#{e.expected}' was '#{e.was}'" unless e.expected.nil? - raise msg if e.fatal? - request_http_digest_authentication(realm, msg) - end - end - - # Authenticate using HTTP Digest, throwing ActionController::HttpAuthentication::Error on failure. - # This allows more detailed analysis of authentication failures - # to be relayed to the client. - def authenticate_with_http_digest!(realm = "Application", &login_procedure) - HttpAuthentication::Digest.authenticate(self, realm, &login_procedure) - end - - # Authenticate with HTTP Digest, returns true or false - def authenticate_with_http_digest(realm = "Application", &login_procedure) - HttpAuthentication::Digest.authenticate(self, realm, &login_procedure) rescue false - end - - # Render output including the HTTP Digest authentication header - def request_http_digest_authentication(realm = "Application", message = nil) - HttpAuthentication::Digest.authentication_request(self, realm, message) - end - - # Add HTTP Digest authentication header to result headers - def http_digest_authentication_header(realm = "Application") - HttpAuthentication::Digest.authentication_header(self, realm) - end - end - - # Raises error unless authentictaion succeeds, returns true otherwise - def authenticate(controller, realm, &password_procedure) - raise Error.new(false), "No authorization header found" unless authorization(controller.request) - validate_digest_response(controller, realm, &password_procedure) - true - end - - def authorization(request) - request.env['HTTP_AUTHORIZATION'] || - request.env['X-HTTP_AUTHORIZATION'] || - request.env['X_HTTP_AUTHORIZATION'] || - request.env['REDIRECT_X_HTTP_AUTHORIZATION'] - end - - # Raises error unless the request credentials response value matches the expected value. - def validate_digest_response(controller, realm, &password_procedure) - credentials = decode_credentials(controller.request) - - # Check the nonce, opaque and realm. - # Ignore nc, as we have no way to validate the number of times this nonce has been used - validate_nonce(controller.request, credentials[:nonce]) - raise Error.new(false, realm, credentials[:realm]), "Realm doesn't match" unless realm == credentials[:realm] - raise Error.new(true, opaque(controller.request), credentials[:opaque]),"Opaque doesn't match" unless opaque(controller.request) == credentials[:opaque] - - password = password_procedure.call(credentials[:username]) - raise Error.new(false), "No password" if password.nil? - expected = expected_response(controller.request.env['REQUEST_METHOD'], controller.request.url, credentials, password) - raise Error.new(false, expected, credentials[:response]), "Invalid response" unless expected == credentials[:response] - end - - # Returns the expected response for a request of +http_method+ to +uri+ with the decoded +credentials+ and the expected +password+ - def expected_response(http_method, uri, credentials, password) - ha1 = ::Digest::MD5.hexdigest([credentials[:username], credentials[:realm], password].join(':')) - ha2 = ::Digest::MD5.hexdigest([http_method.to_s.upcase,uri].join(':')) - ::Digest::MD5.hexdigest([ha1,credentials[:nonce], credentials[:nc], credentials[:cnonce],credentials[:qop],ha2].join(':')) - end - - def encode_credentials(http_method, credentials, password) - credentials[:response] = expected_response(http_method, credentials[:uri], credentials, password) - "Digest " + credentials.sort_by {|x| x[0].to_s }.inject([]) {|a, v| a << "#{v[0]}='#{v[1]}'" }.join(', ') - end - - def decode_credentials(request) - authorization(request).to_s.gsub(/^Digest\s+/,'').split(',').inject({}) do |hash, pair| - key, value = pair.split('=', 2) - hash[key.strip.to_sym] = value.to_s.gsub(/^"|"$/,'').gsub(/'/, '') - hash - end - end - - def authentication_header(controller, realm) - controller.headers["WWW-Authenticate"] = %(Digest realm="#{realm}", qop="auth", algorithm=MD5, nonce="#{nonce(controller.request)}", opaque="#{opaque(controller.request)}") - end - - def authentication_request(controller, realm, message = "HTTP Digest: Access denied") - authentication_header(controller, realm) - controller.send! :render, :text => message, :status => :unauthorized - end - - # Uses an MD5 digest based on time to generate a value to be used only once. - # - # A server-specified data string which should be uniquely generated each time a 401 response is made. - # It is recommended that this string be base64 or hexadecimal data. - # Specifically, since the string is passed in the header lines as a quoted string, the double-quote character is not allowed. - # - # The contents of the nonce are implementation dependent. - # The quality of the implementation depends on a good choice. - # A nonce might, for example, be constructed as the base 64 encoding of - # - # => time-stamp H(time-stamp ":" ETag ":" private-key) - # - # where time-stamp is a server-generated time or other non-repeating value, - # ETag is the value of the HTTP ETag header associated with the requested entity, - # and private-key is data known only to the server. - # With a nonce of this form a server would recalculate the hash portion after receiving the client authentication header and - # reject the request if it did not match the nonce from that header or - # if the time-stamp value is not recent enough. In this way the server can limit the time of the nonce's validity. - # The inclusion of the ETag prevents a replay request for an updated version of the resource. - # (Note: including the IP address of the client in the nonce would appear to offer the server the ability - # to limit the reuse of the nonce to the same client that originally got it. - # However, that would break proxy farms, where requests from a single user often go through different proxies in the farm. - # Also, IP address spoofing is not that hard.) - # - # An implementation might choose not to accept a previously used nonce or a previously used digest, in order to - # protect against a replay attack. Or, an implementation might choose to use one-time nonces or digests for - # POST or PUT requests and a time-stamp for GET requests. For more details on the issues involved see Section 4 - # of this document. - # - # The nonce is opaque to the client. - def nonce(request, time = Time.now) - session_id = request.is_a?(String) ? request : request.session.session_id - t = time.to_i - hashed = [t, session_id] - digest = ::Digest::MD5.hexdigest(hashed.join(":")) - Base64.encode64("#{t}:#{digest}").gsub("\n", '') - end - - def validate_nonce(request, value) - t = Base64.decode64(value).split(":").first.to_i - raise Error.new(true), "Stale Nonce" if (t - Time.now.to_i).abs > 10 * 60 - n = nonce(request, t) - raise Error.new(true, value, n), "Bad Nonce" unless n == value - end - - # Opaque based on digest of session_id - def opaque(request) - session_id = request.is_a?(String) ? request : request.session.session_id - @opaque ||= Base64.encode64(::Digest::MD5::hexdigest(session_id)).gsub("\n", '') - end - end - - class Error < RuntimeError - attr_accessor :expected, :was - def initialize(fatal = false, expected = nil, was = nil) - @fatal = fatal - @expected = expected - @was = was - end - - def fatal?; @fatal; end - end end end diff --git a/actionpack/lib/action_controller/integration.rb b/actionpack/lib/action_controller/integration.rb index ded72a71fb..08b54658ee 100644 --- a/actionpack/lib/action_controller/integration.rb +++ b/actionpack/lib/action_controller/integration.rb @@ -68,15 +68,6 @@ module ActionController # A running counter of the number of requests processed. attr_accessor :request_count - # Nonce value for Digest Authentication, implicitly set on response with WWW-Authentication - attr_accessor :nonce - - # Opaque value for Digest Authentication, implicitly set on response with WWW-Authentication - attr_accessor :opaque - - # Opaque value for Authentication, implicitly set on response with WWW-Authentication - attr_accessor :realm - class MultiPartNeededException < Exception end @@ -252,53 +243,6 @@ module ActionController end alias xhr :xml_http_request - def request_with_noauth(http_method, uri, parameters, headers) - process_with_auth http_method, uri, parameters, headers - end - - # Performs a request with the given http_method and parameters, including HTTP Basic authorization headers. - # See get() for more details on paramters and headers. - # - # You can perform GET, POST, PUT, DELETE, and HEAD requests with #get_with_basic, #post_with_basic, - # #put_with_basic, #delete_with_basic, and #head_with_basic. - def request_with_basic(http_method, uri, parameters, headers, user_name, password) - process_with_auth http_method, uri, parameters, headers.merge(:authorization => ActionController::HttpAuthentication::Basic.encode_credentials(user_name, password)) - end - - # Performs a request with the given http_method and parameters, including HTTP Digest authorization headers. - # See get() for more details on paramters and headers. - # - # You can perform GET, POST, PUT, DELETE, and HEAD requests with #get_with_digest, #post_with_digest, - # #put_with_digest, #delete_with_digest, and #head_with_digest. - def request_with_digest(http_method, uri, parameters, headers, user_name, password) - # Realm, Nonce, and Opaque taken from previoius 401 response - - credentials = { - :username => user_name, - :realm => @realm, - :nonce => @nonce, - :qop => "auth", - :nc => "00000001", - :cnonce => "0a4f113b", - :opaque => @opaque, - :uri => uri - } - - raise "Digest request without previous 401 response" if @opaque.nil? - - process_with_auth http_method, uri, parameters, headers.merge(:authorization => ActionController::HttpAuthentication::Digest.encode_credentials(http_method, credentials, password)) - end - - # def get_with_basic, def post_with_basic, def put_with_basic, def delete_with_basic, def head_with_basic - # def get_with_digest, def post_with_digest, def put_with_digest, def delete_with_digest, def head_with_digest - [:get, :post, :put, :delete, :head].each do |method| - [:noauth, :basic, :digest].each do |auth_type| - define_method("#{method}_with_#{auth_type}") do |uri, parameters, headers, *auth| - send("request_with_#{auth_type}", method, uri, parameters, headers, *auth) - end - end - end - # Returns the URL for the given options, according to the rules specified # in the application's routes. def url_for(options) @@ -423,32 +367,6 @@ module ActionController return status end - # Same as process, but handles authentication returns to perform - # Basic or Digest authentication - def process_with_auth(method, path, parameters = nil, headers = nil) - status = process(method, path, parameters, headers) - - if status == 401 - # Extract authentication information from response - auth_data = @response.headers['WWW-Authenticate'] - if /^Basic /.match(auth_data) - # extract realm, to be used in subsequent request - @realm = auth_header.split(' ')[1] - elsif /^Digest/.match(auth_data) - creds = auth_data.to_s.gsub(/^Digest\s+/,'').split(',').inject({}) do |hash, pair| - key, value = pair.split('=', 2) - hash[key.strip.to_sym] = value.to_s.gsub(/^"|"$/,'').gsub(/'/, '') - hash - end - @realm = creds[:realm] - @nonce = creds[:nonce] - @opaque = creds[:opaque] - end - end - - return status - end - # Encode the cookies hash in a format suitable for passing to a # request. def encode_cookies diff --git a/actionpack/test/controller/http_digest_authentication_test.rb b/actionpack/test/controller/http_digest_authentication_test.rb deleted file mode 100644 index d5c8636a9e..0000000000 --- a/actionpack/test/controller/http_digest_authentication_test.rb +++ /dev/null @@ -1,73 +0,0 @@ -require 'abstract_unit' - -class HttpDigestAuthenticationTest < Test::Unit::TestCase - include ActionController::HttpAuthentication::Digest - - class DummyController - attr_accessor :headers, :renders, :request, :response - - def initialize - @headers, @renders = {}, [] - @request = ActionController::TestRequest.new - @response = ActionController::TestResponse.new - request.session.session_id = "test_session" - end - - def render(options) - self.renderers << options - end - end - - def setup - @controller = DummyController.new - @credentials = { - :username => "dhh", - :realm => "testrealm@host.com", - :nonce => ActionController::HttpAuthentication::Digest.nonce(@controller.request), - :qop => "auth", - :nc => "00000001", - :cnonce => "0a4f113b", - :opaque => ActionController::HttpAuthentication::Digest.opaque(@controller.request), - :uri => "http://test.host/" - } - @encoded_credentials = ActionController::HttpAuthentication::Digest.encode_credentials("GET", @credentials, "secret") - end - - def test_decode_credentials - set_headers - assert_equal @credentials, decode_credentials(@controller.request) - end - - def test_nonce_format - assert_nothing_thrown do - validate_nonce(@controller.request, nonce(@controller.request)) - end - end - - def test_authenticate_should_raise_for_nil_password - set_headers ActionController::HttpAuthentication::Digest.encode_credentials(:get, @credentials, nil) - assert_raise ActionController::HttpAuthentication::Error do - authenticate(@controller, @credentials[:realm]) { |user| user == "dhh" && "secret" } - end - end - - def test_authenticate_should_raise_for_incorrect_password - set_headers - assert_raise ActionController::HttpAuthentication::Error do - authenticate(@controller, @credentials[:realm]) { |user| user == "dhh" && "bad password" } - end - end - - def test_authenticate_should_not_raise_for_correct_password - set_headers - assert_nothing_thrown do - authenticate(@controller, @credentials[:realm]) { |user| user == "dhh" && "secret" } - end - end - - private - def set_headers(value = @encoded_credentials, name = 'HTTP_AUTHORIZATION', method = "GET") - @controller.request.env[name] = value - @controller.request.env["REQUEST_METHOD"] = method - end -end diff --git a/actionpack/test/controller/integration_test.rb b/actionpack/test/controller/integration_test.rb index 7ac9d97096..4f07cbee47 100644 --- a/actionpack/test/controller/integration_test.rb +++ b/actionpack/test/controller/integration_test.rb @@ -8,25 +8,7 @@ class SessionTest < Test::Unit::TestCase } def setup - @credentials = { - :username => "username", - :realm => "MyApp", - :nonce => ActionController::HttpAuthentication::Digest.nonce("session_id"), - :qop => "auth", - :nc => "00000001", - :cnonce => "0a4f113b", - :opaque => ActionController::HttpAuthentication::Digest.opaque("session_id"), - :uri => "/index" - } - @session = ActionController::Integration::Session.new(StubApp) - @session.nonce = @credentials[:nonce] - @session.opaque = @credentials[:opaque] - @session.realm = @credentials[:realm] - end - - def encoded_credentials(method) - ActionController::HttpAuthentication::Digest.encode_credentials(method, @credentials, "password") end def test_https_bang_works_and_sets_truth_by_default @@ -150,76 +132,6 @@ class SessionTest < Test::Unit::TestCase @session.head(path,params,headers) end - def test_get_with_basic - path = "/index"; params = "blah"; headers = {:location => 'blah'} - expected_headers = headers.merge(:authorization => "Basic dXNlcm5hbWU6cGFzc3dvcmQ=\n") - @session.expects(:process).with(:get,path,params,expected_headers) - @session.get_with_basic(path,params,headers,'username','password') - end - - def test_post_with_basic - path = "/index"; params = "blah"; headers = {:location => 'blah'} - expected_headers = headers.merge(:authorization => "Basic dXNlcm5hbWU6cGFzc3dvcmQ=\n") - @session.expects(:process).with(:post,path,params,expected_headers) - @session.post_with_basic(path,params,headers,'username','password') - end - - def test_put_with_basic - path = "/index"; params = "blah"; headers = {:location => 'blah'} - expected_headers = headers.merge(:authorization => "Basic dXNlcm5hbWU6cGFzc3dvcmQ=\n") - @session.expects(:process).with(:put,path,params,expected_headers) - @session.put_with_basic(path,params,headers,'username','password') - end - - def test_delete_with_basic - path = "/index"; params = "blah"; headers = {:location => 'blah'} - expected_headers = headers.merge(:authorization => "Basic dXNlcm5hbWU6cGFzc3dvcmQ=\n") - @session.expects(:process).with(:delete,path,params,expected_headers) - @session.delete_with_basic(path,params,headers,'username','password') - end - - def test_head_with_basic - path = "/index"; params = "blah"; headers = {:location => 'blah'} - expected_headers = headers.merge(:authorization => "Basic dXNlcm5hbWU6cGFzc3dvcmQ=\n") - @session.expects(:process).with(:head,path,params,expected_headers) - @session.head_with_basic(path,params,headers,'username','password') - end - - def test_get_with_digest - path = "/index"; params = "blah"; headers = {:location => 'blah'} - expected_headers = headers.merge(:authorization => encoded_credentials(:get)) - @session.expects(:process).with(:get,path,params,expected_headers) - @session.get_with_digest(path,params,headers,'username','password') - end - - def test_post_with_digest - path = "/index"; params = "blah"; headers = {:location => 'blah'} - expected_headers = headers.merge(:authorization => encoded_credentials(:post)) - @session.expects(:process).with(:post,path,params,expected_headers) - @session.post_with_digest(path,params,headers,'username','password') - end - - def test_put_with_digest - path = "/index"; params = "blah"; headers = {:location => 'blah'} - expected_headers = headers.merge(:authorization => encoded_credentials(:put)) - @session.expects(:process).with(:put,path,params,expected_headers) - @session.put_with_digest(path,params,headers,'username','password') - end - - def test_delete_with_digest - path = "/index"; params = "blah"; headers = {:location => 'blah'} - expected_headers = headers.merge(:authorization => encoded_credentials(:delete)) - @session.expects(:process).with(:delete,path,params,expected_headers) - @session.delete_with_digest(path,params,headers,'username','password') - end - - def test_head_with_digest - path = "/index"; params = "blah"; headers = {:location => 'blah'} - expected_headers = headers.merge(:authorization => encoded_credentials(:head)) - @session.expects(:process).with(:head,path,params,expected_headers) - @session.head_with_digest(path,params,headers,'username','password') - end - def test_xml_http_request_get path = "/index"; params = "blah"; headers = {:location => 'blah'} headers_after_xhr = headers.merge( -- cgit v1.2.3 From b6a94fc1c611216c6d1001bb6044973b01dbba38 Mon Sep 17 00:00:00 2001 From: Cody Fauser Date: Tue, 13 Jan 2009 13:45:10 -0600 Subject: Remove legacy reloadable? method from ActiveRecord::SessionStore [#1745 state:resolved] Signed-off-by: Joshua Peek --- activerecord/lib/active_record/session_store.rb | 5 ----- 1 file changed, 5 deletions(-) diff --git a/activerecord/lib/active_record/session_store.rb b/activerecord/lib/active_record/session_store.rb index bd198c03b2..5e45cf65ab 100644 --- a/activerecord/lib/active_record/session_store.rb +++ b/activerecord/lib/active_record/session_store.rb @@ -53,11 +53,6 @@ module ActiveRecord before_save :raise_on_session_data_overflow! class << self - # Don't try to reload ARStore::Session in dev mode. - def reloadable? #:nodoc: - false - end - def data_column_size_limit @data_column_size_limit ||= columns_hash[@@data_column_name].limit end -- cgit v1.2.3 From d3107ce3b04a14bd5674da6812acbff30aedaf73 Mon Sep 17 00:00:00 2001 From: Cody Fauser Date: Tue, 13 Jan 2009 14:27:23 -0600 Subject: Use :key instead of old :session_key in session_store.rb generator and docs [#1746 state:resovled] Signed-off-by: Joshua Peek --- actionpack/lib/action_controller/session/cookie_store.rb | 6 +++--- railties/configs/initializers/session_store.rb | 2 +- railties/doc/guides/source/security.txt | 2 +- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/actionpack/lib/action_controller/session/cookie_store.rb b/actionpack/lib/action_controller/session/cookie_store.rb index 135bedaf50..e061c4d4a1 100644 --- a/actionpack/lib/action_controller/session/cookie_store.rb +++ b/actionpack/lib/action_controller/session/cookie_store.rb @@ -163,9 +163,9 @@ module ActionController def ensure_session_key(key) if key.blank? - raise ArgumentError, 'A session_key is required to write a ' + + raise ArgumentError, 'A key is required to write a ' + 'cookie containing the session data. Use ' + - 'config.action_controller.session = { :session_key => ' + + 'config.action_controller.session = { :key => ' + '"_myapp_session", :secret => "some secret phrase" } in ' + 'config/environment.rb' end @@ -181,7 +181,7 @@ module ActionController if secret.blank? raise ArgumentError, "A secret is required to generate an " + "integrity hash for cookie session data. Use " + - "config.action_controller.session = { :session_key => " + + "config.action_controller.session = { :key => " + "\"_myapp_session\", :secret => \"some secret phrase of at " + "least #{SECRET_MIN_LENGTH} characters\" } " + "in config/environment.rb" diff --git a/railties/configs/initializers/session_store.rb b/railties/configs/initializers/session_store.rb index 40179e0aa3..4499ab84b6 100644 --- a/railties/configs/initializers/session_store.rb +++ b/railties/configs/initializers/session_store.rb @@ -5,7 +5,7 @@ # Make sure the secret is at least 30 characters and all random, # no regular words or you'll be exposed to dictionary attacks. ActionController::Base.session = { - :session_key => '_<%= app_name %>_session', + :key => '_<%= app_name %>_session', :secret => '<%= app_secret %>' } diff --git a/railties/doc/guides/source/security.txt b/railties/doc/guides/source/security.txt index 9b3f47932e..b4e8bb4b41 100644 --- a/railties/doc/guides/source/security.txt +++ b/railties/doc/guides/source/security.txt @@ -93,7 +93,7 @@ That means the security of this storage depends on this secret (and of the diges .................................... config.action_controller.session = { - :session_key => ‘_app_session’, + :key => ‘_app_session’, :secret => ‘0x0dkfj3927dkc7djdh36rkckdfzsg...’ } .................................... -- cgit v1.2.3 From 5a43908c7414996354ca427354d98d789e0210e7 Mon Sep 17 00:00:00 2001 From: Bryan Ash Date: Tue, 13 Jan 2009 14:42:43 -0600 Subject: Explicitly read as binary in multipart_body for Windows [#1065 state:resolved] Signed-off-by: Joshua Peek --- actionpack/lib/action_controller/integration.rb | 2 +- .../controller/request/multipart_params_parsing_test.rb | 14 +++++++------- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/actionpack/lib/action_controller/integration.rb b/actionpack/lib/action_controller/integration.rb index 08b54658ee..5b08e30d49 100644 --- a/actionpack/lib/action_controller/integration.rb +++ b/actionpack/lib/action_controller/integration.rb @@ -431,7 +431,7 @@ module ActionController def multipart_body(params, boundary) multipart_requestify(params).map do |key, value| if value.respond_to?(:original_filename) - File.open(value.path) do |f| + File.open(value.path, "rb") do |f| f.set_encoding(Encoding::BINARY) if f.respond_to?(:set_encoding) <<-EOF diff --git a/actionpack/test/controller/request/multipart_params_parsing_test.rb b/actionpack/test/controller/request/multipart_params_parsing_test.rb index 03ab164972..ce28ff46fe 100644 --- a/actionpack/test/controller/request/multipart_params_parsing_test.rb +++ b/actionpack/test/controller/request/multipart_params_parsing_test.rb @@ -3,11 +3,10 @@ require 'abstract_unit' class MultipartParamsParsingTest < ActionController::IntegrationTest class TestController < ActionController::Base class << self - attr_accessor :last_request_parameters, :last_request_type + attr_accessor :last_request_parameters end def parse - self.class.last_request_type = ActionController::Base.param_parsers[request.content_type] self.class.last_request_parameters = request.request_parameters head :ok end @@ -21,7 +20,6 @@ class MultipartParamsParsingTest < ActionController::IntegrationTest def teardown TestController.last_request_parameters = nil - TestController.last_request_type = nil end test "parses single parameter" do @@ -103,11 +101,13 @@ class MultipartParamsParsingTest < ActionController::IntegrationTest assert_equal 19756, files.size end - test "uploads and parses parameters" do + test "uploads and reads binary file" do with_test_routing do - params = { :uploaded_data => fixture_file_upload(FIXTURE_PATH + "/mona_lisa.jpg", "image/jpg") } - post '/parse', params, :location => 'blah' - assert_equal(:multipart_form, TestController.last_request_type) + fixture = FIXTURE_PATH + "/mona_lisa.jpg" + params = { :uploaded_data => fixture_file_upload(fixture, "image/jpg") } + post '/read', params + expected_length = 'File: '.length + File.size(fixture) + assert_equal expected_length, response.content_length end end -- cgit v1.2.3 From 1adc1496f9152c893e1f08abcb1e5e7272829899 Mon Sep 17 00:00:00 2001 From: Joshua Peek Date: Tue, 13 Jan 2009 16:09:51 -0600 Subject: Add RewindableInput wrapper to fix issues with middleware that impolitely eat up non-rewindable input --- actionpack/lib/action_controller.rb | 13 ++++--- actionpack/lib/action_controller/middlewares.rb | 1 + .../lib/action_controller/rewindable_input.rb | 40 +++++++++++++++++++ .../request/multipart_params_parsing_test.rb | 45 +++++++++++++++++++++- .../request/url_encoded_params_parsing_test.rb | 37 ++++++++++++++++++ 5 files changed, 128 insertions(+), 8 deletions(-) create mode 100644 actionpack/lib/action_controller/rewindable_input.rb diff --git a/actionpack/lib/action_controller.rb b/actionpack/lib/action_controller.rb index 8c022e5625..0d81b9c346 100644 --- a/actionpack/lib/action_controller.rb +++ b/actionpack/lib/action_controller.rb @@ -59,16 +59,14 @@ module ActionController autoload :MiddlewareStack, 'action_controller/middleware_stack' autoload :MimeResponds, 'action_controller/mime_responds' autoload :PolymorphicRoutes, 'action_controller/polymorphic_routes' - autoload :Request, 'action_controller/request' - autoload :RequestParser, 'action_controller/request_parser' - autoload :UrlEncodedPairParser, 'action_controller/url_encoded_pair_parser' - autoload :UploadedStringIO, 'action_controller/uploaded_file' - autoload :UploadedTempfile, 'action_controller/uploaded_file' autoload :RecordIdentifier, 'action_controller/record_identifier' - autoload :Response, 'action_controller/response' + autoload :Request, 'action_controller/request' autoload :RequestForgeryProtection, 'action_controller/request_forgery_protection' + autoload :RequestParser, 'action_controller/request_parser' autoload :Rescue, 'action_controller/rescue' autoload :Resources, 'action_controller/resources' + autoload :Response, 'action_controller/response' + autoload :RewindableInput, 'action_controller/rewindable_input' autoload :Routing, 'action_controller/routing' autoload :SessionManagement, 'action_controller/session_management' autoload :StatusCodes, 'action_controller/status_codes' @@ -76,6 +74,9 @@ module ActionController autoload :TestCase, 'action_controller/test_case' autoload :TestProcess, 'action_controller/test_process' autoload :Translation, 'action_controller/translation' + autoload :UploadedStringIO, 'action_controller/uploaded_file' + autoload :UploadedTempfile, 'action_controller/uploaded_file' + autoload :UrlEncodedPairParser, 'action_controller/url_encoded_pair_parser' autoload :UrlRewriter, 'action_controller/url_rewriter' autoload :UrlWriter, 'action_controller/url_rewriter' autoload :VerbPiggybacking, 'action_controller/verb_piggybacking' diff --git a/actionpack/lib/action_controller/middlewares.rb b/actionpack/lib/action_controller/middlewares.rb index 793739723f..914750bc0c 100644 --- a/actionpack/lib/action_controller/middlewares.rb +++ b/actionpack/lib/action_controller/middlewares.rb @@ -18,4 +18,5 @@ use "ActiveRecord::QueryCache", :if => lambda { defined?(ActiveRecord) } ) end +use ActionController::RewindableInput use ActionController::VerbPiggybacking diff --git a/actionpack/lib/action_controller/rewindable_input.rb b/actionpack/lib/action_controller/rewindable_input.rb new file mode 100644 index 0000000000..296d8aed22 --- /dev/null +++ b/actionpack/lib/action_controller/rewindable_input.rb @@ -0,0 +1,40 @@ +module ActionController + class RewindableInput + class RewindableIO < ActiveSupport::BasicObject + def initialize(io) + @io = io + end + + def read(*args) + read_original_io + @io.read(*args) + end + + def rewind + read_original_io + @io.rewind + end + + def method_missing(method, *args, &block) + @io.send(method, *args, &block) + end + + private + def read_original_io + unless @str + @str = @io.read + @io = StringIO.new(@str) + end + end + end + + def initialize(app) + @app = app + end + + def call(env) + env['rack.input'] = RewindableIO.new(env['rack.input']) + @app.call(env) + end + end +end diff --git a/actionpack/test/controller/request/multipart_params_parsing_test.rb b/actionpack/test/controller/request/multipart_params_parsing_test.rb index ce28ff46fe..d976ab8512 100644 --- a/actionpack/test/controller/request/multipart_params_parsing_test.rb +++ b/actionpack/test/controller/request/multipart_params_parsing_test.rb @@ -123,14 +123,14 @@ class MultipartParamsParsingTest < ActionController::IntegrationTest InputWrapper = Rack::Lint::InputWrapper test "parses unwindable stream" do - InputWrapper.any_instance.expects(:rewind).raises(Errno::ESPIPE) + InputWrapper.any_instance.stubs(:rewind).raises(Errno::ESPIPE) params = parse_multipart('large_text_file') assert_equal %w(file foo), params.keys.sort assert_equal 'bar', params['foo'] end test "uploads and reads file with unwindable input" do - InputWrapper.any_instance.expects(:rewind).raises(Errno::ESPIPE) + InputWrapper.any_instance.stubs(:rewind).raises(Errno::ESPIPE) with_test_routing do post '/read', :uploaded_data => fixture_file_upload(FIXTURE_PATH + "/hello.txt", "text/plain") @@ -138,6 +138,26 @@ class MultipartParamsParsingTest < ActionController::IntegrationTest end end + test "passes through rack middleware and uploads file" do + with_muck_middleware do + with_test_routing do + post '/read', :uploaded_data => fixture_file_upload(FIXTURE_PATH + "/hello.txt", "text/plain") + assert_equal "File: Hello", response.body + end + end + end + + test "passes through rack middleware and uploads file with unwindable input" do + InputWrapper.any_instance.stubs(:rewind).raises(Errno::ESPIPE) + + with_muck_middleware do + with_test_routing do + post '/read', :uploaded_data => fixture_file_upload(FIXTURE_PATH + "/hello.txt", "text/plain") + assert_equal "File: Hello", response.body + end + end + end + private def fixture(name) File.open(File.join(FIXTURE_PATH, name), 'rb') do |file| @@ -164,4 +184,25 @@ class MultipartParamsParsingTest < ActionController::IntegrationTest yield end end + + class MuckMiddleware + def initialize(app) + @app = app + end + + def call(env) + req = Rack::Request.new(env) + req.params # Parse params + @app.call(env) + end + end + + def with_muck_middleware + original_middleware = ActionController::Dispatcher.middleware + middleware = original_middleware.dup + middleware.use MuckMiddleware + ActionController::Dispatcher.middleware = middleware + yield + ActionController::Dispatcher.middleware = original_middleware + end end diff --git a/actionpack/test/controller/request/url_encoded_params_parsing_test.rb b/actionpack/test/controller/request/url_encoded_params_parsing_test.rb index 26a4538ca6..b162796e5b 100644 --- a/actionpack/test/controller/request/url_encoded_params_parsing_test.rb +++ b/actionpack/test/controller/request/url_encoded_params_parsing_test.rb @@ -150,8 +150,45 @@ class UrlEncodedParamsParsingTest < ActionController::IntegrationTest assert_parses expected, query end + test "passes through rack middleware and parses params" do + with_muck_middleware do + assert_parses({ "a" => { "b" => "c" } }, "a[b]=c") + end + end + + # The lint wrapper is used in integration tests + # instead of a normal StringIO class + InputWrapper = Rack::Lint::InputWrapper + + test "passes through rack middleware and parses params with unwindable input" do + InputWrapper.any_instance.stubs(:rewind).raises(Errno::ESPIPE) + with_muck_middleware do + assert_parses({ "a" => { "b" => "c" } }, "a[b]=c") + end + end private + class MuckMiddleware + def initialize(app) + @app = app + end + + def call(env) + req = Rack::Request.new(env) + req.params # Parse params + @app.call(env) + end + end + + def with_muck_middleware + original_middleware = ActionController::Dispatcher.middleware + middleware = original_middleware.dup + middleware.use MuckMiddleware + ActionController::Dispatcher.middleware = middleware + yield + ActionController::Dispatcher.middleware = original_middleware + end + def with_test_routing with_routing do |set| set.draw do |map| -- cgit v1.2.3 From 9775c25824feb35a5c42f3838d21c7e5faba9ca0 Mon Sep 17 00:00:00 2001 From: Joshua Peek Date: Tue, 13 Jan 2009 17:21:45 -0600 Subject: Update multipart tests to expose (another) bug in Rack's multipart parser --- actionpack/lib/action_controller.rb | 1 + actionpack/lib/action_controller/integration.rb | 11 ----------- .../lib/action_controller/middleware_stack.rb | 2 ++ actionpack/lib/action_controller/rack_ext.rb | 22 ++++++++++++++++++++++ .../lib/action_controller/rewindable_input.rb | 10 +++++++--- .../request/multipart_params_parsing_test.rb | 2 +- .../request/url_encoded_params_parsing_test.rb | 2 +- 7 files changed, 34 insertions(+), 16 deletions(-) create mode 100644 actionpack/lib/action_controller/rack_ext.rb diff --git a/actionpack/lib/action_controller.rb b/actionpack/lib/action_controller.rb index 0d81b9c346..a9cd09e58d 100644 --- a/actionpack/lib/action_controller.rb +++ b/actionpack/lib/action_controller.rb @@ -33,6 +33,7 @@ end gem 'rack', '>= 0.9.0' require 'rack' +require 'action_controller/rack_ext' module ActionController # TODO: Review explicit to see if they will automatically be handled by diff --git a/actionpack/lib/action_controller/integration.rb b/actionpack/lib/action_controller/integration.rb index 5b08e30d49..163ba84a3e 100644 --- a/actionpack/lib/action_controller/integration.rb +++ b/actionpack/lib/action_controller/integration.rb @@ -2,17 +2,6 @@ require 'stringio' require 'uri' require 'active_support/test_case' -# Monkey patch Rack::Lint to support rewind -module Rack - class Lint - class InputWrapper - def rewind - @input.rewind - end - end - end -end - module ActionController module Integration #:nodoc: # An integration Session instance represents a set of requests and responses diff --git a/actionpack/lib/action_controller/middleware_stack.rb b/actionpack/lib/action_controller/middleware_stack.rb index 2bccba2ba1..b94bf6ec4a 100644 --- a/actionpack/lib/action_controller/middleware_stack.rb +++ b/actionpack/lib/action_controller/middleware_stack.rb @@ -32,6 +32,8 @@ module ActionController else @klass.to_s.constantize end + rescue NameError + @klass end def active? diff --git a/actionpack/lib/action_controller/rack_ext.rb b/actionpack/lib/action_controller/rack_ext.rb new file mode 100644 index 0000000000..3b142307e9 --- /dev/null +++ b/actionpack/lib/action_controller/rack_ext.rb @@ -0,0 +1,22 @@ +module Rack + module Utils + module Multipart + class << self + def parse_multipart_with_rewind(env) + result = parse_multipart_without_rewind(env) + + begin + env['rack.input'].rewind if env['rack.input'].respond_to?(:rewind) + rescue Errno::ESPIPE + # Handles exceptions raised by input streams that cannot be rewound + # such as when using plain CGI under Apache + end + + result + end + + alias_method_chain :parse_multipart, :rewind + end + end + end +end diff --git a/actionpack/lib/action_controller/rewindable_input.rb b/actionpack/lib/action_controller/rewindable_input.rb index 296d8aed22..058453ea68 100644 --- a/actionpack/lib/action_controller/rewindable_input.rb +++ b/actionpack/lib/action_controller/rewindable_input.rb @@ -15,15 +15,19 @@ module ActionController @io.rewind end + def string + @string + end + def method_missing(method, *args, &block) @io.send(method, *args, &block) end private def read_original_io - unless @str - @str = @io.read - @io = StringIO.new(@str) + unless @string + @string = @io.read + @io = StringIO.new(@string) end end end diff --git a/actionpack/test/controller/request/multipart_params_parsing_test.rb b/actionpack/test/controller/request/multipart_params_parsing_test.rb index d976ab8512..137fdbee54 100644 --- a/actionpack/test/controller/request/multipart_params_parsing_test.rb +++ b/actionpack/test/controller/request/multipart_params_parsing_test.rb @@ -200,7 +200,7 @@ class MultipartParamsParsingTest < ActionController::IntegrationTest def with_muck_middleware original_middleware = ActionController::Dispatcher.middleware middleware = original_middleware.dup - middleware.use MuckMiddleware + middleware.insert_after ActionController::RewindableInput, MuckMiddleware ActionController::Dispatcher.middleware = middleware yield ActionController::Dispatcher.middleware = original_middleware diff --git a/actionpack/test/controller/request/url_encoded_params_parsing_test.rb b/actionpack/test/controller/request/url_encoded_params_parsing_test.rb index b162796e5b..ee2a239d50 100644 --- a/actionpack/test/controller/request/url_encoded_params_parsing_test.rb +++ b/actionpack/test/controller/request/url_encoded_params_parsing_test.rb @@ -183,7 +183,7 @@ class UrlEncodedParamsParsingTest < ActionController::IntegrationTest def with_muck_middleware original_middleware = ActionController::Dispatcher.middleware middleware = original_middleware.dup - middleware.use MuckMiddleware + middleware.insert_after ActionController::RewindableInput, MuckMiddleware ActionController::Dispatcher.middleware = middleware yield ActionController::Dispatcher.middleware = original_middleware -- cgit v1.2.3 From b281a6a5b2137548501ef590379d7af5f6955d2d Mon Sep 17 00:00:00 2001 From: Joshua Peek Date: Tue, 13 Jan 2009 17:26:29 -0600 Subject: Use Rack's MethodOverride lib [#1699 state:resolved] --- actionpack/lib/action_controller.rb | 1 - actionpack/lib/action_controller/middlewares.rb | 2 +- .../lib/action_controller/verb_piggybacking.rb | 24 ---------------------- railties/lib/initializer.rb | 2 +- 4 files changed, 2 insertions(+), 27 deletions(-) delete mode 100644 actionpack/lib/action_controller/verb_piggybacking.rb diff --git a/actionpack/lib/action_controller.rb b/actionpack/lib/action_controller.rb index a9cd09e58d..f808bdd910 100644 --- a/actionpack/lib/action_controller.rb +++ b/actionpack/lib/action_controller.rb @@ -80,7 +80,6 @@ module ActionController autoload :UrlEncodedPairParser, 'action_controller/url_encoded_pair_parser' autoload :UrlRewriter, 'action_controller/url_rewriter' autoload :UrlWriter, 'action_controller/url_rewriter' - autoload :VerbPiggybacking, 'action_controller/verb_piggybacking' autoload :Verification, 'action_controller/verification' module Assertions diff --git a/actionpack/lib/action_controller/middlewares.rb b/actionpack/lib/action_controller/middlewares.rb index 914750bc0c..0f038b8856 100644 --- a/actionpack/lib/action_controller/middlewares.rb +++ b/actionpack/lib/action_controller/middlewares.rb @@ -19,4 +19,4 @@ use "ActiveRecord::QueryCache", :if => lambda { defined?(ActiveRecord) } end use ActionController::RewindableInput -use ActionController::VerbPiggybacking +use Rack::MethodOverride diff --git a/actionpack/lib/action_controller/verb_piggybacking.rb b/actionpack/lib/action_controller/verb_piggybacking.rb deleted file mode 100644 index 86cde304a0..0000000000 --- a/actionpack/lib/action_controller/verb_piggybacking.rb +++ /dev/null @@ -1,24 +0,0 @@ -module ActionController - # TODO: Use Rack::MethodOverride when it is released - class VerbPiggybacking - HTTP_METHODS = %w(GET HEAD PUT POST DELETE OPTIONS) - - def initialize(app) - @app = app - end - - def call(env) - if env["REQUEST_METHOD"] == "POST" - req = Request.new(env) - if method = (req.parameters[:_method] || env["HTTP_X_HTTP_METHOD_OVERRIDE"]) - method = method.to_s.upcase - if HTTP_METHODS.include?(method) - env["REQUEST_METHOD"] = method - end - end - end - - @app.call(env) - end - end -end diff --git a/railties/lib/initializer.rb b/railties/lib/initializer.rb index 619701460d..824d1d6096 100644 --- a/railties/lib/initializer.rb +++ b/railties/lib/initializer.rb @@ -537,7 +537,7 @@ Run `rake gems:install` to install the missing gems. end def initialize_metal - configuration.middleware.insert_before(:"ActionController::VerbPiggybacking", Rails::Rack::Metal) + configuration.middleware.insert_before(:"ActionController::RewindableInput", Rails::Rack::Metal) end # Initializes framework-specific settings for each of the loaded frameworks -- cgit v1.2.3 From 9bcf01b23c25e640da7908ac8b1b49fbf7d2e51a Mon Sep 17 00:00:00 2001 From: "Hongli Lai (Phusion)" Date: Tue, 13 Jan 2009 16:01:44 +0100 Subject: Fix PostgreSQL unit test failures that only occur when using the old 'postgres' driver. [#1748 state:committed] Signed-off-by: Jeremy Kemper --- .../connection_adapters/abstract/database_statements.rb | 12 ++++++++---- .../active_record/connection_adapters/postgresql_adapter.rb | 10 ++++------ activerecord/test/cases/transactions_test.rb | 3 +-- 3 files changed, 13 insertions(+), 12 deletions(-) diff --git a/activerecord/lib/active_record/connection_adapters/abstract/database_statements.rb b/activerecord/lib/active_record/connection_adapters/abstract/database_statements.rb index 39118583bd..08601da00a 100644 --- a/activerecord/lib/active_record/connection_adapters/abstract/database_statements.rb +++ b/activerecord/lib/active_record/connection_adapters/abstract/database_statements.rb @@ -89,7 +89,7 @@ module ActiveRecord # - The block will be run without doing anything. All database statements # that happen within the block are effectively appended to the already # open database transaction. - # - However, if +requires_new+ is set, the block will be wrapped in a + # - However, if +:requires_new+ is set, the block will be wrapped in a # database savepoint acting as a sub-transaction. # # === Caveats @@ -113,8 +113,12 @@ module ActiveRecord def transaction(options = {}) options.assert_valid_keys :requires_new, :joinable - last_transaction_joinable, @transaction_joinable = - @transaction_joinable, options[:joinable] || true + last_transaction_joinable = @transaction_joinable + if options.has_key?(:joinable) + @transaction_joinable = options[:joinable] + else + @transaction_joinable = true + end requires_new = options[:requires_new] || !last_transaction_joinable transaction_open = false @@ -141,7 +145,7 @@ module ActiveRecord rollback_to_savepoint end end - raise unless database_transaction_rollback.is_a? ActiveRecord::Rollback + raise unless database_transaction_rollback.is_a?(ActiveRecord::Rollback) end ensure @transaction_joinable = last_transaction_joinable diff --git a/activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb b/activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb index 5a8d99924d..913bb521ca 100644 --- a/activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb +++ b/activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb @@ -533,13 +533,11 @@ module ActiveRecord execute "ROLLBACK" end - if PGconn.public_method_defined?(:transaction_status) - # ruby-pg defines Ruby constants for transaction status, - # ruby-postgres does not. - PQTRANS_IDLE = defined?(PGconn::PQTRANS_IDLE) ? PGconn::PQTRANS_IDLE : 0 - + if defined?(PGconn::PQTRANS_IDLE) + # The ruby-pg driver supports inspecting the transaction status, + # while the ruby-postgres driver does not. def outside_transaction? - @connection.transaction_status == PQTRANS_IDLE + @connection.transaction_status == PGconn::PQTRANS_IDLE end end diff --git a/activerecord/test/cases/transactions_test.rb b/activerecord/test/cases/transactions_test.rb index f07fad1828..4a07a8bb1d 100644 --- a/activerecord/test/cases/transactions_test.rb +++ b/activerecord/test/cases/transactions_test.rb @@ -321,9 +321,8 @@ class TransactionTest < ActiveRecord::TestCase end end - if current_adapter?(:PostgreSQLAdapter) && PGconn.public_method_defined?(:transaction_status) + if current_adapter?(:PostgreSQLAdapter) && defined?(PGconn::PQTRANS_IDLE) def test_outside_transaction_works - Topic.logger.info("-------------") assert Topic.connection.outside_transaction? Topic.connection.begin_db_transaction assert !Topic.connection.outside_transaction? -- cgit v1.2.3