Doctrine 2 - ORM
  1. Doctrine 2 - ORM
  2. DDC-61

OneToOne relation with an entity using Class Table Inheritance fails

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Critical Critical
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 2.0-ALPHA3
    • Component/s: ORM
    • Security Level: All
    • Labels:
      None

      Description

      The bug occurs when there's a lazy bidirectional one-to-one relation where the inverse side only specifies the root entity. In this case, the final type of the target-entity is not resolved correctly. See the attached file for a failing test case.

      1. cti_test.diff
        2 kB
        Ville Väänänen
      2. cti_test.diff
        2 kB
        Ville Väänänen
      3. StandardEntityPersister.php.diff
        6 kB
        Ville Väänänen

        Activity

        Ville Väänänen created issue -
        Hide
        Ville Väänänen added a comment -

        The situation is actually worse if the relation is eager. In that case not only is the classname incorrectly resolved, but if the root-entity is an abstract class, the execution ends up in a fatal error.

        It would seem to me that there are two ways to deal with the described problem:

        1. join all the tables that contain the discriminator-fields, so that the correct subclasses can be inferred
        OR
        2. change the proxy pattern to a variant which is used in the FLOW3 framework, where there's only one proxy-class

        Show
        Ville Väänänen added a comment - The situation is actually worse if the relation is eager. In that case not only is the classname incorrectly resolved, but if the root-entity is an abstract class, the execution ends up in a fatal error. It would seem to me that there are two ways to deal with the described problem: 1. join all the tables that contain the discriminator-fields, so that the correct subclasses can be inferred OR 2. change the proxy pattern to a variant which is used in the FLOW3 framework, where there's only one proxy-class
        Hide
        Ville Väänänen added a comment - - edited

        A fix to the patch. Use the newer attachment.

        Show
        Ville Väänänen added a comment - - edited A fix to the patch. Use the newer attachment.
        Ville Väänänen made changes -
        Field Original Value New Value
        Attachment cti_test.diff [ 10103 ]
        Roman S. Borschel made changes -
        Priority Major [ 3 ] Critical [ 2 ]
        Hide
        Ville Väänänen added a comment -

        I have patches for both of the options above. Should this be a configuration option? As it stands now, OneToMany and ManyToMany relationships enjoy the advantage of having the Collection in between the target and source entities. We could even use a hybrid approach, where a general proxy-object is created only when the object type cannot be inferred without joins.

        Show
        Ville Väänänen added a comment - I have patches for both of the options above. Should this be a configuration option? As it stands now, OneToMany and ManyToMany relationships enjoy the advantage of having the Collection in between the target and source entities. We could even use a hybrid approach, where a general proxy-object is created only when the object type cannot be inferred without joins.
        Hide
        Roman S. Borschel added a comment -

        It should of course be Nr. 1. The discriminator column is always stored in the topmost (root) table and tables of base classes must be joined, always. If this is not the case this is a bug. Can you show your patch?

        Show
        Roman S. Borschel added a comment - It should of course be Nr. 1. The discriminator column is always stored in the topmost (root) table and tables of base classes must be joined, always. If this is not the case this is a bug. Can you show your patch?
        Hide
        Ville Väänänen added a comment -

        It would seem to me that this would cause an inconsistency in how similar associations with different kinds of entities are treated.

        From DDC-38: But inverse sides of one-one associations can never be lazy. Now going with option 1 would mean that in case the association is with an entity using class-table inheritance, these lazy associations would be allowed (because if we're joining the base-table anyway, there's no reason to forbid this) . This begs the question that if we're ready to do the joins in this case, should we reconsider doing them also to find out about target-existency when dealing with other types of entities?

        Show
        Ville Väänänen added a comment - It would seem to me that this would cause an inconsistency in how similar associations with different kinds of entities are treated. From DDC-38 : But inverse sides of one-one associations can never be lazy. Now going with option 1 would mean that in case the association is with an entity using class-table inheritance, these lazy associations would be allowed (because if we're joining the base-table anyway, there's no reason to forbid this) . This begs the question that if we're ready to do the joins in this case, should we reconsider doing them also to find out about target-existency when dealing with other types of entities?
        Hide
        Ville Väänänen added a comment - - edited

        Describing the changes StandardEntityPersister.php.diff:

        • _getSelectEntitiesSql
          • For every inverse OneToOne relation join the target-table/base-table and find out the existence of the target-entity and the value of the discriminator column if one should exist. These values are given the names of the sourceFieldNames in the result array
        • _createEntity
          • Pass the existence and discrimnator column values to unitOfWork->createEntity in the hints['associations'] array. This part is only speculative.

        This selects the existence/discrimator-column information when an entity is queried using EntityManager->find, it's doesn't help if one is using a DQL query.

        Show
        Ville Väänänen added a comment - - edited Describing the changes StandardEntityPersister.php.diff: _getSelectEntitiesSql For every inverse OneToOne relation join the target-table/base-table and find out the existence of the target-entity and the value of the discriminator column if one should exist. These values are given the names of the sourceFieldNames in the result array _createEntity Pass the existence and discrimnator column values to unitOfWork->createEntity in the hints ['associations'] array. This part is only speculative. This selects the existence/discrimator-column information when an entity is queried using EntityManager->find, it's doesn't help if one is using a DQL query.
        Ville Väänänen made changes -
        Attachment StandardEntityPersister.php.diff [ 10132 ]
        Hide
        Roman S. Borschel added a comment -

        I understand now what the problem is. Its tricky. Your testcase however has a wrong mapping. The way you mapped it, any event that is fetched of a company will automatically be the main event In this case it would be correct to put a join column on the organization (main_event_id). But I understand the problem and will create a separate new test for it.

        It will probably be treated similar to inverse sides of one-one. That means, when a one-one associations references a target entity that is a base type (not an outermost type, that means there are still possible subtypes), whether the association is the owning side or not it must be eagerly fetched.

        Show
        Roman S. Borschel added a comment - I understand now what the problem is. Its tricky. Your testcase however has a wrong mapping. The way you mapped it, any event that is fetched of a company will automatically be the main event In this case it would be correct to put a join column on the organization (main_event_id). But I understand the problem and will create a separate new test for it. It will probably be treated similar to inverse sides of one-one. That means, when a one-one associations references a target entity that is a base type (not an outermost type, that means there are still possible subtypes), whether the association is the owning side or not it must be eagerly fetched.
        Roman S. Borschel made changes -
        Status Open [ 1 ] In Progress [ 3 ]
        Roman S. Borschel made changes -
        Status In Progress [ 3 ] Closed [ 6 ]
        Fix Version/s 2.0-ALPHA3 [ 10029 ]
        Resolution Fixed [ 1 ]
        Hide
        Ville Väänänen added a comment -

        Okay, so not allowing lazy-loading in this case at all. Why is this? As I stated earlier there are at least to ways to solve this without resorting to eager-loading. If using class-table inheritance means forgetting about lazy-load, to me at least it's a huge disadvantage and would be a deal breaker in using this inheritance-type. The problem with eager-load is not the first fetch that follows, but the uncertainty of how many fetches are going to follow in total. If the target-entity is the tip of a huge object-hierarchy full of entities using class-table inheritance, the current strategy is unusable. Having a couple of extra tables joined is nothing compared to the snowball that eager-fetching might get rolling.

        Show
        Ville Väänänen added a comment - Okay, so not allowing lazy-loading in this case at all. Why is this? As I stated earlier there are at least to ways to solve this without resorting to eager-loading. If using class-table inheritance means forgetting about lazy-load, to me at least it's a huge disadvantage and would be a deal breaker in using this inheritance-type. The problem with eager-load is not the first fetch that follows, but the uncertainty of how many fetches are going to follow in total. If the target-entity is the tip of a huge object-hierarchy full of entities using class-table inheritance, the current strategy is unusable. Having a couple of extra tables joined is nothing compared to the snowball that eager-fetching might get rolling.
        Hide
        Roman S. Borschel added a comment -

        Well, option Nr.1 is too complicated and can lead to lots of new issues. You can not always join arbitrary tables, not when it comes to DQL where it alters the overall result. The situation is much more complex than it might look to you right now.

        Nr.2 is no option at all because such a proxy approach does not even hold for instanceof checks. An absolute no-go.

        What it does now is the best thing from my point of view. If performance becomes an issue you can always improve upon that by eager fetching with DQL. Also, you did not yet understand how it works. The eager load does not happen always with class table inheritance, only when the target entity is a base type. If it is a subtype that does not have any further subtypes, then a proxy can be used without fear. Same goes for single table inheritance.

        Show
        Roman S. Borschel added a comment - Well, option Nr.1 is too complicated and can lead to lots of new issues. You can not always join arbitrary tables, not when it comes to DQL where it alters the overall result. The situation is much more complex than it might look to you right now. Nr.2 is no option at all because such a proxy approach does not even hold for instanceof checks. An absolute no-go. What it does now is the best thing from my point of view. If performance becomes an issue you can always improve upon that by eager fetching with DQL. Also, you did not yet understand how it works. The eager load does not happen always with class table inheritance, only when the target entity is a base type. If it is a subtype that does not have any further subtypes, then a proxy can be used without fear. Same goes for single table inheritance.
        Hide
        Roman S. Borschel added a comment -

        If you still feel this is too bad open a new issue as "improvement" and move any old/new patches you have there. There would need to be much more tests for such a change. Also you did not mention whether your patch actually passes all existing unit tests.

        Show
        Roman S. Borschel added a comment - If you still feel this is too bad open a new issue as "improvement" and move any old/new patches you have there. There would need to be much more tests for such a change. Also you did not mention whether your patch actually passes all existing unit tests.
        Hide
        Ville Väänänen added a comment -

        Sure I understood how it works. But to me one of the biggest advantages of class-table inheritance is exactly that the parent doesn't have to know the final type., and so in the configuration the association is pointed to the base-type. Yes, I accept that the issue might be way too complicated in the DQL case.

        Option number two can be improved if the parent model that might have these general proxies invokes a load in it's getters. Exactly the way the generated proxies work now. Of course, this means that the models need to be aware of the possible proxies. It's not perfect, but maybe it could be an option?

        Show
        Ville Väänänen added a comment - Sure I understood how it works. But to me one of the biggest advantages of class-table inheritance is exactly that the parent doesn't have to know the final type., and so in the configuration the association is pointed to the base-type. Yes, I accept that the issue might be way too complicated in the DQL case. Option number two can be improved if the parent model that might have these general proxies invokes a load in it's getters. Exactly the way the generated proxies work now. Of course, this means that the models need to be aware of the possible proxies. It's not perfect, but maybe it could be an option?
        Hide
        Roman S. Borschel added a comment -

        Please open a new issue, I think this has potential as an improvement that can complement the current behavior.

        What I mean is: for normal find/load ... operations that are not triggered by DQL we can probably do the join and in the case of DQL the current behavior is used, if you want to avoid the extra query in that case its as simple as adding the join to DQL.

        So I think the approaches can be complementary.

        Show
        Roman S. Borschel added a comment - Please open a new issue, I think this has potential as an improvement that can complement the current behavior. What I mean is: for normal find/load ... operations that are not triggered by DQL we can probably do the join and in the case of DQL the current behavior is used, if you want to avoid the extra query in that case its as simple as adding the join to DQL. So I think the approaches can be complementary.
        Hide
        Ville Väänänen added a comment -

        Sure like the sound of that! I will be very happy to open this as an improvement. Thanks for your patience .

        Show
        Ville Väänänen added a comment - Sure like the sound of that! I will be very happy to open this as an improvement. Thanks for your patience .
        Benjamin Eberlei made changes -
        Workflow jira [ 10241 ] jira-feedback [ 15445 ]
        Benjamin Eberlei made changes -
        Workflow jira-feedback [ 15445 ] jira-feedback2 [ 17309 ]
        Benjamin Eberlei made changes -
        Workflow jira-feedback2 [ 17309 ] jira-feedback3 [ 19566 ]

        This list may be incomplete, as errors occurred whilst retrieving source from linked applications:

        • Request to http://www.doctrine-project.org/fisheye/ failed: Error in remote call to 'FishEye 0 (http://www.doctrine-project.org/fisheye/)' (http://www.doctrine-project.org/fisheye) [AbstractRestCommand{path='/rest-service-fe/search-v1/crossRepositoryQuery', params={query=DDC-61, expand=changesets[0:20].revisions[0:29],reviews}, methodType=GET}] : Received status code 503 (Service Temporarily Unavailable)

          People

          • Assignee:
            Roman S. Borschel
            Reporter:
            Ville Väänänen
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: