cphalcon icon indicating copy to clipboard operation
cphalcon copied to clipboard

[BUG]:Model::save(), hasOne relation, cause insertion of related

Open zordon13 opened this issue 3 years ago • 9 comments

Describe the bug I have table test in database (PostgreSQL) and have view test_data of this table. When i save to table test from model - everything is fine. When i getRealted model (from test_data view) and after save test model - orm try insert realted model (into view!)

To Reproduce Steps to reproduce the behavior:


CREATE TABLE public.test
(
    id serial,
    str text COLLATE pg_catalog."default",
    CONSTRAINT test_pkey PRIMARY KEY (id)
)

CREATE OR REPLACE VIEW public.test_data
 AS
 SELECT test.id,
    test.str,
    now() AS dtn
   FROM test;



class TestData extends \Phalcon\Mvc\Model{    

    public $id;
    public $str;
    public $dtn;  

    public function initialize() {
        $this->setSchema("public");
        $this->setSource('test_data');        
    }
}

class Test extends \Phalcon\Mvc\Model{    
    public $id;
    public $str;   

    public function initialize() {
        $this->setSchema("public");
        $this->setSource('test');
        $this->hasOne('id', TestData::class, 'id', ['alias' => 'TestData']);
    }
    public function getTestData(): TestData {
        return $this->getRelated('TestData');
    } 
}

Normal behavior 1:

$test = new  Test;
$test->str = 'Hello';
$test->save();

Normal behavior 2:

$test = Test::findFirst();
$test->str = 'Hello World';
$test->save();

Unexpected behavior

$test = Test::findFirst();
var_dump($test->toArray());
//get related "view"
$testd = $test->getTestData();
var_dump($testd->toArray());        
//modify Test (not TestData)
$test->str = 'Good Bye World!';        
$test->save();

Output

array(2) {
  ["id"]=>
  int(1)
  ["str"]=>
  string(11) "Hello World"
}
array(3) {
  ["id"]=>
  int(1)
  ["str"]=>
  string(11) "Hello World"
  ["dtn"]=>
  string(29) "2022-06-25 14:36:08.890285+03"
}
SQLSTATE[0A000]: Feature not supported: 7 ERROR:  cannot insert into column "dtn" of view "test_data"
DETAIL:  View columns that are not columns of their base relation are not updatable.

**SQL log ** from event manager

[2022-06-25T14:38:43+03:00][DEBUG] SELECT "test"."id", "test"."str" FROM "public"."test" LIMIT :APL0
[2022-06-25T14:38:43+03:00][DEBUG] array (
  'APL0' => 1,
)
[2022-06-25T14:38:43+03:00][DEBUG] SELECT "test_data"."id", "test_data"."str", "test_data"."dtn" FROM "public"."test_data" WHERE "test_data"."id" = :APR0 LIMIT :APL0
[2022-06-25T14:38:43+03:00][DEBUG] array (
  'APR0' => 1,
  'APL0' => 1,
)
[2022-06-25T14:38:43+03:00][DEBUG] UPDATE "public"."test" SET "str" = ? WHERE "id" = ?
[2022-06-25T14:38:43+03:00][DEBUG] array (
  0 => 'Good Bye World!',
  1 => 1,
)
[2022-06-25T14:38:43+03:00][DEBUG] INSERT INTO "public"."test_data" ("id", "str", "dtn") VALUES (?, ?, ?)
[2022-06-25T14:38:43+03:00][DEBUG] array (
  0 => 1,
  1 => 'Hello World',
  2 => '2022-06-25 14:38:43.161401+03',
)

Details

  • Phalcon version: (php --ri phalcon)
phalcon


Phalcon is a full stack PHP framework, delivered as a PHP extension, offering lower resource consumption and high performance.
phalcon => enabled
Author => Phalcon Team and contributors
Version => 5.0.0RC2
Build Date => Jun 24 2022 23:12:58
Powered by Zephir => Version 0.16.0-4fac47b

