ronantakizawa/codereview-bench huggingface dataset:
Repository: rails/rails
File: activerecord/lib/active_record/signed_id.rb (Ruby)

Code before:
# frozen_string_literal: true

module ActiveRecord
  # = Active Record Signed Id
  module SignedId
    extend ActiveSupport::Concern

    included do
      ##
      # :singleton-method:
      # Set the secret used for the signed id verifier instance when using Active Record outside of \Rails.
      # Within \Rails, this is automatically set using the \Rails application key generator.
      class_attribute :signed_id_verifier_secret, instance_writer: false

      # :nodoc:
      class_attribute :_signed_id_verifier, instance_accessor: false, instance_predicate: false

      class << self
        alias_method :original_signed_id_verifier_secret=, :signed_id_verifier_secret=
        # :nodoc:
        def signed_id_verifier_secret=(secret)
          ActiveRecord.deprecator.warn(<<~MSG)
            ActiveRecord::Base.signed_id_verifier_secret is deprecated and will be removed in the future. The secret will be derived from 'secret_key_base' instead.
          MSG

          self.original_signed_id_verifier_secret = secret
        end
      end
    end

    module RelationMethods # :nodoc:
      def find_signed(...)
        scoping { model.find_signed(...) }
      end

      def find_signed!(...)
        scoping { model.find_signed!(...) }
      end
    end


