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

e-vil-dev · 2023-03-27T19:16:10Z

Fix for #9467 when insert and update entity with joined inheritance + update values from database

Entities with #[InheritanceType('JOINED')] attribute process by JoinedSubclassPersister, according to UnitOfWork::getEntityPersister():

orm/lib/Doctrine/ORM/UnitOfWork.php

Lines 3227 to 3228 in da9b9de

case $class->isInheritanceTypeJoined():

$persister = new JoinedSubclassPersister($this->em, $class);

The persister override getInsertColumnList() without excluding notInsertable columns like parent do:

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

Line 1478 in da9b9de

if (isset($this->class->fieldMappings[$name]['notInsertable'])) {

so when it use prepared sql-statement with parameters prepared by parent (AbstractEntityInheritancePersister::prepareInsertData() reuse BasicEntityPersister::prepareInsertData() reuse BasicEntityPersister::prepareUpdateData() with $isInsert mode):

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

Lines 640 to 659 in da9b9de

    
           foreach ($uow->getEntityChangeSet($entity) as $field => $change) { 
        
               if (isset($versionField) && $versionField === $field) { 
        
                   continue; 
        
               } 
        
               if (isset($this->class->embeddedClasses[$field])) { 
        
                   continue; 
        
               } 
        
               $newVal = $change[1]; 
        
               if (! isset($this->class->associationMappings[$field])) { 
        
                   $fieldMapping = $this->class->fieldMappings[$field]; 
        
                   $columnName   = $fieldMapping['columnName']; 
        
                   if (! $isInsert && isset($fieldMapping['notUpdatable'])) { 
        
                       continue; 
        
                   } 
        
                   if ($isInsert && isset($fieldMapping['notInsertable'])) {

we get error from database, e.g. postgresql:

DOException: SQLSTATE[08P01]: <<Unknown error>>: 7 ERROR:  bind message supplies 4 parameters, but prepared statement "" requires 6

So fixed JoinedSubclassPersister::getInsertColumnList(). There is no such error for update queries, because parent methods used in JoinedSubclassPersister::update(): BasicEntityPersister::prepareUpdateData() and BasicEntityPersister::updateTable().

There were problems with logic to update entity properties with database values in generated columns (notInsertable|notUpdatable|version) after flush - JoinedSubclassPersister::assignDefaultVersionAndUpsertableValues():

orm/lib/Doctrine/ORM/Persisters/Entity/JoinedSubclassPersister.php

Lines 548 to 555 in da9b9de

    
           protected function assignDefaultVersionAndUpsertableValues($entity, array $id) 
        
           { 
        
               $values = $this->fetchVersionAndNotUpsertableValues($this->getVersionedClassMetadata(), $id); 
        
               foreach ($values as $field => $value) { 
        
                   $value = Type::getType($this->class->fieldMappings[$field]['type'])->convertToPHPValue($value, $this->platform); 
        
                   $this->class->setFieldValue($entity, $field, $value);

flag which activates it (ClassMetadataInfo::$requiresFetchAfterChange) in both cases (executeInserts() and update()) doesnt inherited from entity parent-class. This fixed by tracking of parent mappings in ClassMetadataInfo::addInheritedFieldMapping() like it done for child entity in mapField() (may be add setter for factory):

orm/lib/Doctrine/ORM/Mapping/ClassMetadataInfo.php

Lines 2814 to 2820 in da9b9de

    
           public function mapField(array $mapping) 
        
           { 
        
               $mapping = $this->validateAndCompleteFieldMapping($mapping); 
        
               $this->assertFieldNotMapped($mapping['fieldName']); 
        
               if (isset($mapping['generated'])) { 
        
                   $this->requiresFetchAfterChange = true;

it use parent select query to get generated columns, while it could be in both tables (child and parent), so override query builder in persister - JoinedSubclassPersister::fetchVersionAndNotUpsertableValues() - where added join (reuse JoinedSubclassPersister::getJoinSql()) and column aliases. Make parent extractIdentifierTypes() and $identifierFlattener make them protected.
because generated column may be in child tables logic should be after insert into child tables in persister insert() method to avoid LengthException('Unexpected empty result for database query.').

For functional tests added JoinedInheritanceRoot entity, with children for each case and JoinedInheritanceChild to test root entity columns. Declare database default column values to avoid platform-specific columnDefinition. Use generated: 'ALWAYS' in hope to obtain error from update() method

mrVrAlex · 2023-03-27T23:34:00Z

P.S. This is replacement for PR #9486 but with functional tests and covered more problem of combination cases (like nonInsertable column in parent entity) which not worked even after apply fixes from previous PR.

mrVrAlex · 2023-03-30T13:34:45Z

@greg0ire Can you approve workflow runs checks on this?
Also maybe need choose 2.15.x as base branch for this, or patch release it is ok?

greg0ire · 2023-03-30T13:51:35Z

Based on the title, it's not really a fix, but a new feature, so I'd say 2.15.x

mrVrAlex · 2023-03-31T08:10:48Z

@greg0ire Done. Please approve CI again (also failed tests for php7.2 was fixed).
I think it's ready for review now)

greg0ire · 2023-03-31T11:12:50Z

@mehldau please review. Also please advise on whether this is a patch or an improvement. When reading the description, I'm now on the verge of considering this as a fix, since it's a special case of something supposed to work, and not something extra you did not do deliberately.

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

mrVrAlex · 2023-05-10T11:59:29Z

@greg0ire As I understand we wait @mehldau decision?
Just until we wait - 2.15 was already released)

greg0ire · 2023-05-11T06:19:16Z

@mehldau doesn't seem to respond, so let's not wait for them.

tests/Doctrine/Tests/Models/Upsertable/JoinedInheritanceRoot.php

tests/Doctrine/Tests/Models/Upsertable/JoinedInheritanceNonUpdatableColumn.php

greg0ire · 2023-05-15T08:46:09Z

Please address the coding standard issues.

greg0ire · 2023-05-15T09:30:38Z

Please address the coding standard issues.

I think they are not caused by you, I'll handle them separately.

greg0ire · 2023-05-15T09:53:38Z

Fixed. You just need to rebase.

mrVrAlex · 2023-05-15T10:47:43Z

Fixed. You just need to rebase.

Rebased.

greg0ire · 2023-05-15T12:14:28Z

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

@@ -556,6 +560,60 @@ protected function assignDefaultVersionAndUpsertableValues($entity, array $id)
        }
    }

+    /**
+     * {@inheritdoc}


Suggested change

* {@inheritdoc}

* {@inheritDoc}

SenseException

Sometimes you use clear and find in the tests and sometimes you don't. Could you please use them all the same in the test methods?

SenseException · 2023-05-23T21:55:37Z

tests/Doctrine/Tests/ORM/Functional/Ticket/GH9467/GH9467Test.php

+        $this->_em->persist($entity);
+        $this->_em->flush();
+
+        // check fetch generated values override prefilled for notUpdatable


Suggested change

// check fetch generated values override prefilled for notUpdatable

$this->_em->clear();

$entity = $this->_em->find(JoinedInheritanceChild::class, $entity->id);

// check fetch generated values override prefilled for notUpdatable

SenseException · 2023-05-23T22:03:44Z

tests/Doctrine/Tests/ORM/Functional/Ticket/GH9467/GH9467Test.php

+        $this->_em->persist($entity);
+        $this->_em->flush();
+
+        // check no any changes for writable value


"Check for no changes for ..."?

SenseException · 2023-05-23T22:04:49Z

tests/Doctrine/Tests/ORM/Functional/Ticket/GH9467/GH9467Test.php

+        $this->_em->clear();
+        $cleanEntity = $this->_em->find(JoinedInheritanceNonWritableColumn::class, $entity->id);
+        self::assertInstanceOf(JoinedInheritanceNonWritableColumn::class, $cleanEntity);
+        self::assertEquals('dbDefault', $cleanEntity->nonWritableContent);


But rootField should have changed, right?

Yes. I'll add this check, just tried to keep the test methods short

e-vil-dev · 2023-05-26T15:32:03Z

Sometimes you use clear and find in the tests and sometimes you don't. Could you please use them all the same in the test methods?

Ok. Testing before clear checks that flush may fetch not Insertable/Updatable properties, and after clear + find that these values are indeed from the database. Also to test update query results, I skip clear to avoid find by reuse just inserted entity.
I'll try improve readability of unit-test

mrVrAlex · 2023-06-03T15:30:05Z

So guys, what's next step on this?
BTW: We use these fixes on production (big) project already more then 2 months without problem.
And maybe after merge it we will report (and fix) the next issue with unnecessary UPDATE xx SET id = yyy WHERE id = yyy queries in some such cases (with joined inheritance).

mrVrAlex

@SenseException @derrabus @greg0ire
Maybe already need rebase this to 2.16.x branch? What you think (how long you will hold it)?

greg0ire · 2023-06-05T11:23:08Z

If it is not a bugfix then yes, it should be rebased on 2.16.x. I don't understand your second question.

mrVrAlex · 2023-06-05T12:00:30Z

I'm not sure what it is: bugfix or improvement (as you mention it 2 month ago about it).
I know one thing: generated columns not works (since this feature was appear in v2.11) with JOINED inheritance type.
We ready to move it to any release. But IMHO: developers expect (#9467) what virtual / generated columns should work with any exist features (entity type) in doctrine. When we test fixes in previous PR (#9486) we discovered what not all cases covered by it, so we re-writed replacement solution (and it worked on our projects).
Now just need hard maintainer decision where it should merge

greg0ire · 2023-06-05T12:08:38Z

@SenseException please give this another review.

mrVrAlex · 2023-06-26T11:13:49Z

@SenseException @greg0ire so what you decided with it, remain 2.15.x branch (release in next v2.15.4?) or should rebase to 2.16 branch?
Let's go finally somewhere and will continue thinking further)

SenseException · 2023-07-06T22:59:07Z

Based on the title, it's not really a fix

@greg0ire @mrVrAlex The description in #9467 sounds more like a feature so, without taking a deeper look, I'd say rebase to 2.16. Otherwise this looks good to me.

…inheritance type 1. Postgres gives error when insert root entity ($rootTableStmt->executeStatement()) in JoinedSubclassPersister::executeInserts(): PDOException: SQLSTATE[08P01]: <<Unknown error>>: 7 ERROR: bind message supplies 4 parameters, but prepared statement "" requires 6 so exclude notInsertable columns from JoinedSubclassPersister::getInsertColumnList() like it done in parent::prepareInsertData() which call BasicEntityPersister::prepareUpdateData(isInsert: true) where we have condition: if ($isInsert && isset($fieldMapping['notInsertable'])) 2. Try to get generated (notInsertable|notUpdatable) column value on flush() with JoinedSubclassPersister::executeInserts() also fails: Unexpected empty result for database query. because method it calls $this->assignDefaultVersionAndUpsertableValues() after insert root entity row, while generated columns in child-entity table, so move call just after insert child row 3. Use option['default'] = 'dbDefault' in functional test entities, to emulate generated value on insert, but declare as generated = 'ALWAYS' for tests purpose (correctness of JoinedSubclassPersister::fetchVersionAndNotUpsertableValues() sql-query) 4. Use JoinedInheritanceRoot::rootField to skip JoinedSubclassPersister::update() optimization for empty changeset in updatable:false columns tests

…d inheritance type 1. Inherit ClassMetadataInfo::requiresFetchAfterChange flag from root entity when process parent columns mapping (see ClassMetadataInfo::addInheritedFieldMapping(), it uses same condition as ClassMetadataInfo::mapField()) so JoinedSubclassPersister::assignDefaultVersionAndUpsertableValues() to be called in JoinedSubclassPersister::executeInserts(). 2. Override JoinedSubclassPersister::fetchVersionAndNotUpsertableValues() to fetch all parent tables (see $this->getJoinSql() call) generated columns. So make protected BasicEntityPersister::identifierFlattener stateless service (use it flattenIdentifier() method) and BasicEntityPersister::extractIdentifierTypes() (to avoid copy-paste). 3. JoinedSubclassPersister::fetchVersionAndNotUpsertableValues() doesnt check empty $columnNames because it would be an error if ClassMetadataInfo::requiresFetchAfterChange is true while no generated columns in inheritance hierarchy. 4. Initialize JoinedInheritanceRoot not-nullable string properties with insertable=false attribute to avoid attempt to insert default null data which cause error: PDOException: SQLSTATE[23502]: Not null violation: 7 ERROR: null value in column "rootwritablecontent" of relation "joined_inheritance_root" violates not-null constraint DETAIL: Failing row contains (5, null, dbDefault, dbDefault, , nonUpdatable). while $rootTableStmt->executeStatement() because JoinedSubclassPersister::getInsertColumnList() have no $insertData (prepared later) to decide is generated column provided by client code or not (so need to skip column)

Co-authored-by: Grégoire Paris <[email protected]>

e-vil-dev · 2023-07-11T08:45:52Z

@SenseException rebased to 2.16

greg0ire · 2023-07-11T21:48:35Z

Thanks everyone!

rcadhikari · 2024-07-01T09:33:50Z

I had similar issue, however setting the insertable & updatable to false at ORM\Column level helped me to resolve the issue.

for example.

/**
     * My Column desc
     *
     * @var integer|null
     * @ORM\Column(
     *     name="your_column_name",
     *     type="integer",
     *     length=12,
     *     nullable=true,
     *     insertable=false,
     *     updatable=false,
     * )
     */
    protected $yourColumnName;

e-vil-dev force-pushed the fix-generated-for-joined-inheritance branch from c9bf2a7 to 134e175 Compare March 31, 2023 07:55

e-vil-dev changed the base branch from 2.14.x to 2.15.x March 31, 2023 07:56

greg0ire reviewed Mar 31, 2023

View reviewed changes

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

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

greg0ire requested review from SenseException and derrabus May 11, 2023 06:19

greg0ire reviewed May 11, 2023

View reviewed changes

tests/Doctrine/Tests/Models/Upsertable/JoinedInheritanceRoot.php Outdated Show resolved Hide resolved

greg0ire reviewed May 11, 2023

View reviewed changes

tests/Doctrine/Tests/Models/Upsertable/JoinedInheritanceNonUpdatableColumn.php Outdated Show resolved Hide resolved

e-vil-dev force-pushed the fix-generated-for-joined-inheritance branch 2 times, most recently from 31354d5 to 2c9f10f Compare May 15, 2023 06:19

e-vil-dev force-pushed the fix-generated-for-joined-inheritance branch from 2c9f10f to 5b264de Compare May 15, 2023 10:25

greg0ire reviewed May 15, 2023

View reviewed changes

e-vil-dev force-pushed the fix-generated-for-joined-inheritance branch from 5b264de to 623af8f Compare May 15, 2023 12:24

SenseException requested changes May 23, 2023

View reviewed changes

e-vil-dev force-pushed the fix-generated-for-joined-inheritance branch from 623af8f to 7fb0634 Compare May 26, 2023 15:55

e-vil-dev requested a review from SenseException May 26, 2023 15:57

mrVrAlex approved these changes Jun 5, 2023

View reviewed changes

e-vil-dev and others added 4 commits July 11, 2023 09:38

Add final to protected

fa5c37e

Co-authored-by: Grégoire Paris <[email protected]>

mistake on final property

3b3056f

e-vil-dev force-pushed the fix-generated-for-joined-inheritance branch from 7fb0634 to 3b3056f Compare July 11, 2023 08:36

e-vil-dev changed the base branch from 2.15.x to 2.16.x July 11, 2023 08:37

SenseException approved these changes Jul 11, 2023

View reviewed changes

SenseException requested a review from greg0ire July 11, 2023 20:24

greg0ire merged commit b5987ad into doctrine:2.16.x Jul 11, 2023

greg0ire added this to the 2.16.0 milestone Jul 11, 2023

github-actions bot mentioned this pull request Aug 1, 2023

Merge release 2.16.0 into 3.0.x #10861

Closed

This was referenced Aug 22, 2023

JoinedSubclassPersister fix nonInsertable columns with JOINED inheritance type + test #9486

Closed

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

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

e-vil-dev commented Mar 27, 2023

mrVrAlex commented Mar 27, 2023

mrVrAlex commented Mar 30, 2023 •

edited

Loading

greg0ire commented Mar 30, 2023

mrVrAlex commented Mar 31, 2023 •

edited

Loading

greg0ire commented Mar 31, 2023

mrVrAlex commented May 10, 2023

greg0ire commented May 11, 2023

greg0ire commented May 15, 2023

greg0ire commented May 15, 2023

greg0ire commented May 15, 2023

mrVrAlex commented May 15, 2023

greg0ire May 15, 2023

SenseException left a comment

SenseException May 23, 2023

SenseException May 23, 2023

SenseException May 23, 2023

e-vil-dev May 26, 2023

e-vil-dev commented May 26, 2023

mrVrAlex commented Jun 3, 2023 •

edited

Loading

mrVrAlex left a comment

greg0ire commented Jun 5, 2023

mrVrAlex commented Jun 5, 2023

greg0ire commented Jun 5, 2023

mrVrAlex commented Jun 26, 2023

SenseException commented Jul 6, 2023

e-vil-dev commented Jul 11, 2023

greg0ire commented Jul 11, 2023

rcadhikari commented Jul 1, 2024

	case $class->isInheritanceTypeJoined():
	$persister = new JoinedSubclassPersister($this->em, $class);

	foreach ($uow->getEntityChangeSet($entity) as $field => $change) {
	if (isset($versionField) && $versionField === $field) {
	continue;
	}

	if (isset($this->class->embeddedClasses[$field])) {
	continue;
	}

	$newVal = $change[1];

	if (! isset($this->class->associationMappings[$field])) {
	$fieldMapping = $this->class->fieldMappings[$field];
	$columnName = $fieldMapping['columnName'];

	if (! $isInsert && isset($fieldMapping['notUpdatable'])) {
	continue;
	}

	if ($isInsert && isset($fieldMapping['notInsertable'])) {

	protected function assignDefaultVersionAndUpsertableValues($entity, array $id)
	{
	$values = $this->fetchVersionAndNotUpsertableValues($this->getVersionedClassMetadata(), $id);

	foreach ($values as $field => $value) {
	$value = Type::getType($this->class->fieldMappings[$field]['type'])->convertToPHPValue($value, $this->platform);

	$this->class->setFieldValue($entity, $field, $value);

	public function mapField(array $mapping)
	{
	$mapping = $this->validateAndCompleteFieldMapping($mapping);
	$this->assertFieldNotMapped($mapping['fieldName']);

	if (isset($mapping['generated'])) {
	$this->requiresFetchAfterChange = true;

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

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

Conversation

e-vil-dev commented Mar 27, 2023

mrVrAlex commented Mar 27, 2023

mrVrAlex commented Mar 30, 2023 • edited Loading

greg0ire commented Mar 30, 2023

mrVrAlex commented Mar 31, 2023 • edited Loading

greg0ire commented Mar 31, 2023

mrVrAlex commented May 10, 2023

greg0ire commented May 11, 2023

greg0ire commented May 15, 2023

greg0ire commented May 15, 2023

greg0ire commented May 15, 2023

mrVrAlex commented May 15, 2023

greg0ire May 15, 2023

Choose a reason for hiding this comment

SenseException left a comment

Choose a reason for hiding this comment

SenseException May 23, 2023

Choose a reason for hiding this comment

SenseException May 23, 2023

Choose a reason for hiding this comment

SenseException May 23, 2023

Choose a reason for hiding this comment

e-vil-dev May 26, 2023

Choose a reason for hiding this comment

e-vil-dev commented May 26, 2023

mrVrAlex commented Jun 3, 2023 • edited Loading

mrVrAlex left a comment

Choose a reason for hiding this comment

greg0ire commented Jun 5, 2023

mrVrAlex commented Jun 5, 2023

greg0ire commented Jun 5, 2023

mrVrAlex commented Jun 26, 2023

SenseException commented Jul 6, 2023

e-vil-dev commented Jul 11, 2023

greg0ire commented Jul 11, 2023

rcadhikari commented Jul 1, 2024

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

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

mrVrAlex commented Mar 30, 2023 •

edited

Loading

mrVrAlex commented Mar 31, 2023 •

edited

Loading

mrVrAlex commented Jun 3, 2023 •

edited

Loading