CacheBuster refactoring (#4766)

* Turn CacheBuster into a module

This class used no internal state, so repeatedly creating short-lived objects seems wasteful.

* Consistently use string interpolation and parenthesis

* Destructure arrays into meaningful names, formatting

* Fix request spec for internal classified listings controller

Interestingly this works when asserting directly on the module, but not on a double.
Asserting directly in the module seems sufficient for this test so the indirection
was removed.

* Turn CacheBuster into a module

This class used no internal state, so repeatedly creating short-lived objects seems wasteful.

* Fix specs after rebasing
This commit is contained in:
Michael Kohl 2019-11-13 22:51:23 +07:00 committed by Ben Halpern
parent 61788e5dec
commit b57ced5466
40 changed files with 101 additions and 116 deletions

View file

@ -38,7 +38,7 @@ class Internal::ArticlesController < Internal::ApplicationController
article.user_id = article_params[:user_id].to_i
article.update!(article_params)
Article.where.not(id: article.id).where(live_now: true).update_all(live_now: false) if article.live_now
CacheBuster.new.bust "/live_articles"
CacheBuster.bust("/live_articles")
render body: nil
end

View file

@ -27,7 +27,7 @@ module Internal
@event = Event.find(params[:id])
@events = Event.order("starts_at DESC")
if @event.update(event_params)
CacheBuster.new.bust "/live_articles"
CacheBuster.bust("/live_articles")
flash[:success] = "#{@event.title} was successfully updated"
redirect_to "/internal/events"
else

View file

@ -35,15 +35,14 @@ class Internal::ToolsController < Internal::ApplicationController
def handle_article_cache
article = Article.find(params[:bust_article].to_i)
article.touch(:last_commented_at)
CacheBuster.new.bust_article(article)
CacheBuster.bust_article(article)
end
def bust_link(link)
cb = CacheBuster.new
link.sub!("https://dev.to", "") if link.starts_with?("https://dev.to")
cb.bust(link)
cb.bust(link + "/")
cb.bust(link + "?i=i")
cb.bust(link + "/?i=i")
CacheBuster.bust(link)
CacheBuster.bust("#{link}/")
CacheBuster.bust("#{link}?i=i")
CacheBuster.bust("#{link}/?i=i")
end
end

View file

@ -31,7 +31,7 @@ class ProfilePinsController < ApplicationController
end
def bust_user_profile
CacheBuster.new.bust(current_user.path)
CacheBuster.new.bust(current_user.path + "?i=i")
CacheBuster.bust(current_user.path)
CacheBuster.bust("#{current_user.path}?i=i")
end
end

View file

@ -2,7 +2,7 @@ module Articles
class BustCacheJob < ApplicationJob
queue_as :articles_bust_cache
def perform(article_id, cache_buster = CacheBuster.new)
def perform(article_id, cache_buster = CacheBuster)
article = Article.find_by(id: article_id)
cache_buster.bust_article(article) if article

View file

@ -2,10 +2,10 @@ module Articles
class BustMultipleCachesJob < ApplicationJob
queue_as :articles_bust_multiple_caches
def perform(article_ids, cache_buster = CacheBuster.new)
def perform(article_ids, cache_buster = CacheBuster)
Article.select(:id, :path).where(id: article_ids).find_each do |article|
cache_buster.bust(article.path)
cache_buster.bust(article.path + "?i=i")
cache_buster.bust("#{article.path}?i=i")
end
end
end

View file

@ -2,7 +2,7 @@ module ClassifiedListings
class BustCacheJob < ApplicationJob
queue_as :classified_listings_bust_cache
def perform(classified_listing_id, cache_buster = CacheBuster.new)
def perform(classified_listing_id, cache_buster = CacheBuster)
classified_listing = ClassifiedListing.find_by(id: classified_listing_id)
return unless classified_listing

View file

@ -2,7 +2,7 @@ module Events
class BustCacheJob < ApplicationJob
queue_as :events_bust_cache
def perform(cache_buster = CacheBuster.new)
def perform(cache_buster = CacheBuster)
cache_buster.bust_events
end
end

View file

@ -2,7 +2,7 @@ module Organizations
class BustCacheJob < ApplicationJob
queue_as :organizations_bust_cache
def perform(organization_id, slug, cache_buster = CacheBuster.new)
def perform(organization_id, slug, cache_buster = CacheBuster)
organization = Organization.find_by(id: organization_id)
return unless organization

View file

@ -2,7 +2,7 @@ module Pages
class BustCacheJob < ApplicationJob
queue_as :pages_bust_cache
def perform(slug, cache_buster = CacheBuster.new)
def perform(slug, cache_buster = CacheBuster)
cache_buster.bust_page(slug)
end
end

View file

@ -2,7 +2,7 @@ module PodcastEpisodes
class BustCacheJob < ApplicationJob
queue_as :podcast_episodes_bust_cache
def perform(podcast_episode_id, path, podcast_slug, cache_buster = CacheBuster.new)
def perform(podcast_episode_id, path, podcast_slug, cache_buster = CacheBuster)
podcast_episode = PodcastEpisode.find_by(id: podcast_episode_id)
return unless podcast_episode

View file

@ -2,7 +2,7 @@ module Podcasts
class BustCacheJob < ApplicationJob
queue_as :podcasts_bust_cache
def perform(path, cache_buster = CacheBuster.new)
def perform(path, cache_buster = CacheBuster)
cache_buster.bust_podcast(path)
end
end

View file

@ -2,7 +2,7 @@ module Reactions
class BustHomepageCacheJob < ApplicationJob
queue_as :bust_homepage_cache_from_reactions
def perform(reaction_id, cache_buster = CacheBuster.new)
def perform(reaction_id, cache_buster = CacheBuster)
reaction = Reaction.find_by(id: reaction_id, reactable_type: "Article")
return unless reaction&.reactable
@ -10,10 +10,10 @@ module Reactions
return unless featured_articles_ids.include?(reaction.reactable_id)
reaction.reactable.touch
cache_buster.bust "/"
cache_buster.bust "/"
cache_buster.bust "/?i=i"
cache_buster.bust "?i=i"
cache_buster.bust("/")
cache_buster.bust("/")
cache_buster.bust("/?i=i")
cache_buster.bust("?i=i")
end
end
end

View file

@ -2,7 +2,7 @@ module Reactions
class BustReactableCacheJob < ApplicationJob
queue_as :bust_reactable_cache
def perform(reaction_id, cache_buster = CacheBuster.new)
def perform(reaction_id, cache_buster = CacheBuster)
reaction = Reaction.find_by(id: reaction_id)
return unless reaction&.reactable
@ -11,7 +11,7 @@ module Reactions
cache_buster.bust "/reactions?article_id=#{reaction.reactable_id}"
elsif reaction.reactable_type == "Comment"
path = "/reactions?commentable_id=#{reaction.reactable.commentable_id}&commentable_type=#{reaction.reactable.commentable_type}"
cache_buster.bust path
cache_buster.bust(path)
end
end
end

View file

@ -2,7 +2,7 @@ module Tags
class BustCacheJob < ApplicationJob
queue_as :tags_bust_cache
def perform(name, cache_buster = CacheBuster.new)
def perform(name, cache_buster = CacheBuster)
cache_buster.bust_tag(name)
end
end

View file

@ -2,7 +2,7 @@ module Users
class BustCacheJob < ApplicationJob
queue_as :users_bust_cache
def perform(user_id, cache_buster = CacheBuster.new)
def perform(user_id, cache_buster = CacheBuster)
user = User.find_by(id: user_id)
return unless user

View file

@ -1,9 +1,12 @@
class CacheBuster
module CacheBuster
TIMEFRAMES = [
[1.week.ago, "week"], [1.month.ago, "month"], [1.year.ago, "year"], [5.years.ago, "infinity"]
[1.week.ago, "week"],
[1.month.ago, "month"],
[1.year.ago, "year"],
[5.years.ago, "infinity"],
].freeze
def bust(path)
def self.bust(path)
return unless Rails.env.production?
HTTParty.post("https://api.fastly.com/purge/https://dev.to#{path}",
@ -14,7 +17,7 @@ class CacheBuster
Rails.logger.error("Trying to bust cache of an invalid uri: #{e}")
end
def bust_comment(commentable)
def self.bust_comment(commentable)
return unless commentable
bust_article_comment(commentable) if commentable.is_a?(Article)
@ -24,20 +27,20 @@ class CacheBuster
bust(commentable.path.to_s)
commentable.comments.includes(:user).find_each do |comment|
bust(comment.path)
bust(comment.path + "?i=i")
bust("#{comment.path}?i=i")
end
bust("#{commentable.path}/comments/*")
end
def bust_article(article)
bust("/" + article.user.username)
def self.bust_article(article)
bust(article.path)
bust(article.path + "/")
bust(article.path + "?i=i")
bust(article.path + "/?i=i")
bust(article.path + "/comments")
bust(article.path + "?preview=" + article.password)
bust(article.path + "?preview=" + article.password + "&i=i")
bust("/#{article.user.username}")
bust("#{article.path}/")
bust("#{article.path}?i=i")
bust("#{article.path}/?i=i")
bust("#{article.path}/comments")
bust("#{article.path}?preview=#{article.password}")
bust("#{article.path}?preview=#{article.password}&i=i")
bust("/#{article.organization.slug}") if article.organization.present?
bust_home_pages(article)
bust_tag_pages(article)
@ -49,21 +52,21 @@ class CacheBuster
end
end
def bust_home_pages(article)
def self.bust_home_pages(article)
if article.featured_number.to_i > Time.current.to_i
bust("/")
bust("?i=i")
end
if article.video.present? && article.featured_number.to_i > 10.days.ago.to_i
CacheBuster.new.bust "/videos"
CacheBuster.new.bust "/videos?i=i"
bust("/videos")
bust("/videos?i=i")
end
TIMEFRAMES.each do |timeframe|
if Article.published.where("published_at > ?", timeframe[0]).
TIMEFRAMES.each do |timestamp, interval|
if Article.published.where("published_at > ?", timestamp).
order("positive_reactions_count DESC").limit(3).pluck(:id).include?(article.id)
bust("/top/#{timeframe[1]}")
bust("/top/#{timeframe[1]}?i=i")
bust("/top/#{timeframe[1]}/?i=i")
bust("/top/#{interval}")
bust("/top/#{interval}?i=i")
bust("/top/#{interval}/?i=i")
end
end
if article.published && article.published_at > 1.hour.ago
@ -73,7 +76,7 @@ class CacheBuster
bust("/") if Article.published.order("hotness_score DESC").limit(4).pluck(:id).include?(article.id)
end
def bust_tag_pages(article)
def self.bust_tag_pages(article)
return unless article.published
article.tag_list.each do |tag|
@ -81,12 +84,12 @@ class CacheBuster
bust("/t/#{tag}/latest")
bust("/t/#{tag}/latest?i=i")
end
TIMEFRAMES.each do |timeframe|
if Article.published.where("published_at > ?", timeframe[0]).tagged_with(tag).
TIMEFRAMES.each do |timestamp, interval|
if Article.published.where("published_at > ?", timestamp).tagged_with(tag).
order("positive_reactions_count DESC").limit(3).pluck(:id).include?(article.id)
bust("/top/#{timeframe[1]}")
bust("/top/#{timeframe[1]}?i=i")
bust("/top/#{timeframe[1]}/?i=i")
bust("/top/#{interval}")
bust("/top/#{interval}?i=i")
bust("/top/#{interval}/?i=i")
12.times do |i|
bust("/api/articles?tag=#{tag}&top=#{i}")
end
@ -101,14 +104,14 @@ class CacheBuster
end
end
def bust_page(slug)
def self.bust_page(slug)
bust "/page/#{slug}"
bust "/page/#{slug}?i=i"
bust "/#{slug}"
bust "/#{slug}?i=i"
end
def bust_tag(name)
def self.bust_tag(name)
bust("/t/#{name}")
bust("/t/#{name}?i=i")
bust("/t/#{name}/?i=i")
@ -116,16 +119,16 @@ class CacheBuster
bust("/tags")
end
def bust_events
def self.bust_events
bust("/events")
bust("/events?i=i")
end
def bust_podcast(path)
def self.bust_podcast(path)
bust("/" + path)
end
def bust_organization(organization, slug)
def self.bust_organization(organization, slug)
bust("/#{slug}")
begin
organization.articles.find_each do |article|
@ -136,7 +139,7 @@ class CacheBuster
end
end
def bust_podcast_episode(podcast_episode, path, podcast_slug)
def self.bust_podcast_episode(podcast_episode, path, podcast_slug)
podcast_episode.purge
podcast_episode.purge_all
begin
@ -151,7 +154,7 @@ class CacheBuster
podcast_episode.purge_all
end
def bust_classified_listings(classified_listing)
def self.bust_classified_listings(classified_listing)
bust("/listings")
bust("/listings?i=i")
bust("/listings/#{classified_listing.category}/#{classified_listing.slug}")
@ -159,7 +162,7 @@ class CacheBuster
bust("/listings/#{classified_listing.category}")
end
def bust_user(user)
def self.bust_user(user)
username = user.username
paths = [
"/#{username}", "/#{username}?i=i",
@ -172,7 +175,7 @@ class CacheBuster
end
# bust commentable if it's an article
def bust_article_comment(commentable)
def self.bust_article_comment(commentable)
bust("/") if Article.published.order("hotness_score DESC").limit(3).pluck(:id).include?(commentable.id)
if commentable.decorate.cached_tag_list_array.include?("discuss") &&
commentable.featured_number.to_i > 35.hours.ago.to_i

View file

@ -618,10 +618,9 @@ class Article < ApplicationRecord
def bust_cache
return unless Rails.env.production?
cache_buster = CacheBuster.new
cache_buster.bust(path)
cache_buster.bust(path + "?i=i")
cache_buster.bust(path + "?preview=" + password)
CacheBuster.bust(path)
CacheBuster.bust("#{path}?i=i")
CacheBuster.bust("#{path}?preview=#{password}")
async_bust
end

View file

@ -23,8 +23,7 @@ class Badge < ApplicationRecord
end
def bust_path
cache_buster = CacheBuster.new
cache_buster.bust path
cache_buster.bust path + "?i=i"
CacheBuster.bust(path)
CacheBuster.bust("#{path}?i=i")
end
end

View file

@ -279,8 +279,7 @@ class Comment < ApplicationRecord
def synchronous_bust
commentable.touch(:last_comment_at) if commentable.respond_to?(:last_comment_at)
user.touch(:last_comment_at)
cache_buster = CacheBuster.new
cache_buster.bust(commentable.path.to_s) if commentable
CacheBuster.bust(commentable.path.to_s) if commentable
expire_root_fragment
end

View file

@ -47,9 +47,8 @@ class GithubRepo < ApplicationRecord
return if user.blank?
user.touch
cache_buster = CacheBuster.new
cache_buster.bust user.path
cache_buster.bust user.path + "?i=i"
cache_buster.bust user.path + "/?i=i"
CacheBuster.bust(user.path)
CacheBuster.bust("#{user.path}?i=i")
CacheBuster.bust("#{user.path}/?i=i")
end
end

View file

@ -79,7 +79,7 @@ class Reaction < ApplicationRecord
private
def cache_buster
@cache_buster ||= CacheBuster.new
CacheBuster
end
def touch_user

View file

@ -408,11 +408,10 @@ class User < ApplicationRecord
end
def resave_articles
cache_buster = CacheBuster.new
articles.find_each do |article|
if article.path
cache_buster.bust(article.path)
cache_buster.bust(article.path + "?i=i")
CacheBuster.bust(article.path)
CacheBuster.bust("#{article.path}?i=i")
end
article.save
end

View file

@ -1,7 +1,7 @@
module EdgeCache
module Commentable
class Bust
def initialize(commentable, cache_buster = CacheBuster.new)
def initialize(commentable, cache_buster = CacheBuster)
@commentable = commentable
@cache_buster = cache_buster
end

View file

@ -32,7 +32,7 @@ module Moderator
new_username = "spam_#{rand(1_000_000)}"
end
user.update_columns(name: new_name, username: new_username, old_username: user.username, profile_updated_at: Time.current)
CacheBuster.new.bust("/#{user.old_username}")
CacheBuster.bust("/#{user.old_username}")
end
def remove_profile_info

View file

@ -21,7 +21,7 @@ module Moderator
reassign_articles
reassign_comments
delete_non_content_activity_and_user
CacheBuster.new.bust("/ghost")
CacheBuster.bust("/ghost")
end
private
@ -29,7 +29,7 @@ module Moderator
def delete_non_content_activity_and_user
delete_user_activity
user.unsubscribe_from_newsletters
CacheBuster.new.bust("/#{user.username}")
CacheBuster.bust("/#{user.username}")
user.delete
end

View file

@ -25,7 +25,7 @@ module Moderator
@delete_user.delete
@keep_user.touch(:profile_updated_at)
CacheBuster.new.bust "/#{@keep_user.username}"
CacheBuster.bust("/#{@keep_user.username}")
end
private

View file

@ -9,7 +9,7 @@ module Users
delete_articles
delete_user_activity
user.unsubscribe_from_newsletters
CacheBuster.new.bust("/#{user.username}")
CacheBuster.bust("/#{user.username}")
user.delete
end

View file

@ -2,7 +2,7 @@ module Users
module DeleteArticles
module_function
def call(user, cache_buster = CacheBuster.new)
def call(user, cache_buster = CacheBuster)
return unless user.articles.any?
virtual_articles = user.articles.map { |article| Article.new(article.attributes) }

View file

@ -2,7 +2,7 @@ module Users
module DeleteComments
module_function
def call(user, cache_buster = CacheBuster.new)
def call(user, cache_buster = CacheBuster)
return unless user.comments.any?
user.comments.find_each do |comment|

View file

@ -7,14 +7,13 @@ task get_podcast_episodes: :environment do
end
task periodic_cache_bust: :environment do
cache_buster = CacheBuster.new
cache_buster.bust("/feed.xml")
cache_buster.bust("/badge")
cache_buster.bust("/shecoded")
CacheBuster.bust("/feed.xml")
CacheBuster.bust("/badge")
CacheBuster.bust("/shecoded")
end
task hourly_bust: :environment do
CacheBuster.new.bust("/")
CacheBuster.bust("/")
end
task fetch_all_rss: :environment do

View file

@ -1,10 +1,9 @@
require "rails_helper"
RSpec.describe Events::BustCacheJob do
let(:cache_buster) { instance_double(CacheBuster) }
let(:cache_buster) { class_double(CacheBuster) }
before do
allow(CacheBuster).to receive(:new).and_return(cache_buster)
allow(cache_buster).to receive(:bust_events)
end

View file

@ -5,10 +5,9 @@ RSpec.describe Organizations::BustCacheJob, type: :job do
describe "#perform_now" do
let!(:organization) { FactoryBot.create(:organization) }
let(:cache_buster) { instance_double(CacheBuster) }
let(:cache_buster) { class_double(CacheBuster) }
before do
allow(CacheBuster).to receive(:new).and_return(cache_buster)
allow(cache_buster).to receive(:bust_organization)
end

View file

@ -1,10 +1,9 @@
require "rails_helper"
RSpec.describe Pages::BustCacheJob do
let(:cache_buster) { instance_double(CacheBuster) }
let(:cache_buster) { class_double(CacheBuster) }
before do
allow(CacheBuster).to receive(:new).and_return(cache_buster)
allow(cache_buster).to receive(:bust_page)
end

View file

@ -6,10 +6,9 @@ RSpec.describe PodcastEpisodes::BustCacheJob do
describe "#perform_now" do
let!(:podcast) { create(:podcast) }
let!(:podcast_episode) { FactoryBot.create(:podcast_episode, podcast_id: podcast.id) }
let(:cache_buster) { instance_double(CacheBuster) }
let(:cache_buster) { class_double(CacheBuster) }
before do
allow(CacheBuster).to receive(:new).and_return(cache_buster)
allow(cache_buster).to receive(:bust_podcast_episode)
end

View file

@ -1,10 +1,9 @@
require "rails_helper"
RSpec.describe Podcasts::BustCacheJob do
let(:cache_buster) { instance_double(CacheBuster) }
let(:cache_buster) { class_double(CacheBuster) }
before do
allow(CacheBuster).to receive(:new).and_return(cache_buster)
allow(cache_buster).to receive(:bust_podcast)
end

View file

@ -1,10 +1,9 @@
require "rails_helper"
RSpec.describe Tags::BustCacheJob do
let(:cache_buster) { instance_double(CacheBuster) }
let(:cache_buster) { class_double(CacheBuster) }
before do
allow(CacheBuster).to receive(:new).and_return(cache_buster)
allow(cache_buster).to receive(:bust_tag)
end

View file

@ -1,7 +1,7 @@
require "rails_helper"
RSpec.describe CacheBuster do
let(:cache_buster) { described_class.new }
let(:cache_buster) { described_class }
let(:user) { create(:user) }
let(:article) { create(:article, user_id: user.id) }
let(:comment) { create(:comment, user_id: user.id, commentable_id: article.id) }
@ -79,7 +79,6 @@ RSpec.describe CacheBuster do
it "logs an error from bust_podcast_episode" do
allow(Rails.logger).to receive(:warn)
allow(described_class).to receive(:new).and_return(cache_buster)
allow(cache_buster).to receive(:bust).and_raise(StandardError)
cache_buster.bust_podcast_episode(podcast_episode, 12, "-007")
expect(Rails.logger).to have_received(:warn).once

View file

@ -4,11 +4,9 @@ RSpec.describe "/internal/listings", type: :request do
describe "PUT /internal/listings/:id" do
let(:admin) { create(:user, :super_admin) }
let(:classified_listing) { create(:classified_listing, user_id: admin.id) }
let(:cache_buster) { instance_double(CacheBuster) }
before do
allow(CacheBuster).to receive(:new).and_return(cache_buster)
allow(cache_buster).to receive(:bust_classified_listings)
allow(CacheBuster).to receive(:bust_classified_listings)
sign_in admin
end
@ -16,7 +14,7 @@ RSpec.describe "/internal/listings", type: :request do
put "/internal/listings/#{classified_listing.id}", params: {
classified_listing: { title: "updated" }
}
expect(cache_buster).to have_received(:bust_classified_listings)
expect(CacheBuster).to have_received(:bust_classified_listings)
end
end
end

View file

@ -9,11 +9,9 @@ RSpec.describe Users::Delete, type: :service do
end
it "busts user profile page" do
buster = double
allow(buster).to receive(:bust)
allow(CacheBuster).to receive(:new).and_return(buster)
allow(CacheBuster).to receive(:bust)
described_class.new(user).call
expect(buster).to have_received(:bust).with("/#{user.username}")
expect(CacheBuster).to have_received(:bust).with("/#{user.username}")
end
it "deletes user's follows" do