maverick/docbrown
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

Replace activerecord-import with Rails 6 insert_all and upsert_all (#8407)

Browse source 
* Replace Credits.import with .insert_all

* Fix Notifications::Reactions::Send and replace .import with .upsert to avoid race conditions

* Replace .import! with .upsert_all in Notifications::NotifiableAction::Send

* Replace PodcastEpisode.import! with upsert

* Stop testing gems, test the code

* Remove activerecord-import

* Fix redundancies

* clarify comment

* Replace Credit.import with insert_all

* hello knapsack

* Do not try to insert if there is nothing to
This commit is contained in:
rhymes 2020-06-12 11:16:06 +02:00 • committed by GitHub
parent 8ef9a27a97
commit c356983de8
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
10 changed files with 178 additions and 133 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

1
Gemfile
View file

@ -11,7 +11,6 @@ group :production do
end
gem "active_record_union", "~> 1.3" # Adds proper union and union_all methods to ActiveRecord::Relation
gem "activerecord-import", "~> 1.0" # Adds ability to bulk create activerecord objects
gem "acts-as-taggable-on", "~> 6.5" # A tagging plugin for Rails applications that allows for custom tagging along dynamic contexts
gem "acts_as_follower", github: "thepracticaldev/acts_as_follower", branch: "master" # Allow any model to follow any other model
gem "addressable", "~> 2.7" # A replacement for the URI implementation that is part of Ruby's standard library

3
Gemfile.lock
View file

@ -65,8 +65,6 @@ GEM
activerecord (6.0.3.1)
activemodel (= 6.0.3.1)
activesupport (= 6.0.3.1)
activerecord-import (1.0.5)
activerecord (>= 3.2)
activestorage (6.0.3.1)
actionpack (= 6.0.3.1)
activejob (= 6.0.3.1)
@ -873,7 +871,6 @@ PLATFORMS
DEPENDENCIES
active_record_union (~> 1.3)
activerecord-import (~> 1.0)
acts-as-taggable-on (~> 6.5)
acts_as_follower!
addressable (~> 2.7)

16
app/controllers/credits_controller.rb
View file

@ -26,16 +26,24 @@ class CreditsController < ApplicationController
return unless make_payment
credit_objects = Array.new(@number_to_purchase) do
credits_attributes = Array.new(@number_to_purchase) do
# unfortunately Rails requires the timestamps to be present and doesn't add them automatically
# see <https://github.com/rails/rails/issues/35493>
now = Time.current
attrs = { created_at: now, updated_at: now, cost: cost_per_credit / 100.0 }
if params[:organization_id].present?
@purchaser = Organization.find(params[:organization_id])
Credit.new(organization_id: params[:organization_id], cost: cost_per_credit / 100.0)
attrs[:organization_id] = params[:organization_id]
else
@purchaser = current_user
Credit.new(user_id: current_user.id, cost: cost_per_credit / 100.0)
attrs[:user_id] = current_user.id
end
attrs
end
Credit.import credit_objects
Credit.insert_all(credits_attributes)
@purchaser.credits_count = @purchaser.credits.size
@purchaser.spent_credits_count = @purchaser.credits.spent.size
@purchaser.unspent_credits_count = @purchaser.credits.unspent.size

15
app/models/credit.rb
View file

@ -19,9 +19,20 @@ class Credit < ApplicationRecord
end
def self.add_to(user_or_org, amount)
return unless amount.positive?
now = Time.current
association_id = "#{user_or_org.class.name.underscore}_id"
credits = Array.new(amount) { new(association_id => user_or_org.id) }
import(credits)
attributes = Array.new(amount) do
{
association_id => user_or_org.id,
:created_at => now,
:updated_at => now
}
end
insert_all(attributes)
update_cache_columns(user_or_org)
end

34
app/services/notifications/notifiable_action/send.rb
View file

@ -21,28 +21,32 @@ module Notifications
article: article_data(notifiable)
}
json_data[:organization] = organization_data(notifiable.organization) if notifiable.organization_id
notifications = []
notifications_attributes = []
# followers is an array and not an activerecord object
# followers can occasionally be nil because orphaned follows can possibly exist in the db (for now)
followers.sort_by(&:updated_at).reverse[0..10_000].each do |follower|
notifications.push Notification.new(
now = Time.current
notifications_attributes.push(
user_id: follower.id,
notifiable_id: notifiable.id,
notifiable_type: notifiable.class.name,
action: action,
json_data: json_data,
notified_at: Time.current,
created_at: now,
notified_at: now,
updated_at: now,
)
end
conflict_target = %i[notifiable_id notifiable_type user_id]
conflict_target << :action if action.present?
index_predicate = "action IS#{action.present? ? ' NOT ' : ' '}NULL"
Notification.import! notifications,
on_duplicate_key_update: {
conflict_target: conflict_target,
index_predicate: index_predicate,
columns: %i[json_data notified_at read]
}
return if notifications_attributes.blank?
upsert_index = choose_upsert_index(action)
Notification.upsert_all(
notifications_attributes,
unique_by: upsert_index,
returning: %i[id],
)
end
private
@ -54,6 +58,12 @@ module Notifications
followers += notifiable.organization.followers_scoped.where(subscription_status: "all_articles").map(&:follower) if notifiable.organization_id
followers.uniq.compact
end
def choose_upsert_index(action)
return :index_notifications_on_user_notifiable_and_action_not_null if action.present?
:index_notifications_on_user_notifiable_action_is_null
end
end
end
end

67
app/services/notifications/reactions/send.rb
View file

@ -55,10 +55,7 @@ module Notifications
notification.notified_at = Time.current
notification.read = false if json_data[:reaction][:aggregated_siblings].size > previous_siblings_size
# temporarily returning validations to prevent creating duplicate notifications
# notification_id = save_notification(notification)
notification.save!
notification_id = notification.id
notification_id = save_notification(notification_params, notification)
OpenStruct.new(action: :saved, notification_id: notification_id)
end
@ -68,29 +65,53 @@ module Notifications
attr_reader :reaction, :receiver
# when a notification exists in the db already it's safe to just save
# when it doesn't, there could be a race condition when 2 jobs try to create duplicate notifications concurrently
# in this case upsert is used to rely on postgres constraints and update or insert depending on if the record exists in the db at this point
# currently, activerecord-import upsert is used
# when the app is upgraded to Rails 6 this can be refactored to use rails upsert
def save_notification(notification)
# when a notification exists in the DB already it's safe to just save it,
# when it doesn't, there could be a race condition when 2 jobs try to create
# duplicate notifications concurrently, in this case upsert is used to
# rely on PostgreSQL constraints, thus we use `.upsert`
def save_notification(params, notification)
if notification.persisted?
notification.save!
notification.id
else
# conflict target and index_predicate specify the index to use
conflict_target = %i[notifiable_id notifiable_type]
conflict_target << :action if notification.action?
conflict_target << :user_id if notification.user_id?
conflict_target << :organization_id if notification.organization_id?
index_predicate = "action IS#{notification.action? ? ' NOT ' : ' '}NULL"
import_result = Notification.import! [notification],
on_duplicate_key_update: {
conflict_target: conflict_target,
index_predicate: index_predicate,
columns: %i[json_data notified_at read]
}
import_result.ids.first
# in the upsert, we are only interested in updating the following columns:
# json_data, notified_at, read and the notification params used by `.find_or_initialize_by`
upsert_columns = %w[json_data notified_at read]
upsert_attributes = notification.attributes.select { |k| upsert_columns.include?(k) }
upsert_attributes.merge!(params)
# unfortunately Rails requires the timestamps to be present even if unused in case of upsert
# see <https://github.com/rails/rails/issues/35493>
now = Time.current
upsert_attributes["created_at"] = upsert_attributes["updated_at"] = now
# we also need to select the correct index to let PostgreSQL know
# how to determine conflict on rows
upsert_index = choose_upsert_index(notification)
upsert_result = Notification.upsert(
upsert_attributes,
unique_by: upsert_index,
returning: %i[id],
)
upsert_result.to_a.first["id"]
end
end
def choose_upsert_index(notification)
# if none of these is present, we use the two columns for the upsert
return %i[notifiable_id notifiable_type] unless
notification.action? || notification.user_id? || notification.organization_id?
if notification.action?
return :index_notifications_on_user_notifiable_and_action_not_null if notification.user_id?
:index_notifications_on_org_notifiable_and_action_not_null
else
return :index_notifications_on_user_notifiable_action_is_null if notification.user_id?
:index_notifications_on_org_notifiable_action_is_null
end
end

70
app/services/podcasts/create_episode.rb
View file

@ -10,27 +10,20 @@ module Podcasts
end
def call
ep = PodcastEpisode.new
ep.title = item.title
ep.podcast_id = podcast_id
ep.slug = item.title.parameterize
ep.subtitle = item.itunes_subtitle
ep.summary = item.itunes_summary
ep.website_url = item.link
ep.guid = item.guid
get_media_url(ep) if item.enclosure_url
begin
ep.published_at = item.pubDate.to_date
rescue ArgumentError, NoMethodError => e
Rails.logger.error("not a valid date: #{e}")
end
ep.body = item.body
import_result = PodcastEpisode.import! [ep], on_duplicate_key_update: {
conflict_target: %i[media_url],
columns: %i[title slug subtitle summary website_url published_at reachable media_url https body]
}
episode = PodcastEpisode.find(import_result.ids.first)
attributes = podcast_episode_attributes
attributes = add_media_url(attributes)
attributes = add_published_at(attributes)
upsert_result = PodcastEpisode.upsert(
attributes,
unique_by: :index_podcast_episodes_on_media_url,
returning: %i[id],
)
episode = PodcastEpisode.find(upsert_result.to_a.first["id"])
finalize(episode)
episode
end
@ -38,11 +31,40 @@ module Podcasts
attr_reader :podcast_id, :item
def get_media_url(episode)
def podcast_episode_attributes
now = Time.current
{
podcast_id: podcast_id,
title: item.title,
slug: item.title.parameterize,
subtitle: item.itunes_subtitle,
summary: item.itunes_summary,
website_url: item.link,
guid: item.guid,
body: item.body,
created_at: now,
updated_at: now
}
end
def add_media_url(attributes)
return attributes if item.enclosure_url.blank?
result = GetMediaUrl.call(item.enclosure_url)
episode.reachable = result.reachable
episode.media_url = result.url
episode.https = result.https
attributes.merge(
reachable: result.reachable,
media_url: result.url,
https: result.https,
)
end
def add_published_at(attributes)
attributes.merge(published_at: item.pubDate.to_date)
rescue ArgumentError, NoMethodError => e
Rails.logger.error("not a valid date: #{e}")
attributes
end
def finalize(episode)

102
spec/models/notification_spec.rb
View file

@ -21,79 +21,55 @@ RSpec.describe Notification, type: :model do
end
describe "when trying to create duplicate notifications" do
# Duplicate notifications are not allowed even when validations are skipped
it "doesn't allow to create a duplicate notification via import" do
create(:notification, user: user, notifiable: article, action: "Reaction")
duplicate_notification = build(:notification, user: user, notifiable: article, action: "Reaction")
expect do
described_class.import([duplicate_notification],
on_duplicate_key_update: {
conflict_target: %i[notifiable_id notifiable_type user_id action],
index_predicate: "action IS NOT NULL",
columns: %i[json_data notified_at read]
})
end.not_to change(described_class, :count)
end
describe "when notifiable is an Article" do
it "doesn't allow to create duplicated article reaction notifications for a user" do
reaction = create(:reaction, reactable: article, user: user2)
it "updates when trying to create a duplicate notification via import" do
notification = create(
:notification, user: user, notifiable: article, action: "Reaction", json_data: { "user_id" => 1 }
)
duplicate_notification = build(
:notification, user: user, notifiable: article, action: "Reaction", json_data: { "user_id" => 2 }
)
described_class.import([duplicate_notification],
on_duplicate_key_update: {
conflict_target: %i[notifiable_id notifiable_type user_id action],
index_predicate: "action IS NOT NULL",
columns: %i[json_data notified_at read]
})
notification.reload
expect(notification.json_data["user_id"]).to eq(2)
end
described_class.send_reaction_notification_without_delay(reaction, article.user)
expect(article.user.notifications.count).to eq(1)
it "doesn't allow to create a duplicate organization notification via import" do
create(:notification, organization: organization, notifiable: article, action: "Reaction")
duplicate_notification = build(:notification, organization: organization, notifiable: article, action: "Reaction")
expect do
described_class.import([duplicate_notification],
on_duplicate_key_update: {
conflict_target: %i[notifiable_id notifiable_type organization_id action],
index_predicate: "action IS NOT NULL",
columns: %i[json_data notified_at read]
})
end.not_to change(described_class, :count)
expect do
described_class.send_reaction_notification_without_delay(reaction, article.user)
end.not_to change(article.user.notifications, :count)
end
it "doesn't allow to create duplicated article reaction notifications for an organization" do
article = create(:article, organization: organization)
reaction = create(:reaction, reactable: article, user: user2)
described_class.send_reaction_notification_without_delay(reaction, article.organization)
expect(article.organization.notifications.count).to eq(1)
expect do
described_class.send_reaction_notification_without_delay(reaction, article.organization)
end.not_to change(article.organization.notifications, :count)
end
end
describe "when notifiable is a Comment" do
let!(:comment) { create(:comment, commentable: article) }
it "doesn't allow to create duplicated comment reaction notifications for a user" do
comment = create(:comment, commentable: article)
reaction = create(:reaction, reactable: comment, user: user2)
described_class.send_reaction_notification_without_delay(reaction, comment.user)
expect(comment.user.notifications.count).to eq(1)
it "doesn't allow to create a duplicate user notification via import when action is nil" do
notification_attributes = { user: user, notifiable: comment, action: nil }
create(:notification, notification_attributes)
duplicate_notification = build(:notification, notification_attributes)
expect do
described_class.import([duplicate_notification],
on_duplicate_key_update: {
conflict_target: %i[notifiable_id notifiable_type user_id],
index_predicate: "action IS NULL",
columns: %i[json_data notified_at read]
})
end.not_to change(described_class, :count)
described_class.send_reaction_notification_without_delay(reaction, comment.user)
end.not_to change(comment.user.notifications, :count)
end
it "doesn't allow to create a duplicate org notification via import when action is nil" do
notification_attributes = { organization: organization, notifiable: comment, action: nil }
create(:notification, notification_attributes)
duplicate_notification = build(:notification, notification_attributes)
it "doesn't allow to create duplicated comment reaction notifications for an organization" do
article = create(:article, organization: organization)
comment = create(:comment, commentable: article)
reaction = create(:reaction, reactable: comment, user: user2)
described_class.send_reaction_notification_without_delay(reaction, article.organization)
expect(article.organization.notifications.count).to eq(1)
expect do
described_class.import([duplicate_notification],
on_duplicate_key_update: {
conflict_target: %i[notifiable_id notifiable_type organization_id],
index_predicate: "action IS NULL",
columns: %i[json_data notified_at read]
})
end.not_to change(described_class, :count)
described_class.send_reaction_notification_without_delay(reaction, article.organization)
end.not_to change(article.organization.notifications, :count)
end
end
end

3
spec/services/notifications/notifiable_action/send_spec.rb
View file

@ -52,6 +52,7 @@ RSpec.describe Notifications::NotifiableAction::Send, type: :service do
it "doesn't fail if the notification already exists" do
notification = create(:notification, user: user2, action: "Published", notifiable: article)
result = described_class.call(article, "Published")
expect(result.ids).to include(notification.id)
ids = result.to_a.map { |r| r["id"] }
expect(ids).to include(notification.id)
end
end

BIN
vendor/cache/activerecord-import-1.0.5.gem vendored
View file

Binary file not shown.