From 8c72f7bfbcfaab59540c59703ccf15a023ed6730 Mon Sep 17 00:00:00 2001 From: Eric Pugh Date: Fri, 10 Jan 2025 08:05:57 -0500 Subject: [PATCH] Backout counter_cache (#1165) It caused other issues like in tests, and I'm not sure it was a slam dunk for performance. Instead we need to be smarter about how we decide to load this data. --- .env.example | 3 ++ .../api/v1/cases/dropdown_controller.rb | 2 +- .../authentication/current_book_manager.rb | 34 ++++--------------- .../authentication/current_case_manager.rb | 34 +++---------------- app/controllers/home_controller.rb | 18 +++++----- app/javascript/application2.js | 1 + app/models/book.rb | 11 +++++- app/models/query_doc_pair.rb | 2 +- app/views/home/_book_summary.html.erb | 5 +-- app/views/home/show.html.erb | 2 +- app/views/layouts/_dropdown_case.html.erb | 2 ++ app/views/layouts/_header.html.erb | 5 +-- ...3404_add_query_doc_pairs_count_to_books.rb | 5 --- ...218173819_reset_all_book_cache_counters.rb | 17 ---------- db/schema.rb | 3 +- .../api/v1/cases/dropdown_controller_test.rb | 4 +-- test/fixtures/books.yml | 1 - test/models/book_test.rb | 1 - 18 files changed, 45 insertions(+), 105 deletions(-) create mode 100644 app/views/layouts/_dropdown_case.html.erb delete mode 100644 db/migrate/20241218173404_add_query_doc_pairs_count_to_books.rb delete mode 100644 db/migrate/20241218173819_reset_all_book_cache_counters.rb diff --git a/.env.example b/.env.example index 607a709dc..e37cca7f9 100644 --- a/.env.example +++ b/.env.example @@ -5,6 +5,9 @@ PORT=3000 RACK_ENV=development RAILS_ENV=development +# This makes Quepid display detailed error messages. +QUEPID_CONSIDER_ALL_REQUESTS_LOCAL=true + MAX_THREADS=2 WEB_CONCURRENCY=2 diff --git a/app/controllers/api/v1/cases/dropdown_controller.rb b/app/controllers/api/v1/cases/dropdown_controller.rb index e1663f85e..085ff061f 100644 --- a/app/controllers/api/v1/cases/dropdown_controller.rb +++ b/app/controllers/api/v1/cases/dropdown_controller.rb @@ -5,7 +5,7 @@ module V1 module Cases class DropdownController < Api::ApiController def index - @cases = recent_cases 3 + @cases = recent_cases 4 respond_with @cases end diff --git a/app/controllers/concerns/authentication/current_book_manager.rb b/app/controllers/concerns/authentication/current_book_manager.rb index 6147558b0..eff4f7e06 100644 --- a/app/controllers/concerns/authentication/current_book_manager.rb +++ b/app/controllers/concerns/authentication/current_book_manager.rb @@ -20,45 +20,23 @@ def check_book end def set_recent_books - @recent_books = recent_books(3) + @recent_books = recent_books(4) end - # rubocop:disable Metrics/MethodLength def recent_books count if current_user - # Using joins/includes will not return the proper list in the - # correct order because rails refuses to include the - # `book_metadata`.`last_viewed_at` column in the SELECT statement - # which will then cause the ordering not to work properly. - # So instead, we have this beauty! - sql = " - SELECT DISTINCT `books`.`id`, `book_metadata`.`last_viewed_at` - FROM `books` - LEFT OUTER JOIN `book_metadata` ON `book_metadata`.`book_id` = `books`.`id` - LEFT OUTER JOIN `teams_books` ON `teams_books`.`book_id` = `books`.`id` - LEFT OUTER JOIN `teams` ON `teams`.`id` = `teams_books`.`team_id` - LEFT OUTER JOIN `teams_members` ON `teams_members`.`team_id` = `teams`.`id` - LEFT OUTER JOIN `users` ON `users`.`id` = `teams_members`.`member_id` - WHERE (`teams_members`.`member_id` = #{current_user.id} OR `books`.`owner_id` = #{current_user.id}) - ORDER BY `book_metadata`.`last_viewed_at` DESC, `books`.`id` DESC - LIMIT #{count} - " - - results = ActiveRecord::Base.connection.execute(sql) - - book_ids = results.map do |row| - row.first.to_i - end + book_ids = current_user.book_metadata.order(last_viewed_at: :desc).limit(count).pluck(:book_id) # map to objects - # cases = Case.includes(:tries).where(id: [ case_ids ]) - books = Book.where(id: [ book_ids ]) + books = current_user.books_involved_with.where(id: book_ids) + + # the WHERE IN clause doesn't guarantee returning in order, so this sorts the cases in order of last viewing. books = books.sort_by { |x| book_ids.index x.id } + else books = [] end books end - # rubocop:enable Metrics/MethodLength end end diff --git a/app/controllers/concerns/authentication/current_case_manager.rb b/app/controllers/concerns/authentication/current_case_manager.rb index bc6fc6c76..bbe8b3c38 100644 --- a/app/controllers/concerns/authentication/current_case_manager.rb +++ b/app/controllers/concerns/authentication/current_case_manager.rb @@ -35,47 +35,23 @@ def set_case end def set_recent_cases - @recent_cases = recent_cases(3) + @recent_cases = recent_cases(4) end - # rubocop:disable Metrics/MethodLength def recent_cases count if current_user - # Using joins/includes will not return the proper list in the - # correct order because rails refuses to include the - # `case_metadata`.`last_viewed_at` column in the SELECT statement - # which will then cause the ordering not to work properly. - # So instead, we have this beauty! - sql = " - SELECT DISTINCT `cases`.`id`, `case_metadata`.`last_viewed_at` - FROM `cases` - LEFT OUTER JOIN `case_metadata` ON `case_metadata`.`case_id` = `cases`.`id` - LEFT OUTER JOIN `teams_cases` ON `teams_cases`.`case_id` = `cases`.`id` - LEFT OUTER JOIN `teams` ON `teams`.`id` = `teams_cases`.`team_id` - LEFT OUTER JOIN `teams_members` ON `teams_members`.`team_id` = `teams`.`id` - LEFT OUTER JOIN `users` ON `users`.`id` = `teams_members`.`member_id` - WHERE (`teams_members`.`member_id` = #{current_user.id} OR `cases`.`owner_id` = #{current_user.id}) - AND (`cases`.`archived` = false OR `cases`.`archived` IS NULL) - ORDER BY `case_metadata`.`last_viewed_at` DESC, `cases`.`id` DESC - LIMIT #{count} - " - - results = ActiveRecord::Base.connection.execute(sql) - - case_ids = results.map do |row| - row.first.to_i - end + case_ids = current_user.case_metadata.order(last_viewed_at: :desc).limit(count).pluck(:case_id) # map to objects - # cases = Case.includes(:tries).where(id: [ case_ids ]) - cases = Case.where(id: [ case_ids ]) + cases = current_user.cases_involved_with.where(id: case_ids).not_archived + + # the WHERE IN clause doesn't guarantee returning in order, so this sorts the cases in order of last viewing. cases = cases.sort_by { |x| case_ids.index x.id } else cases = [] end cases end - # rubocop:enable Metrics/MethodLength def check_case render json: { message: 'Case not found!' }, status: :not_found unless @case diff --git a/app/controllers/home_controller.rb b/app/controllers/home_controller.rb index ae2a15f1d..ee36b3a8a 100644 --- a/app/controllers/home_controller.rb +++ b/app/controllers/home_controller.rb @@ -7,6 +7,7 @@ class HomeController < ApplicationController def show # with_counts adds a `case.queries_count` field, which avoids loading # all queries and makes bullet happy. + # @cases = recent_cases(30) @cases = @current_user.cases_involved_with.not_archived.with_counts .includes([ :metadata ]) .order('`case_metadata`.`last_viewed_at` DESC, `cases`.`id` DESC') @@ -15,15 +16,16 @@ def show @most_recent_cases = @cases[0...4].sort_by { |c| c.case_name.downcase } @most_recent_books = recent_books(4) - @lookup_for_books = {} - @most_recent_books.each do |book| - judged_by_current_user = book.judgements.where(user: @current_user).count - if judged_by_current_user.positive? && judged_by_current_user < book.query_doc_pairs.size - @lookup_for_books[book] = book.query_doc_pairs.size - judged_by_current_user - end - end - @most_recent_books.sort_by!(&:name) + # @lookup_for_books = {} + # @most_recent_books.each do |book| + # judged_by_current_user = book.judgements.where(user: @current_user).count + # if judged_by_current_user.positive? && judged_by_current_user < book.query_doc_pairs.size + # @lookup_for_books[book] = book.query_doc_pairs.size - judged_by_current_user + # end + # end + + @most_recent_books = @most_recent_books.sort_by { |b| b.name.downcase } # Homepage is too slow so we have to cut some stuff out ;-( # candidate_cases = @cases.select { |kase| kase.scores.scored.count.positive? } diff --git a/app/javascript/application2.js b/app/javascript/application2.js index d6ae5ddcf..d57d1b3b6 100644 --- a/app/javascript/application2.js +++ b/app/javascript/application2.js @@ -12,6 +12,7 @@ import Cookies from 'js-cookie' LocalTime.start() Turbo.config.drive.progressBarDelay = 1 +Turbo.session.drive = false import "vega" import "vega-lite" diff --git a/app/models/book.rb b/app/models/book.rb index d0336954b..a53beaa6d 100644 --- a/app/models/book.rb +++ b/app/models/book.rb @@ -9,7 +9,6 @@ # import_job :string(255) # name :string(255) # populate_job :string(255) -# query_doc_pairs_count :integer default(0), not null # show_rank :boolean default(FALSE) # support_implicit_judgements :boolean # created_at :datetime not null @@ -66,6 +65,16 @@ class Book < ApplicationRecord # Scopes include ForUserScope + scope :with_counts, -> { + select <<~SQL.squish + books.*, + ( + SELECT COUNT(query_doc_pairs.id) FROM query_doc_pairs + WHERE book_id = books.id + ) AS query_doc_pairs_count + SQL + } + private def delete_attachments diff --git a/app/models/query_doc_pair.rb b/app/models/query_doc_pair.rb index 227b79ba3..d1ca4669f 100644 --- a/app/models/query_doc_pair.rb +++ b/app/models/query_doc_pair.rb @@ -25,7 +25,7 @@ # fk_rails_... (book_id => books.id) # class QueryDocPair < ApplicationRecord - belongs_to :book, counter_cache: true + belongs_to :book has_many :judgements, dependent: :destroy, autosave: true validates :query_text, presence: true, length: { maximum: 2048 } diff --git a/app/views/home/_book_summary.html.erb b/app/views/home/_book_summary.html.erb index cd12c0e98..3bb1227c6 100644 --- a/app/views/home/_book_summary.html.erb +++ b/app/views/home/_book_summary.html.erb @@ -12,10 +12,7 @@

- - <% if @lookup_for_books[book] && @lookup_for_books[book] > 0 %> -

You need to judge <%= @lookup_for_books[book] %> out <%= book.query_doc_pairs.size %> query/doc pairs.

- <% end %> + <%= link_to 'Judge', book_judge_path(book), data: { turbo_prefetch: false }, class: 'btn btn-sm btn-primary', role: 'button' %> diff --git a/app/views/home/show.html.erb b/app/views/home/show.html.erb index f63485afd..293709562 100644 --- a/app/views/home/show.html.erb +++ b/app/views/home/show.html.erb @@ -54,7 +54,7 @@ Inspired by https://dev.to/themesberg/tutorial-how-to-build-a-simple-admin-dashb - <%= render partial: "case", collection: @cases, as: :kase %> + <%= render partial: "case", collection: @cases, as: :kase, cached: true %> diff --git a/app/views/layouts/_dropdown_case.html.erb b/app/views/layouts/_dropdown_case.html.erb new file mode 100644 index 000000000..20faeda53 --- /dev/null +++ b/app/views/layouts/_dropdown_case.html.erb @@ -0,0 +1,2 @@ +
  • <%= link_to_core_case kase.case_name, kase, kase.last_try_number %>
    • +
    • diff --git a/app/views/layouts/_header.html.erb b/app/views/layouts/_header.html.erb index 833081725..5006e88d2 100644 --- a/app/views/layouts/_header.html.erb +++ b/app/views/layouts/_header.html.erb @@ -18,10 +18,7 @@ RECENT CASES
    • - <% set_recent_cases.each do |kase| %> -
  • <%= link_to_core_case kase.case_name, kase, kase.last_try_number %>
    • -
    • - <% end %> + <%= render partial: 'layouts/dropdown_case', collection: set_recent_cases, as: :kase, cached: true %>
  • diff --git a/db/migrate/20241218173404_add_query_doc_pairs_count_to_books.rb b/db/migrate/20241218173404_add_query_doc_pairs_count_to_books.rb deleted file mode 100644 index 07580b9dc..000000000 --- a/db/migrate/20241218173404_add_query_doc_pairs_count_to_books.rb +++ /dev/null @@ -1,5 +0,0 @@ -class AddQueryDocPairsCountToBooks < ActiveRecord::Migration[8.0] - def change - add_column :books, :query_doc_pairs_count, :integer, default: 0, null: false - end -end diff --git a/db/migrate/20241218173819_reset_all_book_cache_counters.rb b/db/migrate/20241218173819_reset_all_book_cache_counters.rb deleted file mode 100644 index fa5ccd897..000000000 --- a/db/migrate/20241218173819_reset_all_book_cache_counters.rb +++ /dev/null @@ -1,17 +0,0 @@ -class ResetAllBookCacheCounters < ActiveRecord::Migration[8.0] - def up - - Book.all.each do |book| - - Book.reset_counters(book.id, :query_doc_pairs) - - end - - end - - def down - - # no rollback needed - - end -end diff --git a/db/schema.rb b/db/schema.rb index 12533d079..878471aa8 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[8.0].define(version: 2025_01_07_195202) do +ActiveRecord::Schema[8.0].define(version: 2025_01_08_171851) do create_table "active_storage_attachments", charset: "utf8mb4", collation: "utf8mb4_bin", force: :cascade do |t| t.string "name", null: false t.string "record_type", null: false @@ -196,7 +196,6 @@ t.string "export_job" t.string "import_job" t.string "populate_job" - t.integer "query_doc_pairs_count", default: 0, null: false t.index ["selection_strategy_id"], name: "index_books_on_selection_strategy_id" end diff --git a/test/controllers/api/v1/cases/dropdown_controller_test.rb b/test/controllers/api/v1/cases/dropdown_controller_test.rb index 8b9d3a57d..8d8bcc90b 100644 --- a/test/controllers/api/v1/cases/dropdown_controller_test.rb +++ b/test/controllers/api/v1/cases/dropdown_controller_test.rb @@ -87,7 +87,7 @@ class DropdownControllerTest < ActionController::TestCase assert_not_includes ids, archived.id end - test 'limits list to 3 cases' do + test 'limits list to 4 cases' do get :index assert_response :ok @@ -95,7 +95,7 @@ class DropdownControllerTest < ActionController::TestCase body = response.parsed_body cases = body['all_cases'] - assert cases.length <= 3 + assert cases.length <= 4 end test 'returns list of cases ordered by last viewed date' do diff --git a/test/fixtures/books.yml b/test/fixtures/books.yml index df3543bc8..426f33fb1 100644 --- a/test/fixtures/books.yml +++ b/test/fixtures/books.yml @@ -7,7 +7,6 @@ # import_job :string(255) # name :string(255) # populate_job :string(255) -# query_doc_pairs_count :integer default(0), not null # show_rank :boolean default(FALSE) # support_implicit_judgements :boolean # created_at :datetime not null diff --git a/test/models/book_test.rb b/test/models/book_test.rb index 48eaf75f1..6334f13e6 100644 --- a/test/models/book_test.rb +++ b/test/models/book_test.rb @@ -9,7 +9,6 @@ # import_job :string(255) # name :string(255) # populate_job :string(255) -# query_doc_pairs_count :integer default(0), not null # show_rank :boolean default(FALSE) # support_implicit_judgements :boolean # created_at :datetime not null