Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Bidirectional Mapping with multiple targetDocuments not automatically updated on flush #442

Closed
alexcasalboni opened this Issue Nov 6, 2012 · 12 comments

Comments

Projects
None yet
4 participants

I've reproduced my situation here: I need a bidirectional mapping between a document (let's say, a Person) and multiple documents (let's say Car and Computer) as a ReferenceMany field called $items, therefore I cannot specify the targetDocument and I should have simple=false in order to keep the collection reference.
Person is mapped as owner in the other documents (with simple=true, inversedBy="items", etc).
I need to keep track of the items of a Person and in general let's say that the owner of an item could change quite often, so I'd like to call setOwner($person2) and dynamically remove the item from the old Person's items and add it to the new one's.

I've both addItems and removeItems methods, that are used inside the setOwner methods of the different items and correctly update the objects status.

Here's the problem: this mechanism could be realized through a form submission and in my specific case I don't want to manually persist the 3 involved objects (since the controller belongs an external repo), so I'd like to persist only the modified object (the item) and automatically update both the new and the old associated person.

If I only persist the item what I get is a consistency problem, since the old Person is not updated and still has the binded item, while the item and the new Person are correctly updated.
Another problem is that the new Person is configured with a PreUpdate method (in order to update other fields, such as lastUpdateTime, etc), that is not triggered before the flush.

In order to let the configuration without targetDocument work, I had to remove the mappedBy attribute from the ReferenceMany relationship: could it be a cause? (I did it because If I keep mappedBy="owner" the collection is not stored/updated at all!)

Thank you

@jmikola please read this issue, it's a blocking bug for us, thank's.

Owner

jmikola commented Nov 7, 2012

Based on your Person model, you're using DEFERRED_EXPLICIT as the change-tracking policy:

The deferred explicit policy is similar to the deferred implicit policy in that it detects changes through a property-by-property comparison at commit time. The difference is that only documents are considered that have been explicitly marked for change detection through a call to DocumentManager#persist(document) or through a save cascade. All other documents are skipped.
[source]

None of the cascades in your models include save, and you mentioned that you're only persisting the modified object (Car or Computer in your example). In that case, I wouldn't expect any of the Person documents to be updated at all. I'm not sure why the Person being assigned the item is updated but not the original Person.

If the preUpdate event is not triggered, that means that UnitOfWork::commit() has no updates to perform. Otherwise, it would call UnitOfWork::executeUpdates(), which in turn dispatches the event. You can trace through execution of commit() to see what's going on. executeUpdates() happens well after document changesets have been calculated, so if the Person object isn't being modified (for reasons discussed above), the preUpdate() lifecycle callback will never be invoked.

In order to let the configuration without targetDocument work, I had to remove the mappedBy attribute from the ReferenceMany relationship: could it be a cause? (I did it because If I keep mappedBy="owner" the collection is not stored/updated at all!)

In my experience, I've only used flexible references like this when all possible referenced classes were inheriting some abstract mapping that had a discriminator field. In those cases, I mapped the reference with only a targetDocument and ODM handled the discrimination on the reference automatically. I don't have thorough knowledge of the reference internals, but I'd think that mappedBy would need a targetDocument to infer the field on the referenced document's side.

@jmikola thank's for your reply. Following your suggests, Alex is refactoring the code right now to check if it works.

About the 'targetDocument' problem, I red here: http://docs.doctrine-project.org/projects/doctrine-mongodb-odm/en/latest/reference/reference-mapping.html#mixing-document-types that mix document types is possible, isn't it?

Thank you for your help, @jmikola

I've set DEFERRED_IMPLICIT and created an abstract MappedSuperclass BindableItem, so that I can use it as targetDocument in the ReferenceMany.

You can check it in this new branch.
Here are my questions:

  • why doesn't mongodb store the binded items if I use mappedBy="owner"?
  • why isn't the discriminator field used anymore? isn't it needed anymore because of targetDocument?

In the test action now I'm getting this error, during the collection loading:

Undefined index: in /home/alex/domains/wetalkcms.it/wtms/vendor/doctrine/mongodb-odm/lib/Doctrine/ODM/MongoDB/Mapping/ClassMetadataInfo.php line 1263 

caused by

$idType = $this->fieldMappings[$this->identifier]['type'];

Is it looking for the abstract-document identifier? It's defined in the subclasses, but nothing changes if I define it in the superclass (anyway, I also have the need of different identifiers for the subclasses!).

Thanks

Owner

jmikola commented Nov 7, 2012

About the 'targetDocument' problem, I redd here: http://docs.doctrine-project.org/projects/doctrine-mongodb-odm/en/latest/reference/reference-mapping.html#mixing-document-types that mix document types is possible, isn't it?

I never made use of _doctrine_class_name. At Exercise.com, we had a document using single collection inheritance. There were two classes, the base class and an inheriting class, and both were concrete (not abstract). The base class had a discriminator field and map defined for both classes. The maps defined the class name by discriminator string.

References to these classes used the base class as the target document and ODM handled storing a discriminator in the raw object representation corresponding to the MongoDBRef class. Obviously, MongoDBRef has no knowledge of the discriminator field, or _doctrine_class_name, so that's something ODM read on its own.

Owner

jmikola commented Nov 7, 2012

I've set DEFERRED_IMPLICIT and created an abstract MappedSuperclass BindableItem, so that I can use it as targetDocument in the ReferenceMany.

DEFERRED_IMPLICIT is the default, so you shouldn't have to specify it.

why isn't the discriminator field used anymore? isn't it needed anymore because of targetDocument?

I don't think you need to have targetDocument="BindableItem", in the reference mapping on the Person class. Since you're not using single collection inheritance, you do need to specify the discriminator map, though.

why doesn't mongodb store the binded items if I use mappedBy="owner"?

I don't follow the question. Can you point me towards a code example where this is failing? I believe this had to do entirely with cascading and the change tracking per document class.

In the test action now I'm getting this error, during the collection loading

This may be due to you specifying an abstract class as targetDocument. It would be helpful if you debugged $this->filedMappings and the value of $this->identifier.

Ok @jmikola I have managed to have everything updated within a single persist operation on the item object (using no targetDocument with distriminatorMap and an abstract MappedSuperclass).
For some reason, if I define mappedBy="owner" nothing works; and, anyway, nothing changes if I don't define inversedBy="items".

By the way, now everything is well persisted but here are some new problems/questions:

  • the preUpdate event is triggered only on the new Person object (even if the old one is correctly updated!) -> how is it possible that it updates and stores the object without calling the lifecycle event?
  • I've successfully moved the MappedSuperclass annotation to the interface (IBindableItem), so that I can use other classes that cannot extend BindableItem (since we don't have multiple inheritance!), but the field mapping still has to be included in the abstract class (or the implementing class!) -> isn't there a way to put everything in the interface? (something like method-mapping, I know it can be done in Java, for example)