Review comment (suggestion):
RDoc does not see `class_attribute` by default.  (That's why we add `:singleton-method:` when needed.)

```suggestion
```


Code after:
# frozen_string_literal: true

module ActiveRecord
  # = Active Record Signed Id
  module SignedId
    extend ActiveSupport::Concern

    included do
      class_attribute :_signed_id_verifier, instance_accessor: false, instance_predicate: false

      ##
      # :singleton-method:
      # Set the secret used for the signed id verifier instance when using Active Record outside of \Rails.
      # Within \Rails, this is automatically set using the \Rails application key generator.
      class_attribute :signed_id_verifier_secret, instance_writer: false
      module DeprecateSignedIdVerifierSecret
        def signed_id_verifier_secret=(secret)
          ActiveRecord.deprecator.warn(<<~MSG)
            ActiveRecord::Base.signed_id_verifier_secret is deprecated and will be removed in the future.

            If the secret is model-specific, set Model.signed_id_verifier instead.

            Otherwise, configure Rails.application.message_verifiers (or ActiveRecord.message_verifiers) with the secret.
          MSG

          super
        end
      end
      singleton_class.prepend DeprecateSignedIdVerifierSecret
    end

    module RelationMethods # :nodoc:
      def find_signed(...)
        scoping { model.find_signed(...) }
      end

      def find_signed!(...)
        scoping { model.find_signed!(...) }
      end
    end
Repository: rails/rails
File: activerecord/test/cases/signed_id_test.rb (Ruby)

Code before:
# frozen_string_literal: true

require "cases/helper"
require "models/account"
require "models/company"
require "models/toy"
require "models/matey"

class SignedIdTest < ActiveRecord::TestCase
  class GetSignedIDInCallback < ActiveRecord::Base
    self.table_name = "accounts"
    after_create :set_signed_id
    attr_reader :signed_id_from_callback

    private
      def set_signed_id
        @signed_id_from_callback = signed_id
      end
  end

  fixtures :accounts, :toys, :companies

  setup do
    ActiveRecord::Base.signed_id_verifier = ActiveSupport::MessageVerifier.new("secret")

    @account = Account.first
    @toy = Toy.first
  end

  teardown do
    ActiveRecord::Base.signed_id_verifier = nil
  end

  test "find signed record" do
    assert_equal @account, Account.find_signed(@account.signed_id)
  end

  test "find signed record on relation" do
    assert_equal @account, Account.where("1=1").find_signed(@account.signed_id)

    assert_nil Account.where("1=0").find_signed(@account.signed_id)
  end

  test "find signed record with custom primary key" do
    assert_equal @toy, Toy.find_signed(@toy.signed_id)
  end

  test "find signed record for single table inheritance (STI Models)" do
    assert_equal Company.first, Company.find_signed(Company.first.signed_id)

Review comment (suggestion):
The base case is using `ActiveRecord.message_verifiers`, not `ActiveRecord::Base.signed_id_verifier`.  So we should set `ActiveRecord.message_verifiers` here, and only set `ActiveRecord::Base.signed_id_verifier` in tests where we want to exercise its code path.

Code after:
# frozen_string_literal: true

require "cases/helper"
require "models/account"
require "models/company"
require "models/toy"
require "models/matey"

class SignedIdTest < ActiveRecord::TestCase
  class GetSignedIDInCallback < ActiveRecord::Base
    self.table_name = "accounts"
    after_create :set_signed_id
    attr_reader :signed_id_from_callback

    private
      def set_signed_id
        @signed_id_from_callback = signed_id
      end
  end

  fixtures :accounts, :toys, :companies

  setup do
    @original_message_verifiers = ActiveRecord.message_verifiers
    ActiveRecord.message_verifiers = ActiveSupport::MessageVerifiers.new { "secret" }.rotate_defaults

    @account = Account.first
    @toy = Toy.first
  end

  teardown do
    ActiveRecord.message_verifiers = @original_message_verifiers
  end

  test "find signed record" do
    assert_equal @account, Account.find_signed(@account.signed_id)
  end

  test "find signed record on relation" do
    assert_equal @account, Account.where("1=1").find_signed(@account.signed_id)

    assert_nil Account.where("1=0").find_signed(@account.signed_id)
  end

  test "find signed record with custom primary key" do
    assert_equal @toy, Toy.find_signed(@toy.signed_id)
  end

  test "find signed record for single table inheritance (STI Models)" do
Repository: rails/rails
File: activerecord/test/cases/serialized_attribute_test.rb (Ruby)

Code before:
  end

  def test_mutation_detection_does_not_double_serialize
    coder = Object.new
    def coder.dump(value)
      return if value.nil?
      value + " encoded"
    end
    def coder.load(value)
      return if value.nil?
      value.gsub(" encoded", "")
    end
    type = Class.new(ActiveModel::Type::Value) do
      def serialize(value)
        return if value.nil?
        value + " serialized"
      end

      def deserialize(value)
        return if value.nil?
        value.gsub(" serialized", "")
      end

      def cast(value)
        deserialize(serialize(value))
      end
    end.new
    model = Class.new(Topic) do
      attribute :foo, type
      serialize :foo, coder: coder
    end

    topic = model.create!(foo: "bar")
    topic.foo
    assert_not_predicate topic, :changed?
  end

  def test_serialized_attribute_works_under_concurrent_initial_access
    model = Class.new(Topic)

    topic = model.create!
    topic.update group: "1"

    model.serialize :group, coder: JSON
    model.reset_column_information

    # This isn't strictly necessary for the test, but a little bit of
    # knowledge of internals allows us to make failures far more likely.
    model.define_singleton_method(:define_attribute) do |*args, **options|
      Thread.pass
      super(*args, **options)

Review comment (suggestion):
If `mutable` becomes the default, these The `Mutable#cast` implementation (and other related methods) should move into `Value`, and the `Value#cast` implementation (and other related methods) should move into `Immutable`.

Code after:
  def test_mutation_detection_does_not_double_serialize
    coder = Object.new
    def coder.dump(value)
      return if value.nil?
      value + " encoded"
    end
    def coder.load(value)
      return if value.nil?
      value.gsub(" encoded", "")
    end
    type = Class.new(ActiveModel::Type::Value) do
      include ActiveModel::Type::Helpers::Mutable

      def serialize(value)
        return if value.nil?
        value + " serialized"
      end

      def deserialize(value)
        return if value.nil?
        value.gsub(" serialized", "")
      end
    end.new
    model = Class.new(Topic) do
      attribute :foo, type
      serialize :foo, coder: coder
    end

    topic = model.create!(foo: "bar")
    topic.foo
    assert_not_predicate topic, :changed?
  end

  def test_serialized_attribute_works_under_concurrent_initial_access
    model = Class.new(Topic)

    topic = model.create!
    topic.update group: "1"

    model.serialize :group, coder: JSON
    model.reset_column_information

    # This isn't strictly necessary for the test, but a little bit of
    # knowledge of internals allows us to make failures far more likely.
    model.define_singleton_method(:define_attribute) do |*args, **options|
      Thread.pass
      super(*args, **options)
    end

    threads = 4.times.map do
      Thread.new do
Repository: rails/rails
File: activesupport/lib/active_support/error_reporter.rb (Ruby)

Code before:
        end

        count = 0
        while error.backtrace_locations.first&.path == __FILE__
          count += 1
          error.backtrace_locations.shift
        end

        error.backtrace.shift(count)
      end

      class ErrorContextMiddlewareStack
        def initialize
          @stack = []
        end

        def use(middleware)
          unless middleware.respond_to?(:call)
            raise ArgumentError, "Error context middleware must respond to #call"
          end

          @stack << middleware
        end

        def execute(error, context)
          @stack.each { |middleware| middleware.call(error, context) }
        end
      end
  end
end


Review comment (suggestion):
I was thingking:

```suggestion
          @stack.inject(context) { |middleware| middleware.call(error, context) }
```

This way the contract is that the middleware return the extended context. You cna mutate it if you want but also return a different one.

It also open the door to return `false` or something like that to stop the reporting.

Code after:
        error = error.cause
      end

      nil
    end

    private
      def ensure_backtrace(error)
        return if error.frozen? # re-raising won't add a backtrace
        return unless error.backtrace.nil?

        begin
          # We could use Exception#set_backtrace, but until Ruby 3.4
          # it only support setting `Exception#backtrace` and not
          # `Exception#backtrace_locations`. So raising the exception
          # is a good way to build a real backtrace.
          raise error
        rescue error.class => error
        end

        count = 0
        while error.backtrace_locations.first&.path == __FILE__
          count += 1
          error.backtrace_locations.shift
        end

        error.backtrace.shift(count)
      end

      class ErrorContextMiddlewareStack # :nodoc:
        def initialize
          @stack = []
        end

        # Add a middleware to the error context stack.
        def use(middleware)
          unless middleware.respond_to?(:call)
            raise ArgumentError, "Error context middleware must respond to #call"
          end

          @stack << middleware
        end

        # Run all middlewares in the stack on an error and context.
        def execute(error, context)
          @stack.inject(context) { |c, middleware| middleware.call(error, c) }
        end
      end
  end
end
Repository: rails/rails
File: railties/test/generators/app_generator_test.rb (Ruby)

Code before:
      assert_match(%r{queue:}, content)
      assert_match(%r{cable:}, content)
    end
  end

  def test_inclusion_of_kamal_files
    generator [destination_root]
    run_generator_instance

    assert_file "config/deploy.yml"
    assert_file ".kamal/secrets"
  end

  def test_kamal_files_are_skipped_if_required
    generator [destination_root], ["--skip-kamal"]
    run_generator_instance

    assert_empty @bundle_commands.grep("binstubs kamal")
    assert_empty @bundle_commands.grep("exec kamal init")

    assert_no_file "config/deploy.yml"
    assert_no_file ".kamal/secrets"
  end

  def test_database_yml_skip_kamal
    # Regression test for #54549
    run_generator [destination_root, "--skip-kamal"]

    assert_file("config/database.yml") do |content|
      assert_match("db/queue_migrate", content)
      assert_match("db/cache_migrate", content)
      assert_match("db/cable_migrate", content)
    end
  end

  def test_inclusion_of_kamal_storage_volume
    generator [destination_root]
    run_generator_instance

    assert_equal 1, @bundle_commands.count("binstubs kamal")
    assert_equal 1, @bundle_commands.count("exec kamal init")

    assert_file "config/deploy.yml" do |content|
      assert_match(%r{storage:/rails/storage}, content)
    end
  end

  def test_inclusion_of_kamal_storage_volume_if_only_skip_active_storage_is_given
    generator [destination_root], ["--skip-active-storage"]
    run_generator_instance


Review comment (suggestion):
I think git history will usually indicate why a change was made so I'm not sure the comment is necessary

Code after:
      assert_match(%r{queue:}, content)
      assert_match(%r{cable:}, content)
    end
  end

  def test_inclusion_of_kamal_files
    generator [destination_root]
    run_generator_instance

    assert_file "config/deploy.yml"
    assert_file ".kamal/secrets"
  end

  def test_kamal_files_are_skipped_if_required
    generator [destination_root], ["--skip-kamal"]
    run_generator_instance

    assert_empty @bundle_commands.grep("binstubs kamal")
    assert_empty @bundle_commands.grep("exec kamal init")

    assert_no_file "config/deploy.yml"
    assert_no_file ".kamal/secrets"
  end

  def test_database_yml_skip_kamal
    run_generator [destination_root, "--skip-kamal"]

    assert_file("config/database.yml") do |content|
      assert_match("db/queue_migrate", content)
      assert_match("db/cache_migrate", content)
      assert_match("db/cable_migrate", content)
    end
  end

  def test_inclusion_of_kamal_storage_volume
    generator [destination_root]
    run_generator_instance

    assert_equal 1, @bundle_commands.count("binstubs kamal")
    assert_equal 1, @bundle_commands.count("exec kamal init")

    assert_file "config/deploy.yml" do |content|
      assert_match(%r{storage:/rails/storage}, content)
    end
  end

  def test_inclusion_of_kamal_storage_volume_if_only_skip_active_storage_is_given
    generator [destination_root], ["--skip-active-storage"]
    run_generator_instance

    assert_equal 1, @bundle_commands.count("binstubs kamal")
Repository: rails/rails
File: activerecord/lib/active_record/relation/finder_methods.rb (Ruby)

Code before:
          else
            []
          end
        end
      end

      def find_nth_from_last(index)
        if loaded?
          records[-index]
        else
          relation = ordered_relation

          if relation.order_values.empty? || relation.has_limit_or_offset?
            relation.records[-index]
          else
            relation.reverse_order.offset(index - 1).first
          end
        end
      end

      def find_last(limit)
        limit ? records.last(limit) : records.last
      end

      def ordered_relation
        if order_values.empty? && _order_columns.any?
          order(_order_columns.map { |column| table[column].asc })
        else
          self
        end
      end

      def _order_columns
        oc = []

        oc << model.implicit_order_column if model.implicit_order_column
        oc << model.query_constraints_list if model.query_constraints_list

        if model.primary_key && model.query_constraints_list.nil?
          oc << model.primary_key
        end

        oc.flatten.uniq.compact
      end
  end
end


Review comment (suggestion):
personal pet peeve. `any?` isn't the inverse of `empty?`, it ignores falsy members.

Not a huge deal here, but IMO `!_order_columns.empty?` is clearer.

Code after:
            relation.limit(limit).to_a
          else
            []
          end
        end
      end

      def find_nth_from_last(index)
        if loaded?
          records[-index]
        else
          relation = ordered_relation

          if relation.order_values.empty? || relation.has_limit_or_offset?
            relation.records[-index]
          else
            relation.reverse_order.offset(index - 1).first
          end
        end
      end

      def find_last(limit)
        limit ? records.last(limit) : records.last
      end

      def ordered_relation
        if order_values.empty? && !_order_columns.empty?
          order(_order_columns.map { |column| table[column].asc })
        else
          self
        end
      end

      def _order_columns
        oc = []

        oc << model.implicit_order_column if model.implicit_order_column
        oc << model.query_constraints_list if model.query_constraints_list

        if model.primary_key && model.query_constraints_list.nil?
          oc << model.primary_key
        end

        oc.flatten.uniq.compact
      end
  end
end
Repository: rails/rails
File: activerecord/lib/active_record/relation/finder_methods.rb (Ruby)

Code before:
        if loaded?
          records[-index]
        else
          relation = ordered_relation

          if relation.order_values.empty? || relation.has_limit_or_offset?
            relation.records[-index]
          else
            relation.reverse_order.offset(index - 1).first
          end
        end
      end

      def find_last(limit)
        limit ? records.last(limit) : records.last
      end

      def ordered_relation
        if order_values.empty? && !_order_columns.empty?
          order(_order_columns.map { |column| table[column].asc })
        else
          self
        end
      end

      def _order_columns
        columns = Array(model.implicit_order_column)

        return columns.compact if columns.length.positive? && columns.last.nil?

        columns += Array(model.query_constraints_list || model.primary_key)

        columns.uniq.compact
      end
  end
end


Review comment (suggestion):
```suggestion
              Relation has no order values, and #{model.name} has no order columns to use as a default.
```

`model` is a AR class here right? I think we should just use the name, because otherwise `AR::Base.inspect` can be super long and hard to read.

Code after:
        if loaded?
          records[-index]
        else
          relation = ordered_relation

          if relation.order_values.empty? || relation.has_limit_or_offset?
            relation.records[-index]
          else
            relation.reverse_order.offset(index - 1).first
          end
        end
      end

      def find_last(limit)
        limit ? records.last(limit) : records.last
      end

      def ordered_relation
        if order_values.empty?
          if !_order_columns.empty?
            return order(_order_columns.map { |column| table[column].asc })
          end

          if ActiveRecord.raise_on_missing_required_finder_order_columns
            raise MissingRequiredOrderError, <<~MSG.squish
              Relation has no order values, and #{model} has no order columns to use as a default.
              Set at least one of `implicit_order_column`, `query_constraints` or `primary_key` on
              the model when no `order `is specified on the relation.
            MSG
          else
            ActiveRecord.deprecator.warn(<<~MSG)
              Calling order dependent finder methods (e.g. `#first`, `#second`) without `order` values on the relation,
              and on a model (#{model}) that does not have any order columns (`implicit_order_column`, `query_constraints`,
              or `primary_key`) to fall back on is deprecated and will raise `ActiveRecord::MissingRequiredOrderError`
              in Rails 8.2.
            MSG

            self
          end
        else
          self
        end
      end

      def _order_columns
        columns = Array(model.implicit_order_column)

        return columns.compact if columns.length.positive? && columns.last.nil?

        columns += Array(model.query_constraints_list || model.primary_key)
Repository: rails/rails
File: activerecord/lib/active_record/relation/finder_methods.rb (Ruby)

Code before:
          if relation.order_values.empty? || relation.has_limit_or_offset?
            relation.records[-index]
          else
            relation.reverse_order.offset(index - 1).first
          end
        end
      end

      def find_last(limit)
        limit ? records.last(limit) : records.last
      end

      def ordered_relation
        if order_values.empty? && !_order_columns.empty?
          order(_order_columns.map { |column| table[column].asc })
        else
          self
        end
      end

      def _order_columns
        oc = []

        oc << model.implicit_order_column if model.implicit_order_column

        return oc.flatten.uniq.compact if oc.flatten.include?(nil)

        oc << model.query_constraints_list if model.query_constraints_list

        if model.primary_key && model.query_constraints_list.nil?
          oc << model.primary_key
        end

        oc.flatten.uniq.compact
      end
  end
end


Review comment (bug):
We should probably avoid the extra `flatten` here... it's not a super hot path, but still, this is an edge case behaviour, so unfortunate to add an allocation for every existing call.

We don't need to consider multiply-nested array levels; the existing unrestrained `flatten` below is just doing a bit more than it needs to.

So given that, I feel like we can ditch most flattening here entirely :thinking:

(untested speculation)

```ruby
columns = Array(model.implicit_order_column)
return columns.compact if columns.last.nil?

columns += Array(model.query_constraints_list || model.primary_key)

columns.uniq.compact
```

.. or something? 🤷🏻‍♂️

Code after:
          if relation.order_values.empty? || relation.has_limit_or_offset?
            relation.records[-index]
          else
            relation.reverse_order.offset(index - 1).first
          end
        end
      end

      def find_last(limit)
        limit ? records.last(limit) : records.last
      end

      def ordered_relation
        if order_values.empty? && !_order_columns.empty?
          order(_order_columns.map { |column| table[column].asc })
        else
          self
        end
      end

      def _order_columns
        columns = Array(model.implicit_order_column)

        return columns.compact if columns.length.positive? && columns.last.nil?

        columns += Array(model.query_constraints_list || model.primary_key)

        columns.uniq.compact
      end
  end
end