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_names plucks multiple times #84

Open
Skulli opened this issue Aug 22, 2022 · 3 comments
Open

tag_names plucks multiple times #84

Skulli opened this issue Aug 22, 2022 · 3 comments

Comments

@Skulli
Copy link

Skulli commented Aug 22, 2022

Have an existing Rails 7.0.3 app and installed that gem.

When calling a model with tag_names on an XYZ.last

Console puts out

Gutentag::Tag Pluck (1.2ms)  SELECT "gutentag_tags"."name" FROM "gutentag_tags" INNER JOIN "gutentag_taggings" ON "gutentag_tags"."id" = "gutentag_taggings"."tag_id" WHERE "gutentag_taggings"."taggable_id" = $1 AND "gutentag_taggings"."taggable_type" = $2  [["taggable_id", 38], ["taggable_type", "XYZ"]]
Gutentag::Tag Pluck (0.3ms)  SELECT "gutentag_tags"."name" FROM "gutentag_tags" INNER JOIN "gutentag_taggings" ON "gutentag_tags"."id" = "gutentag_taggings"."tag_id" WHERE "gutentag_taggings"."taggable_id" = $1 AND "gutentag_taggings"."taggable_type" = $2  [["taggable_id", 38], ["taggable_type", "XYZ"]]

Oddly when i do Gutentag::ActiveRecord.call self on the User-class (used with devise) and i do an User.last

User.last
User Load (1.1ms)  SELECT "users".* FROM "users" ORDER BY "users"."id" DESC LIMIT $1  [["LIMIT", 1]]
Gutentag::Tag Pluck (0.3ms)  SELECT "gutentag_tags"."name" FROM "gutentag_tags" INNER JOIN "gutentag_taggings" ON "gutentag_tags"."id" = "gutentag_taggings"."tag_id" WHERE "gutentag_taggings"."taggable_id" = $1 AND "gutentag_taggings"."taggable_type" = $2  [["taggable_id", 4], ["taggable_type", "User"]]
Gutentag::Tag Pluck (0.2ms)  SELECT "gutentag_tags"."name" FROM "gutentag_tags" INNER JOIN "gutentag_taggings" ON "gutentag_tags"."id" = "gutentag_taggings"."tag_id" WHERE "gutentag_taggings"."taggable_id" = $1 AND "gutentag_taggings"."taggable_type" = $2  [["taggable_id", 4], ["taggable_type", "User"]]

it calls tag_names directly. Seems the tag_names attribute is calculated on initialize. Did somebody had similar issues?
The second issue might be a different issue on my side, have to check wich addition causes that. Maybe paper_trail or something.

@rromanchuk
Copy link
Contributor

@Skulli I actually noticed something sort of related to your issue, but still digging in a bit.

The biggest thing I found is mostly my own user error, which basically stemmed from abusing convenience methods like tag_names and over complicating things.

tag_names is really just the same thing as post.tags.pluck(:name) and my extra database calls were from calling these in places i didn't realize i was calling it from. Especially tags_as_string (in my own concern), treating it like it was just an array or string, when they are associated, relational models.

For example, a case where it makes a lot more sense to just use the ActiveRecord

class PostsController < ApplicationController
  def show
    @post = Post.
                     .includes(:tags)
                     .friendly
                     .find(params[:id])
  end
end
<%# app/views/posts/show.html.erb %>
<%= render partial: 'posts/tag', collection: @post.tags, cached: true %>`
<%# app/views/posts/_tag.html.erb %>
<%= link_to tag.name, tag_posts_path(tag_name: tag.name), class: 'badge bg-secondary text-decoration-none link-light', data: {turbo_action: 'replace', turbo_frame: "_top" } %>

vs... things i was doing before, with zero consideration of eager loading or n+1 potential. Just in this simple example below i managed to basically break cache, yield a JOIN call to the layout and then again enumerate in the template as if they were just model properties of the instance.

<%= content_for(:page_keywords, post.tags_as_string %>

<% cache post do %>
  <% post.tag_names.each do |name| %>
    <%= link_to name, tag_posts_path(tag_name: name), class: 'badge bg-secondary text-decoration-none link-light', data: {turbo_action: 'replace', turbo_frame: "_top" } %>
  <% end %>
<% end %>

So my first guess would be something really subtle is actually triggering those loads. I can't actually repo with Post.last, but this is how it felt when i saw hits to the db well after the controller and during/in the middle of partial rendering.

pat added a commit that referenced this issue Oct 30, 2022
Related to #84.

It gets a bit complex in here - we want to ensure the dirty-state logic
from ActiveRecord behaves properly - knowing both what tag names were
and what they’ve become.

I’ve switched the default reset value back to an empty array rather than
nil, but we’re also resetting whether the value has been prepared, to
ensure the latest is obtained from the database _when required_.
@pat
Copy link
Owner

pat commented Oct 30, 2022

Thanks for reporting this @Skulli - it was indeed the fault of Gutentag, as part of the complexity of playing nicely with ActiveRecord's dirty state logic. If you want to give the latest commit a spin to confirm it works for you, that'd be appreciated - but either way, I'll try to get a new gem release out soon.

Also, with regards to the auto-populating of tag names when an instance is loaded - I wasn't able to reproduce that, so, as you suggest, I think it might be something in your app.

@pat
Copy link
Owner

pat commented Nov 5, 2022

Just released v2.6.2 with this fix :)

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