jaculabilis/danbooru
1
0
Fork 0
Code Issues Pull Requests Projects Releases Wiki Activity

users: drop id_to_name, name_to_id caching.

Browse Source 
Changes:

* Drop Users.id_to_name.
* Don't cache Users.name_to_id.
* Replace calls to name_to_id with find_by_name when possible.
* Don't autodefine creator_name in belongs_to_creator.
* Don't autodefine updater_name in belongs_to_updater.
* Instead manually define creator_name / updater_name only on models that need
  to return these fields in the api.

id_to_name was cached to reduce the impact of N+1 query patterns in
certain places, especially in api responses that return creator_name /
updater_name fields. But it still meant we were doing N calls to
memcache. Using `includes` to prefetch users avoids this N+1 pattern.

name_to_id had no need be cached, it was never used in any performance-
sensitive contexts.

Avoiding caching also avoids the need to keep these caches consistent.
This commit is contained in:
evazion
2019-08-18 03:50:43 -05:00
parent 7871dced00
commit 59b277ead1
39 changed files with 70 additions and 91 deletions
Download Patch File Download Diff File Expand all files Collapse all files

2
app/controllers/artist_versions_controller.rb
View File

@@ -2,7 +2,7 @@ class ArtistVersionsController < ApplicationController
respond_to :html, :xml, :json
def index
@artist_versions = ArtistVersion.search(search_params).paginate(params[:page], :limit => params[:limit], :search_count => params[:search])
@artist_versions = ArtistVersion.includes(:updater).search(search_params).paginate(params[:page], :limit => params[:limit], :search_count => params[:search])
respond_with(@artist_versions) do |format|
format.xml do
render :xml => @artist_versions.to_xml(:root => "artist-versions")

2
app/controllers/comments_controller.rb
View File

@@ -83,7 +83,7 @@ private
end
def index_by_comment
@comments = Comment.search(search_params).paginate(params[:page], :limit => params[:limit], :search_count => params[:search])
@comments = Comment.includes(:creator, :updater).search(search_params).paginate(params[:page], :limit => params[:limit], :search_count => params[:search])
respond_with(@comments) do |format|
format.atom do
@comments = @comments.includes(:post, :creator).load

2
app/controllers/notes_controller.rb
View File

@@ -6,7 +6,7 @@ class NotesController < ApplicationController
end
def index
@notes = Note.search(search_params).paginate(params[:page], :limit => params[:limit], :search_count => params[:search])
@notes = Note.includes(:creator).search(search_params).paginate(params[:page], :limit => params[:limit], :search_count => params[:search])
respond_with(@notes) do |format|
format.html { @notes = @notes.includes(:creator) }
format.xml do

2
app/controllers/pools_controller.rb
View File

@@ -17,7 +17,7 @@ class PoolsController < ApplicationController
end
def index
@pools = Pool.search(search_params).paginate(params[:page], :limit => params[:limit], :search_count => params[:search])
@pools = Pool.includes(:creator).search(search_params).paginate(params[:page], :limit => params[:limit], :search_count => params[:search])
respond_with(@pools) do |format|
format.xml do
render :xml => @pools.to_xml(:root => "pools")

2
app/controllers/wiki_pages_controller.rb
View File

@@ -15,7 +15,7 @@ class WikiPagesController < ApplicationController
end
def index
@wiki_pages = WikiPage.search(search_params).paginate(params[:page], :limit => params[:limit], :search_count => params[:search])
@wiki_pages = WikiPage.includes(:creator).search(search_params).paginate(params[:page], :limit => params[:limit], :search_count => params[:search])
respond_with(@wiki_pages) do |format|
format.html do
if params[:page].nil? || params[:page].to_i == 1

4
app/logical/post_sets/post.rb
View File

@@ -135,6 +135,10 @@ module PostSets
else
temp = ::Post.tag_match(tag_string).where("true /* PostSets::Post#posts:2 */").paginate(page, :count => post_count, :limit => per_page)
end
# XXX HACK: uploader_name is needed in api responses and in data-uploader attribs (visible to mods only).
temp = temp.includes(:uploader) if format.to_sym != :html || CurrentUser.is_moderator?
temp.each # hack to force rails to eager load
temp
end

8
app/models/application_record.rb
View File

