doctrine / orm

Doctrine Object Relational Mapper (ORM)
https://www.doctrine-project.org/projects/orm.html
MIT License
9.93k stars 2.52k forks source link

Support for immutable embeddable entities #7539

Open alekitto opened 5 years ago

alekitto commented 5 years ago

Feature Request

Add support for immutable embeddable entities for VOs that shouldn't be modified by UoW operations.

Q A
New Feature yes
RFC yes
BC Break no

Summary

I'm using Money class from moneyphp package as an embeddable entity. Money instances are immutable as every method call will not change its internal state, but will return a new instance of the same class instead.

Now, if a record is modified and a refresh operation is called on the main entity object, the internal state of the embedded entity is changed potentially causing bugs if the application rely on the immutability of the VO. In my case, the record can be modified by a concurrent operation and the Money object must be refreshed changing its internal state and making old references to that object unreliable.

My proposal is to add an immutable property to the embeddable mapping signaling the unit of work to create a new instance of the object if one of the property has changed its value.

Ocramius commented 5 years ago

Embeddable objects are not tracked: if you want them to be treated as immutable, use @Entity(readOnly=true) in the entity that uses the embeddable type.

alekitto commented 5 years ago

In my case all the entities embedding the value object are mutable, only the Money objects are immutable, so marking all with readOnly=true is not an option. I'm now calling a clone on every money getter, but i'm wondering if the orm layer could handle this case

Ocramius commented 5 years ago

The ORM only synchronises in-memory and DB state: designing your objects as immutable is really up to you, not to the ORM

alekitto commented 5 years ago

I disagree, the ORM should help to write better code abstracting the problem of syncing the database data with the application objects. I can design and implement my objects as immutable as I could, but now I should fight against the ORM because the immutability is broken by the operation executed by the ORM itself that changes the internal state of the objects. Also, the read only flag on the main entity doesn't help resolve the issue as the refresh operation in the unit of work does not check it at all.

If this is the intended behaviour it's ok, but should be documented clearly that doctrine is NOT suitable for immutable objects as the internal operations could change the internal state of them making the immutability implementation unreliable in certain situations.

Ocramius commented 5 years ago

No, the ORM doesn't help with designing: it can actually get in the way of proper object design, in many cases.

The ORM maps data from one domain (yours, designed by you) and the database.

How your domain is designed (mutable, immutable, spooky-action-at-a-distance-as-core, etc) is entirely up to you, within the boundaries of what the ORM can map.

The tool will limit itself to copying your memory into the UnitOfWork and writing it to DB when changes are detected. How those changes happen is absolutely not the business of the ORM, and should be designed, tested and maintained within your business domain, not in the tool, to the point where the tool should be entirely unknown to your domain.

the immutability is broken by the operation executed by the ORM itself that changes the internal state of the objects.

This can happen when refreshing memory information from DB state (expected: we are resetting state), but other scenarios where this may happen are bugs: please report a bug with a test case, if you found something else that changes in-memory state in an unexpected way.

Ocramius commented 5 years ago

Let me be more specific: refresh() will change deep state of your objects, and there is nothing that can be done about it, because this is by design, and re-creating embedded instances leads to even more accidental complexity. My suggestion is to not refresh(), but rather clear() and reload.

Ocramius commented 5 years ago

BTW, I agree with you on immutability, I just don't think we can change @Embeddable and refresh() semantics. We'd probably need a separate concept, such as @Value, or @Atom in order to define elements that are supposed to be fully replaced: this is a good idea for ORM v3 or later.

alekitto commented 5 years ago

Ok, I see your point. If you don’t mind I’ll try to implement a PoC for @Atom in the next days.

Ocramius commented 5 years ago

Give it a try, let us know 👌