diff options
112 files changed, 1683 insertions, 889 deletions
diff --git a/.codeclimate.yml b/.codeclimate.yml index b8bebe4d42..7114a98266 100644 --- a/.codeclimate.yml +++ b/.codeclimate.yml @@ -1,3 +1,5 @@ +version: "2" + checks: argument-count: enabled: false @@ -20,11 +22,9 @@ checks: identical-code: enabled: false -engines: +plugins: rubocop: enabled: true - channel: rubocop-0-66 + channel: rubocop-0-67 -ratings: - paths: - - "**.rb" +exclude_patterns: [] diff --git a/.rubocop.yml b/.rubocop.yml index 3dbd4a27a6..0cfe5d5d84 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -1,3 +1,5 @@ +require: rubocop-performance + AllCops: TargetRubyVersion: 2.5 # RuboCop has a bunch of cops enabled by default. This setting tells RuboCop @@ -29,6 +29,7 @@ gem "uglifier", ">= 1.3.0", require: false gem "json", ">= 2.0.0" gem "rubocop", ">= 0.47", require: false +gem "rubocop-performance", require: false group :doc do gem "sdoc", "~> 1.0" diff --git a/Gemfile.lock b/Gemfile.lock index 8e219f3f31..62ad1134d5 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -71,7 +71,7 @@ PATH i18n (>= 0.7, < 2) minitest (~> 5.1) tzinfo (~> 1.1) - zeitwerk (~> 2.1) + zeitwerk (~> 2.1, >= 2.1.2) rails (6.0.0.beta3) actioncable (= 6.0.0.beta3) actionmailbox (= 6.0.0.beta3) @@ -411,7 +411,7 @@ GEM resque (~> 1.26) rufus-scheduler (~> 3.2) retriable (3.1.2) - rubocop (0.66.0) + rubocop (0.67.2) jaro_winkler (~> 1.5.1) parallel (~> 1.10) parser (>= 2.5, != 2.5.1.1) @@ -419,6 +419,8 @@ GEM rainbow (>= 2.2.2, < 4.0) ruby-progressbar (~> 1.7) unicode-display_width (>= 1.4.0, < 1.6) + rubocop-performance (1.1.0) + rubocop (>= 0.67.0) ruby-progressbar (1.10.0) ruby-vips (2.0.13) ffi (~> 1.9) @@ -526,7 +528,7 @@ GEM websocket-extensions (0.1.3) xpath (3.2.0) nokogiri (~> 1.8) - zeitwerk (2.1.0) + zeitwerk (2.1.2) PLATFORMS java @@ -581,6 +583,7 @@ DEPENDENCIES resque resque-scheduler rubocop (>= 0.47) + rubocop-performance sass-rails sdoc (~> 1.0) selenium-webdriver (>= 3.5.0, < 3.13.0) diff --git a/actionmailbox/README.md b/actionmailbox/README.md index 9a47223d3b..593bd429ae 100644 --- a/actionmailbox/README.md +++ b/actionmailbox/README.md @@ -1,6 +1,6 @@ # Action Mailbox -Action Mailbox routes incoming emails to controller-like mailboxes for processing in Rails. It ships with ingresses for Amazon SES, Mailgun, Mandrill, Postmark, and SendGrid. You can also handle inbound mails directly via the built-in Exim, Postfix, and Qmail ingresses. +Action Mailbox routes incoming emails to controller-like mailboxes for processing in Rails. It ships with ingresses for Mailgun, Mandrill, Postmark, and SendGrid. You can also handle inbound mails directly via the built-in Exim, Postfix, and Qmail ingresses. The inbound emails are turned into `InboundEmail` records using Active Record and feature lifecycle tracking, storage of the original email on cloud storage via Active Storage, and responsible data handling with on-by-default incineration. diff --git a/actionmailbox/app/controllers/action_mailbox/base_controller.rb b/actionmailbox/app/controllers/action_mailbox/base_controller.rb index 92477b86a8..80a14355b7 100644 --- a/actionmailbox/app/controllers/action_mailbox/base_controller.rb +++ b/actionmailbox/app/controllers/action_mailbox/base_controller.rb @@ -7,10 +7,6 @@ module ActionMailbox before_action :ensure_configured - def self.prepare - # Override in concrete controllers to run code on load. - end - private def ensure_configured unless ActionMailbox.ingress == ingress_name diff --git a/actionmailbox/app/controllers/action_mailbox/ingresses/amazon/inbound_emails_controller.rb b/actionmailbox/app/controllers/action_mailbox/ingresses/amazon/inbound_emails_controller.rb deleted file mode 100644 index e0a187054e..0000000000 --- a/actionmailbox/app/controllers/action_mailbox/ingresses/amazon/inbound_emails_controller.rb +++ /dev/null @@ -1,54 +0,0 @@ -# frozen_string_literal: true - -module ActionMailbox - # Ingests inbound emails from Amazon's Simple Email Service (SES). - # - # Requires the full RFC 822 message in the +content+ parameter. Authenticates requests by validating their signatures. - # - # Returns: - # - # - <tt>204 No Content</tt> if an inbound email is successfully recorded and enqueued for routing to the appropriate mailbox - # - <tt>401 Unauthorized</tt> if the request's signature could not be validated - # - <tt>404 Not Found</tt> if Action Mailbox is not configured to accept inbound emails from SES - # - <tt>422 Unprocessable Entity</tt> if the request is missing the required +content+ parameter - # - <tt>500 Server Error</tt> if one of the Active Record database, the Active Storage service, or - # the Active Job backend is misconfigured or unavailable - # - # == Usage - # - # 1. Install the {aws-sdk-sns}[https://rubygems.org/gems/aws-sdk-sns] gem: - # - # # Gemfile - # gem "aws-sdk-sns", ">= 1.9.0", require: false - # - # 2. Tell Action Mailbox to accept emails from SES: - # - # # config/environments/production.rb - # config.action_mailbox.ingress = :amazon - # - # 3. {Configure SES}[https://docs.aws.amazon.com/ses/latest/DeveloperGuide/receiving-email-notifications.html] - # to deliver emails to your application via POST requests to +/rails/action_mailbox/amazon/inbound_emails+. - # If your application lived at <tt>https://example.com</tt>, you would specify the fully-qualified URL - # <tt>https://example.com/rails/action_mailbox/amazon/inbound_emails</tt>. - class Ingresses::Amazon::InboundEmailsController < BaseController - before_action :authenticate - - cattr_accessor :verifier - - def self.prepare - self.verifier ||= begin - require "aws-sdk-sns" - Aws::SNS::MessageVerifier.new - end - end - - def create - ActionMailbox::InboundEmail.create_and_extract_message_id! params.require(:content) - end - - private - def authenticate - head :unauthorized unless verifier.authentic?(request.body) - end - end -end diff --git a/actionmailbox/config/routes.rb b/actionmailbox/config/routes.rb index 517d2835af..1496d6f0b3 100644 --- a/actionmailbox/config/routes.rb +++ b/actionmailbox/config/routes.rb @@ -2,7 +2,6 @@ Rails.application.routes.draw do scope "/rails/action_mailbox", module: "action_mailbox/ingresses" do - post "/amazon/inbound_emails" => "amazon/inbound_emails#create", as: :rails_amazon_inbound_emails post "/mandrill/inbound_emails" => "mandrill/inbound_emails#create", as: :rails_mandrill_inbound_emails post "/postmark/inbound_emails" => "postmark/inbound_emails#create", as: :rails_postmark_inbound_emails post "/relay/inbound_emails" => "relay/inbound_emails#create", as: :rails_relay_inbound_emails diff --git a/actionmailbox/lib/action_mailbox/engine.rb b/actionmailbox/lib/action_mailbox/engine.rb index 039f04ac2f..bab3964d93 100644 --- a/actionmailbox/lib/action_mailbox/engine.rb +++ b/actionmailbox/lib/action_mailbox/engine.rb @@ -26,16 +26,7 @@ module ActionMailbox ActionMailbox.incinerate = app.config.action_mailbox.incinerate.nil? ? true : app.config.action_mailbox.incinerate ActionMailbox.incinerate_after = app.config.action_mailbox.incinerate_after || 30.days ActionMailbox.queues = app.config.action_mailbox.queues || {} - end - end - - initializer "action_mailbox.ingress" do |app| - config.to_prepare do - if ActionMailbox.ingress = app.config.action_mailbox.ingress.presence - if ingress_controller_class = "ActionMailbox::Ingresses::#{ActionMailbox.ingress.to_s.classify}::InboundEmailsController".safe_constantize - ingress_controller_class.prepare - end - end + ActionMailbox.ingress = app.config.action_mailbox.ingress end end end diff --git a/actionmailbox/lib/rails/generators/installer.rb b/actionmailbox/lib/rails/generators/installer.rb index 25cf528ef5..2864ea4e62 100644 --- a/actionmailbox/lib/rails/generators/installer.rb +++ b/actionmailbox/lib/rails/generators/installer.rb @@ -5,6 +5,6 @@ copy_file "#{__dir__}/mailbox/templates/application_mailbox.rb", "app/mailboxes/ environment <<~end_of_config, env: "production" # Prepare the ingress controller used to receive mail - # config.action_mailbox.ingress = :amazon + # config.action_mailbox.ingress = :postfix end_of_config diff --git a/actionmailbox/test/controllers/ingresses/amazon/inbound_emails_controller_test.rb b/actionmailbox/test/controllers/ingresses/amazon/inbound_emails_controller_test.rb deleted file mode 100644 index e10985553e..0000000000 --- a/actionmailbox/test/controllers/ingresses/amazon/inbound_emails_controller_test.rb +++ /dev/null @@ -1,22 +0,0 @@ -# frozen_string_literal: true - -require "test_helper" - -ActionMailbox::Ingresses::Amazon::InboundEmailsController.verifier = - Module.new { def self.authentic?(message); true; end } - -class ActionMailbox::Ingresses::Amazon::InboundEmailsControllerTest < ActionDispatch::IntegrationTest - setup { ActionMailbox.ingress = :amazon } - - test "receiving an inbound email from Amazon" do - assert_difference -> { ActionMailbox::InboundEmail.count }, +1 do - post rails_amazon_inbound_emails_url, params: { content: file_fixture("../files/welcome.eml").read }, as: :json - end - - assert_response :no_content - - inbound_email = ActionMailbox::InboundEmail.last - assert_equal file_fixture("../files/welcome.eml").read, inbound_email.raw_email.download - assert_equal "0CB459E0-0336-41DA-BC88-E6E28C697DDB@37signals.com", inbound_email.message_id - end -end diff --git a/actionmailbox/test/dummy/config/environments/production.rb b/actionmailbox/test/dummy/config/environments/production.rb index 1731582220..932858fdb6 100644 --- a/actionmailbox/test/dummy/config/environments/production.rb +++ b/actionmailbox/test/dummy/config/environments/production.rb @@ -1,9 +1,4 @@ Rails.application.configure do - # Prepare the ingress controller used to receive mail - # config.action_mailbox.ingress = :amazon - - # Verifies that versions and hashed value of the package contents in the project's package.json - config.webpacker.check_yarn_integrity = false # Settings specified here will take precedence over those in config/application.rb. # Code is not reloaded between requests. @@ -96,4 +91,10 @@ Rails.application.configure do # Do not dump schema after migrations. config.active_record.dump_schema_after_migration = false + + # Prepare the ingress controller used to receive mail + # config.action_mailbox.ingress = :postfix + + # Verifies that versions and hashed value of the package contents in the project's package.json + config.webpacker.check_yarn_integrity = false end diff --git a/actionpack/lib/action_controller/metal/params_wrapper.rb b/actionpack/lib/action_controller/metal/params_wrapper.rb index 09716f7588..e635abec8e 100644 --- a/actionpack/lib/action_controller/metal/params_wrapper.rb +++ b/actionpack/lib/action_controller/metal/params_wrapper.rb @@ -93,7 +93,7 @@ module ActionController end def model - super || synchronize { super || self.model = _default_wrap_model } + super || self.model = _default_wrap_model end def include diff --git a/actionpack/lib/action_controller/metal/strong_parameters.rb b/actionpack/lib/action_controller/metal/strong_parameters.rb index 815f82a1f2..ae774b01f1 100644 --- a/actionpack/lib/action_controller/metal/strong_parameters.rb +++ b/actionpack/lib/action_controller/metal/strong_parameters.rb @@ -4,7 +4,6 @@ require "active_support/core_ext/hash/indifferent_access" require "active_support/core_ext/array/wrap" require "active_support/core_ext/string/filters" require "active_support/core_ext/object/to_query" -require "active_support/rescuable" require "action_dispatch/http/upload" require "rack/test" require "stringio" @@ -1092,9 +1091,6 @@ module ActionController # See ActionController::Parameters.require and ActionController::Parameters.permit # for more information. module StrongParameters - extend ActiveSupport::Concern - include ActiveSupport::Rescuable - # Returns a new ActionController::Parameters object that # has been instantiated with the <tt>request.parameters</tt>. def params diff --git a/actionpack/lib/action_dispatch/journey/path/pattern.rb b/actionpack/lib/action_dispatch/journey/path/pattern.rb index a968df5f19..dee2980eb1 100644 --- a/actionpack/lib/action_dispatch/journey/path/pattern.rb +++ b/actionpack/lib/action_dispatch/journey/path/pattern.rb @@ -119,7 +119,8 @@ module ActionDispatch class UnanchoredRegexp < AnchoredRegexp # :nodoc: def accept(node) - %r{\A#{visit node}(?:\b|\Z)} + path = visit node + path == "/" ? %r{\A/} : %r{\A#{path}(?:\b|\Z|/)} end end diff --git a/actionpack/lib/action_dispatch/routing/route_set.rb b/actionpack/lib/action_dispatch/routing/route_set.rb index d0a7eadf45..4a24c35efb 100644 --- a/actionpack/lib/action_dispatch/routing/route_set.rb +++ b/actionpack/lib/action_dispatch/routing/route_set.rb @@ -317,18 +317,16 @@ module ActionDispatch # def define_url_helper(mod, route, name, opts, route_key, url_strategy) helper = UrlHelper.create(route, opts, route_key, url_strategy) - mod.module_eval do - define_method(name) do |*args| - last = args.last - options = \ - case last - when Hash - args.pop - when ActionController::Parameters - args.pop.to_h - end - helper.call self, args, options - end + mod.define_method(name) do |*args| + last = args.last + options = \ + case last + when Hash + args.pop + when ActionController::Parameters + args.pop.to_h + end + helper.call self, args, options end end end diff --git a/actionpack/test/dispatch/mount_test.rb b/actionpack/test/dispatch/mount_test.rb index e42ea89f6f..758cee9930 100644 --- a/actionpack/test/dispatch/mount_test.rb +++ b/actionpack/test/dispatch/mount_test.rb @@ -27,6 +27,7 @@ class TestRoutingMount < ActionDispatch::IntegrationTest } mount SprocketsApp, at: "/sprockets" + mount SprocketsApp, at: "/star*" mount SprocketsApp => "/shorthand" mount SinatraLikeApp, at: "/fakeengine", as: :fake @@ -58,6 +59,14 @@ class TestRoutingMount < ActionDispatch::IntegrationTest def test_mounting_at_root_path get "/omg" assert_equal " -- /omg", response.body + + get "/~omg" + assert_equal " -- /~omg", response.body + end + + def test_mounting_at_path_with_non_word_character + get "/star*/omg" + assert_equal "/star* -- /omg", response.body end def test_mounting_sets_script_name diff --git a/actionpack/test/journey/path/pattern_test.rb b/actionpack/test/journey/path/pattern_test.rb index 2f39abcb92..77c19369b0 100644 --- a/actionpack/test/journey/path/pattern_test.rb +++ b/actionpack/test/journey/path/pattern_test.rb @@ -34,17 +34,17 @@ module ActionDispatch end { - "/:controller(/:action)" => %r{\A/(#{x})(?:/([^/.?]+))?(?:\b|\Z)}, - "/:controller/foo" => %r{\A/(#{x})/foo(?:\b|\Z)}, - "/:controller/:action" => %r{\A/(#{x})/([^/.?]+)(?:\b|\Z)}, - "/:controller" => %r{\A/(#{x})(?:\b|\Z)}, - "/:controller(/:action(/:id))" => %r{\A/(#{x})(?:/([^/.?]+)(?:/([^/.?]+))?)?(?:\b|\Z)}, - "/:controller/:action.xml" => %r{\A/(#{x})/([^/.?]+)\.xml(?:\b|\Z)}, - "/:controller.:format" => %r{\A/(#{x})\.([^/.?]+)(?:\b|\Z)}, - "/:controller(.:format)" => %r{\A/(#{x})(?:\.([^/.?]+))?(?:\b|\Z)}, - "/:controller/*foo" => %r{\A/(#{x})/(.+)(?:\b|\Z)}, - "/:controller/*foo/bar" => %r{\A/(#{x})/(.+)/bar(?:\b|\Z)}, - "/:foo|*bar" => %r{\A/(?:([^/.?]+)|(.+))(?:\b|\Z)}, + "/:controller(/:action)" => %r{\A/(#{x})(?:/([^/.?]+))?(?:\b|\Z|/)}, + "/:controller/foo" => %r{\A/(#{x})/foo(?:\b|\Z|/)}, + "/:controller/:action" => %r{\A/(#{x})/([^/.?]+)(?:\b|\Z|/)}, + "/:controller" => %r{\A/(#{x})(?:\b|\Z|/)}, + "/:controller(/:action(/:id))" => %r{\A/(#{x})(?:/([^/.?]+)(?:/([^/.?]+))?)?(?:\b|\Z|/)}, + "/:controller/:action.xml" => %r{\A/(#{x})/([^/.?]+)\.xml(?:\b|\Z|/)}, + "/:controller.:format" => %r{\A/(#{x})\.([^/.?]+)(?:\b|\Z|/)}, + "/:controller(.:format)" => %r{\A/(#{x})(?:\.([^/.?]+))?(?:\b|\Z|/)}, + "/:controller/*foo" => %r{\A/(#{x})/(.+)(?:\b|\Z|/)}, + "/:controller/*foo/bar" => %r{\A/(#{x})/(.+)/bar(?:\b|\Z|/)}, + "/:foo|*bar" => %r{\A/(?:([^/.?]+)|(.+))(?:\b|\Z|/)}, }.each do |path, expected| define_method(:"test_to_non_anchored_regexp_#{Regexp.escape(path)}") do path = Pattern.build( diff --git a/actionview/lib/action_view.rb b/actionview/lib/action_view.rb index f398ba0ff0..7f85bf2a5e 100644 --- a/actionview/lib/action_view.rb +++ b/actionview/lib/action_view.rb @@ -44,7 +44,7 @@ module ActionView autoload :Rendering autoload :RoutingUrlFor autoload :Template - autoload :FileTemplate + autoload :UnboundTemplate autoload :ViewPaths autoload_under "renderer" do diff --git a/actionview/lib/action_view/file_template.rb b/actionview/lib/action_view/file_template.rb deleted file mode 100644 index dea02176eb..0000000000 --- a/actionview/lib/action_view/file_template.rb +++ /dev/null @@ -1,33 +0,0 @@ -# frozen_string_literal: true - -require "action_view/template" - -module ActionView - class FileTemplate < Template - def initialize(filename, handler, details) - @filename = filename - - super(nil, filename, handler, details) - end - - def source - File.binread @filename - end - - def refresh(_) - self - end - - # Exceptions are marshalled when using the parallel test runner with DRb, so we need - # to ensure that references to the template object can be marshalled as well. This means forgoing - # the marshalling of the compiler mutex and instantiating that again on unmarshalling. - def marshal_dump # :nodoc: - [ @identifier, @handler, @compiled, @locals, @virtual_path, @updated_at, @format, @variant ] - end - - def marshal_load(array) # :nodoc: - @identifier, @handler, @compiled, @locals, @virtual_path, @updated_at, @format, @variant = *array - @compile_mutex = Mutex.new - end - end -end diff --git a/actionview/lib/action_view/template.rb b/actionview/lib/action_view/template.rb index 94f8a194a0..b80bf56c1b 100644 --- a/actionview/lib/action_view/template.rb +++ b/actionview/lib/action_view/template.rb @@ -117,13 +117,14 @@ module ActionView autoload :Handlers autoload :HTML autoload :Inline + autoload :Sources autoload :Text autoload :Types end extend Template::Handlers - attr_reader :source, :identifier, :handler, :original_encoding, :updated_at + attr_reader :identifier, :handler, :original_encoding, :updated_at attr_reader :variable, :format, :variant, :locals, :virtual_path def initialize(source, identifier, handler, format: nil, variant: nil, locals: nil, virtual_path: nil, updated_at: nil) @@ -164,6 +165,7 @@ module ActionView deprecate def formats; Array(format); end deprecate def variants=(_); end deprecate def variants; [variant]; end + deprecate def refresh(_); self; end # Returns whether the underlying handler supports streaming. If so, # a streaming buffer *may* be passed when it starts rendering. @@ -190,25 +192,6 @@ module ActionView @type ||= Types[format] end - # Receives a view object and return a template similar to self by using @virtual_path. - # - # This method is useful if you have a template object but it does not contain its source - # anymore since it was already compiled. In such cases, all you need to do is to call - # refresh passing in the view object. - # - # Notice this method raises an error if the template to be refreshed does not have a - # virtual path set (true just for inline templates). - def refresh(view) - raise "A template needs to have a virtual path in order to be refreshed" unless @virtual_path - lookup = view.lookup_context - pieces = @virtual_path.split("/") - name = pieces.pop - partial = !!name.sub!(/^_/, "") - lookup.disable_cache do - lookup.find_template(name, [ pieces.join("/") ], partial, @locals) - end - end - def short_identifier @short_identifier ||= defined?(Rails.root) ? identifier.sub("#{Rails.root}/", "") : identifier end @@ -217,6 +200,10 @@ module ActionView "#<#{self.class.name} #{short_identifier} locals=#{@locals.inspect}>" end + def source + @source.to_s + end + # This method is responsible for properly setting the encoding of the # source. Until this point, we assume that the source is BINARY data. # If no additional information is supplied, we assume the encoding is @@ -298,9 +285,6 @@ module ActionView compile(mod) end - # Just discard the source if we have a virtual path. This - # means we can get the template back. - @source = nil if @virtual_path @compiled = true end end @@ -368,12 +352,7 @@ module ActionView e.sub_template_of(self) raise e else - template = self - unless template.source - template = refresh(view) - template.encode! - end - raise Template::Error.new(template) + raise Template::Error.new(self) end end diff --git a/actionview/lib/action_view/template/handlers/erb/erubi.rb b/actionview/lib/action_view/template/handlers/erb/erubi.rb index 307b852440..e602aa117a 100644 --- a/actionview/lib/action_view/template/handlers/erb/erubi.rb +++ b/actionview/lib/action_view/template/handlers/erb/erubi.rb @@ -25,7 +25,7 @@ module ActionView src = @src view = Class.new(ActionView::Base) { include action_view_erb_handler_context._routes.url_helpers - class_eval("define_method(:_template) { |local_assigns, output_buffer| #{src} }", @filename || "(erubi)", 0) + class_eval("define_method(:_template) { |local_assigns, output_buffer| #{src} }", defined?(@filename) ? @filename : "(erubi)", 0) }.empty view._run(:_template, nil, {}, ActionView::OutputBuffer.new) end diff --git a/actionview/lib/action_view/template/resolver.rb b/actionview/lib/action_view/template/resolver.rb index d5cb3c823a..ce53eb046d 100644 --- a/actionview/lib/action_view/template/resolver.rb +++ b/actionview/lib/action_view/template/resolver.rb @@ -118,7 +118,7 @@ module ActionView locals = locals.map(&:to_s).sort!.freeze cached(key, [name, prefix, partial], details, locals) do - find_templates(name, prefix, partial, details, locals) + _find_all(name, prefix, partial, details, key, locals) end end @@ -131,6 +131,10 @@ module ActionView private + def _find_all(name, prefix, partial, details, key, locals) + find_templates(name, prefix, partial, details, locals) + end + delegate :caching?, to: :class # This is what child classes implement. No defaults are needed @@ -169,33 +173,51 @@ module ActionView else @pattern = DEFAULT_PATTERN end + @unbound_templates = Concurrent::Map.new + super() + end + + def clear_cache + @unbound_templates.clear super() end private - def find_templates(name, prefix, partial, details, locals) + def _find_all(name, prefix, partial, details, key, locals) path = Path.build(name, prefix, partial) - query(path, details, details[:formats], locals) + query(path, details, details[:formats], locals, cache: !!key) end - def query(path, details, formats, locals) + def query(path, details, formats, locals, cache:) template_paths = find_template_paths_from_details(path, details) template_paths = reject_files_external_to_app(template_paths) template_paths.map do |template| - build_template(template, path.virtual, locals) + unbound_template = + if cache + @unbound_templates.compute_if_absent([template, path.virtual]) do + build_unbound_template(template, path.virtual) + end + else + build_unbound_template(template, path.virtual) + end + + unbound_template.bind_locals(locals) end end - def build_template(template, virtual_path, locals) + def build_unbound_template(template, virtual_path) handler, format, variant = extract_handler_and_format_and_variant(template) + source = Template::Sources::File.new(template) - FileTemplate.new(File.expand_path(template), handler, + UnboundTemplate.new( + source, + template, + handler, virtual_path: virtual_path, format: format, variant: variant, - locals: locals ) end @@ -361,8 +383,8 @@ module ActionView [new(""), new("/")] end - def build_template(template, virtual_path, locals) - super(template, nil, locals) + def build_unbound_template(template, _) + super(template, nil) end def reject_files_external_to_app(files) diff --git a/actionview/lib/action_view/template/sources.rb b/actionview/lib/action_view/template/sources.rb new file mode 100644 index 0000000000..8b89535741 --- /dev/null +++ b/actionview/lib/action_view/template/sources.rb @@ -0,0 +1,13 @@ +# frozen_string_literal: true + +module ActionView + class Template + module Sources + extend ActiveSupport::Autoload + + eager_autoload do + autoload :File + end + end + end +end diff --git a/actionview/lib/action_view/template/sources/file.rb b/actionview/lib/action_view/template/sources/file.rb new file mode 100644 index 0000000000..2d3a7dec7f --- /dev/null +++ b/actionview/lib/action_view/template/sources/file.rb @@ -0,0 +1,17 @@ +# frozen_string_literal: true + +module ActionView + class Template + module Sources + class File + def initialize(filename) + @filename = filename + end + + def to_s + ::File.binread @filename + end + end + end + end +end diff --git a/actionview/lib/action_view/testing/resolvers.rb b/actionview/lib/action_view/testing/resolvers.rb index a97fb71b26..1bedf44934 100644 --- a/actionview/lib/action_view/testing/resolvers.rb +++ b/actionview/lib/action_view/testing/resolvers.rb @@ -23,7 +23,7 @@ module ActionView #:nodoc: private - def query(path, exts, _, locals) + def query(path, exts, _, locals, cache:) query = +"" EXTENSIONS.each do |ext, prefix| query << "(" << exts[ext].map { |e| e && Regexp.escape("#{prefix}#{e}") }.join("|") << "|)" @@ -47,7 +47,7 @@ module ActionView #:nodoc: end class NullResolver < PathResolver - def query(path, exts, _, locals) + def query(path, exts, _, locals, cache:) handler, format, variant = extract_handler_and_format_and_variant(path) [ActionView::Template.new("Template generated by Null Resolver", path.virtual, handler, virtual_path: path.virtual, format: format, variant: variant, locals: locals)] end diff --git a/actionview/lib/action_view/unbound_template.rb b/actionview/lib/action_view/unbound_template.rb new file mode 100644 index 0000000000..db69b6d016 --- /dev/null +++ b/actionview/lib/action_view/unbound_template.rb @@ -0,0 +1,32 @@ +# frozen_string_literal: true + +require "concurrent/map" + +module ActionView + class UnboundTemplate + def initialize(source, identifer, handler, options) + @source = source + @identifer = identifer + @handler = handler + @options = options + + @templates = Concurrent::Map.new(initial_capacity: 2) + end + + def bind_locals(locals) + @templates[locals] ||= build_template(locals) + end + + private + + def build_template(locals) + options = @options.merge(locals: locals) + Template.new( + @source, + @identifer, + @handler, + options + ) + end + end +end diff --git a/actionview/test/template/file_system_resolver_test.rb b/actionview/test/template/file_system_resolver_test.rb new file mode 100644 index 0000000000..aa03fdcb13 --- /dev/null +++ b/actionview/test/template/file_system_resolver_test.rb @@ -0,0 +1,12 @@ +# frozen_string_literal: true + +require "abstract_unit" +require "template/resolver_shared_tests" + +class FileSystemResolverTest < ActiveSupport::TestCase + include ResolverSharedTests + + def resolver + ActionView::FileSystemResolver.new(tmpdir) + end +end diff --git a/actionview/test/template/optimized_file_system_resolver_test.rb b/actionview/test/template/optimized_file_system_resolver_test.rb new file mode 100644 index 0000000000..c0c64357ce --- /dev/null +++ b/actionview/test/template/optimized_file_system_resolver_test.rb @@ -0,0 +1,12 @@ +# frozen_string_literal: true + +require "abstract_unit" +require "template/resolver_shared_tests" + +class OptimizedFileSystemResolverTest < ActiveSupport::TestCase + include ResolverSharedTests + + def resolver + ActionView::OptimizedFileSystemResolver.new(tmpdir) + end +end diff --git a/actionview/test/template/resolver_shared_tests.rb b/actionview/test/template/resolver_shared_tests.rb new file mode 100644 index 0000000000..8b47c5bc89 --- /dev/null +++ b/actionview/test/template/resolver_shared_tests.rb @@ -0,0 +1,148 @@ +# frozen_string_literal: true + +module ResolverSharedTests + attr_reader :tmpdir + + def run(*args) + capture_exceptions do + Dir.mktmpdir(nil, __dir__) { |dir| @tmpdir = dir; super } + end + end + + def with_file(filename, source = "File at #{filename}") + path = File.join(tmpdir, filename) + FileUtils.mkdir_p(File.dirname(path)) + File.write(path, source) + end + + def context + @context ||= ActionView::LookupContext.new(resolver) + end + + def test_can_find_with_no_extensions + with_file "test/hello_world", "Hello default!" + + templates = resolver.find_all("hello_world", "test", false, locale: [:en], formats: [:html], variants: [:phone], handlers: [:erb]) + assert_equal 1, templates.size + assert_equal "Hello default!", templates[0].source + assert_equal "test/hello_world", templates[0].virtual_path + assert_nil templates[0].format + assert_nil templates[0].variant + assert_kind_of ActionView::Template::Handlers::Raw, templates[0].handler + end + + def test_can_find_with_just_handler + with_file "test/hello_world.erb", "Hello erb!" + + templates = resolver.find_all("hello_world", "test", false, locale: [:en], formats: [:html], variants: [:phone], handlers: [:erb]) + assert_equal 1, templates.size + assert_equal "Hello erb!", templates[0].source + assert_equal "test/hello_world", templates[0].virtual_path + assert_nil templates[0].format + assert_nil templates[0].variant + assert_kind_of ActionView::Template::Handlers::ERB, templates[0].handler + end + + def test_can_find_with_format_and_handler + with_file "test/hello_world.text.builder", "Hello plain text!" + + templates = resolver.find_all("hello_world", "test", false, locale: [:en], formats: [:html, :text], variants: [:phone], handlers: [:erb, :builder]) + assert_equal 1, templates.size + assert_equal "Hello plain text!", templates[0].source + assert_equal "test/hello_world", templates[0].virtual_path + assert_equal :text, templates[0].format + assert_nil templates[0].variant + assert_kind_of ActionView::Template::Handlers::Builder, templates[0].handler + end + + def test_can_find_with_variant_format_and_handler + with_file "test/hello_world.html+phone.erb", "Hello plain text!" + + templates = resolver.find_all("hello_world", "test", false, locale: [:en], formats: [:html], variants: [:phone], handlers: [:erb]) + assert_equal 1, templates.size + assert_equal "Hello plain text!", templates[0].source + assert_equal "test/hello_world", templates[0].virtual_path + assert_equal :html, templates[0].format + assert_equal "phone", templates[0].variant + assert_kind_of ActionView::Template::Handlers::ERB, templates[0].handler + end + + def test_can_find_with_any_variant_format_and_handler + with_file "test/hello_world.html+phone.erb", "Hello plain text!" + + templates = resolver.find_all("hello_world", "test", false, locale: [:en], formats: [:html], variants: :any, handlers: [:erb]) + assert_equal 1, templates.size + assert_equal "Hello plain text!", templates[0].source + assert_equal "test/hello_world", templates[0].virtual_path + assert_equal :html, templates[0].format + assert_equal "phone", templates[0].variant + assert_kind_of ActionView::Template::Handlers::ERB, templates[0].handler + end + + def test_doesnt_find_template_with_wrong_details + with_file "test/hello_world.html.erb", "Hello plain text!" + + templates = resolver.find_all("hello_world", "test", false, locale: [], formats: [:xml], variants: :any, handlers: [:builder]) + assert_equal 0, templates.size + + templates = resolver.find_all("hello_world", "test", false, locale: [], formats: [:xml], variants: :any, handlers: [:erb]) + assert_equal 0, templates.size + end + + def test_found_template_is_cached + with_file "test/hello_world.html.erb", "Hello HTML!" + + a = context.find("hello_world", "test", false, [], {}) + b = context.find("hello_world", "test", false, [], {}) + assert_same a, b + end + + def test_different_templates_when_cache_disabled + with_file "test/hello_world.html.erb", "Hello HTML!" + + a = context.find("hello_world", "test", false, [], {}) + b = context.disable_cache { context.find("hello_world", "test", false, [], {}) } + c = context.find("hello_world", "test", false, [], {}) + + # disable_cache should give us a new object + assert_not_same a, b + + # but it should not clear the cache + assert_same a, c + end + + def test_same_template_from_different_details_is_same_object + with_file "test/hello_world.html.erb", "Hello HTML!" + + a = context.find("hello_world", "test", false, [], locale: [:en]) + b = context.find("hello_world", "test", false, [], locale: [:fr]) + assert_same a, b + end + + def test_templates_with_optional_locale_shares_common_object + with_file "test/hello_world.text.erb", "Generic plain text!" + with_file "test/hello_world.fr.text.erb", "Texte en Francais!" + + en = context.find_all("hello_world", "test", false, [], locale: [:en]) + fr = context.find_all("hello_world", "test", false, [], locale: [:fr]) + + assert_equal 1, en.size + assert_equal 2, fr.size + + assert_equal "Generic plain text!", en[0].source + assert_equal "Texte en Francais!", fr[0].source + assert_equal "Generic plain text!", fr[1].source + + assert_same en[0], fr[1] + end + + def test_virtual_path_is_preserved_with_dot + with_file "test/hello_world.html.erb", "Hello html!" + + template = context.find("hello_world.html", "test", false, [], {}) + assert_equal "test/hello_world.html", template.virtual_path + + template = context.find("hello_world", "test", false, [], {}) + assert_equal "test/hello_world", template.virtual_path + end +end diff --git a/actionview/test/template/template_test.rb b/actionview/test/template/template_test.rb index 71fb99115b..049e0bc129 100644 --- a/actionview/test/template/template_test.rb +++ b/actionview/test/template/template_test.rb @@ -91,10 +91,10 @@ class TestERBTemplate < ActiveSupport::TestCase assert_equal "<%= hello %>", render end - def test_template_loses_its_source_after_rendering + def test_template_does_not_lose_its_source_after_rendering @template = new_template render - assert_nil @template.source + assert_equal "<%= hello %>", @template.source end def test_template_does_not_lose_its_source_after_rendering_if_it_does_not_have_a_virtual_path @@ -121,24 +121,10 @@ class TestERBTemplate < ActiveSupport::TestCase assert_equal "hellopartialhello", render end - def test_refresh_with_templates + def test_refresh_is_deprecated @template = new_template("Hello", virtual_path: "test/foo/bar", locals: [:key]) - assert_called_with(@context.lookup_context, :find_template, ["bar", %w(test/foo), false, [:key]], returns: "template") do - assert_equal "template", @template.refresh(@context) - end - end - - def test_refresh_with_partials - @template = new_template("Hello", virtual_path: "test/_foo", locals: [:key]) - assert_called_with(@context.lookup_context, :find_template, ["foo", %w(test), true, [:key]], returns: "partial") do - assert_equal "partial", @template.refresh(@context) - end - end - - def test_refresh_raises_an_error_without_virtual_path - @template = new_template("Hello", virtual_path: nil) - assert_raise RuntimeError do - @template.refresh(@context) + assert_deprecated do + assert_same @template, @template.refresh(@context) end end diff --git a/activejob/CHANGELOG.md b/activejob/CHANGELOG.md index 138c8a8ef4..c4e21b48ac 100644 --- a/activejob/CHANGELOG.md +++ b/activejob/CHANGELOG.md @@ -1,3 +1,7 @@ +* Use individual execution counters when calculating retry delay. + + *Patrik Bóna* + * Make job argument assertions with `Time`, `ActiveSupport::TimeWithZone`, and `DateTime` work by dropping microseconds. Microsecond precision is lost during serialization. *Gannon McGibbon* diff --git a/activejob/lib/active_job/exceptions.rb b/activejob/lib/active_job/exceptions.rb index 9e00942a1c..35c1476368 100644 --- a/activejob/lib/active_job/exceptions.rb +++ b/activejob/lib/active_job/exceptions.rb @@ -54,7 +54,7 @@ module ActiveJob self.exception_executions[exceptions.to_s] = (exception_executions[exceptions.to_s] || 0) + 1 if exception_executions[exceptions.to_s] < attempts - retry_job wait: determine_delay(wait), queue: queue, priority: priority, error: error + retry_job wait: determine_delay(seconds_or_duration_or_algorithm: wait, executions: exception_executions[exceptions.to_s]), queue: queue, priority: priority, error: error else if block_given? instrument :retry_stopped, error: error do @@ -123,7 +123,7 @@ module ActiveJob end private - def determine_delay(seconds_or_duration_or_algorithm) + def determine_delay(seconds_or_duration_or_algorithm:, executions:) case seconds_or_duration_or_algorithm when :exponentially_longer (executions**4) + 2 diff --git a/activejob/test/cases/exceptions_test.rb b/activejob/test/cases/exceptions_test.rb index c88162bf58..840f4d40b5 100644 --- a/activejob/test/cases/exceptions_test.rb +++ b/activejob/test/cases/exceptions_test.rb @@ -140,6 +140,26 @@ class ExceptionsTest < ActiveSupport::TestCase ], JobBuffer.values end + test "use individual execution timers when calculating retry delay" do + travel_to Time.now + + exceptions_to_raise = %w(ExponentialWaitTenAttemptsError CustomWaitTenAttemptsError ExponentialWaitTenAttemptsError CustomWaitTenAttemptsError) + + RetryJob.perform_later exceptions_to_raise, 5, :log_scheduled_at + + assert_equal [ + "Raised ExponentialWaitTenAttemptsError for the 1st time", + "Next execution scheduled at #{(Time.now + 3.seconds).to_f}", + "Raised CustomWaitTenAttemptsError for the 2nd time", + "Next execution scheduled at #{(Time.now + 2.seconds).to_f}", + "Raised ExponentialWaitTenAttemptsError for the 3rd time", + "Next execution scheduled at #{(Time.now + 18.seconds).to_f}", + "Raised CustomWaitTenAttemptsError for the 4th time", + "Next execution scheduled at #{(Time.now + 4.seconds).to_f}", + "Successfully completed job" + ], JobBuffer.values + end + test "successfully retry job throwing one of two retryable exceptions" do RetryJob.perform_later "SecondRetryableErrorOfTwo", 3 diff --git a/activemodel/lib/active_model/attribute_methods.rb b/activemodel/lib/active_model/attribute_methods.rb index 5c4670f393..0075cd3c01 100644 --- a/activemodel/lib/active_model/attribute_methods.rb +++ b/activemodel/lib/active_model/attribute_methods.rb @@ -286,12 +286,12 @@ module ActiveModel method_name = matcher.method_name(attr_name) unless instance_method_already_implemented?(method_name) - generate_method = "define_method_#{matcher.method_missing_target}" + generate_method = "define_method_#{matcher.target}" if respond_to?(generate_method, true) send(generate_method, attr_name.to_s) else - define_proxy_call true, generated_attribute_methods, method_name, matcher.method_missing_target, attr_name.to_s + define_proxy_call true, generated_attribute_methods, method_name, matcher.target, attr_name.to_s end end end @@ -355,14 +355,14 @@ module ActiveModel # Must try to match prefixes/suffixes first, or else the matcher with no prefix/suffix # will match every time. matchers = attribute_method_matchers.partition(&:plain?).reverse.flatten(1) - matchers.map { |method| method.match(method_name) }.compact + matchers.map { |matcher| matcher.match(method_name) }.compact end end # Define a method `name` in `mod` that dispatches to `send` # using the given `extra` args. This falls back on `define_method` # and `send` if the given names cannot be compiled. - def define_proxy_call(include_private, mod, name, send, *extra) + def define_proxy_call(include_private, mod, name, target, *extra) defn = if NAME_COMPILABLE_REGEXP.match?(name) "def #{name}(*args)" else @@ -371,34 +371,34 @@ module ActiveModel extra = (extra.map!(&:inspect) << "*args").join(", ") - target = if CALL_COMPILABLE_REGEXP.match?(send) - "#{"self." unless include_private}#{send}(#{extra})" + body = if CALL_COMPILABLE_REGEXP.match?(target) + "#{"self." unless include_private}#{target}(#{extra})" else - "send(:'#{send}', #{extra})" + "send(:'#{target}', #{extra})" end mod.module_eval <<-RUBY, __FILE__, __LINE__ + 1 #{defn} - #{target} + #{body} end RUBY end class AttributeMethodMatcher #:nodoc: - attr_reader :prefix, :suffix, :method_missing_target + attr_reader :prefix, :suffix, :target - AttributeMethodMatch = Struct.new(:target, :attr_name, :method_name) + AttributeMethodMatch = Struct.new(:target, :attr_name) def initialize(options = {}) @prefix, @suffix = options.fetch(:prefix, ""), options.fetch(:suffix, "") @regex = /^(?:#{Regexp.escape(@prefix)})(.*)(?:#{Regexp.escape(@suffix)})$/ - @method_missing_target = "#{@prefix}attribute#{@suffix}" + @target = "#{@prefix}attribute#{@suffix}" @method_name = "#{prefix}%s#{suffix}" end def match(method_name) if @regex =~ method_name - AttributeMethodMatch.new(method_missing_target, $1, method_name) + AttributeMethodMatch.new(target, $1) end end diff --git a/activemodel/lib/active_model/attribute_mutation_tracker.rb b/activemodel/lib/active_model/attribute_mutation_tracker.rb index 6abf37bd44..d8cd48a53b 100644 --- a/activemodel/lib/active_model/attribute_mutation_tracker.rb +++ b/activemodel/lib/active_model/attribute_mutation_tracker.rb @@ -1,14 +1,15 @@ # frozen_string_literal: true require "active_support/core_ext/hash/indifferent_access" +require "active_support/core_ext/object/duplicable" module ActiveModel class AttributeMutationTracker # :nodoc: OPTION_NOT_GIVEN = Object.new - def initialize(attributes) + def initialize(attributes, forced_changes = Set.new) @attributes = attributes - @forced_changes = Set.new + @forced_changes = forced_changes end def changed_attribute_names @@ -18,24 +19,22 @@ module ActiveModel def changed_values attr_names.each_with_object({}.with_indifferent_access) do |attr_name, result| if changed?(attr_name) - result[attr_name] = attributes[attr_name].original_value + result[attr_name] = original_value(attr_name) end end end def changes attr_names.each_with_object({}.with_indifferent_access) do |attr_name, result| - change = change_to_attribute(attr_name) - if change + if change = change_to_attribute(attr_name) result.merge!(attr_name => change) end end end def change_to_attribute(attr_name) - attr_name = attr_name.to_s if changed?(attr_name) - [attributes[attr_name].original_value, attributes.fetch_value(attr_name)] + [original_value(attr_name), fetch_value(attr_name)] end end @@ -44,29 +43,26 @@ module ActiveModel end def changed?(attr_name, from: OPTION_NOT_GIVEN, to: OPTION_NOT_GIVEN) - attr_name = attr_name.to_s - forced_changes.include?(attr_name) || - attributes[attr_name].changed? && - (OPTION_NOT_GIVEN == from || attributes[attr_name].original_value == from) && - (OPTION_NOT_GIVEN == to || attributes[attr_name].value == to) + attribute_changed?(attr_name) && + (OPTION_NOT_GIVEN == from || original_value(attr_name) == from) && + (OPTION_NOT_GIVEN == to || fetch_value(attr_name) == to) end def changed_in_place?(attr_name) - attributes[attr_name.to_s].changed_in_place? + attributes[attr_name].changed_in_place? end def forget_change(attr_name) - attr_name = attr_name.to_s attributes[attr_name] = attributes[attr_name].forgetting_assignment forced_changes.delete(attr_name) end def original_value(attr_name) - attributes[attr_name.to_s].original_value + attributes[attr_name].original_value end def force_change(attr_name) - forced_changes << attr_name.to_s + forced_changes << attr_name end private @@ -75,45 +71,108 @@ module ActiveModel def attr_names attributes.keys end + + def attribute_changed?(attr_name) + forced_changes.include?(attr_name) || !!attributes[attr_name].changed? + end + + def fetch_value(attr_name) + attributes.fetch_value(attr_name) + end + end + + class ForcedMutationTracker < AttributeMutationTracker # :nodoc: + def initialize(attributes, forced_changes = {}) + super + @finalized_changes = nil + end + + def changed_in_place?(attr_name) + false + end + + def change_to_attribute(attr_name) + if finalized_changes&.include?(attr_name) + finalized_changes[attr_name].dup + else + super + end + end + + def forget_change(attr_name) + forced_changes.delete(attr_name) + end + + def original_value(attr_name) + if changed?(attr_name) + forced_changes[attr_name] + else + fetch_value(attr_name) + end + end + + def force_change(attr_name) + forced_changes[attr_name] = clone_value(attr_name) unless attribute_changed?(attr_name) + end + + def finalize_changes + @finalized_changes = changes + end + + private + attr_reader :finalized_changes + + def attr_names + forced_changes.keys + end + + def attribute_changed?(attr_name) + forced_changes.include?(attr_name) + end + + def fetch_value(attr_name) + attributes.send(:_read_attribute, attr_name) + end + + def clone_value(attr_name) + value = fetch_value(attr_name) + value.duplicable? ? value.clone : value + rescue TypeError, NoMethodError + value + end end class NullMutationTracker # :nodoc: include Singleton - def changed_attribute_names(*) + def changed_attribute_names [] end - def changed_values(*) + def changed_values {} end - def changes(*) + def changes {} end def change_to_attribute(attr_name) end - def any_changes?(*) + def any_changes? false end - def changed?(*) + def changed?(attr_name, **) false end - def changed_in_place?(*) + def changed_in_place?(attr_name) false end - def forget_change(*) - end - - def original_value(*) - end - - def force_change(*) + def original_value(attr_name) end end end diff --git a/activemodel/lib/active_model/attributes.rb b/activemodel/lib/active_model/attributes.rb index c3a446098c..b7b2f35bcc 100644 --- a/activemodel/lib/active_model/attributes.rb +++ b/activemodel/lib/active_model/attributes.rb @@ -91,7 +91,7 @@ module ActiveModel @attributes.fetch_value(name) end - # Handle *= for method_missing. + # Dispatch target for <tt>*=</tt> attribute methods. def attribute=(attribute_name, value) write_attribute(attribute_name, value) end diff --git a/activemodel/lib/active_model/dirty.rb b/activemodel/lib/active_model/dirty.rb index 0d9e761b1e..35a587658c 100644 --- a/activemodel/lib/active_model/dirty.rb +++ b/activemodel/lib/active_model/dirty.rb @@ -1,7 +1,5 @@ # frozen_string_literal: true -require "active_support/hash_with_indifferent_access" -require "active_support/core_ext/object/duplicable" require "active_model/attribute_mutation_tracker" module ActiveModel @@ -122,9 +120,6 @@ module ActiveModel extend ActiveSupport::Concern include ActiveModel::AttributeMethods - OPTION_NOT_GIVEN = Object.new # :nodoc: - private_constant :OPTION_NOT_GIVEN - included do attribute_method_suffix "_changed?", "_change", "_will_change!", "_was" attribute_method_suffix "_previously_changed?", "_previous_change" @@ -145,10 +140,9 @@ module ActiveModel # +mutations_from_database+ to +mutations_before_last_save+ respectively. def changes_applied unless defined?(@attributes) - @previously_changed = changes + mutations_from_database.finalize_changes end @mutations_before_last_save = mutations_from_database - @attributes_changed_by_setter = ActiveSupport::HashWithIndifferentAccess.new forget_attribute_assignments @mutations_from_database = nil end @@ -159,7 +153,7 @@ module ActiveModel # person.name = 'bob' # person.changed? # => true def changed? - changed_attributes.present? + mutations_from_database.any_changes? end # Returns an array with the name of the attributes with unsaved changes. @@ -168,42 +162,37 @@ module ActiveModel # person.name = 'bob' # person.changed # => ["name"] def changed - changed_attributes.keys + mutations_from_database.changed_attribute_names end - # Handles <tt>*_changed?</tt> for +method_missing+. - def attribute_changed?(attr, from: OPTION_NOT_GIVEN, to: OPTION_NOT_GIVEN) # :nodoc: - !!changes_include?(attr) && - (to == OPTION_NOT_GIVEN || to == _read_attribute(attr)) && - (from == OPTION_NOT_GIVEN || from == changed_attributes[attr]) + # Dispatch target for <tt>*_changed?</tt> attribute methods. + def attribute_changed?(attr_name, **options) # :nodoc: + mutations_from_database.changed?(attr_name.to_s, options) end - # Handles <tt>*_was</tt> for +method_missing+. - def attribute_was(attr) # :nodoc: - attribute_changed?(attr) ? changed_attributes[attr] : _read_attribute(attr) + # Dispatch target for <tt>*_was</tt> attribute methods. + def attribute_was(attr_name) # :nodoc: + mutations_from_database.original_value(attr_name.to_s) end - # Handles <tt>*_previously_changed?</tt> for +method_missing+. - def attribute_previously_changed?(attr) #:nodoc: - previous_changes_include?(attr) + # Dispatch target for <tt>*_previously_changed?</tt> attribute methods. + def attribute_previously_changed?(attr_name) # :nodoc: + mutations_before_last_save.changed?(attr_name.to_s) end # Restore all previous data of the provided attributes. - def restore_attributes(attributes = changed) - attributes.each { |attr| restore_attribute! attr } + def restore_attributes(attr_names = changed) + attr_names.each { |attr_name| restore_attribute!(attr_name) } end # Clears all dirty data: current changes and previous changes. def clear_changes_information - @previously_changed = ActiveSupport::HashWithIndifferentAccess.new @mutations_before_last_save = nil - @attributes_changed_by_setter = ActiveSupport::HashWithIndifferentAccess.new forget_attribute_assignments @mutations_from_database = nil end def clear_attribute_changes(attr_names) - attributes_changed_by_setter.except!(*attr_names) attr_names.each do |attr_name| clear_attribute_change(attr_name) end @@ -216,13 +205,7 @@ module ActiveModel # person.name = 'robert' # person.changed_attributes # => {"name" => "bob"} def changed_attributes - # This should only be set by methods which will call changed_attributes - # multiple times when it is known that the computed value cannot change. - if defined?(@cached_changed_attributes) - @cached_changed_attributes - else - attributes_changed_by_setter.reverse_merge(mutations_from_database.changed_values).freeze - end + mutations_from_database.changed_values end # Returns a hash of changed attributes indicating their original @@ -232,9 +215,7 @@ module ActiveModel # person.name = 'bob' # person.changes # => { "name" => ["bill", "bob"] } def changes - cache_changed_attributes do - ActiveSupport::HashWithIndifferentAccess[changed.map { |attr| [attr, attribute_change(attr)] }] - end + mutations_from_database.changes end # Returns a hash of attributes that were changed before the model was saved. @@ -244,27 +225,23 @@ module ActiveModel # person.save # person.previous_changes # => {"name" => ["bob", "robert"]} def previous_changes - @previously_changed ||= ActiveSupport::HashWithIndifferentAccess.new - @previously_changed.merge(mutations_before_last_save.changes) + mutations_before_last_save.changes end def attribute_changed_in_place?(attr_name) # :nodoc: - mutations_from_database.changed_in_place?(attr_name) + mutations_from_database.changed_in_place?(attr_name.to_s) end private def clear_attribute_change(attr_name) - mutations_from_database.forget_change(attr_name) + mutations_from_database.forget_change(attr_name.to_s) end def mutations_from_database - unless defined?(@mutations_from_database) - @mutations_from_database = nil - end @mutations_from_database ||= if defined?(@attributes) ActiveModel::AttributeMutationTracker.new(@attributes) else - NullMutationTracker.instance + ActiveModel::ForcedMutationTracker.new(self) end end @@ -276,68 +253,28 @@ module ActiveModel @mutations_before_last_save ||= ActiveModel::NullMutationTracker.instance end - def cache_changed_attributes - @cached_changed_attributes = changed_attributes - yield - ensure - clear_changed_attributes_cache - end - - def clear_changed_attributes_cache - remove_instance_variable(:@cached_changed_attributes) if defined?(@cached_changed_attributes) - end - - # Returns +true+ if attr_name is changed, +false+ otherwise. - def changes_include?(attr_name) - attributes_changed_by_setter.include?(attr_name) || mutations_from_database.changed?(attr_name) - end - alias attribute_changed_by_setter? changes_include? - - # Returns +true+ if attr_name were changed before the model was saved, - # +false+ otherwise. - def previous_changes_include?(attr_name) - previous_changes.include?(attr_name) - end - - # Handles <tt>*_change</tt> for +method_missing+. - def attribute_change(attr) - [changed_attributes[attr], _read_attribute(attr)] if attribute_changed?(attr) + # Dispatch target for <tt>*_change</tt> attribute methods. + def attribute_change(attr_name) + mutations_from_database.change_to_attribute(attr_name.to_s) end - # Handles <tt>*_previous_change</tt> for +method_missing+. - def attribute_previous_change(attr) - previous_changes[attr] + # Dispatch target for <tt>*_previous_change</tt> attribute methods. + def attribute_previous_change(attr_name) + mutations_before_last_save.change_to_attribute(attr_name.to_s) end - # Handles <tt>*_will_change!</tt> for +method_missing+. - def attribute_will_change!(attr) - unless attribute_changed?(attr) - begin - value = _read_attribute(attr) - value = value.duplicable? ? value.clone : value - rescue TypeError, NoMethodError - end - - set_attribute_was(attr, value) - end - mutations_from_database.force_change(attr) + # Dispatch target for <tt>*_will_change!</tt> attribute methods. + def attribute_will_change!(attr_name) + mutations_from_database.force_change(attr_name.to_s) end - # Handles <tt>restore_*!</tt> for +method_missing+. - def restore_attribute!(attr) - if attribute_changed?(attr) - __send__("#{attr}=", changed_attributes[attr]) - clear_attribute_changes([attr]) + # Dispatch target for <tt>restore_*!</tt> attribute methods. + def restore_attribute!(attr_name) + attr_name = attr_name.to_s + if attribute_changed?(attr_name) + __send__("#{attr_name}=", attribute_was(attr_name)) + clear_attribute_change(attr_name) end end - - def attributes_changed_by_setter - @attributes_changed_by_setter ||= ActiveSupport::HashWithIndifferentAccess.new - end - - # Force an attribute to have a particular "before" value - def set_attribute_was(attr, old_value) - attributes_changed_by_setter[attr] = old_value - end end end diff --git a/activemodel/test/cases/attribute_methods_test.rb b/activemodel/test/cases/attribute_methods_test.rb index ebb6cc542d..4e228032c3 100644 --- a/activemodel/test/cases/attribute_methods_test.rb +++ b/activemodel/test/cases/attribute_methods_test.rb @@ -264,6 +264,5 @@ class AttributeMethodsTest < ActiveModel::TestCase assert_equal "foo", match.attr_name assert_equal "attribute_test", match.target - assert_equal "foo_test", match.method_name end end diff --git a/activemodel/test/cases/validations/acceptance_validation_test.rb b/activemodel/test/cases/validations/acceptance_validation_test.rb index 7662f996ae..72baf6e7a7 100644 --- a/activemodel/test/cases/validations/acceptance_validation_test.rb +++ b/activemodel/test/cases/validations/acceptance_validation_test.rb @@ -7,21 +7,23 @@ require "models/reply" require "models/person" class AcceptanceValidationTest < ActiveModel::TestCase - def teardown - Topic.clear_validators! + teardown do + self.class.send(:remove_const, :TestClass) end def test_terms_of_service_agreement_no_acceptance - Topic.validates_acceptance_of(:terms_of_service) + klass = define_test_class(Topic) + klass.validates_acceptance_of(:terms_of_service) - t = Topic.new("title" => "We should not be confirmed") + t = klass.new("title" => "We should not be confirmed") assert_predicate t, :valid? end def test_terms_of_service_agreement - Topic.validates_acceptance_of(:terms_of_service) + klass = define_test_class(Topic) + klass.validates_acceptance_of(:terms_of_service) - t = Topic.new("title" => "We should be confirmed", "terms_of_service" => "") + t = klass.new("title" => "We should be confirmed", "terms_of_service" => "") assert_predicate t, :invalid? assert_equal ["must be accepted"], t.errors[:terms_of_service] @@ -30,9 +32,10 @@ class AcceptanceValidationTest < ActiveModel::TestCase end def test_eula - Topic.validates_acceptance_of(:eula, message: "must be abided") + klass = define_test_class(Topic) + klass.validates_acceptance_of(:eula, message: "must be abided") - t = Topic.new("title" => "We should be confirmed", "eula" => "") + t = klass.new("title" => "We should be confirmed", "eula" => "") assert_predicate t, :invalid? assert_equal ["must be abided"], t.errors[:eula] @@ -41,9 +44,10 @@ class AcceptanceValidationTest < ActiveModel::TestCase end def test_terms_of_service_agreement_with_accept_value - Topic.validates_acceptance_of(:terms_of_service, accept: "I agree.") + klass = define_test_class(Topic) + klass.validates_acceptance_of(:terms_of_service, accept: "I agree.") - t = Topic.new("title" => "We should be confirmed", "terms_of_service" => "") + t = klass.new("title" => "We should be confirmed", "terms_of_service" => "") assert_predicate t, :invalid? assert_equal ["must be accepted"], t.errors[:terms_of_service] @@ -52,9 +56,10 @@ class AcceptanceValidationTest < ActiveModel::TestCase end def test_terms_of_service_agreement_with_multiple_accept_values - Topic.validates_acceptance_of(:terms_of_service, accept: [1, "I concur."]) + klass = define_test_class(Topic) + klass.validates_acceptance_of(:terms_of_service, accept: [1, "I concur."]) - t = Topic.new("title" => "We should be confirmed", "terms_of_service" => "") + t = klass.new("title" => "We should be confirmed", "terms_of_service" => "") assert_predicate t, :invalid? assert_equal ["must be accepted"], t.errors[:terms_of_service] @@ -66,9 +71,10 @@ class AcceptanceValidationTest < ActiveModel::TestCase end def test_validates_acceptance_of_for_ruby_class - Person.validates_acceptance_of :karma + klass = define_test_class(Person) + klass.validates_acceptance_of :karma - p = Person.new + p = klass.new p.karma = "" assert_predicate p, :invalid? @@ -76,13 +82,21 @@ class AcceptanceValidationTest < ActiveModel::TestCase p.karma = "1" assert_predicate p, :valid? - ensure - Person.clear_validators! end def test_validates_acceptance_of_true - Topic.validates_acceptance_of(:terms_of_service) + klass = define_test_class(Topic) + klass.validates_acceptance_of(:terms_of_service) - assert_predicate Topic.new(terms_of_service: true), :valid? + assert_predicate klass.new(terms_of_service: true), :valid? end + + private + + # Acceptance validator includes anonymous module into class, which cannot + # be cleared, so to avoid multiple inclusions we use a named subclass which + # we can remove in teardown. + def define_test_class(parent) + self.class.const_set(:TestClass, Class.new(parent)) + end end diff --git a/activemodel/test/cases/validations/i18n_validation_test.rb b/activemodel/test/cases/validations/i18n_validation_test.rb index eb03e837f1..35bb918f26 100644 --- a/activemodel/test/cases/validations/i18n_validation_test.rb +++ b/activemodel/test/cases/validations/i18n_validation_test.rb @@ -6,7 +6,7 @@ require "models/person" class I18nValidationTest < ActiveModel::TestCase def setup Person.clear_validators! - @person = Person.new + @person = person_class.new @old_load_path, @old_backend = I18n.load_path.dup, I18n.backend I18n.load_path.clear @@ -18,7 +18,9 @@ class I18nValidationTest < ActiveModel::TestCase end def teardown - Person.clear_validators! + person_class.clear_validators! + self.class.send(:remove_const, :Person) + @person_stub = nil I18n.load_path.replace @old_load_path I18n.backend = @old_backend I18n.backend.reload! @@ -28,14 +30,14 @@ class I18nValidationTest < ActiveModel::TestCase def test_full_message_encoding I18n.backend.store_translations("en", errors: { messages: { too_short: "猫舌" } }) - Person.validates_length_of :title, within: 3..5 + person_class.validates_length_of :title, within: 3..5 @person.valid? assert_equal ["Title 猫舌"], @person.errors.full_messages end def test_errors_full_messages_translates_human_attribute_name_for_model_attributes @person.errors.add(:name, "not found") - assert_called_with(Person, :human_attribute_name, ["name", default: "Name"], returns: "Person's name") do + assert_called_with(person_class, :human_attribute_name, ["name", default: "Name"], returns: "Person's name") do assert_equal ["Person's name not found"], @person.errors.full_messages end end @@ -52,7 +54,7 @@ class I18nValidationTest < ActiveModel::TestCase I18n.backend.store_translations("en", activemodel: { errors: { models: { person: { attributes: { name: { format: "%{message}" } } } } } }) - person = Person.new + person = person_class.new assert_equal "Name cannot be blank", person.errors.full_message(:name, "cannot be blank") assert_equal "Name test cannot be blank", person.errors.full_message(:name_test, "cannot be blank") end @@ -63,7 +65,7 @@ class I18nValidationTest < ActiveModel::TestCase I18n.backend.store_translations("en", activemodel: { errors: { models: { person: { attributes: { name: { format: "%{message}" } } } } } }) - person = Person.new + person = person_class.new assert_equal "cannot be blank", person.errors.full_message(:name, "cannot be blank") assert_equal "Name test cannot be blank", person.errors.full_message(:name_test, "cannot be blank") end @@ -74,7 +76,7 @@ class I18nValidationTest < ActiveModel::TestCase I18n.backend.store_translations("en", activemodel: { errors: { models: { person: { format: "%{message}" } } } }) - person = Person.new + person = person_class.new assert_equal "cannot be blank", person.errors.full_message(:name, "cannot be blank") assert_equal "cannot be blank", person.errors.full_message(:name_test, "cannot be blank") end @@ -85,7 +87,7 @@ class I18nValidationTest < ActiveModel::TestCase I18n.backend.store_translations("en", activemodel: { errors: { models: { 'person/contacts/addresses': { attributes: { street: { format: "%{message}" } } } } } }) - person = Person.new + person = person_class.new assert_equal "cannot be blank", person.errors.full_message(:'contacts/addresses.street', "cannot be blank") assert_equal "Contacts/addresses country cannot be blank", person.errors.full_message(:'contacts/addresses.country', "cannot be blank") end @@ -96,7 +98,7 @@ class I18nValidationTest < ActiveModel::TestCase I18n.backend.store_translations("en", activemodel: { errors: { models: { 'person/contacts/addresses': { format: "%{message}" } } } }) - person = Person.new + person = person_class.new assert_equal "cannot be blank", person.errors.full_message(:'contacts/addresses.street', "cannot be blank") assert_equal "cannot be blank", person.errors.full_message(:'contacts/addresses.country', "cannot be blank") end @@ -107,7 +109,7 @@ class I18nValidationTest < ActiveModel::TestCase I18n.backend.store_translations("en", activemodel: { errors: { models: { 'person/contacts/addresses': { attributes: { street: { format: "%{message}" } } } } } }) - person = Person.new + person = person_class.new assert_equal "cannot be blank", person.errors.full_message(:'contacts[0]/addresses[0].street', "cannot be blank") assert_equal "Contacts/addresses country cannot be blank", person.errors.full_message(:'contacts[0]/addresses[0].country', "cannot be blank") end @@ -118,7 +120,7 @@ class I18nValidationTest < ActiveModel::TestCase I18n.backend.store_translations("en", activemodel: { errors: { models: { 'person/contacts/addresses': { format: "%{message}" } } } }) - person = Person.new + person = person_class.new assert_equal "cannot be blank", person.errors.full_message(:'contacts[0]/addresses[0].street', "cannot be blank") assert_equal "cannot be blank", person.errors.full_message(:'contacts[0]/addresses[0].country', "cannot be blank") end @@ -130,7 +132,7 @@ class I18nValidationTest < ActiveModel::TestCase attributes: { 'person/contacts/addresses': { country: "Country" } } }) - person = Person.new + person = person_class.new assert_equal "Contacts/addresses street cannot be blank", person.errors.full_message(:'contacts[0]/addresses[0].street', "cannot be blank") assert_equal "Country cannot be blank", person.errors.full_message(:'contacts[0]/addresses[0].country', "cannot be blank") end @@ -141,7 +143,7 @@ class I18nValidationTest < ActiveModel::TestCase I18n.backend.store_translations("en", activemodel: { errors: { models: { 'person/contacts/addresses': { attributes: { street: { format: "%{message}" } } } } } }) - person = Person.new + person = person_class.new assert_equal "Contacts[0]/addresses[0] street cannot be blank", person.errors.full_message(:'contacts[0]/addresses[0].street', "cannot be blank") assert_equal "Contacts[0]/addresses[0] country cannot be blank", person.errors.full_message(:'contacts[0]/addresses[0].country', "cannot be blank") end @@ -153,7 +155,7 @@ class I18nValidationTest < ActiveModel::TestCase attributes: { 'person/contacts[0]/addresses[0]': { country: "Country" } } }) - person = Person.new + person = person_class.new assert_equal "Contacts[0]/addresses[0] street cannot be blank", person.errors.full_message(:'contacts[0]/addresses[0].street', "cannot be blank") assert_equal "Country cannot be blank", person.errors.full_message(:'contacts[0]/addresses[0].country', "cannot be blank") end @@ -174,7 +176,7 @@ class I18nValidationTest < ActiveModel::TestCase COMMON_CASES.each do |name, validation_options, generate_message_options| test "validates_confirmation_of on generated message #{name}" do - Person.validates_confirmation_of :title, validation_options + person_class.validates_confirmation_of :title, validation_options @person.title_confirmation = "foo" call = [:title_confirmation, :confirmation, generate_message_options.merge(attribute: "Title")] assert_called_with(@person.errors, :generate_message, call) do @@ -185,7 +187,7 @@ class I18nValidationTest < ActiveModel::TestCase COMMON_CASES.each do |name, validation_options, generate_message_options| test "validates_acceptance_of on generated message #{name}" do - Person.validates_acceptance_of :title, validation_options.merge(allow_nil: false) + person_class.validates_acceptance_of :title, validation_options.merge(allow_nil: false) call = [:title, :accepted, generate_message_options] assert_called_with(@person.errors, :generate_message, call) do @person.valid? @@ -195,7 +197,7 @@ class I18nValidationTest < ActiveModel::TestCase COMMON_CASES.each do |name, validation_options, generate_message_options| test "validates_presence_of on generated message #{name}" do - Person.validates_presence_of :title, validation_options + person_class.validates_presence_of :title, validation_options call = [:title, :blank, generate_message_options] assert_called_with(@person.errors, :generate_message, call) do @person.valid? @@ -205,7 +207,7 @@ class I18nValidationTest < ActiveModel::TestCase COMMON_CASES.each do |name, validation_options, generate_message_options| test "validates_length_of for :within on generated message when too short #{name}" do - Person.validates_length_of :title, validation_options.merge(within: 3..5) + person_class.validates_length_of :title, validation_options.merge(within: 3..5) call = [:title, :too_short, generate_message_options.merge(count: 3)] assert_called_with(@person.errors, :generate_message, call) do @person.valid? @@ -215,7 +217,7 @@ class I18nValidationTest < ActiveModel::TestCase COMMON_CASES.each do |name, validation_options, generate_message_options| test "validates_length_of for :too_long generated message #{name}" do - Person.validates_length_of :title, validation_options.merge(within: 3..5) + person_class.validates_length_of :title, validation_options.merge(within: 3..5) @person.title = "this title is too long" call = [:title, :too_long, generate_message_options.merge(count: 5)] assert_called_with(@person.errors, :generate_message, call) do @@ -226,7 +228,7 @@ class I18nValidationTest < ActiveModel::TestCase COMMON_CASES.each do |name, validation_options, generate_message_options| test "validates_length_of for :is on generated message #{name}" do - Person.validates_length_of :title, validation_options.merge(is: 5) + person_class.validates_length_of :title, validation_options.merge(is: 5) call = [:title, :wrong_length, generate_message_options.merge(count: 5)] assert_called_with(@person.errors, :generate_message, call) do @person.valid? @@ -236,7 +238,7 @@ class I18nValidationTest < ActiveModel::TestCase COMMON_CASES.each do |name, validation_options, generate_message_options| test "validates_format_of on generated message #{name}" do - Person.validates_format_of :title, validation_options.merge(with: /\A[1-9][0-9]*\z/) + person_class.validates_format_of :title, validation_options.merge(with: /\A[1-9][0-9]*\z/) @person.title = "72x" call = [:title, :invalid, generate_message_options.merge(value: "72x")] assert_called_with(@person.errors, :generate_message, call) do @@ -247,7 +249,7 @@ class I18nValidationTest < ActiveModel::TestCase COMMON_CASES.each do |name, validation_options, generate_message_options| test "validates_inclusion_of on generated message #{name}" do - Person.validates_inclusion_of :title, validation_options.merge(in: %w(a b c)) + person_class.validates_inclusion_of :title, validation_options.merge(in: %w(a b c)) @person.title = "z" call = [:title, :inclusion, generate_message_options.merge(value: "z")] assert_called_with(@person.errors, :generate_message, call) do @@ -258,7 +260,7 @@ class I18nValidationTest < ActiveModel::TestCase COMMON_CASES.each do |name, validation_options, generate_message_options| test "validates_inclusion_of using :within on generated message #{name}" do - Person.validates_inclusion_of :title, validation_options.merge(within: %w(a b c)) + person_class.validates_inclusion_of :title, validation_options.merge(within: %w(a b c)) @person.title = "z" call = [:title, :inclusion, generate_message_options.merge(value: "z")] assert_called_with(@person.errors, :generate_message, call) do @@ -269,7 +271,7 @@ class I18nValidationTest < ActiveModel::TestCase COMMON_CASES.each do |name, validation_options, generate_message_options| test "validates_exclusion_of generated message #{name}" do - Person.validates_exclusion_of :title, validation_options.merge(in: %w(a b c)) + person_class.validates_exclusion_of :title, validation_options.merge(in: %w(a b c)) @person.title = "a" call = [:title, :exclusion, generate_message_options.merge(value: "a")] assert_called_with(@person.errors, :generate_message, call) do @@ -280,7 +282,7 @@ class I18nValidationTest < ActiveModel::TestCase COMMON_CASES.each do |name, validation_options, generate_message_options| test "validates_exclusion_of using :within generated message #{name}" do - Person.validates_exclusion_of :title, validation_options.merge(within: %w(a b c)) + person_class.validates_exclusion_of :title, validation_options.merge(within: %w(a b c)) @person.title = "a" call = [:title, :exclusion, generate_message_options.merge(value: "a")] assert_called_with(@person.errors, :generate_message, call) do @@ -291,7 +293,7 @@ class I18nValidationTest < ActiveModel::TestCase COMMON_CASES.each do |name, validation_options, generate_message_options| test "validates_numericality_of generated message #{name}" do - Person.validates_numericality_of :title, validation_options + person_class.validates_numericality_of :title, validation_options @person.title = "a" call = [:title, :not_a_number, generate_message_options.merge(value: "a")] assert_called_with(@person.errors, :generate_message, call) do @@ -302,7 +304,7 @@ class I18nValidationTest < ActiveModel::TestCase COMMON_CASES.each do |name, validation_options, generate_message_options| test "validates_numericality_of for :only_integer on generated message #{name}" do - Person.validates_numericality_of :title, validation_options.merge(only_integer: true) + person_class.validates_numericality_of :title, validation_options.merge(only_integer: true) @person.title = "0.0" call = [:title, :not_an_integer, generate_message_options.merge(value: "0.0")] assert_called_with(@person.errors, :generate_message, call) do @@ -313,7 +315,7 @@ class I18nValidationTest < ActiveModel::TestCase COMMON_CASES.each do |name, validation_options, generate_message_options| test "validates_numericality_of for :odd on generated message #{name}" do - Person.validates_numericality_of :title, validation_options.merge(only_integer: true, odd: true) + person_class.validates_numericality_of :title, validation_options.merge(only_integer: true, odd: true) @person.title = 0 call = [:title, :odd, generate_message_options.merge(value: 0)] assert_called_with(@person.errors, :generate_message, call) do @@ -324,7 +326,7 @@ class I18nValidationTest < ActiveModel::TestCase COMMON_CASES.each do |name, validation_options, generate_message_options| test "validates_numericality_of for :less_than on generated message #{name}" do - Person.validates_numericality_of :title, validation_options.merge(only_integer: true, less_than: 0) + person_class.validates_numericality_of :title, validation_options.merge(only_integer: true, less_than: 0) @person.title = 1 call = [:title, :less_than, generate_message_options.merge(value: 1, count: 0)] assert_called_with(@person.errors, :generate_message, call) do @@ -369,67 +371,67 @@ class I18nValidationTest < ActiveModel::TestCase end set_expectations_for_validation "validates_confirmation_of", :confirmation do |person, options_to_merge| - Person.validates_confirmation_of :title, options_to_merge + person.class.validates_confirmation_of :title, options_to_merge person.title_confirmation = "foo" end set_expectations_for_validation "validates_acceptance_of", :accepted do |person, options_to_merge| - Person.validates_acceptance_of :title, options_to_merge.merge(allow_nil: false) + person.class.validates_acceptance_of :title, options_to_merge.merge(allow_nil: false) end set_expectations_for_validation "validates_presence_of", :blank do |person, options_to_merge| - Person.validates_presence_of :title, options_to_merge + person.class.validates_presence_of :title, options_to_merge end set_expectations_for_validation "validates_length_of", :too_short do |person, options_to_merge| - Person.validates_length_of :title, options_to_merge.merge(within: 3..5) + person.class.validates_length_of :title, options_to_merge.merge(within: 3..5) end set_expectations_for_validation "validates_length_of", :too_long do |person, options_to_merge| - Person.validates_length_of :title, options_to_merge.merge(within: 3..5) + person.class.validates_length_of :title, options_to_merge.merge(within: 3..5) person.title = "too long" end set_expectations_for_validation "validates_length_of", :wrong_length do |person, options_to_merge| - Person.validates_length_of :title, options_to_merge.merge(is: 5) + person.class.validates_length_of :title, options_to_merge.merge(is: 5) end set_expectations_for_validation "validates_format_of", :invalid do |person, options_to_merge| - Person.validates_format_of :title, options_to_merge.merge(with: /\A[1-9][0-9]*\z/) + person.class.validates_format_of :title, options_to_merge.merge(with: /\A[1-9][0-9]*\z/) end set_expectations_for_validation "validates_inclusion_of", :inclusion do |person, options_to_merge| - Person.validates_inclusion_of :title, options_to_merge.merge(in: %w(a b c)) + person.class.validates_inclusion_of :title, options_to_merge.merge(in: %w(a b c)) end set_expectations_for_validation "validates_exclusion_of", :exclusion do |person, options_to_merge| - Person.validates_exclusion_of :title, options_to_merge.merge(in: %w(a b c)) + person.class.validates_exclusion_of :title, options_to_merge.merge(in: %w(a b c)) person.title = "a" end set_expectations_for_validation "validates_numericality_of", :not_a_number do |person, options_to_merge| - Person.validates_numericality_of :title, options_to_merge + person.class.validates_numericality_of :title, options_to_merge person.title = "a" end set_expectations_for_validation "validates_numericality_of", :not_an_integer do |person, options_to_merge| - Person.validates_numericality_of :title, options_to_merge.merge(only_integer: true) + person.class.validates_numericality_of :title, options_to_merge.merge(only_integer: true) person.title = "1.0" end set_expectations_for_validation "validates_numericality_of", :odd do |person, options_to_merge| - Person.validates_numericality_of :title, options_to_merge.merge(only_integer: true, odd: true) + person.class.validates_numericality_of :title, options_to_merge.merge(only_integer: true, odd: true) person.title = 0 end set_expectations_for_validation "validates_numericality_of", :less_than do |person, options_to_merge| - Person.validates_numericality_of :title, options_to_merge.merge(only_integer: true, less_than: 0) + person.class.validates_numericality_of :title, options_to_merge.merge(only_integer: true, less_than: 0) person.title = 1 end def test_validations_with_message_symbol_must_translate I18n.backend.store_translations "en", errors: { messages: { custom_error: "I am a custom error" } } - Person.validates_presence_of :title, message: :custom_error + person_class.validates_presence_of :title, message: :custom_error @person.title = nil @person.valid? assert_equal ["I am a custom error"], @person.errors[:title] @@ -437,7 +439,7 @@ class I18nValidationTest < ActiveModel::TestCase def test_validates_with_message_symbol_must_translate_per_attribute I18n.backend.store_translations "en", activemodel: { errors: { models: { person: { attributes: { title: { custom_error: "I am a custom error" } } } } } } - Person.validates_presence_of :title, message: :custom_error + person_class.validates_presence_of :title, message: :custom_error @person.title = nil @person.valid? assert_equal ["I am a custom error"], @person.errors[:title] @@ -445,16 +447,20 @@ class I18nValidationTest < ActiveModel::TestCase def test_validates_with_message_symbol_must_translate_per_model I18n.backend.store_translations "en", activemodel: { errors: { models: { person: { custom_error: "I am a custom error" } } } } - Person.validates_presence_of :title, message: :custom_error + person_class.validates_presence_of :title, message: :custom_error @person.title = nil @person.valid? assert_equal ["I am a custom error"], @person.errors[:title] end def test_validates_with_message_string - Person.validates_presence_of :title, message: "I am a custom error" + person_class.validates_presence_of :title, message: "I am a custom error" @person.title = nil @person.valid? assert_equal ["I am a custom error"], @person.errors[:title] end + + def person_class + @person_stub ||= self.class.const_set(:Person, Class.new(Person)) + end end diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md index 1cf3f3352f..8daa6c0ce5 100644 --- a/activerecord/CHANGELOG.md +++ b/activerecord/CHANGELOG.md @@ -1,3 +1,20 @@ +* Fix dirty tracking for `touch` to track saved changes. + + Fixes #33429. + + *Ryuta Kamzono* + +* `change_column_comment` and `change_table_comment` are invertible only if + `to` and `from` options are specified. + + *Yoshiyuki Kinjo* + +* Don't call commit/rollback callbacks when a record isn't saved. + + Fixes #29747. + + *Ryuta Kamizono* + * Fix circular `autosave: true` causes invalid records to be saved. Prior to the fix, when there was a circular series of `autosave: true` diff --git a/activerecord/lib/active_record/associations/builder/association.rb b/activerecord/lib/active_record/associations/builder/association.rb index 7c69cd65ee..3b4b243148 100644 --- a/activerecord/lib/active_record/associations/builder/association.rb +++ b/activerecord/lib/active_record/associations/builder/association.rb @@ -27,40 +27,32 @@ module ActiveRecord::Associations::Builder # :nodoc: "Please choose a different association name." end - extension = define_extensions model, name, &block - reflection = create_reflection model, name, scope, options, extension + reflection = create_reflection(model, name, scope, options, &block) define_accessors model, reflection define_callbacks model, reflection define_validations model, reflection reflection end - def self.create_reflection(model, name, scope, options, extension = nil) + def self.create_reflection(model, name, scope, options, &block) raise ArgumentError, "association names must be a Symbol" unless name.kind_of?(Symbol) validate_options(options) - scope = build_scope(scope, extension) + extension = define_extensions(model, name, &block) + options[:extend] = [*options[:extend], extension] if extension + + scope = build_scope(scope) ActiveRecord::Reflection.create(macro, name, scope, options, model) end - def self.build_scope(scope, extension) - new_scope = scope - + def self.build_scope(scope) if scope && scope.arity == 0 - new_scope = proc { instance_exec(&scope) } - end - - if extension - new_scope = wrap_scope new_scope, extension + proc { instance_exec(&scope) } + else + scope end - - new_scope - end - - def self.wrap_scope(scope, extension) - scope end def self.macro diff --git a/activerecord/lib/active_record/associations/builder/collection_association.rb b/activerecord/lib/active_record/associations/builder/collection_association.rb index 5848cd9112..9fccfcce0c 100644 --- a/activerecord/lib/active_record/associations/builder/collection_association.rb +++ b/activerecord/lib/active_record/associations/builder/collection_association.rb @@ -66,17 +66,5 @@ module ActiveRecord::Associations::Builder # :nodoc: end CODE end - - def self.wrap_scope(scope, mod) - if scope - if scope.arity > 0 - proc { |owner| instance_exec(owner, &scope).extending(mod) } - else - proc { instance_exec(&scope).extending(mod) } - end - else - proc { extending(mod) } - end - end end end diff --git a/activerecord/lib/active_record/attribute_methods/before_type_cast.rb b/activerecord/lib/active_record/attribute_methods/before_type_cast.rb index 5941f51a1a..dc239ff9ea 100644 --- a/activerecord/lib/active_record/attribute_methods/before_type_cast.rb +++ b/activerecord/lib/active_record/attribute_methods/before_type_cast.rb @@ -65,7 +65,7 @@ module ActiveRecord private - # Handle *_before_type_cast for method_missing. + # Dispatch target for <tt>*_before_type_cast</tt> attribute methods. def attribute_before_type_cast(attribute_name) read_attribute_before_type_cast(attribute_name) end diff --git a/activerecord/lib/active_record/attribute_methods/dirty.rb b/activerecord/lib/active_record/attribute_methods/dirty.rb index 45e4b8adfa..68ac8475b0 100644 --- a/activerecord/lib/active_record/attribute_methods/dirty.rb +++ b/activerecord/lib/active_record/attribute_methods/dirty.rb @@ -29,9 +29,7 @@ module ActiveRecord # <tt>reload</tt> the record and clears changed attributes. def reload(*) super.tap do - @previously_changed = ActiveSupport::HashWithIndifferentAccess.new @mutations_before_last_save = nil - @attributes_changed_by_setter = ActiveSupport::HashWithIndifferentAccess.new @mutations_from_database = nil end end @@ -51,7 +49,7 @@ module ActiveRecord # +to+ When passed, this method will return false unless the value was # changed to the given value def saved_change_to_attribute?(attr_name, **options) - mutations_before_last_save.changed?(attr_name, **options) + mutations_before_last_save.changed?(attr_name.to_s, options) end # Returns the change to an attribute during the last save. If the @@ -63,7 +61,7 @@ module ActiveRecord # invoked as +saved_change_to_name+ instead of # <tt>saved_change_to_attribute("name")</tt>. def saved_change_to_attribute(attr_name) - mutations_before_last_save.change_to_attribute(attr_name) + mutations_before_last_save.change_to_attribute(attr_name.to_s) end # Returns the original value of an attribute before the last save. @@ -73,7 +71,7 @@ module ActiveRecord # invoked as +name_before_last_save+ instead of # <tt>attribute_before_last_save("name")</tt>. def attribute_before_last_save(attr_name) - mutations_before_last_save.original_value(attr_name) + mutations_before_last_save.original_value(attr_name.to_s) end # Did the last call to +save+ have any changes to change? @@ -101,7 +99,7 @@ module ActiveRecord # +to+ When passed, this method will return false unless the value will be # changed to the given value def will_save_change_to_attribute?(attr_name, **options) - mutations_from_database.changed?(attr_name, **options) + mutations_from_database.changed?(attr_name.to_s, options) end # Returns the change to an attribute that will be persisted during the @@ -115,7 +113,7 @@ module ActiveRecord # If the attribute will change, the result will be an array containing the # original value and the new value about to be saved. def attribute_change_to_be_saved(attr_name) - mutations_from_database.change_to_attribute(attr_name) + mutations_from_database.change_to_attribute(attr_name.to_s) end # Returns the value of an attribute in the database, as opposed to the @@ -127,7 +125,7 @@ module ActiveRecord # saved. It can be invoked as +name_in_database+ instead of # <tt>attribute_in_database("name")</tt>. def attribute_in_database(attr_name) - mutations_from_database.original_value(attr_name) + mutations_from_database.original_value(attr_name.to_s) end # Will the next call to +save+ have any changes to persist? @@ -168,6 +166,30 @@ module ActiveRecord result end + def _touch_row(attribute_names, time) + @_touch_attr_names = Set.new(attribute_names) + + affected_rows = super + + changes = {} + @attributes.keys.each do |attr_name| + next if @_touch_attr_names.include?(attr_name) + + if attribute_changed?(attr_name) + changes[attr_name] = _read_attribute(attr_name) + _write_attribute(attr_name, attribute_was(attr_name)) + clear_attribute_change(attr_name) + end + end + + changes_applied + changes.each { |attr_name, value| _write_attribute(attr_name, value) } + + affected_rows + ensure + @_touch_attr_names = nil + end + def _update_record(attribute_names = attribute_names_for_partial_writes) affected_rows = super changes_applied diff --git a/activerecord/lib/active_record/attribute_methods/query.rb b/activerecord/lib/active_record/attribute_methods/query.rb index 6811f54b10..0cf67644af 100644 --- a/activerecord/lib/active_record/attribute_methods/query.rb +++ b/activerecord/lib/active_record/attribute_methods/query.rb @@ -32,7 +32,7 @@ module ActiveRecord end private - # Handle *? for method_missing. + # Dispatch target for <tt>*?</tt> attribute methods. def attribute?(attribute_name) query_attribute(attribute_name) end diff --git a/activerecord/lib/active_record/attribute_methods/write.rb b/activerecord/lib/active_record/attribute_methods/write.rb index 455e67e19b..d5ba2f42cb 100644 --- a/activerecord/lib/active_record/attribute_methods/write.rb +++ b/activerecord/lib/active_record/attribute_methods/write.rb @@ -58,7 +58,7 @@ module ActiveRecord value end - # Handle *= for method_missing. + # Dispatch target for <tt>*=</tt> attribute methods. def attribute=(attribute_name, value) _write_attribute(attribute_name, value) end diff --git a/activerecord/lib/active_record/connection_adapters/abstract/schema_statements.rb b/activerecord/lib/active_record/connection_adapters/abstract/schema_statements.rb index 4840307094..7041d92586 100644 --- a/activerecord/lib/active_record/connection_adapters/abstract/schema_statements.rb +++ b/activerecord/lib/active_record/connection_adapters/abstract/schema_statements.rb @@ -1185,12 +1185,22 @@ module ActiveRecord end # Changes the comment for a table or removes it if +nil+. - def change_table_comment(table_name, comment) + # + # Passing a hash containing +:from+ and +:to+ will make this change + # reversible in migration: + # + # change_table_comment(:posts, from: "old_comment", to: "new_comment") + def change_table_comment(table_name, comment_or_changes) raise NotImplementedError, "#{self.class} does not support changing table comments" end # Changes the comment for a column or removes it if +nil+. - def change_column_comment(table_name, column_name, comment) + # + # Passing a hash containing +:from+ and +:to+ will make this change + # reversible in migration: + # + # change_column_comment(:posts, :state, from: "old_comment", to: "new_comment") + def change_column_comment(table_name, column_name, comment_or_changes) raise NotImplementedError, "#{self.class} does not support changing column comments" end @@ -1374,11 +1384,37 @@ module ActiveRecord default_or_changes end end + alias :extract_new_comment_value :extract_new_default_value def can_remove_index_by_name?(options) options.is_a?(Hash) && options.key?(:name) && options.except(:name, :algorithm).empty? end + def bulk_change_table(table_name, operations) + sql_fragments = [] + non_combinable_operations = [] + + operations.each do |command, args| + table, arguments = args.shift, args + method = :"#{command}_for_alter" + + if respond_to?(method, true) + sqls, procs = Array(send(method, table, *arguments)).partition { |v| v.is_a?(String) } + sql_fragments << sqls + non_combinable_operations.concat(procs) + else + execute "ALTER TABLE #{quote_table_name(table_name)} #{sql_fragments.join(", ")}" unless sql_fragments.empty? + non_combinable_operations.each(&:call) + sql_fragments = [] + non_combinable_operations = [] + send(command, table, *arguments) + end + end + + execute "ALTER TABLE #{quote_table_name(table_name)} #{sql_fragments.join(", ")}" unless sql_fragments.empty? + non_combinable_operations.each(&:call) + end + def add_column_for_alter(table_name, column_name, type, options = {}) td = create_table_definition(table_name) cd = td.new_column_definition(column_name, type, options) diff --git a/activerecord/lib/active_record/connection_adapters/abstract_mysql_adapter.rb b/activerecord/lib/active_record/connection_adapters/abstract_mysql_adapter.rb index a7be475618..282b2b1838 100644 --- a/activerecord/lib/active_record/connection_adapters/abstract_mysql_adapter.rb +++ b/activerecord/lib/active_record/connection_adapters/abstract_mysql_adapter.rb @@ -65,7 +65,7 @@ module ActiveRecord /mariadb/i.match?(full_version) end - def supports_bulk_alter? #:nodoc: + def supports_bulk_alter? true end @@ -287,22 +287,8 @@ module ActiveRecord SQL end - def bulk_change_table(table_name, operations) #:nodoc: - sqls = operations.flat_map do |command, args| - table, arguments = args.shift, args - method = :"#{command}_for_alter" - - if respond_to?(method, true) - send(method, table, *arguments) - else - raise "Unknown method called : #{method}(#{arguments.inspect})" - end - end.join(", ") - - execute("ALTER TABLE #{quote_table_name(table_name)} #{sqls}") - end - - def change_table_comment(table_name, comment) #:nodoc: + def change_table_comment(table_name, comment_or_changes) # :nodoc: + comment = extract_new_comment_value(comment_or_changes) comment = "" if comment.nil? execute("ALTER TABLE #{quote_table_name(table_name)} COMMENT #{quote(comment)}") end @@ -358,7 +344,8 @@ module ActiveRecord change_column table_name, column_name, nil, null: null end - def change_column_comment(table_name, column_name, comment) #:nodoc: + def change_column_comment(table_name, column_name, comment_or_changes) # :nodoc: + comment = extract_new_comment_value(comment_or_changes) change_column table_name, column_name, nil, comment: comment end diff --git a/activerecord/lib/active_record/connection_adapters/mysql/type_metadata.rb b/activerecord/lib/active_record/connection_adapters/mysql/type_metadata.rb index 56479f27bf..9167593064 100644 --- a/activerecord/lib/active_record/connection_adapters/mysql/type_metadata.rb +++ b/activerecord/lib/active_record/connection_adapters/mysql/type_metadata.rb @@ -10,12 +10,11 @@ module ActiveRecord def initialize(type_metadata, extra: "") super(type_metadata) - @type_metadata = type_metadata @extra = extra end def ==(other) - other.is_a?(MySQL::TypeMetadata) && + other.is_a?(TypeMetadata) && __getobj__ == other.__getobj__ && extra == other.extra end diff --git a/activerecord/lib/active_record/connection_adapters/postgresql/column.rb b/activerecord/lib/active_record/connection_adapters/postgresql/column.rb index ef98d2b37a..ec25bb1e19 100644 --- a/activerecord/lib/active_record/connection_adapters/postgresql/column.rb +++ b/activerecord/lib/active_record/connection_adapters/postgresql/column.rb @@ -4,8 +4,7 @@ module ActiveRecord module ConnectionAdapters module PostgreSQL class Column < ConnectionAdapters::Column # :nodoc: - delegate :array, :oid, :fmod, to: :sql_type_metadata - alias :array? :array + delegate :oid, :fmod, to: :sql_type_metadata def initialize(*, serial: nil, **) super @@ -15,6 +14,15 @@ module ActiveRecord def serial? @serial end + + def array + sql_type_metadata.sql_type.end_with?("[]") + end + alias :array? :array + + def sql_type + super.sub(/\[\]\z/, "") + end end end PostgreSQLColumn = PostgreSQL::Column # :nodoc: diff --git a/activerecord/lib/active_record/connection_adapters/postgresql/schema_statements.rb b/activerecord/lib/active_record/connection_adapters/postgresql/schema_statements.rb index c412d1f34c..40c5e51d92 100644 --- a/activerecord/lib/active_record/connection_adapters/postgresql/schema_statements.rb +++ b/activerecord/lib/active_record/connection_adapters/postgresql/schema_statements.rb @@ -368,31 +368,6 @@ module ActiveRecord SQL end - def bulk_change_table(table_name, operations) - sql_fragments = [] - non_combinable_operations = [] - - operations.each do |command, args| - table, arguments = args.shift, args - method = :"#{command}_for_alter" - - if respond_to?(method, true) - sqls, procs = Array(send(method, table, *arguments)).partition { |v| v.is_a?(String) } - sql_fragments << sqls - non_combinable_operations.concat(procs) - else - execute "ALTER TABLE #{quote_table_name(table_name)} #{sql_fragments.join(", ")}" unless sql_fragments.empty? - non_combinable_operations.each(&:call) - sql_fragments = [] - non_combinable_operations = [] - send(command, table, *arguments) - end - end - - execute "ALTER TABLE #{quote_table_name(table_name)} #{sql_fragments.join(", ")}" unless sql_fragments.empty? - non_combinable_operations.each(&:call) - end - # Renames a table. # Also renames a table's primary key sequence if the sequence name exists and # matches the Active Record default. @@ -443,14 +418,16 @@ module ActiveRecord end # Adds comment for given table column or drops it if +comment+ is a +nil+ - def change_column_comment(table_name, column_name, comment) # :nodoc: + def change_column_comment(table_name, column_name, comment_or_changes) # :nodoc: clear_cache! + comment = extract_new_comment_value(comment_or_changes) execute "COMMENT ON COLUMN #{quote_table_name(table_name)}.#{quote_column_name(column_name)} IS #{quote(comment)}" end # Adds comment for given table or drops it if +comment+ is a +nil+ - def change_table_comment(table_name, comment) # :nodoc: + def change_table_comment(table_name, comment_or_changes) # :nodoc: clear_cache! + comment = extract_new_comment_value(comment_or_changes) execute "COMMENT ON TABLE #{quote_table_name(table_name)} IS #{quote(comment)}" end @@ -675,7 +652,7 @@ module ActiveRecord precision: cast_type.precision, scale: cast_type.scale, ) - PostgreSQLTypeMetadata.new(simple_type, oid: oid, fmod: fmod) + PostgreSQL::TypeMetadata.new(simple_type, oid: oid, fmod: fmod) end def sequence_name_from_parts(table_name, column_name, suffix) diff --git a/activerecord/lib/active_record/connection_adapters/postgresql/type_metadata.rb b/activerecord/lib/active_record/connection_adapters/postgresql/type_metadata.rb index 403b3ead98..8bdec623af 100644 --- a/activerecord/lib/active_record/connection_adapters/postgresql/type_metadata.rb +++ b/activerecord/lib/active_record/connection_adapters/postgresql/type_metadata.rb @@ -3,39 +3,34 @@ module ActiveRecord # :stopdoc: module ConnectionAdapters - class PostgreSQLTypeMetadata < DelegateClass(SqlTypeMetadata) - undef to_yaml if method_defined?(:to_yaml) + module PostgreSQL + class TypeMetadata < DelegateClass(SqlTypeMetadata) + undef to_yaml if method_defined?(:to_yaml) - attr_reader :oid, :fmod, :array + attr_reader :oid, :fmod - def initialize(type_metadata, oid: nil, fmod: nil) - super(type_metadata) - @type_metadata = type_metadata - @oid = oid - @fmod = fmod - @array = /\[\]$/.match?(type_metadata.sql_type) - end - - def sql_type - super.gsub(/\[\]$/, "") - end + def initialize(type_metadata, oid: nil, fmod: nil) + super(type_metadata) + @oid = oid + @fmod = fmod + end - def ==(other) - other.is_a?(PostgreSQLTypeMetadata) && - __getobj__ == other.__getobj__ && - oid == other.oid && - fmod == other.fmod && - array == other.array - end - alias eql? == + def ==(other) + other.is_a?(TypeMetadata) && + __getobj__ == other.__getobj__ && + oid == other.oid && + fmod == other.fmod + end + alias eql? == - def hash - PostgreSQLTypeMetadata.hash ^ - __getobj__.hash ^ - oid.hash ^ - fmod.hash ^ - array.hash + def hash + TypeMetadata.hash ^ + __getobj__.hash ^ + oid.hash ^ + fmod.hash + end end end + PostgreSQLTypeMetadata = PostgreSQL::TypeMetadata end end diff --git a/activerecord/lib/active_record/connection_adapters/sqlite3/database_statements.rb b/activerecord/lib/active_record/connection_adapters/sqlite3/database_statements.rb index ffa75172b5..46ce1a15b5 100644 --- a/activerecord/lib/active_record/connection_adapters/sqlite3/database_statements.rb +++ b/activerecord/lib/active_record/connection_adapters/sqlite3/database_statements.rb @@ -4,6 +4,82 @@ module ActiveRecord module ConnectionAdapters module SQLite3 module DatabaseStatements + READ_QUERY = ActiveRecord::ConnectionAdapters::AbstractAdapter.build_read_query_regexp(:begin, :commit, :explain, :select, :pragma, :release, :savepoint, :rollback) # :nodoc: + private_constant :READ_QUERY + + def write_query?(sql) # :nodoc: + !READ_QUERY.match?(sql) + end + + def execute(sql, name = nil) #:nodoc: + if preventing_writes? && write_query?(sql) + raise ActiveRecord::ReadOnlyError, "Write query attempted while in readonly mode: #{sql}" + end + + materialize_transactions + + log(sql, name) do + ActiveSupport::Dependencies.interlock.permit_concurrent_loads do + @connection.execute(sql) + end + end + end + + def exec_query(sql, name = nil, binds = [], prepare: false) + if preventing_writes? && write_query?(sql) + raise ActiveRecord::ReadOnlyError, "Write query attempted while in readonly mode: #{sql}" + end + + materialize_transactions + + type_casted_binds = type_casted_binds(binds) + + log(sql, name, binds, type_casted_binds) do + ActiveSupport::Dependencies.interlock.permit_concurrent_loads do + # Don't cache statements if they are not prepared + unless prepare + stmt = @connection.prepare(sql) + begin + cols = stmt.columns + unless without_prepared_statement?(binds) + stmt.bind_params(type_casted_binds) + end + records = stmt.to_a + ensure + stmt.close + end + else + stmt = @statements[sql] ||= @connection.prepare(sql) + cols = stmt.columns + stmt.reset! + stmt.bind_params(type_casted_binds) + records = stmt.to_a + end + + ActiveRecord::Result.new(cols, records) + end + end + end + + def exec_delete(sql, name = "SQL", binds = []) + exec_query(sql, name, binds) + @connection.changes + end + alias :exec_update :exec_delete + + def begin_db_transaction #:nodoc: + log("begin transaction", nil) { @connection.transaction } + end + + def commit_db_transaction #:nodoc: + log("commit transaction", nil) { @connection.commit } + end + + def exec_rollback_db_transaction #:nodoc: + log("rollback transaction", nil) { @connection.rollback } + end + + private def execute_batch(sql, name = nil) if preventing_writes? && write_query?(sql) @@ -19,6 +95,10 @@ module ActiveRecord end end + def last_inserted_id(result) + @connection.last_insert_row_id + end + def build_fixture_statements(fixture_set) fixture_set.flat_map do |table_name, fixtures| next if fixtures.empty? diff --git a/activerecord/lib/active_record/connection_adapters/sqlite3_adapter.rb b/activerecord/lib/active_record/connection_adapters/sqlite3_adapter.rb index 1801924c09..f5f5827d04 100644 --- a/activerecord/lib/active_record/connection_adapters/sqlite3_adapter.rb +++ b/activerecord/lib/active_record/connection_adapters/sqlite3_adapter.rb @@ -204,91 +204,11 @@ module ActiveRecord #-- # DATABASE STATEMENTS ====================================== #++ - - READ_QUERY = ActiveRecord::ConnectionAdapters::AbstractAdapter.build_read_query_regexp(:begin, :commit, :explain, :select, :pragma, :release, :savepoint, :rollback) # :nodoc: - private_constant :READ_QUERY - - def write_query?(sql) # :nodoc: - !READ_QUERY.match?(sql) - end - def explain(arel, binds = []) sql = "EXPLAIN QUERY PLAN #{to_sql(arel, binds)}" SQLite3::ExplainPrettyPrinter.new.pp(exec_query(sql, "EXPLAIN", [])) end - def exec_query(sql, name = nil, binds = [], prepare: false) - if preventing_writes? && write_query?(sql) - raise ActiveRecord::ReadOnlyError, "Write query attempted while in readonly mode: #{sql}" - end - - materialize_transactions - - type_casted_binds = type_casted_binds(binds) - - log(sql, name, binds, type_casted_binds) do - ActiveSupport::Dependencies.interlock.permit_concurrent_loads do - # Don't cache statements if they are not prepared - unless prepare - stmt = @connection.prepare(sql) - begin - cols = stmt.columns - unless without_prepared_statement?(binds) - stmt.bind_params(type_casted_binds) - end - records = stmt.to_a - ensure - stmt.close - end - else - stmt = @statements[sql] ||= @connection.prepare(sql) - cols = stmt.columns - stmt.reset! - stmt.bind_params(type_casted_binds) - records = stmt.to_a - end - - ActiveRecord::Result.new(cols, records) - end - end - end - - def exec_delete(sql, name = "SQL", binds = []) - exec_query(sql, name, binds) - @connection.changes - end - alias :exec_update :exec_delete - - def last_inserted_id(result) - @connection.last_insert_row_id - end - - def execute(sql, name = nil) #:nodoc: - if preventing_writes? && write_query?(sql) - raise ActiveRecord::ReadOnlyError, "Write query attempted while in readonly mode: #{sql}" - end - - materialize_transactions - - log(sql, name) do - ActiveSupport::Dependencies.interlock.permit_concurrent_loads do - @connection.execute(sql) - end - end - end - - def begin_db_transaction #:nodoc: - log("begin transaction", nil) { @connection.transaction } - end - - def commit_db_transaction #:nodoc: - log("commit transaction", nil) { @connection.commit } - end - - def exec_rollback_db_transaction #:nodoc: - log("rollback transaction", nil) { @connection.rollback } - end - # SCHEMA STATEMENTS ======================================== def primary_keys(table_name) # :nodoc: diff --git a/activerecord/lib/active_record/connection_handling.rb b/activerecord/lib/active_record/connection_handling.rb index 53069cd899..040ebdb960 100644 --- a/activerecord/lib/active_record/connection_handling.rb +++ b/activerecord/lib/active_record/connection_handling.rb @@ -85,14 +85,14 @@ module ActiveRecord # based on the requested role: # # ActiveRecord::Base.connected_to(role: :writing) do - # Dog.create! # creates dog using dog connection + # Dog.create! # creates dog using dog writing connection # end # # ActiveRecord::Base.connected_to(role: :reading) do # Dog.create! # throws exception because we're on a replica # end # - # ActiveRecord::Base.connected_to(role: :unknown_ode) do + # ActiveRecord::Base.connected_to(role: :unknown_role) do # # raises exception due to non-existent role # end # @@ -100,11 +100,20 @@ module ActiveRecord # you can use +connected_to+ with a +database+ argument. The +database+ argument # expects a symbol that corresponds to the database key in your config. # - # This will connect to a new database for the queries inside the block. - # # ActiveRecord::Base.connected_to(database: :animals_slow_replica) do # Dog.run_a_long_query # runs a long query while connected to the +animals_slow_replica+ # end + # + # This will connect to a new database for the queries inside the block. By + # default the `:writing` role will be used since all connections must be assigned + # a role. If you would like to use a different role you can pass a hash to database: + # + # ActiveRecord::Base.connected_to(database: { readonly_slow: :animals_slow_replica }) do + # # runs a long query while connected to the +animals_slow_replica+ using the readonly_slow role. + # Dog.run_a_long_query + # end + # + # When using the database key a new connection will be established every time. def connected_to(database: nil, role: nil, &blk) if database && role raise ArgumentError, "connected_to can only accept a `database` or a `role` argument, but not both arguments." @@ -112,17 +121,14 @@ module ActiveRecord if database.is_a?(Hash) role, database = database.first role = role.to_sym - else - role = database.to_sym end config_hash = resolve_config_for_connection(database) handler = lookup_connection_handler(role) - with_handler(role) do - handler.establish_connection(config_hash) - yield - end + handler.establish_connection(config_hash) + + with_handler(role, &blk) elsif role with_handler(role.to_sym, &blk) else @@ -154,6 +160,7 @@ module ActiveRecord end def lookup_connection_handler(handler_key) # :nodoc: + handler_key ||= ActiveRecord::Base.writing_role connection_handlers[handler_key] ||= ActiveRecord::ConnectionAdapters::ConnectionHandler.new end diff --git a/activerecord/lib/active_record/core.rb b/activerecord/lib/active_record/core.rb index eb4b48bc37..6fed3e5c19 100644 --- a/activerecord/lib/active_record/core.rb +++ b/activerecord/lib/active_record/core.rb @@ -466,6 +466,7 @@ module ActiveRecord # Returns +true+ if the attributes hash has been frozen. def frozen? + sync_with_transaction_state @attributes.frozen? end @@ -583,7 +584,7 @@ module ActiveRecord end def thaw - if frozen? + if @attributes.frozen? @attributes = @attributes.dup end end diff --git a/activerecord/lib/active_record/locking/optimistic.rb b/activerecord/lib/active_record/locking/optimistic.rb index 4a3a31fc95..b7eecda59e 100644 --- a/activerecord/lib/active_record/locking/optimistic.rb +++ b/activerecord/lib/active_record/locking/optimistic.rb @@ -71,9 +71,8 @@ module ActiveRecord end def _touch_row(attribute_names, time) + @_touch_attr_names << self.class.locking_column if locking_enabled? super - ensure - clear_attribute_change(self.class.locking_column) if locking_enabled? end def _update_row(attribute_names, attempted_action = "update") diff --git a/activerecord/lib/active_record/log_subscriber.rb b/activerecord/lib/active_record/log_subscriber.rb index 6b84431343..6248c2f578 100644 --- a/activerecord/lib/active_record/log_subscriber.rb +++ b/activerecord/lib/active_record/log_subscriber.rb @@ -110,7 +110,7 @@ module ActiveRecord end def extract_query_source_location(locations) - backtrace_cleaner.clean(locations).first + backtrace_cleaner.clean(locations.lazy).first end end end diff --git a/activerecord/lib/active_record/migration/command_recorder.rb b/activerecord/lib/active_record/migration/command_recorder.rb index 8e7f596076..efed4b0e26 100644 --- a/activerecord/lib/active_record/migration/command_recorder.rb +++ b/activerecord/lib/active_record/migration/command_recorder.rb @@ -14,6 +14,8 @@ module ActiveRecord # * change_column # * change_column_default (must supply a :from and :to option) # * change_column_null + # * change_column_comment (must supply a :from and :to option) + # * change_table_comment (must supply a :from and :to option) # * create_join_table # * create_table # * disable_extension @@ -35,7 +37,8 @@ module ActiveRecord :change_column_default, :add_reference, :remove_reference, :transaction, :drop_join_table, :drop_table, :execute_block, :enable_extension, :disable_extension, :change_column, :execute, :remove_columns, :change_column_null, - :add_foreign_key, :remove_foreign_key + :add_foreign_key, :remove_foreign_key, + :change_column_comment, :change_table_comment ] include JoinTable @@ -244,6 +247,26 @@ module ActiveRecord [:add_foreign_key, reversed_args] end + def invert_change_column_comment(args) + table, column, options = *args + + unless options && options.is_a?(Hash) && options.has_key?(:from) && options.has_key?(:to) + raise ActiveRecord::IrreversibleMigration, "change_column_comment is only reversible if given a :from and :to option." + end + + [:change_column_comment, [table, column, from: options[:to], to: options[:from]]] + end + + def invert_change_table_comment(args) + table, options = *args + + unless options && options.is_a?(Hash) && options.has_key?(:from) && options.has_key?(:to) + raise ActiveRecord::IrreversibleMigration, "change_table_comment is only reversible if given a :from and :to option." + end + + [:change_table_comment, [table, from: options[:to], to: options[:from]]] + end + def respond_to_missing?(method, _) super || delegate.respond_to?(method) end diff --git a/activerecord/lib/active_record/migration/compatibility.rb b/activerecord/lib/active_record/migration/compatibility.rb index abc939826b..ff91218696 100644 --- a/activerecord/lib/active_record/migration/compatibility.rb +++ b/activerecord/lib/active_record/migration/compatibility.rb @@ -27,6 +27,16 @@ module ActiveRecord def invert_transaction(args, &block) [:transaction, args, block] end + + def invert_change_column_comment(args) + table_name, column_name, comment = args + [:change_column_comment, [table_name, column_name, from: comment, to: comment]] + end + + def invert_change_table_comment(args) + table_name, comment = args + [:change_table_comment, [table_name, from: comment, to: comment]] + end end def create_table(table_name, **options) diff --git a/activerecord/lib/active_record/persistence.rb b/activerecord/lib/active_record/persistence.rb index e05490753f..8bade8cd28 100644 --- a/activerecord/lib/active_record/persistence.rb +++ b/activerecord/lib/active_record/persistence.rb @@ -530,7 +530,6 @@ module ActiveRecord def destroy _raise_readonly_record_error if readonly? destroy_associations - self.class.connection.add_transaction_record(self) @_trigger_destroy_callback = if persisted? destroy_row > 0 else @@ -852,7 +851,9 @@ module ActiveRecord end attribute_names = timestamp_attributes_for_update_in_model - attribute_names |= names.map(&:to_s) + attribute_names |= names.map!(&:to_s).map! { |name| + self.class.attribute_alias?(name) ? self.class.attribute_alias(name) : name + } unless attribute_names.empty? affected_rows = _touch_row(attribute_names, time) @@ -880,8 +881,7 @@ module ActiveRecord time ||= current_time_from_proper_timezone attribute_names.each do |attr_name| - write_attribute(attr_name, time) - clear_attribute_change(attr_name) + _write_attribute(attr_name, time) end _update_row(attribute_names, "touch") diff --git a/activerecord/lib/active_record/reflection.rb b/activerecord/lib/active_record/reflection.rb index 3452cf971b..1312bf6f91 100644 --- a/activerecord/lib/active_record/reflection.rb +++ b/activerecord/lib/active_record/reflection.rb @@ -21,12 +21,12 @@ module ActiveRecord def add_reflection(ar, name, reflection) ar.clear_reflections_cache - name = name.to_s + name = -name.to_s ar._reflections = ar._reflections.except(name).merge!(name => reflection) end def add_aggregate_reflection(ar, name, reflection) - ar.aggregate_reflections = ar.aggregate_reflections.merge(name.to_s => reflection) + ar.aggregate_reflections = ar.aggregate_reflections.merge(-name.to_s => reflection) end private diff --git a/activerecord/lib/active_record/touch_later.rb b/activerecord/lib/active_record/touch_later.rb index 5dc88fb26c..980e42664b 100644 --- a/activerecord/lib/active_record/touch_later.rb +++ b/activerecord/lib/active_record/touch_later.rb @@ -22,7 +22,7 @@ module ActiveRecord @_touch_time = current_time_from_proper_timezone surreptitiously_touch @_defer_touch_attrs - self.class.connection.add_transaction_record self + add_to_transaction # touch the parents as we are not calling the after_save callbacks self.class.reflect_on_all_associations(:belongs_to).each do |r| diff --git a/activerecord/lib/active_record/transactions.rb b/activerecord/lib/active_record/transactions.rb index a45d228298..03a373f0af 100644 --- a/activerecord/lib/active_record/transactions.rb +++ b/activerecord/lib/active_record/transactions.rb @@ -355,18 +355,6 @@ module ActiveRecord clear_transaction_record_state end - # Add the record to the current transaction so that the #after_rollback and #after_commit callbacks - # can be called. - def add_to_transaction - if has_transactional_callbacks? - self.class.connection.add_transaction_record(self) - else - sync_with_transaction_state - set_transaction_state(self.class.connection.transaction_state) - end - remember_transaction_record_state - end - # Executes +method+ within a transaction and captures its return value as a # status flag. If the status is true the transaction is committed, otherwise # a ROLLBACK is issued. In any case the status flag is returned. @@ -376,9 +364,19 @@ module ActiveRecord def with_transaction_returning_status status = nil self.class.transaction do - add_to_transaction + unless has_transactional_callbacks? + sync_with_transaction_state + @transaction_state = self.class.connection.transaction_state + end + remember_transaction_record_state + status = yield raise ActiveRecord::Rollback unless status + ensure + if has_transactional_callbacks? && + (@_new_record_before_last_commit && !new_record? || _trigger_update_callback || _trigger_destroy_callback) + add_to_transaction + end end status end @@ -415,13 +413,14 @@ module ActiveRecord # Force to clear the transaction record state. def force_clear_transaction_record_state @_start_transaction_state.clear + @transaction_state = nil end # Restore the new record state and id of a record that was previously saved by a call to save_record_state. - def restore_transaction_record_state(force = false) + def restore_transaction_record_state(force_restore_state = false) unless @_start_transaction_state.empty? transaction_level = (@_start_transaction_state[:level] || 0) - 1 - if transaction_level < 1 || force + if transaction_level < 1 || force_restore_state restore_state = @_start_transaction_state thaw @new_record = restore_state[:new_record] @@ -449,8 +448,10 @@ module ActiveRecord end end - def set_transaction_state(state) - @transaction_state = state + # Add the record to the current transaction so that the #after_rollback and #after_commit + # callbacks can be called. + def add_to_transaction + self.class.connection.add_transaction_record(self) end def has_transactional_callbacks? @@ -470,19 +471,17 @@ module ActiveRecord # This method checks to see if the ActiveRecord object's state reflects # the TransactionState, and rolls back or commits the Active Record object # as appropriate. - # - # Since Active Record objects can be inside multiple transactions, this - # method recursively goes through the parent of the TransactionState and - # checks if the Active Record object reflects the state of the object. def sync_with_transaction_state - update_attributes_from_transaction_state(@transaction_state) - end - - def update_attributes_from_transaction_state(transaction_state) - if transaction_state && transaction_state.finalized? - restore_transaction_record_state(transaction_state.fully_rolledback?) if transaction_state.rolledback? - force_clear_transaction_record_state if transaction_state.fully_committed? - clear_transaction_record_state if transaction_state.fully_completed? + if (transaction_state = @transaction_state)&.finalized? + if transaction_state.fully_committed? + force_clear_transaction_record_state + elsif transaction_state.committed? + clear_transaction_record_state + elsif transaction_state.rolledback? + force_restore_state = transaction_state.fully_rolledback? + restore_transaction_record_state(force_restore_state) + clear_transaction_record_state + end end end end diff --git a/activerecord/lib/arel/visitors/oracle.rb b/activerecord/lib/arel/visitors/oracle.rb index f96bf65ee5..500974dff5 100644 --- a/activerecord/lib/arel/visitors/oracle.rb +++ b/activerecord/lib/arel/visitors/oracle.rb @@ -87,6 +87,50 @@ module Arel # :nodoc: all collector << " )" end + def visit_Arel_Nodes_In(o, collector) + if Array === o.right && !o.right.empty? + o.right.delete_if { |value| unboundable?(value) } + end + + if Array === o.right && o.right.empty? + collector << "1=0" + else + first = true + o.right.each_slice(in_clause_length) do |sliced_o_right| + collector << " OR " unless first + first = false + + collector = visit o.left, collector + collector << " IN (" + visit(sliced_o_right, collector) + collector << ")" + end + end + collector + end + + def visit_Arel_Nodes_NotIn(o, collector) + if Array === o.right && !o.right.empty? + o.right.delete_if { |value| unboundable?(value) } + end + + if Array === o.right && o.right.empty? + collector << "1=1" + else + first = true + o.right.each_slice(in_clause_length) do |sliced_o_right| + collector << " AND " unless first + first = false + + collector = visit o.left, collector + collector << " NOT IN (" + visit(sliced_o_right, collector) + collector << ")" + end + end + collector + end + def visit_Arel_Nodes_UpdateStatement(o, collector) # Oracle does not allow ORDER BY/LIMIT in UPDATEs. if o.orders.any? && o.limit.nil? @@ -154,6 +198,10 @@ module Arel # :nodoc: all collector = visit [o.left, o.right, 0, 1], collector collector << ")" end + + def in_clause_length + 1000 + end end end end diff --git a/activerecord/lib/arel/visitors/oracle12.rb b/activerecord/lib/arel/visitors/oracle12.rb index 9a7fe4d626..8e0f07fca9 100644 --- a/activerecord/lib/arel/visitors/oracle12.rb +++ b/activerecord/lib/arel/visitors/oracle12.rb @@ -41,6 +41,50 @@ module Arel # :nodoc: all collector << " )" end + def visit_Arel_Nodes_In(o, collector) + if Array === o.right && !o.right.empty? + o.right.delete_if { |value| unboundable?(value) } + end + + if Array === o.right && o.right.empty? + collector << "1=0" + else + first = true + o.right.each_slice(in_clause_length) do |sliced_o_right| + collector << " OR " unless first + first = false + + collector = visit o.left, collector + collector << " IN (" + visit(sliced_o_right, collector) + collector << ")" + end + end + collector + end + + def visit_Arel_Nodes_NotIn(o, collector) + if Array === o.right && !o.right.empty? + o.right.delete_if { |value| unboundable?(value) } + end + + if Array === o.right && o.right.empty? + collector << "1=1" + else + first = true + o.right.each_slice(in_clause_length) do |sliced_o_right| + collector << " AND " unless first + first = false + + collector = visit o.left, collector + collector << " NOT IN (" + visit(sliced_o_right, collector) + collector << ")" + end + end + collector + end + def visit_Arel_Nodes_UpdateStatement(o, collector) # Oracle does not allow ORDER BY/LIMIT in UPDATEs. if o.orders.any? && o.limit.nil? @@ -62,6 +106,10 @@ module Arel # :nodoc: all collector = visit [o.left, o.right, 0, 1], collector collector << ")" end + + def in_clause_length + 1000 + end end end end diff --git a/activerecord/lib/rails/generators/active_record/model/templates/model.rb.tt b/activerecord/lib/rails/generators/active_record/model/templates/model.rb.tt index c1c03e2762..77b9ea1c86 100644 --- a/activerecord/lib/rails/generators/active_record/model/templates/model.rb.tt +++ b/activerecord/lib/rails/generators/active_record/model/templates/model.rb.tt @@ -1,7 +1,7 @@ <% module_namespacing do -%> class <%= class_name %> < <%= parent_class_name.classify %> <% attributes.select(&:reference?).each do |attribute| -%> - belongs_to :<%= attribute.name %><%= ', polymorphic: true' if attribute.polymorphic? %><%= ', required: true' if attribute.required? %> + belongs_to :<%= attribute.name %><%= ', polymorphic: true' if attribute.polymorphic? %> <% end -%> <% attributes.select(&:rich_text?).each do |attribute| -%> has_rich_text :<%= attribute.name %> diff --git a/activerecord/test/cases/adapter_test.rb b/activerecord/test/cases/adapter_test.rb index ba04859bf0..ce2ed06c1d 100644 --- a/activerecord/test/cases/adapter_test.rb +++ b/activerecord/test/cases/adapter_test.rb @@ -490,6 +490,8 @@ module ActiveRecord @connection.truncate("posts") assert_equal 0, Post.count + ensure + reset_fixtures("posts") end def test_truncate_with_query_cache @@ -501,6 +503,7 @@ module ActiveRecord assert_equal 0, Post.count ensure + reset_fixtures("posts") @connection.disable_query_cache! end @@ -514,6 +517,8 @@ module ActiveRecord assert_equal 0, Post.count assert_equal 0, Author.count assert_equal 0, AuthorAddress.count + ensure + reset_fixtures("posts", "authors", "author_addresses") end def test_truncate_tables_with_query_cache @@ -529,6 +534,7 @@ module ActiveRecord assert_equal 0, Author.count assert_equal 0, AuthorAddress.count ensure + reset_fixtures("posts", "authors", "author_addresses") @connection.disable_query_cache! end @@ -551,6 +557,16 @@ module ActiveRecord assert_nothing_raised { sub.save! } end end + + private + + def reset_fixtures(*fixture_names) + ActiveRecord::FixtureSet.reset_cache + + fixture_names.each do |fixture_name| + ActiveRecord::FixtureSet.create_fixtures(FIXTURES_ROOT, fixture_name) + end + end end end diff --git a/activerecord/test/cases/adapters/mysql2/active_schema_test.rb b/activerecord/test/cases/adapters/mysql2/active_schema_test.rb index 88c2ac5d0a..c2c357d0c1 100644 --- a/activerecord/test/cases/adapters/mysql2/active_schema_test.rb +++ b/activerecord/test/cases/adapters/mysql2/active_schema_test.rb @@ -10,7 +10,15 @@ class Mysql2ActiveSchemaTest < ActiveRecord::Mysql2TestCase ActiveRecord::Base.connection.send(:default_row_format) ActiveRecord::Base.connection.singleton_class.class_eval do alias_method :execute_without_stub, :execute - def execute(sql, name = nil) sql end + def execute(sql, name = nil) + ActiveSupport::Notifications.instrumenter.instrument( + "sql.active_record", + sql: sql, + name: name, + connection: self) do + sql + end + end end end @@ -89,17 +97,19 @@ class Mysql2ActiveSchemaTest < ActiveRecord::Mysql2TestCase %w(SPATIAL FULLTEXT UNIQUE).each do |type| expected = "ALTER TABLE `people` ADD #{type} INDEX `index_people_on_last_name` (`last_name`)" - actual = ActiveRecord::Base.connection.change_table(:people, bulk: true) do |t| - t.index :last_name, type: type + assert_sql(expected) do + ActiveRecord::Base.connection.change_table(:people, bulk: true) do |t| + t.index :last_name, type: type + end end - assert_equal expected, actual end expected = "ALTER TABLE `people` ADD INDEX `index_people_on_last_name` USING btree (`last_name`(10)), ALGORITHM = COPY" - actual = ActiveRecord::Base.connection.change_table(:people, bulk: true) do |t| - t.index :last_name, length: 10, using: :btree, algorithm: :copy + assert_sql(expected) do + ActiveRecord::Base.connection.change_table(:people, bulk: true) do |t| + t.index :last_name, length: 10, using: :btree, algorithm: :copy + end end - assert_equal expected, actual end def test_drop_table diff --git a/activerecord/test/cases/ar_schema_test.rb b/activerecord/test/cases/ar_schema_test.rb index 9027cc582a..7aa6d089c5 100644 --- a/activerecord/test/cases/ar_schema_test.rb +++ b/activerecord/test/cases/ar_schema_test.rb @@ -160,7 +160,7 @@ class ActiveRecordSchemaTest < ActiveRecord::TestCase end if subsecond_precision_supported? - def test_timestamps_sets_presicion_on_create_table + def test_timestamps_sets_precision_on_create_table ActiveRecord::Schema.define do create_table :has_timestamps do |t| t.timestamps @@ -171,7 +171,7 @@ class ActiveRecordSchemaTest < ActiveRecord::TestCase assert @connection.column_exists?(:has_timestamps, :updated_at, precision: 6, null: false) end - def test_timestamps_sets_presicion_on_change_table + def test_timestamps_sets_precision_on_change_table ActiveRecord::Schema.define do create_table :has_timestamps @@ -185,7 +185,7 @@ class ActiveRecordSchemaTest < ActiveRecord::TestCase end if ActiveRecord::Base.connection.supports_bulk_alter? - def test_timestamps_sets_presicion_on_change_table_with_bulk + def test_timestamps_sets_precision_on_change_table_with_bulk ActiveRecord::Schema.define do create_table :has_timestamps @@ -199,7 +199,7 @@ class ActiveRecordSchemaTest < ActiveRecord::TestCase end end - def test_timestamps_sets_presicion_on_add_timestamps + def test_timestamps_sets_precision_on_add_timestamps ActiveRecord::Schema.define do create_table :has_timestamps add_timestamps :has_timestamps, default: Time.now diff --git a/activerecord/test/cases/arel/visitors/oracle12_test.rb b/activerecord/test/cases/arel/visitors/oracle12_test.rb index 4ce5cab4db..ebea12910d 100644 --- a/activerecord/test/cases/arel/visitors/oracle12_test.rb +++ b/activerecord/test/cases/arel/visitors/oracle12_test.rb @@ -8,6 +8,7 @@ module Arel before do @visitor = Oracle12.new Table.engine.connection @table = Table.new(:users) + @attr = @table[:id] end def compile(node) @@ -95,6 +96,26 @@ module Arel sql.must_be_like %{ "users"."name" IS NOT NULL } end end + + describe "Nodes::In" do + it "should know how to visit" do + ary = (1 .. 1001).to_a + node = @attr.in ary + compile(node).must_be_like %{ + "users"."id" IN (1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27, 28, 29, 30, 31, 32, 33, 34, 35, 36, 37, 38, 39, 40, 41, 42, 43, 44, 45, 46, 47, 48, 49, 50, 51, 52, 53, 54, 55, 56, 57, 58, 59, 60, 61, 62, 63, 64, 65, 66, 67, 68, 69, 70, 71, 72, 73, 74, 75, 76, 77, 78, 79, 80, 81, 82, 83, 84, 85, 86, 87, 88, 89, 90, 91, 92, 93, 94, 95, 96, 97, 98, 99, 100, 101, 102, 103, 104, 105, 106, 107, 108, 109, 110, 111, 112, 113, 114, 115, 116, 117, 118, 119, 120, 121, 122, 123, 124, 125, 126, 127, 128, 129, 130, 131, 132, 133, 134, 135, 136, 137, 138, 139, 140, 141, 142, 143, 144, 145, 146, 147, 148, 149, 150, 151, 152, 153, 154, 155, 156, 157, 158, 159, 160, 161, 162, 163, 164, 165, 166, 167, 168, 169, 170, 171, 172, 173, 174, 175, 176, 177, 178, 179, 180, 181, 182, 183, 184, 185, 186, 187, 188, 189, 190, 191, 192, 193, 194, 195, 196, 197, 198, 199, 200, 201, 202, 203, 204, 205, 206, 207, 208, 209, 210, 211, 212, 213, 214, 215, 216, 217, 218, 219, 220, 221, 222, 223, 224, 225, 226, 227, 228, 229, 230, 231, 232, 233, 234, 235, 236, 237, 238, 239, 240, 241, 242, 243, 244, 245, 246, 247, 248, 249, 250, 251, 252, 253, 254, 255, 256, 257, 258, 259, 260, 261, 262, 263, 264, 265, 266, 267, 268, 269, 270, 271, 272, 273, 274, 275, 276, 277, 278, 279, 280, 281, 282, 283, 284, 285, 286, 287, 288, 289, 290, 291, 292, 293, 294, 295, 296, 297, 298, 299, 300, 301, 302, 303, 304, 305, 306, 307, 308, 309, 310, 311, 312, 313, 314, 315, 316, 317, 318, 319, 320, 321, 322, 323, 324, 325, 326, 327, 328, 329, 330, 331, 332, 333, 334, 335, 336, 337, 338, 339, 340, 341, 342, 343, 344, 345, 346, 347, 348, 349, 350, 351, 352, 353, 354, 355, 356, 357, 358, 359, 360, 361, 362, 363, 364, 365, 366, 367, 368, 369, 370, 371, 372, 373, 374, 375, 376, 377, 378, 379, 380, 381, 382, 383, 384, 385, 386, 387, 388, 389, 390, 391, 392, 393, 394, 395, 396, 397, 398, 399, 400, 401, 402, 403, 404, 405, 406, 407, 408, 409, 410, 411, 412, 413, 414, 415, 416, 417, 418, 419, 420, 421, 422, 423, 424, 425, 426, 427, 428, 429, 430, 431, 432, 433, 434, 435, 436, 437, 438, 439, 440, 441, 442, 443, 444, 445, 446, 447, 448, 449, 450, 451, 452, 453, 454, 455, 456, 457, 458, 459, 460, 461, 462, 463, 464, 465, 466, 467, 468, 469, 470, 471, 472, 473, 474, 475, 476, 477, 478, 479, 480, 481, 482, 483, 484, 485, 486, 487, 488, 489, 490, 491, 492, 493, 494, 495, 496, 497, 498, 499, 500, 501, 502, 503, 504, 505, 506, 507, 508, 509, 510, 511, 512, 513, 514, 515, 516, 517, 518, 519, 520, 521, 522, 523, 524, 525, 526, 527, 528, 529, 530, 531, 532, 533, 534, 535, 536, 537, 538, 539, 540, 541, 542, 543, 544, 545, 546, 547, 548, 549, 550, 551, 552, 553, 554, 555, 556, 557, 558, 559, 560, 561, 562, 563, 564, 565, 566, 567, 568, 569, 570, 571, 572, 573, 574, 575, 576, 577, 578, 579, 580, 581, 582, 583, 584, 585, 586, 587, 588, 589, 590, 591, 592, 593, 594, 595, 596, 597, 598, 599, 600, 601, 602, 603, 604, 605, 606, 607, 608, 609, 610, 611, 612, 613, 614, 615, 616, 617, 618, 619, 620, 621, 622, 623, 624, 625, 626, 627, 628, 629, 630, 631, 632, 633, 634, 635, 636, 637, 638, 639, 640, 641, 642, 643, 644, 645, 646, 647, 648, 649, 650, 651, 652, 653, 654, 655, 656, 657, 658, 659, 660, 661, 662, 663, 664, 665, 666, 667, 668, 669, 670, 671, 672, 673, 674, 675, 676, 677, 678, 679, 680, 681, 682, 683, 684, 685, 686, 687, 688, 689, 690, 691, 692, 693, 694, 695, 696, 697, 698, 699, 700, 701, 702, 703, 704, 705, 706, 707, 708, 709, 710, 711, 712, 713, 714, 715, 716, 717, 718, 719, 720, 721, 722, 723, 724, 725, 726, 727, 728, 729, 730, 731, 732, 733, 734, 735, 736, 737, 738, 739, 740, 741, 742, 743, 744, 745, 746, 747, 748, 749, 750, 751, 752, 753, 754, 755, 756, 757, 758, 759, 760, 761, 762, 763, 764, 765, 766, 767, 768, 769, 770, 771, 772, 773, 774, 775, 776, 777, 778, 779, 780, 781, 782, 783, 784, 785, 786, 787, 788, 789, 790, 791, 792, 793, 794, 795, 796, 797, 798, 799, 800, 801, 802, 803, 804, 805, 806, 807, 808, 809, 810, 811, 812, 813, 814, 815, 816, 817, 818, 819, 820, 821, 822, 823, 824, 825, 826, 827, 828, 829, 830, 831, 832, 833, 834, 835, 836, 837, 838, 839, 840, 841, 842, 843, 844, 845, 846, 847, 848, 849, 850, 851, 852, 853, 854, 855, 856, 857, 858, 859, 860, 861, 862, 863, 864, 865, 866, 867, 868, 869, 870, 871, 872, 873, 874, 875, 876, 877, 878, 879, 880, 881, 882, 883, 884, 885, 886, 887, 888, 889, 890, 891, 892, 893, 894, 895, 896, 897, 898, 899, 900, 901, 902, 903, 904, 905, 906, 907, 908, 909, 910, 911, 912, 913, 914, 915, 916, 917, 918, 919, 920, 921, 922, 923, 924, 925, 926, 927, 928, 929, 930, 931, 932, 933, 934, 935, 936, 937, 938, 939, 940, 941, 942, 943, 944, 945, 946, 947, 948, 949, 950, 951, 952, 953, 954, 955, 956, 957, 958, 959, 960, 961, 962, 963, 964, 965, 966, 967, 968, 969, 970, 971, 972, 973, 974, 975, 976, 977, 978, 979, 980, 981, 982, 983, 984, 985, 986, 987, 988, 989, 990, 991, 992, 993, 994, 995, 996, 997, 998, 999, 1000) OR \"users\".\"id\" IN (1001) + } + end + end + + describe "Nodes::NotIn" do + it "should know how to visit" do + ary = (1 .. 1001).to_a + node = @attr.not_in ary + compile(node).must_be_like %{ + "users"."id" NOT IN (1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27, 28, 29, 30, 31, 32, 33, 34, 35, 36, 37, 38, 39, 40, 41, 42, 43, 44, 45, 46, 47, 48, 49, 50, 51, 52, 53, 54, 55, 56, 57, 58, 59, 60, 61, 62, 63, 64, 65, 66, 67, 68, 69, 70, 71, 72, 73, 74, 75, 76, 77, 78, 79, 80, 81, 82, 83, 84, 85, 86, 87, 88, 89, 90, 91, 92, 93, 94, 95, 96, 97, 98, 99, 100, 101, 102, 103, 104, 105, 106, 107, 108, 109, 110, 111, 112, 113, 114, 115, 116, 117, 118, 119, 120, 121, 122, 123, 124, 125, 126, 127, 128, 129, 130, 131, 132, 133, 134, 135, 136, 137, 138, 139, 140, 141, 142, 143, 144, 145, 146, 147, 148, 149, 150, 151, 152, 153, 154, 155, 156, 157, 158, 159, 160, 161, 162, 163, 164, 165, 166, 167, 168, 169, 170, 171, 172, 173, 174, 175, 176, 177, 178, 179, 180, 181, 182, 183, 184, 185, 186, 187, 188, 189, 190, 191, 192, 193, 194, 195, 196, 197, 198, 199, 200, 201, 202, 203, 204, 205, 206, 207, 208, 209, 210, 211, 212, 213, 214, 215, 216, 217, 218, 219, 220, 221, 222, 223, 224, 225, 226, 227, 228, 229, 230, 231, 232, 233, 234, 235, 236, 237, 238, 239, 240, 241, 242, 243, 244, 245, 246, 247, 248, 249, 250, 251, 252, 253, 254, 255, 256, 257, 258, 259, 260, 261, 262, 263, 264, 265, 266, 267, 268, 269, 270, 271, 272, 273, 274, 275, 276, 277, 278, 279, 280, 281, 282, 283, 284, 285, 286, 287, 288, 289, 290, 291, 292, 293, 294, 295, 296, 297, 298, 299, 300, 301, 302, 303, 304, 305, 306, 307, 308, 309, 310, 311, 312, 313, 314, 315, 316, 317, 318, 319, 320, 321, 322, 323, 324, 325, 326, 327, 328, 329, 330, 331, 332, 333, 334, 335, 336, 337, 338, 339, 340, 341, 342, 343, 344, 345, 346, 347, 348, 349, 350, 351, 352, 353, 354, 355, 356, 357, 358, 359, 360, 361, 362, 363, 364, 365, 366, 367, 368, 369, 370, 371, 372, 373, 374, 375, 376, 377, 378, 379, 380, 381, 382, 383, 384, 385, 386, 387, 388, 389, 390, 391, 392, 393, 394, 395, 396, 397, 398, 399, 400, 401, 402, 403, 404, 405, 406, 407, 408, 409, 410, 411, 412, 413, 414, 415, 416, 417, 418, 419, 420, 421, 422, 423, 424, 425, 426, 427, 428, 429, 430, 431, 432, 433, 434, 435, 436, 437, 438, 439, 440, 441, 442, 443, 444, 445, 446, 447, 448, 449, 450, 451, 452, 453, 454, 455, 456, 457, 458, 459, 460, 461, 462, 463, 464, 465, 466, 467, 468, 469, 470, 471, 472, 473, 474, 475, 476, 477, 478, 479, 480, 481, 482, 483, 484, 485, 486, 487, 488, 489, 490, 491, 492, 493, 494, 495, 496, 497, 498, 499, 500, 501, 502, 503, 504, 505, 506, 507, 508, 509, 510, 511, 512, 513, 514, 515, 516, 517, 518, 519, 520, 521, 522, 523, 524, 525, 526, 527, 528, 529, 530, 531, 532, 533, 534, 535, 536, 537, 538, 539, 540, 541, 542, 543, 544, 545, 546, 547, 548, 549, 550, 551, 552, 553, 554, 555, 556, 557, 558, 559, 560, 561, 562, 563, 564, 565, 566, 567, 568, 569, 570, 571, 572, 573, 574, 575, 576, 577, 578, 579, 580, 581, 582, 583, 584, 585, 586, 587, 588, 589, 590, 591, 592, 593, 594, 595, 596, 597, 598, 599, 600, 601, 602, 603, 604, 605, 606, 607, 608, 609, 610, 611, 612, 613, 614, 615, 616, 617, 618, 619, 620, 621, 622, 623, 624, 625, 626, 627, 628, 629, 630, 631, 632, 633, 634, 635, 636, 637, 638, 639, 640, 641, 642, 643, 644, 645, 646, 647, 648, 649, 650, 651, 652, 653, 654, 655, 656, 657, 658, 659, 660, 661, 662, 663, 664, 665, 666, 667, 668, 669, 670, 671, 672, 673, 674, 675, 676, 677, 678, 679, 680, 681, 682, 683, 684, 685, 686, 687, 688, 689, 690, 691, 692, 693, 694, 695, 696, 697, 698, 699, 700, 701, 702, 703, 704, 705, 706, 707, 708, 709, 710, 711, 712, 713, 714, 715, 716, 717, 718, 719, 720, 721, 722, 723, 724, 725, 726, 727, 728, 729, 730, 731, 732, 733, 734, 735, 736, 737, 738, 739, 740, 741, 742, 743, 744, 745, 746, 747, 748, 749, 750, 751, 752, 753, 754, 755, 756, 757, 758, 759, 760, 761, 762, 763, 764, 765, 766, 767, 768, 769, 770, 771, 772, 773, 774, 775, 776, 777, 778, 779, 780, 781, 782, 783, 784, 785, 786, 787, 788, 789, 790, 791, 792, 793, 794, 795, 796, 797, 798, 799, 800, 801, 802, 803, 804, 805, 806, 807, 808, 809, 810, 811, 812, 813, 814, 815, 816, 817, 818, 819, 820, 821, 822, 823, 824, 825, 826, 827, 828, 829, 830, 831, 832, 833, 834, 835, 836, 837, 838, 839, 840, 841, 842, 843, 844, 845, 846, 847, 848, 849, 850, 851, 852, 853, 854, 855, 856, 857, 858, 859, 860, 861, 862, 863, 864, 865, 866, 867, 868, 869, 870, 871, 872, 873, 874, 875, 876, 877, 878, 879, 880, 881, 882, 883, 884, 885, 886, 887, 888, 889, 890, 891, 892, 893, 894, 895, 896, 897, 898, 899, 900, 901, 902, 903, 904, 905, 906, 907, 908, 909, 910, 911, 912, 913, 914, 915, 916, 917, 918, 919, 920, 921, 922, 923, 924, 925, 926, 927, 928, 929, 930, 931, 932, 933, 934, 935, 936, 937, 938, 939, 940, 941, 942, 943, 944, 945, 946, 947, 948, 949, 950, 951, 952, 953, 954, 955, 956, 957, 958, 959, 960, 961, 962, 963, 964, 965, 966, 967, 968, 969, 970, 971, 972, 973, 974, 975, 976, 977, 978, 979, 980, 981, 982, 983, 984, 985, 986, 987, 988, 989, 990, 991, 992, 993, 994, 995, 996, 997, 998, 999, 1000) AND \"users\".\"id\" NOT IN (1001) + } + end + end end end end diff --git a/activerecord/test/cases/arel/visitors/oracle_test.rb b/activerecord/test/cases/arel/visitors/oracle_test.rb index 893edc7f74..f69b201855 100644 --- a/activerecord/test/cases/arel/visitors/oracle_test.rb +++ b/activerecord/test/cases/arel/visitors/oracle_test.rb @@ -8,6 +8,7 @@ module Arel before do @visitor = Oracle.new Table.engine.connection @table = Table.new(:users) + @attr = @table[:id] end def compile(node) @@ -231,6 +232,26 @@ module Arel sql.must_be_like %{ "users"."name" IS NOT NULL } end end + + describe "Nodes::In" do + it "should know how to visit" do + ary = (1 .. 1001).to_a + node = @attr.in ary + compile(node).must_be_like %{ + "users"."id" IN (1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27, 28, 29, 30, 31, 32, 33, 34, 35, 36, 37, 38, 39, 40, 41, 42, 43, 44, 45, 46, 47, 48, 49, 50, 51, 52, 53, 54, 55, 56, 57, 58, 59, 60, 61, 62, 63, 64, 65, 66, 67, 68, 69, 70, 71, 72, 73, 74, 75, 76, 77, 78, 79, 80, 81, 82, 83, 84, 85, 86, 87, 88, 89, 90, 91, 92, 93, 94, 95, 96, 97, 98, 99, 100, 101, 102, 103, 104, 105, 106, 107, 108, 109, 110, 111, 112, 113, 114, 115, 116, 117, 118, 119, 120, 121, 122, 123, 124, 125, 126, 127, 128, 129, 130, 131, 132, 133, 134, 135, 136, 137, 138, 139, 140, 141, 142, 143, 144, 145, 146, 147, 148, 149, 150, 151, 152, 153, 154, 155, 156, 157, 158, 159, 160, 161, 162, 163, 164, 165, 166, 167, 168, 169, 170, 171, 172, 173, 174, 175, 176, 177, 178, 179, 180, 181, 182, 183, 184, 185, 186, 187, 188, 189, 190, 191, 192, 193, 194, 195, 196, 197, 198, 199, 200, 201, 202, 203, 204, 205, 206, 207, 208, 209, 210, 211, 212, 213, 214, 215, 216, 217, 218, 219, 220, 221, 222, 223, 224, 225, 226, 227, 228, 229, 230, 231, 232, 233, 234, 235, 236, 237, 238, 239, 240, 241, 242, 243, 244, 245, 246, 247, 248, 249, 250, 251, 252, 253, 254, 255, 256, 257, 258, 259, 260, 261, 262, 263, 264, 265, 266, 267, 268, 269, 270, 271, 272, 273, 274, 275, 276, 277, 278, 279, 280, 281, 282, 283, 284, 285, 286, 287, 288, 289, 290, 291, 292, 293, 294, 295, 296, 297, 298, 299, 300, 301, 302, 303, 304, 305, 306, 307, 308, 309, 310, 311, 312, 313, 314, 315, 316, 317, 318, 319, 320, 321, 322, 323, 324, 325, 326, 327, 328, 329, 330, 331, 332, 333, 334, 335, 336, 337, 338, 339, 340, 341, 342, 343, 344, 345, 346, 347, 348, 349, 350, 351, 352, 353, 354, 355, 356, 357, 358, 359, 360, 361, 362, 363, 364, 365, 366, 367, 368, 369, 370, 371, 372, 373, 374, 375, 376, 377, 378, 379, 380, 381, 382, 383, 384, 385, 386, 387, 388, 389, 390, 391, 392, 393, 394, 395, 396, 397, 398, 399, 400, 401, 402, 403, 404, 405, 406, 407, 408, 409, 410, 411, 412, 413, 414, 415, 416, 417, 418, 419, 420, 421, 422, 423, 424, 425, 426, 427, 428, 429, 430, 431, 432, 433, 434, 435, 436, 437, 438, 439, 440, 441, 442, 443, 444, 445, 446, 447, 448, 449, 450, 451, 452, 453, 454, 455, 456, 457, 458, 459, 460, 461, 462, 463, 464, 465, 466, 467, 468, 469, 470, 471, 472, 473, 474, 475, 476, 477, 478, 479, 480, 481, 482, 483, 484, 485, 486, 487, 488, 489, 490, 491, 492, 493, 494, 495, 496, 497, 498, 499, 500, 501, 502, 503, 504, 505, 506, 507, 508, 509, 510, 511, 512, 513, 514, 515, 516, 517, 518, 519, 520, 521, 522, 523, 524, 525, 526, 527, 528, 529, 530, 531, 532, 533, 534, 535, 536, 537, 538, 539, 540, 541, 542, 543, 544, 545, 546, 547, 548, 549, 550, 551, 552, 553, 554, 555, 556, 557, 558, 559, 560, 561, 562, 563, 564, 565, 566, 567, 568, 569, 570, 571, 572, 573, 574, 575, 576, 577, 578, 579, 580, 581, 582, 583, 584, 585, 586, 587, 588, 589, 590, 591, 592, 593, 594, 595, 596, 597, 598, 599, 600, 601, 602, 603, 604, 605, 606, 607, 608, 609, 610, 611, 612, 613, 614, 615, 616, 617, 618, 619, 620, 621, 622, 623, 624, 625, 626, 627, 628, 629, 630, 631, 632, 633, 634, 635, 636, 637, 638, 639, 640, 641, 642, 643, 644, 645, 646, 647, 648, 649, 650, 651, 652, 653, 654, 655, 656, 657, 658, 659, 660, 661, 662, 663, 664, 665, 666, 667, 668, 669, 670, 671, 672, 673, 674, 675, 676, 677, 678, 679, 680, 681, 682, 683, 684, 685, 686, 687, 688, 689, 690, 691, 692, 693, 694, 695, 696, 697, 698, 699, 700, 701, 702, 703, 704, 705, 706, 707, 708, 709, 710, 711, 712, 713, 714, 715, 716, 717, 718, 719, 720, 721, 722, 723, 724, 725, 726, 727, 728, 729, 730, 731, 732, 733, 734, 735, 736, 737, 738, 739, 740, 741, 742, 743, 744, 745, 746, 747, 748, 749, 750, 751, 752, 753, 754, 755, 756, 757, 758, 759, 760, 761, 762, 763, 764, 765, 766, 767, 768, 769, 770, 771, 772, 773, 774, 775, 776, 777, 778, 779, 780, 781, 782, 783, 784, 785, 786, 787, 788, 789, 790, 791, 792, 793, 794, 795, 796, 797, 798, 799, 800, 801, 802, 803, 804, 805, 806, 807, 808, 809, 810, 811, 812, 813, 814, 815, 816, 817, 818, 819, 820, 821, 822, 823, 824, 825, 826, 827, 828, 829, 830, 831, 832, 833, 834, 835, 836, 837, 838, 839, 840, 841, 842, 843, 844, 845, 846, 847, 848, 849, 850, 851, 852, 853, 854, 855, 856, 857, 858, 859, 860, 861, 862, 863, 864, 865, 866, 867, 868, 869, 870, 871, 872, 873, 874, 875, 876, 877, 878, 879, 880, 881, 882, 883, 884, 885, 886, 887, 888, 889, 890, 891, 892, 893, 894, 895, 896, 897, 898, 899, 900, 901, 902, 903, 904, 905, 906, 907, 908, 909, 910, 911, 912, 913, 914, 915, 916, 917, 918, 919, 920, 921, 922, 923, 924, 925, 926, 927, 928, 929, 930, 931, 932, 933, 934, 935, 936, 937, 938, 939, 940, 941, 942, 943, 944, 945, 946, 947, 948, 949, 950, 951, 952, 953, 954, 955, 956, 957, 958, 959, 960, 961, 962, 963, 964, 965, 966, 967, 968, 969, 970, 971, 972, 973, 974, 975, 976, 977, 978, 979, 980, 981, 982, 983, 984, 985, 986, 987, 988, 989, 990, 991, 992, 993, 994, 995, 996, 997, 998, 999, 1000) OR \"users\".\"id\" IN (1001) + } + end + end + + describe "Nodes::NotIn" do + it "should know how to visit" do + ary = (1 .. 1001).to_a + node = @attr.not_in ary + compile(node).must_be_like %{ + "users"."id" NOT IN (1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27, 28, 29, 30, 31, 32, 33, 34, 35, 36, 37, 38, 39, 40, 41, 42, 43, 44, 45, 46, 47, 48, 49, 50, 51, 52, 53, 54, 55, 56, 57, 58, 59, 60, 61, 62, 63, 64, 65, 66, 67, 68, 69, 70, 71, 72, 73, 74, 75, 76, 77, 78, 79, 80, 81, 82, 83, 84, 85, 86, 87, 88, 89, 90, 91, 92, 93, 94, 95, 96, 97, 98, 99, 100, 101, 102, 103, 104, 105, 106, 107, 108, 109, 110, 111, 112, 113, 114, 115, 116, 117, 118, 119, 120, 121, 122, 123, 124, 125, 126, 127, 128, 129, 130, 131, 132, 133, 134, 135, 136, 137, 138, 139, 140, 141, 142, 143, 144, 145, 146, 147, 148, 149, 150, 151, 152, 153, 154, 155, 156, 157, 158, 159, 160, 161, 162, 163, 164, 165, 166, 167, 168, 169, 170, 171, 172, 173, 174, 175, 176, 177, 178, 179, 180, 181, 182, 183, 184, 185, 186, 187, 188, 189, 190, 191, 192, 193, 194, 195, 196, 197, 198, 199, 200, 201, 202, 203, 204, 205, 206, 207, 208, 209, 210, 211, 212, 213, 214, 215, 216, 217, 218, 219, 220, 221, 222, 223, 224, 225, 226, 227, 228, 229, 230, 231, 232, 233, 234, 235, 236, 237, 238, 239, 240, 241, 242, 243, 244, 245, 246, 247, 248, 249, 250, 251, 252, 253, 254, 255, 256, 257, 258, 259, 260, 261, 262, 263, 264, 265, 266, 267, 268, 269, 270, 271, 272, 273, 274, 275, 276, 277, 278, 279, 280, 281, 282, 283, 284, 285, 286, 287, 288, 289, 290, 291, 292, 293, 294, 295, 296, 297, 298, 299, 300, 301, 302, 303, 304, 305, 306, 307, 308, 309, 310, 311, 312, 313, 314, 315, 316, 317, 318, 319, 320, 321, 322, 323, 324, 325, 326, 327, 328, 329, 330, 331, 332, 333, 334, 335, 336, 337, 338, 339, 340, 341, 342, 343, 344, 345, 346, 347, 348, 349, 350, 351, 352, 353, 354, 355, 356, 357, 358, 359, 360, 361, 362, 363, 364, 365, 366, 367, 368, 369, 370, 371, 372, 373, 374, 375, 376, 377, 378, 379, 380, 381, 382, 383, 384, 385, 386, 387, 388, 389, 390, 391, 392, 393, 394, 395, 396, 397, 398, 399, 400, 401, 402, 403, 404, 405, 406, 407, 408, 409, 410, 411, 412, 413, 414, 415, 416, 417, 418, 419, 420, 421, 422, 423, 424, 425, 426, 427, 428, 429, 430, 431, 432, 433, 434, 435, 436, 437, 438, 439, 440, 441, 442, 443, 444, 445, 446, 447, 448, 449, 450, 451, 452, 453, 454, 455, 456, 457, 458, 459, 460, 461, 462, 463, 464, 465, 466, 467, 468, 469, 470, 471, 472, 473, 474, 475, 476, 477, 478, 479, 480, 481, 482, 483, 484, 485, 486, 487, 488, 489, 490, 491, 492, 493, 494, 495, 496, 497, 498, 499, 500, 501, 502, 503, 504, 505, 506, 507, 508, 509, 510, 511, 512, 513, 514, 515, 516, 517, 518, 519, 520, 521, 522, 523, 524, 525, 526, 527, 528, 529, 530, 531, 532, 533, 534, 535, 536, 537, 538, 539, 540, 541, 542, 543, 544, 545, 546, 547, 548, 549, 550, 551, 552, 553, 554, 555, 556, 557, 558, 559, 560, 561, 562, 563, 564, 565, 566, 567, 568, 569, 570, 571, 572, 573, 574, 575, 576, 577, 578, 579, 580, 581, 582, 583, 584, 585, 586, 587, 588, 589, 590, 591, 592, 593, 594, 595, 596, 597, 598, 599, 600, 601, 602, 603, 604, 605, 606, 607, 608, 609, 610, 611, 612, 613, 614, 615, 616, 617, 618, 619, 620, 621, 622, 623, 624, 625, 626, 627, 628, 629, 630, 631, 632, 633, 634, 635, 636, 637, 638, 639, 640, 641, 642, 643, 644, 645, 646, 647, 648, 649, 650, 651, 652, 653, 654, 655, 656, 657, 658, 659, 660, 661, 662, 663, 664, 665, 666, 667, 668, 669, 670, 671, 672, 673, 674, 675, 676, 677, 678, 679, 680, 681, 682, 683, 684, 685, 686, 687, 688, 689, 690, 691, 692, 693, 694, 695, 696, 697, 698, 699, 700, 701, 702, 703, 704, 705, 706, 707, 708, 709, 710, 711, 712, 713, 714, 715, 716, 717, 718, 719, 720, 721, 722, 723, 724, 725, 726, 727, 728, 729, 730, 731, 732, 733, 734, 735, 736, 737, 738, 739, 740, 741, 742, 743, 744, 745, 746, 747, 748, 749, 750, 751, 752, 753, 754, 755, 756, 757, 758, 759, 760, 761, 762, 763, 764, 765, 766, 767, 768, 769, 770, 771, 772, 773, 774, 775, 776, 777, 778, 779, 780, 781, 782, 783, 784, 785, 786, 787, 788, 789, 790, 791, 792, 793, 794, 795, 796, 797, 798, 799, 800, 801, 802, 803, 804, 805, 806, 807, 808, 809, 810, 811, 812, 813, 814, 815, 816, 817, 818, 819, 820, 821, 822, 823, 824, 825, 826, 827, 828, 829, 830, 831, 832, 833, 834, 835, 836, 837, 838, 839, 840, 841, 842, 843, 844, 845, 846, 847, 848, 849, 850, 851, 852, 853, 854, 855, 856, 857, 858, 859, 860, 861, 862, 863, 864, 865, 866, 867, 868, 869, 870, 871, 872, 873, 874, 875, 876, 877, 878, 879, 880, 881, 882, 883, 884, 885, 886, 887, 888, 889, 890, 891, 892, 893, 894, 895, 896, 897, 898, 899, 900, 901, 902, 903, 904, 905, 906, 907, 908, 909, 910, 911, 912, 913, 914, 915, 916, 917, 918, 919, 920, 921, 922, 923, 924, 925, 926, 927, 928, 929, 930, 931, 932, 933, 934, 935, 936, 937, 938, 939, 940, 941, 942, 943, 944, 945, 946, 947, 948, 949, 950, 951, 952, 953, 954, 955, 956, 957, 958, 959, 960, 961, 962, 963, 964, 965, 966, 967, 968, 969, 970, 971, 972, 973, 974, 975, 976, 977, 978, 979, 980, 981, 982, 983, 984, 985, 986, 987, 988, 989, 990, 991, 992, 993, 994, 995, 996, 997, 998, 999, 1000) AND \"users\".\"id\" NOT IN (1001) + } + end + end end end end diff --git a/activerecord/test/cases/associations/eager_load_nested_include_test.rb b/activerecord/test/cases/associations/eager_load_nested_include_test.rb index 525ad3197a..849939de75 100644 --- a/activerecord/test/cases/associations/eager_load_nested_include_test.rb +++ b/activerecord/test/cases/associations/eager_load_nested_include_test.rb @@ -110,10 +110,10 @@ class EagerLoadNestedIncludeWithMissingDataTest < ActiveRecord::TestCase end teardown do - @davey_mcdave.destroy - @first_post.destroy @first_comment.destroy @first_categorization.destroy + @davey_mcdave.destroy + @first_post.destroy end def test_missing_data_in_a_nested_include_should_not_cause_errors_when_constructing_objects diff --git a/activerecord/test/cases/associations/has_many_associations_test.rb b/activerecord/test/cases/associations/has_many_associations_test.rb index 6a7efe2121..32285f269a 100644 --- a/activerecord/test/cases/associations/has_many_associations_test.rb +++ b/activerecord/test/cases/associations/has_many_associations_test.rb @@ -2578,22 +2578,22 @@ class HasManyAssociationsTest < ActiveRecord::TestCase test "association with extend option" do post = posts(:welcome) - assert_equal "lifo", post.comments_with_extend.author - assert_equal "hello", post.comments_with_extend.greeting + assert_equal "lifo", post.comments_with_extend.author + assert_equal "hello :)", post.comments_with_extend.greeting end test "association with extend option with multiple extensions" do post = posts(:welcome) - assert_equal "lifo", post.comments_with_extend_2.author - assert_equal "hullo", post.comments_with_extend_2.greeting + assert_equal "lifo", post.comments_with_extend_2.author + assert_equal "hullo :)", post.comments_with_extend_2.greeting end test "extend option affects per association" do post = posts(:welcome) - assert_equal "lifo", post.comments_with_extend.author - assert_equal "lifo", post.comments_with_extend_2.author - assert_equal "hello", post.comments_with_extend.greeting - assert_equal "hullo", post.comments_with_extend_2.greeting + assert_equal "lifo", post.comments_with_extend.author + assert_equal "lifo", post.comments_with_extend_2.author + assert_equal "hello :)", post.comments_with_extend.greeting + assert_equal "hullo :)", post.comments_with_extend_2.greeting end test "delete record with complex joins" do diff --git a/activerecord/test/cases/associations/inverse_associations_test.rb b/activerecord/test/cases/associations/inverse_associations_test.rb index da3a42e2b5..669e176dcb 100644 --- a/activerecord/test/cases/associations/inverse_associations_test.rb +++ b/activerecord/test/cases/associations/inverse_associations_test.rb @@ -8,6 +8,7 @@ require "models/zine" require "models/club" require "models/sponsor" require "models/rating" +require "models/post" require "models/comment" require "models/car" require "models/bulb" @@ -62,6 +63,14 @@ class AutomaticInverseFindingTests < ActiveRecord::TestCase assert_equal rating_reflection, comment_reflection.inverse_of, "The Comment reflection's inverse should be the Rating reflection" end + def test_has_many_and_belongs_to_should_find_inverse_automatically_for_extension_block + comment_reflection = Comment.reflect_on_association(:post) + post_reflection = Post.reflect_on_association(:comments) + + assert_predicate post_reflection, :has_inverse? + assert_equal comment_reflection, post_reflection.inverse_of + end + def test_has_many_and_belongs_to_should_find_inverse_automatically_for_sti author_reflection = Author.reflect_on_association(:posts) author_child_reflection = Author.reflect_on_association(:special_posts) diff --git a/activerecord/test/cases/callbacks_test.rb b/activerecord/test/cases/callbacks_test.rb index 4d6a112af5..b4026078f1 100644 --- a/activerecord/test/cases/callbacks_test.rb +++ b/activerecord/test/cases/callbacks_test.rb @@ -458,10 +458,6 @@ class CallbacksTest < ActiveRecord::TestCase [ :before_validation, :object ], [ :before_validation, :block ], [ :before_validation, :throwing_abort ], - [ :after_rollback, :block ], - [ :after_rollback, :object ], - [ :after_rollback, :proc ], - [ :after_rollback, :method ], ], david.history end diff --git a/activerecord/test/cases/connection_adapters/connection_handlers_multi_db_test.rb b/activerecord/test/cases/connection_adapters/connection_handlers_multi_db_test.rb index 36591097b6..d3184f39f5 100644 --- a/activerecord/test/cases/connection_adapters/connection_handlers_multi_db_test.rb +++ b/activerecord/test/cases/connection_adapters/connection_handlers_multi_db_test.rb @@ -203,26 +203,53 @@ module ActiveRecord assert_equal "must provide a `database` or a `role`.", error.message end - def test_switching_connections_with_database_symbol + def test_switching_connections_with_database_symbol_uses_default_role previous_env, ENV["RAILS_ENV"] = ENV["RAILS_ENV"], "default_env" config = { "default_env" => { - "readonly" => { adapter: "sqlite3", database: "db/readonly.sqlite3" }, - "primary" => { adapter: "sqlite3", database: "db/primary.sqlite3" } + "animals" => { adapter: "sqlite3", database: "db/animals.sqlite3" }, + "primary" => { adapter: "sqlite3", database: "db/primary.sqlite3" } } } @prev_configs, ActiveRecord::Base.configurations = ActiveRecord::Base.configurations, config - ActiveRecord::Base.connected_to(database: :readonly) do - assert_equal :readonly, ActiveRecord::Base.current_role - assert ActiveRecord::Base.connected_to?(role: :readonly) + ActiveRecord::Base.connected_to(database: :animals) do + assert_equal :writing, ActiveRecord::Base.current_role + assert ActiveRecord::Base.connected_to?(role: :writing) handler = ActiveRecord::Base.connection_handler - assert_equal handler, ActiveRecord::Base.connection_handlers[:readonly] + assert_equal handler, ActiveRecord::Base.connection_handlers[:writing] + + assert_not_nil pool = handler.retrieve_connection_pool("primary") + assert_equal(config["default_env"]["animals"], pool.spec.config) + end + ensure + ActiveRecord::Base.configurations = @prev_configs + ActiveRecord::Base.establish_connection(:arunit) + ENV["RAILS_ENV"] = previous_env + end + + def test_switching_connections_with_database_hash_uses_passed_role_and_database + previous_env, ENV["RAILS_ENV"] = ENV["RAILS_ENV"], "default_env" + + config = { + "default_env" => { + "animals" => { adapter: "sqlite3", database: "db/animals.sqlite3" }, + "primary" => { adapter: "sqlite3", database: "db/primary.sqlite3" } + } + } + @prev_configs, ActiveRecord::Base.configurations = ActiveRecord::Base.configurations, config + + ActiveRecord::Base.connected_to(database: { writing: :primary }) do + assert_equal :writing, ActiveRecord::Base.current_role + assert ActiveRecord::Base.connected_to?(role: :writing) + + handler = ActiveRecord::Base.connection_handler + assert_equal handler, ActiveRecord::Base.connection_handlers[:writing] assert_not_nil pool = handler.retrieve_connection_pool("primary") - assert_equal(config["default_env"]["readonly"], pool.spec.config) + assert_equal(config["default_env"]["primary"], pool.spec.config) end ensure ActiveRecord::Base.configurations = @prev_configs diff --git a/activerecord/test/cases/invertible_migration_test.rb b/activerecord/test/cases/invertible_migration_test.rb index d68cc40107..7f67b945f0 100644 --- a/activerecord/test/cases/invertible_migration_test.rb +++ b/activerecord/test/cases/invertible_migration_test.rb @@ -103,6 +103,32 @@ module ActiveRecord end end + class ChangeColumnComment1 < SilentMigration + def change + create_table("horses") do |t| + t.column :name, :string, comment: "Sekitoba" + end + end + end + + class ChangeColumnComment2 < SilentMigration + def change + change_column_comment :horses, :name, from: "Sekitoba", to: "Diomed" + end + end + + class ChangeTableComment1 < SilentMigration + def change + create_table("horses", comment: "Sekitoba") + end + end + + class ChangeTableComment2 < SilentMigration + def change + change_table_comment :horses, from: "Sekitoba", to: "Diomed" + end + end + class DisableExtension1 < SilentMigration def change enable_extension "hstore" @@ -290,6 +316,7 @@ module ActiveRecord def test_migrate_revert_change_column_default migration1 = ChangeColumnDefault1.new migration1.migrate(:up) + Horse.reset_column_information assert_equal "Sekitoba", Horse.new.name migration2 = ChangeColumnDefault2.new @@ -302,6 +329,38 @@ module ActiveRecord assert_equal "Sekitoba", Horse.new.name end + if ActiveRecord::Base.connection.supports_comments? + def test_migrate_revert_change_column_comment + migration1 = ChangeColumnComment1.new + migration1.migrate(:up) + Horse.reset_column_information + assert_equal "Sekitoba", Horse.columns_hash["name"].comment + + migration2 = ChangeColumnComment2.new + migration2.migrate(:up) + Horse.reset_column_information + assert_equal "Diomed", Horse.columns_hash["name"].comment + + migration2.migrate(:down) + Horse.reset_column_information + assert_equal "Sekitoba", Horse.columns_hash["name"].comment + end + + def test_migrate_revert_change_table_comment + connection = ActiveRecord::Base.connection + migration1 = ChangeTableComment1.new + migration1.migrate(:up) + assert_equal "Sekitoba", connection.table_comment("horses") + + migration2 = ChangeTableComment2.new + migration2.migrate(:up) + assert_equal "Diomed", connection.table_comment("horses") + + migration2.migrate(:down) + assert_equal "Sekitoba", connection.table_comment("horses") + end + end + if current_adapter?(:PostgreSQLAdapter) def test_migrate_enable_and_disable_extension migration1 = InvertibleMigration.new diff --git a/activerecord/test/cases/locking_test.rb b/activerecord/test/cases/locking_test.rb index 33bd74e114..04f9b26960 100644 --- a/activerecord/test/cases/locking_test.rb +++ b/activerecord/test/cases/locking_test.rb @@ -182,7 +182,9 @@ class OptimisticLockingTest < ActiveRecord::TestCase p1.touch assert_equal 1, p1.lock_version - assert_not p1.changed?, "Changes should have been cleared" + assert_not_predicate p1, :changed?, "Changes should have been cleared" + assert_predicate p1, :saved_changes? + assert_equal ["lock_version", "updated_at"], p1.saved_changes.keys.sort end def test_touch_stale_object @@ -193,6 +195,8 @@ class OptimisticLockingTest < ActiveRecord::TestCase assert_raises(ActiveRecord::StaleObjectError) do stale_person.touch end + + assert_not_predicate stale_person, :saved_changes? end def test_update_with_dirty_primary_key @@ -296,6 +300,9 @@ class OptimisticLockingTest < ActiveRecord::TestCase t1.touch assert_equal 1, t1.lock_version + assert_not_predicate t1, :changed? + assert_predicate t1, :saved_changes? + assert_equal ["lock_version", "updated_at"], t1.saved_changes.keys.sort end def test_touch_stale_object_with_lock_without_default @@ -307,6 +314,8 @@ class OptimisticLockingTest < ActiveRecord::TestCase assert_raises(ActiveRecord::StaleObjectError) do stale_object.touch end + + assert_not_predicate stale_object, :saved_changes? end def test_lock_without_default_should_work_with_null_in_the_database diff --git a/activerecord/test/cases/migration/command_recorder_test.rb b/activerecord/test/cases/migration/command_recorder_test.rb index 01f8628fc5..c9f3756b1f 100644 --- a/activerecord/test/cases/migration/command_recorder_test.rb +++ b/activerecord/test/cases/migration/command_recorder_test.rb @@ -182,6 +182,40 @@ module ActiveRecord assert_equal [:change_column_default, [:table, :column, from: false, to: true]], change end + if ActiveRecord::Base.connection.supports_comments? + def test_invert_change_column_comment + assert_raises(ActiveRecord::IrreversibleMigration) do + @recorder.inverse_of :change_column_comment, [:table, :column, "comment"] + end + end + + def test_invert_change_column_comment_with_from_and_to + change = @recorder.inverse_of :change_column_comment, [:table, :column, from: "old_value", to: "new_value"] + assert_equal [:change_column_comment, [:table, :column, from: "new_value", to: "old_value"]], change + end + + def test_invert_change_column_comment_with_from_and_to_with_nil + change = @recorder.inverse_of :change_column_comment, [:table, :column, from: nil, to: "new_value"] + assert_equal [:change_column_comment, [:table, :column, from: "new_value", to: nil]], change + end + + def test_invert_change_table_comment + assert_raises(ActiveRecord::IrreversibleMigration) do + @recorder.inverse_of :change_column_comment, [:table, :column, "comment"] + end + end + + def test_invert_change_table_comment_with_from_and_to + change = @recorder.inverse_of :change_table_comment, [:table, from: "old_value", to: "new_value"] + assert_equal [:change_table_comment, [:table, from: "new_value", to: "old_value"]], change + end + + def test_invert_change_table_comment_with_from_and_to_with_nil + change = @recorder.inverse_of :change_table_comment, [:table, from: nil, to: "new_value"] + assert_equal [:change_table_comment, [:table, from: "new_value", to: nil]], change + end + end + def test_invert_change_column_null add = @recorder.inverse_of :change_column_null, [:table, :column, true] assert_equal [:change_column_null, [:table, :column, false]], add diff --git a/activerecord/test/cases/migration/compatibility_test.rb b/activerecord/test/cases/migration/compatibility_test.rb index 5753bd7117..726ccf925e 100644 --- a/activerecord/test/cases/migration/compatibility_test.rb +++ b/activerecord/test/cases/migration/compatibility_test.rb @@ -220,6 +220,35 @@ module ActiveRecord end end + if ActiveRecord::Base.connection.supports_comments? + def test_change_column_comment_can_be_reverted + migration = Class.new(ActiveRecord::Migration[5.2]) { + def migrate(x) + revert do + change_column_comment(:testings, :foo, "comment") + end + end + }.new + + ActiveRecord::Migrator.new(:up, [migration]).migrate + assert connection.column_exists?(:testings, :foo, comment: "comment") + end + + def test_change_table_comment_can_be_reverted + migration = Class.new(ActiveRecord::Migration[5.2]) { + def migrate(x) + revert do + change_table_comment(:testings, "comment") + end + end + }.new + + ActiveRecord::Migrator.new(:up, [migration]).migrate + + assert_equal "comment", connection.table_comment("testings") + end + end + if current_adapter?(:PostgreSQLAdapter) class Testing < ActiveRecord::Base end @@ -242,9 +271,9 @@ module ActiveRecord private def precision_implicit_default if current_adapter?(:Mysql2Adapter) - { presicion: 0 } + { precision: 0 } else - { presicion: nil } + { precision: nil } end end end diff --git a/activerecord/test/cases/timestamp_test.rb b/activerecord/test/cases/timestamp_test.rb index 75ecd6fc40..232e018e03 100644 --- a/activerecord/test/cases/timestamp_test.rb +++ b/activerecord/test/cases/timestamp_test.rb @@ -40,17 +40,25 @@ class TimestampTest < ActiveRecord::TestCase assert_not_equal @previously_updated_at, @developer.updated_at assert_equal previous_salary + 10000, @developer.salary - assert @developer.salary_changed?, "developer salary should have changed" - assert @developer.changed?, "developer should be marked as changed" + assert_predicate @developer, :salary_changed?, "developer salary should have changed" + assert_predicate @developer, :changed?, "developer should be marked as changed" + assert_equal ["salary"], @developer.changed + assert_predicate @developer, :saved_changes? + assert_equal ["updated_at", "updated_on"], @developer.saved_changes.keys.sort + @developer.reload assert_equal previous_salary, @developer.salary end def test_touching_a_record_with_default_scope_that_excludes_it_updates_its_timestamp developer = @developer.becomes(DeveloperCalledJamis) - developer.touch + assert_not_equal @previously_updated_at, developer.updated_at + assert_not_predicate developer, :changed? + assert_predicate developer, :saved_changes? + assert_equal ["updated_at", "updated_on"], developer.saved_changes.keys.sort + developer.reload assert_not_equal @previously_updated_at, developer.updated_at end diff --git a/activerecord/test/cases/transaction_callbacks_test.rb b/activerecord/test/cases/transaction_callbacks_test.rb index e88d20a453..53fe31e087 100644 --- a/activerecord/test/cases/transaction_callbacks_test.rb +++ b/activerecord/test/cases/transaction_callbacks_test.rb @@ -111,6 +111,32 @@ class TransactionCallbacksTest < ActiveRecord::TestCase assert_equal [:after_commit], @first.history end + def test_dont_call_any_callbacks_after_transaction_commits_for_invalid_record + @first.after_commit_block { |r| r.history << :after_commit } + @first.after_rollback_block { |r| r.history << :after_rollback } + + def @first.valid?(*) + false + end + + assert_not @first.save + assert_equal [], @first.history + end + + def test_dont_call_any_callbacks_after_explicit_transaction_commits_for_invalid_record + @first.after_commit_block { |r| r.history << :after_commit } + @first.after_rollback_block { |r| r.history << :after_rollback } + + def @first.valid?(*) + false + end + + @first.transaction do + assert_not @first.save + end + assert_equal [], @first.history + end + def test_only_call_after_commit_on_save_after_transaction_commits_for_saving_record record = TopicWithCallbacks.new(title: "New topic", written_on: Date.today) record.after_commit_block(:save) { |r| r.history << :after_save } @@ -598,7 +624,7 @@ class TransactionEnrollmentCallbacksTest < ActiveRecord::TestCase @topic.content = "foo" @topic.save! end - @topic.class.connection.add_transaction_record(@topic) + @topic.send(:add_to_transaction) end assert_equal [:before_commit, :after_commit], @topic.history end @@ -608,7 +634,7 @@ class TransactionEnrollmentCallbacksTest < ActiveRecord::TestCase @topic.transaction(requires_new: true) do @topic.content = "foo" @topic.save! - @topic.class.connection.add_transaction_record(@topic) + @topic.send(:add_to_transaction) end end assert_equal [:before_commit, :after_commit], @topic.history @@ -629,7 +655,7 @@ class TransactionEnrollmentCallbacksTest < ActiveRecord::TestCase @topic.content = "foo" @topic.save! end - @topic.class.connection.add_transaction_record(@topic) + @topic.send(:add_to_transaction) raise ActiveRecord::Rollback end assert_equal [:rollback], @topic.history diff --git a/activerecord/test/cases/transactions_test.rb b/activerecord/test/cases/transactions_test.rb index 410f07d3ab..7bad3de343 100644 --- a/activerecord/test/cases/transactions_test.rb +++ b/activerecord/test/cases/transactions_test.rb @@ -26,13 +26,15 @@ class TransactionTest < ActiveRecord::TestCase def test_raise_after_destroy assert_not_predicate @first, :frozen? - assert_raises(RuntimeError) { - Topic.transaction do - @first.destroy - assert_predicate @first, :frozen? - raise + assert_not_called(@first, :rolledback!) do + assert_raises(RuntimeError) do + Topic.transaction do + @first.destroy + assert_predicate @first, :frozen? + raise + end end - } + end assert_not_predicate @first, :frozen? end @@ -63,7 +65,7 @@ class TransactionTest < ActiveRecord::TestCase def test_add_to_null_transaction topic = Topic.new - topic.add_to_transaction + topic.send(:add_to_transaction) end def test_successful_with_return diff --git a/activerecord/test/cases/validations_test.rb b/activerecord/test/cases/validations_test.rb index 9a70934b7e..4f98a6b7fc 100644 --- a/activerecord/test/cases/validations_test.rb +++ b/activerecord/test/cases/validations_test.rb @@ -145,15 +145,17 @@ class ValidationsTest < ActiveRecord::TestCase end def test_validates_acceptance_of_with_undefined_attribute_methods - Topic.validates_acceptance_of(:approved) - topic = Topic.new(approved: true) - Topic.undefine_attribute_methods + klass = Class.new(Topic) + klass.validates_acceptance_of(:approved) + topic = klass.new(approved: true) + klass.undefine_attribute_methods assert topic.approved end def test_validates_acceptance_of_as_database_column - Topic.validates_acceptance_of(:approved) - topic = Topic.create("approved" => true) + klass = Class.new(Topic) + klass.validates_acceptance_of(:approved) + topic = klass.create("approved" => true) assert topic["approved"] end diff --git a/activerecord/test/models/post.rb b/activerecord/test/models/post.rb index 395b534c63..c34968590f 100644 --- a/activerecord/test/models/post.rb +++ b/activerecord/test/models/post.rb @@ -11,6 +11,10 @@ class Post < ActiveRecord::Base def author "lifo" end + + def greeting + super + " :)" + end end module NamedExtension2 diff --git a/activesupport/activesupport.gemspec b/activesupport/activesupport.gemspec index 546cc1797a..dd2340cdd3 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", "~> 2.1" + s.add_dependency "zeitwerk", "~> 2.1", ">= 2.1.2" end diff --git a/activesupport/lib/active_support/backtrace_cleaner.rb b/activesupport/lib/active_support/backtrace_cleaner.rb index 62973eca58..02cbfbaee6 100644 --- a/activesupport/lib/active_support/backtrace_cleaner.rb +++ b/activesupport/lib/active_support/backtrace_cleaner.rb @@ -122,7 +122,11 @@ module ActiveSupport end def noise(backtrace) - backtrace - silence(backtrace) + backtrace.select do |line| + @silencers.any? do |s| + s.call(line) + end + end end end end diff --git a/activesupport/lib/active_support/cache/redis_cache_store.rb b/activesupport/lib/active_support/cache/redis_cache_store.rb index 9a55e49e27..87f9aa5346 100644 --- a/activesupport/lib/active_support/cache/redis_cache_store.rb +++ b/activesupport/lib/active_support/cache/redis_cache_store.rb @@ -361,6 +361,7 @@ module ActiveSupport def read_multi_mget(*names) options = names.extract_options! options = merged_options(options) + return {} if names == [] keys = names.map { |name| normalize_key(name, options) } diff --git a/activesupport/lib/active_support/dependencies/zeitwerk_integration.rb b/activesupport/lib/active_support/dependencies/zeitwerk_integration.rb index b0f7a3f9cc..d5dc7c2ff4 100644 --- a/activesupport/lib/active_support/dependencies/zeitwerk_integration.rb +++ b/activesupport/lib/active_support/dependencies/zeitwerk_integration.rb @@ -10,6 +10,8 @@ module ActiveSupport def clear Dependencies.unload_interlock do Rails.autoloaders.main.reload + rescue Zeitwerk::ReloadingDisabledError + raise "reloading is disabled because config.cache_classes is true" end end diff --git a/activesupport/test/cache/stores/redis_cache_store_test.rb b/activesupport/test/cache/stores/redis_cache_store_test.rb index 1d87f74347..790534cd3c 100644 --- a/activesupport/test/cache/stores/redis_cache_store_test.rb +++ b/activesupport/test/cache/stores/redis_cache_store_test.rb @@ -140,6 +140,12 @@ module ActiveSupport::Cache::RedisCacheStoreTests end end + def test_fetch_multi_without_names + assert_not_called(@cache.redis, :mget) do + @cache.fetch_multi() { } + end + end + def test_increment_expires_in assert_called_with @cache.redis, :incrby, [ "#{@namespace}:foo", 1 ] do assert_called_with @cache.redis, :expire, [ "#{@namespace}:foo", 60 ] do diff --git a/guides/source/action_mailbox_basics.md b/guides/source/action_mailbox_basics.md index c90892d456..de92401226 100644 --- a/guides/source/action_mailbox_basics.md +++ b/guides/source/action_mailbox_basics.md @@ -19,9 +19,9 @@ Introduction ------------ Action Mailbox routes incoming emails to controller-like mailboxes for -processing in Rails. It ships with ingresses for Amazon SES, Mailgun, Mandrill, -Postmark, and SendGrid. You can also handle inbound mails directly via the -built-in Exim, Postfix, and Qmail ingresses. +processing in Rails. It ships with ingresses for Mailgun, Mandrill, Postmark, +and SendGrid. You can also handle inbound mails directly via the built-in Exim, +Postfix, and Qmail ingresses. The inbound emails are turned into `InboundEmail` records using Active Record and feature lifecycle tracking, storage of the original email on cloud storage @@ -43,28 +43,6 @@ $ rails db:migrate ## Configuration -### Amazon SES - -Install the [`aws-sdk-sns`](https://rubygems.org/gems/aws-sdk-sns) gem: - -```ruby -# Gemfile -gem "aws-sdk-sns", ">= 1.9.0", require: false -``` - -Tell Action Mailbox to accept emails from SES: - -```ruby -# config/environments/production.rb -config.action_mailbox.ingress = :amazon -``` - -[Configure SES](https://docs.aws.amazon.com/ses/latest/DeveloperGuide/receiving-email-notifications.html) -to deliver emails to your application via POST requests to -`/rails/action_mailbox/amazon/inbound_emails`. If your application lived at -`https://example.com`, you would specify the fully-qualified URL -`https://example.com/rails/action_mailbox/amazon/inbound_emails`. - ### Exim Tell Action Mailbox to accept emails from an SMTP relay: diff --git a/guides/source/command_line.md b/guides/source/command_line.md index a83724f1bb..0091271363 100644 --- a/guides/source/command_line.md +++ b/guides/source/command_line.md @@ -481,6 +481,22 @@ lib/school.rb: * [ 17] [FIXME] ``` +#### Tags + +You can add more default tags to search for by using `config.annotations.register_tags`. It receives a list of tags. + +```ruby +config.annotations.register_tags("DEPRECATEME", "TESTME") +``` + +```bash +$ rails notes +app/controllers/admin/users_controller.rb: + * [ 20] [TODO] do A/B testing on this + * [ 42] [TESTME] this needs more functional tests + * [132] [DEPRECATEME] ensure this method is deprecated in next release +``` + #### Directories You can add more default directories to search from by using `config.annotations.register_directories`. It receives a list of directory names. diff --git a/guides/source/documents.yaml b/guides/source/documents.yaml index 25e4fdb4e6..1e67b2bce7 100644 --- a/guides/source/documents.yaml +++ b/guides/source/documents.yaml @@ -155,6 +155,11 @@ name: Using Rails for API-only Applications url: api_app.html description: This guide explains how to effectively use Rails to develop a JSON API application. + - + name: Active Record and PostgreSQL + work_in_progress: true + url: active_record_postgresql.html + description: This guide covers PostgreSQL specific usage of Active Record. - name: Extending Rails diff --git a/guides/source/engines.md b/guides/source/engines.md index 068f1efae3..3031c62928 100644 --- a/guides/source/engines.md +++ b/guides/source/engines.md @@ -218,9 +218,7 @@ important parts about namespacing, and is discussed later in the Inside the `app` directory are the standard `assets`, `controllers`, `helpers`, `jobs`, `mailers`, `models`, and `views` directories that you should be familiar with -from an application. The `helpers`, `mailers`, and `models` directories are -empty, so they aren't described in this section. We'll look more into models in -a future section, when we're writing the engine. +from an application. We'll look more into models in a future section, when we're writing the engine. Within the `app/assets` directory, there are the `images`, `javascripts` and `stylesheets` directories which, again, you should be familiar with due to their @@ -261,6 +259,30 @@ WARNING: Don't use `require` because it will break the automatic reloading of cl in the development environment - using `require_dependency` ensures that classes are loaded and unloaded in the correct manner. +Within the `app/helpers` directory there is a `blorgh` directory that +contains a file called `application_helper.rb`. This file will provide any +common functionality for the helpers of the engine. The `blorgh` directory +is where the other helpers for the engine will go. By placing them within +this namespaced directory, you prevent them from possibly clashing with +identically-named helpers within other engines or even within the +application. + +Within the `app/jobs` directory there is a `blorgh` directory that +contains a file called `application_job.rb`. This file will provide any +common functionality for the jobs of the engine. The `blorgh` directory +is where the other jobs for the engine will go. By placing them within +this namespaced directory, you prevent them from possibly clashing with +identically-named jobs within other engines or even within the +application. + +Within the `app/mailers` directory there is a `blorgh` directory that +contains a file called `application_mailer.rb`. This file will provide any +common functionality for the mailers of the engine. The `blorgh` directory +is where the other mailers for the engine will go. By placing them within +this namespaced directory, you prevent them from possibly clashing with +identically-named mailers within other engines or even within the +application. + Lastly, the `app/views` directory contains a `layouts` folder, which contains a file at `blorgh/application.html.erb`. This file allows you to specify a layout for the engine. If this engine is to be used as a stand-alone engine, then you @@ -1129,10 +1151,11 @@ end ``` ```ruby -# Blorgh/app/models/article.rb - -class Article < ApplicationRecord - has_many :comments +# Blorgh/app/models/blorgh/article.rb +module Blorgh + class Article < ApplicationRecord + has_many :comments + end end ``` @@ -1150,12 +1173,13 @@ end ``` ```ruby -# Blorgh/app/models/article.rb - -class Article < ApplicationRecord - has_many :comments - def summary - "#{title}" +# Blorgh/app/models/blorgh/article.rb +module Blorgh + class Article < ApplicationRecord + has_many :comments + def summary + "#{title}" + end end end ``` @@ -1187,10 +1211,11 @@ end ``` ```ruby -# Blorgh/app/models/article.rb - -class Article < ApplicationRecord - include Blorgh::Concerns::Models::Article +# Blorgh/app/models/blorgh/article.rb +module Blorgh + class Article < ApplicationRecord + include Blorgh::Concerns::Models::Article + end end ``` diff --git a/railties/CHANGELOG.md b/railties/CHANGELOG.md index ab400e5982..109c4836d5 100644 --- a/railties/CHANGELOG.md +++ b/railties/CHANGELOG.md @@ -1,3 +1,12 @@ +* New applications get `config.cache_classes = false` in `config/environments/test.rb` + unless `--skip-spring`. + + *Xavier Noria* + +* Autoloading during initialization is deprecated. + + *Xavier Noria* + * Only force `:async` ActiveJob adapter to `:inline` during seeding. *BatedUrGonnaDie* diff --git a/railties/lib/rails/application/finisher.rb b/railties/lib/rails/application/finisher.rb index 1cf44a480c..109c560c80 100644 --- a/railties/lib/rails/application/finisher.rb +++ b/railties/lib/rails/application/finisher.rb @@ -1,5 +1,8 @@ # frozen_string_literal: true +require "active_support/core_ext/string/inflections" +require "active_support/core_ext/array/conversions" + module Rails class Application module Finisher @@ -21,6 +24,40 @@ module Rails end end + # This will become an error if/when we remove classic mode. The plan is + # autoloaders won't be configured up to this point in the finisher, so + # constants just won't be found, raising regular NameError exceptions. + initializer :warn_if_autoloaded, before: :let_zeitwerk_take_over do + next if config.cache_classes + next if ActiveSupport::Dependencies.autoloaded_constants.empty? + + autoloaded = ActiveSupport::Dependencies.autoloaded_constants + constants = "constant".pluralize(autoloaded.size) + enum = autoloaded.to_sentence + have = autoloaded.size == 1 ? "has" : "have" + these = autoloaded.size == 1 ? "This" : "These" + example = autoloaded.first + example_klass = example.constantize.class + + ActiveSupport::DescendantsTracker.clear + ActiveSupport::Dependencies.clear + + ActiveSupport::Deprecation.warn(<<~WARNING) + Initialization autoloaded the #{constants} #{enum}. + + Being able to do this is deprecated. Autoloading during initialization is going + to be an error condition in future versions of Rails. + + Reloading does not reboot the application, and therefore code executed during + initialization does not run again. So, if you reload #{example}, for example, + the expected changes won't be reflected in that stale #{example_klass} object. + + #{these} autoloaded #{constants} #{have} been unloaded. + + Please, check the "Autoloading and Reloading Constants" guide for solutions. + WARNING + end + initializer :let_zeitwerk_take_over do if config.autoloader == :zeitwerk require "active_support/dependencies/zeitwerk_integration" diff --git a/railties/lib/rails/command/environment_argument.rb b/railties/lib/rails/command/environment_argument.rb index 0cb3f1ce1e..9945fd1430 100644 --- a/railties/lib/rails/command/environment_argument.rb +++ b/railties/lib/rails/command/environment_argument.rb @@ -9,7 +9,9 @@ module Rails extend ActiveSupport::Concern included do - class_attribute :environment_desc, default: "Specifies the environment to run this #{self.command_name} under (test/development/production)." + no_commands do + class_attribute :environment_desc, default: "Specifies the environment to run this #{self.command_name} under (test/development/production)." + end class_option :environment, aliases: "-e", type: :string, desc: environment_desc end diff --git a/railties/lib/rails/commands/notes/notes_command.rb b/railties/lib/rails/commands/notes/notes_command.rb index 64b339b3cd..94cf183855 100644 --- a/railties/lib/rails/commands/notes/notes_command.rb +++ b/railties/lib/rails/commands/notes/notes_command.rb @@ -5,7 +5,7 @@ require "rails/source_annotation_extractor" module Rails module Command class NotesCommand < Base # :nodoc: - class_option :annotations, aliases: "-a", desc: "Filter by specific annotations, e.g. Foobar TODO", type: :array, default: %w(OPTIMIZE FIXME TODO) + class_option :annotations, aliases: "-a", desc: "Filter by specific annotations, e.g. Foobar TODO", type: :array, default: Rails::SourceAnnotationExtractor::Annotation.tags def perform(*) require_application_and_environment! diff --git a/railties/lib/rails/generators/rails/app/templates/config/environments/test.rb.tt b/railties/lib/rails/generators/rails/app/templates/config/environments/test.rb.tt index 63ed3fa952..c66e349442 100644 --- a/railties/lib/rails/generators/rails/app/templates/config/environments/test.rb.tt +++ b/railties/lib/rails/generators/rails/app/templates/config/environments/test.rb.tt @@ -1,11 +1,16 @@ +# The test environment is used exclusively to run your application's +# test suite. You never need to work with it otherwise. Remember that +# your test database is "scratch space" for the test suite and is wiped +# and recreated between test runs. Don't rely on the data there! + Rails.application.configure do # Settings specified here will take precedence over those in config/application.rb. - - # The test environment is used exclusively to run your application's - # test suite. You never need to work with it otherwise. Remember that - # your test database is "scratch space" for the test suite and is wiped - # and recreated between test runs. Don't rely on the data there! + <%# Spring executes the reloaders when files change. %> + <%- if spring_install? -%> + config.cache_classes = false + <%- else -%> config.cache_classes = true + <%- end -%> # Do not eager load code on boot. This avoids loading your whole application # just for the purpose of running a single test. If you are using a tool that diff --git a/railties/lib/rails/source_annotation_extractor.rb b/railties/lib/rails/source_annotation_extractor.rb index d7170e6282..9ce22b96a6 100644 --- a/railties/lib/rails/source_annotation_extractor.rb +++ b/railties/lib/rails/source_annotation_extractor.rb @@ -29,6 +29,16 @@ module Rails directories.push(*dirs) end + def self.tags + @@tags ||= %w(OPTIMIZE FIXME TODO) + end + + # Registers additional tags + # Rails::SourceAnnotationExtractor::Annotation.register_tags("TESTME", "DEPRECATEME") + def self.register_tags(*additional_tags) + tags.push(*additional_tags) + end + def self.extensions @@extensions ||= {} end @@ -66,6 +76,8 @@ module Rails # Prints all annotations with tag +tag+ under the root directories +app+, # +config+, +db+, +lib+, and +test+ (recursively). # + # If +tag+ is <tt>nil</tt>, annotations with either default or registered tags are printed. + # # Specific directories can be explicitly set using the <tt>:dirs</tt> key in +options+. # # Rails::SourceAnnotationExtractor.enumerate 'TODO|FIXME', dirs: %w(app lib), tag: true @@ -75,7 +87,8 @@ module Rails # See <tt>#find_in</tt> for a list of file extensions that will be taken into account. # # This class method is the single entry point for the `rails notes` command. - def self.enumerate(tag, options = {}) + def self.enumerate(tag = nil, options = {}) + tag ||= Annotation.tags.join("|") extractor = new(tag) dirs = options.delete(:dirs) || Annotation.directories extractor.display(extractor.find(dirs), options) diff --git a/railties/test/application/configuration_test.rb b/railties/test/application/configuration_test.rb index b8e167b488..a2e3e781c0 100644 --- a/railties/test/application/configuration_test.rb +++ b/railties/test/application/configuration_test.rb @@ -1704,6 +1704,61 @@ module ApplicationTests end end + test "autoloading during initialization gets deprecation message and clearing if config.cache_classes is false" do + app_file "lib/c.rb", <<~EOS + class C + extend ActiveSupport::DescendantsTracker + end + + class X < C + end + EOS + + app_file "app/models/d.rb", <<~EOS + require "c" + + class D < C + end + EOS + + app_file "config/initializers/autoload.rb", "D" + + app "development" + + # TODO: Test deprecation message, assert_depcrecated { app "development" } + # does not collect it. + + assert_equal [X], C.descendants + assert_empty ActiveSupport::Dependencies.autoloaded_constants + end + + test "autoloading during initialization triggers nothing if config.cache_classes is true" do + app_file "lib/c.rb", <<~EOS + class C + extend ActiveSupport::DescendantsTracker + end + + class X < C + end + EOS + + app_file "app/models/d.rb", <<~EOS + require "c" + + class D < C + end + EOS + + app_file "config/initializers/autoload.rb", "D" + + app "production" + + # TODO: Test no deprecation message is issued. + + assert_equal [X, D], C.descendants + end + + test "raises with proper error message if no database configuration found" do FileUtils.rm("#{app_path}/config/database.yml") err = assert_raises RuntimeError do diff --git a/railties/test/application/rake/routes_test.rb b/railties/test/application/rake/routes_test.rb index 9879d1f047..dffdae7bde 100644 --- a/railties/test/application/rake/routes_test.rb +++ b/railties/test/application/rake/routes_test.rb @@ -20,7 +20,6 @@ module ApplicationTests assert_equal <<~MESSAGE, run_rake_routes Prefix Verb URI Pattern Controller#Action cart GET /cart(.:format) cart#show - rails_amazon_inbound_emails POST /rails/action_mailbox/amazon/inbound_emails(.:format) action_mailbox/ingresses/amazon/inbound_emails#create rails_mandrill_inbound_emails POST /rails/action_mailbox/mandrill/inbound_emails(.:format) action_mailbox/ingresses/mandrill/inbound_emails#create rails_postmark_inbound_emails POST /rails/action_mailbox/postmark/inbound_emails(.:format) action_mailbox/ingresses/postmark/inbound_emails#create rails_relay_inbound_emails POST /rails/action_mailbox/relay/inbound_emails(.:format) action_mailbox/ingresses/relay/inbound_emails#create diff --git a/railties/test/application/zeitwerk_integration_test.rb b/railties/test/application/zeitwerk_integration_test.rb index b248459e47..40d06ee999 100644 --- a/railties/test/application/zeitwerk_integration_test.rb +++ b/railties/test/application/zeitwerk_integration_test.rb @@ -156,6 +156,15 @@ class ZeitwerkIntegrationTest < ActiveSupport::TestCase assert_not Rails.autoloaders.once.reloading_enabled? end + test "reloading raises if config.cache_classes is true" do + boot("production") + + e = assert_raises(StandardError) do + deps.clear + end + assert_equal "reloading is disabled because config.cache_classes is true", e.message + end + test "eager loading loads code in engines" do $test_blog_engine_eager_loaded = false diff --git a/railties/test/backtrace_cleaner_test.rb b/railties/test/backtrace_cleaner_test.rb index ec512b6b64..6de23acebe 100644 --- a/railties/test/backtrace_cleaner_test.rb +++ b/railties/test/backtrace_cleaner_test.rb @@ -17,6 +17,16 @@ class BacktraceCleanerTest < ActiveSupport::TestCase assert_equal 1, result.length end + test "can filter for noise" do + backtrace = [ "(irb):1", + "/Path/to/rails/railties/lib/rails/commands/console.rb:77:in `start'", + "bin/rails:4:in `<main>'" ] + result = @cleaner.clean(backtrace, :noise) + assert_equal "/Path/to/rails/railties/lib/rails/commands/console.rb:77:in `start'", result[0] + assert_equal "bin/rails:4:in `<main>'", result[1] + assert_equal 2, result.length + end + test "should omit ActionView template methods names" do method_name = ActionView::Template.new(nil, "app/views/application/index.html.erb", nil, locals: []).send :method_name backtrace = [ "app/views/application/index.html.erb:4:in `block in #{method_name}'"] diff --git a/railties/test/commands/notes_test.rb b/railties/test/commands/notes_test.rb index 147019e299..9182541413 100644 --- a/railties/test/commands/notes_test.rb +++ b/railties/test/commands/notes_test.rb @@ -121,6 +121,47 @@ class Rails::Command::NotesTest < ActiveSupport::TestCase OUTPUT end + test "displays results from additional tags added to the default tags from a config file" do + app_file "app/models/profile.rb", "# TESTME: some method to test" + app_file "app/controllers/hello_controller.rb", "# DEPRECATEME: this action is no longer needed" + app_file "db/some_seeds.rb", "# TODO: default tags such as TODO are still present" + + add_to_config 'config.annotations.register_tags "TESTME", "DEPRECATEME"' + + assert_equal <<~OUTPUT, run_notes_command + app/controllers/hello_controller.rb: + * [1] [DEPRECATEME] this action is no longer needed + + app/models/profile.rb: + * [1] [TESTME] some method to test + + db/some_seeds.rb: + * [1] [TODO] default tags such as TODO are still present + + OUTPUT + end + + test "does not display results from tags that are neither default nor registered" do + app_file "app/models/profile.rb", "# TESTME: some method to test" + app_file "app/controllers/hello_controller.rb", "# DEPRECATEME: this action is no longer needed" + app_file "db/some_seeds.rb", "# TODO: default tags such as TODO are still present" + app_file "db/some_other_seeds.rb", "# BAD: this note should not be listed" + + add_to_config 'config.annotations.register_tags "TESTME", "DEPRECATEME"' + + assert_equal <<~OUTPUT, run_notes_command + app/controllers/hello_controller.rb: + * [1] [DEPRECATEME] this action is no longer needed + + app/models/profile.rb: + * [1] [TESTME] some method to test + + db/some_seeds.rb: + * [1] [TODO] default tags such as TODO are still present + + OUTPUT + end + private def run_notes_command(args = []) rails "notes", args diff --git a/railties/test/commands/routes_test.rb b/railties/test/commands/routes_test.rb index b4f927060e..a2dbd944f5 100644 --- a/railties/test/commands/routes_test.rb +++ b/railties/test/commands/routes_test.rb @@ -62,7 +62,6 @@ class Rails::Command::RoutesTest < ActiveSupport::TestCase assert_equal <<~MESSAGE, run_routes_command([ "-g", "POST" ]) Prefix Verb URI Pattern Controller#Action POST /cart(.:format) cart#create - rails_amazon_inbound_emails POST /rails/action_mailbox/amazon/inbound_emails(.:format) action_mailbox/ingresses/amazon/inbound_emails#create rails_mandrill_inbound_emails POST /rails/action_mailbox/mandrill/inbound_emails(.:format) action_mailbox/ingresses/mandrill/inbound_emails#create rails_postmark_inbound_emails POST /rails/action_mailbox/postmark/inbound_emails(.:format) action_mailbox/ingresses/postmark/inbound_emails#create rails_relay_inbound_emails POST /rails/action_mailbox/relay/inbound_emails(.:format) action_mailbox/ingresses/relay/inbound_emails#create @@ -166,7 +165,6 @@ class Rails::Command::RoutesTest < ActiveSupport::TestCase assert_equal <<~MESSAGE, run_routes_command Prefix Verb URI Pattern Controller#Action - rails_amazon_inbound_emails POST /rails/action_mailbox/amazon/inbound_emails(.:format) action_mailbox/ingresses/amazon/inbound_emails#create rails_mandrill_inbound_emails POST /rails/action_mailbox/mandrill/inbound_emails(.:format) action_mailbox/ingresses/mandrill/inbound_emails#create rails_postmark_inbound_emails POST /rails/action_mailbox/postmark/inbound_emails(.:format) action_mailbox/ingresses/postmark/inbound_emails#create rails_relay_inbound_emails POST /rails/action_mailbox/relay/inbound_emails(.:format) action_mailbox/ingresses/relay/inbound_emails#create @@ -207,101 +205,96 @@ class Rails::Command::RoutesTest < ActiveSupport::TestCase URI | /cart(.:format) Controller#Action | cart#show --[ Route 2 ]-------------- - Prefix | rails_amazon_inbound_emails - Verb | POST - URI | /rails/action_mailbox/amazon/inbound_emails(.:format) - Controller#Action | action_mailbox/ingresses/amazon/inbound_emails#create - --[ Route 3 ]-------------- Prefix | rails_mandrill_inbound_emails Verb | POST URI | /rails/action_mailbox/mandrill/inbound_emails(.:format) Controller#Action | action_mailbox/ingresses/mandrill/inbound_emails#create - --[ Route 4 ]-------------- + --[ Route 3 ]-------------- Prefix | rails_postmark_inbound_emails Verb | POST URI | /rails/action_mailbox/postmark/inbound_emails(.:format) Controller#Action | action_mailbox/ingresses/postmark/inbound_emails#create - --[ Route 5 ]-------------- + --[ Route 4 ]-------------- Prefix | rails_relay_inbound_emails Verb | POST URI | /rails/action_mailbox/relay/inbound_emails(.:format) Controller#Action | action_mailbox/ingresses/relay/inbound_emails#create - --[ Route 6 ]-------------- + --[ Route 5 ]-------------- Prefix | rails_sendgrid_inbound_emails Verb | POST URI | /rails/action_mailbox/sendgrid/inbound_emails(.:format) Controller#Action | action_mailbox/ingresses/sendgrid/inbound_emails#create - --[ Route 7 ]-------------- + --[ Route 6 ]-------------- Prefix | rails_mailgun_inbound_emails Verb | POST URI | /rails/action_mailbox/mailgun/inbound_emails/mime(.:format) Controller#Action | action_mailbox/ingresses/mailgun/inbound_emails#create - --[ Route 8 ]-------------- + --[ Route 7 ]-------------- Prefix | rails_conductor_inbound_emails Verb | GET URI | /rails/conductor/action_mailbox/inbound_emails(.:format) Controller#Action | rails/conductor/action_mailbox/inbound_emails#index - --[ Route 9 ]-------------- + --[ Route 8 ]-------------- Prefix | Verb | POST URI | /rails/conductor/action_mailbox/inbound_emails(.:format) Controller#Action | rails/conductor/action_mailbox/inbound_emails#create - --[ Route 10 ]------------- + --[ Route 9 ]-------------- Prefix | new_rails_conductor_inbound_email Verb | GET URI | /rails/conductor/action_mailbox/inbound_emails/new(.:format) Controller#Action | rails/conductor/action_mailbox/inbound_emails#new - --[ Route 11 ]------------- + --[ Route 10 ]------------- Prefix | edit_rails_conductor_inbound_email Verb | GET URI | /rails/conductor/action_mailbox/inbound_emails/:id/edit(.:format) Controller#Action | rails/conductor/action_mailbox/inbound_emails#edit - --[ Route 12 ]------------- + --[ Route 11 ]------------- Prefix | rails_conductor_inbound_email Verb | GET URI | /rails/conductor/action_mailbox/inbound_emails/:id(.:format) Controller#Action | rails/conductor/action_mailbox/inbound_emails#show - --[ Route 13 ]------------- + --[ Route 12 ]------------- Prefix | Verb | PATCH URI | /rails/conductor/action_mailbox/inbound_emails/:id(.:format) Controller#Action | rails/conductor/action_mailbox/inbound_emails#update - --[ Route 14 ]------------- + --[ Route 13 ]------------- Prefix | Verb | PUT URI | /rails/conductor/action_mailbox/inbound_emails/:id(.:format) Controller#Action | rails/conductor/action_mailbox/inbound_emails#update - --[ Route 15 ]------------- + --[ Route 14 ]------------- Prefix | Verb | DELETE URI | /rails/conductor/action_mailbox/inbound_emails/:id(.:format) Controller#Action | rails/conductor/action_mailbox/inbound_emails#destroy - --[ Route 16 ]------------- + --[ Route 15 ]------------- Prefix | rails_conductor_inbound_email_reroute Verb | POST URI | /rails/conductor/action_mailbox/:inbound_email_id/reroute(.:format) Controller#Action | rails/conductor/action_mailbox/reroutes#create - --[ Route 17 ]------------- + --[ Route 16 ]------------- Prefix | rails_service_blob Verb | GET URI | /rails/active_storage/blobs/:signed_id/*filename(.:format) Controller#Action | active_storage/blobs#show - --[ Route 18 ]------------- + --[ Route 17 ]------------- Prefix | rails_blob_representation Verb | GET URI | /rails/active_storage/representations/:signed_blob_id/:variation_key/*filename(.:format) Controller#Action | active_storage/representations#show - --[ Route 19 ]------------- + --[ Route 18 ]------------- Prefix | rails_disk_service Verb | GET URI | /rails/active_storage/disk/:encoded_key/*filename(.:format) Controller#Action | active_storage/disk#show - --[ Route 20 ]------------- + --[ Route 19 ]------------- Prefix | update_rails_disk_service Verb | PUT URI | /rails/active_storage/disk/:encoded_token(.:format) Controller#Action | active_storage/disk#update - --[ Route 21 ]------------- + --[ Route 20 ]------------- Prefix | rails_direct_uploads Verb | POST URI | /rails/active_storage/direct_uploads(.:format) diff --git a/railties/test/generators/actions_test.rb b/railties/test/generators/actions_test.rb index 44d4e92256..d913bb5438 100644 --- a/railties/test/generators/actions_test.rb +++ b/railties/test/generators/actions_test.rb @@ -200,7 +200,7 @@ class ActionsTest < Rails::Generators::TestCase run_generator action :environment do - _ = "# This wont be added"# assignment to silence parse-time warning "unused literal ignored" + _ = "# This wont be added" # assignment to silence parse-time warning "unused literal ignored" "# This will be added" end diff --git a/railties/test/generators/app_generator_test.rb b/railties/test/generators/app_generator_test.rb index 3bdbc70990..8771b53071 100644 --- a/railties/test/generators/app_generator_test.rb +++ b/railties/test/generators/app_generator_test.rb @@ -345,39 +345,44 @@ class AppGeneratorTest < Rails::Generators::TestCase app_root = File.join(destination_root, "myapp") run_generator [app_root, "--skip-spring"] - FileUtils.cd(app_root) do - quietly { system("bin/rails app:update") } - end + stub_rails_application(app_root) do + generator = Rails::Generators::AppGenerator.new ["rails"], { update: true, skip_spring: true }, { destination_root: app_root, shell: @shell } + generator.send(:app_const) + quietly { generator.send(:update_config_files) } - assert_no_file "#{app_root}/config/spring.rb" + assert_no_file "#{app_root}/config/spring.rb" + end end def test_app_update_does_not_generate_action_cable_contents_when_skip_action_cable_is_given app_root = File.join(destination_root, "myapp") run_generator [app_root, "--skip-action-cable"] - FileUtils.cd(app_root) do - quietly { system("bin/rails app:update") } - end + stub_rails_application(app_root) do + generator = Rails::Generators::AppGenerator.new ["rails"], { update: true, skip_action_cable: true }, { destination_root: app_root, shell: @shell } + generator.send(:app_const) + quietly { generator.send(:update_config_files) } - assert_no_file "#{app_root}/config/cable.yml" - assert_file "#{app_root}/config/environments/production.rb" do |content| - assert_no_match(/config\.action_cable/, content) + assert_no_file "#{app_root}/config/cable.yml" + assert_file "#{app_root}/config/environments/production.rb" do |content| + assert_no_match(/config\.action_cable/, content) + end + assert_no_file "#{app_root}/test/channels/application_cable/connection_test.rb" end - - assert_no_file "#{app_root}/test/channels/application_cable/connection_test.rb" end def test_app_update_does_not_generate_bootsnap_contents_when_skip_bootsnap_is_given app_root = File.join(destination_root, "myapp") run_generator [app_root, "--skip-bootsnap"] - FileUtils.cd(app_root) do - quietly { system("bin/rails app:update") } - end + stub_rails_application(app_root) do + generator = Rails::Generators::AppGenerator.new ["rails"], { update: true, skip_bootsnap: true }, { destination_root: app_root, shell: @shell } + generator.send(:app_const) + quietly { generator.send(:update_config_files) } - assert_file "#{app_root}/config/boot.rb" do |content| - assert_no_match(/require 'bootsnap\/setup'/, content) + assert_file "#{app_root}/config/boot.rb" do |content| + assert_no_match(/require 'bootsnap\/setup'/, content) + end end end @@ -396,46 +401,50 @@ class AppGeneratorTest < Rails::Generators::TestCase app_root = File.join(destination_root, "myapp") run_generator [app_root, "--skip-active-storage"] - FileUtils.cd(app_root) do - quietly { system("bin/rails app:update") } - end + stub_rails_application(app_root) do + generator = Rails::Generators::AppGenerator.new ["rails"], { update: true, skip_active_storage: true }, { destination_root: app_root, shell: @shell } + generator.send(:app_const) + quietly { generator.send(:update_config_files) } - assert_file "#{app_root}/config/environments/development.rb" do |content| - assert_no_match(/config\.active_storage/, content) - end + assert_file "#{app_root}/config/environments/development.rb" do |content| + assert_no_match(/config\.active_storage/, content) + end - assert_file "#{app_root}/config/environments/production.rb" do |content| - assert_no_match(/config\.active_storage/, content) - end + assert_file "#{app_root}/config/environments/production.rb" do |content| + assert_no_match(/config\.active_storage/, content) + end - assert_file "#{app_root}/config/environments/test.rb" do |content| - assert_no_match(/config\.active_storage/, content) - end + assert_file "#{app_root}/config/environments/test.rb" do |content| + assert_no_match(/config\.active_storage/, content) + end - assert_no_file "#{app_root}/config/storage.yml" + assert_no_file "#{app_root}/config/storage.yml" + end end def test_app_update_does_not_generate_active_storage_contents_when_skip_active_record_is_given app_root = File.join(destination_root, "myapp") run_generator [app_root, "--skip-active-record"] - FileUtils.cd(app_root) do - quietly { system("bin/rails app:update") } - end + stub_rails_application(app_root) do + generator = Rails::Generators::AppGenerator.new ["rails"], { update: true, skip_active_record: true }, { destination_root: app_root, shell: @shell } + generator.send(:app_const) + quietly { generator.send(:update_config_files) } - assert_file "#{app_root}/config/environments/development.rb" do |content| - assert_no_match(/config\.active_storage/, content) - end + assert_file "#{app_root}/config/environments/development.rb" do |content| + assert_no_match(/config\.active_storage/, content) + end - assert_file "#{app_root}/config/environments/production.rb" do |content| - assert_no_match(/config\.active_storage/, content) - end + assert_file "#{app_root}/config/environments/production.rb" do |content| + assert_no_match(/config\.active_storage/, content) + end - assert_file "#{app_root}/config/environments/test.rb" do |content| - assert_no_match(/config\.active_storage/, content) - end + assert_file "#{app_root}/config/environments/test.rb" do |content| + assert_no_match(/config\.active_storage/, content) + end - assert_no_file "#{app_root}/config/storage.yml" + assert_no_file "#{app_root}/config/storage.yml" + end end def test_generator_skips_action_mailbox_when_skip_action_mailbox_is_given @@ -469,16 +478,17 @@ class AppGeneratorTest < Rails::Generators::TestCase end def test_app_update_does_not_change_config_target_version - run_generator + app_root = File.join(destination_root, "myapp") + run_generator [app_root, "--skip-spring"] - FileUtils.cd(destination_root) do + FileUtils.cd(app_root) do config = "config/application.rb" content = File.read(config) File.write(config, content.gsub(/config\.load_defaults #{Rails::VERSION::STRING.to_f}/, "config.load_defaults 5.1")) quietly { system("bin/rails app:update") } end - assert_file "config/application.rb", /\s+config\.load_defaults 5\.1/ + assert_file "#{app_root}/config/application.rb", /\s+config\.load_defaults 5\.1/ end def test_app_update_does_not_change_app_name_when_app_name_is_hyphenated_name @@ -495,13 +505,15 @@ class AppGeneratorTest < Rails::Generators::TestCase assert_no_match(/hyphenated-app/, content) end - FileUtils.cd(app_root) do - quietly { system("bin/rails app:update") } - end + stub_rails_application(app_root) do + generator = Rails::Generators::AppGenerator.new ["rails"], { update: true }, { destination_root: app_root, shell: @shell } + generator.send(:app_const) + quietly { generator.send(:update_config_files) } - assert_file "#{app_root}/config/cable.yml" do |content| - assert_match(/hyphenated_app/, content) - assert_no_match(/hyphenated-app/, content) + assert_file "#{app_root}/config/cable.yml" do |content| + assert_match(/hyphenated_app/, content) + assert_no_match(/hyphenated-app/, content) + end end end @@ -827,6 +839,9 @@ class AppGeneratorTest < Rails::Generators::TestCase def test_spring run_generator assert_gem "spring" + assert_file("config/environments/test.rb") do |contents| + assert_match("config.cache_classes = false", contents) + end end def test_bundler_binstub @@ -845,7 +860,7 @@ class AppGeneratorTest < Rails::Generators::TestCase def test_spring_no_fork jruby_skip "spring doesn't run on JRuby" - assert_called_with(Process, :respond_to?, [[:fork], [:fork]], returns: false) do + assert_called_with(Process, :respond_to?, [[:fork], [:fork], [:fork]], returns: false) do run_generator assert_no_gem "spring" @@ -857,6 +872,9 @@ class AppGeneratorTest < Rails::Generators::TestCase assert_no_file "config/spring.rb" assert_no_gem "spring" + assert_file("config/environments/test.rb") do |contents| + assert_match("config.cache_classes = true", contents) + end end def test_spring_with_dev_option diff --git a/railties/test/generators/model_generator_test.rb b/railties/test/generators/model_generator_test.rb index c97cd17ec6..1b86e509ee 100644 --- a/railties/test/generators/model_generator_test.rb +++ b/railties/test/generators/model_generator_test.rb @@ -414,23 +414,12 @@ class ModelGeneratorTest < Rails::Generators::TestCase end end - def test_required_belongs_to_adds_required_association - run_generator ["account", "supplier:references{required}"] - - expected_file = <<~FILE - class Account < ApplicationRecord - belongs_to :supplier, required: true - end - FILE - assert_file "app/models/account.rb", expected_file - end - def test_required_polymorphic_belongs_to_generages_correct_model run_generator ["account", "supplier:references{required,polymorphic}"] expected_file = <<~FILE class Account < ApplicationRecord - belongs_to :supplier, polymorphic: true, required: true + belongs_to :supplier, polymorphic: true end FILE assert_file "app/models/account.rb", expected_file @@ -441,7 +430,7 @@ class ModelGeneratorTest < Rails::Generators::TestCase expected_file = <<~FILE class Account < ApplicationRecord - belongs_to :supplier, polymorphic: true, required: true + belongs_to :supplier, polymorphic: true end FILE assert_file "app/models/account.rb", expected_file |