lucaspalomodevelop/core
1
0
Fork 0
mirror of https://github.com/lucaspalomodevelop/core.git synced 2026-03-14 16:44:39 +00:00
Code Issues Packages Projects Releases Wiki Activity

Firewall: Aliases / generic MVC - performance improvments, closes https://github.com/opnsense/core/issues/7509

Browse Source 
This commit improves alias save/validate performance by fixing two main issues:

1) Faster unique constraint handling using caching, in order to prevent nested loops we should cache our results while still in the validation cycle.

This required an attribute to count validation cycles so we know the model could not have changed in between, getValidationSequence() helps to reach that goal

2) Alias::getByName() more optimistic caching, also prevening nested loops when locating aliases.

Although this is slighly more optimistic than the previous construction, in practice when validating data the set won't change, if there is a risk of mutations, we should flush the set using the "flush" parameter (same as before, but a bit more sensitive).
This commit is contained in:
Ad Schellevis 2024-06-04 16:12:10 +02:00
parent e3856e03a3
commit 930fa4e39f
5 changed files with 106 additions and 78 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

2
src/opnsense/mvc/app/controllers/OPNsense/Firewall/Api/AliasController.php
View File

@ -405,7 +405,7 @@ class AliasController extends ApiMutableModelControllerBase
foreach ($data['aliases']['alias'] as $uuid => $content) {
$type = !empty($content['type']) ? $content['type'] : "";
if (is_array($content) && !empty($content['name']) && $type != 'internal') {
$node = $this->getModel()->getByName($content['name']);
$node = $this->getModel()->getByName($content['name'], true);
if ($node == null) {
$node = $this->getModel()->aliases->alias->Add();
$result['new'] += 1;

19
src/opnsense/mvc/app/models/OPNsense/Base/BaseModel.php
View File

@ -91,6 +91,11 @@ abstract class BaseModel
*/
private $internalMissingUuids = false;
/**
* @var int internal validation sequence (number of times validation has run)
*/
private int $internalValidationSequence = 0;
/**
* If the model needs a custom initializer, override this init() method
* Default behaviour is to do nothing in this init.
@ -110,7 +115,7 @@ abstract class BaseModel
if ($xmlNode->count() == 0) {
$result = (string)$xmlNode;
} else {
$result = array();
$result = [];
foreach ($xmlNode->children() as $childNode) {
// item keys can be overwritten using value attributes
if (!isset($childNode->attributes()['value'])) {
@ -467,6 +472,16 @@ abstract class BaseModel
return str_ends_with($this->internal_mountpoint, '+') && strpos($this->internal_mountpoint, "//") !== 0;
}
/**
* Return the number of times performValidation() has been called.
* This can be practical if validations need to cache outcomes which are consistent for the full validation
* sequence.
* @return int
*/
public function getValidationSequence()
{
return $this->internalValidationSequence;
}
/**
* validate full model using all fields and data in a single (1 deep) array
@ -480,6 +495,8 @@ abstract class BaseModel
$validation_data = array();
$all_nodes = $this->internalData->getFlatNodes();
$this->internalValidationSequence++;
foreach ($all_nodes as $key => $node) {
if ($validateFullModel || $node->isFieldChanged()) {
$node_validators = $node->getValidators();

66
src/opnsense/mvc/app/models/OPNsense/Base/Constraints/UniqueConstraint.php
View File

@ -36,6 +36,9 @@ namespace OPNsense\Base\Constraints;
*/
class UniqueConstraint extends BaseConstraint
{
private static $itemmap = [];
private static $validationSequence = 0;
/**
* Executes validation
*
@ -51,38 +54,53 @@ class UniqueConstraint extends BaseConstraint
if (!$node->isRequired() && empty((string)$node)) {
return true;
}
$containerNode = $node;
$nodeName = $node->getInternalXMLTagName();
$parentNode = $node->getParentNode();
$level = 0;
// dive into parent
while ($containerNode != null && !$containerNode->isArrayType()) {
$containerNode = $containerNode->getParentNode();
$level++;
$mdl = $node->getParentModel();
if ($mdl === null || $mdl->getValidationSequence() != static::$validationSequence) {
// reset cache, new validation round.
static::$validationSequence = $mdl !== null ? $mdl->getValidationSequence() : 0;
static::$itemmap = [];
}
if ($containerNode != null && $level == 2) {
// collect (additional) key fields
$keyFields = array($nodeName);
$keyFields = array_unique(array_merge($keyFields, $this->getOptionValueList('addFields')));
// calculate the key for this node
$nodeKey = '';
foreach ($keyFields as $field) {
$nodeKey .= $fieldSeparator . $parentNode->$field;
$keyFields = array_unique(
array_merge(
[$node->getInternalXMLTagName()],
$this->getOptionValueList('addFields')
)
);
asort($keyFields);
$nodeKey = implode('|', $keyFields);
$parentNode = $node->getParentNode();
// calculate the key for this node
if (!isset(static::$itemmap[$nodeKey])) {
static::$itemmap[$nodeKey] = [];
$level = 0;
// dive into parent
$containerNode = $node;
while ($containerNode != null && !$containerNode->isArrayType()) {
$containerNode = $containerNode->getParentNode();
$level++;
}
// when an ArrayField is found in range, traverse nodes and compare keys
foreach ($containerNode->iterateItems() as $item) {
if ($item !== $parentNode) {
$itemKey = '';
if ($containerNode != null && $level == 2) {
// when an ArrayField is found in range, traverse nodes and compare keys
foreach ($containerNode->iterateItems() as $item) {
$itemValue = '';
foreach ($keyFields as $field) {
$itemKey .= $fieldSeparator . $item->$field;
$itemValue .= $fieldSeparator . $item->$field;
}
if ($itemKey == $nodeKey) {
$this->appendMessage($validator, $attribute);
return false;
if (empty(static::$itemmap[$nodeKey][$itemValue])) {
static::$itemmap[$nodeKey][$itemValue] = 0;
}
static::$itemmap[$nodeKey][$itemValue]++ ;
}
}
}
$nodeValue = '';
foreach ($keyFields as $field) {
$nodeValue .= $fieldSeparator . $parentNode->$field;
}
if ((static::$itemmap[$nodeKey][$nodeValue] ?? 0) > 1) {
$this->appendMessage($validator, $attribute);
return false;
}
}
return true;
}

31
src/opnsense/mvc/app/models/OPNsense/Firewall/Alias.php
View File

@ -217,28 +217,21 @@ class Alias extends BaseModel
public function getByName($name, $flush = false)
{
if ($flush) {
// Flush false negative results (searched earlier, didn't exist at the time) as positive matches will
// always be resolved.
// Flush cache in case model data has been changed.
$this->aliasReferenceCache = [];
}
// cache alias uuid references, but always validate existence before return.
if (empty($this->aliasReferenceCache)) {
// cache alias uuid references, but always validate existence before return.
foreach ($this->aliases->alias->iterateItems() as $uuid => $alias) {
if ((string)$alias->name == $name) {
$this->aliasReferenceCache[(string)$alias->name] = $alias;
}
}
}
if (isset($this->aliasReferenceCache[$name])) {
$uuid = $this->aliasReferenceCache[$name];
if ($uuid === false) {
return null;
}
$alias = $this->getNodeByReference('aliases.alias.' . $uuid);
if ($alias != null && (string)$alias->name == $name) {
return $alias;
}
return $this->aliasReferenceCache[$name];
} else {
return null;
}
foreach ($this->aliases->alias->iterateItems() as $uuid => $alias) {
if ((string)$alias->name == $name) {
$this->aliasReferenceCache[$name] = $uuid;
return $alias;
}
}
$this->aliasReferenceCache[$name] = false;
return null;
}
}

66
src/opnsense/mvc/tests/app/models/OPNsense/Base/Constraints/UniqueConstraintTest.php
View File

@ -34,9 +34,8 @@ use OPNsense\Base\FieldTypes\TextField;
class UniqueTestContainer extends ArrayField
{
private $uniqueConstraints = [];
private $valuesRequired = false;
private $internalNodes = [];
private bool $valuesRequired = false;
private array $internalNodes = [];
/**
* @param $nodes a single node or an array of nodes
@ -48,47 +47,48 @@ class UniqueTestContainer extends ArrayField
{
// UniqueConstraint requires a depth of 2, so add a container node
$container = new ContainerField();
$constraint = new UniqueConstraint();
$this->addChildNode('UniqueTest', $container);
$addFields = [];
$fields_added = false;
$idx = count($this->internalNodes);
$this->addChildNode('item'. $idx, $container);
foreach ($nodes as $name => $value) {
$node = new TextField(null, $name);
$node->setRequired($this->valuesRequired ? "Y" : "N");
if ($name === array_key_first($nodes)) {
$constraint->setOption('node', $node);
$constraint->setOption('name', $name);
$constraint->setOption('ValidationMessage', 'Validation Failed');
} else {
$addFields[] = $name;
$fields_added = true;
}
$node->setValue($value);
$node->setValue((string)$value);
$container->addChildNode($name, $node);
$this->internalNodes[] = $node;
}
if ($fields_added) {
$constraint->setOption('addFields', $addFields);
}
$this->uniqueConstraints[] = $constraint;
$this->internalNodes[] = $nodes;
}
public function setRequired($required)
{
$this->valuesRequired = $required;
foreach ($this->internalNodes as $node) {
/* cover earlier set nodes */
$node->setRequired($this->valuesRequired ? "Y" : "N");
foreach ($this->iterateItems() as $records) {
foreach ($records->iterateItems() as $node) {
/* cover earlier set nodes */
$node->setRequired($this->valuesRequired ? "Y" : "N");
}
}
}
public function validate()
{
$uniqueConstraints = [];
$validator = new \OPNsense\Base\Validation();
foreach ($this->uniqueConstraints as $idx => $constraint) {
foreach ($this->internalNodes as $idx => $nodes) {
$addFields = [];
$constraint = new UniqueConstraint();
foreach ($nodes as $name => $value) {
if ($name === array_key_first($nodes)) {
$constraint->setOption('node', $this->{'item'. $idx}->$name);
$constraint->setOption('name', $name);
$constraint->setOption('ValidationMessage', 'Validation Failed');
} else {
$addFields[] = $name;
}
}
if ($addFields) {
$constraint->setOption('addFields', $addFields);
}
$validator->add($idx, $constraint);
}
$msgs = $validator->validate([]);
@ -111,7 +111,8 @@ class UniqueConstraintTest extends \PHPUnit\Framework\Testcase
$container->addNode(['unique_test' => 'value1']);
$msgs = $container->validate();
$this->assertEquals(1, count($msgs));
$this->assertEquals(2, count($msgs));
}
public function testMultipleNonEqualAndEqualValues()
@ -126,7 +127,7 @@ class UniqueConstraintTest extends \PHPUnit\Framework\Testcase
$container->addNode(['unique_test' => 'value1', 'unique_test2' => 'value2']);
$msgs = $container->validate();
$this->assertEquals(1, count($msgs));
$this->assertEquals(2, count($msgs));
}
public function testEmptyValuesNotRequiredAndRequired()
@ -143,7 +144,7 @@ class UniqueConstraintTest extends \PHPUnit\Framework\Testcase
$msgs = $container->validate();
$this->assertEquals(1, count($msgs));
$this->assertEquals(2, count($msgs));
}
public function testMultipleEmptyValuesNotRequiredAndRequired()
@ -151,7 +152,6 @@ class UniqueConstraintTest extends \PHPUnit\Framework\Testcase
$container = new UniqueTestContainer();
$container->addNode(['unique_test' => '', 'unique_test2' => '']);
$container->addNode(['unique_test' => '', 'unique_test2' => '']);
$msgs = $container->validate();
$this->assertEquals(0, count($msgs));
@ -160,7 +160,7 @@ class UniqueConstraintTest extends \PHPUnit\Framework\Testcase
$msgs = $container->validate();
$this->assertEquals(1, count($msgs));
$this->assertEquals(2, count($msgs));
}
public function testFirstValueEmptyPassAll()
@ -177,6 +177,6 @@ class UniqueConstraintTest extends \PHPUnit\Framework\Testcase
$msgs = $container->validate();
$this->assertEquals(1, count($msgs));
$this->assertEquals(2, count($msgs));
}
}