From b767bd3f2d9d679b842b2f2dc5abbe7fd2f2a079 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 6 Aug 2013 11:32:13 -0700 Subject: [PATCH] Move Herald rule querying into HeraldRuleQuery Summary: Ref T2769. The `HeraldRule` class has some query logic; move it into `HeraldRuleQuery`. Also some minor cleanup. Test Plan: Ran test console, created a new revision, used `reparse.php --herald`. Verified rules triggered correctly. Reviewers: btrahan Reviewed By: btrahan CC: aran Maniphest Tasks: T2769 Differential Revision: https://secure.phabricator.com/D6689 --- .../herald/adapter/HeraldCommitAdapter.php | 1 + .../HeraldDifferentialRevisionAdapter.php | 1 + .../HeraldTestConsoleController.php | 10 +- .../herald/engine/HeraldEngine.php | 19 +-- .../herald/query/HeraldRuleQuery.php | 112 ++++++++++++++++++ .../herald/storage/HeraldRule.php | 82 ++----------- ...habricatorRepositoryCommitHeraldWorker.php | 13 +- 7 files changed, 149 insertions(+), 89 deletions(-) diff --git a/src/applications/herald/adapter/HeraldCommitAdapter.php b/src/applications/herald/adapter/HeraldCommitAdapter.php index 639410d753..35aafd77ca 100644 --- a/src/applications/herald/adapter/HeraldCommitAdapter.php +++ b/src/applications/herald/adapter/HeraldCommitAdapter.php @@ -100,6 +100,7 @@ final class HeraldCommitAdapter extends HeraldAdapter { self::ACTION_ADD_CC, self::ACTION_REMOVE_CC, self::ACTION_EMAIL, + self::ACTION_FLAG, self::ACTION_NOTHING, ); } diff --git a/src/applications/herald/adapter/HeraldDifferentialRevisionAdapter.php b/src/applications/herald/adapter/HeraldDifferentialRevisionAdapter.php index f59271db0f..ce3287dd11 100644 --- a/src/applications/herald/adapter/HeraldDifferentialRevisionAdapter.php +++ b/src/applications/herald/adapter/HeraldDifferentialRevisionAdapter.php @@ -258,6 +258,7 @@ final class HeraldDifferentialRevisionAdapter extends HeraldAdapter { self::ACTION_ADD_CC, self::ACTION_REMOVE_CC, self::ACTION_EMAIL, + self::ACTION_FLAG, self::ACTION_NOTHING, ); } diff --git a/src/applications/herald/controller/HeraldTestConsoleController.php b/src/applications/herald/controller/HeraldTestConsoleController.php index 26d682624b..bccdf2769d 100644 --- a/src/applications/herald/controller/HeraldTestConsoleController.php +++ b/src/applications/herald/controller/HeraldTestConsoleController.php @@ -68,9 +68,13 @@ final class HeraldTestConsoleController extends HeraldController { throw new Exception("Can not build adapter for object!"); } - $rules = HeraldRule::loadAllByContentTypeWithFullData( - $adapter->getAdapterContentType(), - $object->getPHID()); + $rules = id(new HeraldRuleQuery()) + ->setViewer($user) + ->withContentTypes(array($adapter->getAdapterContentType())) + ->needConditionsAndActions(true) + ->needAppliedToPHIDs(array($object->getPHID())) + ->needValidateAuthors(true) + ->execute(); $engine = new HeraldEngine(); $effects = $engine->applyRules($rules, $adapter); diff --git a/src/applications/herald/engine/HeraldEngine.php b/src/applications/herald/engine/HeraldEngine.php index b92371ccff..895ef95cb9 100644 --- a/src/applications/herald/engine/HeraldEngine.php +++ b/src/applications/herald/engine/HeraldEngine.php @@ -14,15 +14,18 @@ final class HeraldEngine { return idx($this->rules, $id); } - public static function loadAndApplyRules(HeraldAdapter $object) { - $content_type = $object->getAdapterContentType(); - $rules = HeraldRule::loadAllByContentTypeWithFullData( - $content_type, - $object->getPHID()); + public static function loadAndApplyRules(HeraldAdapter $adapter) { + $rules = id(new HeraldRuleQuery()) + ->setViewer(PhabricatorUser::getOmnipotentUser()) + ->withContentTypes(array($adapter->getAdapterContentType())) + ->needConditionsAndActions(true) + ->needAppliedToPHIDs(array($adapter->getPHID())) + ->needValidateAuthors(true) + ->execute(); $engine = new HeraldEngine(); - $effects = $engine->applyRules($rules, $object); - $engine->applyEffects($effects, $object, $rules); + $effects = $engine->applyRules($rules, $adapter); + $engine->applyEffects($effects, $adapter, $rules); return $engine->getTranscript(); } @@ -215,7 +218,7 @@ final class HeraldEngine { } else if (!$conditions) { $reason = "Rule failed automatically because it has no conditions."; $result = false; - } else if ($rule->hasInvalidOwner()) { + } else if (!$rule->hasValidAuthor()) { $reason = "Rule failed automatically because its owner is invalid ". "or disabled."; $result = false; diff --git a/src/applications/herald/query/HeraldRuleQuery.php b/src/applications/herald/query/HeraldRuleQuery.php index df0e31d045..7b23231e6f 100644 --- a/src/applications/herald/query/HeraldRuleQuery.php +++ b/src/applications/herald/query/HeraldRuleQuery.php @@ -9,6 +9,10 @@ final class HeraldRuleQuery private $ruleTypes; private $contentTypes; + private $needConditionsAndActions; + private $needAppliedToPHIDs; + private $needValidateAuthors; + public function withIDs(array $ids) { $this->ids = $ids; return $this; @@ -34,6 +38,26 @@ final class HeraldRuleQuery return $this; } + public function withExecutableRules($executable) { + $this->executable = $executable; + return $this; + } + + public function needConditionsAndActions($need) { + $this->needConditionsAndActions = $need; + return $this; + } + + public function needAppliedToPHIDs(array $phids) { + $this->needAppliedToPHIDs = $phids; + return $this; + } + + public function needValidateAuthors($need) { + $this->needValidateAuthors = $need; + return $this; + } + public function loadPage() { $table = new HeraldRule(); $conn_r = $table->establishConnection('r'); @@ -49,6 +73,56 @@ final class HeraldRuleQuery return $table->loadAllFromArray($data); } + public function willFilterPage(array $rules) { + $rule_ids = mpull($rules, 'getID'); + + if ($this->needValidateAuthors) { + $this->validateRuleAuthors($rules); + } + + if ($this->needConditionsAndActions) { + $conditions = id(new HeraldCondition())->loadAllWhere( + 'ruleID IN (%Ld)', + $rule_ids); + $conditions = mgroup($conditions, 'getRuleID'); + + $actions = id(new HeraldAction())->loadAllWhere( + 'ruleID IN (%Ld)', + $rule_ids); + $actions = mgroup($actions, 'getRuleID'); + + foreach ($rules as $rule) { + $rule->attachActions(idx($actions, $rule->getID(), array())); + $rule->attachConditions(idx($conditions, $rule->getID(), array())); + } + } + + if ($this->needAppliedToPHIDs) { + $conn_r = id(new HeraldRule())->establishConnection('r'); + $applied = queryfx_all( + $conn_r, + 'SELECT * FROM %T WHERE ruleID IN (%Ld) AND phid IN (%Ls)', + HeraldRule::TABLE_RULE_APPLIED, + $rule_ids, + $this->needAppliedToPHIDs); + + $map = array(); + foreach ($applied as $row) { + $map[$row['ruleID']][$row['phid']] = true; + } + + foreach ($rules as $rule) { + foreach ($this->needAppliedToPHIDs as $phid) { + $rule->setRuleApplied( + $phid, + isset($map[$rule->getID()][$phid])); + } + } + } + + return $rules; + } + private function buildWhereClause($conn_r) { $where = array(); @@ -92,4 +166,42 @@ final class HeraldRuleQuery return $this->formatWhereClause($where); } + private function validateRuleAuthors(array $rules) { + + // "Global" rules always have valid authors. + foreach ($rules as $key => $rule) { + if ($rule->isGlobalRule()) { + $rule->attachValidAuthor(true); + unset($rules[$key]); + continue; + } + } + + if (!$rules) { + return; + } + + // For personal rules, the author needs to exist and not be disabled. + $user_phids = mpull($rules, 'getAuthorPHID'); + $users = id(new PhabricatorPeopleQuery()) + ->setViewer($this->getViewer()) + ->withPHIDs($user_phids) + ->execute(); + $users = mpull($users, null, 'getPHID'); + + foreach ($rules as $key => $rule) { + $author_phid = $rule->getAuthorPHID(); + if (empty($users[$author_phid])) { + $rule->attachValidAuthor(false); + continue; + } + if ($users[$author_phid]->getIsDisabled()) { + $rule->attachValidAuthor(false); + continue; + } + + $rule->attachValidAuthor(true); + } + } + } diff --git a/src/applications/herald/storage/HeraldRule.php b/src/applications/herald/storage/HeraldRule.php index 8b326b20cc..b5dc2f62cd 100644 --- a/src/applications/herald/storage/HeraldRule.php +++ b/src/applications/herald/storage/HeraldRule.php @@ -16,7 +16,7 @@ final class HeraldRule extends HeraldDAO protected $configVersion = 9; private $ruleApplied = array(); // phids for which this rule has been applied - private $invalidOwner = false; + private $validAuthor = self::ATTACHABLE; private $conditions; private $actions; @@ -30,77 +30,6 @@ final class HeraldRule extends HeraldDAO return PhabricatorPHID::generateNewPHID(HeraldPHIDTypeRule::TYPECONST); } - public static function loadAllByContentTypeWithFullData( - $content_type, - $object_phid) { - - $rules = id(new HeraldRule())->loadAllWhere( - 'contentType = %s', - $content_type); - - if (!$rules) { - return array(); - } - - self::flagDisabledUserRules($rules); - - $rule_ids = mpull($rules, 'getID'); - - $conditions = id(new HeraldCondition())->loadAllWhere( - 'ruleID in (%Ld)', - $rule_ids); - - $actions = id(new HeraldAction())->loadAllWhere( - 'ruleID in (%Ld)', - $rule_ids); - - $applied = queryfx_all( - id(new HeraldRule())->establishConnection('r'), - 'SELECT * FROM %T WHERE phid = %s', - self::TABLE_RULE_APPLIED, - $object_phid); - $applied = ipull($applied, null, 'ruleID'); - - $conditions = mgroup($conditions, 'getRuleID'); - $actions = mgroup($actions, 'getRuleID'); - $applied = igroup($applied, 'ruleID'); - - foreach ($rules as $rule) { - $rule->setRuleApplied($object_phid, isset($applied[$rule->getID()])); - - $rule->attachConditions(idx($conditions, $rule->getID(), array())); - $rule->attachActions(idx($actions, $rule->getID(), array())); - } - - return $rules; - } - - private static function flagDisabledUserRules(array $rules) { - assert_instances_of($rules, 'HeraldRule'); - - $users = array(); - foreach ($rules as $rule) { - if ($rule->getRuleType() != HeraldRuleTypeConfig::RULE_TYPE_PERSONAL) { - continue; - } - $users[$rule->getAuthorPHID()] = true; - } - - $handles = id(new PhabricatorObjectHandleData(array_keys($users))) - ->setViewer(PhabricatorUser::getOmnipotentUser()) - ->loadHandles(); - - foreach ($rules as $key => $rule) { - if ($rule->getRuleType() != HeraldRuleTypeConfig::RULE_TYPE_PERSONAL) { - continue; - } - $handle = $handles[$rule->getAuthorPHID()]; - if (!$handle->isComplete() || $handle->isDisabled()) { - $rule->invalidOwner = true; - } - } - } - public function getRuleApplied($phid) { if (idx($this->ruleApplied, $phid) === null) { throw new Exception("Call setRuleApplied() before getRuleApplied()!"); @@ -229,8 +158,13 @@ final class HeraldRule extends HeraldDAO // $this->saveTransaction(); } - public function hasInvalidOwner() { - return $this->invalidOwner; + public function hasValidAuthor() { + return $this->assertAttached($this->validAuthor); + } + + public function attachValidAuthor($valid) { + $this->validAuthor = $valid; + return $this; } public function isGlobalRule() { diff --git a/src/applications/repository/worker/PhabricatorRepositoryCommitHeraldWorker.php b/src/applications/repository/worker/PhabricatorRepositoryCommitHeraldWorker.php index 9e74c6a820..9ea559a369 100644 --- a/src/applications/repository/worker/PhabricatorRepositoryCommitHeraldWorker.php +++ b/src/applications/repository/worker/PhabricatorRepositoryCommitHeraldWorker.php @@ -21,14 +21,19 @@ final class PhabricatorRepositoryCommitHeraldWorker return; } - $rules = HeraldRule::loadAllByContentTypeWithFullData( - id(new HeraldCommitAdapter())->getAdapterContentType(), - $commit->getPHID()); - $adapter = HeraldCommitAdapter::newLegacyAdapter( $repository, $commit, $data); + + $rules = id(new HeraldRuleQuery()) + ->setViewer(PhabricatorUser::getOmnipotentUser()) + ->withContentTypes(array($adapter->getAdapterContentType())) + ->needConditionsAndActions(true) + ->needAppliedToPHIDs(array($adapter->getPHID())) + ->needValidateAuthors(true) + ->execute(); + $engine = new HeraldEngine(); $effects = $engine->applyRules($rules, $adapter);