Page MenuHomePhabricator

Allow typeaheads to pass nonscalar data to datasources
ClosedPublic

Authored by epriestley on Feb 11 2019, 2:54 PM.

Details

Summary

Ref T13250. Currently, datasources have a setParameters(...) method. This method accepts a dictionary and adds the key/value pairs to the raw HTTP request to the datasource endpoint.

Since D20049, this no longer works. Since D20116, it fatals explicitly.

In general, the datasource endpoint accepts other values (like query, offset, and limit), and even before these changes, using secret reserved keys in setParameters(...) would silently cause program misbehavior.

To deal with this, pass parameters as a JSON string named "parameters". This fixes the HTTP query issue (the more pressing issue affecting users today) and prevents the "shadowing reserved keys" issue (a theoretical issue which might affect users some day).

(I may revisit the phutil_build_http_querystring() behavior and possibly let it make this work again, but I think avoiding the duplicate key issue makes this change desirable even if the querystring behavior changes.)

Test Plan
  • Used "Land Revision", selected branches.
  • Configured a custom Maniphest "users" field, used the search typeahead, selected users.
  • Manually browsed to /typeahead/class/PhabricatorPeopleDatasource/?query=hi&parameters=xyz to see the JSON decode exception.

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

epriestley created this revision.Feb 11 2019, 2:54 PM
epriestley requested review of this revision.Feb 11 2019, 2:56 PM
amckinley accepted this revision.Feb 11 2019, 6:10 PM

Does it make any sense to change phutil_build_http_querystring to implement this this "if it's an object or an array, turn it into a JSON string" behavior?

src/applications/typeahead/controller/PhabricatorTypeaheadModularDatasourceController.php
50–51

"Yo dawg, I heard you liked parameters..."

No one should ever see this anyway, but this sentence is kind of a mess.

This revision is now accepted and ready to land.Feb 11 2019, 6:10 PM

Does it make any sense to change phutil_build_http_querystring to implement this this "if it's an object or an array, turn it into a JSON string" behavior?

I think this is too magical to put in phutil_build_http_querystring(). I like that we're moving toward a very dumb, no-assumptions implementation of query string behavior in D20136, and think that's generally the right direction: the basic primitives shouldn't have any implicit magic.

It would be reasonable to put in some other wrapper, but I think this pattern is relatively rare, and we're often building the JSON payload in Javascript when we do use the pattern (e.g., to save a Herald rule). Worth keeping an eye on and it might happen more often in the future if Ajax stuff gets fancier, but I'm hard-pressed to think of even a second callsite offhand (but maybe other uses are just slipping my mind).

In trying to come up with examples, I think when we do stuff like this (pass complex data from one PHP endpoint to another PHP endpoint without using a form), it's usually passed to the Javascript layer as Javelin metadata (which does get implicitly JSON'd / un-jSON'd) and then passed back to us by Javascript, often after modifications. A recent example is that timeline rendering change to Differential in D19918 or some followup (where Differential needs to pass which diff you're looking at to the timeline rendering code, so it can link inline comments either to anchors on the current page or to other pages).

This revision was automatically updated to reflect the committed changes.