Page MenuHomePhabricator

Remove "Effective User" attachment from Repository Identities
ClosedPublic

Authored by epriestley on Feb 28 2019, 4:30 PM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Dec 25, 7:55 AM
Unknown Object (File)
Fri, Dec 20, 7:46 PM
Unknown Object (File)
Thu, Dec 19, 8:44 PM
Unknown Object (File)
Fri, Dec 13, 8:56 PM
Unknown Object (File)
Sun, Dec 8, 6:59 AM
Unknown Object (File)
Sun, Dec 8, 2:27 AM
Unknown Object (File)
Tue, Dec 3, 8:57 AM
Unknown Object (File)
Sat, Nov 30, 11:28 PM
Subscribers
None

Details

Summary

See https://discourse.phabricator-community.org/t/phabricatorrepositoryidentity-attacheffectiveuser-must-be-an-instance-of-phabricatoruser-null-given/1820/3.

It's possible for an Idenitity to be bound to user X, then for that user to be deleted, e.g. with bin/remove destroy X. In this case, we'll fail to load/attach the user to the identity.

Currently, we don't actually use this anywhere, so just stop loading it. Once we start using it (if we ever do), we could figure out what an appropriate policy is in this case.

Test Plan

Browsed around Diffusion, grepped for affected "effective user" methods and found no callsites.

Diff Detail

Repository
rP Phabricator
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

(Even if we do need this eventually, we clearly don't need it terribly often, so putting it behind a needEffectiveUsers(true) check would probably make sense.)

(I'll also make bin/remove destroy clean up this table in a followup.)

This turns out to be a bit tricky and doesn't really do anything for us today, so I'll just note it on T12164.

This is tricky because destroying a user may possibly require us to update an arbitrary number of RepositoryIdentity objects, and the most correct update is to rebuild the identity completely (i.e., recompute the "guess"). I think we'd have to queue some stuff into the task queue to make this scalable in the general case, so a fully general version of this requires a fair bit of code shuffling.

This revision is now accepted and ready to land.Mar 5 2019, 7:30 PM
This revision was automatically updated to reflect the committed changes.