From aeb5ed9e8abe8926555b9033af5e42d7f16ef8f7 Mon Sep 17 00:00:00 2001 From: Rudi Servo Date: Thu, 10 Aug 2023 15:57:09 +0000 Subject: [PATCH] Fixed infinite save loop #16395 --- CHANGELOG-5.0.md | 6 ++++ phalcon/Mvc/Model.zep | 50 +++++++++++++++++++++------ tests/database/Mvc/Model/SaveCest.php | 29 ++++++++++++++++ 3 files changed, 74 insertions(+), 11 deletions(-) diff --git a/CHANGELOG-5.0.md b/CHANGELOG-5.0.md index 728ae381424..87dab113a01 100644 --- a/CHANGELOG-5.0.md +++ b/CHANGELOG-5.0.md @@ -1,5 +1,11 @@ # Changelog +## [5.3.1](https://github.com/phalcon/cphalcon/releases/tag/v5.3.1) (xxxx-xx-xx) + +### Fixed +- Infinite save loop in Model::save() [#16395](https://github.com/phalcon/cphalcon/issues/16395) + + ## [5.3.0](https://github.com/phalcon/cphalcon/releases/tag/v5.3.0) (2023-08-15) ### Added diff --git a/phalcon/Mvc/Model.zep b/phalcon/Mvc/Model.zep index fa7096ebbad..e5f5bf9b9ac 100644 --- a/phalcon/Mvc/Model.zep +++ b/phalcon/Mvc/Model.zep @@ -41,6 +41,8 @@ use Phalcon\Mvc\Model\TransactionInterface; use Phalcon\Mvc\Model\ValidationFailed; use Phalcon\Mvc\ModelInterface; use Phalcon\Filter\Validation\ValidationInterface; +use Phalcon\Support\Collection; +use Phalcon\Support\Collection\CollectionInterface; use Serializable; /** @@ -2579,12 +2581,35 @@ abstract class Model extends AbstractInjectionAware implements EntityInterface, * $robot->save(); *``` */ + public function save() -> bool + { + var visited; + let visited = new Collection(); + return this->doSave(visited); + } + + /** + * Inserted or updates model instance, expects a visited list of objects. + * + * @param CollectionInterface $visited + * + * @return bool + */ + public function doSave( visited) -> bool { var metaData, schema, writeConnection, readConnection, source, table, - identityField, exists, success, relatedToSave; + identityField, exists, success, relatedToSave, objId; bool hasRelatedToSave; + let objId = spl_object_id(this); + + if true === visited->has(objId) { + return true; + } + + visited->set(objId, this); + let metaData = this->getModelsMetaData(); /** @@ -2610,7 +2635,7 @@ abstract class Model extends AbstractInjectionAware implements EntityInterface, let hasRelatedToSave = count(relatedToSave) > 0; if hasRelatedToSave { - if this->preSaveRelatedRecords(writeConnection, relatedToSave) === false { + if this->preSaveRelatedRecords(writeConnection, relatedToSave, visited) === false { return false; } } @@ -2695,7 +2720,7 @@ abstract class Model extends AbstractInjectionAware implements EntityInterface, /** * Change the dirty state to persistent */ - if success { + if true === success { let this->dirtyState = self::DIRTY_STATE_PERSISTENT; } @@ -2711,7 +2736,8 @@ abstract class Model extends AbstractInjectionAware implements EntityInterface, */ let success = this->postSaveRelatedRecords( writeConnection, - relatedToSave + relatedToSave, + visited ); } } @@ -4964,9 +4990,11 @@ abstract class Model extends AbstractInjectionAware implements EntityInterface, * Saves related records that must be stored prior to save the master record * * @param ModelInterface[] related + * @param CollectionInterface visited * @return bool */ - protected function preSaveRelatedRecords( connection, related) -> bool + + protected function preSaveRelatedRecords( connection, related, visited) -> bool { var className, manager, type, relation, columns, referencedFields, nesting, name, record; @@ -5006,7 +5034,6 @@ abstract class Model extends AbstractInjectionAware implements EntityInterface, "Only objects can be stored as part of belongs-to relations in '" . get_class(this) . "' Relation " . name ); } - let columns = relation->getFields(), referencedFields = relation->getReferencedFields(); // let columns = relation->getFields(), @@ -5023,7 +5050,7 @@ abstract class Model extends AbstractInjectionAware implements EntityInterface, * If dynamic update is enabled, saving the record must not take any action * Only save if the model is dirty to prevent circular relations causing an infinite loop */ - if record->dirtyState !== Model::DIRTY_STATE_PERSISTENT && !record->save() { + if record->dirtyState !== Model::DIRTY_STATE_PERSISTENT && !record->doSave(visited) { /** * Get the validation messages generated by the * referenced model @@ -5072,9 +5099,10 @@ abstract class Model extends AbstractInjectionAware implements EntityInterface, * Save the related records assigned in the has-one/has-many relations * * @param ModelInterface[] related + * @param CollectionInterface visited * @return bool */ - protected function postSaveRelatedRecords( connection, related) -> bool + protected function postSaveRelatedRecords( connection, related, visited) -> bool { var nesting, className, manager, relation, name, record, columns, referencedModel, referencedFields, relatedRecords, value, @@ -5157,7 +5185,7 @@ abstract class Model extends AbstractInjectionAware implements EntityInterface, /** * Save the record and get messages */ - if !recordAfter->save() { + if !recordAfter->doSave(visited) { /** * Get the validation messages generated by the * referenced model @@ -5221,7 +5249,7 @@ abstract class Model extends AbstractInjectionAware implements EntityInterface, /** * Save the record and get messages */ - if !intermediateModel->save() { + if !intermediateModel->doSave(visited) { /** * Get the validation messages generated by the referenced model */ @@ -5244,7 +5272,7 @@ abstract class Model extends AbstractInjectionAware implements EntityInterface, /** * Save the record and get messages */ - if !recordAfter->save() { + if !recordAfter->doSave(visited) { /** * Get the validation messages generated by the * referenced model diff --git a/tests/database/Mvc/Model/SaveCest.php b/tests/database/Mvc/Model/SaveCest.php index 3bf55a34f72..9f4915db4c6 100644 --- a/tests/database/Mvc/Model/SaveCest.php +++ b/tests/database/Mvc/Model/SaveCest.php @@ -644,6 +644,35 @@ public function mvcModelSaveWithRelatedManyAndBelongsRecordsProperty(DatabaseTes $I->assertEquals($expected, $actual); } + /** + * Tests Phalcon\Mvc\Model\ :: save() Infinite Loop + * + * @author Phalcon Team + * @since 2023-08-09 + * @issue https://github.com/phalcon/cphalcon/issues/16395 + * + * @group mysql + * @group sqlite + */ + public function infiniteSaveLoop(DatabaseTester $I) + { + $I->wantToTest('Mvc\Model - save() infinite Save loop'); + + /** @var \PDO $connection */ + $connection = $I->getConnection(); + $invoicesMigration = new InvoicesMigration($connection); + $invoicesMigration->insert(77, 1, 0, uniqid('inv-', true)); + + $customersMigration = new CustomersMigration($connection); + $customersMigration->insert(1, 1, 'test_firstName_1', 'test_lastName_1'); + + $customer = Customers::findFirst(1); + $invoice = Invoices::findFirst(77); + $invoice->customer = $customer; + $customer->invoices = [$invoice]; + $customer->save(); + } + /** * @return \string[][] */