Page MenuHomePhorge

Feed: Do not query and display data of uninstalled applications
AcceptedPublic

Authored by aklapper on Dec 25 2023, 21:20.

Details

Summary

Make it less likely to run into a "Query overheated" error in the Feed Transaction Log by not querying data of applications that got uninstalled and thus are not accessible anymore anyway.

Mitigates T15642

Test Plan
  • Install D25500
  • Create 900 tasks with a View Policy set to User1
  • As an admin, uninstall Maniphest via /applications/view/PhabricatorManiphestApplication/
  • Log into Phorge as User2; go to /feed/transactions/query/all/ and see no "Query overheated" anymore due to no Maniphest task activity listed in transaction feed query results anymore
  • Set "Can Use Application" on /applications/view/PhabricatorManiphestApplication/ to Administrators
  • Perform steps above as non-admin

Diff Detail

Repository
rP Phorge
Branch
arcpatch-D25501
Lint
Lint Passed
Unit
Tests Passed
Build Status
Buildable 1214
Build 1214: arc lint + arc unit

Event Timeline

aklapper edited the test plan for this revision. (Show Details)

There might be room for performance improvements - currently for each $query, PhabricatorApplication::isClassInstalledForViewer is called again though some queries share the same application.

This also needs rework because I still get a Query overheated when not being logged in and going to http://phorge.localhost/feed/transactions/query/all/

20after4 added inline comments.
src/applications/feed/query/PhabricatorFeedTransactionQuery.php
165

isClassInstalledForViewer already does some caching so this shouldn't be terribly inefficient.

This also needs rework because I still get a Query overheated when not being logged in and going to http://phorge.localhost/feed/transactions/query/all/

Are there any feed transactions that are visible to logged out users? Maybe that feed page should just 404 when logged out?

Are there any feed transactions that are visible to logged out users?

Yes, basically all. See https://we.phorge.it/feed/transactions/query/advanced/ in a private browser window

Fix D25501#14443: Do not show an "Query overheated" error when the user is anonymous and the application has been uninstalled

In what case, a class can be installed "for the viewer", but generally uninstalled?

Edited: sorry I get this. I think that application "Visible to: (Registered) Users" triggers the "not installed".

Right. Sorry, my previous comment was very misleading!

It makes sense thanks. Minor note:

If the application is not visible (false), there is no need to also check for isClassInstalled() (that may return true). This can also lead to a situation where the app is not visible to that user, but the query is shown.

Maybe we can simplify things introducing a commodity "if viewer exists" method like:

// Remove TransactionQuery classes of uninstalled apps. Increases query
// performance and decreases likeliness of an "Query Overheated" error if
// an app got uninstalled so data in it cannot be accessed anymore anyway.
// See https://secure.phabricator.com/T13133, https://we.phorge.it/T15642
foreach ($queries as $key => $query) {
  $app = $query->getQueryApplicationClass();
  if ($app !== null && PhabricatorApplication::isClassInstalledForViewerIfAny($app, $viewer)) {
    unset($queries[$key]);
  }
}
PhabricatorApplication
final public static isClassInstalledForViewerIfAny(
  $class,
  PhabricatorUser $viewer) {

  return $viewer
    ? self::isClassInstalledForViewer( $class, $viewer )
    : self::isClassInstalled( $class );
}

Maybe we can simplify things introducing a commodity "if viewer exists" method like:

I like that idea.

Evaluate the suggested refactor just for readability reasons :) fine also without that.

This revision is now accepted and ready to land.Mar 4 2024, 09:17

Break long line into two