Add a thin encapsulation around .pluck(:id)
What does this MR do?
Adds a pluck_primary_key
method to ApplicationRecord
. This is available in every ActiveRecord::Relation
, meaning that we can call, for instance, Project.scopeA.scopeB.pluck_primary_key
to get a list of IDs for the project.
Typically, pluck(:id)
is forbidden by the CodeReuse/ActiveRecord
cop. Justifications for this revolve around the fact that it leaks knowledge about the database schema from app/models
into other areas where it shoudn't exist, and that it's difficult to maintain this as the schema changes.
We often need to acquire a list of IDs legitimate reasons; I've modified a few sites in app/services
to use this helper, mostly to highlight ways it's being used at present. In reviews, I'm noticing that people are just starting to sprinkle # rubocop:disable CodeReuse/ActiveRecord
around instead of trying to avoid or relocate the pluck
, which is very sub-optimal. Hopefully, this thin wrapper treads a nice middle ground between the two concerns.
Of course, one should (almost) never use pluck
to get a list of IDs to feed back into another SQL query, and this helper should not be used as such. I don't believe it makes it more likely.
cc @yorickpeterse @lulalala @dbalexandre
What are the relevant issue numbers?
Does this MR meet the acceptance criteria?
-
Changelog entry added, if necessary -
Documentation created/updated via this MR -
Documentation reviewed by technical writer or follow-up review issue created -
Tests added for this feature/bug -
Tested in all supported browsers -
Conforms to the code review guidelines -
Conforms to the merge request performance guidelines -
Conforms to the style guides -
Conforms to the database guides -
Link to e2e tests MR added if this MR has Requires e2e tests label. See the Test Planning Process. -
Security reports checked/validated by reviewer