Page MenuHomePhabricator

Remove "Effective User" attachment from Repository Identities
ClosedPublic

Authored by epriestley on Feb 28 2019, 4:30 PM.

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
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

epriestley created this revision.Feb 28 2019, 4:30 PM

(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.)

epriestley requested review of this revision.Feb 28 2019, 4:32 PM
epriestley edited the summary of this revision. (Show Details)Feb 28 2019, 5:26 PM

(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.

amckinley accepted this revision.Mar 5 2019, 7:30 PM
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.