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

Compute with "readonly=False" as an alternative to onchange #32784

Draft
wants to merge 93 commits into
base: master
from

Conversation

Projects
None yet
@fpodoo
Copy link
Contributor

commented Apr 17, 2019

Support for compute, with readonly=False as an alternative to onchange & conditional defaults.

Rationale

Most onchange includes business logic (ex: changing a partner on an invoice sets the
fiscal position, and the payment terms). But the business logic should not be exclusive to
the user interface. Thus, most of these onchanges should actually be compute fields with
store=True and readonly=False.

Compute fields are usually orthogonal between each others, thus simpler to implement and
inherits. Example: on an invoice, it's easy to implement _compute_fiscal_position (it's a
related partner_id.fiscal_position_id), but very complex to implement _onchange_partner_id
(it has impact on lots of different concepts depending on installed modules: fiscal positions,
payment terms, address, ...). By splitting this big onchange into smaller compute, it should
simplify the code and inheritability.

USE CASE 1:

If onchanges are replaced by compute fields, the code to create an invoice, or automated tests
it is simplified as you can let the business logic do its job:

self.env['account.invoice'].create({
    'partner_id': 1
    'line_ids': [{
        'product_id': 1
    })
)}

-> the compute fields will compute the right fiscal position, taxes, customer address, etc. It's
not the role of the sale order to implement this logic anymore.

This simplifies creation of records in the code, but also import. (you import only some data, and you can let the business logic do the rest: import customers of your invoice, and the system will setup the journal, currency, fiscal position, taxes, etx)

USE CASE 2:

Let's say the module l10n_mx want to add a TAXCODE field on an invoice,
there is two way to do it:

@api.onchange['partner_id')
def _change_partner_id(self):
    super()
    self.taxcode = self.partner_id.taxcode

or, the new approach:

@api.depends('partner_id')
def _compute_taxcode(self):
    self.taxcode = self.partner_id.taxcode

The onchange approach creates a big issue: you will have to manage this logic by creating
bridge modules for every module that creates an invoice (e.g. l10n_mx_sale and
l10n_mx_subsription).

With the new approach, the TAXCODE is delegated to the invoice, and handled automatically
at creation, unless provided explicitly. Every module who create an invoice will have the new
business logic applied automatically: no need to create bridge modules.

Having some data that are computed downstream and others upstream, and some backend and others for the interface only is, in my opinion, a big design flaw in Odoo's framework. It is much easier if everything is computed in the same direction, with only one dependency tree, and one concept. (imagine computed fields are a like an excel cell, whose formula depends on others cell; imagine the mess if Excel would have introduced "onchange")

Technicals

The main change is to evaluate computed fields only when we need it, instead of "at every write & create". Thus, computed fields are evaluated at the end of the transaction (commit), when you read a field that is marked as "to recompute", or when you search on a field that have record marked as "to recompute".

As opposed to the current version, the value in cache is usually not invalidated. Instead, we use the existing "todo" concept of the framework to mark fields to be recomputed. Except that we don't process the "todo" at each write() / create() anymore.

That allows multiple speed improvements:

  • do not recompute fields, when we write the same value than already in the cache (when validating an invoice, a lot of computed fields are currently evaluated several times during the time of building the invoice. -> the company_id is set by default, but then computed as related to the journal_id --> triggers most computed fields)
  • in case of multiple write/create, the computed fields are evaluated only once: if you add lines one by one on an invoice, the total/taxes of the invoice will still be computed only once, at the end of the transaction.

So, the following code will evaluate invoice's computed fields only once (e.g., the subtotal & taxes):

invoice = Invoice.create(...)
invoice.write(...)
for line in ...:
    InvoiceLine.create({... 'invoice_id': invoice.id)

But the triggers to evaluate compute fields' dependencies are still called once every create/write. Thus, it's still a good optimization to create all the lines at once to avoid the triggers. (but I think the triggers cost can be reduced a lot, see bellow)

Compatibilities

This version of the ORM is mostly compatible with the current version in master (all the tests of the base module passed without modifications). But there are subtles impacts that requires to fix some business modules. The main ones are:

  • if you perform direct SQL queries, you might want to do self.recompute() or self.recompute_fields(...) to evaluate and write the computed values before the SQL queries. (In master, you had to do it in computed fields, but you were safe after a write().
  • there are some bugs to fix in business modules that were not noticeable because function fields were updated, at the end of write() or create(). (the main ones are missing dependencies in @Depends() who were not a big deal before and can create bugs now)
  • recursive fields feature has been removed. Instead, explicitly call a self.add_todo() on children to mark them as to "recompute". --> it's actually very easy to put them back, but as they are not used a lot, I think it's better to remove this feature from the framework and explicitly manage recursivity in the business code.

Side Effect

Performances seem much better with this refactoring. The following code is 2.32x faster with demo data (0.0086s vs 0.0037s):

for partner in self.env['res.partner'].search([]):
    if partner.state_id:
        cn = partner.state_id.country_id.name

Writing and creating complex records (with related fields, computed fields) seems faster too, but I still have to do the benchmark on real business modules. The method testme() in this branch, that create a complex object with one2many, multiple related and computed fields is:

  • 1.82x faster: 0.0351s vs 0.0192s
  • 1.66x less SQL queries: 50 vs 30

Stack trace is shorter when crashing in computed fields.

This branch will also allow to remove computed fields' triggers using inverse fields instead or hard SQL queries. I expect this to speed up most write() / create() by reducing SQL queries by an extra ~35%, which should translates into an extra speed improvement.

Misc Notes

  • record.field = 3 is now 100% equivalent to record.write({'field': 3})
  • record.write(...) doet not write, but update in-memory
  • _inherits are not anymore handled by write(...), but by related fields inverse's method
  • prefetch should be improved (what about passing the record set instead of an iterable on ids)
  • recompute computes in-memory, but does not write anymore
  • stored fields have only one value in cache (cache does not depend on the context for those fields)
  • @Depends are evaluated in memory through inverse fields when possible, instead of hard SQL queries
  • towrite holds value that have changed but not written to the DB anymore --> use towrite_flush() before direct SQL queries
  • ACLs checks are made in write(). --> _write(...) which become a low level function that do not check ACL
  • removed "global" from ir.rule and check if groups are set instead (global is a python protected keyword)
  • boolean: is false at pg level instead of python level

Status

  • the compute alternative to onchange works perfectly
  • base module installs and pass all tests
  • but there are still bugs to fix, and business modules to review

It's a quick POC written in a few evenings; it still requires a huge cleanup.

Once these are done, ideas for future improvements: https://pad.odoo.com/p/r.896af7a5c2a2fc86575f8c5b4d306419

About onchange

Onchange will remain supported in v13, but should be limited to pure UI changes, without business logic. (so only a few; pretty much everything should be implemented with computed fields instead)

--
I confirm I have signed the CLA and read the PR guidelines at www.odoo.com/submit-pr

fpodoo added some commits Mar 30, 2019

[IMP] base: support for compute, with store=True as a better alternat…
…ive to onchange

Rationale:

Most onchange includes business logic (ex: changing a partner on an
invoice sets the fiscal position, and the payment terms). But the
business logic should not be exclusive to the user interface. Thus,
most of these onchanges should actually be compute with store=True
and readonly=False.

Compute fields are usually orthogonal between each others, thus simpler
to implement and inherits. Example: on an invoice, it's easy to
implement _compute_fiscal_position (it's a related
partner_id.fiscal_position_id), but very complex to implement
_onchange_partner_id (it has impact on lots of different concepts
depending on installed modules: fiscal positions, payment terms,
address, ...). By splitting this big onchange into smaller compute,
it should simplify the code and inheritability.

USE CASE 1:

If onchanges are replaced by compute fields, the code to create an
invoice is simplified as you can let the business logic do its job:

self.env['account.invoice'].create({
    'partner_id': 1
    'line_ids': [{
        'product_id': 1
    })
)}

-> the compute fields will compute the right fiscal position, taxes,
customer address, etc. It's not the role of the sale order to implement
this logic anymore.

This should simplify tests too as you cover more business logic code
with less data to provide.

USE CASE 2:

Let's say the module l10n_mx want to add a TAXCODE field on an invoice,
there is two way to do it:

@api.onchange['partner_id')
def _change_partner_id(self):
    super()
    self.taxcode = self.partner_id.taxcode

or, the new approach:

@api.depends('partner_id')
def _compute_taxcode(self):
    self.taxcode = self.partner_id.taxcode

The onchange approach creates a big issue: you will have to create modules
like l10n_mx_sale and l10n_mx_subsription. Because, when these modules create
an invoice, they will have to call the code to set the taxcode.

With the new approach, there is nothing to do. Every module who create
an invoice will have the new business logic applied automatically: no
need to create glue modules.
fix
wip
wip
wip
wip

fpodoo added some commits May 1, 2019

fix
fix
fix
fix
@nhomar

This comment has been minimized.

Copy link
Collaborator

commented on odoo/addons/base/models/ir_module.py in 6888bcf May 12, 2019

just to let you know, in devel time, if you add a new dependency, and you set store=True you will probably face a problem with the updating process of the dependencies. Did you test such case?

fpodoo and others added some commits May 12, 2019

fix
fix
fix
acl
@fpodoo

This comment has been minimized.

Copy link

commented on odoo/api.py in 0470d55 May 17, 2019

to check with RCO that fixed that in his branch

@fpodoo

This comment has been minimized.

Copy link

commented on odoo/fields.py in 0470d55 May 17, 2019

to revert, and check why you have a one2many in DB

@fpodoo

This comment has been minimized.

Copy link

commented on odoo/fields.py in 0470d55 May 17, 2019

To check: I think you break inverse fields (thus, inheritancies)

@fpodoo

This comment has been minimized.

Copy link

commented on odoo/models.py in 0470d55 May 17, 2019

To check: what about doing a setdefault on the cache: do not update the cache if value already in cache (independant from to write)
@rco-odoo

@fpodoo

This comment has been minimized.

Copy link

commented on odoo/models.py in 0470d55 May 17, 2019

add fields as arguments of towrite_flush()

@fpodoo

This comment has been minimized.

Copy link

commented on odoo/models.py in 0470d55 May 17, 2019

What about putting self._ids[key] in _prefetch when doing a slice.
@rco-odoo

@fpodoo

This comment has been minimized.

Copy link

commented on odoo/addons/base/models/res_partner.py in 0470d55 May 17, 2019

Cela crash à l'install de base à cause de cela.

beledouxdenis added some commits May 20, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.