Tryton - Issues

 

Issue3516

Title Fix anti-pattern about create/write in loop
Priority feature Status in-progress
Superseder Allow to write different values in one call, Use instances in Invoice
View: 3499, 5751
Nosy List ced, pokoli, reviewbot, roundup-bot
Type performance Components account, account_asset, account_dunning, account_invoice, account_statement, calendar, calendar_scheduling, calendar_todo, commission, product_cost_fifo, production, project_plan, purchase, sale, sale_shipment_cost, sale_supply, sale_supply_drop_shipment, stock, stock_forecast, stock_lot, stock_supply, stock_supply_production, trytond
Assigned To pokoli Keywords review
Reviews 10861002, 13951002, 12931002, 10151002, 13941002, 11861002, 6011002, 13931002, 8921002, 14941002, 14931003, 12941002, 7911003, 6901002, 13961002, 6891002, 14951002, 5991003, 11861004, 11871002, 6011003, 11861003, 10151003, 27501002, 43951002
View: 10861002, 13951002, 12931002, 10151002, 13941002, 11861002, 6011002, 13931002, 8921002, 14941002, 14931003, 12941002, 7911003, 6901002, 13961002, 6891002, 14951002, 5991003, 11861004, 11871002, 6011003, 11861003, 10151003, 27501002, 43951002

Created on 2013-11-21.23:52:37 by ced, last changed by pokoli.

Files
File name Uploaded Type Edit Remove
tryton-antipatern.txt pokoli, 2013-11-23.14:39:09 text/plain
Messages
New changeset 24da2e72ee3c by Sergi Almacellas Abellana in branch 'default':
Fix antipattern of write in loop
http://hg.tryton.org/modules/account_dunning/rev/24da2e72ee3c
New review43951002 at https://codereview.tryton.org/43951002/#ps1
New changeset 7af815d16729 by Sergi Almacellas Abellana in branch 'default':
Fix antipatern of create/write in loop
http://hg.tryton.org/modules/sale_supply_drop_shipment/rev/7af815d16729
review11871002 updated at https://codereview.tryton.org/11871002/#ps20001
review14951002 updated at https://codereview.tryton.org/14951002/#ps20001
review5991003 updated at https://codereview.tryton.org/5991003/#ps40001
New changeset f396057c6521 by Sergi Almacellas Abellana in branch 'default':
Fix antipatern of create/write in loop
http://hg.tryton.org/modules/stock_supply/rev/f396057c6521
review5991003 updated at https://codereview.tryton.org/5991003/#ps20001
msg27391 (view) Author: [hidden] (pokoli) (Tryton committer) (Tryton translator) Date: 2016-07-27.09:23:51
Indeed sale_supply depended on sale patch. But I removed the dependency with the patch (to be managed on a separate issue) and update it to tip.
msg27373 (view) Author: [hidden] (pokoli) (Tryton committer) (Tryton translator) Date: 2016-07-26.18:06:02
Remaining work:

- sale_supply (review5991003)
- sale_supply_drop_shipment (review14951002)
- calendar (review11861002 and review13941002)
- sale (review11871002, review11861004)
- purchase (review11861003 and review6011003)
- production (review10861002 and review12941002)
- commission: (review12931002)

