diff --git a/app/controllers/api/v4/application_controller.rb b/app/controllers/api/v4/application_controller.rb index 428d5e243..2dfffd201 100644 --- a/app/controllers/api/v4/application_controller.rb +++ b/app/controllers/api/v4/application_controller.rb @@ -71,9 +71,9 @@ def set_runner def set_run @run = if params[:historic] == "1" - Run.includes(:game, :category, :user, :histories, segments: [:histories]).find36(params[:run]) + Run.includes(:user, :histories, :video, category: [:srdc], game: [:srdc], segments: [:histories]).find36(params[:run]) else - Run.includes(:game, :category, :user, :segments).find36(params[:run]) + Run.includes(:user, :segments, :video, category: [:srdc], game: [:srdc]).find36(params[:run]) end rescue ActiveRecord::RecordNotFound render not_found(:run) @@ -110,7 +110,7 @@ def validate_user end def set_race(param: :race_id) # rubocop:disable Naming/AccessorMethodName - @race = Race.find(params[param]) + @race = Race.includes(category: [:srdc], entries: [:creator], game: [:srdc]).find(params[param]) return unless @race.secret_visibility? && !@race.joinable?(user: current_user, token: params[:join_token]) render status: :forbidden, json: { diff --git a/app/controllers/api/v4/games_controller.rb b/app/controllers/api/v4/games_controller.rb index 394a72087..21e6c50bb 100644 --- a/app/controllers/api/v4/games_controller.rb +++ b/app/controllers/api/v4/games_controller.rb @@ -2,11 +2,12 @@ class Api::V4::GamesController < Api::V4::ApplicationController before_action :set_game, only: [:show] def index + games = Game.includes(:srdc, categories: [:srdc]) if params[:search].blank? - @games = Game.joins(:srdc).includes(:srdc, categories: [:srdc]).order(:name) + @games = games.joins(:srdc).order(:name) else - id_search = Game.find_by(id: params[:search]) - @games = Game.search(params[:search]).includes(:srdc, categories: [:srdc]) + id_search = games.find_by(id: params[:search]) + @games = games.search(params[:search]) @games = @games.to_ary.unshift(id_search) if id_search.present? end render json: Api::V4::GameBlueprint.render(@games, root: :games, toplevel: :game) diff --git a/app/controllers/api/v4/races/entries_controller.rb b/app/controllers/api/v4/races/entries_controller.rb index 69ead8d23..c89b2e474 100644 --- a/app/controllers/api/v4/races/entries_controller.rb +++ b/app/controllers/api/v4/races/entries_controller.rb @@ -112,7 +112,7 @@ def check_permission end def set_entry(param: :id) # rubocop:disable Naming/AccessorMethodName - @entry = @race.entries.find(params[param]) + @entry = @race.entries.includes(:creator).find(params[param]) return if @entry.creator == current_user render status: :forbidden, json: { diff --git a/app/controllers/games_controller.rb b/app/controllers/games_controller.rb index ffaa1735b..a7095f654 100644 --- a/app/controllers/games_controller.rb +++ b/app/controllers/games_controller.rb @@ -22,8 +22,9 @@ def authorize end def set_game - @game = Game.joins(:srdc).find_by(speedrun_dot_com_games: {shortname: params[:game]}) || - Game.includes(:srdc).find(id: params[:game]) + games = Game.includes(:srdc, :srl) + @game = games.joins(:srdc).find_by(speedrun_dot_com_games: {shortname: params[:game]}) || + games.find(id: params[:game]) redirect_to game_path(@game) if @game.srdc.try(:shortname).present? && params[:game] == @game.id.to_s rescue ActiveRecord::RecordNotFound diff --git a/app/controllers/runs_controller.rb b/app/controllers/runs_controller.rb index aae56e18e..daad3cb22 100644 --- a/app/controllers/runs_controller.rb +++ b/app/controllers/runs_controller.rb @@ -115,8 +115,8 @@ def destroy private def set_run - @run = Run.includes(segments: {icon_attachment: :blob}).find_by(id: params[:id].to_i(36)) || - Run.includes(segments: {icon_attachment: :blob}).find_by!(nick: params[:id]) + runs = Run.includes(:entry, :highlight_suggestion, :segments, :user, :video, category: [:srdc], game: [:srdc], segments: {icon_attachment: :blob}) + @run = runs.find_by(id: params[:id].to_i(36)) || runs.find_by!(nick: params[:id]) timing = params[:timing] || @run.default_timing unless [Run::REAL, Run::GAME].include?(timing) redirect_to request.path, alert: 'Timing can only be real or game.' diff --git a/app/controllers/sessions_controller.rb b/app/controllers/sessions_controller.rb index 78ee6a65d..ede5d5747 100644 --- a/app/controllers/sessions_controller.rb +++ b/app/controllers/sessions_controller.rb @@ -4,7 +4,7 @@ def new end def create - @user = User.find_by(email: session_params[:email]) + @user = User.includes(:google, :twitch).find_by(email: session_params[:email]) if @user.nil? redirect_back( alert: "No account exists with that email address.", @@ -85,7 +85,7 @@ def failure def sign_in(user) create_auth_session(user) - auth_session.persist! + auth_session.persist end def redirect_path diff --git a/app/models/category.rb b/app/models/category.rb index c98d60d6a..92349cb12 100644 --- a/app/models/category.rb +++ b/app/models/category.rb @@ -93,16 +93,15 @@ def merge_into!(category) # route returns a run whose route is the most popular in this category, as determined by the number of runs sharing # its segment names and order. Returns nil if no runs exist in this category. def route - result = Run.select('MIN(runs.id) as id') - .joins(:segments) - .where(category: self) - .group(:segment_number, :name) - .order(Arel.sql('COUNT(*) DESC')) - .first + result = runs.select('MIN(runs.id) as id') + .joins(:segments) + .group(:segment_number, :name) + .order(Arel.sql('COUNT(*) DESC')) + .first return nil if result.nil? - Run.find(result.id) + Run.includes(:user).find(result.id) end # median_duration returns the median duration of completed attempts for this category. If attempt_number is given, it diff --git a/app/models/concerns/forgetful_persons_run.rb b/app/models/concerns/forgetful_persons_run.rb index 7868ac781..91baabfb7 100644 --- a/app/models/concerns/forgetful_persons_run.rb +++ b/app/models/concerns/forgetful_persons_run.rb @@ -8,36 +8,43 @@ module ForgetfulPersonsRun # is not complete (i.e. the last segment and any # segments directly before it have nil durations), we don't # return that last stretch of segments at all. def collapsed_segments(timing) - in_progress_segments = segments.reverse.take_while { |segment| segment.duration(timing).nil? } - segments[0..segments.count - 1 - in_progress_segments.count].reduce([]) do |segs, seg| - if segs.any? && segs.last.try(:duration, timing).nil? - skipped_seg = segs.last - segs + [Segment.new( - segs.pop.attributes.merge( - name: "#{skipped_seg.name} + #{seg.name}", - - realtime_start_ms: skipped_seg.start(Run::REAL).to_ms, - realtime_end_ms: seg.end(Run::REAL).to_ms, - realtime_duration_ms: seg.duration(Run::REAL).to_ms, - realtime_shortest_duration_ms: [skipped_seg, seg].map { |s| s.shortest_duration(Run::REAL).to_ms }.compact.sum(0), - - gametime_start_ms: skipped_seg.start(Run::GAME).to_ms, - gametime_end_ms: skipped_seg.end(Run::GAME).to_ms, - gametime_duration_ms: seg.duration(Run::GAME).to_ms, - gametime_shortest_duration_ms: [skipped_seg, seg].map { |s| s.shortest_duration(Run::GAME).to_ms }.compact.sum(0), - ).merge( - case timing - when Run::REAL - {realtime_reduced: true} - when Run::GAME - {gametime_reduced: true} - end - ) - )] - else - segs + [seg] - end - end.push(*in_progress_segments) + @collapsed_segments ||= begin + in_progress_segments = segments.reverse.take_while { |segment| segment.duration(timing).nil? } + segments[0..segments.count - 1 - in_progress_segments.count].reduce([]) do |segs, next_seg| + if segs.any? && segs.last.try(:duration, timing).nil? + skipped_seg = segs.last + + # We're about to modify this segment in memory for display purposes, + # but we don't want to persist it to the database. + # We also don't want to use Segment.new, + # because its associations (e.g. segment.run) won't be cached so would cause duplicate queries. + skipped_seg.readonly! + + skipped_seg.name = "#{skipped_seg.name} + #{next_seg.name}" + + skipped_seg.realtime_start_ms = skipped_seg.start(Run::REAL).to_ms + skipped_seg.realtime_end_ms = next_seg.end(Run::REAL).to_ms + skipped_seg.realtime_duration_ms = next_seg.duration(Run::REAL).to_ms + skipped_seg.realtime_shortest_duration_ms = [skipped_seg, next_seg].map { |s| s.shortest_duration(Run::REAL).to_ms }.compact.sum(0) + + skipped_seg.gametime_start_ms = skipped_seg.start(Run::GAME).to_ms + skipped_seg.gametime_end_ms = next_seg.end(Run::GAME).to_ms + skipped_seg.gametime_duration_ms = next_seg.duration(Run::GAME).to_ms + skipped_seg.gametime_shortest_duration_ms = [skipped_seg, next_seg].map { |s| s.shortest_duration(Run::GAME).to_ms }.compact.sum(0) + + case timing + when Run::REAL + skipped_seg.realtime_reduced = true + when Run::GAME + skipped_seg.gametime_reduced = true + end + + segs + else + segs + [next_seg] + end + end.push(*in_progress_segments) + end end def skipped_splits(timing) diff --git a/app/models/entry.rb b/app/models/entry.rb index f8b61b063..229348196 100644 --- a/app/models/entry.rb +++ b/app/models/entry.rb @@ -1,6 +1,15 @@ class Entry < ApplicationRecord belongs_to :race, touch: true - belongs_to :runner, class_name: 'User', optional: true + # TODO: RacesController#set_race uses FriendlyUUID to find the record, + # which currently has a bug preventing it from being found when using .includes. + # We should either fix that bug, + # or at least when we upgrade to Rails 7, + # update that method to override strict_loading + # (.strict_loading!(false)) + # so we can remove the override from the entire association here. + # + # See: https://github.com/glacials/friendly_uuid/issues/16 + belongs_to :runner, class_name: 'User', optional: true, strict_loading: false belongs_to :creator, class_name: 'User' belongs_to :run, dependent: :destroy, optional: true diff --git a/app/models/race.rb b/app/models/race.rb index 0117828eb..1095d39ab 100644 --- a/app/models/race.rb +++ b/app/models/race.rb @@ -7,13 +7,41 @@ class Race < ApplicationRecord belongs_to :user - belongs_to :game, optional: true - belongs_to :category, optional: true + # TODO: Race.unfinished uses a union query, + # which does not support calls to .includes. + # When we upgrade to Rails 7, + # update that method to override strict_loading + # (.strict_loading!(false)) + # so we can remove the override from the entire association here. + belongs_to :game, optional: true, strict_loading: false + # TODO: Race.unfinished uses a union query, + # which does not support calls to .includes. + # When we upgrade to Rails 7, + # update that method to override strict_loading + # (.strict_loading!(false)) + # so we can remove the override from the entire association here. + belongs_to :category, optional: true, strict_loading: false enum visibility: { public: 0, invite_only: 1, secret: 2 }, _suffix: true - belongs_to :owner, foreign_key: :user_id, class_name: "User" - has_many :entries, dependent: :destroy + # TODO: Race.unfinished uses a union query, + # which does not support calls to .includes. + # When we upgrade to Rails 7, + # update that method to override strict_loading + # (.strict_loading!(false)) + # so we can remove the override from the entire association here. + belongs_to :owner, foreign_key: :user_id, class_name: "User", strict_loading: false + + # TODO: RacesController#create renders the created race as JSON, + # but it is impossible + # (and nonsensical)) + # to preload associations for a race just created. + # When we upgrade to Rails 7, + # update that method to override strict_loading + # (.strict_loading!(false)) + # so we can remove the override from the entire association here. + has_many :entries, dependent: :destroy, strict_loading: false + has_many :runners, through: :entries has_many :chat_messages, dependent: :destroy diff --git a/app/models/user.rb b/app/models/user.rb index 53c2cb7dd..47b2d8de8 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -4,6 +4,13 @@ class User < ApplicationRecord include RivalUser include RunnerUser + # current_user is controlled by Authie and is difficult to inject .includes calls into. + # Relevant issue: https://github.com/adamcooke/authie/issues/42 + # + # Can also turn this off when upgrading to Rails 7, + # which allows calling .strict_loading!(false) to bypass the requirement on individual record objects. + self.strict_loading_by_default = false + has_many :runs, dependent: :nullify has_many :categories, -> { distinct }, through: :runs has_many :games, -> { distinct }, through: :runs @@ -81,9 +88,9 @@ def to_param def pb_for(timing, category) case timing when Run::REAL - runs.where(category: category).order(realtime_duration_ms: :asc).first + runs.includes(:segments).where(category: category).order(realtime_duration_ms: :asc).first when Run::GAME - runs.where(category: category).order(gametime_duration_ms: :asc).first + runs.includes(:segments).where(category: category).order(gametime_duration_ms: :asc).first end end diff --git a/app/validators/entry_validator.rb b/app/validators/entry_validator.rb index cea3ec136..4e53ae945 100644 --- a/app/validators/entry_validator.rb +++ b/app/validators/entry_validator.rb @@ -7,7 +7,7 @@ def validate(record) validate_pre_start_race(record) unless record.race.started? validate_times(record) validate_in_progress_race(record) if record.race.in_progress? - record.errors[:base] << 'Cannot modify entry in finished race' if record.race.finished? + record.errors.add(:base, 'Cannot modify entry in finished race') if record.race.finished? end private @@ -22,18 +22,18 @@ def validate_run_id(record) readied_at: Time.now.utc ) rescue ActiveRecord::RecordNotFound - record.errors[:run_id] << 'No run with that ID exists' + record.errors.add(:run_id, 'No run with that ID exists') end def validate_new_record(record) # Reject new entry if race has already started if record.race.started? - record.errors[:base] << 'Cannot join race that has already started' + record.errors.add(:base, 'Cannot join race that has already started') end # Reject if entry's user is in another active race if !record.ghost? && record.runner.in_race? - record.errors[:base] << 'Cannot join more than one race at a time' + record.errors.add(:base, 'Cannot join more than one race at a time') end end @@ -41,7 +41,7 @@ def validate_new_record(record) def validate_pre_start_race(record) return unless record.finished_at_changed? || record.forfeited_at_changed? - record.errors[:base] << 'Cannot finish/forfeit before a race starts' + record.errors.add(:base, 'Cannot finish/forfeit before a race starts') end # Rejects times before the race start time @@ -51,19 +51,19 @@ def validate_times(record) [record.finished_at, record.forfeited_at].each do |time| next unless time.present? && time < record.race.started_at - record.errors[:base] << "#{time} cannot be before race start time" + record.errors.add(:base, "#{time} cannot be before race start time") end end def validate_in_progress_race(record) # Reject changing ready time after race has started if record.readied_at_changed? - record.errors[:base] << 'Cannot change ready status once race has started' + record.errors.add(:base, 'Cannot change ready status once race has started') end # Reject both finished_at and forfeited_at being set if record.finished_at.present? && record.forfeited_at.present? - record.errors[:base] << 'Cannot finish and forfeit from the same race' + record.errors.add(:base, 'Cannot finish and forfeit from the same race') end end end diff --git a/app/validators/video_validator.rb b/app/validators/video_validator.rb index fd5c10970..33b53cdd3 100644 --- a/app/validators/video_validator.rb +++ b/app/validators/video_validator.rb @@ -17,7 +17,7 @@ def validate_url(record) end unless valid_domain?(record.url) - record.errors.add 'Your video URL must be a link to a Twitch or YouTube video.' + record.errors.add(:url, 'must be a link to a Twitch or YouTube video') end # Embeds break for URLs like https://www.twitch.tv/videos/29447340?filter=highlights&sort=time, which is what Twitch @@ -26,7 +26,7 @@ def validate_url(record) record.url = URI(record.url).tap { |u| u.query = nil }.to_s end rescue URI::InvalidURIError - record.errors.add 'Your video URL must be a link to a Twitch or YouTube video.' + record.errors.add(:url, 'must be a link to a Twitch or YouTube video') end def valid_domain?(url) diff --git a/app/views/layouts/application.slim b/app/views/layouts/application.slim index ed01c9e24..22cf804ec 100644 --- a/app/views/layouts/application.slim +++ b/app/views/layouts/application.slim @@ -120,7 +120,7 @@ html lang='en' small: #hide-survey-button.text-secondary style='cursor: pointer' no thanks a#survey-button.btn.btn-outline-warning.btn-block.mb-0.text-center href=survey_url ' Help us out by taking the Splits.io survey! - - entry = current_user&.entries&.nonghosts&.active&.first + - entry = current_user&.entries&.includes(:race)&.nonghosts&.active&.first - if entry.present? && request.path != race_path(entry.race) .p-1.col-md-6.mx-auto a.btn.btn-success.btn-block.mb-0.text-center.glow href=race_path(entry.race) diff --git a/app/views/runs/_compare_button.slim b/app/views/runs/_compare_button.slim index d3fd85122..24f09a5df 100644 --- a/app/views/runs/_compare_button.slim +++ b/app/views/runs/_compare_button.slim @@ -14,7 +14,7 @@ ' Compare .dropdown-menu aria={labelledby: 'compare'} style='height: auto; max-height: 400px; overflow-x: hidden' .dropdown-header Mine - - comparable_runs = current_user.present? ? current_user.comparable_runs(timing, run).where.not(id: run.id) : [] + - comparable_runs = current_user.present? ? current_user.comparable_runs(timing, run).where.not(id: run.id).includes(:segments, :video) : [] - if comparable_runs.none? .dropdown-item.disabled: i No comparable runs by me - comparable_runs.each do |comparable_run| diff --git a/app/views/runs/_title.slim b/app/views/runs/_title.slim index 22f0dd771..f29237613 100644 --- a/app/views/runs/_title.slim +++ b/app/views/runs/_title.slim @@ -7,7 +7,7 @@ h5 => icon('fas', 'check') span Personal Best span.mr-2 = render 'runs/timing_badge', run: run, timing: timing - - old_runs = run.user.non_pbs([run.category_id]).group_by(&:category_id)[run.category_id]&.sort_by(&:created_at)&.reverse if run.user + - old_runs = run.user.non_pbs([run.category_id]).includes(:segments).group_by(&:category_id)[run.category_id]&.sort_by(&:created_at)&.reverse if run.user - if old_runs.present? span.dropdown-toggle.mr-2.cursor-pointer( content='Previous times' diff --git a/config/application.rb b/config/application.rb index 94cda5019..031118023 100644 --- a/config/application.rb +++ b/config/application.rb @@ -38,5 +38,9 @@ class Application < Rails::Application config.action_cable.mount_path = "/api/cable" config.active_storage.variant_processor = :vips + + # Help detect n+1 queries, + # but get out of the way in the Rails console. + config.active_record.strict_loading_by_default = !Rails.const_defined?('Console') end end diff --git a/config/environments/production.rb b/config/environments/production.rb index d33996d83..52638a525 100644 --- a/config/environments/production.rb +++ b/config/environments/production.rb @@ -122,4 +122,8 @@ config.stripe.secret_key = ENV['STRIPE_PUBLISHABLE_KEY'] config.stripe.publishable_key = ENV['STRIPE_SECRET_KEY'] + + # For strict loading violations, don't error; only log. + # See config/application.rb for the config.active_record.strict_loading_by_default setting. + config.active_record.action_on_strict_loading_violation = :log end