From 0dfd3b2309d4464829a62d101584653bc1322b17 Mon Sep 17 00:00:00 2001 From: Reid Lynch Date: Thu, 15 Feb 2024 19:58:46 -0500 Subject: [PATCH] Improve performance of ratings in JSON --- .../api/v1/bulk/queries_controller.rb | 2 +- app/controllers/api/v1/queries_controller.rb | 2 +- .../authentication/current_case_manager.rb | 7 ++---- app/models/application_record.rb | 4 ++++ app/models/case.rb | 9 ++++++++ app/models/rating.rb | 2 +- .../api/v1/export/cases/_query.json.jbuilder | 2 +- app/views/api/v1/queries/_query.json.jbuilder | 6 ++--- ...19_update_query_updated_at_with_ratings.rb | 7 ++++++ db/schema.rb | 2 +- test/models/case_test.rb | 22 +++++++++++++++++++ 11 files changed, 51 insertions(+), 14 deletions(-) create mode 100644 db/migrate/20240214020519_update_query_updated_at_with_ratings.rb diff --git a/app/controllers/api/v1/bulk/queries_controller.rb b/app/controllers/api/v1/bulk/queries_controller.rb index 27e39b2e2..f8aa8c691 100644 --- a/app/controllers/api/v1/bulk/queries_controller.rb +++ b/app/controllers/api/v1/bulk/queries_controller.rb @@ -37,7 +37,7 @@ def create if Query.import queries_to_import @case.reload - @queries = @case.queries.includes([ :ratings ]) + @queries = @case.queries @display_order = @queries.map(&:id) respond_with @queries, @display_order diff --git a/app/controllers/api/v1/queries_controller.rb b/app/controllers/api/v1/queries_controller.rb index 6b2d695de..cd14f1ab9 100644 --- a/app/controllers/api/v1/queries_controller.rb +++ b/app/controllers/api/v1/queries_controller.rb @@ -9,7 +9,7 @@ class QueriesController < Api::ApiController before_action :check_query, only: [ :update, :destroy ] def index - @queries = @case.queries.includes([ :ratings ]) + @queries = @case.queries @display_order = @queries.map(&:id) diff --git a/app/controllers/concerns/authentication/current_case_manager.rb b/app/controllers/concerns/authentication/current_case_manager.rb index 8698bb6c4..d5dd956da 100644 --- a/app/controllers/concerns/authentication/current_case_manager.rb +++ b/app/controllers/concerns/authentication/current_case_manager.rb @@ -79,11 +79,8 @@ def case_with_all_the_bells_whistles if current_user @case = current_user .cases_involved_with - .where(id: params[:case_id]) - .includes(:tries ) - .preload([ queries: [ :ratings ], tries: [ :curator_variables, :search_endpoint ] ]) - .order('tries.try_number DESC') - .first + .preload(:queries, tries: [ :curator_variables, :search_endpoint ]) + .find_by(id: params[:case_id]) end @case = Case.public_cases.find_by(id: params[:case_id]) if @case.nil? end diff --git a/app/models/application_record.rb b/app/models/application_record.rb index 08dc53798..74ac34702 100644 --- a/app/models/application_record.rb +++ b/app/models/application_record.rb @@ -2,4 +2,8 @@ class ApplicationRecord < ActiveRecord::Base primary_abstract_class + + def connection + self.class.connection + end end diff --git a/app/models/case.rb b/app/models/case.rb index 1f391cc9c..dd3d7919c 100644 --- a/app/models/case.rb +++ b/app/models/case.rb @@ -195,6 +195,15 @@ def public_id Rails.application.message_verifier('magic').generate(id) end + # Optimized method to retrieve all doc ratings a single time + # Returns [5424, [{"query_id"=>5424, "doc_id"=>"l_21426", "rating"=>1.0}], ...] + def doc_ratings_by_query + @doc_ratings_by_query ||= begin + result = connection.select_all(ratings.select(:query_id, :doc_id, :rating).to_sql) + result.group_by { |r| r['query_id'] } + end + end + private def set_scorer diff --git a/app/models/rating.rb b/app/models/rating.rb index 6e9edd6e2..dcc1f5564 100644 --- a/app/models/rating.rb +++ b/app/models/rating.rb @@ -23,7 +23,7 @@ # class Rating < ApplicationRecord - belongs_to :query + belongs_to :query, touch: true belongs_to :user, optional: true # arguably we shouldn't need this, however today you can have a rating object that doesn't have a diff --git a/app/views/api/v1/export/cases/_query.json.jbuilder b/app/views/api/v1/export/cases/_query.json.jbuilder index 26d09aebe..509a20aec 100644 --- a/app/views/api/v1/export/cases/_query.json.jbuilder +++ b/app/views/api/v1/export/cases/_query.json.jbuilder @@ -8,5 +8,5 @@ json.notes query.notes json.information_need query.information_need json.ratings do - json.array! query.ratings, partial: 'rating', as: :rating + @case.doc_ratings_by_query[query.id]&.each { |rating| json.set! rating['doc_id'], rating['rating'] } end diff --git a/app/views/api/v1/queries/_query.json.jbuilder b/app/views/api/v1/queries/_query.json.jbuilder index bdbfc8ca4..ee9fb45a6 100644 --- a/app/views/api/v1/queries/_query.json.jbuilder +++ b/app/views/api/v1/queries/_query.json.jbuilder @@ -7,10 +7,8 @@ json.query_text query.query_text json.options query.options json.notes query.notes json.information_need query.information_need - -# pick the most recent update between a query and it's ratings to represent modified_at -json.modified_at [ query, query.ratings ].flatten.max_by(&:updated_at).updated_at +json.modified_at query.updated_at json.ratings do - query.ratings.each { |rating| json.set! rating.doc_id, rating.rating } + @case.doc_ratings_by_query[query.id]&.each { |rating| json.set! rating['doc_id'], rating['rating'] } end diff --git a/db/migrate/20240214020519_update_query_updated_at_with_ratings.rb b/db/migrate/20240214020519_update_query_updated_at_with_ratings.rb new file mode 100644 index 000000000..13f688485 --- /dev/null +++ b/db/migrate/20240214020519_update_query_updated_at_with_ratings.rb @@ -0,0 +1,7 @@ +class UpdateQueryUpdatedAtWithRatings < ActiveRecord::Migration[7.1] + def change + execute <<-SQL + UPDATE queries SET updated_at = GREATEST(updated_at, (SELECT MAX(updated_at) FROM ratings WHERE query_id = queries.id)) + SQL + end +end diff --git a/db/schema.rb b/db/schema.rb index 1f916c30f..5d2b3490c 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[7.1].define(version: 2024_02_01_171336) do +ActiveRecord::Schema[7.1].define(version: 2024_02_14_020519) 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 diff --git a/test/models/case_test.rb b/test/models/case_test.rb index b098b584a..c6da8e243 100644 --- a/test/models/case_test.rb +++ b/test/models/case_test.rb @@ -290,4 +290,26 @@ class CaseTest < ActiveSupport::TestCase assert_equal 2, the_case.queries_count end end + + describe 'doc_ratings_by_query' do + let(:the_case) { cases(:random_case) } + + it 'returns an empty hash for a new case' do + assert_equal Case.new.doc_ratings_by_query, {} + end + + it 'returns an empty hash when there are no queries' do + the_case.queries.destroy_all + assert_equal the_case.doc_ratings_by_query, {} + end + + it 'returns a hash of doc ratings by query id' do + assert_equal the_case.doc_ratings_by_query, { + 488_291_725 => [ + { 'query_id' => 488_291_725, 'doc_id' => '234', 'rating' => nil }, + { 'query_id' => 488_291_725, 'doc_id' => '123', 'rating' => 5.0 } + ], + } + end + end end