Page MenuHomePhabricator

transaction.search should accept user PHIDs as constraints
Closed, ResolvedPublic

Description

Steps to reproduce:

  1. Go to https://secure.phabricator.com/conduit/method/transaction.search/
  2. Under objectIdentifier, enter "T8510"
  3. Under constraints, enter {"phids": ["PHID-USER-vg42yoljioxdv5iac3py"]}
  4. Press "Call Method" button

Expected result:
I expected a list of transactions in T8510 which were performed by the user with that PHID (me actually), because since D19047 I can pass "PHIDs as constraints".
If not all 'types' of PHIDs are supported you may want to document/clarify expectations for naive users like me? If not: Also fine, just wanted to point this out.
(OT: The docs for transaction.search are pretty short. To my surprise, it also seems the method does not output any project tag changes that happened on a task?)

Actual result:

{
  "data": [],
  "cursor": {
    "limit": 100,
    "after": null,
    "before": null
  }
}

Version
The very current version of Phabricator on this instance.

Event Timeline

aklapper triaged this task as Wishlist priority.Feb 23 2019, 5:14 AM
aklapper created this task.

The phids constraints matches transactions with specific PHIDs (PHID-XACT-...). Usually, you'll have a list of transaction PHIDs when you're responding to a webhook callback, so the phids constraint is most useful for webhooks.

We can add an authorPHIDs constraint and clean up some of the other particulars here.

Thanks for the quick reply!
Haven't played with webhooks yet so I did not know/realize it's transaction PHIDs only - "PHID" sounded generic enough that I incorrectly expected any kinds of PHIDs to be accepted as input. So this task is somewhere between either "clarify documentation that only transaction PHIDs are supported" or a "support other PHIDs" low-prio enhancement request.

To my surprise, it also seems the method does not output any project tag changes that happened on a task?

Currently, project tag changes and subscriber changes don't expose data in "transaction.search" because there's no "obviously correct" way to represent these changes in a future-proof way.

Some options include:

Full Lists: Return "old" and "new" with lists of PHIDs. This is probably the most simple/natural format (and how we stored these records internally for a long time), but runs into problems with large lists. For example, if an object has 1,000 subscribers, someone becomes the 1,001st subscriber, and we represent the transaction as full lists of "old" and "new" PHIDs, the transaction takes 2,001 PHIDs to represent (a list of 1000 "old" PHIDs, then a list of 1001 "new" PHIDs). See also T13051, where certain merge strategies caused some commits to mention other commits many thousands of times.

Because of the changes in T13051 to scale through this properly, we also can't easily reconstruct the full lists anymore (we have to start at the current object state and work backwards, or start at the original object state and work forwards). This is perhaps an argument that we should do this, since otherwise clients may have to do it and we can do it more easily than clients can, but I suspect it's rare that full reconstruction is necessary or useful (no one has asked for data on these edits until now, even).

That is, I think clients will rarely want to ask "What projects was this task tagged with on such-and-such date?" or "Who was subscribed on such-and-such date?", especially once Facts becomes available and can provide an alternate approach for answering some of these kinds of questions. Current callers to transaction.search are generally concerned with when/how an object changed, not what an object's previous state was.

(Even if there were a compelling set of use cases for rewinding views of objects, providing full lists doesn't help much, and a better API for this would probably be takeATimeMachineToEpoch: ... parameter to methods like maniphest.search, that attempted to rewind the state of each returned object to the specified date. This is probably quite a lot of work to get mostly right, can never be completely accurate, and it's not immediately obvious to me that it's particularly useful.)

"Operation Groups": Internally, we use this format in some places:

{
  "+": [X, Y],
  "-": [U ,V]
}

...to indicate that users "X" and "Y" were added, and users "U" and "V" were removed.

This is fine and there's nothing really wrong with it, but it feels very ad-hoc and not very obvious or expected. We've been moving away from this format for a while and I'd like to leave it behind.

List of Operations?: That probably leaves us with something like this:

[
  {
    "operation": "add",
    "phid": "X"
  },
  {
    "operation": "remove",
    "phid": "Y"
  }
]

This echoes the projects.add, projects.remove, subscribers.add, subscribers.remove, etc., operations in *.edit API methods. It's relatively future-proof, even if we walk back the planned-and-mostly-executed change in T13056 or replace it with some similar system.

It's probably a little inconvenient to process as a client, but it should be obvious how to process it.

Currently, project tag changes and subscriber changes don't expose data in "transaction.search" because there's no "obviously correct" way to represent these changes in a future-proof way.

Oh, the other reason for the current behavior is that only modern "ModularTransactions" support the callback hook to expose rich data to transaction.search. TYPE_EDGE (which includes project tag changes) will upgrade eventually, but this is definitely a large change. But we can cheat for now since I'm satisfied that the "list of operations" representation is probably not a critical mistake.

epriestley closed this task as Resolved.Feb 25 2019, 7:08 PM
epriestley claimed this task.
  • D20208 adds an authorPHIDs constraint.
  • D20209 provides details for transactions which add or remove projects. See above for format and rationale, or just query something and see what pops out.
  • D20211 makes the documentation less bare-bones.

These changes are available in master and will promote to stable in 2019 Week 9.

Broadly, this API method looks like a duck and quacks like a duck (where "a duck" is "a modern <x>.search API method") but is actually a big ball of spaghetti dressed up like a duck. Some day it will be a real duck, but we just have to keep feeding it bread until then. (Or corn or millet or whatever, I guess bread is bad for ducks?)