From e77ae13d5c71e98a40b84d503541d10fc0506a41 Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 9 Nov 2020 10:33:14 -0800 Subject: [PATCH] Provide a more structured result log for Herald conditions Summary: Ref T13586. Currently, Herald condition logs encode "pass" or "fail" robustly, "forbidden" through a sort of awkward side channel, and can not properly encode "invalid" or "exception" outcomes. Structure the condition log so results are represented unambiguously and all possible outcomes (pass, fail, forbidden, invalid, exception) are clearly encoded. Test Plan: {F8446102} {F8446103} Maniphest Tasks: T13586 Differential Revision: https://secure.phabricator.com/D21563 --- resources/celerity/map.php | 4 +- src/__phutil_library_map__.php | 2 + .../herald/adapter/HeraldAdapter.php | 34 ++- .../controller/HeraldTranscriptController.php | 34 +-- .../herald/engine/HeraldEngine.php | 263 +++++++++++++----- .../transcript/HeraldConditionResult.php | 177 ++++++++++++ .../transcript/HeraldConditionTranscript.php | 56 ++-- .../css/application/herald/herald-test.css | 2 + 8 files changed, 440 insertions(+), 132 deletions(-) create mode 100644 src/applications/herald/storage/transcript/HeraldConditionResult.php diff --git a/resources/celerity/map.php b/resources/celerity/map.php index 487da6a02e..69f28b9738 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -78,7 +78,7 @@ return array( 'rsrc/css/application/files/global-drag-and-drop.css' => '1d2713a4', 'rsrc/css/application/flag/flag.css' => '2b77be8d', 'rsrc/css/application/harbormaster/harbormaster.css' => '8dfe16b2', - 'rsrc/css/application/herald/herald-test.css' => 'e004176f', + 'rsrc/css/application/herald/herald-test.css' => '7e7bbdae', 'rsrc/css/application/herald/herald.css' => '648d39e2', 'rsrc/css/application/maniphest/report.css' => '3d53188b', 'rsrc/css/application/maniphest/task-edit.css' => '272daa84', @@ -587,7 +587,7 @@ return array( 'harbormaster-css' => '8dfe16b2', 'herald-css' => '648d39e2', 'herald-rule-editor' => '2633bef7', - 'herald-test-css' => 'e004176f', + 'herald-test-css' => '7e7bbdae', 'inline-comment-summary-css' => '81eb368d', 'javelin-aphlict' => '022516b4', 'javelin-behavior' => '1b6acc2a', diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 79d318b76d..7b6c30ab71 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -1566,6 +1566,7 @@ phutil_register_library_map(array( 'HeraldCommentContentField' => 'applications/herald/field/HeraldCommentContentField.php', 'HeraldCommitAdapter' => 'applications/diffusion/herald/HeraldCommitAdapter.php', 'HeraldCondition' => 'applications/herald/storage/HeraldCondition.php', + 'HeraldConditionResult' => 'applications/herald/storage/transcript/HeraldConditionResult.php', 'HeraldConditionTranscript' => 'applications/herald/storage/transcript/HeraldConditionTranscript.php', 'HeraldContentSourceField' => 'applications/herald/field/HeraldContentSourceField.php', 'HeraldController' => 'applications/herald/controller/HeraldController.php', @@ -7793,6 +7794,7 @@ phutil_register_library_map(array( 'HarbormasterBuildableAdapterInterface', ), 'HeraldCondition' => 'HeraldDAO', + 'HeraldConditionResult' => 'Phobject', 'HeraldConditionTranscript' => 'Phobject', 'HeraldContentSourceField' => 'HeraldField', 'HeraldController' => 'PhabricatorController', diff --git a/src/applications/herald/adapter/HeraldAdapter.php b/src/applications/herald/adapter/HeraldAdapter.php index 58e0143799..ce6f371d91 100644 --- a/src/applications/herald/adapter/HeraldAdapter.php +++ b/src/applications/herald/adapter/HeraldAdapter.php @@ -520,21 +520,29 @@ abstract class HeraldAdapter extends Phobject { case self::CONDITION_NOT_REGEXP: $result_if_match = ($condition_type == self::CONDITION_REGEXP); + // We add the 'S' flag because we use the regexp multiple times. + // It shouldn't cause any troubles if the flag is already there + // - /.*/S is evaluated same as /.*/SS. + $condition_pattern = $condition_value.'S'; + foreach ((array)$field_value as $value) { - // We add the 'S' flag because we use the regexp multiple times. - // It shouldn't cause any troubles if the flag is already there - // - /.*/S is evaluated same as /.*/SS. - $result = @preg_match($condition_value.'S', $value); - if ($result === false) { - throw new HeraldInvalidConditionException( - pht( - 'Regular expression "%s" in Herald rule "%s" is not valid, '. - 'or exceeded backtracking or recursion limits while '. - 'executing. Verify the expression and correct it or rewrite '. - 'it with less backtracking.', - $condition_value, - $rule->getMonogram())); + try { + $result = phutil_preg_match($condition_pattern, $value); + } catch (PhutilRegexException $ex) { + $message = array(); + $message[] = pht( + 'Regular expression "%s" in Herald rule "%s" is not valid, '. + 'or exceeded backtracking or recursion limits while '. + 'executing. Verify the expression and correct it or rewrite '. + 'it with less backtracking.', + $condition_value, + $rule->getMonogram()); + $message[] = $ex->getMessage(); + $message = implode("\n\n", $message); + + throw new HeraldInvalidConditionException($message); } + if ($result) { return $result_if_match; } diff --git a/src/applications/herald/controller/HeraldTranscriptController.php b/src/applications/herald/controller/HeraldTranscriptController.php index 7755afaf74..a7498abdb7 100644 --- a/src/applications/herald/controller/HeraldTranscriptController.php +++ b/src/applications/herald/controller/HeraldTranscriptController.php @@ -269,34 +269,20 @@ final class HeraldTranscriptController extends HeraldController { ->setTarget(phutil_tag('strong', array(), pht('Conditions')))); foreach ($cond_xscripts as $cond_xscript) { - if ($cond_xscript->isForbidden()) { - $icon = 'fa-ban'; - $color = 'indigo'; - $result = pht('Forbidden'); - } else if ($cond_xscript->getResult()) { - $icon = 'fa-check'; - $color = 'green'; - $result = pht('Passed'); - } else { - $icon = 'fa-times'; - $color = 'red'; - $result = pht('Failed'); - } + $result = $cond_xscript->getResult(); - if ($cond_xscript->getNote()) { - $note_text = $cond_xscript->getNote(); - if ($cond_xscript->isForbidden()) { - $note_text = HeraldStateReasons::getExplanation($note_text); - } + $icon = $result->getIconIcon(); + $color = $result->getIconColor(); + $name = $result->getName(); - $note = phutil_tag( + $result_details = $result->newDetailsView(); + if ($result_details !== null) { + $result_details = phutil_tag( 'div', array( 'class' => 'herald-condition-note', ), - $note_text); - } else { - $note = null; + $result_details); } // TODO: This is not really translatable and should be driven through @@ -309,8 +295,8 @@ final class HeraldTranscriptController extends HeraldController { $cond_item = id(new PHUIStatusItemView()) ->setIcon($icon, $color) - ->setTarget($result) - ->setNote(array($explanation, $note)); + ->setTarget($name) + ->setNote(array($explanation, $result_details)); $cond_list->addItem($cond_item); } diff --git a/src/applications/herald/engine/HeraldEngine.php b/src/applications/herald/engine/HeraldEngine.php index 9ca72821e7..78a3c6c695 100644 --- a/src/applications/herald/engine/HeraldEngine.php +++ b/src/applications/herald/engine/HeraldEngine.php @@ -8,7 +8,8 @@ final class HeraldEngine extends Phobject { protected $activeRule; protected $transcript; - protected $fieldCache = array(); + private $fieldCache = array(); + private $fieldExceptions = array(); protected $object; private $dryRun; @@ -64,6 +65,7 @@ final class HeraldEngine extends Phobject { $this->transcript = new HeraldTranscript(); $this->transcript->setObjectPHID((string)$object->getPHID()); $this->fieldCache = array(); + $this->fieldExceptions = array(); $this->results = array(); $this->rules = $rules; $this->object = $object; @@ -74,6 +76,7 @@ final class HeraldEngine extends Phobject { $is_first_only = $rule->isRepeatFirst(); + $caught = null; try { if (!$this->getDryRun() && $is_first_only && @@ -119,7 +122,21 @@ final class HeraldEngine extends Phobject { $names)); } $rule_matches = false; + } catch (Exception $ex) { + $caught = $ex; + } catch (Throwable $ex) { + $caught = $ex; } + + if ($caught) { + $this->newRuleTranscript($rules[$phid]) + ->setResult(false) + ->setReason( + pht( + 'Rule encountered an exception while evaluting.')); + $rule_matches = false; + } + $this->results[$phid] = $rule_matches; if ($rule_matches) { @@ -323,42 +340,18 @@ final class HeraldEngine extends Phobject { $result = false; } else { foreach ($conditions as $condition) { - try { - $this->getConditionObjectValue($condition, $object); - } catch (Exception $ex) { - $reason = pht( - 'Field "%s" does not exist!', - $condition->getFieldName()); - $result = false; - break; - } - - // Here, we're profiling the cost to match the condition value against - // whatever test is configured. Normally, this cost should be very - // small (<<1ms) since it amounts to a single comparison: - // - // [ Task author ][ is any of ][ alice ] - // - // However, it may be expensive in some cases, particularly if you - // write a rule with a very creative regular expression that backtracks - // explosively. - // - // At time of writing, the "Another Herald Rule" field is also - // evaluated inside the matching function. This may be arbitrarily - // expensive (it can prompt us to execute any finite number of other - // Herald rules), although we'll push the profiler stack appropriately - // so we don't count the evaluation time against this rule in the final - // profile. - $caught = null; - $this->pushProfilerRule($rule); try { - $match = $this->doesConditionMatch($rule, $condition, $object); + $match = $this->doesConditionMatch( + $rule, + $condition, + $object); } catch (Exception $ex) { $caught = $ex; + } catch (Throwable $ex) { + $caught = $ex; } - $this->popProfilerRule($rule); if ($caught) { throw $ex; @@ -419,63 +412,176 @@ final class HeraldEngine extends Phobject { return $result; } - protected function doesConditionMatch( + private function doesConditionMatch( HeraldRule $rule, HeraldCondition $condition, - HeraldAdapter $object) { + HeraldAdapter $adapter) { - $object_value = $this->getConditionObjectValue($condition, $object); $transcript = $this->newConditionTranscript($rule, $condition); + $caught = null; + $result_data = array(); + try { - $result = $object->doesConditionMatch( - $this, + $field_key = $condition->getFieldName(); + + $field_value = $this->getProfiledObjectFieldValue( + $adapter, + $field_key); + + $is_match = $this->getProfiledConditionMatch( + $adapter, $rule, $condition, - $object_value); + $field_value); + if ($is_match) { + $result_code = HeraldConditionResult::RESULT_MATCHED; + } else { + $result_code = HeraldConditionResult::RESULT_FAILED; + } } catch (HeraldInvalidConditionException $ex) { - $result = false; - $transcript->setNote($ex->getMessage()); + $result_code = HeraldConditionResult::RESULT_INVALID; + $caught = $ex; + } catch (Exception $ex) { + $result_code = HeraldConditionResult::RESULT_EXCEPTION; + $caught = $ex; + } catch (Throwable $ex) { + $result_code = HeraldConditionResult::RESULT_EXCEPTION; + $caught = $ex; } + if ($caught) { + $result_data = array( + 'exception.class' => get_class($caught), + 'exception.message' => $ex->getMessage(), + ); + } + + $result = HeraldConditionResult::newFromResultCode($result_code) + ->setResultData($result_data); + $transcript->setResult($result); - return $result; - } - - protected function getConditionObjectValue( - HeraldCondition $condition, - HeraldAdapter $object) { - - $field = $condition->getFieldName(); - - return $this->getObjectFieldValue($field); - } - - public function getObjectFieldValue($field) { - if (!array_key_exists($field, $this->fieldCache)) { - $adapter = $this->object; - - $adapter->willGetHeraldField($field); - - $caught = null; - - $this->pushProfilerField($field); - try { - $value = $adapter->getHeraldField($field); - } catch (Exception $ex) { - $caught = $ex; - } - $this->popProfilerField($field); - - if ($caught) { - throw $caught; - } - - $this->fieldCache[$field] = $value; + if ($caught) { + throw $caught; } - return $this->fieldCache[$field]; + return $result->getIsMatch(); + } + + private function getProfiledConditionMatch( + HeraldAdapter $adapter, + HeraldRule $rule, + HeraldCondition $condition, + $field_value) { + + // Here, we're profiling the cost to match the condition value against + // whatever test is configured. Normally, this cost should be very + // small (<<1ms) since it amounts to a single comparison: + // + // [ Task author ][ is any of ][ alice ] + // + // However, it may be expensive in some cases, particularly if you + // write a rule with a very creative regular expression that backtracks + // explosively. + // + // At time of writing, the "Another Herald Rule" field is also + // evaluated inside the matching function. This may be arbitrarily + // expensive (it can prompt us to execute any finite number of other + // Herald rules), although we'll push the profiler stack appropriately + // so we don't count the evaluation time against this rule in the final + // profile. + + $this->pushProfilerRule($rule); + + $caught = null; + try { + $is_match = $adapter->doesConditionMatch( + $this, + $rule, + $condition, + $field_value); + } catch (Exception $ex) { + $caught = $ex; + } catch (Throwable $ex) { + $caught = $ex; + } + + $this->popProfilerRule($rule); + + if ($caught) { + throw $caught; + } + + return $is_match; + } + + private function getProfiledObjectFieldValue( + HeraldAdapter $adapter, + $field_key) { + + // Before engaging the profiler, make sure the field class is loaded. + + $adapter->willGetHeraldField($field_key); + + // The first time we read a field value, we'll actually generate it, which + // may be slow. + + // After it is generated for the first time, this will just read it from a + // cache, which should be very fast. + + // We still want to profile the request even if it goes to cache so we can + // get an accurate count of how many times we access the field value: when + // trying to improve the performance of Herald rules, it's helpful to know + // how many rules rely on the value of a field which is slow to generate. + + $caught = null; + + $this->pushProfilerField($field_key); + try { + $value = $this->getObjectFieldValue($field_key); + } catch (Exception $ex) { + $caught = $ex; + } catch (Throwable $ex) { + $caught = $ex; + } + $this->popProfilerField($field_key); + + if ($caught) { + throw $caught; + } + + return $value; + } + + private function getObjectFieldValue($field_key) { + if (array_key_exists($field_key, $this->fieldExceptions)) { + throw $this->fieldExceptions[$field_key]; + } + + if (array_key_exists($field_key, $this->fieldCache)) { + return $this->fieldCache[$field_key]; + } + + $adapter = $this->object; + + $caught = null; + try { + $value = $adapter->getHeraldField($field_key); + } catch (Exception $ex) { + $caught = $ex; + } catch (Throwable $ex) { + $caught = $ex; + } + + if ($caught) { + $this->fieldExceptions[$field_key] = $caught; + throw $caught; + } + + $this->fieldCache[$field_key] = $value; + + return $value; } protected function getRuleEffects( @@ -639,9 +745,16 @@ final class HeraldEngine extends Phobject { $forbidden_reason = $this->forbiddenFields[$field_key]; if ($forbidden_reason !== null) { + $result_code = HeraldConditionResult::RESULT_OBJECT_STATE; + $result_data = array( + 'reason' => $forbidden_reason, + ); + + $result = HeraldConditionResult::newFromResultCode($result_code) + ->setResultData($result_data); + $this->newConditionTranscript($rule, $condition) - ->setResult(HeraldConditionTranscript::RESULT_FORBIDDEN) - ->setNote($forbidden_reason); + ->setResult($result); $is_forbidden = true; } diff --git a/src/applications/herald/storage/transcript/HeraldConditionResult.php b/src/applications/herald/storage/transcript/HeraldConditionResult.php new file mode 100644 index 0000000000..16e240b3f8 --- /dev/null +++ b/src/applications/herald/storage/transcript/HeraldConditionResult.php @@ -0,0 +1,177 @@ + $this->getResultCode(), + 'data' => $this->getResultData(), + ); + } + + public static function newFromMap(array $map) { + $result_code = idx($map, 'code'); + $result = self::newFromResultCode($result_code); + + $result_data = idx($map, 'data', array()); + $result->setResultData($result_data); + + return $result; + } + + public static function newFromResultCode($result_code) { + $map = self::getResultSpecification($result_code); + + $result = new self(); + $result->resultCode = $result_code; + + return $result; + } + + public function getResultCode() { + return $this->resultCode; + } + + private function getResultData() { + return $this->resultData; + } + + public function getIconIcon() { + return $this->getSpecificationProperty('icon'); + } + + public function getIconColor() { + return $this->getSpecificationProperty('color.icon'); + } + + public function getIsMatch() { + return ($this->getSpecificationProperty('match') === true); + } + + public function getName() { + return $this->getSpecificationProperty('name'); + } + + public function newDetailsView() { + switch ($this->resultCode) { + case self::RESULT_OBJECT_STATE: + $reason = $this->getDataProperty('reason'); + $details = HeraldStateReasons::getExplanation($reason); + break; + case self::RESULT_INVALID: + case self::RESULT_EXCEPTION: + $error_class = $this->getDataProperty('exception.class'); + $error_message = $this->getDataProperty('exception.message'); + + if (!strlen($error_class)) { + $error_class = pht('Unknown Error'); + } + + switch ($error_class) { + case 'HeraldInvalidConditionException': + $error_class = pht('Invalid Condition'); + break; + } + + if (!strlen($error_message)) { + $error_message = pht( + 'An unknown error occurred while evaluating this condition. No '. + 'additional information is available.'); + } + + $details = pht( + '%s: %s', + phutil_tag('strong', array(), $error_class), + phutil_escape_html_newlines($error_message)); + break; + $details = 'exception'; + break; + default: + $details = null; + break; + } + + return $details; + } + + public function setResultData(array $result_data) { + $this->resultData = $result_data; + return $this; + } + + private function getDataProperty($key) { + $data = $this->getResultData(); + return idx($data, $key); + } + + private function getSpecificationProperty($key) { + $map = self::getResultSpecification($this->resultCode); + return $map[$key]; + } + + private static function getResultSpecification($result_code) { + $map = self::getResultSpecificationMap(); + + if (!isset($map[$result_code])) { + throw new Exception( + pht( + 'Condition result "%s" is unknown.', + $result_code)); + } + + return $map[$result_code]; + } + + private static function getResultSpecificationMap() { + return array( + self::RESULT_MATCHED => array( + 'match' => true, + 'icon' => 'fa-check', + 'color.icon' => 'green', + 'name' => pht('Passed'), + ), + self::RESULT_FAILED => array( + 'match' => false, + 'icon' => 'fa-times', + 'color.icon' => 'red', + 'name' => pht('Failed'), + ), + self::RESULT_OBJECT_STATE => array( + 'match' => null, + 'icon' => 'fa-ban', + 'color.icon' => 'indigo', + 'name' => pht('Forbidden'), + ), + self::RESULT_INVALID => array( + 'match' => null, + 'icon' => 'fa-exclamation-triangle', + 'color.icon' => 'yellow', + 'name' => pht('Invalid'), + ), + self::RESULT_EXCEPTION => array( + 'match' => null, + 'icon' => 'fa-exclamation-triangle', + 'color.icon' => 'red', + 'name' => pht('Exception'), + ), + self::RESULT_UNKNOWN => array( + 'match' => null, + 'icon' => 'fa-question', + 'color.icon' => 'grey', + 'name' => pht('Unknown'), + ), + ); + } + +} diff --git a/src/applications/herald/storage/transcript/HeraldConditionTranscript.php b/src/applications/herald/storage/transcript/HeraldConditionTranscript.php index a7096b6880..2e66cea1fb 100644 --- a/src/applications/herald/storage/transcript/HeraldConditionTranscript.php +++ b/src/applications/herald/storage/transcript/HeraldConditionTranscript.php @@ -7,10 +7,17 @@ final class HeraldConditionTranscript extends Phobject { protected $fieldName; protected $condition; protected $testValue; - protected $note; - protected $result; + protected $resultMap; - const RESULT_FORBIDDEN = 'forbidden'; + // See T13586. Older versions of this record stored a boolean true, boolean + // false, or the string "forbidden" in the "$result" field. They stored a + // human-readable English-language message or a state code in the "$note" + // field. + + // The modern record does not use either field. + + protected $result; + protected $note; public function setRuleID($rule_id) { $this->ruleID = $rule_id; @@ -57,26 +64,39 @@ final class HeraldConditionTranscript extends Phobject { return $this->testValue; } - public function setNote($note) { - $this->note = $note; - return $this; - } - - public function getNote() { - return $this->note; - } - - public function setResult($result) { - $this->result = $result; + public function setResult(HeraldConditionResult $result) { + $this->resultMap = $result->toMap(); return $this; } public function getResult() { - return $this->result; - } + $map = $this->resultMap; - public function isForbidden() { - return ($this->getResult() === self::RESULT_FORBIDDEN); + if (is_array($map)) { + $result = HeraldConditionResult::newFromMap($map); + } else { + $legacy_result = $this->result; + + $result_data = array(); + + if ($legacy_result === 'forbidden') { + $result_code = HeraldConditionResult::RESULT_OBJECT_STATE; + $result_data = array( + 'reason' => $this->note, + ); + } else if ($legacy_result === true) { + $result_code = HeraldConditionResult::RESULT_MATCHED; + } else if ($legacy_result === false) { + $result_code = HeraldConditionResult::RESULT_FAILED; + } else { + $result_code = HeraldConditionResult::RESULT_UNKNOWN; + } + + $result = HeraldConditionResult::newFromResultCode($result_code) + ->setResultData($result_data); + } + + return $result; } } diff --git a/webroot/rsrc/css/application/herald/herald-test.css b/webroot/rsrc/css/application/herald/herald-test.css index 67b49145d5..e673020464 100644 --- a/webroot/rsrc/css/application/herald/herald-test.css +++ b/webroot/rsrc/css/application/herald/herald-test.css @@ -4,6 +4,8 @@ .herald-condition-note { color: {$red}; + padding: 4px 0; + margin: 4px 0 8px; } textarea.herald-field-value-transcript {