Original entity data resolves inverse 1-1 joins #11109
Conversation
|
It does not appear that any static or dynamic failure here has anything to do with the changes in this commit. Is the current 2.17.x branch head passing its own test suite? |
|
Very weird… I tried locally, no issue on 2.17.x, and there were issues with your branch but… with Psalm 🤔 |
|
Ah my bad, I forgot to fetch before trying the build on 2.17.x Now I can see I was testing your branch with Psalm 2.15. |
| @@ -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 = []) | |||
There was a problem hiding this comment.
The class is not marked as internal. This kind of breaking change could be avoided with func_get_args()
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
| * @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 |
There was a problem hiding this comment.
| * not be initialize before this call. This original data is used | |
| * not be initialized before this call. This original data is used |
| /** | ||
| * @var string | ||
| * @Id() | ||
| * @Column(type="string", length=255) |
There was a problem hiding this comment.
The length is not relevant to the issue here, is it?
| * @Column(type="string", length=255) | |
| * @Column(type="string") |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
tests/Doctrine/Tests/Models/OneToOneInverseSideWithAssociativeIdLoad/InverseSide.php
Outdated
Show resolved
Hide resolved
|
Moved from 2.17.x to 2.20.x and consolidated change sets (after we changed the approach to get the original data). If you want this in the default branch (currently 3.2.x) instead please let me know. Could someone please review this? IMO this was ready to go 9 months ago. |
|
According to #11208, since this is a bug, you should pick 2.19.x |
If the source entity for an inverse (non-owning) 1-1 relationship is identified by an association then the identifying association may not be set when an inverse one-to-one association is resolved. This means that no data is available in the entity to resolve the needed column value for the join query. The original entity data can be retrieved from the unit of work and is used as a fallback to populate the query condition. Fixes doctrine#11108
|
Rebased. |
|
Thanks for the rebase Grégoire, even if my local is now confused : ) Now let's get this reviewed and in, please. This change is much more localized (no method signature changes) than the original proposal. |
|
Nothing a good old |
|
Thanks @mcurland ! |
If the source entity for an inverse (non-owning) 1-1 relationship is identified by an association then the identifying association may not be set when an inverse one-to-one association is resolved. This means that no data is available in the entity to resolve the needed column value for the join query.
Provide the original entity data for the source entity as a fallback to resolve the query conditions.
The new test will fail if the fix is removed (comment out lines 840-847 in BasicEntityPersister.php).
Fixes #11108