Cop Idea: DuplicateAssociation cop

[natematykiewicz]

Recently we had a few PRs created that defined the same association. They were both in code review at the same time, so both added the association that did not previously exist. When both got merged, we didn't realize that we now had the same association defined twice in 1 model. It would be nice if there were a cop that caught this and pointed it out.

For example:

belongs_to :foo
belongs_to :bar
belongs_to :blah
belongs_to :foo # <----- Duplicate

has_many :somethings

[andyw8]

I expect this should also cover cases where the definitions refer to the same field but aren't identical, e.g.:

belongs_to :foo
belongs_to :foo, required: false

[pirj]

Wondering how it works. Maybe duplicate declarations complement each other, like e.g. new-style validations do?

belongs_to :user, -> { joins(:friends) }
belongs_to :user, foreign_key: :account_id, class_name: 'Account'
belongs_to :user, counter_cache: true
belongs_to :user, touch: :employees_last_updated_at

If not, why Rails doesn't warn of a duplicate declaration?

Maybe it's like that to be able to group common options?

belongs_to :user, foreign_key: :account_id, class_name: 'Account'
with_options counter_cache: true do
  belongs_to :user
  belongs_to :book
end

What if belongs_to is defined on the parent class, but one decided to duplicate this declaration for explicitness in the child class?

It's fine to tag identical declarations, though. Unless one of them is scoped inside with_options.

[natematykiewicz]

Yeah, multiple belongs_to :foo lines, regardless of the options hash or a with_options are a duplicate association. Since :foo is the name of the association, you can't define it twice. I'm not actually sure what Rails does if you try to. I know with scopes it logs a warning that you're redefining an existing scope. Do associations work the same way? I assume it either uses the first or last definition, but does not merge the definitions. Whatever it does, I didn't notice. I found the duplicate association by looking in the file later.

I could see this sort of thing actually just being a Rails PR, to error on association redefinition. But considering a scope redefinition is just a log line, they might not be up for an error.

There's certainly a lot of cases that a cop couldn't cover. It wouldn't be able to tell if you're redefining an association that's already defined in a parent class or a concern, for example. But it would help with for the same file at least.

[pirj]

Apparently, the last declaration wins. This is quite unfortunate due to the case of with_options and limits its application. However, this behaviour is understandable for the case with redefining the association in a child class.
Since the latter case is legitimate, including prepended/included concerns, it's also understandable why Rails doesn't raise an error or does not warn.

Indeed, defining the same association twice in the same scope is a mistake, as the first declaration will be disregarded, and it's at best a redundant statement. Including the case when one of them is inside with_options.

Would you like to contribute a cop for it provided all necessary guidance?
I couldn't find a resembling cop that you could mimic. This one might be of some help to get an idea how to detect.
There are also duplicate helpers, not sure if they will come handy, though.
A node matcher that you could use is

ASSOCIATIONS = Set.new(%i[belongs_to has_one has_many has_and_belongs_to_many])
def_node_matcher :association?, <<~PATTERN
  (send nil? $#ASSOCIATIONS $_ ...)
PATTERN

Then in the cop:

def on_class(node)
  associations = node.children.map { |child| association?(node) }
  # reject [nil, nil] ones
  # => [[:has_many, :books], [:belongs_to, :city]]
  # now, find duplicates and add an offence:
  add_offense(node) do |corrector|
    # remove duplicates except for the last one
  end
end

The case with with_options might not be worth the effort.

[natematykiewicz]

@pirj I made a PR for this. #602