Load only the necessary work fields when displaying series

Description

We sometimes load works ordered by their positions in a series like this:

This generates a SQL query that includes every single column from the works table, which is bad for 2 reasons:

  • The query is slower for getting unnecessary data.

  • We run migrations to remove seemingly unused columns from the works table, but existing Rails processes will still try to SELECT those columns, leading to errors on series and works in series. This did happen when we deployed AO3-5905, and we fixed the errors by restarting all Rails processes:

For reference, the errors we got were like:

Instances of this query:

How to test:

  • Check that you can view a series and reorder the works in it without errors.

  • Check that you can view the blurbs of works in a series, with the expected link to the series, and without errors.

  • Check that you can view the meta boxes of works in a series, with the expected links to the series + the next/previous works, and without errors.

Activity

Show:
ticking instant
April 23, 2020, 3:39 AM

Two thoughts:

  • Calling includes(:work) alone doesn’t make rails select both tables in a single query. The joint query is caused when you have includes(:work).references(:works), or includes(:work) combined with some where queries that reference the works table (which causes an implicit call to references(:works)). Neither of these occurs in SeriesController#manage, so I don’t believe it needs to be modified to fix the issue with migrations.

  • The code in the work meta calls Work#visible?, which uses a couple of different fields from the Work object: posted, hidden_by_admin, and restricted. Retrieving the id alone will make the visibility check fail for everyone except admins and the owner of the work, because it’ll mean the Work looks like a draft.

 

redsummernight
April 23, 2020, 3:47 AM

Thanks, I updated the issue. I'm not sure we can fix SeriesController#show, but we can certainly fix the query for work metas.

ticking instant
April 23, 2020, 4:19 AM

It requires more work, rather than less, but … maybe it’d work to either avoid where(works: { posted: true }) and just rely on visible? for the permission/visibility check, or to use joins(:work).preload(:work) instead of includes(:work).

Or just flip the query around a little and do Work.joins(:serial_works).merge(@series.serial_works).select("works.*, serial_works.position AS position”), since I think position is the only field required from serial_works. (And I’m not even sure it’s required.)

Ehryn (Danielle Strong)
June 20, 2020, 7:31 AM

Okay, I’m a little confused on this one but:

  • Created new Series with numbered works

  • Successfully reordered said works

  • While logged in as testy was able to view the blurbs of the series works with the correct links to correct works

  • While logged in as testy was able to view the Series information on the works pages & correctly move through the works in the correct (reordered) order

  • Logged out of testy & was able to view blurbs with correct links & move through Series using the Next/Previous buttons in the correct (reordered) order.

I’m not 100% sure if that’s what was being asked for, but at the very least I can confirm that reordering works?

Runt
June 20, 2020, 12:11 PM

Created a series, was able to see work blurbs and navigate to next/previous works correctly without errors

Reordered the series without errors and was able to correct navigate between works with no errors.

Viewed an older series, correctly able to view series inforamtion and all links worked as expected.

DeployedToBeta

Assignee

ticking instant

Reporter

redsummernight

Roadmap

Series

Priority

Medium

Affects versions

Fix versions

Components

BackEnd

Difficulty

Medium

Milestone

Internal 0.9
Configure