atk4 / data

Data Access PHP Framework for SQL & high-latency databases
https://atk4-data.readthedocs.io
MIT License
270 stars 46 forks source link

Assert data are unchanged during save #1053

Open mkrecek234 opened 2 years ago

mkrecek234 commented 2 years ago

Use case: You have code which updates the total sum of an order. Now the customer adds an order position with price = 0. You don't want to deal with it but just save the new total (which is identical to the old total).

Steps to reproduce: 1) You have any model $model with an entity id=1, total_amount =100 2) You do `$model->load(1)->save(['total_amount' => 100]);

You will get this error in recent develop branch: Atk4\Data\Exception: Update failed, exactly 1 row was expected to be affected

I don't think this is an exception - testing for changes is done in save command - if not change happens because re-save of prior set values, then it is not an Exception. This behaviour was added with the recent changes in Atk4/Data

mvorisek commented 2 years ago

it would be great if you can add a simple model code

mkrecek234 commented 2 years ago

Just save a value to any model entity which is already saved, so conceptually:

$model = new Model($app->db);
$entity = $model->load(1);

$entity->save(['name' => $entity->get('name')]);

(sure this is not realistic - but can well be that code will save a newly calculated field which is identical to a stored value. And that is perfectly fine, no need for Exception.

mkrecek234 commented 2 years ago

@mvorisek Sorry for the abbreviated example - I tried to reproduce myself. The error was there when I did the following:

// For your reference: $model is the currently save OrderDocPosition in an beforeInsert hook.
// When I had the following line I go the error on save below - when I changed it to $orderdocmodel = (new \Atk4\Erp\Model\OrderDoc($this->getPersistence())) the error was gone... S

$orderdocmodel =   clone $model->refModel('order_doc_id'); 

        $orderdocmodel->getReference('order_doc_position')->addField('total_gross_pos', ['expr' => 'sum([total_gross])']);
        $orderdocmodel->getReference('order_doc_position')->addField('total_net_pos', ['expr' => 'sum([total_net])']);
        $orderdocmodel->getReference('order_doc_position')->addField('total_tax_pos', ['expr' => 'sum([total_tax])']);

        $orderdocentity = $orderdocmodel->load($this->order_doc_id_to_be_updated ?? $model->get('order_doc_id'));

        $totalgross = $orderdocentity->get('total_gross_pos');
        $totalnet = $orderdocentity->get('total_net_pos');
        $totaltax = $orderdocentity->get('total_tax_pos');

        $this->amount_changed = false;   

// The following threw an error when the new $total_net was identical to the already stored total_net.           
        $orderdocentity->save([
                'total_net' => ($totalnet ??  0),
                'total_gross' => ($totalgross ?? 0),
                'total_tax' => ($totaltax ?? 0)
            ]);

If this is helpful for you, we can keep the issue, otherwise we just close it.

mvorisek commented 2 years ago

Thanks for the repro code. I will repro on my side and double check the exception. Aggregated fields should never be allowed to set in the first place and maybe we can throw an exception earlier.

Atk4\Data\Exception: Update failed, exactly 1 row was expected to be affected

this exception is "last resort" - it prevented you from doiing something nasty correctly

mkrecek234 commented 2 years ago

No, in this example I do not set an aggregate field: I create aggregate fields (see they are notes _pos at the end) to calculate the sum of the positions.

This sum then is stored into a simple field in the orderdoc.

This works always fine, only if you save the identical sum over the already saved one gives you that error. The error says that 1 expected row to be affected but 0 rows changed which is correct - but also not a problem.

mvorisek commented 2 years ago

I still do not understand the repro steps fully.

@mkrecek234 Please post here a simple model /w one aggregate field and simple use of it, which throws the exception.

mkrecek234 commented 2 years ago

@mvorisek I haven't yet achieved to write nice repro code but I analysed it further. The circumstance leading to this error is:

  1. The database was configured to store 10,2 decimals while Atk4/Data is calculating always with 4 digits.
  2. When storing a value for example 100.0213 over an old value of 100.02 the error will arise (and this is great that it highlights it) - the database will not affect any row as the rounded value is the same - however Atk4/Data recognizes it is not the same and expects exactly 1 changed row.

What does this teach me: The framework once again became even better - also these very detailed issues will be highlighted properly. So, thank you @mvorisek

mkrecek234 commented 2 years ago

P.S. This happens also if your database is configured NOT to allow NULL values for a specific field, this field has stored a 0 and you remove the 0 in Atk4/Ui and Save. This would lead Atk4/Data to store null in the database where the database converts it into 0 for a non nullable field and thus no rows were affected by the update - and Atk4/Data will throw an Exception.

So to summarize for other users: Whenever there is a mismatch in field type configuration between the database and Atk4/Data model field setup, you might likely encounter the error above.

mvorisek commented 2 years ago

Yes, a lot of work is done to detect such data mismatches.

I am reopening, as when we read the data after save (insert/update), we can even assert all values are stable, ie. not changed by the DB like rounding or null->empty.

mkrecek234 commented 2 years ago

@mvorisek Let me explain how to reproduce:

1) You have a table with three fields in your MySQL database:

Lets's assume there is a record with id=1 which has the following stored values:

Amount=13.02 and intvalue=0

Now you have a model which is defined in this way:

(You see the mismatch to the DB definition: 4 digit precision vs. 2, and nullable false vs. true

If on this you perform the following save functions: 1) $model->load(1)->save(['amount'=> 13.0188)

or you do 2) $model->load(1)->save(['intvalue'=>null) the exception will happen:

Why? Because correctly Atk4 sees that there changed values, so updateRaw command is executed. However, there are only 0 affected rows wheres 1 expected, because the corrected value by the database (round(amount,2) and also the null value transformed to 0) are the same as the stored value and would not mean an updated row.

So this is unrelated to the aggregate function. Just bad setup by me with mismatching field definitions 😀 And in my setup only with the recent changes in Atk4/Data the error was correctly detected. So thumbs up to those improvements incorporated👍

mvorisek commented 2 years ago

The repro steps/issue is clear since your previous comment.

The Update failed, exactly 1 row was expected to be affected exception was generated as you probably had some unstable value as a condition. I renamed this issue do detect such problem for all fields, not only these used in a condition.