Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Assert data are unchanged during save #1053

Open
mkrecek234 opened this issue Aug 17, 2022 · 11 comments
Open

Assert data are unchanged during save #1053

mkrecek234 opened this issue Aug 17, 2022 · 11 comments
Assignees

Comments

@mkrecek234
Copy link
Contributor

mkrecek234 commented Aug 17, 2022

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
Copy link
Member

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

@mkrecek234
Copy link
Contributor Author

mkrecek234 commented Aug 17, 2022

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 mkrecek234 added the bug label Aug 17, 2022
@mkrecek234
Copy link
Contributor Author

@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 mvorisek removed the bug label Aug 17, 2022
@mvorisek
Copy link
Member

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
Copy link
Contributor Author

mkrecek234 commented Aug 18, 2022

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
Copy link
Member

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
Copy link
Contributor Author

@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
Copy link
Contributor Author

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 mvorisek reopened this Aug 23, 2022
@mvorisek
Copy link
Member

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.

@mvorisek mvorisek changed the title Re-save of same value in model creates Exception Assert data are unchanged during save Aug 23, 2022
@mkrecek234
Copy link
Contributor Author

mkrecek234 commented Aug 23, 2022

@mvorisek Let me explain how to reproduce:

  1. You have a table with three fields in your MySQL database:
  • id (AUTOINCREMENT)
  • amount (DECIMAL 13,2 - so 2-digit precision)
  • intvalue (INTEGER, Do not allow null)

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:

  • Amount (atk4_money)
  • intvalue (integer, nullable=true)

(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
Copy link
Member

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.

@mvorisek mvorisek reopened this Aug 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants