Page MenuHomePhabricator

Fix an issue where file queries would throw incorrectly
ClosedPublic

Authored by epriestley on Mar 12 2014, 2:16 PM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Apr 24, 10:27 PM
Unknown Object (File)
Wed, Apr 24, 6:42 AM
Unknown Object (File)
Wed, Apr 24, 6:42 AM
Unknown Object (File)
Wed, Apr 24, 6:42 AM
Unknown Object (File)
Fri, Apr 19, 3:32 AM
Unknown Object (File)
Fri, Apr 19, 3:32 AM
Unknown Object (File)
Fri, Apr 19, 3:32 AM
Unknown Object (File)
Tue, Apr 16, 9:28 AM

Details

Summary

Ref T4589. When you look at a file, we load attached objects in order to run the "you can see this if you can see any attached object" policy check.

However, right now the subquery inherits the "throw on filter" flag from the parent query. This inheritance makes sense in other cases[1], but because this is an "ANY" rule it does not make sense here. In practice, it means that if the file is attached to several objects, and any of them gets filtered, you can not see the file.

Instead, explicitly drop the flag for this subquery.

[1] Sort of. It doesn't produce wrong results in other cases, but now that I think about it might produce a less-tailored error than it could. I'll look into this the next time I'm poking around.

Test Plan
  • Viewed an "All Users" file attached to a private Mock.
  • Prior to this patch, I incorrectly received an exception when the Mock was loaded. This is wrong; I should be able to see the file because the policy is "All Users".
  • After the patch, I can correctly view the file, just not the associated mock.

Screen_Shot_2014-03-12_at_6.55.17_AM.png (554×1 px, 132 KB)

Diff Detail

Repository
rP Phabricator
Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

epriestley retitled this revision from to Fix an issue where file queries would throw incorrectly.
epriestley updated this object.
epriestley edited the test plan for this revision. (Show Details)
epriestley added a reviewer: btrahan.
btrahan edited edge metadata.
This revision is now accepted and ready to land.Mar 12 2014, 4:18 PM

@epriestley, this one's ready to land, do you have any reason to hold it back?

Because of this bug, the effective, day-to-day behavior of file policies in T4589 is generally closer to the desired behavior than without it.

That is, this bug mostly makes files that you shouldn't be able to see unviewable. It incorrectly makes a small number of files you should be able to see unviewable, but the aggregate behavior is probably closer to the desired behavior than it is with this fixed. Under that theory, I'm tentatively holding it until T4589 gets fixed more properly.

epriestley updated this revision to Diff 24369.

Closed by commit rPc9fe16247005 (authored by @epriestley).