Deprecate plural association names on singular associations

[HParker]

Before this change associations could be called by their plural or singular name,

i.e.

  class Post < ActiveRecord::Base
    has_many :comments
  end

  class Comment < ActiveRecord::Base
    belongs_to :post
  end

  # this works with `post` as the key:
  Comment.where(post: Post.first)
  # this also works with `:posts` as the key (after this change this will warn):
  Comment.where(posts: Post.first)

This PR deprecates using the plural association name when the association was not defined as plural.

Motivated by #45020 which found that singularize call is slow and happens very often when you are querying a column name which is not a relation, so it must try both paths. After this deprecation cycle we can save that time in the where(column_name: "value") case and where(association: "value") case.

After this deprecation cycle, we can also avoid a potentially annoying edge case where you are unable to query a column with a singular name the same as your association's name.

It seems this singularize was added 2 years ago here: #39465 , however inheritance and calculation tests still pass with this change removed. I also added a test for the enum case that is referenced in the the issue associated with the above PR to be extra safe.

for extra clarity, here is a test case that will pass after this deprecation that doesn't today,

# frozen_string_literal: true

require "bundler/inline"

gemfile(true) do
  source "https://rubygems.org"

  git_source(:github) { |repo| "https://github.com/#{repo}.git" }

  gem "rails", github: "rails/rails", branch: "main"
  gem "sqlite3"
end

require "active_record"
require "minitest/autorun"
require "logger"

# This connection will do for database-independent bug reports.
ActiveRecord::Base.establish_connection(adapter: "sqlite3", database: ":memory:")
ActiveRecord::Base.logger = Logger.new(STDOUT)

ActiveRecord::Schema.define do
  create_table :posts, force: true do |t|
  end

  create_table :comments, force: true do |t|
    t.string :posts
    t.integer :post_id
  end
end

class Post < ActiveRecord::Base
  has_many :comments
end

class Comment < ActiveRecord::Base
  belongs_to :post
end

class BugTest < Minitest::Test
  def test_association_stuff
    comment = Comment.create!(posts: "hi there")

    assert_equal 1, Comment.where(posts: "hi there").count
  end
end

[byroot]

Nice find. Looking at the commit that introduced this, it seems like an accidental feature. Honestly I'm not even sure whether this is worth a deprecation cycle, as it never was actually a feature, it just happened to work.

[HParker]

Yeah, I tested this and we never hit the deprecation warning in our test suite. I am up for skipping the deprecation cycle as well unless anyone feels strongly.

[byroot]

I also tested this branch on our app, we do hit this deprecation a bit but it's clearly a mistake and will be very easy to fix on our side. So I really think we should not bother with a deprecation.

[byroot]

At best if we want to make the error easier to fix, we could look into a "did_you_mean" style of error message.

[HParker]

@byroot Updated with a DidYouMean error. I really like the way it reads actually! Should we still have a date/release in mind where we remove this error? We still will be paying the singularize cost when we query by column name with this change.

[byroot]

We still will be paying the singularize cost when we query by column name with this change.

Ah right, I was hopping we wouldn't but I now see how it's tricky.

What about introducing an Active Record setting to disable this check, and have it default to disabled on 7.1 ?

[byroot]

Actually thinking again about it. We really shouldn't bother. Let's just strip it now.

[HParker]

I love deleting code! Updated.

Yeah, later we check if the table_name matches the table name, so I think every real world case will get caught by that before doing anything unexpected.