@recaptime-dev's working patches + fork for Phorge, a community fork of Phabricator. (Upstream dev and stable branches are at upstream/main and upstream/stable respectively.)
hq.recaptime.dev/wiki/Phorge
phorge
phabricator
Make Herald rules more resilient
Summary:
Make Herald conditions and actions more resilient (see discussion in D12896). This protects against invalid rules, which may have been valid in the past but are no longer valid. Specifically:
- If a rule has an invalid field, the conditions fail and the actions do not execute.
- The transcript shows that the rule failed because of an invalid field, and points at the issue.
- If a rule has an invalid action, that action fails but other actions execute.
- The transcript shows that the action failed.
- Everything else (particularly, other rules) continues normally in both cases.
- The edit interface is somewhat working when editing an invalid rule, but it could use some further improvements.
Test Plan:
# Ran this rule on a differential revision and saw the rule fail in the transcript.
# Was able to submit a differential without receiving an `ERR-CONDUIT-CORE`.
# Edited the Herald rule using the UI and was able to save the rule succesfully.
# Ran this rule on a differential revision and saw one success and one failure in the transcript.
# Was able to submit a differential without receiving an `ERR-CONDUIT-CORE`.
# Edited the Herald rule using the UI. Clicking save caused a `HeraldInvalidActionException` to be thrown, but maybe this is okay.
Differential Revision: http://phabricator.local/D41
+65
-14
+15
-3
src/applications/herald/adapter/HeraldAdapter.php
+15
-3
src/applications/herald/adapter/HeraldAdapter.php
···
1203
1203
$rule_global = HeraldRuleTypeConfig::RULE_TYPE_GLOBAL;
1204
1204
1205
1205
$action_type = $action->getAction();
1206
-
$action_name = idx($this->getActionNameMap($rule_global), $action_type);
1206
+
1207
+
$default = $this->isHeraldCustomKey($action_type)
1208
+
? pht('(Unknown Custom Action "%s") equals', $action_type)
1209
+
: pht('(Unknown Action "%s") equals', $action_type);
1210
+
1211
+
$action_name = idx(
1212
+
$this->getActionNameMap($rule_global),
1213
+
$action_type,
1214
+
$default);
1207
1215
1208
1216
$target = $this->renderActionTargetAsText($action, $handles);
1209
1217
···
1525
1533
$supported = $this->getActions($rule_type);
1526
1534
$supported = array_fuse($supported);
1527
1535
if (empty($supported[$action])) {
1528
-
throw new Exception(
1536
+
return new HeraldApplyTranscript(
1537
+
$effect,
1538
+
false,
1529
1539
pht(
1530
1540
'Adapter "%s" does not support action "%s" for rule type "%s".',
1531
1541
get_class($this),
···
1548
1558
$result = $this->handleCustomHeraldEffect($effect);
1549
1559
1550
1560
if (!$result) {
1551
-
throw new Exception(
1561
+
return new HeraldApplyTranscript(
1562
+
$effect,
1563
+
false,
1552
1564
pht(
1553
1565
'No custom action exists to handle rule action "%s".',
1554
1566
$action));
+4
-1
src/applications/herald/adapter/HeraldDifferentialDiffAdapter.php
+4
-1
src/applications/herald/adapter/HeraldDifferentialDiffAdapter.php
+32
-9
src/applications/herald/controller/HeraldRuleController.php
+32
-9
src/applications/herald/controller/HeraldRuleController.php
···
320
320
try {
321
321
$adapter->willSaveAction($rule, $obj);
322
322
} catch (HeraldInvalidActionException $ex) {
323
-
$errors[] = $ex;
323
+
$errors[] = $ex->getMessage();
324
324
}
325
325
326
326
$actions[] = $obj;
···
354
354
if ($rule->getConditions()) {
355
355
$serial_conditions = array();
356
356
foreach ($rule->getConditions() as $condition) {
357
-
358
357
$value = $condition->getValue();
359
358
switch ($condition->getFieldName()) {
360
359
case HeraldAdapter::FIELD_TASK_PRIORITY:
···
394
393
$serial_actions = array(
395
394
array('default', ''),
396
395
);
396
+
397
397
if ($rule->getActions()) {
398
398
$serial_actions = array();
399
399
foreach ($rule->getActions() as $action) {
400
-
401
400
switch ($action->getAction()) {
402
401
case HeraldAdapter::ACTION_FLAG:
403
402
case HeraldAdapter::ACTION_BLOCK:
···
438
437
// names of, so that saving a rule without touching anything doesn't change
439
438
// it.
440
439
foreach ($rule->getConditions() as $condition) {
441
-
if (empty($field_map[$condition->getFieldName()])) {
442
-
$field_map[$condition->getFieldName()] = pht('<Unknown Field>');
440
+
$field_name = $condition->getFieldName();
441
+
442
+
if (empty($field_map[$field_name])) {
443
+
$field_map[$field_name] = pht('<Unknown Field "%s">', $field_name);
443
444
}
444
445
}
445
446
446
447
$actions = $adapter->getActions($rule->getRuleType());
447
448
$action_map = array_select_keys($all_actions, $actions);
448
449
450
+
// Populate any actions which exist in the rule but which we don't know the
451
+
// names of, so that saving a rule without touching anything doesn't change
452
+
// it.
453
+
foreach ($rule->getActions() as $action) {
454
+
$action_name = $action->getAction();
455
+
456
+
if (empty($action_map[$action_name])) {
457
+
$action_map[$action_name] = pht('<Unknown Action "%s">', $action_name);
458
+
}
459
+
}
460
+
461
+
449
462
$config_info = array();
450
463
$config_info['fields'] = $field_map;
451
464
$config_info['conditions'] = $all_conditions;
452
465
$config_info['actions'] = $action_map;
453
466
454
467
foreach ($config_info['fields'] as $field => $name) {
455
-
$field_conditions = $adapter->getConditionsForField($field);
468
+
try {
469
+
$field_conditions = $adapter->getConditionsForField($field);
470
+
} catch (Exception $ex) {
471
+
$field_conditions = array(HeraldAdapter::CONDITION_UNCONDITIONALLY);
472
+
}
456
473
$config_info['conditionMap'][$field] = $field_conditions;
457
474
}
458
475
···
468
485
$config_info['rule_type'] = $rule->getRuleType();
469
486
470
487
foreach ($config_info['actions'] as $action => $name) {
471
-
$config_info['targets'][$action] = $adapter->getValueTypeForAction(
472
-
$action,
473
-
$rule->getRuleType());
488
+
try {
489
+
$action_value = $adapter->getValueTypeForAction(
490
+
$action,
491
+
$rule->getRuleType());
492
+
} catch (Exception $ex) {
493
+
$action_value = array(HeraldAdapter::VALUE_NONE);
494
+
}
495
+
496
+
$config_info['targets'][$action] = $action_value;
474
497
}
475
498
476
499
$changeflag_options =
+4
-1
src/applications/herald/controller/HeraldTranscriptController.php
+4
-1
src/applications/herald/controller/HeraldTranscriptController.php
···
380
380
$item->setState(PHUIObjectItemView::STATE_FAIL);
381
381
}
382
382
383
-
$rule = idx($action_names, $apply_xscript->getAction(), pht('Unknown'));
383
+
$rule = idx(
384
+
$action_names,
385
+
$apply_xscript->getAction(),
386
+
pht('Unknown Action "%s"', $apply_xscript->getAction()));
384
387
385
388
$item->setHeader(pht('%s: %s', $rule, $target));
386
389
$item->addAttribute($apply_xscript->getReason());
+10
src/applications/herald/engine/HeraldEngine.php
+10
src/applications/herald/engine/HeraldEngine.php
···
272
272
$result = false;
273
273
} else {
274
274
foreach ($conditions as $condition) {
275
+
try {
276
+
$object->getHeraldField($condition->getFieldName());
277
+
} catch (Exception $ex) {
278
+
$reason = pht(
279
+
'Field "%s" does not exist!',
280
+
$condition->getFieldName());
281
+
$result = false;
282
+
break;
283
+
}
284
+
275
285
$match = $this->doesConditionMatch($rule, $condition, $object);
276
286
277
287
if (!$all && $match) {