ronantakizawa/codereview-bench huggingface dataset:
Repository: rails/rails
File: activesupport/lib/active_support/benchmark.rb (Ruby)

Code before:
# frozen_string_literal: true

module ActiveSupport
  # = \Benchmark
  module Benchmark
    # Benchmark realtime in the specified time unit. By default,
    # the returned unit is in seconds.
    #
    #   ActiveSupport::Benchmark.realtime { sleep 0.1 }
    #   # => 0.10007
    #
    #   ActiveSupport::Benchmark.realtime(:float_millisecond) { sleep 0.1 }
    #   # => 100.07
    #
    # `unit` can be any of the values accepted by Ruby's `Process.clock_gettime`.
    def self.realtime(unit = :float_second, &block)
      time_start = Process.clock_gettime(Process::CLOCK_MONOTONIC, unit)
      yield
      Process.clock_gettime(Process::CLOCK_MONOTONIC, unit) - time_start
    end
  end
end


Review comment (suggestion):
This should be `# :nodoc:` I don't want people using it.

Code after:
# frozen_string_literal: true

module ActiveSupport
  module Benchmark # :nodoc:
    # Benchmark realtime in the specified time unit. By default,
    # the returned unit is in seconds.
    #
    #   ActiveSupport::Benchmark.realtime { sleep 0.1 }
    #   # => 0.10007
    #
    #   ActiveSupport::Benchmark.realtime(:float_millisecond) { sleep 0.1 }
    #   # => 100.07
    #
    # `unit` can be any of the values accepted by Ruby's `Process.clock_gettime`.
    def self.realtime(unit = :float_second, &block)
      time_start = Process.clock_gettime(Process::CLOCK_MONOTONIC, unit)
      yield
      Process.clock_gettime(Process::CLOCK_MONOTONIC, unit) - time_start
    end
  end
end
Repository: rails/rails
File: actionpack/lib/action_dispatch/http/response.rb (Ruby)

Code before:
# frozen_string_literal: true

# :markup: markdown

require "active_support/core_ext/module/attribute_accessors"
require "action_controller/metal/rate_limiting"
require "action_dispatch/http/filter_redirect"
require "action_dispatch/http/cache"
require "monitor"

module ActionDispatch # :nodoc:
  # # Action Dispatch Response
  #
  # Represents an HTTP response generated by a controller action. Use it to
  # retrieve the current state of the response, or customize the response. It can
  # either represent a real HTTP response (i.e. one that is meant to be sent back
  # to the web browser) or a TestResponse (i.e. one that is generated from
  # integration tests).
  #
  # The Response object for the current request is exposed on controllers as
  # ActionController::Metal#response. ActionController::Metal also provides a few
  # additional methods that delegate to attributes of the Response such as
  # ActionController::Metal#headers.
  #
  # Integration tests will likely also want to inspect responses in more detail.
  # Methods such as Integration::RequestHelpers#get and
  # Integration::RequestHelpers#post return instances of TestResponse (which
  # inherits from Response) for this purpose.
  #
  # For example, the following demo integration test prints the body of the
  # controller response to the console:

Review comment (suggestion):
Action dispatch should not depend on action controller. Just define the method here. No need to put in `action_controller/metal/rate_limiting`

Code after:
# frozen_string_literal: true

# :markup: markdown

require "active_support/core_ext/module/attribute_accessors"
require "action_dispatch/http/filter_redirect"
require "action_dispatch/http/cache"
require "monitor"

module ActionDispatch # :nodoc:
  # # Action Dispatch Response
  #
  # Represents an HTTP response generated by a controller action. Use it to
  # retrieve the current state of the response, or customize the response. It can
  # either represent a real HTTP response (i.e. one that is meant to be sent back
  # to the web browser) or a TestResponse (i.e. one that is generated from
  # integration tests).
  #
  # The Response object for the current request is exposed on controllers as
  # ActionController::Metal#response. ActionController::Metal also provides a few
  # additional methods that delegate to attributes of the Response such as
  # ActionController::Metal#headers.
  #
  # Integration tests will likely also want to inspect responses in more detail.
  # Methods such as Integration::RequestHelpers#get and
  # Integration::RequestHelpers#post return instances of TestResponse (which
  # inherits from Response) for this purpose.
  #
  # For example, the following demo integration test prints the body of the
  # controller response to the console:
  #
