Page MenuHomePhabricator

Add repositories to Diviner
ClosedPublic

Authored by joshuaspence on May 30 2015, 12:08 AM.

Details

Summary

Fixes T8352. Associate Diviner books and atoms with a repository. This relationship is not really surfaced anywhere in the UI but provides metadata that contextualises search results. Depends on D13091.

Test Plan

Ran diviner generate --repository ARC and then went to /diviner/book/arcanist/.

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

There are a very large number of changes, so older changes are hidden. Show Older Changes
joshuaspence retitled this revision from to Add repositories to Diviner.May 30 2015, 12:08 AM
joshuaspence updated this object.
joshuaspence edited the test plan for this revision. (Show Details)
joshuaspence added a reviewer: epriestley.
epriestley requested changes to this revision.May 31 2015, 2:24 PM
epriestley edited edge metadata.

Per T8352, I think this should be nullable.

src/applications/diviner/query/DivinerBookQuery.php
33–36

I think we should always load these and perform policy filtering; if you can't see a repository, you should not be able to see its books (similar to how revisions work).

(This might create some issues with generating public documentation from private repositories, but I don't think that's a major use case and it shouldn't be the default behavior.)

58–60

For consistency, do this in willFilterPage().

131

Prefer !== null, so setRepositoryPHIDs($phids) is an error when $phids is array(), instead of an accidental query for all results.

161

If the repository fails to load, I think we should filter the book out (i.e., same deal as revisions: if you can't see a repository, you can't see its stuff).

src/applications/diviner/workflow/DivinerGenerateWorkflow.php
28–32

I think this should be in the .book file instead, possibly with an optional flag to override it. But bin/diviner generate shouldn't need flags to do the right thing, and you should theoretically be able to generate multiple books across different repositories with one command.

This revision now requires changes to proceed.May 31 2015, 2:24 PM
joshuaspence marked 4 inline comments as done.Jun 1 2015, 12:20 AM
joshuaspence added inline comments.
src/applications/diviner/workflow/DivinerGenerateWorkflow.php
28–32

Yeah, this was just easier for to me implement for now... I was thinking that we should try to determine the repository using the following methods:

  1. From the command line, if specified.
  2. From the .book file.
  3. Automatically from the working copy.
joshuaspence added inline comments.Jun 1 2015, 12:24 AM
src/applications/diviner/controller/DivinerBookController.php
47–53

Not sure if this is valuable/necessary... I primarily added it so that I could verify that this was working.

src/applications/diviner/publisher/DivinerLivePublisher.php
7–45

This seems really gross and I might try to clean it up.

18–19

Maybe we should set this to the view policy of the repository?

53

This is unrelated, but I think that this was wrong originally?

joshuaspence edited edge metadata.
  • Allow repositoryPHID to be nullable
  • Always load repositories
  • Move repository loading to willFilterPage
  • Use $repositoryPHID !== null
  • Filter books that cannot be viewed due to policies
joshuaspence edited edge metadata.

Delete books when the underlying repository is deleted

  • Fix indentation
  • Tidy up DivinerLivePublisher::loadBook method
  • Set book view policy from repository
  • Rename DivinerLivePublisher::loadBook to DivinerLivePublisher::getBook
  • Allow repository to be specified in book file
epriestley accepted this revision.Jun 1 2015, 1:31 AM
epriestley edited edge metadata.

Couple of inlines but this looks good to me.

src/applications/diviner/publisher/DivinerLivePublisher.php
15–20

We should just enforce it implicitly, like with Differential revisions.

The describeAutomaticCapability() method of DivinerLiveBook should be updated to explain this, though (see DifferentialRevision).

53

This looks wrong now, since it passes a Book object to a with...PHIDs() method?

(The original code seems more wrong, but I think there's still a getPHID() missing.)

src/applications/diviner/query/DivinerBookQuery.php
69

This likely needs to $book->attachRepository(null) to avoid throwing when loading books with no repository and later calling getRepository() on them.

(That will imply a corresponding PhabricatorRepository $repository = null default parameter value in attachRepository().)

src/applications/diviner/storage/DivinerLiveBook.php
28

I'd expect this to need a 'repositoryPHID' => 'phid?' to avoid a bin/storage upgrade warning about column nullability.

src/applications/repository/storage/PhabricatorRepository.php
1204

This should be $engine->destroyObject($book) and be fired from destroyObjectPermanently(), not delete().

This revision is now accepted and ready to land.Jun 1 2015, 1:31 AM

Your comments seem accurate, I'm heading out shortly but will fix this up later. I will probably bounce back to you for review since I haven't touched some of this code before.

src/applications/diviner/publisher/DivinerLivePublisher.php
15–20

And then not set a view policy on the DivinerLiveBook at all?

53

Oh you're right about the missing getPHID. It's confusing because some classes (such as DivinerAtom) have getBook() and setBook() methods which handle PHIDs rather than book objects.

joshuaspence marked 9 inline comments as done.
joshuaspence edited edge metadata.

As requested

joshuaspence added inline comments.Jun 1 2015, 6:09 AM
src/applications/diviner/publisher/DivinerLivePublisher.php
53

Actually yeah... in the DivinerAtom class, $book is a PHID, not an instance of DivinerLiveBook.

Add repositoryPHID to DivinerLiveSymbol

After D13091 it will be possible to add repositories from the UI as well.

joshuaspence marked 2 inline comments as done.
  • Remove --repository argument from DivinerGenerateWorkflow
  • Rename a variable
  • Allow repository to be null
joshuaspence requested a review of this revision.Jun 1 2015, 12:33 PM
joshuaspence edited edge metadata.

Pushing this back for review... a couple of questions:

  1. Should I attempt to automatically determine the repository from the working copy?
  2. Should repositoryPHID be an attribute of DivinerLiveAtom or DivinerLiveSymbol?
  3. Did I do the automatic capabilities properly? Not sure how to test this.
src/applications/diviner/workflow/DivinerGenerateWorkflow.php
28–32

I'm actually not sure about this anymore... in D13091 I add the functionality to change the repository from the web UI, but maybe this doesn't make sense? I think that, ideally, the repository would be automatically determined from the working copy. If that doesn't work then I guess we could maybe allow the repository to be specified in the .book file. Is this sufficient? Do we need any further customizations?

joshuaspence updated this object.Jun 1 2015, 1:19 PM
joshuaspence edited edge metadata.

Should I attempt to automatically determine the repository from the working copy?

I probably wouldn't bother for now. I think the code Arcanist uses to do this is nontrivial, and we might get it free later if more of this ends up in Arcanist.

Should repositoryPHID be an attribute of DivinerLiveAtom or DivinerLiveSymbol?

It should be a property of DivinerLiveSymbol (so we can query for it -- all the user-facing query UI operates on Symbols). For technical/internal reasons, it might also end up somewhere on LiveAtom (either as a property or in a JSON blob somewhere), but that would just be to make getting it onto Symbol easier.

Did I do the automatic capabilities properly? Not sure how to test this.

It's correct as written, or you could just remove it for now. Lengthy reasoning and then relevant discussion:

"Automatic Capabilities" are a formal concept which allow users with a strong connection to an object (like a task owner, or revision author) to always have a capability, even if the policy says they don't. For example, a revision with "Edit Policy: No One" really means "Edit Policy: No One (Except the Author)". Automatic capabilities always weaken policies, and let users who do not pass policy checks take actions because of a strong relationship to the object.

"Implicit Policies" aren't really formal right now, although there's a reasonable chance that they'll become formal in order to fix T7703 (maybe as "Extended Policies"). For some kinds of objects, you need to be able to see or edit some related objects in order to see or edit a target object. For example, in Differential, you must be able to see the repository a revision is associated with in order to see the revision. In Phriction, you need view/edit capabilities on all parents of a document. In Phame, you (probably?) need to be able to see a blog in order to see posts on it. In a few other cases there are more infrastructure-level implicit capabilities: for example, you must be able to see an event in order to see its invitees.

Currently, these are implemented as (a) a "load, attach, discard records with no corresponding object" step in the Query, and (b) a human-readable note in describeAutomaticCapability(). This is somewhat confusing because the note isn't describing an automatic capability at all, but it puts the information in the right place in the UI and it wasn't a case I was thinking about when I wrote the method. A better name for this method might just be describeCapability() or describeSpecialRulesForCapability().

After T7703, they will probably also be implemented as an explicit step in PolicyFilter. Specifically, we would probably add some method like this after T7703:

public function getExtendedPolicyRequirements($capability) {
  return array(
    // Some datastructure which means "you must be able to
    // see the repository to see the book".
  );
}

These implicit policies always strengthen policies: they prevent users who would otherwise pass the policy check from taking actions.

After T3820, Spaces will also essentially put an implicit policy layer on top of all objects. So policy evaluation will roughly be:

(<Viewer Can See Space> AND <Viewer Can See Implicit Related Objects> AND (<Viewer Satisfies Policy> OR <Viewer Has Automatic Capability>))

So, in this specific case, the automatic capability implementation is correct, but it's also moot, and it would be equally correct if removed: repositories do not have any automatic capabilities, because no users have an exceptionally strong relationship to repositories: they aren't currently "owned" objects. If we gave some users a strong relationship to repositories in the future, it might or might not make sense for that relationship to also imply an automatic capability on books.

For example, if the relationship was "ownership", it might be reasonable to say that repository owners can always see books in their repository.

If the relationship was something like "permission to build/mirror" to support some kind of Harbormaster thing (I'm just making this up as an example, I can't really think of a good one), it might not make sense for that relationship to extend to books.

I'd maybe slightly favor just removing the code. Although "ownership" is by far the most likely automatic capability we'd add in the future and the code would be correct/necessary if we did, it does nothing today, I don't currently plan to add "ownership", and the code might cause us to apply too liberal a policy if we implement some other kind of automatic capability. Even if we implement "ownership", an explicit test to see if the viewer is a repository owner might be a better fix.

(There's also a third kind of implicit/indirect policy relationship, where completely-dependent sub-objects just directly reflect the policies of their parent objects. An example of this is Changesets in Differential. It never makes sense for a Changeset to have any policy other than the policy of its parent Diff, so the Changeset has no viewPolicy/editPolicy fields and just proxies Diff for the purposes of policy checks. This kind of object is straightforward and not affected by T7703, because it isn't adding any policy checks of its own. In this case, it's appropriate to just call through to the parent/container object.)

src/applications/diviner/publisher/DivinerLivePublisher.php
15–21

Right. In a future diff, the new DivinerLiveBook() can become DivinerLiveBook::initializeNewBook() which would read the default application view/edit policies.

53

Oh, yuck. We should maybe fix this at some point, although that might be involved.

src/applications/diviner/storage/DivinerLiveBook.php
113–114

(See discussion.)

I think it makes sense to leave the automatic capability... it seems logical that access to view the repository implies access to view the documentation.

  • Use DivinerBookQuery
joshuaspence added a parent revision: D13091: Modernize Diviner.
epriestley accepted this revision.Jun 16 2015, 2:12 PM
epriestley edited edge metadata.
epriestley added inline comments.
src/applications/diviner/publisher/DivinerLivePublisher.php
28

We'll have to revisit this at some point if we build arc diviner or similar, but it should be fine for now.

src/applications/diviner/query/DivinerBookQuery.php
61

This will technically work fine, but mpull($books, 'getRepositoryPHID') might return something like array(null, null, null). This might be slightly clearer (although also slightly longer) with more effort to handle that case.

75–76

Per that recent handle stuff, should didRejectResult() here.

This revision is now accepted and ready to land.Jun 16 2015, 2:12 PM

I had a thought... we could use the phabricator_repository.repository_path and phabricator_repository.repository_pathchange tables in order to determine the repository based on the path to the book. Thoughts?

We could, but that seems like a lot of effort for not much benefit to me. I think the existing mechanisms in arc (primarily, remote URI) are much better.

If you wanted to hack something in now, it's probably a comparable amount of effort to instantiate an Arcanist WorkingCopy object and then directly use the code to resolve the repository than try to do the path stuff (although this might be more complicated than I believe).

I'll defer until a later diff... I'm just going to move it to an argument (diviner generate --repository $callsign) for now.

joshuaspence edited edge metadata.

Almost completely working. The only issue at the moment is that running diviner generate --repository X does not update the repository for the Diviner atoms if they had already previously been published.

Almost completely working. The only issue at the moment is that running diviner generate --repository X does not update the repository for the Diviner atoms if they had already previously been published.

@epriestley, do you have any thoughts on how to go about this? I was thinking of just adding queryfx($conn_w, 'UPDATE %T SET repositoryPHID = %s WHERE bookPHID = %s', id(new DivinerLiveSymbol())->getTableName(), $book->getRepositoryPHID(), $book->getPHID()) to DivinerLivePublisher::loadBook(), but that's pretty gross.

Somewhat hacky way to set repository for atoms

joshuaspence added inline comments.Jun 19 2015, 7:54 AM
src/applications/diviner/controller/DivinerBookEditController.php
85–87

This seems really gross

Final touches

This revision was automatically updated to reflect the committed changes.
epriestley added inline comments.Jun 19 2015, 12:40 PM
src/applications/diviner/controller/DivinerBookEditController.php
85–87

Yeah, we end up doing this in a fair number of places.

It would probably be OK to just let you setValue() a string and convert it into an array (only if the limit is 1?), or maybe add setSingleValue()? We could clean up a few things.

For the atom stuff, I thiiink there's probably some global/book cachekey we can put the repo PHID into to automatically invalidate everything, but we might need to add that. Hack fix seems fine for now.