orm icon indicating copy to clipboard operation
orm copied to clipboard

Experiment: Use comparator functions from a global registry for changeset detection

Open mpdude opened this issue 4 months ago • 7 comments

This is an experiment towards #5542. Can we have something to do comparison of objects to get away from === checks in changeset computation, but also in Criteria comparisons and sorting (in ClosureExpressionVisitor)?

We cannot expect users to implement interfaces for this in their objects. For example, users might be bringing their own subclasses of DateTime (like Carbon or Chronos libraries) or get their DateTime objects e. g. from clock mocking libraries. So, these objects might be coming from very differnet places and the classes used or created cannot all implement Doctrine interfaces.

Thus, the mechanism for bringing in the comparisons has to be attached or brought to the objects from the outside.

A global-state registry is not nice, but a very simple approach to start the discussion.

In this PR, comparators are registered based on class or interface names. Registration happens in order, and the first one matching an object at hand that returns a result different from null will win.

X-Ref https://github.com/doctrine/collections/pull/454

mpdude avatar Oct 09 '25 12:10 mpdude

As it turns out, the problem is not only figuring out that two different objects may acutally represent the same value.

Even more complicated is the case of detecting changes to mutable objects.

In the UoW, we keep the object (not clones of it) as the previous value, so for mutable objects, we have nothing to compare against.

mpdude avatar Oct 09 '25 13:10 mpdude

  • Comparators are on the hot path of writing data
  • Need not only be able to compare objects of the same class but of different classes as well
  • Order of registration should not be relevant
  • Finding the comparator "closest" to a given class may be O(n) in the number of comparators

mpdude avatar Oct 09 '25 14:10 mpdude

We may get away by emphasizing that we do not support mutable objects and/or that users have to deal with the quirk (as since the beginning) that they need to replace objects themselves.

But, working with immutable objects (like DateTimeImmutable) still may cause new instances of the same value be created, e. g. Symfony Form system binding data, putting in new DateTimeImmutable with new values.

-> In this case we'd still benefit from not considering this a change.

mpdude avatar Oct 09 '25 14:10 mpdude

The case of tracking changes for mutable value objects has never been supported by the ORM ever since 2.0. It always required you to clone the object before mutation and then assign that clone. So this case of mutable value object is a totally different feature than detecting equivalent immutable value objects to avoid over-writing in the flush. And that's probably a feature we should not even attempt to implement, keeping the rule that you need to not mutate the persisted value object (for which we should recommend using immutable ones then).

stof avatar Oct 09 '25 15:10 stof

  • Comparators are on the hot path of writing data

but in the hot path of changeset computation.

beberlei avatar Oct 09 '25 19:10 beberlei

Note to self:

  • Make sure we don't apply comparator logic to fields that are associations (contain entities), only fields that are "values" from the ORM point of view
  • Look into how this applies to embeddables (comparing entire embeddables vs. comparing them at the field level?)

mpdude avatar Oct 10 '25 08:10 mpdude

Another thought:

We may have an entity that has two fields holding e. g. DateTimeImmutable instances. But one field may be configured as a date_immutable, the other one as datetime_immutable.

In these cases, changeset computation and/or collection comparisons need to do different comparisons that cannot be derived from the class of the object alone; the context (which field it belongs to and how that is mapped) is relevant.

mpdude avatar Oct 22 '25 21:10 mpdude