Repository: rails/rails
File: actionpack/test/controller/rate_limiting_test.rb (Ruby)

Code before:
# frozen_string_literal: true

require "abstract_unit"

class RateLimitedController < ActionController::Base
  self.cache_store = ActiveSupport::Cache::MemoryStore.new
  rate_limit to: 2, within: 2.seconds, only: :limited
  rate_limit to: 5, within: 1.minute, name: "long-term", only: :limited

  def limited
    head :ok
  end

  rate_limit to: 2, within: 2.seconds, by: -> { params[:rate_limit_key] }, with: -> { head :forbidden }, only: :limited_with
  def limited_with
    head :ok
  end
end

class RateLimitedSharedController < ActionController::Base
  self.cache_store = ActiveSupport::Cache::MemoryStore.new
  rate_limit to: 2, within: 2.seconds, context: "shared"
end

class RateLimitedSharedOneController < RateLimitedSharedController
  def limited_shared_one
    head :ok
  end
end

class RateLimitedSharedTwoController < RateLimitedSharedController
  def limited_shared_two
    head :ok
  end
end

class RateLimitingTest < ActionController::TestCase
  tests RateLimitedController

  setup do
    RateLimitedController.cache_store.clear
  end

  test "exceeding basic limit" do
    get :limited
    get :limited
    assert_response :ok

Review comment (suggestion):
Shouldn't it be `scope:` instead of `context:`? However, I have no idea how the test can be successful.

Code after:
# frozen_string_literal: true

require "abstract_unit"

class RateLimitedController < ActionController::Base
  self.cache_store = ActiveSupport::Cache::MemoryStore.new
  rate_limit to: 2, within: 2.seconds, only: :limited
  rate_limit to: 5, within: 1.minute, name: "long-term", only: :limited

  def limited
    head :ok
  end

  rate_limit to: 2, within: 2.seconds, by: -> { params[:rate_limit_key] }, with: -> { head :forbidden }, only: :limited_with
  def limited_with
    head :ok
  end
end

class RateLimitedBaseController < ActionController::Base
  self.cache_store = ActiveSupport::Cache::MemoryStore.new
end

class RateLimitedSharedOneController < RateLimitedBaseController
  rate_limit to: 2, within: 2.seconds, scope: "shared"

  def limited_shared_one
    head :ok
  end
end

class RateLimitedSharedTwoController < RateLimitedBaseController
  rate_limit to: 2, within: 2.seconds, scope: "shared"

  def limited_shared_two
    head :ok
  end
end

class RateLimitedSharedController < ActionController::Base
  self.cache_store = ActiveSupport::Cache::MemoryStore.new
  rate_limit to: 2, within: 2.seconds
end

class RateLimitedSharedThreeController < RateLimitedSharedController
  def limited_shared_three
    head :ok
Repository: rails/rails
File: railties/lib/rails/generators/rails/authentication/authentication_generator.rb (Ruby)

Code before:
      end

      def enable_bcrypt
        if File.read(File.expand_path("Gemfile", destination_root)).include?('gem "bcrypt"')
          uncomment_lines "Gemfile", /gem "bcrypt"/
          Bundler.with_original_env { execute_command :bundle, "install --quiet" }
        else
          Bundler.with_original_env { execute_command :bundle, "add bcrypt", capture: true }
        end
      end

      def add_migrations
        generate "migration", "CreateUsers", "email_address:string!:uniq password_digest:string!", "--force"
        generate "migration", "CreateSessions", "user:references ip_address:string user_agent:string", "--force"
      end

      def configure_test_helper
        inject_into_file "test/test_helper.rb", after: "# Add more helper methods to be used by all tests here...\n" do
          <<-RUBY
    def login_as(user)
      session = user.sessions.create!
      Current.session = session
      request = ActionDispatch::Request.new(Rails.application.env_config)
      cookies = request.cookie_jar
      cookies.signed[:session_id] = { value: session.id, httponly: true, same_site: :lax }
    end
          RUBY
        end
      end

      hook_for :test_framework
    end
  end
end


Review comment (suggestion):
I had success with `logout` locally.

```suggestion
    def login_as(user)
      session = user.sessions.create!
      Current.session = session
      request = ActionDispatch::Request.new(Rails.application.env_config)
      cookies = request.cookie_jar
      cookies.signed[:session_id] = { value: session.id, httponly: true, same_site: :lax }
    end
    
    def logout
      Current.session.destroy
      request = ActionDispatch::Request.new(Rails.application.env_config)
      cookies = request.cookie_jar
      cookies.delete(:session_id)
    end
```