Do you think we can do something for the preUpdate method invocation on the old Person object? (besides calling it manually in the BindableItem setter!).

Thank you for your help

Owner

jmikola commented Nov 8, 2012

the preUpdate event is triggered only on the new Person object (even if the old one is correctly updated!) -> how is it possible that it updates and stores the object without calling the lifecycle event?

Are you talking about a lifecycle callback on the model class or a lifecycle event listener service? You should be able to see what's going on my looking at the dispatch behavior in UnitOfWork::executeUpdates(). $hasPreUpdateLifecycleCallbacks and $hasPreUpdateListeners are determined by the class, and all documents of a class are processed at once. If a preUpdate event isn't being triggered for a document, then it must not be modified during executeUpdates().

isn't there a way to put everything in the interface?

ODM will only read mapping information from abstract and concrete classes. Interfaces aren't supported for the very reason that multiple inheritance could lead to ambiguity.

I'm talking about a lifecycle callback on the model class. I've checked the UnitOfWork::executeUpdates() method: the problem is that I don't have any Person object inside $this->documentUpdates.
It seems that a document is not marked as updated if a referenced (or embedded) property has changed (since collections are updated using $this->collectionUpdates). That's why I have everything updated in the db but no lifecycle events triggered on the Person objects.

As a simple workaround for this, I could just modify some internal properties (such as the updateTime field) inside the adder and the remover, so that the UnitOfWork can detect the Person documents as changed.

So now the question becomes: why doesn't the UnitOfWork logic mark documents as updated in case of updated collection(s)?
It's an important issue for our logic, since a field has been updated and we need to have the preUpdate event triggered.

I saw that this mechanism is already implemented for embedded fields, while it's disabled for referenced fields (see here and here).

Thank you again, @jmikola

Member

malarzm commented Jan 4, 2016

So now the question becomes: why doesn't the UnitOfWork logic mark documents as updated in case of updated collection(s)?

In case anyone would get here this was fixed in 4ab0fbc

@malarzm malarzm closed this Jan 4, 2016

Thank you @malarzm.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment