Page MenuHomePhabricator

Add caching for database queries in ReadingLists HookHandler
Closed, DeclinedPublic3 Estimated Story Points

Description

In the onSkinTemplateNavigation__Universal in ReadingLists HookHandler, there are various database queries run on page views for users with ReadingLists enabled (e.g. to get the default reading list url for a user and number of items in the reading list)

onSkinTemplateNavigation__Universal:

  • getDefaultReadingListUrl - this should very rarely change and should be cached per-user
  • setupForUser - gets list metadata
    • gets the reading list size (from rl_size column) to pass to frontend for metrics collection (in the reading list experiment)
    • reading list id

We should invalidate the relevant caches when a page is added or removed from the reading list (for the reading list size cache or metadata)

In the ReadingListRepository, we could consider using WANObjectCache::getWithSetCallback()


reading list contents:

  • getListsByPage()
    • gets whether a page is in the user's default reading list (whether to show the bookmarkOutline or bookmark icon - save or unsave the page)
    • gets the reading list entry id (for the JS api call to save/unsave)

possibly cache per user:

cache key: readinglists:user_bookmarks:{userId}:{readingListId}

$bookmarkedPages = $cache->getWithSetCallback($cacheKey, 3600, function() use ($repository) {
    return $repository->getUserBookmarkMap($userId);
});

getUserBookmarkMap would be a method in the repository that builds a map (array) with page id -> reading list entry id

$bookmarks[$row->rle_title] = (int)$row->rle_id;

Event Timeline

aude triaged this task as High priority.Sep 22 2025, 7:47 PM
aude moved this task from Incoming to Needs refinement on the Reader Experience Team board.
aude set the point value for this task to 3.Sep 23 2025, 3:01 PM
aude moved this task from Needs refinement to Ready for sprint on the Reader Experience Team board.

Change #1191757 had a related patch set uploaded (by Aude; author: Aude):

[mediawiki/extensions/ReadingLists@master] Add caching for getDefaultListIdForUser

https://gerrit.wikimedia.org/r/1191757

In OutputPage, MediaWiki checks the If-Modified-Since header sent by the browser and compares it with the modified times for the page + user touched + cache epoch setting

$modifiedTimes = [
    'page' => $timestamp,
    'user' => $this->getUser()->getTouched(),
    'epoch' => $config->get( MainConfigNames::CacheEpoch )
];
$maxModified = max( $modifiedTimes );

For the watchlist, the WatchlistManager calls User::invalidateCache to ensure user touched timestamp is updated. this ensures user-specific skin/menu data is updated for logged-in users:

For ReadingLists, we should make sure user touched is updated when some saves or unsaves a page from the default reading list, so that they see correct "bookmark" page saved status.

Change #1192568 had a related patch set uploaded (by Aude; author: Aude):

[mediawiki/extensions/ReadingLists@master] Introduce ReadingListRepositoryFactory

https://gerrit.wikimedia.org/r/1192568

Change #1192600 had a related patch set uploaded (by Aude; author: Aude):

[mediawiki/extensions/ReadingLists@master] Invalidate user cache (touched) for default reading list changes

https://gerrit.wikimedia.org/r/1192600

Change #1191757 abandoned by Aude:

[mediawiki/extensions/ReadingLists@master] Add caching and user cache invalidation for default reading list

Reason:

splitting into smaller patches

https://gerrit.wikimedia.org/r/1191757

SToyofuku-WMF lowered the priority of this task from High to Medium.Oct 8 2025, 6:07 PM

@aude can you help me understand why we need caching here? When I bookmark https://test.wikipedia.org/wiki/Schloss_Reinbek and refresh the page, the bookmark icon continues to be filled. Under what circumstances would we serve an outdated cache?

Change #1192568 merged by jenkins-bot:

[mediawiki/extensions/ReadingLists@master] Introduce ReadingListRepositoryFactory

https://gerrit.wikimedia.org/r/1192568

@aude not super high priority, but when you get a chance can you answer Jon's question above?

Change #1192600 abandoned by Aude:

[mediawiki/extensions/ReadingLists@master] Invalidate user cache (touched) for default reading list changes

https://gerrit.wikimedia.org/r/1192600