Code after:
        route "resource :session"
      end

      def enable_bcrypt
        if File.read(File.expand_path("Gemfile", destination_root)).include?('gem "bcrypt"')
          uncomment_lines "Gemfile", /gem "bcrypt"/
          Bundler.with_original_env { execute_command :bundle, "install --quiet" }
        else
          Bundler.with_original_env { execute_command :bundle, "add bcrypt", capture: true }
        end
      end

      def add_migrations
        generate "migration", "CreateUsers", "email_address:string!:uniq password_digest:string!", "--force"
        generate "migration", "CreateSessions", "user:references ip_address:string user_agent:string", "--force"
      end

      def configure_test_helper
        inject_into_file "test/test_helper.rb", "require_relative \"helpers/session_test_helper\"\n", after: "require \"rails/test_help\"\n"
        inject_into_class "test/test_helper.rb", "TestCase", "    include SessionTestHelper\n"
      end

      hook_for :test_framework
    end
  end
end
Repository: rails/rails
File: railties/lib/rails/generators/test_unit/authentication/templates/test/controllers/sessions_controller_test.rb (Ruby)

Code before:
require "test_helper"

class SessionsControllerTest < ActionDispatch::IntegrationTest
  test "new" do
    get new_session_url
    assert_response :success
  end

  test "create with valid credentials" do
    post session_url, params: { email_address: "one@example.com", password: "password" }

    assert_redirected_to root_url
    assert parsed_cookies.signed[:session_id]
  end

  test "create with invalid credentials" do
    post session_url, params: { email_address: "one@example.com", password: "wrong" }

    assert_redirected_to new_session_url
    assert_nil parsed_cookies.signed[:session_id]
  end

  test "destroy" do
    sign_in_as(User.take)

    delete session_url

    assert_redirected_to new_session_url
    assert_empty cookies[:session_id]
  end
end


Review comment (suggestion):
I left `"one@example.com"` hardcoded since `"password"` needs to be hardcoded anyways, but if needed I am happy to do something like `setup { @user = User.take }` with the following:

```ruby
    post session_url, params: { email_address: @user.email_address, password: "password" }
```

Code after:
require "test_helper"

class SessionsControllerTest < ActionDispatch::IntegrationTest
  setup { @user = User.take }

  test "new" do
    get new_session_path
    assert_response :success
  end

  test "create with valid credentials" do
    post session_path, params: { email_address: @user.email_address, password: "password" }

    assert_redirected_to root_path
    assert cookies[:session_id]
  end

  test "create with invalid credentials" do
    post session_path, params: { email_address: @user.email_address, password: "wrong" }

    assert_redirected_to new_session_path
    assert_nil cookies[:session_id]
  end

  test "destroy" do
    sign_in_as(User.take)

    delete session_path

    assert_redirected_to new_session_path
    assert_empty cookies[:session_id]
  end
end
Repository: rails/rails
File: activemodel/lib/active_model/attribute.rb (Ruby)

Code before:
    def forgetting_assignment
      with_value_from_database(value_for_database)
    end

    def with_value_from_user(value)
      type.assert_valid_value(value)
      self.class.from_user(name, value, type, original_attribute || self)
    end

    def with_value_from_database(value)
      self.class.from_database(name, value, type)
    end

    def with_cast_value(value)
      self.class.with_cast_value(name, value, type)
    end

    def with_type(type)
      if changed_in_place?
        with_value_from_user(value).with_type(type)
      else
        self.class.new(name, value_before_type_cast, type, original_attribute)
      end
    end

    def deep_dup # :nodoc:
      if @type.mutable?
        dup
      else
        self # If the underlying type is immutable we can get away with not duping
      end
    end

    def type_cast(*)
      raise NotImplementedError
    end

    def initialized?
      true
    end

    def came_from_user?
      false
    end

    def has_been_read?
      defined?(@value)
    end

    def ==(other)
      self.class == other.class &&

Review comment (suggestion):
Could we apply this optimization on `dup` in general? The caller is nearby, so it's hardly a layering violation, but it still feels just a little more implementation-aware than we could be, maybe?

