zenstruck
Wrong entity creation order with OneToOne relationship (due to Doctine issue)
Before the PR #300, define a OneToOne bidirectional relation, with a cascade persist on the owner side the Factory do:
- create Acoustic
- persist Acoustic
- flush Acoustic
- create ProductionTarget
- persist ProductionTarget
- flush ProductionTarget
Now, with #300, the Factory do:
- create ProductionTarget
- persist ProductionTarget
- flush ProductionTarget
- create Acoustic
- persist Acoustic
- flush Acoustic
Is it expected to inverse the order ? In our case, it breaks things, because of a listener on the ProductionTarget that need to have access to the Acoustic.
The above example use that definition:
class Acoustic
{
#[ORM\OneToOne(inversedBy: 'acoustic', cascade: ['persist'])]
private ?ProductionTarget $productionTarget = null;
}
class ProductionTarget
{
#[Assert\NotNull]
#[ORM\OneToOne(mappedBy: 'productionTarget')]
private ?Acoustic $acoustic = null;
ProductionTargetFactory::createOne([
'acoustic' => AcousticFactory::new(),
]);
Hello there, I have not read my fix again but here is my first insight: If you need Acoustic to be created first, I think the owner of the relation should be ProductionTarget, not Acoustic as in your example. This happens because the Acoustic entity needs ProductionTarget to be persisted first in order to set its id on Acoustic table.
Can you provide more details about why you chose this side for the relationship please?
Also, if you think this is wrong, I will gladly read your opinion about it.
I think the owner of the relation should be ProductionTarget, not Acoustic
that was also my first feeling
Hi, here the choice is due to the fact that an Acoustic own a ProductionTarget. The Acoustic can live without a ProductionTarget, but a ProductionTarget cannot live without an Acoustic.
With Doctrine, when using a ManyToMany (both side) or a OneToMany, we have a PersistantCollection: that one is lazy, before Iterate it, Doctrine do not process database query.
The ManyToOne, because owning the relation have the ID, then can provide us a ProxyEntity, like PersistantCollection, before accessing a property, Doctrine do not query the database.
In the case of a OneToOne relation, it's slightly different: the owning side could lazy load the related entity, but the reverse side can't.
Like ManyToOne, the OneToOne owner side, own the relation (have the ID) of the related entity, and can provide us a ProxyEntity and only process query against the database if we access a property on it.
But the reverse side do not have the related id, then doctrine always process the query to load the related entity, it cause the n+1 effect.
Because of that, by putting the owning side on the ProductionTarget cause: on Acoustic load a second query to for the ProductionTarget. We always have both, but the Acoustic should be use without the ProductionTarget, this supplementary query is not attended.
By putting the owning side on the Acoustic, this one have a ProductionTarget Proxy Entity, and request it only when needed: this is the attended behavior. When we load a ProductionTarget, this one always process a second query to access the Acoustic. This behavior is okay, because the ProductionTarget always need it acoustic.
Find that post that explain it, maybe better than me: https://developer.happyr.com/choose-owning-side-in-onetoone-relation
The Acoustic can live without a Production target, a ProductionTarget canot live without an Acoustic.
In my opinion it is the opposite of your implementation. If Acoustic can live without a ProductionTarget, then Acoustic id should be on ProductionTarget, not the other way around (ProductionTarget "belongs to" Acoustic, even if Acoustic can only have one ProductionTarget).
Though, I do understand your issue with lazy loading. This is a known Doctrine issue. See https://github.com/doctrine/orm/issues/4389 (as you can see, multiple other packages run into related issues due to Doctrine "won't fix").
I think the code changing the behavior is the Factory::inverseRelationshipField method.
Sadly, I do not think we can "fix" this issue. We cannot assume you have implemented this workaround. For the Factory, the owner of the relation is Acoustic, therefore ProductionTarget must be persisted first. Assuming it is implementing the workaround would break both logic and non-workaround OneToOne. In my team, i.e., we do not use this workaround as we prefer to keep database model coherence. Instead, we use custom query builders when we do not want the relation to be loaded. I will not debate on the chosen workaround because it is a separating issue with Doctrine. I see some workarounds to make the Factory flush in correct order:
- Creating directly the Acoustic Factory by calling
->create(). This only works when using a simple Factory construction as in your example ; otherwise it may create multiple unwanted Acoustic (you can refer to the docs).
ProductionTargetFactory::createOne([
'acoustic' => AcousticFactory::new()->create(),
]);
- Using
beforeInstantiateorafterInstantiatecallbacks. The drawback is also related to creating multiple unwanted entities if you add the callback multiple times.
ProductionTargetFactory::new()->beforeInstantiate(function (array $attributes): array {
$attributes['acoustic'] = Acoustic::createOne();
return $attributes;
})->create();
ProductionTargetFactory::new()->afterInstantiate(function (ProductionTarget $productionTarget): array {
$productionTarget->setAcoustic(Acoustic::createOne());
})->create();
- Using beforeInstantiate callback with a private property (see below). I do not see drawbacks with this solution right now.
final class FactoryUniqueBeforeInstantiate {
public function __construct(
private array $attributes,
private bool $createFactories = false,
)
{
}
public function mergeAttributes(array $newAttributes): void
{
$this->attributes = array_merge($this->attributes, $newAttributes);
}
public function apply(array $factoryAttributes): array
{
if ($this->createFactories) {
foreach ($this->attributes as $key => $value) {
if (!($value instanceof Factory)) {
continue;
}
$this->attributes[$key] = $factory->create();
}
}
return array_merge($factoryAttributes, $this->attributes);
}
}
final class ProductionTargetFactory {
private FactoryUniqueBeforeInstantiate $uniqueBeforeInstantiate;
private bool $hasUniqueInstantiateCallback = false;
public function __construct()
{
$this->uniqueBeforeInstantiate = new FactoryUniqueBeforeInstantiate([], true);
}
public function withUniqueBeforeInstantiate(FactoryUniqueBeforeInstantiate $uniqueBeforeInstantiate): self
{
$new = clone $this;
$new->uniqueBeforeInstantiate = $uniqueBeforeInstantiate;
return $new;
}
public function withUniqueBeforeInstantiateAttributes(array $attributes): self
{
$this->uniqueBeforeInstantiate->mergeAttributes($attributes);
return $this;
}
public function addUniqueBeforeInstatiateCallback(): self
{
if ($this->hasUniqueInstantiateCallback) {
throw new \LogicException('Factory already has unique beforeInstantiate callback. ProductionTargetFactory::addUniqueBeforeInstatiateCallback should be called last.');
}
return $this->->beforeInstantiate(function(array $attributes) {
return $this->uniqueBeforeInstantiate->apply($attributes);
});
}
// ...other logic
}
ProductionTargetFactory::new()
->withUniqueBeforeInstantiateAttributes([
'acoustic' => Acoustic::new(),
])
// this will not create unwanted Acoustic entities
// ->withUniqueBeforeInstantiateAttributes([
// 'acoustic' => Acoustic::new(),
// ])
->create()
;
I did this last implementation this way because of how Factory internally works: each time you call ->withAttributes(), the object is cloned. This also means that when using a callback with private properties will call the Factory instance on which the callback was added, and we cannot retrieve the latest Factory because it is not given as an argument to those callbacks (this is a bit complex but I did not open an issue here). Using a private property with an instance of an object allows you to use the same instance accross all instances. Note that if you want to completely override the FactoryUniqueBeforeInstantiate, you should use withUniqueBeforeInstantiate.
(I do not have time to test it right now, but it should work)
Maybe @kbond or @nikophil have further thoughts on this?
Also, @nikophil could someone rename this issue Wrong entity creation order with OneToOne relationship (due to Doctine issue) or something like it in order for other people having this issue to find it easily?
Yes, I totally can create it in a different way for test purpose and DataFixtures.
The main goal of that issue is just to define what is the attended order of the creation of Entities by the Factory, and I think keep it fixed in the future to avoid errors. Like before, or like now, in my case, not really matter, but avoid BC breaks in the future seam important (this minor update force to refactor lot of tests and Fixtures because of inverse the Factory way to create Entities).
The other thing I don't know is, if the previous order was important or not (original behavior).
(4) on this list looks related.
I'm a little lost here - I personally try to avoid bi-directional associations as much as possible.
I am concerned that this was a BC break for @mpiot but I'm not seeing how to avoid...
Is there a test we can write that prevents further BC break on this topic?
I actually tried to avoid as much as possible the BC with my fix. Thing is, everything seem to work correctly from a logical point of view; IMO, having a listener requiring the relation ship on creation on a non-owning side breaks logic -> this is what causes the breaking change here.
I understand the issue though, I really tried to avoid those cases, but I did not implement tests which are not intended implementations... We could add a test for this specific case, because it is a known issue with Doctrine and this workaround is mentionned (multiple times, accross different sources).
hey, I'm closing this as this was a BC break we decided to keep. I think the subject is closed now