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)
Thu, Mar 28, 9:18 AM
Unknown Object (File)
Mon, Mar 4, 10:14 PM
Unknown Object (File)
Feb 3 2024, 9:33 PM
Unknown Object (File)
Dec 29 2023, 1:55 PM
Unknown Object (File)
Dec 28 2023, 11:55 PM
Unknown Object (File)
Dec 28 2023, 5:28 PM
Unknown Object (File)
Dec 24 2023, 5:23 PM
Unknown Object (File)
Dec 22 2023, 1:46 AM
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
Branch
effectiveuser1
Lint
Lint Passed
Unit
Tests Passed
Build Status
Buildable 22146
Build 30261: Run Core Tests
Build 30260: arc lint + arc unit

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.