Uploaded image for project: 'Doctrine 2 - ORM'
  1. Doctrine 2 - ORM
  2. DDC-119

Collection change tracking broken with NOTIFY policy

    Details

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

      Description

      Looks like change tracking of collections together with the NOTIFY policy doesnt work well as collection updates are detected in _computeAssociationChanges. Perhaps the collection itself should inform the UnitOfWork directly?

        Activity

        romanb Roman S. Borschel created issue -
        romanb Roman S. Borschel made changes -
        Field Original Value New Value
        Priority Major [ 3 ] Critical [ 2 ]
        Hide
        ksid Kawsar Saiyeed added a comment -

        Not sure if the issue is identical but seems at least related. Using NOTIFY change tracking with many-to-many bidirectional associations does not work. Objects added to the associations do not get persisted when calling EntityManager#flush.

        Tested on r7404.

        Show
        ksid Kawsar Saiyeed added a comment - Not sure if the issue is identical but seems at least related. Using NOTIFY change tracking with many-to-many bidirectional associations does not work. Objects added to the associations do not get persisted when calling EntityManager#flush. Tested on r7404.
        romanb Roman S. Borschel made changes -
        Fix Version/s 2.0-BETA3 [ 10060 ]
        Fix Version/s 2.0 [ 10021 ]
        romanb Roman S. Borschel made changes -
        Status Open [ 1 ] In Progress [ 3 ]
        romanb Roman S. Borschel made changes -
        Status In Progress [ 3 ] Closed [ 6 ]
        Resolution Fixed [ 1 ]
        Hide
        mzach Michael Zach added a comment -

        Dear Roman,

        the line # 456 in UnitOfWork.php seems wrong to me:

                    $isChangeTrackingNotify = isset($this->entityChangeSets[$oid]);
        

        Shouldn't this only be set if the entity has

         @ChangeTrackingPolicy("NOTIFY") *
        

        set in his class docBlock? The current behaviour now is to assign $changeset if changes exists, leaving the NOTIFY tracking policy out:

                     $changeSet = $isChangeTrackingNotify ? $this->entityChangeSets[$oid] : array();
        

        Because of this change, all our unit tests involving saving of entites break (basically, the whole application), which implement @postUpdate for logging purposes logging an own computed changeset.

        Show
        mzach Michael Zach added a comment - Dear Roman, the line # 456 in UnitOfWork.php seems wrong to me: $isChangeTrackingNotify = isset($ this ->entityChangeSets[$oid]); Shouldn't this only be set if the entity has @ChangeTrackingPolicy( "NOTIFY" ) * set in his class docBlock? The current behaviour now is to assign $changeset if changes exists, leaving the NOTIFY tracking policy out: $changeSet = $isChangeTrackingNotify ? $ this ->entityChangeSets[$oid] : array(); Because of this change, all our unit tests involving saving of entites break (basically, the whole application), which implement @postUpdate for logging purposes logging an own computed changeset.
        Hide
        romanb Roman S. Borschel added a comment -

        Hi,

        you're right, I did that naivly because I thought the only case where an entity would already have a changeset on flush is with the NOTIFY policy. I did not think of custom use cases like yours. It is fixed now in master. My apologies. However, this still means your approach would be broken if you would use the NOTIFY policy right? That sounds like maybe there is potential to improve the approach you're currently using. If you're missing anything in the API or implementation that forbids a different approach feel free to raise some new JIRA issues so we can possibly improve the situation.

        Show
        romanb Roman S. Borschel added a comment - Hi, you're right, I did that naivly because I thought the only case where an entity would already have a changeset on flush is with the NOTIFY policy. I did not think of custom use cases like yours. It is fixed now in master. My apologies. However, this still means your approach would be broken if you would use the NOTIFY policy right? That sounds like maybe there is potential to improve the approach you're currently using. If you're missing anything in the API or implementation that forbids a different approach feel free to raise some new JIRA issues so we can possibly improve the situation.
        Hide
        mzach Michael Zach added a comment -

        Thank you for fixing this real quick! Our current approach is to compute the changeset on @preUpdate and after a successful save write the changeset to the database in @postUpdate. This conflicted with the changes made, I will however look into it and see if it's feasible for us to implement another approach.

        Once again, thanks for your reply and fix.

        Show
        mzach Michael Zach added a comment - Thank you for fixing this real quick! Our current approach is to compute the changeset on @preUpdate and after a successful save write the changeset to the database in @postUpdate. This conflicted with the changes made, I will however look into it and see if it's feasible for us to implement another approach. Once again, thanks for your reply and fix.
        beberlei Benjamin Eberlei made changes -
        Workflow jira [ 10349 ] jira-feedback [ 15466 ]
        beberlei Benjamin Eberlei made changes -
        Workflow jira-feedback [ 15466 ] jira-feedback2 [ 17330 ]
        beberlei Benjamin Eberlei made changes -
        Workflow jira-feedback2 [ 17330 ] jira-feedback3 [ 19587 ]

        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={expand=changesets[0:20].revisions[0:29],reviews, query=DDC-119}, methodType=GET}] : Received status code 503 (Service Temporarily Unavailable)

          People

          • Assignee:
            romanb Roman S. Borschel
            Reporter:
            romanb Roman S. Borschel
          • Votes:
            1 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: