From 147a5d0ffb206a4447b62f327d1819cd70466446 Mon Sep 17 00:00:00 2001 From: Chad Sikorra Date: Wed, 16 Sep 2026 20:56:07 -0400 Subject: [PATCH 1/4] Update only the index rows a write actually changes. Ask for equality only between values that could possibly be equal. --- .../DirectoryServerContainerProvider.php | 1 + .../Matching/EqualityComparatorResolver.php | 21 +- .../Ldap/Schema/Matching/EquivalentValues.php | 119 ++++++++++ .../Schema/Validation/SchemaValidator.php | 59 ++--- .../Adapter/Dialect/PdoDialectTrait.php | 5 +- .../Dialect/PdoEntryDialectInterface.php | 4 +- .../Storage/Adapter/Pdo/EntryIndexWriter.php | 167 +++++++++++--- .../SubstringIndex/TrigramSubstringIndex.php | 21 +- .../Storage/Concern/WriteTestsTrait.php | 72 +++++++ .../Schema/Matching/EquivalentValuesTest.php | 106 +++++++++ .../Schema/Validation/SchemaValidatorTest.php | 137 ++++++++++++ .../Storage/Adapter/PdoStorageTest.php | 203 ++++++++++++++++++ 12 files changed, 837 insertions(+), 78 deletions(-) create mode 100644 src/FreeDSx/Ldap/Schema/Matching/EquivalentValues.php create mode 100644 tests/unit/Schema/Matching/EquivalentValuesTest.php diff --git a/src/FreeDSx/Ldap/Container/Provider/DirectoryServerContainerProvider.php b/src/FreeDSx/Ldap/Container/Provider/DirectoryServerContainerProvider.php index ec9a2ae6..89170c56 100644 --- a/src/FreeDSx/Ldap/Container/Provider/DirectoryServerContainerProvider.php +++ b/src/FreeDSx/Ldap/Container/Provider/DirectoryServerContainerProvider.php @@ -479,6 +479,7 @@ private function buildSchemaValidator(Container $container): SchemaValidator return new SchemaValidator( $options->getSchema(), $options->getSchemaValidationMode(), + equalityResolver: $container->get(EqualityComparatorResolver::class), ); } diff --git a/src/FreeDSx/Ldap/Schema/Matching/EqualityComparatorResolver.php b/src/FreeDSx/Ldap/Schema/Matching/EqualityComparatorResolver.php index 5c8c086e..fcb9885d 100644 --- a/src/FreeDSx/Ldap/Schema/Matching/EqualityComparatorResolver.php +++ b/src/FreeDSx/Ldap/Schema/Matching/EqualityComparatorResolver.php @@ -22,11 +22,16 @@ * * @author Chad Sikorra */ -final readonly class EqualityComparatorResolver +final class EqualityComparatorResolver { + /** + * @var array Memoized per type, since resolving walks the SUP chain. + */ + private array $resolved = []; + public function __construct( - private Schema $schema, - private MatchingRuleComparatorInterface $default = new CaseIgnoreComparator(), + private readonly Schema $schema, + private readonly MatchingRuleComparatorInterface $default = new CaseIgnoreComparator(), ) {} /** @@ -34,8 +39,14 @@ public function __construct( */ public function for(string $attributeName): MatchingRuleComparatorInterface { - // Options are not part of the type, so they are dropped before asking the schema about it. - $equalityOid = $this->schema->getEqualityRuleOid(Attribute::normalizeName($attributeName)); + $type = Attribute::normalizeName($attributeName); + + return $this->resolved[$type] ??= $this->resolve($type); + } + + private function resolve(string $type): MatchingRuleComparatorInterface + { + $equalityOid = $this->schema->getEqualityRuleOid($type); $comparator = $equalityOid !== null ? $this->schema->getComparator($equalityOid) : null; diff --git a/src/FreeDSx/Ldap/Schema/Matching/EquivalentValues.php b/src/FreeDSx/Ldap/Schema/Matching/EquivalentValues.php new file mode 100644 index 00000000..740e7686 --- /dev/null +++ b/src/FreeDSx/Ldap/Schema/Matching/EquivalentValues.php @@ -0,0 +1,119 @@ + + * + * For the full copyright and license information, please view the LICENSE + * file that was distributed with this source code. + */ + +namespace FreeDSx\Ldap\Schema\Matching; + +/** + * An attribute's values grouped by their rule's index key, so a candidate is only put to the values that could match. + * + * @author Chad Sikorra + */ +final class EquivalentValues +{ + /** + * @var array> Values by index key, each keyed by the position it arrived at. + */ + private array $keyed = []; + + /** + * @var array Values the rule cannot key, keyed by the position each arrived at. + */ + private array $unkeyed = []; + + private int $position = 0; + + private function __construct( + private readonly MatchingRuleComparatorInterface $comparator, + private readonly ?IndexableComparatorInterface $indexable, + ) {} + + /** + * @param iterable $values + */ + public static function of( + MatchingRuleComparatorInterface $comparator, + iterable $values, + ): self { + $held = new self( + $comparator, + $comparator instanceof IndexableComparatorInterface + ? $comparator + : null, + ); + + foreach ($values as $value) { + $held->add($value); + } + + return $held; + } + + /** + * The first value equivalent to one before it, or null when every value is distinct under the rule. + * + * @param iterable $values + */ + public static function firstDuplicate( + MatchingRuleComparatorInterface $comparator, + iterable $values, + ): ?string { + $held = self::of($comparator, []); + + foreach ($values as $value) { + if ($held->containsEquivalentOf($value)) { + return $value; + } + + $held->add($value); + } + + return null; + } + + public function add(string $value): void + { + $key = $this->indexable?->indexKey($value); + + if ($key === null) { + $this->unkeyed[$this->position++] = $value; + + return; + } + + $this->keyed[$key][$this->position++] = $value; + } + + public function containsEquivalentOf(string $candidate): bool + { + return $this->matching($candidate) !== []; + } + + /** + * The held values equal to the candidate, keyed by the position each arrived at. + * + * @return array + */ + public function matching(string $candidate): array + { + // Equal values share a key, so one the rule cannot key can only equal another it cannot key. + $key = $this->indexable?->indexKey($candidate); + $candidates = $key === null + ? $this->unkeyed + : $this->keyed[$key] ?? []; + + return array_filter( + $candidates, + fn(string $held): bool => $this->comparator->equals($held, $candidate), + ); + } +} diff --git a/src/FreeDSx/Ldap/Schema/Validation/SchemaValidator.php b/src/FreeDSx/Ldap/Schema/Validation/SchemaValidator.php index b617aed9..2df87ef0 100644 --- a/src/FreeDSx/Ldap/Schema/Validation/SchemaValidator.php +++ b/src/FreeDSx/Ldap/Schema/Validation/SchemaValidator.php @@ -23,7 +23,7 @@ use FreeDSx\Ldap\Schema\Definition\ObjectClass; use FreeDSx\Ldap\Schema\Definition\ObjectClassType; use FreeDSx\Ldap\Schema\Matching\EqualityComparatorResolver; -use FreeDSx\Ldap\Schema\Matching\MatchingRuleComparatorInterface; +use FreeDSx\Ldap\Schema\Matching\EquivalentValues; use FreeDSx\Ldap\Schema\Schema; use FreeDSx\Ldap\Schema\SchemaValidationMode; use FreeDSx\Ldap\Schema\Validation\Syntax\AttributeSyntaxResolver; @@ -106,7 +106,11 @@ public function validateModify( if (!$isSystem) { $this->checkNoUserModificationInChanges($command->changes); } - $this->checkNoEquivalentValues($result); + // Only what the change touched, since the rest was already checked when it was written. + $this->checkNoEquivalentValues( + $result, + self::namesChangedBy($command->changes), + ); $this->checkStructuralClassUnchanged($result); $this->validateStructure($result); } @@ -267,11 +271,18 @@ private function checkDistinctAttributeDescriptions(Entry $entry): void /** * RFC 4511 §4.1.7: no two of an attribute's values may be equivalent. * + * @param ?array $only Lowercased attribute names to check, or null for every attribute. * @throws OperationException */ - private function checkNoEquivalentValues(Entry $entry): void - { + private function checkNoEquivalentValues( + Entry $entry, + ?array $only = null, + ): void { foreach ($entry->getAttributes() as $attr) { + if ($only !== null && !isset($only[Attribute::normalizeName($attr->getDescription())])) { + continue; + } + if (!$this->hasEquivalentValues($attr)) { continue; } @@ -284,39 +295,31 @@ private function checkNoEquivalentValues(Entry $entry): void } /** - * Equivalence is the type's equality rule rather than a normalized key, so each value is put to the ones before it. + * The attribute types a change list touches, normalized the way every other type lookup normalizes. + * + * @param Change[] $changes + * @return array */ - private function hasEquivalentValues(Attribute $attr): bool + private static function namesChangedBy(array $changes): array { - $comparator = $this->equalityResolver->for($attr->getName()); - $seen = []; + $names = []; - foreach ($attr->getValues() as $value) { - if ($this->equalsAny($comparator, $seen, $value)) { - return true; - } - - $seen[] = $value; + foreach ($changes as $change) { + $names[Attribute::normalizeName($change->getAttribute()->getDescription())] = true; } - return false; + return $names; } /** - * @param list $values + * Grouped by the rule's index key, so equality is only asked of values that could answer it. */ - private function equalsAny( - MatchingRuleComparatorInterface $comparator, - array $values, - string $candidate, - ): bool { - foreach ($values as $value) { - if ($comparator->equals($value, $candidate)) { - return true; - } - } - - return false; + private function hasEquivalentValues(Attribute $attr): bool + { + return EquivalentValues::firstDuplicate( + $this->equalityResolver->for($attr->getName()), + $attr->getValues(), + ) !== null; } /** diff --git a/src/FreeDSx/Ldap/Server/Backend/Storage/Adapter/Dialect/PdoDialectTrait.php b/src/FreeDSx/Ldap/Server/Backend/Storage/Adapter/Dialect/PdoDialectTrait.php index 54b18d23..be991171 100644 --- a/src/FreeDSx/Ldap/Server/Backend/Storage/Adapter/Dialect/PdoDialectTrait.php +++ b/src/FreeDSx/Ldap/Server/Backend/Storage/Adapter/Dialect/PdoDialectTrait.php @@ -250,14 +250,15 @@ public function querySidecarDelete(): string SQL; } - public function querySidecarDeleteNames(int $count): string + public function querySidecarDeleteValues(int $count): string { $markers = SqlFilterUtility::markers($count); return <<changedNames($entry, $current); + $next = $this->valuesByName($entry); + $previous = $this->valuesByName($current); + $changed = $this->changedNames($next, $previous); + if ($changed === []) { return; } - $this->statements->execute( - $this->dialect->querySidecarDeleteNames(count($changed)), - [$entryId, ...$changed], - ); - $this->insertRows( - $entryId, - $entry, - array_fill_keys($changed, true), - ); + foreach ($changed as $name) { + $this->applyValueDelta( + $entryId, + $name, + $next[$name] ?? [], + $previous[$name] ?? [], + ); + } // A substring index keys off its own attribute set, so it only needs redoing when one of those changed. if (!$this->indexCovers($changed)) { @@ -80,18 +87,79 @@ public function update( $this->maintainSubstringIndex($entryId, $entry); } + /** + * Writes one attribute's difference, so a single added value costs one row rather than a full rewrite. + * + * @param list $next + * @param list $previous + */ + private function applyValueDelta( + int $entryId, + string $attrNameLower, + array $next, + array $previous, + ): void { + // Diffed on the stored row, not the raw value, so a difference the index key folds away cannot strand a row. + $wanted = $this->rowsByKey($attrNameLower, $next); + $held = $this->rowsByKey($attrNameLower, $previous); + + $removed = array_keys(array_diff_key($held, $wanted)); + $added = array_values(array_diff_key($wanted, $held)); + + foreach (array_chunk($removed, self::SIDECAR_ROWS_PER_STATEMENT) as $chunk) { + $this->statements->execute( + $this->dialect->querySidecarDeleteValues(count($chunk)), + [ + $entryId, + $attrNameLower, + ...$chunk, + ], + ); + } + + $this->insertRowsFor( + $entryId, + $attrNameLower, + $added, + ); + } + + /** + * One attribute's values in their stored form, keyed by the index key the sidecar row carries. + * + * @param list $values + * + * @return array value_lower => [value_lower, value_original] + */ + private function rowsByKey( + string $attrNameLower, + array $values, + ): array { + $rows = []; + + foreach ($values as $value) { + $key = $this->valueLower($attrNameLower, $value); + $rows[$key] = [ + $key, + $this->valueOriginal($attrNameLower, $value), + ]; + } + + return $rows; + } + /** * Lowercased names whose value set differs between the two entries, in either direction. * + * @param array> $next + * @param array> $previous + * * @return list */ private function changedNames( - Entry $entry, - Entry $current, + array $next, + array $previous, ): array { - $next = $this->valuesByName($entry); - $previous = $this->valuesByName($current); - $changed = []; foreach ($next as $name => $values) { if (($previous[$name] ?? null) !== $values) { @@ -179,27 +247,58 @@ private function insertRows( Entry $entry, ?array $only = null, ): void { - $rows = $this->buildRows($entryId, $entry, $only); - if ($rows === []) { - return; - } + $this->insert($this->buildRows($entryId, $entry, $only)); + } - $placeholders = SqlFilterUtility::markers( - count($rows), - '(?, ?, ?, ?)', - ); - $params = []; - foreach ($rows as $row) { - $params[] = $row[0]; - $params[] = $row[1]; - $params[] = $row[2]; - $params[] = $row[3]; + /** + * The rows for one attribute's added values, already reduced to their stored form by the delta. + * + * @param list $values [value_lower, value_original] pairs + */ + private function insertRowsFor( + int $entryId, + string $attrNameLower, + array $values, + ): void { + $rows = []; + + foreach ($values as [$valueLower, $valueOriginal]) { + $rows[] = [ + $entryId, + $attrNameLower, + $valueLower, + $valueOriginal, + ]; } - $this->statements->execute( - $this->dialect->querySidecarInsertPrefix() . $placeholders, - $params, - ); + $this->insert($rows); + } + + /** + * Chunked so the placeholder count stays inside the bound SQLite builds before 3.32 compile in. + * + * @param list $rows + */ + private function insert(array $rows): void + { + foreach (array_chunk($rows, self::SIDECAR_ROWS_PER_STATEMENT) as $chunk) { + $placeholders = SqlFilterUtility::markers( + count($chunk), + '(?, ?, ?, ?)', + ); + $params = []; + foreach ($chunk as $row) { + $params[] = $row[0]; + $params[] = $row[1]; + $params[] = $row[2]; + $params[] = $row[3]; + } + + $this->statements->execute( + $this->dialect->querySidecarInsertPrefix() . $placeholders, + $params, + ); + } } /** diff --git a/src/FreeDSx/Ldap/Server/Backend/Storage/Adapter/SubstringIndex/TrigramSubstringIndex.php b/src/FreeDSx/Ldap/Server/Backend/Storage/Adapter/SubstringIndex/TrigramSubstringIndex.php index 073db46e..e458eec7 100644 --- a/src/FreeDSx/Ldap/Server/Backend/Storage/Adapter/SubstringIndex/TrigramSubstringIndex.php +++ b/src/FreeDSx/Ldap/Server/Backend/Storage/Adapter/SubstringIndex/TrigramSubstringIndex.php @@ -47,6 +47,11 @@ final class TrigramSubstringIndex implements SubstringIndexInterface */ private const UNCOVERED = "\x01"; + /** + * Rows per insert. Three placeholders each, inside the 999 bound SQLite builds before 3.32 compile in. + */ + private const ROWS_PER_STATEMENT = 300; + private const DELETE_SQL = <<placeholders(count($rows)), - ), - $this->flatten($rows), - ); + foreach (array_chunk($rows, self::ROWS_PER_STATEMENT) as $chunk) { + $execute( + sprintf( + self::INSERT_SQL, + $this->placeholders(count($chunk)), + ), + $this->flatten($chunk), + ); + } } public function buildSubstringPredicate( diff --git a/tests/integration/Storage/Concern/WriteTestsTrait.php b/tests/integration/Storage/Concern/WriteTestsTrait.php index 40e88008..84c7648f 100644 --- a/tests/integration/Storage/Concern/WriteTestsTrait.php +++ b/tests/integration/Storage/Concern/WriteTestsTrait.php @@ -1424,6 +1424,78 @@ public function testAddStoresAValueLargerThanSixtyFourKilobytes(): void $this->ldapClient()->delete('cn=oversized,dc=foo,dc=bar'); } + public function testAddStoresAnAttributeWiderThanOneIndexStatement(): void + { + $this->authenticateAdmin(); + $values = []; + foreach (range(1, 300) as $i) { + $values[] = "wide value {$i}"; + } + + $this->ldapClient()->create(new Entry( + 'cn=wideattr,dc=foo,dc=bar', + new Attribute('objectClass', 'top', 'inetOrgPerson'), + new Attribute('cn', 'wideattr'), + new Attribute('sn', 'Wide'), + new Attribute('description', ...$values), + )); + + try { + self::assertCount( + 300, + $this->ldapClient()->read('cn=wideattr,dc=foo,dc=bar')?->get('description')?->getValues() ?? [], + ); + self::assertCount( + 1, + $this->ldapClient()->search( + Operations::search(Filters::equal('description', 'wide value 300'), 'cn') + ->base('dc=foo,dc=bar') + ->useSubtreeScope(), + )->toArray(), + ); + } finally { + $this->ldapClient()->delete('cn=wideattr,dc=foo,dc=bar'); + } + } + + public function testModifyOfAWideAttributeKeepsEveryOtherValueSearchable(): void + { + $this->authenticateAdmin(); + $values = []; + foreach (range(1, 300) as $i) { + $values[] = "growing value {$i}"; + } + + $this->ldapClient()->create(new Entry( + 'cn=growingattr,dc=foo,dc=bar', + new Attribute('objectClass', 'top', 'inetOrgPerson'), + new Attribute('cn', 'growingattr'), + new Attribute('sn', 'Growing'), + new Attribute('description', ...$values), + )); + + try { + $this->ldapClient()->send(Operations::modify( + 'cn=growingattr,dc=foo,dc=bar', + Change::add(new Attribute('description', 'growing value 301')), + )); + + foreach (['growing value 1', 'growing value 301'] as $value) { + self::assertCount( + 1, + $this->ldapClient()->search( + Operations::search(Filters::equal('description', $value), 'cn') + ->base('dc=foo,dc=bar') + ->useSubtreeScope(), + )->toArray(), + $value, + ); + } + } finally { + $this->ldapClient()->delete('cn=growingattr,dc=foo,dc=bar'); + } + } + public function testAnAttributeTypeWithinTheStorageBoundIsStored(): void { $this->authenticateAdmin(); diff --git a/tests/unit/Schema/Matching/EquivalentValuesTest.php b/tests/unit/Schema/Matching/EquivalentValuesTest.php new file mode 100644 index 00000000..a8bec8bd --- /dev/null +++ b/tests/unit/Schema/Matching/EquivalentValuesTest.php @@ -0,0 +1,106 @@ + + * + * For the full copyright and license information, please view the LICENSE + * file that was distributed with this source code. + */ + +namespace Tests\Unit\FreeDSx\Ldap\Schema\Matching; + +use FreeDSx\Ldap\Schema\Matching\Comparator\CaseExactComparator; +use FreeDSx\Ldap\Schema\Matching\Comparator\CaseIgnoreComparator; +use FreeDSx\Ldap\Schema\Matching\Comparator\GeneralizedTimeComparator; +use FreeDSx\Ldap\Schema\Matching\Comparator\IntegerComparator; +use FreeDSx\Ldap\Schema\Matching\Comparator\NameAndOptionalUidComparator; +use FreeDSx\Ldap\Schema\Matching\EquivalentValues; +use PHPUnit\Framework\TestCase; + +final class EquivalentValuesTest extends TestCase +{ + public function test_it_finds_a_duplicate_the_rule_folds_together(): void + { + self::assertSame( + 'same', + EquivalentValues::firstDuplicate( + new CaseIgnoreComparator(), + ['SAME', 'other', 'same'], + ), + ); + } + + public function test_it_keeps_values_a_case_exact_rule_treats_as_distinct(): void + { + self::assertNull(EquivalentValues::firstDuplicate( + new CaseExactComparator(), + ['https://Example.test', 'https://example.test'], + )); + } + + public function test_it_keeps_values_whose_index_keys_collide_on_the_separator(): void + { + $comparator = new NameAndOptionalUidComparator(); + $withUid = "cn=a,dc=x#'0101'B"; + $withoutUid = 'cn=a,dc=x#0101'; + + self::assertSame( + $comparator->indexKey($withUid), + $comparator->indexKey($withoutUid), + ); + self::assertNull(EquivalentValues::firstDuplicate( + $comparator, + [$withUid, $withoutUid], + )); + } + + public function test_it_keeps_two_values_the_rule_cannot_key(): void + { + self::assertNull(EquivalentValues::firstDuplicate( + new GeneralizedTimeComparator(), + ['not a time', 'also not a time'], + )); + } + + public function test_it_finds_a_duplicate_under_a_rule_that_cannot_be_keyed(): void + { + self::assertSame( + '01', + EquivalentValues::firstDuplicate( + new IntegerComparator(), + ['1', '01'], + ), + ); + } + + public function test_it_reports_the_positions_the_matching_values_arrived_at(): void + { + $held = EquivalentValues::of( + new CaseIgnoreComparator(), + ['alpha', 'beta', 'ALPHA'], + ); + + self::assertSame( + [0 => 'alpha', 2 => 'ALPHA'], + $held->matching('Alpha'), + ); + } + + public function test_it_reports_nothing_matching_a_value_it_does_not_hold(): void + { + $held = EquivalentValues::of( + new CaseIgnoreComparator(), + ['alpha'], + ); + + self::assertSame( + [], + $held->matching('beta'), + ); + self::assertFalse($held->containsEquivalentOf('beta')); + } +} diff --git a/tests/unit/Schema/Validation/SchemaValidatorTest.php b/tests/unit/Schema/Validation/SchemaValidatorTest.php index 34a16e9f..7e5e17c6 100644 --- a/tests/unit/Schema/Validation/SchemaValidatorTest.php +++ b/tests/unit/Schema/Validation/SchemaValidatorTest.php @@ -21,15 +21,21 @@ use FreeDSx\Ldap\Exception\OperationException; use FreeDSx\Ldap\Operation\ResultCode; use FreeDSx\Ldap\Schema\Definition\AttributeType; +use FreeDSx\Ldap\Schema\Definition\MatchingRule; use FreeDSx\Ldap\Schema\Definition\ObjectClass; use FreeDSx\Ldap\Schema\Definition\ObjectClassType; use FreeDSx\Ldap\Schema\Definition\SyntaxOid; +use FreeDSx\Ldap\Schema\Matching\Comparator\CaseIgnoreComparator; +use FreeDSx\Ldap\Schema\Matching\IndexableComparatorInterface; +use FreeDSx\Ldap\Schema\Matching\MatchingRuleComparatorInterface; use FreeDSx\Ldap\Schema\Schema; use FreeDSx\Ldap\Schema\SchemaValidationMode; use FreeDSx\Ldap\Schema\SchemaResource; use FreeDSx\Ldap\Schema\Validation\SchemaValidator; use FreeDSx\Ldap\Server\Backend\Write\Command\UpdateCommand; use PHPUnit\Framework\Attributes\DataProvider; +use PHPUnit\Framework\MockObject\MockObject; +use PHPUnit\Framework\MockObject\Rule\InvocationOrder; use PHPUnit\Framework\TestCase; final class SchemaValidatorTest extends TestCase @@ -303,6 +309,72 @@ public function test_add_keeps_values_a_case_exact_rule_treats_as_distinct(): vo $this->subject->validateAdd($entry); } + public function test_add_asks_no_equality_of_values_that_cannot_match(): void + { + $values = []; + foreach (range(1, 200) as $i) { + $values[] = "value {$i}"; + } + + $this->widgetValidator(self::never())->validateAdd(new Entry( + new Dn('cn=counted,dc=example,dc=com'), + new Attribute('objectClass', 'widget'), + new Attribute('widgetLabel', ...$values), + )); + } + + public function test_add_asks_equality_only_of_values_sharing_an_index_key(): void + { + $values = []; + foreach (range(1, 199) as $i) { + $values[] = "value {$i}"; + } + $values[] = 'VALUE 199'; + + self::expectException(OperationException::class); + self::expectExceptionCode(ResultCode::ATTRIBUTE_OR_VALUE_EXISTS); + + $this->widgetValidator(self::once())->validateAdd(new Entry( + new Dn('cn=counted,dc=example,dc=com'), + new Attribute('objectClass', 'widget'), + new Attribute('widgetLabel', ...$values), + )); + } + + public function test_modify_rejects_a_duplicate_the_change_introduces(): void + { + $command = new UpdateCommand( + new Dn('cn=counted,dc=example,dc=com'), + [Change::add(new Attribute('widgetLabel', 'SAME'))], + ); + $result = new Entry( + new Dn('cn=counted,dc=example,dc=com'), + new Attribute('objectClass', 'widget'), + new Attribute('widgetLabel', 'same', 'SAME'), + ); + + self::expectException(OperationException::class); + self::expectExceptionCode(ResultCode::ATTRIBUTE_OR_VALUE_EXISTS); + + $this->widgetValidator(self::once())->validateModify($command, $result); + } + + public function test_modify_leaves_an_attribute_the_change_does_not_touch_unchecked(): void + { + $command = new UpdateCommand( + new Dn('cn=counted,dc=example,dc=com'), + [Change::replace(new Attribute('widgetNote', 'after'))], + ); + $result = new Entry( + new Dn('cn=counted,dc=example,dc=com'), + new Attribute('objectClass', 'widget'), + new Attribute('widgetNote', 'after'), + new Attribute('widgetLabel', 'same', 'SAME'), + ); + + $this->widgetValidator(self::never())->validateModify($command, $result); + } + public function test_valid_modify_passes(): void { $this->expectNotToPerformAssertions(); @@ -646,6 +718,71 @@ private function syntaxValidator(): SchemaValidator ); } + /** + * A caseIgnore rule expecting a given number of equality questions from the validator. + */ + private function comparatorExpecting( + InvocationOrder $equalityCalls, + ): MatchingRuleComparatorInterface&IndexableComparatorInterface { + $inner = new CaseIgnoreComparator(); + + /** @var MatchingRuleComparatorInterface&IndexableComparatorInterface&MockObject $comparator */ + $comparator = $this->createMockForIntersectionOfInterfaces([ + MatchingRuleComparatorInterface::class, + IndexableComparatorInterface::class, + ]); + $comparator->expects($equalityCalls) + ->method('equals') + ->willReturnCallback($inner->equals(...)); + $comparator->method('indexKey') + ->willReturnCallback($inner->indexKey(...)); + + return $comparator; + } + + /** + * A schema whose widget attributes carry a rule the test can bound the equality cost of. + */ + private function widgetValidator(InvocationOrder $equalityCalls): SchemaValidator + { + $schema = (new Schema()) + ->addMatchingRule(new MatchingRule( + '1.900', + ['widgetMatch'], + SyntaxOid::OID_DIRECTORY_STRING, + $this->comparatorExpecting($equalityCalls), + )) + ->addAttributeType(new AttributeType( + '1.5', + ['objectClass'], + syntaxOid: SyntaxOid::OID_OID, + )) + ->addAttributeType(new AttributeType( + '1.20', + ['widgetLabel'], + equalityOid: '1.900', + syntaxOid: SyntaxOid::OID_DIRECTORY_STRING, + )) + ->addAttributeType(new AttributeType( + '1.21', + ['widgetNote'], + equalityOid: '1.900', + syntaxOid: SyntaxOid::OID_DIRECTORY_STRING, + )) + ->addObjectClass(new ObjectClass( + '2.20', + ['widget'], + ObjectClassType::StructuralClass, + must: ['objectClass'], + may: ['widgetLabel', 'widgetNote'], + )); + + return new SchemaValidator( + $schema, + SchemaValidationMode::Strict, + ); + } + private function structuralChainValidator(): SchemaValidator { $schema = (new Schema()) diff --git a/tests/unit/Server/Backend/Storage/Adapter/PdoStorageTest.php b/tests/unit/Server/Backend/Storage/Adapter/PdoStorageTest.php index 75a02538..10f333f6 100644 --- a/tests/unit/Server/Backend/Storage/Adapter/PdoStorageTest.php +++ b/tests/unit/Server/Backend/Storage/Adapter/PdoStorageTest.php @@ -240,6 +240,192 @@ public function test_remove_all_spans_more_entries_than_one_batch(): void self::assertFalse($this->storage->exists(new Dn('cn=b1200,dc=example,dc=com'))); } + public function test_an_attribute_wider_than_one_statement_writes_every_index_row(): void + { + $pdo = new RecordingPdo('sqlite::memory:'); + PdoStorage::initialize( + $pdo, + new SqliteDialect(), + ); + $storage = $this->storageOver($pdo); + $values = []; + foreach (range(1, 1000) as $i) { + $values[] = "wide value {$i}"; + } + + $storage->store(new Entry( + new Dn('cn=wide,dc=example,dc=com'), + new Attribute('cn', 'wide'), + new Attribute('description', ...$values), + )); + + self::assertSame( + 200, + $this->widestSidecarInsert($pdo), + ); + self::assertCount( + 1, + iterator_to_array($storage->list(new StorageListOptions( + baseDn: new Dn('dc=example,dc=com'), + subtree: true, + filter: Filters::equal('description', 'wide value 999'), + ))->entries()), + ); + } + + public function test_adding_one_value_writes_one_index_row_rather_than_the_whole_attribute(): void + { + $pdo = new RecordingPdo('sqlite::memory:'); + PdoStorage::initialize( + $pdo, + new SqliteDialect(), + ); + $storage = $this->storageOver($pdo); + $dn = new Dn('cn=growing,dc=example,dc=com'); + $values = []; + foreach (range(1, 300) as $i) { + $values[] = "growing value {$i}"; + } + $storage->store(new Entry( + $dn, + new Attribute('cn', 'growing'), + new Attribute('description', ...$values), + )); + $pdo->prepared = []; + + $values[] = 'growing value 301'; + $storage->store(new Entry( + $dn, + new Attribute('cn', 'growing'), + new Attribute('description', ...$values), + )); + + self::assertSame( + 1, + $this->widestSidecarInsert($pdo), + ); + self::assertCount( + 0, + $pdo->preparedMatching('DELETE FROM entry_attribute_values'), + ); + } + + public function test_removing_one_value_deletes_one_index_row_and_inserts_nothing(): void + { + $pdo = new RecordingPdo('sqlite::memory:'); + PdoStorage::initialize( + $pdo, + new SqliteDialect(), + ); + $storage = $this->storageOver($pdo); + $dn = new Dn('cn=shrinking,dc=example,dc=com'); + $values = []; + foreach (range(1, 300) as $i) { + $values[] = "shrinking value {$i}"; + } + $storage->store(new Entry( + $dn, + new Attribute('cn', 'shrinking'), + new Attribute('description', ...$values), + )); + $pdo->prepared = []; + + array_pop($values); + $storage->store(new Entry( + $dn, + new Attribute('cn', 'shrinking'), + new Attribute('description', ...$values), + )); + + self::assertCount( + 1, + $pdo->preparedMatching('AND value_lower IN'), + ); + self::assertCount( + 0, + $pdo->preparedMatching('INSERT INTO entry_attribute_values'), + ); + } + + public function test_a_modify_of_another_attribute_leaves_a_wide_attributes_index_untouched(): void + { + $pdo = new RecordingPdo('sqlite::memory:'); + PdoStorage::initialize( + $pdo, + new SqliteDialect(), + ); + $storage = $this->storageOver($pdo); + $dn = new Dn('cn=stable,dc=example,dc=com'); + $values = []; + foreach (range(1, 300) as $i) { + $values[] = "stable value {$i}"; + } + $storage->store(new Entry( + $dn, + new Attribute('cn', 'stable'), + new Attribute('title', 'before'), + new Attribute('description', ...$values), + )); + $pdo->prepared = []; + + $storage->store(new Entry( + $dn, + new Attribute('cn', 'stable'), + new Attribute('title', 'after'), + new Attribute('description', ...$values), + )); + + self::assertSame( + 1, + $this->widestSidecarInsert($pdo), + ); + self::assertCount( + 1, + iterator_to_array($storage->list(new StorageListOptions( + baseDn: new Dn('dc=example,dc=com'), + subtree: true, + filter: Filters::equal('description', 'stable value 300'), + ))->entries()), + ); + } + + public function test_a_swapped_value_leaves_the_index_matching_the_survivors_and_the_new_value(): void + { + $dn = new Dn('cn=swapped,dc=example,dc=com'); + $this->storage->store(new Entry( + $dn, + new Attribute('cn', 'swapped'), + new Attribute('description', 'kept value', 'old value'), + )); + + $this->storage->store(new Entry( + $dn, + new Attribute('cn', 'swapped'), + new Attribute('description', 'kept value', 'new value'), + )); + + foreach (['kept value', 'new value'] as $value) { + self::assertCount( + 1, + iterator_to_array($this->storage->list(new StorageListOptions( + baseDn: new Dn('dc=example,dc=com'), + subtree: true, + filter: Filters::equal('description', $value), + ))->entries()), + $value, + ); + } + + self::assertCount( + 0, + iterator_to_array($this->storage->list(new StorageListOptions( + baseDn: new Dn('dc=example,dc=com'), + subtree: true, + filter: Filters::equal('description', 'old value'), + ))->entries()), + ); + } + public function test_a_modified_value_stops_matching_its_old_value_and_starts_matching_the_new(): void { $dn = new Dn('cn=drift,dc=example,dc=com'); @@ -1704,6 +1890,23 @@ private function originalValuesFor( return $found; } + /** + * Tuples in the largest sidecar insert prepared so far, or zero when none was, which bounds the placeholder count. + */ + private function widestSidecarInsert(RecordingPdo $pdo): int + { + $widest = 0; + + foreach ($pdo->preparedMatching('INSERT INTO entry_attribute_values') as $sql) { + $widest = max( + $widest, + substr_count($sql, '(?, ?, ?, ?)'), + ); + } + + return $widest; + } + /** * The container vends the storage interface, while this file asserts on the PDO adapter specifically. */ From f0dba6f8f0dc471a0ee501f4cf7cce2267e913a2 Mon Sep 17 00:00:00 2001 From: Chad Sikorra Date: Wed, 16 Sep 2026 21:11:31 -0400 Subject: [PATCH 2/4] Resolve a value against the ones it could match, not against every value held. --- src/FreeDSx/Ldap/Entry/Attribute.php | 73 +++++++++--- .../Adapter/Operation/RdnAttributeValues.php | 15 +-- .../Adapter/Operation/UpdateOperation.php | 105 ++++++++---------- tests/unit/Entry/AttributeTest.php | 55 +++++++++ 4 files changed, 166 insertions(+), 82 deletions(-) diff --git a/src/FreeDSx/Ldap/Entry/Attribute.php b/src/FreeDSx/Ldap/Entry/Attribute.php index 7e4f8d70..b3ff63a4 100644 --- a/src/FreeDSx/Ldap/Entry/Attribute.php +++ b/src/FreeDSx/Ldap/Entry/Attribute.php @@ -19,8 +19,8 @@ use Stringable; use Traversable; +use function array_count_values; use function array_keys; -use function array_search; use function array_shift; use function array_values; use function count; @@ -156,22 +156,13 @@ public function removeValues( array $values, bool $caseSensitive = true, ): self { - foreach ($values as $value) { - if ($caseSensitive) { - if (($i = array_search($value, $this->values, true)) !== false) { - unset($this->values[$i]); - } - - continue; - } - - foreach ($this->values as $i => $existing) { - if (strcasecmp($existing, $value) === 0) { - unset($this->values[$i]); - } - } + if ($values === []) { + return $this; } - $this->values = array_values($this->values); + + $this->values = $caseSensitive + ? $this->withoutOneOfEach($values) + : $this->withoutAnyCaseOf($values); return $this; } @@ -379,4 +370,54 @@ private function options(): Options return $this->options; } + + /** + * Drops one held value for each value listed, leaving any further occurrences in place. + * + * @param string[] $values + * @return list + */ + private function withoutOneOfEach(array $values): array + { + $remaining = array_count_values($values); + $kept = []; + + foreach ($this->values as $value) { + if (($remaining[$value] ?? 0) > 0) { + $remaining[$value]--; + + continue; + } + + $kept[] = $value; + } + + return $kept; + } + + /** + * Drops every held value matching a listed value, ignoring ASCII case as strcasecmp does. + * + * @param string[] $values + * @return list + */ + private function withoutAnyCaseOf(array $values): array + { + $removed = []; + + foreach ($values as $value) { + $removed[strtolower($value)] = true; + } + $kept = []; + + foreach ($this->values as $value) { + if (isset($removed[strtolower($value)])) { + continue; + } + + $kept[] = $value; + } + + return $kept; + } } diff --git a/src/FreeDSx/Ldap/Server/Backend/Storage/Adapter/Operation/RdnAttributeValues.php b/src/FreeDSx/Ldap/Server/Backend/Storage/Adapter/Operation/RdnAttributeValues.php index c7e256a7..75fe8eb9 100644 --- a/src/FreeDSx/Ldap/Server/Backend/Storage/Adapter/Operation/RdnAttributeValues.php +++ b/src/FreeDSx/Ldap/Server/Backend/Storage/Adapter/Operation/RdnAttributeValues.php @@ -17,8 +17,8 @@ use FreeDSx\Ldap\Entry\Entry; use FreeDSx\Ldap\Entry\Rdn; use FreeDSx\Ldap\Schema\Matching\EqualityComparatorResolver; +use FreeDSx\Ldap\Schema\Matching\EquivalentValues; -use function array_filter; use function array_values; /** @@ -94,14 +94,11 @@ private function valuesEqualTo( Attribute $attribute, string $value, ): array { - $comparator = $this->equalityResolver->for($attribute->getName()); - - return array_values(array_filter( + $held = EquivalentValues::of( + $this->equalityResolver->for($attribute->getName()), $attribute->getValues(), - static fn(string $stored): bool => $comparator->equals( - $stored, - $value, - ), - )); + ); + + return array_values($held->matching($value)); } } diff --git a/src/FreeDSx/Ldap/Server/Backend/Storage/Adapter/Operation/UpdateOperation.php b/src/FreeDSx/Ldap/Server/Backend/Storage/Adapter/Operation/UpdateOperation.php index 99dd31ec..09353c92 100644 --- a/src/FreeDSx/Ldap/Server/Backend/Storage/Adapter/Operation/UpdateOperation.php +++ b/src/FreeDSx/Ldap/Server/Backend/Storage/Adapter/Operation/UpdateOperation.php @@ -20,7 +20,7 @@ use FreeDSx\Ldap\Exception\OperationException; use FreeDSx\Ldap\Operation\ResultCode; use FreeDSx\Ldap\Schema\Matching\EqualityComparatorResolver; -use FreeDSx\Ldap\Schema\Matching\MatchingRuleComparatorInterface; +use FreeDSx\Ldap\Schema\Matching\EquivalentValues; use FreeDSx\Ldap\Server\Backend\Write\Command\UpdateCommand; /** @@ -76,10 +76,13 @@ private function applyAdd( $entry->add($attribute); return; } - $comparator = $this->equalityResolver->for($attribute->getName()); + $held = EquivalentValues::of( + $this->equalityResolver->for($attribute->getName()), + $existing->getValues(), + ); foreach ($attribute->getValues() as $value) { - if ($this->matchesAny($comparator, $existing->getValues(), $value)) { + if ($held->containsEquivalentOf($value)) { throw new OperationException( sprintf('Attribute "%s" already contains the given value.', $attribute->getName()), ResultCode::ATTRIBUTE_OR_VALUE_EXISTS, @@ -156,9 +159,13 @@ private function deleteSpecificValues( $attribute->getName(), ); $comparator = $this->equalityResolver->for($attribute->getName()); + $held = EquivalentValues::of($comparator, $existing->getValues()); + $matched = []; foreach ($values as $value) { - if (!$this->matchesAny($comparator, $existing->getValues(), $value)) { + $matches = $held->matching($value); + + if ($matches === []) { throw new OperationException( sprintf('The given value does not exist in attribute "%s".', $attribute->getName()), ResultCode::NO_SUCH_ATTRIBUTE, @@ -174,13 +181,12 @@ private function deleteSpecificValues( ResultCode::NOT_ALLOWED_ON_RDN, ); } + + // Keyed by position: two requested values naming one stored value remove it once. + $matched += $matches; } - $existing->removeValues($this->valuesMatching( - $comparator, - $existing->getValues(), - $values, - )); + $existing->removeValues(array_values($matched)); // RFC 4511 §4.6: listing every value an attribute currently holds removes the attribute itself. if ($existing->getValues() === []) { @@ -188,46 +194,6 @@ private function deleteSpecificValues( } } - /** - * The stored values a delete names, resolved by the type's equality rule rather than by their spelling. - * - * @param string[] $stored - * @param string[] $requested - * @return list - */ - private function valuesMatching( - MatchingRuleComparatorInterface $comparator, - array $stored, - array $requested, - ): array { - $matched = []; - - foreach ($stored as $value) { - if ($this->matchesAny($comparator, $requested, $value)) { - $matched[] = $value; - } - } - - return $matched; - } - - /** - * @param string[] $values - */ - private function matchesAny( - MatchingRuleComparatorInterface $comparator, - array $values, - string $candidate, - ): bool { - foreach ($values as $value) { - if ($comparator->equals($value, $candidate)) { - return true; - } - } - - return false; - } - /** * @throws OperationException */ @@ -246,21 +212,46 @@ private function applyReplace(Entry $entry, Change $change): void } $rdnValue = $this->getRdnValueForAttribute($entry, $attribute->getName()); - $comparator = $this->equalityResolver->for($attribute->getName()); - if ($rdnValue !== null && !$this->matchesAny($comparator, $values, $rdnValue)) { - throw new OperationException( - sprintf( - 'Replacing attribute "%s" must retain its RDN value.', - $attribute->getName(), - ), - ResultCode::NOT_ALLOWED_ON_RDN, + if ($rdnValue !== null) { + $this->checkRetainsRdnValue( + $attribute, + $values, + $rdnValue, ); } $entry->set($attribute); } + /** + * @param string[] $values + * + * @throws OperationException + */ + private function checkRetainsRdnValue( + Attribute $attribute, + array $values, + string $rdnValue, + ): void { + $replacement = EquivalentValues::of( + $this->equalityResolver->for($attribute->getName()), + $values, + ); + + if ($replacement->containsEquivalentOf($rdnValue)) { + return; + } + + throw new OperationException( + sprintf( + 'Replacing attribute "%s" must retain its RDN value.', + $attribute->getName(), + ), + ResultCode::NOT_ALLOWED_ON_RDN, + ); + } + /** * @throws OperationException */ diff --git a/tests/unit/Entry/AttributeTest.php b/tests/unit/Entry/AttributeTest.php index 8d3f6e10..0b9f2f7c 100644 --- a/tests/unit/Entry/AttributeTest.php +++ b/tests/unit/Entry/AttributeTest.php @@ -160,6 +160,61 @@ public function test_removing_the_first_value_reindexes_the_list(): void ); } + public function test_removing_a_value_removes_one_occurrence_per_listed_value(): void + { + $subject = new Attribute( + 'cn', + 'foo', + 'foo', + 'bar', + ); + + $subject->removeValues(['foo']); + + self::assertSame( + ['foo', 'bar'], + $subject->getValues(), + ); + } + + public function test_removing_values_case_insensitively_removes_every_match(): void + { + $subject = new Attribute( + 'cn', + 'FOO', + 'foo', + 'bar', + ); + + $subject->removeValues( + ['foo'], + false, + ); + + self::assertSame( + ['bar'], + $subject->getValues(), + ); + } + + public function test_removing_values_case_insensitively_leaves_high_byte_values_untouched(): void + { + $subject = new Attribute( + 'cn', + "\xc3\x89cole", + ); + + $subject->removeValues( + ["\xc3\xa9cole"], + false, + ); + + self::assertSame( + ["\xc3\x89cole"], + $subject->getValues(), + ); + } + public function test_it_should_set_values(): void { $this->subject->set('foo'); From fbc2f88c5dbb6ed09b0dfc4e31c248d9baeb2713 Mon Sep 17 00:00:00 2001 From: Chad Sikorra Date: Wed, 16 Sep 2026 22:17:27 -0400 Subject: [PATCH 3/4] Settle an equality filter in SQL when the rule's index key is canonical. --- .../Matching/CanonicalIndexKeyInterface.php | 21 +++++++++ .../Comparator/BitStringComparator.php | 4 +- .../Comparator/CaseExactComparator.php | 4 +- .../Comparator/CaseIgnoreComparator.php | 4 +- .../Comparator/CaseIgnoreIa5Comparator.php | 4 +- .../DistinguishedNameComparator.php | 4 +- .../Comparator/NumericStringComparator.php | 4 +- .../Comparator/PreparedStringComparator.php | 4 +- .../Comparator/TelephoneNumberComparator.php | 4 +- .../SqlFilter/SqlFilterTranslatorTrait.php | 2 +- .../Storage/Schema/AttributeIndexForms.php | 9 ++++ .../Adapter/MysqlFilterTranslatorTest.php | 44 +++++++++++++++++++ .../Adapter/SqliteFilterTranslatorTest.php | 44 +++++++++++++++++++ 13 files changed, 135 insertions(+), 17 deletions(-) create mode 100644 src/FreeDSx/Ldap/Schema/Matching/CanonicalIndexKeyInterface.php diff --git a/src/FreeDSx/Ldap/Schema/Matching/CanonicalIndexKeyInterface.php b/src/FreeDSx/Ldap/Schema/Matching/CanonicalIndexKeyInterface.php new file mode 100644 index 00000000..32839856 --- /dev/null +++ b/src/FreeDSx/Ldap/Schema/Matching/CanonicalIndexKeyInterface.php @@ -0,0 +1,21 @@ + + * + * For the full copyright and license information, please view the LICENSE + * file that was distributed with this source code. + */ + +namespace FreeDSx\Ldap\Schema\Matching; + +/** + * A rule whose index key is canonical: values sharing a key are equal under it. + * + * @author Chad Sikorra + */ +interface CanonicalIndexKeyInterface extends IndexableComparatorInterface {} diff --git a/src/FreeDSx/Ldap/Schema/Matching/Comparator/BitStringComparator.php b/src/FreeDSx/Ldap/Schema/Matching/Comparator/BitStringComparator.php index 6f738f13..ea00dd3f 100644 --- a/src/FreeDSx/Ldap/Schema/Matching/Comparator/BitStringComparator.php +++ b/src/FreeDSx/Ldap/Schema/Matching/Comparator/BitStringComparator.php @@ -13,14 +13,14 @@ namespace FreeDSx\Ldap\Schema\Matching\Comparator; -use FreeDSx\Ldap\Schema\Matching\IndexableComparatorInterface; +use FreeDSx\Ldap\Schema\Matching\CanonicalIndexKeyInterface; use FreeDSx\Ldap\Schema\Matching\MatchingRuleComparatorInterface; use FreeDSx\Ldap\Schema\Matching\SubstringAssertion; /** * Bit string comparator matching the bits within the 'nnnn'B form (RFC 4517 section 4.2.1). */ -final class BitStringComparator implements MatchingRuleComparatorInterface, IndexableComparatorInterface +final class BitStringComparator implements MatchingRuleComparatorInterface, CanonicalIndexKeyInterface { public function equals( string $a, diff --git a/src/FreeDSx/Ldap/Schema/Matching/Comparator/CaseExactComparator.php b/src/FreeDSx/Ldap/Schema/Matching/Comparator/CaseExactComparator.php index ae89b9c1..ac0af5ab 100644 --- a/src/FreeDSx/Ldap/Schema/Matching/Comparator/CaseExactComparator.php +++ b/src/FreeDSx/Ldap/Schema/Matching/Comparator/CaseExactComparator.php @@ -13,7 +13,7 @@ namespace FreeDSx\Ldap\Schema\Matching\Comparator; -use FreeDSx\Ldap\Schema\Matching\IndexableComparatorInterface; +use FreeDSx\Ldap\Schema\Matching\CanonicalIndexKeyInterface; use FreeDSx\Ldap\Schema\Matching\MatchingRuleComparatorInterface; use FreeDSx\Ldap\Schema\Matching\StringPrep; use FreeDSx\Ldap\Schema\Matching\SubstringAssertion; @@ -21,7 +21,7 @@ /** * Case-sensitive comparator (caseExactMatch / caseExactSubstringsMatch / caseExactOrderingMatch) using RFC 4518 prep without case folding. */ -final readonly class CaseExactComparator implements MatchingRuleComparatorInterface, IndexableComparatorInterface +final readonly class CaseExactComparator implements MatchingRuleComparatorInterface, CanonicalIndexKeyInterface { private PreparedStringComparator $inner; diff --git a/src/FreeDSx/Ldap/Schema/Matching/Comparator/CaseIgnoreComparator.php b/src/FreeDSx/Ldap/Schema/Matching/Comparator/CaseIgnoreComparator.php index d431e5b9..00f1a790 100644 --- a/src/FreeDSx/Ldap/Schema/Matching/Comparator/CaseIgnoreComparator.php +++ b/src/FreeDSx/Ldap/Schema/Matching/Comparator/CaseIgnoreComparator.php @@ -13,7 +13,7 @@ namespace FreeDSx\Ldap\Schema\Matching\Comparator; -use FreeDSx\Ldap\Schema\Matching\IndexableComparatorInterface; +use FreeDSx\Ldap\Schema\Matching\CanonicalIndexKeyInterface; use FreeDSx\Ldap\Schema\Matching\MatchingRuleComparatorInterface; use FreeDSx\Ldap\Schema\Matching\StringPrep; use FreeDSx\Ldap\Schema\Matching\SubstringAssertion; @@ -21,7 +21,7 @@ /** * Case-insensitive string comparator (caseIgnoreMatch / caseIgnoreSubstringsMatch / caseIgnoreOrderingMatch). */ -final readonly class CaseIgnoreComparator implements MatchingRuleComparatorInterface, IndexableComparatorInterface +final readonly class CaseIgnoreComparator implements MatchingRuleComparatorInterface, CanonicalIndexKeyInterface { private PreparedStringComparator $inner; diff --git a/src/FreeDSx/Ldap/Schema/Matching/Comparator/CaseIgnoreIa5Comparator.php b/src/FreeDSx/Ldap/Schema/Matching/Comparator/CaseIgnoreIa5Comparator.php index 5f38e2ad..c1ad6f03 100644 --- a/src/FreeDSx/Ldap/Schema/Matching/Comparator/CaseIgnoreIa5Comparator.php +++ b/src/FreeDSx/Ldap/Schema/Matching/Comparator/CaseIgnoreIa5Comparator.php @@ -13,7 +13,7 @@ namespace FreeDSx\Ldap\Schema\Matching\Comparator; -use FreeDSx\Ldap\Schema\Matching\IndexableComparatorInterface; +use FreeDSx\Ldap\Schema\Matching\CanonicalIndexKeyInterface; use FreeDSx\Ldap\Schema\Matching\MatchingRuleComparatorInterface; use FreeDSx\Ldap\Schema\Matching\SubstringAssertion; @@ -21,7 +21,7 @@ * Case-insensitive IA5 (ASCII) string comparator (caseIgnoreIA5Match / caseIgnoreIA5SubstringsMatch). * Behaviorally identical to CaseIgnoreComparator since IA5 is a subset of ASCII. */ -final readonly class CaseIgnoreIa5Comparator implements MatchingRuleComparatorInterface, IndexableComparatorInterface +final readonly class CaseIgnoreIa5Comparator implements MatchingRuleComparatorInterface, CanonicalIndexKeyInterface { private CaseIgnoreComparator $inner; diff --git a/src/FreeDSx/Ldap/Schema/Matching/Comparator/DistinguishedNameComparator.php b/src/FreeDSx/Ldap/Schema/Matching/Comparator/DistinguishedNameComparator.php index ffb1bcac..d2fc655f 100644 --- a/src/FreeDSx/Ldap/Schema/Matching/Comparator/DistinguishedNameComparator.php +++ b/src/FreeDSx/Ldap/Schema/Matching/Comparator/DistinguishedNameComparator.php @@ -14,14 +14,14 @@ namespace FreeDSx\Ldap\Schema\Matching\Comparator; use FreeDSx\Ldap\Entry\Dn; -use FreeDSx\Ldap\Schema\Matching\IndexableComparatorInterface; +use FreeDSx\Ldap\Schema\Matching\CanonicalIndexKeyInterface; use FreeDSx\Ldap\Schema\Matching\MatchingRuleComparatorInterface; use FreeDSx\Ldap\Schema\Matching\SubstringAssertion; /** * DN equality comparator (distinguishedNameMatch): normalizes both sides before comparing. */ -final class DistinguishedNameComparator implements MatchingRuleComparatorInterface, IndexableComparatorInterface +final class DistinguishedNameComparator implements MatchingRuleComparatorInterface, CanonicalIndexKeyInterface { public function equals( string $a, diff --git a/src/FreeDSx/Ldap/Schema/Matching/Comparator/NumericStringComparator.php b/src/FreeDSx/Ldap/Schema/Matching/Comparator/NumericStringComparator.php index b0ca117f..796bc349 100644 --- a/src/FreeDSx/Ldap/Schema/Matching/Comparator/NumericStringComparator.php +++ b/src/FreeDSx/Ldap/Schema/Matching/Comparator/NumericStringComparator.php @@ -13,7 +13,7 @@ namespace FreeDSx\Ldap\Schema\Matching\Comparator; -use FreeDSx\Ldap\Schema\Matching\IndexableComparatorInterface; +use FreeDSx\Ldap\Schema\Matching\CanonicalIndexKeyInterface; use FreeDSx\Ldap\Schema\Matching\MatchingRuleComparatorInterface; use FreeDSx\Ldap\Schema\Matching\StringPrep; use FreeDSx\Ldap\Schema\Matching\SubstringAssertion; @@ -21,7 +21,7 @@ /** * Numeric string comparator treating spaces as insignificant (RFC 4517 section 4.2.22). */ -final class NumericStringComparator implements MatchingRuleComparatorInterface, IndexableComparatorInterface +final class NumericStringComparator implements MatchingRuleComparatorInterface, CanonicalIndexKeyInterface { use NormalizedIndexFormsTrait; diff --git a/src/FreeDSx/Ldap/Schema/Matching/Comparator/PreparedStringComparator.php b/src/FreeDSx/Ldap/Schema/Matching/Comparator/PreparedStringComparator.php index 81def651..d72c39d4 100644 --- a/src/FreeDSx/Ldap/Schema/Matching/Comparator/PreparedStringComparator.php +++ b/src/FreeDSx/Ldap/Schema/Matching/Comparator/PreparedStringComparator.php @@ -13,7 +13,7 @@ namespace FreeDSx\Ldap\Schema\Matching\Comparator; -use FreeDSx\Ldap\Schema\Matching\IndexableComparatorInterface; +use FreeDSx\Ldap\Schema\Matching\CanonicalIndexKeyInterface; use FreeDSx\Ldap\Schema\Matching\MatchingRuleComparatorInterface; use FreeDSx\Ldap\Schema\Matching\StringPrep; use FreeDSx\Ldap\Schema\Matching\SubstringAssertion; @@ -21,7 +21,7 @@ /** * String comparator that applies an RFC 4518 preparation profile, then matches byte-exact. */ -final readonly class PreparedStringComparator implements MatchingRuleComparatorInterface, IndexableComparatorInterface +final readonly class PreparedStringComparator implements MatchingRuleComparatorInterface, CanonicalIndexKeyInterface { private OctetStringComparator $matcher; diff --git a/src/FreeDSx/Ldap/Schema/Matching/Comparator/TelephoneNumberComparator.php b/src/FreeDSx/Ldap/Schema/Matching/Comparator/TelephoneNumberComparator.php index 81ab3f6d..5b677199 100644 --- a/src/FreeDSx/Ldap/Schema/Matching/Comparator/TelephoneNumberComparator.php +++ b/src/FreeDSx/Ldap/Schema/Matching/Comparator/TelephoneNumberComparator.php @@ -13,7 +13,7 @@ namespace FreeDSx\Ldap\Schema\Matching\Comparator; -use FreeDSx\Ldap\Schema\Matching\IndexableComparatorInterface; +use FreeDSx\Ldap\Schema\Matching\CanonicalIndexKeyInterface; use FreeDSx\Ldap\Schema\Matching\MatchingRuleComparatorInterface; use FreeDSx\Ldap\Schema\Matching\StringPrep; use FreeDSx\Ldap\Schema\Matching\SubstringAssertion; @@ -21,7 +21,7 @@ /** * Telephone number comparator (telephoneNumberMatch): strips spaces and hyphens before comparing case-insensitively. */ -final class TelephoneNumberComparator implements MatchingRuleComparatorInterface, IndexableComparatorInterface +final class TelephoneNumberComparator implements MatchingRuleComparatorInterface, CanonicalIndexKeyInterface { use NormalizedIndexFormsTrait; diff --git a/src/FreeDSx/Ldap/Server/Backend/Storage/Adapter/SqlFilter/SqlFilterTranslatorTrait.php b/src/FreeDSx/Ldap/Server/Backend/Storage/Adapter/SqlFilter/SqlFilterTranslatorTrait.php index 24c4da60..5617b504 100644 --- a/src/FreeDSx/Ldap/Server/Backend/Storage/Adapter/SqlFilter/SqlFilterTranslatorTrait.php +++ b/src/FreeDSx/Ldap/Server/Backend/Storage/Adapter/SqlFilter/SqlFilterTranslatorTrait.php @@ -317,7 +317,7 @@ private function translateRuleEquality( $this->buildValueExists($attribute, $condition), [SqlFilterUtility::truncate($key)], isExact: $this->isExactEquality($key) - && $this->matchesCaseFolded($attribute) + && $this->indexForms->hasCanonicalEqualityKey($rawAttribute) && !$this->attributeHasOption($rawAttribute), sidecarCondition: $this->sidecarCondition( $attribute, diff --git a/src/FreeDSx/Ldap/Server/Backend/Storage/Schema/AttributeIndexForms.php b/src/FreeDSx/Ldap/Server/Backend/Storage/Schema/AttributeIndexForms.php index a2da4889..2afa6fdd 100644 --- a/src/FreeDSx/Ldap/Server/Backend/Storage/Schema/AttributeIndexForms.php +++ b/src/FreeDSx/Ldap/Server/Backend/Storage/Schema/AttributeIndexForms.php @@ -15,6 +15,7 @@ use FreeDSx\Ldap\Entry\Attribute; use FreeDSx\Ldap\Schema\Definition\MatchingRuleOid; +use FreeDSx\Ldap\Schema\Matching\CanonicalIndexKeyInterface; use FreeDSx\Ldap\Schema\Matching\EqualityComparatorResolver; use FreeDSx\Ldap\Schema\Matching\IndexableComparatorInterface; use FreeDSx\Ldap\Schema\Matching\MatchingRuleComparatorInterface; @@ -60,6 +61,14 @@ public function key( : null; } + /** + * Whether values sharing a stored key are equal under the rule, letting a store settle equality on its own. + */ + public function hasCanonicalEqualityKey(string $attribute): bool + { + return $this->equality->for($attribute) instanceof CanonicalIndexKeyInterface; + } + /** * One column holds one preparation, so an ordering comparison only reaches it when the same rule owns both. */ diff --git a/tests/unit/Server/Backend/Storage/Adapter/MysqlFilterTranslatorTest.php b/tests/unit/Server/Backend/Storage/Adapter/MysqlFilterTranslatorTest.php index 7a92ab86..ca3fc411 100644 --- a/tests/unit/Server/Backend/Storage/Adapter/MysqlFilterTranslatorTest.php +++ b/tests/unit/Server/Backend/Storage/Adapter/MysqlFilterTranslatorTest.php @@ -61,6 +61,50 @@ protected function setUp(): void ); } + public function test_equality_on_a_dn_valued_attribute_is_exact(): void + { + $result = $this->subject->translate(new EqualityFilter( + 'member', + 'cn=Alice,dc=example,dc=com', + )); + + self::assertNotNull($result); + self::assertTrue($result->isExact); + } + + public function test_equality_on_an_octet_string_attribute_is_inexact(): void + { + $result = $this->subject->translate(new EqualityFilter( + 'userPassword', + 'secret', + )); + + self::assertNotNull($result); + self::assertFalse($result->isExact); + } + + public function test_equality_on_a_generalized_time_attribute_is_inexact(): void + { + $result = $this->subject->translate(new EqualityFilter( + 'createTimestamp', + '20260916000000Z', + )); + + self::assertNotNull($result); + self::assertFalse($result->isExact); + } + + public function test_equality_on_a_name_and_optional_uid_attribute_is_inexact(): void + { + $result = $this->subject->translate(new EqualityFilter( + 'uniqueMember', + 'cn=Alice,dc=example,dc=com', + )); + + self::assertNotNull($result); + self::assertFalse($result->isExact); + } + public function test_present_filter_returns_sidecar_presence_exists(): void { $result = $this->subject->translate(new PresentFilter('cn')); diff --git a/tests/unit/Server/Backend/Storage/Adapter/SqliteFilterTranslatorTest.php b/tests/unit/Server/Backend/Storage/Adapter/SqliteFilterTranslatorTest.php index d1fc109e..4cd446f5 100644 --- a/tests/unit/Server/Backend/Storage/Adapter/SqliteFilterTranslatorTest.php +++ b/tests/unit/Server/Backend/Storage/Adapter/SqliteFilterTranslatorTest.php @@ -260,6 +260,50 @@ public function test_equality_with_long_value_is_inexact(): void self::assertFalse($result->isExact); } + public function test_equality_on_a_dn_valued_attribute_is_exact(): void + { + $result = $this->subject->translate(new EqualityFilter( + 'member', + 'cn=Alice,dc=example,dc=com', + )); + + self::assertNotNull($result); + self::assertTrue($result->isExact); + } + + public function test_equality_on_an_octet_string_attribute_is_inexact(): void + { + $result = $this->subject->translate(new EqualityFilter( + 'userPassword', + 'secret', + )); + + self::assertNotNull($result); + self::assertFalse($result->isExact); + } + + public function test_equality_on_a_generalized_time_attribute_is_inexact(): void + { + $result = $this->subject->translate(new EqualityFilter( + 'createTimestamp', + '20260916000000Z', + )); + + self::assertNotNull($result); + self::assertFalse($result->isExact); + } + + public function test_equality_on_a_name_and_optional_uid_attribute_is_inexact(): void + { + $result = $this->subject->translate(new EqualityFilter( + 'uniqueMember', + 'cn=Alice,dc=example,dc=com', + )); + + self::assertNotNull($result); + self::assertFalse($result->isExact); + } + public function test_equality_value_is_lowercased_and_truncated(): void { $result = $this->subject->translate(new EqualityFilter( From 071e1fa2e98592d3d889f094994ba196e43e0e6f Mon Sep 17 00:00:00 2001 From: Chad Sikorra Date: Thu, 17 Sep 2026 12:28:33 -0400 Subject: [PATCH 4/4] Write an entry's changed index rows in one delete and one insert. --- .../Adapter/Dialect/PdoDialectTrait.php | 8 +- .../Dialect/PdoEntryDialectInterface.php | 2 +- .../Storage/Adapter/Pdo/EntryIndexWriter.php | 91 ++++++++----------- .../Storage/Adapter/PdoStorageTest.php | 2 +- 4 files changed, 44 insertions(+), 59 deletions(-) diff --git a/src/FreeDSx/Ldap/Server/Backend/Storage/Adapter/Dialect/PdoDialectTrait.php b/src/FreeDSx/Ldap/Server/Backend/Storage/Adapter/Dialect/PdoDialectTrait.php index be991171..7bbe81c2 100644 --- a/src/FreeDSx/Ldap/Server/Backend/Storage/Adapter/Dialect/PdoDialectTrait.php +++ b/src/FreeDSx/Ldap/Server/Backend/Storage/Adapter/Dialect/PdoDialectTrait.php @@ -252,13 +252,15 @@ public function querySidecarDelete(): string public function querySidecarDeleteValues(int $count): string { - $markers = SqlFilterUtility::markers($count); + $markers = SqlFilterUtility::markers( + $count, + '(?, ?)', + ); return <<applyValueDelta( - $entryId, - $name, - $next[$name] ?? [], - $previous[$name] ?? [], - ); + // Diffed on the stored row, not the raw value, so a difference the index key folds away cannot strand a row. + $wanted = $this->rowsByKey($name, $next[$name] ?? []); + $held = $this->rowsByKey($name, $previous[$name] ?? []); + + foreach (array_keys(array_diff_key($held, $wanted)) as $valueLower) { + $removed[] = [$name, $valueLower]; + } + + foreach (array_diff_key($wanted, $held) as [$valueLower, $valueOriginal]) { + $added[] = [ + $entryId, + $name, + $valueLower, + $valueOriginal, + ]; + } } + $this->deleteValues($entryId, $removed); + $this->insert($added); + // A substring index keys off its own attribute set, so it only needs redoing when one of those changed. if (!$this->indexCovers($changed)) { return; @@ -88,40 +105,27 @@ public function update( } /** - * Writes one attribute's difference, so a single added value costs one row rather than a full rewrite. + * Removes the named rows in one statement per chunk, in the order every writer builds them. * - * @param list $next - * @param list $previous + * @param list $values [attr_name_lower, value_lower] pairs */ - private function applyValueDelta( + private function deleteValues( int $entryId, - string $attrNameLower, - array $next, - array $previous, + array $values, ): void { - // Diffed on the stored row, not the raw value, so a difference the index key folds away cannot strand a row. - $wanted = $this->rowsByKey($attrNameLower, $next); - $held = $this->rowsByKey($attrNameLower, $previous); + foreach (array_chunk($values, self::SIDECAR_ROWS_PER_STATEMENT) as $chunk) { + $params = [$entryId]; - $removed = array_keys(array_diff_key($held, $wanted)); - $added = array_values(array_diff_key($wanted, $held)); + foreach ($chunk as [$attrNameLower, $valueLower]) { + $params[] = $attrNameLower; + $params[] = $valueLower; + } - foreach (array_chunk($removed, self::SIDECAR_ROWS_PER_STATEMENT) as $chunk) { $this->statements->execute( $this->dialect->querySidecarDeleteValues(count($chunk)), - [ - $entryId, - $attrNameLower, - ...$chunk, - ], + $params, ); } - - $this->insertRowsFor( - $entryId, - $attrNameLower, - $added, - ); } /** @@ -173,6 +177,9 @@ private function changedNames( } } + // Sorted, so concurrent writers take the sidecar's index locks in one order. + sort($changed); + return $changed; } @@ -250,30 +257,6 @@ private function insertRows( $this->insert($this->buildRows($entryId, $entry, $only)); } - /** - * The rows for one attribute's added values, already reduced to their stored form by the delta. - * - * @param list $values [value_lower, value_original] pairs - */ - private function insertRowsFor( - int $entryId, - string $attrNameLower, - array $values, - ): void { - $rows = []; - - foreach ($values as [$valueLower, $valueOriginal]) { - $rows[] = [ - $entryId, - $attrNameLower, - $valueLower, - $valueOriginal, - ]; - } - - $this->insert($rows); - } - /** * Chunked so the placeholder count stays inside the bound SQLite builds before 3.32 compile in. * diff --git a/tests/unit/Server/Backend/Storage/Adapter/PdoStorageTest.php b/tests/unit/Server/Backend/Storage/Adapter/PdoStorageTest.php index 10f333f6..d49c2cc8 100644 --- a/tests/unit/Server/Backend/Storage/Adapter/PdoStorageTest.php +++ b/tests/unit/Server/Backend/Storage/Adapter/PdoStorageTest.php @@ -339,7 +339,7 @@ public function test_removing_one_value_deletes_one_index_row_and_inserts_nothin self::assertCount( 1, - $pdo->preparedMatching('AND value_lower IN'), + $pdo->preparedMatching('(attr_name_lower, value_lower) IN'), ); self::assertCount( 0,