And it have to be tested if new modules are affected.
msg27346 (view) Author: [hidden] (ced) (Tryton committer) (Tryton translator) Date: 2016-07-26.12:05:19
For clarity, I move the invoice changes into issue5751.
review12931002 updated at https://codereview.tryton.org/12931002/#ps20001
review6901002 updated at https://codereview.tryton.org/6901002/#ps20001
New review27501002 at https://codereview.tryton.org/27501002/#ps1
review13931002 updated at https://codereview.tryton.org/13931002/#ps80001
review10861002 updated at https://codereview.tryton.org/10861002/#ps1
review12941002 updated at https://codereview.tryton.org/12941002/#ps1
review6901002 updated at https://codereview.tryton.org/6901002/#ps1
review14951002 updated at https://codereview.tryton.org/14951002/#ps1
review5991003 updated at https://codereview.tryton.org/5991003/#ps1
review11861004 updated at https://codereview.tryton.org/11861004/#ps1
review6011003 updated at https://codereview.tryton.org/6011003/#ps1
review11861003 updated at https://codereview.tryton.org/11861003/#ps1
review12931002 updated at https://codereview.tryton.org/12931002/#ps1
review11871002 updated at https://codereview.tryton.org/11871002/#ps1
review13931002 updated at https://codereview.tryton.org/13931002/#ps60001
New changeset 9d4ad7d5059b by Sergi Almacellas Abellana in branch 'default':
Fix antipatern of create/write in loop
http://hg.tryton.org/modules/stock_lot/rev/9d4ad7d5059b
New changeset c5140009ad0b by Sergi Almacellas Abellana in branch 'default':
Fix antipatern of create/write in loop
http://hg.tryton.org/modules/stock/rev/c5140009ad0b
review7911003 updated at https://codereview.tryton.org/7911003/#ps60001
review7911003 updated at https://codereview.tryton.org/7911003/#ps40001
review6891002 updated at https://codereview.tryton.org/6891002/#ps40001
review6891002 updated at https://codereview.tryton.org/6891002/#ps20001
New changeset 0c6724c8b4e4 by Sergi Almacellas Abellana in branch 'default':
Fix antipatern of create/write in loop
http://hg.tryton.org/modules/stock_forecast/rev/0c6724c8b4e4
review13961002 updated at https://codereview.tryton.org/13961002/#ps20001
New changeset 8237f9e3afaf by Sergi Almacellas Abellana in branch 'default':
Fix antipatern of create/write in loop
http://hg.tryton.org/modules/product_cost_fifo/rev/8237f9e3afaf
New changeset b267a76f7fd4 by Sergi Almacellas Abellana in branch 'default':
Fix antipatern of create/write in loop
http://hg.tryton.org/modules/account/rev/b267a76f7fd4
New changeset dadac00c5874 by Sergi Almacellas Abellana in branch 'default':
Fix antipatern of create/write in loop
http://hg.tryton.org/modules/project_plan/rev/dadac00c5874
New changeset 9f78fa911076 by Sergi Almacellas Abellana in branch 'default':
Fix antipatern of create/write in loop
http://hg.tryton.org/modules/account_asset/rev/9f78fa911076
New changeset de90bf5435ce by Sergi Almacellas Abellana in branch 'default':
Fix antipatern of create/write in loop
http://hg.tryton.org/trytond/rev/de90bf5435ce
msg22232 (view) Author: [hidden] (ced) (Tryton committer) (Tryton translator) Date: 2015-08-28.09:41:09
@pokoli if you want such patch to be in 3.6, it is time to push them.
msg20329 (view) Author: [hidden] (pokoli) (Tryton committer) (Tryton translator) Date: 2015-02-20.17:44:13
Could you review first this patches (non dependant): 

trytond: http://codereview.tryton.org/14931003/

project_plan: http://codereview.tryton.org/10151003
account: http://codereview.tryton.org/14941002
stock_forecast: http://codereview.tryton.org/13961002
product_cost_fifo: http://codereview.tryton.org/13951002


This must commited at the same time (little dependency)
stock: http://codereview.tryton.org/6891002
stock_lot: http://codereview.tryton.org/7911003


And then I will join all the dependant stuff in a single patch.
msg20050 (view) Author: [hidden] (pokoli) (Tryton committer) (Tryton translator) Date: 2015-02-05.09:41:01
I closed the existing review and added one per repository as suggested
msg19830 (view) Author: [hidden] (ced) (Tryton committer) (Tryton translator) Date: 2015-01-27.00:28:42
I really would prefer small patches instead of one big that will take ages to review. Especially because such work doesn't require to be done at once and also the reviewbot will not be able to work on it.
msg19829 (view) Author: [hidden] (pokoli) (Tryton committer) (Tryton translator) Date: 2015-01-27.00:23:48
I added a WIP patch on the attached review. 

There are some modules that miss the reverse relation field (many2one exisit but reverse one2many no), so they can't be saved with multi. I will update it soon, but main effort is there.  Affected modules are project_invoice, sale_shipment_cost and sale_shipment_cost. 

There is a big api change in invoice, sale, purchase, and production. So i'm wondering if we should add some note in the wiki about it. 

