Page MenuHomePhabricator
Create Task
Maniphest T13212

Add 'ownerPHIDs' query constraint to 'drydock.lease.search' conduit call
Closed, ResolvedPublic
Actions

Description

An Arcanist workflow downstream requests information on leases visible to the user. The user is only interested in leases for which they are the owner, so Arcanist iterates over the results and returns only the ones with a matching ownerPHID. It's so much cheaper to have the server do this filtering than the client.

The proposed change will be small but necessary for the above feature, and will allow conduit calls to drydock.lease.search to specify an ownerPHIDs array as a constraint. Per the use case above, the workflow will provide the authorized user's PHID as owner, but I'm sure this change opens the door for a lot of people's wishlist features.

Revisions and Commits

rP Phabricator
D16594rP5f3a7cb41b17 Expose Drydock leases via Conduit

Related Objects
Search...

Event Timeline

jwarner triaged this task as Normal priority.Oct 12 2018, 4:27 PM
jwarner created this task.
jwarner updated the task description. (Show Details)
epriestley added a subscriber: epriestley.Oct 16 2018, 2:32 PM

This is entirely reasonable, the only reason it doesn't already exist is that the lease owner may be any of several different types of objects (I think: user, repository, another Drydock object, maybe null?) and we don't have a convenient UI datasource for letting users type to select any one of these various object types.

We can implement this anyway, using a PhabricatorPHIDsSearchField, which will expose it over Conduit in a fully-functional way (at least, for your purposes) and just not render a UI control. At some future date, this can upgrade seamlessly to a DatasourceField with a UserOrRepositoryOrDrydockThingDatasource. This would enable the UI to have a typeahead, and for the UI and API to accept typeahead functions (e.g., viewer()).

(We could also possibly implement it using a PhabricatorPeopleDatasource. This would still accept non-user PHIDs via the API, but would make the UI more functional and enable user-based functions like viewer() and members(#project). However, it wouldn't easily support searching for leases owned by a repository, another Drydock object, etc., in the UI, which might be misleading, and would probably have some display/rendering/behavior issues if you explicitly navigated to /lease/query/?ownerPHIDs=PHID-REPO-xyz.)

The required changes are likely:

  • Add withOwnerPHIDs() to DrydockLeaseQuery.
  • Add an ownerPHIDs field to DrydockLeaseSearchEngine using a PhabricatorPHIDsSearchField field type.

I'm happy to make these changes myself, or you mentioned wanting to contribute a patch?

epriestley added a project: Drydock.Oct 16 2018, 2:32 PM
epriestley added a comment.Oct 24 2018, 12:17 AM

I'm happy to make these changes myself, or you mentioned wanting to contribute a patch?

(I'll aim to get this into the Week 43 release this Friday if nothing materializes before then.)

epriestley mentioned this in T13210: Plans: 2018 Week 41-44 Bonus Content.Oct 25 2018, 11:11 AM
epriestley added a comment.Oct 25 2018, 11:38 AM

Complicating this: there is no drydock.lease.search call upstream. So you're probably running some variation of D16594? But that already has ownerPHIDs.

I'll just upstream some variation of D16594, which is 95% fine as-is and I started writing the exact same patch before realizing that we're missing the actual API method.

epriestley mentioned this in D16594: Expose Drydock leases via Conduit.Oct 25 2018, 11:39 AM
epriestley added a revision: D16594: Expose Drydock leases via Conduit.
epriestley added a parent task: T11694: Allow clients to generally reason about Drydock leases over the API.Oct 25 2018, 11:48 AM

I believe D16594 should implement this, one way or another, unless I'm misunderstanding the request.

epriestley moved this task from Backlog to Now on the Drydock board.Oct 25 2018, 12:05 PM
epriestley closed this task as Resolved by committing rP5f3a7cb41b17: Expose Drydock leases via Conduit.Oct 26 2018, 1:12 PM
epriestley added a commit: rP5f3a7cb41b17: Expose Drydock leases via Conduit.
jwarner added a comment.Oct 30 2018, 9:39 PM

Fantastic, thanks very much @epriestley! I had indeed intended to take care of this myself was on other work this and last week and planned to come back to this. It also would have taken me much longer to realize that drydock.lease.search wasn't yet upstream and how to proceed from there, so I'm glad to see you were able to handle this so easily!

Phabricator · Built by Phacility