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

tag_list is declared an attribute #1064

Open
akostadinov opened this issue Nov 25, 2021 · 3 comments
Open

tag_list is declared an attribute #1064

akostadinov opened this issue Nov 25, 2021 · 3 comments

Comments

@akostadinov
Copy link

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:

  ActsAsTaggableOn::Tagging Load (0.3ms)  SELECT `taggings`.* FROM `taggings` WHERE `taggings`.`taggable_id` = 3 AND `taggings`.`taggable_type` = 'CMS::File'
  ActsAsTaggableOn::Tag Load (0.4ms)  SELECT `tags`.* FROM `tags` INNER JOIN `taggings` ON `tags`.`id` = `taggings`.`tag_id` WHERE `taggings`.`taggable_id` = 3 AND `taggings`.`taggable_type` = 'CMS::File' AND (taggings.context = 'tags' AND taggings.tagger_id IS NULL)

To avoid this, I have to add to the model a method like this:

  def as_json(options = {})
    except = Array.wrap(options[:except]) + [:tag_list]
    super options.merge({except: except})
  end

I believe it is safer (and less unexpected) not to declare tag_list as an attribute. It is more natural to have it in includes than always being retrieved. No different from any other linked model. I don't think tag_list is more important than e.g. orders or invoices.

[1] https://github.com/mbleigh/acts-as-taggable-on/pull/887/files#diff-f3570227aa94bb5887e3cb70c6a69fb30861dbecaa1ee498153371a1e10fc314R41

@seuros
Copy link
Collaborator

seuros commented Nov 25, 2021

Do you want to open a PR to fix this ?

@akostadinov
Copy link
Author

@seuros , it probably won't be hard to fix. If it was on me I'd just remove this attribute declaration. But I have no idea it if has any usefulness for anybody. In case there are good use cases, another approach might be to hack #as_json from acts-as-taggable-on (which I don't really like).

WDYT, can we just remove it as an attribute or do you know good use cases for that?

akostadinov added a commit to 3scale/porta that referenced this issue Nov 29, 2021
`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
akostadinov added a commit to 3scale/porta that referenced this issue Dec 4, 2021
`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
@Adverbly
Copy link

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:

  def tag_names
    tags.loaded? ? tags&.map(&:name) : tag_list
  end

  def as_json
    super(except: :tag_list).merge(tag_list: tag_names)
  end

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

No branches or pull requests

3 participants