OCA / pylint-odoo

Odoo plugin for Pylint
http://www.pylint.org
143 stars 168 forks source link

New linter: Don't `write` on a computed field #356

Closed pedrobaeza closed 1 year ago

pedrobaeza commented 3 years ago

Or bad things will happen

Example:

https://github.com/OCA/stock-logistics-warehouse/blob/9f0124ade73a167c7f512f7f4aef57e6a4ffceb0/sale_stock_available_info_popup/models/sale_order.py#L39

Immediate solution: do update instead.

moylop260 commented 3 years ago

Hi Pedro

Thank you for creating this issue

I have been this lint in my mind a long time ago

FedericoGregori commented 2 years ago

Sorry I don't understand the "Or bad things will happen". Can I ask an example? This is a huge thing to teach to every dev I think.

pedrobaeza commented 2 years ago

Onchange methods are executed in "edit-mode", so you need to save to get the definitive values saved on the DB, but if you do a write on it, that changes are immediately saved to the DB, so if you discard the changes on UI, you will have inconsistent data. That's why you have to do update instead, that only modifies the temporary dataset.

The same applies with create and with unlink.

voronind commented 2 years ago

It is hard to distinguish write on record or non-record object.

moylop260 commented 2 years ago

It is hard to distinguish write on record or non-record object.

Yes, it is hard.

Instead, We could check all the fields with compute parameter and check if that method has a write

I mean,

name = fields.Char(compute="_compute_name")

Then get the method "_compute_name" and check if is using ".write("

voronind commented 2 years ago

I mean

def _compute_name(self):
    ...
    with open('file.txt', 'wb') as file:
        file.write(b'some')
moylop260 commented 2 years ago

In this case the write is a method of a built-in object file

Ast detects this so we could discard all detected ones

moylop260 commented 2 years ago

@antonag32 Could you check it, please?

antonag32 commented 2 years ago

@moylop260 Yes, will start working on it

voronind commented 2 years ago

In this case the write is a method of a built-in object file

Ast detects this so we could discard all detected ones

OK. And in this case?

def _compute_name(self):
    ...
    unknown_type_object = self._get_object()
    unknown_type_object.write(b'some')
moylop260 commented 2 years ago

It should came from a browse variable or only self

moylop260 commented 1 year ago

I was testing the checker and the following line was detected in Odoo core

Do you know if there is a valid case to use write or could be it a valid error?

moylop260 commented 1 year ago

New findings updated from odoo repos

odoo/odoo@16.0

addons/fleet/models/fleet_vehicle.py:141:12: E8135: Compute method calling `write`. Use `update` instead. (no-write-in-compute)
addons/product_margin/models/product_product.py:179:12: E8135: Compute method calling `write`. Use `update` instead. (no-write-in-compute)
addons/resource/models/resource.py:228:12: E8135: Compute method calling `write`. Use `update` instead. (no-write-in-compute)
addons/resource/models/resource.py:239:12: E8135: Compute method calling `write`. Use `update` instead. (no-write-in-compute)

odoo/enterprise@16.0

account_batch_payment/models/account_payment.py:22:12: E8135: Compute method calling `write`. Use `update` instead. (no-write-in-compute)
helpdesk/models/helpdesk_ticket.py:368:12: E8135: Compute method calling `write`. Use `update` instead. (no-write-in-compute)
hr_payroll/models/hr_payslip_run.py:48:16: E8135: Compute method calling `write`. Use `update` instead. (no-write-in-compute)
hr_payroll/models/hr_payslip.py:755:8: E8135: Compute method calling `write`. Use `update` instead. (no-write-in-compute)
hr_payroll/models/hr_payslip.py:766:12: E8135: Compute method calling `write`. Use `update` instead. (no-write-in-compute)
l10n_be_hr_payroll/models/l10n_be_274_XX.py:174:12: E8135: Compute method calling `write`. Use `update` instead. (no-write-in-compute)
l10n_be_hr_payroll/models/l10n_be_281_10.py:166:12: E8135: Compute method calling `write`. Use `update` instead. (no-write-in-compute)
l10n_be_hr_payroll/models/l10n_be_281_45.py:89:12: E8135: Compute method calling `write`. Use `update` instead. (no-write-in-compute)
l10n_be_hr_payroll/models/l10n_be_individual_account.py:54:12: E8135: Compute method calling `write`. Use `update` instead. (no-write-in-compute)
pedrobaeza commented 1 year ago

I would say all of them are errors.

moylop260 commented 1 year ago

FYI @xmo-odoo

tde-banana-odoo commented 1 year ago

Lot of them are related to HR apps, I warned mighty @tivisse.

tivisse commented 1 year ago

Incoming fixes: https://github.com/odoo/odoo/pull/104419 https://github.com/odoo/enterprise/pull/33366

Thanks for the report ! :100:

Have a nice day,

Yannick.