Page MenuHomePhabricator

Don't allow empty list constraints in Conduit calls
ClosedPublic

Authored by epriestley on Aug 14 2016, 2:31 PM.
Tags
None
Referenced Files
F13062051: D16396.id39429.diff
Fri, Apr 19, 8:57 PM
F13049891: D16396.id39431.diff
Fri, Apr 19, 2:30 AM
F13049890: D16396.id39429.diff
Fri, Apr 19, 2:30 AM
F13049889: D16396.id.diff
Fri, Apr 19, 2:30 AM
Unknown Object (File)
Thu, Apr 11, 11:42 AM
Unknown Object (File)
Thu, Apr 11, 3:50 AM
Unknown Object (File)
Sun, Apr 7, 12:38 PM
Unknown Object (File)
Sun, Apr 7, 10:23 AM
Subscribers
None

Details

Summary

Ref T11473. If you write a method like get_stuff(ids) and then call it with an empty list of IDs, you can end up passing an empty constraint to Conduit.

If you run a *.search method with such a constraint, like this one:

{
  "ids": []
}

...we have three possible beahviors:

  1. Treat it like the user passed no constraint (basically, ignore the constraint).
  2. Respect the constraint (return no results).
  3. Error.

Currently, we do (1). However, this is pretty confusing and I think clearly the worst option, since it means get_stuff(array()) in client code will often tend to return a ton of results.

We could do (2) instead, but this is also sort of confusing (it may not be obvious why nothing matched, even though it's an application bug) and I think most reasonable client code should be doing an if ($ids) test: this test makes clients a little more complicated, but they can save a network call, and I think they often need to do this test anyway (for example, to show the user a different message).

This implements (3), and just considers these to be errors: this is the least tricky behavior, it's consistent with what we do in PHP, makes fairly good sense, and the only cost for this is that client code may need to be slightly more complex, but this slightly more complex code is usually better code.

Test Plan

Ran Conduit *.search queries with "ids":[] and "phids":[], got sensible errors instead of runaway result sets.

Diff Detail

Repository
rP Phabricator
Branch
nullconstraint
Lint
Lint Passed
Unit
Tests Passed
Build Status
Buildable 13310
Build 17069: Run Core Tests
Build 17068: arc lint + arc unit

Event Timeline

epriestley retitled this revision from to Don't allow empty list constraints in Conduit calls.
epriestley updated this object.
epriestley edited the test plan for this revision. (Show Details)
epriestley added a reviewer: chad.
chad edited edge metadata.
This revision is now accepted and ready to land.Aug 14 2016, 3:06 PM
This revision was automatically updated to reflect the committed changes.