ronantakizawa/codereview-bench huggingface dataset:
Repository: rails/rails
File: activestorage/app/models/active_storage/preview.rb (Ruby)

Code before:
  end

  # Returns the URL of the preview's variant on the service. Raises ActiveStorage::Preview::UnprocessedError if the
  # preview has not been processed yet.
  #
  # This method synchronously processes a variant of the preview image, so do not call it in views. Instead, generate
  # a stable URL that redirects to the URL returned by this method.
  def url(**options)
    if processed?
      variant.url(**options)
    else
      raise UnprocessedError
    end
  end

  # Returns a combination key of the blob and the variation that together identifies a specific variant.
  def key
    variant
    "variants/#{image.key}/#{Digest::SHA256.hexdigest(variation.key)}"
  end

  alias_method :service_url, :url
  deprecate service_url: :url

  private
    def processed?
      image.attached?
    end

    def process
      previewer.preview(service_name: blob.service_name) do |attachable|
        ActiveRecord::Base.connected_to(role: ActiveRecord::Base.writing_role) do
          image.attach(attachable)
        end
      end
    end

    def variant
      image.variant(variation).processed
    end


    def previewer
      previewer_class.new(blob)
    end

    def previewer_class
      ActiveStorage.previewers.detect { |klass| klass.accept?(blob) }
    end
end


Review comment (suggestion):
This is incompatible with database-tracked variants. We can implement `ActiveStorage::VariantWithRecord#key` and ask the variant for its key rather than reconstructing it here.

```ruby
# Here:
def key
  if processed?
    variant.key
  else
    raise UnprocessedError
  end
end
```

```ruby
# lib/active_storage/variant_with_record.rb
def key
  image&.key
end
```

Code after:
  end

  # Returns the URL of the preview's variant on the service. Raises ActiveStorage::Preview::UnprocessedError if the
  # preview has not been processed yet.
  #
  # This method synchronously processes a variant of the preview image, so do not call it in views. Instead, generate
  # a stable URL that redirects to the URL returned by this method.
  def url(**options)
    if processed?
      variant.url(**options)
    else
      raise UnprocessedError
    end
  end

  alias_method :service_url, :url
  deprecate service_url: :url

  # Returns a combination key of the blob and the variation that together identifies a specific variant.
  def key
    if processed?
      variant.key
    else
      raise UnprocessedError
    end
  end

  def download(&block)
    if processed?
      variant.download(&block)
    else
      raise UnprocessedError
    end
  end

  private
    def processed?
      image.attached?
    end

    def process
      previewer.preview(service_name: blob.service_name) do |attachable|
        ActiveRecord::Base.connected_to(role: ActiveRecord::Base.writing_role) do
          image.attach(attachable)
        end
      end
    end

    def variant
      image.variant(variation).processed
    end
Repository: rails/rails
File: actionpack/lib/action_dispatch/system_testing/driver.rb (Ruby)

Code before:
# frozen_string_literal: true

module ActionDispatch
  module SystemTesting
    class Driver # :nodoc:
      def initialize(name, **options, &capabilities)
        @name = name
        @screen_size = options[:screen_size]
        @options = options[:options] || {}
        @capabilities = capabilities

        if [:poltergeist, :webkit].include?(name)
          ActiveSupport::Deprecation.warn <<~MSG.squish
            Poltergeist and capybara-webkit are not maintained already.
            Driver registration of :poltergeist or :webkit is deprecated and will be removed in Rails 7.1.
            You can still use :selenium, and also :cuprite is available for alternative to Poltergeist.
          MSG
        end

        if name == :selenium
          require "selenium/webdriver"
          @browser = Browser.new(options[:using])
          @browser.preload
        else
          @browser = nil
        end
      end

      def use
        register if registerable?

        setup
      end

      private
        def registerable?
          [:selenium, :poltergeist, :webkit, :cuprite, :rack_test].include?(@name)
        end

        def register
          @browser&.configure(&@capabilities)

          Capybara.register_driver @name do |app|
            case @name
            when :selenium then register_selenium(app)

Review comment (suggestion):
`[driver_type, options[:using], object_id].compact.map(&:to_s).join("_").to_sym` feels needlessly verbose. I don't understand why we wouldn't use only `driver_type` if name is not supplied.

Code after:
# frozen_string_literal: true