sale_opportunity module is dificult to implement a multi save, as each opportunity is linked to a sale, and lines are also linked to them, so i left it as is for now.
msg18789 (view) Author: [hidden] (pokoli) (Tryton committer) (Tryton translator) Date: 2014-11-03.13:01:53
Dualmethod introduced in issue4197 can be used to fix this issue.
msg17603 (view) Author: [hidden] (pokoli) (Tryton committer) (Tryton translator) Date: 2014-07-17.19:19:46
I reopen it ass the issue is still pending to resolve
New changeset a1966be55b72 by Sergi Almacellas Abellana in branch 'default':
Use grouped_slice helper.
http://hg.tryton.org/modules/timesheet/rev/a1966be55b72
New changeset 05cc5c115585 by Sergi Almacellas Abellana in branch 'default':
Use grouped_slice helper.
http://hg.tryton.org/modules/stock_supply_production/rev/05cc5c115585
New changeset 59a0f06b82a3 by Sergi Almacellas Abellana in branch 'default':
Use grouped_slice helper.
http://hg.tryton.org/modules/stock_supply/rev/59a0f06b82a3
New changeset a8a2bd431758 by Sergi Almacellas Abellana in branch 'default':
Use grouped_slice helper.
http://hg.tryton.org/modules/stock_forecast/rev/a8a2bd431758
New changeset 64d376876173 by Sergi Almacellas Abellana in branch 'default':
Use grouped_slice helper.
http://hg.tryton.org/modules/stock/rev/64d376876173
New changeset 39a9ca86c9e5 by Sergi Almacellas Abellana in branch 'default':
Use grouped_slice helper.
http://hg.tryton.org/modules/sale_supply/rev/39a9ca86c9e5
New changeset 4bf5ea1ac8ee by Sergi Almacellas Abellana in branch 'default':
Use grouped_slice helper.
http://hg.tryton.org/modules/project_revenue/rev/4bf5ea1ac8ee
New changeset fd59763ec6fe by Sergi Almacellas Abellana in branch 'default':
Use grouped_slice helper.
http://hg.tryton.org/modules/project_invoice/rev/fd59763ec6fe
New changeset db60988b0909 by Sergi Almacellas Abellana in branch 'default':
Use grouped_slice helper.
http://hg.tryton.org/modules/party_vcarddav/rev/db60988b0909
New changeset 6033546acbd3 by Sergi Almacellas Abellana in branch 'default':
Use grouped_slice helper.
http://hg.tryton.org/modules/calendar_todo/rev/6033546acbd3
New changeset c82efa3f59dd by Sergi Almacellas Abellana in branch 'default':
Use grouped_slice helper.
http://hg.tryton.org/modules/calendar_classification/rev/c82efa3f59dd
New changeset 1add6ef6690f by Sergi Almacellas Abellana in branch 'default':
Use grouped_slice helper.
http://hg.tryton.org/modules/calendar/rev/1add6ef6690f
New changeset d43a991e46c2 by Sergi Almacellas Abellana in branch 'default':
Use grouped_slice helper.
http://hg.tryton.org/modules/account_payment_sepa/rev/d43a991e46c2
New changeset d4ee1330131f by Sergi Almacellas Abellana in branch 'default':
Use grouped_slice helper.
http://hg.tryton.org/modules/account_invoice/rev/d4ee1330131f
New changeset dc767754be57 by Sergi Almacellas Abellana in branch 'default':
Use grouped_slice helper.
http://hg.tryton.org/modules/account_dunning_letter/rev/dc767754be57
New changeset 86c0ed26fce0 by Sergi Almacellas Abellana in branch 'default':
Use grouped_slice helper.
http://hg.tryton.org/modules/account_asset/rev/86c0ed26fce0
New changeset 094ee9e21ff8 by Sergi Almacellas Abellana in branch 'default':
Use grouped_slice helper.
http://hg.tryton.org/modules/account/rev/094ee9e21ff8
New changeset f00ae016f341 by Sergi Almacellas Abellana in branch 'default':
Add grouped_slice helper and use it.
http://hg.tryton.org/trytond/rev/f00ae016f341
msg16784 (view) Author: [hidden] (pokoli) (Tryton committer) (Tryton translator) Date: 2014-05-09.23:56:55
Just added a review that implements the grouped_slice helper
msg16282 (view) Author: [hidden] (pokoli) (Tryton committer) (Tryton translator) Date: 2014-03-23.13:58:17
I have seen that in a lot of cases where in_max is used, reduce_ids is also
used, so I'm wondering if we can add a third optional argument to grouped_slice,
which will indicate the column to reduce the id and return directly the reduce_sql.
msg16278 (view) Author: [hidden] (pokoli) (Tryton committer) (Tryton translator) Date: 2014-03-22.12:56:53
About grouped_slice, i'm wondering if this should be managed in a separate issue
as there is a lot of places where the "in_max" loop is done and then implement
this issue on top of it. 