@@ -274,10 +274,6 @@ class ApplicationRecord < ActiveRecord::Base
rec.creator_ip_addr = CurrentUser.ip_addr if rec.respond_to?(:creator_ip_addr=)
end
end
define_method :creator_name do
User.id_to_name(creator_id)
end
end
end
@@ -288,10 +284,6 @@ class ApplicationRecord < ActiveRecord::Base
rec.updater_id = CurrentUser.id
rec.updater_ip_addr = CurrentUser.ip_addr if rec.respond_to?(:updater_ip_addr=)
end
define_method :updater_name do
User.id_to_name(updater_id)
end
end
end
end

2
app/models/ban.rb
View File

@@ -101,7 +101,7 @@ class Ban < ApplicationRecord
end
def user_name=(username)
self.user_id = User.name_to_id(username)
self.user = User.find_by_name(username)
end
def duration=(dur)

14
app/models/comment.rb
View File

@@ -17,8 +17,8 @@ class Comment < ApplicationRecord
end
mentionable(
:message_field => :body,
:title => ->(user_name) {"#{creator_name} mentioned you in a comment on post ##{post_id}"},
:body => ->(user_name) {"@#{creator_name} mentioned you in a \"comment\":/posts/#{post_id}#comment-#{id} on post ##{post_id}:\n\n[quote]\n#{DText.excerpt(body, "@"+user_name)}\n[/quote]\n"},
:title => ->(user_name) {"#{creator.name} mentioned you in a comment on post ##{post_id}"},
:body => ->(user_name) {"@#{creator.name} mentioned you in a \"comment\":/posts/#{post_id}#comment-#{id} on post ##{post_id}:\n\n[quote]\n#{DText.excerpt(body, "@"+user_name)}\n[/quote]\n"},
)
module SearchMethods
@@ -183,6 +183,14 @@ class Comment < ApplicationRecord
super + [:creator_name, :updater_name]
end
def creator_name
creator.name
end
def updater_name
updater.name
end
def delete!
update(is_deleted: true)
end
@@ -192,6 +200,6 @@ class Comment < ApplicationRecord
end
def quoted_response
DText.quote(body, creator_name)
DText.quote(body, creator.name)
end
end

2
app/models/dmail.rb
View File

@@ -49,7 +49,7 @@ class Dmail < ApplicationRecord
module AddressMethods
def to_name=(name)
self.to_id = User.name_to_id(name)
self.to = User.find_by_name(name)
end
def initialize_attributes

14
app/models/forum_post.rb
View File

