-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
tag_list is declared an attribute #1064
Comments
Do you want to open a PR to fix this ? |
@seuros , it probably won't be hard to fix. If it was on me I'd just remove this WDYT, can we just remove it as an attribute or do you know good use cases for that? |
`has_attribute?` is not enough because one can have an attribute not associated with a column. Like in acts-as-taggable-on version 6+, see mbleigh/acts-as-taggable-on#1064
`has_attribute?` is not enough because one can have an attribute not associated with a column. Like in acts-as-taggable-on version 6+, see mbleigh/acts-as-taggable-on#1064
This is especially troublesome when combined with the lack of eager-loading functionality(#91 (comment)). I found myself needing something like the following to get certain use cases to work:
|
tag_list
being an attribute seems to be introduced with [1]. Now whenever a list of taggables are converted to JSON, there are N+1 queries performed to fetch all tags. Actually 2 additional queries per object:To avoid this, I have to add to the model a method like this:
I believe it is safer (and less unexpected) not to declare
tag_list
as an attribute. It is more natural to have it inincludes
than always being retrieved. No different from any other linked model. I don't thinktag_list
is more important than e.g.orders
orinvoices
.[1] https://github.com/mbleigh/acts-as-taggable-on/pull/887/files#diff-f3570227aa94bb5887e3cb70c6a69fb30861dbecaa1ee498153371a1e10fc314R41
The text was updated successfully, but these errors were encountered: