Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Rails/InverseOf false positive #5236

Closed
printercu opened this issue Dec 13, 2017 · 6 comments
Closed

Rails/InverseOf false positive #5236

printercu opened this issue Dec 13, 2017 · 6 comments

Comments

@printercu
Copy link

Same as #5235 but for InverseOf.

  with_options inverse_of: :user do
    has_many :portfolio_items, through: :portfolios
  end
  has_many :portfolio_items, through: :portfolios, inverse_of: :user

=>

app/models/user.rb:11:5: C: Rails/InverseOf: Specify an :inverse_of option.
    has_many :portfolio_items, through: :portfolios
    ^^^^^^^^
@delphaber
Copy link

delphaber commented Dec 14, 2017 •
edited
Loading

I'm reading this and it says:

There are a few limitations to :inverse_of support:

  • They do not work with :through associations.
  • They do not work with :polymorphic associations.
  • They do not work with :as associations.

So why do rubocop tell me to add inverse_of on has_many ... through: ... association ? 🤔

@j15e
Copy link

j15e commented Dec 18, 2017

Same issue as #4751 . A fix was made here : 0ec9f6e

So we could add a similar check for :inverse_of

@AndKiel AndKiel mentioned this issue Dec 19, 2017
Closed
@wata727
Copy link
Contributor

wata727 commented Dec 19, 2017

I tried verifying some patterns, but it seems that :inverse_of works with :as associations correctly.

class Picture < ApplicationRecord
  belongs_to :imageable, polymorphic: true
end
 
class Employee < ApplicationRecord
  has_many :pictures, as: :imageable, inverse_of: :imageable
end
 
class Product < ApplicationRecord
  has_many :pictures, as: :imageable
end
irb(main):001:0> employee = Employee.first
irb(main):002:0> picture = employee.pictures.build
irb(main):003:0> employee.equal? picture.imageable
=> true

However, it does not seem to work with :through or :polymorphic. I'm planning to fix this.

tagliala added a commit to diowa/icare that referenced this issue Dec 21, 2017
@tagliala
Ignore RuboCop false positives
da56ae8 
Ref: rubocop/rubocop#5236, ref: rubocop/rubocop#5230
@wata727 wata727 mentioned this issue Dec 21, 2017
Merged
8 tasks
wata727 added a commit to wata727/rubocop that referenced this issue Dec 23, 2017
@wata727
Improve inspection for Rails/InverseOf
98640a4 
See rubocop#5236

Currently, `Rails/InverseOf` Cop checks associations unspecified
`inverse_of` and included `conditions`, `through`, `polymorphic`,
or `foreign_key`. However, this inspection is incorrect.

For example, for `through` and `polymorphic` you have to add
`inverse_of` to the inverse association of them.

```ruby

class Physician < ApplicationRecord
  has_many :appointments
  has_many :patients, through: :appointments, inverse_of: physicians
end

class Appointment < ApplicationRecord
  belongs_to :physician
  belongs_to :patient
end

class Patient < ApplicationRecord
  has_many :appointments
  has_many :physicians, through: :appointments, inverse_of: :patients
end

class Physician < ApplicationRecord
  has_many :appointments
  has_many :patients, through: :appointments
end

class Appointment < ApplicationRecord
  belongs_to :physician, inverse_of: :appointments
  belongs_to :patient, inverse_of: :appointments
end

class Patient < ApplicationRecord
  has_many :appointments
  has_many :physicians, through: :appointments
end
```

```ruby

class Picture < ApplicationRecord
  belongs_to :imageable, polymorphic: true, inverse_of: :pictures
end

class Employee < ApplicationRecord
  has_many :pictures, as: :imageable
end

class Product < ApplicationRecord
  has_many :pictures, as: :imageable
end

class Picture < ApplicationRecord
  belongs_to :imageable, polymorphic: true
end

class Employee < ApplicationRecord
  has_many :pictures, as: :imageable, inverse_of: :imageable
end

class Product < ApplicationRecord
  has_many :pictures, as: :imageable, inverse_of: :imageable
end
```

Unfortunately, even if this Cop looks at the structure of the `send`
node, it can't check for `through` associations. So, it ignores them.
In the case of `polymorphic`, this Cop should check on the inverse
`as` associations. In addition, I added the check of `class_name`
option.

This Cop still includes false positives when using `Object#with_options`.
Perhaps I will fix this problem as another pull request.
bbatsov pushed a commit that referenced this issue Dec 24, 2017
@wata727 @bbatsov
Improve inspection for Rails/InverseOf
c3253dd 
See #5236

Currently, `Rails/InverseOf` Cop checks associations unspecified
`inverse_of` and included `conditions`, `through`, `polymorphic`,
or `foreign_key`. However, this inspection is incorrect.

For example, for `through` and `polymorphic` you have to add
`inverse_of` to the inverse association of them.

```ruby

class Physician < ApplicationRecord
  has_many :appointments
  has_many :patients, through: :appointments, inverse_of: physicians
end

class Appointment < ApplicationRecord
  belongs_to :physician
  belongs_to :patient
end

class Patient < ApplicationRecord
  has_many :appointments
  has_many :physicians, through: :appointments, inverse_of: :patients
end

class Physician < ApplicationRecord
  has_many :appointments
  has_many :patients, through: :appointments
end

class Appointment < ApplicationRecord
  belongs_to :physician, inverse_of: :appointments
  belongs_to :patient, inverse_of: :appointments
end

class Patient < ApplicationRecord
  has_many :appointments
  has_many :physicians, through: :appointments
end
```

```ruby

class Picture < ApplicationRecord
  belongs_to :imageable, polymorphic: true, inverse_of: :pictures
end

class Employee < ApplicationRecord
  has_many :pictures, as: :imageable
end

class Product < ApplicationRecord
  has_many :pictures, as: :imageable
end

class Picture < ApplicationRecord
  belongs_to :imageable, polymorphic: true
end

class Employee < ApplicationRecord
  has_many :pictures, as: :imageable, inverse_of: :imageable
end

class Product < ApplicationRecord
  has_many :pictures, as: :imageable, inverse_of: :imageable
end
```

Unfortunately, even if this Cop looks at the structure of the `send`
node, it can't check for `through` associations. So, it ignores them.
In the case of `polymorphic`, this Cop should check on the inverse
`as` associations. In addition, I added the check of `class_name`
option.

This Cop still includes false positives when using `Object#with_options`.
Perhaps I will fix this problem as another pull request.
@wata727 wata727 mentioned this issue Dec 25, 2017
Merged
8 tasks
wata727 added a commit to wata727/rubocop that referenced this issue Dec 26, 2017
@wata727
[Fix rubocop#5236] Fix false positives for Rails/InverseOf
f9e03d1 
Fixes rubocop#5236

When using `Object#with_options`, `:inverse_of` works even if
doesn't declare it directly in associations.

```ruby
class Blog < ApplicationRecord
  with_options inverse_of: :blog do # good
    has_many :posts, -> { order(published_at: :desc) }
  end
end
```

So if the association is in a block with `with_options`, this Cop
check that options.
bbatsov pushed a commit that referenced this issue Dec 26, 2017
@wata727 @bbatsov
[Fix #5236] Fix false positives for Rails/InverseOf
5057b24 
Fixes #5236

When using `Object#with_options`, `:inverse_of` works even if
doesn't declare it directly in associations.

```ruby
class Blog < ApplicationRecord
  with_options inverse_of: :blog do # good
    has_many :posts, -> { order(published_at: :desc) }
  end
end
```

So if the association is in a block with `with_options`, this Cop
check that options.
@wintersolutions wintersolutions mentioned this issue Dec 27, 2017
Closed
This was referenced Dec 27, 2017
Open
Closed
@bdewater
Copy link
Contributor

bdewater commented Jan 4, 2018

@delphaber AFAICS the documentation is wrong, I have a PR out to fix it: rails/rails#31446

@swrobel
Copy link
Contributor

swrobel commented Feb 14, 2018

Worth noting that inverse_of is automatically inferred from as in Rails 5.2 so this can be relaxed a bit for >= 5.2. rails/rails#28808

@bdewater
Copy link
Contributor

That was already taken care of in the original implementation of the cop - if it doesn't work it's a bug.

@akihikodaki akihikodaki mentioned this issue Feb 20, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
6 participants
@delphaber @j15e @swrobel @bdewater @printercu @wata727