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

Conversation

mcurland
Copy link

@mcurland mcurland commented Dec 6, 2023

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

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 doctrine#11108
@mcurland
Copy link
Author

mcurland commented Dec 7, 2023

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?

@greg0ire
Copy link
Member

greg0ire commented Dec 8, 2023

Very weird… I tried locally, no issue on 2.17.x, and there were issues with your branch but… with Psalm 🤔
Let's close and reopen to trigger a whole run of the CI.

@greg0ire greg0ire closed this Dec 8, 2023
@greg0ire greg0ire reopened this Dec 8, 2023
@greg0ire
Copy link
Member

greg0ire commented Dec 8, 2023

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 = [])
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.

* @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

/**
* @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.

The original entity data for any entity is available from the unit of
work. Revert the loadOneToOneEntity signature change that passed the
original data directly to loadOneToOneEntity.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Loading inverse side of a one-to-one fails if target entity has an association id
2 participants