Generated/Virtual Columns: Insertable / Updateable by mehldau · Pull Request #9118 · doctrine/orm

mehldau · 2021-10-12T09:48:07Z

Defines whether a column is included in an SQL INSERT and/or UPDATE statement.

insertable: false - do not include this column as part of INSERT statement for entities of the type.
updatable: false - do not include this column as part of UPDATE statement
generated: "always" - after inserting or updating this entity, always fetch an updated value from the database.
generated: "insert" - after inserting this entity, fetch an updated value from the database.

Use Cases

Map a column several times.

#[Entity] 
class User
{
     #[ManyToOne(targetEntity: Country:class), JoinColumn(name: "country_code", referencedColumnName: "country_code")]
     public $country;
     
     #[Column(type: "string", name: "country_code", insertable: false, updatable: false)]
     public $countryCode;
}

Columns with database updates

#[Entity]
class Article
{
    #[Column(type: "datetime", 
        columnDefinition: "TIMESTAMP DEFAULT CURRENT_TIMESTAMP ON UPDATE CURRENT_TIMESTAMP", 
        insertable: false,
        updatable: false,
        generated: "ALWAYS")]
    public $created;
}

#5728

greg0ire · 2021-10-12T20:36:24Z

Please rebase on 2.11.x and force push, new features should go on that branch now. Also, please add documentation about this (in docs)

beberlei

This is already amazing work and i am happy this looks like it be implemented with just a few lines in BasicEntityPersister.

I made a few suggestions about optimizations and towards more ORM idomatic style.

One thing I checked is weather we should even allow non updateable / insertable columns to show up in a changeset. But I believe this would cause more harm to performance since there is no access to $fieldMapping in changeset computation.

One thing this PR does not address here is auto updated columns in the database. In my opinion if there is a column that is not insertable / updateable, that means we need to fetch them after an insert / update and set them on the entity. The updatable = false should not be used to help with immutability, it means the database updates this value itself (via trigger for example).

lib/Doctrine/ORM/Persisters/Entity/BasicEntityPersister.php

lib/Doctrine/ORM/Mapping/ClassMetadataInfo.php

mehldau · 2021-11-09T16:16:22Z

This is already amazing work and i am happy this looks like it be implemented with just a few lines in BasicEntityPersister.

Thank you @beberlei

I made a few suggestions about optimizations and towards more ORM idomatic style.

I addressed this and your proposal of inverting 'insertable' and 'updateable' reg. optimization of the ClassMetadata cache in the current PR.

One thing this PR does not address here is auto updated columns in the database. In my opinion if there is a column that is not insertable / updateable, that means we need to fetch them after an insert / update and set them on the entity. The updatable = false should not be used to help with immutability, it means the database updates this value itself (via trigger for example).

You're absolutely right. Maybe you could lent me your expertise to find the most suitable hook point to implement this. Maybe remember the necessity of reloading the Entity within BasicEntityPersister::prepareUpdateData() and use this information somewhere within the update / insert - processing? Or could I use the event system either way?

beberlei · 2021-11-20T08:25:57Z

@mehldau the event system should only be used by outside integrations. We hardcode our logic into the persisters somewhere.

There is already precedent for this in the isVersioned property and its code in assignDefaultVersionValue to fetch the database assigned version id. We can generalize this code.

a.) Introduce a new property requiresFetchAfterUpdate that defaults to false. if a versioned field is set or any notUpdateable / notInsertable property exists set this property true.
b.) Refactor assignDefaultVersionValue to work over all not insertable / updateable / versioned columns.

mehldau · 2021-11-24T08:22:05Z

There is already precedent for this in the isVersioned property and its code in assignDefaultVersionValue to fetch the database assigned version id. We can generalize this code.

a.) Introduce a new property requiresFetchAfterUpdate that defaults to false. if a versioned field is set or any notUpdateable / notInsertable property exists set this property true. b.) Refactor assignDefaultVersionValue to work over all not insertable / updateable / versioned columns.

@beberlei All right. I implemented the changes as suggested and updated the pull request.

Nevertheless, I'm wondering whether we have to reload 'non insertable' columns in the edge case of 'updating' an entity which only has 'non insertable' columns defined.

beberlei

Getting very close here :)

A few questions:

Did we miss the notUpdateable now? Not sure what condition/code prevents a field to be updated. Can you point me to it?
Can you add a test for a joined table updateable as well?

lib/Doctrine/ORM/Mapping/ClassMetadataInfo.php

lib/Doctrine/ORM/Persisters/Entity/BasicEntityPersister.php

beberlei · 2021-12-19T00:33:52Z

@mehldau I took the day to improve this PR, catching a nasty edge case with Second Level Cache that we haven't thought of yet.

I removed the exception when updating a field that is not updatable, this value just gets overwritten with the correct one from the DB now. There are still these todos I want to tackle before merging this:

Add a test with joined table inheritance
Add tests for AbstractMappingDriver using Insertable and Updatable entities, will also test xml, attribute and other drivers.
Fetch only the right columns on insert or update, not all of them

beberlei · 2021-12-19T16:32:21Z

One point that i am still struggling with, there are cases where you want to disable insertable or updateable but don't want data to be fetched from the database after each write. For example if you want to use updateable: false for immutable properties, then why fetch them from the database?

Hibernate (but not JPA) differentiates with another option @Generated(time) where time is an enum with the values always or insert. We should copy this behavior instead of magically adding this when just using insertable/updateable.

derrabus · 2021-12-20T15:06:44Z

What is our BC policy regarding BasicEntityPersister? The class is not final and full of protected members. This PR renames some protected methods and changes the signature of others. Do we expect userland code to override that class and specific methods in it? If so, shall we document this PR as a minor BC break?

beberlei · 2021-12-20T22:59:17Z

@derrabus its not possible to override the persisters, their factory method is guarded by final things in EM and UoW.

derrabus · 2021-12-21T06:22:47Z

So we should probably finalize them as well, I guess. For 3.0 at least.🤔

derrabus · 2021-12-21T06:26:14Z

Anyway, this would be out if scope here. Thanks for the explanation.

beberlei · 2022-01-01T15:12:58Z

I believe I want to simplify this to use virtual: true instead of insertable/updatable, because to me the use-cases for disabling only one of insert/update for a column seems to be a more than edgy edge case that we can just choose not to support.

beberlei · 2022-01-01T16:17:52Z

Hm ok, virtual makes no sense in naming either, because that would mean the column does not exist (??) in the schema? Maybe the naming is fine the way it is here.

lib/Doctrine/ORM/Mapping/ClassMetadataInfo.php

lib/Doctrine/ORM/Mapping/Column.php

lib/Doctrine/ORM/Mapping/Driver/AnnotationDriver.php

lib/Doctrine/ORM/Mapping/ClassMetadataInfo.php

Defines whether a column is included in an SQL INSERT and/or UPDATE statement. Throws an exception for UPDATE statements attempting to update this field/column. Closes doctrine#5728

docs/en/reference/attributes-reference.rst

lib/Doctrine/ORM/Mapping/Builder/FieldBuilder.php

lib/Doctrine/ORM/Mapping/ClassMetadataInfo.php

lib/Doctrine/ORM/Persisters/Entity/BasicEntityPersister.php

Co-authored-by: Grégoire Paris <postmaster@greg0ire.fr>

beberlei · 2022-01-12T07:06:24Z

@mehldau Thank you very much for your work on this important feature!