Directive => Local Value => Master Value
phalcon.db.escape_identifiers => On => On
phalcon.db.force_casting => Off => Off
phalcon.orm.case_insensitive_column_map => Off => Off
phalcon.orm.cast_last_insert_id_to_int => Off => Off
phalcon.orm.cast_on_hydrate => Off => Off
phalcon.orm.column_renaming => On => On
phalcon.orm.disable_assign_setters => Off => Off
phalcon.orm.enable_implicit_joins => On => On
phalcon.orm.enable_literals => On => On
phalcon.orm.events => On => On
phalcon.orm.exception_on_failed_save => Off => Off
phalcon.orm.exception_on_failed_metadata_save => On => On
phalcon.orm.ignore_unknown_columns => Off => Off
phalcon.orm.late_state_binding => Off => Off
phalcon.orm.not_null_validations => On => On
phalcon.orm.resultset_prefetch_records => 0 => 0
phalcon.orm.update_snapshot_on_save => On => On
phalcon.orm.virtual_foreign_keys => On => On
phalcon.warning.enable => On => On
  • PHP Version: (php -v)
PHP 8.1.7 (cli) (built: Jun 14 2022 10:26:22) (NTS)
Copyright (c) The PHP Group
Zend Engine v4.1.7, Copyright (c) Zend Technologies
   with Zend OPcache v8.1.7, Copyright (c), by Zend Technologies
  • Operating System: Operating System: Debian GNU/Linux 10 (buster)

  • Installation type: Compiling from source

  • Other related info (Database, table schema): (PostgreSQL) 14.4 (Debian 14.4-1.pgdg100+1) Additional context Add any other context about the problem here.

zordon13 avatar Jun 25 '22 11:06 zordon13

I don't see where this is a bug? You create a model for a view. The Model does not check if you using a view or a table. But the Model expects you to use a table. So you try to accomplish something that's not supported. Not a bug.

But maybe you can try this as a work around: Overwrite the save method of the TestData model to always return true.

public function save(): bool
{
    return truel;
}

Maybe this prevents the action.

noone-silent avatar Jun 27 '22 01:06 noone-silent

The Model does not check if you using a view or a table.

setSource('test') and setSource('test_data') do this! I manipulate only Test model fields. I don't change TesData fields, and i don't call save() on TestData model!

zordon13 avatar Jun 27 '22 04:06 zordon13

The Model does not check if you using a view or a table.

setSource('test') and setSource('test_data') do this! I manipulate only Test model fields. I don't change TesData fields, and i don't call save() on TestData model!

You just set the source. Phalcon expects a full working table not a view!

You load with getRelated a relation into TestModel. If you change something in TestModel it tries to save all related models, because it may have an impact on the related table too. In your case to avoid writing into a view, you need to disable all save related methods, by overwriting them and return true.

Just add this to TestData:

public function save(): bool
{
    return true;
}

noone-silent avatar Jun 27 '22 04:06 noone-silent

I understand... But in Phalcon 4.1.2 (PHP 7.4.29 ) other behavior.

Maybe, the bug in ORM in 4.1.2 verion?

zordon13 avatar Jun 27 '22 07:06 zordon13

Hi folks,

I'm also affected by this. This doesn't seem like an expected behaviour and wasn't the case with previous versions of Phalcon. Just to make sure we understand the problem here, what happens is that:

Model A has a related Model B (a view).

  1. Model A is retrieved.
  2. Related Model B is retrieved (BUT NOT UPDATED NOR SET on the Model A).
  3. Saving Model A INSERTS already existing Model B that was never touched at all in any way - all operations before were read operations.

It seems that the related record is incorrectly marked as dirty, and then the decision is taken to create it.

Thanks

durasj avatar Dec 17 '22 15:12 durasj

This seems a touch worse. I now found out that this is silently causing issues in the background as related records that are not touched cause UPDATEs and sometimes INSERTs irrespective of whether it's a view or a table. It was just easier to spot on views as those are often not writeable and so it throws instead of just causing some extra SQL mutations in the background.

This means users of Phalcon may see:

  • extra load on DB from all unnecessary UPDATEs or INSERTs,
  • duplicate data or other issues connected to triggers that will run on mutations,
  • unwanted updates of columns - e.g. modifiedAt/updatedAt/updatedBy columns.

