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.

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

Improve inspection for Rails/InverseOf #5297

Merged

Conversation

wata727
Copy link
Contributor

@wata727 wata727 commented Dec 21, 2017

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.

# bad
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

# good
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
# bad
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

# good
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.


Before submitting the PR make sure the following are checked:

  • Wrote good commit messages.
  • Commit message starts with [Fix #issue-number] (if the related issue exists).
  • Feature branch is up-to-date with master (if not - rebase it).
  • Squashed related commits together.
  • Added tests.
  • Added an entry to the Changelog if the new code introduces user-observable changes. See changelog entry format.
  • The PR relates to only one subject with a clear title
    and description in grammatically correct, complete sentences.
  • Run rake default or rake parallel. It executes all tests and RuboCop for itself, and generates the documentation.

@koic
Copy link
Member

koic commented Dec 23, 2017

Thanks for opening PR. #5236 reports on false positives with with_options.

This Cop still includes false positives when using Object#with_options. Perhaps I will fix this problem as another pull request.

If you don't resolve with_options in this PR, you should not write Fixes #5236 in the body of the commit message. Because it closes #5236 unresolved.

@wata727 wata727 force-pushed the improve_detection_for_rails_inverse_of branch from 9747a4a to adee543 Compare December 23, 2017 11:16
@wata727 wata727 changed the title [Fix #5236] Improve inspection for Rails/InverseOf Improve inspection for Rails/InverseOf Dec 23, 2017
@wata727
Copy link
Contributor Author

wata727 commented Dec 23, 2017

Thanks for the advice. I removed Fix #5236 from this pull request and this commit :)

@koic
Copy link
Member

koic commented Dec 23, 2017

You are welcome. I think that writing a link to #5236 in the commit message is good for showing a practical issue. For example, I prefer Refer #5236, Follow up of #5236, and others 😃

In this case it would be better to show the issue without using the following keywords.
https://help.github.com/articles/closing-issues-using-keywords/

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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants