Page MenuHomePhabricator

Correct a datasource issue when viewing repository URIs in "Manage Repository"

Authored by epriestley on Jun 30 2017, 2:01 PM.



Fixes T12884. In cases other than this UI, applications access URIs through the Repository they're part of. This means that applications interact with URIs which have gone through the correction/adjustment logic in PhabricatorRepository->attachURIs(), which fixes up "builtin" URIs to have the right values based on configuration.

In this case (and, as far as I can tell, only this case) we load the URI directly and act on its properties which depend on configuration and repository state.

This can mean we're using a different view of the URI than we should be.

To fix this: after loading the URI, reload it through the repository so the relevant adjustments are applied.

I think this is the most reasonable fix. We could try to make RepositoryURIQuery somehow enforce this, but the cost of this error is small (mild confusion about display state), the other things which do direct loads don't depend on this state (editing), and everything else loads via a repository and is likely to continue doing that forever.

Test Plan

Diff Detail

rP Phabricator
Automatic diff as part of commit; lint not applicable.
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

This revision is now accepted and ready to land.Jun 30 2017, 2:05 PM
This revision was automatically updated to reflect the committed changes.
jmeador added inline comments.

@epriestley I believe this query is resulting in 10 second page loads for us. I haven't tested it, though so I can't be sure. It does look like it is loading every URI for every repository though. Is this intentional? would a constraint on this query break how this fix works?

Yikes! Pretty sure the query is just completely wrong, give me a second and I'll get a fix out.

D19036 should make the cost of this page more reasonable.