To again repeat what's going on here, we see this issue during simple scenarios like:

$product = Products::findFirst('id = 1');
$product->getUser();
$product->price = 9000.69;
$product->save();

This will lead to one (expected) UPDATE of Products and one (unexpected) UPDATE or sometimes even INSERT of Users. I haven't figured out when it's an INSERT, but it's obviously more damaging of the two. So, from my point of view, related Users record is incorrectly marked as dirty.

The workaround for this is pretty simple - don't retrieve any related entities (getRelated('x'), getX ...) or retrieve the update entity again before saving it, i.e.:

$product = Products::findFirst('id = 1');
$product->getUser();
$product = Products::findFirst('id = 1');
$product->price = 9000.69;
$product->save();

durasj avatar Jan 04 '23 11:01 durasj

I can confirm this, but even worse. It happens also, even when not calling getRelated or ->RelatedName() or anything to get a related record, it still tries to update or insert.

The validation gets triggered in the related class. We use Annotations for the Model fields

CREATE TABLE test (
  id SERIAL,
  test SMALLINT NOT NULL
);

CREATE TABLE test_related (
  id SERIAL,
  id_test SMALLINT NOT NULL,
  test_value VARCHAR(50) NOT NULL
);
INSERT INTO test VALUES (1, 1);
INSERT INTO test_related VALUES (1, 1, 'testing');
class TestRelated {
    /**
     * @Primary
     * @Identity
     * @Column(type="integer", nullable=false, column="id")
     *
     * @var int|null
     */
    public ?int $id = null;

    /**
     * @Column(type="integer", nullable=false, column="id_test")
     *
     * @var int|null
     */
    public ?int $idTest = null;

  /**
   * @column(type='string', nullable=false, column='test_value')
   * @var string|null
   */
   public ?string $testValue = null;
}

class Test {
    /**
     * @Primary
     * @Identity
     * @Column(type="integer", nullable=false, column="id")
     *
     * @var int|null
     */
    public ?int $id = null;

    /**
     * @Column(type='integer', nullable=false, column='test')
     */
    public ?int $test = null;

    public function initialize(): void
    {
       $this->hasOne('id', TestRelated::class, 'idTest', ['alias' => 'testRelated', 'reusable' => true]);
    }
}

$tests = Test::find();
foreach ($tests as $test) {
    ++$test->test;
    $test->save(); // Error with 'testValue is required' from related class.
}

Version is Phalcon v5.1.2 PHP 8.1.17

This are the lines of code, nowhere we call getRelated('VsTariff') or anything like that in ContractCalculator. I triple checked that:

foreach ($contracts as $contract) {
            $calculator = new ContractCalculator($contract);
            $summary = $calculator->getFeeSummary();
            $contract->yearNetValue = $summary['total']['net'];

            if ($contract->save() === false) {
                return $this->setError($contract->getMessagesAsString());
            }
        }

This is the message proof from related class VsTariff called from class Contracts grafik

noone-silent avatar Apr 03 '23 04:04 noone-silent

Another thing I noticed. If you modify a field in afterFetch, it will marked as changed in the snapshots. For example. You have to un-/serialize data after the fetch or before saving. This will trigger the field as changed and forces an update and also triggers the related updates.

noone-silent avatar Apr 04 '23 04:04 noone-silent

After update to v5.4.0, again broke save for my case. save() {return true;} in Data model ( "view" form table) no longer works.

 ПОДРОБНОСТИ:  Views that do not select from a single table or view are not automatically updatable.
 ПОДСКАЗКА:  To enable inserting into the view, provide an INSTEAD OF INSERT trigger or an unconditional ON INSERT DO INSTEAD rule.
...
 5 => #0 (): execute
 6 => #1 (): executePrepared
 7 => #2 (): execute
 8 => #3 (): insert
 9 => #4 (): doLowInsert
 10 => #5 (): doSave
 11 => #6 (): preSaveRelatedRecords
 12 => #7 (): doSave
...```

zordon13 avatar Nov 03 '23 20:11 zordon13