module ActionDispatch
  module SystemTesting
    class Driver # :nodoc:
      attr_reader :name

      def initialize(driver_type, **options, &capabilities)
        @driver_type = driver_type
        @screen_size = options[:screen_size]
        @options = options[:options] || {}
        @name = @options.delete(:name) || driver_type
        @capabilities = capabilities

        if [:poltergeist, :webkit].include?(driver_type)
          ActiveSupport::Deprecation.warn <<~MSG.squish
            Poltergeist and capybara-webkit are not maintained already.
            Driver registration of :poltergeist or :webkit is deprecated and will be removed in Rails 7.1.
            You can still use :selenium, and also :cuprite is available for alternative to Poltergeist.
          MSG
        end

        if driver_type == :selenium
          require "selenium/webdriver"
          @browser = Browser.new(options[:using])
          @browser.preload
        else
          @browser = nil
        end
      end

      def use
        register if registerable?

        setup
      end

      private
        def registerable?
          [:selenium, :poltergeist, :webkit, :cuprite, :rack_test].include?(@driver_type)
        end

        def register
          @browser&.configure(&@capabilities)
Repository: rails/rails
File: actionpack/test/dispatch/system_testing/system_test_case_test.rb (Ruby)

Code before:
# frozen_string_literal: true

require "abstract_unit"
require "selenium/webdriver"

class SetDriverToRackTestTest < DrivenByRackTest
  test "uses rack_test" do
    assert_equal :rack_test, Capybara.current_driver
  end
end

class OverrideSeleniumSubclassToRackTestTest < DrivenBySeleniumWithChrome
  driven_by :rack_test

  test "uses rack_test" do
    assert_equal :rack_test, Capybara.current_driver
  end
end

class SetDriverToSeleniumTest < DrivenBySeleniumWithChrome
  test "uses selenium" do
    assert_equal :selenium, Capybara.current_driver
  end
end

class SetDriverToSeleniumHeadlessChromeTest < DrivenBySeleniumWithHeadlessChrome
  test "uses selenium headless chrome" do
    assert_equal :selenium, Capybara.current_driver
  end
end

class SetDriverToSeleniumHeadlessFirefoxTest < DrivenBySeleniumWithHeadlessFirefox
  test "uses selenium headless firefox" do

Review comment (bug):
These test changes on `current_driver` feel incorrect to me. `Capbyara.current_driver` isn't an API we control - it should still return the same symbol it was returning before. If we also want to test that `driver_type` returns `:rack_test` then that should be an additional assertion rather than replacing this assertion.

Code after:
# frozen_string_literal: true

require "abstract_unit"
require "selenium/webdriver"

class SetDriverToRackTestTest < DrivenByRackTest
  test "uses rack_test" do
    assert_equal :rack_test, Capybara.current_driver
  end
end

class OverrideSeleniumSubclassToRackTestTest < DrivenBySeleniumWithChrome
  driven_by :rack_test

  test "uses rack_test" do
    assert_equal :rack_test, Capybara.current_driver
  end
end

class OverrideDriverWithExplicitName < DrivenBySeleniumWithChrome
  driven_by :selenium, options: { name: :best_driver }

  test "uses specified driver name" do
    assert_equal :best_driver, Capybara.current_driver
  end
end

class SetDriverToSeleniumTest < DrivenBySeleniumWithChrome
  test "uses selenium" do
    assert_equal :selenium, Capybara.current_driver
  end
end
Repository: rails/rails
File: actionpack/lib/action_dispatch/system_testing/driver.rb (Ruby)

Code before:
# frozen_string_literal: true

module ActionDispatch
  module SystemTesting
    class Driver # :nodoc:
      def initialize(name, **options, &capabilities)
        @name = name
        @screen_size = options[:screen_size]
        @options = options[:options] || {}
        @capabilities = capabilities

        if [:poltergeist, :webkit].include?(name)
          ActiveSupport::Deprecation.warn <<~MSG.squish
            Poltergeist and capybara-webkit are not maintained already.
            Driver registration of :poltergeist or :webkit is deprecated and will be removed in Rails 7.1.
            You can still use :selenium, and also :cuprite is available for alternative to Poltergeist.
          MSG
        end

        if name == :selenium
          require "selenium/webdriver"
          @browser = Browser.new(options[:using])
          @browser.preload
        else
          @browser = nil
        end
      end

      def use
        register if registerable?

        setup
      end

      private
        def registerable?
          [:selenium, :poltergeist, :webkit, :cuprite, :rack_test].include?(@name)

Review comment (suggestion):
Should this have been `options.delete(:name)`? Based on of this comment: https://github.com/rails/rails/issues/32468#issuecomment-946056251

Code after:
# frozen_string_literal: true

