Page MenuHomePhabricator

Implement differential.revision.search
Closed, ResolvedPublic

Description

Some background: I'm an engineer on @yelirekim's team at Uber ATC and I'm trying to solve the pain point of applying bulk patches. arc patch works great when you know the revision id, but it's become quite common for individuals to pull down a set of revisions based on some search criteria so that they can perform more thorough testing locally on their machines before accepting them. The workflow looks something like:

  • search for all open revisions from Project X
  • select a subset of those revisions
  • patch each one (anywhere from 1 to a dozen)

I'm building a graphical tool that automates this process. For the revision searching aspect, Conduit's differential.query method gets me pretty close, but it lacks the projectPHIDs param. Adding support for that would solve my needs, though a differential.search or a more general 'advanced search' api method (that accepts document type) may be more appropriate. Do you think this kind of API change is something you could support, or do you have any other suggestions on how best to achieve this? Thanks!

Event Timeline

timhirsh created this task.Jun 9 2016, 3:47 PM
epriestley renamed this task from Search for revisions by project via conduit to Implement differential.revision.search.Jun 9 2016, 3:51 PM
epriestley triaged this task as Normal priority.
epriestley added projects: Differential, Conduit.

You essentially only need the revision IDs in the response, right?

I can give you a good-enough version of this with that information pretty easily. Fleshing it out in the longer run may take more thought/work but the basic skeleton is straightforward.

The only reason not to do this is that it will make it slightly harder for us to move arc to differential.revision.search in the future because we'll likely need to both detect its existence and detect that it has all the stuff we later add so that arc has everything it needs. However, that's not particularly difficult and not a meaningful reason to hold this.

Actually, this is slightly more involved because you want to select open revisions.

Currently, revision status is stored internally as an integer constant, and implemented in the SearchEngine as a dropdown of hard-codes which expands into internal integer constants. These approaches are both outdated and API-hostile: we'd rather give callers meaningful constants like "open" as an API than meaningless integers like "2", and the dropdown-into-hardcodes makes selecting certain state filters impossible.

Until that is cleaned up, you may be able to execute a custom query for "open" revisions, grab the key from the URI, provide that as a the "queryKey", layer your project constraints on top of that, and get the desired results. We can fix this more broadly but it will take more work, bleeds into arc changes, and gets tangled with possible new states in T2543.

Anyway, let me know what you need beyond what D16089 provides and we can figure out how to move forward. I'll push it here in a minute if you want to poke at it.

Awesome, thanks for the quick turnaround. This looks good to me. Currently I'm using revision ID, title, status, and authorPHID in the response for display purposes. I'll try your recommendation for 'open' revisions using the queryKey to handle the status.

I return the other fields, but don't currently return "status" because I'd have to return an internal numeric constant. So we may need to fix that more thoroughly before you can move forward.

(As a real gross workaround you may be able to make several queries, one for each status, then infer the status based on which query you executed. This will be slow/horrible, though.)

I was able to filter by status using the queryKey, which is much more valuable than displaying the status. So omitting status from the response is ok by me considering the level of effort required to refactor the numeric constants. Thanks again, much appreciated!

epriestley closed this task as Resolved.Jun 9 2016, 5:39 PM
epriestley claimed this task.

Sounds good. I'll call this done for now, let us know if you run into issues.