Fetching entities with Composite Key Relations and null values

michalbundyra commented 3 weeks ago

Recreation of #8425 as it was closed but never actually resolved and the issues still persist.

Description from the previous PR (Background):

In our database design we are using entities with composite keys, and we are using composite keys relations between entities.

Let say we have the following entities with Primary Keys (PK):

Company (PK: company_code)
Customer (PK composite: company_code + customer_code)
Invoice (PK composite: company_code + invoice_code)

and the following relations in Invoice:

to Company (using company_code), not nullable
to Customer (using composite key company_code + customer_code), nullable

Now we have company_code (not null) but no customer_code (null). We want to fetch Invoice entity, so that we get not null Company, and null Customer.

The problem is that loading Invoice with null Customer is not working - we are getting the following exception:

Exception: [Doctrine\Common\Proxy\Exception\OutOfBoundsException] Missing value for primary key code on Doctrine\Tests\Models\CompositeKeyRelations\CustomerClass

This PR provide a fix, to load the Customer in the described case. The change is pretty simply, and no other tests are affected.

NOTE: Loading Company is not a problem as it is using relation on just one column (not a composite key). The description includes it just for completeness of the example.

Workaround

We have discovered that we could use one of these workarounds - but both are not perfect:

do not use null field, use instead empty value (empty string)
use fetch=EAGER, what might be not efficient and actually might be not needed

Solution

The solution proposed in this PR is very minimal - just removal od one of the condition, which seems to be not covered, and I couldn't find track why it has been even added there. Based on the example provided in this PR, the removed condition seems to be invalid.

PR contains two commits:

tests with failing scenario
fix for the presented issue

Additional considerations

The following comment confused me a lot: https://github.com/doctrine/orm/blob/9d4f54b9a476f13479c3845350b12c466873fc42/src/UnitOfWork.php#L2456

and I did additional investigation if the proposed solution here is correct. In the below we are removing just first part of the condition: https://github.com/doctrine/orm/blob/9d4f54b9a476f13479c3845350b12c466873fc42/src/UnitOfWork.php#L2470-L2476 and I was wonder if the target entity could/should know if it is a part of a FK somewhere else - and I can't even see the reason behind that, so I believe this is the correct fix as it is the case when the filed "is part of target's entity primary key".

Hope it all makes sense and we can get it in. Thanks 🙌

darrenedale commented 3 weeks ago

This would definitely help with some of our use cases. 👍

greg0ire commented 1 week ago

Does this affect 3.x only? If not, you should target 2.19.x and then we merge this up.

michalbundyra commented 1 week ago

@greg0ire thanks, this is also a problem on 2.19.x, so I have recreated the PR here: https://github.com/doctrine/orm/pull/11506

Rebasing this one is quite tricky, as 2.19.x is still using annotations, support older PHP versions and has slightly different CS rules.

Thanks

SenseException commented 20 hours ago

I assume this will wait until the 2.19 branch gets upmerged before the work of this PR can proceed

greg0ire commented 4 hours ago

Here we go: https://github.com/doctrine/orm/pull/11525

doctrine / orm