diff options
41 files changed, 452 insertions, 156 deletions
diff --git a/actionpack/lib/abstract_controller.rb b/actionpack/lib/abstract_controller.rb index 0477e7f1c9..3a98931167 100644 --- a/actionpack/lib/abstract_controller.rb +++ b/actionpack/lib/abstract_controller.rb @@ -7,6 +7,7 @@ require "active_support/i18n" module AbstractController extend ActiveSupport::Autoload + autoload :ActionNotFound, "abstract_controller/base" autoload :Base autoload :Caching autoload :Callbacks diff --git a/actionpack/lib/action_dispatch/middleware/debug_exceptions.rb b/actionpack/lib/action_dispatch/middleware/debug_exceptions.rb index 5f5fdbc66a..7669767ae3 100644 --- a/actionpack/lib/action_dispatch/middleware/debug_exceptions.rb +++ b/actionpack/lib/action_dispatch/middleware/debug_exceptions.rb @@ -180,11 +180,14 @@ module ActionDispatch trace = wrapper.framework_trace if trace.empty? ActiveSupport::Deprecation.silence do - logger.fatal " " - logger.fatal "#{exception.class} (#{exception.message}):" - log_array logger, exception.annoted_source_code if exception.respond_to?(:annoted_source_code) - logger.fatal " " - log_array logger, trace + message = [] + message << " " + message << "#{exception.class} (#{exception.message}):" + message.concat(exception.annoted_source_code) if exception.respond_to?(:annoted_source_code) + message << " " + message.concat(trace) + + log_array(logger, message) end end diff --git a/actionpack/test/abstract_unit.rb b/actionpack/test/abstract_unit.rb index 65dd28b3d7..f23151e518 100644 --- a/actionpack/test/abstract_unit.rb +++ b/actionpack/test/abstract_unit.rb @@ -13,13 +13,6 @@ silence_warnings do Encoding.default_external = Encoding::UTF_8 end -require "drb" -begin - require "drb/unix" -rescue LoadError - puts "'drb/unix' is not available" -end - if ENV["TRAVIS"] PROCESS_COUNT = 0 else @@ -80,7 +73,7 @@ end module ActiveSupport class TestCase if RUBY_ENGINE == "ruby" && PROCESS_COUNT > 0 - parallelize_me! + parallelize(workers: PROCESS_COUNT) end end end @@ -359,75 +352,6 @@ class ImagesController < ResourcesController; end require "active_support/testing/method_call_assertions" -class ForkingExecutor - class Server - include DRb::DRbUndumped - - def initialize - @queue = Queue.new - end - - def record(reporter, result) - reporter.record result - end - - def <<(o) - o[2] = DRbObject.new(o[2]) if o - @queue << o - end - def pop; @queue.pop; end - end - - def initialize(size) - @size = size - @queue = Server.new - @pool = nil - @url = DRb.start_service("drbunix:", @queue).uri - end - - def <<(work); @queue << work; end - - def shutdown - pool = @size.times.map { - fork { - DRb.stop_service - queue = DRbObject.new_with_uri @url - while job = queue.pop - klass = job[0] - method = job[1] - reporter = job[2] - result = Minitest.run_one_method klass, method - if result.error? - translate_exceptions result - end - queue.record reporter, result - end - } - } - @size.times { @queue << nil } - pool.each { |pid| Process.waitpid pid } - end - - private - def translate_exceptions(result) - result.failures.map! { |e| - begin - Marshal.dump e - e - rescue TypeError - ex = Exception.new e.message - ex.set_backtrace e.backtrace - Minitest::UnexpectedError.new ex - end - } - end -end - -if RUBY_ENGINE == "ruby" && PROCESS_COUNT > 0 - # Use N processes (N defaults to 4) - Minitest.parallel_executor = ForkingExecutor.new(PROCESS_COUNT) -end - class ActiveSupport::TestCase include ActiveSupport::Testing::MethodCallAssertions diff --git a/activejob/lib/active_job/arguments.rb b/activejob/lib/active_job/arguments.rb index fa58c50ed0..92eb58aaaf 100644 --- a/activejob/lib/active_job/arguments.rb +++ b/activejob/lib/active_job/arguments.rb @@ -91,7 +91,7 @@ module ActiveJob def deserialize_argument(argument) case argument when String - GlobalID::Locator.locate(argument) || argument + argument when *PERMITTED_TYPES argument when Array diff --git a/activejob/test/cases/argument_serialization_test.rb b/activejob/test/cases/argument_serialization_test.rb index e4e14016d9..da198abc0b 100644 --- a/activejob/test/cases/argument_serialization_test.rb +++ b/activejob/test/cases/argument_serialization_test.rb @@ -41,6 +41,10 @@ class ArgumentSerializationTest < ActiveSupport::TestCase assert_arguments_roundtrip [@person] end + test "should keep Global IDs strings as they are" do + assert_arguments_roundtrip [@person.to_gid.to_s] + end + test "should dive deep into arrays and hashes" do assert_arguments_roundtrip [3, [@person]] assert_arguments_roundtrip [{ "a" => @person }] diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md index 13cc486e7f..5b07d63684 100644 --- a/activerecord/CHANGELOG.md +++ b/activerecord/CHANGELOG.md @@ -1,3 +1,42 @@ +* Allow aliased attributes to be used in `#update_columns` and `#update`. + + *Gannon McGibbon* + +* Allow spaces in postgres table names. + + Fixes issue where "user post" is misinterpreted as "\"user\".\"post\"" when quoting table names with the postgres adapter. + + *Gannon McGibbon* + +* Cached columns_hash fields should be excluded from ResultSet#column_types + + PR #34528 addresses the inconsistent behaviour when attribute is defined for an ignored column. The following test + was passing for SQLite and MySQL, but failed for PostgreSQL: + + ```ruby + class DeveloperName < ActiveRecord::Type::String + def deserialize(value) + "Developer: #{value}" + end + end + + class AttributedDeveloper < ActiveRecord::Base + self.table_name = "developers" + + attribute :name, DeveloperName.new + + self.ignored_columns += ["name"] + end + + developer = AttributedDeveloper.create + developer.update_column :name, "name" + + loaded_developer = AttributedDeveloper.where(id: developer.id).select("*").first + puts loaded_developer.name # should be "Developer: name" but it's just "name" + ``` + + *Dmitry Tsepelev* + * Make the implicit order column configurable. When calling ordered finder methods such as +first+ or +last+ without an diff --git a/activerecord/lib/active_record/attribute_methods/dirty.rb b/activerecord/lib/active_record/attribute_methods/dirty.rb index ebc2252c50..45e4b8adfa 100644 --- a/activerecord/lib/active_record/attribute_methods/dirty.rb +++ b/activerecord/lib/active_record/attribute_methods/dirty.rb @@ -158,9 +158,13 @@ module ActiveRecord end private - def write_attribute_without_type_cast(attr_name, _) - result = super - clear_attribute_change(attr_name) + def write_attribute_without_type_cast(attr_name, value) + name = attr_name.to_s + if self.class.attribute_alias?(name) + name = self.class.attribute_alias(name) + end + result = super(name, value) + clear_attribute_change(name) result end diff --git a/activerecord/lib/active_record/connection_adapters/connection_specification.rb b/activerecord/lib/active_record/connection_adapters/connection_specification.rb index 2e7a78215a..f60d8469cc 100644 --- a/activerecord/lib/active_record/connection_adapters/connection_specification.rb +++ b/activerecord/lib/active_record/connection_adapters/connection_specification.rb @@ -174,12 +174,12 @@ module ActiveRecord if e.path == path_to_adapter # We can assume that a non-builtin adapter was specified, so it's # either misspelled or missing from Gemfile. - raise e.class, "Could not load the '#{spec[:adapter]}' Active Record adapter. Ensure that the adapter is spelled correctly in config/database.yml and that you've added the necessary adapter gem to your Gemfile.", e.backtrace + raise LoadError, "Could not load the '#{spec[:adapter]}' Active Record adapter. Ensure that the adapter is spelled correctly in config/database.yml and that you've added the necessary adapter gem to your Gemfile.", e.backtrace # Bubbled up from the adapter require. Prefix the exception message # with some guidance about how to address it and reraise. else - raise e.class, "Error loading the '#{spec[:adapter]}' Active Record adapter. Missing a gem it depends on? #{e.message}", e.backtrace + raise LoadError, "Error loading the '#{spec[:adapter]}' Active Record adapter. Missing a gem it depends on? #{e.message}", e.backtrace end end diff --git a/activerecord/lib/active_record/connection_adapters/postgresql/utils.rb b/activerecord/lib/active_record/connection_adapters/postgresql/utils.rb index bfd300723d..f2f4701500 100644 --- a/activerecord/lib/active_record/connection_adapters/postgresql/utils.rb +++ b/activerecord/lib/active_record/connection_adapters/postgresql/utils.rb @@ -68,7 +68,7 @@ module ActiveRecord # * <tt>"schema_name".table_name</tt> # * <tt>"schema.name"."table name"</tt> def extract_schema_qualified_name(string) - schema, table = string.scan(/[^".\s]+|"[^"]*"/) + schema, table = string.scan(/[^".]+|"[^"]*"/) if table.nil? table = schema schema = nil diff --git a/activerecord/lib/active_record/integration.rb b/activerecord/lib/active_record/integration.rb index 33c4066b89..90fb10a1f1 100644 --- a/activerecord/lib/active_record/integration.rb +++ b/activerecord/lib/active_record/integration.rb @@ -96,8 +96,19 @@ module ActiveRecord # Note, this method will return nil if ActiveRecord::Base.cache_versioning is set to # +false+ (which it is by default until Rails 6.0). def cache_version - if cache_versioning && timestamp = try(:updated_at) - timestamp.utc.to_s(:usec) + return unless cache_versioning + + if has_attribute?("updated_at") + timestamp = updated_at_before_type_cast + if can_use_fast_cache_version?(timestamp) + raw_timestamp_to_cache_version(timestamp) + elsif timestamp = updated_at + timestamp.utc.to_s(cache_timestamp_format) + end + else + if self.class.has_attribute?("updated_at") + raise ActiveModel::MissingAttributeError, "missing attribute: updated_at" + end end end @@ -151,5 +162,43 @@ module ActiveRecord end end end + + private + # Detects if the value before type cast + # can be used to generate a cache_version. + # + # The fast cache version only works with a + # string value directly from the database. + # + # We also must check if the timestamp format has been changed + # or if the timezone is not set to UTC then + # we cannot apply our transformations correctly. + def can_use_fast_cache_version?(timestamp) + timestamp.is_a?(String) && + cache_timestamp_format == :usec && + default_timezone == :utc && + !updated_at_came_from_user? + end + + # Converts a raw database string to `:usec` + # format. + # + # Example: + # + # timestamp = "2018-10-15 20:02:15.266505" + # raw_timestamp_to_cache_version(timestamp) + # # => "20181015200215266505" + # + # Postgres truncates trailing zeros, + # https://github.com/postgres/postgres/commit/3e1beda2cde3495f41290e1ece5d544525810214 + # to account for this we pad the output with zeros + def raw_timestamp_to_cache_version(timestamp) + key = timestamp.delete("- :.") + if key.length < 20 + key.ljust(20, "0") + else + key + end + end end end diff --git a/activerecord/lib/active_record/querying.rb b/activerecord/lib/active_record/querying.rb index c84f3d0fbb..8c1b2e2be1 100644 --- a/activerecord/lib/active_record/querying.rb +++ b/activerecord/lib/active_record/querying.rb @@ -16,17 +16,17 @@ module ActiveRecord delegate :pluck, :pick, :ids, to: :all # Executes a custom SQL query against your database and returns all the results. The results will - # be returned as an array with columns requested encapsulated as attributes of the model you call - # this method from. If you call <tt>Product.find_by_sql</tt> then the results will be returned in + # be returned as an array, with the requested columns encapsulated as attributes of the model you call + # this method from. For example, if you call <tt>Product.find_by_sql</tt>, then the results will be returned in # a +Product+ object with the attributes you specified in the SQL query. # - # If you call a complicated SQL query which spans multiple tables the columns specified by the + # If you call a complicated SQL query which spans multiple tables, the columns specified by the # SELECT will be attributes of the model, whether or not they are columns of the corresponding # table. # - # The +sql+ parameter is a full SQL query as a string. It will be called as is, there will be - # no database agnostic conversions performed. This should be a last resort because using, for example, - # MySQL specific terms will lock you to using that particular database engine or require you to + # The +sql+ parameter is a full SQL query as a string. It will be called as is; there will be + # no database agnostic conversions performed. This should be a last resort because using + # database-specific terms will lock you into using that particular database engine, or require you to # change your call if you switch engines. # # # A simple SQL query spanning multiple tables @@ -40,7 +40,8 @@ module ActiveRecord def find_by_sql(sql, binds = [], preparable: nil, &block) result_set = connection.select_all(sanitize_sql(sql), "#{name} Load", binds, preparable: preparable) column_types = result_set.column_types.dup - columns_hash.each_key { |k| column_types.delete k } + cached_columns_hash = connection.schema_cache.columns_hash(table_name) + cached_columns_hash.each_key { |k| column_types.delete k } message_bus = ActiveSupport::Notifications.instrumenter payload = { @@ -60,7 +61,9 @@ module ActiveRecord # Returns the result of an SQL statement that should only include a COUNT(*) in the SELECT part. # The use of this method should be restricted to complicated SQL queries that can't be executed - # using the ActiveRecord::Calculations class methods. Look into those before using this. + # using the ActiveRecord::Calculations class methods. Look into those before using this method, + # as it could lock you into a specific database engine or require a code change to switch + # database engines. # # Product.count_by_sql "SELECT COUNT(*) FROM sales s, customers c WHERE s.customer_id = c.id" # # => 12 diff --git a/activerecord/test/cases/adapters/postgresql/quoting_test.rb b/activerecord/test/cases/adapters/postgresql/quoting_test.rb index d50dc49276..d571355a9c 100644 --- a/activerecord/test/cases/adapters/postgresql/quoting_test.rb +++ b/activerecord/test/cases/adapters/postgresql/quoting_test.rb @@ -39,6 +39,11 @@ module ActiveRecord type = OID::Bit.new assert_nil @conn.quote(type.serialize(value)) end + + def test_quote_table_name_with_spaces + value = "user posts" + assert_equal "\"user posts\"", @conn.quote_table_name(value) + end end end end diff --git a/activerecord/test/cases/attribute_methods_test.rb b/activerecord/test/cases/attribute_methods_test.rb index 12a4027a4e..d341dd0083 100644 --- a/activerecord/test/cases/attribute_methods_test.rb +++ b/activerecord/test/cases/attribute_methods_test.rb @@ -323,6 +323,12 @@ class AttributeMethodsTest < ActiveRecord::TestCase assert_raises(ActiveModel::UnknownAttributeError) { topic.update(no_column_exists: "Hello!") } end + test "write_attribute allows writing to aliased attributes" do + topic = Topic.first + assert_nothing_raised { topic.update_columns(heading: "Hello!") } + assert_nothing_raised { topic.update(heading: "Hello!") } + end + test "read_attribute" do topic = Topic.new topic.title = "Don't change the topic" diff --git a/activerecord/test/cases/base_test.rb b/activerecord/test/cases/base_test.rb index 09e5517449..63a73101c4 100644 --- a/activerecord/test/cases/base_test.rb +++ b/activerecord/test/cases/base_test.rb @@ -1447,6 +1447,14 @@ class BasicsTest < ActiveRecord::TestCase assert_not_respond_to developer, :first_name= end + test "when ignored attribute is loaded, cast type should be preferred over DB type" do + developer = AttributedDeveloper.create + developer.update_column :name, "name" + + loaded_developer = AttributedDeveloper.where(id: developer.id).select("*").first + assert_equal "Developer: name", loaded_developer.name + end + test "ignored columns not included in SELECT" do query = Developer.all.to_sql.downcase diff --git a/activerecord/test/cases/cache_key_test.rb b/activerecord/test/cases/cache_key_test.rb index 3a569f226e..3a06b1c795 100644 --- a/activerecord/test/cases/cache_key_test.rb +++ b/activerecord/test/cases/cache_key_test.rb @@ -44,10 +44,88 @@ module ActiveRecord test "cache_key_with_version always has both key and version" do r1 = CacheMeWithVersion.create - assert_equal "active_record/cache_key_test/cache_me_with_versions/#{r1.id}-#{r1.updated_at.to_s(:usec)}", r1.cache_key_with_version + assert_equal "active_record/cache_key_test/cache_me_with_versions/#{r1.id}-#{r1.updated_at.utc.to_s(:usec)}", r1.cache_key_with_version r2 = CacheMe.create - assert_equal "active_record/cache_key_test/cache_mes/#{r2.id}-#{r2.updated_at.to_s(:usec)}", r2.cache_key_with_version + assert_equal "active_record/cache_key_test/cache_mes/#{r2.id}-#{r2.updated_at.utc.to_s(:usec)}", r2.cache_key_with_version + end + + test "cache_version is the same when it comes from the DB or from the user" do + skip("Mysql2 does not return a string value for updated_at") if current_adapter?(:Mysql2Adapter) + + record = CacheMeWithVersion.create + record_from_db = CacheMeWithVersion.find(record.id) + assert_not_called(record_from_db, :updated_at) do + record_from_db.cache_version + end + + assert_equal record.cache_version, record_from_db.cache_version + end + + test "cache_version does not truncate zeros when timestamp ends in zeros" do + skip("Mysql2 does not return a string value for updated_at") if current_adapter?(:Mysql2Adapter) + + travel_to Time.now.beginning_of_day do + record = CacheMeWithVersion.create + record_from_db = CacheMeWithVersion.find(record.id) + assert_not_called(record_from_db, :updated_at) do + record_from_db.cache_version + end + + assert_equal record.cache_version, record_from_db.cache_version + end + end + + test "cache_version calls updated_at when the value is generated at create time" do + record = CacheMeWithVersion.create + assert_called(record, :updated_at) do + record.cache_version + end + end + + test "cache_version does NOT call updated_at when value is from the database" do + skip("Mysql2 does not return a string value for updated_at") if current_adapter?(:Mysql2Adapter) + + record = CacheMeWithVersion.create + record_from_db = CacheMeWithVersion.find(record.id) + assert_not_called(record_from_db, :updated_at) do + record_from_db.cache_version + end + end + + test "cache_version does call updated_at when it is assigned via a Time object" do + record = CacheMeWithVersion.create + record_from_db = CacheMeWithVersion.find(record.id) + assert_called(record_from_db, :updated_at) do + record_from_db.updated_at = Time.now + record_from_db.cache_version + end + end + + test "cache_version does call updated_at when it is assigned via a string" do + record = CacheMeWithVersion.create + record_from_db = CacheMeWithVersion.find(record.id) + assert_called(record_from_db, :updated_at) do + record_from_db.updated_at = Time.now.to_s + record_from_db.cache_version + end + end + + test "cache_version does call updated_at when it is assigned via a hash" do + record = CacheMeWithVersion.create + record_from_db = CacheMeWithVersion.find(record.id) + assert_called(record_from_db, :updated_at) do + record_from_db.updated_at = { 1 => 2016, 2 => 11, 3 => 12, 4 => 1, 5 => 2, 6 => 3, 7 => 22 } + record_from_db.cache_version + end + end + + test "updated_at on class but not on instance raises an error" do + record = CacheMeWithVersion.create + record_from_db = CacheMeWithVersion.where(id: record.id).select(:id).first + assert_raises(ActiveModel::MissingAttributeError) do + record_from_db.cache_version + end end end end diff --git a/activerecord/test/cases/instrumentation_test.rb b/activerecord/test/cases/instrumentation_test.rb index e6e8468757..c09ea32991 100644 --- a/activerecord/test/cases/instrumentation_test.rb +++ b/activerecord/test/cases/instrumentation_test.rb @@ -5,6 +5,10 @@ require "models/book" module ActiveRecord class InstrumentationTest < ActiveRecord::TestCase + def setup + ActiveRecord::Base.connection.schema_cache.add(Book.table_name) + end + def test_payload_name_on_load Book.create(name: "test book") subscriber = ActiveSupport::Notifications.subscribe("sql.active_record") do |*args| diff --git a/activerecord/test/models/developer.rb b/activerecord/test/models/developer.rb index 8881c69368..ec48094207 100644 --- a/activerecord/test/models/developer.rb +++ b/activerecord/test/models/developer.rb @@ -279,3 +279,17 @@ class DeveloperWithIncorrectlyOrderedHasManyThrough < ActiveRecord::Base has_many :companies, through: :contracts has_many :contracts, foreign_key: :developer_id end + +class DeveloperName < ActiveRecord::Type::String + def deserialize(value) + "Developer: #{value}" + end +end + +class AttributedDeveloper < ActiveRecord::Base + self.table_name = "developers" + + attribute :name, DeveloperName.new + + self.ignored_columns += ["name"] +end diff --git a/activestorage/CHANGELOG.md b/activestorage/CHANGELOG.md index ba57315301..ea9b10add9 100644 --- a/activestorage/CHANGELOG.md +++ b/activestorage/CHANGELOG.md @@ -1,3 +1,7 @@ +* Fix `ArgumentError` when uploading to amazon s3 + + *Hiroki Sanpei* + * Add progressive JPG to default list of variable content types *Maurice Kühlborn* diff --git a/activestorage/app/controllers/active_storage/disk_controller.rb b/activestorage/app/controllers/active_storage/disk_controller.rb index 99982202dd..df8d73cc91 100644 --- a/activestorage/app/controllers/active_storage/disk_controller.rb +++ b/activestorage/app/controllers/active_storage/disk_controller.rb @@ -9,7 +9,7 @@ class ActiveStorage::DiskController < ActiveStorage::BaseController def show if key = decode_verified_key - serve_file disk_service.path_for(key), content_type: params[:content_type], disposition: params[:disposition] + serve_file disk_service.path_for(key[:key]), content_type: key[:content_type], disposition: key[:disposition] else head :not_found end diff --git a/activestorage/app/models/active_storage/blob.rb b/activestorage/app/models/active_storage/blob.rb index 53aa9f0237..04f9dbff9f 100644 --- a/activestorage/app/models/active_storage/blob.rb +++ b/activestorage/app/models/active_storage/blob.rb @@ -40,7 +40,7 @@ class ActiveStorage::Blob < ActiveRecord::Base end class << self - # You can used the signed ID of a blob to refer to it on the client side without fear of tampering. + # You can use the signed ID of a blob to refer to it on the client side without fear of tampering. # This is particularly helpful for direct uploads where the client-side needs to refer to the blob # that was created ahead of the upload itself on form submission. # @@ -130,8 +130,8 @@ class ActiveStorage::Blob < ActiveRecord::Base def service_url(expires_in: ActiveStorage.service_urls_expire_in, disposition: :inline, filename: nil, **options) filename = ActiveStorage::Filename.wrap(filename || self.filename) - service.url key, expires_in: expires_in, filename: filename, content_type: content_type, - disposition: forcibly_serve_as_binary? ? :attachment : disposition, **options + service.url key, expires_in: expires_in, filename: filename, content_type: content_type_for_service_url, + disposition: forced_disposition_for_service_url || disposition, **options end # Returns a URL that can be used to directly upload a file for this blob on the service. This URL is intended to be @@ -170,7 +170,7 @@ class ActiveStorage::Blob < ActiveRecord::Base end def upload_without_unfurling(io) #:nodoc: - service.upload key, io, checksum: checksum + service.upload key, io, checksum: checksum, **service_metadata end # Downloads the file associated with this blob. If no block is given, the entire file is read into memory and returned. @@ -239,5 +239,29 @@ class ActiveStorage::Blob < ActiveRecord::Base ActiveStorage.content_types_to_serve_as_binary.include?(content_type) end + def allowed_inline? + ActiveStorage.content_types_allowed_inline.include?(content_type) + end + + def content_type_for_service_url + forcibly_serve_as_binary? ? ActiveStorage.binary_content_type : content_type + end + + def forced_disposition_for_service_url + if forcibly_serve_as_binary? || !allowed_inline? + :attachment + end + end + + def service_metadata + if forcibly_serve_as_binary? + { content_type: ActiveStorage.binary_content_type, disposition: :attachment, filename: filename } + elsif !allowed_inline? + { content_type: content_type, disposition: :attachment, filename: filename } + else + { content_type: content_type } + end + end + ActiveSupport.run_load_hooks(:active_storage_blob, self) end diff --git a/activestorage/app/models/active_storage/blob/identifiable.rb b/activestorage/app/models/active_storage/blob/identifiable.rb index 2c17ddc25f..924bd06131 100644 --- a/activestorage/app/models/active_storage/blob/identifiable.rb +++ b/activestorage/app/models/active_storage/blob/identifiable.rb @@ -2,7 +2,10 @@ module ActiveStorage::Blob::Identifiable def identify - update! content_type: identify_content_type, identified: true unless identified? + unless identified? + update! content_type: identify_content_type, identified: true + update_service_metadata + end end def identified? @@ -21,4 +24,8 @@ module ActiveStorage::Blob::Identifiable "" end end + + def update_service_metadata + service.update_metadata key, service_metadata if service_metadata.any? + end end diff --git a/activestorage/lib/active_storage.rb b/activestorage/lib/active_storage.rb index a94ef626f2..19c7f6be2a 100644 --- a/activestorage/lib/active_storage.rb +++ b/activestorage/lib/active_storage.rb @@ -49,6 +49,8 @@ module ActiveStorage mattr_accessor :paths, default: {} mattr_accessor :variable_content_types, default: [] mattr_accessor :content_types_to_serve_as_binary, default: [] + mattr_accessor :content_types_allowed_inline, default: [] + mattr_accessor :binary_content_type, default: "application/octet-stream" mattr_accessor :service_urls_expire_in, default: 5.minutes mattr_accessor :routes_prefix, default: "/rails/active_storage" diff --git a/activestorage/lib/active_storage/engine.rb b/activestorage/lib/active_storage/engine.rb index d51e806220..b3bcf48d6f 100644 --- a/activestorage/lib/active_storage/engine.rb +++ b/activestorage/lib/active_storage/engine.rb @@ -40,6 +40,18 @@ module ActiveStorage text/xml application/xml application/xhtml+xml + application/mathml+xml + text/cache-manifest + ) + + config.active_storage.content_types_allowed_inline = %w( + image/png + image/gif + image/jpg + image/jpeg + image/vnd.adobe.photoshop + image/vnd.microsoft.icon + application/pdf ) config.eager_load_namespaces << ActiveStorage @@ -57,6 +69,8 @@ module ActiveStorage ActiveStorage.variable_content_types = app.config.active_storage.variable_content_types || [] ActiveStorage.content_types_to_serve_as_binary = app.config.active_storage.content_types_to_serve_as_binary || [] ActiveStorage.service_urls_expire_in = app.config.active_storage.service_urls_expire_in || 5.minutes + ActiveStorage.content_types_allowed_inline = app.config.active_storage.content_types_allowed_inline || [] + ActiveStorage.binary_content_type = app.config.active_storage.binary_content_type || "application/octet-stream" end end diff --git a/activestorage/lib/active_storage/service.rb b/activestorage/lib/active_storage/service.rb index 54ba08fb87..c18fccbb1d 100644 --- a/activestorage/lib/active_storage/service.rb +++ b/activestorage/lib/active_storage/service.rb @@ -62,10 +62,16 @@ module ActiveStorage # Upload the +io+ to the +key+ specified. If a +checksum+ is provided, the service will # ensure a match when the upload has completed or raise an ActiveStorage::IntegrityError. - def upload(key, io, checksum: nil) + def upload(key, io, checksum: nil, **options) raise NotImplementedError end + # Update metadata for the file identified by +key+ in the service. + # Override in subclasses only if the service needs to store specific + # metadata that has to be updated upon identification. + def update_metadata(key, **metadata) + end + # Return the content of the file at the +key+. def download(key) raise NotImplementedError diff --git a/activestorage/lib/active_storage/service/azure_storage_service.rb b/activestorage/lib/active_storage/service/azure_storage_service.rb index 8de3889cb5..a2c4b4d57c 100644 --- a/activestorage/lib/active_storage/service/azure_storage_service.rb +++ b/activestorage/lib/active_storage/service/azure_storage_service.rb @@ -17,7 +17,7 @@ module ActiveStorage @container = container end - def upload(key, io, checksum: nil) + def upload(key, io, checksum: nil, **) instrument :upload, key: key, checksum: checksum do handle_errors do blobs.create_block_blob(container, key, IO.try_convert(io) || io, content_md5: checksum) diff --git a/activestorage/lib/active_storage/service/disk_service.rb b/activestorage/lib/active_storage/service/disk_service.rb index 52f3a3df16..2588c41760 100644 --- a/activestorage/lib/active_storage/service/disk_service.rb +++ b/activestorage/lib/active_storage/service/disk_service.rb @@ -15,7 +15,7 @@ module ActiveStorage @root = root end - def upload(key, io, checksum: nil) + def upload(key, io, checksum: nil, **) instrument :upload, key: key, checksum: checksum do IO.copy_stream(io, make_path_for(key)) ensure_integrity_of(key, checksum) if checksum @@ -79,17 +79,23 @@ module ActiveStorage def url(key, expires_in:, filename:, disposition:, content_type:) instrument :url, key: key do |payload| - verified_key_with_expiration = ActiveStorage.verifier.generate(key, expires_in: expires_in, purpose: :blob_key) - - generated_url = - url_helpers.rails_disk_service_url( - verified_key_with_expiration, - host: current_host, - filename: filename, - disposition: content_disposition_with(type: disposition, filename: filename), + content_disposition = content_disposition_with(type: disposition, filename: filename) + verified_key_with_expiration = ActiveStorage.verifier.generate( + { + key: key, + disposition: content_disposition, content_type: content_type - ) + }, + { expires_in: expires_in, + purpose: :blob_key } + ) + generated_url = url_helpers.rails_disk_service_url(verified_key_with_expiration, + host: current_host, + disposition: content_disposition, + content_type: content_type, + filename: filename + ) payload[:url] = generated_url generated_url diff --git a/activestorage/lib/active_storage/service/gcs_service.rb b/activestorage/lib/active_storage/service/gcs_service.rb index 18c0f14cfc..e2590aa35d 100644 --- a/activestorage/lib/active_storage/service/gcs_service.rb +++ b/activestorage/lib/active_storage/service/gcs_service.rb @@ -11,16 +11,15 @@ module ActiveStorage @config = config end - def upload(key, io, checksum: nil) + def upload(key, io, checksum: nil, content_type: nil, disposition: nil, filename: nil) instrument :upload, key: key, checksum: checksum do begin - # The official GCS client library doesn't allow us to create a file with no Content-Type metadata. - # We need the file we create to have no Content-Type so we can control it via the response-content-type - # param in signed URLs. Workaround: let the GCS client create the file with an inferred - # Content-Type (usually "application/octet-stream") then clear it. - bucket.create_file(io, key, md5: checksum).update do |file| - file.content_type = nil - end + # GCS's signed URLs don't include params such as response-content-type response-content_disposition + # in the signature, which means an attacker can modify them and bypass our effort to force these to + # binary and attachment when the file's content type requires it. The only way to force them is to + # store them as object's metadata. + content_disposition = content_disposition_with(type: disposition, filename: filename) if disposition && filename + bucket.create_file(io, key, md5: checksum, content_type: content_type, content_disposition: content_disposition) rescue Google::Cloud::InvalidArgumentError raise ActiveStorage::IntegrityError end @@ -43,6 +42,15 @@ module ActiveStorage end end + def update_metadata(key, content_type:, disposition: nil, filename: nil) + instrument :update_metadata, key: key, content_type: content_type, disposition: disposition do + file_for(key).update do |file| + file.content_type = content_type + file.content_disposition = content_disposition_with(type: disposition, filename: filename) if disposition && filename + end + end + end + def download_chunk(key, range) instrument :download_chunk, key: key, range: range do begin diff --git a/activestorage/lib/active_storage/service/mirror_service.rb b/activestorage/lib/active_storage/service/mirror_service.rb index 6002ef5a00..75274f81b3 100644 --- a/activestorage/lib/active_storage/service/mirror_service.rb +++ b/activestorage/lib/active_storage/service/mirror_service.rb @@ -24,9 +24,9 @@ module ActiveStorage # Upload the +io+ to the +key+ specified to all services. If a +checksum+ is provided, all services will # ensure a match when the upload has completed or raise an ActiveStorage::IntegrityError. - def upload(key, io, checksum: nil) + def upload(key, io, checksum: nil, **options) each_service.collect do |service| - service.upload key, io.tap(&:rewind), checksum: checksum + service.upload key, io.tap(&:rewind), checksum: checksum, **options end end diff --git a/activestorage/lib/active_storage/service/s3_service.rb b/activestorage/lib/active_storage/service/s3_service.rb index 89a9e54158..90c3ae1b62 100644 --- a/activestorage/lib/active_storage/service/s3_service.rb +++ b/activestorage/lib/active_storage/service/s3_service.rb @@ -16,7 +16,7 @@ module ActiveStorage @upload_options = upload end - def upload(key, io, checksum: nil) + def upload(key, io, checksum: nil, **) instrument :upload, key: key, checksum: checksum do begin object_for(key).put(upload_options.merge(body: io, content_md5: checksum)) diff --git a/activestorage/test/controllers/disk_controller_test.rb b/activestorage/test/controllers/disk_controller_test.rb index 7b5e989699..a723b4d56a 100644 --- a/activestorage/test/controllers/disk_controller_test.rb +++ b/activestorage/test/controllers/disk_controller_test.rb @@ -5,11 +5,12 @@ require "database/setup" class ActiveStorage::DiskControllerTest < ActionDispatch::IntegrationTest test "showing blob inline" do - blob = create_blob + blob = create_blob(filename: "hello.jpg", content_type: "image/jpg") + get blob.service_url assert_response :ok - assert_equal "inline; filename=\"hello.txt\"; filename*=UTF-8''hello.txt", response.headers["Content-Disposition"] - assert_equal "text/plain", response.headers["Content-Type"] + assert_equal "inline; filename=\"hello.jpg\"; filename*=UTF-8''hello.jpg", response.headers["Content-Disposition"] + assert_equal "image/jpg", response.headers["Content-Type"] assert_equal "Hello world!", response.body end @@ -22,11 +23,11 @@ class ActiveStorage::DiskControllerTest < ActionDispatch::IntegrationTest assert_equal "Hello world!", response.body end - test "showing blob range inline" do + test "showing blob range" do blob = create_blob get blob.service_url, headers: { "Range" => "bytes=5-9" } assert_response :partial_content - assert_equal "inline; filename=\"hello.txt\"; filename*=UTF-8''hello.txt", response.headers["Content-Disposition"] + assert_equal "attachment; filename=\"hello.txt\"; filename*=UTF-8''hello.txt", response.headers["Content-Disposition"] assert_equal "text/plain", response.headers["Content-Type"] assert_equal " worl", response.body end @@ -36,6 +37,10 @@ class ActiveStorage::DiskControllerTest < ActionDispatch::IntegrationTest blob.delete get blob.service_url + end + + test "showing blob with invalid key" do + get rails_disk_service_url(encoded_key: "Invalid key", filename: "hello.txt") assert_response :not_found end diff --git a/activestorage/test/models/blob_test.rb b/activestorage/test/models/blob_test.rb index 1a6a89de56..1503f5fc50 100644 --- a/activestorage/test/models/blob_test.rb +++ b/activestorage/test/models/blob_test.rb @@ -121,12 +121,21 @@ class ActiveStorage::BlobTest < ActiveSupport::TestCase end end - test "urls force attachment as content disposition for content types served as binary" do + test "urls force content_type to binary and attachment as content disposition for content types served as binary" do blob = create_blob(content_type: "text/html") freeze_time do - assert_equal expected_url_for(blob, disposition: :attachment), blob.service_url - assert_equal expected_url_for(blob, disposition: :attachment), blob.service_url(disposition: :inline) + assert_equal expected_url_for(blob, disposition: :attachment, content_type: "application/octet-stream"), blob.service_url + assert_equal expected_url_for(blob, disposition: :attachment, content_type: "application/octet-stream"), blob.service_url(disposition: :inline) + end + end + + test "urls force attachment as content disposition when the content type is not allowed inline" do + blob = create_blob(content_type: "application/zip") + + freeze_time do + assert_equal expected_url_for(blob, disposition: :attachment, content_type: "application/zip"), blob.service_url + assert_equal expected_url_for(blob, disposition: :attachment, content_type: "application/zip"), blob.service_url(disposition: :inline) end end @@ -148,7 +157,7 @@ class ActiveStorage::BlobTest < ActiveSupport::TestCase arguments = [ blob.key, expires_in: ActiveStorage.service_urls_expire_in, - disposition: :inline, + disposition: :attachment, content_type: blob.content_type, filename: blob.filename, thumb_size: "300x300", @@ -183,9 +192,13 @@ class ActiveStorage::BlobTest < ActiveSupport::TestCase end private - def expected_url_for(blob, disposition: :inline, filename: nil) + def expected_url_for(blob, disposition: :attachment, filename: nil, content_type: nil) filename ||= blob.filename - query_string = { content_type: blob.content_type, disposition: ActionDispatch::Http::ContentDisposition.format(disposition: disposition, filename: filename.sanitized) }.to_param - "https://example.com/rails/active_storage/disk/#{ActiveStorage.verifier.generate(blob.key, expires_in: 5.minutes, purpose: :blob_key)}/#{filename}?#{query_string}" + content_type ||= blob.content_type + + query = { disposition: ActionDispatch::Http::ContentDisposition.format(disposition: disposition, filename: filename.sanitized), content_type: content_type } + key_params = { key: blob.key }.merge(query) + + "https://example.com/rails/active_storage/disk/#{ActiveStorage.verifier.generate(key_params, expires_in: 5.minutes, purpose: :blob_key)}/#{filename}?#{query.to_param}" end end diff --git a/activestorage/test/models/variant_test.rb b/activestorage/test/models/variant_test.rb index 6577f1cd9f..8552080e7b 100644 --- a/activestorage/test/models/variant_test.rb +++ b/activestorage/test/models/variant_test.rb @@ -156,7 +156,7 @@ class ActiveStorage::VariantTest < ActiveSupport::TestCase test "service_url doesn't grow in length despite long variant options" do blob = create_file_blob(filename: "racecar.jpg") variant = blob.variant(font: "a" * 10_000).processed - assert_operator variant.service_url.length, :<, 525 + assert_operator variant.service_url.length, :<, 726 end test "works for vips processor" do diff --git a/activestorage/test/service/gcs_service_test.rb b/activestorage/test/service/gcs_service_test.rb index 2ba2f8b346..af27946357 100644 --- a/activestorage/test/service/gcs_service_test.rb +++ b/activestorage/test/service/gcs_service_test.rb @@ -31,24 +31,59 @@ if SERVICE_CONFIGURATIONS[:gcs] end end - test "signed URL generation" do - assert_match(/storage\.googleapis\.com\/.*response-content-disposition=inline.*test\.txt.*response-content-type=text%2Fplain/, - @service.url(@key, expires_in: 2.minutes, disposition: :inline, filename: ActiveStorage::Filename.new("test.txt"), content_type: "text/plain")) + test "upload with content_type and content_disposition" do + begin + key = SecureRandom.base58(24) + data = "Something else entirely!" + + @service.upload(key, StringIO.new(data), checksum: Digest::MD5.base64digest(data), disposition: :attachment, filename: ActiveStorage::Filename.new("test.txt"), content_type: "text/plain") + + url = @service.url(key, expires_in: 2.minutes, disposition: :inline, content_type: "text/html", filename: ActiveStorage::Filename.new("test.html")) + response = Net::HTTP.get_response(URI(url)) + assert_equal "text/plain", response.content_type + assert_match(/attachment;.*test.txt/, response["Content-Disposition"]) + ensure + @service.delete key + end end - test "signed URL response headers" do + test "upload with content_type" do begin - key = SecureRandom.base58(24) - data = "Something else entirely!" - @service.upload(key, StringIO.new(data), checksum: Digest::MD5.base64digest(data)) + key = SecureRandom.base58(24) + data = "Something else entirely!" - url = @service.url(key, expires_in: 2.minutes, disposition: :inline, filename: ActiveStorage::Filename.new("test.txt"), content_type: "text/plain") + @service.upload(key, StringIO.new(data), checksum: Digest::MD5.base64digest(data), content_type: "text/plain") + + url = @service.url(key, expires_in: 2.minutes, disposition: :inline, content_type: "text/html", filename: ActiveStorage::Filename.new("test.html")) response = Net::HTTP.get_response(URI(url)) assert_equal "text/plain", response.content_type + assert_match(/inline;.*test.html/, response["Content-Disposition"]) ensure @service.delete key end end + + test "update metadata" do + begin + key = SecureRandom.base58(24) + data = "Something else entirely!" + @service.upload(key, StringIO.new(data), checksum: Digest::MD5.base64digest(data), disposition: :attachment, filename: ActiveStorage::Filename.new("test.html"), content_type: "text/html") + + @service.update_metadata(key, disposition: :inline, filename: ActiveStorage::Filename.new("test.txt"), content_type: "text/plain") + url = @service.url(key, expires_in: 2.minutes, disposition: :attachment, content_type: "text/html", filename: ActiveStorage::Filename.new("test.html")) + + response = Net::HTTP.get_response(URI(url)) + assert_equal "text/plain", response.content_type + assert_match(/inline;.*test.txt/, response["Content-Disposition"]) + ensure + @service.delete key + end + end + + test "signed URL generation" do + assert_match(/storage\.googleapis\.com\/.*response-content-disposition=inline.*test\.txt.*response-content-type=text%2Fplain/, + @service.url(@key, expires_in: 2.minutes, disposition: :inline, filename: ActiveStorage::Filename.new("test.txt"), content_type: "text/plain")) + end end else puts "Skipping GCS Service tests because no GCS configuration was supplied" diff --git a/activesupport/CHANGELOG.md b/activesupport/CHANGELOG.md index 98d232da33..ba8aaf47f9 100644 --- a/activesupport/CHANGELOG.md +++ b/activesupport/CHANGELOG.md @@ -1,3 +1,7 @@ +* If the same block is `included` multiple times for a Concern, an exception is no longer raised. + + *Mark J. Titorenko*, *Vlad Bokov* + * Fix bug where `#to_options` for `ActiveSupport::HashWithIndifferentAccess` would not act as alias for `#symbolize_keys`. diff --git a/activesupport/lib/active_support/concern.rb b/activesupport/lib/active_support/concern.rb index b0a0d845e5..5d356a0ab6 100644 --- a/activesupport/lib/active_support/concern.rb +++ b/activesupport/lib/active_support/concern.rb @@ -125,9 +125,13 @@ module ActiveSupport def included(base = nil, &block) if base.nil? - raise MultipleIncludedBlocks if instance_variable_defined?(:@_included_block) - - @_included_block = block + if instance_variable_defined?(:@_included_block) + if @_included_block.source_location != block.source_location + raise MultipleIncludedBlocks + end + else + @_included_block = block + end else super end diff --git a/activesupport/lib/active_support/notifications/instrumenter.rb b/activesupport/lib/active_support/notifications/instrumenter.rb index a721187d2d..125c06f37a 100644 --- a/activesupport/lib/active_support/notifications/instrumenter.rb +++ b/activesupport/lib/active_support/notifications/instrumenter.rb @@ -56,8 +56,9 @@ module ActiveSupport def self.clock_gettime_supported? # :nodoc: defined?(Process::CLOCK_PROCESS_CPUTIME_ID) && - !Gem.win_platform? + !Gem.win_platform? end + private_class_method :clock_gettime_supported? def initialize(name, start, ending, transaction_id, payload) @name = name diff --git a/activesupport/lib/active_support/testing/parallelization.rb b/activesupport/lib/active_support/testing/parallelization.rb index 8de01eb19b..c5d3a88131 100644 --- a/activesupport/lib/active_support/testing/parallelization.rb +++ b/activesupport/lib/active_support/testing/parallelization.rb @@ -23,6 +23,7 @@ module ActiveSupport end def <<(o) + o[2] = DRbObject.new(o[2]) if o @queue << o end diff --git a/activesupport/test/concern_test.rb b/activesupport/test/concern_test.rb index 98d8f3ee0d..4b3cfcd1d2 100644 --- a/activesupport/test/concern_test.rb +++ b/activesupport/test/concern_test.rb @@ -128,4 +128,12 @@ class ConcernTest < ActiveSupport::TestCase end end end + + def test_no_raise_on_same_included_call + assert_nothing_raised do + 2.times do + load File.expand_path("../fixtures/concern/some_concern.rb", __FILE__) + end + end + end end diff --git a/activesupport/test/fixtures/concern/some_concern.rb b/activesupport/test/fixtures/concern/some_concern.rb new file mode 100644 index 0000000000..87f660a81e --- /dev/null +++ b/activesupport/test/fixtures/concern/some_concern.rb @@ -0,0 +1,11 @@ +# frozen_string_literal: true + +require "active_support/concern" + +module SomeConcern + extend ActiveSupport::Concern + + included do + # shouldn't raise when module is loaded more than once + end +end diff --git a/guides/source/active_job_basics.md b/guides/source/active_job_basics.md index 39239852ca..0ebef46373 100644 --- a/guides/source/active_job_basics.md +++ b/guides/source/active_job_basics.md @@ -165,6 +165,7 @@ Here is a noncomprehensive list of documentation: - [Sneakers](https://github.com/jondot/sneakers/wiki/How-To:-Rails-Background-Jobs-with-ActiveJob) - [Sucker Punch](https://github.com/brandonhilkert/sucker_punch#active-job) - [Queue Classic](https://github.com/QueueClassic/queue_classic#active-job) +- [Delayed Job](https://github.com/collectiveidea/delayed_job#active-job) Queues ------ diff --git a/guides/source/active_record_querying.md b/guides/source/active_record_querying.md index 02055e59f0..fd1dcf22c0 100644 --- a/guides/source/active_record_querying.md +++ b/guides/source/active_record_querying.md @@ -1267,7 +1267,7 @@ This is because it is ambiguous whether they should appear on the parent record, Scopes ------ -Scoping allows you to specify commonly-used queries which can be referenced as method calls on the association objects or models. With these scopes, you can use every method previously covered such as `where`, `joins` and `includes`. All scope methods will return an `ActiveRecord::Relation` object which will allow for further methods (such as other scopes) to be called on it. +Scoping allows you to specify commonly-used queries which can be referenced as method calls on the association objects or models. With these scopes, you can use every method previously covered such as `where`, `joins` and `includes`. All scope bodies should return an `ActiveRecord::Relation` or `nil` to allow for further methods (such as other scopes) to be called on it. To define a simple scope, we use the `scope` method inside the class, passing the query that we'd like to run when this scope is called: |