diff --git a/Gemfile b/Gemfile index acc098082..ae94bb4dc 100644 --- a/Gemfile +++ b/Gemfile @@ -41,6 +41,7 @@ gem "fog-aws", "~> 3.10" # 'fog' gem to support Amazon Web Services gem "front_matter_parser", "~> 1.0" # Parse a front matter from syntactically correct strings or files gem "gemoji", "~> 4.0.0.rc2" # Character information and metadata for standard and custom emoji gem "gibbon", "~> 3.4" # API wrapper for MailChimp's API +gem "hairtrigger", "~> 0.2.24" # HairTrigger lets you create and manage database triggers in a concise, db-agnostic, Rails-y way. gem "honeybadger", "~> 4.8" # Used for tracking application errors gem "honeycomb-beeline", "~> 2.4.0" # Monitoring and Observability gem gem "html_truncator", "~> 0.4" # Truncate an HTML string properly diff --git a/Gemfile.lock b/Gemfile.lock index 804d01c9f..b83ab75b0 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -340,6 +340,10 @@ GEM guard (~> 2.8) guard-compat (~> 1.0) multi_json (~> 1.8) + hairtrigger (0.2.24) + activerecord (>= 5.0, < 7) + ruby2ruby (~> 2.4) + ruby_parser (~> 3.10) hashdiff (1.0.1) hashie (4.1.0) heapy (0.2.0) @@ -684,6 +688,11 @@ GEM ruby-vips (2.1.0) ffi (~> 1.12) ruby2_keywords (0.0.4) + ruby2ruby (2.4.4) + ruby_parser (~> 3.1) + sexp_processor (~> 4.6) + ruby_parser (3.15.1) + sexp_processor (~> 4.9) rubyzip (2.3.0) s3_direct_upload (0.1.7) coffee-rails (>= 3.1) @@ -715,6 +724,7 @@ GEM childprocess (>= 0.5, < 4.0) rubyzip (>= 1.2.2) semantic_range (3.0.0) + sexp_processor (4.15.2) shellany (0.0.1) shoulda-matchers (4.5.1) activesupport (>= 4.2.0) @@ -892,6 +902,7 @@ DEPENDENCIES gibbon (~> 3.4) guard (~> 2.16) guard-livereload (~> 2.5) + hairtrigger (~> 0.2.24) honeybadger (~> 4.8) honeycomb-beeline (~> 2.4.0) html_truncator (~> 0.4) diff --git a/app/models/article.rb b/app/models/article.rb index 2f41a947a..0f976f109 100644 --- a/app/models/article.rb +++ b/app/models/article.rb @@ -114,18 +114,55 @@ class Article < ApplicationRecord after_commit :sync_related_elasticsearch_docs, on: %i[update] after_commit :remove_from_elasticsearch, on: [:destroy] + # The trigger `update_reading_list_document` is used to keep the `articles.reading_list_document` column updated. + # + # Its body is inserted in a PostgreSQL trigger function and that joins the columns values + # needed to search documents in the context of a "reading list". + # + # Please refer to https://github.com/jenseng/hair_trigger#usage in case you want to change or update the trigger. + # + # Additional information on how triggers work can be found in + # => https://www.postgresql.org/docs/11/trigger-definition.html + # => https://www.cybertec-postgresql.com/en/postgresql-how-to-write-a-trigger/ + # + # Adapted from https://dba.stackexchange.com/a/289361/226575 + trigger + .name(:update_reading_list_document).before(:insert, :update).for_each(:row) + .declare("l_org_vector tsvector; l_user_vector tsvector") do + <<~SQL + NEW.reading_list_document := + to_tsvector('simple'::regconfig, unaccent(coalesce(NEW.body_markdown, ''))) || + to_tsvector('simple'::regconfig, unaccent(coalesce(NEW.cached_tag_list, ''))) || + to_tsvector('simple'::regconfig, unaccent(coalesce(NEW.cached_user_name, ''))) || + to_tsvector('simple'::regconfig, unaccent(coalesce(NEW.cached_user_username, ''))) || + to_tsvector('simple'::regconfig, unaccent(coalesce(NEW.title, ''))) || + to_tsvector('simple'::regconfig, + unaccent( + coalesce( + array_to_string( + -- cached_organization is serialized to the DB as a YAML string, we extract only the name attribute + regexp_match(NEW.cached_organization, 'name: (.*)$', 'n'), + ' ' + ), + '' + ) + ) + ); + SQL + end + serialize :cached_user serialize :cached_organization - # [@rhymes] this is adapted from the `search_fields` property in - # `config/elasticsearch/mappings/feed_content.json` pg_search_scope :search_reading_list, - against: %i[body_markdown title cached_tag_list], - associated_against: { - organization: %i[name], - user: %i[name username] + against: :reading_list_document, + using: { + tsearch: { + prefix: true, + tsvector_column: :reading_list_document + } }, - using: { tsearch: { prefix: true } } + ignoring: :accents # [@jgaskins] We use an index on `published`, but since it's a boolean value # the Postgres query planner often skips it due to lack of diversity of the diff --git a/app/models/organization.rb b/app/models/organization.rb index 1d06f958d..49c059e84 100644 --- a/app/models/organization.rb +++ b/app/models/organization.rb @@ -14,6 +14,9 @@ class Organization < ApplicationRecord before_validation :downcase_slug before_validation :check_for_slug_change before_validation :evaluate_markdown + + # TODO: [@rhymes] revisit this callback, `update_articles_cached_organization` and `article_sync` + # when we remove Elasticsearch before_save :update_articles before_save :remove_at_from_usernames before_save :generate_secret @@ -22,6 +25,14 @@ class Organization < ApplicationRecord # https://guides.rubyonrails.org/active_record_callbacks.html#destroying-an-object before_destroy :cache_article_ids + after_save :bust_cache + + # This callback will eventually invoke Article.update_cached_user to update the organization.name + # only when it has been changed, thus invoking the trigger on Article.reading_list_document + after_update_commit :update_articles_cached_organization + after_update_commit :sync_related_elasticsearch_docs + after_destroy_commit :bust_cache, :article_sync + has_many :articles, dependent: :nullify has_many :collections, dependent: :nullify has_many :credits, dependent: :restrict_with_error @@ -63,11 +74,6 @@ class Organization < ApplicationRecord validate :unique_slug_including_users_and_podcasts, if: :slug_changed? - after_save :bust_cache - - after_commit :sync_related_elasticsearch_docs, on: :update - after_commit :bust_cache, :article_sync, on: :destroy - mount_uploader :profile_image, ProfileImageUploader mount_uploader :nav_image, ProfileImageUploader mount_uploader :dark_nav_image, ProfileImageUploader @@ -146,6 +152,12 @@ class Organization < ApplicationRecord articles.update(cached_organization: Articles::CachedEntity.from_object(self)) end + def update_articles_cached_organization + return unless saved_change_to_attribute?(:name) + + articles.update(cached_organization: Articles::CachedEntity.from_object(self)) + end + def bust_cache Organizations::BustCacheWorker.perform_async(id, slug) end diff --git a/app/services/search/postgres/reading_list.rb b/app/services/search/postgres/reading_list.rb index dca6aef71..45568cac0 100644 --- a/app/services/search/postgres/reading_list.rb +++ b/app/services/search/postgres/reading_list.rb @@ -12,6 +12,7 @@ module Search "reactions.id AS reaction_id", "reactions.user_id AS reaction_user_id", ].freeze + REACTION_ATTRIBUTES = %i[id reactable_id user_id].freeze USER_ATTRIBUTES = %i[id name profile_image username].freeze DEFAULT_STATUSES = %w[confirmed valid].freeze @@ -31,7 +32,7 @@ module Search per_page = [(per_page || DEFAULT_PER_PAGE).to_i, MAX_PER_PAGE].min result = find_articles( - user_id: user.id, + user: user, term: term, statuses: statuses, tags: tags, @@ -58,12 +59,20 @@ module Search } end - def self.find_articles(user_id:, term:, statuses:, tags:, page:, per_page:) - relation = ::Article - .joins(:reactions) - .where("reactions.category": :readinglist) - .where("reactions.user_id": user_id) - .where("reactions.status": statuses) + def self.find_articles(user:, term:, statuses:, tags:, page:, per_page:) + # [@jgaskins, @rhymes] as `reactions` is potentially a big table, adding pagination + # to an INNER JOIN (eg. `joins(:reactions)`) exponentially decreases the performance, + # incrementing query time as the database has to scan all the rows just to discard + # them right after if they lie outside the bounds of the `OFFSET`. + # Even though it should have had a similar performance, we realized that a subquery + # enabled PostgreSQL query planner to drastically decrease the planned time (ca. 145x) + reaction_query_sql = user.reactions.readinglist + .where(status: statuses, reactable_type: "Article") + .order(created_at: :desc) + .select(*REACTION_ATTRIBUTES) + .to_sql + + relation = Article.joins("INNER JOIN (#{reaction_query_sql}) reactions ON reactions.reactable_id = articles.id") relation = relation.search_reading_list(term) if term.present? @@ -93,7 +102,7 @@ module Search # because we need to fetch the total number of articles, pre pagination total = relation.count - relation = relation.select(*ATTRIBUTES).order("reactions.created_at": :desc) + relation = relation.select(*ATTRIBUTES) relation = relation.page(page).per(per_page) { diff --git a/db/migrate/20210405143721_add_reading_list_document_to_articles.rb b/db/migrate/20210405143721_add_reading_list_document_to_articles.rb new file mode 100644 index 000000000..8b4f02e4e --- /dev/null +++ b/db/migrate/20210405143721_add_reading_list_document_to_articles.rb @@ -0,0 +1,6 @@ +class AddReadingListDocumentToArticles < ActiveRecord::Migration[6.0] + def change + add_column :articles, :reading_list_document, :tsvector + end +end + diff --git a/db/migrate/20210405145104_add_index_to_reading_list_document_on_articles.rb b/db/migrate/20210405145104_add_index_to_reading_list_document_on_articles.rb new file mode 100644 index 000000000..3132f3b14 --- /dev/null +++ b/db/migrate/20210405145104_add_index_to_reading_list_document_on_articles.rb @@ -0,0 +1,7 @@ +class AddIndexToReadingListDocumentOnArticles < ActiveRecord::Migration[6.0] + disable_ddl_transaction! + + def change + add_index :articles, :reading_list_document, using: "gin", algorithm: :concurrently + end +end diff --git a/db/migrate/20210407172628_create_trigger_articles_insert_update.rb b/db/migrate/20210407172628_create_trigger_articles_insert_update.rb new file mode 100644 index 000000000..b7fe7db1c --- /dev/null +++ b/db/migrate/20210407172628_create_trigger_articles_insert_update.rb @@ -0,0 +1,39 @@ +# This migration was auto-generated via `rake db:generate_trigger_migration'. +# While you can edit this file, any changes you make to the definitions here +# will be undone by the next auto-generated trigger migration. + +class CreateTriggerArticlesInsertUpdate < ActiveRecord::Migration[6.1] + def up + create_trigger("update_reading_list_document", :generated => true, :compatibility => 1). + on("articles"). + name("update_reading_list_document"). + before(:insert, :update). + for_each(:row). + declare("l_org_vector tsvector; l_user_vector tsvector") do + <<-SQL_ACTIONS +NEW.reading_list_document := + to_tsvector('simple'::regconfig, unaccent(coalesce(NEW.body_markdown, ''))) || + to_tsvector('simple'::regconfig, unaccent(coalesce(NEW.cached_tag_list, ''))) || + to_tsvector('simple'::regconfig, unaccent(coalesce(NEW.cached_user_name, ''))) || + to_tsvector('simple'::regconfig, unaccent(coalesce(NEW.cached_user_username, ''))) || + to_tsvector('simple'::regconfig, unaccent(coalesce(NEW.title, ''))) || + to_tsvector('simple'::regconfig, + unaccent( + coalesce( + array_to_string( + -- cached_organization is serialized to the DB as a YAML string, we extract only the name attribute + regexp_match(NEW.cached_organization, 'name: (.*)$', 'n'), + ' ' + ), + '' + ) + ) + ); + SQL_ACTIONS + end + end + + def down + drop_trigger("update_reading_list_document", "articles", :generated => true) + end +end diff --git a/db/schema.rb b/db/schema.rb index d9734c957..885b28536 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 2021_03_31_181505) do +ActiveRecord::Schema.define(version: 2021_04_07_172628) do # These are extensions that must be enabled in order to support this database enable_extension "citext" @@ -131,6 +131,7 @@ ActiveRecord::Schema.define(version: 2021_03_31_181505) do t.boolean "published_from_feed", default: false t.integer "rating_votes_count", default: 0, null: false t.integer "reactions_count", default: 0, null: false + t.tsvector "reading_list_document" t.integer "reading_time", default: 0 t.boolean "receive_notifications", default: true t.integer "score", default: 0 @@ -164,6 +165,7 @@ ActiveRecord::Schema.define(version: 2021_03_31_181505) do t.index ["public_reactions_count"], name: "index_articles_on_public_reactions_count", order: :desc t.index ["published"], name: "index_articles_on_published" t.index ["published_at"], name: "index_articles_on_published_at" + t.index ["reading_list_document"], name: "index_articles_on_reading_list_document", using: :gin t.index ["slug", "user_id"], name: "index_articles_on_slug_and_user_id", unique: true t.index ["user_id"], name: "index_articles_on_user_id" end @@ -1537,4 +1539,32 @@ ActiveRecord::Schema.define(version: 2021_03_31_181505) do add_foreign_key "users_settings", "users" add_foreign_key "webhook_endpoints", "oauth_applications" add_foreign_key "webhook_endpoints", "users" + create_trigger("update_reading_list_document", :generated => true, :compatibility => 1). + on("articles"). + name("update_reading_list_document"). + before(:insert, :update). + for_each(:row). + declare("l_org_vector tsvector; l_user_vector tsvector") do + <<-SQL_ACTIONS +NEW.reading_list_document := + to_tsvector('simple'::regconfig, unaccent(coalesce(NEW.body_markdown, ''))) || + to_tsvector('simple'::regconfig, unaccent(coalesce(NEW.cached_tag_list, ''))) || + to_tsvector('simple'::regconfig, unaccent(coalesce(NEW.cached_user_name, ''))) || + to_tsvector('simple'::regconfig, unaccent(coalesce(NEW.cached_user_username, ''))) || + to_tsvector('simple'::regconfig, unaccent(coalesce(NEW.title, ''))) || + to_tsvector('simple'::regconfig, + unaccent( + coalesce( + array_to_string( + -- cached_organization is serialized to the DB as a YAML string, we extract only the name attribute + regexp_match(NEW.cached_organization, 'name: (.*)$', 'n'), + ' ' + ), + '' + ) + ) + ); + SQL_ACTIONS + end + end diff --git a/lib/data_update_scripts/20210405152916_invoke_update_reading_list_document_trigger_on_articles.rb b/lib/data_update_scripts/20210405152916_invoke_update_reading_list_document_trigger_on_articles.rb new file mode 100644 index 000000000..1c89fb475 --- /dev/null +++ b/lib/data_update_scripts/20210405152916_invoke_update_reading_list_document_trigger_on_articles.rb @@ -0,0 +1,11 @@ +module DataUpdateScripts + class InvokeUpdateReadingListDocumentTriggerOnArticles + def run + return unless ActiveRecord::Base.connection.column_exists?(:articles, :reading_list_document) + + # by updating `reading_list_document` to `NULL`, + # we invoke the `update_reading_list_document` trigger, bypassing Rails callbacks + Article.in_batches.update_all(reading_list_document: nil) + end + end +end diff --git a/spec/models/article_spec.rb b/spec/models/article_spec.rb index 81602a8a1..9f0d4e301 100644 --- a/spec/models/article_spec.rb +++ b/spec/models/article_spec.rb @@ -997,6 +997,44 @@ RSpec.describe Article, type: :model do end end + context "when triggers are invoked" do + let(:article) { create(:article) } + + before do + article.update(body_markdown: "An intense movie") + end + + it "sets .reading_list_document on insert" do + expect(article.reload.reading_list_document).to be_present + end + + it "updates .reading_list_document with body_markdown" do + article.update(body_markdown: "Something has changed") + + expect(article.reload.reading_list_document).to include("something") + end + + it "updates .reading_list_document with cached_tag_list" do + article.update(tag_list: %w[rust python]) + + expect(article.reload.reading_list_document).to include("rust") + end + + it "updates .reading_list_document with title" do + article.update(title: "Synecdoche, Los Angeles") + + expect(article.reload.reading_list_document).to include("angeles") + end + + it "removes a previous value from .reading_list_document on update", :aggregate_failures do + tag = article.tags.first.name + article.update(tag_list: %w[fsharp go]) + + expect(article.reload.reading_list_document).not_to include(tag) + expect(article.reload.reading_list_document).to include("fsharp") + end + end + describe ".feed" do it "returns records with a subset of attributes" do feed_article = described_class.feed.first diff --git a/spec/models/organization_spec.rb b/spec/models/organization_spec.rb index 947f80551..f9e1e44f1 100644 --- a/spec/models/organization_spec.rb +++ b/spec/models/organization_spec.rb @@ -254,7 +254,9 @@ RSpec.describe Organization, type: :model do context "when dealing with organization articles" do before do create(:organization_membership, user: user, organization: organization, type_of_user: "admin") - create(:article, organization: organization, user: user) + article = create(:article, organization: organization, user: user) + organization.articles << article + organization.save end it "updates the paths of the organization's articles" do @@ -278,6 +280,39 @@ RSpec.describe Organization, type: :model do article = Article.find_by(organization_id: organization.id) expect(article.cached_organization.slug).to eq(new_slug) end + + # these tests rely on `Organization.update_articles_cached_organization` callback, + # which will eventually invoke the trigger + # rubocop:disable RSpec/NestedGroups + context "when callbacks eventually invoke the trigger on Article.reading_list_document" do + it "updates the articles .reading_list_document when updating the name" do + article = Article.find_by(organization_id: organization.id) + old_reading_list_document = article.reading_list_document + + organization.update(name: "ACME Org") + + expect(article.reload.reading_list_document).not_to eq(old_reading_list_document) + end + + it "does not update the articles .reading_list_document when updating the company_size" do + article = Article.find_by(organization_id: organization.id) + old_reading_list_document = article.reading_list_document + + organization.update(company_size: "200") + + expect(article.reload.reading_list_document).to eq(old_reading_list_document) + end + + it "removes the organization name from the .reading_list_document after destroy" do + article = Article.find_by(organization_id: organization.id) + + organization.update(name: "ACME") + organization.destroy + + expect(article.reload.reading_list_document).not_to include("acme") + end + end + # rubocop:enable RSpec/NestedGroups end end diff --git a/spec/services/search/postgres/reading_list_spec.rb b/spec/services/search/postgres/reading_list_spec.rb index c99576b33..03fdc6ae5 100644 --- a/spec/services/search/postgres/reading_list_spec.rb +++ b/spec/services/search/postgres/reading_list_spec.rb @@ -231,24 +231,18 @@ RSpec.describe Search::Postgres::ReadingList, type: :service do end it "matches against the article's user's name", :aggregate_failures do - article_user = article.user - article_user.update_columns(name: "Friday Sunday") - - result = described_class.search_documents(user, term: "Frida") + result = described_class.search_documents(user, term: article.user_name.first(3)) expect(extract_from_results(result, :path)).to include(article.path) - result = described_class.search_documents(user, term: "Sat") + result = described_class.search_documents(user, term: "notaname") expect(extract_from_results(result, :path)).to be_empty end it "matches against the article's user's username", :aggregate_failures do - article_user = article.user - article_user.update_columns(username: "fridaysunday") - - result = described_class.search_documents(user, term: "Frida") + result = described_class.search_documents(user, term: article.user_username.first(3)) expect(extract_from_results(result, :path)).to include(article.path) - result = described_class.search_documents(user, term: "Sat") + result = described_class.search_documents(user, term: "notausername") expect(extract_from_results(result, :path)).to be_empty end end diff --git a/vendor/cache/hairtrigger-0.2.24.gem b/vendor/cache/hairtrigger-0.2.24.gem new file mode 100644 index 000000000..9d69a1290 Binary files /dev/null and b/vendor/cache/hairtrigger-0.2.24.gem differ diff --git a/vendor/cache/ruby2ruby-2.4.4.gem b/vendor/cache/ruby2ruby-2.4.4.gem new file mode 100644 index 000000000..b15fae493 Binary files /dev/null and b/vendor/cache/ruby2ruby-2.4.4.gem differ diff --git a/vendor/cache/ruby_parser-3.15.1.gem b/vendor/cache/ruby_parser-3.15.1.gem new file mode 100644 index 000000000..ac82c6e76 Binary files /dev/null and b/vendor/cache/ruby_parser-3.15.1.gem differ diff --git a/vendor/cache/sexp_processor-4.15.2.gem b/vendor/cache/sexp_processor-4.15.2.gem new file mode 100644 index 000000000..596a74deb Binary files /dev/null and b/vendor/cache/sexp_processor-4.15.2.gem differ