@@ -26,8 +26,8 @@ class ForumPost < ApplicationRecord
end
mentionable(
:message_field => :body,
:title => ->(user_name) {%{#{creator_name} mentioned you in topic ##{topic_id} (#{topic.title})}},
:body => ->(user_name) {%{@#{creator_name} mentioned you in topic ##{topic_id} ("#{topic.title}":[/forum_topics/#{topic_id}?page=#{forum_topic_page}]):\n\n[quote]\n#{DText.excerpt(body, "@"+user_name)}\n[/quote]\n}},
:title => ->(user_name) {%{#{creator.name} mentioned you in topic ##{topic_id} (#{topic.title})}},
:body => ->(user_name) {%{@#{creator.name} mentioned you in topic ##{topic_id} ("#{topic.title}":[/forum_topics/#{topic_id}?page=#{forum_topic_page}]):\n\n[quote]\n#{DText.excerpt(body, "@"+user_name)}\n[/quote]\n}},
)
module SearchMethods
@@ -209,16 +209,8 @@ class ForumPost < ApplicationRecord
self.is_deleted = false if is_deleted.nil?
end
def creator_name
User.id_to_name(creator_id)
end
def updater_name
User.id_to_name(updater_id)
end
def quoted_response
DText.quote(body, creator_name)
DText.quote(body, creator.name)
end
def forum_topic_page

2
app/models/janitor_trial.rb
View File

@@ -30,7 +30,7 @@ class JanitorTrial < ApplicationRecord
end
def user_name=(name)
self.user_id = User.name_to_id(name)
self.user = User.find_by_name(name)
end
def send_dmail

4
app/models/note.rb
View File

@@ -64,6 +64,10 @@ class Note < ApplicationRecord
end
end
def creator_name
creator.name
end
extend SearchMethods
include ApiMethods

4
app/models/pool.rb
View File

@@ -289,6 +289,10 @@ class Pool < ApplicationRecord
super + [:creator_name, :post_count]
end
def creator_name
creator.name
end
def update_category_pseudo_tags_for_posts_async
if saved_change_to_category?
UpdatePoolPseudoTagsJob.perform_later(self)

4
app/models/pool_archive.rb
View File

@@ -95,10 +95,6 @@ class PoolArchive < ApplicationRecord
User.find(updater_id)
end
def updater_name
User.id_to_name(updater_id)
end
def pretty_name
name.tr("_", " ")
end

4
app/models/post.rb
View File

@@ -1015,7 +1015,7 @@ class Post < ApplicationRecord
end
def uploader_name
User.id_to_name(uploader_id)
uploader.name
end
end
@@ -1507,7 +1507,7 @@ class Post < ApplicationRecord
"created_at" => created_at.to_formatted_s(:db),
"has_notes" => has_notes?,
"rating" => rating,
"author" => uploader_name,
"author" => uploader.name,
"creator_id" => uploader_id,
"width" => image_width,
"source" => source,

4
app/models/post_archive.rb
View File

@@ -256,6 +256,10 @@ class PostArchive < ApplicationRecord
post.save!
end
def updater_name
updater.name
end
def method_attributes
super + [:obsolete_added_tags, :obsolete_removed_tags, :unchanged_tags, :updater_name]
end

2
app/models/post_version.rb
View File

@@ -156,7 +156,7 @@ class PostVersion < ApplicationRecord
end
def updater_name
User.id_to_name(updater_id)
updater.name
end
def method_attributes

2
app/models/upload.rb
View File

@@ -155,7 +155,7 @@ class Upload < ApplicationRecord
module UploaderMethods
def uploader_name
User.id_to_name(uploader_id)
uploader.name
end
end

17
app/models/user.rb
View File

@@ -21,7 +21,6 @@ class User < ApplicationRecord
:approver,
:voter,
:super_voter,
:verified,
]
# candidates for removal:
@@ -84,7 +83,6 @@ class User < ApplicationRecord
before_validation :normalize_email
before_create :encrypt_password_on_create
before_update :encrypt_password_on_update
after_save :update_cache
before_create :promote_to_admin_if_first_user
before_create :customize_new_user
has_many :feedback, :class_name => "UserFeedback", :dependent => :destroy
@@ -131,15 +129,7 @@ class User < ApplicationRecord
concerning :NameMethods do
class_methods do
def name_to_id(name)
Cache.get("uni:#{Cache.hash(name)}", 4.hours) do
find_by_name(name).try(:id)
end
end
def id_to_name(user_id)
Cache.get("uin:#{user_id}", 4.hours) do
find_by_id(user_id).try(:name) || Danbooru.config.default_guest_name
end
find_by_name(name).try(:id)
end
# XXX downcasing is the wrong way to do case-insensitive comparison for unicode (should use casefolding).
@@ -155,11 +145,6 @@ class User < ApplicationRecord
def pretty_name
name.gsub(/([^_])_+(?=[^_])/, "\\1 \\2")
end
def update_cache
Cache.put("uin:#{id}", name, 4.hours)
Cache.put("uni:#{Cache.hash(name)}", id, 4.hours)
end
end
module PasswordMethods

14
app/models/user_feedback.rb
View File

@@ -9,10 +9,10 @@ class UserFeedback < ApplicationRecord
validate :user_is_not_creator
after_create :create_dmail, unless: :disable_dmail_notification
after_update(:if => ->(rec) { CurrentUser.id != rec.creator_id}) do |rec|
ModAction.log(%{#{CurrentUser.name} updated user feedback for "#{rec.user_name}":/users/#{rec.user_id}},:user_feedback_update)
ModAction.log(%{#{CurrentUser.name} updated user feedback for "#{rec.user.name}":/users/#{rec.user_id}},:user_feedback_update)
end
after_destroy(:if => ->(rec) { CurrentUser.id != rec.creator_id}) do |rec|
ModAction.log(%{#{CurrentUser.name} deleted user feedback for "#{rec.user_name}":/users/#{rec.user_id}},:user_feedback_delete)
ModAction.log(%{#{CurrentUser.name} deleted user feedback for "#{rec.user.name}":/users/#{rec.user_id}},:user_feedback_delete)
end
module SearchMethods
@@ -76,12 +76,8 @@ class UserFeedback < ApplicationRecord
extend SearchMethods
def user_name
User.id_to_name(user_id)
end
def user_name=(name)
self.user_id = User.name_to_id(name)
self.user = User.find_by_name(name)
end
def disclaimer
@@ -89,11 +85,11 @@ class UserFeedback < ApplicationRecord
return nil
end
"The purpose of feedback is to help you become a valuable member of the site by highlighting adverse behaviors. The author, #{creator_name}, should have sent you a message in the recent past as a warning. The fact that you're receiving this feedback now implies you've ignored their advice.\n\nYou can protest this feedback by petitioning the mods and admins in the forum. If #{creator_name} fails to provide sufficient evidence, you can have the feedback removed. However, if you fail to defend yourself against the accusations, you will likely earn yourself another negative feedback.\n\nNegative feedback generally doesn't affect your usability of the site. But it does mean other users may trust you less and give you less benefit of the doubt.\n\n"
"The purpose of feedback is to help you become a valuable member of the site by highlighting adverse behaviors. The author, #{creator.name}, should have sent you a message in the recent past as a warning. The fact that you're receiving this feedback now implies you've ignored their advice.\n\nYou can protest this feedback by petitioning the mods and admins in the forum. If #{creator.name} fails to provide sufficient evidence, you can have the feedback removed. However, if you fail to defend yourself against the accusations, you will likely earn yourself another negative feedback.\n\nNegative feedback generally doesn't affect your usability of the site. But it does mean other users may trust you less and give you less benefit of the doubt.\n\n"
end
def create_dmail
body = %{#{disclaimer}@#{creator_name} created a "#{category} record":/user_feedbacks?search[user_id]=#{user_id} for your account:\n\n#{self.body}}
body = %{#{disclaimer}@#{creator.name} created a "#{category} record":/user_feedbacks?search[user_id]=#{user_id} for your account:\n\n#{self.body}}
Dmail.create_automated(:to_id => user_id, :title => "Your user record has been updated", :body => body)
end

4
app/models/wiki_page.rb
View File

@@ -108,6 +108,10 @@ class WikiPage < ApplicationRecord
end
end
def creator_name
creator.name
end
extend SearchMethods
include ApiMethods

2
app/presenters/post_presenter.rb
View File

@@ -117,7 +117,7 @@ class PostPresenter < Presenter
}
if CurrentUser.is_moderator?
attributes["data-uploader"] = post.uploader_name
attributes["data-uploader"] = post.uploader.name
end
if post.visible?

2
app/views/artist_versions/_standard_listing.html.erb
View File

@@ -37,7 +37,7 @@
</td>
<td>
<%= link_to_user artist_version.updater %>
<%= link_to "»", artist_versions_path(search: { updater_name: artist_version.updater_name }) %>
<%= link_to "»", artist_versions_path(search: { updater_name: artist_version.updater.name }) %>
<p>
<%= compact_time(artist_version.updated_at) %>
<% if CurrentUser.is_moderator? %>

4
app/views/comments/index.atom.builder
View File

@@ -9,7 +9,7 @@ atom_feed(root_url: comments_url(host: Danbooru.config.hostname)) do |feed|
@comments.each do |comment|
feed.entry(comment, published: comment.created_at, updated: comment.updated_at) do |entry|
entry.title("@#{comment.creator_name} on post ##{comment.post_id} (#{comment.post.presenter.humanized_essential_tag_string})")
entry.title("@#{comment.creator.name} on post ##{comment.post_id} (#{comment.post.presenter.humanized_essential_tag_string})")
entry.content(<<-EOS.strip_heredoc, type: "html")
<img src="#{comment.post.preview_file_url}"/>
@@ -17,7 +17,7 @@ atom_feed(root_url: comments_url(host: Danbooru.config.hostname)) do |feed|
EOS
entry.author do |author|
author.name(comment.creator_name)
author.name(comment.creator.name)
author.uri(user_url(comment.creator))
end
end

2
app/views/comments/partials/show/_comment.html.erb
View File

@@ -1,6 +1,6 @@
<% if CurrentUser.is_moderator? || (params[:search] && params[:search][:is_deleted] =~ /t/) || !comment.is_deleted? %>
<a name="comment-<%= comment.id %>"></a>
<article class="comment <%= "below-threshold" if comment.below_threshold? %>" data-post-id="<%= comment.post_id %>" data-comment-id="<%= comment.id %>" data-score="<%= comment.score %>" data-creator="<%= comment.creator_name %>" data-is-sticky="<%= comment.is_sticky %>">
<article class="comment <%= "below-threshold" if comment.below_threshold? %>" data-post-id="<%= comment.post_id %>" data-comment-id="<%= comment.id %>" data-score="<%= comment.score %>" data-creator="<%= comment.creator.name %>" data-is-sticky="<%= comment.is_sticky %>">
<div class="author">
<h1>
<%= link_to_user comment.creator %>

2
app/views/posts/index.atom.erb
View File

@@ -29,7 +29,7 @@
</div>
</content>
<author>
<name><%= post.uploader_name %></name>
<name><%= post.uploader.name %></name>
</author>
</entry>
<% end %>

2
app/views/posts/partials/show/_image.html.erb
View File

@@ -1,3 +1,3 @@
<% if post.visible? %>
<%= image_tag(post.file_url_for(CurrentUser.user), :width => post.image_width_for(CurrentUser.user), :height => post.image_height_for(CurrentUser.user), :id => "image", "data-original-width" => post.image_width, "data-original-height" => post.image_height, "data-large-width" => post.large_image_width, "data-large-height" => post.large_image_height, "data-tags" => post.tag_string, :alt => post.presenter.humanized_essential_tag_string, "data-uploader" => post.uploader_name, "data-rating" => post.rating, "data-flags" => post.status_flags, "data-parent-id" => post.parent_id, "data-has-children" => post.has_children?, "data-has-active-children" => post.has_active_children?, "data-score" => post.score, "data-fav-count" => post.fav_count, "itemprop" => "contentUrl") %>
<%= image_tag(post.file_url_for(CurrentUser.user), :width => post.image_width_for(CurrentUser.user), :height => post.image_height_for(CurrentUser.user), :id => "image", "data-original-width" => post.image_width, "data-original-height" => post.image_height, "data-large-width" => post.large_image_width, "data-large-height" => post.large_image_height, "data-tags" => post.tag_string, :alt => post.presenter.humanized_essential_tag_string, "data-uploader" => post.uploader.name, "data-rating" => post.rating, "data-flags" => post.status_flags, "data-parent-id" => post.parent_id, "data-has-children" => post.has_children?, "data-has-active-children" => post.has_active_children?, "data-score" => post.score, "data-fav-count" => post.fav_count, "itemprop" => "contentUrl") %>
<% end %>

2
app/views/posts/partials/show/_ugoira_original.html.erb
View File

@@ -9,7 +9,7 @@
"data-large-width" => post.image_width,
"data-large-height" => post.image_height,
"data-tags" => post.tag_string,
"data-uploader" => post.uploader_name,
"data-uploader" => post.uploader.name,
"data-rating" => post.rating,
"data-flags" => post.status_flags,
"data-parent-id" => post.parent_id,

2
app/views/posts/show.html.erb
View File

@@ -120,7 +120,7 @@
<% if !CurrentUser.user.is_builder? %>
<h2>Before commenting, read the <%= link_to "how to comment guide", wiki_pages_path(:search => {:title => "howto:comment"}) %>.</h2>
<% end %>
<%= render "comments/partials/index/list", :comments => @post.comments.visible(CurrentUser.user), :post => @post, :show_header => false %>
<%= render "comments/partials/index/list", :comments => @post.comments.visible(CurrentUser.user).includes(:creator), :post => @post, :show_header => false %>
</section>
<section id="notes" style="display: none;">

2
app/views/reports/upload_tags.html.erb
View File

@@ -15,7 +15,7 @@
<tbody>
<% @upload_reports.each do |upload_report| %>
<tr>
<td><%= PostPresenter.preview(upload_report.becomes(Post), show_deleted: true, tags: "user:#{upload_report.uploader_name}") %></td>
<td><%= PostPresenter.preview(upload_report.becomes(Post), show_deleted: true, tags: "user:#{upload_report.uploader.name}") %></td>
<td>
<%= TagSetPresenter.new(upload_report.uploader_tags_array).inline_tag_list_html %>
</td>

4
app/views/uploads/index.html.erb
View File

@@ -16,7 +16,7 @@
<% @uploads.each do |upload| %>
<tr>
<td>
<%= PostPresenter.preview(upload.post, tags: "user:#{upload.uploader_name}", show_deleted: true) %>
<%= PostPresenter.preview(upload.post, tags: "user:#{upload.uploader.name}", show_deleted: true) %>
</td>
<td class="col-expand upload-info">
<span class="info">
@@ -62,7 +62,7 @@
</td>
<td>
<%= link_to_user upload.uploader %>
<%= link_to "»", uploads_path(search: params[:search].merge(uploader_name: upload.uploader_name)) %>
<%= link_to "»", uploads_path(search: params[:search].merge(uploader_name: upload.uploader.name)) %>
<br><%= time_ago_in_words_tagged upload.created_at %>
</td>
<td class="col-normal">

2
app/views/user_feedbacks/show.html.erb
View File

@@ -18,5 +18,5 @@
<%= render "secondary_links" %>
<% content_for(:page_title) do %>
Feedback - <%= @user_feedback.user_name %> - <%= Danbooru.config.app_name %>
Feedback - <%= @user_feedback.user.name %> - <%= Danbooru.config.app_name %>
<% end %>

2
app/views/user_mailer/forum_notice.html.erb
View File

@@ -2,7 +2,7 @@
<% @forum_posts.each do |forum_post| %>
<p>
<strong><%= forum_post.creator_name %> said:</strong>
<strong><%= forum_post.creator.name %> said:</strong>
</p>
<%= format_text(forum_post.body, base_url: root_url) %>
<br>

2
test/functional/notes_controller_test.rb
View File

@@ -23,7 +23,7 @@ class NotesControllerTest < ActionDispatch::IntegrationTest
is_active: true,
post_id: @note.post_id,
post_tags_match: @note.post.tag_array.first,
creator_name: @note.creator_name,
creator_name: @note.creator.name,
creator_id: @note.creator_id,
}
}

2
test/functional/uploads_controller_test.rb
View File

@@ -148,7 +148,7 @@ class UploadsControllerTest < ActionDispatch::IntegrationTest
context "with search parameters" do
should "render" do
search_params = {
uploader_name: @upload.uploader_name,
uploader_name: @upload.uploader.name,
source_matches: @upload.source,
rating: @upload.rating,
has_post: "yes",

2
test/unit/comment_test.rb
View File

@@ -288,7 +288,7 @@ class CommentTest < ActiveSupport::TestCase
assert_equal(<<-EOS.strip_heredoc, comment.quoted_response)
[quote]
#{comment.creator_name} said:
#{comment.creator.name} said:
paragraph one

2
test/unit/post_test.rb
View File

@@ -1793,7 +1793,7 @@ class PostTest < ActiveSupport::TestCase
post.uploader_id = user2.id
assert_equal(user2.id, post.uploader_id)
assert_equal(user2.id, post.uploader_id)
assert_equal(user2.name, post.uploader_name)
assert_equal(user2.name, post.uploader.name)
end
context "tag post counts" do

10
test/unit/user_test.rb
View File

@@ -123,10 +123,6 @@ class UserTest < ActiveSupport::TestCase
end
context "name" do
should "be #{Danbooru.config.default_guest_name} given an invalid user id" do
assert_equal(Danbooru.config.default_guest_name, User.id_to_name(-1))
end
should "not contain whitespace" do
# U+2007: https://en.wikipedia.org/wiki/Figure_space
user = FactoryBot.build(:user, :name => "foo\u2007bar")
@@ -152,15 +148,9 @@ class UserTest < ActiveSupport::TestCase
assert_equal(["Name cannot begin or end with an underscore"], user.errors.full_messages)
end
should "be fetched given a user id" do
@user = FactoryBot.create(:user)
assert_equal(@user.name, User.id_to_name(@user.id))
end
should "be updated" do
@user = FactoryBot.create(:user)
@user.update_attribute(:name, "danzig")
assert_equal(@user.name, User.id_to_name(@user.id))
end
end