Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Original entity data resolves inverse 1-1 joins #11109

Open
wants to merge 4 commits into
base: 2.17.x
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -582,9 +582,9 @@ public function loadOneToManyCollection(array $assoc, $sourceEntity, PersistentC
/**
* {@inheritDoc}
*/
public function loadOneToOneEntity(array $assoc, $sourceEntity, array $identifier = [])
public function loadOneToOneEntity(array $assoc, $sourceEntity, array $identifier = [], array $sourceEntityData = [])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The class is not marked as internal. This kind of breaking change could be avoided with func_get_args()

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't call that one in someone else's code base. I see some internal methods, but no internal classes. Is there anywhere you know of where this is used by something other than a UnitOfWork?

I would assume I'd get a lot of pushback from automated code checkers if I tried to slip something like this in the back door and pass more arguments than declared.

An id hash isn't available (all of the id data is still null in the entity), but the same original entity data should be available from the persister with $this->em->getUnitOfWork()->getOriginalEntityData($sourceEntity). This seems like a lot of work to do instead of just forwarding $data, but I would much rather take this approach than use func_get_args. This would also eliminate signature changes in most of the files.

Would you like me to take this approach to preserve the existing public signature?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, if by "a lot of work" you mean typing a few extra chars as opposed to a lot of work for php. I'm on my phone so can't check myself. As for internal classes, maybe they 're in another project, or just in an Internal namespace

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The runtime hit is small, so I removed the signature change. The fix is now limited to BasicEntityPersister (plus the tests).

I'm going to tangent for an opinion (for a possible separate defect in the same code path):
The identifier is not provided by UoW for the inverse one-to-one path. This looks like a defect, and may be the source of an issue I've known about for a while. Basically, in 1-1 scenarios, if you modify an entity that was pulled as part of a 1-1 then later request a related 1-1 entity from the repository before flushing, then the entity will be replaced with fresh data from the db and your edits will be lost.

I accepted this as 'part of the library' and spit code to cache all related 1-1s when an entity is retrieved, which redirects any request for a previously fetched related 1-1 before the UoW sees it. However, entity caching is the job of the UoW, so I'd rather not keep the secondary hack cache.

I think omitting the identifier argument on the loadOneToOneEntity call in UnitOfWork.php line 2966 is the likely cause of this problem. Do you think this should be investigated? (No rush, you probably don't want to do this one from your phone).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After reading https://www.doctrine-project.org/projects/doctrine-orm/en/2.17/reference/working-with-objects.html#entities-and-the-identity-map, my opinion is that this is indeed a bug since that behavior is not mentioned as a known issue or particular case. I think you can investigate it and that whatever you find, it could result in either a bugfix or a documentation improvement.

{
return $this->persister->loadOneToOneEntity($assoc, $sourceEntity, $identifier);
return $this->persister->loadOneToOneEntity($assoc, $sourceEntity, $identifier, $sourceEntityData);
}

/**
Expand Down
34 changes: 26 additions & 8 deletions lib/Doctrine/ORM/Persisters/Entity/BasicEntityPersister.php
Original file line number Diff line number Diff line change
Expand Up @@ -792,7 +792,7 @@ public function loadById(array $identifier, $entity = null)
/**
* {@inheritDoc}
*/
public function loadOneToOneEntity(array $assoc, $sourceEntity, array $identifier = [])
public function loadOneToOneEntity(array $assoc, $sourceEntity, array $identifier = [], array $sourceEntityData = [])
{
$foundEntity = $this->em->getUnitOfWork()->tryGetById($identifier, $assoc['targetEntity']);
if ($foundEntity !== false) {
Expand Down Expand Up @@ -830,14 +830,32 @@ public function loadOneToOneEntity(array $assoc, $sourceEntity, array $identifie
// TRICKY: since the association is specular source and target are flipped
foreach ($owningAssoc['targetToSourceKeyColumns'] as $sourceKeyColumn => $targetKeyColumn) {
if (! isset($sourceClass->fieldNames[$sourceKeyColumn])) {
throw MappingException::joinColumnMustPointToMappedField(
$sourceClass->name,
$sourceKeyColumn
);
}
// The likely case here is that the column is a join column
// in an association mapping. However, there is no guarantee
// at this point that a corresponding (generally identifying)
// association has been mapped in the source entity. To handle
// this case we directly reference the column-keyed data used
// to initialize the source entity before throwing an exception.
$resolvedSourceData = false;
if (isset($sourceEntityData[$sourceKeyColumn])) {
$dataValue = $sourceEntityData[$sourceKeyColumn];
if ($dataValue !== null) {
$resolvedSourceData = true;
$computedIdentifier[$targetClass->getFieldForColumn($targetKeyColumn)] =
$dataValue;
}
}

$computedIdentifier[$targetClass->getFieldForColumn($targetKeyColumn)] =
$sourceClass->reflFields[$sourceClass->fieldNames[$sourceKeyColumn]]->getValue($sourceEntity);
if (! $resolvedSourceData) {
throw MappingException::joinColumnMustPointToMappedField(
$sourceClass->name,
$sourceKeyColumn
);
}
} else {
$computedIdentifier[$targetClass->getFieldForColumn($targetKeyColumn)] =
$sourceClass->reflFields[$sourceClass->fieldNames[$sourceKeyColumn]]->getValue($sourceEntity);
}
}

$targetEntity = $this->load($computedIdentifier, null, $assoc);
Expand Down
15 changes: 10 additions & 5 deletions lib/Doctrine/ORM/Persisters/Entity/EntityPersister.php
Original file line number Diff line number Diff line change
Expand Up @@ -217,16 +217,21 @@ public function loadById(array $identifier, $entity = null);
* association from another entity.
*
* @param object $sourceEntity The entity that owns the association (not necessarily the "owning side").
* @psalm-param array<string, mixed> $identifier The identifier of the entity to load. Must be provided if
* the association to load represents the owning side, otherwise
* the identifier is derived from the $sourceEntity.
* @psalm-param AssociationMapping $assoc The association to load.
* @psalm-param array<string, mixed> $sourceEntityData The data used to create the sourceEntity. If the sourceEntity
* is identified by an association, then that association may
* not be initialize before this call. This original data is used
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* not be initialize before this call. This original data is used
* not be initialized before this call. This original data is used

* as a backup to access originating state that has not yet been
* written to a newly initialized sourceEntity.
* @psalm-param array<string, mixed> $identifier The identifier of the entity to load. Must be provided if
* the association to load represents the owning side, otherwise
* the identifier is derived from the $sourceEntity.
* @psalm-param AssociationMapping $assoc The association to load.
*
* @return object The loaded and managed entity instance or NULL if the entity can not be found.
*
* @throws MappingException
*/
public function loadOneToOneEntity(array $assoc, $sourceEntity, array $identifier = []);
public function loadOneToOneEntity(array $assoc, $sourceEntity, array $identifier = [], array $sourceEntityData = []);

/**
* Refreshes a managed entity.
Expand Down
4 changes: 2 additions & 2 deletions lib/Doctrine/ORM/UnitOfWork.php
Original file line number Diff line number Diff line change
Expand Up @@ -2963,7 +2963,7 @@ public function createEntity($className, array $data, &$hints = [])
}

// Inverse side of x-to-one can never be lazy
$class->reflFields[$field]->setValue($entity, $this->getEntityPersister($assoc['targetEntity'])->loadOneToOneEntity($assoc, $entity));
$class->reflFields[$field]->setValue($entity, $this->getEntityPersister($assoc['targetEntity'])->loadOneToOneEntity($assoc, $entity, [], $data));

continue 2;
}
Expand Down Expand Up @@ -3038,7 +3038,7 @@ public function createEntity($className, array $data, &$hints = [])
// If it might be a subtype, it can not be lazy. There isn't even
// a way to solve this with deferred eager loading, which means putting
// an entity with subclasses at a *-to-one location is really bad! (performance-wise)
$newValue = $this->getEntityPersister($assoc['targetEntity'])->loadOneToOneEntity($assoc, $entity, $associatedId);
$newValue = $this->getEntityPersister($assoc['targetEntity'])->loadOneToOneEntity($assoc, $entity, $associatedId, $data);
break;

default:
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
<?php

declare(strict_types=1);

namespace Doctrine\Tests\Models\OneToOneInverseSideWithAssociativeIdLoad;

use Doctrine\ORM\Mapping\Entity;
use Doctrine\ORM\Mapping\Id;
use Doctrine\ORM\Mapping\JoinColumn;
use Doctrine\ORM\Mapping\OneToOne;
use Doctrine\ORM\Mapping\Table;

/**
* @Entity()
* @Table(name="one_to_one_inverse_side_assoc_id_load_inverse")
*/
class InverseSide
greg0ire marked this conversation as resolved.
Show resolved Hide resolved
{
/**
* Associative id (owning identifier)
*
* @var InverseSideIdTarget
* @Id()
* @OneToOne(targetEntity=InverseSideIdTarget::class, inversedBy="inverseSide")
* @JoinColumn(nullable=false, name="associativeId")
*/
public $associativeId;

/**
* @var OwningSide
* @OneToOne(targetEntity=OwningSide::class, mappedBy="inverse")
*/
public $owning;
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
<?php

declare(strict_types=1);

namespace Doctrine\Tests\Models\OneToOneInverseSideWithAssociativeIdLoad;

use Doctrine\ORM\Mapping\Column;
use Doctrine\ORM\Mapping\Entity;
use Doctrine\ORM\Mapping\GeneratedValue;
use Doctrine\ORM\Mapping\Id;
use Doctrine\ORM\Mapping\OneToOne;
use Doctrine\ORM\Mapping\Table;

/**
* @Entity()
* @Table(name="one_to_one_inverse_side_assoc_id_load_inverse_id_target")
*/
class InverseSideIdTarget
{
/**
* @var string
* @Id()
* @Column(type="string", length=255)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The length is not relevant to the issue here, is it?

Suggested change
* @Column(type="string", length=255)
* @Column(type="string")

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, it isn't relevant here or in the OneToOneInverseSideLoad model this was directly copied from and is also the pattern in other tests I looked at. I generally have length-bounds on any column used in a key or index because dbs have limits, so I didn't think to remove it.

The more interesting question would be a different identifying type altogether (int or UUID, etc.).

The one concern I had with this fix, though, is that I wasn't sure if I should be using the raw input data. Normally this is processed through a reflField set/get value calls. I did not look into whether those ever normalize data or not. My rationale for skipping this question is that the raw data has just arrived from a SQL query and is being fed straight back into a sql query here, so should already be in a form suitable to use as a SQL parameter. Do you agree with this analysis?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, it isn't relevant here or in the OneToOneInverseSideLoad model this was directly copied from and is also the pattern in other tests I looked at.

I think I would remove any distracting things that aren't strictly necessary to reproduce the issue.

The one concern I had with this fix, though, is that I wasn't sure if I should be using the raw input data. Normally this is processed through a reflField set/get value calls. I did not look into whether those ever normalize data or not.

I don't know either, hopefully other maintainers that are more familiar with the internals of the ORM will.

My rationale for skipping this question is that the raw data has just arrived from a SQL query and is being fed straight back into a sql query here, so should already be in a form suitable to use as a SQL parameter. Do you agree with this analysis?

I think I do, but again, I'm not sure my opinion is worth much here.

* @GeneratedValue(strategy="NONE")
*/
public $id;

/**
* @var InverseSide
* @OneToOne(targetEntity=InverseSide::class, mappedBy="associativeId")
*/
public $inverseSide;
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
<?php

declare(strict_types=1);

namespace Doctrine\Tests\Models\OneToOneInverseSideWithAssociativeIdLoad;

use Doctrine\ORM\Mapping\Column;
use Doctrine\ORM\Mapping\Entity;
use Doctrine\ORM\Mapping\GeneratedValue;
use Doctrine\ORM\Mapping\Id;
use Doctrine\ORM\Mapping\JoinColumn;
use Doctrine\ORM\Mapping\OneToOne;
use Doctrine\ORM\Mapping\Table;

/**
* @Entity()
* @Table(name="one_to_one_inverse_side_assoc_id_load_owning")
*/
class OwningSide
{
/**
* @var string
* @Id()
* @Column(type="string", length=255)
* @GeneratedValue(strategy="NONE")
*/
public $id;

/**
* Owning side
*
* @var InverseSide
* @OneToOne(targetEntity=InverseSide::class, inversedBy="owning")
* @JoinColumn(name="inverse", referencedColumnName="associativeId")
*/
public $inverse;
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
<?php

declare(strict_types=1);

namespace Doctrine\Tests\ORM\Functional;

use Doctrine\Tests\Models\OneToOneInverseSideWithAssociativeIdLoad\InverseSide;
use Doctrine\Tests\Models\OneToOneInverseSideWithAssociativeIdLoad\InverseSideIdTarget;
use Doctrine\Tests\Models\OneToOneInverseSideWithAssociativeIdLoad\OwningSide;
use Doctrine\Tests\OrmFunctionalTestCase;

use function assert;

class OneToOneInverseSideWithAssociativeIdLoadAfterDqlQueryTest extends OrmFunctionalTestCase
{
protected function setUp(): void
{
parent::setUp();

$this->createSchemaForModels(OwningSide::class, InverseSideIdTarget::class, InverseSide::class);
}

/** @group GH-11108 */
public function testInverseSideWithAssociativeIdOneToOneLoadedAfterDqlQuery(): void
{
$owner = new OwningSide();
$inverseId = new InverseSideIdTarget();
$inverse = new InverseSide();

$owner->id = 'owner';
$inverseId->id = 'inverseId';
$inverseId->inverseSide = $inverse;
$inverse->associativeId = $inverseId;
$owner->inverse = $inverse;
$inverse->owning = $owner;

$this->_em->persist($owner);
$this->_em->persist($inverseId);
$this->_em->persist($inverse);
$this->_em->flush();
$this->_em->clear();

$fetchedInverse = $this
->_em
->createQueryBuilder()
->select('inverse')
->from(InverseSide::class, 'inverse')
->andWhere('inverse.associativeId = :associativeId')
->setParameter('associativeId', 'inverseId')
->getQuery()
->getSingleResult();
assert($fetchedInverse instanceof InverseSide);

self::assertInstanceOf(InverseSide::class, $fetchedInverse);
self::assertInstanceOf(InverseSideIdTarget::class, $fetchedInverse->associativeId);
self::assertInstanceOf(OwningSide::class, $fetchedInverse->owning);

$this->assertSQLEquals(
'select o0_.associativeid as associativeid_0 from one_to_one_inverse_side_assoc_id_load_inverse o0_ where o0_.associativeid = ?',
$this->getLastLoggedQuery(1)['sql']
);

$this->assertSQLEquals(
'select t0.id as id_1, t0.inverse as inverse_2 from one_to_one_inverse_side_assoc_id_load_owning t0 where t0.inverse = ?',
$this->getLastLoggedQuery()['sql']
);
}
}
Loading