(Incidentally notable: our actual `initialize_dup`'s value-dupping seems surprisingly casual -- likely doing so when unnecessary, possibly missing necessary depth in other cases?)

Code after:
    def forgetting_assignment
      with_value_from_database(value_for_database)
    end

    def with_value_from_user(value)
      type.assert_valid_value(value)
      self.class.from_user(name, value, type, original_attribute || self)
    end

    def with_value_from_database(value)
      self.class.from_database(name, value, type)
    end

    def with_cast_value(value)
      self.class.with_cast_value(name, value, type)
    end

    def with_type(type)
      if changed_in_place?
        with_value_from_user(value).with_type(type)
      else
        self.class.new(name, value_before_type_cast, type, original_attribute)
      end
    end

    def dup_or_share # :nodoc:
      if @type.mutable?
        dup
      else
        self # If the underlying type is immutable we can get away with not duping
      end
    end

    def type_cast(*)
      raise NotImplementedError
    end

    def initialized?
      true
    end

    def came_from_user?
      false
    end

    def has_been_read?
      defined?(@value)
    end

    def ==(other)
      self.class == other.class &&
Repository: rails/rails
File: activesupport/lib/active_support/core_ext/object/to_query.rb (Ruby)

Code before:
# frozen_string_literal: true

require "cgi"

class Object
  # Alias of <tt>to_s</tt>.
  def to_param
    to_s
  end

  # Converts an object into a string suitable for use as a URL query string,
  # using the given <tt>key</tt> as the param name.
  def to_query(key)
    "#{CGI.escape(key.to_param)}=#{CGI.escape(to_param.to_s)}"
  end
end

class NilClass
  # Returns a CGI-escaped +key+.
  def to_query(key)
    "#{CGI.escape(key.to_param)}"
  end

  # Returns +self+.
  def to_param
    self
  end
end

class TrueClass
  # Returns +self+.
  def to_param
    self
  end
end

class FalseClass
  # Returns +self+.
  def to_param
    self
  end
end

class Array
  # Calls <tt>to_param</tt> on all its elements and joins the result with
  # slashes. This is used by <tt>url_for</tt> in Action Pack.

Review comment (security):
What is the significance of the wrapping String interpolation?

Can `CGI.escape` return something other than a String? Does interpolation produce something different from `to_s`?

Code after:
# frozen_string_literal: true

require "cgi"

class Object
  # Alias of <tt>to_s</tt>.
  def to_param
    to_s
  end

  # Converts an object into a string suitable for use as a URL query string,
  # using the given <tt>key</tt> as the param name.
  def to_query(key)
    "#{CGI.escape(key.to_param)}=#{CGI.escape(to_param.to_s)}"
  end
end

class NilClass
  # Returns a CGI-escaped +key+.
  def to_query(key)
    CGI.escape(key.to_param)
  end

  # Returns +self+.
  def to_param
    self
  end
end

class TrueClass
  # Returns +self+.
  def to_param
    self
  end
end

class FalseClass
  # Returns +self+.
  def to_param
    self
  end
end

class Array
  # Calls <tt>to_param</tt> on all its elements and joins the result with
  # slashes. This is used by <tt>url_for</tt> in Action Pack.
Repository: rails/rails
File: activestorage/lib/active_storage.rb (Ruby)

Code before:
    "wave",
    "wavelet_denoise",
    "weight",
    "white_balance",
    "white_point",
    "white_threshold",
    "window",
    "window_group"
  ]
  mattr_accessor :unsupported_image_processing_arguments

  mattr_accessor :service_urls_expire_in, default: 5.minutes
  mattr_accessor :touch_attachment_records, default: true
  mattr_accessor :urls_expire_in

  mattr_accessor :routes_prefix, default: "/rails/active_storage"
  mattr_accessor :draw_routes, default: true
  mattr_accessor :resolve_model_to_route, default: :rails_storage_redirect

  mattr_accessor :track_variants, default: false

  mattr_accessor :video_preview_arguments, default: "-y -vframes 1 -f image2"

  module Transformers
    extend ActiveSupport::Autoload

    autoload :Transformer
    autoload :ImageProcessingTransformer
  end
end


Review comment (bug):
I did quick check and it seems DigestError is raised. Is it worth to prevent general rescue here?