Reviewing the diferents uses cases, the number of records to group is variable,
so i think that the function signature will be:

grouped_slice(records, in_max)
msg14775 (view) Author: [hidden] (pokoli) (Tryton committer) (Tryton translator) Date: 2013-11-27.19:50:23
Maybe copy should be improved also as suggested on

https://groups.google.com/d/msg/tryton-dev/PU_P_Z8wjoE/OZbgWTGILjoJ
msg14729 (view) Author: [hidden] (pokoli) (Tryton committer) (Tryton translator) Date: 2013-11-23.14:39:09
I attach a file with all the places I found this antipatern. 

NOTE: I haven't reviewed the test files.
msg14722 (view) Author: [hidden] (pokoli) (Tryton committer) (Tryton translator) Date: 2013-11-22.10:32:23
More places: 

http://hg.tryton.org/modules/account/file/933f85b58a36/move.py#l358
http://hg.tryton.org/modules/account/file/933f85b58a36/fiscalyear.py#l270
http://hg.tryton.org/modules/account/file/933f85b58a36/fiscalyear.py#l307
msg14721 (view) Author: [hidden] (ced) (Tryton committer) (Tryton translator) Date: 2013-11-22.00:00:28
It should probably be implemented after issue3499
msg14720 (view) Author: [hidden] (ced) (Tryton committer) (Tryton translator) Date: 2013-11-21.23:57:15
Here are some place that should be fixed:

http://hg.tryton.org/modules/stock/file/f148528765ea/move.py#l577
http://hg.tryton.org/modules/stock/file/f148528765ea/move.py#l590
http://hg.tryton.org/modules/stock/file/f148528765ea/move.py#l613
msg14719 (view) Author: [hidden] (ced) (Tryton committer) (Tryton translator) Date: 2013-11-21.23:52:36
There are in some place this anti-pattern:

    for record in records:
        # do some stuffs
        record.save()
        # or
        cls.create()
        # or
        cls.write()

This pattern is not very good for the cache management of Tryton as for each
create/write the cache of all the record in the records list will be reset.
So it doesn't take advantage at all of the smart cache.

Indeed this pattern should be used:

    for sub_records in grouped_slice(records):
        for record in sub_records:
            # do some stuffs
            # store change
        cls.create(to_create)
        cls.write(to_write)

grouped_slice will be a helper to make the "in_max" loop.
History
Date User Action Args
2018-03-09 11:49:49pokolisetstatus: resolved -> in-progress
2018-03-09 11:49:11roundup-botsetstatus: in-progress -> resolved
messages: + msg38890
2018-01-31 17:50:12reviewbotsetmessages: + msg38135
2018-01-31 17:50:11reviewbotsetreviews: 10861002, 13951002, 12931002, 10151002, 13941002, 11861002, 6011002, 13931002, 8921002, 14941002, 14931003, 12941002, 7911003, 6901002, 13961002, 6891002, 14951002, 5991003, 11861004, 11871002, 6011003, 11861003, 10151003, 27501002 -> 10861002, 13951002, 12931002, 10151002, 13941002, 11861002, 6011002, 13931002, 8921002, 14941002, 14931003, 12941002, 7911003, 6901002, 13961002, 6891002, 14951002, 5991003, 11861004, 11871002, 6011003, 11861003, 10151003, 27501002, 43951002
2018-01-31 17:22:59pokolisetcomponent: + account_dunning
2016-12-19 09:57:56pokolisetstatus: resolved -> in-progress
2016-12-19 09:56:30roundup-botsetstatus: in-progress -> resolved
messages: + msg30809
2016-09-21 16:52:26reviewbotsetmessages: + msg28900
2016-09-21 16:28:44reviewbotsetmessages: + msg28896
2016-09-21 16:28:40reviewbotsetmessages: + msg28895

Showing 10 items. Show all history (warning: this could be VERY long)