From a7747eadd5cf80b2d824a60de391cf2b4fe694a1 Mon Sep 17 00:00:00 2001 From: Johannes Meyer Date: Thu, 20 Aug 2026 14:04:28 +0200 Subject: [PATCH 1/5] BelongsToMany: Accept `NULL` as target foreign/candidate key This is required to establish type symmetry as the base's methods also accept `NULL` to be able to direcly pass a getters return value to the appropriate setter. --- src/Relation/BelongsToMany.php | 8 ++++---- tests/BelongsToManyTest.php | 10 ++++++++++ 2 files changed, 14 insertions(+), 4 deletions(-) diff --git a/src/Relation/BelongsToMany.php b/src/Relation/BelongsToMany.php index bf570f63..504cfbaa 100644 --- a/src/Relation/BelongsToMany.php +++ b/src/Relation/BelongsToMany.php @@ -142,11 +142,11 @@ public function getTargetForeignKey(): string|array|null /** * Set the column name(s) of the target model's foreign key found in the join table * - * @param string|array $targetForeignKey Array if the foreign key is compound, string otherwise + * @param string|array|null $targetForeignKey Array if the foreign key is compound, string otherwise * * @return $this */ - public function setTargetForeignKey(string|array $targetForeignKey): static + public function setTargetForeignKey(string|array|null $targetForeignKey): static { $this->targetForeignKey = $targetForeignKey; @@ -166,11 +166,11 @@ public function getTargetCandidateKey(): string|array|null /** * Set the candidate key column name(s) in the target table which references the target foreign key * - * @param string|array $targetCandidateKey Array if the foreign key is compound, string otherwise + * @param string|array|null $targetCandidateKey Array if the foreign key is compound, string otherwise * * @return $this */ - public function setTargetCandidateKey(string|array $targetCandidateKey): static + public function setTargetCandidateKey(string|array|null $targetCandidateKey): static { $this->targetCandidateKey = $targetCandidateKey; diff --git a/tests/BelongsToManyTest.php b/tests/BelongsToManyTest.php index cf37846a..8e4b6056 100644 --- a/tests/BelongsToManyTest.php +++ b/tests/BelongsToManyTest.php @@ -174,4 +174,14 @@ public function testResolveYieldsJunctionAndTargetRelationsWithTheirFiltersAndJo 'The target join does not carry the relation filter' ); } + + public function testSetTargetForeignKeyAcceptsNull() + { + $this->assertNull((new BelongsToMany())->setTargetForeignKey(null)->getTargetForeignKey()); + } + + public function testSetTargetCandidateKeyAcceptsNull() + { + $this->assertNull((new BelongsToMany())->setTargetCandidateKey(null)->getTargetCandidateKey()); + } } From 3eb984d58f6fd3df0afb7770b8831281e7dcfdd9 Mon Sep 17 00:00:00 2001 From: Johannes Meyer Date: Thu, 20 Aug 2026 14:17:09 +0200 Subject: [PATCH 2/5] Resolver: Leave it up to a relation to resolve it There is now `Relation::bindTo(Model, string, Resolver)` in order to pass control to relations how they're prepared. Since the introduction of `BelongsToMany`, it is established that a relation may resolve to multiple hops and thus needs to perform steps n-times rather than a single time. It's this reason because registering the alias and resolving the filter is now a responsibility of a relation rather than the resolver. Relations know it better how to and the override of `bindTo` in `BelongsToMany` proves it as it turned out that it is necessary to allow referencing the junction table in either the filter or the through filter in order to be able to better reverse relations. Qualification must be done by a relation in turn as well, as otherwise there's a mis-match with what's allowed to reference and what can be qualified. My initial attempt was to teach `Relation::resolve()` this, but without passing it the resolver and changing the return value this doesn't make sense. Sadly, this is out of the question as this is a breaking change. Say hello to `Relation::setFilterSubjects()` due to this. --- src/Query.php | 17 ++++-- src/Relation.php | 62 +++++++++++++++++++- src/Relation/BelongsToMany.php | 61 ++++++++++++++++++++ src/Resolver.php | 100 +++++++++------------------------ tests/BelongsToManyTest.php | 46 ++++++++------- tests/Lib/Model/Department.php | 2 +- tests/ResolverTest.php | 45 +++++++++++---- 7 files changed, 224 insertions(+), 109 deletions(-) diff --git a/src/Query.php b/src/Query.php index e1b47f31..745e1795 100644 --- a/src/Query.php +++ b/src/Query.php @@ -333,7 +333,7 @@ public function getSelectBase(): Select $visibilityFilter = FilterProcessor::assembleFilter( $this->getResolver()->qualifyFilter( $this->getResolver()->getVisibilityFilter($this->getModel()), - $this->getModel() + ...[$this->getModel()->getTableAlias() => $this->getModel()] ) ); if ($visibilityFilter) { @@ -516,15 +516,21 @@ public function assembleSelect(): Select foreach ($relation->resolve() as $targetRelation => [$source, $target, $relatedKeys]) { if (is_int($targetRelation)) { $targetRelation = $relation; + $relationFilter = Filter::any(); trigger_error(sprintf( 'Relation implementation of %s::resolve() returned a numeric key for the target' . ' relation. This is deprecated and will be removed in a future version. Please return' . ' the target relation as key instead.', $relation::class ), E_USER_DEPRECATED); + } else { + /** @var Relation $targetRelation */ + $relationFilter = $resolver->qualifyFilter( + $targetRelation->getFilter(), + ...$targetRelation->getFilterSubjects() + ); } - /** @var Relation $targetRelation */ /** @var Model $source */ /** @var Model $target */ @@ -541,8 +547,11 @@ public function assembleSelect(): Select } $visibilityConditions = FilterProcessor::assembleFilter(Filter::all( - $resolver->qualifyFilter($targetRelation->getFilter(), $targetRelation), - $resolver->qualifyFilter($resolver->getVisibilityFilter($target), $target) + $relationFilter, + $resolver->qualifyFilter( + $resolver->getVisibilityFilter($target), + ...[$target->getTableAlias() => $target] + ) )); if ($visibilityConditions) { $conditions[] = $visibilityConditions; diff --git a/src/Relation.php b/src/Relation.php index d9a9d9f8..9b775196 100644 --- a/src/Relation.php +++ b/src/Relation.php @@ -5,6 +5,7 @@ use Generator; use ipl\Stdlib\Filter; use ipl\Stdlib\Filter\Rule; +use LogicException; use UnexpectedValueException; /** @@ -43,6 +44,8 @@ class Relation /** @var ?Filter\Chain Additional JOIN conditions */ protected ?Filter\Chain $filter = null; + /** @var ?array Models additional JOIN conditions may reference, keyed by their alias */ + protected ?array $filterSubjects = null; /** * Get the default column name(s) in the source table used to match the foreign key * @@ -298,6 +301,34 @@ public function setFilter(Filter\Rule $filter): static return $this; } + /** + * Get subjects the relation filter may reference + * + * @return array + */ + public function getFilterSubjects(): array + { + return $this->filterSubjects ?? throw new LogicException(sprintf( + 'Cannot get filter subjects of an unbound relation. Please call %s::bindTo() first.', + static::class + )); + } + + /** + * Add subjects the relation filter may reference, while keeping existing ones + * + * @param array ...$subjects + * + * @return $this + */ + public function addFilterSubjects(Model ...$subjects): static + { + $this->filterSubjects ??= []; + $this->filterSubjects += $subjects; + + return $this; + } + /** * Determine the candidate key-foreign key construct of the relation * @@ -348,13 +379,42 @@ public function determineKeys(Model $source): array return array_combine($foreignKey, $candidateKey); } + /** + * Bind the relation to the given source using the passed resolver + * + * @param Model $source The model to use as source + * @param string $path The path the relation has been resolved at + * @param Resolver $resolver The resolver to register the relation's target alias + * + * @return $this + */ + public function bindTo(Model $source, string $path, Resolver $resolver): static + { + $this->setSource($source); + $target = $this->getTarget(); + + $subjects = [ + $this->getName() => $target, + $target->getTableAlias() => $target, + $source->getTableAlias() => $source + ]; + + $this->addFilterSubjects(...$subjects); + + $resolver->resolveRelationFilter($this->getFilter(), $this->getName(), ...$subjects); + $resolver->setAlias($target, str_replace('.', '_', $path)); + + return $this; + } + /** * Resolve the relation * * Yields the relation to join as key and a three-element array consisting of the source model, * target model and the join keys as value. * - * @return Generator}, void> + * @return Generator}, void> + * @phpstan-return Generator}, mixed, void> */ public function resolve(): Generator { diff --git a/src/Relation/BelongsToMany.php b/src/Relation/BelongsToMany.php index 504cfbaa..2bda2e96 100644 --- a/src/Relation/BelongsToMany.php +++ b/src/Relation/BelongsToMany.php @@ -6,6 +6,7 @@ use ipl\Orm\Model; use ipl\Orm\Relation; use ipl\Orm\Relations; +use ipl\Orm\Resolver; use ipl\Stdlib\Filter; use ipl\Stdlib\Filter\Rule; use LogicException; @@ -38,6 +39,9 @@ class BelongsToMany extends Relation /** @var ?Filter\Chain Additional JOIN conditions for the join table */ protected ?Filter\Chain $throughFilter = null; + /** @var ?array Models additional join table conditions may reference, keyed by their alias */ + protected ?array $throughFilterSubjects = null; + /** * Get the name of the join table or junction model class * @@ -211,6 +215,61 @@ public function setThroughFilter(Filter\Rule $filter): static return $this; } + /** + * Get subjects the join table filter may reference + * + * @return array + */ + public function getThroughFilterSubjects(): array + { + return $this->throughFilterSubjects ?? throw new LogicException(sprintf( + 'Cannot get filter subjects of an unbound relation. Please call %s::bindTo() first.', + static::class + )); + } + + /** + * Add subjects the join table filter may reference, while keeping existing ones + * + * @param array ...$subjects + * + * @return $this + */ + public function addThroughFilterSubjects(Model ...$subjects): static + { + $this->throughFilterSubjects ??= []; + $this->throughFilterSubjects += $subjects; + + return $this; + } + + public function bindTo(Model $source, string $path, Resolver $resolver): static + { + // Allow to reference the join table in the second hop + $this->addFilterSubjects(...[$this->getThroughAlias() => $this->getThrough()]); + + parent::bindTo($source, $path, $resolver); + + $this->addThroughFilterSubjects(...[ + $this->getSource()->getTableAlias() => $this->getSource(), + $this->getThrough()->getTableAlias() => $this->getThrough(), + $this->getThroughAlias() => $this->getThrough() + ]); + + $resolver->resolveRelationFilter( + $this->getThroughFilter(), + $this->getThroughAlias(), + ...$this->getThroughFilterSubjects() + ); + + $resolver->setAlias($this->getThrough(), join('_', array_merge( + array_slice(explode('.', $path), 0, -1), + [$this->getThroughAlias()] + ))); + + return $this; + } + public function resolve(): Generator { $source = $this->getSource(); @@ -250,6 +309,7 @@ public function resolve(): Generator ->setSource($source) ->setTarget($junction) ->setFilter($this->getThroughFilter()) + ->addFilterSubjects(...$this->getThroughFilterSubjects()) ->setCandidateKey($this->extractKey($possibleCandidateKey)) ->setForeignKey($this->extractKey($possibleForeignKey)) ->setJoinType($this->getJoinType()); @@ -262,6 +322,7 @@ public function resolve(): Generator ->setSource($junction) ->setTarget($target) ->setFilter($this->getFilter()) + ->addFilterSubjects(...$this->getFilterSubjects()) ->setCandidateKey($this->extractKey($possibleTargetCandidateKey)) ->setForeignKey($this->extractKey($possibleTargetForeignKey)) ->setJoinType($this->getJoinType()); diff --git a/src/Resolver.php b/src/Resolver.php index 68cf0902..fb2fb862 100644 --- a/src/Resolver.php +++ b/src/Resolver.php @@ -500,37 +500,32 @@ public function qualifyPath(string $path, string $tableName): string /** * Resolve the given relation filter * - * Resolves each condition's column according to the referenced subject or, by default, the target. - * The target may also be referenced by the relation's name. + * Resolves each condition's column according to the referenced models or to the given default. * * @param Filter\Chain $filter - * @param string $name The name of the relation - * @param Model $source - * @param Model $target + * @param string $default Must be a valid subject + * @param array $subjects Models keyed by their name * * @throws InvalidArgumentException If a non-condition rule or invalid column is used in the filter */ - public function resolveRelationFilter(Filter\Chain $filter, string $name, Model $source, Model $target): void + public function resolveRelationFilter(Filter\Chain $filter, string $default, Model ...$subjects): void { - $resolveColumn = function (string $column) use ($name, $source, $target): string { + $resolveColumn = function (string $column) use ($default, $subjects): string { // A column may reference the source or target table by its alias, defaulting to the target if (str_contains($column, '.')) { [$alias, $column] = explode('.', $column, 2); } else { - $alias = $target->getTableAlias(); + $alias = $default; } - $subject = match ($alias) { - $name => $target, - $source->getTableAlias() => $source, - $target->getTableAlias() => $target, - default => throw new InvalidArgumentException(sprintf( - 'Invalid relation alias "%s" for models "%s" and "%s"', - $alias, - get_class($source), - get_class($target) + $subject = $subjects[$alias] ?? throw new InvalidArgumentException(sprintf( + 'Invalid relation alias "%s". Available options are: %s', + $alias, + join(', ', array_map( + fn($k) => sprintf('%s => %s', $k, get_class($subjects[$k])), + array_keys($subjects) )) - }; + )); if (! $subject instanceof Junction && ! $this->hasSelectableColumn($subject, $column)) { throw new InvalidArgumentException(sprintf( @@ -546,8 +541,7 @@ public function resolveRelationFilter(Filter\Chain $filter, string $name, Model foreach ($filter->yieldRules() as $rule) { if (! $rule instanceof Filter\Condition) { throw new InvalidArgumentException(sprintf( - 'Relation filter for model "%s" contains a non-condition rule of type "%s"', - get_class($target), + 'Relation filter contains a non-condition rule of type "%s"', get_class($rule) )); } @@ -566,42 +560,24 @@ public function resolveRelationFilter(Filter\Chain $filter, string $name, Model * Qualify the columns of the given filter * * @param Filter\Chain $filter - * @param Model|Relation $subject + * @param array $subjects Models keyed by their name * * @return Filter\Chain * * @throws InvalidArgumentException If a non-condition rule is used or an unknown model is referenced */ - public function qualifyFilter(Filter\Chain $filter, Model|Relation $subject): Filter\Chain + public function qualifyFilter(Filter\Chain $filter, Model ...$subjects): Filter\Chain { - $qualifyColumn = function (string $column) use ($subject): string { + $qualifyColumn = function (string $column) use ($subjects): string { [$alias, $column] = explode('.', $column, 2); - if ($subject instanceof Model) { - if ($subject->getTableAlias() !== $alias) { - throw new InvalidArgumentException(sprintf( - 'Unknown model alias "%s" for filter column "%s"', - $alias, - $column - )); - } - - return $this->qualifyColumn($column, $this->getAlias($subject)); - } + $subject = $subjects[$alias] ?? throw new InvalidArgumentException(sprintf( + 'Unknown model alias "%s" for filter column "%s"', + $alias, + $column + )); - return $this->qualifyColumn( - $column, - match ($alias) { - $subject->getSource()->getTableAlias() => $this->getAlias($subject->getSource()), - $subject->getTarget()->getTableAlias() => $this->getAlias($subject->getTarget()), - $subject->getName() => $this->getAlias($subject->getTarget()), - default => throw new InvalidArgumentException(sprintf( - 'Unknown model alias "%s" for filter column "%s"', - $alias, - $column - )) - } - ); + return $this->qualifyColumn($column, $this->getAlias($subject)); }; $filter = clone $filter; // Deep clone @@ -670,7 +646,9 @@ public function resolveRelation(string $path, ?Model $subject = null): Relation * @param string $path * @param ?Model $subject * - * @return Generator + * @return Generator + * @phpstan-return Generator + * * @throws InvalidArgumentException In case $path is not fully qualified * @throws InvalidRelationException In case a relation is unknown */ @@ -715,32 +693,10 @@ public function resolveRelations(string $path, ?Model $subject = null): Generato throw new InvalidRelationException($relationName, $target); } - $relation = $targetRelations->get($relationName); - $relation->setSource($target); - $this->resolveRelationFilter( - $relation->getFilter(), - $relationName, - $target, - $relation->getTarget() - ); + $relation = $targetRelations->get($relationName) + ->bindTo($target, $relationPath, $this); $resolvedRelations[$relationPath] = $relation; - - if ($relation instanceof BelongsToMany) { - $this->resolveRelationFilter( - $relation->getThroughFilter(), - $relationName, - $target, - $relation->getThrough() - ); - - $this->setAlias($relation->getThrough(), join('_', array_merge( - array_slice($segments, 0, -1), - [$relation->getThroughAlias()] - ))); - } - - $this->setAlias($relation->getTarget(), join('_', $segments)); } yield $relationPath => $relation; diff --git a/tests/BelongsToManyTest.php b/tests/BelongsToManyTest.php index 8e4b6056..f1fbf468 100644 --- a/tests/BelongsToManyTest.php +++ b/tests/BelongsToManyTest.php @@ -5,6 +5,7 @@ use ipl\Orm\Query; use ipl\Orm\Relation\BelongsToMany; use ipl\Orm\Relations; +use ipl\Orm\Resolver; use ipl\Sql\Test\SqlAssertions; use ipl\Stdlib\Filter; @@ -40,7 +41,7 @@ public function testResolveDefaultKeys() foreach ( $relations ->get('user') - ->setSource($model) + ->bindTo($model, 'car.user', $this->createStub(Resolver::class)) ->resolve() as [$from, $to, $keys] ) { reset($keys); @@ -77,7 +78,7 @@ public function testResolveRespectsCustomKeysInTroughModels() foreach ( $relations ->get('user_custom_keys') - ->setSource($model) + ->bindTo($model, 'car.user_custom_keys', $this->createStub(Resolver::class)) ->resolve() as [$from, $to, $keys] ) { reset($keys); @@ -133,22 +134,17 @@ public function testSetThroughFilterWrapsABareConditionInAnAllChain() public function testResolveYieldsJunctionAndTargetRelationsWithTheirFiltersAndJoinType() { - $model = new Car(); - $relations = new Relations(); - $model->createRelations($relations); + $query = (new Query())->setModel(new Car()); + $resolver = $query->getResolver(); - $throughFilter = Filter::equal('user_id', 5); - $targetFilter = Filter::equal('username', 'root'); - - $relation = $relations - ->get('user') - ->setSource($model) + $resolver->getRelations($query->getModel())->get('user') ->setJoinType('LEFT') - ->setThroughFilter($throughFilter) - ->setFilter($targetFilter); + ->setThroughFilter(Filter::equal('user_id', 5)) + ->setFilter(Filter::equal('username', 'root')) + ->bindTo($query->getModel(), 'car.user', $resolver); $resolved = []; - foreach ($relation->resolve() as $key => $_) { + foreach ($resolver->resolveRelation('car.user')->resolve() as $key => $_) { $resolved[] = $key; } @@ -160,18 +156,28 @@ public function testResolveYieldsJunctionAndTargetRelationsWithTheirFiltersAndJo $this->assertSame('LEFT', $toJunction->getJoinType()); $this->assertSame('LEFT', $toTarget->getJoinType()); + // The junction sits between source and target + $this->assertSame('car', $toJunction->getSource()->getTableName()); + $this->assertSame('car_user', $toJunction->getTarget()->getTableName()); + $this->assertSame('car_user', $toTarget->getSource()->getTableName()); + $this->assertSame('user', $toTarget->getTarget()->getTableName()); + // The junction join carries the through filter ... + $throughFilter = iterator_to_array($toJunction->getFilter()->yieldRules()); + $this->assertNotEmpty($throughFilter, 'The junction join does not carry the through filter'); $this->assertSame( - [$throughFilter], - iterator_to_array($toJunction->getFilter()), - 'The junction join does not carry the through filter' + 'car_user.user_id', + $throughFilter[0]->getColumn(), + 'The through filter column is incorrectly resolved' ); // ... and the target join carries the relation filter + $relationFilter = iterator_to_array($toTarget->getFilter()->yieldRules()); + $this->assertNotEmpty($relationFilter, 'The target join does not carry the relation filter'); $this->assertSame( - [$targetFilter], - iterator_to_array($toTarget->getFilter()), - 'The target join does not carry the relation filter' + 'user.username', + $relationFilter[0]->getColumn(), + 'The relation filter column is incorrectly resolved' ); } diff --git a/tests/Lib/Model/Department.php b/tests/Lib/Model/Department.php index 1c4cdfd7..ce72ef13 100644 --- a/tests/Lib/Model/Department.php +++ b/tests/Lib/Model/Department.php @@ -33,7 +33,7 @@ public function createRelations(Relations $relations) // Relation filter referencing the target (default) and the source table alias $relations->hasMany('lead', Employee::class) ->setFilter(Filter::all( - Filter::equal('role', 'lead'), + Filter::equal('employee.role', 'lead'), Filter::equal('department.name', 'Engineering') )); } diff --git a/tests/ResolverTest.php b/tests/ResolverTest.php index 633cdc6a..a47cd8f7 100644 --- a/tests/ResolverTest.php +++ b/tests/ResolverTest.php @@ -226,10 +226,14 @@ public function testResolveRelationFilterQualifiesTargetColumnsByDefault() $resolver = (new Query())->setModel(new Department())->getResolver(); $filter = Filter::all(Filter::equal('active', 'y'), Filter::equal('employee.role', 'lead')); - $resolver->resolveRelationFilter($filter, 'relation', new Department(), new Employee()); + $resolver->resolveRelationFilter($filter, 'relation', ...[ + 'relation' => new Employee(), + 'employee' => new Employee(), + 'department' => new Department() + ]); $columns = array_map(fn ($rule) => $rule->getColumn(), iterator_to_array($filter->yieldRules())); - $this->assertSame(['employee.active', 'employee.role'], $columns); + $this->assertSame(['relation.active', 'employee.role'], $columns); } public function testResolveRelationFilterQualifiesSourceColumns() @@ -237,7 +241,11 @@ public function testResolveRelationFilterQualifiesSourceColumns() $resolver = (new Query())->setModel(new Department())->getResolver(); $filter = Filter::all(Filter::equal('department.name', 'Engineering')); - $resolver->resolveRelationFilter($filter, 'relation', new Department(), new Employee()); + $resolver->resolveRelationFilter($filter, 'relation', ...[ + 'relation' => new Employee(), + 'employee' => new Employee(), + 'department' => new Department() + ]); $this->assertSame('department.name', iterator_to_array($filter->yieldRules())[0]->getColumn()); } @@ -247,7 +255,10 @@ public function testResolveRelationFilterQualifiesRelationColumns() $resolver = (new Query())->setModel(new Department())->getResolver(); $filter = Filter::all(Filter::equal('supplementary.name', 'Q/A')); - $resolver->resolveRelationFilter($filter, 'supplementary', new Department(), new Department()); + $resolver->resolveRelationFilter($filter, 'supplementary', ...[ + 'supplementary' => new Department(), + 'department' => new Department() + ]); $this->assertSame('supplementary.name', iterator_to_array($filter->yieldRules())[0]->getColumn()); } @@ -262,8 +273,11 @@ public function testResolveRelationFilterThrowsForAnUnknownAlias() $resolver->resolveRelationFilter( Filter::all(Filter::equal('office.city', 'London')), 'relation', - new Department(), - new Employee() + ...[ + 'relation' => new Employee(), + 'employee' => new Employee(), + 'department' => new Department() + ] ); } @@ -277,8 +291,11 @@ public function testResolveRelationFilterThrowsForANonSelectableColumn() $resolver->resolveRelationFilter( Filter::all(Filter::equal('unknown', 'x')), 'relation', - new Department(), - new Employee() + ...[ + 'relation' => new Employee(), + 'employee' => new Employee(), + 'department' => new Department() + ] ); } @@ -288,7 +305,11 @@ public function testResolveRelationFilterDoesNotValidateJunctionColumns() $junction = (new Junction())->setTableName('membership'); $filter = Filter::all(Filter::equal('membership.since', '2020')); - $resolver->resolveRelationFilter($filter, 'relation', new Department(), $junction); + $resolver->resolveRelationFilter($filter, 'relation', ...[ + 'relation' => $junction, + 'membership' => $junction, + 'department' => new Department() + ]); $this->assertSame('membership.since', iterator_to_array($filter->yieldRules())[0]->getColumn()); } @@ -302,7 +323,7 @@ public function testQualifyFilterThrowsForAnUnknownModelAlias() $query->getResolver()->qualifyFilter( Filter::all(Filter::equal('employee.active', 'y')), - $query->getModel() + ...[$query->getModel()->getTableAlias() => $query->getModel()] ); } @@ -311,7 +332,9 @@ public function testQualifyFilterDoesNotModifyTheGivenFilter() $query = (new Query())->setModel(new Department()); $original = Filter::all(Filter::equal('department.name', 'Engineering')); - $qualified = $query->getResolver()->qualifyFilter($original, $query->getModel()); + $qualified = $query->getResolver()->qualifyFilter($original, ...[ + $query->getModel()->getTableAlias() => $query->getModel() + ]); // The chain is deep cloned, hence the original is left untouched $this->assertNotSame($original, $qualified, 'The given filter has not been cloned'); From 7ed01ec4e5d4f5d0f4fea8dc272aa54e0f9fdb1b Mon Sep 17 00:00:00 2001 From: Johannes Meyer Date: Thu, 20 Aug 2026 14:21:47 +0200 Subject: [PATCH 3/5] Relation: Add method `reverse(Resolver): Generator` Changes the way relations can be reversed drastically as it is now possible to influence the relation to use during reversal with `::setReverseName(string)` which allows Icinga DB Web to drop the error-prone `to.from` and `from.to` relations. An additional change is that it is now not mandatory anymore to define relations that are solely being required because of sub-queries. Missing relations on the reversed path are automatically registered. For this, each relation type now has its specific counterpart which is possible to override with `::setReverseClass(class-string)`. The default however, is to use the same type which is the case for `BelongsToOne` and `BelongsToMany`. For `BelongsTo` a sane override has been chosen that is based on how it's used at the moment in our products, as `HasOne` and `HasMany` may both be appropriate. But the latter clearly is used more often. --- src/Relation.php | 149 +++++++++++++++++++++++++++++++++ src/Relation/BelongsTo.php | 2 + src/Relation/BelongsToMany.php | 39 +++++++++ src/Relation/HasMany.php | 2 + src/Relation/HasOne.php | 1 + tests/BelongsToManyTest.php | 87 +++++++++++++++++++ tests/Lib/Model/Author.php | 33 ++++++++ tests/Lib/Model/Book.php | 40 +++++++++ tests/Lib/Model/Chair.php | 2 +- tests/RelationTest.php | 126 ++++++++++++++++++++++++++++ 10 files changed, 480 insertions(+), 1 deletion(-) create mode 100644 tests/Lib/Model/Author.php create mode 100644 tests/Lib/Model/Book.php diff --git a/src/Relation.php b/src/Relation.php index 9b775196..179cd41c 100644 --- a/src/Relation.php +++ b/src/Relation.php @@ -6,6 +6,7 @@ use ipl\Stdlib\Filter; use ipl\Stdlib\Filter\Rule; use LogicException; +use RuntimeException; use UnexpectedValueException; /** @@ -17,6 +18,12 @@ class Relation /** @var string Name of the relation */ protected $name; + /** @var ?string Name of the reversed relation */ + protected ?string $reverseName = null; + + /** @var ?class-string The class to reverse the relation */ + protected ?string $reverseClass = null; + /** @var Model Source model */ protected $source; @@ -46,6 +53,10 @@ class Relation /** @var ?array Models additional JOIN conditions may reference, keyed by their alias */ protected ?array $filterSubjects = null; + + /** @var ?string The name of the relation prior reversal */ + private ?string $forwardRelationName = null; + /** * Get the default column name(s) in the source table used to match the foreign key * @@ -115,6 +126,56 @@ public function setName(string $name): static return $this; } + /** + * Get the reverse name of the relation + * + * @return ?string + */ + public function getReverseName(): ?string + { + return $this->reverseName; + } + + /** + * Set the reverse name of the relation + * + * The source's table alias is used by default. + * + * @param string $name + * + * @return $this + */ + public function setReverseName(string $name): static + { + $this->reverseName = $name; + + return $this; + } + + /** + * Get the class to reverse the relation + * + * @return class-string + */ + public function getReverseClass(): string + { + return $this->reverseClass ?? static::class; + } + + /** + * Set the class to reverse the relation + * + * @param class-string $reverseClass + * + * @return $this + */ + public function setReverseClass(string $reverseClass): static + { + $this->reverseClass = $reverseClass; + + return $this; + } + /** * Get the source model of the relation * @@ -398,6 +459,9 @@ public function bindTo(Model $source, string $path, Resolver $resolver): static $target->getTableAlias() => $target, $source->getTableAlias() => $source ]; + if ($this->forwardRelationName !== null) { + $subjects[$this->forwardRelationName] = $source; + } $this->addFilterSubjects(...$subjects); @@ -422,4 +486,89 @@ public function resolve(): Generator yield $this => [$source, $this->getTarget(), $this->determineKeys($source)]; } + + /** + * Reverse the relation + * + * Uses the passed resolver to eagerly create a relation on the reversed path. Either way, + * the result is still unknown to the given resolver and must be registered with it. + * + * @param Resolver $resolver + * + * @return Relation The reversed relation + * + * @throws LogicException In case the relation is not bound yet (has no source) or has already been reversed + * @throws RuntimeException In case the model of the forward relation is incompatible with the reversed relation's + */ + public function reverse(Resolver $resolver): Relation + { + if ($this->getSource() === null) { + throw new LogicException('Cannot reverse an unbound relation.'); + } elseif (isset($this->forwardRelationName)) { + throw new LogicException('Cannot undo a reverse.'); + } + + $reverseName = $this->getReverseName() ?? $this->getSource()->getTableAlias(); + + $relations = $resolver->getRelations($this->getTarget()); + + $exists = false; + if ($relations->has($reverseName)) { + if (! is_a($relations->get($reverseName), $this->getReverseClass())) { + trigger_error(sprintf( + 'The relation "%s" (%s) already exists but is not compatible with the expected reverse' + . ' relation class (%s != %s). To solve this, either use the same relation class, a different' + . ' reverse name or a different relation name on the target. You may also remove the relation' + . ' altogether, if the only reason it exists was to support sub-queries.', + $reverseName, + get_class($this->getTarget()), + get_class($relations->get($reverseName)), + $this->getReverseClass() + ), E_USER_DEPRECATED); + } else { + $exists = true; + } + } + + if ($exists) { + // Explicit reverse relations must be properly set up with corresponding key pairs + $relation = clone $relations->get($reverseName); + + if (! $this->getSource() instanceof ($relation->getTargetClass())) { + throw new RuntimeException(sprintf( + 'The source model of the relation "%s" (%s) is not compatible' + . ' with the target model of the inverse relation (%s)', + $this->getName(), + get_class($this->getSource()), + $relation->getTargetClass() + )); + } + } else { + // Eagerly create the relation in case it's only necessary during reversal + $relation = $relations->create( + $this->getReverseClass(), + $reverseName, + get_class($this->getSource()) + ); + + // Pass on custom configuration + $relation->setCandidateKey($this->getForeignKey()); + $relation->setForeignKey($this->getCandidateKey()); + $relation->setJoinType($this->getJoinType()); + } + + // The previous relation name must be kept for reference as relation filters + // may require it but need to be resolved to the source model instead. + $relation->forwardRelationName = $this->getName(); + + $relation->setTarget($this->getSource()); // Propagates the same instance + + if (! $this->getFilter()->isEmpty()) { + // Do not override set filters with an empty set, however, if the set is not empty + // the forward relation is expected to carry the same semantics as the inverse. + $relation->setFilter(clone $this->getFilter()); + } + + return $relation; + } } diff --git a/src/Relation/BelongsTo.php b/src/Relation/BelongsTo.php index 1982f10b..61419fce 100644 --- a/src/Relation/BelongsTo.php +++ b/src/Relation/BelongsTo.php @@ -10,4 +10,6 @@ class BelongsTo extends Relation { protected bool $inverse = true; + + protected ?string $reverseClass = HasMany::class; } diff --git a/src/Relation/BelongsToMany.php b/src/Relation/BelongsToMany.php index 2bda2e96..18171683 100644 --- a/src/Relation/BelongsToMany.php +++ b/src/Relation/BelongsToMany.php @@ -10,6 +10,7 @@ use ipl\Stdlib\Filter; use ipl\Stdlib\Filter\Rule; use LogicException; +use RuntimeException; /** * Many-to-many relationship @@ -330,6 +331,44 @@ public function resolve(): Generator yield from $toTarget->resolve(); } + public function reverse(Resolver $resolver): Relation + { + $relation = parent::reverse($resolver); + if ($relation->getThroughClass() !== null && $relation->getThroughClass() !== $this->getThroughClass()) { + throw new RuntimeException(sprintf( + 'The junction model of the relation "%s" (%s) is not compatible' + . ' with the junction model of the inverse relation (%s != %s)', + $this->getName(), + get_class($this->getSource()), + $relation->getThroughClass(), + $this->getThroughClass() + )); + } + + $relation->through($this->getThroughClass()); + $relation->setThrough($this->getThrough()); + $relation->setThroughAlias($this->getThroughAlias()); + + // The source table is allowed to reference in a join filter so this must ensure that this works on + // the way back as well. Since the source's instance is kept by parent::reverse() this should be safe. + $relation->addThroughFilterSubjects(...[$relation->getTarget()->getTableAlias() => $relation->getTarget()]); + + if (! $this->getThroughFilter()->isEmpty()) { + $relation->setThroughFilter(clone $this->getThroughFilter()); + } + + if (! $resolver->getRelations($this->getTarget())->has($relation->getName())) { + // The relation is eagerly set up and thus needs proper key pairs, + // but reversed as only the forward relation's pairs are known. + $relation->setCandidateKey($this->getTargetCandidateKey()); + $relation->setForeignKey($this->getTargetForeignKey()); + $relation->setTargetCandidateKey($this->getCandidateKey()); + $relation->setTargetForeignKey($this->getForeignKey()); + } + + return $relation; + } + protected function extractKey(array $possibleKey): string|array|null { $filtered = array_filter($possibleKey); diff --git a/src/Relation/HasMany.php b/src/Relation/HasMany.php index 13d1abc4..82ca669e 100644 --- a/src/Relation/HasMany.php +++ b/src/Relation/HasMany.php @@ -10,4 +10,6 @@ class HasMany extends Relation { protected bool $isOne = false; + + protected ?string $reverseClass = BelongsTo::class; } diff --git a/src/Relation/HasOne.php b/src/Relation/HasOne.php index 8f7a802a..fe56aa4b 100644 --- a/src/Relation/HasOne.php +++ b/src/Relation/HasOne.php @@ -9,4 +9,5 @@ */ class HasOne extends Relation { + protected ?string $reverseClass = BelongsTo::class; } diff --git a/tests/BelongsToManyTest.php b/tests/BelongsToManyTest.php index f1fbf468..4be3069d 100644 --- a/tests/BelongsToManyTest.php +++ b/tests/BelongsToManyTest.php @@ -8,6 +8,8 @@ use ipl\Orm\Resolver; use ipl\Sql\Test\SqlAssertions; use ipl\Stdlib\Filter; +use ipl\Tests\Orm\Lib\Model\Book; +use RuntimeException; class BelongsToManyTest extends \PHPUnit\Framework\TestCase { @@ -132,6 +134,44 @@ public function testSetThroughFilterWrapsABareConditionInAnAllChain() $this->assertSame([$condition], iterator_to_array($filter)); } + public function testThroughFilterSupportsSourceAndJunctionReferencesAtAllTimes(): void + { + $resolver = new Resolver($this->createStub(Query::class)); + $target = new User(); + $source = new Car(); + + $relation = (new BelongsToMany()) + ->setName('user') + ->setTarget($target) + ->through(CarUser::class) + ->setThroughAlias('my_through') + ->bindTo($source, 'car.user', $resolver); + + $this->assertSame( + [ + 'car' => $source, + 'car_user' => $relation->getThrough(), + 'my_through' => $relation->getThrough() + ], + $relation->getThroughFilterSubjects() + ); + + $reversed = $relation->reverse($resolver); + + $newSource = new User(); + $reversed->bindTo($newSource, 'user.car', $resolver); + + $this->assertSame( + [ + 'car' => $source, + 'user' => $newSource, + 'car_user' => $relation->getThrough(), + 'my_through' => $relation->getThrough() + ], + $reversed->getThroughFilterSubjects() + ); + } + public function testResolveYieldsJunctionAndTargetRelationsWithTheirFiltersAndJoinType() { $query = (new Query())->setModel(new Car()); @@ -190,4 +230,51 @@ public function testSetTargetCandidateKeyAcceptsNull() { $this->assertNull((new BelongsToMany())->setTargetCandidateKey(null)->getTargetCandidateKey()); } + + public function testReverseYieldsAnInverseBelongsToManyPreservingTheJunctionAndSwappingTheKeys() + { + $source = new Book(); + $resolver = (new Query())->setModel($source)->getResolver(); + // Book->author: many-to-many through a plain junction with explicit keys; Author declares no inverse, + // so it is created eagerly during reversal (which is where the key pairs must be exchanged) + $forward = $resolver->getRelations($source)->get('author')->bindTo($source, 'book.author', $resolver); + + $inverse = $forward->reverse($resolver); + + $this->assertInstanceOf(BelongsToMany::class, $inverse); + $this->assertSame('book', $inverse->getName()); + $this->assertSame($source, $inverse->getTarget()); + + // The junction is preserved ... + $this->assertSame($forward->getThroughClass(), $inverse->getThroughClass()); + $this->assertSame($forward->getThroughAlias(), $inverse->getThroughAlias()); + + // ... and the source-side and target-side key pairs are exchanged as a whole + $this->assertSame($forward->getTargetCandidateKey(), $inverse->getCandidateKey()); + $this->assertSame($forward->getTargetForeignKey(), $inverse->getForeignKey()); + $this->assertSame($forward->getCandidateKey(), $inverse->getTargetCandidateKey()); + $this->assertSame($forward->getForeignKey(), $inverse->getTargetForeignKey()); + } + + public function testReverseThrowsInCaseTheThroughTableIsDifferent(): void + { + $resolver = new Resolver($this->createStub(Query::class)); + + $relation = (new BelongsToMany()) + ->setName('user') + ->setTargetClass(User::class) + ->through('car_user') + ->bindTo(new Car(), 'car.user', $resolver); + + $this->expectException(RuntimeException::class); + $this->expectExceptionMessage(sprintf( + 'The junction model of the relation "user" (%s) is not compatible' + . ' with the junction model of the inverse relation (%s != %s)', + Car::class, + CarUser::class, + 'car_user' + )); + + $relation->reverse($resolver); + } } diff --git a/tests/Lib/Model/Author.php b/tests/Lib/Model/Author.php new file mode 100644 index 00000000..34c53af3 --- /dev/null +++ b/tests/Lib/Model/Author.php @@ -0,0 +1,33 @@ +author must create it eagerly, and there is + // no junction model to re-derive the keys from, so the reversed keys come solely from reverse(). + } +} diff --git a/tests/Lib/Model/Book.php b/tests/Lib/Model/Book.php new file mode 100644 index 00000000..f4910dcf --- /dev/null +++ b/tests/Lib/Model/Book.php @@ -0,0 +1,40 @@ +belongsToMany('author', Author::class) + ->through('authorship') + ->setCandidateKey('book_no') // book column + ->setForeignKey('authored_book') // junction column referencing the book + ->setTargetForeignKey('authoring') // junction column referencing the author + ->setTargetCandidateKey('author_ref'); // author column + } +} diff --git a/tests/Lib/Model/Chair.php b/tests/Lib/Model/Chair.php index f5e6ab2e..ff326370 100644 --- a/tests/Lib/Model/Chair.php +++ b/tests/Lib/Model/Chair.php @@ -28,6 +28,6 @@ public function getColumns() public function createRelations(Relations $relations) { - $relations->belongsTo('employee', Employee::class); + $relations->hasOne('employee', Employee::class); } } diff --git a/tests/RelationTest.php b/tests/RelationTest.php index c9ceadb3..fe2896fc 100644 --- a/tests/RelationTest.php +++ b/tests/RelationTest.php @@ -2,8 +2,19 @@ namespace ipl\Tests\Orm; +use ipl\Orm\Query; use ipl\Orm\Relation; +use ipl\Orm\Relation\BelongsTo; +use ipl\Orm\Relation\HasMany; +use ipl\Orm\Relation\HasOne; +use ipl\Orm\Resolver; +use ipl\Sql\Connection; use ipl\Stdlib\Filter; +use ipl\Tests\Orm\Lib\Model\Department; +use ipl\Tests\Orm\Lib\Model\Employee; +use ipl\Tests\Orm\Lib\Model\RestrictedUser; +use LogicException; +use RuntimeException; class RelationTest extends \PHPUnit\Framework\TestCase { @@ -206,4 +217,119 @@ public function testResolveYieldsTheRelationItselfAsKey() $this->assertSame([$relation], $keys); } + + public function testGetReverseNameReturnsNullByDefault() + { + $this->assertNull((new Relation())->getReverseName()); + } + + public function testSetReverseNameSetsTheReverseName() + { + $this->assertSame('foo', (new Relation())->setReverseName('foo')->getReverseName()); + } + + public function testGetReverseClassFallsBackToTheRelationsOwnClass() + { + $this->assertSame(Relation::class, (new Relation())->getReverseClass()); + // Subclasses provide sensible defaults + $this->assertSame(BelongsTo::class, (new HasMany())->getReverseClass()); + $this->assertSame(HasMany::class, (new BelongsTo())->getReverseClass()); + } + + public function testSetReverseClassOverridesTheDefault() + { + $this->assertSame( + HasOne::class, + (new BelongsTo())->setReverseClass(HasOne::class)->getReverseClass() + ); + } + + public function testReverseThrowsIfTheRelationIsUnbound() + { + $this->expectException(LogicException::class); + $this->expectExceptionMessage('Cannot reverse an unbound relation'); + + (new HasMany())->reverse($this->createStub(Resolver::class)); + } + + public function testReverseReusesADeclaredInverseRelation() + { + $source = new Department(); + $resolver = (new Query())->setModel($source)->getResolver(); + // Binding qualifies the filter and registers the target alias, as resolveRelations() would + $forward = $resolver->getRelations($source) + ->get('employee') + ->bindTo($source, 'department.employee', $resolver); + + $resolver->getRelations($forward->getTarget()) + ->get('department') + ->setCandidateKey('office_id'); // Silly, but must be retained + + $inverse = $forward->reverse($resolver); + + // Employee declares a matching belongsTo 'department' (named after the source's table alias) which + // is reused as the inverse and re-targeted at the very source instance + $this->assertSame($resolver->getRelations($forward->getTarget())->get('department'), $inverse); + $this->assertSame('office_id', $inverse->getCandidateKey()); + $this->assertSame('department', $inverse->getName()); + $this->assertSame($source, $inverse->getTarget()); + } + + public function testReverseCreatesAnInverseRelationWhenNoneIsDeclared() + { + $source = new RestrictedUser(); + $resolver = (new Query())->setModel($source)->getResolver(); + // RestrictedGroup declares no relations, so the inverse has to be created eagerly + $forward = $resolver->getRelations($source) + ->get('restricted_group') + ->bindTo($source, 'restricted_user.restricted_group', $resolver); + + $inverse = $forward->reverse($resolver); + + $this->assertInstanceOf(BelongsTo::class, $inverse); + $this->assertSame('restricted_user', $inverse->getName()); + $this->assertSame($source, $inverse->getTarget()); + $this->assertInstanceOf(RestrictedUser::class, $inverse->getTarget()); + } + + public function testReverseThrowsIfADeclaredInverseTargetsAnIncompatibleModel() + { + $source = new Department(); + $forward = (new Query())->setModel($source)->getResolver()->getRelations($source)->get('employee') + ->setSource($source) + // Employee.office targets Office, but the source of this relation is a Department + ->setReverseName('office'); + + $this->expectException(RuntimeException::class); + $this->expectExceptionMessage('is not compatible with the target model of the inverse relation'); + + $forward->reverse((new Query())->getResolver()); + } + + public function testReverseInCaseAnExistingReverseNameUsesADifferentReverseClass() + { + $query = Employee::on($this->createMock(Connection::class)); + + $deprecations = []; + set_error_handler(function ($_, $message) use (&$deprecations) { + $deprecations[] = $message; + + return true; + }, E_USER_DEPRECATED); + + try { + $reversed = $query->getResolver()->resolveRelation('employee.chair')->reverse($query->getResolver()); + } finally { + restore_error_handler(); + } + + $this->assertNotEmpty($deprecations, 'Reversal did not emit a deprecation'); + $this->assertStringContainsString( + 'The relation "employee" (ipl\Tests\Orm\Lib\Model\Chair) already exists' + . ' but is not compatible with the expected reverse relation class', + $deprecations[0] + ); + + $this->assertInstanceOf(BelongsTo::class, $reversed); + } } From d264ef7af96a806dc273d8fbe3c4943c7f0efaa0 Mon Sep 17 00:00:00 2001 From: Johannes Meyer Date: Thu, 20 Aug 2026 14:35:13 +0200 Subject: [PATCH 4/5] =?UTF-8?q?Query:=20Use=20`Relation::reverse()`=20inst?= =?UTF-8?q?ead=20of=20`array=5Freverse`=E2=80=A6?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Since `::reverse()` uses the source's table alias by default as reverse name, a deprecation notice is triggered if the original forward path uses a different name, indicating that it is necessary to use this name as explicit reverse name. As `Query::createSubQuery()` now overrides the source's relations, `Query::derive()` cannot use it anymore as otherwise the result will not have the relations it had previously. This must be done in order to avoid breaking current usages that rely on specific relations for filters or columns. A side-effect of this is, that the resulting query does not use the `sub_` alias prefix anymore. fixes #170 --- src/Query.php | 102 +++++++++++++++++++---- src/Relations.php | 2 + src/Resolver.php | 15 ++++ tests/HydratorTest.php | 4 +- tests/Lib/Model/Department.php | 2 +- tests/Lib/Model/Loose.php | 32 +++++++ tests/Lib/Model/Relationship.php | 32 +++++++ tests/QueryTest.php | 32 +++++++ tests/RelationTest.php | 48 ++++++++++- tests/VisibilityFilterTest.php | 139 +++++++++++++++++++++++++++---- 10 files changed, 371 insertions(+), 37 deletions(-) create mode 100644 tests/Lib/Model/Loose.php create mode 100644 tests/Lib/Model/Relationship.php diff --git a/src/Query.php b/src/Query.php index 745e1795..b0da23fc 100644 --- a/src/Query.php +++ b/src/Query.php @@ -7,7 +7,10 @@ use InvalidArgumentException; use ipl\Orm\Common\SortUtil; use ipl\Orm\Compat\FilterProcessor; +use ipl\Orm\Relation\BelongsTo; use ipl\Orm\Relation\BelongsToMany; +use ipl\Orm\Relation\BelongsToOne; +use ipl\Orm\Relation\HasOne; use ipl\Sql\Connection; use ipl\Sql\ExpressionInterface; use ipl\Sql\LimitOffset; @@ -22,6 +25,7 @@ use ipl\Stdlib\Filters; use IteratorAggregate; use ReflectionClass; +use RuntimeException; use SplObjectStorage; use Traversable; @@ -616,21 +620,63 @@ public function createHydrator(): Hydrator /** * Derive a new query to load the specified relation from a concrete model * + * The passed source can be referenced in filters using the `self` relation path. + * * @param string $relation * @param TRow $source * * @return static<*> * * @throws InvalidArgumentException If the relation with the given name does not exist + * @throws RuntimeException If the reversed relation does not meet the expected target */ public function derive($relation, Model $source): static { - // TODO: Think of a way to merge derive() and createSubQuery() - return $this->createSubQuery( - $this->getResolver()->getRelations($source)->get($relation)->getTarget(), + $relation = clone $this->getResolver()->resolveRelation( $this->getResolver()->qualifyPath($relation, $source->getTableAlias()), $source ); + $relation + ->setReverseName('self') + ->setReverseClass(match (get_class($relation)) { + BelongsToMany::class, BelongsToOne::class => BelongsToOne::class, + BelongsTo::class => HasOne::class, + default => BelongsTo::class + }); + + $query = $relation->getTargetClass()::on($this->getDb()); + $resolver = $query->getResolver(); + + $reversed = $relation->reverse($resolver); + + // This will fail if the name ("self") is occupied, but that's fine… + $resolver->getRelations($query->getModel())->add($reversed); + $reversed->bindTo($query->getModel(), $reversed->getName(), $resolver); + + $relatedKeys = null; + foreach ($reversed->resolve() as [$_, $target, $relatedKeys]) { + if ($target === $relation->getTarget()) { + break; + } + } + + if ($relatedKeys === null) { + throw new RuntimeException(sprintf( + 'Reversed relation "%s" (%s) does not resolve to the expected target: %s)', + $relation->getName(), + get_class($source), + $relation->getTargetClass() + )); + } + + foreach ($relatedKeys as $fk => $_) { + $query->filter(Filter::equal( + sprintf('%s.%s', $reversed->getName(), $fk), + $source->$fk + )); + } + + return $query; } /** @@ -651,27 +697,49 @@ public function createSubQuery(Model $target, string $targetPath, ?Model $from = ->setDb($this->getDb()) ->setModel($target); - $sourceParts = array_reverse(explode('.', $targetPath)); - $sourceParts[0] = $target->getTableAlias(); - $subQueryResolver = $subQuery->getResolver(); - $sourcePath = join('.', $sourceParts); - $originalRelations = iterator_to_array($this->getResolver()->resolveRelations($targetPath, $from), false); - foreach ($subQuery->getResolver()->resolveRelations($sourcePath) as $relation) { - $original = array_pop($originalRelations); + $forwardHops = array_slice(explode('.', $targetPath), 0, -1); + $forwardRelations = iterator_to_array($this->getResolver()->resolveRelations($targetPath, $from)); + + $previousHop = $target; + $sourceHops = [$target->getTableAlias()]; + foreach (array_reverse($forwardRelations) as $forwardPath => $relation) { + /** @var Relation $relation */ + $oppositeRelation = $relation->reverse($subQueryResolver); + + $predecessor = array_pop($forwardHops); + if ($relation->getReverseName() === null && $predecessor !== $oppositeRelation->getName()) { + trigger_error(sprintf( + 'Relation "%s" still uses the default table alias during reversal.' + . ' Use `%s::setReverseName("%s")` to get rid of this deprecation notice.', + $forwardPath, + $relation::class, + $predecessor + ), E_USER_DEPRECATED); + $oppositeRelation->setName($predecessor); + } - if ($relation instanceof BelongsToMany) { - $relation->setFilter($original->getThroughFilter()); - $relation->setThroughFilter($original->getFilter()); - } else { - $relation->setFilter($original->getFilter()); + $relations = new Relations(); + $relations->add($oppositeRelation); + foreach ($subQueryResolver->getRelations($previousHop) as $detour) { + // Keep remaining relations. I did not want this initially, but it seems to be required… + if ($detour->getName() !== $oppositeRelation->getName()) { + $relations->add($detour); + } } - $subQueryTarget = $relation->getTarget(); + $subQueryResolver->setRelations($previousHop, $relations); + + $sourceHops[] = $oppositeRelation->getName(); + $previousHop = $oppositeRelation->getTarget(); } - $subQuery->utilize($sourcePath); // TODO: Don't join if there's a matching foreign key + unset($previousHop); + $sourcePath = join('.', $sourceHops); + + // Up until here only the required relations are eagerly registered but not used yet + $subQueryTarget = $subQuery->utilize($sourcePath)->getResolver()->resolveRelation($sourcePath)->getTarget(); if (! $link) { $subQuery->columns(array_map(function ($keyName) use ($sourcePath) { diff --git a/src/Relations.php b/src/Relations.php index e9240969..e5010607 100644 --- a/src/Relations.php +++ b/src/Relations.php @@ -17,6 +17,8 @@ /** * Collection of a model's relations. + * + * @implements IteratorAggregate */ class Relations implements IteratorAggregate { diff --git a/src/Resolver.php b/src/Resolver.php index fb2fb862..17c492a2 100644 --- a/src/Resolver.php +++ b/src/Resolver.php @@ -75,6 +75,21 @@ public function __construct(Query $query) $this->visibilityFilters = new SplObjectStorage(); } + /** + * Override a model's default relations with the given ones + * + * @param Model $model + * @param Relations $relations + * + * @return $this + */ + public function setRelations(Model $model, Relations $relations): static + { + $this->relations->offsetSet($model, $relations); + + return $this; + } + /** * Get a model's relations * diff --git a/tests/HydratorTest.php b/tests/HydratorTest.php index 56368df6..bc3a7ef1 100644 --- a/tests/HydratorTest.php +++ b/tests/HydratorTest.php @@ -30,10 +30,10 @@ public function testWhetherProperlyQualifiedColumnsAreOnlyPassedOnToMatchingTarg $hydrator = $query->createHydrator(); - $subject = new Car(); + $subject = new Car(['id' => 1]); $hydrator->hydrate(['car_user_custom_keys_username' => 'foo'], $subject); - $subject2 = new Car(); + $subject2 = new Car(['id' => 2]); $hydrator->hydrate(['car_user_custom_keys_username' => 'bar'], $subject2); $this->assertFalse( diff --git a/tests/Lib/Model/Department.php b/tests/Lib/Model/Department.php index ce72ef13..1c4cdfd7 100644 --- a/tests/Lib/Model/Department.php +++ b/tests/Lib/Model/Department.php @@ -33,7 +33,7 @@ public function createRelations(Relations $relations) // Relation filter referencing the target (default) and the source table alias $relations->hasMany('lead', Employee::class) ->setFilter(Filter::all( - Filter::equal('employee.role', 'lead'), + Filter::equal('role', 'lead'), Filter::equal('department.name', 'Engineering') )); } diff --git a/tests/Lib/Model/Loose.php b/tests/Lib/Model/Loose.php new file mode 100644 index 00000000..c00caf80 --- /dev/null +++ b/tests/Lib/Model/Loose.php @@ -0,0 +1,32 @@ +hasMany('relationship', Relationship::class) + ->setForeignKey('coupler') + ->setCandidateKey('coupler') + ->setJoinType('LEFT'); + } +} diff --git a/tests/Lib/Model/Relationship.php b/tests/Lib/Model/Relationship.php new file mode 100644 index 00000000..2b1bde32 --- /dev/null +++ b/tests/Lib/Model/Relationship.php @@ -0,0 +1,32 @@ +hasMany('loose', Loose::class) + ->setForeignKey('coupler') + ->setCandidateKey('coupler') + ->setJoinType('LEFT'); + } +} diff --git a/tests/QueryTest.php b/tests/QueryTest.php index 0c009615..e6bd5317 100644 --- a/tests/QueryTest.php +++ b/tests/QueryTest.php @@ -7,6 +7,7 @@ use ipl\Orm\ResolvedExpression; use ipl\Sql\Expression; use ipl\Sql\Test\TestCase; +use ipl\Stdlib\Filter; class QueryTest extends TestCase { @@ -607,4 +608,35 @@ public function testWithoutColumnsDoesNotWorkWithExpressions() $query->assembleSelect() ); } + + /** + * This test asserts that passing an unreferenced target model to {@see \ipl\Orm\Query::createSubQuery} + * works without an error, to ensure that path reversal keeps model references intact. + */ + public function testUnreferencedTargetCanBePassedToCreateSubQuery(): void + { + $query = Profile::on(new TestConnection()) + ->createSubQuery(new User(), 'profile.user'); + + $this->assertInstanceOf(Query::class, $query); + } + + public function testDeriveLetsTheBaseModelBeReferencedAsSelfInFilters(): void + { + $user = new User(['id' => 1, 'username' => 'test']); + $profile = (new Query())->setDb(new TestConnection())->derive('profile', $user); + $profile->filter(Filter::equal('self.username', 'test')); + + $this->assertSql( + <<<'SQL' +SELECT profile.id, profile.user_id, profile.given_name, profile.surname +FROM profile +INNER JOIN user profile_self ON profile_self.id = profile.user_id +WHERE (profile_self.id = ?) AND (profile_self.username = ?) +SQL, + $profile->assembleSelect(), + [1, 'test'], + 'The base model should be referenced as "self" in filters when deriving a query' + ); + } } diff --git a/tests/RelationTest.php b/tests/RelationTest.php index fe2896fc..d76351fd 100644 --- a/tests/RelationTest.php +++ b/tests/RelationTest.php @@ -9,15 +9,24 @@ use ipl\Orm\Relation\HasOne; use ipl\Orm\Resolver; use ipl\Sql\Connection; +use ipl\Sql\Test\SqlAssertions; use ipl\Stdlib\Filter; use ipl\Tests\Orm\Lib\Model\Department; use ipl\Tests\Orm\Lib\Model\Employee; +use ipl\Tests\Orm\Lib\Model\Loose; use ipl\Tests\Orm\Lib\Model\RestrictedUser; use LogicException; use RuntimeException; class RelationTest extends \PHPUnit\Framework\TestCase { + use SqlAssertions; + + public function setUp(): void + { + $this->setUpSqlAssertions(); + } + public function testGetNameReturnsNullIfUnset() { $this->assertNull((new Relation())->getName()); @@ -269,12 +278,49 @@ public function testReverseReusesADeclaredInverseRelation() // Employee declares a matching belongsTo 'department' (named after the source's table alias) which // is reused as the inverse and re-targeted at the very source instance - $this->assertSame($resolver->getRelations($forward->getTarget())->get('department'), $inverse); $this->assertSame('office_id', $inverse->getCandidateKey()); $this->assertSame('department', $inverse->getName()); $this->assertSame($source, $inverse->getTarget()); } + public function testADeclaredInverseRelationCanBeReusedDuringReverse() + { + $connection = $this->createMock(Connection::class); + $connection->method('select')->willReturnCallback(function () { + $stmt = $this->createMock(\PDOStatement::class); + $stmt->expects($this->once())->method('setFetchMode')->with(\PDO::FETCH_ASSOC); + $stmt->method('getIterator')->willReturn(new \ArrayIterator([ + ['id' => 1, 'coupler' => 'test'] + ])); + + return $stmt; + }); + + $loose = Loose::on($connection) + ->filter(Filter::equal('id', 1)) + ->columns('id') + ->first(); + + $others = $loose->relationship->filter(Filter::unequal('loose.id', 1)); + + $this->assertSql( + <<<'SQL' + SELECT relationship.id, relationship.coupler + FROM relationship + LEFT JOIN loose relationship_self ON relationship_self.coupler = relationship.coupler + WHERE (relationship_self.coupler = ?) + AND ((relationship.id NOT IN ((SELECT sub_loose_relationship.id AS sub_loose_relationship_id + FROM loose sub_loose + LEFT JOIN relationship sub_loose_relationship ON sub_loose_relationship.coupler = sub_loose.coupler + WHERE (sub_loose.id = ?) AND (sub_loose_relationship.id IS NOT NULL) + GROUP BY sub_loose_relationship.id + HAVING COUNT(DISTINCT sub_loose.id) >= ?)) OR relationship.id IS NULL)) + SQL, + $others->assembleSelect(), + ['test', 1, 1] + ); + } + public function testReverseCreatesAnInverseRelationWhenNoneIsDeclared() { $source = new RestrictedUser(); diff --git a/tests/VisibilityFilterTest.php b/tests/VisibilityFilterTest.php index 26a7fcbc..03cdff45 100644 --- a/tests/VisibilityFilterTest.php +++ b/tests/VisibilityFilterTest.php @@ -5,6 +5,7 @@ use ipl\Orm\Query; use ipl\Sql\Test\SqlAssertions; use ipl\Stdlib\Filter; +use ipl\Tests\Orm\Lib\Model\Book; use ipl\Tests\Orm\Lib\Model\Department; use ipl\Tests\Orm\Lib\Model\Node; use ipl\Tests\Orm\Lib\Model\RestrictedGroup; @@ -108,6 +109,27 @@ public function testSelfReferencingRelationFilterIsAppliedToTheTarget() ); } + public function testSelfReferencingRelationFilterCanBeFilteredByItsName() + { + $query = Node::on(new TestConnection()) + ->columns('name') + ->filter(Filter::equal('child.name', 'John Doe')); + + $this->assertSql( + <<<'SQL' + SELECT node.name + FROM node + WHERE (node.deleted = ?) AND (node.id IN ((SELECT sub_node_node.id AS sub_node_node_id + FROM node sub_node + INNER JOIN node sub_node_node ON (sub_node_node.id = sub_node.parent_id) + AND ((sub_node.name = ?) AND (sub_node_node.deleted = ?)) + WHERE (sub_node.deleted = ?) AND (sub_node.name = ?)))) + SQL, + $query->assembleSelect(), + ['n', 'foo', 'n', 'n', 'John Doe'] + ); + } + public function testRelationFilterIsAppliedToJoinCondition() { $query = (new Query()) @@ -207,14 +229,54 @@ public function testBelongsToManyThroughAndRelationFiltersAreAppliedToReversedJo FROM car sub_car INNER JOIN car_user sub_car_car_user ON (sub_car_car_user.car_id = sub_car.id) - AND (sub_car.manufacturer = ?) + AND (sub_car_car_user.user_id = ?) INNER JOIN restricted_user sub_car_restricted_user ON (sub_car_restricted_user.id = sub_car_car_user.restricted_user_id) - AND (sub_car_car_user.user_id = ?) + AND (sub_car.manufacturer = ?) WHERE sub_car.model_name = ?)) SQL, $query->assembleSelect(), - ['Icinga', 5, 'volkswagen'] + [5, 'Icinga', 'volkswagen'] + ); + } + + public function testBelongsToManyWithMandatoryKeysJoinsCorrectly() + { + // Sanity anchor for the forward direction of the reversal regression below + $query = (new Query()) + ->setModel(new Book()) + ->columns('title') + ->utilize('author'); + + $this->assertSql( + 'SELECT book.title FROM book' + . ' INNER JOIN authorship book_authorship ON book_authorship.authored_book = book.book_no' + . ' INNER JOIN author book_author ON book_author.author_ref = book_authorship.authoring', + $query->assembleSelect() + ); + } + + public function testBelongsToManyWithMandatoryKeysReversesThemCorrectlyInASubQuery() + { + // Book->author uses a plain junction and non-conventional keys that must be declared explicitly. + // When reversed for the sub-query there is no junction model or default to re-derive them from, so + // the source-side and target-side key pairs must be exchanged as a whole. Regression for the bug + // where BelongsToMany::reverse() only flipped candidate<->foreign within each side. + $query = (new Query()) + ->setDb(new TestConnection()) + ->setModel(new Book()) + ->columns('title') + ->filter(Filter::equal('author.name', 'x')); + + $this->assertSql( + 'SELECT book.title FROM book WHERE book.book_no IN ((SELECT' + . ' sub_author_book.book_no AS sub_author_book_book_no FROM author sub_author' + . ' INNER JOIN authorship sub_author_authorship' + . ' ON sub_author_authorship.authoring = sub_author.author_ref' + . ' INNER JOIN book sub_author_book ON sub_author_book.book_no = sub_author_authorship.authored_book' + . ' WHERE sub_author.name = ?))', + $query->assembleSelect(), + ['x'] ); } @@ -302,12 +364,12 @@ public function testDeriveAppliesTheModelVisibilityFilterAndTheRelationFilter() $derived = $query->derive('employee', new Department(['id' => 1])); $this->assertSql( - 'SELECT sub_employee.id, sub_employee.name, sub_employee.active, sub_employee.deleted,' - . ' sub_employee.role, sub_employee.department_id, sub_employee.office_id' - . ' FROM employee sub_employee' - . ' INNER JOIN department sub_employee_department' - . ' ON (sub_employee_department.id = sub_employee.department_id) AND (sub_employee.active = ?)' - . ' WHERE (sub_employee.deleted = ?) AND (sub_employee_department.id = ?)', + 'SELECT employee.id, employee.name, employee.active, employee.deleted,' + . ' employee.role, employee.department_id, employee.office_id' + . ' FROM employee' + . ' LEFT JOIN department employee_self' + . ' ON (employee_self.id = employee.department_id) AND (employee.active = ?)' + . ' WHERE (employee.deleted = ?) AND (employee_self.id = ?)', $derived->assembleSelect(), ['y', 'n', 1] ); @@ -324,18 +386,63 @@ public function testDeriveAppliesARelationFilterThatReferencesTheSourceTable() $derived = $query->derive('lead', new Department(['id' => 1])); $this->assertSql( - 'SELECT sub_employee.id, sub_employee.name, sub_employee.active, sub_employee.deleted,' - . ' sub_employee.role, sub_employee.department_id, sub_employee.office_id' - . ' FROM employee sub_employee' - . ' INNER JOIN department sub_employee_department' - . ' ON (sub_employee_department.id = sub_employee.department_id)' - . ' AND ((sub_employee.role = ?) AND (sub_employee_department.name = ?))' - . ' WHERE (sub_employee.deleted = ?) AND (sub_employee_department.id = ?)', + 'SELECT employee.id, employee.name, employee.active, employee.deleted,' + . ' employee.role, employee.department_id, employee.office_id' + . ' FROM employee' + . ' INNER JOIN department employee_self' + . ' ON (employee_self.id = employee.department_id)' + . ' AND ((employee.role = ?) AND (employee_self.name = ?))' + . ' WHERE (employee.deleted = ?) AND (employee_self.id = ?)', $derived->assembleSelect(), ['lead', 'Engineering', 'n', 1] ); } + public function testSubQueryReversalEmitsADeprecationWhenARelationUsesTheDefaultReverseName() + { + // Filtering through "lead" (whose name differs from its target's table alias "employee") into the + // deeper to-many "ticket" reverses two hops. Reversing the ticket hop falls back to the source's + // table alias ("employee"), which differs from the path segment ("lead"), so a deprecation nudges + // towards setReverseName(). The produced SQL still uses the path segment for backwards compatibility. + $query = (new Query()) + ->setDb(new TestConnection()) + ->setModel(new Department()) + ->columns('name') + ->filter(Filter::equal('lead.ticket.subject', 'x')); + + $deprecations = []; + set_error_handler(function ($_, $message) use (&$deprecations) { + $deprecations[] = $message; + + return true; + }, E_USER_DEPRECATED); + + try { + $select = $query->assembleSelect(); + } finally { + restore_error_handler(); + } + + $this->assertNotEmpty($deprecations, 'Reversal did not emit a deprecation'); + $this->assertStringContainsString( + 'Relation "department.lead.ticket" still uses the default table alias during reversal', + $deprecations[0] + ); + + $this->assertSql( + 'SELECT department.name FROM department WHERE department.id IN ((SELECT' + . ' sub_ticket_lead_department.id AS sub_ticket_lead_department_id FROM ticket sub_ticket' + . ' INNER JOIN employee sub_ticket_lead ON (sub_ticket_lead.id = sub_ticket.employee_id)' + . ' AND ((sub_ticket.open = ?) AND (sub_ticket_lead.deleted = ?))' + . ' INNER JOIN department sub_ticket_lead_department' + . ' ON (sub_ticket_lead_department.id = sub_ticket_lead.department_id)' + . ' AND ((sub_ticket_lead.role = ?) AND (sub_ticket_lead_department.name = ?))' + . ' WHERE sub_ticket.subject = ?))', + $select, + ['y', 'n', 'lead', 'Engineering', 'x'] + ); + } + public function testModelVisibilityFilterColumnsAreNotValidated() { // Unlike relation filters, a model's visibility filter is not validated against selectable columns; From 3d3229f9cb0bf95041d041aedbf80894d8b3bb21 Mon Sep 17 00:00:00 2001 From: Johannes Meyer Date: Tue, 22 Sep 2026 09:54:36 +0200 Subject: [PATCH 5/5] Query: Return an empty query if derivation is impossible The new implementation of `::derive()` does not join the source table in the resulting query anymore and has no fallback like `::createSubQuery()` still has. The result is that an invalid filter is created with a `NULL` value. To avoid this, I opted to return a so-called `NoopQuery` instead of re-implementing the same fallback which issues a SQL query for no reason. --- src/NoopQuery.php | 51 +++++++++++++++++++++++++++++++++++++++++++++ src/Query.php | 4 ++++ tests/QueryTest.php | 13 ++++++++++++ 3 files changed, 68 insertions(+) create mode 100644 src/NoopQuery.php diff --git a/src/NoopQuery.php b/src/NoopQuery.php new file mode 100644 index 00000000..214e59bd --- /dev/null +++ b/src/NoopQuery.php @@ -0,0 +1,51 @@ + $_) { + if (! isset($source->$fk)) { + return new NoopQuery(); + } + $query->filter(Filter::equal( sprintf('%s.%s', $reversed->getName(), $fk), $source->$fk diff --git a/tests/QueryTest.php b/tests/QueryTest.php index e6bd5317..e0713239 100644 --- a/tests/QueryTest.php +++ b/tests/QueryTest.php @@ -3,6 +3,7 @@ namespace ipl\Tests\Orm; use ipl\Orm\Exception\InvalidRelationException; +use ipl\Orm\NoopQuery; use ipl\Orm\Query; use ipl\Orm\ResolvedExpression; use ipl\Sql\Expression; @@ -639,4 +640,16 @@ public function testDeriveLetsTheBaseModelBeReferencedAsSelfInFilters(): void 'The base model should be referenced as "self" in filters when deriving a query' ); } + + public function testDeriveReturnsAValidButEmptyQueryIfNoLinkIsPossible(): void + { + $user = new User(['username' => 'test']); // Deliberately has no id + $profile = (new Query())->setDb(new TestConnection())->derive('profile', $user); + // Deliberately used as if it is a usual query + $profile->filter(Filter::equal('self.username', 'test')); + + $this->assertInstanceOf(NoopQuery::class, $profile); + $this->assertEmpty(iterator_to_array($profile)); + $this->assertNull($profile->first()); + } }