```
irb(main):004:0> OpenSSL::Digest::MD5.hexdigest("abc")
Traceback (most recent call last):
        8: from /usr/bin/irb:23:in `<main>'
        7: from /usr/bin/irb:23:in `load'
        6: from /usr/lib/ruby/gems/2.7.0/gems/irb-1.2.6/exe/irb:11:in `<top (required)>'
        5: from (irb):4
        4: from /var/lib/gems/2.7.0/gems/openssl-3.3.0/lib/openssl/digest.rb:38:in `block (3 levels) in <class:Digest>'
        3: from /var/lib/gems/2.7.0/gems/openssl-3.3.0/lib/openssl/digest.rb:38:in `new'
        2: from /var/lib/gems/2.7.0/gems/openssl-3.3.0/lib/openssl/digest.rb:31:in `block (3 levels) in <class:Digest>'
        1: from /var/lib/gems/2.7.0/gems/openssl-3.3.0/lib/openssl/digest.rb:31:in `initialize'
OpenSSL::Digest::DigestError (Digest initialization failed: initialization error)

```

Code after:
    "wave",
    "wavelet_denoise",
    "weight",
    "white_balance",
    "white_point",
    "white_threshold",
    "window",
    "window_group"
  ]
  mattr_accessor :unsupported_image_processing_arguments

  mattr_accessor :service_urls_expire_in, default: 5.minutes
  mattr_accessor :touch_attachment_records, default: true
  mattr_accessor :urls_expire_in

  mattr_accessor :routes_prefix, default: "/rails/active_storage"
  mattr_accessor :draw_routes, default: true
  mattr_accessor :resolve_model_to_route, default: :rails_storage_redirect

  mattr_accessor :track_variants, default: false

  singleton_class.attr_accessor :checksum_implementation
  @checksum_implementation = OpenSSL::Digest::MD5
  begin
    @checksum_implementation.hexdigest("test")
  rescue # OpenSSL may have MD5 disabled
    require "digest/md5"
    @checksum_implementation = Digest::MD5
  end

  mattr_accessor :video_preview_arguments, default: "-y -vframes 1 -f image2"

  module Transformers
    extend ActiveSupport::Autoload

    autoload :Transformer
    autoload :ImageProcessingTransformer
  end
end
Repository: rails/rails
File: actionview/lib/action_view/cache_expiry.rb (Ruby)

Code before:
        @previous_change || @watcher.updated?
      end

      def execute
        return unless @watcher

        watcher = nil
        @mutex.synchronize do
          @previous_change = false
          watcher = @watcher
        end
        watcher.execute
      end

      private
        def reload!
          ActionView::LookupContext::DetailsKey.clear
        end

        def build_watcher
          @mutex.synchronize do
            old_watcher = @watcher

            if @watched_dirs != dirs_to_watch
              @watched_dirs = dirs_to_watch
              new_watcher = @watcher_class.new([], @watched_dirs, filter_path: Rails.root.to_s) do
                reload!
              end
              @watcher = new_watcher

              # We must check the old watcher after initializing the new one to
              # ensure we don't miss any events
              @previous_change ||= old_watcher&.updated?
            end
          end
        end

        def rebuild_watcher
          return unless @watcher
          build_watcher
        end

        def dirs_to_watch
          all_view_paths.uniq.sort
        end

        def all_view_paths
          ActionView::PathRegistry.all_file_system_resolvers.map(&:path)
        end
    end
  end

Review comment (bug):
This is coupling Action View with Railties in the wrong place.

We should pass a configuration to ActionView in its Railtie and use it here.

Code after:
        @previous_change || @watcher.updated?
      end

      def execute
        return unless @watcher

        watcher = nil
        @mutex.synchronize do
          @previous_change = false
          watcher = @watcher
        end
        watcher.execute
      end

      private
        def reload!
          ActionView::LookupContext::DetailsKey.clear
        end

        def build_watcher
          @mutex.synchronize do
            old_watcher = @watcher

            if @watched_dirs != dirs_to_watch
              @watched_dirs = dirs_to_watch
              new_watcher = @watcher_class.new([], @watched_dirs) do
                reload!
              end
              @watcher = new_watcher

              # We must check the old watcher after initializing the new one to
              # ensure we don't miss any events
              @previous_change ||= old_watcher&.updated?
            end
          end
        end

        def rebuild_watcher
          return unless @watcher
          build_watcher
        end

        def dirs_to_watch
          all_view_paths.uniq.sort
        end

        def all_view_paths
          ActionView::PathRegistry.all_file_system_resolvers.map(&:path)
        end
    end
  end