diff options
82 files changed, 1043 insertions, 128 deletions
@@ -44,7 +44,7 @@ gem "libxml-ruby", platforms: :ruby gem "connection_pool", require: false # for railties app_generator_test -gem "bootsnap", ">= 1.4.0", require: false +gem "bootsnap", ">= 1.4.2", require: false # Active Job group :job do @@ -92,7 +92,7 @@ gem "webmock" group :ujs do gem "qunit-selenium" - gem "chromedriver-helper" + gem "webdrivers" end # Add your own local bundler stuff. diff --git a/Gemfile.lock b/Gemfile.lock index ed10cae358..62cab7261b 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -70,7 +70,7 @@ PATH i18n (>= 0.7, < 2) minitest (~> 5.1) tzinfo (~> 1.1) - zeitwerk (~> 1.4) + zeitwerk (~> 1.4, >= 1.4.3) rails (6.0.0.beta3) actioncable (= 6.0.0.beta3) actionmailbox (= 6.0.0.beta3) @@ -110,8 +110,6 @@ GEM addressable (2.5.2) public_suffix (>= 2.0.2, < 4.0) amq-protocol (2.3.0) - archive-zip (0.11.0) - io-like (~> 0.3.0) ast (2.4.0) aws-eventstream (1.0.1) aws-partitions (1.111.0) @@ -167,9 +165,9 @@ GEM childprocess faraday selenium-webdriver - bootsnap (1.4.0) + bootsnap (1.4.2) msgpack (~> 1.0) - bootsnap (1.4.0-java) + bootsnap (1.4.2-java) msgpack (~> 1.0) builder (3.2.3) bunny (2.13.0) @@ -185,9 +183,6 @@ GEM xpath (~> 3.2) childprocess (0.9.0) ffi (~> 1.0, >= 1.0.11) - chromedriver-helper (2.1.0) - archive-zip (~> 0.10) - nokogiri (~> 1.8) coffee-script (2.4.1) coffee-script-source execjs @@ -282,7 +277,6 @@ GEM image_processing (1.7.1) mini_magick (~> 4.0) ruby-vips (>= 2.0.13, < 3) - io-like (0.3.0) jaro_winkler (1.5.2) jaro_winkler (1.5.2-java) jdbc-mysql (5.1.46) @@ -325,10 +319,10 @@ GEM minitest-server (1.0.5) minitest (~> 5.0) mono_logger (1.1.0) - msgpack (1.2.6) - msgpack (1.2.6-java) - msgpack (1.2.6-x64-mingw32) - msgpack (1.2.6-x86-mingw32) + msgpack (1.2.9) + msgpack (1.2.9-java) + msgpack (1.2.9-x64-mingw32) + msgpack (1.2.9-x86-mingw32) multi_json (1.13.1) multipart-post (2.0.0) mustache (1.1.0) @@ -336,6 +330,7 @@ GEM mysql2 (0.5.2) mysql2 (0.5.2-x64-mingw32) mysql2 (0.5.2-x86-mingw32) + net_http_ssl_fix (0.0.10) nio4r (2.3.1) nio4r (2.3.1-java) nokogiri (1.9.1) @@ -504,6 +499,11 @@ GEM json (>= 1.8) nokogiri (~> 1.6) wdm (0.1.1) + webdrivers (3.7.0) + net_http_ssl_fix + nokogiri (~> 1.6) + rubyzip (~> 1.0) + selenium-webdriver (~> 3.0) webmock (3.4.2) addressable (>= 2.3.6) crack (>= 0.3.2) @@ -520,7 +520,7 @@ GEM websocket-extensions (0.1.3) xpath (3.2.0) nokogiri (~> 1.8) - zeitwerk (1.4.0) + zeitwerk (1.4.3) PLATFORMS java @@ -540,10 +540,9 @@ DEPENDENCIES benchmark-ips blade blade-sauce_labs_plugin - bootsnap (>= 1.4.0) + bootsnap (>= 1.4.2) byebug capybara (>= 2.15) - chromedriver-helper connection_pool dalli delayed_job @@ -590,6 +589,7 @@ DEPENDENCIES uglifier (>= 1.3.0) w3c_validators wdm (>= 0.1.0) + webdrivers webmock webpacker (~> 4.0) websocket-client-simple! diff --git a/actionpack/lib/action_dispatch/middleware/debug_exceptions.rb b/actionpack/lib/action_dispatch/middleware/debug_exceptions.rb index 61773d97a2..bb49bc4dda 100644 --- a/actionpack/lib/action_dispatch/middleware/debug_exceptions.rb +++ b/actionpack/lib/action_dispatch/middleware/debug_exceptions.rb @@ -60,7 +60,11 @@ module ActionDispatch log_error(request, wrapper) if request.get_header("action_dispatch.show_detailed_exceptions") - content_type = request.formats.first + begin + content_type = request.formats.first + rescue Mime::Type::InvalidMimeType + render_for_api_request(Mime[:text], wrapper) + end if api_request?(content_type) render_for_api_request(content_type, wrapper) diff --git a/actionpack/lib/action_dispatch/middleware/exception_wrapper.rb b/actionpack/lib/action_dispatch/middleware/exception_wrapper.rb index 1fb3e9db00..0cc56f5013 100644 --- a/actionpack/lib/action_dispatch/middleware/exception_wrapper.rb +++ b/actionpack/lib/action_dispatch/middleware/exception_wrapper.rb @@ -12,6 +12,7 @@ module ActionDispatch "ActionController::UnknownHttpMethod" => :method_not_allowed, "ActionController::NotImplemented" => :not_implemented, "ActionController::UnknownFormat" => :not_acceptable, + "Mime::Type::InvalidMimeType" => :not_acceptable, "ActionController::MissingExactTemplate" => :not_acceptable, "ActionController::InvalidAuthenticityToken" => :unprocessable_entity, "ActionController::InvalidCrossOriginRequest" => :unprocessable_entity, diff --git a/actionpack/lib/action_dispatch/middleware/public_exceptions.rb b/actionpack/lib/action_dispatch/middleware/public_exceptions.rb index 3feb3a19f3..a88ad40f21 100644 --- a/actionpack/lib/action_dispatch/middleware/public_exceptions.rb +++ b/actionpack/lib/action_dispatch/middleware/public_exceptions.rb @@ -21,8 +21,12 @@ module ActionDispatch def call(env) request = ActionDispatch::Request.new(env) status = request.path_info[1..-1].to_i - content_type = request.formats.first - body = { status: status, error: Rack::Utils::HTTP_STATUS_CODES.fetch(status, Rack::Utils::HTTP_STATUS_CODES[500]) } + begin + content_type = request.formats.first + rescue Mime::Type::InvalidMimeType + content_type = Mime[:text] + end + body = { status: status, error: Rack::Utils::HTTP_STATUS_CODES.fetch(status, Rack::Utils::HTTP_STATUS_CODES[500]) } render(status, content_type, body) end diff --git a/actionpack/test/dispatch/debug_exceptions_test.rb b/actionpack/test/dispatch/debug_exceptions_test.rb index c85476fa38..8b1b3c0277 100644 --- a/actionpack/test/dispatch/debug_exceptions_test.rb +++ b/actionpack/test/dispatch/debug_exceptions_test.rb @@ -58,6 +58,8 @@ class DebugExceptionsTest < ActionDispatch::IntegrationTest raise ActionController::NotImplemented when "/unprocessable_entity" raise ActionController::InvalidAuthenticityToken + when "/invalid_mimetype" + raise Mime::Type::InvalidMimeType when "/not_found_original_exception" begin raise AbstractController::ActionNotFound.new @@ -178,6 +180,10 @@ class DebugExceptionsTest < ActionDispatch::IntegrationTest get "/parameter_missing", headers: { "action_dispatch.show_exceptions" => true } assert_response 400 assert_match(/ActionController::ParameterMissing/, body) + + get "/invalid_mimetype", headers: { "Accept" => "text/html,*", "action_dispatch.show_exceptions" => true } + assert_response 406 + assert_match(/Mime::Type::InvalidMimeType/, body) end test "rescue with text error for xhr request" do diff --git a/actionpack/test/dispatch/show_exceptions_test.rb b/actionpack/test/dispatch/show_exceptions_test.rb index f802abc653..6fafa4e426 100644 --- a/actionpack/test/dispatch/show_exceptions_test.rb +++ b/actionpack/test/dispatch/show_exceptions_test.rb @@ -9,6 +9,8 @@ class ShowExceptionsTest < ActionDispatch::IntegrationTest case req.path when "/not_found" raise AbstractController::ActionNotFound + when "/invalid_mimetype" + raise Mime::Type::InvalidMimeType when "/bad_params", "/bad_params.json" begin raise StandardError.new @@ -62,6 +64,10 @@ class ShowExceptionsTest < ActionDispatch::IntegrationTest get "/unknown_http_method", env: { "action_dispatch.show_exceptions" => true } assert_response 405 assert_equal "", body + + get "/invalid_mimetype", headers: { "Accept" => "text/html,*", "action_dispatch.show_exceptions" => true } + assert_response 406 + assert_equal "", body end test "localize rescue error page" do diff --git a/actiontext/lib/action_text/attribute.rb b/actiontext/lib/action_text/attribute.rb index f9a604096c..ddc6822a4c 100644 --- a/actiontext/lib/action_text/attribute.rb +++ b/actiontext/lib/action_text/attribute.rb @@ -26,7 +26,7 @@ module ActionText def has_rich_text(name) class_eval <<-CODE, __FILE__, __LINE__ + 1 def #{name} - self.rich_text_#{name} ||= ActionText::RichText.new(name: "#{name}", record: self) + rich_text_#{name} || build_rich_text_#{name} end def #{name}=(body) diff --git a/actiontext/test/unit/model_test.rb b/actiontext/test/unit/model_test.rb index f4328ba2ce..af53f88caa 100644 --- a/actiontext/test/unit/model_test.rb +++ b/actiontext/test/unit/model_test.rb @@ -67,4 +67,12 @@ class ActionText::ModelTest < ActiveSupport::TestCase message.update! review_attributes: { id: message.review.id, content: "Great work!" } assert_equal "Great work!", message.review.reload.content.to_plain_text end + + test "building content lazily on existing record" do + message = Message.create!(subject: "Greetings") + + assert_no_difference -> { ActionText::RichText.count } do + assert_kind_of ActionText::RichText, message.content + end + end end diff --git a/actionview/CHANGELOG.md b/actionview/CHANGELOG.md index d07794ddf3..43688fc8a7 100644 --- a/actionview/CHANGELOG.md +++ b/actionview/CHANGELOG.md @@ -1,6 +1,15 @@ ## Rails 6.0.0.beta3 (March 11, 2019) ## -* No changes. +* Only accept formats from registered mime types + + A lack of filtering on mime types could allow an attacker to read + arbitrary files on the target server or to perform a denial of service + attack. + + Fixes CVE-2019-5418 + Fixes CVE-2019-5419 + + *John Hawthorn*, *Eileen M. Uchitelle*, *Aaron Patterson* ## Rails 6.0.0.beta2 (February 25, 2019) ## diff --git a/actionview/lib/action_view/template/resolver.rb b/actionview/lib/action_view/template/resolver.rb index 1c577348e5..3b4c8a94bc 100644 --- a/actionview/lib/action_view/template/resolver.rb +++ b/actionview/lib/action_view/template/resolver.rb @@ -168,7 +168,12 @@ module ActionView DEFAULT_PATTERN = ":prefix/:action{.:locale,}{.:formats,}{+:variants,}{.:handlers,}" def initialize(pattern = nil) - @pattern = pattern || DEFAULT_PATTERN + if pattern + ActiveSupport::Deprecation.warn "Specifying a custom path for #{self.class} is deprecated. Implement a custom Resolver subclass instead." + @pattern = pattern + else + @pattern = DEFAULT_PATTERN + end super() end @@ -273,44 +278,7 @@ module ActionView end end - # A resolver that loads files from the filesystem. It allows setting your own - # resolving pattern. Such pattern can be a glob string supported by some variables. - # - # ==== Examples - # - # Default pattern, loads views the same way as previous versions of rails, eg. when you're - # looking for <tt>users/new</tt> it will produce query glob: <tt>users/new{.{en},}{.{html,js},}{.{erb,haml},}</tt> - # - # FileSystemResolver.new("/path/to/views", ":prefix/:action{.:locale,}{.:formats,}{+:variants,}{.:handlers,}") - # - # This one allows you to keep files with different formats in separate subdirectories, - # eg. <tt>users/new.html</tt> will be loaded from <tt>users/html/new.erb</tt> or <tt>users/new.html.erb</tt>, - # <tt>users/new.js</tt> from <tt>users/js/new.erb</tt> or <tt>users/new.js.erb</tt>, etc. - # - # FileSystemResolver.new("/path/to/views", ":prefix/{:formats/,}:action{.:locale,}{.:formats,}{+:variants,}{.:handlers,}") - # - # If you don't specify a pattern then the default will be used. - # - # In order to use any of the customized resolvers above in a Rails application, you just need - # to configure ActionController::Base.view_paths in an initializer, for example: - # - # ActionController::Base.view_paths = FileSystemResolver.new( - # Rails.root.join("app/views"), - # ":prefix/:action{.:locale,}{.:formats,}{+:variants,}{.:handlers,}", - # ) - # - # ==== Pattern format and variables - # - # Pattern has to be a valid glob string, and it allows you to use the - # following variables: - # - # * <tt>:prefix</tt> - usually the controller path - # * <tt>:action</tt> - name of the action - # * <tt>:locale</tt> - possible locale versions - # * <tt>:formats</tt> - possible request formats (for example html, json, xml...) - # * <tt>:variants</tt> - possible request variants (for example phone, tablet...) - # * <tt>:handlers</tt> - possible handlers (for example erb, haml, builder...) - # + # A resolver that loads files from the filesystem. class FileSystemResolver < PathResolver def initialize(path, pattern = nil) raise ArgumentError, "path already is a Resolver class" if path.is_a?(Resolver) @@ -331,6 +299,10 @@ module ActionView # An Optimized resolver for Rails' most common case. class OptimizedFileSystemResolver < FileSystemResolver #:nodoc: + def initialize(path) + super(path) + end + private def find_template_paths_from_details(path, details) diff --git a/actionview/test/template/resolver_patterns_test.rb b/actionview/test/template/resolver_patterns_test.rb index 8122de779f..22815c8dbe 100644 --- a/actionview/test/template/resolver_patterns_test.rb +++ b/actionview/test/template/resolver_patterns_test.rb @@ -6,7 +6,10 @@ class ResolverPatternsTest < ActiveSupport::TestCase def setup path = File.expand_path("../fixtures", __dir__) pattern = ":prefix/{:formats/,}:action{.:formats,}{+:variants,}{.:handlers,}" - @resolver = ActionView::FileSystemResolver.new(path, pattern) + + assert_deprecated do + @resolver = ActionView::FileSystemResolver.new(path, pattern) + end end def test_should_return_empty_list_for_unknown_path diff --git a/activejob/CHANGELOG.md b/activejob/CHANGELOG.md index de375baa9c..e09ba4b055 100644 --- a/activejob/CHANGELOG.md +++ b/activejob/CHANGELOG.md @@ -1,3 +1,8 @@ +* Make job argument assertions with `Time`, `ActiveSupport::TimeWithZone`, and `DateTime` work by dropping microseconds. Microsecond precision is lost during serialization. + + *Gannon McGibbon* + + ## Rails 6.0.0.beta3 (March 11, 2019) ## * No changes. diff --git a/activejob/lib/active_job/test_helper.rb b/activejob/lib/active_job/test_helper.rb index f03780b91e..e5e2b086bc 100644 --- a/activejob/lib/active_job/test_helper.rb +++ b/activejob/lib/active_job/test_helper.rb @@ -631,6 +631,20 @@ module ActiveJob def prepare_args_for_assertion(args) args.dup.tap do |arguments| arguments[:at] = arguments[:at].to_f if arguments[:at] + arguments[:args] = round_time_arguments(arguments[:args]) if arguments[:args] + end + end + + def round_time_arguments(argument) + case argument + when Time, ActiveSupport::TimeWithZone, DateTime + argument.change(usec: 0) + when Hash + argument.transform_values { |value| round_time_arguments(value) } + when Array + argument.map { |element| round_time_arguments(element) } + else + argument end end diff --git a/activejob/test/cases/test_helper_test.rb b/activejob/test/cases/test_helper_test.rb index 4d934df31b..d6607cb6b6 100644 --- a/activejob/test/cases/test_helper_test.rb +++ b/activejob/test/cases/test_helper_test.rb @@ -581,6 +581,33 @@ class EnqueuedJobsTest < ActiveJob::TestCase end end + def test_assert_enqueued_with_time + now = Time.now + args = [{ argument1: [now] }] + + assert_enqueued_with(job: MultipleKwargsJob, args: args) do + MultipleKwargsJob.perform_later(argument1: [now]) + end + end + + def test_assert_enqueued_with_date_time + now = DateTime.now + args = [{ argument1: [now] }] + + assert_enqueued_with(job: MultipleKwargsJob, args: args) do + MultipleKwargsJob.perform_later(argument1: [now]) + end + end + + def test_assert_enqueued_with_time_with_zone + now = Time.now.in_time_zone("Tokyo") + args = [{ argument1: [now] }] + + assert_enqueued_with(job: MultipleKwargsJob, args: args) do + MultipleKwargsJob.perform_later(argument1: [now]) + end + end + def test_assert_enqueued_with_with_no_block_args assert_raise ArgumentError do NestedJob.set(wait_until: Date.tomorrow.noon).perform_later @@ -1681,6 +1708,33 @@ class PerformedJobsTest < ActiveJob::TestCase end end + def test_assert_performed_with_time + now = Time.now + args = [{ argument1: { now: now } }] + + assert_enqueued_with(job: MultipleKwargsJob, args: args) do + MultipleKwargsJob.perform_later(argument1: { now: now }) + end + end + + def test_assert_performed_with_date_time + now = DateTime.now + args = [{ argument1: { now: now } }] + + assert_enqueued_with(job: MultipleKwargsJob, args: args) do + MultipleKwargsJob.perform_later(argument1: { now: now }) + end + end + + def test_assert_performed_with_time_with_zone + now = Time.now.in_time_zone("Tokyo") + args = [{ argument1: { now: now } }] + + assert_enqueued_with(job: MultipleKwargsJob, args: args) do + MultipleKwargsJob.perform_later(argument1: { now: now }) + end + end + def test_assert_performed_with_with_global_id_args ricardo = Person.new(9) assert_performed_with(job: HelloJob, args: [ricardo]) do diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md index 148b3800a8..df781b4f1e 100644 --- a/activerecord/CHANGELOG.md +++ b/activerecord/CHANGELOG.md @@ -1,6 +1,19 @@ +* Add `ActiveRecord::Relation#annotate` for adding SQL comments to its queries. + + For example: + + ``` + Post.where(id: 123).annotate("this is a comment").to_sql + # SELECT "posts".* FROM "posts" WHERE "posts"."id" = 123 /* this is a comment */ + ``` + + This can be useful in instrumentation or other analysis of issued queries. + + *Matt Yoho* + * Support Optimizer Hints. - In most databases, there is a way to control the optimizer is by using optimizer hints, + In most databases, a way to control the optimizer is by using optimizer hints, which can be specified within individual statements. Example (for MySQL): diff --git a/activerecord/lib/active_record/associations/preloader/through_association.rb b/activerecord/lib/active_record/associations/preloader/through_association.rb index 32653956b2..db89b77629 100644 --- a/activerecord/lib/active_record/associations/preloader/through_association.rb +++ b/activerecord/lib/active_record/associations/preloader/through_association.rb @@ -61,11 +61,15 @@ module ActiveRecord scope = through_reflection.klass.unscoped options = reflection.options + values = reflection_scope.values + if annotations = values[:annotate] + scope.annotate!(*annotations) + end + if options[:source_type] scope.where! reflection.foreign_type => options[:source_type] elsif !reflection_scope.where_clause.empty? scope.where_clause = reflection_scope.where_clause - values = reflection_scope.values if includes = values[:includes] scope.includes!(source_reflection.name => includes) diff --git a/activerecord/lib/active_record/autosave_association.rb b/activerecord/lib/active_record/autosave_association.rb index d77d76cb1e..fe94662543 100644 --- a/activerecord/lib/active_record/autosave_association.rb +++ b/activerecord/lib/active_record/autosave_association.rb @@ -457,10 +457,16 @@ module ActiveRecord # If the record is new or it has changed, returns true. def record_changed?(reflection, record, key) record.new_record? || - (record.has_attribute?(reflection.foreign_key) && record[reflection.foreign_key] != key) || + association_foreign_key_changed?(reflection, record, key) || record.will_save_change_to_attribute?(reflection.foreign_key) end + def association_foreign_key_changed?(reflection, record, key) + return false if reflection.through_reflection? + + record.has_attribute?(reflection.foreign_key) && record[reflection.foreign_key] != key + end + # Saves the associated record if it's new or <tt>:autosave</tt> is enabled. # # In addition, it will destroy the association if it was marked for destruction. diff --git a/activerecord/lib/active_record/persistence.rb b/activerecord/lib/active_record/persistence.rb index ba03a3773a..7705cefa59 100644 --- a/activerecord/lib/active_record/persistence.rb +++ b/activerecord/lib/active_record/persistence.rb @@ -88,7 +88,7 @@ module ActiveRecord # (Postgres-only) An array of attributes to return for all successfully # inserted records, which by default is the primary key. # Pass <tt>returning: %w[ id name ]</tt> for both id and name - # or <tt>returning: false</tt> to omit the underlying RETURNING SQL + # or <tt>returning: false</tt> to omit the underlying <tt>RETURNING</tt> SQL # clause entirely. # # [:unique_by] @@ -157,7 +157,7 @@ module ActiveRecord # (Postgres-only) An array of attributes to return for all successfully # inserted records, which by default is the primary key. # Pass <tt>returning: %w[ id name ]</tt> for both id and name - # or <tt>returning: false</tt> to omit the underlying RETURNING SQL + # or <tt>returning: false</tt> to omit the underlying <tt>RETURNING</tt> SQL # clause entirely. # # ==== Examples @@ -205,7 +205,7 @@ module ActiveRecord # (Postgres-only) An array of attributes to return for all successfully # inserted records, which by default is the primary key. # Pass <tt>returning: %w[ id name ]</tt> for both id and name - # or <tt>returning: false</tt> to omit the underlying RETURNING SQL + # or <tt>returning: false</tt> to omit the underlying <tt>RETURNING</tt> SQL # clause entirely. # # [:unique_by] diff --git a/activerecord/lib/active_record/querying.rb b/activerecord/lib/active_record/querying.rb index 86021f0a80..81ab502824 100644 --- a/activerecord/lib/active_record/querying.rb +++ b/activerecord/lib/active_record/querying.rb @@ -15,7 +15,7 @@ module ActiveRecord :select, :reselect, :order, :reorder, :group, :limit, :offset, :joins, :left_joins, :left_outer_joins, :where, :rewhere, :preload, :eager_load, :includes, :from, :lock, :readonly, :extending, :or, :having, :create_with, :distinct, :references, :none, :unscope, :optimizer_hints, :merge, :except, :only, - :count, :average, :minimum, :maximum, :sum, :calculate, + :count, :average, :minimum, :maximum, :sum, :calculate, :annotate, :pluck, :pick, :ids ].freeze # :nodoc: delegate(*QUERYING_METHODS, to: :all) diff --git a/activerecord/lib/active_record/relation.rb b/activerecord/lib/active_record/relation.rb index 6d954a2b2e..36c2422d84 100644 --- a/activerecord/lib/active_record/relation.rb +++ b/activerecord/lib/active_record/relation.rb @@ -5,7 +5,7 @@ module ActiveRecord class Relation MULTI_VALUE_METHODS = [:includes, :eager_load, :preload, :select, :group, :order, :joins, :left_outer_joins, :references, - :extending, :unscope, :optimizer_hints] + :extending, :unscope, :optimizer_hints, :annotate] SINGLE_VALUE_METHODS = [:limit, :offset, :lock, :readonly, :reordering, :reverse_order, :distinct, :create_with, :skip_query_cache] @@ -389,6 +389,8 @@ module ActiveRecord stmt.set Arel.sql(klass.sanitize_sql_for_assignment(updates, table.name)) end + stmt.comment(*arel.comment_node.values) if arel.comment_node + @klass.connection.update stmt, "#{@klass} Update All" end @@ -504,6 +506,7 @@ module ActiveRecord stmt.offset(arel.offset) stmt.order(*arel.orders) stmt.wheres = arel.constraints + stmt.comment(*arel.comment_node.values) if arel.comment_node affected = @klass.connection.delete(stmt, "#{@klass} Destroy") diff --git a/activerecord/lib/active_record/relation/query_methods.rb b/activerecord/lib/active_record/relation/query_methods.rb index 05ea0850d3..6f0f2125dc 100644 --- a/activerecord/lib/active_record/relation/query_methods.rb +++ b/activerecord/lib/active_record/relation/query_methods.rb @@ -349,7 +349,7 @@ module ActiveRecord end VALID_UNSCOPING_VALUES = Set.new([:where, :select, :group, :order, :lock, - :limit, :offset, :joins, :left_outer_joins, + :limit, :offset, :joins, :left_outer_joins, :annotate, :includes, :from, :readonly, :having, :optimizer_hints]) # Removes an unwanted relation that is already defined on a chain of relations. @@ -948,6 +948,26 @@ module ActiveRecord self end + # Adds an SQL comment to queries generated from this relation. For example: + # + # User.annotate("selecting user names").select(:name) + # # SELECT "users"."name" FROM "users" /* selecting user names */ + # + # User.annotate("selecting", "user", "names").select(:name) + # # SELECT "users"."name" FROM "users" /* selecting */ /* user */ /* names */ + # + # The SQL block comment delimiters, "/*" and "*/", will be added automatically. + def annotate(*args) + check_if_method_has_arguments!(:annotate, args) + spawn.annotate!(*args) + end + + # Like #annotate, but modifies relation in place. + def annotate!(*args) # :nodoc: + self.annotate_values += args + self + end + # Returns the Arel object associated with the relation. def arel(aliases = nil) # :nodoc: @arel ||= build_arel(aliases) @@ -1004,6 +1024,7 @@ module ActiveRecord arel.distinct(distinct_value) arel.from(build_from) unless from_clause.empty? arel.lock(lock_value) if lock_value + arel.comment(*annotate_values) unless annotate_values.empty? arel end diff --git a/activerecord/lib/arel/nodes.rb b/activerecord/lib/arel/nodes.rb index 2f6dd9bc45..f994754620 100644 --- a/activerecord/lib/arel/nodes.rb +++ b/activerecord/lib/arel/nodes.rb @@ -61,6 +61,8 @@ require "arel/nodes/outer_join" require "arel/nodes/right_outer_join" require "arel/nodes/string_join" +require "arel/nodes/comment" + require "arel/nodes/sql_literal" require "arel/nodes/casted" diff --git a/activerecord/lib/arel/nodes/comment.rb b/activerecord/lib/arel/nodes/comment.rb new file mode 100644 index 0000000000..237ff27e7e --- /dev/null +++ b/activerecord/lib/arel/nodes/comment.rb @@ -0,0 +1,29 @@ +# frozen_string_literal: true + +module Arel # :nodoc: all + module Nodes + class Comment < Arel::Nodes::Node + attr_reader :values + + def initialize(values) + super() + @values = values + end + + def initialize_copy(other) + super + @values = @values.clone + end + + def hash + [@values].hash + end + + def eql?(other) + self.class == other.class && + self.values == other.values + end + alias :== :eql? + end + end +end diff --git a/activerecord/lib/arel/nodes/delete_statement.rb b/activerecord/lib/arel/nodes/delete_statement.rb index a419975335..56249b2bad 100644 --- a/activerecord/lib/arel/nodes/delete_statement.rb +++ b/activerecord/lib/arel/nodes/delete_statement.rb @@ -3,7 +3,7 @@ module Arel # :nodoc: all module Nodes class DeleteStatement < Arel::Nodes::Node - attr_accessor :left, :right, :orders, :limit, :offset, :key + attr_accessor :left, :right, :orders, :limit, :offset, :key, :comment alias :relation :left alias :relation= :left= @@ -18,16 +18,18 @@ module Arel # :nodoc: all @limit = nil @offset = nil @key = nil + @comment = nil end def initialize_copy(other) super @left = @left.clone if @left @right = @right.clone if @right + @comment = @comment.clone if @comment end def hash - [self.class, @left, @right, @orders, @limit, @offset, @key].hash + [self.class, @left, @right, @orders, @limit, @offset, @key, @comment].hash end def eql?(other) @@ -37,7 +39,8 @@ module Arel # :nodoc: all self.orders == other.orders && self.limit == other.limit && self.offset == other.offset && - self.key == other.key + self.key == other.key && + self.comment == other.comment end alias :== :eql? end diff --git a/activerecord/lib/arel/nodes/insert_statement.rb b/activerecord/lib/arel/nodes/insert_statement.rb index d28fd1f6c8..8430dd23da 100644 --- a/activerecord/lib/arel/nodes/insert_statement.rb +++ b/activerecord/lib/arel/nodes/insert_statement.rb @@ -3,7 +3,7 @@ module Arel # :nodoc: all module Nodes class InsertStatement < Arel::Nodes::Node - attr_accessor :relation, :columns, :values, :select + attr_accessor :relation, :columns, :values, :select, :comment def initialize super() @@ -11,6 +11,7 @@ module Arel # :nodoc: all @columns = [] @values = nil @select = nil + @comment = nil end def initialize_copy(other) @@ -18,10 +19,11 @@ module Arel # :nodoc: all @columns = @columns.clone @values = @values.clone if @values @select = @select.clone if @select + @comment = @comment.clone if @comment end def hash - [@relation, @columns, @values, @select].hash + [@relation, @columns, @values, @select, @comment].hash end def eql?(other) @@ -29,7 +31,8 @@ module Arel # :nodoc: all self.relation == other.relation && self.columns == other.columns && self.select == other.select && - self.values == other.values + self.values == other.values && + self.comment == other.comment end alias :== :eql? end diff --git a/activerecord/lib/arel/nodes/select_core.rb b/activerecord/lib/arel/nodes/select_core.rb index 5df6ac8412..b6154b7ff4 100644 --- a/activerecord/lib/arel/nodes/select_core.rb +++ b/activerecord/lib/arel/nodes/select_core.rb @@ -3,7 +3,7 @@ module Arel # :nodoc: all module Nodes class SelectCore < Arel::Nodes::Node - attr_accessor :projections, :wheres, :groups, :windows + attr_accessor :projections, :wheres, :groups, :windows, :comment attr_accessor :havings, :source, :set_quantifier, :optimizer_hints def initialize @@ -18,6 +18,7 @@ module Arel # :nodoc: all @groups = [] @havings = [] @windows = [] + @comment = nil end def from @@ -39,12 +40,13 @@ module Arel # :nodoc: all @groups = @groups.clone @havings = @havings.clone @windows = @windows.clone + @comment = @comment.clone if @comment end def hash [ @source, @set_quantifier, @projections, @optimizer_hints, - @wheres, @groups, @havings, @windows + @wheres, @groups, @havings, @windows, @comment ].hash end @@ -57,7 +59,8 @@ module Arel # :nodoc: all self.wheres == other.wheres && self.groups == other.groups && self.havings == other.havings && - self.windows == other.windows + self.windows == other.windows && + self.comment == other.comment end alias :== :eql? end diff --git a/activerecord/lib/arel/nodes/update_statement.rb b/activerecord/lib/arel/nodes/update_statement.rb index cfaa19e392..015bcd7613 100644 --- a/activerecord/lib/arel/nodes/update_statement.rb +++ b/activerecord/lib/arel/nodes/update_statement.rb @@ -3,7 +3,7 @@ module Arel # :nodoc: all module Nodes class UpdateStatement < Arel::Nodes::Node - attr_accessor :relation, :wheres, :values, :orders, :limit, :offset, :key + attr_accessor :relation, :wheres, :values, :orders, :limit, :offset, :key, :comment def initialize @relation = nil @@ -13,16 +13,18 @@ module Arel # :nodoc: all @limit = nil @offset = nil @key = nil + @comment = nil end def initialize_copy(other) super @wheres = @wheres.clone @values = @values.clone + @comment = @comment.clone if @comment end def hash - [@relation, @wheres, @values, @orders, @limit, @offset, @key].hash + [@relation, @wheres, @values, @orders, @limit, @offset, @key, @comment].hash end def eql?(other) @@ -33,7 +35,8 @@ module Arel # :nodoc: all self.orders == other.orders && self.limit == other.limit && self.offset == other.offset && - self.key == other.key + self.key == other.key && + self.comment == other.comment end alias :== :eql? end diff --git a/activerecord/lib/arel/select_manager.rb b/activerecord/lib/arel/select_manager.rb index 32286b67f4..4e9f527235 100644 --- a/activerecord/lib/arel/select_manager.rb +++ b/activerecord/lib/arel/select_manager.rb @@ -244,6 +244,15 @@ module Arel # :nodoc: all @ctx.source end + def comment(*values) + @ctx.comment = Nodes::Comment.new(values) + self + end + + def comment_node + @ctx.comment + end + private def collapse(exprs) exprs = exprs.compact diff --git a/activerecord/lib/arel/tree_manager.rb b/activerecord/lib/arel/tree_manager.rb index 0476399618..326c4f995c 100644 --- a/activerecord/lib/arel/tree_manager.rb +++ b/activerecord/lib/arel/tree_manager.rb @@ -36,6 +36,11 @@ module Arel # :nodoc: all @ast.wheres << expr self end + + def comment(*values) + @ast.comment = Nodes::Comment.new(values) + self + end end attr_reader :ast diff --git a/activerecord/lib/arel/visitors/depth_first.rb b/activerecord/lib/arel/visitors/depth_first.rb index 109afb7402..d696edc507 100644 --- a/activerecord/lib/arel/visitors/depth_first.rb +++ b/activerecord/lib/arel/visitors/depth_first.rb @@ -181,6 +181,10 @@ module Arel # :nodoc: all visit o.limit end + def visit_Arel_Nodes_Comment(o) + visit o.values + end + def visit_Array(o) o.each { |i| visit i } end diff --git a/activerecord/lib/arel/visitors/dot.rb b/activerecord/lib/arel/visitors/dot.rb index 37803ce0c0..ecc386de07 100644 --- a/activerecord/lib/arel/visitors/dot.rb +++ b/activerecord/lib/arel/visitors/dot.rb @@ -234,6 +234,10 @@ module Arel # :nodoc: all end alias :visit_Set :visit_Array + def visit_Arel_Nodes_Comment(o) + visit_edge(o, "values") + end + def visit_edge(o, method) edge(method) { visit o.send(method) } end diff --git a/activerecord/lib/arel/visitors/to_sql.rb b/activerecord/lib/arel/visitors/to_sql.rb index 1630226085..4192d9efdc 100644 --- a/activerecord/lib/arel/visitors/to_sql.rb +++ b/activerecord/lib/arel/visitors/to_sql.rb @@ -35,6 +35,7 @@ module Arel # :nodoc: all collect_nodes_for o.wheres, collector, " WHERE ", " AND " collect_nodes_for o.orders, collector, " ORDER BY " maybe_visit o.limit, collector + maybe_visit o.comment, collector end def visit_Arel_Nodes_UpdateStatement(o, collector) @@ -47,6 +48,7 @@ module Arel # :nodoc: all collect_nodes_for o.wheres, collector, " WHERE ", " AND " collect_nodes_for o.orders, collector, " ORDER BY " maybe_visit o.limit, collector + maybe_visit o.comment, collector end def visit_Arel_Nodes_InsertStatement(o, collector) @@ -62,9 +64,9 @@ module Arel # :nodoc: all maybe_visit o.values, collector elsif o.select maybe_visit o.select, collector - else - collector end + + maybe_visit o.comment, collector end def visit_Arel_Nodes_Exists(o, collector) @@ -162,7 +164,7 @@ module Arel # :nodoc: all collect_nodes_for o.havings, collector, " HAVING ", " AND " collect_nodes_for o.windows, collector, " WINDOW " - collector + maybe_visit o.comment, collector end def visit_Arel_Nodes_OptimizerHints(o, collector) @@ -170,6 +172,10 @@ module Arel # :nodoc: all collector << "/*+ #{hints} */" end + def visit_Arel_Nodes_Comment(o, collector) + collector << o.values.map { |v| "/* #{sanitize_as_sql_comment(v)} */" }.join(" ") + end + def collect_nodes_for(nodes, collector, spacer, connector = ", ") unless nodes.empty? collector << spacer diff --git a/activerecord/test/cases/adapters/mysql2/annotate_test.rb b/activerecord/test/cases/adapters/mysql2/annotate_test.rb new file mode 100644 index 0000000000..b512540073 --- /dev/null +++ b/activerecord/test/cases/adapters/mysql2/annotate_test.rb @@ -0,0 +1,37 @@ +# frozen_string_literal: true + +require "cases/helper" +require "models/post" + +class Mysql2AnnotateTest < ActiveRecord::Mysql2TestCase + fixtures :posts + + def test_annotate_wraps_content_in_an_inline_comment + assert_sql(%r{\ASELECT `posts`\.`id` FROM `posts` /\* foo \*/}) do + posts = Post.select(:id).annotate("foo") + assert posts.first + end + end + + def test_annotate_is_sanitized + assert_sql(%r{\ASELECT `posts`\.`id` FROM `posts` /\* foo \*/}) do + posts = Post.select(:id).annotate("*/foo/*") + assert posts.first + end + + assert_sql(%r{\ASELECT `posts`\.`id` FROM `posts` /\* foo \*/}) do + posts = Post.select(:id).annotate("**//foo//**") + assert posts.first + end + + assert_sql(%r{\ASELECT `posts`\.`id` FROM `posts` /\* foo \*/ /\* bar \*/}) do + posts = Post.select(:id).annotate("*/foo/*").annotate("*/bar") + assert posts.first + end + + assert_sql(%r{\ASELECT `posts`\.`id` FROM `posts` /\* \+ MAX_EXECUTION_TIME\(1\) \*/}) do + posts = Post.select(:id).annotate("+ MAX_EXECUTION_TIME(1)") + assert posts.first + end + end +end diff --git a/activerecord/test/cases/adapters/postgresql/annotate_test.rb b/activerecord/test/cases/adapters/postgresql/annotate_test.rb new file mode 100644 index 0000000000..42a2861511 --- /dev/null +++ b/activerecord/test/cases/adapters/postgresql/annotate_test.rb @@ -0,0 +1,37 @@ +# frozen_string_literal: true + +require "cases/helper" +require "models/post" + +class PostgresqlAnnotateTest < ActiveRecord::PostgreSQLTestCase + fixtures :posts + + def test_annotate_wraps_content_in_an_inline_comment + assert_sql(%r{\ASELECT "posts"\."id" FROM "posts" /\* foo \*/}) do + posts = Post.select(:id).annotate("foo") + assert posts.first + end + end + + def test_annotate_is_sanitized + assert_sql(%r{\ASELECT "posts"\."id" FROM "posts" /\* foo \*/}) do + posts = Post.select(:id).annotate("*/foo/*") + assert posts.first + end + + assert_sql(%r{\ASELECT "posts"\."id" FROM "posts" /\* foo \*/}) do + posts = Post.select(:id).annotate("**//foo//**") + assert posts.first + end + + assert_sql(%r{\ASELECT "posts"\."id" FROM "posts" /\* foo \*/ /\* bar \*/}) do + posts = Post.select(:id).annotate("*/foo/*").annotate("*/bar") + assert posts.first + end + + assert_sql(%r{\ASELECT "posts"\."id" FROM "posts" /\* \+ MAX_EXECUTION_TIME\(1\) \*/}) do + posts = Post.select(:id).annotate("+ MAX_EXECUTION_TIME(1)") + assert posts.first + end + end +end diff --git a/activerecord/test/cases/adapters/sqlite3/annotate_test.rb b/activerecord/test/cases/adapters/sqlite3/annotate_test.rb new file mode 100644 index 0000000000..6567a5eca3 --- /dev/null +++ b/activerecord/test/cases/adapters/sqlite3/annotate_test.rb @@ -0,0 +1,37 @@ +# frozen_string_literal: true + +require "cases/helper" +require "models/post" + +class SQLite3AnnotateTest < ActiveRecord::SQLite3TestCase + fixtures :posts + + def test_annotate_wraps_content_in_an_inline_comment + assert_sql(%r{\ASELECT "posts"\."id" FROM "posts" /\* foo \*/}) do + posts = Post.select(:id).annotate("foo") + assert posts.first + end + end + + def test_annotate_is_sanitized + assert_sql(%r{\ASELECT "posts"\."id" FROM "posts" /\* foo \*/}) do + posts = Post.select(:id).annotate("*/foo/*") + assert posts.first + end + + assert_sql(%r{\ASELECT "posts"\."id" FROM "posts" /\* foo \*/}) do + posts = Post.select(:id).annotate("**//foo//**") + assert posts.first + end + + assert_sql(%r{\ASELECT "posts"\."id" FROM "posts" /\* foo \*/ /\* bar \*/}) do + posts = Post.select(:id).annotate("*/foo/*").annotate("*/bar") + assert posts.first + end + + assert_sql(%r{\ASELECT "posts"\."id" FROM "posts" /\* \+ MAX_EXECUTION_TIME\(1\) \*/}) do + posts = Post.select(:id).annotate("+ MAX_EXECUTION_TIME(1)") + assert posts.first + end + end +end diff --git a/activerecord/test/cases/arel/delete_manager_test.rb b/activerecord/test/cases/arel/delete_manager_test.rb index 0bad02f4d2..63cd1bffe3 100644 --- a/activerecord/test/cases/arel/delete_manager_test.rb +++ b/activerecord/test/cases/arel/delete_manager_test.rb @@ -49,5 +49,23 @@ module Arel dm.where(table[:id].eq(10)).must_equal dm end end + + describe "comment" do + it "chains" do + manager = Arel::DeleteManager.new + manager.comment("deleting").must_equal manager + end + + it "appends a comment to the generated query" do + table = Table.new(:users) + dm = Arel::DeleteManager.new + dm.from table + dm.comment("deletion") + assert_match(%r{DELETE FROM "users" /\* deletion \*/}, dm.to_sql) + + dm.comment("deletion", "with", "comment") + assert_match(%r{DELETE FROM "users" /\* deletion \*/ /\* with \*/ /\* comment \*/}, dm.to_sql) + end + end end end diff --git a/activerecord/test/cases/arel/nodes/comment_test.rb b/activerecord/test/cases/arel/nodes/comment_test.rb new file mode 100644 index 0000000000..bf5eaf4c5a --- /dev/null +++ b/activerecord/test/cases/arel/nodes/comment_test.rb @@ -0,0 +1,22 @@ +# frozen_string_literal: true + +require_relative "../helper" +require "yaml" + +module Arel + module Nodes + class CommentTest < Arel::Spec + describe "equality" do + it "is equal with equal contents" do + array = [Comment.new(["foo"]), Comment.new(["foo"])] + assert_equal 1, array.uniq.size + end + + it "is not equal with different contents" do + array = [Comment.new(["foo"]), Comment.new(["bar"])] + assert_equal 2, array.uniq.size + end + end + end + end +end diff --git a/activerecord/test/cases/arel/nodes/delete_statement_test.rb b/activerecord/test/cases/arel/nodes/delete_statement_test.rb index 3f078063a4..8ba268653d 100644 --- a/activerecord/test/cases/arel/nodes/delete_statement_test.rb +++ b/activerecord/test/cases/arel/nodes/delete_statement_test.rb @@ -18,8 +18,10 @@ describe Arel::Nodes::DeleteStatement do it "is equal with equal ivars" do statement1 = Arel::Nodes::DeleteStatement.new statement1.wheres = %w[a b c] + statement1.comment = Arel::Nodes::Comment.new(["comment"]) statement2 = Arel::Nodes::DeleteStatement.new statement2.wheres = %w[a b c] + statement2.comment = Arel::Nodes::Comment.new(["comment"]) array = [statement1, statement2] assert_equal 1, array.uniq.size end @@ -27,8 +29,14 @@ describe Arel::Nodes::DeleteStatement do it "is not equal with different ivars" do statement1 = Arel::Nodes::DeleteStatement.new statement1.wheres = %w[a b c] + statement1.comment = Arel::Nodes::Comment.new(["comment"]) statement2 = Arel::Nodes::DeleteStatement.new statement2.wheres = %w[1 2 3] + statement2.comment = Arel::Nodes::Comment.new(["comment"]) + array = [statement1, statement2] + assert_equal 2, array.uniq.size + statement2.wheres = %w[a b c] + statement2.comment = Arel::Nodes::Comment.new(["other"]) array = [statement1, statement2] assert_equal 2, array.uniq.size end diff --git a/activerecord/test/cases/arel/nodes/insert_statement_test.rb b/activerecord/test/cases/arel/nodes/insert_statement_test.rb index 252a0d0d0b..036576b231 100644 --- a/activerecord/test/cases/arel/nodes/insert_statement_test.rb +++ b/activerecord/test/cases/arel/nodes/insert_statement_test.rb @@ -23,9 +23,11 @@ describe Arel::Nodes::InsertStatement do statement1 = Arel::Nodes::InsertStatement.new statement1.columns = %w[a b c] statement1.values = %w[x y z] + statement1.comment = Arel::Nodes::Comment.new(["comment"]) statement2 = Arel::Nodes::InsertStatement.new statement2.columns = %w[a b c] statement2.values = %w[x y z] + statement2.comment = Arel::Nodes::Comment.new(["comment"]) array = [statement1, statement2] assert_equal 1, array.uniq.size end @@ -34,9 +36,15 @@ describe Arel::Nodes::InsertStatement do statement1 = Arel::Nodes::InsertStatement.new statement1.columns = %w[a b c] statement1.values = %w[x y z] + statement1.comment = Arel::Nodes::Comment.new(["comment"]) statement2 = Arel::Nodes::InsertStatement.new statement2.columns = %w[a b c] statement2.values = %w[1 2 3] + statement2.comment = Arel::Nodes::Comment.new(["comment"]) + array = [statement1, statement2] + assert_equal 2, array.uniq.size + statement2.values = %w[x y z] + statement2.comment = Arel::Nodes::Comment.new("other") array = [statement1, statement2] assert_equal 2, array.uniq.size end diff --git a/activerecord/test/cases/arel/nodes/select_core_test.rb b/activerecord/test/cases/arel/nodes/select_core_test.rb index 0b698205ff..6860f2a395 100644 --- a/activerecord/test/cases/arel/nodes/select_core_test.rb +++ b/activerecord/test/cases/arel/nodes/select_core_test.rb @@ -37,6 +37,7 @@ module Arel core1.groups = %w[j k l] core1.windows = %w[m n o] core1.havings = %w[p q r] + core1.comment = Arel::Nodes::Comment.new(["comment"]) core2 = SelectCore.new core2.froms = %w[a b c] core2.projections = %w[d e f] @@ -44,6 +45,7 @@ module Arel core2.groups = %w[j k l] core2.windows = %w[m n o] core2.havings = %w[p q r] + core2.comment = Arel::Nodes::Comment.new(["comment"]) array = [core1, core2] assert_equal 1, array.uniq.size end @@ -56,6 +58,7 @@ module Arel core1.groups = %w[j k l] core1.windows = %w[m n o] core1.havings = %w[p q r] + core1.comment = Arel::Nodes::Comment.new(["comment"]) core2 = SelectCore.new core2.froms = %w[a b c] core2.projections = %w[d e f] @@ -63,6 +66,11 @@ module Arel core2.groups = %w[j k l] core2.windows = %w[m n o] core2.havings = %w[l o l] + core2.comment = Arel::Nodes::Comment.new(["comment"]) + array = [core1, core2] + assert_equal 2, array.uniq.size + core2.havings = %w[p q r] + core2.comment = Arel::Nodes::Comment.new(["other"]) array = [core1, core2] assert_equal 2, array.uniq.size end diff --git a/activerecord/test/cases/arel/nodes/update_statement_test.rb b/activerecord/test/cases/arel/nodes/update_statement_test.rb index a83ce32f68..f133ddf7eb 100644 --- a/activerecord/test/cases/arel/nodes/update_statement_test.rb +++ b/activerecord/test/cases/arel/nodes/update_statement_test.rb @@ -27,6 +27,7 @@ describe Arel::Nodes::UpdateStatement do statement1.orders = %w[x y z] statement1.limit = 42 statement1.key = "zomg" + statement1.comment = Arel::Nodes::Comment.new(["comment"]) statement2 = Arel::Nodes::UpdateStatement.new statement2.relation = "zomg" statement2.wheres = 2 @@ -34,6 +35,7 @@ describe Arel::Nodes::UpdateStatement do statement2.orders = %w[x y z] statement2.limit = 42 statement2.key = "zomg" + statement2.comment = Arel::Nodes::Comment.new(["comment"]) array = [statement1, statement2] assert_equal 1, array.uniq.size end @@ -46,6 +48,7 @@ describe Arel::Nodes::UpdateStatement do statement1.orders = %w[x y z] statement1.limit = 42 statement1.key = "zomg" + statement1.comment = Arel::Nodes::Comment.new(["comment"]) statement2 = Arel::Nodes::UpdateStatement.new statement2.relation = "zomg" statement2.wheres = 2 @@ -53,6 +56,11 @@ describe Arel::Nodes::UpdateStatement do statement2.orders = %w[x y z] statement2.limit = 42 statement2.key = "wth" + statement2.comment = Arel::Nodes::Comment.new(["comment"]) + array = [statement1, statement2] + assert_equal 2, array.uniq.size + statement2.key = "zomg" + statement2.comment = Arel::Nodes::Comment.new(["other"]) array = [statement1, statement2] assert_equal 2, array.uniq.size end diff --git a/activerecord/test/cases/arel/select_manager_test.rb b/activerecord/test/cases/arel/select_manager_test.rb index 5220950905..e6c49cd429 100644 --- a/activerecord/test/cases/arel/select_manager_test.rb +++ b/activerecord/test/cases/arel/select_manager_test.rb @@ -1221,5 +1221,28 @@ module Arel manager.distinct_on(false).must_equal manager end end + + describe "comment" do + it "chains" do + manager = Arel::SelectManager.new + manager.comment("selecting").must_equal manager + end + + it "appends a comment to the generated query" do + manager = Arel::SelectManager.new + table = Table.new :users + manager.from(table).project(table["id"]) + + manager.comment("selecting") + manager.to_sql.must_be_like %{ + SELECT "users"."id" FROM "users" /* selecting */ + } + + manager.comment("selecting", "with", "comment") + manager.to_sql.must_be_like %{ + SELECT "users"."id" FROM "users" /* selecting */ /* with */ /* comment */ + } + end + end end end diff --git a/activerecord/test/cases/arel/support/fake_record.rb b/activerecord/test/cases/arel/support/fake_record.rb index 559ff5d4e6..18e6c10c9d 100644 --- a/activerecord/test/cases/arel/support/fake_record.rb +++ b/activerecord/test/cases/arel/support/fake_record.rb @@ -58,6 +58,10 @@ module FakeRecord "\"#{name}\"" end + def sanitize_as_sql_comment(comment) + comment + end + def schema_cache self end diff --git a/activerecord/test/cases/arel/update_manager_test.rb b/activerecord/test/cases/arel/update_manager_test.rb index cc1b9ac5b3..e13cb6aa52 100644 --- a/activerecord/test/cases/arel/update_manager_test.rb +++ b/activerecord/test/cases/arel/update_manager_test.rb @@ -122,5 +122,29 @@ module Arel @um.key.must_equal @table[:foo] end end + + describe "comment" do + it "chains" do + manager = Arel::UpdateManager.new + manager.comment("updating").must_equal manager + end + + it "appends a comment to the generated query" do + table = Table.new :users + + manager = Arel::UpdateManager.new + manager.table table + + manager.comment("updating") + manager.to_sql.must_be_like %{ + UPDATE "users" /* updating */ + } + + manager.comment("updating", "with", "comment") + manager.to_sql.must_be_like %{ + UPDATE "users" /* updating */ /* with */ /* comment */ + } + end + end end end diff --git a/activerecord/test/cases/arel/visitors/depth_first_test.rb b/activerecord/test/cases/arel/visitors/depth_first_test.rb index 4a57608411..106be2311d 100644 --- a/activerecord/test/cases/arel/visitors/depth_first_test.rb +++ b/activerecord/test/cases/arel/visitors/depth_first_test.rb @@ -101,6 +101,12 @@ module Arel assert_equal [:a, :b, join], @collector.calls end + def test_comment + comment = Nodes::Comment.new ["foo"] + @visitor.accept comment + assert_equal ["foo", ["foo"], comment], @collector.calls + end + [ Arel::Nodes::Assignment, Arel::Nodes::Between, diff --git a/activerecord/test/cases/associations/cascaded_eager_loading_test.rb b/activerecord/test/cases/associations/cascaded_eager_loading_test.rb index b9e16cab21..49f754be63 100644 --- a/activerecord/test/cases/associations/cascaded_eager_loading_test.rb +++ b/activerecord/test/cases/associations/cascaded_eager_loading_test.rb @@ -121,7 +121,7 @@ class CascadedEagerLoadingTest < ActiveRecord::TestCase assert reply.save topics = Topic.all.merge!(includes: :replies, order: ["topics.id", "replies_topics.id"]).to_a - assert_no_queries do + assert_queries(0) do assert_equal 2, topics[0].replies.size assert_equal 0, topics[1].replies.size end diff --git a/activerecord/test/cases/associations/eager_load_includes_full_sti_class_test.rb b/activerecord/test/cases/associations/eager_load_includes_full_sti_class_test.rb index 5fca972aee..673d5f1dcf 100644 --- a/activerecord/test/cases/associations/eager_load_includes_full_sti_class_test.rb +++ b/activerecord/test/cases/associations/eager_load_includes_full_sti_class_test.rb @@ -21,7 +21,7 @@ module PolymorphicFullStiClassNamesSharedTest ActiveRecord::Base.store_full_sti_class = store_full_sti_class post = Namespaced::Post.create(title: "Great stuff", body: "This is not", author_id: 1) - @tagging = Tagging.create(taggable: post) + @tagging = post.create_tagging! end def teardown diff --git a/activerecord/test/cases/associations_test.rb b/activerecord/test/cases/associations_test.rb index 081da95df7..84130ec208 100644 --- a/activerecord/test/cases/associations_test.rb +++ b/activerecord/test/cases/associations_test.rb @@ -21,6 +21,11 @@ require "models/molecule" require "models/electron" require "models/man" require "models/interest" +require "models/pirate" +require "models/parrot" +require "models/bird" +require "models/treasure" +require "models/price_estimate" class AssociationsTest < ActiveRecord::TestCase fixtures :accounts, :companies, :developers, :projects, :developers_projects, @@ -368,3 +373,97 @@ class GeneratedMethodsTest < ActiveRecord::TestCase assert_equal :none, MyArticle.new.comments end end + +class WithAnnotationsTest < ActiveRecord::TestCase + fixtures :pirates, :parrots + + def test_belongs_to_with_annotation_includes_a_query_comment + pirate = SpacePirate.where.not(parrot_id: nil).first + assert pirate, "should have a Pirate record" + + log = capture_sql do + pirate.parrot + end + assert_not_predicate log, :empty? + assert_predicate log.select { |query| query.match?(%r{/\*}) }, :empty? + + assert_sql(%r{/\* that tells jokes \*/}) do + pirate.parrot_with_annotation + end + end + + def test_has_and_belongs_to_many_with_annotation_includes_a_query_comment + pirate = SpacePirate.first + assert pirate, "should have a Pirate record" + + log = capture_sql do + pirate.parrots.first + end + assert_not_predicate log, :empty? + assert_predicate log.select { |query| query.match?(%r{/\*}) }, :empty? + + assert_sql(%r{/\* that are very colorful \*/}) do + pirate.parrots_with_annotation.first + end + end + + def test_has_one_with_annotation_includes_a_query_comment + pirate = SpacePirate.first + assert pirate, "should have a Pirate record" + + log = capture_sql do + pirate.ship + end + assert_not_predicate log, :empty? + assert_predicate log.select { |query| query.match?(%r{/\*}) }, :empty? + + assert_sql(%r{/\* that is a rocket \*/}) do + pirate.ship_with_annotation + end + end + + def test_has_many_with_annotation_includes_a_query_comment + pirate = SpacePirate.first + assert pirate, "should have a Pirate record" + + log = capture_sql do + pirate.birds.first + end + assert_not_predicate log, :empty? + assert_predicate log.select { |query| query.match?(%r{/\*}) }, :empty? + + assert_sql(%r{/\* that are also parrots \*/}) do + pirate.birds_with_annotation.first + end + end + + def test_has_many_through_with_annotation_includes_a_query_comment + pirate = SpacePirate.first + assert pirate, "should have a Pirate record" + + log = capture_sql do + pirate.treasure_estimates.first + end + assert_not_predicate log, :empty? + assert_predicate log.select { |query| query.match?(%r{/\*}) }, :empty? + + assert_sql(%r{/\* yarrr \*/}) do + pirate.treasure_estimates_with_annotation.first + end + end + + def test_has_many_through_with_annotation_includes_a_query_comment_when_eager_loading + pirate = SpacePirate.first + assert pirate, "should have a Pirate record" + + log = capture_sql do + pirate.treasure_estimates.first + end + assert_not_predicate log, :empty? + assert_predicate log.select { |query| query.match?(%r{/\*}) }, :empty? + + assert_sql(%r{/\* yarrr \*/}) do + SpacePirate.includes(:treasure_estimates_with_annotation, :treasures).first + end + end +end diff --git a/activerecord/test/cases/autosave_association_test.rb b/activerecord/test/cases/autosave_association_test.rb index 54eb885f6a..1a0732c14b 100644 --- a/activerecord/test/cases/autosave_association_test.rb +++ b/activerecord/test/cases/autosave_association_test.rb @@ -1,6 +1,7 @@ # frozen_string_literal: true require "cases/helper" +require "models/author" require "models/bird" require "models/post" require "models/comment" @@ -1319,21 +1320,45 @@ end class TestAutosaveAssociationOnAHasOneThroughAssociation < ActiveRecord::TestCase self.use_transactional_tests = false unless supports_savepoints? - def setup - super + def create_member_with_organization organization = Organization.create - @member = Member.create - MemberDetail.create(organization: organization, member: @member) + member = Member.create + MemberDetail.create(organization: organization, member: member) + + member end def test_should_not_has_one_through_model - class << @member.organization + member = create_member_with_organization + + class << member.organization + def save(*args) + super + raise "Oh noes!" + end + end + assert_nothing_raised { member.save } + end + + def create_author_with_post_with_comment + Author.create! name: "David" # make comment_id not match author_id + author = Author.create! name: "Sergiy" + post = Post.create! author: author, title: "foo", body: "bar" + Comment.create! post: post, body: "cool comment" + + author + end + + def test_should_not_reversed_has_one_through_model + author = create_author_with_post_with_comment + + class << author.comment_on_first_post def save(*args) super raise "Oh noes!" end end - assert_nothing_raised { @member.save } + assert_nothing_raised { author.save } end end diff --git a/activerecord/test/cases/relation/delete_all_test.rb b/activerecord/test/cases/relation/delete_all_test.rb index d1c13fa1b5..9b76936b7e 100644 --- a/activerecord/test/cases/relation/delete_all_test.rb +++ b/activerecord/test/cases/relation/delete_all_test.rb @@ -99,4 +99,23 @@ class DeleteAllTest < ActiveRecord::TestCase assert_raise(ActiveRecord::RecordNotFound) { posts(:thinking) } assert posts(:welcome) end + + def test_delete_all_with_annotation_includes_a_query_comment + davids = Author.where(name: "David").annotate("deleting all") + + assert_sql(%r{/\* deleting all \*/}) do + assert_difference("Author.count", -1) { davids.delete_all } + end + end + + def test_delete_all_without_annotation_does_not_include_an_empty_comment + davids = Author.where(name: "David") + + log = capture_sql do + assert_difference("Author.count", -1) { davids.delete_all } + end + + assert_not_predicate log, :empty? + assert_predicate log.select { |query| query.match?(%r{/\*}) }, :empty? + end end diff --git a/activerecord/test/cases/relation/merging_test.rb b/activerecord/test/cases/relation/merging_test.rb index 224e4f39a8..5c5e760e34 100644 --- a/activerecord/test/cases/relation/merging_test.rb +++ b/activerecord/test/cases/relation/merging_test.rb @@ -135,6 +135,18 @@ class RelationMergingTest < ActiveRecord::TestCase relation = Post.all.merge(Post.order(Arel.sql("title LIKE '%?'"))) assert_equal ["title LIKE '%?'"], relation.order_values end + + def test_merging_annotations_respects_merge_order + assert_sql(%r{/\* foo \*/ /\* bar \*/}) do + Post.annotate("foo").merge(Post.annotate("bar")).first + end + assert_sql(%r{/\* bar \*/ /\* foo \*/}) do + Post.annotate("bar").merge(Post.annotate("foo")).first + end + assert_sql(%r{/\* foo \*/ /\* bar \*/ /\* baz \*/ /\* qux \*/}) do + Post.annotate("foo").annotate("bar").merge(Post.annotate("baz").annotate("qux")).first + end + end end class MergingDifferentRelationsTest < ActiveRecord::TestCase diff --git a/activerecord/test/cases/relation/update_all_test.rb b/activerecord/test/cases/relation/update_all_test.rb index 0500574f28..526f926841 100644 --- a/activerecord/test/cases/relation/update_all_test.rb +++ b/activerecord/test/cases/relation/update_all_test.rb @@ -241,6 +241,33 @@ class UpdateAllTest < ActiveRecord::TestCase end end + def test_update_all_with_annotation_includes_a_query_comment + tag = Tag.first + + assert_sql(%r{/\* updating all \*/}) do + Post.tagged_with(tag.id).annotate("updating all").update_all(title: "rofl") + end + + posts = Post.tagged_with(tag.id).all.to_a + assert_operator posts.length, :>, 0 + posts.each { |post| assert_equal "rofl", post.title } + end + + def test_update_all_without_annotation_does_not_include_an_empty_comment + tag = Tag.first + + log = capture_sql do + Post.tagged_with(tag.id).update_all(title: "rofl") + end + + assert_not_predicate log, :empty? + assert_predicate log.select { |query| query.match?(%r{/\*}) }, :empty? + + posts = Post.tagged_with(tag.id).all.to_a + assert_operator posts.length, :>, 0 + posts.each { |post| assert_equal "rofl", post.title } + end + # Oracle UPDATE does not support ORDER BY unless current_adapter?(:OracleAdapter) def test_update_all_ignores_order_without_limit_from_association diff --git a/activerecord/test/cases/relation_test.rb b/activerecord/test/cases/relation_test.rb index 68161f6a84..00a7b3841f 100644 --- a/activerecord/test/cases/relation_test.rb +++ b/activerecord/test/cases/relation_test.rb @@ -307,6 +307,58 @@ module ActiveRecord assert_equal 3, ratings.count end + def test_relation_with_annotation_includes_comment_in_to_sql + post_with_annotation = Post.where(id: 1).annotate("foo") + assert_match %r{= 1 /\* foo \*/}, post_with_annotation.to_sql + end + + def test_relation_with_annotation_includes_comment_in_sql + post_with_annotation = Post.where(id: 1).annotate("foo") + assert_sql(%r{/\* foo \*/}) do + assert post_with_annotation.first, "record should be found" + end + end + + def test_relation_with_annotation_chains_sql_comments + post_with_annotation = Post.where(id: 1).annotate("foo").annotate("bar") + assert_sql(%r{/\* foo \*/ /\* bar \*/}) do + assert post_with_annotation.first, "record should be found" + end + end + + def test_relation_with_annotation_filters_sql_comment_delimiters + post_with_annotation = Post.where(id: 1).annotate("**//foo//**") + assert_match %r{= 1 /\* foo \*/}, post_with_annotation.to_sql + end + + def test_relation_with_annotation_includes_comment_in_count_query + post_with_annotation = Post.annotate("foo") + all_count = Post.all.to_a.count + assert_sql(%r{/\* foo \*/}) do + assert_equal all_count, post_with_annotation.count + end + end + + def test_relation_without_annotation_does_not_include_an_empty_comment + log = capture_sql do + Post.where(id: 1).first + end + + assert_not_predicate log, :empty? + assert_predicate log.select { |query| query.match?(%r{/\*}) }, :empty? + end + + def test_relation_with_optimizer_hints_filters_sql_comment_delimiters + post_with_hint = Post.where(id: 1).optimizer_hints("**//BADHINT//**") + assert_match %r{BADHINT}, post_with_hint.to_sql + assert_no_match %r{\*/BADHINT}, post_with_hint.to_sql + assert_no_match %r{\*//BADHINT}, post_with_hint.to_sql + assert_no_match %r{BADHINT/\*}, post_with_hint.to_sql + assert_no_match %r{BADHINT//\*}, post_with_hint.to_sql + post_with_hint = Post.where(id: 1).optimizer_hints("/*+ BADHINT */") + assert_match %r{/\*\+ BADHINT \*/}, post_with_hint.to_sql + end + class EnsureRoundTripTypeCasting < ActiveRecord::Type::Value def type :string diff --git a/activerecord/test/cases/scoping/named_scoping_test.rb b/activerecord/test/cases/scoping/named_scoping_test.rb index 8b08e40468..3488442cab 100644 --- a/activerecord/test/cases/scoping/named_scoping_test.rb +++ b/activerecord/test/cases/scoping/named_scoping_test.rb @@ -602,4 +602,14 @@ class NamedScopingTest < ActiveRecord::TestCase Topic.create! assert_predicate Topic, :one? end + + def test_scope_with_annotation + Topic.class_eval do + scope :including_annotate_in_scope, Proc.new { annotate("from-scope") } + end + + assert_sql(%r{/\* from-scope \*/}) do + assert Topic.including_annotate_in_scope.to_a, Topic.all.to_a + end + end end diff --git a/activerecord/test/cases/scoping/relation_scoping_test.rb b/activerecord/test/cases/scoping/relation_scoping_test.rb index b1f2ffe29c..a95ab0f429 100644 --- a/activerecord/test/cases/scoping/relation_scoping_test.rb +++ b/activerecord/test/cases/scoping/relation_scoping_test.rb @@ -130,6 +130,44 @@ class RelationScopingTest < ActiveRecord::TestCase end end + def test_scoped_find_with_annotation + Developer.annotate("scoped").scoping do + developer = nil + assert_sql(%r{/\* scoped \*/}) do + developer = Developer.where("name = 'David'").first + end + assert_equal "David", developer.name + end + end + + def test_find_with_annotation_unscoped + Developer.annotate("scoped").unscoped do + developer = nil + log = capture_sql do + developer = Developer.where("name = 'David'").first + end + + assert_not_predicate log, :empty? + assert_predicate log.select { |query| query.match?(%r{/\* scoped \*/}) }, :empty? + + assert_equal "David", developer.name + end + end + + def test_find_with_annotation_unscope + developer = nil + log = capture_sql do + developer = Developer.annotate("unscope"). + where("name = 'David'"). + unscope(:annotate).first + end + + assert_not_predicate log, :empty? + assert_predicate log.select { |query| query.match?(%r{/\* unscope \*/}) }, :empty? + + assert_equal "David", developer.name + end + def test_scoped_find_include # with the include, will retrieve only developers for the given project scoped_developers = Developer.includes(:projects).scoping do diff --git a/activerecord/test/models/pirate.rb b/activerecord/test/models/pirate.rb index fd5083e597..8733398697 100644 --- a/activerecord/test/models/pirate.rb +++ b/activerecord/test/models/pirate.rb @@ -98,3 +98,19 @@ class FamousPirate < ActiveRecord::Base has_many :famous_ships validates_presence_of :catchphrase, on: :conference end + +class SpacePirate < ActiveRecord::Base + self.table_name = "pirates" + + belongs_to :parrot + belongs_to :parrot_with_annotation, -> { annotate("that tells jokes") }, class_name: :Parrot, foreign_key: :parrot_id + has_and_belongs_to_many :parrots, foreign_key: :pirate_id + has_and_belongs_to_many :parrots_with_annotation, -> { annotate("that are very colorful") }, class_name: :Parrot, foreign_key: :pirate_id + has_one :ship, foreign_key: :pirate_id + has_one :ship_with_annotation, -> { annotate("that is a rocket") }, class_name: :Ship, foreign_key: :pirate_id + has_many :birds, foreign_key: :pirate_id + has_many :birds_with_annotation, -> { annotate("that are also parrots") }, class_name: :Bird, foreign_key: :pirate_id + has_many :treasures, as: :looter + has_many :treasure_estimates, through: :treasures, source: :price_estimates + has_many :treasure_estimates_with_annotation, -> { annotate("yarrr") }, through: :treasures, source: :price_estimates +end diff --git a/activesupport/CHANGELOG.md b/activesupport/CHANGELOG.md index 63e2e44597..0551f0781f 100644 --- a/activesupport/CHANGELOG.md +++ b/activesupport/CHANGELOG.md @@ -1,3 +1,15 @@ +* Use weak references in descendants tracker to allow anonymous subclasses to + be garbage collected. + + *Edgars Beigarts* + +* Update `ActiveSupport::Notifications::Instrumenter#instrument` to make + passing a block optional. This will let users use + `ActiveSupport::Notifications` messaging features outside of + instrumentation. + + *Ali Ibrahim* + * Fix `Time#advance` to work with dates before 1001-03-07 Before: diff --git a/activesupport/activesupport.gemspec b/activesupport/activesupport.gemspec index 51f5086cca..bf0fe0f76d 100644 --- a/activesupport/activesupport.gemspec +++ b/activesupport/activesupport.gemspec @@ -34,5 +34,5 @@ Gem::Specification.new do |s| s.add_dependency "tzinfo", "~> 1.1" s.add_dependency "minitest", "~> 5.1" s.add_dependency "concurrent-ruby", "~> 1.0", ">= 1.0.2" - s.add_dependency "zeitwerk", "~> 1.4" + s.add_dependency "zeitwerk", "~> 1.4", ">= 1.4.3" end diff --git a/activesupport/lib/active_support/descendants_tracker.rb b/activesupport/lib/active_support/descendants_tracker.rb index 05236d3162..2dca990712 100644 --- a/activesupport/lib/active_support/descendants_tracker.rb +++ b/activesupport/lib/active_support/descendants_tracker.rb @@ -1,5 +1,7 @@ # frozen_string_literal: true +require "weakref" + module ActiveSupport # This module provides an internal implementation to track descendants # which is faster than iterating through ObjectSpace. @@ -8,7 +10,8 @@ module ActiveSupport class << self def direct_descendants(klass) - @@direct_descendants[klass] || [] + descendants = @@direct_descendants[klass] + descendants ? descendants.to_a : [] end def descendants(klass) @@ -34,15 +37,17 @@ module ActiveSupport # This is the only method that is not thread safe, but is only ever called # during the eager loading phase. def store_inherited(klass, descendant) - (@@direct_descendants[klass] ||= []) << descendant + (@@direct_descendants[klass] ||= DescendantsArray.new) << descendant end private def accumulate_descendants(klass, acc) if direct_descendants = @@direct_descendants[klass] - acc.concat(direct_descendants) - direct_descendants.each { |direct_descendant| accumulate_descendants(direct_descendant, acc) } + direct_descendants.each do |direct_descendant| + acc << direct_descendant + accumulate_descendants(direct_descendant, acc) + end end end end @@ -59,5 +64,46 @@ module ActiveSupport def descendants DescendantsTracker.descendants(self) end + + # DescendantsArray is an array that contains weak references to classes. + class DescendantsArray # :nodoc: + include Enumerable + + def initialize + @refs = [] + end + + def initialize_copy(orig) + @refs = @refs.dup + end + + def <<(klass) + cleanup! + @refs << WeakRef.new(klass) + end + + def each + @refs.each do |ref| + yield ref.__getobj__ + rescue WeakRef::RefError + end + end + + def refs_size + @refs.size + end + + def cleanup! + @refs.delete_if { |ref| !ref.weakref_alive? } + end + + def reject! + @refs.reject! do |ref| + yield ref.__getobj__ + rescue WeakRef::RefError + true + end + end + end end end diff --git a/activesupport/lib/active_support/notifications/instrumenter.rb b/activesupport/lib/active_support/notifications/instrumenter.rb index 00a57c38c9..a03e7e483e 100644 --- a/activesupport/lib/active_support/notifications/instrumenter.rb +++ b/activesupport/lib/active_support/notifications/instrumenter.rb @@ -13,14 +13,15 @@ module ActiveSupport @notifier = notifier end - # Instrument the given block by measuring the time taken to execute it - # and publish it. Notice that events get sent even if an error occurs - # in the passed-in block. + # Given a block, instrument it by measuring the time taken to execute + # and publish it. Without a block, simply send a message via the + # notifier. Notice that events get sent even if an error occurs in the + # passed-in block. def instrument(name, payload = {}) # some of the listeners might have state listeners_state = start name, payload begin - yield payload + yield payload if block_given? rescue Exception => e payload[:exception] = [e.class.name, e.message] payload[:exception_object] = e diff --git a/activesupport/test/descendants_tracker_test_cases.rb b/activesupport/test/descendants_tracker_test_cases.rb index 2c94c3c56c..f8752688d2 100644 --- a/activesupport/test/descendants_tracker_test_cases.rb +++ b/activesupport/test/descendants_tracker_test_cases.rb @@ -27,6 +27,15 @@ module DescendantsTrackerTestCases assert_equal_sets [], Child2.descendants end + def test_descendants_with_garbage_collected_classes + 1.times do + child_klass = Class.new(Parent) + assert_equal_sets [Child1, Grandchild1, Grandchild2, Child2, child_klass], Parent.descendants + end + GC.start + assert_equal_sets [Child1, Grandchild1, Grandchild2, Child2], Parent.descendants + end + def test_direct_descendants assert_equal_sets [Child1, Child2], Parent.direct_descendants assert_equal_sets [Grandchild1, Grandchild2], Child1.direct_descendants diff --git a/activesupport/test/notifications/instrumenter_test.rb b/activesupport/test/notifications/instrumenter_test.rb index d5c9e82e9f..9729ad5c89 100644 --- a/activesupport/test/notifications/instrumenter_test.rb +++ b/activesupport/test/notifications/instrumenter_test.rb @@ -44,6 +44,12 @@ module ActiveSupport assert_equal Hash[result: 2], payload end + def test_instrument_works_without_a_block + instrumenter.instrument("no.block", payload) + assert_equal 1, notifier.finishes.size + assert_equal "no.block", notifier.finishes.first.first + end + def test_start instrumenter.start("foo", payload) assert_equal [["foo", instrumenter.id, payload]], notifier.starts diff --git a/ci/qunit-selenium-runner.rb b/ci/qunit-selenium-runner.rb index 1df6aedb36..b7013c258a 100644 --- a/ci/qunit-selenium-runner.rb +++ b/ci/qunit-selenium-runner.rb @@ -5,7 +5,7 @@ require "qunit/selenium/test_runner" if ARGV[1] driver = ::Selenium::WebDriver.for(:remote, url: ARGV[1], desired_capabilities: :chrome) else - require "chromedriver-helper" + require "webdrivers" driver_options = Selenium::WebDriver::Chrome::Options.new driver_options.add_argument("--headless") diff --git a/guides/rails_guides/generator.rb b/guides/rails_guides/generator.rb index c3b77aa7bb..7d4a15962c 100644 --- a/guides/rails_guides/generator.rb +++ b/guides/rails_guides/generator.rb @@ -164,7 +164,7 @@ module RailsGuides # Generate the special pages like the home. # Passing a template handler in the template name is deprecated. So pass the file name without the extension. - result = view.render(layout: layout, formats: [$1], file: $`) + result = view.render(layout: layout, formats: [$1.to_sym], file: $`) else body = File.read("#{@source_dir}/#{guide}") result = RailsGuides::Markdown.new( diff --git a/guides/source/6_0_release_notes.md b/guides/source/6_0_release_notes.md index 0cf9ca09c7..04a02259e9 100644 --- a/guides/source/6_0_release_notes.md +++ b/guides/source/6_0_release_notes.md @@ -138,7 +138,7 @@ Please refer to the [Changelog][railties] for detailed changes. the generators. ([Pull Request](https://github.com/rails/rails/pull/34021)) -* Add support for multi environment credentials=. +* Add support for multi environment credentials. ([Pull Request](https://github.com/rails/rails/pull/33521)) * Make `null_store` as default cache store in test environment. diff --git a/guides/source/active_record_querying.md b/guides/source/active_record_querying.md index 71a03e11d9..cc6e08aaec 100644 --- a/guides/source/active_record_querying.md +++ b/guides/source/active_record_querying.md @@ -59,6 +59,7 @@ To retrieve objects from the database, Active Record provides several finder met The methods are: +* `annotate` * `find` * `create_with` * `distinct` diff --git a/guides/source/active_support_instrumentation.md b/guides/source/active_support_instrumentation.md index 89e0e3afa8..d7dbc5cea8 100644 --- a/guides/source/active_support_instrumentation.md +++ b/guides/source/active_support_instrumentation.md @@ -692,5 +692,16 @@ ActiveSupport::Notifications.subscribe "my.custom.event" do |name, started, fini end ``` +You also have the option to call instrument without passing a block. This lets you leverage the +instrumentation infrastructure for other messaging uses. + +```ruby +ActiveSupport::Notifications.instrument "my.custom.event", this: :data + +ActiveSupport::Notifications.subscribe "my.custom.event" do |name, started, finished, unique_id, data| + puts data.inspect # {:this=>:data} +end +``` + You should follow Rails conventions when defining your own events. The format is: `event.library`. If your application is sending Tweets, you should create an event named `tweet.twitter`. diff --git a/guides/source/command_line.md b/guides/source/command_line.md index 04c8352b90..ddb30a3aec 100644 --- a/guides/source/command_line.md +++ b/guides/source/command_line.md @@ -606,7 +606,7 @@ $ rails "task_name[value 1]" # entire argument string should be quoted $ rails db:nothing ``` -NOTE: If your need to interact with your application models, perform database queries, and so on, your task should depend on the `environment` task, which will load your application code. +NOTE: If you need to interact with your application models, perform database queries, and so on, your task should depend on the `environment` task, which will load your application code. The Rails Advanced Command Line ------------------------------- diff --git a/guides/source/configuring.md b/guides/source/configuring.md index a61ba5dc9f..04ad5a56a2 100644 --- a/guides/source/configuring.md +++ b/guides/source/configuring.md @@ -86,6 +86,8 @@ application. Accepts a valid week day symbol (e.g. `:monday`). end ``` +* `config.disable_sandbox` controls whether or not someone can start a console in sandbox mode. This is helpful to avoid a long running session of sandbox console, that could lead a database server to run out of memory. Defaults to false. + * `config.eager_load` when `true`, eager loads all registered `config.eager_load_namespaces`. This includes your application, engines, Rails frameworks, and any other registered namespace. * `config.eager_load_namespaces` registers namespaces that are eager loaded when `config.eager_load` is `true`. All namespaces in the list must respond to the `eager_load!` method. @@ -581,7 +583,7 @@ Defaults to `'signed cookie'`. The default setting is `true`, which uses the partial at `/admin/articles/_article.erb`. Setting the value to `false` would render `/articles/_article.erb`, which is the same behavior as rendering from a non-namespaced controller such as `ArticlesController`. * `config.action_view.raise_on_missing_translations` determines whether an - error should be raised for missing translations. + error should be raised for missing translations. This defaults to `false`. * `config.action_view.automatically_disable_submit_tag` determines whether `submit_tag` should automatically disable on click, this defaults to `true`. @@ -725,7 +727,7 @@ There are a few configuration options available in Active Support: * `ActiveSupport::Deprecation.silence` takes a block in which all deprecation warnings are silenced. -* `ActiveSupport::Deprecation.silenced` sets whether or not to display deprecation warnings. +* `ActiveSupport::Deprecation.silenced` sets whether or not to display deprecation warnings. The default is `false`. ### Configuring Active Job diff --git a/guides/source/debugging_rails_applications.md b/guides/source/debugging_rails_applications.md index 3a383cbd4d..77513c3a84 100644 --- a/guides/source/debugging_rails_applications.md +++ b/guides/source/debugging_rails_applications.md @@ -147,7 +147,7 @@ TIP: The default Rails log level is `debug` in all environments. ### Sending Messages -To write in the current log use the `logger.(debug|info|warn|error|fatal)` method from within a controller, model, or mailer: +To write in the current log use the `logger.(debug|info|warn|error|fatal|unknown)` method from within a controller, model, or mailer: ```ruby logger.debug "Person attributes hash: #{@person.attributes.inspect}" diff --git a/railties/CHANGELOG.md b/railties/CHANGELOG.md index 226b949b34..df0d6d4fa0 100644 --- a/railties/CHANGELOG.md +++ b/railties/CHANGELOG.md @@ -1,10 +1,33 @@ +* Applications running in `:zeitwerk` mode that use `bootsnap` need + to upgrade `bootsnap` to at least 1.4.2. + + *Xavier Noria* + +* Add `config.disable_sandbox` option to Rails console. + + This setting will disable `rails console --sandbox` mode, preventing + developer from accidentally starting a sandbox console, + which when left inactive, can cause the database server to run out of memory. + + *Prem Sichanugrist* + * Add `-e/--environment` option to `rails initializers`. *Yuji Yaginuma* ## Rails 6.0.0.beta3 (March 11, 2019) ## -* No changes. +* Generate random development secrets + + A random development secret is now generated to tmp/development_secret.txt + + This avoids an issue where development mode servers were vulnerable to + remote code execution. + + Fixes CVE-2019-5420 + + *Eileen M. Uchitelle*, *Aaron Patterson*, *John Hawthorn* + ## Rails 6.0.0.beta2 (February 25, 2019) ## diff --git a/railties/lib/rails/application.rb b/railties/lib/rails/application.rb index 6bc6c548d2..038284ebdd 100644 --- a/railties/lib/rails/application.rb +++ b/railties/lib/rails/application.rb @@ -409,7 +409,8 @@ module Rails # The secret_key_base is used as the input secret to the application's key generator, which in turn # is used to create all MessageVerifiers/MessageEncryptors, including the ones that sign and encrypt cookies. # - # In test and development, this is simply derived as a MD5 hash of the application's name. + # In development and test, this is randomly generated and stored in a + # temporary file in <tt>tmp/development_secret.txt</tt>. # # In all other environments, we look for it first in ENV["SECRET_KEY_BASE"], # then credentials.secret_key_base, and finally secrets.secret_key_base. For most applications, diff --git a/railties/lib/rails/application/configuration.rb b/railties/lib/rails/application/configuration.rb index 83a7b6cf01..b79dbdbc6f 100644 --- a/railties/lib/rails/application/configuration.rb +++ b/railties/lib/rails/application/configuration.rb @@ -18,7 +18,8 @@ module Rails :session_options, :time_zone, :reload_classes_only_on_change, :beginning_of_week, :filter_redirect, :x, :enable_dependency_loading, :read_encrypted_secrets, :log_level, :content_security_policy_report_only, - :content_security_policy_nonce_generator, :require_master_key, :credentials + :content_security_policy_nonce_generator, :require_master_key, :credentials, + :disable_sandbox attr_reader :encoding, :api_only, :loaded_config_version, :autoloader @@ -65,6 +66,7 @@ module Rails @credentials.content_path = default_credentials_content_path @credentials.key_path = default_credentials_key_path @autoloader = :classic + @disable_sandbox = false end def load_defaults(target_version) diff --git a/railties/lib/rails/commands/console/console_command.rb b/railties/lib/rails/commands/console/console_command.rb index e35faa5b01..7a9eaefea1 100644 --- a/railties/lib/rails/commands/console/console_command.rb +++ b/railties/lib/rails/commands/console/console_command.rb @@ -26,6 +26,12 @@ module Rails @options = options app.sandbox = sandbox? + + if sandbox? && app.config.disable_sandbox + puts "Error: Unable to start console in sandbox mode as sandbox mode is disabled (config.disable_sandbox is true)." + exit 1 + end + app.load_console @console = app.config.console || IRB diff --git a/railties/lib/rails/commands/credentials/credentials_command.rb b/railties/lib/rails/commands/credentials/credentials_command.rb index a22b1f3f84..e23a1b3008 100644 --- a/railties/lib/rails/commands/credentials/credentials_command.rb +++ b/railties/lib/rails/commands/credentials/credentials_command.rb @@ -56,7 +56,11 @@ module Rails end def ensure_credentials_have_been_added - encrypted_file_generator.add_encrypted_file_silently(content_path, key_path) + if options[:environment] + encrypted_file_generator.add_encrypted_file_silently(content_path, key_path) + else + credentials_generator.add_credentials_file_silently + end end def change_credentials_in_system_editor @@ -96,6 +100,13 @@ module Rails Rails::Generators::EncryptedFileGenerator.new end + + def credentials_generator + require "rails/generators" + require "rails/generators/rails/credentials/credentials_generator" + + Rails::Generators::CredentialsGenerator.new + end end end end diff --git a/railties/lib/rails/generators/rails/app/templates/Gemfile.tt b/railties/lib/rails/generators/rails/app/templates/Gemfile.tt index 18de6948f0..d7221453e7 100644 --- a/railties/lib/rails/generators/rails/app/templates/Gemfile.tt +++ b/railties/lib/rails/generators/rails/app/templates/Gemfile.tt @@ -28,7 +28,7 @@ ruby <%= "'#{RUBY_VERSION}'" -%> <% if depend_on_bootsnap? -%> # Reduces boot times through caching; required in config/boot.rb -gem 'bootsnap', '>= 1.4.1', require: false +gem 'bootsnap', '>= 1.4.2', require: false <%- end -%> <%- if options.api? -%> @@ -69,8 +69,8 @@ group :test do # Adds support for Capybara system testing and selenium driver gem 'capybara', '>= 2.15' gem 'selenium-webdriver' - # Easy installation and use of chromedriver to run system tests with Chrome - gem 'chromedriver-helper' + # Easy installation and use of web drivers to run system tests with browsers + gem 'webdrivers' end <%- end -%> diff --git a/railties/test/application/configuration_test.rb b/railties/test/application/configuration_test.rb index e34c075a1c..b8e167b488 100644 --- a/railties/test/application/configuration_test.rb +++ b/railties/test/application/configuration_test.rb @@ -2476,6 +2476,22 @@ module ApplicationTests assert_includes Rails.application.config.hosts, ".localhost" end + test "disable_sandbox is false by default" do + app "development" + + assert_equal false, Rails.configuration.disable_sandbox + end + + test "disable_sandbox can be overridden" do + add_to_config <<-RUBY + config.disable_sandbox = true + RUBY + + app "development" + + assert Rails.configuration.disable_sandbox + end + private def force_lazy_load_hooks yield # Tasty clarifying sugar, homie! We only need to reference a constant to load it. diff --git a/railties/test/application/console_test.rb b/railties/test/application/console_test.rb index b6270525f0..db16f4cc56 100644 --- a/railties/test/application/console_test.rb +++ b/railties/test/application/console_test.rb @@ -123,13 +123,17 @@ class FullStackConsoleTest < ActiveSupport::TestCase assert_output "> ", @primary end - def spawn_console(options) - Process.spawn( + def spawn_console(options, wait_for_prompt: true) + pid = Process.spawn( "#{app_path}/bin/rails console #{options}", in: @replica, out: @replica, err: @replica ) - assert_output "> ", @primary, 30 + if wait_for_prompt + assert_output "> ", @primary, 30 + end + + pid end def test_sandbox @@ -148,6 +152,17 @@ class FullStackConsoleTest < ActiveSupport::TestCase @primary.puts "quit" end + def test_sandbox_when_sandbox_is_disabled + add_to_config <<-RUBY + config.disable_sandbox = true + RUBY + + output = `#{app_path}/bin/rails console --sandbox` + + assert_includes output, "sandbox mode is disabled" + assert_equal 1, $?.exitstatus + end + def test_environment_option_and_irb_option spawn_console("-e test -- --verbose") diff --git a/railties/test/commands/console_test.rb b/railties/test/commands/console_test.rb index 1941c83d6d..f6df2b694a 100644 --- a/railties/test/commands/console_test.rb +++ b/railties/test/commands/console_test.rb @@ -129,7 +129,7 @@ class Rails::ConsoleTest < ActiveSupport::TestCase def build_app(console) mocked_console = Class.new do attr_accessor :sandbox - attr_reader :console + attr_reader :console, :disable_sandbox def initialize(console) @console = console diff --git a/railties/test/commands/credentials_test.rb b/railties/test/commands/credentials_test.rb index 3654e96aed..0ee36081c0 100644 --- a/railties/test/commands/credentials_test.rb +++ b/railties/test/commands/credentials_test.rb @@ -63,7 +63,7 @@ class Rails::Command::CredentialsCommandTest < ActiveSupport::TestCase end end - test "edit command properly expand environment option" do + test "edit command properly expands environment option" do assert_match(/access_key_id: 123/, run_edit_command(environment: "prod")) Dir.chdir(app_path) do assert File.exist?("config/credentials/production.key") @@ -79,11 +79,20 @@ class Rails::Command::CredentialsCommandTest < ActiveSupport::TestCase assert_match(/access_key_id: 123/, run_edit_command(environment: "qa")) end + test "edit command generates template file when the file does not exist" do + FileUtils.rm("#{app_path}/config/credentials.yml.enc") + run_edit_command + + output = run_show_command + assert_match(/access_key_id: 123/, output) + assert_match(/secret_key_base/, output) + end + test "show credentials" do assert_match(/access_key_id: 123/, run_show_command) end - test "show command raise error when require_master_key is specified and key does not exist" do + test "show command raises error when require_master_key is specified and key does not exist" do remove_file "config/master.key" add_to_config "config.require_master_key = true" @@ -103,10 +112,12 @@ class Rails::Command::CredentialsCommandTest < ActiveSupport::TestCase assert_match(/access_key_id: 123/, run_show_command(environment: "production")) end - test "show command properly expand environment option" do + test "show command properly expands environment option" do run_edit_command(environment: "production") - assert_match(/access_key_id: 123/, run_show_command(environment: "prod")) + output = run_show_command(environment: "prod") + assert_match(/access_key_id: 123/, output) + assert_no_match(/secret_key_base/, output) end private diff --git a/railties/test/generators/app_generator_test.rb b/railties/test/generators/app_generator_test.rb index 1ee9e43e89..d5a3599f67 100644 --- a/railties/test/generators/app_generator_test.rb +++ b/railties/test/generators/app_generator_test.rb @@ -617,7 +617,7 @@ class AppGeneratorTest < Rails::Generators::TestCase assert_no_gem "capybara" assert_no_gem "selenium-webdriver" - assert_no_gem "chromedriver-helper" + assert_no_gem "webdrivers" assert_no_directory("test") end @@ -626,7 +626,7 @@ class AppGeneratorTest < Rails::Generators::TestCase run_generator [destination_root, "--skip-system-test"] assert_no_gem "capybara" assert_no_gem "selenium-webdriver" - assert_no_gem "chromedriver-helper" + assert_no_gem "webdrivers" assert_directory("test") |