Page MenuHomePhorge

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

Authored by aklapper on Dec 25 2023, 21:20.
Tags
None
Referenced Files
Unknown Object (File)
Thu, Apr 25, 22:32
Unknown Object (File)
Thu, Apr 25, 22:32
Unknown Object (File)
Thu, Apr 25, 22:25
Unknown Object (File)
Thu, Apr 25, 22:25
Unknown Object (File)
Thu, Apr 25, 17:00
Unknown Object (File)
Mon, Apr 22, 21:00
Unknown Object (File)
Mon, Apr 22, 14:54
Unknown Object (File)
Mon, Apr 22, 06:24

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
T15642transactionQueryOverheat (branched from master)
Lint
Lint Warnings
SeverityLocationCodeMessage
Warningsrc/applications/base/PhabricatorApplication.php:489XHP25Spaces Inside Parentheses
Warningsrc/applications/base/PhabricatorApplication.php:490XHP25Spaces Inside Parentheses
Warningsrc/applications/feed/query/PhabricatorFeedTransactionQuery.php:165TXT3Line Too Long
Unit
Tests Passed
Build Status
Buildable 1109
Build 1109: 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