Page MenuHomePhabricator

Update Conduit for ApplicationTransactions, CustomFields and Edges
Closed, ResolvedPublic

Assigned To
Authored By
epriestley
Aug 13 2014, 5:00 PM
Referenced Files
None
Tokens
"Like" token, awarded by jonathanrseawright."Like" token, awarded by featherless."Love" token, awarded by 20after4."Manufacturing Defect?" token, awarded by ramesh_dharan."Pterodactyl" token, awarded by eadler."Like" token, awarded by chad."Like" token, awarded by aadis."Like" token, awarded by altan."Like" token, awarded by sh1mmer."Like" token, awarded by austinstoker."Like" token, awarded by michaeloa.

Description

Many Conduit methods for updating or reading objects are now out of date. In particular, they generally do not support ApplicationTransactions or CustomFields. This means some data (like inline comments, and usually custom field values) often can not be read, and some data can not be written. In action-oriented applications like Differential and Diffusion, some actions can not be taken.

We also have uneven support for querying attached objects. Sometimes these are just loaded and provided explicitly; sometimes they are inaccessible.

A number of open tasks request access to these fields or edges. We want to move forward in a general way which accommodates ApplicationTransactions and CustomFields, rather than adding one-off properties to objects. However, this requires that we think through the design carefully. It is also likely to make backward-incompatible changes in at least some methods which have incompatible partial support with these systems.

Reading Custom Fields

This is probably the simplest thing to do. Letting custom fields elect to appear in Conduit responses and exposing them from app.query methods seems straightforward. Open issues:

  • differential.query emits an auxiliary dictionary with custom field values.
  • maniphest.query emits an auxiliary dictionary with custom field values.
  • Maniphest data is keyed by field key (often nonobvious and internal). Differential data is keyed by "conduit field key" (better-ish but distinct).
  • Maybe pulling this data should be optional?
  • Should we have keys which are easier to use? Probably...

Reading Transactions

This is easy to implement as a low-level API, like transactions.query. Open issues:

  • Transaction formats are very hard for third-party code to consume. Lots of internal datastructures, and nothing is rendered.
    • For one example, Differential inline comments are bound to changesetIDs, which probably can not be retrieved over the API.
  • Transaction formats aren't very stable.
  • differential.getrevisioncomments provides access to Differential comments only.
  • maniphest.gettasktransactions provides access to Maniphest transactions only.
  • Neither expose all of the information (like comment edits / removal).
  • Putting this in "transactions.query" instead of "application.querytransactions" might make it hard for users to find.

Possible solutions:

  • Overall, the data format is probably not totally unreasonable. It's not pristine, but most technical users can likely make sense of it.
  • Expose this, call it "low-level", make some effort to pretty it up a bit (add extra data, like a text rendering of the transaction)?
  • Maybe also expose a light/easy way to query just comments? Not sure this is useful.

Reading Edges

Open issues:

  • We can expose edges.query, but this makes it harder to use than just getting the data back on the objects.
  • However, putting taskPHIDs, revisionPHIDs, commitPHIDs, etc., on every object which is related to other objects seems clearly unworkable, and won't ever work with third-party applications.
  • A few relationships are not edges (like workboard columns, and I worry reviewers/auditors should maybe not really be edges).
  • Edges have meaningless constant identifiers.
  • Some methods already expose related objects.

Possible solutions:

  • I think this should probably be in edges.query and we can just smooth over the other stuff as best we can (e.g., let edge types define readable names).
  • The other alternative I can come up with is adding a new "connections" idea, which is a generalization over edges but could include other connections which are not technically stored as edges, like workboard columns. Primary query methods like differential.query could then have an option to pull these connections, and/or we could pull them all by default. I kind of like this, but we could build edges.query first and then look into this as an ease-of-use refinement.

Writing Edges

We can not allow direct edge writes, since you can violate security in all sorts of ways by performing them.

Writing Custom Fields

We should not allow direct writes to custom fields, except maybe as a convenience. Write transactions instead.

Writing Transactions

This is the biggest mess. In particular:

  • Transactions don't have as much type validation as they should to be exposed as public APIs.
  • Constructing transaction values is very much type-specific, inconsistent black magic (for example, the magic + and - keys when doing subscriptions and edges).
  • Some transactions currently require the caller to provide both oldValue and newValue, but we generally should not permit callers to control oldValue.
  • Transaction constants are not consistently meaningful and not stable.
  • Transaction formats are not stable.

Possible solutions:

  • I think this probably needs a layer of glue code to make the API usable, with operations like "comment", "subscribers.add", "subscribers.remove", "subscribers.set", etc., and less black magic? This will be somewhat involved to develop.

Related Objects

StatusAssignedTask
OpenNone
Resolvedepriestley
OpenNone
Resolvedepriestley
Wontfixepriestley
Resolvedbtrahan
Resolvedepriestley
Resolvedepriestley
Resolvedepriestley
Resolvedepriestley
Resolvedepriestley
Resolvedchad
Resolvedchad
Resolvedchad
Resolvedchad
Resolvedchad
Resolvedchad
OpenNone
ResolvedNone
DuplicateNone

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

Broad update here after T9132:

Reading Custom Fields: I think we have reasonable answers to the open questions, but this is blocked on T7715 providing new ApplicationSearch-driven endpoints (which isn't blocked on anything now, just a priority issue).

Reading Transactions: Nothing here is too difficult to overcome, but it should probably wait for T9789 and go on top of that to reduce churn.

Reading Edges: I haven't developed my thinking on this in any greater detail, but I don't anticipate any real difficulty finding a pathway forward here.

Writing Transactions: Now supported approximately as described (via a layer of sugar/glue code) via new *.edit endpoints (paste.edit, owners.edit, maniphest.edit). This isn't fully fleshed out yet (some transaction types are not yet reachable, some of the sugar isn't as sugary as it should be) but that work should be fairly straightforward in most cases. (For some applications, it may make practical sense to wait for T9789 before writing all the glue.)

Reading Custom Fields is now supported in new *.search endpoints (paste.search, maniphest.search, owners.search).

Some edges (subscriptions, projects) are also readable today via attachments on these endpoints.

eadler added a project: Restricted Project.Jan 9 2016, 12:50 AM
eadler moved this task from Restricted Project Column to Restricted Project Column on the Restricted Project board.
eadler moved this task from Restricted Project Column to Restricted Project Column on the Restricted Project board.Feb 23 2016, 6:13 PM
cburroughs moved this task from Restricted Project Column to Restricted Project Column on the Restricted Project board.

@epriestley Is there a pathway for reading (not writing) edges which would be open to community contribution? Having arc patch Tnnnn functionality, pulling all the revisions attached to a particular task, would be really attractive for us as we tend to have a larger number of revisions per task.

huangsj moved this task from Future to vNext on the Conduit board.
epriestley claimed this task.

I'm going to call this effectively resolved:

  • *.edit methods support writing transactions, custom fields, and edges.
  • *.search methods support reading custom fields (and some edges).
  • transaction.search supports reading transactions.
  • edge.search supports reading edges.

Not every transaction, field, or edge is fully readable or writable, but they never will be (we will always have some internal transactions, edges, etc., which are not exposed to the API for policy, security, or technical reasons).

I think we're on firm enough ground here to deal with remaining issues on a case-by-case basis (e.g., with good use case justification).