From 38ae81fb3916a57dac920708f548583bd3bcb605 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Wed, 7 Sep 2016 07:21:41 -0700
Subject: [PATCH] Throw when callers pass an invalid constraint to a "*.search"
 method

Summary:
Ref T11593. When you call a `*.search` method like `maniphest.search`, we don't currently validate that all the constraints you pass are recognized.

I think there were two very weak arguments for not doing this:

  - It makes compatibility in `arc` across versions slightly easier: if we add a new constraint, we could add it to `arc` but also do client-side filtering for a while.
  - Conduit parameter types //could//, in theory, accept multiple inputs or optional/alias inputs.

These reasons are pretty fluff and T11593 is a concrete issue caused by not validating. Just validate instead.

Test Plan:
  - Made a `maniphest.search` call with a bogus constraint, got an explicit error about the bad constraint.
  - Made a `maniphest.search` call with a valid constraint (`"ids"`).

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11593

Differential Revision: https://secure.phabricator.com/D16507
---
 .../parametertype/ConduitParameterType.php       |  7 +++++++
 .../PhabricatorApplicationSearchEngine.php       | 16 ++++++++++++++++
 .../search/field/PhabricatorSearchField.php      |  5 +++++
 3 files changed, 28 insertions(+)

diff --git a/src/applications/conduit/parametertype/ConduitParameterType.php b/src/applications/conduit/parametertype/ConduitParameterType.php
index 6468b099a0..011401433e 100644
--- a/src/applications/conduit/parametertype/ConduitParameterType.php
+++ b/src/applications/conduit/parametertype/ConduitParameterType.php
@@ -38,6 +38,9 @@ abstract class ConduitParameterType extends Phobject {
     return $this->getParameterValue($request, $key);
   }
 
+  final public function getKeys($key) {
+    return $this->getParameterKeys($key);
+  }
 
   final public function getDefaultValue() {
     return $this->getParameterDefault();
@@ -86,6 +89,10 @@ abstract class ConduitParameterType extends Phobject {
     return $request[$key];
   }
 
+  protected function getParameterKeys($key) {
+    return array($key);
+  }
+
   abstract protected function getParameterTypeName();
 
 
diff --git a/src/applications/search/engine/PhabricatorApplicationSearchEngine.php b/src/applications/search/engine/PhabricatorApplicationSearchEngine.php
index d2ce3ad946..13606c542f 100644
--- a/src/applications/search/engine/PhabricatorApplicationSearchEngine.php
+++ b/src/applications/search/engine/PhabricatorApplicationSearchEngine.php
@@ -1094,6 +1094,22 @@ abstract class PhabricatorApplicationSearchEngine extends Phobject {
       }
     }
 
+    $valid_constraints = array();
+    foreach ($fields as $field) {
+      foreach ($field->getValidConstraintKeys() as $key) {
+        $valid_constraints[$key] = true;
+      }
+    }
+
+    foreach ($constraints as $key => $constraint) {
+      if (empty($valid_constraints[$key])) {
+        throw new Exception(
+          pht(
+            'Constraint "%s" is not a valid constraint for this query.',
+            $key));
+      }
+    }
+
     foreach ($fields as $field) {
       if (!$field->getValueExistsInConduitRequest($constraints)) {
         continue;
diff --git a/src/applications/search/field/PhabricatorSearchField.php b/src/applications/search/field/PhabricatorSearchField.php
index 9624fe22f9..d31aeb1950 100644
--- a/src/applications/search/field/PhabricatorSearchField.php
+++ b/src/applications/search/field/PhabricatorSearchField.php
@@ -329,6 +329,11 @@ abstract class PhabricatorSearchField extends Phobject {
       $this->getConduitKey());
   }
 
+  public function getValidConstraintKeys() {
+    return $this->getConduitParameterType()->getKeys(
+      $this->getConduitKey());
+  }
+
 
 /* -(  Utility Methods )----------------------------------------------------- */