module ActionDispatch
  module SystemTesting
    class Driver # :nodoc:
      attr_reader :name

      def initialize(driver_type, **options, &capabilities)
        @driver_type = driver_type
        @screen_size = options[:screen_size]
        @options = options[:options] || {}
        @name = @options.delete(:name) || driver_type
        @capabilities = capabilities

        if [:poltergeist, :webkit].include?(driver_type)
          ActiveSupport::Deprecation.warn <<~MSG.squish
            Poltergeist and capybara-webkit are not maintained already.
            Driver registration of :poltergeist or :webkit is deprecated and will be removed in Rails 7.1.
            You can still use :selenium, and also :cuprite is available for alternative to Poltergeist.
          MSG
        end

        if driver_type == :selenium
          require "selenium/webdriver"
          @browser = Browser.new(options[:using])
          @browser.preload
        else
          @browser = nil
        end
      end

      def use
        register if registerable?

        setup
      end
Repository: rails/rails
File: activestorage/lib/active_storage.rb (Ruby)

Code before:
require "active_support"
require "active_support/rails"
require "active_support/core_ext/numeric/time"

require "active_storage/version"
require "active_storage/deprecator"
require "active_storage/errors"

require "marcel"

# :markup: markdown
# :include: ../README.md
module ActiveStorage
  extend ActiveSupport::Autoload

  autoload :Attached
  autoload :FixtureSet
  autoload :Service
  autoload :Previewer
  autoload :Analyzer

  mattr_accessor :logger
  mattr_accessor :verifier
  mattr_accessor :variant_processor, default: :mini_magick

  mattr_accessor :queues, default: {}

  mattr_accessor :previewers, default: []
  mattr_accessor :analyzers,  default: []

  mattr_accessor :paths, default: {}

  mattr_accessor :variable_content_types,           default: []
  mattr_accessor :web_image_content_types,          default: []
  mattr_accessor :binary_content_type,              default: "application/octet-stream"
  mattr_accessor :content_types_to_serve_as_binary, default: []
  mattr_accessor :content_types_allowed_inline,     default: []

  mattr_accessor :supported_image_processing_methods, default: [
    "adaptive_blur",
    "adaptive_resize",
    "adaptive_sharpen",
    "adjoin",
    "affine",
    "alpha",
    "annotate",
    "antialias",
    "append",
    "apply",
    "attenuate",
    "authenticate",

Review comment (suggestion):
Note to self: we should probably convert all these `mattr_accessor` to `attr_accessor`.

Code after:
require "active_support"
require "active_support/rails"
require "active_support/core_ext/numeric/time"

require "active_storage/version"
require "active_storage/deprecator"
require "active_storage/errors"

require "marcel"

# :markup: markdown
# :include: ../README.md
module ActiveStorage
  extend ActiveSupport::Autoload

  autoload :Attached
  autoload :FixtureSet
  autoload :Service
  autoload :Previewer
  autoload :Analyzer

  mattr_accessor :logger
  mattr_accessor :verifier
  mattr_accessor :variant_processor, default: :mini_magick

  mattr_accessor :variant_transformer

  mattr_accessor :queues, default: {}

  mattr_accessor :previewers, default: []
  mattr_accessor :analyzers,  default: []

  mattr_accessor :paths, default: {}

  mattr_accessor :variable_content_types,           default: []
  mattr_accessor :web_image_content_types,          default: []
  mattr_accessor :binary_content_type,              default: "application/octet-stream"
  mattr_accessor :content_types_to_serve_as_binary, default: []
  mattr_accessor :content_types_allowed_inline,     default: []

  mattr_accessor :supported_image_processing_methods, default: [
    "adaptive_blur",
    "adaptive_resize",
    "adaptive_sharpen",
    "adjoin",
    "affine",
    "alpha",
    "annotate",
    "antialias",
    "append",
    "apply",
Repository: rails/rails
File: activestorage/lib/active_storage/analyzer/image_analyzer/image_magick.rb (Ruby)

Code before:
# frozen_string_literal: true

module ActiveStorage
  # This analyzer relies on the third-party {MiniMagick}[https://github.com/minimagick/minimagick] gem. MiniMagick requires
  # the {ImageMagick}[http://www.imagemagick.org] system library.
  class Analyzer::ImageAnalyzer::ImageMagick < Analyzer::ImageAnalyzer
    def self.accept?(blob)
      super && ActiveStorage.variant_processor == :mini_magick
    end

    private
      def read_image
        begin
          require "mini_magick"
        rescue LoadError
          logger.info "Skipping image analysis because the mini_magick gem isn't installed"
          return {}
        end

        download_blob_to_tempfile do |file|
          image = instrument("mini_magick") do
            MiniMagick::Image.new(file.path)
          end

          if image.valid?
            yield image
          else
            logger.info "Skipping image analysis because ImageMagick doesn't support the file"
            {}
          end
        rescue MiniMagick::Error => error

Review comment (suggestion):
Copy-paste error? I think this will cause it to not show the deprecation warning.
```suggestion
  raise error unless error.message.include?("mini_magick")
```

Code after:
# frozen_string_literal: true

begin
  gem "mini_magick"
rescue LoadError => error
  raise error unless error.message.include?("ruby-vips")
end

module ActiveStorage
  # This analyzer relies on the third-party {MiniMagick}[https://github.com/minimagick/minimagick] gem. MiniMagick requires
  # the {ImageMagick}[http://www.imagemagick.org] system library.
  class Analyzer::ImageAnalyzer::ImageMagick < Analyzer::ImageAnalyzer
    private
      def read_image
        download_blob_to_tempfile do |file|
          image = instrument("mini_magick") do
            MiniMagick::Image.new(file.path)
          end

          if image.valid?
            yield image
          else
            logger.info "Skipping image analysis because ImageMagick doesn't support the file"
            {}
          end
        rescue MiniMagick::Error => error
          logger.error "Skipping image analysis due to an ImageMagick error: #{error.message}"
          {}
        end
      end
Repository: rails/rails
File: activestorage/lib/active_storage/analyzer/image_analyzer/image_magick.rb (Ruby)

Code before:
# frozen_string_literal: true

module ActiveStorage
  # This analyzer relies on the third-party {MiniMagick}[https://github.com/minimagick/minimagick] gem. MiniMagick requires
  # the {ImageMagick}[http://www.imagemagick.org] system library.
  class Analyzer::ImageAnalyzer::ImageMagick < Analyzer::ImageAnalyzer
    def self.accept?(blob)
      super && ActiveStorage.variant_processor == :mini_magick
    end

    private
      def read_image
        begin
          require "mini_magick"
        rescue LoadError
          logger.info "Skipping image analysis because the mini_magick gem isn't installed"
          return {}
        end

        download_blob_to_tempfile do |file|
          image = instrument("mini_magick") do
            MiniMagick::Image.new(file.path)
          end

          if image.valid?
            yield image
          else
            logger.info "Skipping image analysis because ImageMagick doesn't support the file"
            {}

Review comment (suggestion):
@skipkayhil how come you don't `require` the gem here? You do in the vips anlayzer, and I think that's causing issues, but I can't work if that's an intentional oversight.

```ruby
Tanda @ Rails 8.1.0.alpha (test) :008 > gem "mini_magick"
true
Tanda @ Rails 8.1.0.alpha (test) :009 > MiniMagick
(web):9:in `<main>': uninitialized constant MiniMagick (NameError)

MiniMagick
^^^^^^^^^^
Did you mean?  MiniMime
Tanda @ Rails 8.1.0.alpha (test) :010 > ::MiniMagick
(web):10:in `<main>': uninitialized constant MiniMagick (NameError)

::MiniMagick
^^^^^^^^^^^^
Did you mean?  MiniMime
Tanda @ Rails 8.1.0.alpha (test) :011 > require "mini_magick"
true
Tanda @ Rails 8.1.0.alpha (test) :012 > MiniMagick
MiniMagick
```

Code after:
# frozen_string_literal: true

begin
  gem "mini_magick"
rescue LoadError => error
  raise error unless error.message.include?("ruby-vips")
end

module ActiveStorage
  # This analyzer relies on the third-party {MiniMagick}[https://github.com/minimagick/minimagick] gem. MiniMagick requires
  # the {ImageMagick}[http://www.imagemagick.org] system library.
  class Analyzer::ImageAnalyzer::ImageMagick < Analyzer::ImageAnalyzer
    private
      def read_image
        download_blob_to_tempfile do |file|
          image = instrument("mini_magick") do
            MiniMagick::Image.new(file.path)
          end

          if image.valid?
            yield image
          else
            logger.info "Skipping image analysis because ImageMagick doesn't support the file"
            {}
          end
        rescue MiniMagick::Error => error
          logger.error "Skipping image analysis due to an ImageMagick error: #{error.message}"
          {}
        end
Repository: rails/rails
File: activerecord/lib/arel/nodes/homogeneous_in.rb (Ruby)

Code before:

      def invert
        Arel::Nodes::HomogeneousIn.new(values, attribute, type == :in ? :notin : :in)
      end

      def left
        attribute
      end

      def right
        attribute.quoted_array(values)
      end

      def casted_values
        type = attribute.type_caster

        casted_values = values.map do |raw_value|
          type.serialize(raw_value) if type.serializable?(raw_value)
        end

        casted_values.compact!
        casted_values
      end

      def proc_for_binds
        -> value { ActiveModel::Attribute.from_database(attribute.name, value, ActiveModel::Type.default_value) }
      end

      def fetch_attribute(&block)
        if attribute
          yield attribute
        else
          expr.fetch_attribute(&block)
        end
      end

      protected
        def ivars
          [@attribute, @values, @type]
        end
    end
  end
end


Review comment (suggestion):
The fix here is actually due to changing `attribute.type_caster` to `ActiveModel::Type.default_value` because that changes the subsequent `serialize` call into a no-op:

https://github.com/rails/rails/blob/a409c63541e4686202a04b772fe2993ab254ee57/activemodel/lib/active_model/type/value.rb#L65-L67

As such, changing `with_cast_value` to `from_database` has no effect because the methods that differ between them (see [`WithCastValue`](https://github.com/rails/rails/blob/a409c63541e4686202a04b772fe2993ab254ee57/activemodel/lib/active_model/attribute.rb#L212-L220) and [`FromDatabase`](https://github.com/rails/rails/blob/a409c63541e4686202a04b772fe2993ab254ee57/activemodel/lib/active_model/attribute.rb#L173-L195)) aren't being called.


Code after:

      def invert
        Arel::Nodes::HomogeneousIn.new(values, attribute, type == :in ? :notin : :in)
      end

      def left
        attribute
      end

      def right
        attribute.quoted_array(values)
      end

      def casted_values
        type = attribute.type_caster

        casted_values = values.map do |raw_value|
          type.serialize(raw_value) if type.serializable?(raw_value)
        end

        casted_values.compact!
        casted_values
      end

      def proc_for_binds
        -> value { ActiveModel::Attribute.with_cast_value(attribute.name, value, ActiveModel::Type.default_value) }
      end

      def fetch_attribute(&block)
        if attribute
          yield attribute
        else
          expr.fetch_attribute(&block)
        end
      end

      protected
        def ivars
          [@attribute, @values, @type]
        end
    end
  end
end
Repository: rails/rails
File: activerecord/test/cases/log_subscriber_test.rb (Ruby)

Code before:
    wait
    assert_equal 0, @logger.logged(:debug).size
  end

  def test_cached_queries_doesnt_log_when_level_is_not_debug
    @logger.level = INFO
    ActiveRecord::Base.cache do
      Developer.all.load
      Developer.all.load
    end
    wait
    assert_equal 0, @logger.logged(:debug).size
  end

  if ActiveRecord::Base.connection.prepared_statements
    def test_where_in_binds_logging_include_attribute_names
      Developer.where(id: [1, 2, 3, 4, 5]).load
      wait
      assert_match(%{["id", 1], ["id", 2], ["id", 3], ["id", 4], ["id", 5]}, @logger.logged(:debug).last)
    end

    def test_where_in_binds_with_json_attribute
      Admin::User.where(json_options: [{ a: 1 }, { b: 2 }]).load
      wait
      assert_match(%{[["json_options", "{\\"a\\":1}"], ["json_options", "{\\"b\\":2}"]]}, @logger.logged(:debug).last)
    end

    def test_binary_data_is_not_logged
      Binary.create(data: "some binary data")
      wait
      assert_match(/<16 bytes of binary data>/, @logger.logged(:debug).join)
    end

    def test_binary_data_hash
      Binary.create(data: { a: 1 })
      wait
      assert_match(/<7 bytes of binary data>/, @logger.logged(:debug).join)
    end
  end
end


Review comment (suggestion):
This tests the logging output, but what we're really trying to fix is the query, so I think we should test the query results instead.


Code after:
  end

  def test_cached_queries_doesnt_log_when_level_is_not_debug
    @logger.level = INFO
    ActiveRecord::Base.cache do
      Developer.all.load
      Developer.all.load
    end
    wait
    assert_equal 0, @logger.logged(:debug).size
  end

  if ActiveRecord::Base.connection.prepared_statements
    def test_where_in_binds_logging_include_attribute_names
      Developer.where(id: [1, 2, 3, 4, 5]).load
      wait
      assert_match(%{["id", 1], ["id", 2], ["id", 3], ["id", 4], ["id", 5]}, @logger.logged(:debug).last)
    end

    def test_binary_data_is_not_logged
      Binary.create(data: "some binary data")
      wait
      assert_match(/<16 bytes of binary data>/, @logger.logged(:debug).join)
    end

    def test_binary_data_hash
      Binary.create(data: { a: 1 })
      wait
      assert_match(/<7 bytes of binary data>/, @logger.logged(:debug).join)
    end
  end
end