* 2.11.x: Add errors caused by the lexer update to the baselines (doctrine#9360) Generated/Virtual Columns: Insertable / Updateable (doctrine#9118) Remove the composer/package-versions-deprecated package Relax assertion to include null as possible outcome (doctrine#9355)

* 2.11.x: Use EntityManagerInterface in type declarations (doctrine#9325) Add errors caused by the lexer update to the baselines (doctrine#9360) Generated/Virtual Columns: Insertable / Updateable (doctrine#9118) Remove the composer/package-versions-deprecated package Relax assertion to include null as possible outcome (doctrine#9355)

greg0ire changed the base branch from 2.10.x to 2.11.x October 12, 2021 20:35

mehldau force-pushed the upsertable branch 2 times, most recently from 5cb323e to 22e7692 Compare October 19, 2021 10:02

beberlei added this to the 2.11.0 milestone Nov 7, 2021

beberlei requested changes Nov 7, 2021

View reviewed changes

mehldau force-pushed the upsertable branch from 22e7692 to 19db8e4 Compare November 9, 2021 16:00

mehldau force-pushed the upsertable branch from 19db8e4 to d105a94 Compare November 24, 2021 08:19

beberlei requested changes Dec 12, 2021

View reviewed changes

lib/Doctrine/ORM/Mapping/ClassMetadataInfo.php Outdated Show resolved Hide resolved

lib/Doctrine/ORM/Persisters/Entity/BasicEntityPersister.php Show resolved Hide resolved

lib/Doctrine/ORM/Persisters/Entity/BasicEntityPersister.php Outdated Show resolved Hide resolved

beberlei requested a review from greg0ire December 19, 2021 00:34

beberlei force-pushed the upsertable branch from 61480f3 to 9674f31 Compare December 19, 2021 17:08

derrabus reviewed Jan 8, 2022

View reviewed changes

greg0ire force-pushed the upsertable branch 2 times, most recently from 540552f to 717acaa Compare January 9, 2022 15:50

greg0ire reviewed Jan 9, 2022

View reviewed changes

lib/Doctrine/ORM/Mapping/ClassMetadataInfo.php Outdated Show resolved Hide resolved

Generated/Virtual Columns: Insertable / Updateable

8b666a3

Defines whether a column is included in an SQL INSERT and/or UPDATE statement. Throws an exception for UPDATE statements attempting to update this field/column. Closes doctrine#5728

greg0ire force-pushed the upsertable branch from 717acaa to 8b666a3 Compare January 9, 2022 15:53

greg0ire reviewed Jan 10, 2022

View reviewed changes

docs/en/reference/attributes-reference.rst Show resolved Hide resolved

greg0ire reviewed Jan 10, 2022

View reviewed changes

beberlei and others added 2 commits January 10, 2022 19:53

Apply suggestions from code review

128c54d

Co-authored-by: Grégoire Paris <postmaster@greg0ire.fr>

Add example for virtual column usage in attributes to docs.

9947189

This was referenced Jan 10, 2022

Add support for updatable columns #6673

Closed

Add support for insertable columns #6671

Closed

greg0ire approved these changes Jan 10, 2022

View reviewed changes

derrabus approved these changes Jan 10, 2022

View reviewed changes

SenseException approved these changes Jan 10, 2022

View reviewed changes

beberlei approved these changes Jan 12, 2022

View reviewed changes

beberlei merged commit e369cb6 into doctrine:2.11.x Jan 12, 2022

github-actions bot mentioned this pull request Jan 12, 2022

Merge release 2.11.0 into 3.0.x #9369

Merged

beberlei mentioned this pull request Feb 20, 2022

Using nonInsertable/nonUpdateable columns with JOINED inheritance type triggers SQL error #9467

Closed

mrVrAlex mentioned this pull request May 12, 2022

Generated columns not implemented in JoinedSubclassPersister? #9759

Closed

tgauges mentioned this pull request Jul 19, 2022

Implement Insertable / Updateable for ManyToOne and OneToOne #9916

Open

greg0ire mentioned this pull request Nov 13, 2022

Widen parameter type #10229

Merged

mrVrAlex mentioned this pull request Jun 5, 2023

Support not Insertable/Updateable columns for entities with JOINED inheritance type #10598

Merged

alexander-schranz mentioned this pull request Sep 26, 2024

Add missing generated option to documentation #11613

Merged

Conversation

mehldau commented Oct 12, 2021 • edited by beberlei Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Use Cases

Map a column several times.

Columns with database updates

Uh oh!

greg0ire commented Oct 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

beberlei left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mehldau commented Nov 9, 2021

Uh oh!

beberlei commented Nov 20, 2021

Uh oh!

mehldau commented Nov 24, 2021

Uh oh!

beberlei left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

beberlei commented Dec 19, 2021

Uh oh!

beberlei commented Dec 19, 2021

Uh oh!

derrabus commented Dec 20, 2021

Uh oh!

beberlei commented Dec 20, 2021

Uh oh!

derrabus commented Dec 21, 2021

Uh oh!

derrabus commented Dec 21, 2021

Uh oh!

beberlei commented Jan 1, 2022

Uh oh!

beberlei commented Jan 1, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

beberlei commented Jan 12, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

mehldau commented Oct 12, 2021 •

edited by beberlei

Loading

greg0ire commented Oct 12, 2021 •

edited

Loading