There are two related issues around how Query classes throw policy exceptions.

The normal behavior is that executeOne() rises policy exceptions, while execute() does not.

That is, if you executeOne() (load a single object) and the object isn't visible to you because of policy evaluation, we normally raise a policy exception. For example, you visit /D123 but can't see that revision. You get a "You Shall Not Pass" page.

If you execute() (load a page of results) and some objects aren't visible to you because of policy evaluation, they are skipped. For example, you visit /differential/ and can't see some revisions. We show you the revisions you can see.

Regardless of context, either behavior (raising or not raising) is generally correct from a purely technical perspective: neither generally has security implications of violates policy rules or anything. This is mostly a product decision aimed at giving users the most useful/helpful product behavior. The default behaviors (exception on executeOne(), filter on execute()) are almost always the correct product behavior.

However, there are a couple of cases where our behavior isn't as good as it could be:

Spaces: See PHI1031. If you executeOne() and an object is filtered because of a Space rule, we don't raise a policy exception. This is technical: we do the filtering in the query, so we can't distinguish between "object does not exist" and "object can not be seen".

Probably, we should skip the database filtering if the query is raising policy exceptions and do only application filtering. This would let us raise a normal policy exception. The database filtering is more efficient so it's possible this will create some performance issues in some obscure part of the application doing some kind of bizarre weird thing, but they're likely easy to fix if they do crop up. In almost all cases where policy exceptions are enabled, the query selects only one result anyway, so it is not important to do database filtering.

We can continue doing database filtering when we are not raising policy exceptions.

Disabling Policy Filtering With executeOne(): Currently, executeOne() causes us to raise policy exceptions unconditionally: there is no way to disable it. Specifically executeOne() overrides setRaisePolicyExceptions().

This leads to these three "patterns":

try {
  $result = $query->executeOne();
} catch (PhabricatorPolicyException $ex) {
  $result = null;
}

$results = $query->setLimit(1)->execute();
$result = head($results);

// Does not work!
$query->setShouldRaisePolicyExceptions(false)->executeOne();

Fixing this is slightly involved because the flag setting is propagated down the stack. If we change the behavior to "raise policy exceptions, unless a caller has called setRaisePolicyExceptions(), in which case do that", we might cause behavioral changes when executeOne() [no policy exceptions] calls invoke query() calls that invoke executeOne() calls. The old behavior for the third-level call would be "raise"; the new behavior would be "do not raise".

We can fix this with an internal setDefaultPolicyExceptionBehavior(), or an executeOneWithNoPolicyExceptions().

A bigger part of resolving this is that these callsites are hard to identify since they can't easily be found with grep. But, e.g., PhabricatorPackagesVersionPackageTransaction has an example of "Antipattern 3" even though this pattern does not actually work.

I also believe this is a very minor problem and that we have fewer than 10 total callsites affected by it, it would just be nice to have an unambiguously correct approach when this does crop up.